Re: [PATCH v4 6/7] riscv: dts: eswin: add I2C controller support

[Pinkesh Vaghela]

Hi Samuel,

> -----Original Message-----
> From: Samuel Holland <samuel.holland@xxxxxxxxxx>
> Sent: Tuesday, September 29, 2026 6:40 PM
> To: Pinkesh Vaghela <pinkesh.vaghela@xxxxxxxxxxxxxx>
> Cc: Yulin Lu <luyulin@xxxxxxxxxxxxxxxxxx>; Darshan Prajapati
> <darshan.prajapati@xxxxxxxxxxxxxx>; Pritesh Patel
> <pritesh.patel@xxxxxxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Paul
> Walmsley <pjw@xxxxxxxxxx>; Palmer Dabbelt <palmer@xxxxxxxxxxx>; Albert
> Ou <aou@xxxxxxxxxxxxxxxxx>; Alexandre Ghiti <alex@xxxxxxxx>; Andi Shyti
> <andi.shyti@xxxxxxxxxx>; Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> riscv@xxxxxxxxxxxxxxxxxxx; Min Lin <linmin@xxxxxxxxxxxxxxxxxx>
> Subject: [External] Re: [PATCH v4 6/7] riscv: dts: eswin: add I2C controller
> support
> 
> Hi Pinkesh,
> 
> On 2026-08-14 2:00 AM, Pinkesh Vaghela wrote:
> > From: Pritesh Patel <pritesh.patel@xxxxxxxxxxxxxx>
> >
> > Add I2C nodes for EIC7700 SoC.
> > Also add nodes for corresponding slave devices in dts file and enable
> > them for HiFive Premier P550 board
> >
> > Signed-off-by: Pritesh Patel <pritesh.patel@xxxxxxxxxxxxxx>
> > Signed-off-by: Pinkesh Vaghela <pinkesh.vaghela@xxxxxxxxxxxxxx>
> > ---
> >  .../dts/eswin/eic7700-hifive-premier-p550.dts |  56 +++++++
> >  arch/riscv/boot/dts/eswin/eic7700.dtsi        | 156 ++++++++++++++++++
> >  2 files changed, 212 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > index 3101fd90d171..5dddcf13fd25 100644
> > --- a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > +++ b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > @@ -130,6 +130,62 @@ &gpio111_pins {
> >       input-disable;
> >  };
> >
> > +&aon_i2c0 {
> > +     status = "okay";
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&i2c10_pins>;
> > +
> > +     eeprom@50 {
> > +             compatible = "atmel,24c02";
> > +             reg = <0x50>;
> > +     };
> > +};
> > +
> > +&aon_i2c1 {
> > +     status = "okay";
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&i2c11_pins>;
> > +
> > +     adc@10 {
> > +             compatible = "microchip,pac1934";
> > +             reg = <0x10>;
> > +             #address-cells = <1>;
> > +             #size-cells = <0>;
> > +
> > +             channel@1 {
> > +                     reg = <0x1>;
> > +                     shunt-resistor-micro-ohms = <1000>;
> > +                     label = "VDD_SOM";
> > +             };
> > +
> > +             channel@2 {
> > +                     reg = <0x2>;
> > +                     shunt-resistor-micro-ohms = <1000>;
> > +                     label = "VDD_SOC";
> > +             };
> > +
> > +             channel@3 {
> > +                     reg = <0x3>;
> > +                     shunt-resistor-micro-ohms = <1000>;
> > +                     label = "VDD_CPU";
> > +             };
> > +
> > +             channel@4 {
> > +                     reg = <0x4>;
> > +                     shunt-resistor-micro-ohms = <1000>;
> > +                     label = "VDD_LPDDR";
> > +             };
> > +     };
> > +
> > +     power-sensor@44 {
> > +             compatible = "ti,ina226";
> > +             reg = <0x44>;
> > +             #io-channel-cells = <1>;
> > +             label = "sys_power";
> > +             shunt-resistor = <1000>;
> > +     };
> > +};
> > +
> >  &pinctrl {
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&gpio0_pins>, <&gpio5_pins>, <&gpio11_pins>, diff
> > --git a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > index 771a88645d83..c13432d14504 100644
> > --- a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > +++ b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > @@ -315,6 +315,162 @@ uart4: serial@50940000 {
> >                       status = "disabled";
> >               };
> >
> > +             i2c0: i2c@50950000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x50950000 0x0 0x8000>;
> > +                     interrupts = <105>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C0_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C0>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> 
> Each controller that has only one possible pinmux should reference the
> corresponding pinctrl nodes, so it doesn't have to be duplicated in every
> board DTS.

We will update this in next version.

> 
> > +             };
> > +
> > +             i2c1: i2c@50960000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x50960000 0x0 0x8000>;
> > +                     interrupts = <106>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C1_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C1>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c2: i2c@50970000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x50970000 0x0 0x8000>;
> > +                     interrupts = <107>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C2_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C2>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c3: i2c@50980000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x50980000 0x0 0x8000>;
> > +                     interrupts = <108>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C3_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C3>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c4: i2c@50990000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x50990000 0x0 0x8000>;
> > +                     interrupts = <109>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C4_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C4>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c5: i2c@509a0000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x509a0000 0x0 0x8000>;
> > +                     interrupts = <110>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C5_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C5>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c6: i2c@509b0000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x509b0000 0x0 0x8000>;
> > +                     interrupts = <111>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C6_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C6>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c7: i2c@509c0000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x509c0000 0x0 0x8000>;
> > +                     interrupts = <112>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C7_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C7>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c8: i2c@509d0000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x509d0000 0x0 0x8000>;
> > +                     interrupts = <113>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C8_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C8>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             i2c9: i2c@509e0000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x509e0000 0x0 0x8000>;
> > +                     interrupts = <114>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_I2C9_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_I2C9>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             aon_i2c0: i2c@51830000 {
> 
> Please keep nodes sorted (the aon block controllers go below GPIO, etc.).

As per DTS coding style document [1], "Order of Nodes" states that
"Nodes on any bus, thus using unit addresses for children, shall be ordered by unit
address in ascending order. Alternatively for some subarchitectures, nodes of the 
same type can be grouped together, e.g. all I2C controllers one after another even
if this breaks unit address ordering."

Do you still prefer to change order?

[1] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html

Regards,
Pinkesh

> 
> Regards,
> Samuel
> 
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x51830000 0x0 0x8000>;
> > +                     interrupts = <290>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_AON_I2C0_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_ANO0>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             aon_i2c1: i2c@51838000 {
> > +                     compatible = "eswin,eic7700-i2c", "snps,designware-i2c";
> > +                     reg = <0x0 0x51838000 0x0 0x8000>;
> > +                     interrupts = <291>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_AON_I2C1_PCLK>;
> > +                     clock-names = "ref";
> > +                     clock-frequency = <100000>;
> > +                     resets = <&reset EIC7700_RESET_ANO1>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     status = "disabled";
> > +             };
> > +
> >               gpio@51600000 {
> >                       compatible = "snps,dw-apb-gpio";
> >                       reg = <0x0 0x51600000 0x0 0x80>;