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

[Jonathan Cameron <jic23@kernel.org>]

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?


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