Re: [PATCH v11 3/8] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Jun 5, 2024 at 9:29 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@xxxxxxxxxx> wrote:

...

> > +/**
> > + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling
> > + *     @dst: the destination u8 array of interleaved bytes
> > + *     @rising: rising mask
> > + *     @falling: falling mask
>
> Why so many spaces before @? One is enough.
>
> > + *
> > + * Interleaves the little-endian bytes from @rising and @falling words.
> > + *
> > + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is
> > + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3).
> > + *
> > + * The MCU receives an interrupt mask and reports a pending interrupt bitmap in
> > + * this interleaved format. The rationale behind this is that the low-indexed
> > + * bits are more important - in many cases, the user will be interested only in
> > + * interrupts with indexes 0 to 7, and so the system can stop reading after
> > + * first 2 bytes (r0, f0), to save time on the slow I2C bus.
> > + *
> > + * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
> > + * and use an appropriate bitmap_*() function once such a function exists.
> > + */
> > +static void
> > +omnia_mask_interleave(u8 *dst, unsigned long rising, unsigned long falling)

> But rising and failing should be either u64 or unsigned long *.

Ah, sorry, I misread the use, discard this single comment.

> > +{
> > +       for (int i = 0; i < sizeof(u32); ++i) {
>
> In other cases you use:
> 1) unsigned
> 2) post-increment
>
> What makes this one special?
>
> > +               dst[2 * i] = rising >> (8 * i);
> > +               dst[2 * i + 1] = falling >> (8 * i);
> > +       }
> > +}
>
> ...
>
> > +/**
> > + * omnia_mask_deinterleave - Deinterleaves the bytes into @rising and @falling
> > + *     @src: the source u8 array containing the interleaved bytes
> > + *     @rising: pointer where to store the rising mask gathered from @src
> > + *     @falling: pointer where to store the falling mask gathered from @src
> > + *
> > + * This is the inverse function to omnia_mask_interleave.
> > + */
> > +static void omnia_mask_deinterleave(const u8 *src, unsigned long *rising,
> > +                                   unsigned long *falling)
> > +{
> > +       *rising = *falling = 0;
> > +
> > +       for (int i = 0; i < sizeof(u32); ++i) {
> > +               *rising |= src[2 * i] << (8 * i);
> > +               *falling |= src[2 * i + 1] << (8 * i);
> > +       }
> > +}
>
> Same comments as per above function.

-- 
With Best Regards,
Andy Shevchenko