[PATCH 0/2] ASoC: MioA701 board

[PATCH 0/2] ASoC: MioA701 board

[Robert Jarzmik]

Hi,

These 2 patches have been already reviewed in April 2008 and have been siting in
Mark's asoc-v2 tree since then. They have been ported back to asoc-v1 for
mainline submission.

Please consider them for review if need be and scheduling for the next queue.

The serie was build upon Takashi's topic/asoc branch, commit "ASoC: Clean up
kerneldoc warnings" of id ac11a2b35cc25c77d28218aaf60e7f7c6c7ee5d3.

Cheers.

--
Robert

Robert Jarzmik (2):
  Add initial support of Mitac mioa701 device SoC.
  Add initial support of Mitac mioa701 master volume.

 sound/soc/pxa/Kconfig              |    9 +
 sound/soc/pxa/Makefile             |    2 +
 sound/soc/pxa/mioa701_masterctrl.c |  243 +++++++++++++++
 sound/soc/pxa/mioa701_wm9713.c     |  578 ++++++++++++++++++++++++++++++++++++
 4 files changed, 832 insertions(+), 0 deletions(-)
 create mode 100644 sound/soc/pxa/mioa701_masterctrl.c
 create mode 100644 sound/soc/pxa/mioa701_wm9713.c

[PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Robert Jarzmik]

This machine driver enables sound functions on Mitac mio
a701 smartphone. Build upon ASoC v1, it handles :
 - rear speaker
 - front speaker
 - microphone
 - GSM

A global "Mio Mode" switch is provided to cope with audio
path setup. It ensures balance on audio chip line, which if
not respected can produce a lot of heat and even fry the
battery behind the wm9713 and the speaker amplificator.

It doesn't cope with :
 - headset jack (will be integrade after jack support has
   hit ASoC v2)
 - master volume control (depending on current Mio Mode,
   will be submitted in a second patch)

This driver is backported from ASoc v2.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 sound/soc/pxa/Kconfig          |    9 +
 sound/soc/pxa/Makefile         |    2 +
 sound/soc/pxa/mioa701_wm9713.c |  571 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 582 insertions(+), 0 deletions(-)
 create mode 100644 sound/soc/pxa/mioa701_wm9713.c

diff --git a/sound/soc/pxa/Kconfig b/sound/soc/pxa/Kconfig
index f82e106..b385404 100644
--- a/sound/soc/pxa/Kconfig
+++ b/sound/soc/pxa/Kconfig
@@ -97,3 +97,12 @@ config SND_SOC_ZYLONITE
 	help
 	  Say Y if you want to add support for SoC audio on the
 	  Marvell Zylonite reference platform.
+
+config SND_PXA2XX_SOC_MIOA701
+        tristate "SoC Audio support for MIO A701"
+        depends on SND_PXA2XX_SOC && MACH_MIOA701
+        select SND_PXA2XX_SOC_AC97
+        select SND_SOC_WM9713
+        help
+          Say Y if you want to add support for SoC audio on the
+          MIO A701.
diff --git a/sound/soc/pxa/Makefile b/sound/soc/pxa/Makefile
index 08a9f27..9a6d27c 100644
--- a/sound/soc/pxa/Makefile
+++ b/sound/soc/pxa/Makefile
@@ -18,6 +18,7 @@ snd-soc-spitz-objs := spitz.o
 snd-soc-em-x270-objs := em-x270.o
 snd-soc-palm27x-objs := palm27x.o
 snd-soc-zylonite-objs := zylonite.o
+snd-soc-mioa701-objs := mioa701_wm9713.o
 
 obj-$(CONFIG_SND_PXA2XX_SOC_CORGI) += snd-soc-corgi.o
 obj-$(CONFIG_SND_PXA2XX_SOC_POODLE) += snd-soc-poodle.o
@@ -26,4 +27,5 @@ obj-$(CONFIG_SND_PXA2XX_SOC_E800) += snd-soc-e800.o
 obj-$(CONFIG_SND_PXA2XX_SOC_SPITZ) += snd-soc-spitz.o
 obj-$(CONFIG_SND_PXA2XX_SOC_EM_X270) += snd-soc-em-x270.o
 obj-$(CONFIG_SND_PXA2XX_SOC_PALM27X) += snd-soc-palm27x.o
+obj-$(CONFIG_SND_PXA2XX_SOC_MIOA701) += snd-soc-mioa701.o
 obj-$(CONFIG_SND_SOC_ZYLONITE) += snd-soc-zylonite.o
