[PATCH v5 00/12] arm64: dts: rockchip: Refine dts for Radxa ROCK 5C

[PATCH v5 00/12] arm64: dts: rockchip: Refine dts for Radxa ROCK 5C

[FUKAUMI Naoki]

This patch series includes fixes for difference from schematic and
vendor kernel, minor improvements, cosmetic changes, etc.

FUKAUMI Naoki (12):
  arm64: dts: rockchip: Change function name for blue LED for Radxa ROCK
    5C
  arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C
  arm64: dts: rockchip: Use a longer PWM period for the fan on Radxa
    ROCK 5C
  arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C
  arm64: dts: rockchip: Add properties for pcie2x1l2 for Radxa ROCK 5C
  arm64: dts: rockchip: Remove RTC and related node for Radxa ROCK 5C
  arm64: dts: rockchip: Separate pinctrl for vcc_5v0 for Radxa ROCK 5C
  arm64: dts: rockchip: Add cd-gpios for sdmmc for Radxa ROCK 5C
  arm64: dts: rockchip: Fix pmic dcdc-reg7 regulator-name for Radxa ROCK
    5C
  arm64: dts: rockchip: Fix pmic dcdc-reg10 label for Radxa ROCK 5C
  arm64: dts: rockchip: Fix proparties for pmic regulators for Radxa
    ROCK 5C
  arm64: dts: rockchip: Sort nodes for Radxa ROCK 5C

 .../boot/dts/rockchip/rk3588s-rock-5c.dts     | 62 +++++++------------
 1 file changed, 22 insertions(+), 40 deletions(-)

-- 
2.43.0

[PATCH v5 01/12] arm64: dts: rockchip: Change function name for blue LED for Radxa ROCK 5C

[FUKAUMI Naoki]

Since Radxa ROCK 5C is updated version of Radxa ROCK 5A, everything
should be compatible as much as possible.

