Re: [PATCH 1/3] ASoC: WM8903: Add wm8903_set_gpio

Re: [PATCH 1/3] ASoC: WM8903: Add wm8903_set_gpio

[Mark Brown]

On Tue, Jan 18, 2011 at 04:37:37PM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>

This should be implemented using gpiolib - see WM8962 for an example of
this.  That way the control can be plugged into more other subsystems in
the kernel (probably including whenever I or someone else gets around to
it a widget like the one you open code in a later patch).  I know some
of the drivers do export the GPIOs using custom APIs but that's
something that should be fixed.

> Also, fix platform_data handling to have an explicit "don't touch this pin"
> option.

This should've been a separate patch since it's OK to go in as-is.

> Temporarily hack out the mic detection platform_data handling for simplicity

There's no patch restoring the functionality...

> tesing the GPIO changes. Should the write-sequencer enabling be conditional?

Yes.  It's not actually enabling the write sequencer itself, it's
enabling a low rate clock that's shared between a number of functions in
the device including jack detection.  Without the clock jack detection
will not function.

Re: [PATCH 0/3] Tegra: Add internal speaker support

[Mark Brown]

On Tue, Jan 18, 2011 at 04:37:36PM -0700, Stephen Warren wrote:

> a) Is this the right way to expose GPIO APIs from a codec? The tlv320aic3
>    seems to do it this way, but wm8962.c uses the generic GPIO API. I shied
>    away from the latter, since I wasn't sure how to name the WM8903's GPIOs
>    in the gpio_* calls. I suppose it'd need a bunch of GPIO driver platform
>    data to hook it all together and name GPIOs from a mach-tegra/include
>    header file, but if this is the way to go, I need to read up on that more.

What naming is required for the GPIOs?  GPIOs are referred to by number
in Linux.

> b) Is the WM8903 platform_data handling full fleshed out; simply by having
>    platform_data, WM8903_WSEQ_ENA is set, whereas without any pdata, it
>    isn't. Should this be conditional, or should I always have specified some
>    platform data for Tegra, and I'm just getting lucky that it works without
>    any?

Ah, your comment about the sequencer is clearer now.  This is not
enabling the write sequencer, that'll be done transiently when required
if it's not set here.  It's enabling a clock which is used by several
parts of the chip including the write sequencer all the time so that the
mic detection can function in the absence of any other software managing
it.  This is why it's conditional on microphone detection being
configured, other functions can have this clock disabled most of the
time.

Re: [PATCH 0/3] Tegra: Add internal speaker support

[Stephen Warren]

Mark Brown wrote:
> On Tue, Jan 18, 2011 at 04:37:36PM -0700, Stephen Warren wrote:
> 
> > a) Is this the right way to expose GPIO APIs from a codec? The tlv320aic3
> >    seems to do it this way, but wm8962.c uses the generic GPIO API. I shied
> >    away from the latter, since I wasn't sure how to name the WM8903's GPIOs
> >    in the gpio_* calls. I suppose it'd need a bunch of GPIO driver platform
> >    data to hook it all together and name GPIOs from a mach-tegra/include
> >    header file, but if this is the way to go, I need to read up on that more.
> 
> What naming is required for the GPIOs?  GPIOs are referred to by number
> in Linux.

Naming/numbering mean the same thing to me; I have to pass a number to
gpio_request/gpio_set_value, either by hard-coded number or name (i.e. #define
referring to some number).

I'll go read up on gpiolib some more to see how those API-level numbers get
mapped to a specific controller and GPIO number.

-- 
nvpublic

Re: [PATCH 0/3] Tegra: Add internal speaker support

[Mark Brown]

On Tue, Jan 18, 2011 at 04:43:33PM -0800, Colin Cross wrote:
> On Tue, Jan 18, 2011 at 4:29 PM, Stephen Warren <swarren@nvidia.com> wrote:

> > I'll go read up on gpiolib some more to see how those API-level numbers get
> > mapped to a specific controller and GPIO number.

> They usually come in through platform data from the board file.  See
> Olof's recent Tegra sdhci patches for an example.  Tegra GPIO names
> are defined in arch/arm/mach-tegra/gpio-names.h.

Yup, and gpiolib also supports automatic assignment of numbers if you
specify -1 as the base number though that's a lot less useful within the
kernel.

Re: [PATCH 0/3] Tegra: Add internal speaker support

[Liam Girdwood]

On Tue, 2011-01-18 at 16:37 -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Harmony has headers on the board for an internal speaker and internal mic.
> I've started looking at enabling these, along with full jack detection etc.
> This first set of patches is an RFC (Request For Comments) on whether it's
> the correct direction, particularly with respect to:
> 
> a) Is this the right way to expose GPIO APIs from a codec? The tlv320aic3
>    seems to do it this way, but wm8962.c uses the generic GPIO API. I shied
>    away from the latter, since I wasn't sure how to name the WM8903's GPIOs
>    in the gpio_* calls. I suppose it'd need a bunch of GPIO driver platform
>    data to hook it all together and name GPIOs from a mach-tegra/include
>    header file, but if this is the way to go, I need to read up on that more.
> 
> b) Is the WM8903 platform_data handling full fleshed out; simply by having
>    platform_data, WM8903_WSEQ_ENA is set, whereas without any pdata, it
>    isn't. Should this be conditional, or should I always have specified some
>    platform data for Tegra, and I'm just getting lucky that it works without
>    any?
> 
> Thanks for any coments.
> 

2 & 3 look fine and

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

[PATCH v2 0/4] Tegra: Harmony: Add internal speaker support

[Stephen Warren]

Harmony has headers on the board for an internal speaker and internal mic.
This patch series adds various infra-structure, and enables usage of the
internal speaker.

The patch series includes both changes to code within the ASoC
subsystem and Tegra subsystem, the latter provided only for context when
reading the ASoC changes. Would you prefer I separated these so that the
ASoC maintainers can grab a whole series and apply it, rather than just
a subset of the patches?

While patch 2 is physically located in the Tegra subsystem, I'd
consider it part of the ASoC driver. Can such a change be checked into
the ASoC tree? This new header file will be required to build to ASoC driver
once the following patches are in. I was considering placing this in
include/sound, following the example of various codecs, but it doesn't seem
a good idea to pollute that directory with platform-specific files. The
current location for the file follows the example of s3c24xx/simtec. Perhaps
it should be in sound/soc/tegra?

Stephen Warren (4):
  ASoC: WM8903: Expose GPIOs through gpiolib
  ARM: tegra: Add Harmony sound platform data type
  ARM: tegra: Platform data fixes for ASoC driver updates
  ASoC: tegra: Harmony: Support the internal speaker

 arch/arm/mach-tegra/board-harmony.c              |   31 ++++++
 arch/arm/mach-tegra/gpio-names.h                 |    2 +
 arch/arm/mach-tegra/include/mach/harmony_audio.h |   19 ++++
 include/sound/wm8903.h                           |    7 +-
 sound/soc/codecs/wm8903.c                        |  117 +++++++++++++++++++++-
 sound/soc/tegra/harmony.c                        |   89 ++++++++++++++--
 6 files changed, 251 insertions(+), 14 deletions(-)
 create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h

Re: [PATCH v2 0/4] Tegra: Harmony: Add internal speaker support

[Liam Girdwood]

On Wed, 2011-01-19 at 13:50 -0700, Stephen Warren wrote:
> Harmony has headers on the board for an internal speaker and internal mic.
> This patch series adds various infra-structure, and enables usage of the
> internal speaker.
> 
> The patch series includes both changes to code within the ASoC
> subsystem and Tegra subsystem, the latter provided only for context when
> reading the ASoC changes. Would you prefer I separated these so that the
> ASoC maintainers can grab a whole series and apply it, rather than just
> a subset of the patches?
> 
> While patch 2 is physically located in the Tegra subsystem, I'd
> consider it part of the ASoC driver. Can such a change be checked into
> the ASoC tree? This new header file will be required to build to ASoC driver
> once the following patches are in. I was considering placing this in
> include/sound, following the example of various codecs, but it doesn't seem
> a good idea to pollute that directory with platform-specific files. The
> current location for the file follows the example of s3c24xx/simtec. Perhaps
> it should be in sound/soc/tegra?
> 
> Stephen Warren (4):
>   ASoC: WM8903: Expose GPIOs through gpiolib
>   ARM: tegra: Add Harmony sound platform data type
>   ARM: tegra: Platform data fixes for ASoC driver updates
>   ASoC: tegra: Harmony: Support the internal speaker
> 
>  arch/arm/mach-tegra/board-harmony.c              |   31 ++++++
>  arch/arm/mach-tegra/gpio-names.h                 |    2 +
>  arch/arm/mach-tegra/include/mach/harmony_audio.h |   19 ++++
>  include/sound/wm8903.h                           |    7 +-
>  sound/soc/codecs/wm8903.c                        |  117 +++++++++++++++++++++-
>  sound/soc/tegra/harmony.c                        |   89 ++++++++++++++--
>  6 files changed, 251 insertions(+), 14 deletions(-)
>  create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h
> 

All

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

[PATCH v2 1/4] ASoC: WM8903: Expose GPIOs through gpiolib

[Stephen Warren]

Also, fix platform_data GPIO handling to have an explicit "don't touch this
pin" option.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 include/sound/wm8903.h    |    7 ++-
 sound/soc/codecs/wm8903.c |  117 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 121 insertions(+), 3 deletions(-)

diff --git a/include/sound/wm8903.h b/include/sound/wm8903.h
index b4a0db2..c835f2c 100644
--- a/include/sound/wm8903.h
+++ b/include/sound/wm8903.h
@@ -12,7 +12,7 @@
 #define __LINUX_SND_WM8903_H
 
 /* Used to enable configuration of a GPIO to all zeros */
