Re: [PATCH 2/2] mfd: wm8994: Add some OF properties

From: Sylwester Nawrocki
Date: Thu Apr 11 2013 - 09:38:29 EST

Mark,

On 04/10/2013 04:39 PM, Mark Brown wrote:
> Add properties for some of the more important bits of platform data and
> fill out the binding document.
>
> Not all of the current platform data is suitable for the sort of fixed
> configuration that is done using DT, some of it should have runtime
> mechanisms added instead and some is unlikely to ever be used in practical
> systems.
>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> ---
>
> Untested at present.

I've tested it with wm1811 codec and found a few issues/things that are
a bit confusing to me.

> Documentation/devicetree/bindings/sound/wm8994.txt | 56 +++++++++++++-
> drivers/mfd/wm8994-core.c | 81 +++++++++++++++++++-
> 2 files changed, 134 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/wm8994.txt b/Documentation/devicetree/bindings/sound/wm8994.txt
> index 7a7eb1e..51edc5f 100644
> --- a/Documentation/devicetree/bindings/sound/wm8994.txt
> +++ b/Documentation/devicetree/bindings/sound/wm8994.txt
> @@ -5,14 +5,68 @@ on the board).
>
> Required properties:
>
> - - compatible : "wlf,wm1811", "wlf,wm8994", "wlf,wm8958"
> + - compatible : One of "wlf,wm1811", "wlf,wm8994" or "wlf,wm8958"
>
> - reg : the I2C address of the device for I2C, the chip select
> number for SPI.
>
> + - gpio-controller : Indicates this device is a GPIO controller.
> + - #gpio-cells : Must be 2. The first cell is the pin number and the
> + second cell is used to specify optional parameters (currently unused).
> +
> + - AVDD2-supply, DBVDD1-supply, DBVDD2-supply, DBVDD3-supply, CPVDD-supply,
> + SPKVDD1-supply, SPKVDD2-supply : power supplies for the device, as covered

These capitalized regulator supply names look like standard ones. Sorry,
I'm quite new to all this ASoC stuff. I was wondering, what about LDO1, LDO2
regulators that are present in the wm1811 chip for instance ? Are those
regulators supposed to be associated with some of the supply names above ?

AFAICS LDO1, LDO2 need to be enabled before we can actually perform any I2C
communication.

Besides that, I needed to specify following regulator supplies in order to
to get the wm8994 codec successfully initialized:

DCVDD-supply
AVDD1-supply

That might be something specific to the sound machine driver though, I'm not
certain.

> + in Documentation/devicetree/bindings/regulator/regulator.txt
> +
> +Optional properties:
> +
> + - interrupts : The interrupt line the IRQ signal for the device is
> + connected to. This is optional, if it is not connected then none
> + of the interrupt related properties should be specified.
> + - interrupt-controller : These devices contain interrupt controllers
> + and may provide interrupt services to other devices if they have an
> + interrupt line connected.
> + - interrupt-parent : The parent interrupt controller.
> + - #interrupt-cells: the number of cells to describe an IRQ, this should be 2.
> + The first cell is the IRQ number.
> + The second cell is the flags, encoded as the trigger masks from
> + Documentation/devicetree/bindings/interrupts.txt
> +
> + - gpio-cfg : A list of GPIO configuration register values. If absent,
> + no configuration of these registers is performed. If any value is
> + over 0xffff then the register will be left as default. If present 11
> + values must be supplied.
> +
> + - micbias-cfg : Three MICBIAS register values for WM1811 or

Aren't there just 2 MICBIAS registers ? At least I've found the wm8994 driver
handles only 2 values.

> + WM8958. If absent the register defaults will be used.
> +
> + - ldo1ena : GPIO specifier for control of LDO1ENA input to device.
> + - ldo2ena : GPIO specifier for control of LDO2ENA input to device.

Hmm, don't we need to specify the constraints for the regulators as well ?
AFAICS for wm8994 you choose not to specify functions of this MFD as child
DT nodes.

I might be missing something, but to make the codec working I have defined
regulator as child node of this mfd device node, i.e.

i2c@138A0000 {
...
wm1811: wm1811@1a {
compatible = "wlf,wm1811";
reg = <0x1a>;
interrupt-parent = <&gpx3>;
interrupts = <6 4>;

gpio-cfg = <0x3 0x0 0x0 0x0
0x0 0x0 0x0 0x8000
0x0 0x0 0x0>;
micbias-cfg = <0x2f 0x2b>;

lineout2-feedback;
lineout1-se;
lineout2-se;

AVDD2-supply = <&vbatt_reg>;
DBVDD1-supply = <&ldo3_reg>;
DBVDD2-supply = <&wm1811_ldo1_reg>;
DBVDD3-supply = <&vbatt_reg>;
CPVDD-supply = <&vbatt_reg>;
SPKVDD1-supply = <&vbatt_reg>;
SPKVDD2-supply = <&vbatt_reg>;
DCVDD-supply = <&vbatt_reg>;
AVDD1-supply = <&vbatt_reg>;

wm1811_ldo1_reg: ldo1 {
compatible = "wlf,wm8994-ldo";

regulator-compatible = "LDO1";
regulator-name = "WM1811_LDO1";
gpio = <&gpj0 4 0>;
regulator-always-on;
};
};
};

Then in wm8994_ldo_probe() I made a change as below:

