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

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]

Date:	Sun, 20 May 2012 10:01:12 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	"lrg@...com" <lrg@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: core: use correct device for device supply
 lookup

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?

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)