-#define WM8903_GPIO_NO_CONFIG 0x8000
+#define WM8903_GPIO_NO_CONFIG 0x10000
 
 /*
  * R6 (0x06) - Mic Bias Control 0
@@ -231,6 +231,8 @@
 #define WM8903_GP5_DB_SHIFT                          0  /* GP5_DB */
 #define WM8903_GP5_DB_WIDTH                          1  /* GP5_DB */
 
+#define WM8903_NUM_GPIO 5
+
 struct wm8903_platform_data {
 	bool irq_active_low;   /* Set if IRQ active low, default high */
 
@@ -243,7 +245,8 @@ struct wm8903_platform_data {
 
 	int micdet_delay;      /* Delay after microphone detection (ms) */
 
-	u32 gpio_cfg[5];       /* Default register values for GPIO pin mux */
+	int gpio_base;
+	u32 gpio_cfg[WM8903_NUM_GPIO]; /* Default register values for GPIO pin mux */
 };
 
 #endif
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
index a2a446c..db641c5 100644
--- a/sound/soc/codecs/wm8903.c
+++ b/sound/soc/codecs/wm8903.c
@@ -2,6 +2,7 @@
  * wm8903.c  --  WM8903 ALSA SoC Audio driver
  *
  * Copyright 2008 Wolfson Microelectronics
+ * Copyright 2011 NVIDIA, Inc.
  *
  * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
  *
@@ -19,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/pm.h>
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
@@ -213,6 +215,7 @@ static u16 wm8903_reg_defaults[] = {
 };
 
 struct wm8903_priv {
+	struct snd_soc_codec *codec;
 
 	int sysclk;
 	int irq;
@@ -230,6 +233,10 @@ struct wm8903_priv {
 	int mic_short;
 	int mic_last_report;
 	int mic_delay;
+
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gpio_chip;
+#endif
 };
 
 static int wm8903_volatile_register(struct snd_soc_codec *codec, unsigned int reg)
@@ -1635,6 +1642,110 @@ static int wm8903_resume(struct snd_soc_codec *codec)
 	return 0;
 }
 
+#ifdef CONFIG_GPIOLIB
+static inline struct wm8903_priv *gpio_to_wm8903(struct gpio_chip *chip)
+{
+	return container_of(chip, struct wm8903_priv, gpio_chip);
+}
+
+static int wm8903_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	if (offset >= WM8903_NUM_GPIO)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+
+	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
+				   WM8903_GP1_DIR_MASK, WM8903_GP1_DIR);
+}
+
+static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+	int reg;
+
+	reg = snd_soc_read(codec, WM8903_GPIO_CONTROL_1 + offset);
+
+	return (reg & WM8903_GP1_LVL_MASK) >> WM8903_GP1_LVL_SHIFT;
+}
+
+static int wm8903_gpio_direction_out(struct gpio_chip *chip,
+				     unsigned offset, int value)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+
+	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
+				   WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK,
+				   value << WM8903_GP2_LVL_SHIFT);
+}
+
+static void wm8903_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+
+	snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
+			    WM8903_GP1_LVL_MASK, value << WM8903_GP1_LVL_SHIFT);
+}
+
+static struct gpio_chip wm8903_template_chip = {
+	.label			= "wm8903",
+	.owner			= THIS_MODULE,
+	.request		= wm8903_gpio_request,
+	.direction_input	= wm8903_gpio_direction_in,
+	.get			= wm8903_gpio_get,
+	.direction_output	= wm8903_gpio_direction_out,
+	.set			= wm8903_gpio_set,
+	.can_sleep		= 1,
+};
+
+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);
+	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;
+
+	ret = gpiochip_add(&wm8903->gpio_chip);
+	if (ret != 0)
+		dev_err(codec->dev, "Failed to add GPIOs: %d\n", ret);
+}
+
+static void wm8903_free_gpio(struct snd_soc_codec *codec)
+{
+	struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	ret = gpiochip_remove(&wm8903->gpio_chip);
+	if (ret != 0)
+		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_free_gpio(struct snd_soc_codec *codec)
+{
+}
+#endif
+
 static int wm8903_probe(struct snd_soc_codec *codec)
 {
 	struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev);
@@ -1643,6 +1754,7 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 	int trigger, irq_pol;
 	u16 val;
 
+	wm8903->codec = codec;
 	init_completion(&wm8903->wseq);
 
 	ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_I2C);
@@ -1667,7 +1779,7 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 	/* Set up GPIOs and microphone detection */
 	if (pdata) {
 		for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) {
-			if (!pdata->gpio_cfg[i])
+			if (pdata->gpio_cfg[i] == WM8903_GPIO_NO_CONFIG)
 				continue;
 
 			snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i,
@@ -1749,12 +1861,15 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 				ARRAY_SIZE(wm8903_snd_controls));
 	wm8903_add_widgets(codec);
 
+	wm8903_init_gpio(codec);
+
 	return ret;
 }
 
 /* power down chip */
 static int wm8903_remove(struct snd_soc_codec *codec)
 {
+	wm8903_free_gpio(codec);
 	wm8903_set_bias_level(codec, SND_SOC_BIAS_OFF);
 	return 0;
 }
-- 
1.7.1

Re: [PATCH v2 1/4] ASoC: WM8903: Expose GPIOs through gpiolib

[Mark Brown]

On Wed, Jan 19, 2011 at 01:50:02PM -0700, Stephen Warren wrote:

This looks good, one query and one minor omission though.

>  /* Used to enable configuration of a GPIO to all zeros */
> -#define WM8903_GPIO_NO_CONFIG 0x8000
> +#define WM8903_GPIO_NO_CONFIG 0x10000

Why this change?  The top bit in the registers is never actually used,
really they're all 15 bit.

> +static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
> +	struct snd_soc_codec *codec = wm8903->codec;
> +
> +	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
> +				   WM8903_GP1_DIR_MASK, WM8903_GP1_DIR);
> +}

> +static int wm8903_gpio_direction_out(struct gpio_chip *chip,
> +				     unsigned offset, int value)
> +{
> +	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
> +	struct snd_soc_codec *codec = wm8903->codec;
> +
> +	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
> +				   WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK,
> +				   value << WM8903_GP2_LVL_SHIFT);
> +}

These should also set GPn_FN - to zero for GPIO output, 3 for GPIO
input.  Otherwise changing between input and output mode at runtime
won't do the right thing.  As a side effect of that you probably
wouldn't need to specify the GPIO configuration in platform data.

Re: [PATCH v2 1/4] ASoC: WM8903: Expose GPIOs through gpiolib

[Stephen Warren]

Mark Brown wrote on Thursday, January 20, 2011 4:53 AM:
> 
> On Wed, Jan 19, 2011 at 01:50:02PM -0700, Stephen Warren wrote:
> 
> This looks good, one query and one minor omission though.
> 
> >  /* Used to enable configuration of a GPIO to all zeros */
> > -#define WM8903_GPIO_NO_CONFIG 0x8000
> > +#define WM8903_GPIO_NO_CONFIG 0x10000
> 
> Why this change?  The top bit in the registers is never actually used,
> really they're all 15 bit.

Some other registers appear to have bit 15 defined, so I made sure this flag
was completely outside any valid register bits.

Although mainly, I just made it consistent with wm8962.h, which I assume
picked that value for the same reason.

That said, the original value was OK for the GPIO registers, so I can revert
that if you want.

> > +static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
> > +{
> > +	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
> > +	struct snd_soc_codec *codec = wm8903->codec;
> > +
> > +	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
> > +				   WM8903_GP1_DIR_MASK, WM8903_GP1_DIR);
> > +}
> 
> > +static int wm8903_gpio_direction_out(struct gpio_chip *chip,
> > +				     unsigned offset, int value)
> > +{
> > +	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
> > +	struct snd_soc_codec *codec = wm8903->codec;
> > +
> > +	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
> > +				   WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK,
> > +				   value << WM8903_GP2_LVL_SHIFT);
> > +}
> 
> These should also set GPn_FN - to zero for GPIO output, 3 for GPIO
> input.  Otherwise changing between input and output mode at runtime
> won't do the right thing.  As a side effect of that you probably
> wouldn't need to specify the GPIO configuration in platform data.

Ooops. I didn't notice that. How does this interact with bit 7; GPn_DIR? I
assume both need to be set appropriately since they're both defined bits.

-- 
nvpublic

Re: [PATCH v2 1/4] ASoC: WM8903: Expose GPIOs through gpiolib

[Mark Brown]

On Thu, Jan 20, 2011 at 09:23:45AM -0800, Stephen Warren wrote:

> Although mainly, I just made it consistent with wm8962.h, which I assume
> picked that value for the same reason.

8962 has a very different register map as it has compatiblity with some
very old devices.

> That said, the original value was OK for the GPIO registers, so I can revert
> that if you want.

It did stick out as a bit random.

> > These should also set GPn_FN - to zero for GPIO output, 3 for GPIO
> > input.  Otherwise changing between input and output mode at runtime
> > won't do the right thing.  As a side effect of that you probably
> > wouldn't need to specify the GPIO configuration in platform data.

> Ooops. I didn't notice that. How does this interact with bit 7; GPn_DIR? I
> assume both need to be set appropriately since they're both defined bits.

Yes, one controls the function and the other controls the pad mode.

[PATCH v2 2/4] ARM: tegra: Add Harmony sound platform data type

[Stephen Warren]

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/mach-tegra/include/mach/harmony_audio.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h

