Received-SPF: pass (google.com: domain of linux-kernel+bounces-195077-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <0e971f0b885bd360e33ef472d96e3d9e0ab56405.camel@ew.tq-group.com>
Subject: Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH
 interrupt type
From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>, Bartosz Golaszewski
 <brgl@bgdev.pl>, Andrew Lunn <andrew@lunn.ch>, linux-gpio@vger.kernel.org, 
 linux-kernel@vger.kernel.org, Gregor Herburger
 <gregor.herburger@tq-group.com>,  linux@ew.tq-group.com
Date: Thu, 30 May 2024 10:39:25 +0200
In-Reply-To: <8689fbcd-3fa3-410b-8fc9-7a699bf163b8@moroto.mountain>
References: <cover.1716967982.git.matthias.schiffer@ew.tq-group.com>
	 <2c265b6bcfcde7d2327b94c4f6e3ad6d4f1e2de7.1716967982.git.matthias.schiffer@ew.tq-group.com>
	 <8689fbcd-3fa3-410b-8fc9-7a699bf163b8@moroto.mountain>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0

On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote:
>=20
> 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 =3D hwirq - TQMX86_NGPO;
> >  	u8 type =3D TQMX86_INT_TRIG_NONE, mask, val;
> > =20
> > -	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
> > +	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
> >  		type =3D gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
> > =20
> > +		if (type =3D=3D TQMX86_INT_TRIG_BOTH)
> > +			type =3D tqmx86_gpio_get(&gpio->chip, hwirq)
>                                                             ^^^^^
>=20
> > +				? TQMX86_INT_TRIG_FALLING
> > +				: TQMX86_INT_TRIG_RISING;
> > +	}
> > +
> >  	mask =3D TQMX86_GPII_MASK(offset);
>                                 ^^^^^^
> >  	val =3D TQMX86_GPII_CONFIG(offset, type);
>                                  ^^^^^^
> >  	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
>=20
> 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"?
>=20
> 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.
>=20
> 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 cor=
respond to GPIOs 4-7,
while GPIOs 0-3 don't have interrupt support. We will soon be mainlining su=
pport 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 m=
ore IRQs - up to 4 config
registers (2 bits for each IRQ) and 3 status registers (IRQs 0-3 in the fir=
st 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")=20

As the mapping between GPIOs and IRQs will become dynamic with these change=
s, I'd rather keep
TQMX86_GPII_* using IRQ numbers instead of GPIO numbers. We will be introdu=
cing 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


>=20
> regards,
> dan carpenter
>=20

--=20
TQ-Systems GmbH | M=C3=BChlstra=C3=9Fe 2, Gut Delling | 82229 Seefeld, Germ=
any
Amtsgericht M=C3=BCnchen, HRB 105018
Gesch=C3=A4ftsf=C3=BChrer: Detlef Schneider, R=C3=BCdiger Stahl, Stefan Sch=
neider
https://www.tq-group.com/