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 <broo...@opensource.wolfsonmicro.com> > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/