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

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]