[PATCH] regulator: core: use correct device for device supply lookup

[PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

When registering the regulator driver, use the rdev->dev for
getting the regulator device of given supply instead of parent
device.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7584a74..f820137 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3150,7 +3150,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (supply) {
 		struct regulator_dev *r;
 
-		r = regulator_dev_lookup(dev, supply, &ret);
+		r = regulator_dev_lookup(&rdev->dev, supply, &ret);
 
 		if (!r) {
 			dev_err(dev, "Failed to find supply %s\n", supply);
-- 
1.7.1.1

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sat, May 19, 2012 at 07:44:06PM +0530, Laxman Dewangan wrote:
> When registering the regulator driver, use the rdev->dev for
> getting the regulator device of given supply instead of parent
> device.

You're providing no motivation for this and it's difficult to see how it
improves things.  The class device is dynamically numbered so it's not
suitable for specifying supplies on a non-DT system and for a DT system
it's not obvious to me that we would want to involve the class device in
anything, it requires an additional layer of indirection but that's
about it.

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Saturday 19 May 2012 10:11 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Sat, May 19, 2012 at 07:44:06PM +0530, Laxman Dewangan wrote:
>> When registering the regulator driver, use the rdev->dev for
>> getting the regulator device of given supply instead of parent
>> device.
> You're providing no motivation for this and it's difficult to see how it
> improves things.  The class device is dynamically numbered so it's not
> suitable for specifying supplies on a non-DT system and for a DT system
> it's not obvious to me that we would want to involve the class device in
> anything, it requires an additional layer of indirection but that's
> about it.
If I dont do this then it will not enter in the following case for 
getting the regulator_dev of supply regulator because dev->of_node is 
null, the tps65910-regulator driver have not set the pdev->dev.ofnode.

static struct regulator_dev *regulator_dev_lookup(struct device *dev,
                                                   const char *supply,
                                                   int *ret)
{
         /* first do a dt based lookup */
         if (dev && dev->of_node) {
::::::::::::::::
             /* Get the regulator device */

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Saturday 19 May 2012 10:44 PM, Laxman Dewangan wrote:
> On Saturday 19 May 2012 10:11 PM, Mark Brown wrote:
>> * PGP Signed by an unknown key
>>
>> On Sat, May 19, 2012 at 07:44:06PM +0530, Laxman Dewangan wrote:
>>> When registering the regulator driver, use the rdev->dev for
>>> getting the regulator device of given supply instead of parent
>>> device.
>> You're providing no motivation for this and it's difficult to see how it
>> improves things.  The class device is dynamically numbered so it's not
>> suitable for specifying supplies on a non-DT system and for a DT system
>> it's not obvious to me that we would want to involve the class device in
>> anything, it requires an additional layer of indirection but that's
>> about it.
> If I dont do this then it will not enter in the following case for
> getting the regulator_dev of supply regulator because dev->of_node is
> null, the tps65910-regulator driver have not set the pdev->dev.ofnode.
>
> static struct regulator_dev *regulator_dev_lookup(struct device *dev,
>                                                     const char *supply,
>                                                     int *ret)
> {
>           /* first do a dt based lookup */
>           if (dev&&  dev->of_node) {
> ::::::::::::::::
>               /* Get the regulator device */
>
>       }
> ::::::::::::::::::
> }
Also in regulator_register we set the of_node as
        rdev->dev.of_node = config->of_node;
         rdev->dev.parent = dev;

Passed config->of_node will only be used if we pass the rdev->dev, not 
rdev->dev.parent

Am I missing anything here in understanding?

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sat, May 19, 2012 at 10:50:54PM +0530, Laxman Dewangan wrote:

> > static struct regulator_dev *regulator_dev_lookup(struct device *dev,
> >                                                   const char *supply,
> >                                                   int *ret)
> >

> Also in regulator_register we set the of_node as

>        rdev->dev.of_node = config->of_node;

So, here we're just setting it to whatever the driver told us to use
which seems about right.

>         rdev->dev.parent = dev;

And here we're parenting the class device with the real device we were
passed which again seems obvious.

> Passed config->of_node will only be used if we pass the rdev->dev,
> not rdev->dev.parent

What does this mean and how is it related to anything you've said above?
I'm sorry but I really can't make head nor tail of what you're trying to
say here, there's just lots and lots of statements here but I'm
struggling to understand how they are related to each other.

> Am I missing anything here in understanding?

I certainly am.  Please go back to square one: what's the problem you
are seeing here?  Then go forward and step by step relate it to the code
change.

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Saturday 19 May 2012 11:10 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Sat, May 19, 2012 at 10:50:54PM +0530, Laxman Dewangan wrote:
>
>> Am I missing anything here in understanding?
> I certainly am.  Please go back to square one: what's the problem you
> are seeing here?  Then go forward and step by step relate it to the code
> change.

Sorry again for not clearing the things.
Here is my connection:
There is two rail V1 and V2. V2 is supplied by V1. There is some devices 
on V1 and V2.
V1---->V2-----device-v2-1
      |------------------device-v1-1

Now I make the dts as:
v1_reg: v1@0 {
     regulator-name="v1";
   :::::::::::
};

v2_reg: v2@1 {
     regulator-name="v2";
     v2-supply=<&v1_reg>;
::::::::::::::
}


Now when registering the v1, I am setting init_data->input_supply = NULL 
and reg_desc->supply_name = NULL;
Config->of_node is the node for v1_reg;
dev->of_node is NULL as it is mfd sub device driver tps65910-pmic.

So registration went fine.

When registering the V2, I am setting init_data->input_supply = NULL and 
reg_desc->supply_name = v2.
config->of_node is node for v2_reg;
dev->of_node is NULL as it is mfd sub device driver tps65910-pmic.

At the time of registration,  as becasue there is valid 
reg_desc->supply_name and hence it tries to lookup the entry for 
<name>-supply i.e. v2-supply in this case for getting regulator_dev.
regulator_register() {
:::::::::::
       if (init_data && init_data->supply_regulator)
                 supply = init_data->supply_regulator;
         else if (regulator_desc->supply_name)
                 supply = regulator_desc->supply_name;

         if (supply) {
                 struct regulator_dev *r;

                 r = regulator_dev_lookup(dev, supply, &ret);
::::::::::::::::::::::::
}

static struct regulator_dev *regulator_dev_lookup(struct device *dev,
                                                   const char *supply,
                                                   int *ret)
{
         /* first do a dt based lookup */
----> Checked here, dev is not null but dev->of_node is null.
         if (dev && dev->of_node) {

------------>The issue is that I am not getting here as dev->node is 
null here.

                 node = of_get_regulator(dev, supply);
                 if (node) {

         }

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sat, May 19, 2012 at 11:26:00PM +0530, Laxman Dewangan wrote:

> At the time of registration,  as becasue there is valid
> reg_desc->supply_name and hence it tries to lookup the entry for
> <name>-supply i.e. v2-supply in this case for getting regulator_dev.
> regulator_register() {

> static struct regulator_dev *regulator_dev_lookup(struct device *dev,
>                                                   const char *supply,
>                                                   int *ret)
> {
>         /* first do a dt based lookup */
> ----> Checked here, dev is not null but dev->of_node is null.
>         if (dev && dev->of_node) {
> 
> ------------>The issue is that I am not getting here as dev->node is
> null here.

But how is this related your patch?  What your patch does is change
things so that instead of trying to look up the supply in the context of
whatever device was passed in by the driver we try to look it up in the
context of the class device we create.  I can't think of any situation
where I'd expect that to make matters any better - the class device
should certainly never appear in the device tree and isn't going to have
a stable name for non-DT systems either.

I'm just not seeing any problem in the core here.  It sounds to me like
the problem might be either with the regulator driver doing something
odd with the struct device it specifies when registering the regulator
(though I'm guessing that it's the tps65910 which looks to be doing
something sensible currently) or the device tree for the board being
odd.  Looking at the changes you posted to tps65910 I suspect the issue
is that you've changed the driver to pass in the platform device for the
regulators as their device rather than the I2C device but it's the I2C
device which appears in the device tree bindings.

If there is a change needed in the core you need to explain what you
believe that change will do.

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Saturday 19 May 2012 11:56 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Sat, May 19, 2012 at 11:26:00PM +0530, Laxman Dewangan wrote:
>
>> At the time of registration,  as becasue there is valid
>> reg_desc->supply_name and hence it tries to lookup the entry for
>> <name>-supply i.e. v2-supply in this case for getting regulator_dev.
>> regulator_register() {
>> static struct regulator_dev *regulator_dev_lookup(struct device *dev,
>>                                                    const char *supply,
>>                                                    int *ret)
>> {
>>          /* first do a dt based lookup */
>> ---->  Checked here, dev is not null but dev->of_node is null.
>>          if (dev&&  dev->of_node) {
>>
>> ------------>The issue is that I am not getting here as dev->node is
>> null here.
> But how is this related your patch?  What your patch does is change
> things so that instead of trying to look up the supply in the context of
> whatever device was passed in by the driver we try to look it up in the
> context of the class device we create.  I can't think of any situation
> where I'd expect that to make matters any better - the class device
> should certainly never appear in the device tree and isn't going to have
> a stable name for non-DT systems either.
>
> I'm just not seeing any problem in the core here.  It sounds to me like
> the problem might be either with the regulator driver doing something
> odd with the struct device it specifies when registering the regulator
> (though I'm guessing that it's the tps65910 which looks to be doing
> something sensible currently) or the device tree for the board being
> odd.  Looking at the changes you posted to tps65910 I suspect the issue
> is that you've changed the driver to pass in the platform device for the
> regulators as their device rather than the I2C device but it's the I2C
> device which appears in the device tree bindings.
>
My board dts file is
       pmu: tps65910@d2 {
                 compatible = "ti,tps65910";
                 reg = <0xd2>;
                 interrupt-parent = <&intc>;
                 interrupts = < 0 118 0x04 >;

                 #gpio-cells = <2>;
                 gpio-controller;

                 #interrupt-cells = <2>;
                 interrupt-controller;

                 regulators {
                         vdd1_reg: vdd1 {
                                 regulator-min-microvolt = < 600000>;
                                 regulator-max-microvolt = <1500000>;
                                 regulator-always-on;
                                 regulator-boot-on;
                                 ti,regulator-ext-sleep-control = <0>;
                         };
                         vdd2_reg: vdd2 {
                                 regulator-min-microvolt = < 600000>;
                                 regulator-max-microvolt = <1500000>;
                                 regulator-always-on;
                                 regulator-boot-on;
                                 ti,regulator-ext-sleep-control = <4>;
                         };
             };
     };

So currently, when regulator_register gets called in 
tps65910-regulator.c, it sets the
config.dev = tps65910->dev;
config.of_node as
                 config.of_node = 
of_find_node_by_name(tps65910->dev->of_node,
                                                         info->name);
So here config.of_node always shows NULL as the in 
tps65910->dev->of_node does not have name same as info->name ie. 
regulator name like vdd1, vdd2.

So I changed it to pass the node containing"regulators" for 
of_find_node_by_name() and then it got proper of_node like vdd1_reg or 
vdd2_reg as per info_name.

Now this node for vdd1_reg, vdd2_reg, is being passed to 
regulator_register. And for lookup of node, we should use the 
config.of_node which is set.

If we still want to use the parent device for lookup then other way to 
use the config.of_node is to take the parameter of device node in this api.

For this it is require to change the apis as

static struct regulator_dev *regulator_dev_lookup(struct device *dev,
                                                   const char *supply,
                                                   int *ret)

to
static struct regulator_dev *regulator_dev_lookup(struct device *dev, 
struct device_node *dev_node,
                                                   const char *supply,
                                                   int *ret)


and at the time of regulator registration, we should pass config->of_node.
In this way we can still pass parent device and of_node which is passed 
by  regulator driver.
         if (supply) {
                 struct regulator_dev *r;

                 r = regulator_dev_lookup(dev, config->of_node, supply, 
&ret);


> If there is a change needed in the core you need to explain what you
> believe that change will do.
>
I though this is straight but seems it is becoming more complex now.
I will describe all this details if we agree to change require.

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sun, May 20, 2012 at 12:33:32AM +0530, Laxman Dewangan wrote:

> My board dts file is
>       pmu: tps65910@d2 {
>                 compatible = "ti,tps65910";
>                 reg = <0xd2>;
>                 interrupt-parent = <&intc>;
>                 interrupts = < 0 118 0x04 >;
> 
>                 #gpio-cells = <2>;
>                 gpio-controller;
> 
>                 #interrupt-cells = <2>;
>                 interrupt-controller;
> 
>                 regulators {
>                         vdd1_reg: vdd1 {
>                                 regulator-min-microvolt = < 600000>;
>                                 regulator-max-microvolt = <1500000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
>                                 ti,regulator-ext-sleep-control = <0>;
>                         };
>                         vdd2_reg: vdd2 {
>                                 regulator-min-microvolt = < 600000>;
>                                 regulator-max-microvolt = <1500000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
>                                 ti,regulator-ext-sleep-control = <4>;
>                         };
>             };
>     };

Supplies aren't specified for any of the regulators here...

> So currently, when regulator_register gets called in
> tps65910-regulator.c, it sets the
> config.dev = tps65910->dev;
> config.of_node as
>                 config.of_node =
> of_find_node_by_name(tps65910->dev->of_node,
>                                                         info->name);
> So here config.of_node always shows NULL as the in
> tps65910->dev->of_node does not have name same as info->name ie.
> regulator name like vdd1, vdd2.

Of course, this is just like any other supply - since you've not
specified a mapping for it of course the framework isn't able to look it
up.

> >If there is a change needed in the core you need to explain what you
> >believe that change will do.

> I though this is straight but seems it is becoming more complex now.
> I will describe all this details if we agree to change require.

I still don't see any change needed here, from the above it simply looks
like the supplies aren't set up.

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Sunday 20 May 2012 02:20 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Sun, May 20, 2012 at 12:33:32AM +0530, Laxman Dewangan wrote:
>
>> My board dts file is
>>        pmu: tps65910@d2 {
>>                  compatible = "ti,tps65910";
>>                  reg =<0xd2>;
>>                  interrupt-parent =<&intc>;
>>                  interrupts =<  0 118 0x04>;
>>
>>                  #gpio-cells =<2>;
>>                  gpio-controller;
>>
>>                  #interrupt-cells =<2>;
>>                  interrupt-controller;
>>
>>                  regulators {
>>                          vdd1_reg: vdd1 {
>>                                  regulator-min-microvolt =<  600000>;
>>                                  regulator-max-microvolt =<1500000>;
>>                                  regulator-always-on;
>>                                  regulator-boot-on;
>>                                  ti,regulator-ext-sleep-control =<0>;
>>                          };
>>                          vdd2_reg: vdd2 {
>>                                  regulator-min-microvolt =<  600000>;
>>                                  regulator-max-microvolt =<1500000>;
>>                                  regulator-always-on;
>>                                  regulator-boot-on;
>>                                  ti,regulator-ext-sleep-control =<4>;
>>                          };
>>              };
>>      };
> Supplies aren't specified for any of the regulators here...
Sorry, I missed this, I added the supply as
vdd2-supply = <&vdd1_reg>; in the vdd2-reg.


>> So currently, when regulator_register gets called in
>> tps65910-regulator.c, it sets the
>> config.dev = tps65910->dev;
>> config.of_node as
>>                  config.of_node =
>> of_find_node_by_name(tps65910->dev->of_node,
>>                                                          info->name);
>> So here config.of_node always shows NULL as the in
>> tps65910->dev->of_node does not have name same as info->name ie.
>> regulator name like vdd1, vdd2.
> Of course, this is just like any other supply - since you've not
> specified a mapping for it of course the framework isn't able to look it
> up.
>

For mapping, the node should start from "regulators", not from pmu on 
this example.
This is what we already did for regulator match
static struct tps65910_board *tps65910_parse_dt_reg_data(
                                     struct platform_device *pdev)
     {

         struct device_node *np = pdev->dev.parent->of_node;
         struct device_node *regulators;

         regulators = of_find_node_by_name(np, "regulators");
         ret = of_regulator_match(pdev->dev.parent, regulators, matches, 
count);
:::::::::::
}


Here my understanding is that config->of_node should contain the node 
information of the regulator being registered only. In DT case, it 
should not be null.

So here also we need to pass as
            config.of_node = of_find_node_by_name(regulators, info->name);

>>> If there is a change needed in the core you need to explain what you
>>> believe that change will do.
>> I though this is straight but seems it is becoming more complex now.
>> I will describe all this details if we agree to change require.
> I still don't see any change needed here, from the above it simply looks
> like the supplies aren't set up.
Unfortunately,
My regulator_get is failing if I dont correct the above logic to have 
proper config.of_node.

Also regulator registration failed if
- there is supply and if I dont use the config_of_node in lookup after 
fixing the config.of_node issue.

This is happening with pmu (tps65910) regulator only. Fixed regulators 
are working fine here if they dont depends on tps6510.  Input supply for 
fixed regulator from other fixed regulators are also working fine.
Each Fixed regulators are independent platform driver and hence there is 
no issue as dev->of_node is perfectly set/used.

> * Unknown Key
> * 0x6E30FDDD

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sun, May 20, 2012 at 02:43:18AM +0530, Laxman Dewangan wrote:

> For mapping, the node should start from "regulators", not from pmu
> on this example.

What makes you say this?  I'm really not even sure what it means.
How does a node "start" from something?  Supply mappings are direct
links between consumers and regulators.

> Here my understanding is that config->of_node should contain the
> node information of the regulator being registered only. In DT case,
> it should not be null.

Right, but this is unrelated to what we're doing when the regulator is a
consumer.  Then we just do the same thing as regulator_get(dev, name).

> >I still don't see any change needed here, from the above it simply looks
> >like the supplies aren't set up.

> Unfortunately,
> My regulator_get is failing if I dont correct the above logic to
> have proper config.of_node.

But this seems like it is unrelated to the patch we're discussing!  Your
patch does nothing to config.of_node, it changes the device used to look
up the supply.  To repeat yet again:

| context of the class device we create.  I can't think of any situation
| where I'd expect that to make matters any better - the class device
| should certainly never appear in the device tree and isn't going to have
| a stable name for non-DT systems either.

*Please* engage with this, especially the non-DT part.  You need to
explain how what you're saying is related to the patch you posted, you
keep talking about a "proper" config.of_node and saying this happens to
make your system work but this isn't visibily related to the patch you
posted.

What is not "proper" about the of_node that was supplied for the
regulator being registered?  In what way is this related to the device
used by the regulator functioning as a consumer to request a supply?

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Sunday 20 May 2012 04:43 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Sun, May 20, 2012 at 02:43:18AM +0530, Laxman Dewangan wrote:
>
>> For mapping, the node should start from "regulators", not from pmu
>> on this example.
> What makes you say this?  I'm really not even sure what it means.
> How does a node "start" from something?  Supply mappings are direct
> links between consumers and regulators.
>
Sorry for long mail:
This is the issue in the tps65910-regulator.c where config.of_node is 
not being passed correctly.

The flow for my debugging the issue is as follows:

The dts file looks like:

tps65911: tps65911@2d {
     reg = <0x2d>
     #gpio_cells = <2>
     gpio_controller;
     :::::::::
     regulator {
         ldo1_reg: ldo1 {
             ::::::::
             /** regulator entry */
             ::::::::::::
             ::::::::::::
             /* There is NO input supply on this node */
         };
         ldo2_reg: ldo2 {
             ::::::::
             /** regulator entry */
             ::::::::::::
             /* There is NO input supply on this node */
         };
     };
};

Now in the driver, when we register ldo1, the config.of_node should 
contain the node of "ldo1_reg" and when we register ldo2 then 
config.of_node should contain the node of "ldo2_reg".

In the tps65910-regulator.c, the parent device node is containing node 
of "tps65911" i.e. pdev->dev.parent->of_node.
The same is also accessed by tps65910->dev->of_node as tps65910->dev is 
pdev->dev.parent.
By executing the following code in tps65910-regulator.c, ptobe(),
                config.of_node = 
of_find_node_by_name(tps65910->dev->of_node,
                                                         info->name);
is always returning NULL.
This is because the info->name which are "ldo1" or "ldo2" are looked on 
the parent node i.e. pdev->dev.parent->of_node, not inside child node 
"regulator" of pdev->dev.parent->of_node.  The function 
of_find_node_by_name() only looked for props on that node ("tps65911"), 
does not search from child node "regulator".

So for fixing this,
The search for info->name should start from the child node "regulator" 
of the "tps65911" to get proper regulator of_node for for regulator 
being register.
So I first searched for child node "regulator" from parent node 
"tps65911" and then search for info->name ("ldo1" or "ldo2") from this 
child node "regulator":

          struct device_node *np = pdev->dev.parent->of_node;
          struct device_node *regulators;

          regulators = of_find_node_by_name(np, "regulators");
          if (regulators)
          config.of_node = of_find_node_by_name(regulators, info->name);

After fixing this piece of code, regulator_get() from any driver get 
success.

This particular change should be in tps65910-regulator.c file. I fixed 
this in my yesterday's patch 2/5 but lack of explanation on the change 
log, it was unclear.

 From consumer perspecive:

when ldo1_reg get registerd, the config.of_node should contain the node 
handle for ldo1_reg and so it will get stored in rdev->dev.of_node as 
ldo1_reg.
Similarly for ldo2_reg, config.of_node should contain node for ldo2_reg 
and so will get stored in the rdev->dev.of_node as ldo2_reg;

ldo1 and ldo2 are get added in the regulator_list after successfully 
registration for regualtors.

The consumer defines the supply on the dts file as
    xyz_dev: xyz {
     :::::::::::::::
     vmmc-supply = <&ldo1_reg>;
     ::::::::::::
}

consumer driver calls the regulator_get as
regulator_get(dev, "vmmc");

Here dev->of_node contains the node for device" xyz_dev".
The "vmmc-supply" is being searched in xyz_dev and so it founds the 
regulator node as ldo1_reg.
This node get compared from all regulaors's of_node available in 
regulator_list as:
                 node = of_get_regulator(dev, supply);
                 if (node) {
                         list_for_each_entry(r, &regulator_list, list)
                                 if (r->dev.parent &&
                                         node == r->dev.of_node)
                                         return r;
         }

So to get this search success, the r->dev.of_node should contain the 
regulator specific nodes i.e. ldo1_reg or ldo2_reg.
So after fixing the above code,it worked fine.


> | context of the class device we create.  I can't think of any situation
> | where I'd expect that to make matters any better - the class device
> | should certainly never appear in the device tree and isn't going to have
> | a stable name for non-DT systems either.
>
> *Please* engage with this, especially the non-DT part.  You need to
> explain how what you're saying is related to the patch you posted, you
> keep talking about a "proper" config.of_node and saying this happens to
> make your system work but this isn't visibily related to the patch you
> posted.
>
> What is not "proper" about the of_node that was supplied for the
> regulator being registered?  In what way is this related to the device
> used by the regulator functioning as a consumer to request a supply?

This is the issue arise when regulator being registered have the input 
supply.
I am assuming the above fix is there in tps65910-regulator.c.
The dts file looks as

tps65911: tps65911@2d {
     reg = <0x2d>
     #gpio_cells = <2>
     gpio_controller;
     :::::::::
     regulator {
         ldo1_reg: ldo1 {
             ::::::::
             /** regulatr entry */
             ::::::::::::
             ::::::::::::
             /* There is NO input supply on this node */
         };
         ldo2_reg: ldo2 {
             ::::::::
             /** regulatr entry */
             ::::::::::::
             ldo2-supply = <&ldo1_reg>; /* So ldo1 supply the ldo2. */
         };
     };
};

ldo1 registration went fine.
During ldo2 registration, I passed the regulator_desc->supply_name as ldo2.

Here we are passing the config.dev = pdev->dev.parent.
And hence the config.dev.of_node is containing the node of "tps6511".
As I have fixed in tps65911-regulator.c, config.of_node contains the 
node i.e. "ldo1_reg" or "ldo2_reg" for regulator being registered.
In regulator_register(), following piece of code actually fails in the 
case config.of_node is not same as the dev->of_node and in my case it is 
not same. For fixed regulator case it is same and hence it is passing.


         if (supply) {
                 struct regulator_dev *r;

                 r = regulator_dev_lookup(dev, supply, &ret);


Here dev->of_node is containing the node of "tps65911" and so search of 
property "ldo1-supply" failed.

Does following change in core.c make sense to handle the case where 
config->of_node and dev->of_node is not same? Here we still use the dev 
which is passed by config->dev and make use of config->of_node.


-                r = regulator_dev_lookup(dev, supply, &ret);
+               r = regulator_dev_lookup(dev, config->of_node, supply, 
&ret);

  and regulator_dev_lookup() should look for of_node which is passed 
rather than the dev->of_node.

  static struct regulator_dev *regulator_dev_lookup(struct device *dev,
+                         struct device_node *of_node,
                                                    const char *supply,
                                                    int *ret)
  {
          struct regulator_dev *r;
          struct device_node *node;

          /* first do a dt based lookup */
-         if (dev && dev->of_node) {
-                 node = of_get_regulator(dev, supply);
+         if (dev && of_node) {
+                 node = of_get_regulator(dev, of_node, supply);

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

- static struct device_node *of_get_regulator(struct device *dev, const 
char *supply)
+ static struct device_node *of_get_regulator(struct device *dev, struct 
device_node *of_node, const char *supply)
  {
          struct device_node *regnode = NULL;
          char prop_name[32]; /* 32 is max size of property name */

          snprintf(prop_name, 32, "%s-supply", supply);
-         regnode = of_parse_phandle(dev->of_node, prop_name, 0);
+         regnode = of_parse_phandle(of_node, prop_name, 0);
  :::::::::

}

static struct regulator *_regulator_get(struct device *dev, const char *id,
                                         int exclusive)
{
         struct regulator_dev *rdev;
         struct regulator *regulator = ERR_PTR(-EPROBE_DEFER);
         const char *devname = NULL;
+        struct device_node *of_node = NULL;
         int ret;

         if (id == NULL) {
                 pr_err("get() with no identifier\n");
                 return regulator;
         }

         if (dev) {
                 devname = dev_name(dev);
+                of_node = dev->of_node;
     }
         mutex_lock(&regulator_list_mutex);

-        rdev = regulator_dev_lookup(dev, id, &ret);
+        rdev = regulator_dev_lookup(dev, of_node, id, &ret);
         if (rdev)
                 goto found;

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sun, May 20, 2012 at 01:04:05PM +0530, Laxman Dewangan wrote:

> By executing the following code in tps65910-regulator.c, ptobe(),
>                config.of_node =
> of_find_node_by_name(tps65910->dev->of_node,
>                                                         info->name);
> is always returning NULL.

> This is because the info->name which are "ldo1" or "ldo2" are looked
> on the parent node i.e. pdev->dev.parent->of_node, not inside child
> node "regulator" of pdev->dev.parent->of_node.  The function

No.  This is happening because the device tree doesn't have any supplies
mapped for the regulators.  This is nothing at all to do with where the
code looks for the supplies, no matter where it looks there's nothing to
find.

> of_find_node_by_name() only looked for props on that node
> ("tps65911"), does not search from child node "regulator".

This is exactly what I'd expect it to do.  Why would it do anything
different?  Why does it make sense to change the code rather than map
the supplies where the code is currently looking for them?

> So for fixing this,
> The search for info->name should start from the child node
> "regulator" of the "tps65911" to get proper regulator of_node for
> for regulator being register.

You keep saying this but you're still not giving any motivation at all
for making this change.  It's *vital* that you explain why you want to
make this change.  Simply saying that this is the "proper node" over and
over again doesn't do that.

>         ldo2_reg: ldo2 {
>             ::::::::
>             /** regulatr entry */
>             ::::::::::::
>             ldo2-supply = <&ldo1_reg>; /* So ldo1 supply the ldo2. */

This mapping should be moved up to the chip top level; this is just like
any other supply for the chip.

> ldo1 registration went fine.
> During ldo2 registration, I passed the regulator_desc->supply_name as ldo2.

I'd be somewhat surprised if this is what the pin is actually called,
idiomatically the supply name should be whatever the pin is named on the
chip.

> Here we are passing the config.dev = pdev->dev.parent.
> And hence the config.dev.of_node is containing the node of "tps6511".

Of course, what's the problem here?

> Does following change in core.c make sense to handle the case where
> config->of_node and dev->of_node is not same? Here we still use the
> dev which is passed by config->dev and make use of config->of_node.

But *why* do we want to use config->of_node?  This is the bit that's
missing, you keep on repeating that this is the "proper node" over and
over again but you've not been explaining the reasoning behind this.

Supplies for regulators should be no different to supplies for anything
else in the system but you're trying to add a special case for them.
This isn't obviously sensible.  What makes them special?

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sun, May 20, 2012 at 04:09:43PM +0530, Laxman Dewangan wrote:
> On Sunday 20 May 2012 02:31 PM, Mark Brown wrote:

> >No.  This is happening because the device tree doesn't have any supplies
> >mapped for the regulators.  This is nothing at all to do with where the
> >code looks for the supplies, no matter where it looks there's nothing to
> >find.

> No, we should not put the regulator mapping under parent, need to
> have under "regulator" otherwise we need to fix the issue in dt
> parsing where first it looks for "regulator" and then parse the rail
> mapping.

What is this issue and why should we not fix it?

> Now when compare to driver mc13892-regulator.c, the
> tps65910-regulator is almost same like this.
> The driver mc13892-regulator.c have following code in probe:

...

> I want to have similar fix in my tps65910-regulator.c.

So why can't you do what mc13892 is doing?

> I am sorry that I am not able to explain the issue correctly. I think
> I will take help from Stephen Warren here to first explain him  and
> then I will come back for core changes.

OK, I guess.  I think a key thing here is that these shouldn't be any
different to any other supply.  Adding something that is specific to
regulator-regulator supplies doesn't do that so is a clear sign that
something has been missed.

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

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Sunday 20 May 2012 05:36 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Sun, May 20, 2012 at 04:09:43PM +0530, Laxman Dewangan wrote:
>> On Sunday 20 May 2012 02:31 PM, Mark Brown wrote:
>>> No.  This is happening because the device tree doesn't have any supplies
>>> mapped for the regulators.  This is nothing at all to do with where the
>>> code looks for the supplies, no matter where it looks there's nothing to
>>> find.
>> No, we should not put the regulator mapping under parent, need to
>> have under "regulator" otherwise we need to fix the issue in dt
>> parsing where first it looks for "regulator" and then parse the rail
>> mapping.
> What is this issue and why should we not fix it?
>

I will go with the way it is done in mc13892 driver and then it is not 
require to change the node layouts for regulator.

>
>
>> I want to have similar fix in my tps65910-regulator.c.
> So why can't you do what mc13892 is doing?
>

Fine, I will post the similar fix in tps65910-regulator to match with 
the mc13892 regulator driver.
I tested this and it worked fine if changes are done in same way.


>> I am sorry that I am not able to explain the issue correctly. I think
>> I will take help from Stephen Warren here to first explain him  and
>> then I will come back for core changes.
> OK, I guess.  I think a key thing here is that these shouldn't be any
> different to any other supply.  Adding something that is specific to
> regulator-regulator supplies doesn't do that so is a clear sign that
> something has been missed.

I tested by moving the regulator supply to top level as you suggested 
and then core driver change does not needed. So I will add this in dt 
documentation for tps65911 and do some more changes in driver to take 
proper pin name.

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Laxman Dewangan]

On Sunday 20 May 2012 02:31 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Sun, May 20, 2012 at 01:04:05PM +0530, Laxman Dewangan wrote:
>
>>          ldo2_reg: ldo2 {
>>              ::::::::
>>              /** regulatr entry */
>>              ::::::::::::
>>              ldo2-supply =<&ldo1_reg>; /* So ldo1 supply the ldo2. */
> This mapping should be moved up to the chip top level; this is just like
> any other supply for the chip.
>

Ok, After moving this mapping (ldo2-supply = <&ldo1_reg>;) to top level 
under tps65911,  then it worked without core driver changes.
Becasue at this time, the ldo2->desc->supply_name = "ldo2" get find on 
the chip level node and then it return the regulator node properly.


>> ldo1 registration went fine.
>> During ldo2 registration, I passed the regulator_desc->supply_name as ldo2.
> I'd be somewhat surprised if this is what the pin is actually called,
> idiomatically the supply name should be whatever the pin is named on the
> chip.
Yes, I need to add the code in tps65911-regulator.c to use the proper 
pin name as per datasheet for looking for input supply. Like "vcc3" is 
the pin name for ldo6,ldo7 and ldo8 supply and hence look for 
"vcc3-supply" in the chip level rather than "ldo6-supply" or 
"ldo7-supply" etc.
I will post the patch for these changes.

Re: [PATCH] regulator: core: use correct device for device supply lookup

[Mark Brown]

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

On Sat, May 19, 2012 at 10:44:00PM +0530, Laxman Dewangan wrote:
> On Saturday 19 May 2012 10:11 PM, Mark Brown wrote:
> >On Sat, May 19, 2012 at 07:44:06PM +0530, Laxman Dewangan wrote:

> >>When registering the regulator driver, use the rdev->dev for
> >>getting the regulator device of given supply instead of parent
> >>device.

> >You're providing no motivation for this and it's difficult to see how it
> >improves things.  The class device is dynamically numbered so it's not
> >suitable for specifying supplies on a non-DT system and for a DT system
> >it's not obvious to me that we would want to involve the class device in
> >anything, it requires an additional layer of indirection but that's
> >about it.

> If I dont do this then it will not enter in the following case for
> getting the regulator_dev of supply regulator because dev->of_node
> is null, the tps65910-regulator driver have not set the
> pdev->dev.ofnode.

But why, why should it do that and how is this related to your patch?
You're *really* not explaining anything clearly with what you're doing
with device tree...  it's not clear what you're trying to do here or
that you've understood what's there currently.

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