From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
Date: Sat, 24 Apr 2021 14:35:37 +0300 [thread overview]
Message-ID: <CAHp75VeZK+wyWBfwr9K7sQY=mET7DxcpE7OYxdAN_hJQodBtdg@mail.gmail.com> (raw)
In-Reply-To: <20210424103544.GA4353@alpha.franken.de>
On Sat, Apr 24, 2021 at 1:47 PM Thomas Bogendoerfer
<tsbogend@alpha.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@alpha.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
next prev parent reply other threads:[~2021-04-24 11:35 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-22 15:20 [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
2021-04-22 15:20 ` [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer
2021-04-23 17:53 ` Rob Herring
2021-04-23 15:37 ` [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko
2021-04-24 10:35 ` Thomas Bogendoerfer
2021-04-24 11:35 ` Andy Shevchenko [this message]
2021-04-23 15:56 ` Andy Shevchenko
2021-04-26 9:33 ` Thomas Bogendoerfer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75VeZK+wyWBfwr9K7sQY=mET7DxcpE7OYxdAN_hJQodBtdg@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=bgolaszewski@baylibre.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tsbogend@alpha.franken.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.