[RFC PATCH] mfd: arizona: Update device tree regulator bindings

[RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Charles Keepax]

The current binding will not be able to locate the regulators DBVDD2,
DBVDD3, CPVDD, SPKVDDL and SPKVDDR as they are required to be present on
the CODEC driver of_node but presently are attached to the MFD of_node.

This patch creates a sub-node for the codec and attaches the required
regulators here using the of_compatible field on the mfd_cell.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
Hi,

Another issue with the Arizona bindings, I am not sure
exactly what the best approach is this patch seems like the
"correct" fix, but it does require a change to the binding,
although the current binding is non-functional.

Alternatively I could simply set the codec of_node to match
the mfd one manually in the codec driver which wouldn't need
the binding changing but feels a bit like a work around.

Any thoughts would be greatly appreciated?

Thanks,
Charles


 Documentation/devicetree/bindings/mfd/arizona.txt |   28 ++++++++++++++++++--
 drivers/mfd/arizona-core.c                        |   15 +++++++++--
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
index aa609a6..49592b2 100644
--- a/Documentation/devicetree/bindings/mfd/arizona.txt
+++ b/Documentation/devicetree/bindings/mfd/arizona.txt
@@ -3,7 +3,7 @@ Wolfson Arizona class audio SoCs
 These devices are audio SoCs with extensive digital capabilites and a range
 of analogue I/O.
 
-Required properties:
+Required properties, nodes:
 
   - compatible : one of the following chip-specific strings:
 	"wlf,wm5102"
@@ -26,10 +26,19 @@ Required properties:
   - #gpio-cells : Must be 2. The first cell is the pin number and the
     second cell is used to specify optional parameters (currently unused).
 
-  - AVDD1-supply, DBVDD1-supply, DBVDD2-supply, DBVDD3-supply, CPVDD-supply,
-    SPKVDDL-supply, SPKVDDR-supply : power supplies for the device, as covered
+  - AVDD-supply, DBVDD1-supply : power supplies for the device, as covered
     in Documentation/devicetree/bindings/regulator/regulator.txt
 
+  - codec {} : Represents the audio part of the chip
+      - compatible : one of the following chip-specific strings:
+          "wlf,wm5102-codec"
+          "wlf,wm5110-codec"
+          "wlf,wm8997-codec"
+
+      - DBVDD2-supply, DBVDD3-supply, CPVDD-supply, SPKVDDL-supply,
+        SPKVDDR-supply : power supplies for the codec, as covered in
+        Documentation/devicetree/bindings/regulator/regulator.txt
+
 Optional properties:
 
   - wlf,reset : GPIO specifier for the GPIO controlling /RESET
@@ -60,4 +69,17 @@ codec: wm5102@1a {
 		0xffffffff,
 		0xffffffff,
 	>;
+
+	AVDD-supply = <&example_reg>;
+	DBVDD1-supply = <&example_reg>;
+
+	codec {
+		compatible = "wlf,wm5102-codec";
+
+		DBVDD2-supply = <&example_reg>;
+		DBVDD3-supply = <&example_reg>;
+		CPVDD-supply = <&example_reg>;
+		SPKVDDL-supply = <&example_reg>;
+		SPKVDDR-supply = <&example_reg>;
+	};
 };
diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 5ac3aa4..10f8ab7 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -575,7 +575,10 @@ static struct mfd_cell wm5102_devs[] = {
 	{ .name = "arizona-gpio" },
 	{ .name = "arizona-haptics" },
 	{ .name = "arizona-pwm" },
-	{ .name = "wm5102-codec" },
+	{
+		.name = "wm5102-codec",
+		.of_compatible = "wlf,wm5102-codec",
+	},
 };
 
 static struct mfd_cell wm5110_devs[] = {
@@ -584,7 +587,10 @@ static struct mfd_cell wm5110_devs[] = {
 	{ .name = "arizona-gpio" },
 	{ .name = "arizona-haptics" },
 	{ .name = "arizona-pwm" },
-	{ .name = "wm5110-codec" },
+	{
+		.name = "wm5110-codec",
+		.of_compatible = "wlf,wm5110-codec",
+	};
 };
 
 static struct mfd_cell wm8997_devs[] = {
@@ -593,7 +599,10 @@ static struct mfd_cell wm8997_devs[] = {
 	{ .name = "arizona-gpio" },
 	{ .name = "arizona-haptics" },
 	{ .name = "arizona-pwm" },
-	{ .name = "wm8997-codec" },
+	{
+		.name = "wm8997-codec",
+		.of_compatible = "wlf,wm8997-codec",
+	},
 };
 
 int arizona_dev_init(struct arizona *arizona)
-- 
1.7.2.5

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Mark Brown]

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

On Wed, Sep 25, 2013 at 06:47:52PM +0100, Charles Keepax wrote:

> This patch creates a sub-node for the codec and attaches the required
> regulators here using the of_compatible field on the mfd_cell.

> Another issue with the Arizona bindings, I am not sure
> exactly what the best approach is this patch seems like the
> "correct" fix, but it does require a change to the binding,
> although the current binding is non-functional.

No, this is not sensible - what you're doing here is clearly a Linux
implementation detail.  

> Alternatively I could simply set the codec of_node to match
> the mfd one manually in the codec driver which wouldn't need
> the binding changing but feels a bit like a work around.

That's not clever either, fiddling around with the data structures like
that is bound to cause referencing problems ("I just freed the device,
let's clean up all the memory") and general fragility.

> Any thoughts would be greatly appreciated?

You need to do something like arrange for the regulator API to resolve
attempts to look up regulators on the subnode using the parent, the
device is virtual after all.

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

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Charles Keepax]

Mark,

What are your thoughts on this approach? I basically add a way to
specify in the DAPM regulator supply widget that the supply lives
on the parent rather than the current device. If you like it I
will send it in as a proper patch.

I had considered adding something like a recursive regulator get
that works it's way up the tree but this felt rather invasive and
most places the user code does the regulator get rather than a
framework so the issue is only really likely to crop up with
regards to ASoC.

Thanks,
Charles
---
 include/sound/soc-dapm.h  |    2 ++
 sound/soc/codecs/wm5102.c |   13 +++++++------
 sound/soc/soc-dapm.c      |    7 ++++++-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 27a72d5..f21b261 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -360,6 +360,8 @@ struct device;
 
 /* regulator widget flags */
 #define SND_SOC_DAPM_REGULATOR_BYPASS     0x1     /* bypass when disabled */
+/* use codec parent device for regulator get */
+#define SND_SOC_DAPM_REGULATOR_USE_PARENT 0x2
 
 struct snd_soc_dapm_widget;
 enum snd_soc_dapm_type;
diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c
index 8bbddc1..ee31f15 100644
--- a/sound/soc/codecs/wm5102.c
+++ b/sound/soc/codecs/wm5102.c
@@ -980,12 +980,13 @@ SND_SOC_DAPM_SUPPLY("OPCLK", ARIZONA_OUTPUT_SYSTEM_CLOCK,
 SND_SOC_DAPM_SUPPLY("ASYNCOPCLK", ARIZONA_OUTPUT_ASYNC_CLOCK,
 		    ARIZONA_OPCLK_ASYNC_ENA_SHIFT, 0, NULL, 0),
 
-SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD2", 0, 0),
-SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD3", 0, 0),
-SND_SOC_DAPM_REGULATOR_SUPPLY("CPVDD", 20, 0),
-SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
-SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDL", 0, 0),
-SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDR", 0, 0),
+SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD2", 0, SND_SOC_DAPM_REGULATOR_USE_PARENT),
+SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD3", 0, SND_SOC_DAPM_REGULATOR_USE_PARENT),
+SND_SOC_DAPM_REGULATOR_SUPPLY("CPVDD", 20, SND_SOC_DAPM_REGULATOR_USE_PARENT),
+SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS |
+					   SND_SOC_DAPM_REGULATOR_USE_PARENT),
+SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDL", 0, SND_SOC_DAPM_REGULATOR_USE_PARENT),
+SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDR", 0, SND_SOC_DAPM_REGULATOR_USE_PARENT),
 
 SND_SOC_DAPM_SIGGEN("TONE"),
 SND_SOC_DAPM_SIGGEN("NOISE"),
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index c17c14c..899bd2e 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3181,7 +3181,12 @@ snd_soc_dapm_new_control(struct snd_soc_dapm_context *dapm,
 
 	switch (w->id) {
 	case snd_soc_dapm_regulator_supply:
-		w->regulator = devm_regulator_get(dapm->dev, w->name);
+		if (w->on_val & SND_SOC_DAPM_REGULATOR_USE_PARENT)
+			w->regulator = devm_regulator_get(dapm->dev->parent,
+							  w->name);
+		else
+			w->regulator = devm_regulator_get(dapm->dev, w->name);
+
 		if (IS_ERR(w->regulator)) {
 			ret = PTR_ERR(w->regulator);
 			dev_err(dapm->dev, "ASoC: Failed to request %s: %d\n",
-- 
1.7.2.5

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Mark Brown]

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

On Thu, Sep 26, 2013 at 04:29:41PM +0100, Charles Keepax wrote:

Please include some context in replies...

> I had considered adding something like a recursive regulator get
> that works it's way up the tree but this felt rather invasive and
> most places the user code does the regulator get rather than a
> framework so the issue is only really likely to crop up with
> regards to ASoC.

I don't see why this would only be an issue for ASoC - it happens to
have more helpers for this right now than anything else but I'd hope
that at some point in the future we can get some of the common patterns
for holding regulators into the device framework.  You also have the
potential for this to do the wrong thing if it doesn't happen to be
talking to an MFD which is doing this since it just unconditionally
looks at the parent.

I would suggest that rather than unconditionally doing this on lookup
it'd be easier to do it the other way round and explicitly add mappings
from the parent to the child when registering the device.  That doesn't
have the potential to explode and get the wrong thing like this does.

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

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Charles Keepax]

On Thu, Sep 26, 2013 at 06:58:43PM +0100, Mark Brown wrote:
> I don't see why this would only be an issue for ASoC - it happens to
> have more helpers for this right now than anything else but I'd hope
> that at some point in the future we can get some of the common patterns
> for holding regulators into the device framework.  You also have the
> potential for this to do the wrong thing if it doesn't happen to be
> talking to an MFD which is doing this since it just unconditionally
> looks at the parent.
> 
> I would suggest that rather than unconditionally doing this on lookup
> it'd be easier to do it the other way round and explicitly add mappings
> from the parent to the child when registering the device.  That doesn't
> have the potential to explode and get the wrong thing like this does.

Been a bit delayed on this as I have been investigating the
situation a little further. Is it just moving the supplies of the
parent node we object to? Or do we want to keep everything tidied
onto the one device tree node? The reason I ask is because
looking at this further we run into other problems, the MFD
framework is clearly intended to have a sub-node for each device.

For example, the GPIO driver has a similar issue if anything else
wishes to use an Arizona devices GPIO, because the GPIO driver
is on a different device to the MFD so again it can't locate it.
I haven't checked yet but I am guessing there will be similar
issues with the interrupts.

Thanks,
Charles

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Mark Brown]

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

On Sat, Sep 28, 2013 at 04:53:08PM +0100, Charles Keepax wrote:

> > I don't see why this would only be an issue for ASoC - it happens to

> > I would suggest that rather than unconditionally doing this on lookup
> > it'd be easier to do it the other way round and explicitly add mappings
> > from the parent to the child when registering the device.  That doesn't
> > have the potential to explode and get the wrong thing like this does.

> Been a bit delayed on this as I have been investigating the
> situation a little further. Is it just moving the supplies of the
> parent node we object to? Or do we want to keep everything tidied

No, having the supplies bound to the parent is desired (especially given
that there isn't a child node) - it's the fact that you're bodging this
in the framework by just randomly peering at the parent device and
hoping it's an MFD parent when a lookup fails.  That's not a safe thing
to do.  

Like I said in the quote above trying to handle this in the child isn't
a good approach, it's both more idiomatic and more robust to put the
mappings from the parent device to the child devices in when creating
the child devices.

> onto the one device tree node? The reason I ask is because

Yes, for a device like this.

> looking at this further we run into other problems, the MFD
> framework is clearly intended to have a sub-node for each device.

No it isn't, see recent discussions on the list and look at the existing
drivers.  This is supported because for some devices it makes sense to
split things up but it's not something that needs to be used for every
device, it's useful when for example a device embeds a reusable generic
IP.  If you're ending up with the subdevices defined with the same level
of granularity as the parent device then that's not happening, and
remember that for these devices you do also need to support non-DT
systems.

> For example, the GPIO driver has a similar issue if anything else
> wishes to use an Arizona devices GPIO, because the GPIO driver
> is on a different device to the MFD so again it can't locate it.
> I haven't checked yet but I am guessing there will be similar
> issues with the interrupts.

No, this isn't an issue at all.  Look at how the regulator API resolves
DT lookups for example, the structure of the driver offering the service
should have no impact on anything referencing it.  The fact that Linux
happens to split things up into a particular set of subsystems at the
current time should have no bearing on the way that the DT bindings are
written since that's just a detail of how Linux works.

Linux (or any other operating system) can always decide to refactor
later on, something we take advantage of relatively often.  For example
with these devices the clocks are currently described as part of the
audio CODEC but which should probably in future move over to the clock
API once that's generally available since they're shared within the
device and can be brought out of the device to other system components.
The device tree for a system using the device shouldn't need to change
when that happens since nothing about the hardware changed.

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

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Charles Keepax]

On Sat, Sep 28, 2013 at 11:55:35PM +0100, Mark Brown wrote:
> No, having the supplies bound to the parent is desired (especially given
> that there isn't a child node) - it's the fact that you're bodging this
> in the framework by just randomly peering at the parent device and
> hoping it's an MFD parent when a lookup fails.  That's not a safe thing
> to do.  
> 
> Like I said in the quote above trying to handle this in the child isn't
> a good approach, it's both more idiomatic and more robust to put the
> mappings from the parent device to the child devices in when creating
> the child devices.

Apologies I didn't mean to convey I was still pushing for the
above patch. I am just trying to get a good handle on what needs
to be done here, and thought I best clarify about the gpio
functionality as it is starting to look like changes in quite a
few places and I am wondering if there is a better more central
place to handle the issue.

> > For example, the GPIO driver has a similar issue if anything else
> > wishes to use an Arizona devices GPIO, because the GPIO driver
> > is on a different device to the MFD so again it can't locate it.
> > I haven't checked yet but I am guessing there will be similar
> > issues with the interrupts.
> 
> No, this isn't an issue at all.  Look at how the regulator API resolves
> DT lookups for example, the structure of the driver offering the service
> should have no impact on anything referencing it.  The fact that Linux
> happens to split things up into a particular set of subsystems at the
> current time should have no bearing on the way that the DT bindings are
> written since that's just a detail of how Linux works.

There is currently only one other MFD driver (tps65910) which defines
the GPIOs on the main MFD node as we do in the Arizona driver and it
uses the 'hack' that I suggested in my first email of copying the
of_node.

tps65910_gpio->gpio_chip.of_node = tps65910->dev->of_node;

Looking around there seem to be quite a few drivers that copy
of_node pointers like this are we sure this isn't an acceptable
solution? I mean the arizona driver we know the components will
always be loaded as part of the MFD and thus will be freed before
the parent node.

Thanks,
Charles

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Mark Brown]

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

On Sun, Sep 29, 2013 at 03:11:37PM +0100, Charles Keepax wrote:

> There is currently only one other MFD driver (tps65910) which defines
> the GPIOs on the main MFD node as we do in the Arizona driver and it
> uses the 'hack' that I suggested in my first email of copying the
> of_node.

> tps65910_gpio->gpio_chip.of_node = tps65910->dev->of_node;

Look where it's copying it to - this isn't the hack you were suggesting
before.  What you were suggesting before was setting the of_node of the
child struct device which is a managed thing that the driver core knows
about, the device is the owner of the of_node hanging off it.  You would
end up with the same of_node referenced from two struct devices which
isn't clever.  The above is putting a pointer into the gpio_chip which
is just telling gpiolib that it should use this of_node when it needs
one, gpiolib won't try and lifecycle manage the of_node.

> Looking around there seem to be quite a few drivers that copy
> of_node pointers like this are we sure this isn't an acceptable
> solution? I mean the arizona driver we know the components will
> always be loaded as part of the MFD and thus will be freed before
> the parent node.

It's fine to reference an of_node, that's obviously something that will
need to happen at some point in order to do anything useful with the
data.

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

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Charles Keepax]

On Sun, Sep 29, 2013 at 06:52:36PM +0100, Mark Brown wrote:
> On Sun, Sep 29, 2013 at 03:11:37PM +0100, Charles Keepax wrote:
> 
> > There is currently only one other MFD driver (tps65910) which defines
> > the GPIOs on the main MFD node as we do in the Arizona driver and it
> > uses the 'hack' that I suggested in my first email of copying the
> > of_node.
> 
> > tps65910_gpio->gpio_chip.of_node = tps65910->dev->of_node;
> 
> Look where it's copying it to - this isn't the hack you were suggesting
> before.  What you were suggesting before was setting the of_node of the
> child struct device which is a managed thing that the driver core knows
> about, the device is the owner of the of_node hanging off it.  You would
> end up with the same of_node referenced from two struct devices which
> isn't clever.  The above is putting a pointer into the gpio_chip which
> is just telling gpiolib that it should use this of_node when it needs
> one, gpiolib won't try and lifecycle manage the of_node.

Ah, of course, I see now.


Thanks,
Charles

Re: [RFC PATCH] mfd: arizona: Update device tree regulator bindings

[Lee Jones]

Always a good idea to CC MFD maintainers on MFD patches. :)

-- 
Lee Jones
Linaro ST Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog