Message-ID: <3a2475f8-6373-4dd1-b605-b58c74a97fee@kernel.org>
Date: Fri, 29 Aug 2025 12:51:12 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Christophe Leroy <christophe.leroy@...roup.eu>,
 Rob Herring <robh@...nel.org>
Cc: Qiang Zhao <qiang.zhao@....com>, Linus Walleij
 <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
 linuxppc-dev@...ts.ozlabs.org, linux-arm-kernel@...ts.infradead.org,
 linux-gpio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 5/6] dt-bindings: soc: fsl: qe: Add support of IRQ in
 QE GPIO

On 29/08/2025 11:41, Christophe Leroy wrote:
>>>
>>> That's more or less the same here with my series, patch 1 implements an
>>> interrupt controller (documented in patch 6) and then the GPIO
>>> controllers consume the interrupts, for instance in gpiolib functions
>>> gpio_sysfs_request_irq() [drivers/gpio/gpiolib-sysfs.c] or
>>> edge_detector_setup() or debounce_setup() [drivers/gpio/gpiolib-cdev.c]
>>>
>>> External drivers also use interrupts indirectly. For example driver
>>> sound/soc/soc-jack.c, it doesn't have any direct reference to an
>>> interrupt. The driver is given an array of GPIOs and then installs an
>>> IRQ in function snd_soc_jack_add_gpios() by doing
>>>
>>> 	request_any_context_irq(gpiod_to_irq(gpios[i].desc),
>>> 					      gpio_handler,
>>> 					      IRQF_SHARED |
>>> 					      IRQF_TRIGGER_RISING |
>>> 					      IRQF_TRIGGER_FALLING,
>>> 					      gpios[i].name,
>>> 					      &gpios[i]);
>>
>>
>> External drivers do not matter then. Your GPIO controller receives
>> specific interrupts (that's the interrupt property) and knows exactly
>> how each GPIO maps to it.
>>
> 
> Do you mean then that the GPIO driver should already know which line has 

The SoC knows, that's fixed information, so shall GPIO driver know as well.

> an interrupt and which one doesn't ?
> 
> The interrupts are fixed per soc, but today the GPIO driver is generic 
> and used on different SOCs that don't have interrupts on the same lines. 

How you write drivers is one thing, but that's never a reason alone to
add properties to the DT.

> And even on the given SOCs, not all ports have interrupts on the same 

That's pretty standard between all GPIO/pinctrl drivers. I would
generalize that's pretty standard for all SoCs - they have differences
within devices, some pins do that, some do different things.

> lines. Should all possibilities be hard-coded inside the driver for each 
> possible compatible ? The property 'fsl,qe-gpio-irq-mask' is there to 

There are many ways how to do it in the driver, that feels like one of
them, so yes, it should.

> avoid that and keep the GPIO driver as generic as possible with a single 

Sorry, that approach, which leads to moving such stuff to DT, was many
times on mailing list rejected. You use the same argument as that "one
clock, one device node" TI approach. It got it way to kernel long time
ago but since then was pretty discouraged (rejected for new SoCs). It
re-appeared again few months ago in a form of "I have two registers, so
I have two device nodes in DT", so it seems poor code keeps re-appearing.

> compatible 'fsl,mpc8323-qe-pario-bank' that is found in the DTS of 8323 
> but also in DTS of 8360, in DTS of 8569, etc... :
> 
> arch/powerpc/boot/dts/fsl/mpc8569mds.dts: 
>              "fsl,mpc8323-qe-pario-bank";
> arch/powerpc/boot/dts/fsl/mpc8569mds.dts: 
>              "fsl,mpc8323-qe-pario-bank";
> arch/powerpc/boot/dts/kmeter1.dts: 
>      "fsl,mpc8323-qe-pario-bank";
> arch/powerpc/boot/dts/mpc832x_rdb.dts: 
> compatible = "fsl,mpc8323-qe-pario-bank";
> arch/powerpc/boot/dts/mpc836x_rdk.dts: 
> "fsl,mpc8323-qe-pario-bank";
> arch/powerpc/boot/dts/mpc836x_rdk.dts: 
> "fsl,mpc8323-qe-pario-bank";
> 
> Do you mean we should define one compatible for each of the ports of 
> each soc, and encode the mask of interrupts that define which line of 
> the port has interrupts in the data field ?

I don't know that good your hardware to tell.

> 
> Something like:
> 
> static const struct of_device_id qe_gpio_match[] = {
> 	{
> 		.compatible = "fsl,mpc8323-qe-pario-bank-a",
> 		.data = (void *)0x00a00028,

There is no DTS in your patchset, so I cannot help really. I just don't
have time to imagine such DTS and try to figure out how it can be
written. Posting complete picture usually helps.

I don't follow what is the bank.

You have a device, yes?
It has some grouped GPIOs (banks?) and some within group/bank can handle
interrupts?
Are these fixed per SoC? Yes. Well, that's standard and every other
vendor has the same. They solve it in the drivers differently, though.

> 	},
> 	{
> 		.compatible = "fsl,mpc8323-qe-pario-bank-b",
> 		.data = (void *)0x01400050,
> 	},
> 	{
> 		.compatible = "fsl,mpc8323-qe-pario-bank-c",
> 		.data = (void *)0x00000084,
> 	},
> 	{
> 		.compatible = "fsl,mpc8323-qe-pario-bank-d",
> 		.data = (void *)0,
> 	},
> 	{
> 		.compatible = "fsl,mpc8360-qe-pario-bank-a",
> 		.data = (void *)0xXXXXXXXX,
> 	},
> 	{
> 		.compatible = "fsl,mpc8360-qe-pario-bank-b",
> 		.data = (void *)0xXXXXXXXX,
> 	},
> ....
> 	{},
> };
> MODULE_DEVICE_TABLE(of, qe_gpio_match);
> 
> That would be feasible but would mean updating the driver each time a 
> new SOC is added.

BTW, like every other platform.

> 
> Do you mean it should be done that way ?
> 
> Isn't the purpose of the device tree to keep drivers as generic as 
> possible ?

Not at all, sorry. The purpose of DT is not to keep drivers generic.


Best regards,
Krzysztof

Powered by blists - more mailing lists