Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register

[Tim Bird <tim.bird@sonymobile.com>]

On 02/06/2015 11:56 AM, Tim Bird wrote:
> 
> 
> On 02/06/2015 03:49 AM, Mark Brown wrote:
>> On Thu, Feb 05, 2015 at 04:52:40PM -0800, Bjorn Andersson wrote:
>>> On Thu 05 Feb 16:32 PST 2015, Mark Brown wrote:
>>>> On Thu, Feb 05, 2015 at 02:08:54PM -0800, Bjorn Andersson wrote:
>>
>>>>> However this only works for the non-supply regulator properties - and
>>>>> this is where Tim's patch is trying to sort out.
>>
>>>> No, this works completely fine for supply properties - to repeat what I
>>>> said in reply to the original patch the supply is a supply to the chip
>>>> not to an individual IP on the chip.
>>
>>> It does make some sense to consider the vbus-supply being connected to
>>> the block, rather than directly to the vbus-switch. So it would work for
>>> Tim's use case.
>>
>> Like I say if we do that then we don't have consistency in how we map a
>> schematic into a DT binding - you have to dig into the binding of each
>> device and figure out if the supply is viewed as being for blocks or for
>> the chip as a whole and we've got the potential for problems in the
>> binding if we figure out that a supply is actually used by other blocks
>> later on and don't want to break existing DTs.
> 
> OK - the light bulb finally went on for me on this one.
> So a chip can have multiple supplies (I saw examples of this 
> poking around in other source), and the details of
> internal routing in the chip don't have to be expressed in
> DT at all (in fact shouldn't, for the reason you mention).
> 
> Thanks - I will implement along these lines.

Mark,

Just to follow up on this, I got things working with the current code
to my satisfaction.  I ended up just punting on having multiple
regulators come from the charger block.  Instead I expose a single regulator
from the charger driver, and just put all regulator attributes, including
the supply reference, directly in the charger DT block.
And I gave the charger block a label I could reference by phandle in the USB code.

I wanted to describe the difficulties I had, just for your
consideration.  Maybe something can be done (either in doc or in
code), to help others avoid my pain.

My problem was in assuming that I could have a regulator
with dev.of_node different from config.of_node.

The definition of of_get_regulator_init_data() implies that this
is OK, because it accepts both a struct device and a struct device_node.

Also, when registering a regulator, you can pass a different
of_node in config (struct regulator_config) than the one in dev
(struct device)

However, this has problems in the current code, as the test in
regulator_dev_lookup requires that the device_node found by of_get_regulator()
match the dev.of_node in the regulator in the regulator list.

See this code in regulator_dev_lookup():

         node = of_get_regulator(dev, NULL, supply);
         if (node) {
                  list_for_each_entry(r, &regulator_list, list)
                          if (r->dev.parent &&
                                  node == r->dev.of_node)
                                  return r;


It took me a while to figure this out, because a regulator defined
with dev.of_node != config.of_node worked fine, when accessed
directly by name (not using a supply-name).  I only had problems
when I accessed the regulator using the "-supply" indirection technique
in DT.

In other words, using my previous DT configuration, I could do this:
   reg = regulator_get(dev, "chg_otg");
and everything would work fine, but if I did this:
(in dt)    vbus-supply = <&chg_otg>;
   reg = regulator_get(dev, "vbus");
would not work.  (And using the "-supply" indirection
in DT worked for other regulators).

Anyway, this caused me some confusion.  If it's required to 
have dev.of_node == config.of_node, than maybe this field should
be removed from struct regulator_config, and a separate device_node
arg not be passed to of_get_regulator_init_data().

Just my 2 cents.

Thanks,
 -- Tim