diff --git a/sound/soc/pxa/mioa701_wm9713.c b/sound/soc/pxa/mioa701_wm9713.c
new file mode 100644
index 0000000..dbf6799
--- /dev/null
+++ b/sound/soc/pxa/mioa701_wm9713.c
@@ -0,0 +1,571 @@
+/*
+ * Handles the Mitac mioa701 SoC system
+ *
+ * Copyright (C) 2008 Robert Jarzmik
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+
+#include <asm/mach-types.h>
+#include <mach/audio.h>
+
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+#include <sound/initval.h>
+#include <sound/ac97_codec.h>
+
+#include "pxa2xx-pcm.h"
+#include "pxa2xx-ac97.h"
+#include "../codecs/wm9713.h"
+
+#define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
+
+#define AC97_GPIO_PULL		0x58
+
+/* define the scenarios */
+#define MIO_AUDIO_OFF			0
+#define MIO_GSM_AUDIO_HANDSET		1
+#define MIO_GSM_AUDIO_HEADSET		2
+#define MIO_GSM_AUDIO_HANDSFREE		3
+#define MIO_GSM_AUDIO_BLUETOOTH		4
+#define MIO_STEREO_TO_SPEAKER		5
+#define MIO_STEREO_TO_HEADPHONES	6
+#define MIO_CAPTURE_HANDSET		7
+#define MIO_CAPTURE_HEADSET		8
+#define MIO_CAPTURE_BLUETOOTH		9
+
+static int mio_scenario = MIO_AUDIO_OFF;
+
+static int phone_stream_start(struct snd_soc_codec *codec);
+static int phone_stream_stop(struct snd_soc_codec *codec);
+
+struct mio_mixes_t {
+	char *mixname;
+	int  val;
+};
+
+static const struct mio_mixes_t mixes_reset_all[] = {
+	/* left HP mixer */
+	{"Left HP Mixer PC Beep Playback Switch", 0},
+	{"Left HP Mixer Voice Playback Switch",	  0},
+	{"Left HP Mixer Aux Playback Switch",	  0},
+	{"Left HP Mixer Bypass Playback Switch",  0},
+	{"Left HP Mixer PCM Playback Switch",	  0},
+	{"Left HP Mixer MonoIn Playback Switch",  0},
+	{"Left HP Mixer Capture Headphone Mux",	  0},
+
+	/* right HP mixer */
+	{"Right HP Mixer PC Beep Playback Switch", 0},
+	{"Right HP Mixer Voice Playback Switch",   0},
+	{"Right HP Mixer Aux Playback Switch",	   0},
+	{"Right HP Mixer Bypass Playback Switch",  0},
+	{"Right HP Mixer PCM Playback Switch",	   0},
+	{"Right HP Mixer MonoIn Playback Switch",  0},
+	{"Right HP Mixer Capture Headphone Mux",   0},
+
+	/* speaker mixer */
+	{"Speaker Mixer PC Beep Playback Switch", 0},
+	{"Speaker Mixer Voice Playback Switch",	  0},
+	{"Speaker Mixer Aux Playback Switch",	  0},
+	{"Speaker Mixer Bypass Playback Switch",  0},
+	{"Speaker Mixer PCM Playback Switch",	  0},
+	{"Speaker Mixer MonoIn Playback Switch",  0},
+	{"Speaker Mixer Mic 1 Sidetone Switch",	  0},
+
+	/* mono mixer */
+	{"Mono Mixer PC Beep Playback Switch", 0},
+	{"Mono Mixer Voice Playback Switch",   0},
+	{"Mono Mixer Aux Playback Switch",     0},
+	{"Mono Mixer Bypass Playback Switch",  0},
+	{"Mono Mixer PCM Playback Switch",     0},
+	{"Mono Mixer Capture Mono Mux",	       0},
+	{"Mono Mixer MonoIn Playback Switch",  0},
+	{"Mono Mixer Mic 1 Sidetone Switch",   0},
+	{"Mono Playback Switch",	       0},
+
+	/* headphone muxers */
+	{"Left Headphone Out Mux", 0},
+	{"Right Headphone Out Mux", 0},
+
+	/* speaker muxer */
+	{"Left Speaker Out Mux", 0},
+	{"Right Speaker Out Mux", 0},
+
+	/* Out3 muxer */
+	{ "Out 3 Mux", 0},
+
+	{ NULL, 0 }
+};
+
+static const struct mio_mixes_t mixes_gsm_call_headset[] = {
+	/*
+	 * GSM Out to Headset HPL Path
+	 *   => PCBeep -> Headphone Mixer, Headphone Mixer -> HPL
+	 */
+	{ "Left HP Mixer PC Beep Playback Switch", 1 },
+	{ "Left Headphone Out Mux", 2 },
+	/*
+	 * GSM Out to Headset HPR Path
+	 *   => MonoIn -> Headphone Mixer, Headphone Mixer -> HPR
+	 */
+	{ "Right HP Mixer MonoIn Playback Switch" , 1 },
+	{ "Right Headphone Out Mux", 2 },
+	/*
+	 * LineL to GSM In
+	 * LineL -> MonoMixer, MonoMixer -> Mono, Unmute Mono Mixer
+	 */
+	{ "Mono Mixer Bypass Playback Switch",	1},
+	{ "Mono Out Mux", 2 },
+	{ "Mono Playback Switch", 1},
+	{ NULL, 0 }
+};
+
+static const struct mio_mixes_t mixes_gsm_call_handset[] = {
+	/*
+	 * GSM Out to Front Speaker HPL Path
+	 *   => PCBeep -> Headphone Mixer, Headphone Mixer -> HPL
+	 */
+	{ "Left HP Mixer PC Beep Playback Switch", 1 },
+	{ "Left Headphone Out Mux", 2 },
+	/*
+	 * GSM Out to Front Speaker Out3 Path
+	 *   => MonoIn -> Speaker Mixer, Speaker Mixer -> Inv1, Inv1 -> Out3
+	 */
+	{ "Speaker Mixer MonoIn Playback Switch" , 1 },
+	{ "DAC Inv Mux 1", 2 },
+	{ "Out 3 Mux", 2 },
+	/*
+	 * MIC1 to GSM In
+	 *   => MIC1 -> MICA, MICA -> Mono Mixer, Mono Mixer -> MONO,
+	 *      UnMute Mono Mixer
+	 */
+	{ "Mic A Source", 0 },
+	{ "Mono Mixer Mic 1 Sidetone Switch", 1 },
+	{ "Mono Out Mux", 2 },
+	{ "Mono Playback Switch", 1},
+	{ NULL, 0 }
+};
+
+static const struct mio_mixes_t mixes_gsm_call_handsfree[] = {
+	/*
+	 * GSM Out to Rear Speaker SPKL Path
+	 *   => PCBeep -> Speaker Mixer, Speaker Mixer -> Inv1, Inv1 -> SPKL
+	 */
+	{ "Speaker Mixer PC Beep Playback Switch", 1 },
+	{ "DAC Inv Mux 1", 2 },
+	{ "Left Speaker Out Mux", 4 },
+	/*
+	 * GSM Out to Rear Speaker SPKR Path
+	 *   => MonoIn -> Speaker Mixer, Speaker Mixer -> SPKR
+	 */
+	{ "Speaker Mixer MonoIn Playback Switch" , 1 },
+	{ "Right Speaker Out Mux", 3 },
+	/*
+	 * MIC1 to GSM In
+	 *   => MIC1 -> MICA, MICA -> Mono Mixer, Mono Mixer -> MONO,
+	 *      Unmute Mono Mixer
+	 */
+	{ "Mic A Source", 0 },
+	{ "Mono Mixer Mic 1 Sidetone Switch", 1 },
+	{ "Mono Out Mux", 2 },
+	{ "Mono Playback Switch", 1},
+	{ NULL, 0 }
+};
+
+static const struct mio_mixes_t mixes_stereo_to_rearspeaker[] = {
+	/*
+	 * PCM to Rear Speakers
+	 *   => PCM -> Speaker Mixer, Speaker Mixer -> Inv1, Inv1 -> SPKL,
+	 *      Speaker Mixer -> SPKR
+	 */
+	{ "Speaker Mixer PCM Playback Switch", 1},
+	{ "DAC Inv Mux 1", 2 },
+	{ "Left Speaker Out Mux", 4 },
+	{ "Right Speaker Out Mux", 3 },
+	{ NULL, 0 }
+};
+
+struct snd_kcontrol *mioa701_kctrl_byname(struct snd_soc_codec *codec, char *n)
+{
+	struct snd_ctl_elem_id rid;
+
+	memset(&rid, 0, sizeof(rid));
+	rid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	strncpy(rid.name, n, sizeof(rid.name));
+	return snd_ctl_find_id(codec->card, &rid);
+}
+
+void setup_muxers(struct snd_soc_codec *codec, const struct mio_mixes_t mixes[])
+{
+	int pos = 0;
+	struct snd_kcontrol *kctl;
+	struct snd_ctl_elem_value ucontrol;
+	char mname[44];
+
+	while (mixes[pos].mixname) {
+		memset(mname, 0, 44);
+		strncpy(mname, mixes[pos].mixname, 43);
+		kctl = mioa701_kctrl_byname(codec, mname);
+		memset(&ucontrol, 0, sizeof(ucontrol));
+		if (kctl) {
+			kctl->get(kctl, &ucontrol);
+			ucontrol.value.enumerated.item[0] = mixes[pos].val;
+			kctl->put(kctl, &ucontrol);
+		}
+		pos++;
+	}
+}
+
+#define NB_ENDP ARRAY_SIZE(endpn)
+static int set_scenario_endpoints(struct snd_soc_codec *codec, int scenario)
+{
+	static char *endpn[] = { "Front Speaker", "Rear Speaker",
+				 "GSM Line Out", "GSM Line In",
+				 "Headset Mic", "Front Mic", "Headset" };
+	static const int typ_endps[][NB_ENDP] = {
+		{ 0, 0, 0, 0, 0, 0, 0 }, /* MIO_AUDIO_OFF		*/
+		{ 1, 0, 1, 1, 0, 1, 0 }, /* MIO_GSM_AUDIO_HANDSET	*/
+		{ 0, 0, 1, 1, 1, 0, 1 }, /* MIO_GSM_AUDIO_HEADSET	*/
+		{ 0, 1, 1, 1, 0, 1, 0 }, /* MIO_GSM_AUDIO_HANDSFREE*/
+		{ 0, 0, 1, 1, 0, 0, 0 }, /* MIO_GSM_AUDIO_BLUETOOTH*/
+		{ 0, 1, 0, 0, 0, 0, 0 }, /* MIO_STEREO_TO_SPEAKER	*/
+		{ 0, 0, 0, 0, 0, 0, 1 }, /* MIO_STEREO_TO_HEADPHONES	*/
+		{ 0, 0, 0, 0, 0, 1, 0 }, /* MIO_CAPTURE_HANDSET		*/
+		{ 0, 0, 0, 0, 1, 0, 0 }, /* MIO_CAPTURE_HEADSET		*/
+		{ 0, 0, 0, 0, 0, 0, 0 }, /* MIO_CAPTURE_BLUETOOTH	*/
+	};
+	const int *endps = typ_endps[scenario];
+	int i;
+
+	for (i = 0; i < NB_ENDP; i++)
+		if (endps[i])
+			snd_soc_dapm_enable_pin(codec, endpn[i]);
+		else
+			snd_soc_dapm_disable_pin(codec, endpn[i]);
+	snd_soc_dapm_sync(codec);
+
+	return 0;
+}
+
+static int get_scenario(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	ucontrol->value.integer.value[0] = mio_scenario;
+	return 0;
+}
+
+static int isPhoneMode(int scenario)
+{
+	int onPhone = 0;
+
+	switch (scenario) {
+	case MIO_GSM_AUDIO_HANDSET:
+	case MIO_GSM_AUDIO_HEADSET:
+	case MIO_GSM_AUDIO_BLUETOOTH:
+	case MIO_GSM_AUDIO_HANDSFREE:
+		onPhone = 1;
+	}
+
+	return onPhone;
+}
+
+static void switch_mio_mode(struct snd_soc_codec *codec, int new_scenario)
+{
+	int wasPhone = 0, willPhone = 0;
+
+	wasPhone  = isPhoneMode(mio_scenario);
+	willPhone = isPhoneMode(new_scenario);
+
+	mio_scenario = new_scenario;
+	set_scenario_endpoints(codec, mio_scenario);
+
+	if (!wasPhone && willPhone)
+		phone_stream_start(codec);
+	if (wasPhone && !willPhone)
+		phone_stream_stop(codec);
+
+	setup_muxers(codec, mixes_reset_all);
+	switch (mio_scenario) {
+	case MIO_STEREO_TO_SPEAKER:
+		setup_muxers(codec, mixes_stereo_to_rearspeaker);
+		break;
+	case MIO_GSM_AUDIO_HANDSET:
+		setup_muxers(codec, mixes_gsm_call_handset);
+		break;
+	case MIO_GSM_AUDIO_HANDSFREE:
+		setup_muxers(codec, mixes_gsm_call_handsfree);
+		break;
+	case MIO_GSM_AUDIO_HEADSET:
+		setup_muxers(codec, mixes_gsm_call_headset);
+		break;
+	default:
+		break;
+	}
+}
+
+static int set_scenario(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+
+	if (mio_scenario == ucontrol->value.integer.value[0])
+		return 0;
+
+	switch_mio_mode(codec, ucontrol->value.integer.value[0]);
+	return 1;
+}
+
+static int phone_stream_start(struct snd_soc_codec *codec)
+{
+	snd_soc_dapm_stream_event(codec, "AC97 HiFi",
+				  SND_SOC_DAPM_STREAM_START);
+	return 0;
+}
+
+static int phone_stream_stop(struct snd_soc_codec *codec)
+{
+	snd_soc_dapm_stream_event(codec, "AC97 HiFi",
+				  SND_SOC_DAPM_STREAM_STOP);
+	return 0;
+}
+
+/* Use GPIO8 for rear speaker amplificator */
+static int rear_amp_power(struct snd_soc_codec *codec, int power)
+{
+	unsigned short reg;
+
+	if (power) {
+		reg = snd_soc_read(codec, AC97_GPIO_CFG);
+		snd_soc_write(codec, AC97_GPIO_CFG, reg | 0x0100);
+		reg = snd_soc_read(codec, AC97_GPIO_PULL);
+		snd_soc_write(codec, AC97_GPIO_PULL, reg | (1<<15));
+	} else {
+		reg = snd_soc_read(codec, AC97_GPIO_CFG);
+		snd_soc_write(codec, AC97_GPIO_CFG, reg & ~0x0100);
+		reg = snd_soc_read(codec, AC97_GPIO_PULL);
+		snd_soc_write(codec, AC97_GPIO_PULL, reg & ~(1<<15));
+	}
+
+	return 0;
+}
+
+static int rear_amp_event(struct snd_soc_dapm_widget *widget,
+			  struct snd_kcontrol *kctl, int event)
+{
+	struct snd_soc_codec *codec = widget->codec;
+	int rc;
+
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		rc = rear_amp_power(codec, 1);
+	else
+		rc = rear_amp_power(codec, 0);
+
+	return rc;
+}
+
+static const char *mio_scenarios[] = {
+	"Off",
+	"GSM Handset",
+	"GSM Headset",
+	"GSM Handsfree",
+	"GSM Bluetooth",
+	"PCM Speaker",
+	"Headphones",
+	"Capture Handset",
+	"Capture Headset",
+	"Capture Bluetooth"
+};
+static const struct soc_enum mio_scenario_enum[] = {
+	SOC_ENUM_SINGLE_EXT(10, mio_scenarios),
+};
+
+static const struct snd_kcontrol_new mioa701_ctrls[] = {
+	SOC_ENUM_EXT("Mio Mode", mio_scenario_enum[0],
+		     get_scenario, set_scenario),
+};
+
+/* mioa701 machine dapm widgets */
+static const struct snd_soc_dapm_widget mioa701_dapm_widgets[] = {
+	SND_SOC_DAPM_SPK("Front Speaker", NULL),
+	SND_SOC_DAPM_SPK("Rear Speaker", rear_amp_event),
+	SND_SOC_DAPM_MIC("Headset", NULL),
+	SND_SOC_DAPM_LINE("GSM Line Out", NULL),
+	SND_SOC_DAPM_LINE("GSM Line In", NULL),
+	SND_SOC_DAPM_MIC("Headset Mic", NULL),
+	SND_SOC_DAPM_MIC("Front Mic", NULL),
+};
+
+static const struct snd_soc_dapm_route audio_map[] = {
+	/* Call Mic */
+	{"Mic Bias", NULL, "Front Mic"},
+	{"MIC1", NULL, "Mic Bias"},
+
+	/* Headset Mic */
+	{"LINEL", NULL, "Headset Mic"},
+	{"LINER", NULL, "Headset Mic"},
+
+	/* GSM Module */
+	{"MONOIN", NULL, "GSM Line Out"},
+	{"PCBEEP", NULL, "GSM Line Out"},
+	{"GSM Line In", NULL, "MONO"},
+
+	/* headphone connected to HPL, HPR */
+	{"Headset", NULL, "HPL"},
+	{"Headset", NULL, "HPR"},
+
+	/* front speaker connected to HPL, OUT3 */
+	{"Front Speaker", NULL, "HPL"},
+	{"Front Speaker", NULL, "OUT3"},
+
+	/* rear speaker connected to SPKL, SPKR */
+	{"Rear Speaker", NULL, "SPKL"},
+	{"Rear Speaker", NULL, "SPKR"},
+};
+
+static int mioa701_wm9713_init(struct snd_soc_codec *codec)
+{
+	int i, ret;
+	unsigned short reg;
+
+	/* Add test specific controls */
+	for (i = 0; i < ARRAY_SIZE(mioa701_ctrls); i++) {
+		ret = snd_ctl_add(codec->card,
+				  snd_soc_cnew(&mioa701_ctrls[i], codec, NULL));
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Add mioa701 specific widgets */
+	snd_soc_dapm_new_controls(codec, ARRAY_AND_SIZE(mioa701_dapm_widgets));
+
+	/* Set up mioa701 specific audio path audio_mapnects */
+	snd_soc_dapm_add_routes(codec, ARRAY_AND_SIZE(audio_map));
+
+	/* initialize mioa701 codec pins */
+	set_scenario_endpoints(codec, mio_scenario);
+
+	/* Prepare GPIO8 for rear speaker amplificator */
+	reg = codec->read(codec, AC97_GPIO_CFG);
+	codec->write(codec, AC97_GPIO_CFG, reg | 0x0100);
+
+	/* Prepare MIC input */
+	reg = codec->read(codec, AC97_3D_CONTROL);
+	codec->write(codec, AC97_3D_CONTROL, reg | 0xc000);
+
+	snd_soc_dapm_sync(codec);
+
+	return 0;
+}
+
+#define mioa701_wm9713_suspend NULL
+#define mioa701_wm9713_resume  NULL
+
+static struct snd_soc_ops mioa701_ops;
+
+static struct snd_soc_dai_link mioa701_dai[] = {
+	{
+		.name = "AC97",
+		.stream_name = "AC97 HiFi",
+		.cpu_dai = &pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI],
+		.codec_dai = &wm9713_dai[WM9713_DAI_AC97_HIFI],
+		.init = mioa701_wm9713_init,
+		.ops = &mioa701_ops,
+	},
+	{
+		.name = "AC97 Aux",
+		.stream_name = "AC97 Aux",
+		.cpu_dai = &pxa_ac97_dai[PXA2XX_DAI_AC97_AUX],
+		.codec_dai = &wm9713_dai[WM9713_DAI_AC97_AUX],
+		.ops = &mioa701_ops,
+	},
+};
+
+static struct snd_soc_card mioa701 = {
+	.name = "MioA701",
+	.platform = &pxa2xx_soc_platform,
+	.dai_link = mioa701_dai,
+	.num_links = ARRAY_SIZE(mioa701_dai),
+};
+
+static struct snd_soc_device mioa701_snd_devdata = {
+	.card = &mioa701,
+	.codec_dev = &soc_codec_dev_wm9713,
+};
+
+static struct platform_device *mioa701_snd_device;
+
+static int mioa701_wm9713_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	if (!machine_is_mioa701())
+		return -ENODEV;
+
+	mioa701_snd_device = platform_device_alloc("soc-audio", -1);
+	if (!mioa701_snd_device)
+		return -ENOMEM;
+
+	platform_set_drvdata(mioa701_snd_device, &mioa701_snd_devdata);
+	mioa701_snd_devdata.dev = &mioa701_snd_device->dev;
+
+	ret = platform_device_add(mioa701_snd_device);
+	if (!ret)
+		return 0;
+
+	platform_device_put(mioa701_snd_device);
+	return ret;
+}
+
+static int __devexit mioa701_wm9713_remove(struct platform_device *pdev)
+{
+	platform_device_unregister(mioa701_snd_device);
+	return 0;
+}
+
+static struct platform_driver mioa701_wm9713_driver = {
+	.probe		= mioa701_wm9713_probe,
+	.remove		= __devexit_p(mioa701_wm9713_remove),
+	.suspend	= mioa701_wm9713_suspend,
+	.resume		= mioa701_wm9713_resume,
+	.driver		= {
+		.name		= "mioa701-wm9713",
+		.owner		= THIS_MODULE,
+	},
+};
+
+static int __init mioa701_asoc_init(void)
+{
+	return platform_driver_register(&mioa701_wm9713_driver);
+}
+
+static void __exit mioa701_asoc_exit(void)
+{
+	platform_driver_unregister(&mioa701_wm9713_driver);
+}
+
+module_init(mioa701_asoc_init);
+module_exit(mioa701_asoc_exit);
+
+/* Module information */
+MODULE_AUTHOR("Robert Jarzmik (rjarzmik@free.fr)");
+MODULE_DESCRIPTION("ALSA SoC WM9713 MIO A701");
+MODULE_LICENSE("GPL");
-- 
1.5.6.5

