Re: [PATCH v3 07/10] ARM: sunxi: dt: Add x-powers-axp209.dtsi file

[Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>]

On Fri, Mar 28, 2014 at 01:34:38PM +0000, Mark Brown wrote:
> On Thu, Mar 27, 2014 at 10:29:21PM +0100, Carlo Caione wrote:
> > This dtsi describes the axp209 PMIC, and is to be included from inside
> > the i2c controller node to which the axp209 is connected.
> 
> >  arch/arm/boot/dts/x-powers-axp209.dtsi | 54 ++++++++++++++++++++++++++++++++++
> 
> Something is wrong here.  Either the changelog is inaccurate or this is
> broken, either way this should be cleaned up because this looks like
> very bad practice.  If this is a common include file for boards derived
> from some reference design then the changelog is misleading and the DT
> should be clearer about the board tie ins.  Otherwise the .dtsi is
> defining what should be board specific parameters.
> 
> The fact that the DT names everything after the regulator on the PMIC
> and not after the supply names on the board is especially suspicious and
> glancing at a couple of the regulators it looks like the constraints
> here are the maximum ranges the PMIC supports rather than anything to do
> with any board.
> 
> Please take a step back and think about what the regulator constraints
> are intended to do - they're about matching the regulator capabilities
> to the board since it is rare for boards to be able to do everything the
> regulator can support.  Duplicating the basic device capabilities into
> the DT would be a pointless waste of time.
> 
> > +	axp_dcdc2: dcdc2 {
> > +		regulator-min-microvolt = <700000>;
> > +		regulator-max-microvolt = <2275000>;
> > +		regulator-always-on;
> > +	};
> 
> What is this configuration actually for - what guarantee do we have that
> the above is safe for a given board using this regulator?  
> 
> In general I'd expect to see anything specifying variability for a
> regulator to also include the relevant consumer node.

Ok, you are completely right. These values in the DTSI are wrongly the
maximum and minimum ranges for the PMIC. Even worst I didn't specify the
board specific parameters for the regulators.

Thank you for reviewing and noticing it,

-- 
Carlo Caione