Messages in this thread | | | Date | Tue, 23 Aug 2022 13:08:16 +0530 | Subject | Re: [PATCH v2] iio: light: ltrf216a: Add raw attribute | From | Shreeya Patel <> |
| |
On 14/08/22 21:52, Jonathan Cameron wrote: > On Fri, 12 Aug 2022 15:34:24 +0530 > Shreeya Patel <shreeya.patel@collabora.com> wrote: > >> Add IIO_CHAN_INFO_RAW to the mask to be able to read raw values >> from the light sensor. >> >> The userspace code for brightness control in steam deck uses the >> in_illuminance_input value through sysfs and multiplies it >> with a constant stored in BIOS at factory calibration time. >> >> The downstream driver for LTRF216A that we have been using >> has incorrect formula for LUX calculation which we corrected >> in the upstreamed driver. >> >> Now to be able to use the upstreamed driver, we need to add some >> magic in userspace so that the brightness control works like before >> even with the updated LUX formula. >> >> Hence, we need the raw data to calculate a constant that can be >> added in userspace code. >> >> Downstream driver LUX formula :- >> (greendata*8*LTRF216A_WIN_FAC) / (data->als_gain_fac*data->int_time_fac*10) >> >> Upstreamed driver LUX formula :- >> (greendata*45*LTRF216A_WIN_FAC) / (data->als_gain_fac*data->int_time_fac) >> >> greendata is the ALS_DATA which we would like to get through sysfs using >> the raw attribute. >> >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > Hi Shreeya. > > Your description above makes me wonder though if we should support > this as an intensity channel as we did for many of the early Ambient light > sensors. Not sure why it's called 'greendata' btw! > For those early tsl2583 IIRC and similar, we had two sensors with infrared vs > visible+infrared (which is basically what clear is here). > The readings given were of those two sensors then we did a bunch of maths > to convert those to LUX (in simplest drivers we simply subtracted > the infrared part from visible and applied a scale factor) > > That lead to IIO_TYPE_BOTH though we later added IIO_TYPE_CLEAR which is > subtly different as that was for color sensors with RGB and clearish > filters. The value you want here doesn't really correspond to any of > those modifiers > > I guess that brings us back around to LIGHT(illuminance) + raw as you have it. > or adding a 'visible' modifier which is also rather ugly and hard > to define. > > Let's leave this on list a while longer to see if others comment. > For now I'm inclined to just accept this as a dirty hack needed for this > corner case. Hi Jonathan,
I was wondering if it's fine to merge this now since we haven't got any other comments on it.
Thanks Shreeya Patel > Jonathan > >> --- >> >> Changes in v2 >> - Add a better commit message explaining why we want this change. >> - Call ltrf216a_set_power_state(data, false) before return. >> >> >> drivers/iio/light/ltrf216a.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c >> index e6e24e70d2b9..4b8ef36b6912 100644 >> --- a/drivers/iio/light/ltrf216a.c >> +++ b/drivers/iio/light/ltrf216a.c >> @@ -93,6 +93,7 @@ static const struct iio_chan_spec ltrf216a_channels[] = { >> { >> .type = IIO_LIGHT, >> .info_mask_separate = >> + BIT(IIO_CHAN_INFO_RAW) | >> BIT(IIO_CHAN_INFO_PROCESSED) | >> BIT(IIO_CHAN_INFO_INT_TIME), >> .info_mask_separate_available = >> @@ -259,6 +260,18 @@ static int ltrf216a_read_raw(struct iio_dev *indio_dev, >> int ret; >> >> switch (mask) { >> + case IIO_CHAN_INFO_RAW: >> + ret = ltrf216a_set_power_state(data, true); >> + if (ret) >> + return ret; >> + mutex_lock(&data->lock); >> + ret = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0); >> + mutex_unlock(&data->lock); >> + ltrf216a_set_power_state(data, false); >> + if (ret < 0) >> + return ret; >> + *val = ret; >> + return IIO_VAL_INT; >> case IIO_CHAN_INFO_PROCESSED: >> mutex_lock(&data->lock); >> ret = ltrf216a_get_lux(data); >
| |