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

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

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

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

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

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

> 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