[v2,1/2] iio/adc: ingenic: Fix channel offsets in buffer

Commit Message

Artur Rojek May 21, 2023, 10:59 p.m. UTC

Consumers expect the buffer to only contain enabled channels. While
preparing the buffer, the driver makes two mistakes:
1) It inserts empty data for disabled channels.
2) Each ADC readout contains samples for two 16-bit channels. If either
   of them is active, the whole 32-bit sample is pushed into the buffer
   as-is.

Both of those issues cause the active channels to appear at the wrong
offsets in the buffer. Fix the above by demuxing samples for active
channels only.

This has remained unnoticed, as all the consumers so far were only using
channels 0 and 1, leaving them unaffected by changes introduced in this
commit.

Signed-off-by: Artur Rojek <[e-mail]>
Tested-by: Paul Cercueil <[e-mail]>
---

v2: - demux active channels from ADC readouts 
    - clarify in the commit description that this patch doesn't impact
      existing consumers of this driver

 drivers/iio/adc/ingenic-adc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Andy Shevchenko May 22, 2023, 10:15 a.m. UTC | #1

On Mon, May 22, 2023 at 1:59 AM Artur Rojek <[e-mail]> wrote:
>
> Consumers expect the buffer to only contain enabled channels. While
> preparing the buffer, the driver makes two mistakes:
> 1) It inserts empty data for disabled channels.
> 2) Each ADC readout contains samples for two 16-bit channels. If either
>    of them is active, the whole 32-bit sample is pushed into the buffer
>    as-is.
>
> Both of those issues cause the active channels to appear at the wrong
> offsets in the buffer. Fix the above by demuxing samples for active
> channels only.
>
> This has remained unnoticed, as all the consumers so far were only using
> channels 0 and 1, leaving them unaffected by changes introduced in this
> commit.

...

> +       u16 tdat[6];
> +       u32 val;
> +
> +       memset(tdat, 0, ARRAY_SIZE(tdat));

Yeah, as LKP tells us this should be sizeof() instead of ARRAY_SIZE().

> +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> +               if (mask & 0x3) {

(for the consistency it has to be GENMASK(), but see below)

First of all, strictly speaking we should use the full mask without
limiting it to the 0 element in the array (I'm talking about
active_scan_mask).

That said, we may actually use bit operations here in a better way, i.e.

  unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - 1);

  j = 0;
  for_each_set_bit(i, active_scan_mask, ...) {
    val = readl(...);
    /* Two channels per sample. Demux active. */
    tdat[j++] = val >> (16 * (i % 2));
  }

> +                       val = readl(adc->base + JZ_ADC_REG_ADTCH);
> +                       /* Two channels per sample. Demux active. */
> +                       if (mask & BIT(0))
> +                               tdat[i++] = val & 0xffff;
> +                       if (mask & BIT(1))
> +                               tdat[i++] = val >> 16;
> +               }
>         }

Andy Shevchenko May 22, 2023, 10:18 a.m. UTC | #2

On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
<[e-mail]> wrote:
> On Mon, May 22, 2023 at 1:59 AM Artur Rojek <[e-mail]> wrote:

...

> > +       u16 tdat[6];
> > +       u32 val;
> > +
> > +       memset(tdat, 0, ARRAY_SIZE(tdat));
>
> Yeah, as LKP tells us this should be sizeof() instead of ARRAY_SIZE().
>
> > +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > +               if (mask & 0x3) {
>
> (for the consistency it has to be GENMASK(), but see below)
>
> First of all, strictly speaking we should use the full mask without
> limiting it to the 0 element in the array (I'm talking about
> active_scan_mask).
>
> That said, we may actually use bit operations here in a better way, i.e.
>
>   unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - 1);
>
>   j = 0;
>   for_each_set_bit(i, active_scan_mask, ...) {
>     val = readl(...);
>     /* Two channels per sample. Demux active. */
>     tdat[j++] = val >> (16 * (i % 2));

Alternatively

     /* Two channels per sample. Demux active. */
     if (i % 2)
       tdat[j++] = upper_16_bits(val);
     else
       tdat[j++] = lower_16_bits(val);

which may be better to read.

>   }
>
> > +                       val = readl(adc->base + JZ_ADC_REG_ADTCH);
> > +                       /* Two channels per sample. Demux active. */
> > +                       if (mask & BIT(0))
> > +                               tdat[i++] = val & 0xffff;
> > +                       if (mask & BIT(1))
> > +                               tdat[i++] = val >> 16;
> > +               }
> >         }

