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