Re: Re: [PATCH 3/7] riscv: dts: eswin: eic7700: add pinctrl support

[Yulin Lu]

Hi, sashiko.

> [Severity: Low]
> Should this node be named using the generic regulator-* pattern (such as
> regulator-1v8) to avoid dtbs_check warnings for deprecated names?
> There is also an extra leading space on the properties within this node
> that deviates from standard spacing.

I will rename this node to "regulator-1v8".

> > +  regulator-name = "vcc1v8";
> > +  regulator-always-on;
> > +  regulator-boot-on;
> > +  regulator-min-microvolt = <1800000>;
> > +  regulator-max-microvolt = <1800000>;
> > +  };
> >  };
> [ ... ]
> > @@ -25,6 +35,105 @@ &xtal {
>   clock-output-names = "xtal24m";
> >  };
> >  
> > +&gpio0_pins {
> > + bias-disable;
> > + input-enable;
> > +};
> [Severity: High]
> Will these pin configuration overrides be silently ignored by the pinctrl
> core during boot?
> Since these pin definitions are not referenced by a pinctrl-0 property in
> an active device node, and are not configured as pinctrl hogs in the provider,
> the intended hardware states for these pins will not be programmed.

I don't think this is an issue. These pin configurations won't be ignored by the kernel during boot.
Some pins have input-enable, pull-down, or pull-up settings that differ from the chip defaults on
certain boards, these configurations are added these configurations in the board-level DTS.

> [ ... ]
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > new file mode 100644
> > index 0000000000000..7293df146aa7b
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > @@ -0,0 +1,888 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> [ ... ]
> > + hdmi_pins: hdmi-pins {
> > + pins = "hdmi_scl", "hdmi_sda", "hdmi_cec";
> > + function = "hdmi";
> > + };
> > +
> > + rgmii0_pins: rgmii0-pins {
> > + pins = "rgmii0_clk_125", "rgmii0_txen", "rgmii0_txclk", "rgmii0_txd0",
> > +        "rgmii0_txd1", "rgmii0_txd2", "rgmii0_txd3", "rgmii0_rxclk",
> > +        "rgmii0_rxdv", "rgmii0_rxd0", "rgmii0_rxd1", "rgmii0_rxd2",
> > +        "rgmii0_rxd3", "rgmii0_mdc", "rgmii0_mdio", "rgmii0_intb";
> > + function = "hdmi";
> > + };
> [Severity: High]
> Is the hdmi function correct for this network pin group?
> It appears that rgmii0, rgmii1, and the i2s pin groups that follow all use
> the hdmi function. This will prevent the pin controller from routing these
> pins properly, likely leading to driver probe failures when peripherals
> attempt to request them.

I will fix the function of rgmii0_pins and rgmii1_pins to rgmii, and the function of i2s0_pins,
i2s1_pins, and i2s2_pins to i2s.

> [ ... ]
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700.dtsi b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > index a7ebb11159580..8798c50f7584d 100644
> > --- a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > +++ b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > @@ -349,6 +349,11 @@ gpioD: gpio-port@3 {
> >   };
> >   };
> >  
> > + pinctrl: pinctrl@51600080 {
> > + compatible = "eswin,eic7700-pinctrl";
> > + reg = <0x0 0x51600080 0x0 0x1fff80>;
> > + };
> > +
> [Severity: High]
> Should the existing gpio controller nodes (gpioA, gpioB, gpioC, and gpioD)
> be updated with a gpio-ranges property mapping to this new pinctrl provider?
> Without gpio-ranges, the GPIO subsystem cannot translate GPIO offsets into
> pinctrl pins. The pinctrl core's .gpio_request_enable callback will not be
> invoked when a software component requests a GPIO dynamically, preventing
> the multiplexer from automatically switching the pin to GPIO mode.
> >   clk: clock-controller@51828000 {
> >   compatible = "eswin,eic7700-clock";

I think add gpio-ranges is unnecessary, the driver does not implement
the .gpio_request_enable callback.

Regards,
Yulin Lu