Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support

[Sander Vanheule]

Hi Andy,

Thank you for clarifying your remarks. I'll support for building as a
module, and have implemented the gpio_irq_chip->init_hw() callback.

On Mon, 2021-03-29 at 13:26 +0300, Andy Shevchenko wrote:
> On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule <
> sander@xxxxxxxxxxxxx> wrote:
> > On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <
> > > sander@xxxxxxxxxxxxx>
> > > wrote:
> > > > +static const struct of_device_id realtek_gpio_of_match[] = {
> > > > +       { .compatible = "realtek,otto-gpio" },
> > > > +       {
> > > > +               .compatible = "realtek,rtl8380-gpio",
> > > > +               .data = (void *)GPIO_INTERRUPTS
> > > 
> > > Not sure why this flag is needed right now. Drop it completely for
> > > good.
> > > > +       },
> > > > +       {
> > > > +               .compatible = "realtek,rtl8390-gpio",
> > > > +               .data = (void *)GPIO_INTERRUPTS
> > > 
> > > Ditto
> > 
> > Linus Walleij asked this question too after v1:
> > https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@xxxxxxxxxxxxx/
> > 
> > Note that the fall-back compatible doesn't have this flag set.
> 
> AFAICS all, except one have this flag, I suggest you to do other way
> around, i.e. check compatible string in the code. Or do something more
> clever. What happens if you have this flag enabled for the fallback
> node?
> 
> If two people ask the same, it might be a smoking gun.
> 

Testing for the fallback wouldn't work, since of_device_is_compatible()
would always match. Setting the (inverse) flag only on the fallback
would indeed reduce the clutter.

If the port order is reversed w.r.t. to the current implementation,
enabling a GPIO+IRQ would enable the same pin on a different port. I
don't think the result would be catastrophical, but it would result in
unexpected behaviour. When A0 and C0 are then enabled, A0 interrupts
would actually come from C0, and vice versa.

   Intended port | A | B | C | D
-----------------+---+---+---+---
Actual GPIO port | D | C | B | A
 Actual IRQ port | B | A | D | C

If only the actual GPIO ports change, at least you can still use a
modified GPIO line number and polling. The user could just leave out
the optional irq-controller from the devicetree, but I would rather
have it enforced in some way.


Best,
Sander