diff --git a/arch/arm/mach-tegra/include/mach/harmony_audio.h b/arch/arm/mach-tegra/include/mach/harmony_audio.h
new file mode 100644
index 0000000..5c46391
--- /dev/null
+++ b/arch/arm/mach-tegra/include/mach/harmony_audio.h
@@ -0,0 +1,19 @@
+/*
+ * arch/arm/mach-tegra/include/mach/harmony_audio.h
+ *
+ * Copyright 2011 NVIDIA, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+struct harmony_audio_platform_data {
+	int gpio_spkr_en;
+};
-- 
1.7.1

[PATCH v2 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

The WM8903 driver now needs platform data in order to correctly expose its
GPIOs through gpiolib.

The top-level ASoC driver now needs a platform device in order to provide
infra-structure to pass in platform data.

The top-level ASoC driver now needs platform data in order to be informed
which GPIO IDs to use.

This is all needed to allow the top-level ASoC driver to configure the
WM8903's GPIO3 as an output GPIO, and enable it as appropriate. This is the
enable signal for the external speaker amplifier chips.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/mach-tegra/board-harmony.c |   31 +++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/gpio-names.h    |    2 ++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
index 2696a20..c5a4ae0 100644
--- a/arch/arm/mach-tegra/board-harmony.c
+++ b/arch/arm/mach-tegra/board-harmony.c
@@ -2,6 +2,7 @@
  * arch/arm/mach-tegra/board-harmony.c
  *
  * Copyright (C) 2010 Google, Inc.
+ * Copyright (C) 2011 NVIDIA, Inc.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -24,12 +25,15 @@
 #include <linux/i2c.h>
 #include <linux/io.h>
 
+#include <sound/wm8903.h>
+
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/time.h>
 #include <asm/setup.h>
 
 #include <mach/gpio.h>
+#include <mach/harmony_audio.h>
 #include <mach/i2c.h>
 #include <mach/iomap.h>
 #include <mach/irqs.h>
@@ -53,6 +57,8 @@
 #define ATAG_NVIDIA_PRESERVED_MEM_N	2
 #define ATAG_NVIDIA_FORCE_32		0x7fffffff
 
+#define GPIO_WM8903(_x_) (TEGRA_MAX_GPIO + 1 + (_x_))
+
 struct tag_tegra {
 	__u32 bootarg_key;
 	__u32 bootarg_len;
@@ -112,9 +118,33 @@ static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
 	.bus_clk_rate	= { 400000, 0 },
 };
 
+static struct wm8903_platform_data harmony_wm8903_pdata = {
+	.gpio_base = GPIO_WM8903(0),
+	.gpio_cfg = {
+		WM8903_GPIO_NO_CONFIG,
+		WM8903_GPIO_NO_CONFIG,
+		0,
+		WM8903_GPIO_NO_CONFIG,
+		WM8903_GPIO_NO_CONFIG,
+	},
+};
+
 static struct i2c_board_info __initdata harmony_i2c_bus1_board_info[] = {
 	{
 		I2C_BOARD_INFO("wm8903", 0x1a),
+		.platform_data = &harmony_wm8903_pdata,
+	},
+};
+
+static struct harmony_audio_platform_data harmony_audio_pdata = {
+	.gpio_spkr_en = GPIO_WM8903(2),
+};
+
+static struct platform_device harmony_audio_device = {
+	.name   = "tegra-snd-harmony",
+	.id     = 0,
+	.dev    = {
+		.platform_data  = &harmony_audio_pdata,
 	},
 };
 
@@ -127,6 +157,7 @@ static struct platform_device *harmony_devices[] __initdata = {
 	&tegra_i2s_device1,
 	&tegra_das_device,
 	&tegra_pcm_device,
+	&harmony_audio_device,
 };
 
 static void __init tegra_harmony_fixup(struct machine_desc *desc,
diff --git a/arch/arm/mach-tegra/gpio-names.h b/arch/arm/mach-tegra/gpio-names.h
index f28220a..e8b5cf3 100644
--- a/arch/arm/mach-tegra/gpio-names.h
+++ b/arch/arm/mach-tegra/gpio-names.h
@@ -244,4 +244,6 @@
 #define TEGRA_GPIO_PBB6		222
 #define TEGRA_GPIO_PBB7		223
 
+#define TEGRA_MAX_GPIO		TEGRA_GPIO_PBB7
+
 #endif
-- 
1.7.1

[PATCH v2 4/4] ASoC: tegra: Harmony: Support the internal speaker

[Stephen Warren]

Add DAPM widget definitions, and code to control the speaker amplifier
GPIO to the Harmony machine driver. Currently, this path is always enabled,
while playback is active, since jack detection isn't yet implemented.

The driver initialization path has been reworked a little, and a new
driver/device introduced, in order to allow platform data to be passed in,
currently containing just the speaker enable GPIO ID.

The GPIO is only requested during _init, since that's the first time it is
guaranteed that the WM8903 module is loaded, probed, and hence has exported
its GPIO chip.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 sound/soc/tegra/harmony.c |   89 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
index b160b71..0e44dd6 100644
--- a/sound/soc/tegra/harmony.c
+++ b/sound/soc/tegra/harmony.c
@@ -2,7 +2,7 @@
  * harmony.c - Harmony machine ASoC driver
  *
  * Author: Stephen Warren <swarren@nvidia.com>
- * Copyright (C) 2010 - NVIDIA, Inc.
+ * Copyright (C) 2010-2011 - NVIDIA, Inc.
  *
  * Based on code copyright/by:
  *
@@ -29,7 +29,14 @@
  */
 
 #include <asm/mach-types.h>
+
+#include <linux/gpio.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <mach/harmony_audio.h>
+
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -40,9 +47,12 @@
 #include "tegra_pcm.h"
 #include "tegra_asoc_utils.h"
 
-#define PREFIX "ASoC Harmony: "
+#define DRV_NAME "tegra-snd-harmony"
+#define PREFIX DRV_NAME ": "
 
+static struct harmony_audio_platform_data *pdata;
 static struct platform_device *harmony_snd_device;
+static int gpio_spkr_en_requested;
 
 static int harmony_asoc_hw_params(struct snd_pcm_substream *substream,
 					struct snd_pcm_hw_params *params)
@@ -107,7 +117,19 @@ static struct snd_soc_ops harmony_asoc_ops = {
 	.hw_params = harmony_asoc_hw_params,
 };
 
+static int harmony_event_int_spk(struct snd_soc_dapm_widget *w,
+					struct snd_kcontrol *k, int event)
+{
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		gpio_set_value_cansleep(pdata->gpio_spkr_en, 1);
+	else
+		gpio_set_value_cansleep(pdata->gpio_spkr_en, 0);
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget harmony_dapm_widgets[] = {
+	SND_SOC_DAPM_SPK("Int Spk", harmony_event_int_spk),
 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_MIC("Mic Jack", NULL),
 };
@@ -115,6 +137,10 @@ static const struct snd_soc_dapm_widget harmony_dapm_widgets[] = {
 static const struct snd_soc_dapm_route harmony_audio_map[] = {
 	{"Headphone Jack", NULL, "HPOUTR"},
 	{"Headphone Jack", NULL, "HPOUTL"},
+	{"Int Spk", NULL, "ROP"},
+	{"Int Spk", NULL, "RON"},
+	{"Int Spk", NULL, "LOP"},
+	{"Int Spk", NULL, "LON"},
 	{"Mic Bias", NULL, "Mic Jack"},
 	{"IN1L", NULL, "Mic Bias"},
 };
@@ -123,6 +149,7 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_codec *codec = rtd->codec;
 	struct snd_soc_dapm_context *dapm = &codec->dapm;
+	int ret;
 
 	snd_soc_dapm_new_controls(dapm, harmony_dapm_widgets,
 					ARRAY_SIZE(harmony_dapm_widgets));
@@ -131,9 +158,19 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
 				ARRAY_SIZE(harmony_audio_map));
 
 	snd_soc_dapm_enable_pin(dapm, "Headphone Jack");
+	snd_soc_dapm_enable_pin(dapm, "Int Spk");
 	snd_soc_dapm_enable_pin(dapm, "Mic Jack");
 	snd_soc_dapm_sync(dapm);
 
+	ret = gpio_request(pdata->gpio_spkr_en, "spkr_en");
+	if (ret) {
+		pr_err(PREFIX "cannot get spkr_en gpio\n");
+		return ret;
+	}
+	gpio_spkr_en_requested = 1;
+
+	gpio_direction_output(pdata->gpio_spkr_en, 0);
+
 	return 0;
 }
 
@@ -154,13 +191,19 @@ static struct snd_soc_card snd_soc_harmony = {
 	.num_links = 1,
 };
 
-static int __init harmony_soc_modinit(void)
+static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
 {
 	int ret;
 
-	if (!machine_is_harmony()) {
-		pr_err(PREFIX "Not running on Tegra Harmony!\n");
-		return -ENODEV;
+	if (pdev->id != 0) {
+		dev_err(&pdev->dev, "ID %d out of range\n", pdev->id);
+		return -EINVAL;
+	}
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "no platform data supplied\n");
+		return -EINVAL;
 	}
 
 	ret = tegra_asoc_utils_init();
@@ -173,7 +216,7 @@ static int __init harmony_soc_modinit(void)
 	 */
 	harmony_snd_device = platform_device_alloc("soc-audio", -1);
 	if (harmony_snd_device == NULL) {
-		pr_err(PREFIX "platform_device_alloc failed\n");
+		dev_err(&pdev->dev, "platform_device_alloc failed\n");
 		ret = -ENOMEM;
 		goto err_clock_utils;
 	}
@@ -182,7 +225,7 @@ static int __init harmony_soc_modinit(void)
 
 	ret = platform_device_add(harmony_snd_device);
 	if (ret) {
-		pr_err(PREFIX "platform_device_add failed (%d)\n",
+		dev_err(&pdev->dev, "platform_device_add failed (%d)\n",
 			ret);
 		goto err_device_put;
 	}
@@ -195,15 +238,39 @@ err_clock_utils:
 	tegra_asoc_utils_fini();
 	return ret;
 }
-module_init(harmony_soc_modinit);
 
-static void __exit harmony_soc_modexit(void)
+static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
 {
 	platform_device_unregister(harmony_snd_device);
 
+	if (gpio_spkr_en_requested)
+		gpio_free(pdata->gpio_spkr_en);
+
 	tegra_asoc_utils_fini();
+
+	return 0;
+}
+
+static struct platform_driver tegra_snd_harmony_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+	},
+	.probe = tegra_snd_harmony_probe,
+	.remove = __devexit_p(tegra_snd_harmony_remove),
+};
+
+static int __init snd_tegra_harmony_init(void)
+{
+	return platform_driver_register(&tegra_snd_harmony_driver);
+}
+module_init(snd_tegra_harmony_init);
+
+static void __exit snd_tegra_harmony_exit(void)
+{
+	platform_driver_unregister(&tegra_snd_harmony_driver);
 }
