LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] regulator: Support different config and dev of_nodes in regulator_register
@ 2015-02-04 23:19 Tim Bird
  2015-02-05  1:59 ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2015-02-04 23:19 UTC (permalink / raw)
  To: broonie, lgirdwood; +Cc: linux-kernel, Bjorn Andersson

Support calling regulator_register with a dev node and a config node
with different of_nodes.  This is useful when a single driver
wishes to register multiple child regulators.

Without this you get silent failures allocating a supply
for a regulator which is registered using the device node of the
regulator's DT parent (but it's own DT node).

Signed-off-by: Tim Bird <tim.bird@sonymobile.com>
---
I encountered a problem where I did a devm_regulator_register with
a dev (for a charger driver) and a config (for the regulator child
of the driver) that had different of_nodes.  In this case, inside
regulator_dev_lookup the code did not find the supply that I had
specified in the child regulator's DT node.  The DT setup looked
as follows:

charger@1000 {
      compatible = "qcom,pm8941-charger";
      reg = <0x1000 0x700>;
      ....
      chg_otg {
          regulator_name = "chg_otg";
          otg-supply = <&pm8941_mvs1>;
          ...
      }
}

This code checks the of_node of specified in the struct regulator_config
if the supply is not found in the dev.of_node.  This code has no effect
if dev.of_node and config.of_node are the same, so it shouldn't affect
any existing (working) code.

 drivers/regulator/core.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 128b432..36c5d78 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -128,13 +128,16 @@ static bool have_full_constraints(void)
 /**
  * of_get_regulator - get a regulator device node based on supply name
  * @dev: Device pointer for the consumer (of regulator) device
+ * @config: pointer to config for regulator registration
  * @supply: regulator supply name
  *
  * Extract the regulator device node corresponding to the supply name.
  * returns the device node corresponding to the regulator if found, else
  * returns NULL.
  */
-static struct device_node *of_get_regulator(struct device *dev, const char *supply)
+static struct device_node *of_get_regulator(struct device *dev,
+				const struct regulator_config *config,
+				const char *supply)
 {
 	struct device_node *regnode = NULL;
 	char prop_name[32]; /* 32 is max size of property name */
@@ -147,7 +150,15 @@ static struct device_node *of_get_regulator(struct device *dev, const char *supp
 	if (!regnode) {
 		dev_dbg(dev, "Looking up %s property in node %s failed",
 				prop_name, dev->of_node->full_name);
-		return NULL;
+		if (!config || !config->of_node ||
+				config->of_node == dev->of_node)
+			return NULL;
+		regnode = of_parse_phandle(config->of_node, prop_name, 0);
+		if (!regnode) {
+			dev_dbg(dev, "Looking up %s property in node %s failed",
+				prop_name, dev->of_node->full_name);
+			return NULL;
+		}
 	}
 	return regnode;
 }
@@ -1284,9 +1295,10 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
 	}
 }
 
