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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 27 Nov 2023 22:26:53 +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,

Discussion around available on v5 made me look closer at that aspect.
You are providing all the available entries in the callback but they
shouldn't be exposed to actually read unless the *_available bitmap
bits corresponding to them are set.

If you like I can just rip the unused code out whilst applying?
Or if you'd prefer to send a v7 that's great too.

Otherwise everything looks good to me.

Jonathan

> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };
> +
> +static int isl76682_read_avail(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       const int **vals, int *type,
> +			       int *length, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			*vals = illuminance_scale_available;
> +			*length = ARRAY_SIZE(illuminance_scale_available);
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		case IIO_INTENSITY:
> +			*vals = intensity_scale_available;
> +			*length = ARRAY_SIZE(intensity_scale_available);
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_INT_TIME:

Never used.  So can just drop this case which tidies up the question
I h ad earlier on what the single entry array was conveying.

> +		*vals = integration_time_available;
> +		*length = ARRAY_SIZE(integration_time_available);
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_chan_spec isl76682_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),

Without setting	.info_mask_shared_by_all_available (unless we have a bug)
you won't see the available attributes for INT_TIME.

> +	}, {
> +		.type = IIO_INTENSITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +	}
> +};
> +
> +static const struct iio_info isl76682_info = {
> +	.read_avail	= isl76682_read_avail,
> +	.read_raw	= isl76682_read_raw,
> +	.write_raw	= isl76682_write_raw,
> +};


> +static const struct i2c_device_id isl76682_id[] = {
> +	{ "isl76682" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, isl76682_id);
> +
> +static const struct of_device_id isl76682_of_match[] = {
> +	{ .compatible = "isil,isl76682" },
> +	{ }

Completely trivial but why { } here and {} in the similar
case above?  Pick one!

> +};
> +MODULE_DEVICE_TABLE(of, isl76682_of_match);