[PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Wolfram Sang]

Most I2C controllers do not have a dedicated pin for SMBus Alerts. Allow
them to define a GPIO as a side-channel.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 dtschema/schemas/i2c/i2c-controller.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/dtschema/schemas/i2c/i2c-controller.yaml b/dtschema/schemas/i2c/i2c-controller.yaml
index 97d0aaa..487e669 100644
--- a/dtschema/schemas/i2c/i2c-controller.yaml
+++ b/dtschema/schemas/i2c/i2c-controller.yaml
@@ -135,6 +135,11 @@ properties:
       use this information to detect a stalled bus more reliably, for example.
       Can not be combined with 'multi-master'.
 
+  smbalert-gpios:
+    maxItems: 1
+    description:
+      Specifies the GPIO used for the SMBALERT# line. Optional.
+
   smbus:
     type: boolean
     description:
-- 
2.43.0

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Rob Herring]

On Mon, Sep 9, 2024 at 5:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Most I2C controllers do not have a dedicated pin for SMBus Alerts. Allow
> them to define a GPIO as a side-channel.

Most GPIOs are also interrupts, so shouldn't the existing binding be
sufficient? The exception is if the GPIO needs to be polled.

>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  dtschema/schemas/i2c/i2c-controller.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/dtschema/schemas/i2c/i2c-controller.yaml b/dtschema/schemas/i2c/i2c-controller.yaml
> index 97d0aaa..487e669 100644
> --- a/dtschema/schemas/i2c/i2c-controller.yaml
> +++ b/dtschema/schemas/i2c/i2c-controller.yaml
> @@ -135,6 +135,11 @@ properties:
>        use this information to detect a stalled bus more reliably, for example.
>        Can not be combined with 'multi-master'.
>
> +  smbalert-gpios:
> +    maxItems: 1
> +    description:
> +      Specifies the GPIO used for the SMBALERT# line. Optional.
> +
>    smbus:
>      type: boolean
>      description:
> --
> 2.43.0
>
>

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Geert Uytterhoeven]

Hi Rob,

On Mon, Sep 9, 2024 at 3:07 PM Rob Herring <robh@kernel.org> wrote:
> On Mon, Sep 9, 2024 at 5:58 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> >
> > Most I2C controllers do not have a dedicated pin for SMBus Alerts. Allow
> > them to define a GPIO as a side-channel.
>
> Most GPIOs are also interrupts, so shouldn't the existing binding be
> sufficient? The exception is if the GPIO needs to be polled.

If the GPIO pin supports multiple functions, it must be configured as
a GPIO  first. devm_gpiod_get() takes care of that.  Just calling
request_irq() does not.  In addition, the mapping from GPIO to IRQ
number may not be fixed, e.g. in case the GPIO controller supports
less interrupt inputs than GPIOs, and needs to map them when requested.

See also the different handling of interrupts and gpios by gpio-keys.

> > --- a/dtschema/schemas/i2c/i2c-controller.yaml
> > +++ b/dtschema/schemas/i2c/i2c-controller.yaml
> > @@ -135,6 +135,11 @@ properties:
> >        use this information to detect a stalled bus more reliably, for example.
> >        Can not be combined with 'multi-master'.
> >
> > +  smbalert-gpios:
> > +    maxItems: 1
> > +    description:
> > +      Specifies the GPIO used for the SMBALERT# line. Optional.
> > +
> >    smbus:
> >      type: boolean
> >      description:

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Rob Herring]

On Mon, Sep 9, 2024 at 8:31 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Mon, Sep 9, 2024 at 3:07 PM Rob Herring <robh@kernel.org> wrote:
> > On Mon, Sep 9, 2024 at 5:58 AM Wolfram Sang
> > <wsa+renesas@sang-engineering.com> wrote:
> > >
> > > Most I2C controllers do not have a dedicated pin for SMBus Alerts. Allow
> > > them to define a GPIO as a side-channel.
> >
> > Most GPIOs are also interrupts, so shouldn't the existing binding be
> > sufficient? The exception is if the GPIO needs to be polled.
>
> If the GPIO pin supports multiple functions, it must be configured as
> a GPIO  first. devm_gpiod_get() takes care of that.  Just calling
> request_irq() does not.  In addition, the mapping from GPIO to IRQ
> number may not be fixed, e.g. in case the GPIO controller supports
> less interrupt inputs than GPIOs, and needs to map them when requested.

All sounds like Linux problems...

> See also the different handling of interrupts and gpios by gpio-keys.

I believe "gpios" is what was originally supported, but now it is
preferred if GPIOs are used as interrupts then we use interrupts in
DT.

Rob

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Geert Uytterhoeven]

Hi Rob,

On Mon, Sep 9, 2024 at 3:40 PM Rob Herring <robh@kernel.org> wrote:
> On Mon, Sep 9, 2024 at 8:31 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Sep 9, 2024 at 3:07 PM Rob Herring <robh@kernel.org> wrote:
> > > On Mon, Sep 9, 2024 at 5:58 AM Wolfram Sang
> > > <wsa+renesas@sang-engineering.com> wrote:
> > > >
> > > > Most I2C controllers do not have a dedicated pin for SMBus Alerts. Allow
> > > > them to define a GPIO as a side-channel.
> > >
> > > Most GPIOs are also interrupts, so shouldn't the existing binding be
> > > sufficient? The exception is if the GPIO needs to be polled.
> >
> > If the GPIO pin supports multiple functions, it must be configured as
> > a GPIO  first. devm_gpiod_get() takes care of that.  Just calling
> > request_irq() does not.  In addition, the mapping from GPIO to IRQ
> > number may not be fixed, e.g. in case the GPIO controller supports
> > less interrupt inputs than GPIOs, and needs to map them when requested.
>
> All sounds like Linux problems...

