[PATCH v3 1/6] ASoC: WM8903: Disallow all invalid gpio_cfg pdata values

[PATCH v3 1/6] ASoC: WM8903: Disallow all invalid gpio_cfg pdata values

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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;
-- 
1.7.0.4

[PATCH v3 2/6] ASoC: WM8903: Create default platform data structure

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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;
-- 
1.7.0.4

Re: [PATCH v3 2/6] ASoC: WM8903: Create default platform data structure

[Mark Brown]

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.

[PATCH v3 3/6] ASoC: WM8903: Pass pdata to wm8903_init_gpio

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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;
 }
-- 
1.7.0.4

Re: [PATCH v3 3/6] ASoC: WM8903: Pass pdata to wm8903_init_gpio

[Mark Brown]

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.

RE: [PATCH v3 3/6] ASoC: WM8903: Pass pdata to wm8903_init_gpio

[Stephen Warren]

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"?

-- 
nvpublic

Re: [PATCH v3 3/6] ASoC: WM8903: Pass pdata to wm8903_init_gpio

[Mark Brown]

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.

[PATCH v3 4/6] ASoC: WM8903: Remove conditionals checking pdata != NULL

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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 {
-- 
1.7.0.4

[PATCH v3 5/6] ASoC: WM8903: Get default irq_active_low from IRQ controller

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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)
-- 
1.7.0.4

[PATCH v3 6/6] ASoC: WM8903: Add device tree binding

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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;
-- 
1.7.0.4

Re: [PATCH v3 1/6] ASoC: WM8903: Disallow all invalid gpio_cfg pdata values

[Mark Brown]

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.