[alsa-devel] [PATCH v3 1/6] ASoC: WM8903: Disallow all invalid gpio_cfg pdata values
The GPIO registers are 15 bits wide. Hence values, higher than 0x7fff are not legal GPIO register values. Modify the pdata.gpio_cfg handling code to reject all illegal values, not just WM8903_GPIO_NO_CONFIG (0x8000). This will allow the later use of 0xffffffff as an invalid value in future device tree bindings, meaning "don't touch this GPIO's configuration".
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/codecs/wm8903.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 70a2268..0d1640e 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1936,11 +1936,11 @@ 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] == WM8903_GPIO_NO_CONFIG) + if (pdata->gpio_cfg[i] > 0x7fff) continue;
snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i, - pdata->gpio_cfg[i] & 0xffff); + pdata->gpio_cfg[i] & 0x7fff);
val = (pdata->gpio_cfg[i] & WM8903_GP1_FN_MASK) >> WM8903_GP1_FN_SHIFT;
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 | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 0d1640e..8249571 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1905,6 +1905,7 @@ 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_platform_data defpdata; struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec); int ret, i; int trigger, irq_pol; @@ -1931,6 +1932,18 @@ static int wm8903_probe(struct snd_soc_codec *codec)
wm8903_reset(codec);
+ /* Default platform data, for use if none is supplied */ + defpdata.irq_active_low = false; + defpdata.micdet_cfg = 0; + defpdata.micdet_delay = 0; + defpdata.gpio_base = -1; + for (i = 0; i < ARRAY_SIZE(defpdata.gpio_cfg); i++) + defpdata.gpio_cfg[i] = 0xffffffff; + + /* If no platform data was supplied, use the defaults */ + if (!pdata) + pdata = &defpdata; + /* Set up GPIOs and microphone detection */ if (pdata) { bool mic_gpio = false;
On Thu, Dec 01, 2011 at 01:49:20PM -0700, Stephen Warren wrote:
- /* Default platform data, for use if none is supplied */
- defpdata.irq_active_low = false;
- defpdata.micdet_cfg = 0;
- defpdata.micdet_delay = 0;
No need to set things to zero (or just memset the structure).
- defpdata.gpio_base = -1;
- for (i = 0; i < ARRAY_SIZE(defpdata.gpio_cfg); i++)
defpdata.gpio_cfg[i] = 0xffffffff;
These should come after we've taken the copy, checking to see if there'a anything there before overwriting.
Modify wm8903_init_gpio() so that it's passed the pdata structure rather than extracting it from the platform device. This allows the caller to pass in a default pdata structure when the platform device didn't contain one. wm8903_init_gpio() now uses the centralized default gpio_base definition added in the previous 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 | 15 ++++++--------- 1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 8249571..04d2393 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1863,20 +1863,16 @@ static struct gpio_chip wm8903_template_chip = { .can_sleep = 1, };
-static void wm8903_init_gpio(struct snd_soc_codec *codec) +static void wm8903_init_gpio(struct snd_soc_codec *codec, + struct wm8903_platform_data *pdata) { struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec); - struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev); int ret;
wm8903->gpio_chip = wm8903_template_chip; wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO; wm8903->gpio_chip.dev = codec->dev; - - if (pdata && pdata->gpio_base) - wm8903->gpio_chip.base = pdata->gpio_base; - else - wm8903->gpio_chip.base = -1; + wm8903->gpio_chip.base = pdata->gpio_base;
ret = gpiochip_add(&wm8903->gpio_chip); if (ret != 0) @@ -1893,7 +1889,8 @@ static void wm8903_free_gpio(struct snd_soc_codec *codec) dev_err(codec->dev, "Failed to remove GPIOs: %d\n", ret); } #else -static void wm8903_init_gpio(struct snd_soc_codec *codec) +static void wm8903_init_gpio(struct snd_soc_codec *codec, + struct wm8903_platform_data *pdata) { }
@@ -2050,7 +2047,7 @@ static int wm8903_probe(struct snd_soc_codec *codec) snd_soc_add_controls(codec, wm8903_snd_controls, ARRAY_SIZE(wm8903_snd_controls));
- wm8903_init_gpio(codec); + wm8903_init_gpio(codec, pdata);
return ret; }
On Thu, Dec 01, 2011 at 01:49:21PM -0700, Stephen Warren wrote:
- if (pdata && pdata->gpio_base)
wm8903->gpio_chip.base = pdata->gpio_base;- else
wm8903->gpio_chip.base = -1;
- wm8903->gpio_chip.base = pdata->gpio_base;
This will break existing users in counjuntion with the previous patch. Previously if the user provided platform data but left gpio_base as zero we'd use -1 and let gpiolib pick for us. Now instead the driver will take that zero and pass it on to gpiolib, probably failing as the SoC will have taken the low numbered GPIOs.
Mark Brown wrote at Thursday, December 01, 2011 5:28 PM:
On Thu, Dec 01, 2011 at 01:49:21PM -0700, Stephen Warren wrote:
- if (pdata && pdata->gpio_base)
wm8903->gpio_chip.base = pdata->gpio_base;- else
wm8903->gpio_chip.base = -1;
- wm8903->gpio_chip.base = pdata->gpio_base;
This will break existing users in counjuntion with the previous patch. Previously if the user provided platform data but left gpio_base as zero we'd use -1 and let gpiolib pick for us. Now instead the driver will take that zero and pass it on to gpiolib, probably failing as the SoC will have taken the low numbered GPIOs.
Yes, I suppose that's true. However, I don't see it as a problem.
Surely if the user provided pdata, it's their responsibility to fill it in correctly and completely. It seems a little random to take the pdata, and try to guess whether 0 means 0 or "I didn't set the value, so use the default". I think the same comment applies w.r.t to your comment on patch 2 (gpio_cfg); 0 is a perfectly legitimate value for the register; why should the driver double-guess that value and assume 0 means "don't touch the pin"?
On Thu, Dec 01, 2011 at 04:48:11PM -0800, Stephen Warren wrote:
Mark Brown wrote at Thursday, December 01, 2011 5:28 PM:
This will break existing users in counjuntion with the previous patch. Previously if the user provided platform data but left gpio_base as zero we'd use -1 and let gpiolib pick for us. Now instead the driver will take that zero and pass it on to gpiolib, probably failing as the SoC will have taken the low numbered GPIOs.
Yes, I suppose that's true. However, I don't see it as a problem.
Surely if the user provided pdata, it's their responsibility to fill it in correctly and completely. It seems a little random to take the pdata, and try to guess whether 0 means 0 or "I didn't set the value, so use the default". I think the same comment applies w.r.t to your
No, that's not been the general approach as it avoids breaking existing users when you add new elements to the platform data and it means that users don't have to worry about every single field which is much more friendly as the number of fields gets larger. In the case of GPIOs it was frankly a bad idea to have 0 be a valid GPIO value in the first place; it makes the API that little bit more fiddly to work with.
Device tree is much nicer in this regard as you can just omit properties which is unambiguous.
comment on patch 2 (gpio_cfg); 0 is a perfectly legitimate value for the register; why should the driver double-guess that value and assume 0 means "don't touch the pin"?
Yeah, that's annoying. There's a reason why most of the chips do the write of zero by setting an out of bounds bit in the pdata. At least for the devices I deal with directly 0 is fortunately usually a nonsensical value to want to set anyway so it's not so important.
In probe(), 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 | 70 ++++++++++++++++++++++----------------------- 1 files changed, 34 insertions(+), 36 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 04d2393..38e1137 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1907,6 +1907,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;
@@ -1941,51 +1942,48 @@ static int wm8903_probe(struct snd_soc_codec *codec) if (!pdata) pdata = &defpdata;
- /* Set up GPIOs and microphone detection */ - if (pdata) { - bool mic_gpio = false; + /* 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] > 0x7fff) + continue;
- for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) { - if (pdata->gpio_cfg[i] > 0x7fff) - 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; } + } + + /* Set up microphone detection */ + snd_soc_write(codec, WM8903_MIC_BIAS_CONTROL_0, + pdata->micdet_cfg);
- 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 | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 38e1137..6797b0a 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -24,6 +24,7 @@ #include <linux/pm.h> #include <linux/i2c.h> #include <linux/slab.h> +#include <linux/irq.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/pcm.h> @@ -1937,6 +1938,33 @@ static int wm8903_probe(struct snd_soc_codec *codec) defpdata.gpio_base = -1; for (i = 0; i < ARRAY_SIZE(defpdata.gpio_cfg); i++) defpdata.gpio_cfg[i] = 0xffffffff; + if (wm8903->irq) { + struct irq_data *irq_data = irq_get_irq_data(wm8903->irq); + if (!irq_data) { + dev_err(codec->dev, "Invalid IRQ: %d\n", + wm8903->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: + defpdata.irq_active_low = false; + break; + case IRQ_TYPE_LEVEL_LOW: + defpdata.irq_active_low = true; + break; + default: + dev_err(codec->dev, + "Unsupported IRQ_TYPE %x\n", + irqd_get_trigger_type(irq_data)); + return -EINVAL; + } + }
/* If no platform data was supplied, use the defaults */ if (!pdata)
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 | 15 ++++++ 2 files changed, 65 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..14fd3ec --- /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 pin mux 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 6797b0a..bce94af 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1966,6 +1966,21 @@ static int wm8903_probe(struct snd_soc_codec *codec) } }
+ /* Override defaults from device tree, if one is present */ + if (codec->dev->of_node) { + const struct device_node *np = codec->dev->of_node; + u32 val32; + + if (of_property_read_u32(np, "micdet-cfg", &val32) >= 0) + defpdata.micdet_cfg = val32; + + if (of_property_read_u32(np, "micdet-delay", &val32) >= 0) + defpdata.micdet_delay = val32; + + of_property_read_u32_array(np, "gpio-cfg", defpdata.gpio_cfg, + ARRAY_SIZE(defpdata.gpio_cfg)); + } + /* If no platform data was supplied, use the defaults */ if (!pdata) pdata = &defpdata;
On Thu, Dec 01, 2011 at 01:49:19PM -0700, Stephen Warren wrote:
The GPIO registers are 15 bits wide. Hence values, higher than 0x7fff are not legal GPIO register values. Modify the pdata.gpio_cfg handling code to reject all illegal values, not just WM8903_GPIO_NO_CONFIG (0x8000). This will allow the later use of 0xffffffff as an invalid value in future device tree bindings, meaning "don't touch this GPIO's configuration".
Applied, thanks.
participants (2)
-
Mark Brown -
Stephen Warren