-module_exit(harmony_soc_modexit);
+module_exit(snd_tegra_harmony_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren@nvidia.com>");
 MODULE_DESCRIPTION("Harmony machine ASoC driver");
-- 
1.7.1

Re: [PATCH v2 4/4] ASoC: tegra: Harmony: Support the internal speaker

[Mark Brown]

On Wed, Jan 19, 2011 at 01:50:05PM -0700, Stephen Warren wrote:

> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		gpio_set_value_cansleep(pdata->gpio_spkr_en, 1);
> +	else
> +		gpio_set_value_cansleep(pdata->gpio_spkr_en, 0);

Very minor stylistic thing but you could say:

	gpio_set_value_cansleep(pdata->gpio_spkr_en,
				SND_SOC_DAPM_EVENT_ON(event));

instead.

Re: [PATCH v2 4/4] ASoC: tegra: Harmony: Support the internal speaker

[Mark Brown]

On Wed, Jan 19, 2011 at 01:50:05PM -0700, Stephen Warren wrote:

> -static int __init harmony_soc_modinit(void)
> +static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)

Unrelated change...

> -	if (!machine_is_harmony()) {
> -		pr_err(PREFIX "Not running on Tegra Harmony!\n");
> -		return -ENODEV;
> +	if (pdev->id != 0) {
> +		dev_err(&pdev->dev, "ID %d out of range\n", pdev->id);
> +		return -EINVAL;
> +	}
> +
> +	pdata = pdev->dev.platform_data;
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "no platform data supplied\n");
> +		return -EINVAL;

Unless you have more than one sound card your device ID should be -1.
I'd also be inclined to keep the machine_is_harmony() check here for
paranoia.

With the recently added exposure of snd_soc_register_card() you *should*
just be able allocate a regular platform device with a regular name in
your arch/arm code and then register that directly with the ASoC core -
something like:

int __devinit harmony_audio_probe(struct platform_device *pdev)
{
	/* Do GPIO stuff */

	card->dev = &pdev->dev;
	snd_soc_register_card(&snd_soc_harmony);

	/* Error handling */
}

ought to do the trick, and is much neater and more idiomatic than the
soc-audio stuff. 

Re: [PATCH v2 4/4] ASoC: tegra: Harmony: Support the internal speaker

[Stephen Warren]

Mark Brown wrote on Tuesday, January 25, 2011 1:30 PM:
> 
> On Wed, Jan 19, 2011 at 01:50:05PM -0700, Stephen Warren wrote:
> 
> > -static int __init harmony_soc_modinit(void)
> > +static __devinit int tegra_snd_harmony_probe(struct platform_device
> *pdev)
> 
> Unrelated change...

Yeah, I guess I should separate the device probing rework into a separate
patch.

> ...
> With the recently added exposure of snd_soc_register_card() you *should*
> just be able allocate a regular platform device with a regular name in
> your arch/arm code and then register that directly with the ASoC core -
> something like:
> 
> int __devinit harmony_audio_probe(struct platform_device *pdev)
> {
> 	/* Do GPIO stuff */
> 
> 	card->dev = &pdev->dev;
> 	snd_soc_register_card(&snd_soc_harmony);
> 
> 	/* Error handling */
> }
> 
> ought to do the trick, and is much neater and more idiomatic than the
> soc-audio stuff.

With the existing soc-audio structure, one has to:

	platform_set_drvdata(harmony_snd_device, &snd_soc_harmony);

I assume there's no need for this when registering via snd_soc_register_card;
In other words, I'm free to use dev_set_drvdata on the platform_device/device
so I can get rid of all the globals in harmony.y while I'm at it?

I do see some internal use of set_drvdata/get_drvdata in soc-core.c. It
looks like that's restricted to when the soc-audio platform_device is used,
but I don't know if that's just co-incidence, or if it's a guarantee of the
API. Can you confirm this?

Thanks.

--
nvpublic

Re: [PATCH v2 4/4] ASoC: tegra: Harmony: Support the internal speaker

[Mark Brown]

On Tue, Jan 25, 2011 at 07:46:57PM -0800, Stephen Warren wrote:

> With the existing soc-audio structure, one has to:

> 	platform_set_drvdata(harmony_snd_device, &snd_soc_harmony);

> I assume there's no need for this when registering via snd_soc_register_card;
> In other words, I'm free to use dev_set_drvdata on the platform_device/device
> so I can get rid of all the globals in harmony.y while I'm at it?

> I do see some internal use of set_drvdata/get_drvdata in soc-core.c. It
> looks like that's restricted to when the soc-audio platform_device is used,
> but I don't know if that's just co-incidence, or if it's a guarantee of the
> API. Can you confirm this?

Oh, bah.  The code is currently broken - suspend and resume are still
using the platform device.  I'll post a patch later today which removes
this requirement and adds callbacks for you to use.

[PATCH v3 0/4] Tegra: Harmony: Add internal speaker support

[Stephen Warren]

Harmony has headers on the board for an internal speaker and internal mic.
This patch series adds various infra-structure, and enables usage of the
internal speaker.

The patch series includes both changes to code within the ASoC
subsystem and Tegra subsystem, the latter provided only for context when
reading the ASoC changes. Would you prefer I separated these so that the
ASoC maintainers can grab a whole series and apply it, rather than just
a subset of the patches?

While patch 2 is physically located in the Tegra subsystem, I'd
consider it part of the ASoC driver. Can such a change be checked into
the ASoC tree? This new header file will be required to build to ASoC driver
once the following patches are in. I was considering placing this in
include/sound, following the example of various codecs, but it doesn't seem
a good idea to pollute that directory with platform-specific files. The
current location for the file follows the example of s3c24xx/simtec. Perhaps
it should be in sound/soc/tegra?

Changelog:

* v3:
  * Reverted change to WM8903_GPIO_NO_CONFIG
  * Program WM8903_GP1_FN_MASK in direction_{input,output}
  * Simplify harmony_event_int_spk
  * Didn't remove GPIO entries from harmony_wm8903_pdata, since this
    also sets up all the other fields to 0 too.

Stephen Warren (4):
  ASoC: WM8903: Expose GPIOs through gpiolib
  ARM: tegra: Add Harmony sound platform data type
  ARM: tegra: Platform data fixes for ASoC driver updates
  ASoC: tegra: Harmony: Support the internal speaker

 arch/arm/mach-tegra/board-harmony.c              |   31 ++++++
 arch/arm/mach-tegra/gpio-names.h                 |    2 +
 arch/arm/mach-tegra/include/mach/harmony_audio.h |   19 ++++
 include/sound/wm8903.h                           |   20 ++++-
 sound/soc/codecs/wm8903.c                        |  126 +++++++++++++++++++++-
 sound/soc/tegra/harmony.c                        |   87 +++++++++++++--
 6 files changed, 272 insertions(+), 13 deletions(-)
 create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h

[PATCH v3 1/4] ASoC: WM8903: Expose GPIOs through gpiolib

[Stephen Warren]

Also, fix platform_data GPIO handling to have an explicit "don't touch this
pin" option.

Add #defines for the GPIO pin functions.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 include/sound/wm8903.h    |   20 +++++++-
 sound/soc/codecs/wm8903.c |  126 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/include/sound/wm8903.h b/include/sound/wm8903.h
index b4a0db2..86172cf 100644
--- a/include/sound/wm8903.h
+++ b/include/sound/wm8903.h
@@ -37,6 +37,21 @@
 #define WM8903_MICBIAS_ENA_WIDTH                     1  /* MICBIAS_ENA */
 
 /*
+ * WM8903_GPn_FN values
+ *
+ * See datasheets for list of valid values per pin
+ */
+#define WM8903_GPn_FN_GPIO_OUTPUT                    0
+#define WM8903_GPn_FN_BCLK                           1
+#define WM8903_GPn_FN_IRQ_OUTPT                      2
+#define WM8903_GPn_FN_GPIO_INPUT                     3
+#define WM8903_GPn_FN_MICBIAS_CURRENT_DETECT         4
+#define WM8903_GPn_FN_MICBIAS_SHORT_DETECT           5
+#define WM8903_GPn_FN_DMIC_LR_CLK_OUTPUT             6
+#define WM8903_GPn_FN_FLL_LOCK_OUTPUT                8
+#define WM8903_GPn_FN_FLL_CLOCK_OUTPUT               9
+
+/*
  * R116 (0x74) - GPIO Control 1
  */
 #define WM8903_GP1_FN_MASK                      0x1F00  /* GP1_FN - [12:8] */
@@ -231,6 +246,8 @@
 #define WM8903_GP5_DB_SHIFT                          0  /* GP5_DB */
 #define WM8903_GP5_DB_WIDTH                          1  /* GP5_DB */
 
