Re: [PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Stephan,

On 17.06.23 16:15, Stephan Gerhold wrote:
On Sat, Jun 17, 2023 at 02:29:34AM +0200, Jakob Hauser wrote:

...

+		regulators {
+			safe_ldo_reg: SAFE_LDO {
+				regulator-name = "SAFE_LDO";
+				regulator-min-microvolt = <4900000>;
+				regulator-max-microvolt = <4900000>;
+				regulator-always-on;
+			};
+			ldo_reg: LDO {
+				regulator-name = "LDO";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+			};
+			buck_reg: BUCK {
+				regulator-name = "BUCK";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1200000>;
+			};

The "regulator-name"s here don't really seem useful, since they're just
the same as the ones already declared in the driver. Can you drop them?
Alternatively you could assign more useful board-specific names, such as
the CAM_SENSOR_A2.8V that was used downstream.

Also, I think it would be slightly clearer to prefix the regulator
labels (safe_ldo_reg, ldo_reg etc) with rt5033_. Perhaps
"rt5033_ldo_reg" or "rt5033_reg_ldo"?

...
About the "regulator-name"s I wasn't really aware. I don't have a strong opinion on this.

With the downstream names, it would look like this:

regulators {
	rt5033_reg_safe_ldo: SAFE_LDO {
		regulator-name = "RT5033SafeLDO";
		regulator-min-microvolt = <4900000>;
		regulator-max-microvolt = <4900000>;
		regulator-always-on;
	};
	rt5033_reg_ldo: LDO {
		regulator-name = "CAM_SENSOR_A2.8V";
		regulator-min-microvolt = <2800000>;
		regulator-max-microvolt = <2800000>;
	};
	rt5033_reg_buck: BUCK {
		regulator-name = "CAM_SENSOR_CORE_1.25V";
		regulator-min-microvolt = <1200000>;
		regulator-max-microvolt = <1200000>;
	};

Dropping them would look like this:

regulators {
	rt5033_reg_safe_ldo: SAFE_LDO {
		regulator-min-microvolt = <4900000>;
		regulator-max-microvolt = <4900000>;
		regulator-always-on;
	};
	rt5033_reg_ldo: LDO {
		regulator-min-microvolt = <2800000>;
		regulator-max-microvolt = <2800000>;
	};
	rt5033_reg_buck: BUCK {
		regulator-min-microvolt = <1200000>;
		regulator-max-microvolt = <1200000>;
	};

I would rather drop them. The first name "RT5033SafeLDO" doesn't add much information. The other two I'm not fully sure if they provide the cam sensor only or if there might be other users as well. Also it add an additional set of names. When dropping them, the generic names SAFE_LDO, LDO and BUCK are taken from the rt5033-regulator driver.

Unfortunately, I added the example in the dt-bindings with the generic names. So this question might come up again when someone else adds rt5033-regulators to another device.

For the phandle labels I'd go for rt5033_reg_..., I already changed them in the examples above.

Kind regards,
Jakob



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux