diff mbox series

[RFC,1/2] dt-bindings: i2c: renesas,rcar-i2c: document SMBusAlert usage

Message ID 20240826150840.25497-5-wsa+renesas@sang-engineering.com (mailing list archive)
State RFC
Delegated to: Geert Uytterhoeven
Headers show
Series i2c: rcar: add SMBAlert support | expand

Commit Message

Wolfram Sang Aug. 26, 2024, 3:08 p.m. UTC
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Question: Should I remove 'smbus_alert' from the enum of
'interrupt-names'? It is already documented here:

https://github.com/devicetree-org/dt-schema/commit/c51125d571cac9596048e888a856d70650e400e0


 .../bindings/i2c/renesas,rcar-i2c.yaml         | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven Aug. 26, 2024, 3:30 p.m. UTC | #1
Hi Wolfram,

On Mon, Aug 26, 2024 at 5:08 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Question: Should I remove 'smbus_alert' from the enum of
> 'interrupt-names'? It is already documented here:
>
> https://github.com/devicetree-org/dt-schema/commit/c51125d571cac9596048e888a856d70650e400e0

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
> @@ -60,7 +60,20 @@ properties:
>      maxItems: 1
>
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
> +    description:
> +      Without interrupt-names, the first interrupt listed must be the one
> +      of the IP core, the second optional interrupt listed must handle
> +      SMBALERT#, likely a GPIO.
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 2
> +    items:
> +      enum:
> +        - main
> +        - smbus_alert
>
>    clock-frequency:
>      description:

IIUIC, this is not a property of the hardware, but a side-channel
independent from the actual I2C controller hardware? Then a generic
"smbus-alert-gpios" property sounds more appropriate to me.

BTW, are you aware of any I2C controller having a dedicated input pin
for this?

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
Krzysztof Kozlowski Aug. 27, 2024, 6:41 a.m. UTC | #2
On Mon, Aug 26, 2024 at 05:08:42PM +0200, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Question: Should I remove 'smbus_alert' from the enum of
> 'interrupt-names'? It is already documented here:
> 
> https://github.com/devicetree-org/dt-schema/commit/c51125d571cac9596048e888a856d70650e400e0

No, because dtschema is not specific there and allows any combination,
while device bindings should make it constrained.

> 
> 
>  .../bindings/i2c/renesas,rcar-i2c.yaml         | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml b/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
> index 6cc60c3f61cd..2eed3ae7c57d 100644
> --- a/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
> @@ -60,7 +60,20 @@ properties:
>      maxItems: 1
>  
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
> +    description:
> +      Without interrupt-names, the first interrupt listed must be the one
> +      of the IP core, the second optional interrupt listed must handle
> +      SMBALERT#, likely a GPIO.

With interrupt-names the same... unless you want to allow anything? No
clue what is being fixed here, no commit msg. Which interrupts are
flexible? Why main can be skipped suddenly (or was it always)?

> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 2
> +    items:
> +      enum:
> +        - main
> +        - smbus_alert

Best regards,
Krzysztof
Wolfram Sang Aug. 31, 2024, 3:31 p.m. UTC | #3
Hi Geert,

> IIUIC, this is not a property of the hardware, but a side-channel
> independent from the actual I2C controller hardware? Then a generic
> "smbus-alert-gpios" property sounds more appropriate to me.

It is not generic. While it is true that most I2C controllers do need
GPIOs as a side channel, there are controllers having...

> BTW, are you aware of any I2C controller having a dedicated input pin
> for this?

... this. Check 'i2c-stm32f7.c' or 'i2c-xlp9xx.c'.

Thank you for your comments!

   Wolfram
Geert Uytterhoeven Sept. 2, 2024, 9:55 a.m. UTC | #4
Hi Wolfram,

On Sat, Aug 31, 2024 at 5:31 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > IIUIC, this is not a property of the hardware, but a side-channel
> > independent from the actual I2C controller hardware? Then a generic
> > "smbus-alert-gpios" property sounds more appropriate to me.
>
> It is not generic. While it is true that most I2C controllers do need
> GPIOs as a side channel, there are controllers having...
>
> > BTW, are you aware of any I2C controller having a dedicated input pin
> > for this?
>
> ... this. Check 'i2c-stm32f7.c' or 'i2c-xlp9xx.c'.

OK...

Still, this interrupt is not a property of the R-Car i2C hardware block,
so it should not be modelled as such.

To me, this looks similar to hardware flow control on serial ports:
some ports support this in hardware, other ports need to use GPIOs,
or board designers may still decide to use a GPIO anyway.

See Documentation/devicetree/bindings/serial/serial.yaml and
drivers/tty/serial/serial_mctrl_gpio.c, which uses GPIO interrupts.

Gr{oetje,eeting}s,

                        Geert
Wolfram Sang Sept. 2, 2024, 12:04 p.m. UTC | #5
> Still, this interrupt is not a property of the R-Car i2C hardware block,
> so it should not be modelled as such.

Hmm, you are probably right, given that I need this in the board DTS:

===

 &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 have to admit this is not exactly pretty. Pity, though, the I2C core
is all prepared for the above. Seems I have to update the core for
"alert-gpios", after all.

Happy hacking,

   Wolfram
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml b/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
index 6cc60c3f61cd..2eed3ae7c57d 100644
--- a/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/renesas,rcar-i2c.yaml
@@ -60,7 +60,20 @@  properties:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+    description:
+      Without interrupt-names, the first interrupt listed must be the one
+      of the IP core, the second optional interrupt listed must handle
+      SMBALERT#, likely a GPIO.
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      enum:
+        - main
+        - smbus_alert
 
   clock-frequency:
     description:
@@ -155,7 +168,8 @@  examples:
     i2c0: i2c@e6508000 {
         compatible = "renesas,i2c-r8a7791", "renesas,rcar-gen2-i2c";
         reg = <0xe6508000 0x40>;
-        interrupts = <GIC_SPI 287 IRQ_TYPE_LEVEL_HIGH>;
+        interrupts-extended = <&gic GIC_SPI 287 IRQ_TYPE_LEVEL_HIGH>, <&gpio5 1 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-names = "main", "smbus_alert";
         clock-frequency = <400000>;
         clocks = <&cpg CPG_MOD 931>;
         power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;