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

  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 <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
---

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(-)
  

Hi Artur,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on dtor-input/next dtor-input/for-linus linus/master v6.4-rc3 next-20230519]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Artur-Rojek/iio-adc-ingenic-Fix-channel-offsets-in-buffer/20230522-070621
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20230521225901.388455-2-contact%40artur-rojek.eu
patch subject: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ffc01980aa58e54084cc327fd18fb6ceb82d93d8
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Artur-Rojek/iio-adc-ingenic-Fix-channel-offsets-in-buffer/20230522-070621
        git checkout ffc01980aa58e54084cc327fd18fb6ceb82d93d8
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/iio/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305221047.ZAQU4oMN-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/string.h:20,
                    from include/linux/bitmap.h:11,
                    from include/linux/cpumask.h:12,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/mutex.h:17,
                    from include/linux/notifier.h:14,
                    from include/linux/clk.h:14,
                    from drivers/iio/adc/ingenic-adc.c:10:
   drivers/iio/adc/ingenic-adc.c: In function 'ingenic_adc_irq':
>> arch/m68k/include/asm/string.h:48:25: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
      48 | #define memset(d, c, n) __builtin_memset(d, c, n)
         |                         ^~~~~~~~~~~~~~~~
   drivers/iio/adc/ingenic-adc.c:808:9: note: in expansion of macro 'memset'
     808 |         memset(tdat, 0, ARRAY_SIZE(tdat));
         |         ^~~~~~


vim +/memset +48 arch/m68k/include/asm/string.h

ea61bc461d09e8 Greg Ungerer 2010-09-07  45  
ea61bc461d09e8 Greg Ungerer 2010-09-07  46  #define __HAVE_ARCH_MEMSET
ea61bc461d09e8 Greg Ungerer 2010-09-07  47  extern void *memset(void *, int, __kernel_size_t);
ea61bc461d09e8 Greg Ungerer 2010-09-07 @48  #define memset(d, c, n) __builtin_memset(d, c, n)
ea61bc461d09e8 Greg Ungerer 2010-09-07  49
  

On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> 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;
> +               }
>         }
  

On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> 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;
> > +               }
> >         }
  

Hi,

Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit :
> On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu>
> 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;
> > +               }
> >         }
>
  

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
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > <contact@artur-rojek.eu> 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;
> > > +               }
> > >         }
> 
>
  

On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > <contact@artur-rojek.eu> 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;
> > > > +               }
> > > >         }
  

On Mon, May 22, 2023 at 1:20 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu>
> > 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 :-)
  

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 <paul@crapouillou.net>
> wrote:
> > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > > <contact@artur-rojek.eu> 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;
> > > > > +               }
> > > > >         }
>
  

On Mon, May 22, 2023 at 2:35 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 22 mai 2023 à 14:05 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> > > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > > > <contact@artur-rojek.eu> 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
  

On Mon, 22 May 2023 00:59:00 +0200
Artur Rojek <contact@artur-rojek.eu> 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 <contact@artur-rojek.eu>
> Tested-by: Paul Cercueil <paul@crapouillou.net>

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);