Re: [PATCH v1 2/2] iio: light: ltr501: Add support for the LTR-659PS-01 proximity sensor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iio: light: ltr501: scan_index mismatch breaks ltr501_trigger_handler()
- [Medium] iio: light: ltr501: missing IIO_CHAN_INFO_SAMP_FREQ in ltr659_channels
- [Medium] iio: light: ltr501: unconditional ALS register accesses in ltr501_init()
--

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

iio: light: ltr501: Add support for the LTR-659PS-01 proximity sensor

This commit adds support for the Lite-On LTR-659PS-01 proximity-only sensor
to the ltr501 IIO driver.

> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 302be2fc314c5..5cc1141322ad5 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c

[ ... ]

> @@ -649,6 +650,26 @@ static const struct iio_chan_spec ltr301_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(2),
>  };
>  
> +static const struct iio_chan_spec ltr659_channels[] = {
> +	{
> +		.type = IIO_PROXIMITY,
> +		.address = LTR501_PS_DATA,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,

[Severity: High]
Does assigning scan_index 0 here break triggered buffer capture for ltr659?

In ltr501_trigger_handler(), active_scan_mask bits 0 and 1 are hardcoded for
the ALS channels, while only bit 2 is treated as the proximity channel:

	if (test_bit(0, indio_dev->active_scan_mask) ||
	    test_bit(1, indio_dev->active_scan_mask))
		mask |= LTR501_STATUS_ALS_RDY;
	if (test_bit(2, indio_dev->active_scan_mask))
		mask |= LTR501_STATUS_PS_RDY;

	ret = ltr501_drdy(data, mask);
	if (ret < 0)
		goto done;

	if (mask & LTR501_STATUS_ALS_RDY) {
		ret = regmap_bulk_read(data->regmap, LTR501_ALS_DATA1,
				       als_buf, sizeof(als_buf));
		if (ret < 0)
			goto done;
		if (test_bit(0, indio_dev->active_scan_mask))
			scan.channels[j++] = le16_to_cpu(als_buf[1]);

When buffered capture is enabled on ltr659, bit 0 is set in active_scan_mask
and bit 2 is never set, so ltr501_trigger_handler() waits on
LTR501_STATUS_ALS_RDY and reads LTR501_ALS_DATA1 instead of reading
LTR501_PS_DATA.

> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 11,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +		.event_spec = ltr501_pxs_event_spec,
> +		.num_event_specs = ARRAY_SIZE(ltr501_pxs_event_spec),
> +		.ext_info = ltr501_ext_info,
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
>  static int ltr501_read_info_raw(struct ltr501_data *data,
>  				struct iio_chan_spec const *chan,
>  				int *val)
> @@ -1178,6 +1199,12 @@ static struct attribute *ltr301_attributes[] = {
>  	NULL
>  };
>  
> +static struct attribute *ltr659_attributes[] = {
> +	&iio_dev_attr_in_proximity_scale_available.dev_attr.attr,
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL
> +};

[Severity: Medium]
Should ltr659_channels also set BIT(IIO_CHAN_INFO_SAMP_FREQ) on the
IIO_PROXIMITY channel?

ltr659_attributes exposes sampling_frequency_available in sysfs, and
ltr501_read_raw() and __ltr501_write_raw() support IIO_CHAN_INFO_SAMP_FREQ for
IIO_PROXIMITY, but ltr659_channels only sets BIT(IIO_CHAN_INFO_RAW) and
BIT(IIO_CHAN_INFO_SCALE). Without BIT(IIO_CHAN_INFO_SAMP_FREQ) in the channel
spec, no sampling_frequency sysfs attribute is created for ltr659.

[ ... ]

> @@ -1283,6 +1330,15 @@ static const struct ltr501_chip_info ltr501_chip_info_tbl[] = {
>  		.channels = ltr501_channels,
>  		.no_channels = ARRAY_SIZE(ltr501_channels),
>  	},
> +	[ltr659] = {
> +		.partid = 0x09,
> +		.ps_gain = ltr559_ps_gain_tbl,
> +		.ps_gain_tbl_size = ARRAY_SIZE(ltr559_ps_gain_tbl),
> +		.info = &ltr659_info,
> +		.info_no_irq = &ltr659_info_no_irq,
> +		.channels = ltr659_channels,
> +		.no_channels = ARRAY_SIZE(ltr659_channels),
> +	},
>  };
>  
>  static int ltr501_write_contr(struct ltr501_data *data, u8 als_val, u8 ps_val)

[Severity: Medium]
The commit message notes that ALS registers are marked as reserved on the
LTR-659PS-01 and interactions with them are discouraged, but won't
ltr501_init() and ltr501_write_contr() still read and write those registers?

During probe, ltr501_init() unconditionally reads LTR501_ALS_CONTR, calls
ltr501_read_intr_prst(data, IIO_INTENSITY, &data->als_period) which reads the
ALS persistence field and LTR501_ALS_MEAS_RATE, and then calls
ltr501_write_contr():

static int ltr501_write_contr(struct ltr501_data *data, u8 als_val, u8 ps_val)
{
	int ret;

	ret = regmap_write(data->regmap, LTR501_ALS_CONTR, als_val);
	if (ret < 0)
		return ret;

	return regmap_write(data->regmap, LTR501_PS_CONTR, ps_val);
}

ltr501_write_contr() also writes LTR501_ALS_CONTR during ltr501_powerdown()
and ltr501_resume(). Should these ALS register accesses be skipped when
chip_info->als_gain is NULL?

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