Re: [PATCH v6 2/2] iio: light: isl76682: Add ISL76682 driver

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 5 Dec 2023 02:24:51 +0100
Marek Vasut <marex@denx.de> wrote:

> On 12/4/23 15:35, Jonathan Cameron wrote:
> > On Mon, 4 Dec 2023 12:23:06 +0100
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 12/4/23 12:20, Jonathan Cameron wrote:  
> >>> On Mon, 27 Nov 2023 22:26:53 +0100
> >>> Marek Vasut <marex@denx.de> wrote:
> >>>      
> >>>> The ISL76682 is very basic ALS which only supports ALS or IR mode
> >>>> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any
> >>>> other fancy functionality.
> >>>>
> >>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >>>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>  
> >>> Hi Marek,
> >>>
> >>> Discussion around available on v5 made me look closer at that aspect.
> >>> You are providing all the available entries in the callback but they
> >>> shouldn't be exposed to actually read unless the *_available bitmap
> >>> bits corresponding to them are set.
> >>>
> >>> If you like I can just rip the unused code out whilst applying?
> >>> Or if you'd prefer to send a v7 that's great too.
> >>>
> >>> Otherwise everything looks good to me.  
> >>
> >> Maybe just do that while applying and I'll test it right after to see
> >> whether something broke, that's probably fastest. Just let me know where
> >> this got applied. I have the device on my desk .  
> > 
> > Diff is below.  Applied to the togreg  
> 
> I only found the commit in 'testing' branch , so I used that one .
I messed up it seems and didn't actually push the updated version.
Done so now along with squashing in the bracket tidy up.
> 
> > branch of iio.git and initially pushed out
> > as testing for normal reasons + for you to test.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> > 
> > diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c
> > index 15a68609985b..8605187bfb62 100644
> > --- a/drivers/iio/light/isl76682.c
> > +++ b/drivers/iio/light/isl76682.c
> > @@ -184,8 +184,6 @@ static int intensity_scale_available[] = {
> >          0, 673000,
> >   };
> >   
> > -static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };
> > -
> >   static int isl76682_read_avail(struct iio_dev *indio_dev,
> >                                 struct iio_chan_spec const *chan,
> >                                 const int **vals, int *type,
> > @@ -207,11 +205,6 @@ static int isl76682_read_avail(struct iio_dev *indio_dev,
> >                  default:
> >                          return -EINVAL;
> >                  }
> > -       case IIO_CHAN_INFO_INT_TIME:
> > -               *vals = integration_time_available;
> > -               *length = ARRAY_SIZE(integration_time_available);
> > -               *type = IIO_VAL_INT_PLUS_MICRO;
> > -               return IIO_AVAIL_LIST;
> >          default:
> >                  return -EINVAL;
> >          }  
> 
> Ah, looking at the attrs before and after, now I get it:
> 
> $ grep -H . /sys/bus/iio/devices/iio\:device0/*
> /sys/bus/iio/devices/iio:device0/in_illuminance_raw:21
> /sys/bus/iio/devices/iio:device0/in_illuminance_scale:0.015000
> /sys/bus/iio/devices/iio:device0/in_illuminance_scale_available:0.015 
> 0.06 0.24 0.96
> /sys/bus/iio/devices/iio:device0/in_intensity_raw:33
> /sys/bus/iio/devices/iio:device0/in_intensity_scale:0.010500
> /sys/bus/iio/devices/iio:device0/in_intensity_scale_available:0.0105 
> 0.042 0.168 0.673
> /sys/bus/iio/devices/iio:device0/integration_time_available:0.090
> /sys/bus/iio/devices/iio:device0/name:isl76682
> 
> /sys/bus/iio/devices/iio:device0/in_illuminance_raw:22
> /sys/bus/iio/devices/iio:device0/in_illuminance_scale:0.015000
> /sys/bus/iio/devices/iio:device0/in_illuminance_scale_available:0.015000 
> 0.060000 0.240000 0.960000
> /sys/bus/iio/devices/iio:device0/in_intensity_raw:24
> /sys/bus/iio/devices/iio:device0/in_intensity_scale:0.010500
> /sys/bus/iio/devices/iio:device0/in_intensity_scale_available:0.010500 
> 0.042000 0.168000 0.673000
> /sys/bus/iio/devices/iio:device0/integration_time:0.090000
> /sys/bus/iio/devices/iio:device0/name:isl76682
> 
> Thanks !
>