linux-kernel - Re: [PATCH v2 2/2] arm64: dts: rockchip: Enable regulators for Radxa E20C

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <432b6705-4d97-4d47-bbc2-196f9b84da05@kwiboo.se>
Date: Sun, 23 Mar 2025 22:17:06 +0100
From: Jonas Karlman <jonas@...boo.se>
To: Chukun Pan <amadeus@....edu.cn>
Cc: Heiko Stuebner <heiko@...ech.de>, Yao Zi <ziyao@...root.org>,
 Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] arm64: dts: rockchip: Enable regulators for Radxa
 E20C

Hi Chukun,

On 2025-03-18 13:00, Chukun Pan wrote:
> Enable pwm and fixed regulators for Radxa E20C. The pwm regulator is
> used to power the CPU and GPU. Note that the LPDDR4 voltage is 1.1V.
> 
> Signed-off-by: Chukun Pan <amadeus@....edu.cn>
> ---
>  .../boot/dts/rockchip/rk3528-radxa-e20c.dts   | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> index 57a446b5cbd6..159b55aa970d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> @@ -100,6 +100,16 @@ vcc_3v3: regulator-3v3-vcc {
>  		vin-supply = <&vcc5v0_sys>;
>  	};
>  
> +	vcc_ddr: regulator-vcc-ddr {

Since we know the voltage for this regulator please name the node
regulator-1v1-vcc-ddr and place it above the regulator-1v8 to keep node
order by node name (not label name).

> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_ddr";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1100000>;
> +		regulator-max-microvolt = <1100000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
>  	vcc5v0_sys: regulator-5v0-vcc-sys {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc5v0_sys";
> @@ -108,6 +118,56 @@ vcc5v0_sys: regulator-5v0-vcc-sys {
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
>  	};
> +
> +	vdd_0v9: regulator-0v9-vdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vdd_0v9";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <900000>;
> +		regulator-max-microvolt = <900000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};

This node is out of node name order, please place it before the
regulator-1v1-vcc-ddr node.

> +
> +	vdd_arm: regulator-vdd-arm {
> +		compatible = "pwm-regulator";

Suggest we add the pinctrl props here instead:

 		pinctrl-names = "default";
 		pinctrl-0 = <&pwm1m0_pins>;

> +		pwms = <&pwm1 0 5000 1>;

This should use the PWM_POLARITY_INVERTED flag not 1.

Will require include of dt-bindings/pwm/pwm.h.

> +		pwm-supply = <&vcc5v0_sys>;
> +		regulator-name = "vdd_arm";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <746000>;
> +		regulator-max-microvolt = <1201000>;
> +		regulator-settling-time-up-us = <250>;
> +	};
> +
> +	vdd_logic: regulator-vdd-logic {
> +		compatible = "pwm-regulator";

Same here, suggest we add the pinctrl props here instead:

 		pinctrl-names = "default";
 		pinctrl-0 = <&pwm2m0_pins>;

> +		pwms = <&pwm2 0 5000 1>;

Same here, this should use the PWM_POLARITY_INVERTED flag not 1.

> +		pwm-supply = <&vcc5v0_sys>;
> +		regulator-name = "vdd_logic";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <705000>;
> +		regulator-max-microvolt = <1006000>;
> +		regulator-settling-time-up-us = <250>;
> +	};
> +};
> +
> +&cpu0 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu1 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu2 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu3 {
> +	cpu-supply = <&vdd_arm>;
>  };
>  
>  &pinctrl {
> @@ -132,6 +192,18 @@ wan_led_g: wan-led-g {
>  	};
>  };
>  
> +&pwm1 {
> +	pinctrl-0 = <&pwm1m0_pins>;
> +	pinctrl-names = "default";

For consistency please put the pinctrl-names before the pinctrl-X props.
And as stated above, I suggest we move this to the regulator node.

> +	status = "okay";
> +};
> +
> +&pwm2 {
> +	pinctrl-0 = <&pwm2m0_pins>;
> +	pinctrl-names = "default";

Same here.

Regards,
Jonas

> +	status = "okay";
> +};
> +
>  &saradc {
>  	vref-supply = <&vcc_1v8>;
>  	status = "okay";