On Sun, 26 Nov 2023 23:09:36 +0100
Marek Vasut <marex@xxxxxxx> wrote:
> On 11/26/23 19:16, Jonathan Cameron wrote:
> > On Sat, 25 Nov 2023 23:26:23 +0100
> > Marek Vasut <marex@xxxxxxx> 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@xxxxxxxxxxxxxxx>
> >> Reviewed-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> >> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> >
> > 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 .
>
