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

[Javier Carrasco]

Hi Christophe, thank you for your prompt feedback.

On Thu Dec 12, 2024 at 7:15 PM CET, Christophe JAILLET wrote:
> 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@xxxxxxxxxxxxxxx
> > Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> > Tested-by: Christian Eggers <ceggers@xxxxxxx>
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
>
> ...
>
> > 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?
>

The comment is still true, as address = 1 stores the temperature, and
the first color value can be found at address = 2. I think it used to
be more relevant (even if it was not the correct approach) when the
first received element was stored in chan[1]. Nevertheless, as it might
not be obvious without knowing the address map, it could stay where it
is.

> >   		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
>

in the first case (all channels are read), the temperature (scan[0])
only has 12 bits, and there are no overflows. In the second case,
scan.chan[3] is set to zero as it is not used, and there is no need to
force the U16_MAX value.

Best regards,
Javier Carrasco