Re: [PATCH v2 1/4] iio: light: ltr501: Switch to read_avail() op from manual attribute registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] iio: light: ltr501: wrong available masks break scale_available
  sysfs ABI
- [Medium] iio: light: ltr501: ltr501_read_avail() exposes reserved gain
  entries
--

commit 62f3eb145ee7bea38108b1176e688878b094c714
Author: Svyatoslav Ryhel <clamor95@gmail.com>

iio: light: ltr501: Switch to read_avail() op from manual attribute registration

This commit replaces manual *_available sysfs attribute registration with
the IIO read_avail() callback and consolidates the per-chip iio_info
structures.

> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 302be2fc314c5..cc5d7af44d77c 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c
[ ... ]
> @@ -590,6 +604,7 @@ static const struct iio_event_spec ltr501_pxs_event_spec[] = {
>  	.channel2 = (_mod), \
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>  	.info_mask_shared_by_type = (_shared), \
> +	.info_mask_shared_by_all_available = (_shared), \
>  	.scan_index = (_idx), \
>  	.scan_type = { \
>  		.sign = 'u', \

[Severity: High]
Does setting .info_mask_shared_by_all_available = (_shared) break the
sysfs ABI for scale availability?

_shared includes BIT(IIO_CHAN_INFO_SCALE), so placing it in
info_mask_shared_by_all_available registers scale_available instead of
in_intensity_scale_available (which requires
info_mask_shared_by_type_available).

In addition, the IIO_PROXIMITY channel in ltr501_channels sets
BIT(IIO_CHAN_INFO_SCALE) in .info_mask_separate without a matching
.info_mask_separate_available mask, so in_proximity_scale_available is
no longer registered on ltr501 or ltr559 and the proximity scale branch in
ltr501_read_avail() is unreachable.

> @@ -1111,111 +1126,56 @@ static int ltr501_write_event_config(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -static ssize_t ltr501_show_proximity_scale_avail(struct device *dev,
> -						 struct device_attribute *attr,
> -						 char *buf)
> +static int ltr501_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length, long mask)
>  {
> -	struct ltr501_data *data = iio_priv(dev_to_iio_dev(dev));
> +	struct ltr501_data *data = iio_priv(indio_dev);
>  	const struct ltr501_chip_info *info = data->chip_info;
[ ... ]
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_INTENSITY) {
> +			*length = info->als_gain_tbl_size * 2;
> +			*vals = (const int *)info->als_gain;
> +		} else {
> +			*length = info->ps_gain_tbl_size * 2;
> +			*vals = (const int *)info->ps_gain;
> +		}
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;

[Severity: Medium]
Does returning info->als_gain directly as IIO_AVAIL_LIST expose
LTR501_RESERVED_GAIN sentinel entries to userspace?

ltr559_als_gain_tbl (used by ltr303, ltr329, and ltr559) is indexed by
hardware register value and has two reserved entries:

drivers/iio/light/ltr501.c:
static const struct ltr501_gain ltr559_als_gain_tbl[] = {
    ...
	{LTR501_RESERVED_GAIN, LTR501_RESERVED_GAIN},
	{LTR501_RESERVED_GAIN, LTR501_RESERVED_GAIN},
	{0, 20000},
	{0, 10000},
};

ltr501_show_intensity_scale_avail() previously skipped entries equal to
LTR501_RESERVED_GAIN (-1), whereas iio_format_avail_list() formats each
{-1, -1} pair as "-1.000001" in sysfs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106091943.35783-1-clamor95@gmail.com?part=1