Re: [PATCH v2 2/4] arm64: dts: freescale: Add support for the Variscite DART-MX8M-PLUS SoM

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@pengutronix.de/
> > 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 = <&ethphy0>;
> >> +	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