[Pull request] Support for wm9705 codec and two machines that use it.

[Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

The following changes since commit 6d3455ecb7d564789e8dd493fb876f4a8706e3b2:
   Mark Brown (1):
         Fix leftover merge issue.

are available in the git repository at:

   git://git.mnementh.co.uk/linux-2.6-im.git asoc

Ian Molton (3):
       ASoc: codec: Driver for wm9705 AC97 codec.
       ASoC: machine driver for Toshiba e750
       ASoC: machine driver for Toshiba e800

  arch/arm/mach-pxa/e750.c                      |    5 +
  arch/arm/mach-pxa/include/mach/eseries-gpio.h |   10 +
  sound/soc/codecs/Kconfig                      |    4 +
  sound/soc/codecs/Makefile                     |    2 +
  sound/soc/codecs/wm9705.c                     |  401 ++++++++++++++++++
  sound/soc/codecs/wm9705.h                     |   14 +
  sound/soc/pxa/Kconfig                         |    9 +
  sound/soc/pxa/Makefile                        |    2 +
  sound/soc/pxa/e750_wm9705.c                   |  185 ++++++++++++
  sound/soc/pxa/e800_wm9712.c                   |  115 +++++++-
  10 files changed, 733 insertions(+), 14 deletions(-)
  create mode 100644 sound/soc/codecs/wm9705.c
  create mode 100644 sound/soc/codecs/wm9705.h
  create mode 100644 sound/soc/pxa/e750_wm9705.c

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Apologies for the misleading subject line - two machines, one using 
wm9705. the other is wm9712 based.

TTFN.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Takashi Iwai]

At Thu, 15 Jan 2009 01:13:52 +0000,
Ian Molton wrote:
> 
> The following changes since commit 6d3455ecb7d564789e8dd493fb876f4a8706e3b2:
>    Mark Brown (1):
>          Fix leftover merge issue.
> 
> are available in the git repository at:
> 
>    git://git.mnementh.co.uk/linux-2.6-im.git asoc

We need reviews.  Could you post patches as well?


thanks,

Takashi
> 
> Ian Molton (3):
>        ASoc: codec: Driver for wm9705 AC97 codec.
>        ASoC: machine driver for Toshiba e750
>        ASoC: machine driver for Toshiba e800
> 
>   arch/arm/mach-pxa/e750.c                      |    5 +
>   arch/arm/mach-pxa/include/mach/eseries-gpio.h |   10 +
>   sound/soc/codecs/Kconfig                      |    4 +
>   sound/soc/codecs/Makefile                     |    2 +
>   sound/soc/codecs/wm9705.c                     |  401 ++++++++++++++++++
>   sound/soc/codecs/wm9705.h                     |   14 +
>   sound/soc/pxa/Kconfig                         |    9 +
>   sound/soc/pxa/Makefile                        |    2 +
>   sound/soc/pxa/e750_wm9705.c                   |  185 ++++++++++++
>   sound/soc/pxa/e800_wm9712.c                   |  115 +++++++-
>   10 files changed, 733 insertions(+), 14 deletions(-)
>   create mode 100644 sound/soc/codecs/wm9705.c
>   create mode 100644 sound/soc/codecs/wm9705.h
>   create mode 100644 sound/soc/pxa/e750_wm9705.c
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Takashi Iwai wrote:

> We need reviews.  Could you post patches as well?

Sure - attached below:

I also noticed that I forgot to run it by checkpatch. It wasn't bad, but 
now checkpatch is silent.

Side note: I wish diffstat on these patches would keep lines under 80 
chars...


  sound/soc/codecs/Kconfig  |    4 +
  sound/soc/codecs/Makefile |    2 +
  sound/soc/codecs/wm9705.c |  401 
+++++++++++++++++++++++++++++++++++++++++++++
  sound/soc/codecs/wm9705.h |   14 ++
  4 files changed, 421 insertions(+), 0 deletions(-)
  create mode 100644 sound/soc/codecs/wm9705.c
  create mode 100644 sound/soc/codecs/wm9705.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index b32a2b5..9f33c07 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -46,6 +46,7 @@ config SND_SOC_ALL_CODECS
  	select SND_SOC_WM8980 if I2C
  	select SND_SOC_WM8990 if I2C
  	select SND_SOC_WM8991 if I2C
+	select SND_SOC_WM9705 if SND_SOC_AC97_BUS
  	select SND_SOC_WM9712 if SND_SOC_AC97_BUS
  	select SND_SOC_WM9713 if SND_SOC_AC97_BUS
          help
@@ -190,6 +191,9 @@ config SND_SOC_WM8990
  config SND_SOC_WM8991
  	tristate

+config SND_SOC_WM9705
+	tristate
+
  config SND_SOC_WM9712
  	tristate

diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 0a0c9dd..9c61037 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -37,6 +37,7 @@ snd-soc-wm8978-objs := wm8978.o
  snd-soc-wm8980-objs := wm8980.o
  snd-soc-wm8990-objs := wm8990.o
  snd-soc-wm8991-objs := wm8991.o
+snd-soc-wm9705-objs := wm9705.o
  snd-soc-wm9712-objs := wm9712.o
  snd-soc-wm9713-objs := wm9713.o

@@ -78,5 +79,6 @@ obj-$(CONFIG_SND_SOC_WM8978)	+= snd-soc-wm8978.o
  obj-$(CONFIG_SND_SOC_WM8980)	+= snd-soc-wm8980.o
  obj-$(CONFIG_SND_SOC_WM8990)	+= snd-soc-wm8990.o
  obj-$(CONFIG_SND_SOC_WM8991)	+= snd-soc-wm8991.o
+obj-$(CONFIG_SND_SOC_WM9705)	+= snd-soc-wm9705.o
  obj-$(CONFIG_SND_SOC_WM9712)	+= snd-soc-wm9712.o
  obj-$(CONFIG_SND_SOC_WM9713)	+= snd-soc-wm9713.o
diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
new file mode 100644
index 0000000..4ff6a84
--- /dev/null
+++ b/sound/soc/codecs/wm9705.c
@@ -0,0 +1,401 @@
+/*
+ * wm9705.c  --  ALSA Soc WM9705 codec support
+ *
+ * Copyright 2008 Ian Molton <spyro@f2s.com>
+ *
+ *  This program is free software; you can redistribute  it and/or 
modify it
+ *  under  the terms of  the GNU General  Public License as published 
by the
+ *  Free Software Foundation; Version 2 of the  License only.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/version.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/ac97_codec.h>
+#include <sound/initval.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+
+/*
+ * WM9705 register cache
+ */
+static const u16 wm9705_reg[] = {
+	0x6174, 0x8000, 0x8000, 0x8000, /* 0x0  */
+	0x0000, 0x8000, 0x8008, 0x8008, /* 0x8  */
+	0x8808, 0x8808, 0x8808, 0x8808, /* 0x10 */
+	0x8808, 0x0000, 0x8000, 0x0000, /* 0x18 */
+	0x0000, 0x0000, 0x0000, 0x000f, /* 0x20 */
+	0x0605, 0x0000, 0xbb80, 0x0000, /* 0x28 */
+	0x0000, 0xbb80, 0x0000, 0x0000, /* 0x30 */
+	0x0000, 0x2000, 0x0000, 0x0000, /* 0x38 */
+	0x0000, 0x0000, 0x0000, 0x0000, /* 0x40 */
+	0x0000, 0x0000, 0x0000, 0x0000, /* 0x48 */
+	0x0000, 0x0000, 0x0000, 0x0000, /* 0x50 */
+	0x0000, 0x0000, 0x0000, 0x0000, /* 0x58 */
+	0x0000, 0x0000, 0x0000, 0x0000, /* 0x60 */
+	0x0000, 0x0000, 0x0000, 0x0000, /* 0x68 */
+	0x0000, 0x0808, 0x0000, 0x0006, /* 0x70 */
+	0x0000, 0x0000, 0x574d, 0x4c05, /* 0x78 */
+};
+
+static const struct snd_kcontrol_new wm9705_snd_ac97_controls[] = {
+	SOC_DOUBLE("Master Playback Volume", AC97_MASTER, 8, 0, 31, 1),
+	SOC_SINGLE("Master Playback Switch", AC97_MASTER, 15, 1, 1),
+	SOC_DOUBLE("Headphone Playback Volume", AC97_HEADPHONE, 8, 0, 31, 1),
+	SOC_SINGLE("Headphone Playback Switch", AC97_HEADPHONE, 15, 1, 1),
+	SOC_DOUBLE("PCM Playback Volume", AC97_PCM, 8, 0, 31, 1),
+	SOC_SINGLE("PCM Playback Switch", AC97_PCM, 15, 1, 1),
+	SOC_SINGLE("Mono Playback Volume", AC97_MASTER_MONO, 0, 31, 1),
+	SOC_SINGLE("Mono Playback Switch", AC97_MASTER_MONO, 15, 1, 1),
+	SOC_SINGLE("PCBeep Playback Volume", AC97_PC_BEEP, 1, 15, 1),
+	SOC_SINGLE("Phone Playback Volume", AC97_PHONE, 0, 31, 1),
+	SOC_DOUBLE("Line Playback Volume", AC97_LINE, 8, 0, 31, 1),
+	SOC_DOUBLE("CD Playback Volume", AC97_CD, 8, 0, 31, 1),
+	SOC_SINGLE("Mic Playback Volume", AC97_MIC, 0, 31, 1),
+	SOC_SINGLE("Mic 20dB Boost Switch", AC97_MIC, 6, 1, 0),
+	SOC_DOUBLE("PCM Capture Volume", AC97_REC_GAIN, 8, 0, 15, 0),
+	SOC_SINGLE("PCM Capture Switch", AC97_REC_GAIN, 15, 1, 1),
+};
+
+static const char *wm9705_mic[] = {"Mic 1", "Mic 2"};
+static const char *wm9705_rec_sel[] = {"Mic", "CD", "NC", "NC",
+	"Line", "Stereo Mix", "Mono Mix", "Phone"};
+
+static const struct soc_enum wm9705_enum_mic =
+	SOC_ENUM_SINGLE(AC97_GENERAL_PURPOSE, 8, 2, wm9705_mic);
+static const struct soc_enum wm9705_enum_rec_l =
+	SOC_ENUM_SINGLE(AC97_REC_SEL, 8, 8, wm9705_rec_sel);
+static const struct soc_enum wm9705_enum_rec_r =
+	SOC_ENUM_SINGLE(AC97_REC_SEL, 0, 8, wm9705_rec_sel);
+
+/* Headphone Mixer */
+static const struct snd_kcontrol_new wm9705_hp_mixer_controls[] = {
+	SOC_DAPM_SINGLE("PCBeep Playback Switch", AC97_PC_BEEP, 15, 1, 1),
+	SOC_DAPM_SINGLE("CD Playback Switch", AC97_CD, 15, 1, 1),
+	SOC_DAPM_SINGLE("Mic Playback Switch", AC97_MIC, 15, 1, 1),
+	SOC_DAPM_SINGLE("Phone Playback Switch", AC97_PHONE, 15, 1, 1),
+	SOC_DAPM_SINGLE("Line Playback Switch", AC97_LINE, 15, 1, 1),
+};
+
+/* Mic source */
+static const struct snd_kcontrol_new wm9705_mic_src_controls =
+	SOC_DAPM_ENUM("Route", wm9705_enum_mic);
+
+/* Capture source */
+static const struct snd_kcontrol_new wm9705_capture_selectl_controls =
+	SOC_DAPM_ENUM("Route", wm9705_enum_rec_l);
+static const struct snd_kcontrol_new wm9705_capture_selectr_controls =
+	SOC_DAPM_ENUM("Route", wm9705_enum_rec_r);
+
+/* DAPM widgets */
+static const struct snd_soc_dapm_widget wm9705_dapm_widgets[] = {
+	SND_SOC_DAPM_MUX("Mic Source", SND_SOC_NOPM, 0, 0,
+		&wm9705_mic_src_controls),
+	SND_SOC_DAPM_MUX("Left Capture Source", SND_SOC_NOPM, 0, 0,
+		&wm9705_capture_selectl_controls),
+	SND_SOC_DAPM_MUX("Right Capture Source", SND_SOC_NOPM, 0, 0,
+		&wm9705_capture_selectr_controls),
+	SND_SOC_DAPM_DAC("Left DAC", "Left HiFi Playback",
+		SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_DAC("Right DAC", "Right HiFi Playback",
+		SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_MIXER_NAMED_CTL("HP Mixer", SND_SOC_NOPM, 0, 0,
+		&wm9705_hp_mixer_controls[0],
+		ARRAY_SIZE(wm9705_hp_mixer_controls)),
+	SND_SOC_DAPM_MIXER("Mono Mixer", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_ADC("Left ADC", "Left HiFi Capture", SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_ADC("Right ADC", "Right HiFi Capture", SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_PGA("Headphone PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Speaker PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Line PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Line out PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Mono PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Phone PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Mic PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("PCBEEP PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("CD PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("ADC PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
+	SND_SOC_DAPM_OUTPUT("HPOUTL"),
+	SND_SOC_DAPM_OUTPUT("HPOUTR"),
+	SND_SOC_DAPM_OUTPUT("LOUT"),
+	SND_SOC_DAPM_OUTPUT("ROUT"),
+	SND_SOC_DAPM_OUTPUT("MONOOUT"),
+	SND_SOC_DAPM_INPUT("PHONE"),
+	SND_SOC_DAPM_INPUT("LINEINL"),
+	SND_SOC_DAPM_INPUT("LINEINR"),
+	SND_SOC_DAPM_INPUT("CDINL"),
+	SND_SOC_DAPM_INPUT("CDINR"),
+	SND_SOC_DAPM_INPUT("PCBEEP"),
+	SND_SOC_DAPM_INPUT("MIC1"),
+	SND_SOC_DAPM_INPUT("MIC2"),
+};
+
+/* Audio map
+ * WM9705 has no switches to disable the route from the inputs to the 
HP mixer
+ * so in order to prevent active inputs from forcing the audio outputs 
to be
+ * constantly enabled, we use the mutes on those inputs to simulate such
+ * controls.
+ */
+static const struct snd_soc_dapm_route audio_map[] = {
+	/* HP mixer */
+	{"HP Mixer", "PCBeep Playback Switch", "PCBEEP PGA"},
+	{"HP Mixer", "CD Playback Switch", "CD PGA"},
+	{"HP Mixer", "Mic Playback Switch", "Mic PGA"},
+	{"HP Mixer", "Phone Playback Switch", "Phone PGA"},
+	{"HP Mixer", "Line Playback Switch", "Line PGA"},
+	{"HP Mixer", NULL, "Left DAC"},
+	{"HP Mixer", NULL, "Right DAC"},
+
+	/* mono mixer */
+	{"Mono Mixer", NULL, "HP Mixer"},
+
+	/* outputs */
+	{"Headphone PGA", NULL, "HP Mixer"},
+	{"HPOUTL", NULL, "Headphone PGA"},
+	{"HPOUTR", NULL, "Headphone PGA"},
+	{"Line out PGA", NULL, "HP Mixer"},
+	{"LOUT", NULL, "Line out PGA"},
+	{"ROUT", NULL, "Line out PGA"},
+	{"Mono PGA", NULL, "Mono Mixer"},
+	{"MONOOUT", NULL, "Mono PGA"},
+
+	/* inputs */
+	{"CD PGA", NULL, "CDINL"},
+	{"CD PGA", NULL, "CDINR"},
+	{"Line PGA", NULL, "LINEINL"},
+	{"Line PGA", NULL, "LINEINR"},
+	{"Phone PGA", NULL, "PHONE"},
+	{"Mic Source", "Mic 1", "MIC1"},
+	{"Mic Source", "Mic 2", "MIC2"},
+	{"Mic PGA", NULL, "Mic Source"},
+	{"PCBEEP PGA", NULL, "PCBEEP"},
+
+	/* Left capture selector */
+	{"Left Capture Source", "Mic", "Mic Source"},
+	{"Left Capture Source", "CD", "CDINL"},
+	{"Left Capture Source", "Line", "LINEINL"},
+	{"Left Capture Source", "Stereo Mix", "HP Mixer"},
+	{"Left Capture Source", "Mono Mix", "HP Mixer"},
+	{"Left Capture Source", "Phone", "PHONE"},
+
+	/* Right capture source */
+	{"Right Capture Source", "Mic", "Mic Source"},
+	{"Right Capture Source", "CD", "CDINR"},
+	{"Right Capture Source", "Line", "LINEINR"},
+	{"Right Capture Source", "Stereo Mix", "HP Mixer"},
+	{"Right Capture Source", "Mono Mix", "HP Mixer"},
+	{"Right Capture Source", "Phone", "PHONE"},
+
+	{"ADC PGA", NULL, "Left Capture Source"},
+	{"ADC PGA", NULL, "Right Capture Source"},
+
+	/* ADC's */
+	{"Left ADC",  NULL, "ADC PGA"},
+	{"Right ADC", NULL, "ADC PGA"},
+};
+
+static int wm9705_add_widgets(struct snd_soc_codec *codec)
+{
+	snd_soc_dapm_new_controls(codec, wm9705_dapm_widgets,
+					ARRAY_SIZE(wm9705_dapm_widgets));
+	snd_soc_dapm_add_routes(codec, audio_map, ARRAY_SIZE(audio_map));
+	snd_soc_dapm_new_widgets(codec);
+
+	return 0;
+}
+
+/* We use a register cache to enhance read performance. */
+static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int 
reg)
+{
+	u16 *cache = codec->reg_cache;
+
+	switch (reg) {
+	case AC97_RESET:
+	case AC97_VENDOR_ID1:
+	case AC97_VENDOR_ID2:
+		return soc_ac97_ops.read(codec->ac97, reg);
+	default:
+		reg = reg >> 1;
+
+		if (reg > (ARRAY_SIZE(wm9705_reg)))
+			return -EIO;
+
+		return cache[reg];
+	}
+}
+
+static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
+	unsigned int val)
+{
+	u16 *cache = codec->reg_cache;
+
+	soc_ac97_ops.write(codec->ac97, reg, val);
+	reg = reg >> 1;
+	if (reg <= (ARRAY_SIZE(wm9705_reg)))
+		cache[reg] = val;
+
+	return 0;
+}
+
+static int ac97_prepare(struct snd_pcm_substream *substream,
+			struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_device *socdev = rtd->socdev;
+	struct snd_soc_codec *codec = socdev->codec;
+	int reg;
+	u16 vra;
+
+	vra = ac97_read(codec, AC97_EXTENDED_STATUS);
+	ac97_write(codec, AC97_EXTENDED_STATUS, vra | 0x1);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		reg = AC97_PCM_FRONT_DAC_RATE;
+	else
+		reg = AC97_PCM_LR_ADC_RATE;
+
+	return ac97_write(codec, reg, runtime->rate);
+}
+
+#define WM9705_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | \
+			SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \
+			SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \
+			SNDRV_PCM_RATE_48000)
+
+struct snd_soc_dai wm9705_dai[] = {
+	{
+		.name = "AC97 HiFi",
+		.ac97_control = 1,
+		.playback = {
+			.stream_name = "HiFi Playback",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = WM9705_AC97_RATES,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+		.capture = {
+			.stream_name = "HiFi Capture",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = WM9705_AC97_RATES,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+		.ops = {
+			.prepare = ac97_prepare,
+		},
+	},
+	{
+		.name = "AC97 Aux",
+		.playback = {
+			.stream_name = "Aux Playback",
+			.channels_min = 1,
+			.channels_max = 1,
+			.rates = WM9705_AC97_RATES,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+	}
+};
+EXPORT_SYMBOL_GPL(wm9705_dai);
+
+static int wm9705_soc_probe(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec;
+	int ret = 0;
+
+	printk(KERN_INFO "WM9705 SoC Audio Codec\n");
+
+	socdev->codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL);
+	if (socdev->codec == NULL)
+		return -ENOMEM;
+	codec = socdev->codec;
+	mutex_init(&codec->mutex);
+
+	codec->reg_cache =
+		kzalloc(sizeof(u16) * ARRAY_SIZE(wm9705_reg), GFP_KERNEL);
+	if (codec->reg_cache == NULL) {
+		ret = -ENOMEM;
+		goto cache_err;
+	}
+	memcpy(codec->reg_cache, wm9705_reg,
+		sizeof(u16) * ARRAY_SIZE(wm9705_reg));
+	codec->reg_cache_size = sizeof(u16) * ARRAY_SIZE(wm9705_reg);
+	codec->reg_cache_step = 2;
+
+	codec->name = "WM9705";
+	codec->owner = THIS_MODULE;
+	codec->dai = wm9705_dai;
+	codec->num_dai = ARRAY_SIZE(wm9705_dai);
+	codec->write = ac97_write;
+	codec->read = ac97_read;
+	INIT_LIST_HEAD(&codec->dapm_widgets);
+	INIT_LIST_HEAD(&codec->dapm_paths);
+
+	ret = snd_soc_new_ac97_codec(codec, &soc_ac97_ops, 0);
+	if (ret < 0) {
+		printk(KERN_ERR "wm9705: failed to register AC97 codec\n");
+		goto codec_err;
+	}
+
+	/* register pcms */
+	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
+	if (ret < 0)
+		goto pcm_err;
+
+	soc_ac97_ops.reset(codec->ac97);
+
+	snd_soc_add_controls(codec, wm9705_snd_ac97_controls,
+				ARRAY_SIZE(wm9705_snd_ac97_controls));
+	wm9705_add_widgets(codec);
+
+	ret = snd_soc_init_card(socdev);
+	if (ret < 0) {
+		printk(KERN_ERR "wm9705: failed to register card\n");
+		goto pcm_err;
+	}
+
+	return 0;
+
+pcm_err:
+	snd_soc_free_ac97_codec(codec);
+
+codec_err:
+	kfree(codec->reg_cache);
+
+cache_err:
+	kfree(socdev->codec);
+	socdev->codec = NULL;
+	return ret;
+}
+
+static int wm9705_soc_remove(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = socdev->codec;
+
+	if (codec == NULL)
+		return 0;
+
+	snd_soc_dapm_free(socdev);
+	snd_soc_free_pcms(socdev);
+	snd_soc_free_ac97_codec(codec);
+	kfree(codec->reg_cache);
+	kfree(codec);
+	return 0;
+}
+
+struct snd_soc_codec_device soc_codec_dev_wm9705 = {
+	.probe = 	wm9705_soc_probe,
+	.remove = 	wm9705_soc_remove,
+};
+EXPORT_SYMBOL_GPL(soc_codec_dev_wm9705);
+
+MODULE_DESCRIPTION("ASoC WM9705 driver");
+MODULE_AUTHOR("Ian Molton");
+MODULE_LICENSE("GPL v2");
diff --git a/sound/soc/codecs/wm9705.h b/sound/soc/codecs/wm9705.h
new file mode 100644
index 0000000..0f46e66
--- /dev/null
+++ b/sound/soc/codecs/wm9705.h
@@ -0,0 +1,14 @@
+/*
+ * wm9705.h  --  WM9705 Soc Audio driver
+ */
+
+#ifndef _WM9705_H
+#define _WM9705_H
+
+#define WM9705_DAI_AC97_HIFI	0
+#define WM9705_DAI_AC97_AUX		1
+
+extern struct snd_soc_dai wm9705_dai[2];
+extern struct snd_soc_codec_device soc_codec_dev_wm9705;
+
+#endif
-- 
1.5.6.5

 From c1e79376dc51eaae0bd2550029cd0189edfc3722 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Thu, 8 Jan 2009 21:03:55 +0000
Subject: [PATCH] ASoC: machine driver for Toshiba e750

This patch adds support for the wm9705 ac97 codec as used in the Toshiba 
e750
PDA. It includes support for powering up / down the external headphone and
speaker amplifiers on this machine.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
  arch/arm/mach-pxa/e750.c                      |    5 +
  arch/arm/mach-pxa/include/mach/eseries-gpio.h |    5 +
  sound/soc/pxa/Kconfig                         |    9 ++
  sound/soc/pxa/Makefile                        |    2 +
  sound/soc/pxa/e750_wm9705.c                   |  189 
+++++++++++++++++++++++++
  5 files changed, 210 insertions(+), 0 deletions(-)
  create mode 100644 sound/soc/pxa/e750_wm9705.c

diff --git a/arch/arm/mach-pxa/e750.c b/arch/arm/mach-pxa/e750.c
index be1ab8e..665066f 100644
--- a/arch/arm/mach-pxa/e750.c
+++ b/arch/arm/mach-pxa/e750.c
@@ -133,6 +133,11 @@ static unsigned long e750_pin_config[] __initdata = {
  	/* IrDA */
  	GPIO38_GPIO | MFP_LPM_DRIVE_HIGH,

+	/* Audio power control */
+	GPIO4_GPIO,  /* Headphone amp power */
+	GPIO7_GPIO,  /* Speaker amp power */
+	GPIO37_GPIO, /* Headphone detect */
+
  	/* PC Card */
  	GPIO8_GPIO,   /* CD0 */
  	GPIO44_GPIO,  /* CD1 */
diff --git a/arch/arm/mach-pxa/include/mach/eseries-gpio.h 
b/arch/arm/mach-pxa/include/mach/eseries-gpio.h
index efbd2aa..02b28e0 100644
--- a/arch/arm/mach-pxa/include/mach/eseries-gpio.h
+++ b/arch/arm/mach-pxa/include/mach/eseries-gpio.h
@@ -45,6 +45,11 @@
  /* e7xx IrDA power control */
  #define GPIO_E7XX_IR_OFF         38

+/* e750 audio control GPIOs */
+#define GPIO_E750_HP_AMP_OFF      4
+#define GPIO_E750_SPK_AMP_OFF     7
+#define GPIO_E750_HP_DETECT      37
+
  /* ASIC related GPIOs */
  #define GPIO_ESERIES_TMIO_IRQ        5
  #define GPIO_ESERIES_TMIO_PCLR      19
diff --git a/sound/soc/pxa/Kconfig b/sound/soc/pxa/Kconfig
index a00066f..5fb9513 100644
--- a/sound/soc/pxa/Kconfig
+++ b/sound/soc/pxa/Kconfig
@@ -61,6 +61,15 @@ config SND_PXA2XX_SOC_TOSA
  	  Say Y if you want to add support for SoC audio on Sharp
  	  Zaurus SL-C6000x models (Tosa).

+config SND_PXA2XX_SOC_E750
+	tristate "SoC AC97 Audio support for e750"
+	depends on SND_PXA2XX_SOC && MACH_E750
+	select SND_SOC_WM9705
+	select SND_PXA2XX_SOC_AC97
+	help
+	  Say Y if you want to add support for SoC audio on the
+	  toshiba e750 PDA
+
  config SND_PXA2XX_SOC_E800
  	tristate "SoC AC97 Audio support for e800"
  	depends on SND_PXA2XX_SOC && MACH_E800
diff --git a/sound/soc/pxa/Makefile b/sound/soc/pxa/Makefile
index bf974b1..9c7a2a0 100644
--- a/sound/soc/pxa/Makefile
+++ b/sound/soc/pxa/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_SND_PXA_SOC_SSP) += snd-soc-pxa-ssp.o
  snd-soc-corgi-objs := corgi.o
  snd-soc-poodle-objs := poodle.o
  snd-soc-tosa-objs := tosa.o
+snd-soc-e750-objs := e750_wm9705.o
  snd-soc-e800-objs := e800_wm9712.o
  snd-soc-em-x270-objs := em-x270.o
  snd-soc-spitz-objs := spitz.o
@@ -35,6 +36,7 @@ snd-soc-zylonite-objs := zylonite.o
  obj-$(CONFIG_SND_PXA2XX_SOC_CORGI) += snd-soc-corgi.o
  obj-$(CONFIG_SND_PXA2XX_SOC_POODLE) += snd-soc-poodle.o
  obj-$(CONFIG_SND_PXA2XX_SOC_TOSA) += snd-soc-tosa.o
+obj-$(CONFIG_SND_PXA2XX_SOC_E750) += snd-soc-e750.o
  obj-$(CONFIG_SND_PXA2XX_SOC_E800) += snd-soc-e800.o
  obj-$(CONFIG_SND_PXA2XX_SOC_EM_X270) += snd-soc-em-x270.o
  obj-$(CONFIG_SND_PXA2XX_SOC_SPITZ) += snd-soc-spitz.o
diff --git a/sound/soc/pxa/e750_wm9705.c b/sound/soc/pxa/e750_wm9705.c
new file mode 100644
index 0000000..20fbdcf
--- /dev/null
+++ b/sound/soc/pxa/e750_wm9705.c
@@ -0,0 +1,189 @@
+/*
+ * e750-wm9705.c  --  SoC audio for e750
+ *
+ * Copyright 2007 (c) Ian Molton <spyro@f2s.com>
+ *
+ *  This program is free software; you can redistribute  it and/or 
modify it
+ *  under  the terms of  the GNU General  Public License as published 
by the
+ *  Free Software Foundation; version 2 ONLY.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/gpio.h>
+
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+
+#include <mach/pxa-regs.h>
+#include <mach/hardware.h>
+#include <mach/audio.h>
+#include <mach/eseries-gpio.h>
+
+#include <asm/mach-types.h>
+
+#include "../codecs/wm9705.h"
+#include "pxa2xx-pcm.h"
+#include "pxa2xx-ac97.h"
+
+static int e750_spk_amp_event(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *kcontrol, int event)
+{
+	if (event & SND_SOC_DAPM_PRE_PMU)
+		gpio_set_value(GPIO_E750_SPK_AMP_OFF, 0);
+	else if (event & SND_SOC_DAPM_POST_PMD)
+		gpio_set_value(GPIO_E750_SPK_AMP_OFF, 1);
+
+	return 0;
+}
+
+static int e750_hp_amp_event(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *kcontrol, int event)
+{
+	if (event & SND_SOC_DAPM_PRE_PMU)
+		gpio_set_value(GPIO_E750_HP_AMP_OFF, 0);
+	else if (event & SND_SOC_DAPM_POST_PMD)
+		gpio_set_value(GPIO_E750_HP_AMP_OFF, 1);
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget e750_dapm_widgets[] = {
+	SND_SOC_DAPM_HP("Headphone Jack", NULL),
+	SND_SOC_DAPM_SPK("Speaker", NULL),
+	SND_SOC_DAPM_MIC("Mic (Internal)", NULL),
+	SND_SOC_DAPM_PGA_E("Headphone Amp", SND_SOC_NOPM, 0, 0, NULL, 0,
+			e750_hp_amp_event, SND_SOC_DAPM_PRE_PMU |
+			SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_PGA_E("Speaker Amp", SND_SOC_NOPM, 0, 0, NULL, 0,
+			e750_spk_amp_event, SND_SOC_DAPM_PRE_PMU |
+			SND_SOC_DAPM_POST_PMD),
+};
+
+static const struct snd_soc_dapm_route audio_map[] = {
+	{"Headphone Amp", NULL, "HPOUTL"},
+	{"Headphone Amp", NULL, "HPOUTR"},
+	{"Headphone Jack", NULL, "Headphone Amp"},
+
+	{"Speaker Amp", NULL, "MONOOUT"},
+	{"Speaker", NULL, "Speaker Amp"},
+
+	{"MIC1", NULL, "Mic (Internal)"},
+};
+
+static int e750_ac97_init(struct snd_soc_codec *codec)
+{
+	snd_soc_dapm_nc_pin(codec, "LOUT");
+	snd_soc_dapm_nc_pin(codec, "ROUT");
+	snd_soc_dapm_nc_pin(codec, "PHONE");
+	snd_soc_dapm_nc_pin(codec, "LINEINL");
+	snd_soc_dapm_nc_pin(codec, "LINEINR");
+	snd_soc_dapm_nc_pin(codec, "CDINL");
+	snd_soc_dapm_nc_pin(codec, "CDINR");
+	snd_soc_dapm_nc_pin(codec, "PCBEEP");
+	snd_soc_dapm_nc_pin(codec, "MIC2");
+
+	snd_soc_dapm_new_controls(codec, e750_dapm_widgets,
+					ARRAY_SIZE(e750_dapm_widgets));
+
+	snd_soc_dapm_add_routes(codec, audio_map, ARRAY_SIZE(audio_map));
+
+	snd_soc_dapm_sync(codec);
+
+	return 0;
+}
+
+static struct snd_soc_dai_link e750_dai[] = {
+	{
+		.name = "AC97",
+		.stream_name = "AC97 HiFi",
+		.cpu_dai = &pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI],
+		.codec_dai = &wm9705_dai[WM9705_DAI_AC97_HIFI],
+		.init = e750_ac97_init,
+		/* use ops to check startup state */
+	},
+	{
+		.name = "AC97 Aux",
+		.stream_name = "AC97 Aux",
+		.cpu_dai = &pxa_ac97_dai[PXA2XX_DAI_AC97_AUX],
+		.codec_dai = &wm9705_dai[WM9705_DAI_AC97_AUX],
+	},
+};
+
+static struct snd_soc_card e750 = {
+	.name = "Toshiba e750",
+	.platform = &pxa2xx_soc_platform,
+	.dai_link = e750_dai,
+	.num_links = ARRAY_SIZE(e750_dai),
+};
+
+static struct snd_soc_device e750_snd_devdata = {
+	.card = &e750,
+	.codec_dev = &soc_codec_dev_wm9705,
+};
+
+static struct platform_device *e750_snd_device;
+
+static int __init e750_init(void)
+{
+	int ret;
+
+	if (!machine_is_e750())
+		return -ENODEV;
+
+	ret = gpio_request(GPIO_E750_HP_AMP_OFF,  "Headphone amp");
+	if (ret)
+		return ret;
+
+	ret = gpio_request(GPIO_E750_SPK_AMP_OFF, "Speaker amp");
+	if (ret)
+		goto free_hp_amp_gpio;
+
+	ret = gpio_direction_output(GPIO_E750_HP_AMP_OFF, 1);
+	if (ret)
+		goto free_spk_amp_gpio;
+
+	ret = gpio_direction_output(GPIO_E750_SPK_AMP_OFF, 1);
+	if (ret)
+		goto free_spk_amp_gpio;
+
+	e750_snd_device = platform_device_alloc("soc-audio", -1);
+	if (!e750_snd_device) {
+		ret = -ENOMEM;
+		goto free_spk_amp_gpio;
+	}
+
+	platform_set_drvdata(e750_snd_device, &e750_snd_devdata);
+	e750_snd_devdata.dev = &e750_snd_device->dev;
+	ret = platform_device_add(e750_snd_device);
+
+	if (!ret)
+		return 0;
+
+/* Fail gracefully */
+	platform_device_put(e750_snd_device);
+free_spk_amp_gpio:
+	gpio_free(GPIO_E750_SPK_AMP_OFF);
+free_hp_amp_gpio:
+	gpio_free(GPIO_E750_HP_AMP_OFF);
+
+	return ret;
+}
+
+static void __exit e750_exit(void)
+{
+	platform_device_unregister(e750_snd_device);
+	gpio_free(GPIO_E750_SPK_AMP_OFF);
+	gpio_free(GPIO_E750_HP_AMP_OFF);
+}
+
+module_init(e750_init);
+module_exit(e750_exit);
+
+/* Module information */
+MODULE_AUTHOR("Ian Molton <spyro@f2s.com>");
+MODULE_DESCRIPTION("ALSA SoC driver for e750");
+MODULE_LICENSE("GPL v2");
-- 
1.5.6.5

 From aa97a30fdc180c73d7af89debb3324f598ec5705 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Thu, 8 Jan 2009 21:16:05 +0000
Subject: [PATCH] ASoC: machine driver for Toshiba e800

This patch adds support for the wm9712 ac97 codec as used in the Toshiba 
e800
PDA. It includes support for powering up / down the external headphone and
speaker amplifiers on this machine.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
  arch/arm/mach-pxa/include/mach/eseries-gpio.h |    5 +
  sound/soc/pxa/e800_wm9712.c                   |  116 
++++++++++++++++++++++---
  2 files changed, 107 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/eseries-gpio.h 
b/arch/arm/mach-pxa/include/mach/eseries-gpio.h
index 02b28e0..6d6e4d8 100644
--- a/arch/arm/mach-pxa/include/mach/eseries-gpio.h
+++ b/arch/arm/mach-pxa/include/mach/eseries-gpio.h
@@ -50,6 +50,11 @@
  #define GPIO_E750_SPK_AMP_OFF     7
  #define GPIO_E750_HP_DETECT      37

+/* e800 audio control GPIOs */
+#define GPIO_E800_HP_DETECT      81
+#define GPIO_E800_HP_AMP_OFF     82
+#define GPIO_E800_SPK_AMP_ON     83
+
  /* ASIC related GPIOs */
  #define GPIO_ESERIES_TMIO_IRQ        5
  #define GPIO_ESERIES_TMIO_PCLR      19
diff --git a/sound/soc/pxa/e800_wm9712.c b/sound/soc/pxa/e800_wm9712.c
index 2e3386d..78a1770 100644
--- a/sound/soc/pxa/e800_wm9712.c
+++ b/sound/soc/pxa/e800_wm9712.c
@@ -1,8 +1,6 @@
  /*
   * e800-wm9712.c  --  SoC audio for e800
   *
- * Based on tosa.c
- *
   * Copyright 2007 (c) Ian Molton <spyro@f2s.com>
   *
   *  This program is free software; you can redistribute  it and/or 
modify it
@@ -13,31 +11,96 @@

  #include <linux/module.h>
  #include <linux/moduleparam.h>
-#include <linux/device.h>
+#include <linux/gpio.h>

  #include <sound/core.h>
  #include <sound/pcm.h>
  #include <sound/soc.h>
  #include <sound/soc-dapm.h>

-#include <asm/mach-types.h>
  #include <mach/pxa-regs.h>
  #include <mach/hardware.h>
  #include <mach/audio.h>
+#include <mach/eseries-gpio.h>
+
+#include <asm/mach-types.h>

  #include "../codecs/wm9712.h"
  #include "pxa2xx-pcm.h"
  #include "pxa2xx-ac97.h"

-static struct snd_soc_card e800;
+static int e800_spk_amp_event(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *kcontrol, int event)
+{
+	if (event & SND_SOC_DAPM_PRE_PMU)
+		gpio_set_value(GPIO_E800_SPK_AMP_ON, 1);
+	else if (event & SND_SOC_DAPM_POST_PMD)
+		gpio_set_value(GPIO_E800_SPK_AMP_ON, 0);

-static struct snd_soc_dai_link e800_dai[] = {
+	return 0;
+}
+
+static int e800_hp_amp_event(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *kcontrol, int event)
  {
-	.name = "AC97 Aux",
-	.stream_name = "AC97 Aux",
-	.cpu_dai = &pxa_ac97_dai[PXA2XX_DAI_AC97_AUX],
-	.codec_dai = &wm9712_dai[WM9712_DAI_AC97_AUX],
-},
+	if (event & SND_SOC_DAPM_PRE_PMU)
+		gpio_set_value(GPIO_E800_HP_AMP_OFF, 0);
+	else if (event & SND_SOC_DAPM_POST_PMD)
+		gpio_set_value(GPIO_E800_HP_AMP_OFF, 1);
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget e800_dapm_widgets[] = {
+	SND_SOC_DAPM_HP("Headphone Jack", NULL),
+	SND_SOC_DAPM_MIC("Mic (Internal1)", NULL),
+	SND_SOC_DAPM_MIC("Mic (Internal2)", NULL),
+	SND_SOC_DAPM_SPK("Speaker", NULL),
+	SND_SOC_DAPM_PGA_E("Headphone Amp", SND_SOC_NOPM, 0, 0, NULL, 0,
+			e800_hp_amp_event, SND_SOC_DAPM_PRE_PMU |
+			SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_PGA_E("Speaker Amp", SND_SOC_NOPM, 0, 0, NULL, 0,
+			e800_spk_amp_event, SND_SOC_DAPM_PRE_PMU |
+			SND_SOC_DAPM_POST_PMD),
+};
+
+static const struct snd_soc_dapm_route audio_map[] = {
+	{"Headphone Jack", NULL, "HPOUTL"},
+	{"Headphone Jack", NULL, "HPOUTR"},
+	{"Headphone Jack", NULL, "Headphone Amp"},
+
+	{"Speaker Amp", NULL, "MONOOUT"},
+	{"Speaker", NULL, "Speaker Amp"},
+
+	{"MIC1", NULL, "Mic (Internal1)"},
+	{"MIC2", NULL, "Mic (Internal2)"},
+};
+
+static int e800_ac97_init(struct snd_soc_codec *codec)
+{
+	snd_soc_dapm_new_controls(codec, e800_dapm_widgets,
+					ARRAY_SIZE(e800_dapm_widgets));
+
+	snd_soc_dapm_add_routes(codec, audio_map, ARRAY_SIZE(audio_map));
+	snd_soc_dapm_sync(codec);
+
+	return 0;
+}
+
+static struct snd_soc_dai_link e800_dai[] = {
+	{
+		.name = "AC97",
+		.stream_name = "AC97 HiFi",
+		.cpu_dai = &pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI],
+		.codec_dai = &wm9712_dai[WM9712_DAI_AC97_HIFI],
+		.init = e800_ac97_init,
+	},
+	{
+		.name = "AC97 Aux",
+		.stream_name = "AC97 Aux",
+		.cpu_dai = &pxa_ac97_dai[PXA2XX_DAI_AC97_AUX],
+		.codec_dai = &wm9712_dai[WM9712_DAI_AC97_AUX],
+	},
  };

  static struct snd_soc_card e800 = {
@@ -61,6 +124,22 @@ static int __init e800_init(void)
  	if (!machine_is_e800())
  		return -ENODEV;

+	ret = gpio_request(GPIO_E800_HP_AMP_OFF,  "Headphone amp");
+	if (ret)
+		return ret;
+
+	ret = gpio_request(GPIO_E800_SPK_AMP_ON, "Speaker amp");
+	if (ret)
+		goto free_hp_amp_gpio;
+
+	ret = gpio_direction_output(GPIO_E800_HP_AMP_OFF, 1);
+	if (ret)
+		goto free_spk_amp_gpio;
+
+	ret = gpio_direction_output(GPIO_E800_SPK_AMP_ON, 1);
+	if (ret)
+		goto free_spk_amp_gpio;
+
  	e800_snd_device = platform_device_alloc("soc-audio", -1);
  	if (!e800_snd_device)
  		return -ENOMEM;
@@ -69,8 +148,15 @@ static int __init e800_init(void)
  	e800_snd_devdata.dev = &e800_snd_device->dev;
  	ret = platform_device_add(e800_snd_device);

-	if (ret)
-		platform_device_put(e800_snd_device);
+	if (!ret)
+		return 0;
+
+/* Fail gracefully */
+	platform_device_put(e800_snd_device);
+free_spk_amp_gpio:
+	gpio_free(GPIO_E800_SPK_AMP_ON);
+free_hp_amp_gpio:
+	gpio_free(GPIO_E800_HP_AMP_OFF);

  	return ret;
  }
@@ -78,6 +164,8 @@ static int __init e800_init(void)
  static void __exit e800_exit(void)
  {
  	platform_device_unregister(e800_snd_device);
+	gpio_free(GPIO_E800_SPK_AMP_ON);
+	gpio_free(GPIO_E800_HP_AMP_OFF);
  }

  module_init(e800_init);
@@ -86,4 +174,4 @@ module_exit(e800_exit);
  /* Module information */
  MODULE_AUTHOR("Ian Molton <spyro@f2s.com>");
  MODULE_DESCRIPTION("ALSA SoC driver for e800");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
-- 
1.5.6.5

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Russell King - ARM Linux]

Only one comment from me...

On Thu, Jan 15, 2009 at 10:06:53AM +0000, Ian Molton wrote:
> diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
> new file mode 100644
> index 0000000..4ff6a84
> --- /dev/null
> +++ b/sound/soc/codecs/wm9705.c
> @@ -0,0 +1,401 @@
> +/*
> + * wm9705.c  --  ALSA Soc WM9705 codec support
> + *
> + * Copyright 2008 Ian Molton <spyro@f2s.com>
> + *
> + *  This program is free software; you can redistribute  it and/or 
> modify it
> + *  under  the terms of  the GNU General  Public License as published 
> by the
> + *  Free Software Foundation; Version 2 of the  License only.
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/version.h>

Please do not include linux/version.h except where it is really needed
(I don't see anything requiring it in this file.)

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Russell King - ARM Linux wrote:
> Only one comment from me...

<cut>

> Please do not include linux/version.h except where it is really needed
> (I don't see anything requiring it in this file.)

Change made and pushed out.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Thu, Jan 15, 2009 at 10:06:53AM +0000, Ian Molton wrote:
> Takashi Iwai wrote:

> > We need reviews.  Could you post patches as well?

> Sure - attached below:

Always do this for ALSA patch submissions (the same thing will apply to
the majority of kernel subsystems).  Please also CC at least me on ASoC
patches.  I do read the list but it helps to make sure things don't get
missed.

These all look fine apart from one small thing in the codec driver and
the issue Russell already pointed out.

> +
> +	soc_ac97_ops.reset(codec->ac97);

This should check the return value, resets can and do fail.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Mark Brown wrote:

> Always do this for ALSA patch submissions (the same thing will apply to
> the majority of kernel subsystems).  Please also CC at least me on ASoC
> patches.

My bad, sorry.

> I do read the list but it helps to make sure things don't get
> missed.

Sure.

> These all look fine apart from one small thing in the codec driver 

>> +
>> +	soc_ac97_ops.reset(codec->ac97);
> 
> This should check the return value, resets can and do fail.

This call returns void. So no.

I cant see a way to determine the outcome of a reset on wm9705. I could 
program a register, then check to see it is set to its reset value, but 
if the codec is hung, this could cause lockups.

Russell's isue has been fixed.

TTFN!

-Ian

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Thu, Jan 15, 2009 at 10:16:10PM +0000, Ian Molton wrote:
> Mark Brown wrote:

> >>+	soc_ac97_ops.reset(codec->ac97);

> >This should check the return value, resets can and do fail.

> This call returns void. So no.

Oh, sorry - thinko.  You should read back the ID register and verify the
value.

> I cant see a way to determine the outcome of a reset on wm9705. I could 
> program a register, then check to see it is set to its reset value, but 
> if the codec is hung, this could cause lockups.

It shouldn't, the AC97 bus driver should time out.  The goal is to make
sure that the codec is live and the expected device.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Mark Brown wrote:
> On Thu, Jan 15, 2009 at 10:16:10PM +0000, Ian Molton wrote:
>> Mark Brown wrote:
> 
>>>> +	soc_ac97_ops.reset(codec->ac97);
> 
>>> This should check the return value, resets can and do fail.
> 
>> This call returns void. So no.
> 
> Oh, sorry - thinko.  You should read back the ID register and verify the
> value.

Implemented, tested, checkpatch passed.

Pushed out and available at:

git://git.mnementh.co.uk/linux-2.6-im.git asoc

gitweb at:

http://git.mnementh.co.uk/cgi-bin/gitweb.cgi?p=linux-2.6-im.git;a=shortlog;h=refs/heads/asoc

Thanks for the review,

-Ian

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Takashi Iwai]

At Thu, 15 Jan 2009 10:06:53 +0000,
Ian Molton wrote:
> 
> +static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int 
> reg)
> +{
> +	u16 *cache = codec->reg_cache;
> +
> +	switch (reg) {
> +	case AC97_RESET:
> +	case AC97_VENDOR_ID1:
> +	case AC97_VENDOR_ID2:
> +		return soc_ac97_ops.read(codec->ac97, reg);
> +	default:
> +		reg = reg >> 1;
> +
> +		if (reg > (ARRAY_SIZE(wm9705_reg)))
> +			return -EIO;

Shouldn't be this "reg >= ARRAY_SIZE(wm9705_reg)" ?

> +static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
> +	unsigned int val)
> +{
> +	u16 *cache = codec->reg_cache;
> +
> +	soc_ac97_ops.write(codec->ac97, reg, val);
> +	reg = reg >> 1;
> +	if (reg <= (ARRAY_SIZE(wm9705_reg)))
> +		cache[reg] = val;

Ditto, should be "reg < ARRAY_SIZE(wm9705_reg)".

> +static int wm9705_soc_probe(struct platform_device *pdev)
> +{
> +	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> +	struct snd_soc_codec *codec;
> +	int ret = 0;
> +
> +	printk(KERN_INFO "WM9705 SoC Audio Codec\n");
> +
> +	socdev->codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL);
> +	if (socdev->codec == NULL)
> +		return -ENOMEM;
> +	codec = socdev->codec;
> +	mutex_init(&codec->mutex);
> +
> +	codec->reg_cache =
> +		kzalloc(sizeof(u16) * ARRAY_SIZE(wm9705_reg), GFP_KERNEL);
> +	if (codec->reg_cache == NULL) {
> +		ret = -ENOMEM;
> +		goto cache_err;
> +	}
> +	memcpy(codec->reg_cache, wm9705_reg,
> +		sizeof(u16) * ARRAY_SIZE(wm9705_reg));

You can use kmemdup() here.


thanks,

Takashi

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Thu, Jan 15, 2009 at 11:58:36PM +0000, Ian Molton wrote:

> Pushed out and available at:

> git://git.mnementh.co.uk/linux-2.6-im.git asoc

Err... that branch is based off the ASoC dev branch so can't be pulled
for mainline.  I've cherry picked the patches over since the conflicts
were trivial but if you are going to send pull requests please base
things for upstream off the ASoC branch of Takashi's repository at:

	git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

As previously mentioned you really should post patches rather than just
pull requests (pull requests are fine but you should post the patches as
well) - it really is much easier to work with.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Takashi Iwai wrote:
> At Thu, 15 Jan 2009 10:06:53 +0000,
> Ian Molton wrote:

> Shouldn't be this "reg >= ARRAY_SIZE(wm9705_reg)" ?

Looks that way. Will fix.

I've spotted the same error in a couple of other drivers too.

It looks like register caching is a very common thing, would you be 
interested in a patch that consolidates the cache handling in soc-core, 
rather than having multiple possibly broken implementations around?

Also, I could be wrong, but wm8980 caching looks completely broken 
(array is type u16 but there is no register shift applied, AFAICT)

fixed in wm9705 for now anyway.

> Ditto, should be "reg < ARRAY_SIZE(wm9705_reg)".

Fixed.

> You can use kmemdup() here.

Changed.

TTFN,

-Ian

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Fri, Jan 16, 2009 at 11:34:41AM +0000, Ian Molton wrote:

> It looks like register caching is a very common thing, would you be 
> interested in a patch that consolidates the cache handling in soc-core, 
> rather than having multiple possibly broken implementations around?

There's gotchas with variable register sizes and volatile bits (this was
originally done more in the core but pushed out).  I've looked at it
before and it looked like the best way to handle it was to do it along
with a factoring out of common I/O routines.  Possibly best to leave it
for now.

> Also, I could be wrong, but wm8980 caching looks completely broken 
> (array is type u16 but there is no register shift applied, AFAICT)

There's normally a reason for the non-mainline drivers being that way
but in this case that's not it - the chip has 16 bit registers and the
cache is being worked with as u16 * so natural array indexing should
DTRT.

Please post an incremental patch with your other changes.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Mark Brown wrote:

> On Fri, Jan 16, 2009 at 11:34:41AM +0000, Ian Molton wrote:
> 
>> It looks like register caching is a very common thing, would you be
>>  interested in a patch that consolidates the cache handling in
>> soc-core, rather than having multiple possibly broken
>> implementations around?
> 
> There's gotchas with variable register sizes and volatile bits (this
> was originally done more in the core but pushed out).  I've looked at
> it before and it looked like the best way to handle it was to do it
> along with a factoring out of common I/O routines.  Possibly best to
> leave it for now.

It looks to me like most chips could be handled with little difficulty -
a reg mask for some chips that have eg. 9 bit registers represented in a
u16, a reg_step to allow easy handling of different reg sizes, and an
array of register numbers for which the core should not cache values.

Easily 10 chips presently supported could use that scheme, whilst
leaving the function pointer based system available for the few really
weird chips.

>> Also, I could be wrong, but wm8980 caching looks completely broken 
>> (array is type u16 but there is no register shift applied, AFAICT)
> 
> There's normally a reason for the non-mainline drivers being that way
>  but in this case that's not it - the chip has 16 bit registers and
> the cache is being worked with as u16 * so natural array indexing
> should DTRT.

I cant quickly check that chip that now because I've rebased to the 
for-tiwai branch which it isnt in.

What does seem to be clear though is that there seems to be some
confusion as to what the 'reg' parameter to codec->read should be -
should it be a register _address_, or a register _number_.

If the former, then the caching in my wm9705 and a few other drivers is
broken. These treat reg as a register _address_ and shift it right by 1
to get the reg number and thus index into the reg cache.

If the latter, then other drivers may be making flawed assumptions, eg.
wm8753 which uses an array of u16 for the cache and treats 'reg' as a
register _number_ and uses it directly to index it, without a shift.

> Please post an incremental patch with your other changes.

Attached below for review.

I haven't added my SoB: as I havent tested it.

As you can see, the cache handling is done in several ways by different 
codecs, with inconsistent semantics and return codes (some BUG, some 
return 1 some -1 some -EIO. Some wont allow undefined reg access, some 
do, but only uncached.) This seems like ripe territory for bugs (albeit 
fairly harmless ones, but bugs nontheless).

ac97.c seems to have cookie-cutter-copied code to free a cache that it 
doesnt even have...

 From 93188b0b342bd849bc6aa62dca4ed33f71fcba0e Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Fri, 16 Jan 2009 13:31:52 +0000
Subject: [PATCH] ASoC: fixes to caching implementations

This patch takes fixes a number of bugs in the caching code used by
several ASoC codec drivers. Mostly off-by-one fixes.

---
  sound/soc/codecs/ac97.c    |    2 --
  sound/soc/codecs/ad1980.c  |    4 ++--
  sound/soc/codecs/twl4030.c |    3 +++
  sound/soc/codecs/wm8580.c  |    4 ++--
  sound/soc/codecs/wm8728.c  |    4 ++--
  sound/soc/codecs/wm8753.c  |    4 ++--
  sound/soc/codecs/wm8990.c  |    4 ++--
  sound/soc/codecs/wm9705.c  |    4 ++--
  sound/soc/codecs/wm9712.c  |    4 ++--
  sound/soc/codecs/wm9713.c  |    4 ++--
  10 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/sound/soc/codecs/ac97.c b/sound/soc/codecs/ac97.c
index fb53e65..89d4127 100644
--- a/sound/soc/codecs/ac97.c
+++ b/sound/soc/codecs/ac97.c
@@ -123,7 +123,6 @@ bus_err:
  	snd_soc_free_pcms(socdev);

  err:
-	kfree(socdev->codec->reg_cache);
  	kfree(socdev->codec);
  	socdev->codec = NULL;
  	return ret;
@@ -138,7 +137,6 @@ static int ac97_soc_remove(struct platform_device *pdev)
  		return 0;

  	snd_soc_free_pcms(socdev);
-	kfree(socdev->codec->reg_cache);
  	kfree(socdev->codec);

  	return 0;
diff --git a/sound/soc/codecs/ad1980.c b/sound/soc/codecs/ad1980.c
index c3c5d0e..faf3587 100644
--- a/sound/soc/codecs/ad1980.c
+++ b/sound/soc/codecs/ad1980.c
@@ -109,7 +109,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec,
  	default:
  		reg = reg >> 1;

-		if (reg >= (ARRAY_SIZE(ad1980_reg)))
+		if (reg >= ARRAY_SIZE(ad1980_reg))
  			return -EINVAL;

  		return cache[reg];
@@ -123,7 +123,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,

  	soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg < (ARRAY_SIZE(ad1980_reg)))
+	if (reg < ARRAY_SIZE(ad1980_reg))
  		cache[reg] = val;

  	return 0;
diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index ddc9f37..f530c1e 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -125,6 +125,9 @@ static inline unsigned int 
twl4030_read_reg_cache(struct snd_soc_codec *codec,
  {
  	u8 *cache = codec->reg_cache;

+	if (reg >= TWL4030_CACHEREGNUM)
+		return -EIO;
+
  	return cache[reg];
  }

diff --git a/sound/soc/codecs/wm8580.c b/sound/soc/codecs/wm8580.c
index 9b75a37..3faf0e7 100644
--- a/sound/soc/codecs/wm8580.c
+++ b/sound/soc/codecs/wm8580.c
@@ -200,7 +200,7 @@ static inline unsigned int 
wm8580_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > ARRAY_SIZE(wm8580_reg));
+	BUG_ON(reg >= ARRAY_SIZE(wm8580_reg));
  	return cache[reg];
  }

@@ -223,7 +223,7 @@ static int wm8580_write(struct snd_soc_codec *codec, 
unsigned int reg,
  {
  	u8 data[2];

-	BUG_ON(reg > ARRAY_SIZE(wm8580_reg));
+	BUG_ON(reg >= ARRAY_SIZE(wm8580_reg));

  	/* Registers are 9 bits wide */
  	value &= 0x1ff;
diff --git a/sound/soc/codecs/wm8728.c b/sound/soc/codecs/wm8728.c
index defa310..f90dc52 100644
--- a/sound/soc/codecs/wm8728.c
+++ b/sound/soc/codecs/wm8728.c
@@ -47,7 +47,7 @@ static inline unsigned int 
wm8728_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > ARRAY_SIZE(wm8728_reg_defaults));
+	BUG_ON(reg >= ARRAY_SIZE(wm8728_reg_defaults));
  	return cache[reg];
  }

