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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: Artur Rojek <contact@artur-rojek.eu>,
	Jonathan Cameron <jic23@kernel.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Chris Morgan <macromorgan@hotmail.com>,
	linux-mips@vger.kernel.org, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org
Subject: Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
Date: Mon, 22 May 2023 14:05:05 +0300	[thread overview]
Message-ID: <CAHp75VdjAxAvmYVW4qgV2i91L31=Ctx4nx_eAe9+pqPFEArD3w@mail.gmail.com> (raw)
In-Reply-To: <2fc0874ce8a802aeb98e553b15e27fb4d4b75a1c.camel@crapouillou.net>

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;
> > > > +               }
> > > >         }

-- 
With Best Regards,
Andy Shevchenko