[PATCH 01/16] ASoC: core: Add multi reg control struct & macros

[PATCH 01/16] ASoC: core: Add multi reg control struct & macros

[Ola Lilja]

From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

Add support for controls that exposes a single signed value
while spanning multiple codec registers in a MSB/LSB manner.

Definition of a generic control struct added.

soc_mreg_control

Also four generic convenience macros added:

SOC_SINGLE_VALUE_S1R  One control value spans one register
SOC_SINGLE_VALUE_S2R  One control value spans two registers
SOC_SINGLE_VALUE_S4R  One control value spans four registers
SOC_SINGLE_VALUE_S8R  One control value spans eight registers

Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
---
 include/sound/soc.h |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 0992dff..dac20e0 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -185,6 +185,29 @@
 		 .rreg = xreg_right, .shift = xshift, \
 		 .min = xmin, .max = xmax} }
 
+#define SOC_SINGLE_VALUE_S1R(xreg0, xcount, xmin, xmax, xinvert) \
+	((unsigned long)&(struct soc_mreg_control) \
+	{ .reg = ((unsigned int[]){ xreg0 }), \
+	.rcount = 1, .count = xcount, \
+	.invert = xinvert, .min = xmin, .max = xmax})
+#define SOC_SINGLE_VALUE_S2R(xreg0, xreg1, xcount, xmin, xmax, xinvert) \
+	((unsigned long)&(struct soc_mreg_control) \
+	{.reg = ((unsigned int[]){ xreg0, xreg1 }), \
+	.rcount = 2, .count = xcount, \
+	.min = xmin, .max = xmax, .invert = xinvert})
+#define SOC_SINGLE_VALUE_S4R(xreg0, xreg1, xreg2, xreg3, \
+	xcount, xmin, xmax, xinvert) \
+	((unsigned long)&(struct soc_mreg_control) \
+	{.reg = ((unsigned int[]){ xreg0, xreg1, xreg2, xreg3 }), \
+	.rcount = 4, .count = xcount,  \
+	.min = xmin, .max = xmax, .invert = xinvert})
+#define SOC_SINGLE_VALUE_S8R(xreg0, xreg1, xreg2, xreg3, xreg4, \
+		xreg5, xreg6, xreg7, xcount, xmin, xmax, xinvert) \
+	((unsigned long)&(struct soc_mreg_control) \
+	{.reg = ((unsigned int[]){ xreg0, xreg1, xreg2, xreg3, \
+			xreg4, xreg5, xreg6, xreg7 }), \
+	.rcount = 8, .count = xcount, \
+	.min = xmin, .max = xmax, .invert = xinvert})
 
 /*
  * Simplified versions of above macros, declaring a struct and calculating
@@ -875,6 +898,13 @@ struct soc_mixer_control {
 	unsigned int reg, rreg, shift, rshift, invert;
 };
 
+/* multi register control */
+struct soc_mreg_control {
+	long min, max;
+	unsigned int rcount, count, invert;
+	unsigned int *reg;
+};
+
 /* enumerated kcontrol */
 struct soc_enum {
 	unsigned short reg;
-- 
1.7.8.3

[PATCH 02/16] ASoC: core: Add 8bit multi reg control accessors

[Ola Lilja]

From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

Added get/put accessors for controls that span multiple 8bit registers
which together forms a single signed value in a MSB/LSB manner.

snd_soc_get_xr8_sx
snd_soc_put_xr8_sx

Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
---
 include/sound/soc.h  |    4 ++
 sound/soc/soc-core.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index dac20e0..d2e1d07 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -432,6 +432,10 @@ int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_get_xr8_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_put_xr8_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
 
 /**
  * struct snd_soc_reg_access - Describes whether a given register is
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b5ecf6d..b72f238 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2641,6 +2641,91 @@ int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_2r_sx);
 
 /**
+ * snd_soc_get_xr8_sx - signed multi register get callback
+ * @kcontrol: mreg control
+ * @ucontrol: control element information
+ *
+ * Callback to get the value of a control that can
+ * span multiple 8bit codec registers which together
+ * forms a single signed value in a MSB/LSB manner.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_get_xr8_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mreg_control *mc =
+		(struct soc_mreg_control *)kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int *reg = mc->reg;
+	unsigned int rcount = mc->rcount;
+	long min = mc->min;
+	long max = mc->max;
+	unsigned int invert = mc->invert;
+	unsigned long mask = abs(min) | abs(max);
+	long value = 0;
+	int i, rvalue;
+
+	for (i = 0; i < rcount; i++) {
+		rvalue = snd_soc_read(codec, reg[i]) & 0xff;
+		value |= rvalue << (8 * (rcount - i - 1));
+	}
+	value &= mask;
+	if (min < 0 && value > max)
+		value |= ~mask;
+	if (invert)
+		value = ~value;
+	ucontrol->value.integer.value[0] = value;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_get_xr8_sx);
+
+/**
+ * snd_soc_put_xr8_sx - signed multi register get callback
+ * @kcontrol: mreg control
+ * @ucontrol: control element information
+ *
+ * Callback to set the value of a control that can
+ * span multiple 8bit codec registers which together
+ * forms a single signed value in a MSB/LSB manner.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_put_xr8_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mreg_control *mc =
+		(struct soc_mreg_control *)kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int *reg = mc->reg;
+	unsigned int rcount = mc->rcount;
+	long min = mc->min;
+	long max = mc->max;
+	unsigned int invert = mc->invert;
+	unsigned long mask = abs(min) | abs(max);
+	long value = ucontrol->value.integer.value[0];
+	int i, rvalue, err;
+
+	if (invert)
+		value = ~value;
+	if (value > max)
+		value = max;
+	else if (value < min)
+		value = min;
+	value &= mask;
+	for (i = 0; i < rcount; i++) {
+		rvalue = (value >> (8 * (rcount - i - 1))) & 0xff;
+		err = snd_soc_write(codec, reg[i], rvalue);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_put_xr8_sx);
+
+/**
  * snd_soc_dai_set_sysclk - configure DAI system or master clock.
  * @dai: DAI
  * @clk_id: DAI specific clock ID
-- 
1.7.8.3

Re: [PATCH 02/16] ASoC: core: Add 8bit multi reg control accessors

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 579 bytes --]

On Tue, Mar 13, 2012 at 04:11:29PM +0100, Ola Lilja wrote:
> From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
> 
> Added get/put accessors for controls that span multiple 8bit registers
> which together forms a single signed value in a MSB/LSB manner.
> 
> snd_soc_get_xr8_sx
> snd_soc_put_xr8_sx
> 
> Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

This needs to be part of a patch adding one or more actual control
types, just adding bits like this makes things harder to review as it's
hard to see how things fit together.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 02/16] ASoC: core: Add 8bit multi reg control accessors

[Kristoffer KARLSSON]

On 2012-03-13 22:25, Mark Brown wrote:

> On Tue, Mar 13, 2012 at 04:11:29PM +0100, Ola Lilja wrote:
>> From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
>>
>> Added get/put accessors for controls that span multiple 8bit registers
>> which together forms a single signed value in a MSB/LSB manner.
>>
>> snd_soc_get_xr8_sx
>> snd_soc_put_xr8_sx
>>
>> Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
> 
> This needs to be part of a patch adding one or more actual control
> types, just adding bits like this makes things harder to review as it's
> hard to see how things fit together.


I agree. I will provide a more complete patch including actual control type so
that reviewing would be made easier. The patch will expose parameters like
"register base" and "register count". This control will then fully support any
composite values composed from a parameterized number of contiguous 8-bit
registers in only one single macro while fully supporting both writing and
reading back the composite values transparently to the client.

We do have several controls composed by chunks of 8-bit registers that both are
contiguous and supports reading back in our hardware and I guess that this
might not be such an unusual setup for other hardwares either, which would make
for a useful generic control I believe.

Do you agree that such a control type could be an useful generic one in the
framework?

Re: [PATCH 02/16] ASoC: core: Add 8bit multi reg control accessors

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 532 bytes --]

On Thu, Mar 22, 2012 at 05:58:41PM +0100, Kristoffer KARLSSON wrote:

> We do have several controls composed by chunks of 8-bit registers that both are
> contiguous and supports reading back in our hardware and I guess that this
> might not be such an unusual setup for other hardwares either, which would make
> for a useful generic control I believe.

> Do you agree that such a control type could be an useful generic one in the
> framework?

It should support registers of any size, not just 8 bit registers, but
otherwise yes.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 03/16] ASoC: core: Add range of reg control accessors

[Ola Lilja]

From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

Added get/put accessors for controls that maps a range of
consecutive registers exposed as a multiple value control.

This is specifically useful for hardware that exposes for instance
a range of filter parameters as a range of registers that with
this could be written and read back in one single ioctl operation.

snd_soc_get_m1r_sx
snd_soc_put_m1r_sx

Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
---
 include/sound/soc.h  |    4 +++
 sound/soc/soc-core.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index d2e1d07..66953a6 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -436,6 +436,10 @@ int snd_soc_get_xr8_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_xr8_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_get_m1r_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_put_m1r_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
 
 /**
  * struct snd_soc_reg_access - Describes whether a given register is
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b72f238..bacc465 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2726,6 +2726,78 @@ int snd_soc_put_xr8_sx(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_put_xr8_sx);
 
 /**
+ * snd_soc_get_m1r_sx - multiple register get callback
+ * @kcontrol: mreg control
+ * @ucontrol: control element information
+ *
+ * Callback to get the all values of a control
+ * from a chunk of registers.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_get_m1r_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mreg_control *mc =
+		(struct soc_mreg_control *)kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int reg = *mc->reg;
+	unsigned int count = mc->count;
+	unsigned int invert = mc->invert;
+	unsigned int idx;
+	long value;
+
+	for (idx = 0; idx < count; idx++) {
+		value = snd_soc_read(codec, reg + idx);
+		if (invert)
+			value = ~value;
+		ucontrol->value.integer.value[idx] = value;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_get_m1r_sx);
+
+/**
+ * snd_soc_put_m1r_sx - multiple register get callback
+ * @kcontrol: mreg control
+ * @ucontrol: control element information
+ *
+ * Callback to set the all values of a control
+ * from a chunk of registers.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_put_m1r_sx(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mreg_control *mc =
+			(struct soc_mreg_control *) kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int reg = *mc->reg;
+	unsigned int count = mc->count;
+	long min = mc->min;
+	long max = mc->max;
+	unsigned int invert = mc->invert;
+	unsigned int idx;
+	long value;
+
+	for (idx = 0; idx < count; idx++) {
+		value = ucontrol->value.integer.value[idx];
+		if (value > max)
+			value = max;
+		else if (value < min)
+			value = min;
+		if (invert)
+			value = ~value;
+		snd_soc_write(codec, reg + idx, value);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_put_m1r_sx);
+
+/**
  * snd_soc_dai_set_sysclk - configure DAI system or master clock.
  * @dai: DAI
  * @clk_id: DAI specific clock ID
-- 
1.7.8.3

Re: [PATCH 03/16] ASoC: core: Add range of reg control accessors

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 489 bytes --]

On Tue, Mar 13, 2012 at 04:11:30PM +0100, Ola Lilja wrote:
> From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
> 
> Added get/put accessors for controls that maps a range of
> consecutive registers exposed as a multiple value control.
> 
> This is specifically useful for hardware that exposes for instance
> a range of filter parameters as a range of registers that with
> this could be written and read back in one single ioctl operation.

This is SND_SOC_BYTES.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 04/16] ASoC: core: Add info accessor for mreg control

[Ola Lilja]

From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

Added info accessor for mreg controls that includes support for signed values.

snd_soc_info_xr_sx

Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
---
 include/sound/soc.h  |    2 ++
 sound/soc/soc-core.c |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 66953a6..0437c12 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -432,6 +432,8 @@ int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_info_xr_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_info *uinfo);
 int snd_soc_get_xr8_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_xr8_sx(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index bacc465..16d071e 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2641,6 +2641,30 @@ int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_2r_sx);
 
 /**
+ * snd_soc_info_xr_sx - signed multi register info callback
+ * @kcontrol: mreg control
+ * @uinfo: control element information
+ *
+ * Callback to provide information about a control
+ * that supports multiple signed values.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_info_xr_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_info *uinfo)
+{
+	struct soc_mreg_control *mc =
+		(struct soc_mreg_control *)kcontrol->private_value;
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = mc->count;
+	uinfo->value.integer.min = mc->min;
+	uinfo->value.integer.max = mc->max;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_info_xr_sx);
+
+/**
  * snd_soc_get_xr8_sx - signed multi register get callback
  * @kcontrol: mreg control
  * @ucontrol: control element information
-- 
1.7.8.3

[PATCH 05/16] ASoC: core: Add strobe control

[Ola Lilja]

From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

Added support for a control that strobes a bit in
a register to high then back to low (or the inverse).

This is typically useful for hardware that requires
strobing a singe bit to trigger some functionality
and where exposing the bit in a normal enum control
would require the user to first manually set then
again unset the bit again for the strobe to trigger.

Get/put accessors added.

snd_soc_get_enum_strobe
snd_soc_put_enum_strobe

Also a generic convenience macros added.

SOC_ENUM_STROBE

And a macro for declaration of the enum.

SOC_ENUM_STROBE_DECL

Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
---
 include/sound/soc.h  |   11 +++++++++
 sound/soc/soc-core.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 0437c12..a5e782c 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -209,6 +209,11 @@
 	.rcount = 8, .count = xcount, \
 	.min = xmin, .max = xmax, .invert = xinvert})
 
+#define SOC_ENUM_STROBE(xname, xenum) \
+	SOC_ENUM_EXT(xname, xenum, \
+	snd_soc_get_enum_strobe, \
+	snd_soc_put_enum_strobe)
+
 /*
  * Simplified versions of above macros, declaring a struct and calculating
  * ARRAY_SIZE internally
@@ -225,6 +230,8 @@
 							ARRAY_SIZE(xtexts), xtexts, xvalues)
 #define SOC_VALUE_ENUM_SINGLE_DECL(name, xreg, xshift, xmask, xtexts, xvalues) \
 	SOC_VALUE_ENUM_DOUBLE_DECL(name, xreg, xshift, xshift, xmask, xtexts, xvalues)
+#define SOC_ENUM_STROBE_DECL(name, xreg, xbit, xinvert, xtexts) \
+	struct soc_enum name = SOC_ENUM_DOUBLE(xreg, xbit, xinvert, 2, xtexts)
 
 /*
  * Component probe and remove ordering levels for components with runtime
@@ -442,6 +449,10 @@ int snd_soc_get_m1r_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_m1r_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_get_enum_strobe(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_put_enum_strobe(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
 
 /**
  * struct snd_soc_reg_access - Describes whether a given register is
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 16d071e..3a5e519 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2822,6 +2822,64 @@ int snd_soc_put_m1r_sx(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_put_m1r_sx);
 
 /**
+ * snd_soc_get_enum_strobe - enum strobe get callback
+ * @kcontrol: mixer control
+ * @ucontrol: control element information
+ *
+ * Callback get the value of an enum strobe mixer control.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_get_enum_strobe(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int reg = e->reg;
+	unsigned int bit = e->shift_l;
+	unsigned int invert = e->shift_r != 0;
+	unsigned int value = snd_soc_read(codec, reg) & (1 << bit);
+
+	if (bit != 0 && value != 0)
+		value = value >> bit;
+	ucontrol->value.enumerated.item[0] = value ^ invert;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_get_enum_strobe);
+
+/**
+ * snd_soc_put_enum_strobe - enum strobe put callback
+ * @kcontrol: mixer control
+ * @ucontrol: control element information
+ *
+ * Callback strobe a register bit to high then low (or the inverse)
+ * in one pass of a single mixer enum control.
+ *
+ * Returns 1 for success.
+ */
+int snd_soc_put_enum_strobe(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int reg = e->reg;
+	unsigned int bit = e->shift_l;
+	unsigned int invert = e->shift_r != 0;
+	unsigned int strobe = ucontrol->value.enumerated.item[0] != 0;
+	unsigned int clr_mask = (strobe ^ invert) ? 0 : (1 << bit);
+	unsigned int set_mask = (strobe ^ invert) ? (1 << bit) : 0;
+	int err;
+
+	err = snd_soc_update_bits_locked(codec, reg, clr_mask, set_mask);
+	if (err < 0)
+		return err;
+	err = snd_soc_update_bits_locked(codec, reg, set_mask, clr_mask);
+	return err;
+}
+EXPORT_SYMBOL_GPL(snd_soc_put_enum_strobe);
+
+/**
  * snd_soc_dai_set_sysclk - configure DAI system or master clock.
  * @dai: DAI
  * @clk_id: DAI specific clock ID
-- 
1.7.8.3

Re: [PATCH 05/16] ASoC: core: Add strobe control

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 857 bytes --]

On Tue, Mar 13, 2012 at 04:11:32PM +0100, Ola Lilja wrote:
> From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
> 
> Added support for a control that strobes a bit in
> a register to high then back to low (or the inverse).
> 
> This is typically useful for hardware that requires
> strobing a singe bit to trigger some functionality
> and where exposing the bit in a normal enum control
> would require the user to first manually set then
> again unset the bit again for the strobe to trigger.
> 
> Get/put accessors added.
> 
> snd_soc_get_enum_strobe
> snd_soc_put_enum_strobe
> 
> Also a generic convenience macros added.
> 
> SOC_ENUM_STROBE

Based on this description it's hard to see why this control is patterned
after an enum - why would we have an enumerated control to bounce a
single register bit on then off?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 05/16] ASoC: core: Add strobe control

[Kristoffer KARLSSON]

On 2012-03-13 22:33, Mark Brown wrote:

> On Tue, Mar 13, 2012 at 04:11:32PM +0100, Ola Lilja wrote:
>> From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
>>
>> Added support for a control that strobes a bit in
>> a register to high then back to low (or the inverse).
>>
>> This is typically useful for hardware that requires
>> strobing a singe bit to trigger some functionality
>> and where exposing the bit in a normal enum control
>> would require the user to first manually set then
>> again unset the bit again for the strobe to trigger.
>>
>> Get/put accessors added.
>>
>> snd_soc_get_enum_strobe
>> snd_soc_put_enum_strobe
>>
>> Also a generic convenience macros added.
>>
>> SOC_ENUM_STROBE
> 
> Based on this description it's hard to see why this control is patterned
> after an enum - why would we have an enumerated control to bounce a
> single register bit on then off?


I originally chose to pattern this control after an enum since enum controls
would allow for exposing such intuitive textual information to the client about
the state of the control for this type of use case.

The idea being that the enum would have two options like ('Ready'/'Apply' or
possibly 'Idle'/'Activate'). Setting 'Apply' would then strobe the bit high
then low and a consecutive read for a client of this control would then return
'Ready' to signify that the hardware is ready for a new strobe push.

I suppose that the control might as well be modeled after a SOC_SINGLE instead.
I noticed that a SOC_SINGLE_EXT with max = 1 would  set element type to BOOLEAN
in snd_soc_info_volsw which I guess also would work just fine for this type of
control.

Do you think that SOC_SINGLE would be a choice to pattern this control from?

Or would you prefer that the control still be textual but more specifically
force only exactly two textual options and not just any generic enum?

Re: [PATCH 05/16] ASoC: core: Add strobe control

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 727 bytes --]

On Thu, Mar 22, 2012 at 05:20:06PM +0100, Kristoffer KARLSSON wrote:
> On 2012-03-13 22:33, Mark Brown wrote:
> > On Tue, Mar 13, 2012 at 04:11:32PM +0100, Ola Lilja wrote:

> > Based on this description it's hard to see why this control is patterned
> > after an enum - why would we have an enumerated control to bounce a
> > single register bit on then off?

> I originally chose to pattern this control after an enum since enum controls
> would allow for exposing such intuitive textual information to the client about
> the state of the control for this type of use case.

But if this is a strobe control it has no state, you do a write to
generate a strobe but the strobe lasts for no meaningful time so isn't
observable.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 05/16] ASoC: core: Add strobe control

[Kristoffer KARLSSON]

On 2012-03-22 17:33, Mark Brown wrote:

> On Thu, Mar 22, 2012 at 05:20:06PM +0100, Kristoffer KARLSSON wrote:
>> On 2012-03-13 22:33, Mark Brown wrote:
>>> On Tue, Mar 13, 2012 at 04:11:32PM +0100, Ola Lilja wrote:
> 
>>> Based on this description it's hard to see why this control is patterned
>>> after an enum - why would we have an enumerated control to bounce a
>>> single register bit on then off?
> 
>> I originally chose to pattern this control after an enum since enum controls
>> would allow for exposing such intuitive textual information to the client about
>> the state of the control for this type of use case.
> 
> But if this is a strobe control it has no state, you do a write to
> generate a strobe but the strobe lasts for no meaningful time so isn't
> observable.


You are correct. The state would not be the most important thing since normally
there be only one client accessing the card in this very short period of time
like you said. However the allowing for a way to expose a strobe "action"
control would. So do you think it should be modeled after a SOC_SINGLE_EXT
instead or how would you prefer that a strobe "action" control be implemented?

Re: [PATCH 05/16] ASoC: core: Add strobe control

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 494 bytes --]

On Thu, Mar 22, 2012 at 06:09:22PM +0100, Kristoffer KARLSSON wrote:

> You are correct. The state would not be the most important thing since normally
> there be only one client accessing the card in this very short period of time
> like you said. However the allowing for a way to expose a strobe "action"
> control would. So do you think it should be modeled after a SOC_SINGLE_EXT
> instead or how would you prefer that a strobe "action" control be implemented?

More like a a single, yes.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 06/16] ASoC: core: Add macros for 8bit hwdep multi reg cntrl

[Ola Lilja]

From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

Added four comvenience macros for hwdep multi register controls
that exposes a single signed value while spanning multiple
codec registers in a MSB/LSB manner.

SOC_HWDEP_SINGLE_1R8  One hwdep signed control value spans one 8bit register
SOC_HWDEP_SINGLE_2R8  One hwdep signed control value spans two 8bit two registers
SOC_HWDEP_SINGLE_4R8  One hwdep signed control value spans four 8bit registers
SOC_HWDEP_SINGLE_8R8  One hwdep signed control value spans eight 8bit registers

Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
---
 include/sound/soc.h |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index a5e782c..4acdd35 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -208,6 +208,35 @@
 			xreg4, xreg5, xreg6, xreg7 }), \
 	.rcount = 8, .count = xcount, \
 	.min = xmin, .max = xmax, .invert = xinvert})
+#define SOC_HWDEP_SINGLE_1R8(xname, reg0, min, max, invert) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_HWDEP, .name = xname, \
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.info = snd_soc_info_xr_sx, .get = snd_soc_get_xr8_sx, \
+	.put = snd_soc_put_xr8_sx, \
+	.private_value =  SOC_SINGLE_VALUE_S1R(reg0, 1, min, max, invert) }
+#define SOC_HWDEP_SINGLE_2R8(xname, reg0, reg1, min, max, invert) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_HWDEP, .name = xname, \
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.info = snd_soc_info_xr_sx, .get = snd_soc_get_xr8_sx, \
+	.put = snd_soc_put_xr8_sx, \
+	.private_value = SOC_SINGLE_VALUE_S2R(reg0, reg1, \
+				1, min, max, invert) }
+#define SOC_HWDEP_SINGLE_4R8(xname, reg0, reg1, reg2, reg3, min, max, invert) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_HWDEP, .name = xname, \
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.info = snd_soc_info_xr_sx, .get = snd_soc_get_xr8_sx, \
+	.put = snd_soc_put_xr8_sx, \
+	.private_value =  SOC_SINGLE_VALUE_S4R(reg0, reg1, reg2, reg3, \
+				1, min, max, invert) }
+#define SOC_HWDEP_SINGLE_8R8(xname, reg0, reg1, reg2, reg3, \
+		reg4, reg5, reg6, reg7, min, max, invert) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_HWDEP, .name = xname, \
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.info = snd_soc_info_xr_sx, .get = snd_soc_get_xr8_sx, \
+	.put = snd_soc_put_xr8_sx, \
+	.private_value =  \
+		SOC_SINGLE_VALUE_S4R(reg0, reg1, reg2, reg3, \
+			reg4, reg5, reg6, reg7, 1, min, max, invert) }
 
 #define SOC_ENUM_STROBE(xname, xenum) \
 	SOC_ENUM_EXT(xname, xenum, \
-- 
1.7.8.3

Re: [PATCH 06/16] ASoC: core: Add macros for 8bit hwdep multi reg cntrl

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 448 bytes --]

On Tue, Mar 13, 2012 at 04:11:33PM +0100, Ola Lilja wrote:

> Added four comvenience macros for hwdep multi register controls
> that exposes a single signed value while spanning multiple
> codec registers in a MSB/LSB manner.

What is a "hwdep multi register control" and when would someone want to
use one?  Especially the "hwdep" bit, it's very confusing, and since the
actual implementations of the controls aren't included it's hard to
tell...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 07/16] ASoC: core: Add macro for hwdep range of regs control

[Ola Lilja]

From: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>

Added convenience macro for hwdep range of register controls
that maps a range of consecutive registers exposed as
a multiple value control.

This is specifically useful for hardware that exposes for instance
a range of filter parameters as a range of registers that with
this could be written and read back in one single ioctl operation.

SOC_HWDEP_MULTIPLE_1R  Multiple hwdep signed control values spans one register each

Signed-off-by: Kristoffer KARLSSON <kristoffer.karlsson@stericsson.com>
---
 include/sound/soc.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 4acdd35..644167d 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -237,6 +237,12 @@
 	.private_value =  \
 		SOC_SINGLE_VALUE_S4R(reg0, reg1, reg2, reg3, \
 			reg4, reg5, reg6, reg7, 1, min, max, invert) }
+#define SOC_HWDEP_MULTIPLE_1R(xname, reg0, count, min, max, invert) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_HWDEP, .name = xname, \
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.info = snd_soc_info_xr_sx, .get = snd_soc_get_m1r_sx, \
+	.put = snd_soc_put_m1r_sx, \
+	.private_value = SOC_SINGLE_VALUE_S1R(reg0, count, min, max, invert) }
 
 #define SOC_ENUM_STROBE(xname, xenum) \
 	SOC_ENUM_EXT(xname, xenum, \
-- 
1.7.8.3

[PATCH 08/16] ARM: ux500: Add DMA-channels for MSP

[Ola Lilja]

Add DMA-channels for MSP0, MSP1, MSP2 and MSP3.

Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>
---
 arch/arm/mach-ux500/devices-db8500.c        |    6 ++++++
 arch/arm/mach-ux500/include/mach/hardware.h |    2 ++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/devices-db8500.c b/arch/arm/mach-ux500/devices-db8500.c
index a7c6cdc..6e66d37 100644
--- a/arch/arm/mach-ux500/devices-db8500.c
+++ b/arch/arm/mach-ux500/devices-db8500.c
@@ -101,6 +101,9 @@ static const dma_addr_t dma40_tx_map[DB8500_DMA_NR_DEV] = {
 	[DB8500_DMA_DEV41_SD_MM3_TX] = -1,
 	[DB8500_DMA_DEV42_SD_MM4_TX] = -1,
 	[DB8500_DMA_DEV43_SD_MM5_TX] = -1,
+	[DB8500_DMA_DEV14_MSP2_TX] = U8500_MSP2_BASE + MSP_TX_RX_REG_OFFSET,
+	[DB8500_DMA_DEV30_MSP1_TX] = U8500_MSP1_BASE + MSP_TX_RX_REG_OFFSET,
+	[DB8500_DMA_DEV31_MSP0_TX_SLIM0_CH0_TX] = U8500_MSP0_BASE + MSP_TX_RX_REG_OFFSET,
 };
 
 /* Mapping between source event lines and physical device address */
@@ -133,6 +136,9 @@ static const dma_addr_t dma40_rx_map[DB8500_DMA_NR_DEV] = {
 	[DB8500_DMA_DEV41_SD_MM3_RX] = -1,
 	[DB8500_DMA_DEV42_SD_MM4_RX] = -1,
 	[DB8500_DMA_DEV43_SD_MM5_RX] = -1,
+	[DB8500_DMA_DEV14_MSP2_RX] = U8500_MSP2_BASE + MSP_TX_RX_REG_OFFSET,
+	[DB8500_DMA_DEV30_MSP3_RX] = U8500_MSP3_BASE + MSP_TX_RX_REG_OFFSET,
+	[DB8500_DMA_DEV31_MSP0_RX_SLIM0_CH0_RX] = U8500_MSP0_BASE + MSP_TX_RX_REG_OFFSET,
 };
 
 /* Reserved event lines for memcpy only */
diff --git a/arch/arm/mach-ux500/include/mach/hardware.h b/arch/arm/mach-ux500/include/mach/hardware.h
index b6ba26a..d93d6db 100644
--- a/arch/arm/mach-ux500/include/mach/hardware.h
+++ b/arch/arm/mach-ux500/include/mach/hardware.h
@@ -30,6 +30,8 @@
 #include <mach/db8500-regs.h>
 #include <mach/db5500-regs.h>
 
+#define MSP_TX_RX_REG_OFFSET	0
+
 #ifndef __ASSEMBLY__
 
 #include <mach/id.h>
-- 
1.7.8.3

[PATCH 09/16] arm: ux500: Add audio-regulators

[Ola Lilja]

Add regulators Vaud, Vamic1, Vamic2 and Vdmic
used by audio.

Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>
---
 arch/arm/mach-ux500/board-mop500-regulators.c |   28 +++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-regulators.c b/arch/arm/mach-ux500/board-mop500-regulators.c
index 2735d03..52426a4 100644
--- a/arch/arm/mach-ux500/board-mop500-regulators.c
+++ b/arch/arm/mach-ux500/board-mop500-regulators.c
@@ -74,6 +74,26 @@ static struct regulator_consumer_supply ab8500_vtvout_consumers[] = {
 	REGULATOR_SUPPLY("vddadc", "ab8500-gpadc.0"),
 };
 
+static struct regulator_consumer_supply ab8500_vaud_consumers[] = {
+	/* AB8500 audio-codec main supply */
+	REGULATOR_SUPPLY("vaud", "ab8500-codec.0"),
+};
+
+static struct regulator_consumer_supply ab8500_vamic1_consumers[] = {
+	/* AB8500 audio-codec Mic1 supply */
+	REGULATOR_SUPPLY("vamic1", "ab8500-codec.0"),
+};
+
+static struct regulator_consumer_supply ab8500_vamic2_consumers[] = {
+	/* AB8500 audio-codec Mic2 supply */
+	REGULATOR_SUPPLY("vamic2", "ab8500-codec.0"),
+};
+
+static struct regulator_consumer_supply ab8500_vdmic_consumers[] = {
+	/* AB8500 audio-codec DMic supply */
+	REGULATOR_SUPPLY("vdmic", "ab8500-codec.0"),
+};
+
 static struct regulator_consumer_supply ab8500_vintcore_consumers[] = {
 	/* SoC core supply, no device */
 	REGULATOR_SUPPLY("v-intcore", NULL),
@@ -323,6 +343,8 @@ struct regulator_init_data ab8500_regulators[AB8500_NUM_REGULATORS] = {
 			.name = "V-AUD",
 			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
 		},
+		.num_consumer_supplies = ARRAY_SIZE(ab8500_vaud_consumers),
+		.consumer_supplies = ab8500_vaud_consumers,
 	},
 	/* supply for v-anamic1 VAMic1-LDO */
 	[AB8500_LDO_ANAMIC1] = {
@@ -330,6 +352,8 @@ struct regulator_init_data ab8500_regulators[AB8500_NUM_REGULATORS] = {
 			.name = "V-AMIC1",
 			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
 		},
+		.num_consumer_supplies = ARRAY_SIZE(ab8500_vamic1_consumers),
+		.consumer_supplies = ab8500_vamic1_consumers,
 	},
 	/* supply for v-amic2, VAMIC2 LDO, reuse constants for AMIC1 */
 	[AB8500_LDO_ANAMIC2] = {
@@ -337,6 +361,8 @@ struct regulator_init_data ab8500_regulators[AB8500_NUM_REGULATORS] = {
 			.name = "V-AMIC2",
 			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
 		},
+		.num_consumer_supplies = ARRAY_SIZE(ab8500_vamic2_consumers),
+		.consumer_supplies = ab8500_vamic2_consumers,
 	},
 	/* supply for v-dmic, VDMIC LDO */
 	[AB8500_LDO_DMIC] = {
@@ -344,6 +370,8 @@ struct regulator_init_data ab8500_regulators[AB8500_NUM_REGULATORS] = {
 			.name = "V-DMIC",
 			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
 		},
+		.num_consumer_supplies = ARRAY_SIZE(ab8500_vdmic_consumers),
+		.consumer_supplies = ab8500_vdmic_consumers,
 	},
 	/* supply for v-intcore12, VINTCORE12 LDO */
 	[AB8500_LDO_INTCORE] = {
-- 
1.7.8.3

Re: [PATCH 09/16] arm: ux500: Add audio-regulators

[Linus Walleij]

On Tue, Mar 13, 2012 at 4:11 PM, Ola Lilja <ola.o.lilja@stericsson.com> wrote:

> Add regulators Vaud, Vamic1, Vamic2 and Vdmic
> used by audio.
>
> Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>

This is queued for merge into v3.4 through the ARM SoC tree, so
you'll soon see it upstream.

Linus Walleij

[PATCH 10/16] arm: ux500: Add support for MSP I2S-devices

[Ola Lilja]

Create devices for the MSP-blocks (MSP0, MSP1, MSP2 and MSP3)
and associate it with the correct clocks in the clock-framework.

Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>
---
 arch/arm/mach-ux500/Makefile           |    3 +-
 arch/arm/mach-ux500/board-mop500-msp.c |  194 ++++++++++++++++++++++++++++++++
 arch/arm/mach-ux500/board-mop500.c     |    7 +-
 arch/arm/mach-ux500/board-mop500.h     |    1 +
 arch/arm/mach-ux500/clock.c            |    8 +-
 arch/arm/mach-ux500/devices-common.h   |    4 +-
 arch/arm/mach-ux500/include/mach/msp.h |   29 +++++
 7 files changed, 237 insertions(+), 9 deletions(-)
 create mode 100644 arch/arm/mach-ux500/board-mop500-msp.c
 create mode 100644 arch/arm/mach-ux500/include/mach/msp.h

diff --git a/arch/arm/mach-ux500/Makefile b/arch/arm/mach-ux500/Makefile
index 6bd2f45..6ee4e84 100644
--- a/arch/arm/mach-ux500/Makefile
+++ b/arch/arm/mach-ux500/Makefile
@@ -11,7 +11,8 @@ obj-$(CONFIG_MACH_U8500)	+= board-mop500.o board-mop500-sdi.o \
 				board-mop500-regulators.o \
 				board-mop500-uib.o board-mop500-stuib.o \
 				board-mop500-u8500uib.o \
-				board-mop500-pins.o
+				board-mop500-pins.o \
+				board-mop500-msp.o
 obj-$(CONFIG_MACH_U5500)	+= board-u5500.o board-u5500-sdi.o
 obj-$(CONFIG_SMP)		+= platsmp.o headsmp.o
 obj-$(CONFIG_HOTPLUG_CPU)	+= hotplug.o
diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
new file mode 100644
index 0000000..4223b14
--- /dev/null
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -0,0 +1,194 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License terms: GNU General Public License (GPL), version 2
+ */
+
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/gpio.h>
+#include <plat/gpio-nomadik.h>
+
+#include <plat/pincfg.h>
+#include <plat/ste_dma40.h>
+
+#include <mach/devices.h>
+#include <ste-dma40-db8500.h>
+#include <mach/hardware.h>
+#include <mach/irqs.h>
+#include <mach/msp.h>
+
+#include "board-mop500.h"
+#include "devices-db8500.h"
+#include "pins-db8500.h"
+
+/* MSP1/3 Tx/Rx usage protection */
+static DEFINE_SPINLOCK(msp_rxtx_lock);
+
+/* Reference Count */
+static int msp_rxtx_ref;
+
+static pin_cfg_t mop500_msp1_pins_init[] = {
+		GPIO33_MSP1_TXD | PIN_OUTPUT_LOW   | PIN_SLPM_WAKEUP_DISABLE,
+		GPIO34_MSP1_TFS | PIN_INPUT_NOPULL | PIN_SLPM_WAKEUP_DISABLE,
+		GPIO35_MSP1_TCK | PIN_INPUT_NOPULL | PIN_SLPM_WAKEUP_DISABLE,
+		GPIO36_MSP1_RXD | PIN_INPUT_NOPULL | PIN_SLPM_WAKEUP_DISABLE,
+};
+
+static pin_cfg_t mop500_msp1_pins_exit[] = {
+		GPIO33_MSP1_TXD | PIN_OUTPUT_LOW   | PIN_SLPM_WAKEUP_ENABLE,
+		GPIO34_MSP1_TFS | PIN_INPUT_NOPULL | PIN_SLPM_WAKEUP_ENABLE,
+		GPIO35_MSP1_TCK | PIN_INPUT_NOPULL | PIN_SLPM_WAKEUP_ENABLE,
+		GPIO36_MSP1_RXD | PIN_INPUT_NOPULL | PIN_SLPM_WAKEUP_ENABLE,
+};
+
+int msp13_i2s_init(void)
+{
+	int retval = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&msp_rxtx_lock, flags);
+	if (msp_rxtx_ref == 0)
+		retval = nmk_config_pins(
+				ARRAY_AND_SIZE(mop500_msp1_pins_init));
+	if (!retval)
+		msp_rxtx_ref++;
+	spin_unlock_irqrestore(&msp_rxtx_lock, flags);
+
+	return retval;
+}
+
+int msp13_i2s_exit(void)
+{
+	int retval = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&msp_rxtx_lock, flags);
+	WARN_ON(!msp_rxtx_ref);
+	msp_rxtx_ref--;
+	if (msp_rxtx_ref == 0)
+		retval = nmk_config_pins_sleep(
+				ARRAY_AND_SIZE(mop500_msp1_pins_exit));
+	spin_unlock_irqrestore(&msp_rxtx_lock, flags);
+
+	return retval;
+}
+
+static struct stedma40_chan_cfg msp0_dma_rx = {
+	.high_priority = true,
+	.dir = STEDMA40_PERIPH_TO_MEM,
+
+	.src_dev_type = DB8500_DMA_DEV31_MSP0_RX_SLIM0_CH0_RX,
+	.dst_dev_type = STEDMA40_DEV_DST_MEMORY,
+
+	.src_info.psize = STEDMA40_PSIZE_LOG_4,
+	.dst_info.psize = STEDMA40_PSIZE_LOG_4,
+
+	/* data_width is set during configuration */
+};
+
+static struct stedma40_chan_cfg msp0_dma_tx = {
+	.high_priority = true,
+	.dir = STEDMA40_MEM_TO_PERIPH,
+
+	.src_dev_type = STEDMA40_DEV_DST_MEMORY,
+	.dst_dev_type = DB8500_DMA_DEV31_MSP0_TX_SLIM0_CH0_TX,
+
+	.src_info.psize = STEDMA40_PSIZE_LOG_4,
+	.dst_info.psize = STEDMA40_PSIZE_LOG_4,
+
+	/* data_width is set during configuration */
+};
+
+static struct msp_i2s_platform_data msp0_platform_data = {
+	.id = MSP_I2S_0,
+	.msp_i2s_dma_rx = &msp0_dma_rx,
+	.msp_i2s_dma_tx = &msp0_dma_tx,
+};
+
+static struct stedma40_chan_cfg msp1_dma_rx = {
+	.high_priority = true,
+	.dir = STEDMA40_PERIPH_TO_MEM,
+
+	.src_dev_type = DB8500_DMA_DEV30_MSP3_RX,
+	.dst_dev_type = STEDMA40_DEV_DST_MEMORY,
+
+	.src_info.psize = STEDMA40_PSIZE_LOG_4,
+	.dst_info.psize = STEDMA40_PSIZE_LOG_4,
+
+	/* data_width is set during configuration */
+};
+
+static struct stedma40_chan_cfg msp1_dma_tx = {
+	.high_priority = true,
+	.dir = STEDMA40_MEM_TO_PERIPH,
+
+	.src_dev_type = STEDMA40_DEV_DST_MEMORY,
+	.dst_dev_type = DB8500_DMA_DEV30_MSP1_TX,
+
+	.src_info.psize = STEDMA40_PSIZE_LOG_4,
+	.dst_info.psize = STEDMA40_PSIZE_LOG_4,
+
+	/* data_width is set during configuration */
+};
+
+static struct msp_i2s_platform_data msp1_platform_data = {
+	.id = MSP_I2S_1,
+	.msp_i2s_dma_rx = NULL,
+	.msp_i2s_dma_tx = &msp1_dma_tx,
+	.msp_i2s_init = msp13_i2s_init,
+	.msp_i2s_exit = msp13_i2s_exit,
+};
+
+static struct stedma40_chan_cfg msp2_dma_rx = {
+	.high_priority = true,
+	.dir = STEDMA40_PERIPH_TO_MEM,
+
+	.src_dev_type = DB8500_DMA_DEV14_MSP2_RX,
+	.dst_dev_type = STEDMA40_DEV_DST_MEMORY,
+
+	/* MSP2 DMA doesn't work with PSIZE == 4 on DB8500v2 */
+	.src_info.psize = STEDMA40_PSIZE_LOG_1,
+	.dst_info.psize = STEDMA40_PSIZE_LOG_1,
+
+	/* data_width is set during configuration */
+};
+
+static struct stedma40_chan_cfg msp2_dma_tx = {
+	.high_priority = true,
+	.dir = STEDMA40_MEM_TO_PERIPH,
+
+	.src_dev_type = STEDMA40_DEV_DST_MEMORY,
+	.dst_dev_type = DB8500_DMA_DEV14_MSP2_TX,
+
+	.src_info.psize = STEDMA40_PSIZE_LOG_4,
+	.dst_info.psize = STEDMA40_PSIZE_LOG_4,
+
+	.use_fixed_channel = true,
+	.phy_channel = 1,
+
+	/* data_width is set during configuration */
+};
+
+static struct msp_i2s_platform_data msp2_platform_data = {
+	.id = MSP_I2S_2,
+	.msp_i2s_dma_rx = &msp2_dma_rx,
+	.msp_i2s_dma_tx = &msp2_dma_tx,
+};
+
+static struct msp_i2s_platform_data msp3_platform_data = {
+	.id		= MSP_I2S_3,
+	.msp_i2s_dma_rx	= &msp1_dma_rx,
+	.msp_i2s_dma_tx	= NULL,
+	.msp_i2s_init = msp13_i2s_init,
+	.msp_i2s_exit = msp13_i2s_exit,
+};
+
+void __init mop500_msp_init(void)
+{
+	pr_info("Initialize MSP I2S-devices.\n");
+	db8500_add_msp0_i2s(&msp0_platform_data);
+	db8500_add_msp1_i2s(&msp1_platform_data);
+	db8500_add_msp2_i2s(&msp2_platform_data);
+	db8500_add_msp3_i2s(&msp3_platform_data);
+}
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 2df23ed..4b017a1 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -604,7 +604,6 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
 static void __init mop500_init_machine(void)
 {
 	int i2c0_devs;
-
 	mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
 	u8500_init_devices();
@@ -613,12 +612,11 @@ static void __init mop500_init_machine(void)
 
 	platform_add_devices(mop500_platform_devs,
 			ARRAY_SIZE(mop500_platform_devs));
-
 	mop500_i2c_init();
 	mop500_sdi_init();
+	mop500_msp_init();
 	mop500_spi_init();
 	mop500_uart_init();
-
 	i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 
 	i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
@@ -642,6 +640,7 @@ static void __init snowball_init_machine(void)
 
 	mop500_i2c_init();
 	snowball_sdi_init();
+	mop500_msp_init();
 	mop500_spi_init();
 	mop500_uart_init();
 
@@ -657,7 +656,6 @@ static void __init snowball_init_machine(void)
 static void __init hrefv60_init_machine(void)
 {
 	int i2c0_devs;
-
 	/*
 	 * The HREFv60 board removed a GPIO expander and routed
 	 * all these GPIO pins to the internal GPIO controller
@@ -674,6 +672,7 @@ static void __init hrefv60_init_machine(void)
 
 	mop500_i2c_init();
 	hrefv60_sdi_init();
+	mop500_msp_init();
 	mop500_spi_init();
 	mop500_uart_init();
 
diff --git a/arch/arm/mach-ux500/board-mop500.h b/arch/arm/mach-ux500/board-mop500.h
index f926d3d..b377bbe 100644
--- a/arch/arm/mach-ux500/board-mop500.h
+++ b/arch/arm/mach-ux500/board-mop500.h
@@ -81,6 +81,7 @@ extern void hrefv60_sdi_init(void);
 extern void mop500_sdi_tc35892_init(void);
 void __init mop500_u8500uib_init(void);
 void __init mop500_stuib_init(void);
+void __init mop500_msp_init(void);
 void __init mop500_pins_init(void);
 void __init hrefv60_pins_init(void);
 void __init snowball_pins_init(void);
diff --git a/arch/arm/mach-ux500/clock.c b/arch/arm/mach-ux500/clock.c
index 7379075..8e93f1a 100644
--- a/arch/arm/mach-ux500/clock.c
+++ b/arch/arm/mach-ux500/clock.c
@@ -329,6 +329,7 @@ static DEFINE_PRCMU_CLK(uiccclk,	0x4, 1, UICCCLK); /* v1 */
  */
 
 /* Peripheral Cluster #1 */
+static DEFINE_PRCC_CLK(1, msp3,		11, 10, &clk_msp1clk);
 static DEFINE_PRCC_CLK(1, i2c4,		10, 9, &clk_i2cclk);
 static DEFINE_PRCC_CLK(1, gpio0,	9, -1, NULL);
 static DEFINE_PRCC_CLK(1, slimbus0,	8,  8, &clk_slimclk);
@@ -398,7 +399,7 @@ static struct clk_lookup u8500_clks[] = {
 	CLK(slimbus0,	"slimbus0",	NULL),
 	CLK(i2c2,	"nmk-i2c.2",	NULL),
 	CLK(sdi0,	"sdi0",		NULL),
-	CLK(msp0,	"msp0",		NULL),
+	CLK(msp0,	"ux500-msp-i2s.0",	NULL),
 	CLK(i2c1,	"nmk-i2c.1",	NULL),
 	CLK(uart1,	"uart1",	NULL),
 	CLK(uart0,	"uart0",	NULL),
@@ -448,7 +449,8 @@ static struct clk_lookup u8500_clks[] = {
 	/* Peripheral Cluster #1 */
 	CLK(i2c4,	"nmk-i2c.4",	NULL),
 	CLK(spi3,	"spi3",		NULL),
-	CLK(msp1,	"msp1",		NULL),
+	CLK(msp1,	"ux500-msp-i2s.1",	NULL),
+	CLK(msp3,	"ux500-msp-i2s.3",	NULL),
 
 	/* Peripheral Cluster #2 */
 	CLK(gpio1,	"gpio.6",	NULL),
@@ -458,7 +460,7 @@ static struct clk_lookup u8500_clks[] = {
 	CLK(spi0,	"spi0",		NULL),
 	CLK(sdi3,	"sdi3",		NULL),
 	CLK(sdi1,	"sdi1",		NULL),
-	CLK(msp2,	"msp2",		NULL),
+	CLK(msp2,	"ux500-msp-i2s.2",	NULL),
 	CLK(sdi4,	"sdi4",		NULL),
 	CLK(pwl,	"pwl",		NULL),
 	CLK(spi1,	"spi1",		NULL),
diff --git a/arch/arm/mach-ux500/devices-common.h b/arch/arm/mach-ux500/devices-common.h
index 7825705..024d51b 100644
--- a/arch/arm/mach-ux500/devices-common.h
+++ b/arch/arm/mach-ux500/devices-common.h
@@ -69,7 +69,9 @@ static inline struct platform_device *
 dbx500_add_msp_i2s(int id, resource_size_t base, int irq,
 		   struct msp_i2s_platform_data *pdata)
 {
-	return dbx500_add_platform_device_4k1irq("MSP_I2S", id, base, irq,
+	pr_info("Add platform-device 'ux500-msp-i2s', id %d, irq %d\n",
+		id, irq);
+	return dbx500_add_platform_device_4k1irq("ux500-msp-i2s", id, base, irq,
 						 pdata);
 }
 
diff --git a/arch/arm/mach-ux500/include/mach/msp.h b/arch/arm/mach-ux500/include/mach/msp.h
new file mode 100644
index 0000000..798be19
--- /dev/null
+++ b/arch/arm/mach-ux500/include/mach/msp.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
+ * License terms: GNU General Public License (GPL), version 2.
+ */
+
+#ifndef __MSP_H
+#define __MSP_H
+
+#include <plat/ste_dma40.h>
+
+enum msp_i2s_id {
+	MSP_I2S_0 = 0,
+	MSP_I2S_1,
+	MSP_I2S_2,
+	MSP_I2S_3,
+};
+
+/* Platform data structure for a MSP I2S-device */
+struct msp_i2s_platform_data {
+	enum msp_i2s_id id;
+	struct stedma40_chan_cfg *msp_i2s_dma_rx;
+	struct stedma40_chan_cfg *msp_i2s_dma_tx;
+	int (*msp_i2s_init) (void);
+	int (*msp_i2s_exit) (void);
+};
+
+#endif
-- 
1.7.8.3

Re: [PATCH 10/16] arm: ux500: Add support for MSP I2S-devices

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 376 bytes --]

On Tue, Mar 13, 2012 at 04:11:37PM +0100, Ola Lilja wrote:

>  arch/arm/mach-ux500/board-mop500-msp.c |  194 ++++++++++++++++++++++++++++++++
>  arch/arm/mach-ux500/board-mop500.c     |    7 +-
>  arch/arm/mach-ux500/board-mop500.h     |    1 +

Perhaps this is somehow idiomatic for the port but why is this in what
looks like a board specific rather than SoC specific file?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 10/16] arm: ux500: Add support for MSP I2S-devices

[Linus Walleij]

On Tue, Mar 13, 2012 at 10:40 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Mar 13, 2012 at 04:11:37PM +0100, Ola Lilja wrote:
>
>>  arch/arm/mach-ux500/board-mop500-msp.c |  194 ++++++++++++++++++++++++++++++++
>>  arch/arm/mach-ux500/board-mop500.c     |    7 +-
>>  arch/arm/mach-ux500/board-mop500.h     |    1 +
>
> Perhaps this is somehow idiomatic for the port but why is this in what
> looks like a board specific rather than SoC specific file?

Basically because it includes GPIO's which are board-specific.

But with some cleverness I guess it'd be possible to split just
the board-dependent stuff (i.e. GPIO) to board-mop500-msp.c
and all SoC-specific stuff into say cpu-db8500-msp.c or
even cpu-dbx500-msp.c if it's generic across all families
of DBx500.

FOr short term, since the midterm plan is to use device tree
for the whole shebang (I guess).

Yours,
Linus Walleij

Re: [PATCH 10/16] arm: ux500: Add support for MSP I2S-devices

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 989 bytes --]

On Wed, Mar 14, 2012 at 10:39:03AM +0100, Linus Walleij wrote:
> On Tue, Mar 13, 2012 at 10:40 PM, Mark Brown
> >>  arch/arm/mach-ux500/board-mop500-msp.c |  194 ++++++++++++++++++++++++++++++++
> >>  arch/arm/mach-ux500/board-mop500.c     |    7 +-
> >>  arch/arm/mach-ux500/board-mop500.h     |    1 +

> > Perhaps this is somehow idiomatic for the port but why is this in what
> > looks like a board specific rather than SoC specific file?

> Basically because it includes GPIO's which are board-specific.

> But with some cleverness I guess it'd be possible to split just
> the board-dependent stuff (i.e. GPIO) to board-mop500-msp.c
> and all SoC-specific stuff into say cpu-db8500-msp.c or
> even cpu-dbx500-msp.c if it's generic across all families
> of DBx500.

It doesn't even need to be particularly clever - the SoCs that need it
are all succesfully splitting the pin configuration from the basic
device registration so there's plenty of patterns to follow.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 11/16] ARM: ux500: Add placeholder for clk_set_parent

[Ola Lilja]

Calling clk_set_parent (e.g. from Ux500 ASoC-driver) generates
boot-errors.

Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>
---
 arch/arm/mach-ux500/clock.c |    7 +++++++
 arch/arm/mach-ux500/clock.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/clock.c b/arch/arm/mach-ux500/clock.c
index 8e93f1a..700042c 100644
--- a/arch/arm/mach-ux500/clock.c
+++ b/arch/arm/mach-ux500/clock.c
@@ -223,6 +223,13 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 }
 EXPORT_SYMBOL(clk_set_rate);
 
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	/*TODO*/
+	return -ENOSYS;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
 static void clk_prcmu_enable(struct clk *clk)
 {
 	void __iomem *cg_set_reg = __io_address(U8500_PRCMU_BASE)
diff --git a/arch/arm/mach-ux500/clock.h b/arch/arm/mach-ux500/clock.h
index 0744907..d776ada 100644
--- a/arch/arm/mach-ux500/clock.h
+++ b/arch/arm/mach-ux500/clock.h
@@ -21,6 +21,7 @@ struct clkops {
 	void (*enable) (struct clk *);
 	void (*disable) (struct clk *);
 	unsigned long (*get_rate) (struct clk *);
+	int (*set_parent)(struct clk *, struct clk *);
 };
 
 /**
-- 
1.7.8.3

Re: [PATCH 11/16] ARM: ux500: Add placeholder for clk_set_parent

[Linus Walleij]

On Tue, Mar 13, 2012 at 4:11 PM, Ola Lilja <ola.o.lilja@stericsson.com> wrote:

> Calling clk_set_parent (e.g. from Ux500 ASoC-driver) generates
> boot-errors.
>
> Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>

Also queued for 3.4.

Thanks,
Linus Walleij

[PATCH 14/16] ASoC: Ux500: Add platform-driver

[Ola Lilja]

Add platform-driver handling all DMA-activities.

Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>
---
 sound/soc/ux500/Kconfig     |    7 +
 sound/soc/ux500/Makefile    |    4 +
 sound/soc/ux500/ux500_pcm.c |  537 +++++++++++++++++++++++++++++++++++++++++++
 sound/soc/ux500/ux500_pcm.h |   52 ++++
 4 files changed, 600 insertions(+), 0 deletions(-)
 create mode 100644 sound/soc/ux500/ux500_pcm.c
 create mode 100644 sound/soc/ux500/ux500_pcm.h

diff --git a/sound/soc/ux500/Kconfig b/sound/soc/ux500/Kconfig
index 532429b..6c0271c 100644
--- a/sound/soc/ux500/Kconfig
+++ b/sound/soc/ux500/Kconfig
@@ -16,6 +16,13 @@ config SND_SOC_UX500_MSP_I2S
 	help
 		Say Y if you want to enable the Ux500 MSP I2S-driver.
 
+config SND_SOC_UX500_PLATFORM
+	bool "Platform - DB8500 (DMA)"
+	depends on SND_SOC_UX500
+	default n
+	help
+		Say Y if you want to enable the Ux500 platform-driver.
+
 config SND_SOC_UX500_DEBUG
 	bool "Activate Ux500 platform debug-mode (pr_debug)"
 	depends on SND_SOC_UX500
diff --git a/sound/soc/ux500/Makefile b/sound/soc/ux500/Makefile
index 9666b3a..72e806e 100644
--- a/sound/soc/ux500/Makefile
+++ b/sound/soc/ux500/Makefile
@@ -10,4 +10,8 @@ snd-soc-ux500-platform-i2s-objs := ux500_msp_dai.o ux500_msp_i2s.o
 obj-$(CONFIG_SND_SOC_UX500_MSP_I2S) += snd-soc-ux500-platform-i2s.o
 endif
 
+ifdef CONFIG_SND_SOC_UX500_PLATFORM
+snd-soc-ux500-platform-dma-objs := ux500_pcm.o
+obj-$(CONFIG_SND_SOC_UX500_PLATFORM) += snd-soc-ux500-platform-dma.o
+endif
 
diff --git a/sound/soc/ux500/ux500_pcm.c b/sound/soc/ux500/ux500_pcm.c
new file mode 100644
index 0000000..dee8957
--- /dev/null
+++ b/sound/soc/ux500/ux500_pcm.c
@@ -0,0 +1,537 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * Author: Ola Lilja <ola.o.lilja@stericsson.com>,
+ *         Roger Nilsson <roger.xr.nilsson@stericsson.com>
+ *         for ST-Ericsson.
+ *
+ * License terms:
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <asm/page.h>
+
+#include <linux/module.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#include <plat/ste_dma40.h>
+
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "ux500_pcm.h"
+
+static struct snd_pcm_hardware ux500_pcm_hw_playback = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_RESUME |
+		SNDRV_PCM_INFO_PAUSE,
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |
+		SNDRV_PCM_FMTBIT_U16_LE |
+		SNDRV_PCM_FMTBIT_S16_BE |
+		SNDRV_PCM_FMTBIT_U16_BE,
+	.rates = SNDRV_PCM_RATE_KNOT,
+	.rate_min = UX500_PLATFORM_MIN_RATE_PLAYBACK,
+	.rate_max = UX500_PLATFORM_MAX_RATE_PLAYBACK,
+	.channels_min = UX500_PLATFORM_MIN_CHANNELS,
+	.channels_max = UX500_PLATFORM_MAX_CHANNELS,
+	.buffer_bytes_max = UX500_PLATFORM_BUFFER_BYTES_MAX,
+	.period_bytes_min = UX500_PLATFORM_PERIODS_BYTES_MIN,
+	.period_bytes_max = UX500_PLATFORM_PERIODS_BYTES_MAX,
+	.periods_min = UX500_PLATFORM_PERIODS_MIN,
+	.periods_max = UX500_PLATFORM_PERIODS_MAX,
+};
+
+static struct snd_pcm_hardware ux500_pcm_hw_capture = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_RESUME |
+		SNDRV_PCM_INFO_PAUSE,
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |
+		SNDRV_PCM_FMTBIT_U16_LE |
+		SNDRV_PCM_FMTBIT_S16_BE |
+		SNDRV_PCM_FMTBIT_U16_BE,
+	.rates = SNDRV_PCM_RATE_KNOT,
+	.rate_min = UX500_PLATFORM_MIN_RATE_CAPTURE,
+	.rate_max = UX500_PLATFORM_MAX_RATE_CAPTURE,
+	.channels_min = UX500_PLATFORM_MIN_CHANNELS,
+	.channels_max = UX500_PLATFORM_MAX_CHANNELS,
+	.buffer_bytes_max = UX500_PLATFORM_BUFFER_BYTES_MAX,
+	.period_bytes_min = UX500_PLATFORM_PERIODS_BYTES_MIN,
+	.period_bytes_max = UX500_PLATFORM_PERIODS_BYTES_MAX,
+	.periods_min = UX500_PLATFORM_PERIODS_MIN,
+	.periods_max = UX500_PLATFORM_PERIODS_MAX,
+};
+
+static const char *stream_str(struct snd_pcm_substream *substream)
+{
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		return "Playback";
+	else
+		return "Capture";
+}
+
+static void ux500_pcm_dma_eot_handler(void *data)
+{
+	struct snd_pcm_substream *substream = data;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *dai = rtd->cpu_dai;
+
+	dev_dbg(rtd->platform->dev, "%s: MSP %d (%s): Enter.\n", __func__, dai->id,
+		stream_str(substream));
+
+	if (substream) {
+		struct snd_pcm_runtime *runtime = substream->runtime;
+		struct ux500_pcm *ux500_pcm_data = substream->runtime->private_data;
+
+		/* calc the offset in the circular buffer */
+		ux500_pcm_data->offset +=
+			frames_to_bytes(runtime, runtime->period_size);
+		ux500_pcm_data->offset %=
+			frames_to_bytes(runtime, runtime->period_size) *
+			runtime->periods;
+
+		snd_pcm_period_elapsed(substream);
+	}
+}
+
+static int ux500_pcm_dma_start(struct snd_pcm_substream *substream, dma_addr_t dma_addr,
+			int period_cnt, size_t period_len, int dai_idx, int stream_id)
+{
+	dma_cookie_t status_submit;
+	int direction;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct ux500_pcm *ux500_pcm_data = runtime->private_data;
+	struct dma_device *dma_dev = ux500_pcm_data->pipeid->device;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *dai = rtd->cpu_dai;
+	struct dma_async_tx_descriptor *cdesc;
+	struct ux500_pcm_dma_params *dma_params;
+
+	dev_dbg(rtd->platform->dev, "%s: Enter\n", __func__);
+
+	dma_params = snd_soc_dai_get_dma_data(dai, substream);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		direction = DMA_TO_DEVICE;
+	else
+		direction = DMA_FROM_DEVICE;
+
+	cdesc = dma_dev->device_prep_dma_cyclic(ux500_pcm_data->pipeid, dma_addr,
+						period_cnt * period_len, period_len,
+						direction);
+
+	if (IS_ERR(cdesc)) {
+		dev_err(rtd->platform->dev,
+			"%s: ERROR: device_prep_dma_cyclic failed (%ld)!\n", __func__,
+			PTR_ERR(cdesc));
+		return -EINVAL;
+	}
+
+	cdesc->callback = ux500_pcm_dma_eot_handler;
+	cdesc->callback_param = substream;
+
+	status_submit = dmaengine_submit(cdesc);
+
+	if (dma_submit_error(status_submit)) {
+		dev_err(rtd->platform->dev, "%s: ERROR: dmaengine_submit failed!\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	dma_async_issue_pending(ux500_pcm_data->pipeid);
+
+	return 0;
+}
+
+static void ux500_pcm_dma_stop(struct work_struct *work)
+{
+	struct ux500_pcm *ux500_pcm_data = container_of(work, struct ux500_pcm, ws_stop);
+
+	if (ux500_pcm_data->pipeid != NULL) {
+		dmaengine_terminate_all(ux500_pcm_data->pipeid);
+		dma_release_channel(ux500_pcm_data->pipeid);
+		ux500_pcm_data->pipeid = NULL;
+	}
+}
+
+static void ux500_pcm_dma_hw_free(struct device *dev,
+				struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_dma_buffer *buf = runtime->dma_buffer_p;
+
+	if (runtime->dma_area == NULL)
+		return;
+
+	if (buf != &substream->dma_buffer) {
+		dma_free_coherent(buf->dev.dev, buf->bytes, buf->area, buf->addr);
+		kfree(runtime->dma_buffer_p);
+	}
+
+	snd_pcm_set_runtime_buffer(substream, NULL);
+}
+
+static int ux500_pcm_open(struct snd_pcm_substream *substream)
+{
+	int stream_id = substream->pstr->stream;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct ux500_pcm *ux500_pcm_data;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *dai = rtd->cpu_dai;
+	int ret;
+
+	dev_dbg(dai->dev, "%s: MSP %d (%s): Enter.\n", __func__, dai->id, stream_str(substream));
+
+	dev_dbg(dai->dev, "%s: Set runtime hwparams.\n", __func__);
+	if (stream_id == SNDRV_PCM_STREAM_PLAYBACK)
+		snd_soc_set_runtime_hwparams(substream, &ux500_pcm_hw_playback);
+	else
+		snd_soc_set_runtime_hwparams(substream, &ux500_pcm_hw_capture);
+
+	/* ensure that buffer size is a multiple of period size */
+	ret = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
+	if (ret < 0) {
+		dev_err(dai->dev, "%s: Error: snd_pcm_hw_constraints failed (%d)\n",
+			__func__, ret);
+		return ret;
+	}
+
+	dev_dbg(dai->dev, "%s: Init runtime private data.\n", __func__);
+	ux500_pcm_data = kzalloc(sizeof(struct ux500_pcm), GFP_KERNEL);
+	if (ux500_pcm_data == NULL)
+		return -ENOMEM;
+	ux500_pcm_data->msp_id = dai->id;
+	ux500_pcm_data->wq = alloc_ordered_workqueue("ux500/pcm", 0);
+	INIT_WORK(&ux500_pcm_data->ws_stop, ux500_pcm_dma_stop);
+
+	runtime->private_data = ux500_pcm_data;
+
+	dev_dbg(dai->dev, "%s: Set hw-struct for %s.\n", __func__, stream_str(substream));
+	runtime->hw = (stream_id == SNDRV_PCM_STREAM_PLAYBACK) ?
+		ux500_pcm_hw_playback : ux500_pcm_hw_capture;
+
+	return 0;
+}
+
+static int ux500_pcm_close(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *dai = rtd->cpu_dai;
+	struct ux500_pcm *ux500_pcm_data = substream->runtime->private_data;
+
+	dev_dbg(dai->dev, "%s: Enter\n", __func__);
+
+	if (ux500_pcm_data->pipeid != NULL) {
+		dma_release_channel(ux500_pcm_data->pipeid);
+		ux500_pcm_data->pipeid = NULL;
+	}
+
+	destroy_workqueue(ux500_pcm_data->wq);
+	kfree(ux500_pcm_data);
+
+	return 0;
+}
+
+static int ux500_pcm_hw_params(struct snd_pcm_substream *substream,
+			struct snd_pcm_hw_params *hw_params)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_dma_buffer *buf = runtime->dma_buffer_p;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	int ret = 0;
+	int size;
+
+	dev_dbg(rtd->platform->dev, "%s: Enter\n", __func__);
+
+	size = params_buffer_bytes(hw_params);
+
+	if (buf) {
+		if (buf->bytes >= size)
+			goto out;
+		ux500_pcm_dma_hw_free(NULL, substream);
+	}
+
+	if (substream->dma_buffer.area != NULL && substream->dma_buffer.bytes >= size) {
+		buf = &substream->dma_buffer;
+	} else {
+		buf = kmalloc(sizeof(struct snd_dma_buffer), GFP_KERNEL);
+		if (!buf)
+			goto nomem;
+
+		buf->dev.type = SNDRV_DMA_TYPE_DEV;
+		buf->dev.dev = NULL;
+		buf->area = dma_alloc_coherent(NULL, size, &buf->addr, GFP_KERNEL);
+		buf->bytes = size;
+		buf->private_data = NULL;
+
+		if (!buf->area)
+			goto free;
+	}
+	snd_pcm_set_runtime_buffer(substream, buf);
+	ret = 1;
+ out:
+	runtime->dma_bytes = size;
+	return ret;
+
+ free:
+	kfree(buf);
+ nomem:
+	return -ENOMEM;
+}
+
+static int ux500_pcm_hw_free(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+
+	dev_dbg(rtd->platform->dev, "%s: Enter\n", __func__);
+
+	ux500_pcm_dma_hw_free(NULL, substream);
+
+	return 0;
+}
+
+static int ux500_pcm_prepare(struct snd_pcm_substream *substream)
+{
+	struct stedma40_chan_cfg *dma_cfg;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *dai = rtd->cpu_dai;
+	struct ux500_pcm *ux500_pcm_data = runtime->private_data;
+	struct ux500_pcm_dma_params *dma_params;
+	dma_cap_mask_t mask;
+	u16 per_data_width, mem_data_width;
+
+	dev_dbg(rtd->platform->dev, "%s: Enter\n", __func__);
+
+	if (ux500_pcm_data->pipeid != NULL) {
+		dma_release_channel(ux500_pcm_data->pipeid);
+		ux500_pcm_data->pipeid = NULL;
+	}
+
+	dma_params = snd_soc_dai_get_dma_data(dai, substream);
+
+	mem_data_width = STEDMA40_HALFWORD_WIDTH;
+
+	switch (dma_params->data_size) {
+	case 32:
+		per_data_width = STEDMA40_WORD_WIDTH;
+		break;
+	case 16:
+		per_data_width = STEDMA40_HALFWORD_WIDTH;
+		break;
+	case 8:
+		per_data_width = STEDMA40_BYTE_WIDTH;
+		break;
+	default:
+		per_data_width = STEDMA40_WORD_WIDTH;
+		dev_warn(rtd->platform->dev, "%s: Unknown data-size (%d)! Assuming 32 bits.\n",
+			__func__, dma_params->data_size);
+	}
+
+	dma_cfg = dma_params->dma_cfg;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		dma_cfg->src_info.data_width = mem_data_width;
+		dma_cfg->dst_info.data_width = per_data_width;
+	} else {
+		dma_cfg->src_info.data_width = per_data_width;
+		dma_cfg->dst_info.data_width = mem_data_width;
+	}
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	ux500_pcm_data->pipeid = dma_request_channel(mask, stedma40_filter, dma_cfg);
+
+	return 0;
+}
+
+static int ux500_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	int ret = 0;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct ux500_pcm *ux500_pcm_data = runtime->private_data;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+
+	dev_dbg(rtd->platform->dev, "%s: Enter\n", __func__);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		dev_dbg(rtd->platform->dev, "%s: START/PAUSE-RELEASE\n", __func__);
+		if (runtime->status->state == SNDRV_PCM_STATE_XRUN) {
+			dev_dbg(rtd->platform->dev, "XRUN occurred\n");
+				return 0;
+		}
+
+		ux500_pcm_data->offset = 0;
+		ret = ux500_pcm_dma_start(
+				substream,
+				runtime->dma_addr,
+				runtime->periods,
+				frames_to_bytes(runtime, runtime->period_size),
+				ux500_pcm_data->msp_id,
+				substream->pstr->stream);
+		if (ret) {
+			dev_err(rtd->platform->dev, "%s: Failed to configure I2S!\n",
+				__func__);
+			return -EINVAL;
+		}
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+		dev_dbg(rtd->platform->dev, "%s: SNDRV_PCM_TRIGGER_STOP\n", __func__);
+		dev_dbg(rtd->platform->dev, "%s: no_of_underruns = %u\n",
+			__func__,
+			ux500_pcm_data->no_of_underruns);
+		break;
+
+	default:
+		dev_err(rtd->platform->dev, "%s: Invalid command in pcm trigger\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static snd_pcm_uframes_t ux500_pcm_pointer(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct ux500_pcm *ux500_pcm_data = runtime->private_data;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+
+	dev_dbg(rtd->platform->dev, "%s: dma_offset %d frame %ld\n",
+		__func__, ux500_pcm_data->offset,
+		bytes_to_frames(substream->runtime, ux500_pcm_data->offset));
+
+	return bytes_to_frames(substream->runtime, ux500_pcm_data->offset);
+}
+
+static int ux500_pcm_mmap(struct snd_pcm_substream *substream,
+			struct vm_area_struct *vma)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+
+	dev_dbg(rtd->platform->dev, "%s: Enter.\n", __func__);
+
+	return dma_mmap_coherent(NULL, vma, runtime->dma_area, runtime->dma_addr,
+				runtime->dma_bytes);
+}
+
+static const struct snd_pcm_ops ux500_pcm_ops = {
+	.open		= ux500_pcm_open,
+	.close		= ux500_pcm_close,
+	.ioctl		= snd_pcm_lib_ioctl,
+	.hw_params	= ux500_pcm_hw_params,
+	.hw_free	= ux500_pcm_hw_free,
+	.prepare	= ux500_pcm_prepare,
+	.trigger	= ux500_pcm_trigger,
+	.pointer	= ux500_pcm_pointer,
+	.mmap		= ux500_pcm_mmap
+};
+
+int ux500_pcm_new(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_pcm *pcm = rtd->pcm;
+
+	dev_dbg(rtd->platform->dev, "%s: Enter.\n", __func__);
+
+	pcm->info_flags = 0;
+	strcpy(pcm->name, "UX500_PCM");
+	dev_dbg(rtd->platform->dev, "%s: PCM-name: %s\n", __func__, pcm->name);
+
+	return 0;
+}
+
+static void ux500_pcm_free(struct snd_pcm *pcm)
+{
+	pr_debug("%s: Enter\n", __func__);
+}
+
+static int ux500_pcm_suspend(struct snd_soc_dai *dai)
+{
+	dev_dbg(dai->platform->dev, "%s: Enter\n", __func__);
+
+	return 0;
+}
+
+static int ux500_pcm_resume(struct snd_soc_dai *dai)
+{
+	dev_dbg(dai->platform->dev, "%s: Enter\n", __func__);
+
+	return 0;
+}
+
+struct snd_soc_platform_driver ux500_pcm_soc_drv = {
+	.ops		= &ux500_pcm_ops,
+	.pcm_new        = ux500_pcm_new,
+	.pcm_free       = ux500_pcm_free,
+	.suspend        = ux500_pcm_suspend,
+	.resume         = ux500_pcm_resume,
+};
+EXPORT_SYMBOL(ux500_pcm_soc_drv);
+
+static int __devexit ux500_pcm_drv_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	dev_info(&pdev->dev, "%s: Register ux500-pcm SoC platform driver.\n", __func__);
+	ret = snd_soc_register_platform(&pdev->dev, &ux500_pcm_soc_drv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "%s: Error: Failed to register "
+			"ux500-pcm SoC platform driver (%d)!\n",
+			__func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devinit ux500_pcm_drv_remove(struct platform_device *pdev)
+{
+	dev_info(&pdev->dev, "%s: Unregister ux500-pcm SoC platform driver.\n", __func__);
+	snd_soc_unregister_platform(&pdev->dev);
+
+	return 0;
+}
+
+static struct platform_driver ux500_pcm_driver = {
+	.driver = {
+		.name = "ux500-pcm",
+		.owner = THIS_MODULE,
+	},
+
+	.probe = ux500_pcm_drv_probe,
+	.remove = __devexit_p(ux500_pcm_drv_remove),
+};
+
+static int __init ux500_pcm_drv_init(void)
+{
+	pr_debug("%s: Register ux500-pcm platform driver.\n", __func__);
+
+	return platform_driver_register(&ux500_pcm_driver);
+}
+
+static void __exit ux500_pcm_drv_exit(void)
+{
+	pr_debug("%s: Unregister ux500-pcm platform driver.\n", __func__);
+
+	platform_driver_unregister(&ux500_pcm_driver);
+}
+
+module_init(ux500_pcm_drv_init);
+module_exit(ux500_pcm_drv_exit);
+
diff --git a/sound/soc/ux500/ux500_pcm.h b/sound/soc/ux500/ux500_pcm.h
new file mode 100644
index 0000000..6cd78eb
--- /dev/null
+++ b/sound/soc/ux500/ux500_pcm.h
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2012
+ *
+ * Author: Ola Lilja <ola.o.lilja@stericsson.com>,
+ *         Roger Nilsson <roger.xr.nilsson@stericsson.com>
+ *         for ST-Ericsson.
+ *
+ * License terms:
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+#ifndef UX500_PCM_H
+#define UX500_PCM_H
+
+#include <asm/page.h>
+
+#include <linux/workqueue.h>
+
+#define UX500_PLATFORM_MIN_RATE_PLAYBACK 8000
+#define UX500_PLATFORM_MAX_RATE_PLAYBACK 48000
+#define UX500_PLATFORM_MIN_RATE_CAPTURE	8000
+#define UX500_PLATFORM_MAX_RATE_CAPTURE	48000
+
+#define UX500_PLATFORM_MIN_CHANNELS 1
+#define UX500_PLATFORM_MAX_CHANNELS 8
+
+#define UX500_PLATFORM_PERIODS_BYTES_MIN	128
+#define UX500_PLATFORM_PERIODS_BYTES_MAX	(64 * PAGE_SIZE)
+#define UX500_PLATFORM_PERIODS_MIN		2
+#define UX500_PLATFORM_PERIODS_MAX		48
+#define UX500_PLATFORM_BUFFER_BYTES_MAX		(2048 * PAGE_SIZE)
+
+extern struct snd_soc_platform ux500_soc_platform;
+
+struct ux500_pcm {
+	struct dma_chan *pipeid;
+	struct workqueue_struct *wq;
+	struct work_struct ws_stop;
+	int msp_id;
+	int stream_id;
+	unsigned int offset;
+	unsigned int no_of_underruns;
+};
+
+struct ux500_pcm_dma_params {
+	unsigned int data_size;
+	struct stedma40_chan_cfg *dma_cfg;
+};
+
+#endif
-- 
1.7.8.3

Re: [PATCH 14/16] ASoC: Ux500: Add platform-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 707 bytes --]

