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

[Jakob Hauser]

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