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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, May 10, 2024 at 11:09 AM Marek Behún <kabel@xxxxxxxxxx> wrote:
> On Wed, 8 May 2024 14:16:26 +0300
> Andy Shevchenko <andy@xxxxxxxxxx> wrote:
> > On Wed, May 08, 2024 at 12:31:12PM +0200, Marek Behún wrote:

...

> > > +   if (type & IRQ_TYPE_EDGE_RISING)
> > > +           mcu->rising |= bit;
> > > +   else
> > > +           mcu->rising &= ~bit;
> > > +
> > > +   if (type & IRQ_TYPE_EDGE_FALLING)
> > > +           mcu->falling |= bit;
> > > +   else
> > > +           mcu->falling &= ~bit;
> >
> > If those variables was defined as unsigned long, these can be just
> >
> >       __assign_bit()
> >       __assign_bit()
> >
> > And other non-atomic bitops elsewhere, like __clear_bit().
>
> Changing this propagated to many other variables and even required
> some refactoring of the omnia_gpio structure, since the bit, ctl_bit
> and int_bit members are stored as a masks, but __assign_bit() /
> __set_bit() / __clear_bit() requires bit numbers.
>
> For example
>   if (gpio->int_bit && (mcu->is_cached & gpio->int_bit))
>     return !!(mcu->cached & gpio->int_bit);
> needed to change to
>   if (gpio->has_int && (mcu->is_cached & BIT(gpio->int_bit)))
>     return !!(mcu->cached & BIT(gpio->int_bit));

You rather meant

  return test_bit(...);

> and so on.

...and so on :-)

> Moreover, I agree that the if-else statement which you commented on,
> when changed to __assign_bit(), looks much nicer, but some changes that
> sprouted from this are in my opinion less readable.

Maybe. It can be done later on.

> I have prepared the fixup patch, but I am not confident enough that
> everything is done correctly. I would prefer leaving this one for
> later, if it is okay with you.

--
With Best Regards,
Andy Shevchenko