[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75VeZK+wyWBfwr9K7sQY=mET7DxcpE7OYxdAN_hJQodBtdg@mail.gmail.com>
Date: Sat, 24 Apr 2021 14:35:37 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
On Sat, Apr 24, 2021 at 1:47 PM Thomas Bogendoerfer
<tsbogend@...ha.franken.de> 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@...ha.franken.de> 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
Powered by blists - more mailing lists