[PATCH 2/2] Add initial support of Mitac mioa701 master volume.

[Robert Jarzmik]

Depending on the "Mio Mode" chosen, the master volume
controls the widget volume/mute levels, providing userspace
a unified vision of volume control (one control only).

The master volume is adapted to smartphones because :
 - it only handles 2 sub-controls (left and right)
 - the generic code is split apart from mio specific code
 - it abstracts to userland audiopath within the chip

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 sound/soc/pxa/Makefile             |    2 +-
 sound/soc/pxa/mioa701_masterctrl.c |  243 ++++++++++++++++++++++++++++++++++++
 sound/soc/pxa/mioa701_wm9713.c     |    7 +
 3 files changed, 251 insertions(+), 1 deletions(-)
 create mode 100644 sound/soc/pxa/mioa701_masterctrl.c

diff --git a/sound/soc/pxa/Makefile b/sound/soc/pxa/Makefile
index 9a6d27c..f40949f 100644
--- a/sound/soc/pxa/Makefile
+++ b/sound/soc/pxa/Makefile
@@ -18,7 +18,7 @@ snd-soc-spitz-objs := spitz.o
 snd-soc-em-x270-objs := em-x270.o
 snd-soc-palm27x-objs := palm27x.o
 snd-soc-zylonite-objs := zylonite.o
