diff mbox

[v3,1/4] arm/dts: regulator: Add tps65910 device tree data

Message ID 1345547850-29761-2-git-send-email-anilkumar@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

AnilKumar, Chimata Aug. 21, 2012, 11:17 a.m. UTC
Add device tree data for tps65910 regulator by adding all tps65910
regulator nodes. Regulator is initialized based on compatiable
name provided in tps65910 DT file.

All tps65910 PMIC regulator device tree nodes are placed in a
seperate device tree include file (tps65910.dtsi). This patch
was tested on AM335x-EVM.

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
 arch/arm/boot/dts/tps65910.dtsi |   86 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 arch/arm/boot/dts/tps65910.dtsi

Comments

Stephen Warren Aug. 21, 2012, 3:48 p.m. UTC | #1
On 08/21/2012 05:17 AM, AnilKumar Ch wrote:
> Add device tree data for tps65910 regulator by adding all tps65910
> regulator nodes. Regulator is initialized based on compatiable
> name provided in tps65910 DT file.
> 
> All tps65910 PMIC regulator device tree nodes are placed in a
> seperate device tree include file (tps65910.dtsi). This patch
> was tested on AM335x-EVM.

This .dtsi file adds a node for every single regulator within the
TPS65910, which in turn means that of_regulator_match() will find a node
for every regulator, and in turn every regulator will be registered. On
some boards, not all of those regulators will be used, and hence we
don't want all of them to be registered.

If we backport the /delete-node/ feature I implemented, perhaps we could
solve that; see:

http://comments.gmane.org/gmane.linux.drivers.devicetree/19170

> diff --git a/arch/arm/boot/dts/tps65910.dtsi b/arch/arm/boot/dts/tps65910.dtsi

I wonder if there shouldn't be some better common location for .dtsi
files that are ancilliary chips, rather than ARM-specific?