Paul Cercueil May 22, 2023, 10:20 a.m. UTC | #3

Hi,

Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit :
> On Mon, May 22, 2023 at 1:59 AM Artur Rojek <[e-mail]>
> wrote:
> > 
> > Consumers expect the buffer to only contain enabled channels. While
> > preparing the buffer, the driver makes two mistakes:
> > 1) It inserts empty data for disabled channels.
> > 2) Each ADC readout contains samples for two 16-bit channels. If
> > either
> >    of them is active, the whole 32-bit sample is pushed into the
> > buffer
> >    as-is.
> > 
> > Both of those issues cause the active channels to appear at the
> > wrong
> > offsets in the buffer. Fix the above by demuxing samples for active
> > channels only.
> > 
> > This has remained unnoticed, as all the consumers so far were only
> > using
> > channels 0 and 1, leaving them unaffected by changes introduced in
> > this
> > commit.
> 
> ...
> 
> > +       u16 tdat[6];
> > +       u32 val;
> > +
> > +       memset(tdat, 0, ARRAY_SIZE(tdat));
> 
> Yeah, as LKP tells us this should be sizeof() instead of
> ARRAY_SIZE().

Probably "u16 tdat[6] = { 0 };" would work too?

Cheers,
-Paul

> 
> > +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > +               if (mask & 0x3) {
> 
> (for the consistency it has to be GENMASK(), but see below)
> 
> First of all, strictly speaking we should use the full mask without
> limiting it to the 0 element in the array (I'm talking about
> active_scan_mask).
> 
> That said, we may actually use bit operations here in a better way,
> i.e.
> 
>   unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] -
> 1);
> 
>   j = 0;
>   for_each_set_bit(i, active_scan_mask, ...) {
>     val = readl(...);
>     /* Two channels per sample. Demux active. */
>     tdat[j++] = val >> (16 * (i % 2));
>   }
> 
> > +                       val = readl(adc->base + JZ_ADC_REG_ADTCH);
> > +                       /* Two channels per sample. Demux active.
> > */
> > +                       if (mask & BIT(0))
> > +                               tdat[i++] = val & 0xffff;
> > +                       if (mask & BIT(1))
> > +                               tdat[i++] = val >> 16;
> > +               }
> >         }
>

Paul Cercueil May 22, 2023, 10:23 a.m. UTC | #4

Hi Andy,

Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> <[e-mail]> wrote:
> > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > <[e-mail]> wrote:
> 
> ...
> 
> > > +       u16 tdat[6];
> > > +       u32 val;
> > > +
> > > +       memset(tdat, 0, ARRAY_SIZE(tdat));
> > 
> > Yeah, as LKP tells us this should be sizeof() instead of
> > ARRAY_SIZE().
> > 
> > > +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > > +               if (mask & 0x3) {
> > 
> > (for the consistency it has to be GENMASK(), but see below)
> > 
> > First of all, strictly speaking we should use the full mask without
> > limiting it to the 0 element in the array (I'm talking about
> > active_scan_mask).
> > 
> > That said, we may actually use bit operations here in a better way,
> > i.e.
> > 
> >   unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] -
> > 1);
> > 
> >   j = 0;
> >   for_each_set_bit(i, active_scan_mask, ...) {
> >     val = readl(...);
> >     /* Two channels per sample. Demux active. */
> >     tdat[j++] = val >> (16 * (i % 2));
> 
> Alternatively
> 
>      /* Two channels per sample. Demux active. */
>      if (i % 2)
>        tdat[j++] = upper_16_bits(val);
>      else
>        tdat[j++] = lower_16_bits(val);
> 
> which may be better to read.

It's not if/else though. You would check (i % 2) for the upper 16 bits,
and (i % 1) for the lower 16 bits. Both can be valid at the same time.

Cheers,
-Paul

> 
> >   }
> > 
> > > +                       val = readl(adc->base +
> > > JZ_ADC_REG_ADTCH);
> > > +                       /* Two channels per sample. Demux active.
> > > */
> > > +                       if (mask & BIT(0))
> > > +                               tdat[i++] = val & 0xffff;
> > > +                       if (mask & BIT(1))
> > > +                               tdat[i++] = val >> 16;
> > > +               }
> > >         }
> 
>