@@ -55,7 +55,7 @@ static inline void wm8728_write_reg_cache(struct 
snd_soc_codec *codec,
  	u16 reg, unsigned int value)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > ARRAY_SIZE(wm8728_reg_defaults));
+	BUG_ON(reg >= ARRAY_SIZE(wm8728_reg_defaults));
  	cache[reg] = value;
  }

diff --git a/sound/soc/codecs/wm8753.c b/sound/soc/codecs/wm8753.c
index 7283178..5a1c1fc 100644
--- a/sound/soc/codecs/wm8753.c
+++ b/sound/soc/codecs/wm8753.c
@@ -97,7 +97,7 @@ static inline unsigned int 
wm8753_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	if (reg < 1 || reg > (ARRAY_SIZE(wm8753_reg) + 1))
+	if (reg < 1 || reg >= (ARRAY_SIZE(wm8753_reg) + 1))
  		return -1;
  	return cache[reg - 1];
  }
@@ -109,7 +109,7 @@ static inline void wm8753_write_reg_cache(struct 
snd_soc_codec *codec,
  	unsigned int reg, unsigned int value)
  {
  	u16 *cache = codec->reg_cache;
-	if (reg < 1 || reg > 0x3f)
+	if (reg < 1 || reg >= (ARRAY_SIZE(wm8753_reg) + 1))
  		return;
  	cache[reg - 1] = value;
  }