On Tue, Mar 13, 2012 at 04:11:41PM +0100, Ola Lilja wrote:

> +static const char *stream_str(struct snd_pcm_substream *substream)
> +{
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		return "Playback";
> +	else
> +		return "Capture";
> +}

You know how I was mentioning that this isn't at all driver specific...

> +	cdesc = dma_dev->device_prep_dma_cyclic(ux500_pcm_data->pipeid, dma_addr,
> +						period_cnt * period_len, period_len,
> +						direction);

You're using dmaengine here it seems.  Please refactor to use the
dmaengine helper library that was recently contributed by Lars-Peter -
it should save a bunch of code and make rolling out dmaengine framework
improvements much easier.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 14/16] ASoC: Ux500: Add platform-driver

[Linus Walleij]

On Tue, Mar 13, 2012 at 11:48 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

>> +     cdesc = dma_dev->device_prep_dma_cyclic(ux500_pcm_data->pipeid, dma_addr,
>> +                                             period_cnt * period_len, period_len,
>> +                                             direction);
>
> You're using dmaengine here it seems.  Please refactor to use the
> dmaengine helper library that was recently contributed by Lars-Peter -
> it should save a bunch of code and make rolling out dmaengine framework
> improvements much easier.

That stuff is not even in linux-next so I don't know it it will even be
in v3.4. But let's see.

