[PATCH 1/2] ARM: tegra: config: enable TPS65910 drivers

[PATCH 1/2] ARM: tegra: config: enable TPS65910 drivers

[Laxman Dewangan]

Enable TPS65910 mfd, gpio and regulator drivers.
This is the PMIC module for Tegra30 based cardhu
platform.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 arch/arm/configs/tegra_defconfig |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 1198dd6..26ad2b4 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -106,16 +106,19 @@ CONFIG_I2C=y
 CONFIG_I2C_TEGRA=y
 CONFIG_SPI=y
 CONFIG_SPI_TEGRA=y
+CONFIG_GPIO_TPS65910=y
 CONFIG_POWER_SUPPLY=y
 CONFIG_BATTERY_SBS=y
 CONFIG_SENSORS_LM90=y
 CONFIG_MFD_TPS6586X=y
+CONFIG_MFD_TPS65910=y
 CONFIG_REGULATOR=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
 CONFIG_REGULATOR_VIRTUAL_CONSUMER=y
 CONFIG_REGULATOR_GPIO=y
 CONFIG_REGULATOR_TPS62360=y
 CONFIG_REGULATOR_TPS6586X=y
+CONFIG_REGULATOR_TPS65910=y
 CONFIG_SOUND=y
 CONFIG_SND=y
 # CONFIG_SND_SUPPORT_OLD_API is not set
-- 
1.7.1.1

[PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Laxman Dewangan]

Add device info for the PMIC device tps65911 in tegra-cardhu
dts file. This device supports the multiple regulator rails,
gpio, interrupts.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---

 arch/arm/boot/dts/tegra-cardhu.dts |   94 ++++++++++++++++++++++++++++++++++++
 1 files changed, 94 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-cardhu.dts b/arch/arm/boot/dts/tegra-cardhu.dts
index 36321bc..093dde8 100644
--- a/arch/arm/boot/dts/tegra-cardhu.dts
+++ b/arch/arm/boot/dts/tegra-cardhu.dts
@@ -126,6 +126,100 @@
 			ti,vsel0-state-high;
 			ti,vsel1-state-high;
 		};