-static struct regulator_dev *regulator_dev_lookup(struct device *dev,
-						  const char *supply,
-						  int *ret)
+static struct regulator_dev *regulator_dev_config_lookup(struct device *dev,
+					const struct regulator_config *config,
+					const char *supply,
+					int *ret)
 {
 	struct regulator_dev *r;
 	struct device_node *node;
@@ -1297,7 +1309,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 	/* first do a dt based lookup */
 	if (dev && dev->of_node) {
-		node = of_get_regulator(dev, supply);
+		node = of_get_regulator(dev, config, supply);
 		if (node) {
 			list_for_each_entry(r, &regulator_list, list)
 				if (r->dev.parent &&
@@ -1334,7 +1346,6 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 			return map->regulator;
 	}
 
-
 	return NULL;
 }
 
@@ -1362,7 +1373,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 
 	mutex_lock(&regulator_list_mutex);
 
-	rdev = regulator_dev_lookup(dev, id, &ret);
+	rdev = regulator_dev_config_lookup(dev, NULL, id, &ret);
 	if (rdev)
 		goto found;
 
@@ -3642,7 +3653,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_config_lookup(dev, config, supply, &ret);
 
 		if (ret == -ENODEV) {
 			/*
-- 
1.8.2.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-04 23:19 [PATCH] regulator: Support different config and dev of_nodes in regulator_register Tim Bird
@ 2015-02-05  1:59 ` Mark Brown
  2015-02-05 17:33   ` Tim Bird
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-02-05  1:59 UTC (permalink / raw)
  To: Tim Bird; +Cc: lgirdwood, linux-kernel, Bjorn Andersson

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

On Wed, Feb 04, 2015 at 03:19:57PM -0800, Tim Bird wrote:

> Support calling regulator_register with a dev node and a config node
> with different of_nodes.  This is useful when a single driver
> wishes to register multiple child regulators.

> Without this you get silent failures allocating a supply
> for a regulator which is registered using the device node of the
> regulator's DT parent (but it's own DT node).

This is explicitly not supported; such bindings are invariably attempts
to encode the Linux MFD structure into the device tree (which isn't a
wonderful idea as the way we split things into subsystems can and does
change) or...

> charger@1000 {
>       compatible = "qcom,pm8941-charger";
>       reg = <0x1000 0x700>;
>       ....
>       chg_otg {
>           regulator_name = "chg_otg";
>           otg-supply = <&pm8941_mvs1>;
>           ...
>       }
> }

...this which just looks like the supply has been placed in the wrong
place, it should be in the parent node.  Supplies are always defined at
the package level, that way we can consistently define the bindings for
supplies for a device without having to completely support it and we
don't have to bind the same supply multiple times.  It should really be
possible to wire up the supplies based only on the schematics.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-05  1:59 ` Mark Brown
@ 2015-02-05 17:33   ` Tim Bird
  2015-02-05 17:43     ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2015-02-05 17:33 UTC (permalink / raw)
  To: Mark Brown; +Cc: lgirdwood, linux-kernel, "Andersson, Björn"



On 02/04/2015 05:59 PM, Mark Brown wrote:
> On Wed, Feb 04, 2015 at 03:19:57PM -0800, Tim Bird wrote:
> 
>> Support calling regulator_register with a dev node and a config node
>> with different of_nodes.  This is useful when a single driver
>> wishes to register multiple child regulators.
> 
>> Without this you get silent failures allocating a supply
>> for a regulator which is registered using the device node of the
>> regulator's DT parent (but it's own DT node).
> 
> This is explicitly not supported; such bindings are invariably attempts
> to encode the Linux MFD structure into the device tree (which isn't a
> wonderful idea as the way we split things into subsystems can and does
> change) or...

Sorry - what is the "Linux MFD structure"?

> 
>> charger@1000 {
>>       compatible = "qcom,pm8941-charger";
>>       reg = <0x1000 0x700>;
>>       ....
>>       chg_otg {
>>           regulator_name = "chg_otg";
>>           otg-supply = <&pm8941_mvs1>;
>>           ...
>>       }
>> }
> 
> ...this which just looks like the supply has been placed in the wrong
> place, it should be in the parent node.  Supplies are always defined at
> the package level, that way we can consistently define the bindings for
> supplies for a device without having to completely support it and we
> don't have to bind the same supply multiple times.  It should really be
> possible to wire up the supplies based only on the schematics.
> 
Well, the above DT node is not complete.  Let me give some more
context.  I eventually want to have the charger driver support 2
regulators - one for the OTG vbus output (shown above) and one
for a boost hardware device, which controls voltage for this and
other parts of the system.  These are both for IP blocks that are
in the register range of this charger hardware (and hence belong, IMHO
in this driver).  I can easily move the otg-supply to the charger DT node,
as you request, but what do I do about other regulator attributes,
if I need to specify them for both the chg_otg and boost regulators
provided by this driver? How would this be handled?  I can't put them
all in the charger DT node.

Thanks,
 -- Tim



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-05 17:33   ` Tim Bird
@ 2015-02-05 17:43     ` Mark Brown
  2015-02-05 18:37       ` Tim Bird
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-02-05 17:43 UTC (permalink / raw)
  To: Tim Bird; +Cc: lgirdwood, linux-kernel, "Andersson, Björn"

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

On Thu, Feb 05, 2015 at 09:33:31AM -0800, Tim Bird wrote:
> On 02/04/2015 05:59 PM, Mark Brown wrote:

> > This is explicitly not supported; such bindings are invariably attempts
> > to encode the Linux MFD structure into the device tree (which isn't a
> > wonderful idea as the way we split things into subsystems can and does
> > change) or...

> Sorry - what is the "Linux MFD structure"?

The way we split things up into subsystems via drivers/mfd.  Our set of
subsystems is neither fixed nor authorative.

> Well, the above DT node is not complete.  Let me give some more
> context.  I eventually want to have the charger driver support 2
> regulators - one for the OTG vbus output (shown above) and one
> for a boost hardware device, which controls voltage for this and
> other parts of the system.  These are both for IP blocks that are
> in the register range of this charger hardware (and hence belong, IMHO
> in this driver).  I can easily move the otg-supply to the charger DT node,
> as you request, but what do I do about other regulator attributes,
> if I need to specify them for both the chg_otg and boost regulators
> provided by this driver? How would this be handled?  I can't put them
> all in the charger DT node.

This just sounds like a standard multi-regulator PMIC - usually the
nodes for all the regulators end up getting stuffed in a node (typically
called regulators) which the core can then interate over for you.  I'm
just not seeing what's unusual about this device, sorry.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-05 17:43     ` Mark Brown
@ 2015-02-05 18:37       ` Tim Bird
  2015-02-05 19:27         ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2015-02-05 18:37 UTC (permalink / raw)
  To: Mark Brown; +Cc: lgirdwood, linux-kernel, "Andersson, Björn"



On 02/05/2015 09:43 AM, Mark Brown wrote:
> On Thu, Feb 05, 2015 at 09:33:31AM -0800, Tim Bird wrote:
>> On 02/04/2015 05:59 PM, Mark Brown wrote:
> 
>>> This is explicitly not supported; such bindings are invariably attempts
>>> to encode the Linux MFD structure into the device tree (which isn't a
>>> wonderful idea as the way we split things into subsystems can and does
>>> change) or...
> 
>> Sorry - what is the "Linux MFD structure"?
> 
> The way we split things up into subsystems via drivers/mfd.  Our set of
> subsystems is neither fixed nor authorative.

I'm not doing anything in drivers/mfd?  Should I be?
The charger driver is currently in drivers/power, but should it be
moved to drivers/mfd if it's going to expose regulators as
well as power supplies?  I'm sorry, but I'm not following
your point here.  I associated this regulator with the charger
driver because that's where the hardware for it is.  I'm not really
familiar with the complete driver sub-system layout of Linux.
This was not a (conscious) attempt to encode anything about Linux
into the device tree.  I'm just trying to get the dang supply
to hook up to the regulator node.

> 
>> Well, the above DT node is not complete.  Let me give some more
>> context.  I eventually want to have the charger driver support 2
>> regulators - one for the OTG vbus output (shown above) and one
>> for a boost hardware device, which controls voltage for this and
>> other parts of the system.  These are both for IP blocks that are
>> in the register range of this charger hardware (and hence belong, IMHO
>> in this driver).  I can easily move the otg-supply to the charger DT node,
>> as you request, but what do I do about other regulator attributes,
>> if I need to specify them for both the chg_otg and boost regulators
>> provided by this driver? How would this be handled?  I can't put them
>> all in the charger DT node.
> 
> This just sounds like a standard multi-regulator PMIC - usually the
> nodes for all the regulators end up getting stuffed in a node (typically
> called regulators) which the core can then interate over for you.  I'm
> just not seeing what's unusual about this device, sorry.

Thanks for helping me out here...  I apologize if these are newbie
questions as I haven't worked with regulators before.

So you're saying I should have a "regulators" child node of the charger
node, and then define the chg_otg and boost regulators under that, each
with it's own compatible string, so that the DT code can instantiate
all the proper device nodes, of_nodes, regulator attributes, etc.
Or is this instantiation something I do manually in the charger probe
routine?  (That's what I'm doing now, but open coding each regulator
individually.)

How does the core know to iterate over regulators?  Is this something
automatic in the DT code, or something I have to trigger from the
charger probe routine?

As an aside, will each regulator have to have it's own probe routine,
and be capable of probe-deferring?  That is, if I separate the charger
probe from the regulator probes, it seems like I'll have to worry
about probe ordering, as well as manually establishing the linkage
between all these device nodes somehow during probing so the charger
enable/disable routines will be able to get access to the charger
register space.

Is there any way, if I'm open-coding a regulator, to just specify the
supply after it's instantiated?

Can you recommend a driver to look at that does (properly) what
you're describing?

Thanks. (And thanks for your patience),
 -- Tim


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-05 18:37       ` Tim Bird
@ 2015-02-05 19:27         ` Mark Brown
  2015-02-05 22:08           ` Bjorn Andersson
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-02-05 19:27 UTC (permalink / raw)
  To: Tim Bird; +Cc: lgirdwood, linux-kernel, "Andersson, Björn"

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

On Thu, Feb 05, 2015 at 10:37:12AM -0800, Tim Bird wrote:
> On 02/05/2015 09:43 AM, Mark Brown wrote:

> >> Sorry - what is the "Linux MFD structure"?

> > The way we split things up into subsystems via drivers/mfd.  Our set of
> > subsystems is neither fixed nor authorative.

> I'm not doing anything in drivers/mfd?  Should I be?
> The charger driver is currently in drivers/power, but should it be
> moved to drivers/mfd if it's going to expose regulators as
> well as power supplies?  I'm sorry, but I'm not following
> your point here.  I associated this regulator with the charger

Possibly not.  My reply was explaining the sorts of breakage that
allowing the DT node to be overridden is usually intended to support,
the sort of thoughtless bindings for MFDs that I'm describing is one
typical example.

> So you're saying I should have a "regulators" child node of the charger
> node, and then define the chg_otg and boost regulators under that, each
> with it's own compatible string, so that the DT code can instantiate

No, absolutely not - you should not need to put compatible strings for
individual regulators within a single device in the device tree.  Please
take a look at how other devices do this - there are plenty of bindings
for existing devices in tree with matching code in the kernel.

> Or is this instantiation something I do manually in the charger probe
> routine?  (That's what I'm doing now, but open coding each regulator
> individually.)

Given the way you're talking separately about there being a charger
driver and a regulator driver here it sounds like you should be creating
a MFD.  The MFD subsystem exists to provide a way of mapping a single
physical device into multiple kernel subsystems which sounds like it
will be what you're trying to do here.

> Can you recommend a driver to look at that does (properly) what
> you're describing?

Most of drivers/mfd is PMICs doing this, anything recently added should
be reasonable to look at.  Most of the Maxim or Wolfson devices for
example.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-05 19:27         ` Mark Brown
@ 2015-02-05 22:08           ` Bjorn Andersson
  2015-02-06  0:32             ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2015-02-05 22:08 UTC (permalink / raw)
  To: Mark Brown; +Cc: Bird, Tim, lgirdwood, linux-kernel

On Thu 05 Feb 11:27 PST 2015, Mark Brown wrote:

> On Thu, Feb 05, 2015 at 10:37:12AM -0800, Tim Bird wrote:
> > On 02/05/2015 09:43 AM, Mark Brown wrote:
> 
> > >> Sorry - what is the "Linux MFD structure"?
> 
> > > The way we split things up into subsystems via drivers/mfd.  Our set of
> > > subsystems is neither fixed nor authorative.
> 
> > I'm not doing anything in drivers/mfd?  Should I be?
> > The charger driver is currently in drivers/power, but should it be
> > moved to drivers/mfd if it's going to expose regulators as
> > well as power supplies?  I'm sorry, but I'm not following
> > your point here.  I associated this regulator with the charger
> 
> Possibly not.  My reply was explaining the sorts of breakage that
> allowing the DT node to be overridden is usually intended to support,
> the sort of thoughtless bindings for MFDs that I'm describing is one
> typical example.
> 

The charging block have 1 bit that serves as a vbus-switch, when the USB
code detects that we're in host mode and should supply power on VBUS
we need to enable the switch.

This is why we suggested Tim to implement this as a regulator from the
charger driver itself.

> > So you're saying I should have a "regulators" child node of the charger
> > node, and then define the chg_otg and boost regulators under that, each
> > with it's own compatible string, so that the DT code can instantiate
> 
> No, absolutely not - you should not need to put compatible strings for
> individual regulators within a single device in the device tree.  Please
> take a look at how other devices do this - there are plenty of bindings
> for existing devices in tree with matching code in the kernel.
> 

As Tim mentioned, we have (with different naming):

charger {
	compatible = "awesome,charger";
	vbus: vbus {
		vbus-supply = <&upstream_switch>;
	};
};

vbus is not a separate device in any form, it's just one aspect of the
block exposed through a different subsystem (than power). The vbus node
groups regulator properties related to the vbus regulator (and gives us
a possibility to reference it).

Your statement and the code indicates that it is okay to register a
regulator with of_node being different from the device's of_node itself.


However this only works for the non-supply regulator properties - and
this is where Tim's patch is trying to sort out.

I interpreted your first answer as it being incorrect usage to have
dev->of_node != config->of_node, but I'm not so sure anymore.

If it is invalid then we should drop config->of_node and use
dev->of_node in regulator_register, if it's valid we should have all the
standard regulator properties read from the same of_node (i.e. fix
supply processing in regulator_register).

(I have some style comments on the original patch, but would like to
know if we're going the wrong way here or not)

> > Or is this instantiation something I do manually in the charger probe
> > routine?  (That's what I'm doing now, but open coding each regulator
> > individually.)
> 
> Given the way you're talking separately about there being a charger
> driver and a regulator driver here it sounds like you should be creating
> a MFD.  The MFD subsystem exists to provide a way of mapping a single
> physical device into multiple kernel subsystems which sounds like it
> will be what you're trying to do here.
> 

It's a charger block that contains a switch for powering the VBUS line,
it's one hardware block.

> > Can you recommend a driver to look at that does (properly) what
> > you're describing?
> 
> Most of drivers/mfd is PMICs doing this, anything recently added should
> be reasonable to look at.  Most of the Maxim or Wolfson devices for
> example.

Correct, but those are separate hardware blocks (although same chip),
that's not what we have here.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-05 22:08           ` Bjorn Andersson
@ 2015-02-06  0:32             ` Mark Brown
  2015-02-06  0:52               ` Bjorn Andersson
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-02-06  0:32 UTC (permalink / raw)
  To: Bjorn Andersson; +Cc: Bird, Tim, lgirdwood, linux-kernel

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

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-06  0:32             ` Mark Brown
@ 2015-02-06  0:52               ` Bjorn Andersson
  2015-02-06 11:49                 ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2015-02-06  0:52 UTC (permalink / raw)
  To: Mark Brown; +Cc: Bird, Tim, lgirdwood, linux-kernel

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.

Thanks for repeating yourself and sorry about that.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-06  0:52               ` Bjorn Andersson
@ 2015-02-06 11:49                 ` Mark Brown
  2015-02-06 19:56                   ` Tim Bird
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-02-06 11:49 UTC (permalink / raw)
  To: Bjorn Andersson; +Cc: Bird, Tim, lgirdwood, linux-kernel

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

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-06 11:49                 ` Mark Brown
@ 2015-02-06 19:56                   ` Tim Bird
  2015-02-11 17:21                     ` Tim Bird
  0 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2015-02-06 19:56 UTC (permalink / raw)
  To: Mark Brown, "Andersson, Björn"; +Cc: lgirdwood, linux-kernel



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.
 -- Tim




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-06 19:56                   ` Tim Bird
@ 2015-02-11 17:21                     ` Tim Bird
  2015-02-12  2:32                       ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2015-02-11 17:21 UTC (permalink / raw)
  To: Mark Brown, "Andersson, Björn"; +Cc: lgirdwood, linux-kernel



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register
  2015-02-11 17:21                     ` Tim Bird
@ 2015-02-12  2:32                       ` Mark Brown
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2015-02-12  2:32 UTC (permalink / raw)
  To: Tim Bird; +Cc: "Andersson, Björn", lgirdwood, linux-kernel

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

On Wed, Feb 11, 2015 at 09:21:32AM -0800, Tim Bird wrote:

> 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.

...

> 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.

I'm not quite sure exactly what you've done here but I'm pretty sure
there's some confusion somewhere since the -supply based lookups are of
course what everything in mainline is doing except for CPU supplies.
The fact that you are using of_get_regulator_init_data() is a bit of a
red flag here, you shouldn't have to use that unless your bindings are
in some way unusual.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-02-12  2:33 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-04 23:19 [PATCH] regulator: Support different config and dev of_nodes in regulator_register Tim Bird
2015-02-05  1:59 ` Mark Brown
2015-02-05 17:33   ` Tim Bird
2015-02-05 17:43     ` Mark Brown
2015-02-05 18:37       ` Tim Bird
2015-02-05 19:27         ` Mark Brown
2015-02-05 22:08           ` Bjorn Andersson
2015-02-06  0:32             ` Mark Brown
2015-02-06  0:52               ` Bjorn Andersson
2015-02-06 11:49                 ` Mark Brown
2015-02-06 19:56                   ` Tim Bird
2015-02-11 17:21                     ` Tim Bird
2015-02-12  2:32                       ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).