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