[RFC PATCH 0/2] i2c: rcar: add SMBAlert support

[RFC PATCH 0/2] i2c: rcar: add SMBAlert support

[Wolfram Sang]

This series allows an R-Car I2C IP core to handle SMBAlerts using a
GPIO. It was tested on a Renesas Lager board (R-Car H2) and the bindings
were checked with 'dtbs_check' and the latest dtschema which also
include SMBAlert additions[1].

The series is still marked RFC because I have one question left, see the
annotations of patch 1.

Looking forward to comments! Happy hacking...

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


Wolfram Sang (2):
  dt-bindings: i2c: renesas,rcar-i2c: document SMBusAlert usage
  i2c: rcar: support named interrupts

 .../bindings/i2c/renesas,rcar-i2c.yaml         | 18 ++++++++++++++++--
 drivers/i2c/busses/i2c-rcar.c                  | 11 +++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.43.0

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

[Wolfram Sang]

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(-)

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>;
-- 
2.43.0

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

[Geert Uytterhoeven]

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

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

[Wolfram Sang]

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

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


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

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

[Geert Uytterhoeven]

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

-- 
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: [RFC PATCH 1/2] dt-bindings: i2c: renesas,rcar-i2c: document SMBusAlert usage

[Wolfram Sang]

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


> 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


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

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

[Krzysztof Kozlowski]

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

[RFC PATCH 2/2] i2c: rcar: support named interrupts

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 1104aafe4433..1a232bc88bed 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1192,10 +1192,13 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 			goto out_pm_put;
 	}
 
-	ret = platform_get_irq(pdev, 0);
-	if (ret < 0)
-		goto out_pm_put;
-	priv->irq = ret;
+	priv->irq = platform_get_irq_byname_optional(pdev, "main");
+	if (priv->irq < 0) {
+		priv->irq = platform_get_irq(pdev, 0);
+		if (priv->irq < 0)
+			goto out_pm_put;
+	}
+
 	ret = devm_request_irq(dev, priv->irq, irqhandler, irqflags, dev_name(dev), priv);
 	if (ret < 0) {
 		dev_err(dev, "cannot get irq %d\n", priv->irq);
-- 
2.43.0

Re: [RFC PATCH 2/2] i2c: rcar: support named interrupts

[Wolfram Sang]

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

On Mon, Aug 26, 2024 at 05:08:43PM +0200, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 1104aafe4433..1a232bc88bed 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -1192,10 +1192,13 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>  			goto out_pm_put;
>  	}
>  
> -	ret = platform_get_irq(pdev, 0);
> -	if (ret < 0)
> -		goto out_pm_put;
> -	priv->irq = ret;
> +	priv->irq = platform_get_irq_byname_optional(pdev, "main");
> +	if (priv->irq < 0) {
> +		priv->irq = platform_get_irq(pdev, 0);
> +		if (priv->irq < 0)
> +			goto out_pm_put;

'ret' should be set here, of course! Will fix when resending this
series.

> +	}
> +
>  	ret = devm_request_irq(dev, priv->irq, irqhandler, irqflags, dev_name(dev), priv);
>  	if (ret < 0) {
>  		dev_err(dev, "cannot get irq %d\n", priv->irq);
> -- 
> 2.43.0
> 

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