Re: [PATCH 7/8] arm64: dts: qcom: msm8916: Define regulator constraints next to usage

[Stephan Gerhold <stephan@gerhold.net>]

On Fri, May 26, 2023 at 01:35:06AM +0200, Konrad Dybcio wrote:
> On 17.05.2023 20:48, Stephan Gerhold wrote:
> > Right now each MSM8916 device has a huge block of regulator constraints
> > with allowed voltages for each regulator. For lack of better
> > documentation these voltages are often copied as-is from the vendor
> > device tree, without much extra thought.
> > 
> > Unfortunately, the voltages in the vendor device trees are often
> > misleading or even wrong, e.g. because:
> > 
> >  - There is a large voltage range allowed and the actual voltage is
> >    only set somewhere hidden in some messy vendor driver. This is often
> >    the case for pm8916_{l14,l15,l16} because they have a broad range of
> >    1.8-3.3V by default.
> > 
> >  - The voltage is actually wrong but thanks to the voltage constraints
> >    in the RPM firmware it still ends up applying the correct voltage.
> > 
> > To have proper regulator constraints it is important to review them in
> > context of the usage. The current setup in the MSM8916 device trees
> > makes this quite hard because each device duplicates the standard
> > voltages for components of the SoC and mixes those with minor
> > device-specific additions and dummy voltages for completely unused
> > regulators.
> > 
> > The actual usage of the regulators for the SoC components is in
> > msm8916-pm8916.dtsi, so it can and should also define the related
> > voltage constraints. These are not board-specific but defined in the
> > APQ8016E/PM8916 Device Specification. The board DT can then focus on
> > describing the actual board-specific regulators, which makes it much
> > easier to review and spot potential mistakes there.
> > 
> > Note that this commit does not make any functional change. All used
> > regulators still have the same regulator constraints as before. Unused
> > regulators do not have regulator constraints anymore because most of
> > these were too broad or even entirely wrong. They should be added back
> > with proper voltage constraints when there is an actual usage.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> I'm a bit torn between saying "this is very nice already" and "we should
> probably override each regulator individually" like so:
> 
> &pm8916_l17 {
> 	[...]
> }
> 
> to minimize mistakes..
> 
> Not sure what to make of it, I see Bjorn already applied this, so I guess
> I'm just leaving some potential ideas for the future here.
> 

Sorry, could you elaborate a bit on what changes you would make exactly?

The way it works in this patch is that regulators that are used by the
SoC are defined in msm8916-pm8916.dtsi. All other (board-specific)
regulators must be defined together with proper voltages in the board DT.

What kind of mistake are you thinking of?

Thanks,
Stephan