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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 23 Aug 2022 13:08:16 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> 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.

Dirty hack accepted :)

Applied to the togreg branch of iio.git, initially pushed out as testing
to let the autobuilders see what they can break.

Thanks,

Jonathan

> 
> 
> 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);  
> >