;-)

> > See also the different handling of interrupts and gpios by gpio-keys.
>
> I believe "gpios" is what was originally supported, but now it is
> preferred if GPIOs are used as interrupts then we use interrupts in
> DT.

You really do not want to use gpio-keys with interrupts, unless you
have no choice.  Some shortcomings are outlined in "[PATCH RFC 3/3]
Input: gpio-keys - Fix ghost events with both-edge irqs" [1].
They do not matter for SMBALERT, though.

[1] https://lore.kernel.org/r/356b31ade897af77a06d6567601f038f56b3b2a2.1638538079.git.geert+renesas@glider.be

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Geert Uytterhoeven]

Hi Rob,

On Mon, Sep 9, 2024 at 3:40 PM Rob Herring <robh@kernel.org> wrote:
> On Mon, Sep 9, 2024 at 8:31 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Sep 9, 2024 at 3:07 PM Rob Herring <robh@kernel.org> wrote:
> > > On Mon, Sep 9, 2024 at 5:58 AM Wolfram Sang
> > > <wsa+renesas@sang-engineering.com> wrote:
> > > >
> > > > Most I2C controllers do not have a dedicated pin for SMBus Alerts. Allow
> > > > them to define a GPIO as a side-channel.
> > >
> > > Most GPIOs are also interrupts, so shouldn't the existing binding be
> > > sufficient? The exception is if the GPIO needs to be polled.
> >
> > If the GPIO pin supports multiple functions, it must be configured as
> > a GPIO  first. devm_gpiod_get() takes care of that.  Just calling
> > request_irq() does not.  In addition, the mapping from GPIO to IRQ
> > number may not be fixed, e.g. in case the GPIO controller supports
> > less interrupt inputs than GPIOs, and needs to map them when requested.
>
> All sounds like Linux problems...

Let me rephrase in the familiar way: in the case of the latter, the
interrupt number is not a property of the hardware, but depends on
software configuration.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

Hi Rob,

thanks for your review!

> I believe "gpios" is what was originally supported, but now it is
> preferred if GPIOs are used as interrupts then we use interrupts in
> DT.

I had this originally in my RFC[1]. I got convinced by Geert's arguments
because the DT snippet in the board DTS looked kinda ugly. The board
needs to override the DTSI of the SoC to replace "interrupts" with
"interrupts-extended":

===

 &i2c3	{
 	pinctrl-0 = <&i2c3_pins>;
 	pinctrl-names = "i2c-pwr";
+
+	/delete-property/ interrupts;
+	interrupts-extended = <&gic GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>, <&gpio1 26 IRQ_TYPE_EDGE_FALLING>;
+	interrupt-names = "main", "smbus_alert";
+
+	smbus;
 };

===

It works, though.

All the best,

   Wolfram

[1] http://patchwork.ozlabs.org/project/linux-i2c/patch/20240826150840.25497-5-wsa+renesas@sang-engineering.com/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]


> I had this originally in my RFC[1]. I got convinced by Geert's arguments
> because the DT snippet in the board DTS looked kinda ugly. The board
> needs to override the DTSI of the SoC to replace "interrupts" with
> "interrupts-extended":
> 
> ===
> 
>  &i2c3	{
>  	pinctrl-0 = <&i2c3_pins>;
>  	pinctrl-names = "i2c-pwr";
> +
> +	/delete-property/ interrupts;
> +	interrupts-extended = <&gic GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>, <&gpio1 26 IRQ_TYPE_EDGE_FALLING>;
> +	interrupt-names = "main", "smbus_alert";
> +
> +	smbus;
>  };
> 
> ===

I guess my questions here are: is this proper? Is there a better way to
describe it? Is using interrupts still the way to go?

Thanks for the guidance and happy hacking!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]


> > I had this originally in my RFC[1]. I got convinced by Geert's arguments
> > because the DT snippet in the board DTS looked kinda ugly. The board
> > needs to override the DTSI of the SoC to replace "interrupts" with
> > "interrupts-extended":
> > 
> > ===
> > 
> >  &i2c3	{
> >  	pinctrl-0 = <&i2c3_pins>;
> >  	pinctrl-names = "i2c-pwr";
> > +
> > +	/delete-property/ interrupts;
> > +	interrupts-extended = <&gic GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>, <&gpio1 26 IRQ_TYPE_EDGE_FALLING>;
> > +	interrupt-names = "main", "smbus_alert";
> > +
> > +	smbus;
> >  };
> > 
> > ===
> 
> I guess my questions here are: is this proper? Is there a better way to
> describe it? Is using interrupts still the way to go?

Hi Rob,

do you still prefer "interrupts" over "smbalert-gpios" given the above
snippet?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line

[Rob Herring]

On Mon, Sep 9, 2024 at 5:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Most I2C controllers do not have a dedicated pin for SMBus Alerts. Allow
> them to define a GPIO as a side-channel.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  dtschema/schemas/i2c/i2c-controller.yaml | 5 +++++
>  1 file changed, 5 insertions(+)

I guess GPIO rather than interrupt is fine.

Will apply it.

Rob