[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241208165800.1f4504a9@jic23-huawei>
Date: Sun, 8 Dec 2024 16:58:00 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Christian Eggers <ceggers@...i.de>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Antoni Pokusinski
<apokusinski01@...il.com>, Francesco Dolcini <francesco@...cini.it>,
João Paulo Gonçalves
<jpaulo.silvagoncalves@...il.com>, Javier Carrasco
<javier.carrasco.cruz@...il.com>, Jonathan Cameron
<Jonathan.Cameron@...wei.com>, <linux-iio@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, João Paulo Gonçalves <joao.goncalves@...adex.com>, Francesco Dolcini
<francesco.dolcini@...adex.com>, <stable@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: light: as73211: fix channel handling in
only-color triggered buffer
On Wed, 4 Dec 2024 17:20:47 +0100
Christian Eggers <ceggers@...i.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@...r.kernel.org
> > Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.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]));
> > 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@...i.de>
>
>
>
>
Powered by blists - more mailing lists