Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Apr 24, 2021 at 1:47 PM Thomas Bogendoerfer
<tsbogend@xxxxxxxxxxxxxxxx> wrote:
> On Fri, Apr 23, 2021 at 06:37:41PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 22, 2021 at 6:21 PM Thomas Bogendoerfer
> > <tsbogend@xxxxxxxxxxxxxxxx> wrote:

...

> > > +               virq = irq_linear_revmap(gc->irq.domain, bit);
> >
> > Is it guaranteed to be linear always?
>
> yes

OK.

...

> > > +       if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> >
> > There is a _BOTH variant.
>
> that's IRQ_TYPE_EDGE_BOTH. LEVEL_BOTH would be an interesing concept.

Sorry, I meant _MASK in case of level. No need to open code the
existing definition.

> > > +       ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL);
> > > +       if (sense & IRQ_TYPE_LEVEL_HIGH)
> > > +               ilevel |= BIT(d->hwirq);
> > > +       else if (sense & IRQ_TYPE_LEVEL_LOW)
> > > +               ilevel &= ~BIT(d->hwirq);
> >
> > > +       else
> > > +               return -EINVAL;
> >
> > Is it a double check of the above?
>
> no, the above test is for anything not LEVEL and this now takes care
> to be at least LEVEL_LOW or LEVEL_HIGH. This doesn't check for LOW|HIGH,
> which I assumed nobody tries to set...

And? Seems you have it as a dead code.
In your case HIGH is the winner anyway.

...

> > > +       /* Mask interrupts. */
> > > +       ctrl->mask_cache = 0xffffffff;
> > > +       writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> >
> > What about using ->init_hw() call back?
>
> sure, doesn't look like it's worth the effort, but I changed it.

The problem here (which you may not notice from day 1) is the
ordering. We carefully put the ->init_hw() call in the proper place
and time.

...

> > > +       girq->handler = handle_level_irq;
> >
> > handle_bad_irq()
>
> the hardware only supports level interrupts. That's also why there is
> no handler change in idt_gpio_irq_set_type.

After I fixed a nasty issue in the pca953x related to Intel Galileo
platform, I may tell that setting the handler here is equal to putting
a mine for the future blown. When you set it in BAD here it will
reveal issues, if any, sooner than later.

-- 
With Best Regards,
Andy Shevchenko