diff --git a/sound/soc/codecs/wm8990.c b/sound/soc/codecs/wm8990.c
index 6b27786..f93c095 100644
--- a/sound/soc/codecs/wm8990.c
+++ b/sound/soc/codecs/wm8990.c
@@ -116,7 +116,7 @@ static inline unsigned int 
wm8990_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > (ARRAY_SIZE(wm8990_reg)) - 1);
+	BUG_ON(reg >= ARRAY_SIZE(wm8990_reg));
  	return cache[reg];
  }

@@ -129,7 +129,7 @@ static inline void wm8990_write_reg_cache(struct 
snd_soc_codec *codec,
  	u16 *cache = codec->reg_cache;

  	/* Reset register and reserved registers are uncached */
-	if (reg == 0 || reg > ARRAY_SIZE(wm8990_reg) - 1)
+	if (reg == 0 || reg >= ARRAY_SIZE(wm8990_reg))
  		return;

  	cache[reg] = value;
diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
index c6b7dc2..021b8ab 100644
--- a/sound/soc/codecs/wm9705.c
+++ b/sound/soc/codecs/wm9705.c
@@ -221,7 +221,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec, unsigned int reg)
  	default:
  		reg = reg >> 1;

-		if (reg > (ARRAY_SIZE(wm9705_reg)))
+		if (reg >= (ARRAY_SIZE(wm9705_reg)))
  			return -EIO;

  		return cache[reg];