+
+		tps65911: tps65911@2d {
+			compatible = "ti,tps65911";
+			reg = <0x2d>;
+
+			#gpio-cells = <2>;
+			gpio-controller;
+
+			regulators {
+				vdd1_reg: vdd1 {
+					regulator-name = "vdd1";
+					regulator-min-microvolt = < 600000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+
+				vdd2_reg: vdd2 {
+					regulator-name = "vdd2";
+					regulator-min-microvolt = < 600000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				vddctrl_reg: vddctrl {
+					regulator-name = "vddctrl";
+					regulator-min-microvolt = < 600000>;
+					regulator-max-microvolt = <1400000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+
+				vio_reg: vio {
+					regulator-name = "vio";
+					regulator-min-microvolt = <1800000>;
+					regulator-max-microvolt = <1800000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+
+				ldo1_reg: ldo1 {
+					regulator-name = "ldo1";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+				};
+
+				ldo2_reg: ldo2 {
+					regulator-name = "ldo2";
+					regulator-min-microvolt = <1050000>;
+					regulator-max-microvolt = <1050000>;
+				};
+
+				ldo3_reg: ldo3 {
+					regulator-name = "ldo3";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+				};
+
+				ldo4_reg: ldo4 {
+					regulator-name = "ldo4";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+				};
+
+				ldo5_reg: ldo5 {
+					regulator-name = "ldo5";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+				};
+
+				ldo6_reg: ldo6 {
+					regulator-name = "ldo6";
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+				};
+
+				ldo7_reg: ldo7 {
+					regulator-name = "ldo7";
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+
+				ldo8_reg: ldo8 {
+					regulator-name = "ldo8";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+				};
+			};
+		};
 	};
 
 	ahub {
-- 
1.7.1.1

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Stephen Warren]

On 05/22/2012 07:05 AM, Laxman Dewangan wrote:
> Add device info for the PMIC device tps65911 in tegra-cardhu
> dts file. This device supports the multiple regulator rails,
> gpio, interrupts.

FYI, patch 1 in this series looks fine. Some comments below though:

> diff --git a/arch/arm/boot/dts/tegra-cardhu.dts b/arch/arm/boot/dts/tegra-cardhu.dts

> +		tps65911: tps65911@2d {
> +			compatible = "ti,tps65911";
> +			reg = <0x2d>;
> +
> +			#gpio-cells = <2>;
> +			gpio-controller;
> +
> +			regulators {

Please add the following properties here:

	#address-cells = <1>;
	#size-cells = <0>;

> +				vdd1_reg: vdd1 {

This node name should be "regulator", since nodes are generally named
after the class of object they represent. Since all the nodes will then
have the same name, you'll need to add a unit address ("@nnnn") to the
node name.

Nitpicky, but the labels might be more logical as reg_vdd1 rather than
vdd1_reg, but not a big deal.

So, please replace the line above with:

	reg_vdd1: regulator@0 {
		reg = <0>;

> +					regulator-name = "vdd1";
> +					regulator-min-microvolt = < 600000>;
> +					regulator-max-microvolt = <1500000>;
> +					regulator-always-on;
> +					regulator-boot-on;
> +				};
> +
> +				vdd2_reg: vdd2 {

And similarly, that would become:

	reg_vdd2: regulator@1 {
		reg = <1>;

etc.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Laxman Dewangan]

On Tuesday 22 May 2012 10:10 PM, Stephen Warren wrote:
> On 05/22/2012 07:05 AM, Laxman Dewangan wrote:
>> Add device info for the PMIC device tps65911 in tegra-cardhu
>> dts file. This device supports the multiple regulator rails,
>> gpio, interrupts.
> FYI, patch 1 in this series looks fine. Some comments below though:
>
>> diff --git a/arch/arm/boot/dts/tegra-cardhu.dts b/arch/arm/boot/dts/tegra-cardhu.dts
>> +		tps65911: tps65911@2d {
>> +			compatible = "ti,tps65911";
>> +			reg =<0x2d>;
>> +
>> +			#gpio-cells =<2>;
>> +			gpio-controller;
>> +
>> +			regulators {
> Please add the following properties here:
>
> 	#address-cells =<1>;
> 	#size-cells =<0>;
>
>> +				vdd1_reg: vdd1 {
> This node name should be "regulator", since nodes are generally named
> after the class of object they represent. Since all the nodes will then
> have the same name, you'll need to add a unit address ("@nnnn") to the
> node name.
>

Nop, we can not do it. The node name should match with the name 
mentioned in driver otherwise the regulator node search will fail
Following is the excerpt of the code:
int of_regulator_match(struct device *dev, struct device_node *node,
                        struct of_regulator_match *matches,
                        unsigned int num_matches)
{

         for (i = 0; i < num_matches; i++) {
                 struct of_regulator_match *match = &matches[i];
                 struct device_node *child;

                 child = of_find_node_by_name(node, match->name);
                 if (!child)
                         continue;

:::::::::::
}

static struct of_regulator_match tps65911_matches[] = {
         { .name = "vrtc",       .driver_data = (void *) 
&tps65911_regs[0] },
         { .name = "vio",        .driver_data = (void *) 
&tps65911_regs[1] },
         { .name = "vdd1",       .driver_data = (void *) 
&tps65911_regs[2] },
         { .name = "vdd2",       .driver_data = (void *) 
&tps65911_regs[3] },
         { .name = "vddctrl",    .driver_data = (void *) 
&tps65911_regs[4] },
         { .name = "ldo1",       .driver_data = (void *) 
&tps65911_regs[5] },
         { .name = "ldo2",       .driver_data = (void *) 
&tps65911_regs[6] },
  :::::::::::::::::::::::::::::::::::::
         { .name = "ldo8",       .driver_data = (void *) 
&tps65911_regs[12] },
};



So only we can do it as
     reg_vdd1: vdd1 {
                 reg = <0>;
             :::::::::
     };

     reg_vdd2: vdd2 {
             reg = < 1>;
             :::::::::::
     };

> Nitpicky, but the labels might be more logical as reg_vdd1 rather than
> vdd1_reg, but not a big deal.
>
> So, please replace the line above with:
>
> 	reg_vdd1: regulator@0 {
> 		reg =<0>;
>

Why do we really require the reg at all?
I dont think any usage of doing this.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Stephen Warren]

On 05/22/2012 11:09 AM, Laxman Dewangan wrote:
> On Tuesday 22 May 2012 10:10 PM, Stephen Warren wrote:
>> On 05/22/2012 07:05 AM, Laxman Dewangan wrote:
>>> Add device info for the PMIC device tps65911 in tegra-cardhu
>>> dts file. This device supports the multiple regulator rails,
>>> gpio, interrupts.
>> FYI, patch 1 in this series looks fine. Some comments below though:
>>
>>> diff --git a/arch/arm/boot/dts/tegra-cardhu.dts
>>> b/arch/arm/boot/dts/tegra-cardhu.dts
>>> +        tps65911: tps65911@2d {
>>> +            compatible = "ti,tps65911";
>>> +            reg =<0x2d>;
>>> +
>>> +            #gpio-cells =<2>;
>>> +            gpio-controller;
>>> +
>>> +            regulators {
>>
>> Please add the following properties here:
>>
>>     #address-cells =<1>;
>>     #size-cells =<0>;
>>
>>> +                vdd1_reg: vdd1 {
>>
>> This node name should be "regulator", since nodes are generally named
>> after the class of object they represent. Since all the nodes will then
>> have the same name, you'll need to add a unit address ("@nnnn") to the
>> node name.
> 
> Nop, we can not do it. The node name should match with the name
> mentioned in driver otherwise the regulator node search will fail
> Following is the excerpt of the code:

Hmm. That seems wrong. I thought I had seen at least some regulator
bindings where these nodes included a name property so that the nodes
didn't need any particular name.

Olof, what are your thoughts here - do we need to fix the code so the
node names aren't relevant? IIRC, we have had to change some other
bindings due to the same issue.

...
>> Nitpicky, but the labels might be more logical as reg_vdd1 rather than
>> vdd1_reg, but not a big deal.
>>
>> So, please replace the line above with:
>>
>>     reg_vdd1: regulator@0 {
>>         reg =<0>;
>>
> 
> Why do we really require the reg at all?
> I dont think any usage of doing this.

I guess if these regulators are enabled at boot and always on, we don't
even need the labels for now; we could add labels later as/when drivers
begin to dynamically control the regulators.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Laxman Dewangan]

On Tuesday 22 May 2012 10:49 PM, Stephen Warren wrote:
> On 05/22/2012 11:09 AM, Laxman Dewangan wrote:
>> On Tuesday 22 May 2012 10:10 PM, Stephen Warren wrote:
>>> On 05/22/2012 07:05 AM, Laxman Dewangan wrote:
>>>> Add device info for the PMIC device tps65911 in tegra-cardhu
>>>> dts file. This device supports the multiple regulator rails,
>>>> gpio, interrupts.
>>> FYI, patch 1 in this series looks fine. Some comments below though:
>>>
>>>> diff --git a/arch/arm/boot/dts/tegra-cardhu.dts
>>>> b/arch/arm/boot/dts/tegra-cardhu.dts
>>>> +        tps65911: tps65911@2d {
>>>> +            compatible = "ti,tps65911";
>>>> +            reg =<0x2d>;
>>>> +
>>>> +            #gpio-cells =<2>;
>>>> +            gpio-controller;
>>>> +
>>>> +            regulators {
>>> Please add the following properties here:
>>>
>>>      #address-cells =<1>;
>>>      #size-cells =<0>;
>>>
>>>> +                vdd1_reg: vdd1 {
>>> This node name should be "regulator", since nodes are generally named
>>> after the class of object they represent. Since all the nodes will then
>>> have the same name, you'll need to add a unit address ("@nnnn") to the
>>> node name.
>> Nop, we can not do it. The node name should match with the name
>> mentioned in driver otherwise the regulator node search will fail
>> Following is the excerpt of the code:
> Hmm. That seems wrong. I thought I had seen at least some regulator
> bindings where these nodes included a name property so that the nodes
> didn't need any particular name.
It is only applicable for the fixed regulators or the device supports 
only one regulator or each regulator have their own compatibility and 
the matching is done by compatibility, not by node name.


> Olof, what are your thoughts here - do we need to fix the code so the
> node names aren't relevant? IIRC, we have had to change some other
> bindings due to the same issue.
>
> ...
>>> Nitpicky, but the labels might be more logical as reg_vdd1 rather than
>>> vdd1_reg, but not a big deal.
>>>
>>> So, please replace the line above with:
>>>
>>>      reg_vdd1: regulator@0 {
>>>          reg =<0>;
>>>
>> Why do we really require the reg at all?
>> I dont think any usage of doing this.
> I guess if these regulators are enabled at boot and always on, we don't
> even need the labels for now; we could add labels later as/when drivers
> begin to dynamically control the regulators.

I think we should provide the label here whether it is always on or not.
The driver who uses the rails will not aware that rail is always on or not.
Second thing is that this gives uniformity and whenever any consumer get 
added, we will not touch this part, only will be change in the driver 
specific part.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Stephen Warren]

On 05/22/2012 11:56 AM, Laxman Dewangan wrote:
> On Tuesday 22 May 2012 10:49 PM, Stephen Warren wrote:
>> On 05/22/2012 11:09 AM, Laxman Dewangan wrote:
>>> On Tuesday 22 May 2012 10:10 PM, Stephen Warren wrote:
>>>> On 05/22/2012 07:05 AM, Laxman Dewangan wrote:
>>>>> Add device info for the PMIC device tps65911 in tegra-cardhu
>>>>> dts file. This device supports the multiple regulator rails,
>>>>> gpio, interrupts.
...
>>>> Nitpicky, but the labels might be more logical as reg_vdd1 rather than
>>>> vdd1_reg, but not a big deal.
>>>>
>>>> So, please replace the line above with:
>>>>
>>>>      reg_vdd1: regulator@0 {
>>>>          reg = <0>;
>>>
>>> Why do we really require the reg at all?
>>> I dont think any usage of doing this.

Oh, perhaps you meant the reg property not "reg_" in the label name?

It is required because the parent node has #address-cells and
#size-cells and because the node name itself has a unit address ("@nnn").

>> I guess if these regulators are enabled at boot and always on, we don't
>> even need the labels for now; we could add labels later as/when drivers
>> begin to dynamically control the regulators.
> 
> I think we should provide the label here whether it is always on or not.
> The driver who uses the rails will not aware that rail is always on or not.
> Second thing is that this gives uniformity and whenever any consumer get
> added, we will not touch this part, only will be change in the driver
> specific part.

Yes, if drivers are referring to these nodes, you do need the label.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Laxman Dewangan]

On Tuesday 22 May 2012 11:57 PM, Stephen Warren wrote:
> On 05/22/2012 11:56 AM, Laxman Dewangan wrote:
>> On Tuesday 22 May 2012 10:49 PM, Stephen Warren wrote:
>>> On 05/22/2012 11:09 AM, Laxman Dewangan wrote:
>>>> On Tuesday 22 May 2012 10:10 PM, Stephen Warren wrote:
>>>>> On 05/22/2012 07:05 AM, Laxman Dewangan wrote:
>>>>>> Add device info for the PMIC device tps65911 in tegra-cardhu
>>>>>> dts file. This device supports the multiple regulator rails,
>>>>>> gpio, interrupts.
> ...
>>>>> Nitpicky, but the labels might be more logical as reg_vdd1 rather than
>>>>> vdd1_reg, but not a big deal.
>>>>>
>>>>> So, please replace the line above with:
>>>>>
>>>>>       reg_vdd1: regulator@0 {
>>>>>           reg =<0>;
>>>> Why do we really require the reg at all?
>>>> I dont think any usage of doing this.
> Oh, perhaps you meant the reg property not "reg_" in the label name?
>
> It is required because the parent node has #address-cells and
> #size-cells and because the node name itself has a unit address ("@nnn").
>

But we can not put
     reg_vdd1:regulator@0 {
         ::::::::::::::
     }


due to their dt binding with their node names.
In this case still do we need  reg=<0> and #address-cells and #size-cells?

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Stephen Warren]

On 05/22/2012 12:42 PM, Laxman Dewangan wrote:
> On Tuesday 22 May 2012 11:57 PM, Stephen Warren wrote:
>> On 05/22/2012 11:56 AM, Laxman Dewangan wrote:
>>> On Tuesday 22 May 2012 10:49 PM, Stephen Warren wrote:
>>>> On 05/22/2012 11:09 AM, Laxman Dewangan wrote:
>>>>> On Tuesday 22 May 2012 10:10 PM, Stephen Warren wrote:
>>>>>> On 05/22/2012 07:05 AM, Laxman Dewangan wrote:
>>>>>>> Add device info for the PMIC device tps65911 in tegra-cardhu
>>>>>>> dts file. This device supports the multiple regulator rails,
>>>>>>> gpio, interrupts.
>> ...
>>>>>> Nitpicky, but the labels might be more logical as reg_vdd1 rather
>>>>>> than
>>>>>> vdd1_reg, but not a big deal.
>>>>>>
>>>>>> So, please replace the line above with:
>>>>>>
>>>>>>       reg_vdd1: regulator@0 {
>>>>>>           reg =<0>;
>>>>> Why do we really require the reg at all?
>>>>> I dont think any usage of doing this.
>> Oh, perhaps you meant the reg property not "reg_" in the label name?
>>
>> It is required because the parent node has #address-cells and
>> #size-cells and because the node name itself has a unit address ("@nnn").
>>
> 
> But we can not put
>     reg_vdd1:regulator@0 {
>         ::::::::::::::
>     }
> 
> 
> due to their dt binding with their node names.

I spoke to Olof about this on IRC, and he also tended to agree that the
regulator node names should not be used directly.

However, Mark warned that changing this would be a bit painful because
there are already users of the existing scheme. It looks like that's
only tps65910 (which we haven't started using yet), db8500, and ab8500,
so probably not that big a deal.

We could probably amend of_regulator_match() to work in a
backwards-compatible fashion; to look for some name property in each
child node first, and then fall back to using the node name if that
resulted in no matches. That would allow db8500/ab8500 to be converted
at leisure.

We could either augment struct of_regulator_match with an integer ID
field for each regulator (which would perhaps make it slightly painful
to write the nodes and keep the IDs matched up), or add a new property
to each regulator provider node e.g. regulator-id which contained the
name that the regulator driver knows the regulator as (which would match
struct of_regulator_match.name), since the existing regulator-name
property is used for semantically different purposes.

That would result in:

> tps65911: tps65911@2d {
> 	compatible = "ti,tps65911";
> 	reg = <0x2d>;
> 
> 	#gpio-cells = <2>;
> 	gpio-controller;
> 
> 	regulators {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		vdd1_reg: regulator@0 {
> 			reg = <0>;
> 			regulator-id = "vdd1"; /* Internal name */
> 			regulator-name = "vdd_1v2_gen"; /* Signal on schematic */
...
> 		};
> 
> 		vdd2_reg: regulator@1 {
> 			reg = <1>;
> 			regulator-id = "vdd2";
> 			regulator-name = "vdd_1v5_gen";
...

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

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

On Fri, Jun 01, 2012 at 01:23:24PM -0600, Stephen Warren wrote:

> However, Mark warned that changing this would be a bit painful because
> there are already users of the existing scheme. It looks like that's
> only tps65910 (which we haven't started using yet), db8500, and ab8500,
> so probably not that big a deal.

No, there's a bunch of others - some queued for -next, others open
coding the same scheme.  Any device with more than one regulator in a
node should be using the same scheme.

> We could either augment struct of_regulator_match with an integer ID
> field for each regulator (which would perhaps make it slightly painful
> to write the nodes and keep the IDs matched up), or add a new property

No, that's awful.  How's anyone supposed to read stuff like that?  The
interrupt bindings are a disaster, not a model.

> to each regulator provider node e.g. regulator-id which contained the
> name that the regulator driver knows the regulator as (which would match
> struct of_regulator_match.name), since the existing regulator-name
> property is used for semantically different purposes.

Oh, ick.  This isn't nice.  If anything I'd be more inclined to put a
named property in there and have drivers look for its presence.  The
presence of multiple name properties isn't nice.

> > 		vdd1_reg: regulator@0 {

Can't we use the right hand side of this?  It appears to just be
syntactic sugar without any current meaning.

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

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Stephen Warren]

On 06/01/2012 02:40 PM, Mark Brown wrote:
> On Fri, Jun 01, 2012 at 01:23:24PM -0600, Stephen Warren wrote:
> 
>> However, Mark warned that changing this would be a bit painful 
>> because there are already users of the existing scheme. It looks 
>> like that's only tps65910 (which we haven't started using yet), 
>> db8500, and ab8500, so probably not that big a deal.
> 
> No, there's a bunch of others - some queued for -next, others open
>  coding the same scheme.  Any device with more than one regulator
> in a node should be using the same scheme.
> 
>> We could either augment struct of_regulator_match with an
>> integer ID field for each regulator (which would perhaps make it
>> slightly painful to write the nodes and keep the IDs matched up),
>> or add a new property
> 
> No, that's awful.  How's anyone supposed to read stuff like that? 
> The interrupt bindings are a disaster, not a model.
> 
>> to each regulator provider node e.g. regulator-id which
>> contained the name that the regulator driver knows the regulator
>> as (which would match struct of_regulator_match.name), since the
>> existing regulator-name property is used for semantically
>> different purposes.
> 
> Oh, ick.  This isn't nice.  If anything I'd be more inclined to
> put a named property in there and have drivers look for its
> presence. The presence of multiple name properties isn't nice.

Could you expand on "named property" a bit; I'm not quite sure what
you're getting at - literally a property with name "named" (which
would be the same as regulator-id under just a different property
name), or ...?

>>> vdd1_reg: regulator@0 {
> 
> Can't we use the right hand side of this?  It appears to just be 
> syntactic sugar without any current meaning.

The stuff to the right of @ is the "unit address" and must match the
value in the reg property. Using that was the first proposal I had
above (which I also didn't like as much)

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

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

On Fri, Jun 01, 2012 at 02:44:00PM -0600, Stephen Warren wrote:

> Could you expand on "named property" a bit; I'm not quite sure what
> you're getting at - literally a property with name "named" (which
> would be the same as regulator-id under just a different property
> name), or ...?

Just a property where we only care about a name (ie, that the property
is present).

> > Can't we use the right hand side of this?  It appears to just be 
> > syntactic sugar without any current meaning.

> The stuff to the right of @ is the "unit address" and must match the
> value in the reg property. Using that was the first proposal I had
> above (which I also didn't like as much)

The stuff to the left of the @ is just noise right now, though - it has
no meaning currently.  It's filled in with "regulator" because we need
to put something there AFAICT.

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

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Olof Johansson]

[+devicetree-discuss and grant/rob]

On Fri, Jun 1, 2012 at 2:04 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Jun 01, 2012 at 02:44:00PM -0600, Stephen Warren wrote:
>
>> Could you expand on "named property" a bit; I'm not quite sure what
>> you're getting at - literally a property with name "named" (which
>> would be the same as regulator-id under just a different property
>> name), or ...?
>
> Just a property where we only care about a name (ie, that the property
> is present).
>
>> > Can't we use the right hand side of this?  It appears to just be
>> > syntactic sugar without any current meaning.
>
>> The stuff to the right of @ is the "unit address" and must match the
>> value in the reg property. Using that was the first proposal I had
>> above (which I also didn't like as much)
>
> The stuff to the left of the @ is just noise right now, though - it has
> no meaning currently.  It's filled in with "regulator" because we need
> to put something there AFAICT.

Right. In general (and historically) in the device tree, names of the
nodes should have meaning for the person reading the device tree, but
it's not meant to be used for software to figure out the hardware
configuration -- that should instead be handled through compatible +
other properties.

Names are generally kept fairly generic (ethernet, cpus, memory, pci, etc).

Where it starts to become gray area is when it comes down to specific
bindings, and essentially the device nodes underneath of those
devices. It's been generally accepted that we can put meaning to the
names there if needed, but it's still better to avoid it.

I was originally OK with the regulator binding where names have
meaning, but after having looked at it a bit recently when looking at
bindings for some new boards we have, I realized that the original
suggestion for regulator bindings doesn't necessarily isolate the
naming dependencies to only be under the regulators in question. In
particular, for things such as fixed regulators, they can be located
at other places in the device tree.

Maybe the solution to that case is to just aggregate them in one place
and make a pseudo-binding for that (or those, in case of multiple
locations).

On the rest of the name-has-meaning discussion, I think it would be
cleaner to move away from it now while there's relatively few users of
it (with a migratin path), rather than revise it later. But I'll leave
it to Grant and Rob to decide which way the prefer things to be. I
think they might both be travelling around LC/LinuxCon events at the
moment though.


-Olof

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Rob Herring]

On 06/02/2012 04:19 PM, Olof Johansson wrote:
> [+devicetree-discuss and grant/rob]
> 
> On Fri, Jun 1, 2012 at 2:04 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Fri, Jun 01, 2012 at 02:44:00PM -0600, Stephen Warren wrote:
>>
>>> Could you expand on "named property" a bit; I'm not quite sure what
>>> you're getting at - literally a property with name "named" (which
>>> would be the same as regulator-id under just a different property
>>> name), or ...?
>>
>> Just a property where we only care about a name (ie, that the property
>> is present).
>>
>>>> Can't we use the right hand side of this?  It appears to just be
>>>> syntactic sugar without any current meaning.
>>
>>> The stuff to the right of @ is the "unit address" and must match the
>>> value in the reg property. Using that was the first proposal I had
>>> above (which I also didn't like as much)
>>
>> The stuff to the left of the @ is just noise right now, though - it has
>> no meaning currently.  It's filled in with "regulator" because we need
>> to put something there AFAICT.
> 
> Right. In general (and historically) in the device tree, names of the
> nodes should have meaning for the person reading the device tree, but
> it's not meant to be used for software to figure out the hardware
> configuration -- that should instead be handled through compatible +
> other properties.
> 
> Names are generally kept fairly generic (ethernet, cpus, memory, pci, etc).
> 
> Where it starts to become gray area is when it comes down to specific
> bindings, and essentially the device nodes underneath of those
> devices. It's been generally accepted that we can put meaning to the
> names there if needed, but it's still better to avoid it.
> 
> I was originally OK with the regulator binding where names have
> meaning, but after having looked at it a bit recently when looking at
> bindings for some new boards we have, I realized that the original
> suggestion for regulator bindings doesn't necessarily isolate the
> naming dependencies to only be under the regulators in question. In
> particular, for things such as fixed regulators, they can be located
> at other places in the device tree.
> 
> Maybe the solution to that case is to just aggregate them in one place
> and make a pseudo-binding for that (or those, in case of multiple
> locations).
> 
> On the rest of the name-has-meaning discussion, I think it would be
> cleaner to move away from it now while there's relatively few users of
> it (with a migratin path), rather than revise it later. But I'll leave
> it to Grant and Rob to decide which way the prefer things to be. I
> think they might both be travelling around LC/LinuxCon events at the
> moment though.

I tend to agree with Steven's and Olof's comments in this thread. As the
node names generally don't have much meaning, I don't think we should
start now. We've already got multiple styles of bindings and I don't
think we need more.

Rob


> 
> -Olof
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

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

On Sat, Jun 02, 2012 at 09:45:10PM -0500, Rob Herring wrote:

> I tend to agree with Steven's and Olof's comments in this thread. As the
> node names generally don't have much meaning, I don't think we should
> start now. We've already got multiple styles of bindings and I don't
> think we need more.

Well, if we're going to go with an existing idiom the normal thing would
be an ordered array which is absolutely abysmal from a usability
standpoint.  Compatible properties don't work as the whole reason we
have an issue here is that people want to have a single node
representing a group of regulators - for regulators which we can add a
compatible property to we're already doing that and have no issue.

What device tree seems to need rather badly is a way of representing
key/value pairs - aside from the legacy bindings that seems to be the
major source of pain when trying to contort things into DT.

Using the "regulator" string that we have to put in the binding (which
is currently totally meaningless) does seem like a good way forward
here.

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

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

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

On Sun, Jun 03, 2012 at 06:11:21AM -1000, Mitch Bradley wrote:

> >What device tree seems to need rather badly is a way of representing
> >key/value pairs -

> Perhaps ironically, the fundamental device tree construct - the
> "property" - is a key/value pair.

Yeah, I know - the problem is when the value is itself a node, at least
as far as I can tell.  It's also frustrating that nodes have this user
bit of identifying text that we can't seem to use for some reason.

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

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

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

On Sat, Jun 02, 2012 at 02:19:57PM -0700, Olof Johansson wrote:

> naming dependencies to only be under the regulators in question. In
> particular, for things such as fixed regulators, they can be located
> at other places in the device tree.

I'm sorry, I can't parse this at all.  What "naming dependencies" are
you talking about?  This is purely an issue for multi-regulator chips.

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

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Laxman Dewangan]

On Saturday 02 June 2012 12:53 AM, Stephen Warren wrote:
>
> We could either augment struct of_regulator_match with an integer ID
> field for each regulator (which would perhaps make it slightly painful
> to write the nodes and keep the IDs matched up), or add a new property
> to each regulator provider node e.g. regulator-id which contained the
> name that the regulator driver knows the regulator as (which would match
> struct of_regulator_match.name), since the existing regulator-name
> property is used for semantically different purposes.
>
> That would result in:
>
>> tps65911: tps65911@2d {
>> 	compatible = "ti,tps65911";
>> 	reg =<0x2d>;
>>
>> 	#gpio-cells =<2>;
>> 	gpio-controller;
>>
>> 	regulators {
>> 		#address-cells =<1>;
>> 		#size-cells =<0>;
>>
>> 		vdd1_reg: regulator@0 {
>> 			reg =<0>;
>> 			regulator-id = "vdd1"; /* Internal name */
>> 			regulator-name = "vdd_1v2_gen"; /* Signal on schematic */
> ...
>> 		};
>>
>> 		vdd2_reg: regulator@1 {
>> 			reg =<1>;
>> 			regulator-id = "vdd2";
>> 			regulator-name = "vdd_1v5_gen";
> ...


So is it fine to go on the above binding?
In this case we need to find the match_regulator based on regulator-id 
rather than by name.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

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

On Sat, Jun 09, 2012 at 12:52:03AM +0530, Laxman Dewangan wrote:

> So is it fine to go on the above binding?
> In this case we need to find the match_regulator based on
> regulator-id rather than by name.

I guess.  I'm not enthusiastic about it (some way of using the key/value
nature of DT as a hash would be much nicer) but it seems that the
combination of DT and the existing code for it can't really give us
more.  

If we're going to do this we need to update all the existing DT bindings
for drivers that use single node regulators like this.  Please also
change the name used for the property to regulator-compatible to make it
clear that the idea is the same as normal compatible properties.

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

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Stephen Warren]

On 06/08/2012 09:06 PM, Mark Brown wrote:
> On Sat, Jun 09, 2012 at 12:52:03AM +0530, Laxman Dewangan wrote:
> 
>> So is it fine to go on the above binding?
>> In this case we need to find the match_regulator based on
>> regulator-id rather than by name.
> 
> I guess.  I'm not enthusiastic about it (some way of using the key/value
> nature of DT as a hash would be much nicer) but it seems that the
> combination of DT and the existing code for it can't really give us
> more.  
> 
> If we're going to do this we need to update all the existing DT bindings
> for drivers that use single node regulators like this.  Please also
> change the name used for the property to regulator-compatible to make it
> clear that the idea is the same as normal compatible properties.

I'm not sure of the logic behind naming the property
"regulator-compatible"; the standard compatible property identifies that
the node is of a particular type/class, whereas the regulator-id in the
example Laxman quoted would indicate the specific identity/object. Those
seem like different things.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

On Fri, Jun 08, 2012 at 10:24:06PM -0600, Stephen Warren wrote:
> On 06/08/2012 09:06 PM, Mark Brown wrote:

> > If we're going to do this we need to update all the existing DT bindings
> > for drivers that use single node regulators like this.  Please also
> > change the name used for the property to regulator-compatible to make it
> > clear that the idea is the same as normal compatible properties.

> I'm not sure of the logic behind naming the property
> "regulator-compatible"; the standard compatible property identifies that
> the node is of a particular type/class, whereas the regulator-id in the
> example Laxman quoted would indicate the specific identity/object. Those
> seem like different things.

They're both doing the same thing - up until you get the second register
compatible device a compatible binding is referencing a specific thing
too.  It's just saying "handle this like an X".

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Stephen Warren]

On 06/10/2012 08:57 PM, Mark Brown wrote:
> On Fri, Jun 08, 2012 at 10:24:06PM -0600, Stephen Warren wrote:
>> On 06/08/2012 09:06 PM, Mark Brown wrote:
> 
>>> If we're going to do this we need to update all the existing DT bindings
>>> for drivers that use single node regulators like this.  Please also
>>> change the name used for the property to regulator-compatible to make it
>>> clear that the idea is the same as normal compatible properties.
> 
>> I'm not sure of the logic behind naming the property
>> "regulator-compatible"; the standard compatible property identifies that
>> the node is of a particular type/class, whereas the regulator-id in the
>> example Laxman quoted would indicate the specific identity/object. Those
>> seem like different things.
> 
> They're both doing the same thing - up until you get the second register
> compatible device a compatible binding is referencing a specific thing
> too.  It's just saying "handle this like an X".

I believe there's a big semantic difference here.

For every node with compatible="foo", you find a driver for "foo" and
instantiate it. This will work for any number of nodes with that
compatible value. The nodes are completely independent and there are no
particular requirements re: what the parent of those nodes are, beyond
being a bus of an appropriate type such as any old I2C bus.

However, with the regulator identifiers, it's almost exactly the opposite:

* There's no generic "search all busses in the system for this regulator
type", but rather once a particular type of regulator chip gets
instantiated, that chip's HW design defines which specific regulators it
contains, and nodes for those regulators may exist as children of the
regulator chip itself, and nowhere else. The individual driver is then
going to look for child nodes with specific
regulator-id/regulator-compatible values, not some arbitrary centralized
table of possible values.

* Each regulator-id/regulator-compatible value identifies a specific
individual regulator within the chip that contains it. There is only one
of each named regulator, since that's what exists in HW. So, this is
about configuring HW that we know exists (because it's part of the HW
represented by the parent node for the chip) rather than defining which
HW is present on unprobeable busses, as the device-level compatible does.

Given those differences, I really think that using "compatible" in the
name of the property is just going to cause confusion.

Re: [PATCH 2/2] ARM: dt: tegra: cardhu: register core regulator tps65911

[Mark Brown]

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

On Mon, Jun 11, 2012 at 09:56:14AM -0600, Stephen Warren wrote:

> * Each regulator-id/regulator-compatible value identifies a specific
> individual regulator within the chip that contains it. There is only one
> of each named regulator, since that's what exists in HW. So, this is
> about configuring HW that we know exists (because it's part of the HW
> represented by the parent node for the chip) rather than defining which
> HW is present on unprobeable busses, as the device-level compatible does.

> Given those differences, I really think that using "compatible" in the
> name of the property is just going to cause confusion.

This doesn't seem terribly different to me especially in some of the
cases people want to use this for where we try to describe the
subfunctions of the chip using this mechanism.  You have a fixed set of
regulators that might exist in a superset device (possibly with some
incompatibilities, or with additional properties providing more data on
the hardware) and then you mix and match what's in the system based
on the nodes you register for the subset device you're using.  This sort
of thing is actually much more idiomatic with DT than it is with
platform data (look at how people want to put device nodes in for the
MFD subfunctions all the time...).

Really all compatible is saying to me is "here's how you understand
this, handle it like an X" and this feels exactly the same.

Of course, if someone could just fix the DT to actually be able to do
key/value pairs, or allow us to do something useful with the
"regulator" string we need to put in there...

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