Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>]

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. We will soon be mainlining support for our SMARC
modules, which have up to 14 GPIOs, and (on some families) IRQ support for all GPIOs (IRQs 0-13
correspond to GPIOs 0-13).

New interrupt config and status registers have been introduced to support more IRQs - up to 4 config
registers (2 bits for each IRQ) and 3 status registers (IRQs 0-3 in the first one, 4-11 in the
second one, 12-13 in the third one... so this part is a bit more convoluted than just "hwirq % 4") 

As the mapping between GPIOs and IRQs will become dynamic with these changes, I'd rather keep
TQMX86_GPII_* using IRQ numbers instead of GPIO numbers. We will be introducing helpers for
accessing the interrupt registers; the macros deal with individual register bits, and I think they
should be agnostic of the mapping to GPIO/hwirq numbers.

Matthias



> 
> 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/