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

[Marek Vasut <marex@denx.de>]

On 12/4/23 12:20, Jonathan Cameron wrote:
> 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.

Seeing the diff, that now totally makes sense, thanks !

>> +		*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!

I sent a one-liner for this , it might make sense to squash it if possible .