Andy Shevchenko May 22, 2023, 11:05 a.m. UTC | #5

On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <[e-mail]> wrote:
> Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > <[e-mail]> wrote:
> > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > <[e-mail]> wrote:

...

> > > > +       u16 tdat[6];
> > > > +       u32 val;
> > > > +
> > > > +       memset(tdat, 0, ARRAY_SIZE(tdat));
> > >
> > > Yeah, as LKP tells us this should be sizeof() instead of
> > > ARRAY_SIZE().
> > >
> > > > +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > > > +               if (mask & 0x3) {
> > >
> > > (for the consistency it has to be GENMASK(), but see below)
> > >
> > > First of all, strictly speaking we should use the full mask without
> > > limiting it to the 0 element in the array (I'm talking about
> > > active_scan_mask).
> > >
> > > That said, we may actually use bit operations here in a better way,
> > > i.e.
> > >
> > >   unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] -
> > > 1);
> > >
> > >   j = 0;
> > >   for_each_set_bit(i, active_scan_mask, ...) {
> > >     val = readl(...);
> > >     /* Two channels per sample. Demux active. */
> > >     tdat[j++] = val >> (16 * (i % 2));
> >
> > Alternatively
> >
> >      /* Two channels per sample. Demux active. */
> >      if (i % 2)
> >        tdat[j++] = upper_16_bits(val);
> >      else
> >        tdat[j++] = lower_16_bits(val);
> >
> > which may be better to read.
>
> It's not if/else though. You would check (i % 2) for the upper 16 bits,
> and (i % 1) for the lower 16 bits. Both can be valid at the same time.

Are you sure? Have you looked into the proposed code carefully?

What probably can be done differently is the read part, that can be
called once. But looking at it I'm not sure how it's supposed to work
at all, since the address is always the same. How does the code and
hardware are in sync with the channels?

> > >   }
> > >
> > > > +                       val = readl(adc->base +
> > > > JZ_ADC_REG_ADTCH);
> > > > +                       /* Two channels per sample. Demux active.
> > > > */
> > > > +                       if (mask & BIT(0))
> > > > +                               tdat[i++] = val & 0xffff;
> > > > +                       if (mask & BIT(1))
> > > > +                               tdat[i++] = val >> 16;
> > > > +               }
> > > >         }

Andy Shevchenko May 22, 2023, 11:05 a.m. UTC | #6

On Mon, May 22, 2023 at 1:20 PM Paul Cercueil <[e-mail]> wrote:
> Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:59 AM Artur Rojek <[e-mail]>
> > wrote:

...

> > > +       memset(tdat, 0, ARRAY_SIZE(tdat));
> >
> > Yeah, as LKP tells us this should be sizeof() instead of
> > ARRAY_SIZE().
>
> Probably "u16 tdat[6] = { 0 };" would work too?

Without 0 also would work :-)

Paul Cercueil May 22, 2023, 11:35 a.m. UTC | #7

Hi Andy,

