| Message ID | 20240909105835.28531-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [dt-schema] schemas: i2c: add optional GPIO binding for SMBALERT# line | expand |
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 > >
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
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
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
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
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/
> 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!
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:
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(+)