Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 18 Jun 2023 21:11:29 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Jakob Hauser <jahau@rocketmail.com>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Sebastian Reichel <sre@kernel.org>, Lee Jones <lee@kernel.org>,
        Raymond Hackley <raymondhackley@protonmail.com>,
        Henrik Grimler <henrik@grimler.se>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        phone-devel@vger.kernel.org, ~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add
 RT5033 PMIC with charger
Message-ID: <ZI9WjmWhkUh_bdz6@gerhold.net>
References: <20230617002934.39408-1-jahau.ref@rocketmail.com>
 <20230617002934.39408-1-jahau@rocketmail.com>
 <ZI2_565RFDtR3Sa-@gerhold.net>
 <ab3e475f-27ae-e718-60bd-cb22f5070942@rocketmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ab3e475f-27ae-e718-60bd-cb22f5070942@rocketmail.com>
Content-Transfer-Encoding: 7bit
Precedence: bulk

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