+#define WM8903_NUM_GPIO 5
+
 struct wm8903_platform_data {
 	bool irq_active_low;   /* Set if IRQ active low, default high */
 
@@ -243,7 +260,8 @@ struct wm8903_platform_data {
 
 	int micdet_delay;      /* Delay after microphone detection (ms) */
 
-	u32 gpio_cfg[5];       /* Default register values for GPIO pin mux */
+	int gpio_base;
+	u32 gpio_cfg[WM8903_NUM_GPIO]; /* Default register values for GPIO pin mux */
 };
 
 #endif
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
index a2a446c..9c4f2c4 100644
--- a/sound/soc/codecs/wm8903.c
+++ b/sound/soc/codecs/wm8903.c
@@ -2,6 +2,7 @@
  * wm8903.c  --  WM8903 ALSA SoC Audio driver
  *
  * Copyright 2008 Wolfson Microelectronics
+ * Copyright 2011 NVIDIA, Inc.
  *
  * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
  *
@@ -19,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/pm.h>
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
@@ -213,6 +215,7 @@ static u16 wm8903_reg_defaults[] = {
 };
 
 struct wm8903_priv {
+	struct snd_soc_codec *codec;
 
 	int sysclk;
 	int irq;
@@ -230,6 +233,10 @@ struct wm8903_priv {
 	int mic_short;
 	int mic_last_report;
 	int mic_delay;
+
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gpio_chip;
+#endif
 };
 
 static int wm8903_volatile_register(struct snd_soc_codec *codec, unsigned int reg)
@@ -1635,6 +1642,119 @@ static int wm8903_resume(struct snd_soc_codec *codec)
 	return 0;
 }
 
+#ifdef CONFIG_GPIOLIB
+static inline struct wm8903_priv *gpio_to_wm8903(struct gpio_chip *chip)
+{
+	return container_of(chip, struct wm8903_priv, gpio_chip);
+}
+
+static int wm8903_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	if (offset >= WM8903_NUM_GPIO)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+	unsigned int mask, val;
+
+	mask = WM8903_GP1_FN_MASK | WM8903_GP1_DIR_MASK;
+	val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GP1_FN_SHIFT) |
+		WM8903_GP1_DIR;
+
+	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
+				   mask, val);
+}
+
+static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+	int reg;
+
+	reg = snd_soc_read(codec, WM8903_GPIO_CONTROL_1 + offset);
+
+	return (reg & WM8903_GP1_LVL_MASK) >> WM8903_GP1_LVL_SHIFT;
+}
+
+static int wm8903_gpio_direction_out(struct gpio_chip *chip,
+				     unsigned offset, int value)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+	unsigned int mask, val;
+
+	mask = WM8903_GP1_FN_MASK | WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK;
+	val = (WM8903_GPn_FN_GPIO_OUTPUT << WM8903_GP1_FN_SHIFT) |
+		(value << WM8903_GP2_LVL_SHIFT);
+
+	return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
+				   mask, val);
+}
+
+static void wm8903_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
+	struct snd_soc_codec *codec = wm8903->codec;
+
+	snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
+			    WM8903_GP1_LVL_MASK, value << WM8903_GP1_LVL_SHIFT);
+}
+
+static struct gpio_chip wm8903_template_chip = {
+	.label			= "wm8903",
+	.owner			= THIS_MODULE,
+	.request		= wm8903_gpio_request,
+	.direction_input	= wm8903_gpio_direction_in,
+	.get			= wm8903_gpio_get,
+	.direction_output	= wm8903_gpio_direction_out,
+	.set			= wm8903_gpio_set,
+	.can_sleep		= 1,
+};
+
+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);
+	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;
+
+	ret = gpiochip_add(&wm8903->gpio_chip);
+	if (ret != 0)
+		dev_err(codec->dev, "Failed to add GPIOs: %d\n", ret);
+}
+
+static void wm8903_free_gpio(struct snd_soc_codec *codec)
+{
+	struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	ret = gpiochip_remove(&wm8903->gpio_chip);
+	if (ret != 0)
+		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_free_gpio(struct snd_soc_codec *codec)
+{
+}
+#endif
+
 static int wm8903_probe(struct snd_soc_codec *codec)
 {
 	struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev);
@@ -1643,6 +1763,7 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 	int trigger, irq_pol;
 	u16 val;
 
+	wm8903->codec = codec;
 	init_completion(&wm8903->wseq);
 
 	ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_I2C);
@@ -1667,7 +1788,7 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 	/* Set up GPIOs and microphone detection */
 	if (pdata) {
 		for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) {
-			if (!pdata->gpio_cfg[i])
+			if (pdata->gpio_cfg[i] == WM8903_GPIO_NO_CONFIG)
 				continue;
 
 			snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i,
@@ -1749,12 +1870,15 @@ static int wm8903_probe(struct snd_soc_codec *codec)
 				ARRAY_SIZE(wm8903_snd_controls));
 	wm8903_add_widgets(codec);
 
+	wm8903_init_gpio(codec);
+
 	return ret;
 }
 
 /* power down chip */
 static int wm8903_remove(struct snd_soc_codec *codec)
 {
+	wm8903_free_gpio(codec);
 	wm8903_set_bias_level(codec, SND_SOC_BIAS_OFF);
 	return 0;
 }
-- 
1.7.1

Re: [PATCH v3 1/4] ASoC: WM8903: Expose GPIOs through gpiolib

[Mark Brown]

On Thu, Jan 20, 2011 at 01:52:08PM -0700, Stephen Warren wrote:
> Also, fix platform_data GPIO handling to have an explicit "don't touch this
> pin" option.
> 
> Add #defines for the GPIO pin functions.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied this one, thanks.

[PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Stephen Warren]

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/mach-tegra/include/mach/harmony_audio.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h

