linux-kernel - Re: [PATCH] regulator: Support different config and dev of_nodes in regulator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54DB8F9C.1070104@sonymobile.com>
Date:	Wed, 11 Feb 2015 09:21:32 -0800
From:	Tim Bird <tim.bird@...ymobile.com>
To:	Mark Brown <broonie@...nel.org>,
	"Andersson, Björn" 
	<Bjorn.Andersson@...ymobile.com>
CC:	"lgirdwood@...il.com" <lgirdwood@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: Support different config and dev of_nodes
 in regulator_register

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/