Yours,
Linus Walleij

Re: [PATCH 14/16] ASoC: Ux500: Add platform-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 788 bytes --]

On Wed, Mar 14, 2012 at 11:50:28AM +0100, Linus Walleij wrote:
> On Tue, Mar 13, 2012 at 11:48 PM, Mark Brown

> >> +     cdesc = dma_dev->device_prep_dma_cyclic(ux500_pcm_data->pipeid, dma_addr,
> >> +                                             period_cnt * period_len, period_len,
> >> +                                             direction);

> > You're using dmaengine here it seems.  Please refactor to use the
> > dmaengine helper library that was recently contributed by Lars-Peter -
> > it should save a bunch of code and make rolling out dmaengine framework
> > improvements much easier.

> That stuff is not even in linux-next so I don't know it it will even be
> in v3.4. But let's see.

You're not looking at a recent -next, it's been there for a while now.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 15/16] ASoC: Ux500: Activate the Ux500 ASoC-driver

[Ola Lilja]

This patch activates the Ux500 ASoC-driver in the
ASoC Kconfig/Makefile.

Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>
---
 sound/soc/Kconfig  |    1 +
 sound/soc/Makefile |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 35e662d..10c66dc 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -45,6 +45,7 @@ source "sound/soc/s6000/Kconfig"
 source "sound/soc/sh/Kconfig"
 source "sound/soc/tegra/Kconfig"
 source "sound/soc/txx9/Kconfig"