diff --git a/arch/arm/mach-tegra/include/mach/harmony_audio.h b/arch/arm/mach-tegra/include/mach/harmony_audio.h
new file mode 100644
index 0000000..5c46391
--- /dev/null
+++ b/arch/arm/mach-tegra/include/mach/harmony_audio.h
@@ -0,0 +1,19 @@
+/*
+ * arch/arm/mach-tegra/include/mach/harmony_audio.h
+ *
+ * Copyright 2011 NVIDIA, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+struct harmony_audio_platform_data {
+	int gpio_spkr_en;
+};
-- 
1.7.1

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Colin Cross]

On Thu, Jan 20, 2011 at 12:52 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  arch/arm/mach-tegra/include/mach/harmony_audio.h |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h
>
> diff --git a/arch/arm/mach-tegra/include/mach/harmony_audio.h b/arch/arm/mach-tegra/include/mach/harmony_audio.h
> new file mode 100644
> index 0000000..5c46391
> --- /dev/null
> +++ b/arch/arm/mach-tegra/include/mach/harmony_audio.h
> @@ -0,0 +1,19 @@
> +/*
> + * arch/arm/mach-tegra/include/mach/harmony_audio.h
> + *
> + * Copyright 2011 NVIDIA, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +struct harmony_audio_platform_data {
> +       int gpio_spkr_en;
> +};
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Does this file really belong in mach-tegra if its for a driver that is
in a common directory (drivers/sound/soc)?  Putting it in
include/linux would solve all your tree-ordering problems.

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Stephen Warren]

Colin Cross wrote on Friday, January 21, 2011 11:43 AM:
> 
> On Thu, Jan 20, 2011 at 12:52 PM, Stephen Warren wrote:
> >  create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h
>
> Does this file really belong in mach-tegra if its for a driver that is in
> a common directory (drivers/sound/soc)?  Putting it in include/linux would
> solve all your tree-ordering problems.

Colin,

Re: The location I chose; I followed an existing example of another ASoC
driver. Perhaps there is a more appropriate location, although the ASoC
maintainers didn't point one out.

I'm afraid I don't understand how moving the file would solve the
dependency issues though:

There's a change in sound/soc/... that needs this new file.

There's a change in arch/arm/mach-tegra/... that needs this new file.

Both those two changes would typically get checked into two separate git
repositories.

As far as I can tell, we could:

a) Check the new file into both repositories, and git will hopefully just
merge them together in the next merge window. Mark pointed out a long time
ago that this is generally frowned upon though.

b) Check the change into just one repository, e.g. ASoC since it's ready to
accept the sound/soc/... changes that rely on it, and then have Tegra's
for-next merge that (or merge linux-next after it's filtered up there,
although I'm not sure if merging linux-next is workable with git?) (or
merge directly from ASoC's for-2.6.39) and only then apply the arch/arm/tegra
changes that rely on it. Mark pointed out a long time ago that such cross-
subsystem merges are very rare, although it sounded like they do happen
sometimes.

c) Only check in the sound/soc/... changes, wait until they're in 2.6.39-rc1
and hence can be picked up from there for Tegra's for-next, and then check in
the Tegra changes that rely on it. This would delay the complete changes
until 2.6.40:-(

I'm still in the dark how this normally works; it seems like it must be
pretty common to add a new driver to the driver repository, with that driver
defining a new platform_data type, and also needing to add code to the
architecture/chip repository to provide that platform_data, and hopefully
have both those things go in in parallel, rather than being staged across
two kernel releases.

-- 
nvpublic

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Colin Cross]

On Fri, Jan 21, 2011 at 2:35 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Colin Cross wrote on Friday, January 21, 2011 11:43 AM:
>>
>> On Thu, Jan 20, 2011 at 12:52 PM, Stephen Warren wrote:
>> >  create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h
>>
>> Does this file really belong in mach-tegra if its for a driver that is in
>> a common directory (drivers/sound/soc)?  Putting it in include/linux would
>> solve all your tree-ordering problems.
>
> Colin,
>
> Re: The location I chose; I followed an existing example of another ASoC
> driver. Perhaps there is a more appropriate location, although the ASoC
> maintainers didn't point one out.
>
> I'm afraid I don't understand how moving the file would solve the
> dependency issues though:
>
> There's a change in sound/soc/... that needs this new file.
>
> There's a change in arch/arm/mach-tegra/... that needs this new file.
>
> Both those two changes would typically get checked into two separate git
> repositories.
>
> As far as I can tell, we could:
>
> a) Check the new file into both repositories, and git will hopefully just
> merge them together in the next merge window. Mark pointed out a long time
> ago that this is generally frowned upon though.
>
> b) Check the change into just one repository, e.g. ASoC since it's ready to
> accept the sound/soc/... changes that rely on it, and then have Tegra's
> for-next merge that (or merge linux-next after it's filtered up there,
> although I'm not sure if merging linux-next is workable with git?) (or
> merge directly from ASoC's for-2.6.39) and only then apply the arch/arm/tegra
> changes that rely on it. Mark pointed out a long time ago that such cross-
> subsystem merges are very rare, although it sounded like they do happen
> sometimes.
>
> c) Only check in the sound/soc/... changes, wait until they're in 2.6.39-rc1
> and hence can be picked up from there for Tegra's for-next, and then check in
> the Tegra changes that rely on it. This would delay the complete changes
> until 2.6.40:-(
>
> I'm still in the dark how this normally works; it seems like it must be
> pretty common to add a new driver to the driver repository, with that driver
> defining a new platform_data type, and also needing to add code to the
> architecture/chip repository to provide that platform_data, and hopefully
> have both those things go in in parallel, rather than being staged across
> two kernel releases.

Could the same platform_data patch go into both ASoC and Tegra?  It
would fall out in the merge, no matter who gets pulled first.  If we
go that route, whichever place you put the header, one of the trees
will have an include file that is in the other tree's directory.

I still think the header makes more sense in include/sound than
mach-tegra, but if the ASoC maintainers prefer it in mach-tegra,
consider the patch Acked-by: Colin Cross <ccross@android.com> for
inclusion in the ASoC tree.

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Mark Brown]

On Fri, Jan 21, 2011 at 02:41:08PM -0800, Colin Cross wrote:

> Could the same platform_data patch go into both ASoC and Tegra?  It
> would fall out in the merge, no matter who gets pulled first.  If we
> go that route, whichever place you put the header, one of the trees
> will have an include file that is in the other tree's directory.

The usual thing here would be a branch that gets merged into both trees
(to avoid the commit getting duplicated) or to only apply in one tree
and let things sort themselves out in -next and the merge window.

> I still think the header makes more sense in include/sound than
> mach-tegra, but if the ASoC maintainers prefer it in mach-tegra,
> consider the patch Acked-by: Colin Cross <ccross@android.com> for
> inclusion in the ASoC tree.

The general style for architecture specific headers is that they get
included in architecture specific directories to avoid general namespace
pollution.

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Stephen Warren]

Mark Brown wrote on Sent: Friday, January 21, 2011 4:42 PM:
>
> On Fri, Jan 21, 2011 at 02:41:08PM -0800, Colin Cross wrote:
> 
> > Could the same platform_data patch go into both ASoC and Tegra?  It
> > would fall out in the merge, no matter who gets pulled first.  If we
> > go that route, whichever place you put the header, one of the trees
> > will have an include file that is in the other tree's directory.
> 
> The usual thing here would be a branch that gets merged into both trees
> (to avoid the commit getting duplicated) or to only apply in one tree and
> let things sort themselves out in -next and the merge window.

Ah yes, I do remember you mentioning that before. Now I realize exactly
what you mean.

I'm quite happy to create a branch for this, cherry-pick my change onto it,
and tell you where it is, so you can merge from it.

What should I base the branch off; since it's a simple file add, I guess
the current mainline tree would be fine.

I assume there are no specific requirements for where the branch is hosted;
i.e. something you can git clone over HTTP from my personal server would
be fine?

Thanks!

-- 
nvpublic

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Olof Johansson]

On Fri, Jan 21, 2011 at 3:49 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Mark Brown wrote on Sent: Friday, January 21, 2011 4:42 PM:
>> The usual thing here would be a branch that gets merged into both trees
>> (to avoid the commit getting duplicated) or to only apply in one tree and
>> let things sort themselves out in -next and the merge window.
>
> Ah yes, I do remember you mentioning that before. Now I realize exactly
> what you mean.


Sounds overly complicated to me since you don't already have a tree
hosted somewhere.

Mark; why don't you apply it with Colin's acked-by, and Colin can just
cherry-pick from that for-next branch?


-Olof

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Stephen Warren]

Olof Johansson wrote on Friday, January 21, 2011 10:15 PM:
>
> On Fri, Jan 21, 2011 at 3:49 PM, Stephen Warren wrote:
> > Mark Brown wrote on Sent: Friday, January 21, 2011 4:42 PM:
> >> The usual thing here would be a branch that gets merged into both trees
> >> (to avoid the commit getting duplicated) or to only apply in one tree and
> >> let things sort themselves out in -next and the merge window.
> >
> > Ah yes, I do remember you mentioning that before. Now I realize exactly
> > what you mean.
> 
> Sounds overly complicated to me since you don't already have a tree
> hosted somewhere.

Well, I did push a kernel repo to my personal server, which would do
fine for this purpose assuming there's no requirement for it to stick
around for any specific amount of time etc.

-- 
nvpublic

Re: [PATCH v3 2/4] ARM: tegra: Add Harmony sound platform data type

[Olof Johansson]

On Fri, Jan 21, 2011 at 9:34 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote on Friday, January 21, 2011 10:15 PM:
>>
>> On Fri, Jan 21, 2011 at 3:49 PM, Stephen Warren wrote:
>> > Mark Brown wrote on Sent: Friday, January 21, 2011 4:42 PM:
>> >> The usual thing here would be a branch that gets merged into both trees
>> >> (to avoid the commit getting duplicated) or to only apply in one tree and
>> >> let things sort themselves out in -next and the merge window.
>> >
>> > Ah yes, I do remember you mentioning that before. Now I realize exactly
>> > what you mean.
>>
>> Sounds overly complicated to me since you don't already have a tree
>> hosted somewhere.
>
> Well, I did push a kernel repo to my personal server, which would do
> fine for this purpose assuming there's no requirement for it to stick
> around for any specific amount of time etc.


Ok, that works too :)


-Olof

[PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

The WM8903 driver now needs platform data in order to correctly expose its
GPIOs through gpiolib.

The top-level ASoC driver now needs a platform device in order to provide
infra-structure to pass in platform data.

The top-level ASoC driver now needs platform data in order to be informed
which GPIO IDs to use.

This is all needed to allow the top-level ASoC driver to configure the
WM8903's GPIO3 as an output GPIO, and enable it as appropriate. This is the
enable signal for the external speaker amplifier chips.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/mach-tegra/board-harmony.c |   31 +++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/gpio-names.h    |    2 ++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
index 2696a20..c5a4ae0 100644
--- a/arch/arm/mach-tegra/board-harmony.c
+++ b/arch/arm/mach-tegra/board-harmony.c
@@ -2,6 +2,7 @@
  * arch/arm/mach-tegra/board-harmony.c
  *
  * Copyright (C) 2010 Google, Inc.
+ * Copyright (C) 2011 NVIDIA, Inc.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -24,12 +25,15 @@
 #include <linux/i2c.h>
 #include <linux/io.h>
 
+#include <sound/wm8903.h>
+
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/time.h>
 #include <asm/setup.h>
 
 #include <mach/gpio.h>
+#include <mach/harmony_audio.h>
 #include <mach/i2c.h>
 #include <mach/iomap.h>
 #include <mach/irqs.h>
@@ -53,6 +57,8 @@
 #define ATAG_NVIDIA_PRESERVED_MEM_N	2
 #define ATAG_NVIDIA_FORCE_32		0x7fffffff
 
+#define GPIO_WM8903(_x_) (TEGRA_MAX_GPIO + 1 + (_x_))
+
 struct tag_tegra {
 	__u32 bootarg_key;
 	__u32 bootarg_len;
@@ -112,9 +118,33 @@ static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
 	.bus_clk_rate	= { 400000, 0 },
 };
 
+static struct wm8903_platform_data harmony_wm8903_pdata = {
+	.gpio_base = GPIO_WM8903(0),
+	.gpio_cfg = {
+		WM8903_GPIO_NO_CONFIG,
+		WM8903_GPIO_NO_CONFIG,
+		0,
+		WM8903_GPIO_NO_CONFIG,
+		WM8903_GPIO_NO_CONFIG,
+	},
+};
+
 static struct i2c_board_info __initdata harmony_i2c_bus1_board_info[] = {
 	{
 		I2C_BOARD_INFO("wm8903", 0x1a),
+		.platform_data = &harmony_wm8903_pdata,
+	},
+};
+
+static struct harmony_audio_platform_data harmony_audio_pdata = {
+	.gpio_spkr_en = GPIO_WM8903(2),
+};
+
+static struct platform_device harmony_audio_device = {
+	.name   = "tegra-snd-harmony",
+	.id     = 0,
+	.dev    = {
+		.platform_data  = &harmony_audio_pdata,
 	},
 };
 
@@ -127,6 +157,7 @@ static struct platform_device *harmony_devices[] __initdata = {
 	&tegra_i2s_device1,
 	&tegra_das_device,
 	&tegra_pcm_device,
+	&harmony_audio_device,
 };
 
 static void __init tegra_harmony_fixup(struct machine_desc *desc,
diff --git a/arch/arm/mach-tegra/gpio-names.h b/arch/arm/mach-tegra/gpio-names.h
index f28220a..e8b5cf3 100644
--- a/arch/arm/mach-tegra/gpio-names.h
+++ b/arch/arm/mach-tegra/gpio-names.h
@@ -244,4 +244,6 @@
 #define TEGRA_GPIO_PBB6		222
 #define TEGRA_GPIO_PBB7		223
 
+#define TEGRA_MAX_GPIO		TEGRA_GPIO_PBB7
+
 #endif
-- 
1.7.1

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Mark Brown]

On Thu, Jan 20, 2011 at 01:52:10PM -0700, Stephen Warren wrote:
> The WM8903 driver now needs platform data in order to correctly expose its
> GPIOs through gpiolib.

This isn't really a fix - it's enabling a new feature that wasn't
previously supported, describing it as a fix is misleading as that'd
suggest it should be applied to the release branch as a bugfix which
would cause build issues.

I'm happy with this version of the series, I guess Liam will be too.
Ideally someone from the Tegra side should sign off on the arch/arm
side - that'll need to go through the same tree as the ASoC side for
build dependencies.

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Liam Girdwood]

On Thu, 2011-01-20 at 21:22 +0000, Mark Brown wrote:
> On Thu, Jan 20, 2011 at 01:52:10PM -0700, Stephen Warren wrote:
> > The WM8903 driver now needs platform data in order to correctly expose its
> > GPIOs through gpiolib.
> 
> This isn't really a fix - it's enabling a new feature that wasn't
> previously supported, describing it as a fix is misleading as that'd
> suggest it should be applied to the release branch as a bugfix which
> would cause build issues.
> 
> I'm happy with this version of the series, I guess Liam will be too.
> Ideally someone from the Tegra side should sign off on the arch/arm
> side - that'll need to go through the same tree as the ASoC side for
> build dependencies.

I'm happy with this too.

Acked-by: Liam girdwood <lrg@slimlogic.co.uk>

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

[I apologize if the email formatting is terrible; my usual system is broken
right now. I hope this is plain text]

Mark Brown wrote on Thursday, January 20, 2011 14:22:
> On Thu, Jan 20, 2011 at 01:52:10PM -0700, Stephen Warren wrote:
> > The WM8903 driver now needs platform data in order to correctly expose its
> > GPIOs through gpiolib.
> 
> This isn't really a fix - it's enabling a new feature that wasn't
> previously supported

Agreed. I'll reword this when I submit it for inclusion in the Tegra tree.

> I'm happy with this version of the series, I guess Liam will be too.
> Ideally someone from the Tegra side should sign off on the arch/arm
> side - that'll need to go through the same tree as the ASoC side for
> build dependencies.

I assume you're only talking about:

a882c75eadfcdc56f47962dd3f7870d6def62847
ARM: tegra: Add Harmony sound platform data type

The other arch/arm patches won't apply anywhere yet since they build
on top of various other patches that haven't been submitted for or
applied to the linux-tegra tree yet.

CC += Colin/Erik to ack/nack the header addition mentioned above.

Thanks

-- 
nvpublic

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Mark Brown]

On Fri, Jan 21, 2011 at 09:45:56AM -0800, Stephen Warren wrote:

> > I'm happy with this version of the series, I guess Liam will be too.
> > Ideally someone from the Tegra side should sign off on the arch/arm
> > side - that'll need to go through the same tree as the ASoC side for
> > build dependencies.

> I assume you're only talking about:

> a882c75eadfcdc56f47962dd3f7870d6def62847
> ARM: tegra: Add Harmony sound platform data type

That and the addition of the defines for function codes IIRC.

> The other arch/arm patches won't apply anywhere yet since they build
> on top of various other patches that haven't been submitted for or
> applied to the linux-tegra tree yet.

Oh, I see.  I hadn't realised there wer eother dependencies, please
ensure you mention any such dependencies in the changlogs so we avoid
introducing breakage.

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

Mark Brown wrote on Friday, January 21, 2011 10:50:
> On Fri, Jan 21, 2011 at 09:45:56AM -0800, Stephen Warren wrote:
> 
> > > I'm happy with this version of the series, I guess Liam will be too.
> > > Ideally someone from the Tegra side should sign off on the arch/arm
> > > side - that'll need to go through the same tree as the ASoC side for
> > > build dependencies.
> 
> > I assume you're only talking about:
> 
> > a882c75eadfcdc56f47962dd3f7870d6def62847
> > ARM: tegra: Add Harmony sound platform data type
> 
> That and the addition of the defines for function codes IIRC.

The wm8903 GPIO function defines? They're in wm8903.h and don't rely
on anything else. The arch/arm patches don't rely on these defines.

> > The other arch/arm patches won't apply anywhere yet since they build
> > on top of various other patches that haven't been submitted for or
> > applied to the linux-tegra tree yet.
> 
> Oh, I see.  I hadn't realised there wer eother dependencies, please
> ensure you mention any such dependencies in the changlogs so we avoid
> introducing breakage.

That was the intent of what I wrote in the "cover letter" "patch".

To be explicit, the only dependency is that the patch quoted above, which
adds some platform data types, needs to be checked in to te ASoC tree
rather than the usual ARM tree, before the following ASoC patch is
applied:

ASoC: tegra: Harmony: Support th internal speaker

Every other ASoC patch I've submitted doesn't rely on anything outside
the ASoC tree to compile, although actually running it usefully obviously
requires the arch/arm changes.

Hope this clears things up!

--
nvpublic

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Mark Brown]

On Fri, Jan 21, 2011 at 10:06:33AM -0800, Stephen Warren wrote:
> Mark Brown wrote on Friday, January 21, 2011 10:50:

> > Oh, I see.  I hadn't realised there wer eother dependencies, please
> > ensure you mention any such dependencies in the changlogs so we avoid
> > introducing breakage.

> That was the intent of what I wrote in the "cover letter" "patch".

It doesn't refer to anything outside the series itself and the current
state of the relevant trees, which seemed to be what you were saying...

It'd also be helpful if you could stop burying patches in old threads -
it makes it hard to find the most recent stuff.

> To be explicit, the only dependency is that the patch quoted above, which
> adds some platform data types, needs to be checked in to te ASoC tree
> rather than the usual ARM tree, before the following ASoC patch is
> applied:

...but possibly not?

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

Mark Brown wrote:
> It'd also be helpful if you could stop burying patches in old threads -
> it makes it hard to find the most recent stuff.

Sorry. I read somewhere that threading updates into the existing thread was the 
preferred way. I'll stop doing it.

> > To be explicit, the only dependency is that the patch quoted above, which
> > adds some platform data types, needs to be checked in to te ASoC tree
> > rather than the usual ARM tree, before the following ASoC patch is
> > applied:
> 
> ...but possibly not?

Sorry, I don't follow. Possibly not what? Are you saying:

a) Possibly not the only dependency? It's certainly the only one I know of.

b) Possibly not that it needs to be checked into the ASoC git repo, even though
the header is located somewhere the Tegra git repo owns?

I'm fine moving the header into some directory that ASoC owns, and fixing my
arch/arm/mach-tegra patch for the new location. That would mean that when I
submit the Tegra changes to the Tegra tree, the Tegra trees needs to have picked
up the ASoC changes first, but that's fine I think? At the least, it defers any issue
until later.

Otherwise, yes, that patch is needed befoer the ASoC patch that uses it, or there
will be a build break, if anyone enables building that driver.

-- 
nvpublic

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Mark Brown]

On Fri, Jan 21, 2011 at 10:22:11AM -0800, Stephen Warren wrote:
> Mark Brown wrote:

> Sorry, I don't follow. Possibly not what? Are you saying:

> a) Possibly not the only dependency? It's certainly the only one I know of.

> b) Possibly not that it needs to be checked into the ASoC git repo, even though
> the header is located somewhere the Tegra git repo owns?

You said in <74CDBE0F657A3D45AFBB94109FB122FF0310AD4251@HQMAIL01.nvidia.com>:

| The other arch/arm patches won't apply anywhere yet since they build
| on top of various other patches that haven't been submitted for or
| applied to the linux-tegra tree yet.

referring to patches 2 and 3 of the series.  To me that says that these
patches won't apply to either the Tegra or ASoC trees as they stand as
they depend on patches you have not yet submitted.

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

Mark Brown wrote:
> > Sorry, I don't follow. Possibly not what? Are you saying:
> 
> > a) Possibly not the only dependency? It's certainly the only one I know of.
> 
> > b) Possibly not that it needs to be checked into the ASoC git repo, even though
> > the header is located somewhere the Tegra git repo owns?
> 
> You said in <74CDBE0F657A3D45AFBB94109FB122FF0310AD4251@HQMAIL01.nvidia.com>:
> 
> | The other arch/arm patches won't apply anywhere yet since they build
> | on top of various other patches that haven't been submitted for or
> | applied to the linux-tegra tree yet.
> 
> referring to patches 2 and 3 of the series.  To me that says that these
> patches won't apply to either the Tegra or ASoC trees as they stand as
> they depend on patches you have not yet submitted.

Patch 1 (wm8903 GPIO support) has no dependencies.

Patch 2 (add header to define Harmony ASoC driver's platform_data)
doesn't depend on anything else, but is required by patches 3 and 4.

This is the patch which is in a path owned by the Tegra tree, but needs to
be checked into the ASoC tree to enable patch 4 to be checked in there.

Patch 3 (Adds various platform data to arch/arm/mach-tegra) depends
on patch 2, and should be checked into the Tegra for-next tree at some
later time. You can ignore this patch; I presented it simply to show a
complete set of patches. Sorry if the cover letter wasn't clear on this point.

Patch 4 (Adds internal speaker support to ASoC driver) depends on patch 2
being in the ASoC tree somehow. Hence, why you asked the Tegra maintainers
to sign off on checking patch 2 into the ASoC tree, as I understand it.

-- 
nvpublic

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Mark Brown]

On Fri, Jan 21, 2011 at 10:36:42AM -0800, Stephen Warren wrote:

> Patch 3 (Adds various platform data to arch/arm/mach-tegra) depends
> on patch 2, and should be checked into the Tegra for-next tree at some
> later time. You can ignore this patch; I presented it simply to show a
> complete set of patches. Sorry if the cover letter wasn't clear on this point.

I've no problem carrying that in ASoC if Colin is happy with it, it
seems much easier to do that than to have to remember to apply it to the
Tegra tree after the 2.6.39 merge window.

> Patch 4 (Adds internal speaker support to ASoC driver) depends on patch 2
> being in the ASoC tree somehow. Hence, why you asked the Tegra maintainers
> to sign off on checking patch 2 into the ASoC tree, as I understand it.

Yes.  It was your subsequent reply about stuff not having been submitted
that made me think that there were still more dependencies I wasn't
aware of.

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

Mark Brown wrote:
> On Fri, Jan 21, 2011 at 10:36:42AM -0800, Stephen Warren wrote:
> 
> > Patch 3 (Adds various platform data to arch/arm/mach-tegra) depends
> > on patch 2, and should be checked into the Tegra for-next tree at some
> > later time. You can ignore this patch; I presented it simply to show a
> > complete set of patches. Sorry if the cover letter wasn't clear on this point.
> 
> I've no problem carrying that in ASoC if Colin is happy with it, it
> seems much easier to do that than to have to remember to apply it to the
> Tegra tree after the 2.6.39 merge window.

I'd like to get everything, or at least most of this, into 2.6.39 if at all possible.
I guess getting just the basic driver (i.e. before this addition of internal speaker
support) would be OK though, since none of the patches for the basic driver
have any cross-subsystem dependencies.

For the basic driver: Everything is already in the ASoC tree. I'll be pushing
patches to support it (i.e. add the required platform_data, and possibly various
Tegra infra-structure such as clock configuration additions and fixes etc.) to the
Tegra for-next tree once it's open to accept patches:

The problem is that the Tegra for-next branch isn't yet open for submissions,
(I was originally directed to provide patches against linux-tegra-2.6.36 and
just recently directed to provided patches against linux-tegra-2.6.37)
so I've been queueing up all the arch/arm-mach-tegra patches for submission
once Tegra for-next opens. Colin and Erik have said offline that this will be soon.

Hence, the arch/arm-mach-tegra changes in this patchset (excluding the simple
standalone addition of that one new header) build on code that is neither in ASoC's nor
Tegra's for-next branch. Hence, you won't be able to carry that patch yourself
since it won't apply, even with manual merging I suspect.

Sorry the situation is so confusing. I hope this explains some of the background.

-- 
nvpublic

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Mark Brown]

On Fri, Jan 21, 2011 at 10:51:15AM -0800, Stephen Warren wrote:

> Hence, the arch/arm-mach-tegra changes in this patchset (excluding the simple
> standalone addition of that one new header) build on code that is neither in ASoC's nor
> Tegra's for-next branch. Hence, you won't be able to carry that patch yourself
> since it won't apply, even with manual merging I suspect.

So just to be absolutely clear, there really are external dependencies
for the arch/arm bits of this patch series that go beyond the patch
series itself?  That's the information I was asking to be flagged on
future submissions.

Re: [PATCH v3 3/4] ARM: tegra: Platform data fixes for ASoC driver updates

[Stephen Warren]

Mark Brown wrote on Friday, January 21, 2011 11:58 AM:
> On Fri, Jan 21, 2011 at 10:51:15AM -0800, Stephen Warren wrote:
> 
> > Hence, the arch/arm-mach-tegra changes in this patchset (excluding the
> > simple standalone addition of that one new header) build on code that
> > is neither in ASoC's nor Tegra's for-next branch. Hence, you won't be
> > able to carry that patch yourself since it won't apply, even with manual
> > merging I suspect.
> 
> So just to be absolutely clear, there really are external dependencies for
> the arch/arm bits of this patch series that go beyond the patch series
> itself?  That's the information I was asking to be flagged on future
> submissions.

Yes, the arch/arm bits (except that standalone patch to add the new header)
depend on a variety of other patches from before this patchset. Those other
patches fix a few missing things in the mainline Tegra support and/or simply
add platform devices and platform data for the new ASoC driver. At least
some of those patches will have to be reworked a little once Tegra's for-next
comes on line.

The sound/soc bits don't depend on anything except what's already in the
ACoC for-2.6.39 branch.

-- 
nvpublic

[PATCH v3 4/4] ASoC: tegra: Harmony: Support the internal speaker

[Stephen Warren]

Add DAPM widget definitions, and code to control the speaker amplifier
GPIO to the Harmony machine driver. Currently, this path is always enabled,
while playback is active, since jack detection isn't yet implemented.

The driver initialization path has been reworked a little, and a new
driver/device introduced, in order to allow platform data to be passed in,
currently containing just the speaker enable GPIO ID.

The GPIO is only requested during _init, since that's the first time it is
guaranteed that the WM8903 module is loaded, probed, and hence has exported
its GPIO chip.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 sound/soc/tegra/harmony.c |   87 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
index b160b71..e424997 100644
--- a/sound/soc/tegra/harmony.c
+++ b/sound/soc/tegra/harmony.c
@@ -2,7 +2,7 @@
  * harmony.c - Harmony machine ASoC driver
  *
  * Author: Stephen Warren <swarren@nvidia.com>
- * Copyright (C) 2010 - NVIDIA, Inc.
+ * Copyright (C) 2010-2011 - NVIDIA, Inc.
  *
  * Based on code copyright/by:
  *
@@ -29,7 +29,14 @@
  */
 
 #include <asm/mach-types.h>