> +&tps {
> +	compatible = "ti,tps65910";

Perhaps this file should expect some more unique label than just "tps".
Many boards have many power-related chips, all with model names that
start with "tps"; I foresee conflict here.

If dtc allowed /include/ inside a node rather than just at the
top-level, that might solve this. Or, #defines to the board .dts could
define the label name, and the .dtsi reference that label name.
Mark Brown Aug. 21, 2012, 4:38 p.m. UTC | #2
On Tue, Aug 21, 2012 at 09:48:21AM -0600, Stephen Warren wrote:

> This .dtsi file adds a node for every single regulator within the
> TPS65910, which in turn means that of_regulator_match() will find a node
> for every regulator, and in turn every regulator will be registered. On
> some boards, not all of those regulators will be used, and hence we
> don't want all of them to be registered.

This isn't the general view for the regualtor API - we generally want
all regulators to be registered in order to allow us to see what's going
on with things even if we've not figured them out from software.

> If dtc allowed /include/ inside a node rather than just at the
> top-level, that might solve this. Or, #defines to the board .dts could
> define the label name, and the .dtsi reference that label name.

I do constantly wish that more of the people churning out DT stuff would
look at the tooling around it :/
Stephen Warren Aug. 21, 2012, 6:05 p.m. UTC | #3
On 08/21/2012 10:38 AM, Mark Brown wrote:
> On Tue, Aug 21, 2012 at 09:48:21AM -0600, Stephen Warren wrote:
> 
>> This .dtsi file adds a node for every single regulator within
>> the TPS65910, which in turn means that of_regulator_match() will
>> find a node for every regulator, and in turn every regulator will
>> be registered. On some boards, not all of those regulators will
>> be used, and hence we don't want all of them to be registered.
> 
> This isn't the general view for the regualtor API - we generally
> want all regulators to be registered in order to allow us to see
> what's going on with things even if we've not figured them out from
> software.

Oh, I said the above specifically because when I added the LDO
configuration for the regulators that weren't used to the Tegra .dts
files, you told me to remove it, based on the comment I put in there
that they weren't used on the board.
Mark Brown Aug. 21, 2012, 6:08 p.m. UTC | #4
On Tue, Aug 21, 2012 at 12:05:15PM -0600, Stephen Warren wrote:
> On 08/21/2012 10:38 AM, Mark Brown wrote:

> > This isn't the general view for the regualtor API - we generally
> > want all regulators to be registered in order to allow us to see
> > what's going on with things even if we've not figured them out from
> > software.

> Oh, I said the above specifically because when I added the LDO
> configuration for the regulators that weren't used to the Tegra .dts
> files, you told me to remove it, based on the comment I put in there
> that they weren't used on the board.

The board shouldn't have to define the regulators, the regulator driver
really ought to be able to figure out that they're there by itself if
there's no configuration based purely on knowing which chip is there.
From that point of view it's OK for the chip .dtsi to have them (though
ideally the driver wouldn't *need* that either) which was what was
happening here.
Tony Lindgren Aug. 24, 2012, 8:16 p.m. UTC | #5
* Mark Brown <broonie@opensource.wolfsonmicro.com> [120821 11:09]:
> On Tue, Aug 21, 2012 at 12:05:15PM -0600, Stephen Warren wrote:
> > On 08/21/2012 10:38 AM, Mark Brown wrote:
> 
> > > This isn't the general view for the regualtor API - we generally
> > > want all regulators to be registered in order to allow us to see
> > > what's going on with things even if we've not figured them out from
> > > software.
> 
> > Oh, I said the above specifically because when I added the LDO
> > configuration for the regulators that weren't used to the Tegra .dts
> > files, you told me to remove it, based on the comment I put in there
> > that they weren't used on the board.
> 
> The board shouldn't have to define the regulators, the regulator driver
> really ought to be able to figure out that they're there by itself if
> there's no configuration based purely on knowing which chip is there.
> From that point of view it's OK for the chip .dtsi to have them (though
> ideally the driver wouldn't *need* that either) which was what was
> happening here.

So I assume no changes needed here then?

Regards,

Tony
Mark Brown Aug. 27, 2012, 4:51 p.m. UTC | #6
On Fri, Aug 24, 2012 at 01:16:34PM -0700, Tony Lindgren wrote:
> * Mark Brown <broonie@opensource.wolfsonmicro.com> [120821 11:09]:

> > The board shouldn't have to define the regulators, the regulator driver
> > really ought to be able to figure out that they're there by itself if
> > there's no configuration based purely on knowing which chip is there.
> > From that point of view it's OK for the chip .dtsi to have them (though
> > ideally the driver wouldn't *need* that either) which was what was
> > happening here.

> So I assume no changes needed here then?

Seems that way.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/tps65910.dtsi b/arch/arm/boot/dts/tps65910.dtsi
new file mode 100644
index 0000000..92693a8
--- /dev/null
+++ b/arch/arm/boot/dts/tps65910.dtsi
@@ -0,0 +1,86 @@ 
+/*
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * Integrated Power Management Chip
+ * http://www.ti.com/lit/ds/symlink/tps65910.pdf
+ */
+
+&tps {
+	compatible = "ti,tps65910";
+
+	regulators {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		vrtc_reg: regulator@0 {
+			reg = <0>;
+			regulator-compatible = "vrtc";
+		};
+
+		vio_reg: regulator@1 {
+			reg = <1>;
+			regulator-compatible = "vio";
+		};
+
+		vdd1_reg: regulator@2 {
+			reg = <2>;
+			regulator-compatible = "vdd1";
+		};
+
+		vdd2_reg: regulator@3 {
+			reg = <3>;
+			regulator-compatible = "vdd2";
+		};
+
+		vdd3_reg: regulator@4 {
+			reg = <4>;
+			regulator-compatible = "vdd3";
+		};
+
+		vdig1_reg: regulator@5 {
+			reg = <5>;
+			regulator-compatible = "vdig1";
+		};
+
+		vdig2_reg: regulator@6 {
+			reg = <6>;
+			regulator-compatible = "vdig2";
+		};
+
+		vpll_reg: regulator@7 {
+			reg = <7>;
+			regulator-compatible = "vpll";
+		};
+
+		vdac_reg: regulator@8 {
+			reg = <8>;
+			regulator-compatible = "vdac";
+		};
+
+		vaux1_reg: regulator@9 {
+			reg = <9>;
+			regulator-compatible = "vaux1";
+		};
+
+		vaux2_reg: regulator@10 {
+			reg = <10>;
+			regulator-compatible = "vaux2";
+		};
+
+		vaux33_reg: regulator@11 {
+			reg = <11>;
+			regulator-compatible = "vaux33";
+		};
+
+		vmmc_reg: regulator@12 {
+			reg = <12>;
+			regulator-compatible = "vmmc";
+		};
+	};
+};