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.
Cheers,
Ahmad
>
> 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.
>
> Cheers,
> Ahmad
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