-snd-soc-mioa701-objs := mioa701_wm9713.o
+snd-soc-mioa701-objs := mioa701_wm9713.o mioa701_masterctrl.o
 
 obj-$(CONFIG_SND_PXA2XX_SOC_CORGI) += snd-soc-corgi.o
 obj-$(CONFIG_SND_PXA2XX_SOC_POODLE) += snd-soc-poodle.o
diff --git a/sound/soc/pxa/mioa701_masterctrl.c b/sound/soc/pxa/mioa701_masterctrl.c
new file mode 100644
index 0000000..f95e1f7
--- /dev/null
+++ b/sound/soc/pxa/mioa701_masterctrl.c
@@ -0,0 +1,243 @@
+/*
+ * Handles the Mitac mioa701 Master Volume Control
+ *
+ * Copyright (C) 2008 Robert Jarzmik
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+#include <linux/vmalloc.h>
+#include <sound/core.h>
+#include <sound/soc.h>
+
+#define MASTER_MAX_CTL 2
+#define MAST2CODEC(m) (((struct kctl_master_t *)m)->codec)
+#define KCTL2MAST(k) ((struct kctl_master_t *) k->private_data)
+
+struct kctl_master_t {
+	unsigned int count;
+	snd_ctl_elem_type_t type;
+	char *names[MASTER_MAX_CTL];
+	int nidx[MASTER_MAX_CTL];
+	struct snd_soc_codec *codec;
+};
+
+#define MVOL2(name1, idx1, name2, idx2) \
+{	.count = 2, .names = { name1, name2 }, .nidx = { idx1, idx2 }, \
+	.type = SNDRV_CTL_ELEM_TYPE_INTEGER, \
+}
+#define MMUTE2(name1, idx1, name2, idx2) \
+{	.count = 2, .names = { name1, name2 }, .nidx = { idx1, idx2 }, \
+	.type = SNDRV_CTL_ELEM_TYPE_BOOLEAN, \
+}
+#define MVOL2_DEFAULT MVOL2(NULL, 0, NULL, 0)
+#define MMUTE2_DEFAULT MMUTE2(NULL, 0, NULL, 0)
+
+static struct snd_kcontrol *kctrl_byname(struct snd_soc_codec *codec, char *n)
+{
+	struct snd_ctl_elem_id rid;
+
+	memset(&rid, 0, sizeof(rid));
+	rid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	strncpy(rid.name, n, sizeof(rid.name));
+	return snd_ctl_find_id(codec->card, &rid);
+}
+
+static int master_find(struct snd_soc_codec *codec, char *name,
+		int *max, struct snd_kcontrol **kctl)
+{
+	struct snd_ctl_elem_info info;
+	int rc;
+
+	memset(&info, 0, sizeof(struct snd_ctl_elem_info));
+	*kctl = NULL;
+
+	if (name)
+		*kctl = kctrl_byname(codec, name);
+
+	if (*kctl) {
+		(*kctl)->info(*kctl, &info);
+		*max = info.value.integer.max;
+	}
+
+	rc = (*kctl != NULL);
+	return rc;
+}
+
+static int master_info(struct snd_kcontrol *kcontrol,
+		      struct snd_ctl_elem_info *uinfo)
+{
+	struct kctl_master_t *master = KCTL2MAST(kcontrol);
+
+	uinfo->type = master->type;
+	uinfo->count = master->count;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 0;
+	if (master->type & SNDRV_CTL_ELEM_TYPE_BOOLEAN)
+		uinfo->value.integer.max = 1;
+	if (master->type & SNDRV_CTL_ELEM_TYPE_INTEGER)
+		uinfo->value.integer.max = 256;
+	return 0;
+}
+
+static int master_get(struct snd_kcontrol *kcontrol,
+		      struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_kcontrol *kctl;
+	struct snd_ctl_elem_value uc;
+	struct kctl_master_t *master = KCTL2MAST(kcontrol);
+	struct snd_soc_codec *codec = MAST2CODEC(master);
+	int i, ind, max, old;
+
+	for (i = 0; i < master->count; i++) {
+		max = old = 0;
+		ind = master->nidx[i];
+		if (!master_find(codec, master->names[i], &max, &kctl))
+			continue;
+		if (kctl->get(kctl, &uc) < 0)
+			continue;
+		old = uc.value.integer.value[ind];
+		if (master->type == SNDRV_CTL_ELEM_TYPE_INTEGER)
+			old = old * 256 / (max+1);
+		ucontrol->value.integer.value[i] = old;
+	}
+
+	return 0;
+}
+
+static int master_put(struct snd_kcontrol *kcontrol,
+		      struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_kcontrol *kctl;
+	struct snd_ctl_elem_value uc;
+	struct kctl_master_t *master = KCTL2MAST(kcontrol);
+	struct snd_soc_codec *codec = MAST2CODEC(master);
+
+	int i, ind, max, new;
+
+	for (i = 0; i < master->count; i++) {
+		max = new = 0;
+		ind = master->nidx[i];
+		if (!master_find(codec, master->names[i], &max, &kctl))
+			continue;
+		new = ucontrol->value.integer.value[i];
+		if (master->type == SNDRV_CTL_ELEM_TYPE_INTEGER)
+			new = new * (max+1) / 256;
+
+		if (kctl->get(kctl, &uc) < 0)
+			continue;
+
+		uc.value.integer.value[ind] = new;
+		if (kctl->put(kctl, &uc) < 0)
+			continue;
+	}
+
+	return 0;
+}
+
+/*
+ * Beware : masterconf must be of same type, and have same count as the old
+ * master
+ */
+void master_change(struct snd_kcontrol *k, struct kctl_master_t *masterconf)
+{
+	if (masterconf && k)
+		masterconf->codec = MAST2CODEC(k->private_data);
+	if (k)
+		k->private_data = masterconf;
+}
+
+struct snd_kcontrol *master_init(struct snd_soc_codec *codec, char *mname,
+				 struct kctl_master_t *master)
+{
+	struct snd_kcontrol_new *kctln = NULL;
+	struct snd_kcontrol *kctl = NULL;
+	int rc;
+
+	kctln = kmalloc(sizeof(struct snd_kcontrol_new), GFP_KERNEL);
+	if (!kctln)
+		return NULL;
+
+	memset(kctln, 0, sizeof(struct snd_kcontrol_new));
+	kctln->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	kctln->name = mname;
+	kctln->info = master_info;
+	kctln->get = master_get;
+	kctln->put = master_put;
+	master->codec = codec;
+
+	kctl = snd_soc_cnew(kctln, master, NULL);
+
+	rc = snd_ctl_add(codec->card, kctl);
+	if (rc < 0) {
+		kfree(kctln);
+		kctl = NULL;
+	}
+
+	return kctl;
+}
+
+/* MIO Specific */
+static struct kctl_master_t miomastervol[] = {
+	MVOL2_DEFAULT,				/* MIO_AUDIO_OFF */
+	MVOL2("Headphone Playback Volume", 0,	/* MIO_GSM_AUDIO_HANDSET */
+	      "Out3 Playback Volume", 0),
+	MVOL2("Headphone Playback Volume", 0,	/* MIO_GSM_AUDIO_HEADSET */
+	      "Headphone Playback Volume", 1),
+	MVOL2("Speaker Playback Volume", 0,	/* MIO_GSM_AUDIO_HANDSFREE */
+	      "Speaker Playback Volume", 1),
+	MVOL2_DEFAULT,				/* MIO_GSM_AUDIO_BLUETOOTH */
+	MVOL2("Speaker Playback Volume", 0,	/* MIO_STEREO_TO_SPEAKER */
+	      "Speaker Playback Volume", 1),
+	MVOL2("Headphone Playback Volume", 0,	/* MIO_STEREO_TO_HEADPHONES */
+	      "Headphone Playback Volume", 1),
+	MVOL2_DEFAULT,				/* MIO_CAPTURE_HANDSET */
+	MVOL2_DEFAULT,				/* MIO_CAPTURE_HEADSET */
+	MVOL2_DEFAULT,				/* MIO_CAPTURE_BLUETOOTH */
+};
+
+static struct kctl_master_t miomastermute[] = {
+	MMUTE2_DEFAULT,				/* MIO_AUDIO_OFF */
+	MMUTE2("Headphone Playback Switch", 0,	/* MIO_GSM_AUDIO_HANDSET */
+	      "Out3 Playback Switch", 0),
+	MMUTE2("Headphone Playback Switch", 0,	/* MIO_GSM_AUDIO_HEADSET */
+	       "Headphone Playback Switch", 1),
+	MMUTE2("Speaker Playback Switch", 0,	/* MIO_GSM_AUDIO_HANDSFREE */
+	       "Speaker Playback Switch", 1),
+	MMUTE2_DEFAULT,				/* MIO_GSM_AUDIO_BLUETOOTH */
+	MMUTE2("Speaker Playback Switch", 0,	/* MIO_STEREO_TO_SPEAKER */
+	       "Speaker Playback Switch", 1),
+	MMUTE2("Headphone Playback Switch", 0,	/* MIO_STEREO_TO_HEADPHONES */
+	       "Headphone Playback Switch", 1),
+	MMUTE2_DEFAULT,				/* MIO_CAPTURE_HANDSET */
+	MMUTE2_DEFAULT,				/* MIO_CAPTURE_HEADSET */
+	MMUTE2_DEFAULT,				/* MIO_CAPTURE_BLUETOOTH */
+};
+
+static struct snd_kcontrol *miovol, *miomute;
+
+int mioa701_master_init(struct snd_soc_codec *codec)
+{
+	miovol =  master_init(codec, "Mio Volume", &miomastervol[0]);
+	miomute = master_init(codec, "Mio Switch", &miomastermute[0]);
+
+	return 0;
+}
+
+void mioa701_master_change(int scenario)
+{
+	master_change(miovol, &miomastervol[scenario]);
+	master_change(miomute, &miomastermute[scenario]);
+}
diff --git a/sound/soc/pxa/mioa701_wm9713.c b/sound/soc/pxa/mioa701_wm9713.c
index dbf6799..0ffc0c1 100644
--- a/sound/soc/pxa/mioa701_wm9713.c
+++ b/sound/soc/pxa/mioa701_wm9713.c
@@ -55,6 +55,8 @@
 
 static int mio_scenario = MIO_AUDIO_OFF;
 