+source "sound/soc/ux500/Kconfig"
 
 # Supported codecs
 source "sound/soc/codecs/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 9ea8ac8..df87058 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -1,6 +1,8 @@
 snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o
 snd-soc-core-objs += soc-pcm.o soc-io.o
 
+CFLAGS_soc-core.o := -DDEBUG
+
 obj-$(CONFIG_SND_SOC)	+= snd-soc-core.o
 obj-$(CONFIG_SND_SOC)	+= codecs/
 obj-$(CONFIG_SND_SOC)	+= atmel/
@@ -22,3 +24,4 @@ obj-$(CONFIG_SND_SOC)	+= s6000/
 obj-$(CONFIG_SND_SOC)	+= sh/
 obj-$(CONFIG_SND_SOC)	+= tegra/
 obj-$(CONFIG_SND_SOC)	+= txx9/
+obj-$(CONFIG_SND_SOC)	+= ux500/
-- 
1.7.8.3

[PATCH 16/16] ASoC: Ux500: Add machine-driver

[Ola Lilja]

Add machine-driver for ST-Ericsson U8500 platform, including
support for the AB8500-codec.

Signed-off-by: Ola Lilja <ola.o.lilja@stericsson.com>
---
 sound/soc/ux500/Kconfig        |    8 +
 sound/soc/ux500/Makefile       |   13 +
 sound/soc/ux500/u8500.c        |  150 ++++++++
 sound/soc/ux500/ux500_ab8500.c |  828 ++++++++++++++++++++++++++++++++++++++++
 sound/soc/ux500/ux500_ab8500.h |   25 ++
 5 files changed, 1024 insertions(+), 0 deletions(-)
 create mode 100644 sound/soc/ux500/u8500.c
 create mode 100644 sound/soc/ux500/ux500_ab8500.c
 create mode 100644 sound/soc/ux500/ux500_ab8500.h

diff --git a/sound/soc/ux500/Kconfig b/sound/soc/ux500/Kconfig
index 6c0271c..f651564 100644
--- a/sound/soc/ux500/Kconfig
+++ b/sound/soc/ux500/Kconfig
@@ -23,6 +23,14 @@ config SND_SOC_UX500_PLATFORM
 	help
 		Say Y if you want to enable the Ux500 platform-driver.
 
+config SND_SOC_UX500_AB8500
+	bool "Codec/Machine - AB8500 (MSA)"
+	depends on SND_SOC_UX500_MSP_I2S && SND_SOC_UX500_PLATFORM && AB8500_CORE && AB8500_GPADC
+	select SND_SOC_AB8500
+	default n
+	help
+	   Select this to enable the Ux500 machine-driver and the AB8500 codec-driver.
+
 config SND_SOC_UX500_DEBUG
 	bool "Activate Ux500 platform debug-mode (pr_debug)"
 	depends on SND_SOC_UX500
diff --git a/sound/soc/ux500/Makefile b/sound/soc/ux500/Makefile
index 72e806e..7c1cce6 100644
--- a/sound/soc/ux500/Makefile
+++ b/sound/soc/ux500/Makefile
@@ -3,6 +3,9 @@
 ifdef CONFIG_SND_SOC_UX500_DEBUG
 CFLAGS_ux500_msp_dai.o := -DDEBUG
 CFLAGS_ux500_msp_i2s.o := -DDEBUG
+CFLAGS_ux500_pcm.o := -DDEBUG
+CFLAGS_u8500.o := -DDEBUG
+CFLAGS_ux500_ab8500.o := -DDEBUG
 endif
 
 ifdef CONFIG_SND_SOC_UX500_MSP_I2S
@@ -15,3 +18,13 @@ snd-soc-ux500-platform-dma-objs := ux500_pcm.o
 obj-$(CONFIG_SND_SOC_UX500_PLATFORM) += snd-soc-ux500-platform-dma.o
 endif
 
