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

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] [day] [month] [year] [list]

Message-ID: <ZI9WjmWhkUh_bdz6@gerhold.net>
Date:   Sun, 18 Jun 2023 21:11:29 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Jakob Hauser <jahau@...ketmail.com>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Sebastian Reichel <sre@...nel.org>, Lee Jones <lee@...nel.org>,
        Raymond Hackley <raymondhackley@...tonmail.com>,
        Henrik Grimler <henrik@...mler.se>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add
 RT5033 PMIC with charger

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