+extern int mioa701_master_init(struct snd_soc_codec *codec);
+extern void mioa701_master_change(int scenario);
 static int phone_stream_start(struct snd_soc_codec *codec);
 static int phone_stream_stop(struct snd_soc_codec *codec);
 
@@ -320,6 +322,8 @@ static void switch_mio_mode(struct snd_soc_codec *codec, int new_scenario)
 	default:
 		break;
 	}
+
+	mioa701_master_change(mio_scenario);
 }
 
 static int set_scenario(struct snd_kcontrol *kcontrol,
@@ -463,6 +467,9 @@ static int mioa701_wm9713_init(struct snd_soc_codec *codec)
 	/* initialize mioa701 codec pins */
 	set_scenario_endpoints(codec, mio_scenario);
 
+	/* Add mio Masters */
+	mioa701_master_init(codec);
+
 	/* Prepare GPIO8 for rear speaker amplificator */
 	reg = codec->read(codec, AC97_GPIO_CFG);
 	codec->write(codec, AC97_GPIO_CFG, reg | 0x0100);
-- 
1.5.6.5

Re: [PATCH 2/2] Add initial support of Mitac mioa701 master volume.

[Mark Brown]

On Thu, Jan 08, 2009 at 07:42:26PM +0100, Robert Jarzmik wrote:

> Depending on the "Mio Mode" chosen, the master volume
> controls the widget volume/mute levels, providing userspace
> a unified vision of volume control (one control only).

I've not got time to review this properly tonight so I'll just say that
the comments about scenario stuff in my previous mail apply here - if we
want to do this in kernel it's worth lifting out the generic bits of
code so that other machine drivers don't end up implementing the same
stuff again.

> The master volume is adapted to smartphones because :

...

>  - it abstracts to userland audiopath within the chip

Yeah, applications should really be able to see such an abstraction -
it'd make them work a lot better with ASoC cards.

The main concern with doing it in drivers specifically is that this
means the driver needs to know about all the possible scenarios which
gets hard when the user can run their own programs and think up their
own use cases.  It also makes configuring scenarios a bit harder since
you can't tweak the paths with alsamixer so easily, though that's
solvable.

> +static struct snd_kcontrol *kctrl_byname(struct snd_soc_codec *codec, char *n)
> +{
> +	struct snd_ctl_elem_id rid;
> +
> +	memset(&rid, 0, sizeof(rid));
> +	rid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> +	strncpy(rid.name, n, sizeof(rid.name));
> +	return snd_ctl_find_id(codec->card, &rid);
> +}

I'm fairly sure you had the same thing in your machine driver -
definitely worth abstracting regardless of anything else (probably even
worth putting a version in ALSA core, I guess).

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Mark Brown]

On Thu, Jan 08, 2009 at 07:42:25PM +0100, Robert Jarzmik wrote:

> This machine driver enables sound functions on Mitac mio
> a701 smartphone. Build upon ASoC v1, it handles :

It'd probably be easier to split this up still further, with one patch
providing a standard ASoC machine and another adding the scenario stuff
after that.

As well as easing review I have to say that I'm a bit unenthusiastic
about having the scenario stuff in here.  The general plan was to do
this in user space, partly because experience with things like OpenMoko
suggests that some users are going to come up with new and exciting
scenarios and that having to rebuild the kernel is too high a barrier.
I know Graeme has said he regrets doing the in-kernel scenarios there.

Liam's ALSA scenario API was intended to do what's needed in user space:

	http://opensource.wolfsonmicro.com/node/14

though it's still a work in progress.  I'm not 100% against doing this
like you have since we don't have a clear solution right now but I'd
like to stop and think about it.

If we are going to do it in kernel space then it'd be better to lift at
least some of it out of your driver since the mechanics of what you've
got aren't very specific to this machine, just the lists of endpoints.
I'd also suggest masking off the controls you're controlling via the
scenario stuff from user space - Takashi has commit
03ad1d710ea51a51dfbe62caa2bc9a9437ae3fed in his sound-unstable-2.6.git
which adds snd_ctl_activate_id(), allowing drivers to do this.

More comments below, mostly very nitpicky:

> It doesn't cope with :
>  - headset jack (will be integrade after jack support has
>    hit ASoC v2)

I just pushed an implementation of that just now, enjoy :)

> +#define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)

This is also defined somewhere or other in the PXA or ARM architecture
code - there's a danger of multiple definitions of the macro appearing.
checkpatch wants extra () too, I kind of agree with it.

> +void setup_muxers(struct snd_soc_codec *codec, const struct mio_mixes_t mixes[])
> +{
> +	int pos = 0;
> +	struct snd_kcontrol *kctl;
> +	struct snd_ctl_elem_value ucontrol;
> +	char mname[44];
> +
> +	while (mixes[pos].mixname) {

A for loop over the ARRAY_SIZE() might be more idiomatic here...

> +		memset(mname, 0, 44);
> +		strncpy(mname, mixes[pos].mixname, 43);

It's bikesheding but strcpy plus termination would do the job, wouldn't
it?

> +               if (kctl) {

Shouldn't we at least warn if this is false, it's an error in the static
data for the maps.

> +                       kctl->get(kctl, &ucontrol);
> +                       ucontrol.value.enumerated.item[0] = mixes[pos].val;
> +                       kctl->put(kctl, &ucontrol);
> +               }

Does an open application (eg, alsamixer) notice these changes without
the driver telling it?  snd_ctl_notify() is the API call, IIRC though
there's a reasonable chance I am misremembering.  Though if you do mask
them from user space it shoudn't matter anyway, I expect.

> +		{ 0, 0, 1, 1, 1, 0, 1 }, /* MIO_GSM_AUDIO_HEADSET	*/
> +		{ 0, 1, 1, 1, 0, 1, 0 }, /* MIO_GSM_AUDIO_HANDSFREE*/
> +		{ 0, 0, 1, 1, 0, 0, 0 }, /* MIO_GSM_AUDIO_BLUETOOTH*/

Interesting indentation?

> +static int isPhoneMode(int scenario)
> +{
> +	int onPhone = 0;

The kernel doesn't generally go in for mixedCaseNames except when
following standards that use them.

> +	switch (scenario) {
> +	case MIO_GSM_AUDIO_HANDSET:
> +	case MIO_GSM_AUDIO_HEADSET:
> +	case MIO_GSM_AUDIO_BLUETOOTH:
> +	case MIO_GSM_AUDIO_HANDSFREE:
> +		onPhone = 1;

Setting 0 in a default would be slightly easier to read (or you could
just use return statements and not bother with the assignments).

> +	}
> +
> +	return onPhone;
> +}

> +static int phone_stream_start(struct snd_soc_codec *codec)
> +{
> +       snd_soc_dapm_stream_event(codec, "AC97 HiFi",
> +                                 SND_SOC_DAPM_STREAM_START);
> +       return 0;
> +}

Hrm.  What are these needed for?  I'd have expected that DAPM would
power active bypass paths up without this (I test that case frequently)
and this isn't coordinated with what the core is doing at all.

> +/* Use GPIO8 for rear speaker amplificator */

Amplifier.

> +static int rear_amp_event(struct snd_soc_dapm_widget *widget,
> +			  struct snd_kcontrol *kctl, int event)
> +{
> +	struct snd_soc_codec *codec = widget->codec;
> +	int rc;
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		rc = rear_amp_power(codec, 1);
> +	else
> +		rc = rear_amp_power(codec, 0);
> +
> +	return rc;
> +}

May as well just collapse rear_amp_power() in here, it's got the same if
statement in it anyway.

> +#define mioa701_wm9713_suspend NULL
> +#define mioa701_wm9713_resume  NULL

These can just be dropped until they're implemented (they may never need
to be).

> +static int mioa701_wm9713_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	if (!machine_is_mioa701())
> +		return -ENODEV;

Seems redundant to check this when you're using a platform device to
probe - a system should only be defining that device if it can
supported.  It's needed for most drivers which register the ASoC driver
directly on probe.

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Robert Jarzmik]