if (pdata) {
config.init_data = pdata->ldo[id].init_data;
config.ena_gpio = pdata->ldo[id].enable;
- }
+ } else {
+ config.init_data = of_get_regulator_init_data(dev, dev->of_node);
+ config.ena_gpio = of_get_named_gpio(dev->of_node, "gpio", 0);
+ config.of_node = dev->of_node;
+ }


Is there any other way to get the LDO1/LDO2 regulators properly registered
and enabled before any I2C communication is done with the device ?

platform_data (wm8994->dev->platform_data) in wm8994_ldo_probe() is NULL
when booting from DT. I guess something like this could be done, but then
how to associate the voltage regulators with the codec ?

---8<--------------
diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index 1a63261..0235148 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -119,6 +119,9 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
int ret;

+ if (pdev->dev.of_node)
+ pdata = &wm8994->pdata;
+
ldo = devm_kzalloc(&pdev->dev, sizeof(struct wm8994_ldo), GFP_KERNEL);
if (ldo == NULL) {
dev_err(&pdev->dev, "Unable to allocate private data\n");
---8<--------------

The "only" issue I had was that there are 2 wm8994-ldo mfd cells, and for
each of them the mfd core iterated over all wm1811 child nodes, trying
to register each regulator twice. So I temporarily removed one entry from
the wm8994_regulator_devs array.

> + - lineout1-se : If present LINEOUT1 is in single ended mode.
> + - lineout2-se : If present LINEOUT2 is in single ended mode.
> +
> + - lineout1-feedback : If present LINEOUT1 has common mode feedback connected.
> + - lineout2-feedback : If present LINEOUT2 has common mode feedback connected.
> +
> + - ldoena-always-driven : If present LDOENA is always driven

I suppose the custom properties should be prefixed with "wlf,".

> Example:
>
> codec: wm8994@1a {
> compatible = "wlf,wm8994";
> reg = <0x1a>;
> +
> + gpio-controoler;

s/controoler/controller ?

> + #gpio-cells = <2>;
> +
> + lineout1-se;
> +
> + AVDD2-supply = <&regulator>;
> + CPVDD-supply = <&regulator>;
> + DBVDD1-supply = <&regulator>;
> + DBVDD2-supply = <&regulator>;
> + DBVDD3-supply = <&regulator>;
> + SPKVDD1-supply = <&regulator>;
> + SPKVDD2-supply = <&regulator>;
> };
> diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
> index 3f8d591..3f87f25 100644
> --- a/drivers/mfd/wm8994-core.c
> +++ b/drivers/mfd/wm8994-core.c
> @@ -19,6 +19,9 @@
> #include <linux/err.h>
> #include <linux/delay.h>
> #include <linux/mfd/core.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> #include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> @@ -396,6 +399,68 @@ static const struct reg_default wm1811_reva_patch[] = {
> { 0x102, 0x0 },
> };
>
> +#ifdef CONFIG_OF
> +static int wm8994_set_pdata_from_of(struct wm8994 *wm8994)
> +{
> + struct device_node *np = wm8994->dev->of_node;
> + struct wm8994_pdata *pdata = &wm8994->pdata;
> + int i;
> +
> + if (!np)
> + return 0;
> +
> + if (of_property_read_u32_array(np, "gpio-cfg", pdata->gpio_defaults,
> + ARRAY_SIZE(pdata->gpio_defaults)) >= 0) {
> + for (i = 0; i < ARRAY_SIZE(pdata->gpio_defaults); i++) {
> + if (wm8994->pdata.gpio_defaults[i] == 0) {
> + pdata->gpio_defaults[i]
> + = WM8994_CONFIGURE_GPIO;

Hmm, that's not obvious from the binding, that 0 is replaced with 0x10000
by the implementation.

> + } else if (pdata->gpio_defaults[i] == 0xffffffff) {
> + pdata->gpio_defaults[i] = 0;
> + } else if (pdata->gpio_defaults[i] > 0xffff) {

And this is not specified as an error condition at the binding. I expected
in such case the default value of a register would be used.

> + dev_err(wm8994->dev,
> + "Invalid gpio-cfg[%d] %x\n",
> + i, pdata->gpio_defaults[i]);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + of_property_read_u32_array(np, "micbias-cfg", pdata->micbias,
> + ARRAY_SIZE(pdata->micbias));
> +
> + pdata->lineout1_diff = true;
> + pdata->lineout2_diff = true;
> + if (of_find_property(np, "lineout1-se", NULL))
> + pdata->lineout1_diff = false;
> + if (of_find_property(np, "lineout2-se", NULL))
> + pdata->lineout2_diff = false;

I guess you preferred it that way, rather than

pdata->lineout1_diff = !of_property_read_bool(np, "lineout1-se");
pdata->lineout2_diff = !of_property_read_bool(np, "lineout2-se");

?
> + if (of_find_property(np, "lineout1-feedback", NULL))
> + pdata->lineout1fb = true;
> + if (of_find_property(np, "lineout2-feedback", NULL))
> + pdata->lineout2fb = true;
> +
> + if (of_find_property(np, "ldoena-always-driven", NULL))
> + pdata->lineout2fb = true;
> +
> + pdata->ldo[0].enable = of_get_named_gpio(np, "ldo1ena", 0);
> + if (pdata->ldo[0].enable < 0)
> + pdata->ldo[0].enable = 0;
> +
> + pdata->ldo[1].enable = of_get_named_gpio(np, "ldo2ena", 0);
> + if (pdata->ldo[1].enable < 0)
> + pdata->ldo[1].enable = 0;
> +
> + return 0;
> +}


Thanks,
Sylwester

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