| Message ID | 2c265b6bcfcde7d2327b94c4f6e3ad6d4f1e2de7.1716967982.git.matthias.schiffer@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio-tqmx86 fixes | expand |
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 > _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 >
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); @@ -200,8 +206,8 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc) struct gpio_chip *chip = irq_desc_get_handler_data(desc); struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip); struct irq_chip *irq_chip = irq_desc_get_chip(desc); - unsigned long irq_bits; - int i = 0; + unsigned long irq_bits, flags; + int i, hwirq; u8 irq_status; chained_irq_enter(irq_chip, desc); @@ -210,6 +216,36 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc) tqmx86_gpio_write(gpio, TQMX86_GPIIS, irq_status); irq_bits = irq_status; + + raw_spin_lock_irqsave(&gpio->spinlock, flags); + for_each_set_bit(i, &irq_bits, TQMX86_NGPI) { + hwirq = i + TQMX86_NGPO; + + /* + * Edge-both triggers are implemented by flipping the edge + * trigger after each interrupt, as the controller only supports + * either rising or falling edge triggers, but not both. + * + * Internally, the TQMx86 GPIO controller has separate status + * registers for rising and falling edge interrupts. GPIIC + * configures which bits from which register are visible in the + * interrupt status register GPIIS and defines what triggers the + * parent IRQ line. Writing to GPIIS always clears both rising + * and falling interrupt flags internally, regardless of the + * currently configured trigger. + * + * In consequence, we can cleanly implement the edge-both + * trigger in software by first clearing the interrupt and then + * setting the new trigger based on the current GPIO input in + * _tqmx86_gpio_irq_config() - even if an edge arrives between + * reading the input and setting the trigger, we will have a new + * interrupt pending. + */ + if ((gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK) == TQMX86_INT_TRIG_BOTH) + _tqmx86_gpio_irq_config(gpio, hwirq); + } + raw_spin_unlock_irqrestore(&gpio->spinlock, flags); + for_each_set_bit(i, &irq_bits, TQMX86_NGPI) generic_handle_domain_irq(gpio->chip.irq.domain, i + TQMX86_NGPO);
The TQMx86 GPIO controller only supports falling and rising edge triggers, but not both. Fix this by implementing a software both-edge mode that toggles the edge type after every interrupt. Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller") Co-developed-by: Gregor Herburger <gregor.herburger@tq-group.com> Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/gpio/gpio-tqmx86.c | 42 +++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)