On Wed, 4 Dec 2024 17:20:47 +0100 Christian Eggers ceggers@arri.de 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@vger.kernel.org Fixes: 403e5586b52e ("iio: light: as73211: New driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
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]));/* 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@arri.de