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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 26 Nov 2023 23:09:36 +0100
Marek Vasut <marex@denx.de> wrote:

> On 11/26/23 19:16, Jonathan Cameron wrote:
> > On Sat, 25 Nov 2023 23:26:23 +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,
> > 
> > One last question + a comment in general. Act on that if you like.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> >> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };  
> > 
> > Why have an available attribute for a single value. Is it useful for anything?  
> 
> To report it to userspace, iio-sensor-proxy uses that to control the ALS 
> poll interval .

It should use integration_time, not the associated available attribute.

> 
> >> +static int isl76682_probe(struct i2c_client *client)
> >> +{  
> > 
> > ...
> >   
> >> +	indio_dev->info = &isl76682_info;
> >> +	indio_dev->channels = isl76682_channels;
> >> +	indio_dev->num_channels = ARRAY_SIZE(isl76682_channels);
> >> +	indio_dev->name = ISL76682_DRIVER_NAME;  
> > Trivial but I'm not a fan of using defines in cases like this. It just makes
> > me go find the define when I could see the string directly here.
> > 
> > In cases where matching or similar strictly requires the naming to be the same
> > in various places a define is useful. In this case less so.
> > 
> > Anyhow, it's a very minor comment so never mind if you prefer to leave it
> > as it stands.  
> 
> I added it to V6 .
> 
> I'll wait for the integration time reply above and then send V6 .
>