diff mbox

[v2,2/6] pintrl: meson: document GPIO IRQ DT binding

Message ID c5ea65c5-f604-d890-4e73-f53bdd47bd31@gmail.com
State New
Headers show

Commit Message

Heiner Kallweit May 12, 2017, 7:13 p.m. UTC
Document the DT binding for GPIO IRQ support on Amlogic Meson SoC's.

This documentation is intentionally not placed under
interrupt-controllers as GPIO IRQ support on these SoC's acts more
like an interrupt multiplexer.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- remove syscon
---
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Jerome Brunet May 16, 2017, 11:28 p.m. UTC | #1
On Fri, 2017-05-12 at 21:13 +0200, Heiner Kallweit wrote:
> Document the DT binding for GPIO IRQ support on Amlogic Meson SoC's.
> 
> This documentation is intentionally not placed under
> interrupt-controllers as GPIO IRQ support on these SoC's acts more
> like an interrupt multiplexer.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - remove syscon
> ---
>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> index 436b8750..44422b85 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -312,6 +312,19 @@
>  				status = "disabled";
>  			};
>  
> +			gpio_irq@9880 {
> +				compatible = "amlogic,meson-gpio-interrupt";
> +				reg = <0x0 0x09880 0x0 0x10>;
> +				interrupts = <GIC_SPI 64 IRQ_TYPE_NONE>,
> +					     <GIC_SPI 65 IRQ_TYPE_NONE>,
> +					     <GIC_SPI 66 IRQ_TYPE_NONE>,
> +					     <GIC_SPI 67 IRQ_TYPE_NONE>,
> +					     <GIC_SPI 68 IRQ_TYPE_NONE>,
> +					     <GIC_SPI 69 IRQ_TYPE_NONE>,
> +					     <GIC_SPI 70 IRQ_TYPE_NONE>,
> +					     <GIC_SPI 71 IRQ_TYPE_NONE>;
> +			};
> +

Already tried the same thing:
https://marc.info/?l=devicetree&m=147758174404359&w=2

Irq maintainers reminded me that this is not correct as the device is not able
to generate these particular irqs (it is merely routing the signal) and the
flags are meaning less here


>  			watchdog@98d0 {
>  				compatible = "amlogic,meson-gx-wdt",
> "amlogic,meson-gxbb-wdt";
>  				reg = <0x0 0x098d0 0x0 0x10>;
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heiner Kallweit May 17, 2017, 9:02 p.m. UTC | #2
Am 17.05.2017 um 01:28 schrieb Jerome Brunet:
> On Fri, 2017-05-12 at 21:13 +0200, Heiner Kallweit wrote:
>> Document the DT binding for GPIO IRQ support on Amlogic Meson SoC's.
>>
>> This documentation is intentionally not placed under
>> interrupt-controllers as GPIO IRQ support on these SoC's acts more
>> like an interrupt multiplexer.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v2:
>> - remove syscon
>> ---
>>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> index 436b8750..44422b85 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> @@ -312,6 +312,19 @@
>>  				status = "disabled";
>>  			};
>>  
>> +			gpio_irq@9880 {
>> +				compatible = "amlogic,meson-gpio-interrupt";
>> +				reg = <0x0 0x09880 0x0 0x10>;
>> +				interrupts = <GIC_SPI 64 IRQ_TYPE_NONE>,
>> +					     <GIC_SPI 65 IRQ_TYPE_NONE>,
>> +					     <GIC_SPI 66 IRQ_TYPE_NONE>,
>> +					     <GIC_SPI 67 IRQ_TYPE_NONE>,
>> +					     <GIC_SPI 68 IRQ_TYPE_NONE>,
>> +					     <GIC_SPI 69 IRQ_TYPE_NONE>,
>> +					     <GIC_SPI 70 IRQ_TYPE_NONE>,
>> +					     <GIC_SPI 71 IRQ_TYPE_NONE>;
>> +			};
>> +
> 
> Already tried the same thing:
> https://marc.info/?l=devicetree&m=147758174404359&w=2
> 
> Irq maintainers reminded me that this is not correct as the device is not able
> to generate these particular irqs (it is merely routing the signal) and the
> flags are meaning less here
> 
In one review comment to your patch series Rob mentioned to be fine with using
the standard interrupts property.
However other involved people might have had a different opinion, I didn't read
all review comments.

I think the standard interrupts property is the best way.
We need mapped virq's to be used with the irq chaining. And for mapping an
interrupt we need not only the hwirq but also the IRQ domain. The interrupts
property and the related OF IRQ API calls like irq_of_parse_and_map provide
exactly what we need.

