Re: [PATCH v2] iio: light: ltrf216a: Add raw attribute

[Shreeya Patel]

On 14/08/22 21:52, Jonathan Cameron wrote:
> On Fri, 12 Aug 2022 15:34:24 +0530
> Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> 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);