+
+#include <linux/gpio.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <mach/harmony_audio.h>
+
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -40,9 +47,12 @@
 #include "tegra_pcm.h"
 #include "tegra_asoc_utils.h"
 
-#define PREFIX "ASoC Harmony: "
+#define DRV_NAME "tegra-snd-harmony"
+#define PREFIX DRV_NAME ": "
 
+static struct harmony_audio_platform_data *pdata;
 static struct platform_device *harmony_snd_device;
+static int gpio_spkr_en_requested;
 
 static int harmony_asoc_hw_params(struct snd_pcm_substream *substream,
 					struct snd_pcm_hw_params *params)
@@ -107,7 +117,17 @@ static struct snd_soc_ops harmony_asoc_ops = {
 	.hw_params = harmony_asoc_hw_params,
 };
 
+static int harmony_event_int_spk(struct snd_soc_dapm_widget *w,
+					struct snd_kcontrol *k, int event)
+{
+	gpio_set_value_cansleep(pdata->gpio_spkr_en,
+				!!SND_SOC_DAPM_EVENT_ON(event));
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget harmony_dapm_widgets[] = {
+	SND_SOC_DAPM_SPK("Int Spk", harmony_event_int_spk),
 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_MIC("Mic Jack", NULL),
 };
@@ -115,6 +135,10 @@ static const struct snd_soc_dapm_widget harmony_dapm_widgets[] = {
 static const struct snd_soc_dapm_route harmony_audio_map[] = {
 	{"Headphone Jack", NULL, "HPOUTR"},
 	{"Headphone Jack", NULL, "HPOUTL"},
+	{"Int Spk", NULL, "ROP"},
+	{"Int Spk", NULL, "RON"},
+	{"Int Spk", NULL, "LOP"},
+	{"Int Spk", NULL, "LON"},
 	{"Mic Bias", NULL, "Mic Jack"},
 	{"IN1L", NULL, "Mic Bias"},
 };
@@ -123,6 +147,7 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_codec *codec = rtd->codec;
 	struct snd_soc_dapm_context *dapm = &codec->dapm;
+	int ret;
 
 	snd_soc_dapm_new_controls(dapm, harmony_dapm_widgets,
 					ARRAY_SIZE(harmony_dapm_widgets));
@@ -131,9 +156,19 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
 				ARRAY_SIZE(harmony_audio_map));
 
 	snd_soc_dapm_enable_pin(dapm, "Headphone Jack");
+	snd_soc_dapm_enable_pin(dapm, "Int Spk");
 	snd_soc_dapm_enable_pin(dapm, "Mic Jack");
 	snd_soc_dapm_sync(dapm);
 
+	ret = gpio_request(pdata->gpio_spkr_en, "spkr_en");
+	if (ret) {
+		pr_err(PREFIX "cannot get spkr_en gpio\n");
+		return ret;
+	}
+	gpio_spkr_en_requested = 1;
+
+	gpio_direction_output(pdata->gpio_spkr_en, 0);
+
 	return 0;
 }
 