If somebody has serious objections to using the interrupts property then we
basically had to duplicate it and just give it a different name.
And such code duplication to me seems to be worse than using the interrupts
property slightly different than in the theory.

> 
>>  			watchdog@98d0 {
>>  				compatible = "amlogic,meson-gx-wdt",
>> "amlogic,meson-gxbb-wdt";
>>  				reg = <0x0 0x098d0 0x0 0x10>;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jerome Brunet May 23, 2017, 8:35 a.m. UTC | #3
On Wed, 2017-05-17 at 23:02 +0200, Heiner Kallweit wrote:
> Am 17.05.2017 um 01:28 schrieb Jerome Brunet:
> > On Fri, 2017-05-12 at 21:13 +0200, Heiner Kallweit wrote:
> > > Document the DT binding for GPIO IRQ support on Amlogic Meson SoC's.
> > > 
> > > This documentation is intentionally not placed under
> > > interrupt-controllers as GPIO IRQ support on these SoC's acts more
> > > like an interrupt multiplexer.
> > > 
> > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > > ---
> > > v2:
> > > - remove syscon
> > > ---
> > >  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > index 436b8750..44422b85 100644
> > > --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > @@ -312,6 +312,19 @@
> > >  				status = "disabled";
> > >  			};
> > >  
> > > +			gpio_irq@9880 {
> > > +				compatible = "amlogic,meson-gpio-
> > > interrupt";
> > > +				reg = <0x0 0x09880 0x0 0x10>;
> > > +				interrupts = <GIC_SPI 64 IRQ_TYPE_NONE>,
> > > +					     <GIC_SPI 65 IRQ_TYPE_NONE>,
> > > +					     <GIC_SPI 66 IRQ_TYPE_NONE>,
> > > +					     <GIC_SPI 67 IRQ_TYPE_NONE>,
> > > +					     <GIC_SPI 68 IRQ_TYPE_NONE>,
> > > +					     <GIC_SPI 69 IRQ_TYPE_NONE>,
> > > +					     <GIC_SPI 70 IRQ_TYPE_NONE>,
> > > +					     <GIC_SPI 71 IRQ_TYPE_NONE>;
> > > +			};
> > > +
> > 
> > Already tried the same thing:
> > https://marc.info/?l=devicetree&m=147758174404359&w=2
> > 
> > Irq maintainers reminded me that this is not correct as the device is not
> > able
> > to generate these particular irqs (it is merely routing the signal) and the
> > flags are meaning less here
> > 
> 
> In one review comment to your patch series Rob mentioned to be fine with using
> the standard interrupts property.
> However other involved people might have had a different opinion, I didn't
> read
> all review comments.
> 
> I think the standard interrupts property is the best way.
> We need mapped virq's to be used with the irq chaining. And for mapping an
> interrupt we need not only the hwirq but also the IRQ domain. The interrupts
> property and the related OF IRQ API calls like irq_of_parse_and_map provide
> exactly what we need.
> 
> If somebody has serious objections to using the interrupts property then we
> basically had to duplicate it and just give it a different name.

You got the objections in my previous comment.
* This driver does *not* generate these irq, it just route them
* IRQ_TYPE_NONE in meaningless here

This comments initially came from Mark and Marc, on the first RFC I sent lastyear. 

> And such code duplication to me seems to be worse than using the interrupts
> property slightly different than in the theory.
> 
> > 
> > >  			watchdog@98d0 {
> > >  				compatible = "amlogic,meson-gx-wdt",
> > > "amlogic,meson-gxbb-wdt";
> > >  				reg = <0x0 0x098d0 0x0 0x10>;
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 436b8750..44422b85 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -312,6 +312,19 @@ 
 				status = "disabled";
 			};
 
+			gpio_irq@9880 {
+				compatible = "amlogic,meson-gpio-interrupt";
+				reg = <0x0 0x09880 0x0 0x10>;
+				interrupts = <GIC_SPI 64 IRQ_TYPE_NONE>,
+					     <GIC_SPI 65 IRQ_TYPE_NONE>,
+					     <GIC_SPI 66 IRQ_TYPE_NONE>,
+					     <GIC_SPI 67 IRQ_TYPE_NONE>,
+					     <GIC_SPI 68 IRQ_TYPE_NONE>,
+					     <GIC_SPI 69 IRQ_TYPE_NONE>,
+					     <GIC_SPI 70 IRQ_TYPE_NONE>,
+					     <GIC_SPI 71 IRQ_TYPE_NONE>;
+			};
+
 			watchdog@98d0 {
 				compatible = "amlogic,meson-gx-wdt", "amlogic,meson-gxbb-wdt";
 				reg = <0x0 0x098d0 0x0 0x10>;