@@ -235,7 +235,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,

  	soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg <= (ARRAY_SIZE(wm9705_reg)))
+	if (reg < (ARRAY_SIZE(wm9705_reg)))
  		cache[reg] = val;

  	return 0;
diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index 1b0ace0..4dc90d6 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -452,7 +452,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec,
  	else {
  		reg = reg >> 1;

-		if (reg > (ARRAY_SIZE(wm9712_reg)))
+		if (reg >= (ARRAY_SIZE(wm9712_reg)))
  			return -EIO;

  		return cache[reg];
@@ -466,7 +466,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,

  	soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg <= (ARRAY_SIZE(wm9712_reg)))
+	if (reg < (ARRAY_SIZE(wm9712_reg)))
  		cache[reg] = val;

  	return 0;
diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c
index a456226..d8ddca9 100644
--- a/sound/soc/codecs/wm9713.c
+++ b/sound/soc/codecs/wm9713.c
@@ -621,7 +621,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec,
  	else {
  		reg = reg >> 1;

-		if (reg > (ARRAY_SIZE(wm9713_reg)))
+		if (reg >= (ARRAY_SIZE(wm9713_reg)))
  			return -EIO;

  		return cache[reg];
@@ -635,7 +635,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,
  	if (reg < 0x7c)
  		soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg <= (ARRAY_SIZE(wm9713_reg)))
