On Wed, 4 Dec 2024 17:20:47 +0100 Christian Eggers <ceggers@xxxxxxx> wrote: > On Wednesday, 4 December 2024, 00:55:32 CET, Javier Carrasco wrote: > > 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. > > > > Cc: stable@xxxxxxxxxxxxxxx > > Fixes: 403e5586b52e ("iio: light: as73211: New driver") > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> > > --- > > drivers/iio/light/as73211.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c > > index be0068081ebb..2d45dfeda406 100644 > > --- a/drivers/iio/light/as73211.c > > +++ b/drivers/iio/light/as73211.c > > @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) > > > > /* AS73211 starts reading at address 2 */ > > 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 +685,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) { > > + 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); > > + } > > } > > > > iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev)); > > > > > > With this change, having only X, Y and Z in the scan_mask (without the > temperature channel) works fine. > > But it looks that there is still another problem if a single color channel > (e.g. X) is omitted from the scan mask (which probably wouldn't make much > sense in practice). If I am right, the layout of scan.chan[] is also wrong for > that case, so e.g. if omitting X, the application will get the X values where > it expects the temperature value (which isn't read from the hardware at all). > > Does it make sense to write a follow-up patch for this? I fear that taking all > possible combinations into account could make the driver more complicated than > necessary. Or is there a good example how to handle such combinations? > Good spot. I'd fallen for assuming a driver worked the way I thought it would and not checked everything necessary was there. Hmm. This is a bit odd. Driver seems to be written with assumption that the IIO core is doing demux. That doesn't work unless available_scan_masks is set. Make that { AS73211_SCAN_MASK_ALL, AS73211_SCAN_MASK_COLOR, 0, }; And then if you enable fewer channels, the IIO core will still enable one of the sets in available_scan_masks and then do the relevant data manipulation to repack as necessary. I'll not pick this patch up as it makes sense to fix both issues together. Thanks Jonathan > > Tested-by: Christian Eggers <ceggers@xxxxxxx> > > > >