Mark Brown <broonie@sirena.org.uk> writes:

> On Thu, Jan 08, 2009 at 07:42:25PM +0100, Robert Jarzmik wrote:
>
> It'd probably be easier to split this up still further, with one patch
> providing a standard ASoC machine and another adding the scenario stuff
> after that.
Wish for 2 patches instead of that one, one for core support and one for
scenarii ? As this patch has already been reviewed and include into your tree,
I had hopped I shouldn't inject any more work that what had already be done ...

> As well as easing review I have to say that I'm a bit unenthusiastic
> about having the scenario stuff in here.  The general plan was to do
> this in user space, partly because experience with things like OpenMoko
> suggests that some users are going to come up with new and exciting
> scenarios and that having to rebuild the kernel is too high a barrier.
> I know Graeme has said he regrets doing the in-kernel scenarios there.
>
> Liam's ALSA scenario API was intended to do what's needed in user space:
>
> 	http://opensource.wolfsonmicro.com/node/14
>
> though it's still a work in progress.  I'm not 100% against doing this
> like you have since we don't have a clear solution right now but I'd
> like to stop and think about it.

I'd like to recall what happened to one of the mioa701 users here, when scenarii
were handled in userspace. The wm9713 chip overheated, and destroyed the battery
underneath. That was the reason to put that stuff into the kernel :
protection. This was also already discussed when this was _previously_
submitted.

And I know Liam's API, I tried it. Let me tell you my point of view : the
mioa701 users have a standard API (alsa) to control the sound of the mioa701. I
could force them to migrate towards Liam's library. But I can't have a guarantee
of maintainance over that library. I don't even know if it will be the final
solution.

Let me propose a deal : you include that library in the standard alsa-lib, which
will then slowly sink into Angstrom, OpenEmbedded, or whatever distribution
comes along, and then I'll remove all the scenario code away.

>> If we are going to do it in kernel space then it'd be better to lift at
> least some of it out of your driver since the mechanics of what you've
> got aren't very specific to this machine, just the lists of endpoints.
> I'd also suggest masking off the controls you're controlling via the
> scenario stuff from user space - Takashi has commit
> 03ad1d710ea51a51dfbe62caa2bc9a9437ae3fed in his sound-unstable-2.6.git
> which adds snd_ctl_activate_id(), allowing drivers to do this.
Will include into the next submission.

> More comments below, mostly very nitpicky:
I like nipicking :)

>
>> It doesn't cope with :
>>  - headset jack (will be integrade after jack support has
>>    hit ASoC v2)
>
> I just pushed an implementation of that just now, enjoy :)
Good. That would have to wait an ulterior update, though.

>> +#define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
>
> This is also defined somewhere or other in the PXA or ARM architecture
> code - there's a danger of multiple definitions of the macro appearing.
> checkpatch wants extra () too, I kind of agree with it.
There was a discussion to include it in kernel.h. ATM, it is defined in
arch/arm/mach-pxa/generic.h, which is not easily reachable, hence the
definition.
Moreover, if you look at that file, you'll see the definition is the same, and I
think it is perfectly correct.