Le lundi 22 mai 2023 à 14:05 +0300, Andy Shevchenko a écrit :
> On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <[e-mail]>
> wrote:
> > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > > <[e-mail]> wrote:
> > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > > <[e-mail]> wrote:
> 
> ...
> 
> > > > > +       u16 tdat[6];
> > > > > +       u32 val;
> > > > > +
> > > > > +       memset(tdat, 0, ARRAY_SIZE(tdat));
> > > > 
> > > > Yeah, as LKP tells us this should be sizeof() instead of
> > > > ARRAY_SIZE().
> > > > 
> > > > > +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2)
> > > > > {
> > > > > +               if (mask & 0x3) {
> > > > 
> > > > (for the consistency it has to be GENMASK(), but see below)
> > > > 
> > > > First of all, strictly speaking we should use the full mask
> > > > without
> > > > limiting it to the 0 element in the array (I'm talking about
> > > > active_scan_mask).
> > > > 
> > > > That said, we may actually use bit operations here in a better
> > > > way,
> > > > i.e.
> > > > 
> > > >   unsigned long mask = active_scan_mask[0] &
> > > > (active_scan_mask[0] -
> > > > 1);
> > > > 
> > > >   j = 0;
> > > >   for_each_set_bit(i, active_scan_mask, ...) {
> > > >     val = readl(...);
> > > >     /* Two channels per sample. Demux active. */
> > > >     tdat[j++] = val >> (16 * (i % 2));
> > > 
> > > Alternatively
> > > 
> > >      /* Two channels per sample. Demux active. */
> > >      if (i % 2)
> > >        tdat[j++] = upper_16_bits(val);
> > >      else
> > >        tdat[j++] = lower_16_bits(val);
> > > 
> > > which may be better to read.
> > 
> > It's not if/else though. You would check (i % 2) for the upper 16
> > bits,
> > and (i % 1) for the lower 16 bits. Both can be valid at the same
> > time.
> 
> Are you sure? Have you looked into the proposed code carefully?

Yes. I co-wrote the original code, I know what it's supposed to do.

> 
> What probably can be done differently is the read part, that can be
> called once. But looking at it I'm not sure how it's supposed to work
> at all, since the address is always the same. How does the code and
> hardware are in sync with the channels?

It's a FIFO.

Cheers,
-Paul

> 
> > > >   }
> > > > 
> > > > > +                       val = readl(adc->base +
> > > > > JZ_ADC_REG_ADTCH);
> > > > > +                       /* Two channels per sample. Demux
> > > > > active.
> > > > > */
> > > > > +                       if (mask & BIT(0))
> > > > > +                               tdat[i++] = val & 0xffff;
> > > > > +                       if (mask & BIT(1))
> > > > > +                               tdat[i++] = val >> 16;
> > > > > +               }
> > > > >         }
>

Andy Shevchenko May 22, 2023, 7:05 p.m. UTC | #8

On Mon, May 22, 2023 at 2:35 PM Paul Cercueil <[e-mail]> wrote:
> Le lundi 22 mai 2023 à 14:05 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <[e-mail]>
> > wrote:
> > > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > > > <[e-mail]> wrote:
> > > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > > > <[e-mail]> wrote:

...

> > > > > > +       u16 tdat[6];
> > > > > > +       u32 val;
> > > > > > +
> > > > > > +       memset(tdat, 0, ARRAY_SIZE(tdat));
> > > > >
> > > > > Yeah, as LKP tells us this should be sizeof() instead of
> > > > > ARRAY_SIZE().
> > > > >
> > > > > > +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2)
> > > > > > {
> > > > > > +               if (mask & 0x3) {
> > > > >
> > > > > (for the consistency it has to be GENMASK(), but see below)
> > > > >
> > > > > First of all, strictly speaking we should use the full mask
> > > > > without
> > > > > limiting it to the 0 element in the array (I'm talking about
> > > > > active_scan_mask).
> > > > >
> > > > > That said, we may actually use bit operations here in a better
> > > > > way,
> > > > > i.e.
> > > > >
> > > > >   unsigned long mask = active_scan_mask[0] &
> > > > > (active_scan_mask[0] -
> > > > > 1);
> > > > >
> > > > >   j = 0;
> > > > >   for_each_set_bit(i, active_scan_mask, ...) {
> > > > >     val = readl(...);
> > > > >     /* Two channels per sample. Demux active. */
> > > > >     tdat[j++] = val >> (16 * (i % 2));
> > > >
> > > > Alternatively
> > > >
> > > >      /* Two channels per sample. Demux active. */
> > > >      if (i % 2)
> > > >        tdat[j++] = upper_16_bits(val);
> > > >      else
> > > >        tdat[j++] = lower_16_bits(val);
> > > >
> > > > which may be better to read.
> > >
> > > It's not if/else though. You would check (i % 2) for the upper 16
> > > bits,
> > > and (i % 1) for the lower 16 bits. Both can be valid at the same
> > > time.

(i can't be two bits at the same time in my proposal)

> > Are you sure? Have you looked into the proposed code carefully?
>
> Yes. I co-wrote the original code, I know what it's supposed to do.

Yes, but I'm talking about my version to which you commented and I
think it is the correct approach with 'else'. The problematic part in
my proposal is FIFO reading.
So, I have tried to come up with the working solution, but it seems
it's too premature optimization here that's not needed and code,
taking into account readability, will become a bit longer.

That said, let's go with your version for now (implying the GENMASK()
and upper/lower_16_bits() macros in use).

> > What probably can be done differently is the read part, that can be
> > called once. But looking at it I'm not sure how it's supposed to work
> > at all, since the address is always the same. How does the code and
> > hardware are in sync with the channels?
>
> It's a FIFO.

A-ha.

> > > > >   }
> > > > >
> > > > > > +                       val = readl(adc->base +
> > > > > > JZ_ADC_REG_ADTCH);
> > > > > > +                       /* Two channels per sample. Demux
> > > > > > active.
> > > > > > */
> > > > > > +                       if (mask & BIT(0))
> > > > > > +                               tdat[i++] = val & 0xffff;
> > > > > > +                       if (mask & BIT(1))
> > > > > > +                               tdat[i++] = val >> 16;
> > > > > > +               }
> > > > > >         }

