Message-ID: <54DB8F9C.1070104@sonymobile.com>
Date: Wed, 11 Feb 2015 09:21:32 -0800
From: Tim Bird <tim.bird@sonymobile.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0
MIME-Version: 1.0
To: Mark Brown <broonie@kernel.org>,
        =?windows-1252?Q?=22Andersson=2C_B?= =?windows-1252?Q?j=F6rn=22?= 
	<Bjorn.Andersson@sonymobile.com>
CC: "lgirdwood@gmail.com" <lgirdwood@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] regulator: Support different config and dev of_nodes
 in regulator_register
References: <54D2A91D.5090700@sonymobile.com> <20150205015945.GQ21293@sirena.org.uk> <54D3A96B.2020704@sonymobile.com> <20150205174353.GA21293@sirena.org.uk> <54D3B858.9060103@sonymobile.com> <20150205192740.GL21293@sirena.org.uk> <20150205220844.GA3218@sonymobile.com> <20150206003212.GR21293@sirena.org.uk> <20150206005239.GD3218@sonymobile.com> <20150206114953.GT21293@sirena.org.uk> <54D51C5D.7050708@sonymobile.com>
In-Reply-To: <54D51C5D.7050708@sonymobile.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4205
Lines: 101


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/