+	if (reg < (ARRAY_SIZE(wm9713_reg)))
  		cache[reg] = val;

  	return 0;
-- 
1.5.6.5

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:

> It looks to me like most chips could be handled with little difficulty -
> a reg mask for some chips that have eg. 9 bit registers represented in a
> u16, a reg_step to allow easy handling of different reg sizes, and an
> array of register numbers for which the core should not cache values.

> Easily 10 chips presently supported could use that scheme, whilst
> leaving the function pointer based system available for the few really
> weird chips.

Oh, there's a lot of stuff that could be factored out but the whole
register access area needs a good looking at.  It's just not the highest
priority cleanup - that's getting the card registration API converted
over to V2 so we can have multiple cards in one system.

> What does seem to be clear though is that there seems to be some
> confusion as to what the 'reg' parameter to codec->read should be -
> should it be a register _address_, or a register _number_.

It should be the register address as you'd see it in the datasheet
unless there's a *very* good reason for it to be something else.  It's
entirely private to the codec driver what the register parameter means,
the rest of the code only works with values that were given to it by the
codec driver.

> If the former, then the caching in my wm9705 and a few other drivers is
> broken. These treat reg as a register _address_ and shift it right by 1
> to get the reg number and thus index into the reg cache.

Neither scheme is broken.  Most codecs have no gaps in the register
space and therefore just use a straight array with a 1:1 mapping between
registers and cache slots.  The AC97 parts are special because AC97 only
has even numbered registers so they're saving a bit of memory by only
having cache space for registers that actually exist and cutting down on
noise by only showing those registers in the debug output.  It's not a
big saving but it's there.  

> >Please post an incremental patch with your other changes.

> Attached below for review.

Doesn't seem to include the kmemdup() fix for WM9705 - if you could post
the WM9705 changes only together with your signoff I can push the queue
I'm sitting on to Takashi?

> I haven't added my SoB: as I havent tested it.

> As you can see, the cache handling is done in several ways by different 
> codecs, with inconsistent semantics and return codes (some BUG, some 

Yes.  Broadly, writing outside of the register space the driver wants to
support is a clear bug, but the driver can choose to cache some or none
of the registers.  This makes the specifics of the error handling less
important for the system as a whole.

> return 1 some -1 some -EIO. Some wont allow undefined reg access, some 
> do, but only uncached.) This seems like ripe territory for bugs (albeit 

Undefined register access is often a deliberate decision in order to
allow access to functionality of the chip outside the fully documented
sections of the register space.  Having the cache there may not be safe
depending on the behaviour of the undocumented registers.

> fairly harmless ones, but bugs nontheless).

