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