We can actually read this back from the device but we use this when
registered using standard I2C board data registration so make sure
it's there for OF too.
Signed-off-by: Mark Brown <[email protected]>
---
Untested at present.
drivers/mfd/wm8994-core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
index cdea84e..3f8d591 100644
--- a/drivers/mfd/wm8994-core.c
+++ b/drivers/mfd/wm8994-core.c
@@ -673,9 +673,9 @@ static void wm8994_device_exit(struct wm8994 *wm8994)
}
static const struct of_device_id wm8994_of_match[] = {
- { .compatible = "wlf,wm1811", },
- { .compatible = "wlf,wm8994", },
- { .compatible = "wlf,wm8958", },
+ { .compatible = "wlf,wm1811", .data = (void *)WM1811 },
+ { .compatible = "wlf,wm8994", .data = (void *)WM8994 },
+ { .compatible = "wlf,wm8958", .data = (void *)WM8958 },
{ }
};
MODULE_DEVICE_TABLE(of, wm8994_of_match);
--
1.7.10.4
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 <[email protected]>
---
Untested at present.
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
+ 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
+ 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.
+
+ - 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
+
Example:
codec: wm8994@1a {
compatible = "wlf,wm8994";
reg = <0x1a>;
+
+ gpio-controoler;
+ #gpio-cells = <2>;
+
+ lineout1-se;
+
+ AVDD2-supply = <®ulator>;
+ CPVDD-supply = <®ulator>;
+ DBVDD1-supply = <®ulator>;
+ DBVDD2-supply = <®ulator>;
+ DBVDD3-supply = <®ulator>;
+ SPKVDD1-supply = <®ulator>;
+ SPKVDD2-supply = <®ulator>;
};
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;
+ } else if (pdata->gpio_defaults[i] == 0xffffffff) {
+ pdata->gpio_defaults[i] = 0;
+ } else if (pdata->gpio_defaults[i] > 0xffff) {
+ 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;
+
+ 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;
+}
+#else
+static int wm8994_set_pdata_from_of(struct wm8994 *wm8994)
+{
+ return 0;
+}
+#endif
+
/*
* Instantiate the generic non-control parts of the device.
*/
@@ -414,6 +479,10 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq)
}
pdata = &wm8994->pdata;
+ ret = wm8994_set_pdata_from_of(wm8994);
+ if (ret != 0)
+ return ret;
+
dev_set_drvdata(wm8994->dev, wm8994);
/* Add the on-chip regulators first for bootstrapping */
@@ -683,6 +752,7 @@ MODULE_DEVICE_TABLE(of, wm8994_of_match);
static int wm8994_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
+ const struct of_device_id *of_id;
struct wm8994 *wm8994;
int ret;
@@ -693,7 +763,14 @@ static int wm8994_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, wm8994);
wm8994->dev = &i2c->dev;
wm8994->irq = i2c->irq;
- wm8994->type = id->driver_data;
+
+ if (i2c->dev.of_node) {
+ of_id = of_match_device(wm8994_of_match, &i2c->dev);
+ if (of_id)
+ wm8994->type = (int)of_id->data;
+ } else {
+ wm8994->type = id->driver_data;
+ }
wm8994->regmap = devm_regmap_init_i2c(i2c, &wm8994_base_regmap_config);
if (IS_ERR(wm8994->regmap)) {
@@ -733,7 +810,7 @@ static struct i2c_driver wm8994_i2c_driver = {
.name = "wm8994",
.owner = THIS_MODULE,
.pm = &wm8994_pm_ops,
- .of_match_table = wm8994_of_match,
+ .of_match_table = of_match_ptr(wm8994_of_match),
},
.probe = wm8994_i2c_probe,
.remove = wm8994_i2c_remove,
--
1.7.10.4
On 04/10/2013 04:39 PM, Mark Brown wrote:
> We can actually read this back from the device but we use this when
> registered using standard I2C board data registration so make sure
> it's there for OF too.
>
> Signed-off-by: Mark Brown <[email protected]>
> ---
>
> Untested at present.
I have tested this with wm1811 codec, together with the second patch.
The device type was set correctly.
FWIW, Tested-by: Sylwester Nawrocki <[email protected]>
> drivers/mfd/wm8994-core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
> index cdea84e..3f8d591 100644
> --- a/drivers/mfd/wm8994-core.c
> +++ b/drivers/mfd/wm8994-core.c
> @@ -673,9 +673,9 @@ static void wm8994_device_exit(struct wm8994 *wm8994)
> }
>
> static const struct of_device_id wm8994_of_match[] = {
> - { .compatible = "wlf,wm1811", },
> - { .compatible = "wlf,wm8994", },
> - { .compatible = "wlf,wm8958", },
> + { .compatible = "wlf,wm1811", .data = (void *)WM1811 },
> + { .compatible = "wlf,wm8994", .data = (void *)WM8994 },
> + { .compatible = "wlf,wm8958", .data = (void *)WM8958 },
> { }
> };
> MODULE_DEVICE_TABLE(of, wm8994_of_match);
Thanks,
Sylwester
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 <[email protected]>
> ---
>
> 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 = <®ulator>;
> + CPVDD-supply = <®ulator>;
> + DBVDD1-supply = <®ulator>;
> + DBVDD2-supply = <®ulator>;
> + DBVDD3-supply = <®ulator>;
> + SPKVDD1-supply = <®ulator>;
> + SPKVDD2-supply = <®ulator>;
> };
> 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
On Thu, Apr 11, 2013 at 03:38:20PM +0200, Sylwester Nawrocki wrote:
> On 04/10/2013 04:39 PM, Mark Brown wrote:
> > Untested at present.
> I've tested it with wm1811 codec and found a few issues/things that are
> a bit confusing to me.
Wasn't kidding about the lack of testing!
> > + - 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.
Since all designs I've ever seen for this chip use the internal
regulators in a consistent fashion I've added support for setting up the
standard hookup by default so users don't need to specify this by hand
since that wound up being repetitive and boring. This is in -next or
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git topic/wm8994
and should show up in v3.10. The above list includes the supplies for
LDO1 and LDO2. If someone wants to support other configurations the
binding can always be extended with optional properties.
> 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.
No, it's not - it's the above regulator driver changes, those are the
supplies normally supplied by the internal regulators.
> > + - 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.
That'll be a cut'n'paste.
> > + - 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.
For the DT bindings it seemed simplest to assume the default setup so
the regulator driver ought to just do the right thing. If someone has
done something different then you're right and we need to add code for
this.
> > + - ldoena-always-driven : If present LDOENA is always driven
> I suppose the custom properties should be prefixed with "wlf,".
Meh, yeah. Will fix.
> > + 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.
This is an implementation detail due to conversion to platform data.
Since the idiom for platform data is that zero does nothing for platform
data we made the user set an out of range bit (the registers are 16 bit)
to set zero, otherwise we left the value untouched. The result is that
we rewrite to 0x10000 in the platform data which is then rewritten to
zero.
> > + } 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.
Yes, bitrot due to cut'n'paste from two different sources. Will fix.
> 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");
> ?
Yes, I tend to prefer to write things out a bit more explicitly.
On 04/11/2013 04:21 PM, Mark Brown wrote:
> On Thu, Apr 11, 2013 at 03:38:20PM +0200, Sylwester Nawrocki wrote:
>> On 04/10/2013 04:39 PM, Mark Brown wrote:
>
>>> Untested at present.
>
>> I've tested it with wm1811 codec and found a few issues/things that are
>> a bit confusing to me.
>
> Wasn't kidding about the lack of testing!
Anyway it worked pretty well! Just needed to add patches for the regulator
you pointed out and that small patch:
--8<-----------
>From e306f6683d17c3f6aafaae44395548f4703382c8 Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <[email protected]>
Date: Thu, 11 Apr 2013 17:42:51 +0200
Subject: [PATCH] regulator: wm8994: Use GPIO parsed from DT when registering
regulators
Ensure the regulators are registered with a GPIO parsed from the device
tree when available.
Signed-off-by: Sylwester Nawrocki <[email protected]>
---
drivers/regulator/wm8994-regulator.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/wm8994-regulator.c
b/drivers/regulator/wm8994-regulator.c
index 086be66..dab41ae 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -149,9 +149,11 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
config.init_data = &ldo->init_data;
if (pdata)
config.ena_gpio = pdata->ldo[id].enable;
+ else if (wm8994->dev->of_node)
+ config.ena_gpio = wm8994->pdata.ldo[id].enable;
/* Use default constraints if none set up */
- if (!pdata || !pdata->ldo[id].init_data) {
+ if (!pdata || !pdata->ldo[id].init_data || wm8994->dev->of_node) {
dev_dbg(wm8994->dev, "Using default init data, supply %s %s\n",
ldo->supply.dev_name, ldo->supply.supply);
--
1.7.9.5
--8<-----------
If needed I can resend it to you. Or do suggest please anything more relevant.
>>> + - 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.
>
> Since all designs I've ever seen for this chip use the internal
> regulators in a consistent fashion I've added support for setting up the
> standard hookup by default so users don't need to specify this by hand
> since that wound up being repetitive and boring. This is in -next or
>
> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git topic/wm8994
>
> and should show up in v3.10. The above list includes the supplies for
> LDO1 and LDO2. If someone wants to support other configurations the
> binding can always be extended with optional properties.
So far I didn't need anything more than this.
>> 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.
>
> No, it's not - it's the above regulator driver changes, those are the
> supplies normally supplied by the internal regulators.
Ok, I just removed the above two supplies after applying
"regulator: wm8994: Provide default init data".
>>> + 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.
>
> This is an implementation detail due to conversion to platform data.
> Since the idiom for platform data is that zero does nothing for platform
> data we made the user set an out of range bit (the registers are 16 bit)
> to set zero, otherwise we left the value untouched. The result is that
> we rewrite to 0x10000 in the platform data which is then rewritten to
> zero.
Alright, I should have really checked what happens further to gpio_defaults[].
Now, when sound more or less works I need to fix up the irq and the gpio
chip registration.
On Thu, Apr 11, 2013 at 06:17:32PM +0200, Sylwester Nawrocki wrote:
> Anyway it worked pretty well! Just needed to add patches for the regulator
> you pointed out and that small patch:
Ah, great - just redoing the bindings just now for your other comments,
hopefully I won't break anything in the process. Can you post that as a
normal patch for git am please (or I can fish it out by hand)?
Ensure the regulators are registered with a GPIO parsed from the device
tree when available.
Signed-off-by: Sylwester Nawrocki <[email protected]>
---
drivers/regulator/wm8994-regulator.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index 086be66..dab41ae 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -149,9 +149,11 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
config.init_data = &ldo->init_data;
if (pdata)
config.ena_gpio = pdata->ldo[id].enable;
+ else if (wm8994->dev->of_node)
+ config.ena_gpio = wm8994->pdata.ldo[id].enable;
/* Use default constraints if none set up */
- if (!pdata || !pdata->ldo[id].init_data) {
+ if (!pdata || !pdata->ldo[id].init_data || wm8994->dev->of_node) {
dev_dbg(wm8994->dev, "Using default init data, supply %s %s\n",
ldo->supply.dev_name, ldo->supply.supply);
--
1.7.9.5
On Thu, Apr 11, 2013 at 06:36:57PM +0200, Sylwester Nawrocki wrote:
> Ensure the regulators are registered with a GPIO parsed from the device
> tree when available.
Applied, thanks. Should be done more elegantly but I can't think of
something obvious right now. Thanks for testing.