Re: [PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 12/12/2024 à 18:56, Javier Carrasco a écrit :
> The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not
> set (optimized path for color channel readings), and it must be shifted
> instead of leaving an empty channel for the temperature when it is off.
> 
> Once the channel index is fixed, the uninitialized channel must be set
> to zero to avoid pushing uninitialized data.
> 
> Add available_scan_masks for all channels and only-color channels to let
> the IIO core demux and repack the enabled channels.
> 
> Cc: stable@vger.kernel.org
> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> Tested-by: Christian Eggers <ceggers@arri.de>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

...

> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index be0068081ebb..4be2e349a068 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -177,6 +177,12 @@ struct as73211_data {
>   	BIT(AS73211_SCAN_INDEX_TEMP) | \
>   	AS73211_SCAN_MASK_COLOR)
>   
> +static const unsigned long as73211_scan_masks[] = {
> +	AS73211_SCAN_MASK_ALL,
> +	AS73211_SCAN_MASK_COLOR,
> +	0,
> +};
> +
>   static const struct iio_chan_spec as73211_channels[] = {
>   	{
>   		.type = IIO_TEMP,
> @@ -672,9 +678,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>   
>   		/* AS73211 starts reading at address 2 */

Should this comment be updated?

Or maybe moved close to "if (*indio_dev->active_scan_mask == 
AS73211_SCAN_MASK_ALL)" below?

>   		ret = i2c_master_recv(data->client,
> -				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
> +				(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));
>   		if (ret < 0)
>   			goto done;
> +
> +		/* Avoid pushing uninitialized data */
> +		scan.chan[3] = 0;
>   	}
>   
>   	if (data_result) {
> @@ -682,9 +691,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>   		 * Saturate all channels (in case of overflows). Temperature channel
>   		 * is not affected by overflows.
>   		 */
> -		scan.chan[1] = cpu_to_le16(U16_MAX);
> -		scan.chan[2] = cpu_to_le16(U16_MAX);
> -		scan.chan[3] = cpu_to_le16(U16_MAX);
> +		if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {

Should [0]...

> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);
> +			scan.chan[3] = cpu_to_le16(U16_MAX);
> +		} else {
> +			scan.chan[0] = cpu_to_le16(U16_MAX);
> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);

... and [3] be forced as-well?
(just a blind guess)

CJ

> +		}
>   	}
>   
>   	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
> @@ -758,6 +773,7 @@ static int as73211_probe(struct i2c_client *client)
>   	indio_dev->channels = data->spec_dev->channels;
>   	indio_dev->num_channels = data->spec_dev->num_channels;
>   	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->available_scan_masks = as73211_scan_masks;
>   
>   	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR);
>   	if (ret < 0)
> 
> ---
> base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
> change-id: 20241123-iio_memset_scan_holes-a673833ef932
> 
> Best regards,