| Message ID | 1395955764-18103-8-git-send-email-carlo@caione.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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,
diff --git a/arch/arm/boot/dts/x-powers-axp209.dtsi b/arch/arm/boot/dts/x-powers-axp209.dtsi new file mode 100644 index 0000000..b05e54d --- /dev/null +++ b/arch/arm/boot/dts/x-powers-axp209.dtsi @@ -0,0 +1,54 @@ +/* + * x-powers,axp209 common code to be include from inside the axp209 node + * + * Copyright 2014 - Carlo Caione <carlo@caione.org> + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +regulators { + dcdc-freq = <1500>; + + axp_dcdc2: dcdc2 { + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <2275000>; + regulator-always-on; + }; + + axp_dcdc3: dcdc3 { + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <3500000>; + regulator-always-on; + }; + + axp_ldo1: ldo1 { + regulator-min-microvolt = <1300000>; + regulator-max-microvolt = <1300000>; + }; + + axp_ldo2: ldo2 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + regulator-always-on; + }; + + axp_ldo3: ldo3 { + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <3500000>; + }; + + axp_ldo4: ldo4 { + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <3300000>; + }; + + axp_ldo5: ldo5 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + }; +};