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

[Stephan Gerhold <stephan@gerhold.net>]

On Sun, Jun 18, 2023 at 06:49:16PM +0200, Jakob Hauser wrote:
> 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.
> 

Sounds good to me, thanks!

Stephan