Yes, though it's less bad than it looks since each codec driver only has
to agree with itself.

This is why I'm saying the whole area needs going over fairly carefully
- there's useful cleanups that can be done but it needs a degree of care
due to the way the code is now in order to ensure that existing
functionality doesn't get broken.  Fortunately much of the care should
scale well over doing cleanups of the whole area since everything is
private to the individual codec drivers.

> This patch takes fixes a number of bugs in the caching code used by
> several ASoC codec drivers. Mostly off-by-one fixes.

This looks reasonable, though I didn't go through it completely
carefully due to the lack of signoff.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Mark Brown wrote:
> On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:

> Oh, there's a lot of stuff that could be factored out but the whole
> register access area needs a good looking at.

I dont mind consolidating the cache handling code if its worth the 
effort (IOW its not scheduled to get totally re-written)

> It should be the register address as you'd see it in the datasheet
> unless there's a *very* good reason for it to be something else.  It's
> entirely private to the codec driver what the register parameter means,
> the rest of the code only works with values that were given to it by the
> codec driver.

some of the core code for dumping the codecs reg_cache looks quite 
broken... at least its only debug code, but broken debug code is worse 
than useless...

>> If the former, then the caching in my wm9705 and a few other drivers is
>> broken. These treat reg as a register _address_ and shift it right by 1
>> to get the reg number and thus index into the reg cache.
> 
> Neither scheme is broken.

I guess  not as its codec specific (wit the exception noted above)

 > The AC97 parts are special because AC97 only
> has even numbered registers so they're saving a bit of memory by only
> having cache space for registers that actually exist and cutting down on
> noise by only showing those registers in the debug output.  It's not a
> big saving but it's there.

Actually they arent. the regs are 16 bit anyway for the most part so 
there is no saving - Just confusion over the meaning of the 'reg' parameter.

Basically we've got two codec types. BOTH have 16 bit regs, but on some 
the 'reg' parameter is a register address, and on some its the register 
number.

This is fine I supppose if the semantics for the underlying bus are the 
same, eg. the PXA AC97 bus takes reg as an address.

I havent checked to see if any AC97 codecs are passing reg as a register 
number - if they are then thats a clear bug as its not what the AC97 bus 
driver expects.

>>> Please post an incremental patch with your other changes.
> 
>> Attached below for review.
> 
> Doesn't seem to include the kmemdup() fix for WM9705

Whoops. Fixed.

> - if you could post
> the WM9705 changes only together with your signoff I can push the queue
> I'm sitting on to Takashi?

Sure, but I really think the other changes noted below need to  go in 
ASAP as AFAICT they are clearly bogus.

> Undefined register access is often a deliberate decision in order to
> allow access to functionality of the chip outside the fully documented
> sections of the register space.

Yes, I understand that.

> Having the cache there may not be safe
> depending on the behaviour of the undocumented registers.

Of course, but drivers calling ac97_write need to get clear errors back, 
IMHO.

> Yes, though it's less bad than it looks since each codec driver only has
> to agree with itself.

True.

>> This patch takes fixes a number of bugs in the caching code used by
>> several ASoC codec drivers. Mostly off-by-one fixes.
> 
> This looks reasonable, though I didn't go through it completely
> carefully due to the lack of signoff.

I think it needs to get in ASAP. I'll send it along with the wm9705 
stuff again, with my SOB (I've read through it again), but as a seperate 
patchset.

Patches to follow shortly.

-Ian

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Hi,

Mark, the changes requested are implemented below. There are two 
patchsets, one for wm9705 and one for the other codec issues I found.

also available at:

git://git.mnementh.co.uk/linux-2.6-im.git asoc-pu


 From 8e89f1f1a94bc697d1ce1c07ab8426d372b0d6fe Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Fri, 16 Jan 2009 15:37:22 +0000
Subject: [PATCH] ASoC: codec: wm9705 misc cleanup

This patch fixes an off-by-one error in the wm9705 reg_cache 
implementation and
replaces a kzalloc/copy with kmemdup().

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
  sound/soc/codecs/wm9705.c |   11 ++++-------
  1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
index c6b7dc2..cb26b6a 100644
--- a/sound/soc/codecs/wm9705.c
+++ b/sound/soc/codecs/wm9705.c
@@ -221,7 +221,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec, unsigned int reg)
  	default:
  		reg = reg >> 1;

-		if (reg > (ARRAY_SIZE(wm9705_reg)))
+		if (reg >= (ARRAY_SIZE(wm9705_reg)))
  			return -EIO;

  		return cache[reg];
@@ -235,7 +235,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,

  	soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg <= (ARRAY_SIZE(wm9705_reg)))
+	if (reg < (ARRAY_SIZE(wm9705_reg)))
  		cache[reg] = val;

  	return 0;
@@ -327,15 +327,12 @@ static int wm9705_soc_probe(struct platform_device 
*pdev)
  	codec = socdev->codec;
  	mutex_init(&codec->mutex);

-	codec->reg_cache =
-		kzalloc(sizeof(u16) * ARRAY_SIZE(wm9705_reg), GFP_KERNEL);
+	codec->reg_cache = kmemdup(wm9705_reg, sizeof(wm9705_reg), GFP_KERNEL);
  	if (codec->reg_cache == NULL) {
  		ret = -ENOMEM;
  		goto cache_err;
  	}
-	memcpy(codec->reg_cache, wm9705_reg,
-		sizeof(u16) * ARRAY_SIZE(wm9705_reg));
-	codec->reg_cache_size = sizeof(u16) * ARRAY_SIZE(wm9705_reg);
+	codec->reg_cache_size = sizeof(wm9705_reg);
  	codec->reg_cache_step = 2;

  	codec->name = "WM9705";
-- 
1.5.6.5

 From 7ff27c9c1f7a7b7c86d3138060b1b406ad5dd6af Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Fri, 16 Jan 2009 15:03:19 +0000
Subject: [PATCH] ASoC: fixes to caching implementations

This patch takes fixes a number of bugs in the caching code used by
several ASoC codec drivers. Mostly off-by-one fixes.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
  sound/soc/codecs/ac97.c    |    2 --
  sound/soc/codecs/ad1980.c  |    4 ++--
  sound/soc/codecs/twl4030.c |    3 +++
  sound/soc/codecs/wm8580.c  |    4 ++--
  sound/soc/codecs/wm8728.c  |    4 ++--
  sound/soc/codecs/wm8753.c  |    4 ++--
  sound/soc/codecs/wm8990.c  |    4 ++--
  sound/soc/codecs/wm9712.c  |    4 ++--
  sound/soc/codecs/wm9713.c  |    4 ++--
  9 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/ac97.c b/sound/soc/codecs/ac97.c
index fb53e65..89d4127 100644
--- a/sound/soc/codecs/ac97.c
+++ b/sound/soc/codecs/ac97.c
@@ -123,7 +123,6 @@ bus_err:
  	snd_soc_free_pcms(socdev);

  err:
-	kfree(socdev->codec->reg_cache);
  	kfree(socdev->codec);
  	socdev->codec = NULL;
  	return ret;
@@ -138,7 +137,6 @@ static int ac97_soc_remove(struct platform_device *pdev)
  		return 0;

  	snd_soc_free_pcms(socdev);
-	kfree(socdev->codec->reg_cache);
  	kfree(socdev->codec);

  	return 0;
diff --git a/sound/soc/codecs/ad1980.c b/sound/soc/codecs/ad1980.c
index c3c5d0e..faf3587 100644
--- a/sound/soc/codecs/ad1980.c
+++ b/sound/soc/codecs/ad1980.c
@@ -109,7 +109,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec,
  	default:
  		reg = reg >> 1;

-		if (reg >= (ARRAY_SIZE(ad1980_reg)))
+		if (reg >= ARRAY_SIZE(ad1980_reg))
  			return -EINVAL;

  		return cache[reg];
@@ -123,7 +123,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,

  	soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg < (ARRAY_SIZE(ad1980_reg)))
+	if (reg < ARRAY_SIZE(ad1980_reg))
  		cache[reg] = val;

  	return 0;
diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index ddc9f37..f530c1e 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -125,6 +125,9 @@ static inline unsigned int 
twl4030_read_reg_cache(struct snd_soc_codec *codec,
  {
  	u8 *cache = codec->reg_cache;

+	if (reg >= TWL4030_CACHEREGNUM)
+		return -EIO;
+
  	return cache[reg];
  }

diff --git a/sound/soc/codecs/wm8580.c b/sound/soc/codecs/wm8580.c
index 9b75a37..3faf0e7 100644
--- a/sound/soc/codecs/wm8580.c
+++ b/sound/soc/codecs/wm8580.c
@@ -200,7 +200,7 @@ static inline unsigned int 
wm8580_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > ARRAY_SIZE(wm8580_reg));
+	BUG_ON(reg >= ARRAY_SIZE(wm8580_reg));
  	return cache[reg];
  }

@@ -223,7 +223,7 @@ static int wm8580_write(struct snd_soc_codec *codec, 
unsigned int reg,
  {
  	u8 data[2];

-	BUG_ON(reg > ARRAY_SIZE(wm8580_reg));
+	BUG_ON(reg >= ARRAY_SIZE(wm8580_reg));

  	/* Registers are 9 bits wide */
  	value &= 0x1ff;
diff --git a/sound/soc/codecs/wm8728.c b/sound/soc/codecs/wm8728.c
index defa310..f90dc52 100644
--- a/sound/soc/codecs/wm8728.c
+++ b/sound/soc/codecs/wm8728.c
@@ -47,7 +47,7 @@ static inline unsigned int 
wm8728_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > ARRAY_SIZE(wm8728_reg_defaults));
+	BUG_ON(reg >= ARRAY_SIZE(wm8728_reg_defaults));
  	return cache[reg];
  }

@@ -55,7 +55,7 @@ static inline void wm8728_write_reg_cache(struct 
snd_soc_codec *codec,
  	u16 reg, unsigned int value)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > ARRAY_SIZE(wm8728_reg_defaults));
+	BUG_ON(reg >= ARRAY_SIZE(wm8728_reg_defaults));
  	cache[reg] = value;
  }

diff --git a/sound/soc/codecs/wm8753.c b/sound/soc/codecs/wm8753.c
index 7283178..5a1c1fc 100644
--- a/sound/soc/codecs/wm8753.c
+++ b/sound/soc/codecs/wm8753.c
@@ -97,7 +97,7 @@ static inline unsigned int 
wm8753_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	if (reg < 1 || reg > (ARRAY_SIZE(wm8753_reg) + 1))
+	if (reg < 1 || reg >= (ARRAY_SIZE(wm8753_reg) + 1))
  		return -1;
  	return cache[reg - 1];
  }