@@ -154,13 +189,19 @@ static struct snd_soc_card snd_soc_harmony = {
 	.num_links = 1,
 };
 
-static int __init harmony_soc_modinit(void)
+static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
 {
 	int ret;
 
-	if (!machine_is_harmony()) {
-		pr_err(PREFIX "Not running on Tegra Harmony!\n");
-		return -ENODEV;
+	if (pdev->id != 0) {
+		dev_err(&pdev->dev, "ID %d out of range\n", pdev->id);
+		return -EINVAL;
+	}
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "no platform data supplied\n");
+		return -EINVAL;
 	}
 
 	ret = tegra_asoc_utils_init();
@@ -173,7 +214,7 @@ static int __init harmony_soc_modinit(void)
 	 */
 	harmony_snd_device = platform_device_alloc("soc-audio", -1);
 	if (harmony_snd_device == NULL) {
-		pr_err(PREFIX "platform_device_alloc failed\n");
+		dev_err(&pdev->dev, "platform_device_alloc failed\n");
 		ret = -ENOMEM;
 		goto err_clock_utils;
 	}
@@ -182,7 +223,7 @@ static int __init harmony_soc_modinit(void)
 
 	ret = platform_device_add(harmony_snd_device);
 	if (ret) {
-		pr_err(PREFIX "platform_device_add failed (%d)\n",
+		dev_err(&pdev->dev, "platform_device_add failed (%d)\n",
 			ret);
 		goto err_device_put;
 	}
@@ -195,15 +236,39 @@ err_clock_utils:
 	tegra_asoc_utils_fini();
 	return ret;
 }
-module_init(harmony_soc_modinit);
 
-static void __exit harmony_soc_modexit(void)
+static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
 {
 	platform_device_unregister(harmony_snd_device);
 
+	if (gpio_spkr_en_requested)
+		gpio_free(pdata->gpio_spkr_en);
+
 	tegra_asoc_utils_fini();
+
+	return 0;
+}
+
+static struct platform_driver tegra_snd_harmony_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+	},
+	.probe = tegra_snd_harmony_probe,
+	.remove = __devexit_p(tegra_snd_harmony_remove),
+};
+
+static int __init snd_tegra_harmony_init(void)
+{
+	return platform_driver_register(&tegra_snd_harmony_driver);
+}
+module_init(snd_tegra_harmony_init);
+
+static void __exit snd_tegra_harmony_exit(void)
+{
+	platform_driver_unregister(&tegra_snd_harmony_driver);
 }
-module_exit(harmony_soc_modexit);
+module_exit(snd_tegra_harmony_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren@nvidia.com>");
 MODULE_DESCRIPTION("Harmony machine ASoC driver");
-- 
1.7.1