--
With Best Regards,
Andy Shevchenko

Jonathan Cameron May 28, 2023, 5:49 p.m. UTC | #9

On Mon, 22 May 2023 00:59:00 +0200
Artur Rojek <[e-mail]> wrote:

> Consumers expect the buffer to only contain enabled channels. While
> preparing the buffer, the driver makes two mistakes:
> 1) It inserts empty data for disabled channels.
> 2) Each ADC readout contains samples for two 16-bit channels. If either
>    of them is active, the whole 32-bit sample is pushed into the buffer
>    as-is.
> 
> Both of those issues cause the active channels to appear at the wrong
> offsets in the buffer. Fix the above by demuxing samples for active
> channels only.
> 
> This has remained unnoticed, as all the consumers so far were only using
> channels 0 and 1, leaving them unaffected by changes introduced in this
> commit.
> 
> Signed-off-by: Artur Rojek <[e-mail]>
> Tested-by: Paul Cercueil <[e-mail]>

Lazy me suggestions that, as we didn't notice this before, clearly the
vast majority of times the channels are both enabled.
As such you 'could' just set available_scan_masks and burn the overhead
of reading channels you don't want, instead letting the IIO core demux
deal with the data movement if needed.

> ---
> 
> v2: - demux active channels from ADC readouts 
>     - clarify in the commit description that this patch doesn't impact
>       existing consumers of this driver
> 
>  drivers/iio/adc/ingenic-adc.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
> index a7325dbbb99a..093710a7ad4c 100644
> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -802,13 +802,19 @@ static irqreturn_t ingenic_adc_irq(int irq, void *data)
>  	struct ingenic_adc *adc = iio_priv(iio_dev);
>  	unsigned long mask = iio_dev->active_scan_mask[0];
>  	unsigned int i;
> -	u32 tdat[3];
> -
> -	for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) {
> -		if (mask & 0x3)
> -			tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH);
> -		else
> -			tdat[i] = 0;
> +	u16 tdat[6];
> +	u32 val;
> +
> +	memset(tdat, 0, ARRAY_SIZE(tdat));
> +	for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> +		if (mask & 0x3) {
> +			val = readl(adc->base + JZ_ADC_REG_ADTCH);
> +			/* Two channels per sample. Demux active. */
> +			if (mask & BIT(0))
> +				tdat[i++] = val & 0xffff;
> +			if (mask & BIT(1))
> +				tdat[i++] = val >> 16;
> +		}
>  	}
>  
>  	iio_push_to_buffers(iio_dev, tdat);

Patch

diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index a7325dbbb99a..093710a7ad4c 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -802,13 +802,19 @@  static irqreturn_t ingenic_adc_irq(int irq, void *data)
 	struct ingenic_adc *adc = iio_priv(iio_dev);
 	unsigned long mask = iio_dev->active_scan_mask[0];
 	unsigned int i;
-	u32 tdat[3];
-
-	for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) {
-		if (mask & 0x3)
-			tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH);
-		else
-			tdat[i] = 0;
+	u16 tdat[6];
+	u32 val;
+
+	memset(tdat, 0, ARRAY_SIZE(tdat));
+	for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
+		if (mask & 0x3) {
+			val = readl(adc->base + JZ_ADC_REG_ADTCH);
+			/* Two channels per sample. Demux active. */
+			if (mask & BIT(0))
+				tdat[i++] = val & 0xffff;
+			if (mask & BIT(1))
+				tdat[i++] = val >> 16;
+		}
 	}
 
 	iio_push_to_buffers(iio_dev, tdat);