@@ -109,7 +109,7 @@ static inline void wm8753_write_reg_cache(struct 
snd_soc_codec *codec,
  	unsigned int reg, unsigned int value)
  {
  	u16 *cache = codec->reg_cache;
-	if (reg < 1 || reg > 0x3f)
+	if (reg < 1 || reg >= (ARRAY_SIZE(wm8753_reg) + 1))
  		return;
  	cache[reg - 1] = value;
  }
diff --git a/sound/soc/codecs/wm8990.c b/sound/soc/codecs/wm8990.c
index 6b27786..f93c095 100644
--- a/sound/soc/codecs/wm8990.c
+++ b/sound/soc/codecs/wm8990.c
@@ -116,7 +116,7 @@ static inline unsigned int 
wm8990_read_reg_cache(struct snd_soc_codec *codec,
  	unsigned int reg)
  {
  	u16 *cache = codec->reg_cache;
-	BUG_ON(reg > (ARRAY_SIZE(wm8990_reg)) - 1);
+	BUG_ON(reg >= ARRAY_SIZE(wm8990_reg));
  	return cache[reg];
  }

@@ -129,7 +129,7 @@ static inline void wm8990_write_reg_cache(struct 
snd_soc_codec *codec,
  	u16 *cache = codec->reg_cache;

  	/* Reset register and reserved registers are uncached */
-	if (reg == 0 || reg > ARRAY_SIZE(wm8990_reg) - 1)
+	if (reg == 0 || reg >= ARRAY_SIZE(wm8990_reg))
  		return;

  	cache[reg] = value;
diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index 1b0ace0..4dc90d6 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -452,7 +452,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec,
  	else {
  		reg = reg >> 1;

-		if (reg > (ARRAY_SIZE(wm9712_reg)))
+		if (reg >= (ARRAY_SIZE(wm9712_reg)))
  			return -EIO;

  		return cache[reg];
@@ -466,7 +466,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,

  	soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg <= (ARRAY_SIZE(wm9712_reg)))
+	if (reg < (ARRAY_SIZE(wm9712_reg)))
  		cache[reg] = val;

  	return 0;
diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c
index a456226..d8ddca9 100644
--- a/sound/soc/codecs/wm9713.c
+++ b/sound/soc/codecs/wm9713.c
@@ -621,7 +621,7 @@ static unsigned int ac97_read(struct snd_soc_codec 
*codec,
  	else {
  		reg = reg >> 1;

-		if (reg > (ARRAY_SIZE(wm9713_reg)))
+		if (reg >= (ARRAY_SIZE(wm9713_reg)))
  			return -EIO;

  		return cache[reg];
@@ -635,7 +635,7 @@ static int ac97_write(struct snd_soc_codec *codec, 
unsigned int reg,
  	if (reg < 0x7c)
  		soc_ac97_ops.write(codec->ac97, reg, val);
  	reg = reg >> 1;
-	if (reg <= (ARRAY_SIZE(wm9713_reg)))
+	if (reg < (ARRAY_SIZE(wm9713_reg)))
  		cache[reg] = val;

  	return 0;
-- 
1.5.6.5

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Fri, Jan 16, 2009 at 03:00:35PM +0000, Ian Molton wrote:
> Mark Brown wrote:
> >On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:

> >Oh, there's a lot of stuff that could be factored out but the whole
> >register access area needs a good looking at.

> I dont mind consolidating the cache handling code if its worth the 
> effort (IOW its not scheduled to get totally re-written)

It's not likely to be rewritten dramatically, like I say it's more about
the effort taken to do it - I know there's a bunch of people who really
want multiple cards.

> some of the core code for dumping the codecs reg_cache looks quite 
> broken... at least its only debug code, but broken debug code is worse 
> than useless...

Could you be more specific, please?  The read interface is in rather
active use.  The only thing I'm aware of is that only things marked as
cached are visible.  The read interface certainly gets a lot of use, the
write interface less so but still.

> > The AC97 parts are special because AC97 only
> >has even numbered registers so they're saving a bit of memory by only
> >having cache space for registers that actually exist and cutting down on
> >noise by only showing those registers in the debug output.  It's not a
> >big saving but it's there.

> Actually they arent. the regs are 16 bit anyway for the most part so 
> there is no saving - Just confusion over the meaning of the 'reg' parameter.

There is.  The natural layout would be to just have an array of u16s
which gives us an array of 16 bit register slots like this:

  [0][1][2][3][4][5]..

but we're actually storing:

  [0][2][4]...

since we know that the odd numbered registers don't exist.

> Basically we've got two codec types. BOTH have 16 bit regs, but on some 
> the 'reg' parameter is a register address, and on some its the register 
> number.

The registers referred to by ASoC are *always* register numbers as you'd
see in the chip documentation; the storage method used for any register
cache is entirely up to the codec drivers and is not something that the
core concerns itself with.  The core is also unaware of the actual width
of the registers.

You're confusing yourself here by thinking of these things memory mapped
devices - they aren't.  How software chooses to lay out any view it has
of the registers in memory has no meaning in hardware since there is, in
general, no way to interact with the hardware with a resolution less
than a full register.

> This is fine I supppose if the semantics for the underlying bus are the 
> same, eg. the PXA AC97 bus takes reg as an address.

For marshalling purposes all the I2C and SPI codecs are their own "bus
driver" - the only external thing that cares is the hardware itself, the
I2C and SPI bus drivers just deal in byte streams.  AC97 is the only
actual bus in use here and it has the same register number interface
that ASoC uses.

> I havent checked to see if any AC97 codecs are passing reg as a register 
> number - if they are then thats a clear bug as its not what the AC97 bus 
> driver expects.

All the AC97 codecs are using reg as a register number because that is
what both ASoC and the AC97 bus drivers expect.  Note that the AC97 bus
drivers have no idea that the codecs have caches, they just work with
register/value pairs.

> >the WM9705 changes only together with your signoff I can push the queue
> >I'm sitting on to Takashi?

> Sure, but I really think the other changes noted below need to  go in 
> ASAP as AFAICT they are clearly bogus.

They're not *that* urgent given how long they've been there - it's a
nice cleanup not something I'd push to Linus unless they fixed issues on
live systems (in which case I'd really expect to see patches to fix the
callers as well).  The risk/reward doesn't seem worth it.

> >Having the cache there may not be safe
> >depending on the behaviour of the undocumented registers.

> Of course, but drivers calling ac97_write need to get clear errors back, 
> IMHO.

Hardware I/O is a different matter - the error codes there are more
interesting since that could realistically go wrong at runtime.
Accessing invalid registers is a clear coding error, but it's possible
that something is relying on the fact that errors aren't reported
(probably by completely ignoring the return values) so it needs a bit of
care to actually change anything.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Fri, Jan 16, 2009 at 03:46:19PM +0000, Ian Molton wrote:

> Mark, the changes requested are implemented below. There are two 
> patchsets, one for wm9705 and one for the other codec issues I found.

> also available at:

> git://git.mnementh.co.uk/linux-2.6-im.git asoc-pu

Thanks, I've applied the first and squashed it down, will look at the
second one later.

It looks like your MUA has badly whitespace damaged the patches - it's
word wrapped them and it looks like tabs and spaces have been confused.
Might be worth using git send-email to send patches.

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Fri, Jan 16, 2009 at 03:46:19PM +0000, Ian Molton wrote:

> Mark, the changes requested are implemented below. There are two
> patchsets, one for wm9705 and one for the other codec issues I found.

This looks good, thanks - I've applied it.  A couple of the changes
appear to be pure formatting changes:

> --- a/sound/soc/codecs/ad1980.c
> +++ b/sound/soc/codecs/ad1980.c
> @@ -109,7 +109,7 @@ static unsigned int ac97_read(struct snd_soc_codec  
> *codec,
>  	default:
>  		reg = reg >> 1;
>
> -		if (reg >= (ARRAY_SIZE(ad1980_reg)))
> +		if (reg >= ARRAY_SIZE(ad1980_reg))
>  			return -EINVAL;
>
>  		return cache[reg];

Formatting changes only in this driver?  Not sure I really like the
extra brackets.

> diff --git a/sound/soc/codecs/wm8990.c b/sound/soc/codecs/wm8990.c
> index 6b27786..f93c095 100644
> --- a/sound/soc/codecs/wm8990.c
> +++ b/sound/soc/codecs/wm8990.c
> @@ -116,7 +116,7 @@ static inline unsigned int  
> wm8990_read_reg_cache(struct snd_soc_codec *codec,
>  	unsigned int reg)
>  {
>  	u16 *cache = codec->reg_cache;
> -	BUG_ON(reg > (ARRAY_SIZE(wm8990_reg)) - 1);
> +	BUG_ON(reg >= ARRAY_SIZE(wm8990_reg));
>  	return cache[reg];
>  }
>

These two bits of code are equivalent...

Re: [Pull request] Support for wm9705 codec and two machines that use it.

[Ian Molton]

Mark Brown wrote:
> On Fri, Jan 16, 2009 at 03:46:19PM +0000, Ian Molton wrote:
> 
>> Mark, the changes requested are implemented below. There are two
>> patchsets, one for wm9705 and one for the other codec issues I found.
> 
> This looks good, thanks - I've applied it.  A couple of the changes
> appear to be pure formatting changes:

Thanks,

Comments below on your comments:

>> --- a/sound/soc/codecs/ad1980.c
>> +++ b/sound/soc/codecs/ad1980.c
>> @@ -109,7 +109,7 @@ static unsigned int ac97_read(struct snd_soc_codec  
>> *codec,
>>  	default:
>>  		reg = reg >> 1;
>>
>> -		if (reg >= (ARRAY_SIZE(ad1980_reg)))
>> +		if (reg >= ARRAY_SIZE(ad1980_reg))
>>  			return -EINVAL;
>>
>>  		return cache[reg];
> 
> Formatting changes only in this driver?  Not sure I really like the
> extra brackets.

Exactly - nor did I, so  Iremoved them :-)

>> diff --git a/sound/soc/codecs/wm8990.c b/sound/soc/codecs/wm8990.c
>> index 6b27786..f93c095 100644
>> --- a/sound/soc/codecs/wm8990.c
>> +++ b/sound/soc/codecs/wm8990.c
>> @@ -116,7 +116,7 @@ static inline unsigned int  
>> wm8990_read_reg_cache(struct snd_soc_codec *codec,
>>  	unsigned int reg)
>>  {
>>  	u16 *cache = codec->reg_cache;
>> -	BUG_ON(reg > (ARRAY_SIZE(wm8990_reg)) - 1);
>> +	BUG_ON(reg >= ARRAY_SIZE(wm8990_reg));
>>  	return cache[reg];
>>  }
>>
> 
> These two bits of code are equivalent...

Yes but one is clearer and more commonly used. Testing for 'more than 
one less than the array size' is just a bit... meh :-)

(plus its inconsistent with all the other drivers as it was)

-Ian