[alsa-devel] [PATCH v2 0/5] ASoC: WM8903: DT bindings & related
v2: * Fixed default pdata logic so that 0 in pdata fields really means "do nothing". * Added a patch to fix pdata gpio_cfg value confusion. * Moved all the pdata default & DT parsing logic to i2c_probe from codec probe. * Split parsing logic out into separate functions so probe is easier to read. * Rebased on top of the WM8903 cleanup patches Mark posted today.
Stephen Warren (5): ASoC: WM8903: Fix platform data gpio_cfg confusion ASoC: WM8903: Create default platform data structure ASoC: WM8903: Remove conditionals checking pdata != NULL ASoC: WM8903: Get default irq_active_low from IRQ controller ASoC: WM8903: Add device tree binding
Documentation/devicetree/bindings/sound/wm8903.txt | 50 ++++++ arch/arm/mach-tegra/board-harmony.c | 8 +- arch/arm/mach-tegra/board-seaboard.c | 8 +- include/sound/wm8903.h | 7 +- sound/soc/codecs/wm8903.c | 181 +++++++++++++++----- 5 files changed, 205 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/wm8903.txt
wm8903_platform_data.gpio_cfg[] was intended to be interpreted as follows: 0: Don't touch this GPIO's configuration register 1..7fff: Write that value to the GPIO's configuration register 8000: Write zero to the GPIO's configuration register other: Undefined (invalid)
The rationale is that platform data is usually global data, and a value of zero means that the field wasn't explicitly set to anything (e.g. because the field was new to the pdata type, and existing users weren't update to initialize it) and hence the value zero should be ignored. 0x8000 is an explicit way to get 0 in the register.
The code worked this way until commit 7cfe561 "ASoC: wm8903: Expose GPIOs through gpiolib", where the behaviour was changed due to my lack of awareness of the above rationale.
This patch reverts to the intended behaviour, and updates all in-tree users to use the correct scheme. This also makes WM8903 consistent with other devices that use a similar scheme.
WM8903_GPIO_NO_CONFIG is also renamed to WM8903_GPIO_CONFIG_ZERO so that its name accurately reflects its purpose.
Signed-off-by: Stephen Warren swarren@nvidia.com Cc: Olof Johansson olof@lixom.net Cc: Colin Cross ccross@android.com --- Olof, Colin, could you please ack this so that Mark can apply it to the ASoC tree even though it touches Tegra code? Thanks.
arch/arm/mach-tegra/board-harmony.c | 8 ++++---- arch/arm/mach-tegra/board-seaboard.c | 8 ++++---- include/sound/wm8903.h | 7 +++++-- sound/soc/codecs/wm8903.c | 3 ++- 4 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c index 60afd08..a37c11c 100644 --- a/arch/arm/mach-tegra/board-harmony.c +++ b/arch/arm/mach-tegra/board-harmony.c @@ -90,11 +90,11 @@ static struct wm8903_platform_data harmony_wm8903_pdata = { .micdet_delay = 100, .gpio_base = HARMONY_GPIO_WM8903(0), .gpio_cfg = { - WM8903_GPIO_NO_CONFIG, - WM8903_GPIO_NO_CONFIG, 0, - WM8903_GPIO_NO_CONFIG, - WM8903_GPIO_NO_CONFIG, + 0, + WM8903_GPIO_CONFIG_ZERO, + 0, + 0, }, };
diff --git a/arch/arm/mach-tegra/board-seaboard.c b/arch/arm/mach-tegra/board-seaboard.c index ce3c9a2..b8ddf53 100644 --- a/arch/arm/mach-tegra/board-seaboard.c +++ b/arch/arm/mach-tegra/board-seaboard.c @@ -171,11 +171,11 @@ static struct wm8903_platform_data wm8903_pdata = { .micdet_delay = 100, .gpio_base = SEABOARD_GPIO_WM8903(0), .gpio_cfg = { - WM8903_GPIO_NO_CONFIG, - WM8903_GPIO_NO_CONFIG, 0, - WM8903_GPIO_NO_CONFIG, - WM8903_GPIO_NO_CONFIG, + 0, + WM8903_GPIO_CONFIG_ZERO, + 0, + 0, }, };
diff --git a/include/sound/wm8903.h b/include/sound/wm8903.h index cf7ccb7..b310c5a 100644 --- a/include/sound/wm8903.h +++ b/include/sound/wm8903.h @@ -11,8 +11,11 @@ #ifndef __LINUX_SND_WM8903_H #define __LINUX_SND_WM8903_H
-/* Used to enable configuration of a GPIO to all zeros */ -#define WM8903_GPIO_NO_CONFIG 0x8000 +/* + * Used to enable configuration of a GPIO to all zeros; a gpio_cfg value of + * zero in platform data means "don't touch this pin". + */ +#define WM8903_GPIO_CONFIG_ZERO 0x8000
/* * R6 (0x06) - Mic Bias Control 0 diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index a3e5620..3070a94 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1892,7 +1892,8 @@ static int wm8903_probe(struct snd_soc_codec *codec) bool mic_gpio = false;
for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) { - if (pdata->gpio_cfg[i] > 0x7fff) + if ((!pdata->gpio_cfg[i]) || + (pdata->gpio_cfg[i] > WM8903_GPIO_CONFIG_ZERO)) continue;
snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i,
On Fri, Dec 02, 2011 at 03:08:37PM -0700, Stephen Warren wrote:
wm8903_platform_data.gpio_cfg[] was intended to be interpreted as follows: 0: Don't touch this GPIO's configuration register 1..7fff: Write that value to the GPIO's configuration register 8000: Write zero to the GPIO's configuration register other: Undefined (invalid)
Applied this and all the rest, thanks.
On Fri, Dec 02, 2011 at 03:08:37PM -0700, Stephen Warren wrote:
wm8903_platform_data.gpio_cfg[] was intended to be interpreted as follows: 0: Don't touch this GPIO's configuration register 1..7fff: Write that value to the GPIO's configuration register 8000: Write zero to the GPIO's configuration register other: Undefined (invalid)
The rationale is that platform data is usually global data, and a value of zero means that the field wasn't explicitly set to anything (e.g. because the field was new to the pdata type, and existing users weren't update to initialize it) and hence the value zero should be ignored. 0x8000 is an explicit way to get 0 in the register.
The code worked this way until commit 7cfe561 "ASoC: wm8903: Expose GPIOs through gpiolib", where the behaviour was changed due to my lack of awareness of the above rationale.
This patch reverts to the intended behaviour, and updates all in-tree users to use the correct scheme. This also makes WM8903 consistent with other devices that use a similar scheme.
WM8903_GPIO_NO_CONFIG is also renamed to WM8903_GPIO_CONFIG_ZERO so that its name accurately reflects its purpose.
Signed-off-by: Stephen Warren swarren@nvidia.com Cc: Olof Johansson olof@lixom.net Cc: Colin Cross ccross@android.com
Olof, Colin, could you please ack this so that Mark can apply it to the ASoC tree even though it touches Tegra code? Thanks.
Mark, since Stephen is a tegra maintainer, there's no real need to have an explicit ack from one of the others, IMHO. But here you have it. :)
Acked-by: Olof Johansson olof@lixom.net
-Olof
On Wed, Dec 07, 2011 at 03:49:27PM -0800, Olof Johansson wrote:
On Fri, Dec 02, 2011 at 03:08:37PM -0700, Stephen Warren wrote:
Olof, Colin, could you please ack this so that Mark can apply it to the ASoC tree even though it touches Tegra code? Thanks.
Mark, since Stephen is a tegra maintainer, there's no real need to have an explicit ack from one of the others, IMHO. But here you have it. :)
Acked-by: Olof Johansson olof@lixom.net
Too late, I already applied :) Even if Stephen weren't a maintainer I'd probably have gone aheady anyway as it's a trivial platform data change.
When no platform data is supplied, point pdata at a default platform structure. This enables two future changes:
a) Defines the default platform data values in a single place. b) There is always a valid pdata pointer, so some conditional code can be simplified by a later patch.
Based on work by John Bonesio, but significantly reworked since then.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/codecs/wm8903.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 3070a94..9fd89be 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -114,6 +114,7 @@ static const struct reg_default wm8903_reg_defaults[] = { };
struct wm8903_priv { + struct wm8903_platform_data *pdata; struct snd_soc_codec *codec; struct regmap *regmap;
@@ -1834,7 +1835,7 @@ static struct gpio_chip wm8903_template_chip = { static void wm8903_init_gpio(struct snd_soc_codec *codec) { struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec); - struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev); + struct wm8903_platform_data *pdata = wm8903->pdata; int ret;
wm8903->gpio_chip = wm8903_template_chip; @@ -1872,8 +1873,8 @@ static void wm8903_free_gpio(struct snd_soc_codec *codec)
static int wm8903_probe(struct snd_soc_codec *codec) { - struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev); struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec); + struct wm8903_platform_data *pdata = wm8903->pdata; int ret, i; int trigger, irq_pol; u16 val; @@ -2040,6 +2041,7 @@ static const struct regmap_config wm8903_regmap = { static __devinit int wm8903_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { + struct wm8903_platform_data *pdata = dev_get_platdata(&i2c->dev); struct wm8903_priv *wm8903; unsigned int val; int ret; @@ -2060,6 +2062,19 @@ static __devinit int wm8903_i2c_probe(struct i2c_client *i2c, i2c_set_clientdata(i2c, wm8903); wm8903->irq = i2c->irq;
+ /* If no platform data was supplied, create storage for defaults */ + if (pdata) { + wm8903->pdata = pdata; + } else { + wm8903->pdata = devm_kzalloc(&i2c->dev, + sizeof(struct wm8903_platform_data), + GFP_KERNEL); + if (wm8903->pdata == NULL) { + dev_err(&i2c->dev, "Failed to allocate pdata\n"); + return -ENOMEM; + } + } + ret = regmap_read(wm8903->regmap, WM8903_SW_RESET_AND_ID, &val); if (ret != 0) { dev_err(&i2c->dev, "Failed to read chip ID: %d\n", ret);
On Fri, Dec 02, 2011 at 03:08:38PM -0700, Stephen Warren wrote:
When no platform data is supplied, point pdata at a default platform structure. This enables two future changes:
Applied, thanks. The rest are OK too but I'll leave them for a little while for the Tegra guys (though the changes are straightforward enough so I'm not sure how long I'll wait).
The pdata pointer is now always valid. Remove any conditions that check its validity.
This patch is mostly just removing an indentation level. One variable had to be moved due to the removal of a scope, and one comment was split into two. Viewing the patch with git show/diff -b will show that it's actually very small.
Note that WM8903_MIC_BIAS_CONTROL_0 is now written unconditionally, whereas it used to be written only if pdata was supplied. Since defpdata.micdet_cfg = 0, this unconditional write simply echos the HW defaults in the case where pdata is not supplied.
Based on work by John Bonesio, but significantly reworked since then.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/codecs/wm8903.c | 74 ++++++++++++++++++++++----------------------- 1 files changed, 36 insertions(+), 38 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 9fd89be..7e3b960 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1842,7 +1842,7 @@ static void wm8903_init_gpio(struct snd_soc_codec *codec) wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO; wm8903->gpio_chip.dev = codec->dev;
- if (pdata && pdata->gpio_base) + if (pdata->gpio_base) wm8903->gpio_chip.base = pdata->gpio_base; else wm8903->gpio_chip.base = -1; @@ -1878,6 +1878,7 @@ static int wm8903_probe(struct snd_soc_codec *codec) int ret, i; int trigger, irq_pol; u16 val; + bool mic_gpio = false;
wm8903->codec = codec; codec->control_data = wm8903->regmap; @@ -1888,52 +1889,49 @@ static int wm8903_probe(struct snd_soc_codec *codec) return ret; }
- /* Set up GPIOs and microphone detection */ - if (pdata) { - bool mic_gpio = false; - - for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) { - if ((!pdata->gpio_cfg[i]) || - (pdata->gpio_cfg[i] > WM8903_GPIO_CONFIG_ZERO)) - continue; + /* Set up GPIOs, detect if any are MIC detect outputs */ + for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) { + if ((!pdata->gpio_cfg[i]) || + (pdata->gpio_cfg[i] > WM8903_GPIO_CONFIG_ZERO)) + continue;
- snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i, - pdata->gpio_cfg[i] & 0x7fff); + snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i, + pdata->gpio_cfg[i] & 0x7fff);
- val = (pdata->gpio_cfg[i] & WM8903_GP1_FN_MASK) - >> WM8903_GP1_FN_SHIFT; + val = (pdata->gpio_cfg[i] & WM8903_GP1_FN_MASK) + >> WM8903_GP1_FN_SHIFT;
- switch (val) { - case WM8903_GPn_FN_MICBIAS_CURRENT_DETECT: - case WM8903_GPn_FN_MICBIAS_SHORT_DETECT: - mic_gpio = true; - break; - default: - break; - } + switch (val) { + case WM8903_GPn_FN_MICBIAS_CURRENT_DETECT: + case WM8903_GPn_FN_MICBIAS_SHORT_DETECT: + mic_gpio = true; + break; + default: + break; } + }
- snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, - pdata->micdet_cfg); + /* Set up microphone detection */ + snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, + pdata->micdet_cfg);
- /* Microphone detection needs the WSEQ clock */ - if (pdata->micdet_cfg) - snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0, - WM8903_WSEQ_ENA, WM8903_WSEQ_ENA); + /* Microphone detection needs the WSEQ clock */ + if (pdata->micdet_cfg) + snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0, + WM8903_WSEQ_ENA, WM8903_WSEQ_ENA);
- /* If microphone detection is enabled by pdata but - * detected via IRQ then interrupts can be lost before - * the machine driver has set up microphone detection - * IRQs as the IRQs are clear on read. The detection - * will be enabled when the machine driver configures. - */ - WARN_ON(!mic_gpio && (pdata->micdet_cfg & WM8903_MICDET_ENA)); + /* If microphone detection is enabled by pdata but + * detected via IRQ then interrupts can be lost before + * the machine driver has set up microphone detection + * IRQs as the IRQs are clear on read. The detection + * will be enabled when the machine driver configures. + */ + WARN_ON(!mic_gpio && (pdata->micdet_cfg & WM8903_MICDET_ENA)); + + wm8903->mic_delay = pdata->micdet_delay;
- wm8903->mic_delay = pdata->micdet_delay; - } - if (wm8903->irq) { - if (pdata && pdata->irq_active_low) { + if (pdata->irq_active_low) { trigger = IRQF_TRIGGER_LOW; irq_pol = WM8903_IRQ_POL; } else {
If the WM8903 is hooked up to an interrupt, set the irq_active_low flag in the default platform data based on the IRQ's IRQ_TYPE. Map IRQ_TYPE_NONE (a lack of explicit configuration/restriction) to irq_active_low = false; the previous default.
This code is mainly added to support device tree interrupt bindings, although will work perfectly well in a non device tree system too.
Any interrupt controller that supports only a single IRQ_TYPE could set each IRQ's type based on that restriction. This applies equally with and without device tree. To cater for interrupt controllers that don't do this, for which irqd_get_trigger_type() will return IRQ_TYPE_NONE, the platform data irq_active_low field may be used in systems that don't use device tree.
With device tree, every IRQ must have some IRQ_TYPE set.
Controllers that support DT and multiple IRQ_TYPEs must define the interrupts property (as used in interrupt source nodes) such that it defines the IRQ_TYPE to use. When the core DT setup code initializes wm8903->irq, the interrupts property will be parsed, and as a side- effect, set the IRQ's IRQ_TYPE for the WM8903 probe() function to read.
Controllers that support DT and a single IRQ_TYPE could arrange to set the IRQ_TYPE somehow during their initialization, or hard-code it during the processing of the child interrupts property.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/codecs/wm8903.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 7e3b960..a44bbfb 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -25,6 +25,7 @@ #include <linux/i2c.h> #include <linux/regmap.h> #include <linux/slab.h> +#include <linux/irq.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/pcm.h> @@ -2036,6 +2037,39 @@ static const struct regmap_config wm8903_regmap = { .num_reg_defaults = ARRAY_SIZE(wm8903_reg_defaults), };
+static int wm8903_set_pdata_irq_trigger(struct i2c_client *i2c, + struct wm8903_platform_data *pdata) +{ + struct irq_data *irq_data = irq_get_irq_data(i2c->irq); + if (!irq_data) { + dev_err(&i2c->dev, "Invalid IRQ: %d\n", + i2c->irq); + return -EINVAL; + } + + switch (irqd_get_trigger_type(irq_data)) { + case IRQ_TYPE_NONE: + /* + * We assume the controller imposes no restrictions, + * so we are able to select active-high + */ + /* Fall-through */ + case IRQ_TYPE_LEVEL_HIGH: + pdata->irq_active_low = false; + break; + case IRQ_TYPE_LEVEL_LOW: + pdata->irq_active_low = true; + break; + default: + dev_err(&i2c->dev, + "Unsupported IRQ_TYPE %x\n", + irqd_get_trigger_type(irq_data)); + return -EINVAL; + } + + return 0; +} + static __devinit int wm8903_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { @@ -2071,6 +2105,12 @@ static __devinit int wm8903_i2c_probe(struct i2c_client *i2c, dev_err(&i2c->dev, "Failed to allocate pdata\n"); return -ENOMEM; } + + if (i2c->irq) { + ret = wm8903_set_pdata_irq_trigger(i2c, wm8903->pdata); + if (ret != 0) + return ret; + } }
ret = regmap_read(wm8903->regmap, WM8903_SW_RESET_AND_ID, &val);
On Fri, Dec 02, 2011 at 03:08:40PM -0700, Stephen Warren wrote:
- switch (irqd_get_trigger_type(irq_data)) {
- case IRQ_TYPE_NONE:
/** We assume the controller imposes no restrictions,* so we are able to select active-high*//* Fall-through */- case IRQ_TYPE_LEVEL_HIGH:
pdata->irq_active_low = false;break;- case IRQ_TYPE_LEVEL_LOW:
pdata->irq_active_low = true;break;- default:
dev_err(&i2c->dev,"Unsupported IRQ_TYPE %x\n",irqd_get_trigger_type(irq_data));return -EINVAL;
Actually, it occurs to me that we should treat the default case in the same way as IRQ_TYPE_NONE - even if the interrupt controller defaults to an edge triggered mode which we can't use it might also support a level triggered interrupt we can use. If it doesn't then we'll just fail later on when we request the IRQ so the end result should be the same.
Don't worry about respinning for this unless the series needs to get resent for some other reason let's just fix it incrementally (I'll do that myself when I apply unless you want to).
Mark Brown wrote at Saturday, December 03, 2011 4:17 AM:
On Fri, Dec 02, 2011 at 03:08:40PM -0700, Stephen Warren wrote:
- switch (irqd_get_trigger_type(irq_data)) {
- case IRQ_TYPE_NONE:
/** We assume the controller imposes no restrictions,* so we are able to select active-high*//* Fall-through */- case IRQ_TYPE_LEVEL_HIGH:
pdata->irq_active_low = false;break;- case IRQ_TYPE_LEVEL_LOW:
pdata->irq_active_low = true;break;- default:
dev_err(&i2c->dev,"Unsupported IRQ_TYPE %x\n",irqd_get_trigger_type(irq_data));return -EINVAL;Actually, it occurs to me that we should treat the default case in the same way as IRQ_TYPE_NONE - even if the interrupt controller defaults to an edge triggered mode which we can't use it might also support a level triggered interrupt we can use. If it doesn't then we'll just fail later on when we request the IRQ so the end result should be the same.
Yes, deferring the failure to later seems reasonable; it's probably better to work somehow if the HW supports it rather than being anal about e.g. incorrect device tree content.
Don't worry about respinning for this unless the series needs to get resent for some other reason let's just fix it incrementally (I'll do that myself when I apply unless you want to).
I'm fine if you change that when applying it.
Thanks.
Document the device tree binding for the WM8903 codec, and modify the driver to extract platform data from the device tree, if present.
Based on work by John Bonesio, but significantly reworked since then.
Signed-off-by: Stephen Warren swarren@nvidia.com --- Documentation/devicetree/bindings/sound/wm8903.txt | 50 ++++++++++++++++++++ sound/soc/codecs/wm8903.c | 49 +++++++++++++++++++ 2 files changed, 99 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/wm8903.txt
diff --git a/Documentation/devicetree/bindings/sound/wm8903.txt b/Documentation/devicetree/bindings/sound/wm8903.txt new file mode 100644 index 0000000..f102cbc --- /dev/null +++ b/Documentation/devicetree/bindings/sound/wm8903.txt @@ -0,0 +1,50 @@ +WM8903 audio CODEC + +This device supports I2C only. + +Required properties: + + - compatible : "wlf,wm8903" + + - reg : the I2C address of the device. + + - gpio-controller : Indicates this device is a GPIO controller. + + - #gpio-cells : Should be two. The first cell is the pin number and the + second cell is used to specify optional parameters (currently unused). + +Optional properties: + + - interrupts : The interrupt line the codec is connected to. + + - micdet-cfg : Default register value for R6 (Mic Bias). If absent, the + default is 0. + + - micdet-delay : The debounce delay for microphone detection in mS. If + absent, the default is 100. + + - gpio-cfg : A list of GPIO configuration register values. The list must + be 5 entries long. If absent, no configuration of these registers is + performed. If any entry has the value 0xffffffff, that GPIO's + configuration will not be modified. + +Example: + +codec: wm8903@1a { + compatible = "wlf,wm8903"; + reg = <0x1a>; + interrupts = < 347 >; + + gpio-controller; + #gpio-cells = <2>; + + micdet-cfg = <0>; + micdet-delay = <100>; + gpio-cfg = < + 0x0600 /* DMIC_LR, output */ + 0x0680 /* DMIC_DAT, input */ + 0x0000 /* GPIO, output, low */ + 0x0200 /* Interrupt, output */ + 0x01a0 /* BCLK, input, active high */ + >; +}; diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index a44bbfb..c080a87 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -2070,6 +2070,49 @@ static int wm8903_set_pdata_irq_trigger(struct i2c_client *i2c, return 0; }
+static int wm8903_set_pdata_from_of(struct i2c_client *i2c, + struct wm8903_platform_data *pdata) +{ + const struct device_node *np = i2c->dev.of_node; + u32 val32; + int i; + + if (of_property_read_u32(np, "micdet-cfg", &val32) >= 0) + pdata->micdet_cfg = val32; + + if (of_property_read_u32(np, "micdet-delay", &val32) >= 0) + pdata->micdet_delay = val32; + + if (of_property_read_u32_array(np, "gpio-cfg", pdata->gpio_cfg, + ARRAY_SIZE(pdata->gpio_cfg)) >= 0) { + /* + * In device tree: 0 means "write 0", + * 0xffffffff means "don't touch". + * + * In platform data: 0 means "don't touch", + * 0x8000 means "write 0". + * + * Note: WM8903_GPIO_CONFIG_ZERO == 0x8000. + * + * Convert from DT to pdata representation here, + * so no other code needs to change. + */ + for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) { + if (pdata->gpio_cfg[i] == 0) { + pdata->gpio_cfg[i] = WM8903_GPIO_CONFIG_ZERO; + } else if (pdata->gpio_cfg[i] == 0xffffffff) { + pdata->gpio_cfg[i] = 0; + } else if (pdata->gpio_cfg[i] > 0x7fff) { + dev_err(&i2c->dev, "Invalid gpio-cfg[%d] %x\n", + i, pdata->gpio_cfg[i]); + return -EINVAL; + } + } + } + + return 0; +} + static __devinit int wm8903_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { @@ -2111,6 +2154,12 @@ static __devinit int wm8903_i2c_probe(struct i2c_client *i2c, if (ret != 0) return ret; } + + if (i2c->dev.of_node) { + ret = wm8903_set_pdata_from_of(i2c, wm8903->pdata); + if (ret != 0) + return ret; + } }
ret = regmap_read(wm8903->regmap, WM8903_SW_RESET_AND_ID, &val);
Stephen Warren wrote at Friday, December 02, 2011 3:09 PM:
Document the device tree binding for the WM8903 codec, and modify the driver to extract platform data from the device tree, if present.
Mark,
I just realized that when I was re-organizing all the WM8903 patches, I dropped the part that added the of_match table to the driver:
+static const struct of_device_id wm8903_of_match[] __devinitconst = { + { .compatible = "wlf,wm8903", }, + {}, +}; +MODULE_DEVICE_TABLE(of, wm8903_of_match);
Now, everything still works without this. Looking at the Linux OF code, it works by retrieving the compatible property, taking everything after the comma if present, and then creating an i2c_board_info with that type, which in this case is "wm8903" and matches wm8903.c's i2c_device_id table. See drivers/of/of_i2c.c:of_i2c_register_devices() and the call to base.c:of_modalias_node().
So, the question is: Should I go back and add the of_match table, or is I2C intended to work without it perpetually? I notice that you added an of_match table for all the other WM codecs.
Thanks.
On Tue, Dec 06, 2011 at 10:22:24AM -0800, Stephen Warren wrote:
Now, everything still works without this. Looking at the Linux OF code, it works by retrieving the compatible property, taking everything after the comma if present, and then creating an i2c_board_info with that type, which in this case is "wm8903" and matches wm8903.c's i2c_device_id table. See drivers/of/of_i2c.c:of_i2c_register_devices() and the call to base.c:of_modalias_node().
So, the question is: Should I go back and add the of_match table, or is I2C intended to work without it perpetually? I notice that you added an of_match table for all the other WM codecs.
You can't go back and do anything now as the patches are merged, you need to fix incrementally. You should do this for completeness even if currently we work without it.
participants (3)
-
Mark Brown -
Olof Johansson -
Stephen Warren