+ifdef CONFIG_SND_SOC_UX500_AB8500
+snd-soc-ux500-machine-objs += ux500_ab8500.o
+endif
+
+obj-y += snd-soc-ux500-machine.o
+
+ifdef CONFIG_SND_SOC_UX500_PLATFORM
+snd-soc-u8500-objs := u8500.o
+obj-$(CONFIG_SND_SOC_UX500_PLATFORM) += snd-soc-u8500.o
+endif
diff --git a/sound/soc/ux500/u8500.c b/sound/soc/ux500/u8500.c
new file mode 100644
index 0000000..5092698
--- /dev/null
+++ b/sound/soc/ux500/u8500.c
@@ -0,0 +1,150 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2012
+ *
+ * Author: Ola Lilja (ola.o.lilja@stericsson.com)
+ *         for ST-Ericsson.
+ *
+ * License terms:
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <asm/mach-types.h>
+
+#include <linux/io.h>
+#include <linux/spi/spi.h>
+
+#include <sound/soc.h>
+#include <sound/initval.h>
+
+#include "ux500_pcm.h"
+#include "ux500_msp_dai.h"
+
+#ifdef CONFIG_SND_SOC_UX500_AB8500
+#include <ux500_ab8500.h>
+#endif
+
+static struct platform_device *pdev;
+
+/* Create dummy devices for platform drivers */
+
+static struct platform_device ux500_pcm = {
+		.name = "ux500-pcm",
+		.id = 0,
+		.dev = {
+			.platform_data = NULL,
+		},
+};
+
+/* Define the whole U8500 soundcard, linking platform to the codec-drivers  */
+struct snd_soc_dai_link u8500_dai_links[] = {
+	#ifdef CONFIG_SND_SOC_UX500_AB8500
+	{
+	.name = "ab8500_0",
+	.stream_name = "ab8500_0",
+	.cpu_dai_name = "ux500-msp-i2s.1",
+	.codec_dai_name = "ab8500-codec-dai.0",
+	.platform_name = "ux500-pcm.0",
+	.codec_name = "ab8500-codec.0",
+	.init = ux500_ab8500_machine_init,
+	.ops = ux500_ab8500_ops,
+	},
+	{
+	.name = "ab8500_1",
+	.stream_name = "ab8500_1",
+	.cpu_dai_name = "ux500-msp-i2s.3",
+	.codec_dai_name = "ab8500-codec-dai.1",
+	.platform_name = "ux500-pcm.0",
+	.codec_name = "ab8500-codec.0",
+	.init = NULL,
+	.ops = ux500_ab8500_ops,
+	},
+	#endif
+};
+
+static struct snd_soc_card u8500_card = {
+	.name = "U8500-card",
+	.probe = NULL,
+	.dai_link = u8500_dai_links,
+	.num_links = ARRAY_SIZE(u8500_dai_links),
+};
+
+static int __init u8500_soc_init(void)
+{
+	int ret;
+
+	pr_debug("%s: Enter.\n", __func__);
+
+	pdev = platform_device_alloc("soc-audio", -1);
+	if (!pdev)
+		return -ENOMEM;
+	dev_dbg(&pdev->dev, "%s: Platform device 'soc-audio' allocated.\n", __func__);
+
+	dev_dbg(&pdev->dev, "%s: Card %s: Set platform drvdata.\n",
+		__func__,
+		u8500_card.name);
+	platform_set_drvdata(pdev, &u8500_card);
+
+	dev_dbg(&pdev->dev, "%s: Card %s: Add platform device.\n",
+		__func__,
+		u8500_card.name);
+	ret = platform_device_add(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: Error: Failed to add platform device (%s).\n",
+			__func__,
+			u8500_card.name);
+		platform_device_put(pdev);
+	}
+
+	u8500_card.dev = &pdev->dev;
+
+	#ifdef CONFIG_SND_SOC_UX500_AB8500
+	dev_dbg(&pdev->dev, "%s: Calling init-function for AB8500 machine driver.\n",
+		__func__);
+	ret = ux500_ab8500_soc_machine_drv_init();
+	if (ret)
+		dev_err(&pdev->dev, "%s: ux500_ab8500_soc_machine_drv_init failed (%d).\n",
+			__func__, ret);
+	#endif
+
+	dev_dbg(&pdev->dev, "%s: Register device to generate a probe for Ux500-pcm platform.\n",
+		__func__);
+	platform_device_register(&ux500_pcm);
+
+	dev_dbg(&pdev->dev, "%s: Card %s: num_links = %d\n",
+		__func__,
+		u8500_card.name,
+		u8500_card.num_links);
+	dev_dbg(&pdev->dev, "%s: Card %s: DAI-link 0: name = %s\n",
+		__func__,
+		u8500_card.name,
+		u8500_card.dai_link[0].name);
+	dev_dbg(&pdev->dev, "%s: Card %s: DAI-link 0: stream_name = %s\n",
+		__func__,
+		u8500_card.name,
+		u8500_card.dai_link[0].stream_name);
+
+	return ret;
+}
+
+static void __exit u8500_soc_exit(void)
+{
+	pr_debug("%s: Enter.\n", __func__);
+
+	#ifdef CONFIG_SND_SOC_UX500_AB8500
+	pr_debug("%s: Calling exit-function for AB8500 machine driver.\n",
+		__func__);
+	ux500_ab8500_soc_machine_drv_cleanup();
+	#endif
+
+	pr_debug("%s: Unregister platform device (%s).\n",
+		__func__,
+		u8500_card.name);
+	platform_device_unregister(pdev);
+}
+
+module_init(u8500_soc_init);
+module_exit(u8500_soc_exit);
+
diff --git a/sound/soc/ux500/ux500_ab8500.c b/sound/soc/ux500/ux500_ab8500.c
new file mode 100644
index 0000000..faa05bc
--- /dev/null
+++ b/sound/soc/ux500/ux500_ab8500.c
@@ -0,0 +1,828 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2012
+ *
+ * Author: Ola Lilja <ola.o.lilja@stericsson.com>,
+ *         Kristoffer Karlsson <kristoffer.karlsson@stericsson.com>
+ *         for ST-Ericsson.
+ *
+ * License terms:
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/regulator/consumer.h>
+
+#include <mach/hardware.h>
+
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+#include <sound/pcm.h>
+#include <sound/jack.h>
+#include <sound/pcm_params.h>
+
+#include "ux500_pcm.h"
+#include "ux500_msp_dai.h"
+#include "../codecs/ab8500_audio.h"
+
+#define TX_SLOT_MONO	0x0008
+#define TX_SLOT_STEREO	0x000a
+#define RX_SLOT_MONO	0x0001
+#define RX_SLOT_STEREO	0x0003
+#define TX_SLOT_8CH	0x00FF
+#define RX_SLOT_8CH	0x00FF
+
+#define DEF_TX_SLOTS	TX_SLOT_STEREO
+#define DEF_RX_SLOTS	RX_SLOT_MONO
+
+#define DRIVERMODE_NORMAL	0
+#define DRIVERMODE_CODEC_ONLY	1
+
+/* Power-control */
+static DEFINE_MUTEX(power_lock);
+static int ab8500_power_count;
+
+/* Clocks */
+/* audioclk -> intclk -> sysclk/ulpclk */
+static int master_clock_sel;
+static struct clk *clk_ptr_audioclk;
+static struct clk *clk_ptr_intclk;
+static struct clk *clk_ptr_sysclk;
+static struct clk *clk_ptr_ulpclk;
+static struct clk *clk_ptr_gpio1;
+
+static const char * const enum_mclk[] = {
+	"SYSCLK",
+	"ULPCLK"
+};
+static SOC_ENUM_SINGLE_EXT_DECL(soc_enum_mclk, enum_mclk);
+
+/* ANC States */
+static const char * const enum_anc_state[] = {
+	"Unconfigured",
+	"Apply FIR and IIR",
+	"FIR and IIR are configured",
+	"Apply FIR",
+	"FIR is configured",
+	"Apply IIR",
+	"IIR is configured"
+};
+static SOC_ENUM_SINGLE_EXT_DECL(soc_enum_ancstate, enum_anc_state);
+enum anc_state {
+	ANC_UNCONFIGURED = 0,
+	ANC_APPLY_FIR_IIR = 1,
+	ANC_FIR_IIR_CONFIGURED = 2,
+	ANC_APPLY_FIR = 3,
+	ANC_FIR_CONFIGURED = 4,
+	ANC_APPLY_IIR = 5,
+	ANC_IIR_CONFIGURED = 6
+};
+static enum anc_state anc_status = ANC_UNCONFIGURED;
+
+/* Regulators */
+enum regulator_idx {
+	REGULATOR_AUDIO,
+	REGULATOR_DMIC,
+	REGULATOR_AMIC1,
+	REGULATOR_AMIC2
+};
+static struct regulator_bulk_data reg_info[4] = {
+	{	.consumer = NULL, .supply = "V-AUD"	},
+	{	.consumer = NULL, .supply = "V-DMIC"	},
+	{	.consumer = NULL, .supply = "V-AMIC1"	},
+	{	.consumer = NULL, .supply = "V-AMIC2"	}
+};
+static bool reg_enabled[4] =  {
+	false,
+	false,
+	false,
+	false
+};
+static int reg_claim[4];
+enum amic_idx { AMIC_1A, AMIC_1B, AMIC_2 };
+struct amic_conf {
+	enum regulator_idx reg_id;
+	bool enabled;
+	char *name;
+};
+static struct amic_conf amic_info[3] = {
+	{ REGULATOR_AMIC1, false, "amic1a" },
+	{ REGULATOR_AMIC1, false, "amic1b" },
+	{ REGULATOR_AMIC2, false, "amic2" }
+};
+
+/* Slot configuration */
+static unsigned int tx_slots = DEF_TX_SLOTS;
+static unsigned int rx_slots = DEF_RX_SLOTS;
+
+/* Regulators */
+
+static int enable_regulator(struct device *dev, enum regulator_idx idx)
+{
+	int ret;
+
+	if (reg_info[idx].consumer == NULL) {
+		dev_err(dev, "%s: Failure to enable regulator '%s'\n",
+			__func__, reg_info[idx].supply);
+		return -EIO;
+	}
+
+	if (reg_enabled[idx])
+		return 0;
+
+	ret = regulator_enable(reg_info[idx].consumer);
+	if (ret != 0) {
+		dev_err(dev, "%s: Failure to enable regulator '%s' (ret = %d)\n",
+			__func__, reg_info[idx].supply, ret);
+		return -EIO;
+	};
+
+	reg_enabled[idx] = true;
+	dev_dbg(dev, "%s: Enabled regulator '%s', status: %d, %d, %d, %d\n",
+		__func__,
+		reg_info[idx].supply,
+		(int)reg_enabled[0],
+		(int)reg_enabled[1],
+		(int)reg_enabled[2],
+		(int)reg_enabled[3]);
+	return 0;
+}
+
+static void disable_regulator(struct device *dev, enum regulator_idx idx)
+{
+	if (reg_info[idx].consumer == NULL) {
+		dev_err(dev, "%s: Failure to disable regulator '%s'\n",
+			__func__, reg_info[idx].supply);
+		return;
+	}
+
+	if (!reg_enabled[idx])
+		return;
+
+	regulator_disable(reg_info[idx].consumer);
+
+	reg_enabled[idx] = false;
+	dev_dbg(dev, "%s: Disabled regulator '%s', status: %d, %d, %d, %d\n",
+		__func__,
+		reg_info[idx].supply,
+		(int)reg_enabled[0],
+		(int)reg_enabled[1],
+		(int)reg_enabled[2],
+		(int)reg_enabled[3]);
+}
+
+static int create_regulators(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_codec *codec = rtd->codec;
+	struct device *dev = rtd->card->dev;
+	int i, status = 0;
+
+	dev_dbg(dev, "%s: Enter.\n", __func__);
+
+	for (i = 0; i < ARRAY_SIZE(reg_info); ++i)
+		reg_info[i].consumer = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(reg_info); ++i) {
+		reg_info[i].consumer = regulator_get(codec->dev, reg_info[i].supply);
+		if (IS_ERR(reg_info[i].consumer)) {
+			status = PTR_ERR(reg_info[i].consumer);
+			dev_err(dev, "%s: ERROR: Failed to get regulator '%s' "
+				"(ret = %d)!\n", __func__, reg_info[i].supply,
+				status);
+			reg_info[i].consumer = NULL;
+			goto err_get;
+		}
+	}
+
+	return 0;
+
+err_get:
+
+	for (i = 0; i < ARRAY_SIZE(reg_info); ++i) {
+		if (reg_info[i].consumer) {
+			regulator_put(reg_info[i].consumer);
+			reg_info[i].consumer = NULL;
+		}
+	}
+
+	return status;
+}
+
+static int claim_amic_regulator(struct device *dev, enum amic_idx amic_id)
+{
+	enum regulator_idx reg_id = amic_info[amic_id].reg_id;
+	int ret = 0;
+
+	reg_claim[reg_id]++;
+	if (reg_claim[reg_id] > 1)
+		goto cleanup;
+
+	ret = enable_regulator(dev, reg_id);
+	if (ret < 0) {
+		dev_err(dev, "%s: Failed to claim %s for %s (ret = %d)!",
+			__func__, reg_info[reg_id].supply,
+			amic_info[amic_id].name, ret);
+		reg_claim[reg_id]--;
+	}
+
+cleanup:
+	amic_info[amic_id].enabled = (ret == 0);
+
+	return ret;
+}
+
+static void release_amic_regulator(struct device *dev, enum amic_idx amic_id)
+{
+	enum regulator_idx reg_id = amic_info[amic_id].reg_id;
+
+	reg_claim[reg_id]--;
+	if (reg_claim[reg_id] <= 0) {
+		disable_regulator(dev, reg_id);
+		reg_claim[reg_id] = 0;
+	}
+
+	amic_info[amic_id].enabled = false;
+}
+
+/* Power/clock control */
+
+static int ux500_ab8500_power_control_inc(struct snd_soc_codec *codec)
+{
+	int ret = 0;
+	struct device *dev = codec->card->dev;
+
+	dev_dbg(dev, "%s: Enter.\n", __func__);
+
+	mutex_lock(&power_lock);
+
+	ab8500_power_count++;
+	dev_dbg(dev, "%s: ab8500_power_count changed from %d to %d",
+		__func__, ab8500_power_count-1, ab8500_power_count);
+
+	if (ab8500_power_count == 1) {
+		/* Turn on audio-regulator */
+		ret = enable_regulator(dev, REGULATOR_AUDIO);
+		if (ret < 0)
+			goto reg_err;
+
+		ret = clk_set_parent(clk_ptr_intclk,
+				(master_clock_sel == 0) ? clk_ptr_sysclk : clk_ptr_ulpclk);
+		if (ret != 0) {
+			dev_err(dev, "%s: ERROR: Setting master-clock to %s failed (ret = %d)!",
+				__func__, (master_clock_sel == 0) ? "SYSCLK" : "ULPCLK", ret);
+			ret = -EIO;
+			goto clk_err;
+		}
+		dev_dbg(dev, "%s: Enabling master-clock (%s).",
+			__func__, (master_clock_sel == 0) ? "SYSCLK" : "ULPCLK");
+
+		/* Enable audio-clock */
+		ret = clk_enable(clk_ptr_audioclk);
+		if (ret != 0) {
+			dev_err(dev, "%s: ERROR: clk_enable failed (ret = %d)!",
+				__func__, ret);
+			ret = -EIO;
+			goto clk_err;
+		}
+
+		/* Power on audio-parts of AB8500 */
+		ab8500_audio_power_control(codec, true);
+		if (ret < 0)
+			goto pwr_err;
+	}
+
+	goto out;
+
+pwr_err:
+	clk_disable(clk_ptr_audioclk);
+
+clk_err:
+	disable_regulator(dev, REGULATOR_AUDIO);
+
+reg_err:
+	ab8500_power_count = 0;
+
+out:
+	mutex_unlock(&power_lock);
+
+	dev_dbg(dev, "%s: Exit.\n", __func__);
+
+	return ret;
+}
+
+static void ux500_ab8500_power_control_dec(struct snd_soc_codec *codec)
+{
+	struct device *dev = codec->card->dev;
+
+	dev_dbg(dev, "%s: Enter.\n", __func__);
+
+	mutex_lock(&power_lock);
+
+	ab8500_power_count--;
+
+	dev_dbg(dev, "%s: ab8500_power_count changed from %d to %d",
+		__func__,
+		ab8500_power_count+1,
+		ab8500_power_count);
+
+	if (ab8500_power_count == 0) {
+		/* Power off audio-parts of AB8500 */
+		ab8500_audio_power_control(codec, false);
+
+		/* Disable audio-clock */
+		dev_dbg(dev, "%s: Disabling master-clock (%s).",
+			__func__, (master_clock_sel == 0) ? "SYSCLK" : "ULPCLK");
+		clk_disable(clk_ptr_audioclk);
+
+		/* Turn off audio-regulator */
+		disable_regulator(dev, REGULATOR_AUDIO);
+	}
+
+	mutex_unlock(&power_lock);
+
+	dev_dbg(dev, "%s: Exit.\n", __func__);
+}
+
+/* Controls - Non-DAPM Non-ASoC */
+
+static int mclk_input_control_get(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	ucontrol->value.enumerated.item[0] = master_clock_sel;
+
+	return 0;
+}
+
+static int mclk_input_control_put(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	unsigned int val;
+
+	val = (ucontrol->value.enumerated.item[0] != 0);
+	if (master_clock_sel == val)
+		return 0;
+
+	master_clock_sel = val;
+
+	return 1;
+}
+
+static const struct snd_kcontrol_new mclk_input_control = \
+	SOC_ENUM_EXT("Master Clock Select", soc_enum_mclk,
+		mclk_input_control_get, mclk_input_control_put);
+
+static int anc_status_control_get(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+
+	mutex_lock(&codec->mutex);
+	ucontrol->value.integer.value[0] = anc_status;
+	mutex_unlock(&codec->mutex);
+
+	return 0;
+}
+
+static int anc_status_control_put(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	struct device *dev = codec->card->dev;
+	bool apply_fir, apply_iir;
+	int req, ret;
+
+	dev_dbg(dev, "%s: Enter.\n", __func__);
+
+	req = ucontrol->value.integer.value[0];
+	if (req != ANC_APPLY_FIR_IIR && req != ANC_APPLY_FIR &&
+		req != ANC_APPLY_IIR) {
+		dev_err(dev, "%s: ERROR: Unsupported status to set '%s'!\n",
+			__func__, enum_anc_state[req]);
+		return -EINVAL;
+	}
+	apply_fir = req == ANC_APPLY_FIR || req == ANC_APPLY_FIR_IIR;
+	apply_iir = req == ANC_APPLY_IIR || req == ANC_APPLY_FIR_IIR;
+
+	ret = ux500_ab8500_power_control_inc(codec);
+	if (ret < 0) {
+		dev_err(dev, "%s: ERROR: Failed to enable power (ret = %d)!\n",
+			__func__, ret);
+		goto cleanup;
+	}
+
+	mutex_lock(&codec->mutex);
+
+	ab8500_audio_anc_configure(codec, apply_fir, apply_iir);
+
+	if (apply_fir) {
+		if (anc_status == ANC_IIR_CONFIGURED)
+			anc_status = ANC_FIR_IIR_CONFIGURED;
+		else if (anc_status != ANC_FIR_IIR_CONFIGURED)
+			anc_status =  ANC_FIR_CONFIGURED;
+	}
+	if (apply_iir) {
+		if (anc_status == ANC_FIR_CONFIGURED)
+			anc_status = ANC_FIR_IIR_CONFIGURED;
+		else if (anc_status != ANC_FIR_IIR_CONFIGURED)
+			anc_status =  ANC_IIR_CONFIGURED;
+	}
+
+	mutex_unlock(&codec->mutex);
+
+	ux500_ab8500_power_control_dec(codec);
+
+cleanup:
+	if (ret < 0)
+		dev_err(dev, "%s: Unable to configure ANC! (ret = %d)\n",
+			__func__, ret);
+
+	dev_dbg(dev, "%s: Exit.\n", __func__);
+
+	return (ret < 0) ? ret : 1;
+}
+
+static const struct snd_kcontrol_new anc_status_control = \
+	SOC_ENUM_EXT("ANC Status", soc_enum_ancstate,
+		anc_status_control_get, anc_status_control_put);
+
+/* DAPM-events */
+
+static int dapm_audioreg_event(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		ux500_ab8500_power_control_inc(w->codec);
+	else
+		ux500_ab8500_power_control_dec(w->codec);
+
+	return 0;
+}
+
+static int dapm_amicreg_event(struct device *dev, enum amic_idx amic_id, int event)
+{
+	int ret = 0;
+
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		ret = claim_amic_regulator(dev, amic_id);
+	else if (amic_info[amic_id].enabled)
+		release_amic_regulator(dev, amic_id);
+
+	return ret;
+}
+
+static int dapm_amic1areg_event(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	return dapm_amicreg_event(w->dapm->dev, AMIC_1A, event);
+}
+
+static int dapm_amic1breg_event(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	return dapm_amicreg_event(w->dapm->dev, AMIC_1B, event);
+}
+
+static int dapm_amic2reg_event(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	return dapm_amicreg_event(w->dapm->dev, AMIC_2, event);
+}
+
+static int dapm_dmicreg_event(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	struct device *dev = w->dapm->dev;
+	int ret = 0;
+
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		ret = enable_regulator(dev, REGULATOR_DMIC);
+	else
+		disable_regulator(dev, REGULATOR_DMIC);
+
+	return ret;
+}
+
+/* DAPM-widgets */
+
+static const struct snd_soc_dapm_widget ux500_ab8500_dapm_widgets[] = {
+	SND_SOC_DAPM_SUPPLY("AUDIO Regulator",
+			SND_SOC_NOPM, 0, 0, dapm_audioreg_event,
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_SUPPLY("AMIC1A Regulator",
+			SND_SOC_NOPM, 0, 0, dapm_amic1areg_event,
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_SUPPLY("AMIC1B Regulator",
+			SND_SOC_NOPM, 0, 0, dapm_amic1breg_event,
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_SUPPLY("AMIC2 Regulator",
+			SND_SOC_NOPM, 0, 0, dapm_amic2reg_event,
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_SUPPLY("DMIC Regulator",
+			SND_SOC_NOPM, 0, 0, dapm_dmicreg_event,
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+};
+
+/* DAPM-routes */
+
+static const struct snd_soc_dapm_route ux500_ab8500_dapm_intercon[] = {
+
+	/* Power AB8500 audio-block when AD/DA is active */
+	{"DAC", NULL, "AUDIO Regulator"},
+	{"ADC", NULL, "AUDIO Regulator"},
+
+	/* Power configured regulator when an analog mic is enabled */
+	{"MIC1A Input", NULL, "AMIC1A Regulator"},
+	{"MIC1B Input", NULL, "AMIC1B Regulator"},
+	{"MIC2 Input", NULL, "AMIC2 Regulator"},
+
+	/* Power DMIC-regulator when any digital mic is enabled */
+	{"DMic 1", NULL, "DMIC Regulator"},
+	{"DMic 2", NULL, "DMIC Regulator"},
+	{"DMic 3", NULL, "DMIC Regulator"},
+	{"DMic 4", NULL, "DMIC Regulator"},
+	{"DMic 5", NULL, "DMIC Regulator"},
+	{"DMic 6", NULL, "DMIC Regulator"},
+};
+
+static int add_widgets(struct snd_soc_codec *codec)
+{
+	struct device *dev = codec->card->dev;
+	int ret;
+
+	ret = snd_soc_dapm_new_controls(&codec->dapm,
+			ux500_ab8500_dapm_widgets,
+			ARRAY_SIZE(ux500_ab8500_dapm_widgets));
+	if (ret < 0) {
+		dev_err(dev, "%s: Failed to create DAPM controls (%d).\n",
+			__func__, ret);
+		return ret;
+	}
+
+	ret = snd_soc_dapm_add_routes(&codec->dapm,
+				ux500_ab8500_dapm_intercon,
+				ARRAY_SIZE(ux500_ab8500_dapm_intercon));
+	if (ret < 0) {
+		dev_err(dev, "%s: Failed to add DAPM routes (%d).\n",
+			__func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/* ASoC */
+
+int ux500_ab8500_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct device *dev = rtd->card->dev;
+	int ret = 0;
+
+	dev_dbg(dev, "%s: Enter\n", __func__);
+
+	if (IS_ERR(clk_ptr_sysclk)) {
+		dev_err(dev, "%s: ERROR: Clocks needed for streaming not available!",
+			__func__);
+		return -EAGAIN;
+	}
+
+	/* Enable gpio.1-clock (needed by DSP in burst mode) */
+	ret = clk_enable(clk_ptr_gpio1);
+	if (ret) {
+		dev_err(dev, "%s: ERROR: clk_enable(gpio.1) failed (ret = %d)!",
+			__func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+void ux500_ab8500_shutdown(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct device *dev = rtd->card->dev;
+
+	dev_dbg(dev, "%s: Enter\n", __func__);
+
+	/* Reset slots configuration to default(s) */
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		tx_slots = DEF_TX_SLOTS;
+	else
+		rx_slots = DEF_RX_SLOTS;
+
+	clk_disable(clk_ptr_gpio1);
+}
+
+int ux500_ab8500_hw_params(struct snd_pcm_substream *substream,
+			struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct device *dev = rtd->card->dev;
+	unsigned int fmt, fmt_if1;
+	int channels, ret = 0, slots, slot_width, driver_mode;
+	bool is_playback;
+
+	dev_dbg(dev, "%s: Enter\n", __func__);
+
+	dev_dbg(dev, "%s: substream->pcm->name = %s\n"
+		"substream->pcm->id = %s.\n"
+		"substream->name = %s.\n"
+		"substream->number = %d.\n",
+		__func__,
+		substream->pcm->name,
+		substream->pcm->id,
+		substream->name,
+		substream->number);
+
+	channels = params_channels(params);
+
+	/* Setup codec depending on driver-mode */
+	driver_mode = (channels == 8) ?
+		DRIVERMODE_CODEC_ONLY : DRIVERMODE_NORMAL;
+	dev_dbg(dev, "%s: Driver-mode: %s.\n", __func__,
+		(driver_mode == DRIVERMODE_NORMAL) ? "NORMAL" : "CODEC_ONLY");
+
+	ab8500_audio_set_bit_delay(codec_dai, 1);
+
+	if (driver_mode == DRIVERMODE_NORMAL) {
+		ab8500_audio_set_word_length(codec_dai, 16);
+		fmt = SND_SOC_DAIFMT_DSP_A |
+			SND_SOC_DAIFMT_CBM_CFM |
+			SND_SOC_DAIFMT_NB_NF |
+			SND_SOC_DAIFMT_CONT;
+	} else {
+		ab8500_audio_set_word_length(codec_dai, 20);
+		fmt = SND_SOC_DAIFMT_DSP_A |
+			SND_SOC_DAIFMT_CBM_CFM |
+			SND_SOC_DAIFMT_NB_NF |
+			SND_SOC_DAIFMT_GATED;
+	}
+
+	ret = snd_soc_dai_set_fmt(codec_dai, fmt);
+	if (ret < 0) {
+		dev_err(dev, "%s: ERROR: snd_soc_dai_set_fmt failed "
+			"for codec_dai (ret = %d)!\n", __func__, ret);
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_fmt(cpu_dai, fmt);
+	if (ret < 0) {
+		dev_err(dev, "%s: ERROR: snd_soc_dai_set_fmt failed "
+			"for cpu_dai (ret = %d)!\n", __func__, ret);
+		return ret;
+	}
+
+	/* Setup TDM-slots */
+
+	is_playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
+	switch (channels) {
+	case 1:
+		slots = 16;
+		slot_width = 16;
+		tx_slots = (is_playback) ? TX_SLOT_MONO : 0;
+		rx_slots = (is_playback) ? 0 : RX_SLOT_MONO;
+		break;
+	case 2:
+		slots = 16;
+		slot_width = 16;
+		tx_slots = (is_playback) ? TX_SLOT_STEREO : 0;
+		rx_slots = (is_playback) ? 0 : RX_SLOT_STEREO;
+		break;
+	case 8:
+		slots = 16;
+		slot_width = 16;
+		tx_slots = (is_playback) ? TX_SLOT_8CH : 0;
+		rx_slots = (is_playback) ? 0 : RX_SLOT_8CH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "%s: CPU-DAI TDM: TX=0x%04X RX=0x%04x\n", __func__,
+		tx_slots, rx_slots);
+	ret = snd_soc_dai_set_tdm_slot(cpu_dai, tx_slots, rx_slots, slots, slot_width);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "%s: CODEC-DAI TDM: TX=0x%04X RX=0x%04x\n", __func__,
+		tx_slots, rx_slots);
+	ret = snd_soc_dai_set_tdm_slot(codec_dai, tx_slots, rx_slots, slots, slot_width);
+	if (ret)
+		return ret;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		dev_dbg(dev, "%s: Setup IF1 for FM-radio.\n", __func__);
+		fmt_if1 = SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_I2S;
+		ret = ab8500_audio_setup_if1(codec_dai->codec, fmt_if1, 16, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+struct snd_soc_ops ux500_ab8500_ops[] = {
+	{
+	.hw_params = ux500_ab8500_hw_params,
+	.startup = ux500_ab8500_startup,
+	.shutdown = ux500_ab8500_shutdown,
+	}
+};
+
+int ux500_ab8500_machine_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_codec *codec = rtd->codec;
+	struct device *dev = rtd->card->dev;
+	int ret;
+
+	dev_dbg(dev, "%s Enter.\n", __func__);
+
+	/* Add controls */
+	snd_ctl_add(codec->card->snd_card, snd_ctl_new1(&mclk_input_control, codec));
+	snd_ctl_add(codec->card->snd_card, snd_ctl_new1(&anc_status_control, codec));
+
+	ret = create_regulators(rtd);
+	if (ret < 0) {
+		dev_err(dev, "%s: ERROR: Failed to instantiate regulators (ret = %d)!\n",
+			__func__, ret);
+		return ret;
+	}
+
+	/* Get references to clock-nodes */
+	clk_ptr_sysclk = NULL;
+	clk_ptr_ulpclk = NULL;
+	clk_ptr_intclk = NULL;
+	clk_ptr_audioclk = NULL;
+	clk_ptr_gpio1 = NULL;
+	clk_ptr_sysclk = clk_get(codec->dev, "sysclk");
+	if (IS_ERR(clk_ptr_sysclk))
+		dev_warn(dev, "WARNING: clk_get failed for 'sysclk'!\n");
+	clk_ptr_ulpclk = clk_get(codec->dev, "ulpclk");
+	if (IS_ERR(clk_ptr_sysclk))
+		dev_warn(dev, "WARNING: clk_get failed for 'ulpclk'!\n");
+	clk_ptr_intclk = clk_get(codec->dev, "intclk");
+	if (IS_ERR(clk_ptr_audioclk))
+		dev_warn(dev, "WARNING: clk_get failed for 'intclk'!\n");
+	clk_ptr_audioclk = clk_get(codec->dev, "audioclk");
+	if (IS_ERR(clk_ptr_audioclk))
+		dev_warn(dev, "WARNING: clk_get failed for 'audioclk'!\n");
+	clk_ptr_gpio1 = clk_get_sys("gpio.1", NULL);
+	if (IS_ERR(clk_ptr_gpio1))
+		dev_warn(dev, "WARNING: clk_get failed for 'gpio1'!\n");
+
+	/* Set intclk default parent to ulpclk */
+	ret = clk_set_parent(clk_ptr_intclk, clk_ptr_ulpclk);
+	if (ret)
+		dev_warn(dev, "%s: WARNING: Setting intclk parent to ulpclk failed (ret = %d)!",
+			__func__,
+			ret);
+
+	master_clock_sel = 1;
+
+	ab8500_power_count = 0;
+
+	reg_claim[REGULATOR_AMIC1] = 0;
+	reg_claim[REGULATOR_AMIC2] = 0;
+
+	/* Add DAPM-widgets */
+	ret = add_widgets(codec);
+	if (ret < 0) {
+		dev_err(dev, "%s: Failed add widgets (%d).\n", __func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+int ux500_ab8500_soc_machine_drv_init(void)
+{
+	pr_debug("%s: Enter.\n", __func__);
+
+	return 0;
+}
+
+void ux500_ab8500_soc_machine_drv_cleanup(void)
+{
+	pr_debug("%s: Enter.\n", __func__);
+
+	regulator_bulk_free(ARRAY_SIZE(reg_info), reg_info);
+
+	if (clk_ptr_sysclk != NULL)
+		clk_put(clk_ptr_sysclk);
+	if (clk_ptr_ulpclk != NULL)
+		clk_put(clk_ptr_ulpclk);
+	if (clk_ptr_intclk != NULL)
+		clk_put(clk_ptr_intclk);
+	if (clk_ptr_audioclk != NULL)
+		clk_put(clk_ptr_audioclk);
+	if (clk_ptr_gpio1 != NULL)
+		clk_put(clk_ptr_gpio1);
+}
+
diff --git a/sound/soc/ux500/ux500_ab8500.h b/sound/soc/ux500/ux500_ab8500.h
new file mode 100644
index 0000000..4fffb8a
--- /dev/null
+++ b/sound/soc/ux500/ux500_ab8500.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2012
+ *
+ * Author: Ola Lilja <ola.o.lilja@stericsson.com>
+ *         for ST-Ericsson.
+ *
+ * License terms:
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#ifndef UX500_AB8500_H
+#define UX500_AB8500_H
+
+extern struct snd_soc_ops ux500_ab8500_ops[];
+
+int ux500_ab8500_machine_init(struct snd_soc_pcm_runtime *runtime);
+
+int ux500_ab8500_soc_machine_drv_init(void);
+
+void ux500_ab8500_soc_machine_drv_cleanup(void);
+
+#endif
-- 
1.7.8.3

Re: [PATCH 16/16] ASoC: Ux500: Add machine-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 3279 bytes --]

On Tue, Mar 13, 2012 at 04:11:43PM +0100, Ola Lilja wrote:

> +config SND_SOC_UX500_AB8500
> +	bool "Codec/Machine - AB8500 (MSA)"

Wny non-modular?

> +	depends on SND_SOC_UX500_MSP_I2S && SND_SOC_UX500_PLATFORM && AB8500_CORE && AB8500_GPADC

Word wrapping and this should select pretty much all of this with the
possible exception of the MFD core.

> +ifdef CONFIG_SND_SOC_UX500_AB8500
> +snd-soc-ux500-machine-objs += ux500_ab8500.o
> +endif

Again, why are you doing this non-idiomatic stuff?

> +/* Create dummy devices for platform drivers */

> +static struct platform_device ux500_pcm = {
> +		.name = "ux500-pcm",
> +		.id = 0,
> +		.dev = {
> +			.platform_data = NULL,
> +		},
> +};

The SoC or CPU side drivers should be taking care of stuff like this.

> +/* Define the whole U8500 soundcard, linking platform to the codec-drivers  */
> +struct snd_soc_dai_link u8500_dai_links[] = {
> +	#ifdef CONFIG_SND_SOC_UX500_AB8500

What's this ifdef doing...?  It looks very, very suspicous.

> +	{
> +	.name = "ab8500_0",

Your indentation is also fairly broken here.

> +	pdev = platform_device_alloc("soc-audio", -1);
> +	if (!pdev)
> +		return -ENOMEM;

No, probe using the normal device model and register using
snd_soc_register_card().

> +++ b/sound/soc/ux500/ux500_ab8500.c

The fact that you've got multiple files for a machine driver is
worrying...

> +/* ANC States */
> +static const char * const enum_anc_state[] = {
> +	"Unconfigured",
> +	"Apply FIR and IIR",
> +	"FIR and IIR are configured",
> +	"Apply FIR",
> +	"FIR is configured",
> +	"Apply IIR",
> +	"IIR is configured"
> +};

Why is this not part of the CODEC driver?  What makes this machine
specific?

> +/* Regulators */
> +enum regulator_idx {
> +	REGULATOR_AUDIO,
> +	REGULATOR_DMIC,
> +	REGULATOR_AMIC1,
> +	REGULATOR_AMIC2
> +};
> +static struct regulator_bulk_data reg_info[4] = {
> +	{	.consumer = NULL, .supply = "V-AUD"	},
> +	{	.consumer = NULL, .supply = "V-DMIC"	},
> +	{	.consumer = NULL, .supply = "V-AMIC1"	},
> +	{	.consumer = NULL, .supply = "V-AMIC2"	}
> +};
> +static bool reg_enabled[4] =  {
> +	false,
> +	false,
> +	false,
> +	false
> +};
> +static int reg_claim[4];
> +enum amic_idx { AMIC_1A, AMIC_1B, AMIC_2 };
> +struct amic_conf {
> +	enum regulator_idx reg_id;
> +	bool enabled;
> +	char *name;

I have no idea what this code is intended to do but it looks *extremely*
confused, it's especially surprising given that your CODEC makes no use
of regulators even for basic power.

> +static int enable_regulator(struct device *dev, enum regulator_idx idx)
> +{

You appear to be defining some sort of subsystem on top of the regulator
API here but I'm not sure what it's intended to do or why you're doing
it...

> +static const struct snd_soc_dapm_widget ux500_ab8500_dapm_widgets[] = {
> +	SND_SOC_DAPM_SUPPLY("AUDIO Regulator",
> +			SND_SOC_NOPM, 0, 0, dapm_audioreg_event,
> +			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),

DAPM has native support for regulator supplies.

> +	/* Enable gpio.1-clock (needed by DSP in burst mode) */
> +	ret = clk_enable(clk_ptr_gpio1);
> +	if (ret) {
> +		dev_err(dev, "%s: ERROR: clk_enable(gpio.1) failed (ret = %d)!",
> +			__func__, ret);
> +		return ret;
> +	}

Why can't the DSP figure out that it needs to enable the clock?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 01/16] ASoC: core: Add multi reg control struct & macros

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 575 bytes --]

On Tue, Mar 13, 2012 at 04:11:28PM +0100, Ola Lilja wrote:

> SOC_SINGLE_VALUE_S1R  One control value spans one register
> SOC_SINGLE_VALUE_S2R  One control value spans two registers
> SOC_SINGLE_VALUE_S4R  One control value spans four registers
> SOC_SINGLE_VALUE_S8R  One control value spans eight registers

This is fairly painful; the number of registers really ought to be
parameterised rather than having to add a new crop of macros every time
there's a new format or a new count.  Can we possibly make the
simplifying assumption that all the registers are contiguous?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 01/16] ASoC: core: Add multi reg control struct & macros

[Kristoffer KARLSSON]

I believe that the assumption that all the registers are contiguous would pose a
troublesome limitation in this case.
We do have several controls that exposes signed 15-bit values that are mapped
across two 8-bit registers (16-bits in total) in a big-endian order.
While for those particular controls a contiguous (big endian) order like you
suggested would work just fine, we also have other controls with signed 32-bit
values that span four 8-bit registers that mapped in the following manner:

bits	31-24	23-16	15-8	7-0
reg	0x59	0x5A	0x59	0x5A

This means that to write one complete 32-bit value we would actually need to
write to the same 8-bit registers twice.

I originally considered to put in two more parameters to consolidate the four
macros to one, one being no of registers and one specifying big- or
little-endian order. But as you can see unfortunately neither little or big
endian contiguous order would suffice in the case described above so therefore I
opted to give the user full control in specifying the registers and what order
they should map to the composite value when using the macro allowing for every
possible combination which seems to be the most generic approach allowing for
different formats.

Also I believe that the definition S1R,S2R,S4R together with S8R might in fact
already be a fairly complete set with no need to add some new crop of macros for
this in future. Why? Well since the value handled by ASoC-framework when
setting/reading a integer control is of type long (snd_ctl_elem_value) in
combination with the fact that 1 byte being the smallest register size in
framework means that in a 64-bit world this would translate to a maximum of
eight registers mapping the composite value of that long value.
Actually in our driver we only need S1R, S2R and S4R. I added the S8R variant
just to provide a complete implementation covering up to the full 64 bit
(8*8-bit). So until we need to deal with 128-bit computers and someone would
need this exact type of composite control in 128-bit then I believe these four
macros should suffice for most cases.

Anyhow in light of the situation above do you think we could stick to the
submitted approach or do you have some other suggestion on how to define the
signed 32-bit value control with registers mapped in the above described fashion?


On 2012-03-13 22:39, Mark Brown wrote:

> On Tue, Mar 13, 2012 at 04:11:28PM +0100, Ola Lilja wrote:
> 
>> SOC_SINGLE_VALUE_S1R  One control value spans one register
>> SOC_SINGLE_VALUE_S2R  One control value spans two registers
>> SOC_SINGLE_VALUE_S4R  One control value spans four registers
>> SOC_SINGLE_VALUE_S8R  One control value spans eight registers
> 
> This is fairly painful; the number of registers really ought to be
> parameterised rather than having to add a new crop of macros every time
> there's a new format or a new count.  Can we possibly make the
> simplifying assumption that all the registers are contiguous?



-- 


*Kristoffer Karlsson*

Multimedia Platform Customization Lund

*ST-Ericsson*
Product Group Mobile Platforms

Scheelevägen 19B
223 63, Lund
Sweden
www.stericsson.com <http://www.stericsson.com/>
	Office: +46 46 10 3938
Mobile: +46 72 206 5192
Fax: +46 10 715 89 88
Email: kristoffer.karlsson@stericsson.com
<mailto:kristoffer.karlsson@stericsson.com>


This communication is confidential and intended solely for the addressee(s). Any
unauthorized review, use, disclosure or distribution is prohibited. If you
believe this message has been sent to you in error, please notify the sender by
replying to this transmission and delete the message without disclosing it.
Thank you.

E-mail including attachments is susceptible to data corruption, interception,
unauthorized amendment, tampering and viruses, and we only send and receive
emails on the basis that we are not liable for any such corruption,
interception, amendment, tampering or viruses or any consequences thereof.

Re: [PATCH 01/16] ASoC: core: Add multi reg control struct & macros

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1915 bytes --]

On Wed, Mar 21, 2012 at 01:07:47PM +0100, Kristoffer KARLSSON wrote:

*NEVER* top post...

> I believe that the assumption that all the registers are contiguous would pose a
> troublesome limitation in this case.

...it leads to people reading contextless things like this and having no
idea what you're talking about.

You should also fix your mailer to word wrap within 80 columns - note
how the quote above wraps.

> suggested would work just fine, we also have other controls with signed 32-bit
> values that span four 8-bit registers that mapped in the following manner:

> bits	31-24	23-16	15-8	7-0
> reg	0x59	0x5A	0x59	0x5A

> This means that to write one complete 32-bit value we would actually need to
> write to the same 8-bit registers twice.

This makes no sense - how do the vales "span four 8-bit registers" while
using only two register addresses?

> Also I believe that the definition S1R,S2R,S4R together with S8R might in fact
> already be a fairly complete set with no need to add some new crop of macros for
> this in future. Why? Well since the value handled by ASoC-framework when
> setting/reading a integer control is of type long (snd_ctl_elem_value) in
> combination with the fact that 1 byte being the smallest register size in
> framework means that in a 64-bit world this would translate to a maximum of
> eight registers mapping the composite value of that long value.

What happens when someone wants unsigned controls, or stereo controls,
or 24 bit registers or anything else?  There's way more things can vary
than just the word size.

> Anyhow in light of the situation above do you think we could stick to the
> submitted approach or do you have some other suggestion on how to define the
> signed 32-bit value control with registers mapped in the above described fashion?

No, sorry.  This stuff just all seems really painful to use, I'd at
least hope for more parameterisation.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 01/16] ASoC: core: Add multi reg control struct & macros

[Kristoffer KARLSSON]

On 2012-03-21 13:40, Mark Brown wrote:

> On Wed, Mar 21, 2012 at 01:07:47PM +0100, Kristoffer KARLSSON wrote:
> 
> *NEVER* top post...
> 
>> I believe that the assumption that all the registers are contiguous would
>> pose a troublesome limitation in this case.
> 
> ...it leads to people reading contextless things like this and having no
> idea what you're talking about.
> 
> You should also fix your mailer to word wrap within 80 columns - note
> how the quote above wraps.
 

Adjusted mailer. Thank you for the notice.

>> suggested would work just fine, we also have other controls with signed
>> 32-bit values that span four 8-bit registers that mapped in the following
>> manner:
> 
>> bits	31-24	23-16	15-8	7-0
>> reg	0x59	0x5A	0x59	0x5A
> 
>> This means that to write one complete 32-bit value we would actually need
>> to write to the same 8-bit registers twice.
> 
> This makes no sense - how do the vales "span four 8-bit registers" while
> using only two register addresses?


Every write to register 0x5A copies the written data to the correct active
internal chunk of bits of the composite (which after chip boot up is the upper
bit parts of the complete 32-bit parameter) then it automatically (as part of
the write operation) toggles an internal shift pointer inside the chip that
makes the subsequent write to registers 0x59 and 0x5A point to the lower most
bits of the full 32-bit parameter. Writing to register 0x5A the second time
hence copies the written data the lower bit parts and then toggles the internal
shift back to point to the upper bits again allowing for another pass of
writing of the complete 32-bit value parameter.

In our hardware there is no way to determine the current toggle position of the
above described internal shift pointer (or even to reset it) so it becomes very
important to make sure to write exactly twice to both 0x59 and 0x5A and in that
very order to make sure that the internal shift pointer always afterwards is
pointing to the upper most bits of the 32-bit value allowing for configuring of
a new 32-bit value.

>> Also I believe that the definition S1R,S2R,S4R together with S8R might in
>> fact already be a fairly complete set with no need to add some new crop of
>> macros for this in future. Why? Well since the value handled by 
>> ASoC-framework when setting/reading a integer control is of type long 
>> (snd_ctl_elem_value) in combination with the fact that 1 byte being the
>> smallest register size in framework means that in a 64-bit world this would
>> translate to a maximum of eight registers mapping the composite value of.
>> that long value.
> 
> What happens when someone wants unsigned controls, or stereo controls,
> or 24 bit registers or anything else?  There's way more things can vary
> than just the word size.
 

This is true. In light of this a single parametrized macro like you suggested
would be preferable. This will allow for more variants like you said. I agree.

>> Anyhow in light of the situation above do you think we could stick to the
>> submitted approach or do you have some other suggestion on how to define
>> the signed 32-bit value control with registers mapped in the above
>> described fashion?
> 
> No, sorry.  This stuff just all seems really painful to use, I'd at
> least hope for more parameterisation.


I will make a new patch that adds only one macro with a parameter exposing a
register base (ie. the starting register) in combination with a parameter for
register count like you suggested. While this approach will not support the
very type of register mapping discussed above it will work just fine for the
several other multi register controls we also have in our hardware that really
are contiguous and also supports reading back the composite signed value. For
these controls this parameterized single macro would work just fine.

For the three special composite controls we have in our hardware that requires
writing to the same register several times during configuration of one single
parameter I propose a separate solution due to the fact that the hardware does
not even support reading back those values since a read operation does not
trigger the internal shift toggle like the write operations do, which means
that only half the complete composite value can ever be read back. To handle
these three controls I propose a separate new macro that allows for caching of
the lastly written composite long value stored in that control's private value
member allowing for any client reading back the lastly configured parameter
while still the 8-bit chunks of the composite value can be written to the
hardware in the specific sequence required for that parameter.

I will submit this additional second patch also in full so that you can have a
look at it to see if you agree that it would be generic enough for addition to
the asoc-core framework or if we need to keep these additional cached controls
only as a specific implementation in our codec driver due to the
characteristics of our hardware design which does not allow for read back of
the complete composite value in all cases.

But for the first control type (composite control composed of a parameterized
number of contiguous 8-bit registers) I think we agree on the implementation of
a single macro for that now. I believe that it could be called
SOC_SINGLE_XR8_SX then? Do you agree?

Re: [PATCH 01/16] ASoC: core: Add multi reg control struct & macros

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 883 bytes --]

On Thu, Mar 22, 2012 at 04:46:56PM +0100, Kristoffer KARLSSON wrote:
> On 2012-03-21 13:40, Mark Brown wrote:

> > This makes no sense - how do the vales "span four 8-bit registers" while
> > using only two register addresses?

> Every write to register 0x5A copies the written data to the correct active
> internal chunk of bits of the composite (which after chip boot up is the upper
> bit parts of the complete 32-bit parameter) then it automatically (as part of
> the write operation) toggles an internal shift pointer inside the chip that

This *really* isn't what the control you provided describes and is going
to break when people try to do things like readback.  The hardware
design here is sufficiently "creative" that I think the best thing is to
just code it in the driver, ideally doing something more direct which
makes it clear that you've got this shifting going on.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 12/16] ASoC: Ux500: Add MSP I2S-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 9476 bytes --]

On Tue, Mar 13, 2012 at 04:11:39PM +0100, Ola Lilja wrote:

> +menuconfig SND_SOC_UX500
> +	bool "SoC Audio support for Ux500 platform"
> +	depends on SND_SOC
> +	default n

This is redundant, the default default is n.

> +config SND_SOC_UX500_MSP_I2S
> +	bool "Platform - MSP (I2S)"
> +	depends on SND_SOC_UX500
> +	default n
> +	help
> +
> +		Say Y if you want to enable the Ux500 MSP I2S-driver.

Don't do this, the driver is only usable with a machine driver so should
be selected by the machine driver.

> +config SND_SOC_UX500_DEBUG
> +	bool "Activate Ux500 platform debug-mode (pr_debug)"
> +	depends on SND_SOC_UX500
> +	default n
> +	help
> +		Say Y if you want to enable debug-level prints for Ux500 code-files.

No.

> +ifdef CONFIG_SND_SOC_UX500_MSP_I2S
> +snd-soc-ux500-platform-i2s-objs := ux500_msp_dai.o ux500_msp_i2s.o
> +obj-$(CONFIG_SND_SOC_UX500_MSP_I2S) += snd-soc-ux500-platform-i2s.o
> +endif

Do this idiomatically please.  If you need to do use this odd method the
code should explain why.

> +#define to_i2s_controller(d) container_of(d, struct i2s_controller, dev)

Use a function please for type safety (static inline should work out
about the same in terms of the generated code).

> +/*** Protocols ***/
> +enum msp_protocol {
> +	MSP_I2S_PROTOCOL,
> +	MSP_PCM_PROTOCOL,
> +	MSP_PCM_COMPAND_PROTOCOL,
> +	MSP_AC97_PROTOCOL,
> +	MSP_MASTER_SPI_PROTOCOL,
> +	MSP_SLAVE_SPI_PROTOCOL,
> +	MSP_INVALID_PROTOCOL
> +};

Does some of this code want to be in a library somewhere so the hardware
support can be shared with the SPI subsystem?  In general there's a
pretty enormous abstraction layer in here which probably isn't doing all
that much if this stuff is internal to the audio subsystem - my overall
feeling reading the code is that the abstraction layer should be removed
as it's internal to this driver.

> +static struct ux500_platform_drvdata platform_drvdata[UX500_NBR_OF_DAI] = {
> +	{
> +		.msp_i2s_drvdata = NULL,
> +		.fmt = 0,
> +		.slots = 1,
> +		.tx_mask = 0x01,
> +		.rx_mask = 0x01,
> +		.slot_width = 16,
> +		.playback_active = false,
> +		.capture_active = false,
> +		.configured = 0,
> +		.master_clk = UX500_MSP_INTERNAL_CLOCK_FREQ,
> +	},

This looks like it should be dynamically created by the platform
device, not a static struct.

> +static const char *stream_str(struct snd_pcm_substream *substream)
> +{
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		return "Playback";
> +	else
> +		return "Capture";
> +}

Don't add generic code in drivers.

> +	if (mode_playback)
> +		drvdata->playback_active = true;
> +	else
> +		drvdata->capture_active = true;

All this mode_playback stuff is non-idiomatic.

> +static void ux500_msp_dai_shutdown(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct ux500_platform_drvdata *drvdata = &platform_drvdata[dai->id];
> +	bool mode_playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
> +
> +	dev_dbg(dai->dev, "%s: MSP %d (%s): Enter.\n", __func__, dai->id,
> +		stream_str(substream));
> +
> +	if (drvdata == NULL)
> +		return;

Why might that happen?

> +	prot_desc->total_clocks_for_one_frame =
> +			prot_desc->frame_period+1;
> +
> +	dev_dbg(dai->dev, "%s: Total clocks per frame: %u\n",
> +		__func__,
> +		prot_desc->total_clocks_for_one_frame);

This looks like your clock calculation stuff may be replicating
framework features.

> +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +	default:
> +	case SND_SOC_DAIFMT_NB_NF:
> +		break;
> +
> +	case SND_SOC_DAIFMT_NB_IF:
> +		msp_config->tx_frame_sync_pol ^= 1 << TFSPOL_SHIFT;
> +		msp_config->rx_frame_sync_pol ^= 1 << RFSPOL_SHIFT;
> +		break;
> +	}

Shold be returning an error for unsupported modes.

> +	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) == SND_SOC_DAIFMT_CBM_CFM) {
> +		dev_dbg(dai->dev, "%s: Codec is MASTER.\n",

This should be a switch statement and again should error out on
unsupporetd modes.  Use of if statements instead of switch statements
happens elsewhere too.

> +	/* To avoid division by zero in I2S-driver (i2s_setup) */
> +	prot_desc->total_clocks_for_one_frame = 1;


Also many_of_these_variable_names_are_getting_so_long_it_is_hard_to_use_them_without_word_wrapping

> +	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS:
> +		dev_dbg(dai->dev, "%s: SND_SOC_DAIFMT_I2S.\n",
> +			__func__);

In general if you find this logging useful I'd add it to the framework;
perhaps other people will too and it'll save everyone open coding it.

> +
> +		msp_config->default_protocol_desc = 1;
> +		msp_config->protocol = MSP_I2S_PROTOCOL;
> +		break;
> +
> +	default:
> +	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM:

Are you *positive* that all modes you don't explicitly support are
identical to _I2S | _CBM_CFM?

> +	/* If already configured -> not errors reported */
> +	if (mode_playback) {
> +		if ((drvdata->configured & PLAYBACK_CONFIGURED) &&
> +			(drvdata->playback_active))
> +			goto cleanup;
> +	} else {
> +		if ((drvdata->configured & CAPTURE_CONFIGURED) &&
> +			(drvdata->capture_active))
> +			goto cleanup;
> +	}

This looks suspicious...  why are you doing this?

> +			dev_err(dai->dev, "%s: Error: PCM TDM format requires channels "
> +				"to match active slots "
> +				"(channels = %d, active slots = %d)!\n",
> +				__func__, params_channels(params), slots_active);

Don't split error messages over multiple lines, it makes them hard to
search for.

> +	if (!(slots == 1 || slots == 2 || slots == 8 || slots == 16)) {
> +		dev_err(dai->dev, "%s: Error: Unsupported slots (%d)! "
> +			"Supported values are 1/2/8/16.\n",
> +			__func__, slots);
> +		return -EINVAL;
> +	}

This looks like a switch statement....

> +	switch (slots) {
> +	default:
> +	case 1:
> +		cap = 0x01;
> +		break;
> +	case 2:
> +		cap = 0x03;
> +		break;
> +	case 8:
> +		cap = 0xFF;
> +		break;
> +	case 16:
> +		cap = 0xFFFF;
> +		break;
> +	}

...in fact it could be combined with this one.

> +static int ux500_msp_dai_trigger(struct snd_pcm_substream *substream,
> +				int cmd, struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +	struct ux500_platform_drvdata *drvdata = &platform_drvdata[dai->id];
> +
> +	dev_dbg(dai->dev, "%s: MSP %d (%s): Enter (msp->id = %d, cmd = %d).\n",
> +		__func__, dai->id, stream_str(substream),
> +		(int)drvdata->msp_i2s_drvdata->id, cmd);
> +
> +	ret = ux500_msp_i2s_trigger(drvdata->msp_i2s_drvdata, cmd, substream->stream);
> +
> +	return ret;
> +}

This function does nothing, just inline the function you're calling.

> +		.suspend = NULL,
> +		.resume = NULL,

No need to assign nonexistant ops like this.

> +		.ops = (struct snd_soc_dai_ops[]) {
> +			{
> +			.set_sysclk = ux500_msp_dai_set_dai_sysclk,
> +			.set_fmt = ux500_msp_dai_set_dai_fmt,
> +			.set_tdm_slot = ux500_msp_dai_set_tdm_slot,
> +			.startup = ux500_msp_dai_startup,
> +			.shutdown = ux500_msp_dai_shutdown,
> +			.prepare = ux500_msp_dai_prepare,
> +			.trigger = ux500_msp_dai_trigger,
> +			.hw_params = ux500_msp_dai_hw_params,
> +			}

Just declare the struct as a variable, if you need casts like this
something is going wrong and doing this will prevent sharing the ops
struct between the DAIs which is the whole point.

> +EXPORT_SYMBOL(ux500_msp_dai_drv);

Why are you doing this?

> +	dev_err(&pdev->dev, "%s: Enter (pdev->name = %s).\n", __func__, pdev->name);

Remove stuff like this, it's not an error.  You've got lots of logging
that should at most be at dev_dbg() level.

> +	platform_data = (struct msp_i2s_platform_data *)pdev->dev.platform_data;

Why not just inline this function?

> +	msp_i2s_drvdata = ux500_msp_i2s_init(pdev, platform_data);
> +	if (!msp_i2s_drvdata) {
> +		dev_err(&pdev->dev, "%s: ERROR: ux500_msp_i2s_init failed!", __func__);
> +		return -1;
> +	}

Are you sure that -EPERM is the best error code?  If you are then you
should use the constant not the number.

> +	dev_info(&pdev->dev, "%s: Registering ux500-msp-dai SoC CPU-DAI.\n", __func__);

For things like this the core will already log for you.

> +int ux500_msp_drv_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct ux500_msp_i2s_drvdata *msp_i2s_drvdata = dev_get_drvdata(&pdev->dev);
> +
> +	dev_dbg(&pdev->dev, "%s: Enter (pdev->name = %s).\n", __func__, pdev->name);
> +
> +	return ux500_msp_i2s_suspend(msp_i2s_drvdata);

Once again, this function does nothing and should just have the contents
inline.  This should also be done from the ASoC level callbacks so that
it's joined up with the suspend of the audio subsystem as a whole.

> +static struct platform_driver msp_i2s_driver = {
> +	.driver = {
> +		.name = "ux500-msp-i2s",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = ux500_msp_drv_probe,
> +	.remove = ux500_msp_drv_remove,

It looks like this just registers one device for all the ports in the
system.  Are all the ports really part of one IP or should the driver be
controlling a single port and letting the SoC instantiate whatever is
present on a given chip?

> +static int __init ux500_msp_init(void)

module_platform_driver.

> +#ifndef CONFIG_SND_SOC_UX500_AB5500
> +#define UX500_MSP_INTERNAL_CLOCK_FREQ  40000000
> +#define UX500_MSP1_INTERNAL_CLOCK_FREQ UX500_MSP_INTERNAL_CLOCK_FREQ
> +#else
> +#define UX500_MSP_INTERNAL_CLOCK_FREQ 13000000
> +#define UX500_MSP1_INTERNAL_CLOCK_FREQ (UX500_MSP_INTERNAL_CLOCK_FREQ * 2)
> +#endif

This should probably be coming out of the clock framework.

> +int ux500_msp_dai_set_data_delay(struct snd_soc_dai *dai, int delay);

What is this?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 8562 bytes --]

On Tue, Mar 13, 2012 at 04:11:40PM +0100, Ola Lilja wrote:

> +++ b/sound/soc/codecs/Kconfig
> @@ -12,6 +12,7 @@ config SND_SOC_ALL_CODECS
>  	tristate "Build all ASoC CODEC drivers"
>  	select SND_SOC_88PM860X if MFD_88PM860X
>  	select SND_SOC_L3
> +	select SND_SOC_AB8500
>  	select SND_SOC_AC97_CODEC if SND_SOC_AC97_BUS
>  	select SND_SOC_AD1836 if SPI_MASTER
>  	select SND_SOC_AD193X if SND_SOC_I2C_AND_SPI

This driver really has no dependencies?  That seems *extremely*
surprising - have you tried building on x86?

> +
> +ifdef CONFIG_SND_SOC_UX500_DEBUG
> +CFLAGS_ab8500_audio.o := -DDEBUG
> +endif

Once again if you're adding random custom stuff like this for your
driver you shouldn't be.

> diff --git a/sound/soc/codecs/ab8500_audio.c b/sound/soc/codecs/ab8500_audio.c

Everything else uses -codec.

> +/* AB8500 register cache */
> +static const u8 ab8500_reg_cache[AB8500_CACHEREGNUM];

> +static enum sid_state sid_status = SID_UNCONFIGURED;

No static data or custom cache implementations.

> +/* Read a register from the audio-bank of AB8500 */
> +static unsigned int ab8500_codec_read_reg(struct snd_soc_codec *codec, unsigned int reg)
> +{
> +	int status;
> +	unsigned int value = 0;
> +
> +	if (reg < VIRT_REG_BASE) {

Why are you doing this virtual register stuff?  It's making the code a
lot more complex and doesn't look at all driver specific.

> +		} else {
> +			dev_dbg(codec->dev, "%s: Read 0x%02x from register 0x%02x:0x%02x\n",
> +				__func__, value8, (u8)AB8500_AUDIO, (u8)reg);

There's already *plenty* of trace for register I/O in the framework.

> +static const char * const enum_ena_dis[] = {"Enabled", "Disabled"};
> +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"};

Why are the controls using these enums and not Switch controls?  UIs
know how to display switches.

> +/* Earpiece - Mute */
> +static const struct snd_kcontrol_new dapm_ear_mute[] = {
> +	SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF, REG_MUTECONF_MUTEAR, 1, 1),
> +};

This looks like it's just a mute rather than a mixer input enable
(there's only one control...) so should be a regular sound control;
unless there's a very good reason mutes other than mixer inputs normally
don't affect the audio path as you might get pops and you probably don't
want to do things like stop clocking just because the output is
silenced.  Similar thing applies in several other places.

> +/* Earpiece source selector */
> +static const char * const enum_ear_lineout_source[] = {"Headset Left", "IHF Left"};
> +static SOC_ENUM_SINGLE_DECL(dapm_enum_ear_lineout_source, REG_DMICFILTCONF,
> +			REG_DMICFILTCONF_DA3TOEAR, enum_ear_lineout_source);
> +static const struct snd_kcontrol_new dapm_ear_lineout_source =
> +	SOC_DAPM_ENUM("Earpiece or LineOut Mono Source", dapm_enum_ear_lineout_source);

This control can probably have a much better name... 

> +/* IHF - Enable/Disable */
> +static const struct soc_enum enum_ihf_left = SOC_ENUM_SINGLE(0, 0, 2, enum_dis_ena);

80 columns please.

> +/* from -31 to 31 dB in 1 dB steps (mute instead of -32 dB) */
> +static DECLARE_TLV_DB_SCALE(adx_dig_gain_tlv, -3200, 100, 1);

I'm really not a big fan of this sort of comment, adds greatly to the
verbosity but just duplicates the code.

> +static const char * const enum_earselcm[] = {"0.95V", "1.10V", "1.27V", "1.58V"};
> +static SOC_ENUM_SINGLE_DECL(soc_enum_earselcm,
> +	REG_ANACONF1, REG_ANACONF1_EARSELCM, enum_earselcm);

Should this really be configured by the application layer and not
platform data?

> +/* Digital interface - DA from slot mapping */
> +static const char * const enum_da_from_slot_map[] = {"SLOT0",
> +					"SLOT1",

Indentation.  Also are you sure that this isn't DAPM stuff?

> +	SOC_ENUM("Earpiece DAC Low Power Playback Switch",
> +		soc_enum_eardaclowpow),

These aren't switches, they're enums.  Switches are strictly boolean
things with values 0 and 1.

> +	snd_soc_update_bits(codec,
> +			REG_DIGIFCONF3,
> +			BMASK(REG_DIGIFCONF3_IF1MASTER),
> +			BMASK(REG_DIGIFCONF3_IF1MASTER));

You're unconditionally setting these bits?

> +	snd_soc_update_bits(codec, REG_DIGIFCONF4, clr_mask, set_mask);

Either clr_mask and set_mask are misnamed or this doesn't do what you
think it does.  The bits specified by the third argument are set to the
values given in the fourth argument.  It looks like the code thinks that
the bits in the third argument will be cleared and the bits in the
fourth argument will be set.

> +static int ab8500_codec_set_word_length_if1(struct snd_soc_codec *codec, unsigned int wl)
> +{

Just inline stuff like this.

> +static int ab8500_codec_set_bit_delay_if1(struct snd_soc_codec *codec, unsigned int delay)

This too.

> +/* Configures audio macrocell into the AB8500 Chip */
> +static int ab8500_codec_configure_audio_macrocell(struct snd_soc_codec *codec)
> +{
> +	u8 value8;
> +	unsigned int value;
> +	int status;
> +
> +	status = ab8500_sysctrl_write(AB8500_STW4500CTRL3,
> +		AB8500_STW4500CTRL3_CLK32KOUT2DIS | AB8500_STW4500CTRL3_RESETAUDN,
> +		AB8500_STW4500CTRL3_RESETAUDN);
> +	if (status < 0)
> +		return status;

You're just unconditionally setting these values - shouldn't they be
platform data?

> +static int ab8500_codec_add_widgets(struct snd_soc_codec *codec)
> +{
> +	int status;
> +
> +	status = snd_soc_dapm_new_controls(&codec->dapm, ab8500_dapm_widgets,
> +			ARRAY_SIZE(ab8500_dapm_widgets));

Just point at the tables from the snd_soc_codec and remove all this
code.

> +static int ab8500_codec_pcm_hw_params(struct snd_pcm_substream *substream,
> +		struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai)
> +{
> +	dev_dbg(dai->codec->dev, "%s Enter.\n", __func__);
> +
> +	return 0;
> +}

Remove all empty functions.
> +
> +struct snd_soc_dai_driver ab8500_codec_dai[] = {
> +	{
> +		.name = "ab8500-codec-dai.0",
> +		.id = 0,
> +		.playback = {
> +			.stream_name = "ab8500_0p",

You should hook up the links to the widgets using DAPM routes, currently
you're using the implicitly created routes from the stream name which is
still supported but isn't best practice.

> +static int ab8500_codec_remove(struct snd_soc_codec *codec)
> +{
> +	dev_dbg(codec->dev, "%s Enter.\n", __func__);
> +
> +	snd_soc_dapm_free(&codec->dapm);

Why are you doing this?

> +	/* Create register cache */
> +	ab8500_codec_driver.reg_cache_size = AB8500_LAST_REG - AB8500_FIRST_REG;
> +	ab8500_codec_driver.reg_cache_default =
> +		kzalloc(ab8500_codec_driver.reg_cache_size * sizeof(u8), GFP_KERNEL);

The driver struct should be constant, the stuff you're initialising it
with here is compile time constant - why are you doing this dynamically
at runtime/

> +
> +	/* Create driver private-data struct */
> +	drvdata = kzalloc(sizeof(struct ab8500_codec_drvdata), GFP_KERNEL);

Anything you do alloc should be devm_kzalloc().

> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
> +
> +	dev_info(&pdev->dev, "%s Enter.\n", __func__);
> +
> +	kfree(ab8500_codec_driver.reg_cache_default);
> +
> +	kfree(drvdata->virt_reg_cache);
> +	kfree(drvdata);
> +
> +	snd_soc_unregister_codec(&pdev->dev);

This would avoid issues like this where you free the data the device
uses before you free the device.

> +static int __devinit ab8500_codec_platform_driver_init(void)

module_platform_driver.

> +/* Extended interface for codec-driver */
> +
> +int ab8500_audio_power_control(struct snd_soc_codec *codec, bool power_on);
> +int ab8500_audio_set_word_length(struct snd_soc_dai *dai, unsigned int wl);
> +int ab8500_audio_set_bit_delay(struct snd_soc_dai *dai, unsigned int delay);
> +int ab8500_audio_setup_if1(struct snd_soc_codec *codec,
> +			unsigned int fmt,
> +			unsigned int wl,
> +			unsigned int delay);
> +void ab8500_audio_anc_configure(struct snd_soc_codec *codec,
> +			bool apply_fir, bool apply_iir);

No, you shouldn't be exporting this stuff - it all looks like basic
framework stuff.

> +/* Virtual register base
> + * This address should be located above any physical register used by ASoC.
> + * It is used to map virtual registers needed by the codec-driver.
> + */
> +#define VIRT_REG_BASE				0x3000

Why are you doing this virtual register stuff?

> +#define REG_DAPATHCONF_ENDACHSL			5

All these constants need to be driver local or namespaced (preferrably
namespaced whatever).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Ola LILJA2]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: den 13 mars 2012 23:45
> To: Ola LILJA2
> Cc: Liam Girdwood; alsa-devel@alsa-project.org; Linus Walleij
> Subject: Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver
> 
> On Tue, Mar 13, 2012 at 04:11:40PM +0100, Ola Lilja wrote:
> 
> > +++ b/sound/soc/codecs/Kconfig
> > @@ -12,6 +12,7 @@ config SND_SOC_ALL_CODECS
> >  	tristate "Build all ASoC CODEC drivers"
> >  	select SND_SOC_88PM860X if MFD_88PM860X
> >  	select SND_SOC_L3
> > +	select SND_SOC_AB8500
> >  	select SND_SOC_AC97_CODEC if SND_SOC_AC97_BUS
> >  	select SND_SOC_AD1836 if SPI_MASTER
> >  	select SND_SOC_AD193X if SND_SOC_I2C_AND_SPI
> 
> This driver really has no dependencies?  That seems *extremely*
> surprising - have you tried building on x86?

I'll add dependencies, just missed that one.

> 
> > +
> > +ifdef CONFIG_SND_SOC_UX500_DEBUG
> > +CFLAGS_ab8500_audio.o := -DDEBUG
> > +endif
> 
> Once again if you're adding random custom stuff like this for your
> driver you shouldn't be.

I'll removed it for next patch-set.

> 
> > diff --git a/sound/soc/codecs/ab8500_audio.c
> > b/sound/soc/codecs/ab8500_audio.c
> 
> Everything else uses -codec.

Could just see one codec named with "-codec". The reason for appending _audio
Is that the AB8500 is not only audio and we already have files elsewhere named
ab8500.c but I probably could rename it to ab8500-codec.c.

> 
> > +/* AB8500 register cache */
> > +static const u8 ab8500_reg_cache[AB8500_CACHEREGNUM];
> 
> > +static enum sid_state sid_status = SID_UNCONFIGURED;
> 
> No static data or custom cache implementations.

OK, I'll move this into the private data-struct, too.

> 
> > +/* Read a register from the audio-bank of AB8500 */ static unsigned
> > +int ab8500_codec_read_reg(struct snd_soc_codec *codec, unsigned int
> > +reg) {
> > +	int status;
> > +	unsigned int value = 0;
> > +
> > +	if (reg < VIRT_REG_BASE) {
> 
> Why are you doing this virtual register stuff?  It's making the code a
> lot more complex and doesn't look at all driver specific.

There is a chain of events leading up to the decision of having it like this.
The HW is designed so that only one coefficient-register is present and when
we write a value to it the HW increases a HW-index so that the next time we write
to it the value will set the next coefficient and so on.
Also, we don't want to be able to first set all FIR/IIR-coefficients with the ALSA-controls
And when we are happy we write all the coefficients to the HW by committing them 
using the strobe-control.
Furthermore, we cannot read the coefficients from the HW, so to be able to provide this
to userspace we use the SOC_HWDEP_MULTIPLE_1R, and since these values actually is not exposed
as separate registers we use a set of virtual register. When we commit the coefficients
we then take the values from the virtual registers and write them down to the HW in a loop.

> 
> > +		} else {
> > +			dev_dbg(codec->dev, "%s: Read 0x%02x
> from register 0x%02x:0x%02x\n",
> > +				__func__, value8,
> (u8)AB8500_AUDIO, (u8)reg);
> 
> There's already *plenty* of trace for register I/O in the framework.
> 
> > +static const char * const enum_ena_dis[] = {"Enabled", "Disabled"};
> > +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"};
> 
> Why are the controls using these enums and not Switch controls?  UIs
> know how to display switches.

We need to make the switches virtual, as we don't want them to actually set any bits,
just break or complete a DAPM-chain. That is why we made the as MUX:es.

> 
> > +/* Earpiece - Mute */
> > +static const struct snd_kcontrol_new dapm_ear_mute[] = {
> > +	SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF,
> > +REG_MUTECONF_MUTEAR, 1, 1), };
> 
> This looks like it's just a mute rather than a mixer input enable
> (there's only one control...) so should be a regular sound control;
> unless there's a very good reason mutes other than mixer inputs
> normally don't affect the audio path as you might get pops and you
> probably don't want to do things like stop clocking just because the
> output is silenced.  Similar thing applies in several other places.

Most of the mutes need to be apart of the DAPM-chain to actually prevent
click-n-pops. So it cannot be used by the user as a normal ALSA-control.
Muting can be done by setting certain gains to -inf.

> 
> > +/* Earpiece source selector */
> > +static const char * const enum_ear_lineout_source[] = {"Headset
> > +Left", "IHF Left"}; static
> SOC_ENUM_SINGLE_DECL(dapm_enum_ear_lineout_source, REG_DMICFILTCONF,
> > +			REG_DMICFILTCONF_DA3TOEAR,
> enum_ear_lineout_source); static const
> > +struct snd_kcontrol_new dapm_ear_lineout_source =
> > +	SOC_DAPM_ENUM("Earpiece or LineOut Mono Source",
> > +dapm_enum_ear_lineout_source);
> 
> This control can probably have a much better name...
> 
> > +/* IHF - Enable/Disable */
> > +static const struct soc_enum enum_ihf_left = SOC_ENUM_SINGLE(0, 0,
> 2,
> > +enum_dis_ena);
> 
> 80 columns please.

Ugh...

> 
> > +/* from -31 to 31 dB in 1 dB steps (mute instead of -32 dB) */
> static
> > +DECLARE_TLV_DB_SCALE(adx_dig_gain_tlv, -3200, 100, 1);
> 
> I'm really not a big fan of this sort of comment, adds greatly to the
> verbosity but just duplicates the code.

OK.

> 
> > +static const char * const enum_earselcm[] = {"0.95V", "1.10V",
> > +"1.27V", "1.58V"}; static SOC_ENUM_SINGLE_DECL(soc_enum_earselcm,
> > +	REG_ANACONF1, REG_ANACONF1_EARSELCM, enum_earselcm);
> 
> Should this really be configured by the application layer and not
> platform data?

We might be able to move this from user control.

> 
> > +/* Digital interface - DA from slot mapping */ static const char *
> > +const enum_da_from_slot_map[] = {"SLOT0",
> > +					"SLOT1",
> 
> Indentation.  Also are you sure that this isn't DAPM stuff?
> 
> > +	SOC_ENUM("Earpiece DAC Low Power Playback Switch",
> > +		soc_enum_eardaclowpow),
> 
> These aren't switches, they're enums.  Switches are strictly boolean
> things with values 0 and 1.

OK, I'll remove the Playback Switch postfix.

> 
> > +	snd_soc_update_bits(codec,
> > +			REG_DIGIFCONF3,
> > +			BMASK(REG_DIGIFCONF3_IF1MASTER),
> > +			BMASK(REG_DIGIFCONF3_IF1MASTER));
> 
> You're unconditionally setting these bits?

The IF1 is hardwired to a chip only running as slave, but I'll fix it so that
it actually reflect the fmt set from the machine-driver.

> 
> > +	snd_soc_update_bits(codec, REG_DIGIFCONF4, clr_mask,
> set_mask);
> 
> Either clr_mask and set_mask are misnamed or this doesn't do what you
> think it does.  The bits specified by the third argument are set to the
> values given in the fourth argument.  It looks like the code thinks
> that the bits in the third argument will be cleared and the bits in the
> fourth argument will be set.

When we removed the caching-functions from last patch-set, we replaced our
previous update-function with snd_soc_update_bits. Our function did exactly
one clear and then one set. In most of our cases this will result in the
same result as first masking the third argument, setting the fourth argument.
So, I will rename the variables to better match the arguments of snd_soc_update_bits.

> 
> > +static int ab8500_codec_set_word_length_if1(struct snd_soc_codec
> > +*codec, unsigned int wl) {
> 
> Just inline stuff like this.

OK.

> 
> > +static int ab8500_codec_set_bit_delay_if1(struct snd_soc_codec
> > +*codec, unsigned int delay)
> 
> This too.

OK.

> 
> > +/* Configures audio macrocell into the AB8500 Chip */ static int
> > +ab8500_codec_configure_audio_macrocell(struct snd_soc_codec *codec)
> {
> > +	u8 value8;
> > +	unsigned int value;
> > +	int status;
> > +
> > +	status = ab8500_sysctrl_write(AB8500_STW4500CTRL3,
> > +		AB8500_STW4500CTRL3_CLK32KOUT2DIS |
> AB8500_STW4500CTRL3_RESETAUDN,
> > +		AB8500_STW4500CTRL3_RESETAUDN);
> > +	if (status < 0)
> > +		return status;
> 
> You're just unconditionally setting these values - shouldn't they be
> platform data?
> 
> > +static int ab8500_codec_add_widgets(struct snd_soc_codec *codec) {
> > +	int status;
> > +
> > +	status = snd_soc_dapm_new_controls(&codec->dapm,
> ab8500_dapm_widgets,
> > +			ARRAY_SIZE(ab8500_dapm_widgets));
> 
> Just point at the tables from the snd_soc_codec and remove all this
> code.

OK.

> 
> > +static int ab8500_codec_pcm_hw_params(struct snd_pcm_substream
> *substream,
> > +		struct snd_pcm_hw_params *hw_params, struct
> snd_soc_dai *dai) {
> > +	dev_dbg(dai->codec->dev, "%s Enter.\n", __func__);
> > +
> > +	return 0;
> > +}
> 
> Remove all empty functions.

OK.

> > +
> > +struct snd_soc_dai_driver ab8500_codec_dai[] = {
> > +	{
> > +		.name = "ab8500-codec-dai.0",
> > +		.id = 0,
> > +		.playback = {
> > +			.stream_name = "ab8500_0p",
> 
> You should hook up the links to the widgets using DAPM routes,
> currently you're using the implicitly created routes from the stream
> name which is still supported but isn't best practice.
> 
> > +static int ab8500_codec_remove(struct snd_soc_codec *codec) {
> > +	dev_dbg(codec->dev, "%s Enter.\n", __func__);
> > +
> > +	snd_soc_dapm_free(&codec->dapm);
> 
> Why are you doing this?
> 
> > +	/* Create register cache */
> > +	ab8500_codec_driver.reg_cache_size = AB8500_LAST_REG -
> AB8500_FIRST_REG;
> > +	ab8500_codec_driver.reg_cache_default =
> > +		kzalloc(ab8500_codec_driver.reg_cache_size *
> sizeof(u8),
> > +GFP_KERNEL);
> 
> The driver struct should be constant, the stuff you're initialising it
> with here is compile time constant - why are you doing this dynamically
> at runtime/
> 
> > +
> > +	/* Create driver private-data struct */
> > +	drvdata = kzalloc(sizeof(struct ab8500_codec_drvdata),
> GFP_KERNEL);
> 
> Anything you do alloc should be devm_kzalloc().

OK.

> 
> > +	struct ab8500_codec_drvdata *drvdata =
> dev_get_drvdata(&pdev->dev);
> > +
> > +	dev_info(&pdev->dev, "%s Enter.\n", __func__);
> > +
> > +	kfree(ab8500_codec_driver.reg_cache_default);
> > +
> > +	kfree(drvdata->virt_reg_cache);
> > +	kfree(drvdata);
> > +
> > +	snd_soc_unregister_codec(&pdev->dev);
> 
> This would avoid issues like this where you free the data the device
> uses before you free the device.
> 
> > +static int __devinit ab8500_codec_platform_driver_init(void)
> 
> module_platform_driver.

OK.

> 
> > +/* Extended interface for codec-driver */
> > +
> > +int ab8500_audio_power_control(struct snd_soc_codec *codec, bool
> > +power_on); int ab8500_audio_set_word_length(struct snd_soc_dai *dai,
> > +unsigned int wl); int ab8500_audio_set_bit_delay(struct snd_soc_dai
> > +*dai, unsigned int delay); int ab8500_audio_setup_if1(struct
> snd_soc_codec *codec,
> > +			unsigned int fmt,
> > +			unsigned int wl,
> > +			unsigned int delay);
> > +void ab8500_audio_anc_configure(struct snd_soc_codec *codec,
> > +			bool apply_fir, bool apply_iir);
> 
> No, you shouldn't be exporting this stuff - it all looks like basic
> framework stuff.

Hmm.. ok... the power_control is needed for reasons explained before (vibra-driver and
Accessory-detection-magic), but I guess I have to remove these features for now and
I can then remove this export.

> 
> > +/* Virtual register base
> > + * This address should be located above any physical register used
> by ASoC.
> > + * It is used to map virtual registers needed by the codec-driver.
> > + */
> > +#define VIRT_REG_BASE				0x3000
> 
> Why are you doing this virtual register stuff?

See previous answer.

> 
> > +#define REG_DAPATHCONF_ENDACHSL			5
> 
> All these constants need to be driver local or namespaced (preferrably
> namespaced whatever).

OK, will add some suitable prefix instead of REG_.

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 4331 bytes --]

On Wed, Mar 14, 2012 at 02:27:13PM +0100, Ola LILJA2 wrote:

Fix your mailer to word wrap properly, I've reflowed your text for
legibility.

> > > diff --git a/sound/soc/codecs/ab8500_audio.c
> > > b/sound/soc/codecs/ab8500_audio.c

> > Everything else uses -codec.

> Could just see one codec named with "-codec". The reason for appending _audio
> Is that the AB8500 is not only audio and we already have files elsewhere named
> ab8500.c but I probably could rename it to ab8500-codec.c.

The drivers for MFDs pretty much all call themselves -codec internally
(as yours does).

> > Why are you doing this virtual register stuff?  It's making the code a
> > lot more complex and doesn't look at all driver specific.

> There is a chain of events leading up to the decision of having it
> like this.  The HW is designed so that only one coefficient-register
> is present and when we write a value to it the HW increases a HW-index
> so that the next time we write to it the value will set the next
> coefficient and so on.  Also, we don't want to be able to first set
> all FIR/IIR-coefficients with the ALSA-controls And when we are happy
> we write all the coefficients to the HW by committing them using the
> strobe-control.  Furthermore, we cannot read the coefficients from the
> HW, so to be able to provide this to userspace we use the
> SOC_HWDEP_MULTIPLE_1R, and since these values actually is not exposed
> as separate registers we use a set of virtual register. When we commit
> the coefficients we then take the values from the virtual registers
> and write them down to the HW in a loop.

Don't do this, this just makes things a lot more complicated in the
register I/O code and making it much more difficult to work with both
locally and from a framework point of view.  Store the coefficients in
your driver data rather than shoehorning them into the register cache.

> > > +static const char * const enum_ena_dis[] = {"Enabled", "Disabled"};
> > > +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"};

> > Why are the controls using these enums and not Switch controls?  UIs
> > know how to display switches.

> We need to make the switches virtual, as we don't want them to
> actually set any bits, just break or complete a DAPM-chain. That is
> why we made the as MUX:es.

This is completely orthogonal to how the controls are displayed to
userspace, it's an implementation detail of your driver.  Though if your
routing control doesn't actually touch the device one has to wonder what
it actually does...

> > > +/* Earpiece - Mute */
> > > +static const struct snd_kcontrol_new dapm_ear_mute[] = {
> > > +	SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF,
> > > +REG_MUTECONF_MUTEAR, 1, 1), };

> > This looks like it's just a mute rather than a mixer input enable
> > (there's only one control...) so should be a regular sound control;
> > unless there's a very good reason mutes other than mixer inputs
> > normally don't affect the audio path as you might get pops and you
> > probably don't want to do things like stop clocking just because the
> > output is silenced.  Similar thing applies in several other places.

> Most of the mutes need to be apart of the DAPM-chain to actually prevent
> click-n-pops. So it cannot be used by the user as a normal ALSA-control.
> Muting can be done by setting certain gains to -inf.

This explanation doesn't correspond to what you've actually written -
the code above will result in a user visible control.

> > > +			unsigned int fmt,
> > > +			unsigned int wl,
> > > +			unsigned int delay);
> > > +void ab8500_audio_anc_configure(struct snd_soc_codec *codec,
> > > +			bool apply_fir, bool apply_iir);

> > No, you shouldn't be exporting this stuff - it all looks like basic
> > framework stuff.

> Hmm.. ok... the power_control is needed for reasons explained before
> (vibra-driver and Accessory-detection-magic), but I guess I have to
> remove these features for now and I can then remove this export.

You've not mentioned accessory detection before...  there's certainly
no obvious excuse for doing the power management for accessory detection
outside of DAPM, we've got a bunch of drivers in mainline already which
manage to do this quite successfully, but since you've not explained
what the issue you think you see is it's hard to comment.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 03/14/2012 02:45 PM, Mark Brown wrote:
> On Wed, Mar 14, 2012 at 02:27:13PM +0100, Ola LILJA2 wrote:
>
> Fix your mailer to word wrap properly, I've reflowed your text for
> legibility.
>
>>>> diff --git a/sound/soc/codecs/ab8500_audio.c
>>>> b/sound/soc/codecs/ab8500_audio.c
>>> Everything else uses -codec.
>> Could just see one codec named with "-codec". The reason for appending _audio
>> Is that the AB8500 is not only audio and we already have files elsewhere named
>> ab8500.c but I probably could rename it to ab8500-codec.c.
> The drivers for MFDs pretty much all call themselves -codec internally
> (as yours does).
OK, I thought you were comparing the namning of our file with the naming of
other codec-files in the /soc/codecs-folder.
But I guess that you are comparing the filename of our codec to the string we
user for the device in the code. I'll rename it to ab8500-codec.c
>>> Why are you doing this virtual register stuff?  It's making the code a
>>> lot more complex and doesn't look at all driver specific.
>> There is a chain of events leading up to the decision of having it
>> like this.  The HW is designed so that only one coefficient-register
>> is present and when we write a value to it the HW increases a HW-index
>> so that the next time we write to it the value will set the next
>> coefficient and so on.  Also, we don't want to be able to first set
>> all FIR/IIR-coefficients with the ALSA-controls And when we are happy
>> we write all the coefficients to the HW by committing them using the
>> strobe-control.  Furthermore, we cannot read the coefficients from the
>> HW, so to be able to provide this to userspace we use the
>> SOC_HWDEP_MULTIPLE_1R, and since these values actually is not exposed
>> as separate registers we use a set of virtual register. When we commit
>> the coefficients we then take the values from the virtual registers
>> and write them down to the HW in a loop.
> Don't do this, this just makes things a lot more complicated in the
> register I/O code and making it much more difficult to work with both
> locally and from a framework point of view.  Store the coefficients in
> your driver data rather than shoehorning them into the register cache.
>
OK...
>>>> +static const char * const enum_ena_dis[] = {"Enabled", "Disabled"};
>>>> +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"};
>>> Why are the controls using these enums and not Switch controls?  UIs
>>> know how to display switches.
>> We need to make the switches virtual, as we don't want them to
>> actually set any bits, just break or complete a DAPM-chain. That is
>> why we made the as MUX:es.
> This is completely orthogonal to how the controls are displayed to
> userspace, it's an implementation detail of your driver.  Though if your
> routing control doesn't actually touch the device one has to wonder what
> it actually does...
>
I never found a way to have the playback-switch not touching any bits in the HW,
so we used muxes instead. But if you say that is possible I will look into it
again.
>>>> +/* Earpiece - Mute */
>>>> +static const struct snd_kcontrol_new dapm_ear_mute[] = {
>>>> +	SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF,
>>>> +REG_MUTECONF_MUTEAR, 1, 1), };
>>> This looks like it's just a mute rather than a mixer input enable
>>> (there's only one control...) so should be a regular sound control;
>>> unless there's a very good reason mutes other than mixer inputs
>>> normally don't affect the audio path as you might get pops and you
>>> probably don't want to do things like stop clocking just because the
>>> output is silenced.  Similar thing applies in several other places.
>> Most of the mutes need to be apart of the DAPM-chain to actually prevent
>> click-n-pops. So it cannot be used by the user as a normal ALSA-control.
>> Muting can be done by setting certain gains to -inf.
> This explanation doesn't correspond to what you've actually written -
> the code above will result in a user visible control.
That is why I said "most of", since this one was going to be converted to a
virtual type (SND_SOC_ENUM_VIRT). And then we could use the bit
REG_MUTECONF_MUTEAR in the DAPM-chain without having a userspace control setting
it before the chain is getting executed.
Also, the reason for not just changing control-types directly is because our
customers are affected by these kind of changes so we need to do those changes
at times where we can minimize the damge, have time to handle the effects and
when customers actually accept that we do it (we try to diverge as little as
possible from our main-branch).
>>>> +			unsigned int fmt,
>>>> +			unsigned int wl,
>>>> +			unsigned int delay);
>>>> +void ab8500_audio_anc_configure(struct snd_soc_codec *codec,
>>>> +			bool apply_fir, bool apply_iir);
>>> No, you shouldn't be exporting this stuff - it all looks like basic
>>> framework stuff.
>> Hmm.. ok... the power_control is needed for reasons explained before
>> (vibra-driver and Accessory-detection-magic), but I guess I have to
>> remove these features for now and I can then remove this export.
> You've not mentioned accessory detection before...  there's certainly
> no obvious excuse for doing the power management for accessory detection
> outside of DAPM, we've got a bunch of drivers in mainline already which
> manage to do this quite successfully, but since you've not explained
> what the issue you think you see is it's hard to comment.
Accessory detection is just another external user not able to go through the
user-space interface and due to the fact that the algorithms need to detect
several different headset-types by turning on/off regulators and sampling the
voltage on the input in specific sequences, we found no convenient way to do
this since DAPM was controlling the regulators. We then introduced the _inc and
_dec for the power of the audio-part (ab8500_audio_power_control), so that DAPM
one-sided would not turn of the chip if, for example, vibra is on or accessory
detection is detecting.
We could remove both vibra and acc.det from the driver and put the
regulator-control inside the codec-driver, but we would drift even further from
what we actually use internally at ST-Ericsson and what our customers use.

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 4088 bytes --]

On Thu, Mar 15, 2012 at 03:50:41PM +0100, Ola Lilja wrote:
> On 03/14/2012 02:45 PM, Mark Brown wrote:
> > On Wed, Mar 14, 2012 at 02:27:13PM +0100, Ola LILJA2 wrote:

Please include blank lines between paragraphs in your mail for
legibility.

> > This is completely orthogonal to how the controls are displayed to
> > userspace, it's an implementation detail of your driver.  Though if your
> > routing control doesn't actually touch the device one has to wonder what
> > it actually does...

> I never found a way to have the playback-switch not touching any bits
> in the HW, so we used muxes instead. But if you say that is possible I
> will look into it again.

If the hardware has no control the idiomatic thing would be to have a
pin switch in the machine driver.

> >> Most of the mutes need to be apart of the DAPM-chain to actually prevent
> >> click-n-pops. So it cannot be used by the user as a normal ALSA-control.
> >> Muting can be done by setting certain gains to -inf.

> > This explanation doesn't correspond to what you've actually written -
> > the code above will result in a user visible control.

> That is why I said "most of", since this one was going to be converted to a
> virtual type (SND_SOC_ENUM_VIRT). And then we could use the bit
> REG_MUTECONF_MUTEAR in the DAPM-chain without having a userspace control setting
> it before the chain is getting executed.

> Also, the reason for not just changing control-types directly is because our
> customers are affected by these kind of changes so we need to do those changes
> at times where we can minimize the damge, have time to handle the effects and
> when customers actually accept that we do it (we try to diverge as little as
> possible from our main-branch).

This sounds like a very good reason to go for mainline early.

> >> Hmm.. ok... the power_control is needed for reasons explained before
> >> (vibra-driver and Accessory-detection-magic), but I guess I have to
> >> remove these features for now and I can then remove this export.
> > You've not mentioned accessory detection before...  there's certainly
> > no obvious excuse for doing the power management for accessory detection
> > outside of DAPM, we've got a bunch of drivers in mainline already which
> > manage to do this quite successfully, but since you've not explained
> > what the issue you think you see is it's hard to comment.

> Accessory detection is just another external user not able to go through the
> user-space interface and due to the fact that the algorithms need to detect
> several different headset-types by turning on/off regulators and sampling the
> voltage on the input in specific sequences, we found no convenient way to do
> this since DAPM was controlling the regulators. We then introduced the _inc and
> _dec for the power of the audio-part (ab8500_audio_power_control), so that DAPM
> one-sided would not turn of the chip if, for example, vibra is on or accessory
> detection is detecting.

> We could remove both vibra and acc.det from the driver and put the

Remember that the regulator API is reference counting, multiple things
can control the same regulator without you having to layer another layer
of reference counting and whatever else your code is doing on top of it.
This is pretty essential for the regulator API, after all it's entirely
normal for one regulator to supply many devices.

Remember also that anything that goes into mainline is expected to have
been peer reviewed and might get used as a reference by others.  Things
like this that aren't working with the kernel frameworks but instead
layer on top of them really don't fit well with that.

> regulator-control inside the codec-driver, but we would drift even further from
> what we actually use internally at ST-Ericsson and what our customers use.

Given the drivers you originally posted I expect you're already a
substantial distance away from your internal code anyway...  Perhaps you
can use these issues internally as an example of the problems out of
tree development that ends up with substantial non-framework code.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 03/15/2012 04:29 PM, Mark Brown wrote:
> On Thu, Mar 15, 2012 at 03:50:41PM +0100, Ola Lilja wrote:
> > On 03/14/2012 02:45 PM, Mark Brown wrote:
> > > On Wed, Mar 14, 2012 at 02:27:13PM +0100, Ola LILJA2 wrote:
>
> Please include blank lines between paragraphs in your mail for
> legibility.
>
> > > This is completely orthogonal to how the controls are displayed to
> > > userspace, it's an implementation detail of your driver.  Though if your
> > > routing control doesn't actually touch the device one has to wonder what
> > > it actually does...
>
> > I never found a way to have the playback-switch not touching any bits
> > in the HW, so we used muxes instead. But if you say that is possible I
> > will look into it again.
>
> If the hardware has no control the idiomatic thing would be to have a
> pin switch in the machine driver.

We need to break chains on very specific positions to be able to affect only the
parts intended. We also have switches (e.g. loopback) that is not belonging to a
specific pin but is a brigde-switch in the middle of two chains, thus I don't
think we can use what you suggest.
The only solution I've found is to use the enum_virt, but if there is a
possibility to have a switch_virt I would be able to use that.
 
> > >> Most of the mutes need to be apart of the DAPM-chain to actually prevent
> > >> click-n-pops. So it cannot be used by the user as a normal ALSA-control.
> > >> Muting can be done by setting certain gains to -inf.
>
> > > This explanation doesn't correspond to what you've actually written -
> > > the code above will result in a user visible control.
>
> > That is why I said "most of", since this one was going to be converted to a
> > virtual type (SND_SOC_ENUM_VIRT). And then we could use the bit
> > REG_MUTECONF_MUTEAR in the DAPM-chain without having a userspace control setting
> > it before the chain is getting executed.
>
> > Also, the reason for not just changing control-types directly is because our
> > customers are affected by these kind of changes so we need to do those changes
> > at times where we can minimize the damge, have time to handle the effects and
> > when customers actually accept that we do it (we try to diverge as little as
> > possible from our main-branch).
>
> This sounds like a very good reason to go for mainline early.

Yes, but it doesn't solve our current problems.
(see first comment above regarding the enum_virt-stuff)

> > >> Hmm.. ok... the power_control is needed for reasons explained before
> > >> (vibra-driver and Accessory-detection-magic), but I guess I have to
> > >> remove these features for now and I can then remove this export.
> > > You've not mentioned accessory detection before...  there's certainly
> > > no obvious excuse for doing the power management for accessory detection
> > > outside of DAPM, we've got a bunch of drivers in mainline already which
> > > manage to do this quite successfully, but since you've not explained
> > > what the issue you think you see is it's hard to comment.
>
> > Accessory detection is just another external user not able to go through the
> > user-space interface and due to the fact that the algorithms need to detect
> > several different headset-types by turning on/off regulators and sampling the
> > voltage on the input in specific sequences, we found no convenient way to do
> > this since DAPM was controlling the regulators. We then introduced the _inc and
> > _dec for the power of the audio-part (ab8500_audio_power_control), so that DAPM
> > one-sided would not turn of the chip if, for example, vibra is on or accessory
> > detection is detecting.
>
> > We could remove both vibra and acc.det from the driver and put the
>
> Remember that the regulator API is reference counting, multiple things
> can control the same regulator without you having to layer another layer
> of reference counting and whatever else your code is doing on top of it.
> This is pretty essential for the regulator API, after all it's entirely
> normal for one regulator to supply many devices.

Yes, if it were only regulator (and clock) that was needed in our extra
reference-counted power-functions, it would be fine. But vibra needs to be able
to turn on the power of the audio-part of AB8500 (our codec) before it can use
the vibra-functionality. I could move our the reg/clock and assume that the
vibra-driver does this itself and only have the codec-power-register shared
between the external interface and the DAPM.

> Remember also that anything that goes into mainline is expected to have
> been peer reviewed and might get used as a reference by others.  Things
> like this that aren't working with the kernel frameworks but instead
> layer on top of them really don't fit well with that.
>
> > regulator-control inside the codec-driver, but we would drift even further from
> > what we actually use internally at ST-Ericsson and what our customers use.
>
> Given the drivers you originally posted I expect you're already a
> substantial distance away from your internal code anyway...  Perhaps you
> can use these issues internally as an example of the problems out of
> tree development that ends up with substantial non-framework code.

I will try (again) to foward the opinion that we should make greater efforts
(and earlier) to be in sync with mainline-kernel. However, if we get this driver
mainlined we can use all input to push harder for our next generation of
platforms. This driver is for a project that has nearly finished its execution,
but we would like to mainline this first before we go ahead with new drivers.

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2585 bytes --]

On Fri, Mar 16, 2012 at 02:09:38PM +0100, Ola Lilja wrote:
> On 03/15/2012 04:29 PM, Mark Brown wrote:

Please fix the word wrapping in your mail client...

> > If the hardware has no control the idiomatic thing would be to have a
> > pin switch in the machine driver.

> We need to break chains on very specific positions to be able to
> affect only the parts intended. We also have switches (e.g. loopback)
> that is not belonging to a specific pin but is a brigde-switch in the
> middle of two chains, thus I don't think we can use what you suggest.

But if you have a control that actually does something then surely there
should be some interaction with the chip when it is changed?

> The only solution I've found is to use the enum_virt, but if there is
> a possibility to have a switch_virt I would be able to use that.

One of the nice things about open source code is that it's always
possible to extend the core if required.

> Yes, if it were only regulator (and clock) that was needed in our extra
> reference-counted power-functions, it would be fine. But vibra needs to be able
> to turn on the power of the audio-part of AB8500 (our codec) before it can use
> the vibra-functionality. I could move our the reg/clock and assume that the
> vibra-driver does this itself and only have the codec-power-register shared
> between the external interface and the DAPM.

As I said previously there's other drivers with the same setup which
manage to integrate fairly easily without breaking the device model - do
what they do with something like an MFD or just embed a trivial input
driver in the CODEC for the vibra if it's small enough.

> (and earlier) to be in sync with mainline-kernel. However, if we get this driver
> mainlined we can use all input to push harder for our next generation of
> platforms. This driver is for a project that has nearly finished its execution,
> but we would like to mainline this first before we go ahead with new drivers.

Equally well, if it gets mainline without actually meeting mainline
standards that's sending the wrong message :) 

I'd suggest at least adding regulator support into the CODEC driver so
it's managing its own power when required, there's no reason for
anything external to be doing that.  I expect if you bring things into
line with the device model the code will end up being a lot smaller and
clearer and this won't be needed, it feels like a large part of why
you're having to open code all this stuff is that other bits of the code
stepped outside the frameworks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 03/17/2012 11:31 PM, Mark Brown wrote:
> On Fri, Mar 16, 2012 at 02:09:38PM +0100, Ola Lilja wrote:
> > On 03/15/2012 04:29 PM, Mark Brown wrote:
>
> Please fix the word wrapping in your mail client...

Well, I though I did that when I moved over to Thunderbird and set the
wordwrapping to 80 chars (no float). It would help if you told me exactly what
settings it is you want, or provide a link to a description of what settings are
required.

> > > If the hardware has no control the idiomatic thing would be to have a
> > > pin switch in the machine driver.
>
> > We need to break chains on very specific positions to be able to
> > affect only the parts intended. We also have switches (e.g. loopback)
> > that is not belonging to a specific pin but is a brigde-switch in the
> > middle of two chains, thus I don't think we can use what you suggest.
>
> But if you have a control that actually does something then surely there
> should be some interaction with the chip when it is changed?

No, we don't want the control to do anything with the chip before
playback/capture starts, but rather just indicate that a specific path is
active, and then when the stream starts the chain gets complete and the widgets
then do all interaction with the chip in the order that our HW requires. There
is no bit that we can set prior to the execution of the DAPM-chain, without
introducing clicks.

> > The only solution I've found is to use the enum_virt, but if there is
> > a possibility to have a switch_virt I would be able to use that.
>
> One of the nice things about open source code is that it's always
> possible to extend the core if required.

OK, so do you want us to make it possible to have NOPM for a
playback/capture-switch? Can you confirm that this is OK and that it is possible
to accomplish without destroying any mechanism in how it works today?

> > Yes, if it were only regulator (and clock) that was needed in our extra
> > reference-counted power-functions, it would be fine. But vibra needs to be able
> > to turn on the power of the audio-part of AB8500 (our codec) before it can use
> > the vibra-functionality. I could move our the reg/clock and assume that the
> > vibra-driver does this itself and only have the codec-power-register shared
> > between the external interface and the DAPM.
>
> As I said previously there's other drivers with the same setup which
> manage to integrate fairly easily without breaking the device model - do
> what they do with something like an MFD or just embed a trivial input
> driver in the CODEC for the vibra if it's small enough.

So, what you want is another device/driver pair where our current
acc.det.-driver adds a driver to the device we add in the ASoC-driver?
We are thinking of modifying so that we put the Android-vibra-functionality in
userspace, and then it can use vibra through the controls we provide in the
ASoC-driver.

> > (and earlier) to be in sync with mainline-kernel. However, if we get this driver
> > mainlined we can use all input to push harder for our next generation of
> > platforms. This driver is for a project that has nearly finished its execution,
> > but we would like to mainline this first before we go ahead with new drivers.
>
> Equally well, if it gets mainline without actually meeting mainline
> standards that's sending the wrong message :) 
>
> I'd suggest at least adding regulator support into the CODEC driver so
> it's managing its own power when required, there's no reason for
> anything external to be doing that.  I expect if you bring things into
> line with the device model the code will end up being a lot smaller and
> clearer and this won't be needed, it feels like a large part of why
> you're having to open code all this stuff is that other bits of the code
> stepped outside the frameworks.

Regarding the regulators, I was under the impression that everything outside the
actual codec, we could put in the machine-driver (which I think looks pretty good).
This would make a clear distinction between all codec-register-handling and
usage of those external frameworks (regulators and clocks). I have started to
move the code location of regulators/clocks into the codec-file and this results
in that the machine-driver is basically empty of functionality and the
codec-file is growing even bigger. Is this really what we want?
I'm also not sure if you have seen that we actually do control ALL regulators
and clocks via DAPM. The main power-regulator is controlled through a
DAPM-widget which is defined in the machine-driver and connected to the chains
in the codec-driver. The event in the widget will then call the
_inc/_dec-functions in the machine-driver. All other regulators (analog/digital
mic-bias) are also part of the DAPM-chains and turned on/off by the framework
just as it should.
You are right that a large part of this code is there because it is needed by
external requirements, but it does not mean that DAPM is not controlling the
regulators and clocks, just that there is another interface that can also turn
on/off the power.

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Linus Walleij]

On Mon, Mar 19, 2012 at 9:07 AM, Ola Lilja <ola.o.lilja@stericsson.com> wrote:
> Mark Brown wrote:
>> Please fix the word wrapping in your mail client...
>
> Well, I though I did that when I moved over to Thunderbird and set the
> wordwrapping to 80 chars (no float). It would help if you told me exactly what
> settings it is you want, or provide a link to a description of what settings are
> required.

An eternal pain these mailers...

Documentation/email-clients.txt

has a HOWTO for thunderbird which is hopefully uptodate.

Yours,
Linus Walleij

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 4297 bytes --]

On Mon, Mar 19, 2012 at 09:07:16AM +0100, Ola Lilja wrote:
> On 03/17/2012 11:31 PM, Mark Brown wrote:

> > > We need to break chains on very specific positions to be able to
> > > affect only the parts intended. We also have switches (e.g. loopback)
> > > that is not belonging to a specific pin but is a brigde-switch in the
> > > middle of two chains, thus I don't think we can use what you suggest.

> > But if you have a control that actually does something then surely there
> > should be some interaction with the chip when it is changed?

> No, we don't want the control to do anything with the chip before
> playback/capture starts, but rather just indicate that a specific path
> is active, and then when the stream starts the chain gets complete and
> the widgets then do all interaction with the chip in the order that
> our HW requires. There is no bit that we can set prior to the
> execution of the DAPM-chain, without introducing clicks.

In the case of things that go external pins then I'd really expect the
existing solution with pin switches to work, in other cases something
else may be needed but we need to understand what you're trying to do.
The issues with internal paths may need something but please be more
concrete about what the actual register controls are and why you're
doing such unusual things.  The fact that you're using isolated switches
and there's no mixers involved is really surprisng.

> > One of the nice things about open source code is that it's always
> > possible to extend the core if required.

> OK, so do you want us to make it possible to have NOPM for a
> playback/capture-switch? Can you confirm that this is OK and that it
> is possible to accomplish without destroying any mechanism in how it
> works today?

I'm saying that if you're doing things within your driver that go trough
contortions or break the userspace interfaces because the frameworks
didn't support things then you should be fixing the frameworks rather
than doing whatever in the driver.  This sort of thing seems to be at
the root of several of the problems I'm seeing here, it's similar to all
the stuff you're doing with regulators.

> So, what you want is another device/driver pair where our current
> acc.det.-driver adds a driver to the device we add in the ASoC-driver?

I can't parse what you're saying here, sorry.

> We are thinking of modifying so that we put the Android-vibra-functionality in
> userspace, and then it can use vibra through the controls we provide in the
> ASoC-driver.

That'd obviously remove any kernel level problems.

> Regarding the regulators, I was under the impression that everything
> outside the actual codec, we could put in the machine-driver (which I
> think looks pretty good).  This would make a clear distinction between
> all codec-register-handling and usage of those external frameworks
> (regulators and clocks). I have started to move the code location of

No.  That's exactly the sort of stuff we don't want to see.  The fact
that the device needs power to operate isn't something that's specific
to a particular board, it's a property of the silicon.

> regulators/clocks into the codec-file and this results
> in that the machine-driver is basically empty of functionality and the
> codec-file is growing even bigger. Is this really what we want?

Yes, of course!  You need to get away from the idea that the only board
your device will ever be used on is the reference board, things that are
generic to the device should be supported by the device driver for the
device.  If there's a device driver for the part we shouldn't be in the
situation where any new system using the device needs to replicate basic
functionality needed to get the device working.

> I'm also not sure if you have seen that we actually do control ALL regulators
> and clocks via DAPM. The main power-regulator is controlled through a

I've not suggested that.  Look at how other drivers manage their power.

> You are right that a large part of this code is there because it is needed by
> external requirements, but it does not mean that DAPM is not controlling the
> regulators and clocks, just that there is another interface that can also turn
> on/off the power.

I'm not seeing any code in the driver which manages the regulators...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 03/19/2012 01:09 PM, Mark Brown wrote:

> On Mon, Mar 19, 2012 at 09:07:16AM +0100, Ola Lilja wrote:
>> On 03/17/2012 11:31 PM, Mark Brown wrote:
> 
>> > > We need to break chains on very specific positions to be able to
>> > > affect only the parts intended. We also have switches (e.g. loopback)
>> > > that is not belonging to a specific pin but is a brigde-switch in the
>> > > middle of two chains, thus I don't think we can use what you suggest.
> 
>> > But if you have a control that actually does something then surely there
>> > should be some interaction with the chip when it is changed?
> 
>> No, we don't want the control to do anything with the chip before
>> playback/capture starts, but rather just indicate that a specific path
>> is active, and then when the stream starts the chain gets complete and
>> the widgets then do all interaction with the chip in the order that
>> our HW requires. There is no bit that we can set prior to the
>> execution of the DAPM-chain, without introducing clicks.
> 
> In the case of things that go external pins then I'd really expect the
> existing solution with pin switches to work, in other cases something
> else may be needed but we need to understand what you're trying to do.
> The issues with internal paths may need something but please be more
> concrete about what the actual register controls are and why you're
> doing such unusual things.  The fact that you're using isolated switches
> and there's no mixers involved is really surprisng.


I'll try to give some more details with two scenarios below.

Scenario 1:

We have a routing situation that could be rendered something like this:

                            D----->Speaker
                            |
I2S (playback)------>A----->B----->C----->Headset

In this case, let's say that the bit at B needs to be set in HW before C is set
to avoid pop/click noises.
If we just use a normal playback-switch that is associated with the bit at C,
then this switch would directly set the C bit before the stream is active and
then later when the stream is activated, the other widgets are enabled thus B
would be set (i.e. after C was set which is not the desired order).
Our solution to handle this is to introduce a virtual switch (in the form of an
enum_virt) between B and Headset.
Please note that moving the playback-switch to B would not work since that would
affect the Speaker-path enabling/disabling this as well which of course is not
desired to be controlled in this particular use-case.


Scenario 2:

We have a switch with purpose of forwarding sound from LineIn to be mixed with
HeadSet-audio coming from the I2S. However, we also (in parallell) have the
possibility to record the data coming from LineIn independently of the HeadSet
routing.

The routing situation could be rendered something like this:

I2S (playback)------>A----->B----->C----->Headset
                            ^
                            |
         LineIn----->D------E----->F----->I2S (capture)

As you can see neither of the endpoints "belongs" to the bypass-path (E->B) and
therefore I can't see how it should be possible to use a simple pin switch here
as it would always affect other paths.

> 
>> > One of the nice things about open source code is that it's always
>> > possible to extend the core if required.
> 
>> OK, so do you want us to make it possible to have NOPM for a
>> playback/capture-switch? Can you confirm that this is OK and that it
>> is possible to accomplish without destroying any mechanism in how it
>> works today?
> 
> I'm saying that if you're doing things within your driver that go trough
> contortions or break the userspace interfaces because the frameworks
> didn't support things then you should be fixing the frameworks rather
> than doing whatever in the driver.  This sort of thing seems to be at
> the root of several of the problems I'm seeing here, it's similar to all
> the stuff you're doing with regulators.
> 
>> So, what you want is another device/driver pair where our current
>> acc.det.-driver adds a driver to the device we add in the ASoC-driver?
> 
> I can't parse what you're saying here, sorry.


OK, I was trying to understand what you meant with these comments:
"breaking the device model" and "just embed a trivial input
driver in the CODEC for the vibra if it's small enough."
Could you explain this abit more?

> 
>> We are thinking of modifying so that we put the Android-vibra-functionality in
>> userspace, and then it can use vibra through the controls we provide in the
>> ASoC-driver.
> 
> That'd obviously remove any kernel level problems.
> 
>> Regarding the regulators, I was under the impression that everything
>> outside the actual codec, we could put in the machine-driver (which I
>> think looks pretty good).  This would make a clear distinction between
>> all codec-register-handling and usage of those external frameworks
>> (regulators and clocks). I have started to move the code location of
> 
> No.  That's exactly the sort of stuff we don't want to see.  The fact
> that the device needs power to operate isn't something that's specific
> to a particular board, it's a property of the silicon.


I agree that the fact that it needs power is not specific to the board, but we
could have different regulators feeding the codec-chip. The regulators could be
located outside the codec-chip and putting the codec in another board when the
codec-driver requests a specific regulator not present in the new board would
fail. Then we would need to have something like get_regulator("5 volts") to make
it generic between boards having regulators with different names providing the
same voltage.
I'm not into how the regulators work and as the trend is to have the regulator
stuff inside the codec-driver, I'll move it there.
Also, see the last comment below for more info on our regulator-usage.

> 
>> regulators/clocks into the codec-file and this results
>> in that the machine-driver is basically empty of functionality and the
>> codec-file is growing even bigger. Is this really what we want?
> 
> Yes, of course!  You need to get away from the idea that the only board
> your device will ever be used on is the reference board, things that are
> generic to the device should be supported by the device driver for the
> device.  If there's a device driver for the part we shouldn't be in the
> situation where any new system using the device needs to replicate basic
> functionality needed to get the device working.
> 
>> I'm also not sure if you have seen that we actually do control ALL regulators
>> and clocks via DAPM. The main power-regulator is controlled through a
> 
> I've not suggested that.  Look at how other drivers manage their power.


I have looked alot at other drivers, but in many cases I've found that our
situation looks pretty different and often more complex.
See the last comment below for more info on our regulator-usage.

> 
>> You are right that a large part of this code is there because it is needed by
>> external requirements, but it does not mean that DAPM is not controlling the
>> regulators and clocks, just that there is another interface that can also turn
>> on/off the power.
> 
> I'm not seeing any code in the driver which manages the regulators...


This is all our regulator-widgets, currently located in the machine-driver and
connected to chains located in the codec-driver:

	/* Power AB8500 audio-block when AD/DA is active */
	{"DAC", NULL, "AUDIO Regulator"},
	{"ADC", NULL, "AUDIO Regulator"},

	/* Power configured regulator when an analog mic is enabled */
	{"MIC1A Input", NULL, "AMIC1A Regulator"},
	{"MIC1B Input", NULL, "AMIC1B Regulator"},
	{"MIC2 Input", NULL, "AMIC2 Regulator"},

	/* Power DMIC-regulator when any digital mic is enabled */
	{"DMic 1", NULL, "DMIC Regulator"},
	{"DMic 2", NULL, "DMIC Regulator"},
	{"DMic 3", NULL, "DMIC Regulator"},
	{"DMic 4", NULL, "DMIC Regulator"},
	{"DMic 5", NULL, "DMIC Regulator"},
	{"DMic 6", NULL, "DMIC Regulator"},

The regulators are SND_SOC_DAPM_SUPPLY and connected to appropriate widgets
in the codec-driver, e.g.

 "ADC Input" ------------> "DAC"
                             ^
                             |
                      "AUDIO Regulator"

So whenever a stream is active this would lead to the event dapm_audioreg_event
which will call the power_control_inc(w->codec), turning on the regulator, clock
and power-up-register.
This also means that the extra machine-driver interface for acc.det and vibra
can also call the power_control_inc/power_control_dec.

Another example is the mic-regulators, e.g.

"MIC1A Input" --------> "Mic 1A or 1B Select Capture Route" -----> .....
       ^
       |
"AMIC1A Regulator"

Activating a switch completing the Mic1a-path to an output would lead to the
execution of the event in "AMIC1A Regulator": dapm_amic1areg_event. This leads
to regulator_enable, enabling the regulator associated with the Mic1a.

So all paths in the codec-driver leads to an event in the machine-driver
enabling/disabling the regulator.

Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 4481 bytes --]

On Mon, Mar 19, 2012 at 03:54:19PM +0100, Ola Lilja wrote:

> We have a routing situation that could be rendered something like this:
> 
>                             D----->Speaker
>                             |
> I2S (playback)------>A----->B----->C----->Headset

> In this case, let's say that the bit at B needs to be set in HW before C is set
> to avoid pop/click noises.

This looks very standard, you've got a PGA at B and either further PGAs
or output drivers at C and D.

> If we just use a normal playback-switch that is associated with the bit at C,
> then this switch would directly set the C bit before the stream is active and
> then later when the stream is activated, the other widgets are enabled thus B
> would be set (i.e. after C was set which is not the desired order).
> Our solution to handle this is to introduce a virtual switch (in the form of an
> enum_virt) between B and Headset.

It sounds like this non-specific register bit is just the power control
for C in which case it should just be a DAPM widget and machines should
be using a pin switch or jack pins for the outputs.

> I2S (playback)------>A----->B----->C----->Headset
>                             ^
>                             |
>          LineIn----->D------E----->F----->I2S (capture)

> As you can see neither of the endpoints "belongs" to the bypass-path (E->B) and
> therefore I can't see how it should be possible to use a simple pin switch here
> as it would always affect other paths.

Again, this looks *very* standard - it's just a totally normal
bypass/sidetone path as far as I can see.  B is a mixer here and
presumably there's some control on B which turns on and off the path?

Depending on how the path is used you might want to mark the route as a
weak route to suppress power up from that path alone but really as with
the first example this doesn't seem at all unusual unless there's more
going on than your description.

> >> So, what you want is another device/driver pair where our current
> >> acc.det.-driver adds a driver to the device we add in the ASoC-driver?

> > I can't parse what you're saying here, sorry.

> OK, I was trying to understand what you meant with these comments:
> "breaking the device model" and "just embed a trivial input
> driver in the CODEC for the vibra if it's small enough."
> Could you explain this abit more?

The device model is this whole idea that we have devices which are
matched up automatically by the core.  "Embedding" means "putting in" -
see for example wm8962 which has a tiny little input driver in it.

> > No.  That's exactly the sort of stuff we don't want to see.  The fact
> > that the device needs power to operate isn't something that's specific
> > to a particular board, it's a property of the silicon.

> I agree that the fact that it needs power is not specific to the board, but we
> could have different regulators feeding the codec-chip. The regulators could be

You've completely failed to understand how requesting supplies in the
regulator API works, a regulator API like you describe would mean that
no chip driver was ever able to request a supply for itself.  The whole
point of the machine interface is to provide a mechanism for drivers to
talk about their supplies without having to know the details of how
they're wired up on an individual board.

Since your CODEC driver is a driver for a chip your CODEC driver should
be requesting whatever the supplies the chip has.

> > I've not suggested that.  Look at how other drivers manage their power.

> I have looked alot at other drivers, but in many cases I've found that our
> situation looks pretty different and often more complex.
> See the last comment below for more info on our regulator-usage.

I'm sorry but really I don't see anything in the text below which sounds
in the least bit unusual.  Could you be specific about what you think is
special about your device and/or system?

> > I'm not seeing any code in the driver which manages the regulators...

> This is all our regulator-widgets, currently located in the machine-driver and
> connected to chains located in the codec-driver:

Things that are in the machine driver are not in the CODEC driver and
need to be cut and pasted into individual machine drivers.  If it's
something that's genuinely machine specific that's fine but we're
talking about basic core device supplies here.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]