>> +void setup_muxers(struct snd_soc_codec *codec, const struct mio_mixes_t mixes[])
>> +{
>> +	int pos = 0;
>> +	struct snd_kcontrol *kctl;
>> +	struct snd_ctl_elem_value ucontrol;
>> +	char mname[44];
>> +
>> +	while (mixes[pos].mixname) {
>
> A for loop over the ARRAY_SIZE() might be more idiomatic here...
Yes. Will do.

>
>> +		memset(mname, 0, 44);
>> +		strncpy(mname, mixes[pos].mixname, 43);
>
> It's bikesheding but strcpy plus termination would do the job, wouldn't
> it?
I don't get that ...

>
>> +               if (kctl) {
>
> Shouldn't we at least warn if this is false, it's an error in the static
> data for the maps.
Yes. Will do.

>
>> +                       kctl->get(kctl, &ucontrol);
>> +                       ucontrol.value.enumerated.item[0] = mixes[pos].val;
>> +                       kctl->put(kctl, &ucontrol);
>> +               }
>

> Does an open application (eg, alsamixer) notice these changes without
> the driver telling it?  snd_ctl_notify() is the API call, IIRC though
> there's a reasonable chance I am misremembering.  Though if you do mask
> them from user space it shoudn't matter anyway, I expect.
It does. At least when I change the mode, all the muxers the drivers touches
change state. I don't know how, but it works ...

>> +		{ 0, 0, 1, 1, 1, 0, 1 }, /* MIO_GSM_AUDIO_HEADSET	*/
>> +		{ 0, 1, 1, 1, 0, 1, 0 }, /* MIO_GSM_AUDIO_HANDSFREE*/
>> +		{ 0, 0, 1, 1, 0, 0, 0 }, /* MIO_GSM_AUDIO_BLUETOOTH*/
>
> Interesting indentation?
Unfortunate. Will do.

>
>> +static int isPhoneMode(int scenario)
>> +{
>> +	int onPhone = 0;
>
> The kernel doesn't generally go in for mixedCaseNames except when
> following standards that use them.
Yes. Will do.

>
>> +	switch (scenario) {
>> +	case MIO_GSM_AUDIO_HANDSET:
>> +	case MIO_GSM_AUDIO_HEADSET:
>> +	case MIO_GSM_AUDIO_BLUETOOTH:
>> +	case MIO_GSM_AUDIO_HANDSFREE:
>> +		onPhone = 1;
>
> Setting 0 in a default would be slightly easier to read (or you could
> just use return statements and not bother with the assignments).
Yes. Will go for the direct return statements.
>
>> +static int phone_stream_start(struct snd_soc_codec *codec)
>> +{
>> +       snd_soc_dapm_stream_event(codec, "AC97 HiFi",
>> +                                 SND_SOC_DAPM_STREAM_START);
>> +       return 0;
>> +}
>
> Hrm.  What are these needed for?  I'd have expected that DAPM would
> power active bypass paths up without this (I test that case frequently)
> and this isn't coordinated with what the core is doing at all.
Without this, in asoc-v2, the dapm is not powering any of the elements in the
phone audio path without this. Will check in asoc-v1, but I see no reason this
has changed. I don't quite understand what you mean by "active bypass path" so I
will make the test myself.

>
>> +/* Use GPIO8 for rear speaker amplificator */
>
> Amplifier.
Yes. Will do.

>
>> +static int rear_amp_event(struct snd_soc_dapm_widget *widget,
>> +			  struct snd_kcontrol *kctl, int event)
>> +{
>> +	struct snd_soc_codec *codec = widget->codec;
>> +	int rc;
>> +
>> +	if (SND_SOC_DAPM_EVENT_ON(event))
>> +		rc = rear_amp_power(codec, 1);
>> +	else
>> +		rc = rear_amp_power(codec, 0);
>> +
>> +	return rc;
>> +}
>
> May as well just collapse rear_amp_power() in here, it's got the same if
> statement in it anyway.
Yes. Will do.

>
>> +#define mioa701_wm9713_suspend NULL
>> +#define mioa701_wm9713_resume  NULL
>
> These can just be dropped until they're implemented (they may never need
> to be).
Yes. Will do.

>> +static int mioa701_wm9713_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +
>> +	if (!machine_is_mioa701())
>> +		return -ENODEV;
>
> Seems redundant to check this when you're using a platform device to
> probe - a system should only be defining that device if it can
> supported.  It's needed for most drivers which register the ASoC driver
> directly on probe.
Right. Will trash.

--
Robert

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Mark Brown]

On Thu, Jan 08, 2009 at 10:07:04PM +0100, Robert Jarzmik wrote:
> Mark Brown <broonie@sirena.org.uk> writes:

> > It'd probably be easier to split this up still further, with one patch
> > providing a standard ASoC machine and another adding the scenario stuff
> > after that.

> Wish for 2 patches instead of that one, one for core support and one for
> scenarii ? As this patch has already been reviewed and include into your tree,
> I had hopped I shouldn't inject any more work that what had already be done ...

Exactly the same sort of scrutiny is being applied to the v2 core as it
moves into mainline - it's being broken up into smaller patches and
merged gradually too, often in not quite the same form as it went into
the out of tree branch.

Also note that there's a two stage thing I'm saying here: as well as
"should we do scenarios at all?" there's also the question of how it's
implemented.  This is a generic problem and most of your implementation
looks like it's not so far away from something that could be used by
other boards too so it deserves to be hoisted out into something that
those other systems can use, rather than having people reinvent the
wheel or cut'n'paste.

Right now my feeling is that we probably want to do at least some
scenario stuff in kernel space and that what's sensible for a given
machine will depend on both the capabilities of the machine (having both
GSM and WiFi causes the number of use cases to get much larger, for
example) and if there's hardware restrictions that need to be enforced
(like not being able to use both speaker and headphones at the same
time, which is fairly common but easily enforced without scenarios as
such).

> I'd like to recall what happened to one of the mioa701 users here, when scenarii
> were handled in userspace. The wm9713 chip overheated, and destroyed the battery
> underneath. That was the reason to put that stuff into the kernel :
> protection. This was also already discussed when this was _previously_
> submitted.

If the system is overheating that's something that it's worth
preventing in kernel space.  Do you know what they did to make that
happen - is it a case of enabling more outputs than can comfortably be
driven, for example?

> And I know Liam's API, I tried it. Let me tell you my point of view : the
> mioa701 users have a standard API (alsa) to control the sound of the mioa701. I
> could force them to migrate towards Liam's library. But I can't have a guarantee
> of maintainance over that library. I don't even know if it will be the final
> solution.

The idea is not to replace ALSA for regular applications but rather to
provide an adjunct to it which a central management process can use to
do setup depending on the current system state.  There is no desire at
all to replace ALSA, only to configure it.

At the minute the majority of deployments seem to use alsactl and state
files managed by a central daemon to do this job - though obviously
a dedicated scenario API could just be dropped into that daemon.

> >> +#define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)

> > This is also defined somewhere or other in the PXA or ARM architecture

> There was a discussion to include it in kernel.h. ATM, it is defined in

Yeah, I know - it was pretty strongly NACKed so I'd be shocked if it
ever went in.

> arch/arm/mach-pxa/generic.h, which is not easily reachable, hence the
> definition.

#include <mach/generic.h>? (not tested at all)

> Moreover, if you look at that file, you'll see the definition is the same, and I
> think it is perfectly correct.

Given the strong NACKs, though...  I do believe it's OK but it'd be
better to just include the PXA file (the driver is board-specific after
all).

> >> +		memset(mname, 0, 44);
> >> +		strncpy(mname, mixes[pos].mixname, 43);

> > It's bikesheding but strcpy plus termination would do the job, wouldn't
> > it?

> I don't get that ...

The memset() is mostly redundant and the above should be equivalent to
strncpy() plus an assignment to make sure the string is terminated.

> >> +                       kctl->get(kctl, &ucontrol);
> >> +                       ucontrol.value.enumerated.item[0] = mixes[pos].val;
> >> +                       kctl->put(kctl, &ucontrol);
> >> +               }

> > Does an open application (eg, alsamixer) notice these changes without
> > the driver telling it?  snd_ctl_notify() is the API call, IIRC though
> > there's a reasonable chance I am misremembering.  Though if you do mask
> > them from user space it shoudn't matter anyway, I expect.

> It does. At least when I change the mode, all the muxers the drivers touches
> change state. I don't know how, but it works ...

So the UI of interactive applications update immediately?  In that case
it's fine.

<nitpick>mixers and muxes</nitpick>

> >> +static int phone_stream_start(struct snd_soc_codec *codec)
> >> +{
> >> +       snd_soc_dapm_stream_event(codec, "AC97 HiFi",
> >> +                                 SND_SOC_DAPM_STREAM_START);
> >> +       return 0;
> >> +}

> > Hrm.  What are these needed for?  I'd have expected that DAPM would
> > power active bypass paths up without this (I test that case frequently)
> > and this isn't coordinated with what the core is doing at all.

> Without this, in asoc-v2, the dapm is not powering any of the elements in the
> phone audio path without this. Will check in asoc-v1, but I see no reason this

Is the phone using the I2S interface for digital audio out from the GSM
chipset by any chance?  Some comments at the head of the driver
describing the wiring might be useful.

> has changed. I don't quite understand what you mean by "active bypass path" so I
> will make the test myself.

A bypass path is a path through the codec which doesn't use any DACs and
ADCs but is analogue only.

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Robert Jarzmik]

Mark Brown <broonie@sirena.org.uk> writes:

> On Thu, Jan 08, 2009 at 10:07:04PM +0100, Robert Jarzmik wrote:
>> Mark Brown <broonie@sirena.org.uk> writes:
>
>> Wish for 2 patches instead of that one, one for core support and one for
>> scenarii ? As this patch has already been reviewed and include into your tree,
>> I had hopped I shouldn't inject any more work that what had already be done ...
>
> Exactly the same sort of scrutiny is being applied to the v2 core as it
> moves into mainline - it's being broken up into smaller patches and
> merged gradually too, often in not quite the same form as it went into
> the out of tree branch.
So be it.

> Also note that there's a two stage thing I'm saying here: as well as
> "should we do scenarios at all?" there's also the question of how it's
> implemented.  This is a generic problem and most of your implementation
> looks like it's not so far away from something that could be used by
> other boards too so it deserves to be hoisted out into something that
> those other systems can use, rather than having people reinvent the
> wheel or cut'n'paste.
>
> Right now my feeling is that we probably want to do at least some
> scenario stuff in kernel space and that what's sensible for a given
> machine will depend on both the capabilities of the machine (having both
> GSM and WiFi causes the number of use cases to get much larger, for
> example) and if there's hardware restrictions that need to be enforced
> (like not being able to use both speaker and headphones at the same
> time, which is fairly common but easily enforced without scenarios as
> such).
Would it be accepted a generic API in sound/soc/soc-scenario.c, which more or
less the same construction as with mio_mixes_t, get_scenario and set_scenario,
and a possible callback before and after scenario changes ?

I could propose an API if it is agreed that the API will be eventually accepted,
after technical discussion in this ML.

>
> If the system is overheating that's something that it's worth
> preventing in kernel space.  Do you know what they did to make that
> happen - is it a case of enabling more outputs than can comfortably be
> driven, for example?
IIRC, the guy was playing music on the amplified speaker which is connected to
HPL and OUT3 on the wm9713. He tried to increase the volume, but instead of
touching HPL+OUT3, he changed SPKL+SPKR.
>
> The idea is not to replace ALSA for regular applications but rather to
> provide an adjunct to it which a central management process can use to
> do setup depending on the current system state.  There is no desire at
> all to replace ALSA, only to configure it.
>
> At the minute the majority of deployments seem to use alsactl and state
> files managed by a central daemon to do this job - though obviously
> a dedicated scenario API could just be dropped into that daemon.
Agreed. That wouldn't provide overheat protection of course.

And then, if we had to go that way, why not provide a generic pxa2xx-ac97 +
wm9713 codec couple, without board code, drop board code, and let the sound
daemon handle the problems. Personnaly, with my experience on smartphones, I
feel very reluctant to it. I think sound on a phone is a critical enough issue
to be handled in kernel space.

>> >> +#define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
>
> Yeah, I know - it was pretty strongly NACKed so I'd be shocked if it
> ever went in.
You're right, it was NACKed. I don't remember the "strongly", would you please
provide me your source. From what I read, there was bickering about the naming, and
the fact that the macro was generating 2 arguments which could be confusing
(which is neither specified in Documentation/CodingStyle nor wrong, as a macro
is _not_ a function, and does not need to represent a lvalue, but that's another
debate we could talk about another time ...)
>
>> arch/arm/mach-pxa/generic.h, which is not easily reachable, hence the
>> definition.
>
> #include <mach/generic.h>? (not tested at all)
Wouldn't that point to arch/arm/mach-pxa/include/mach instead of
arch/arm/mach-pxa ?

