[PATCH v2 0/5] ASoC: WM8903: DT bindings & related

[PATCH v2 0/5] ASoC: WM8903: DT bindings & related

[Stephen Warren]

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

[PATCH v2 1/5] ASoC: WM8903: Fix platform data gpio_cfg confusion

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
---
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,
-- 
1.7.0.4

Re: [PATCH v2 1/5] ASoC: WM8903: Fix platform data gpio_cfg confusion

[Mark Brown]

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.

Re: [PATCH v2 1/5] ASoC: WM8903: Fix platform data gpio_cfg confusion

[Olof Johansson]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> ---
> 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-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>


-Olof

Re: [PATCH v2 1/5] ASoC: WM8903: Fix platform data gpio_cfg confusion

[Mark Brown]

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-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>

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.

[PATCH v2 2/5] 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 |   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);
-- 
1.7.0.4

Re: [PATCH v2 2/5] ASoC: WM8903: Create default platform data structure

[Mark Brown]

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).

[PATCH v2 3/5] ASoC: WM8903: Remove conditionals checking pdata != NULL

[Stephen Warren]

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 |   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 {
-- 
1.7.0.4

[PATCH v2 4/5] 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 |   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);
-- 
1.7.0.4

Re: [PATCH v2 4/5] ASoC: WM8903: Get default irq_active_low from IRQ controller

[Mark Brown]

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).

RE: [PATCH v2 4/5] ASoC: WM8903: Get default irq_active_low from IRQ controller

[Stephen Warren]

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.

-- 
nvpublic

[PATCH v2 5/5] 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                          |   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);
-- 
1.7.0.4

RE: [PATCH v2 5/5] ASoC: WM8903: Add device tree binding

[Stephen Warren]

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.

-- 
nvpublic

Re: [PATCH v2 5/5] ASoC: WM8903: Add device tree binding

[Rob Herring]

On 12/06/2011 12:22 PM, Stephen Warren wrote:
> 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.

It definitely tries to match first with the OF match table, so it should
probably be added back.

Rob

RE: [PATCH v2 5/5] ASoC: WM8903: Add device tree binding

[Stephen Warren]

Rob Herring wrote at Tuesday, December 06, 2011 12:07 PM:
> On 12/06/2011 12:22 PM, Stephen Warren wrote:
> > 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.
> 
> It definitely tries to match first with the OF match table, so it should
> probably be added back.

OK, I have no issues putting it back.

But, I certainly can't find any code in drivers/i2c or drivers/of/of_i2c.c
that matches the compatible property against the of_match_table. Can
you point it out please. As best I can tell, I2C bus/controller drivers call
of_i2c_register_devices() which calls of_modalias_node() which only
operates in the manner I described. I don't think any generic instantiation
code can be operating instead, since there's nothing in
of_i2c_register_devices() that skips devices that have already been
instantiated via other means. Nothing in drivers/i2c seems to touch of_node
or the compatible property.

-- 
nvpublic

Re: [PATCH v2 5/5] ASoC: WM8903: Add device tree binding

[Rob Herring]

On 12/06/2011 02:09 PM, Stephen Warren wrote:
> Rob Herring wrote at Tuesday, December 06, 2011 12:07 PM:
>> On 12/06/2011 12:22 PM, Stephen Warren wrote:
>>> 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.
>>
>> It definitely tries to match first with the OF match table, so it should
>> probably be added back.
> 
> OK, I have no issues putting it back.
> 
> But, I certainly can't find any code in drivers/i2c or drivers/of/of_i2c.c
> that matches the compatible property against the of_match_table. Can
> you point it out please. As best I can tell, I2C bus/controller drivers call
> of_i2c_register_devices() which calls of_modalias_node() which only
> operates in the manner I described. I don't think any generic instantiation
> code can be operating instead, since there's nothing in
> of_i2c_register_devices() that skips devices that have already been
> instantiated via other means. Nothing in drivers/i2c seems to touch of_node
> or the compatible property.
> 
Because it is not i2c specific code. But look at i2c_device_match which
calls of_driver_match_device:

static int i2c_device_match(struct device *dev, struct device_driver *drv)
{
        struct i2c_client       *client = i2c_verify_client(dev);
        struct i2c_driver       *driver;

        if (!client)
                return 0;

        /* Attempt an OF style match */
        if (of_driver_match_device(dev, drv))
                return 1;

        driver = to_i2c_driver(drv);
        /* match on an id table if there is one */
        if (driver->id_table)
                return i2c_match_id(driver->id_table, client) != NULL;

        return 0;
}

Rob

Re: [PATCH v2 5/5] ASoC: WM8903: Add device tree binding

[Mark Brown]

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.