Re: [PATCH v2 4/4] 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 2 potential
issues to consider:
- [High] iio: light: ltr501: wrong channel lookup in
  ltr501_trigger_handler()
- [Medium] iio: light: ltr501: unguarded ALS register read in
  ltr501_init()
--

commit ee234def6f10d9aedb1149ec4e47b6d46e491495
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 sensor,
gates ALS register accesses behind a no_als_support flag, and updates
ltr501_trigger_handler() to check channel types instead of hardcoded scan
indices.

> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index d5ce6468a229b..e7762115df913 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c

[ ... ]

> @@ -1251,16 +1291,21 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
>  		aligned_s64 ts;
>  	} scan = { };
>  	__le16 als_buf[2];
> +	unsigned int i;
>  	u8 mask = 0;
>  	int j = 0;
>  	int ret, psdata;
>  
>  	/* figure out which data needs to be ready */
> -	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;
> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->num_channels) {
> +		const struct iio_chan_spec *chan = &indio_dev->channels[i];
> +
> +		if (chan->type == IIO_INTENSITY)
> +			mask |= LTR501_STATUS_ALS_RDY;
> +
> +		if (chan->type == IIO_PROXIMITY)
> +			mask |= LTR501_STATUS_PS_RDY;
> +	}

[Severity: High]
Does indexing indio_dev->channels[] with the bit position from
active_scan_mask inspect the wrong channel on existing sensors?

Bit positions in active_scan_mask correspond to chan->scan_index, not the
array index in indio_dev->channels[].  In ltr501_channels and
ltr301_channels, channels[0] is LTR501_LIGHT_CHANNEL() with scan_index -1,
which shifts all buffered channels by one slot: channels[1] and
channels[2] are IIO_INTENSITY (scan_index 0 and 1), and in ltr501_channels
channels[3] is IIO_PROXIMITY (scan_index 2).

As a regression on LTR-501 and LTR-559, enabling buffered proximity sets
bit 2 in active_scan_mask, which inspects channels[2] (IIO_INTENSITY)
instead of channels[3] (IIO_PROXIMITY).  LTR501_STATUS_PS_RDY is never set
in mask, LTR501_PS_DATA is never read, and zeroed proximity values are
pushed to the IIO buffer.  Similarly, enabling only scan_index 0 on any of
the existing ALS sensors inspects channels[0] (IIO_LIGHT), leaving mask at
0 and pushing zeroed ALS data.

> @@ -1328,11 +1373,13 @@ static int ltr501_init(struct ltr501_data *data)
>  {
>  	int ret, status;
>  
> -	ret = regmap_read(data->regmap, LTR501_ALS_CONTR, &status);
> -	if (ret < 0)
> -		return ret;
> +	if (ltr501_has_als_support(data->chip_info)) {
> +		ret = regmap_read(data->regmap, LTR501_ALS_CONTR, &status);
> +		if (ret < 0)
> +			return ret;
>  
> -	data->als_contr = status | data->chip_info->als_mode_active;
> +		data->als_contr = status | data->chip_info->als_mode_active;
> +	}
>  
>  	ret = regmap_read(data->regmap, LTR501_PS_CONTR, &status);
>  	if (ret < 0)

[Severity: Medium]
Should the IIO_INTENSITY persistence read later in ltr501_init() also be
guarded by ltr501_has_als_support()?

drivers/iio/light/ltr501.c:ltr501_init() {
    ...
	if (ltr501_has_irq_support(data->chip_info)) {
		ret = ltr501_read_intr_prst(data, IIO_INTENSITY, &data->als_period);
		if (ret < 0)
			return ret;
    ...
}

Because ltr659_chip_info_tbl has irq support enabled, probing an
LTR-659PS-01 calls ltr501_read_intr_prst() for IIO_INTENSITY, which reads
reg_als_prst and calls ltr501_als_read_samp_period() to read the reserved
LTR501_ALS_MEAS_RATE register.

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