[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aef9b748935eb6d30597cab4faf911c72a45c6e6.camel@ew.tq-group.com>
Date: Thu, 30 May 2024 13:36:10 +0200
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
<brgl@...ev.pl>, Andrew Lunn <andrew@...n.ch>, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org, Gregor Herburger
<gregor.herburger@...group.com>, linux@...tq-group.com
Subject: Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH
interrupt type
On Thu, 2024-05-30 at 13:15 +0200, Matthias Schiffer wrote:
> On Thu, 2024-05-30 at 13:22 +0300, Dan Carpenter wrote:
> >
> > On Thu, May 30, 2024 at 10:39:25AM +0200, Matthias Schiffer wrote:
> > > On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote:
> > > >
> > > > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> > > > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > > > > index c957be3341774..400415676ad5d 100644
> > > > > --- a/drivers/gpio/gpio-tqmx86.c
> > > > > +++ b/drivers/gpio/gpio-tqmx86.c
> > > > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
> > > > > unsigned int offset = hwirq - TQMX86_NGPO;
> > > > > u8 type = TQMX86_INT_TRIG_NONE, mask, val;
> > > > >
> > > > > - if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
> > > > > + if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
> > > > > type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
> > > > >
> > > > > + if (type == TQMX86_INT_TRIG_BOTH)
> > > > > + type = tqmx86_gpio_get(&gpio->chip, hwirq)
> > > > ^^^^^
> > > >
> > > > > + ? TQMX86_INT_TRIG_FALLING
> > > > > + : TQMX86_INT_TRIG_RISING;
> > > > > + }
> > > > > +
> > > > > mask = TQMX86_GPII_MASK(offset);
> > > > ^^^^^^
> > > > > val = TQMX86_GPII_CONFIG(offset, type);
> > > > ^^^^^^
> > > > > _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
> > > >
> > > > The offset stuff wasn't beautiful and I'm glad you are deleting it. My
> > > > understanding is that a hwirq is 0-3 for output or 4-7 input. An offset
> > > > is "hwirq % 4"?
> > > >
> > > > There are a bunch of places which are still marked as taking an offset
> > > > but they all actually take a hwirq. For example, tqmx86_gpio_get()
> > > > above. The only things which still actually take an offset are the
> > > > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros.
> > > >
> > > > Could you:
> > > > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq?
> > > > 2) Rename all the "offset" variables to "hwirq"?
> > >
> > > Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers
> > > depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and
> > > hwirq numbers differ, is it?
> > >
> > > Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7,
> > > while GPIOs 0-3 don't have interrupt support.
> >
> > I'm so confused.
> >
> > So "offset" is the GPIO number and "hwirq" is the IRQ number? If the
> > IRQ numbers are 0-3 then why do we subtract 4 to get the GPIO number in
>
> The current naming in the driver is confusing and I'll fix that in the next round of refactoring
> patches.
>
> Generally, hwirq == GPIO number (I have not found a way to change this mapping - if there is one,
> I'd be interested to try if it makes the code less confusing). "offset" currently always refers to
> some shift in a hardware register. In tqmx86_gpio_get and tqmx86_gpio_set, offset is a GPIO number.
> In all functions dealing with IRQs, offset is an IRQ number (which is different from the hwirq
> number).
>
> Matthias
Ah, I just noticed the "to_irq" function in struct gpio_chip, I guess I simply didn't look in the
right place before. Will have a closer look when I rebase the refactoring patches.
Matthias
>
>
> > _tqmx86_gpio_irq_config()?
> >
> > unsigned int offset = hwirq - TQMX86_NGPO;
> >
> > And again, it's just weird to call:
> >
> > type = tqmx86_gpio_get(&gpio->chip, hwirq);
> >
> > where we're passing "hwirq" when tqmx86_gpio_get() takes an "offset" as
> > an argument.
> >
> > regards,
> > dan carpenter
> >
>
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
Powered by blists - more mailing lists