Change function for blue LED from HEARTBEAT to STATUS.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5
- Reword commit message
Changes in v4
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 9b14d5383cdc..23e9b447b6f0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -62,7 +62,7 @@ led-0 {
 		led-1 {
 			color = <LED_COLOR_ID_BLUE>;
 			default-state = "on";
-			function = LED_FUNCTION_HEARTBEAT;
+			function = LED_FUNCTION_STATUS;
 			gpios = <&gpio3 RK_PD5 GPIO_ACTIVE_HIGH>;
 			linux,default-trigger = "heartbeat";
 		};
-- 
2.43.0

[PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

Use more common name "pwm-fan" for pwm-fan node. No functinal change.

Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5
- Reword commit message
Changes in v4
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 23e9b447b6f0..630f026d856c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -68,7 +68,7 @@ led-1 {
 		};
 	};
 
-	fan {
+	pwm-fan {
 		compatible = "pwm-fan";
 		#cooling-cells = <2>;
 		cooling-levels = <0 64 128 192 255>;
-- 
2.43.0

[PATCH v5 03/12] arm64: dts: rockchip: Use a longer PWM period for the fan on Radxa ROCK 5C

[FUKAUMI Naoki]

The fan on Radxa ROCK 5C is driven via an AO3416 MOSFET, which has a
total switch-on time of 0,6us and a total switch-off time of 6us [1],
meaning that the current PWM period of just 10us is too short for
fine-grained fan speed control. Increase the PWM period to 60us, so
that the switch-on and switch-off time of the MOSFET fall within a
more reasonable ~10% of the full period, thus making lower PWM duty
cycles meaningful.

[1] https://www.aosmd.com/pdfs/datasheet/AO3416.pdf

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message (Alexey Charkov)
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- reword commit message
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 630f026d856c..85589d1a6d3b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -73,7 +73,7 @@ pwm-fan {
 		#cooling-cells = <2>;
 		cooling-levels = <0 64 128 192 255>;
 		fan-supply = <&vcc_5v0>;
-		pwms = <&pwm3 0 10000 0>;
+		pwms = <&pwm3 0 60000 0>;
 	};
 
 	pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
-- 
2.43.0

[PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

Use consistent name with other regulators. No functional change.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- reword commit message
Changes in v3:
- none
Changes in v2:
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 85589d1a6d3b..61d75ab503b2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -76,13 +76,13 @@ pwm-fan {
 		pwms = <&pwm3 0 60000 0>;
 	};
 
-	pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
+	vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
 		compatible = "regulator-fixed";
 		enable-active-high;
 		gpio = <&gpio0 RK_PC5 GPIO_ACTIVE_HIGH>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&pow_en>;
-		regulator-name = "pcie2x1l2_3v3";
+		regulator-name = "vcc3v3_pcie2x1l2";
 		regulator-min-microvolt = <3300000>;
 		regulator-max-microvolt = <3300000>;
 		vin-supply = <&vcc_sysin>;
@@ -421,7 +421,7 @@ &pcie2x1l2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie20x1_2_perstn_m0>;
 	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
-	vpcie3v3-supply = <&pcie2x1l2_3v3>;
+	vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
 	status = "okay";
 };
 
-- 
2.43.0

[PATCH v5 05/12] arm64: dts: rockchip: Add properties for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

By schematic[1], pcie2x1l2 CLKREQ and WAKE pins are connected.
Describe them in dts.

[1] https://dl.radxa.com/rock5/5c/docs/hw/v1100/radxa_rock_5c_schematic_v1100.pdf

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- reword commit message
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 61d75ab503b2..01537b47f880 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -419,7 +419,9 @@ rgmii_phy1: ethernet-phy@1 {
 
 &pcie2x1l2 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie20x1_2_perstn_m0>;
+	pinctrl-0 = <&pcie20x1_2_perstn_m0>,
+		    <&pcie20x1m0_clkreqn>,
+		    <&pcie20x1m0_waken>;
 	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
 	status = "okay";
-- 
2.43.0

[PATCH v5 06/12] arm64: dts: rockchip: Remove RTC and related node for Radxa ROCK 5C

[FUKAUMI Naoki]

RTC doesn't exist on actual board. Remove it.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- new
---
 .../boot/dts/rockchip/rk3588s-rock-5c.dts     | 23 -------------------
 1 file changed, 23 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 01537b47f880..1ba94396aab8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -350,23 +350,6 @@ regulator-state-mem {
 	};
 };
 
-&i2c5 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&i2c5m2_xfer>;
-	status = "okay";
-
-	rtc@51 {
-		compatible = "haoyu,hym8563";
-		reg = <0x51>;
-		#clock-cells = <0>;
-		clock-output-names = "rtcic_32kout";
-		interrupt-parent = <&gpio0>;
-		interrupts = <RK_PB0 IRQ_TYPE_LEVEL_LOW>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&rtc_int_l>;
-	};
-};
-
 &i2c7 {
 	status = "okay";
 
@@ -451,12 +434,6 @@ pow_en: pow-en {
 		};
 	};
 
-	rtc {
-		rtc_int_l: rtc-int-l {
-			rockchip,pins = <0 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
 	usb {
 		usb_host_pwren_h: usb-host-pwren-h {
 			rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
-- 
2.43.0

[PATCH v5 07/12] arm64: dts: rockchip: Separate pinctrl for vcc_5v0 for Radxa ROCK 5C

[FUKAUMI Naoki]

vcc_5v0 is used in many places, separate vcc_5v0_pwren_h from usb in
pinctrl. No functional change.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- none
Changes in v3:
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 1ba94396aab8..b632093183b2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -446,7 +446,9 @@ usb_otg_pwren_h: usb-otg-pwren-h {
 		usb_wifi_pwr: usb-wifi-pwr {
 			rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
+	};
 
+	vcc-5v0 {
 		vcc_5v0_pwren_h: vcc-5v0-pwren-h {
 			rockchip,pins = <4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
-- 
2.43.0

[PATCH v5 08/12] arm64: dts: rockchip: Add cd-gpios for sdmmc for Radxa ROCK 5C

[FUKAUMI Naoki]

By discussion about cd-gpios and sdmmmc_det pin function[1], it's
better to add cd-gpios for now.

[1] https://lore.kernel.org/linux-rockchip/4920950.GXAFRqVoOG@diego/T/#u

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- none
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index b632093183b2..1da082b18c0e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -480,6 +480,7 @@ &sdmmc {
 	bus-width = <4>;
 	cap-mmc-highspeed;
 	cap-sd-highspeed;
+	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
 	disable-wp;
 	no-sdio;
 	no-mmc;
-- 
2.43.0

[PATCH v5 09/12] arm64: dts: rockchip: Fix pmic dcdc-reg7 regulator-name for Radxa ROCK 5C

[FUKAUMI Naoki]

Fix regulator-name to match with node label. No functinal change.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- none
Changes in v3:
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 1da082b18c0e..0f8048f6511b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -634,7 +634,7 @@ regulator-state-mem {
 			};
 
 			vcc_2v0_pldo_s3: dcdc-reg7 {
-				regulator-name = "vdd_2v0_pldo_s3";
+				regulator-name = "vcc_2v0_pldo_s3";
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-min-microvolt = <2000000>;
-- 
2.43.0

[PATCH v5 10/12] arm64: dts: rockchip: Fix pmic dcdc-reg10 label for Radxa ROCK 5C

[FUKAUMI Naoki]

Fix pmic dcdc-reg10 label to match with schematic[1]. No functional
change.

[1] https://dl.radxa.com/rock5/5c/docs/hw/v1100/radxa_rock_5c_schematic_v1100.pdf

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- split from "arm64: dts: rockchip: fix pmic properties for Radxa ROCK 5C"
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 0f8048f6511b..fbe491e63852 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -670,8 +670,8 @@ regulator-state-mem {
 				};
 			};
 
-			vcc1v8_pmu_ddr_s3: dcdc-reg10 {
-				regulator-name = "vcc1v8_pmu_ddr_s3";
+			vcc_1v8_s3: dcdc-reg10 {
+				regulator-name = "vcc_1v8_s3";
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-min-microvolt = <1800000>;
-- 
2.43.0

[PATCH v5 11/12] arm64: dts: rockchip: Fix proparties for pmic regulators for Radxa ROCK 5C

[FUKAUMI Naoki]

Fix proparties for dcdc-reg7 and nldo-reg3 regulators to match with
vendor kernel[1].

[1] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- none
Changes in v3:
- none
Changes in v2:
- split from "arm64: dts: rockchip: fix pmic properties for Radxa ROCK 5C"
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index fbe491e63852..60eabe22ef02 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -639,7 +639,6 @@ vcc_2v0_pldo_s3: dcdc-reg7 {
 				regulator-boot-on;
 				regulator-min-microvolt = <2000000>;
 				regulator-max-microvolt = <2000000>;
-				regulator-ramp-delay = <12500>;
 
 				regulator-state-mem {
 					regulator-on-in-suspend;
@@ -793,7 +792,8 @@ vdda_0v75_s0: nldo-reg3 {
 				regulator-max-microvolt = <750000>;
 
 				regulator-state-mem {
-					regulator-off-in-suspend;
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <750000>;
 				};
 			};
 
-- 
2.43.0

[PATCH v5 12/12] arm64: dts: rockchip: Sort nodes for Radxa ROCK 5C

[FUKAUMI Naoki]

Sort nodes alphanumerically. No functional change.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v5:
- Reword commit message
Changes in v4:
- reflect other commits, no functional change
Changes in v3:
- none
Changes in v2:
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 60eabe22ef02..a987d256c947 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -861,10 +861,6 @@ &uart2 {
 	status = "okay";
 };
 
-&usbdp_phy0 {
-	status = "okay";
-};
-
 &usb_host0_ehci {
 	status = "okay";
 };
@@ -886,7 +882,7 @@ &usb_host2_xhci {
 	status = "okay";
 };
 
-&vop_mmu {
+&usbdp_phy0 {
 	status = "okay";
 };
 
@@ -894,6 +890,10 @@ &vop {
 	status = "okay";
 };
 
+&vop_mmu {
+	status = "okay";
+};
+
 &vp0 {
 	vp0_out_hdmi0: endpoint@ROCKCHIP_VOP2_EP_HDMI0 {
 		reg = <ROCKCHIP_VOP2_EP_HDMI0>;
-- 
2.43.0

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Dragan Simic]

On 2024-12-16 12:30, FUKAUMI Naoki wrote:
> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
> 
> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>

Looking good to me, as a preparatory patch.  Please, feel free
to include:

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
> Changes in v5
> - Reword commit message
> Changes in v4
> - new
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> index 23e9b447b6f0..630f026d856c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> @@ -68,7 +68,7 @@ led-1 {
>  		};
>  	};
> 
> -	fan {
> +	pwm-fan {
>  		compatible = "pwm-fan";
>  		#cooling-cells = <2>;
>  		cooling-levels = <0 64 128 192 255>;

Re: [PATCH v5 03/12] arm64: dts: rockchip: Use a longer PWM period for the fan on Radxa ROCK 5C

[Dragan Simic]

On 2024-12-16 12:30, FUKAUMI Naoki wrote:
> The fan on Radxa ROCK 5C is driven via an AO3416 MOSFET, which has a
> total switch-on time of 0,6us and a total switch-off time of 6us [1],
> meaning that the current PWM period of just 10us is too short for
> fine-grained fan speed control. Increase the PWM period to 60us, so
> that the switch-on and switch-off time of the MOSFET fall within a
> more reasonable ~10% of the full period, thus making lower PWM duty
> cycles meaningful.
> 
> [1] https://www.aosmd.com/pdfs/datasheet/AO3416.pdf
> 
> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>

Looking good to me.  Please, feel free to include:

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
> Changes in v5:
> - Reword commit message (Alexey Charkov)
> Changes in v4:
> - none
> Changes in v3:
> - none
> Changes in v2:
> - reword commit message
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> index 630f026d856c..85589d1a6d3b 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> @@ -73,7 +73,7 @@ pwm-fan {
>  		#cooling-cells = <2>;
>  		cooling-levels = <0 64 128 192 255>;
>  		fan-supply = <&vcc_5v0>;
> -		pwms = <&pwm3 0 10000 0>;
> +		pwms = <&pwm3 0 60000 0>;
>  	};
> 
>  	pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 16/12/2024 12:30, FUKAUMI Naoki wrote:
> Use more common name "pwm-fan" for pwm-fan node. No functinal change.

No, generic name is fan.

Best regards,
Krzysztof

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 16/12/2024 12:30, FUKAUMI Naoki wrote:
> Use consistent name with other regulators. No functional change.
> 
> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
> ---
> Changes in v5:
> - Reword commit message
> Changes in v4:
> - reword commit message
> Changes in v3:
> - none
> Changes in v2:
> - new
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> index 85589d1a6d3b..61d75ab503b2 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> @@ -76,13 +76,13 @@ pwm-fan {
>  		pwms = <&pwm3 0 60000 0>;
>  	};
>  
> -	pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
> +	vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {

No, neither explained, nor correct. See DTS coding style.

Please use name for all fixed regulators which matches current format
recommendation: 'regulator-[0-9]v[0-9]'

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?h=v6.11-rc1#n46



Best regards,
Krzysztof

Re: [PATCH v5 10/12] arm64: dts: rockchip: Fix pmic dcdc-reg10 label for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 16/12/2024 12:30, FUKAUMI Naoki wrote:
> Fix pmic dcdc-reg10 label to match with schematic[1]. No functional
> change.
> 
> [1] https://dl.radxa.com/rock5/5c/docs/hw/v1100/radxa_rock_5c_schematic_v1100.pdf
> 
> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")

All your commits have the same flaw. You claim there is no functional
change, yet there is a real bug to fix.

All such trivial cleanups are not fixes and should be squashed together.
Best regards,
Krzysztof

Re: [PATCH v5 11/12] arm64: dts: rockchip: Fix proparties for pmic regulators for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 16/12/2024 12:30, FUKAUMI Naoki wrote:
> Fix proparties for dcdc-reg7 and nldo-reg3 regulators to match with



You have so many typos in almost each commit msg. Here both in subject
and commit msg.

Run spell check before you send to the lists. Don't expect the reviewers
to be your computer tools.

Explain what is being fixed here. Why would you prefer to match with
vendor kernel?

> vendor kernel[1].
> 
> [1] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
> 
> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
> ---




Best regards,
Krzysztof

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 16/12/2024 13:43, Dragan Simic wrote:
> On 2024-12-16 12:30, FUKAUMI Naoki wrote:
>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
>>
>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
> 
> Looking good to me, as a preparatory patch.  Please, feel free
> to include:
> 
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>

That's just incorrect. If you really want to review such trivial
patches, perform a full review.

Best regards,
Krzysztof

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/16/24 22:37, Krzysztof Kozlowski wrote:
> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
> 
> No, generic name is fan.

  https://lore.kernel.org/all/71aa84af7a030e66487076e0976c8cad@manjaro.org/

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/16/24 22:38, Krzysztof Kozlowski wrote:
> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>> Use consistent name with other regulators. No functional change.
>>
>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>> ---
>> Changes in v5:
>> - Reword commit message
>> Changes in v4:
>> - reword commit message
>> Changes in v3:
>> - none
>> Changes in v2:
>> - new
>> ---
>>   arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>> index 85589d1a6d3b..61d75ab503b2 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>> @@ -76,13 +76,13 @@ pwm-fan {
>>   		pwms = <&pwm3 0 60000 0>;
>>   	};
>>   
>> -	pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>> +	vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
> 
> No, neither explained, nor correct. See DTS coding style.
> 
> Please use name for all fixed regulators which matches current format
> recommendation: 'regulator-[0-9]v[0-9]'
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?h=v6.11-rc1#n46

Thanks for pointing.
(Please blame other dts too)

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 16/12/2024 14:48, FUKAUMI Naoki wrote:
> On 12/16/24 22:37, Krzysztof Kozlowski wrote:
>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
>>
>> No, generic name is fan.
> 
>   https://lore.kernel.org/all/71aa84af7a030e66487076e0976c8cad@manjaro.org/
> 
And? That's incorrect advice.  There is no such device as "pwm-fan".
There is a "fan" and whether it is pwm or gpio it does not matter.

See DT spec and generic names recommendation.

Best regards,
Krzysztof

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/16/24 22:56, Krzysztof Kozlowski wrote:
> On 16/12/2024 14:48, FUKAUMI Naoki wrote:
>> On 12/16/24 22:37, Krzysztof Kozlowski wrote:
>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
>>>
>>> No, generic name is fan.
>>
>>    https://lore.kernel.org/all/71aa84af7a030e66487076e0976c8cad@manjaro.org/
>>
> And? That's incorrect advice.  There is no such device as "pwm-fan".
> There is a "fan" and whether it is pwm or gpio it does not matter.
> 
> See DT spec and generic names recommendation.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml#n67

Is it wrong?

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 16/12/2024 15:19, FUKAUMI Naoki wrote:
> On 12/16/24 22:56, Krzysztof Kozlowski wrote:
>> On 16/12/2024 14:48, FUKAUMI Naoki wrote:
>>> On 12/16/24 22:37, Krzysztof Kozlowski wrote:
>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
>>>>
>>>> No, generic name is fan.
>>>
>>>    https://lore.kernel.org/all/71aa84af7a030e66487076e0976c8cad@manjaro.org/
>>>
>> And? That's incorrect advice.  There is no such device as "pwm-fan".
>> There is a "fan" and whether it is pwm or gpio it does not matter.
>>
>> See DT spec and generic names recommendation.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml#n67
> 
> Is it wrong?
Yes.

Best regards,
Krzysztof

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/16/24 23:27, Krzysztof Kozlowski wrote:
> On 16/12/2024 15:19, FUKAUMI Naoki wrote:
>> On 12/16/24 22:56, Krzysztof Kozlowski wrote:
>>> On 16/12/2024 14:48, FUKAUMI Naoki wrote:
>>>> On 12/16/24 22:37, Krzysztof Kozlowski wrote:
>>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>>>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
>>>>>
>>>>> No, generic name is fan.
>>>>
>>>>     https://lore.kernel.org/all/71aa84af7a030e66487076e0976c8cad@manjaro.org/
>>>>
>>> And? That's incorrect advice.  There is no such device as "pwm-fan".
>>> There is a "fan" and whether it is pwm or gpio it does not matter.
>>>
>>> See DT spec and generic names recommendation.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml#n67
>>
>> Is it wrong?
> Yes.

Okay, please fix it.

Feel free to use:

Reported-by: FUKAUMI Naoki <naoki@radxa.com>

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Dragan Simic]

Hello Krzysztof,

On 2024-12-16 14:42, Krzysztof Kozlowski wrote:
> On 16/12/2024 13:43, Dragan Simic wrote:
>> On 2024-12-16 12:30, FUKAUMI Naoki wrote:
>>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
>>> 
>>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>> 
>> Looking good to me, as a preparatory patch.  Please, feel free
>> to include:
>> 
>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> 
> That's just incorrect. If you really want to review such trivial
> patches, perform a full review.

Well, I don't see what's wrong with reviewing two fan-related patches
that go together?  Moreover, in this case it's about using a generic
name, which you prefer, or using a more common name, which I prefer.
However, as a maintainer, your preference matters more.

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Dragan Simic]

On 2024-12-16 15:38, FUKAUMI Naoki wrote:
> On 12/16/24 23:27, Krzysztof Kozlowski wrote:
>> On 16/12/2024 15:19, FUKAUMI Naoki wrote:
>>> On 12/16/24 22:56, Krzysztof Kozlowski wrote:
>>>> On 16/12/2024 14:48, FUKAUMI Naoki wrote:
>>>>> On 12/16/24 22:37, Krzysztof Kozlowski wrote:
>>>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>>>>> Use more common name "pwm-fan" for pwm-fan node. No functinal 
>>>>>>> change.
>>>>>> 
>>>>>> No, generic name is fan.
>>>>> 
>>>>>     
>>>>> https://lore.kernel.org/all/71aa84af7a030e66487076e0976c8cad@manjaro.org/
>>>>> 
>>>> And? That's incorrect advice.  There is no such device as "pwm-fan".
>>>> There is a "fan" and whether it is pwm or gpio it does not matter.
>>>> 
>>>> See DT spec and generic names recommendation.
>>> 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml#n67
>>> 
>>> Is it wrong?
>> Yes.
> 
> Okay, please fix it.

Good point.

> Feel free to use:
> 
> Reported-by: FUKAUMI Naoki <naoki@radxa.com>

Re: [PATCH v5 02/12] arm64: dts: rockchip: Change node name for pwm-fan for Radxa ROCK 5C

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Mon Dec 16, 2024 at 3:38 PM CET, FUKAUMI Naoki wrote:
> On 12/16/24 23:27, Krzysztof Kozlowski wrote:
> > On 16/12/2024 15:19, FUKAUMI Naoki wrote:
> >> On 12/16/24 22:56, Krzysztof Kozlowski wrote:
> >>> On 16/12/2024 14:48, FUKAUMI Naoki wrote:
> >>>> On 12/16/24 22:37, Krzysztof Kozlowski wrote:
> >>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
> >>>>>> Use more common name "pwm-fan" for pwm-fan node. No functinal change.
> >>>>>
> >>>>> No, generic name is fan.
> >>>>
> >>>>     https://lore.kernel.org/all/71aa84af7a030e66487076e0976c8cad@manjaro.org/
> >>>>
> >>> And? That's incorrect advice.  There is no such device as "pwm-fan".
> >>> There is a "fan" and whether it is pwm or gpio it does not matter.
> >>>
> >>> See DT spec and generic names recommendation.
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml#n67
> >>
> >> Is it wrong?
> > Yes.

There's an(other) issue with the binding:
line 91 references `&fan0` while it isn't defined (in the binding
example)

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 11/12] arm64: dts: rockchip: Fix proparties for pmic regulators for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/16/24 22:41, Krzysztof Kozlowski wrote:
> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>> Fix proparties for dcdc-reg7 and nldo-reg3 regulators to match with
> 
> 
> 
> You have so many typos in almost each commit msg. Here both in subject
> and commit msg.
> 
> Run spell check before you send to the lists. Don't expect the reviewers
> to be your computer tools.

Sorry for my poor English. I'll check it next time.

> Explain what is being fixed here. Why would you prefer to match with
> vendor kernel?

Not my preference, but the only reference.

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

>> vendor kernel[1].
>>
>> [1] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>
>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>> ---
> 
> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v5 10/12] arm64: dts: rockchip: Fix pmic dcdc-reg10 label for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/16/24 22:40, Krzysztof Kozlowski wrote:
> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>> Fix pmic dcdc-reg10 label to match with schematic[1]. No functional
>> change.
>>
>> [1] https://dl.radxa.com/rock5/5c/docs/hw/v1100/radxa_rock_5c_schematic_v1100.pdf
>>
>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
> 
> All your commits have the same flaw. You claim there is no functional
> change, yet there is a real bug to fix.
> 
> All such trivial cleanups are not fixes and should be squashed together.

I see, I'll do "squashed together" for trivial/cosmetic/no functional 
changes without "Fixes" tag.

Thanks for pointing.

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/16/24 22:38, Krzysztof Kozlowski wrote:
> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>> Use consistent name with other regulators. No functional change.
>>
>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>> ---
>> Changes in v5:
>> - Reword commit message
>> Changes in v4:
>> - reword commit message
>> Changes in v3:
>> - none
>> Changes in v2:
>> - new
>> ---
>>   arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>> index 85589d1a6d3b..61d75ab503b2 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>> @@ -76,13 +76,13 @@ pwm-fan {
>>   		pwms = <&pwm3 0 60000 0>;
>>   	};
>>   
>> -	pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>> +	vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
> 
> No, neither explained, nor correct. See DTS coding style.
> 
> Please use name for all fixed regulators which matches current format
> recommendation: 'regulator-[0-9]v[0-9]'
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?h=v6.11-rc1#n46

'regulator-[0-9]v[0-9]' is preferred, but 'regulator-[0-9a-z-]+' is also 
permitted, right?

i.e. regulator-vcc3v3_pcie2x1l2 should be regulator-vcc3v3-pcie2x1l2


Or, should we revert below patch and use 'regulator-[0-9]v[0-9]'?

  https://lore.kernel.org/all/0ae40493-93e9-40cd-9ca9-990ae064f21a@gmail.com/

Is 'regulator-0v0' valid?
Is 'regulator-12v0' invalid?

How should we handle multiple 1v8/3v3/5v0 regulators?

Are examples in fixed-regulator.yaml valid?
'regulator-name' should be '[0-9]v[0-9]' even if there is same name?

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 11/12] arm64: dts: rockchip: Fix proparties for pmic regulators for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 17/12/2024 01:43, FUKAUMI Naoki wrote:
> On 12/16/24 22:41, Krzysztof Kozlowski wrote:
>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>> Fix proparties for dcdc-reg7 and nldo-reg3 regulators to match with
>>
>>
>>
>> You have so many typos in almost each commit msg. Here both in subject
>> and commit msg.
>>
>> Run spell check before you send to the lists. Don't expect the reviewers
>> to be your computer tools.
> 
> Sorry for my poor English. I'll check it next time.

I am not pointing out English. I am pointing out not using standard
tool, spellcheck, to fix issues in effortless way.

> 
>> Explain what is being fixed here. Why would you prefer to match with
>> vendor kernel?
> 
> Not my preference, but the only reference.

Then why the reference is correct? Provide some arguments why this
should be changed to the "reference" and not the "reference" fixed.

Best regards,
Krzysztof

Re: [PATCH v5 11/12] arm64: dts: rockchip: Fix proparties for pmic regulators for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/17/24 15:24, Krzysztof Kozlowski wrote:
> On 17/12/2024 01:43, FUKAUMI Naoki wrote:
>> On 12/16/24 22:41, Krzysztof Kozlowski wrote:
>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>> Fix proparties for dcdc-reg7 and nldo-reg3 regulators to match with
>>>
>>>
>>>
>>> You have so many typos in almost each commit msg. Here both in subject
>>> and commit msg.
>>>
>>> Run spell check before you send to the lists. Don't expect the reviewers
>>> to be your computer tools.
>>
>> Sorry for my poor English. I'll check it next time.
> 
> I am not pointing out English. I am pointing out not using standard
> tool, spellcheck, to fix issues in effortless way.
> 
>>
>>> Explain what is being fixed here. Why would you prefer to match with
>>> vendor kernel?
>>
>> Not my preference, but the only reference.
> 
> Then why the reference is correct? Provide some arguments why this
> should be changed to the "reference" and not the "reference" fixed.

Are you serious?

Why do you think the current code is correct? It's written by me, typo 
and copy-pasto generator. Need more argument?

Best regards,

--
FUKAUMI Naoki
Typo and Copy-Pasto Generator

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

Hi Krzysztof,

Could you please reply to this email?
(Not for me, but for everyone)

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

On 12/17/24 10:11, FUKAUMI Naoki wrote:
> On 12/16/24 22:38, Krzysztof Kozlowski wrote:
>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>> Use consistent name with other regulators. No functional change.
>>>
>>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>>> ---
>>> Changes in v5:
>>> - Reword commit message
>>> Changes in v4:
>>> - reword commit message
>>> Changes in v3:
>>> - none
>>> Changes in v2:
>>> - new
>>> ---
>>>   arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/ 
>>> arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>> index 85589d1a6d3b..61d75ab503b2 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>> @@ -76,13 +76,13 @@ pwm-fan {
>>>           pwms = <&pwm3 0 60000 0>;
>>>       };
>>> -    pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>>> +    vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
>>
>> No, neither explained, nor correct. See DTS coding style.
>>
>> Please use name for all fixed regulators which matches current format
>> recommendation: 'regulator-[0-9]v[0-9]'
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ 
>> tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml? 
>> h=v6.11-rc1#n46
> 
> 'regulator-[0-9]v[0-9]' is preferred, but 'regulator-[0-9a-z-]+' is also 
> permitted, right?
> 
> i.e. regulator-vcc3v3_pcie2x1l2 should be regulator-vcc3v3-pcie2x1l2
> 
> 
> Or, should we revert below patch and use 'regulator-[0-9]v[0-9]'?
> 
>   https://lore.kernel.org/ 
> all/0ae40493-93e9-40cd-9ca9-990ae064f21a@gmail.com/
> 
> Is 'regulator-0v0' valid?
> Is 'regulator-12v0' invalid?
> 
> How should we handle multiple 1v8/3v3/5v0 regulators?
> 
> Are examples in fixed-regulator.yaml valid?
> 'regulator-name' should be '[0-9]v[0-9]' even if there is same name?
> 
> Best regards,
> 
> -- 
> FUKAUMI Naoki
> Radxa Computer (Shenzhen) Co., Ltd.
> 
>> Best regards,
>> Krzysztof
>>
> 

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 20/12/2024 07:51, FUKAUMI Naoki wrote:
> Hi Krzysztof,
> 
> Could you please reply to this email?
> (Not for me, but for everyone)

You have me how much time... 3 days to reply?

> 
> Best regards,
> 
> --
> FUKAUMI Naoki
> Radxa Computer (Shenzhen) Co., Ltd.
> 
> On 12/17/24 10:11, FUKAUMI Naoki wrote:
>> On 12/16/24 22:38, Krzysztof Kozlowski wrote:
>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>> Use consistent name with other regulators. No functional change.
>>>>
>>>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>>>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>>>> ---
>>>> Changes in v5:
>>>> - Reword commit message
>>>> Changes in v4:
>>>> - reword commit message
>>>> Changes in v3:
>>>> - none
>>>> Changes in v2:
>>>> - new
>>>> ---
>>>>   arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/ 
>>>> arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>> index 85589d1a6d3b..61d75ab503b2 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>> @@ -76,13 +76,13 @@ pwm-fan {
>>>>           pwms = <&pwm3 0 60000 0>;
>>>>       };
>>>> -    pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>>>> +    vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
>>>
>>> No, neither explained, nor correct. See DTS coding style.
>>>
>>> Please use name for all fixed regulators which matches current format
>>> recommendation: 'regulator-[0-9]v[0-9]'
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ 
>>> tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml? 
>>> h=v6.11-rc1#n46
>>
>> 'regulator-[0-9]v[0-9]' is preferred, but 'regulator-[0-9a-z-]+' is also 
>> permitted, right?
>>
>> i.e. regulator-vcc3v3_pcie2x1l2 should be regulator-vcc3v3-pcie2x1l2
>>
>>
>> Or, should we revert below patch and use 'regulator-[0-9]v[0-9]'?
>>
>>   https://lore.kernel.org/ 
>> all/0ae40493-93e9-40cd-9ca9-990ae064f21a@gmail.com/
>>
>> Is 'regulator-0v0' valid?

Why would it be valid? Can you have regulator with 0 volts?

>> Is 'regulator-12v0' invalid?

Read the binding. I gave you very specific link.

>>
>> How should we handle multiple 1v8/3v3/5v0 regulators?

Just add suffix. But usually more than one suffix, vcc+3v3+pcie_2x1l2,
means you created a very specific name.



Best regards,
Krzysztof

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

On 12/22/24 05:15, Krzysztof Kozlowski wrote:
> On 20/12/2024 07:51, FUKAUMI Naoki wrote:
>> Hi Krzysztof,
>>
>> Could you please reply to this email?
>> (Not for me, but for everyone)
> 
> You have me how much time... 3 days to reply?

sorry.

>> Best regards,
>>
>> --
>> FUKAUMI Naoki
>> Radxa Computer (Shenzhen) Co., Ltd.
>>
>> On 12/17/24 10:11, FUKAUMI Naoki wrote:
>>> On 12/16/24 22:38, Krzysztof Kozlowski wrote:
>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>>> Use consistent name with other regulators. No functional change.
>>>>>
>>>>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>>>>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>>>>> ---
>>>>> Changes in v5:
>>>>> - Reword commit message
>>>>> Changes in v4:
>>>>> - reword commit message
>>>>> Changes in v3:
>>>>> - none
>>>>> Changes in v2:
>>>>> - new
>>>>> ---
>>>>>    arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/
>>>>> arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>> index 85589d1a6d3b..61d75ab503b2 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>> @@ -76,13 +76,13 @@ pwm-fan {
>>>>>            pwms = <&pwm3 0 60000 0>;
>>>>>        };
>>>>> -    pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>>>>> +    vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
>>>>
>>>> No, neither explained, nor correct. See DTS coding style.
>>>>
>>>> Please use name for all fixed regulators which matches current format
>>>> recommendation: 'regulator-[0-9]v[0-9]'
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>>> tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?
>>>> h=v6.11-rc1#n46
>>>
>>> 'regulator-[0-9]v[0-9]' is preferred, but 'regulator-[0-9a-z-]+' is also
>>> permitted, right?
>>>
>>> i.e. regulator-vcc3v3_pcie2x1l2 should be regulator-vcc3v3-pcie2x1l2
>>>
>>>
>>> Or, should we revert below patch and use 'regulator-[0-9]v[0-9]'?
>>>
>>>    https://lore.kernel.org/
>>> all/0ae40493-93e9-40cd-9ca9-990ae064f21a@gmail.com/
>>>
>>> Is 'regulator-0v0' valid?
> 
> Why would it be valid? Can you have regulator with 0 volts?

There may be a .dtsi that is shared across multiple .dts, so some 
regulators may not be able to set a specific voltage in the .dtsi.

How should I describe it?

>>> Is 'regulator-12v0' invalid?
> 
> Read the binding. I gave you very specific link.

46|       - description: Preferred name is 'regulator-[0-9]v[0-9]'
47|         pattern: '^regulator(-[0-9]+v[0-9]+|-[0-9a-z-]+)?$'

The "description" and "pattern" don't match. What you provided is a link 
to the "description".

>>> How should we handle multiple 1v8/3v3/5v0 regulators?
> 
> Just add suffix. But usually more than one suffix, vcc+3v3+pcie_2x1l2,
> means you created a very specific name.

So shouldn't we refer to the schematic?

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Best regards,
> Krzysztof
> 

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[Krzysztof Kozlowski]

On 22/12/2024 04:18, FUKAUMI Naoki wrote:
> On 12/22/24 05:15, Krzysztof Kozlowski wrote:
>> On 20/12/2024 07:51, FUKAUMI Naoki wrote:
>>> Hi Krzysztof,
>>>
>>> Could you please reply to this email?
>>> (Not for me, but for everyone)
>>
>> You have me how much time... 3 days to reply?
> 
> sorry.
> 
>>> Best regards,
>>>
>>> --
>>> FUKAUMI Naoki
>>> Radxa Computer (Shenzhen) Co., Ltd.
>>>
>>> On 12/17/24 10:11, FUKAUMI Naoki wrote:
>>>> On 12/16/24 22:38, Krzysztof Kozlowski wrote:
>>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>>>> Use consistent name with other regulators. No functional change.
>>>>>>
>>>>>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>>>>>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>>>>>> ---
>>>>>> Changes in v5:
>>>>>> - Reword commit message
>>>>>> Changes in v4:
>>>>>> - reword commit message
>>>>>> Changes in v3:
>>>>>> - none
>>>>>> Changes in v2:
>>>>>> - new
>>>>>> ---
>>>>>>    arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/
>>>>>> arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>> index 85589d1a6d3b..61d75ab503b2 100644
>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>> @@ -76,13 +76,13 @@ pwm-fan {
>>>>>>            pwms = <&pwm3 0 60000 0>;
>>>>>>        };
>>>>>> -    pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>>>>>> +    vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
>>>>>
>>>>> No, neither explained, nor correct. See DTS coding style.
>>>>>
>>>>> Please use name for all fixed regulators which matches current format
>>>>> recommendation: 'regulator-[0-9]v[0-9]'
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>>>> tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?
>>>>> h=v6.11-rc1#n46
>>>>
>>>> 'regulator-[0-9]v[0-9]' is preferred, but 'regulator-[0-9a-z-]+' is also
>>>> permitted, right?
>>>>
>>>> i.e. regulator-vcc3v3_pcie2x1l2 should be regulator-vcc3v3-pcie2x1l2
>>>>
>>>>
>>>> Or, should we revert below patch and use 'regulator-[0-9]v[0-9]'?
>>>>
>>>>    https://lore.kernel.org/
>>>> all/0ae40493-93e9-40cd-9ca9-990ae064f21a@gmail.com/
>>>>
>>>> Is 'regulator-0v0' valid?
>>
>> Why would it be valid? Can you have regulator with 0 volts?
> 
> There may be a .dtsi that is shared across multiple .dts, so some 
> regulators may not be able to set a specific voltage in the .dtsi.
> 
> How should I describe it?

How fixed voltage regulator can have non-specific voltage? Sorry, that's
impossible. Shared DTSI with a regulator means that regulator is shared,
so it cannot be flexible.


> 
>>>> Is 'regulator-12v0' invalid?
>>
>> Read the binding. I gave you very specific link.
> 
> 46|       - description: Preferred name is 'regulator-[0-9]v[0-9]'
> 47|         pattern: '^regulator(-[0-9]+v[0-9]+|-[0-9a-z-]+)?$'
> 
> The "description" and "pattern" don't match. What you provided is a link 
> to the "description".

It's pretty obvious still...

> 
>>>> How should we handle multiple 1v8/3v3/5v0 regulators?
>>
>> Just add suffix. But usually more than one suffix, vcc+3v3+pcie_2x1l2,
>> means you created a very specific name.
> 
> So shouldn't we refer to the schematic?

So that's the argument you define 10 fixed, uncontrollable regulators in
DTS? What is the benefit of that exactly? Just unnecessary bloat in DTS,
in kernel memory and for probe time?


Best regards,
Krzysztof

Re: [PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[Dragan Simic]

Hello Krzysztof,

On 2024-12-22 07:27, Krzysztof Kozlowski wrote:
> On 22/12/2024 04:18, FUKAUMI Naoki wrote:
>> On 12/22/24 05:15, Krzysztof Kozlowski wrote:
>>> On 20/12/2024 07:51, FUKAUMI Naoki wrote:
>>>> On 12/17/24 10:11, FUKAUMI Naoki wrote:
>>>>> On 12/16/24 22:38, Krzysztof Kozlowski wrote:
>>>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>>>>> Use consistent name with other regulators. No functional change.
>>>>>>> 
>>>>>>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>>>>>>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>>>>>>> ---
>>>>>>> Changes in v5:
>>>>>>> - Reword commit message
>>>>>>> Changes in v4:
>>>>>>> - reword commit message
>>>>>>> Changes in v3:
>>>>>>> - none
>>>>>>> Changes in v2:
>>>>>>> - new
>>>>>>> ---
>>>>>>>    arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts 
>>>>>>> b/arch/
>>>>>>> arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>>> index 85589d1a6d3b..61d75ab503b2 100644
>>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>>> @@ -76,13 +76,13 @@ pwm-fan {
>>>>>>>            pwms = <&pwm3 0 60000 0>;
>>>>>>>        };
>>>>>>> -    pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>>>>>>> +    vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
>>>>>> 
>>>>>> No, neither explained, nor correct. See DTS coding style.
>>>>>> 
>>>>>> Please use name for all fixed regulators which matches current 
>>>>>> format
>>>>>> recommendation: 'regulator-[0-9]v[0-9]'
>>>>>> 
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>>>>> tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?
>>>>>> h=v6.11-rc1#n46
>>>>> 
>>>>> 'regulator-[0-9]v[0-9]' is preferred, but 'regulator-[0-9a-z-]+' is 
>>>>> also
>>>>> permitted, right?
>>>>> 
>>>>> i.e. regulator-vcc3v3_pcie2x1l2 should be 
>>>>> regulator-vcc3v3-pcie2x1l2
>>>>> 
>>>>> 
>>>>> Or, should we revert below patch and use 'regulator-[0-9]v[0-9]'?
>>>>> 
>>>>>    https://lore.kernel.org/
>>>>> all/0ae40493-93e9-40cd-9ca9-990ae064f21a@gmail.com/
>>>>> 
>>>>> Is 'regulator-0v0' valid?
>>> 
>>> Why would it be valid? Can you have regulator with 0 volts?
>> 
>> There may be a .dtsi that is shared across multiple .dts, so some
>> regulators may not be able to set a specific voltage in the .dtsi.
>> 
>> How should I describe it?
> 
> How fixed voltage regulator can have non-specific voltage? Sorry, 
> that's
> impossible. Shared DTSI with a regulator means that regulator is 
> shared,
> so it cannot be flexible.
> 
>>>>> Is 'regulator-12v0' invalid?
>>> 
>>> Read the binding. I gave you very specific link.
>> 
>> 46|       - description: Preferred name is 'regulator-[0-9]v[0-9]'
>> 47|         pattern: '^regulator(-[0-9]+v[0-9]+|-[0-9a-z-]+)?$'
>> 
>> The "description" and "pattern" don't match. What you provided is a 
>> link
>> to the "description".
> 
> It's pretty obvious still...
> 
>>>>> How should we handle multiple 1v8/3v3/5v0 regulators?
>>> 
>>> Just add suffix. But usually more than one suffix, 
>>> vcc+3v3+pcie_2x1l2,
>>> means you created a very specific name.
>> 
>> So shouldn't we refer to the schematic?
> 
> So that's the argument you define 10 fixed, uncontrollable regulators 
> in
> DTS? What is the benefit of that exactly? Just unnecessary bloat in 
> DTS,
> in kernel memory and for probe time?

I'm quite surprised to hear you suggesting that some of the hardware
components should be omitted from a board dts(i) file.  Regardless of
some of the hardware components being fixed or adjustable, they should
all be described, for the sake of transcribing the board schematic into
the board dts(i) files accurately and completely.  Furthermore, some
fixed components may actually be needed in the board dts(i) files; for
example, a fixed regulator may be turned on at boot time, or turned on
and off at runtime.

Technically, we could omit the fixed regulators from the board dts(i)
files, _only_ if they aren't powered on and off at boot time or at
runtime, of course, but many more things could technically be omitted
as well from the dts(i) files if we'd follow that rule, which would
result in a huge per-board inconsistency, leading to a mess.