Re: [PATCH v2 2/3] ARM: dts: imx6ul: Add Variscite VAR-SOM-MX6UL SoM support

["Antonin Godard" <antonin.godard@bootlin.com>]

Hi Shawn,

On Sat Feb 22, 2025 at 3:45 PM CET, Shawn Guo wrote:
[...]
>> +	reg_gpio_dvfs: reg-gpio-dvfs {
>> +		compatible = "regulator-gpio";
>> +		regulator-min-microvolt = <1300000>;
>> +		regulator-max-microvolt = <1400000>;
>> +		regulator-name = "gpio_dvfs";
>> +		regulator-type = "voltage";
>> +		gpios = <&gpio4 13 GPIO_ACTIVE_HIGH>;
>
> Don't we need 'enable-active-high'?

I am not specifying 'enable-gpios' but 'gpios' here so from my understanding of
gpio-regulator.yaml and examples from other device trees I don't think so.

>> +		states = <1300000 0x1 1400000 0x0>;
>> +	};
>> +
>> +	rmii_ref_clk: rmii-ref-clk-grp {
>
> What does "-grp" in node name mean?

A mistake on my side, I will rename the label to "rmii-ref-clk".

>> +		compatible = "fixed-clock";
>> +		clock-output-names = "rmii-ref";
>> +	};
>> +};
>> +
>> +&cpu0 {
>> +	dc-supply = <&reg_gpio_dvfs>;
>> +};
>> +
>> +&clks {
>> +	assigned-clocks = <&clks IMX6UL_CLK_PLL4_AUDIO_DIV>;
>> +	assigned-clock-rates = <786432000>;
>> +};
>> +
>> +&fec1 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_enet1>, <&pinctrl_enet1_gpio>, <&pinctrl_enet1_mdio>;
>> +	phy-mode = "rmii";
>> +	phy-reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>> +	phy-reset-duration = <100>;
>> +	phy-handle = <&ethphy0>;
>> +	status = "okay";
>> +
>> +	mdio {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		ethphy0: ethernet-phy@1 {
>> +			compatible = "ethernet-phy-ieee802.3-c22";
>> +			micrel,rmii-reference-clock-select-25-mhz = <1>;
>> +			micrel,led-mode = <1>;
>> +			clocks = <&rmii_ref_clk>;
>> +			clock-names = "rmii-ref";
>> +			reg = <1>;
>
> We generally sort properties like:
>
>   - compatible
>   - reg
>   - generic ones
>   - vendor prefixed ones
>
> So in this case, we will get:
>
> 	compatible = "ethernet-phy-ieee802.3-c22";
> 	reg = <1>;
> 	clocks = <&rmii_ref_clk>;
> 	clock-names = "rmii-ref";
> 	micrel,rmii-reference-clock-select-25-mhz = <1>;
> 	micrel,led-mode = <1>;

Thanks for clarifying that! I will revise this for the next version.

Antonin

-- 
Antonin Godard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com