On Mon, Nov 27, 2023 at 07:13:37AM +0100, Ahmad Fatoum wrote:
> On 27.11.23 06:58, Ahmad Fatoum wrote:
> > Hello Laurent,
>
> Ah, I see now, that this series was about to be merged. I missed it at first,
> because of the MAINTAINERS entry losing a F:, which I now sent a fix for.
>
> Anyways, should you resend to fix the binding errors, you could address some
> of the nitpicks, but I found nothing critical.
As the series hasn't been merged yet, I'll submit a v3 that addresses
your comments. Thanks for the detailed review.
> > On 25.10.23 18:50, Laurent Pinchart wrote:
> >> + reg_eqos_phy: regulator-eqos-phy {
> >> + compatible = "regulator-fixed";
> >> + regulator-name = "eqos-phy";
> >> + regulator-min-microvolt = <3300000>;
> >> + regulator-max-microvolt = <3300000>;
> >> + gpio = <&gpio2 20 GPIO_ACTIVE_HIGH>;
> >> + enable-active-high;
> >> + regulator-always-on;
> >
> > Apparently, https://lore.kernel.org/all/20230721110345.3925719-1-m.felsch@xxxxxxxxxxxxxx/
> > didn't make it upstream. Perhaps you mentioning that you could use this would help get
> > it unstuck? :)
> >
> >> +&eqos {
> >> + pinctrl-names = "default";
> >> + pinctrl-0 = <&pinctrl_eqos>;
> >> + phy-mode = "rgmii";
> >> + phy-handle = <ðphy0>;
> >> + status = "okay";
> >> +
> >> + mdio {
> >> + compatible = "snps,dwmac-mdio";
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + ethphy0: ethernet-phy@0 {
> >> + compatible = "ethernet-phy-ieee802.3-c22";
> >> + reg = <0>;
> >> + eee-broken-1000t;
> >> + reset-gpios = <&gpio2 11 GPIO_ACTIVE_LOW>;
> >
> > Nitpick: Separate pinctrl entry for PHY GPIOs that's added to the PHY node?
> > Makes it easier to check that all used signals are indeed muxed.
> >
> >> + pmic@25 {
> >> + compatible = "nxp,pca9450c";
> >> + reg = <0x25>;
> >> + pinctrl-names = "default";
> >> + pinctrl-0 = <&pinctrl_pmic>;
> >> + interrupt-parent = <&gpio1>;
> >> + interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> >> +
> >> + regulators {
> >> + BUCK1 {
> >> + regulator-name = "BUCK1";
> >> + regulator-min-microvolt = <600000>;
> >> + regulator-max-microvolt = <2187500>;
> >
> > Nitpick: These may be the limits of what the BUCK can output, but they
> > don't look like a safe operating range for the board. The Linux driver already
> > has ranges hardcoded to cover what's possible by the hardware, so if you specify
> > regulator range here, it should pertain to what the board and SoC are designed
> > to handle.
> >
> >> +/* eMMC */
> >> +&usdhc3 {
> >> + pinctrl-names = "default", "state_100mhz", "state_200mhz";
> >> + pinctrl-0 = <&pinctrl_usdhc3>;
> >> + pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> >> + pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> >> + bus-width = <8>;
> >> + non-removable;
> >
> > no-sd
> > no-sdio
> >
> > may give you a tiny bit of speedup during probe, if you know that there will
> > always be an eMMC here.
> >
> >> + pinctrl_i2c1: i2c1grp {
> >> + fsl,pins = <
> >> + MX8MP_IOMUXC_I2C1_SCL__I2C1_SCL 0x400001c2
> >> + MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA 0x400001c2
> >> + >;
> >> + };
> >> +
> >> + pinctrl_i2c1_gpio: i2c1gpiogrp {
> >> + fsl,pins = <
> >> + MX8MP_IOMUXC_I2C1_SCL__GPIO5_IO14 0x1c2
> >> + MX8MP_IOMUXC_I2C1_SDA__GPIO5_IO15 0x1c2
> >
> > This surprises me. I'd expect that the SION bit needs to be set for GPIO bus recovery.
--
Regards,
Laurent Pinchart
