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? > +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. > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + return devm_iio_device_register(dev, indio_dev); > +}