>> Moreover, if you look at that file, you'll see the definition is the same, and I
>> think it is perfectly correct.
>
> Given the strong NACKs, though...  I do believe it's OK but it'd be
> better to just include the PXA file (the driver is board-specific after
> all).
So do I, if there was a simple way.

>
>> >> +		memset(mname, 0, 44);
>> >> +		strncpy(mname, mixes[pos].mixname, 43);
>
>> > It's bikesheding but strcpy plus termination would do the job, wouldn't
>> > it?
>
>> I don't get that ...
>
> The memset() is mostly redundant and the above should be equivalent to
> strncpy() plus an assignment to make sure the string is terminated.
Ah, you mean a strlcpy(). OK, will do.

> <nitpick>mixers and muxes</nitpick>
Mixers is more "frogish", isn't it ? :)
>
> Is the phone using the I2S interface for digital audio out from the GSM
> chipset by any chance?  Some comments at the head of the driver
> describing the wiring might be useful.
No, AC97 drivers the wm9713. The analog phone signal comes from GSM chip in
(MonoIn, PCBeep) and are driver to (HPL, HPR) on headset for example. There is
no digital signal, only analog routing.

The comment are all in mixes_gsm_call_headset. You have the full path. I can
move them in the header if you wish, or try some ascii art of the wm9713
connection on the mioa701 board.
>
>> has changed. I don't quite understand what you mean by "active bypass path" so I
>> will make the test myself.
>
> A bypass path is a path through the codec which doesn't use any DACs and
> ADCs but is analogue only.
Ah, exactly my case. I wonder why dapm can't "guess" that path ... The pins are
activated, the audio pathes are known ... Can I take some debug info that could
help to understand the problem ?

Cheers

--
Robert

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Mark Brown]

On Fri, Jan 09, 2009 at 01:43:38PM +0100, Robert Jarzmik wrote:

> Would it be accepted a generic API in sound/soc/soc-scenario.c, which more or
> less the same construction as with mio_mixes_t, get_scenario and set_scenario,
> and a possible callback before and after scenario changes ?

> I could propose an API if it is agreed that the API will be eventually accepted,
> after technical discussion in this ML.

Yes, I think that's a good approach.  Perhaps if you sketch out the
interface before delving into the actual implementation?

> > If the system is overheating that's something that it's worth
> > preventing in kernel space.  Do you know what they did to make that
> > happen - is it a case of enabling more outputs than can comfortably be
> > driven, for example?

> IIRC, the guy was playing music on the amplified speaker which is connected to
> HPL and OUT3 on the wm9713. He tried to increase the volume, but instead of
> touching HPL+OUT3, he changed SPKL+SPKR.

So the two speakers can't safely be run simultaneously, then?

Actually, looking at your driver are there any pins that are not
connected and never touched by the scenario stuff?  By default pins will
be enabled unless otherwise stated.

> And then, if we had to go that way, why not provide a generic pxa2xx-ac97 +
> wm9713 codec couple, without board code, drop board code, and let the sound
> daemon handle the problems. Personnaly, with my experience on smartphones, I

That's a perfectly sensible approach if there are classes of system
which are generic enough to make the code reuse meanigful - if they just
slightly different sets of pins marked as not connected, for example.
There are some examples already, such as the spitz machine which handles
three different platforms with two options for a GPIO.  At a certain
point there will be enough special cases or something fiddly enough to
make it worth splitting off to make multiple machine drivers.

For really straightfoward systems then the standard ALSA AC97 driver may
be all people need.

> feel very reluctant to it. I think sound on a phone is a critical enough issue
> to be handled in kernel space.

The sound also has to agree with things like the GSM chipset and what
the user is trying to do.

> >> >> +#define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)

> You're right, it was NACKed. I don't remember the "strongly", would you please
> provide me your source. From what I read, there was bickering about the naming, and

Google isn't turning up anything particularly strong so I guess I'm
misremembering.

> >> arch/arm/mach-pxa/generic.h, which is not easily reachable, hence the
> >> definition.

> > #include <mach/generic.h>? (not tested at all)

> Wouldn't that point to arch/arm/mach-pxa/include/mach instead of
> arch/arm/mach-pxa ?

Yes, it would - misread what you wrote there.

> > <nitpick>mixers and muxes</nitpick>

> Mixers is more "frogish", isn't it ? :)

You had "muxers".

> The comment are all in mixes_gsm_call_headset. You have the full path. I can
> move them in the header if you wish, or try some ascii art of the wm9713
> connection on the mioa701 board.

What you've got is fine, it's just that that wouldn't cover non-DAPM
things like a connection to the voice PCM (that should appear in the
DAIs).

> Ah, exactly my case. I wonder why dapm can't "guess" that path ... The pins are
> activated, the audio pathes are known ... Can I take some debug info that could
> help to understand the problem ?

#define DEBUG at the top of soc-dapm.c - it'll be *very* verbose about
the power decisions it's making.

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Liam Girdwood]

On Fri, 2009-01-09 at 01:02 +0000, Mark Brown wrote:

> Right now my feeling is that we probably want to do at least some
> scenario stuff in kernel space and that what's sensible for a given
> machine will depend on both the capabilities of the machine (having both
> GSM and WiFi causes the number of use cases to get much larger, for
> example) and if there's hardware restrictions that need to be enforced
> (like not being able to use both speaker and headphones at the same
> time, which is fairly common but easily enforced without scenarios as
> such).

My feeling is that kernel based scenario really should be minimal to non
existent depending upon machine. If it has to be implemented then it
must be consistent across all devices (probably worth creating a
sound/core/scenario.c for others to use).

Imho it's far better to do most scenario in userspace due to increasing
driver complexity and slow driver scenario development speed (remember
the OSS driver pain involved with this). Userspace will additionally
give us a consistent API for applications and hopefully much better
adoption (esp if merged into alsa-lib / salsa).

Btw, I'll be releasing a new version of the ALSA scenario library early
Feb. http://www.slimlogic.co.uk/?page_id=7


Liam

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Takashi Iwai]

At Fri, 09 Jan 2009 13:20:38 +0000,
Liam Girdwood wrote:
> 
> On Fri, 2009-01-09 at 01:02 +0000, Mark Brown wrote:
> 
> > Right now my feeling is that we probably want to do at least some
> > scenario stuff in kernel space and that what's sensible for a given
> > machine will depend on both the capabilities of the machine (having both
> > GSM and WiFi causes the number of use cases to get much larger, for
> > example) and if there's hardware restrictions that need to be enforced
> > (like not being able to use both speaker and headphones at the same
> > time, which is fairly common but easily enforced without scenarios as
> > such).
> 
> My feeling is that kernel based scenario really should be minimal to non
> existent depending upon machine. If it has to be implemented then it
> must be consistent across all devices (probably worth creating a
> sound/core/scenario.c for others to use).
> 
> Imho it's far better to do most scenario in userspace due to increasing
> driver complexity and slow driver scenario development speed (remember
> the OSS driver pain involved with this). Userspace will additionally
> give us a consistent API for applications and hopefully much better
> adoption (esp if merged into alsa-lib / salsa).

Yep, if the API/implementation gets matured enough, it's worth to merge.

> Btw, I'll be releasing a new version of the ALSA scenario library early
> Feb. http://www.slimlogic.co.uk/?page_id=7

Great.  Please inform if it's finished.


thanks,

Takashi

Re: [PATCH 1/2] Add initial support of Mitac mioa701 device SoC.

[Mark Brown]

On Fri, Jan 09, 2009 at 01:20:38PM +0000, Liam Girdwood wrote:
> On Fri, 2009-01-09 at 01:02 +0000, Mark Brown wrote:

> > Right now my feeling is that we probably want to do at least some
> > scenario stuff in kernel space and that what's sensible for a given

> My feeling is that kernel based scenario really should be minimal to non
> existent depending upon machine. If it has to be implemented then it
> must be consistent across all devices (probably worth creating a
> sound/core/scenario.c for others to use).

Yes, that's pretty much what I'm thinking.  The kernel should have
anything required to prevent hardware damage in it but beyond that
it's getting debatable.

It also seems useful to have some support for doing a fixed rename of
controls in machine drivers to allow things like renaming of outputs and
inputs to match the connections on the machine.

I'd probably be willing to see a small amount of additional scenario
code in kernel space for cases where it's clear that the hardware can
only run in a very small number of scenarios due to the limited features
it has, especially if it ties in with stuff that needs to be done to
prevent hardware damage.  It looks like this device may well fall into
that sort of use case.

> Imho it's far better to do most scenario in userspace due to increasing
> driver complexity and slow driver scenario development speed (remember
> the OSS driver pain involved with this). Userspace will additionally

It's also policy, which on general principle shouldn't go into the
kernel anyway.  With devices like the OpenMoko it looks out you pretty
much need to have any to any routing available to user space anyway to
cover all the use cases people have so you have to have a way of doing
scenarios in user space.

> give us a consistent API for applications and hopefully much better
> adoption (esp if merged into alsa-lib / salsa).

Or adopted by things like the FSO stack.