[PATCH v2 0/3] ASoC: sta32x improvements

[PATCH v2 0/3] ASoC: sta32x improvements

[Johannes Stezenbach]

Hi,

this is a rework of the "ASoC: sta32x: add workaround for ESD reset issue"
patch posted on Nov 9.  Since the review of that patch discovered
a general issue with the coefficient RAM, I split the patch into
three parts: the first fixes the coef reload after power up,
the second adds the missing platform configuration (in place of
hard-coded values), and the third has the ESD workaround.

I think I followed all review comments, except for the ESD
workaround where I decided it is much clearer to call 
sta32x_cache_sync() directly instead of messing around
with sta32x_set_bias_level().

The patches apply to
git://opensource.wolfsonmicro.com/linux-2.6-asoc.git for-3.2
and for-3.3.  checkpatch.pl result is clean.

 include/sound/sta32x.h    |   35 +++++++++++
 sound/soc/codecs/sta32x.c |  143 +++++++++++++++++++++++++++++++++++++++++----
 sound/soc/codecs/sta32x.h |    1 +
 3 files changed, 168 insertions(+), 11 deletions(-)


Thanks
Johannes

[PATCH 1/3] ASoC: sta32x: preserve coefficient RAM

[Johannes Stezenbach]

The coefficient RAM must be saved in a shadow so it can
be restored when the codec is powered on using
regulator_bulk_enable().

Cc: Sven Neumann <s.neumann@raumfeld.com>
Cc: Daniel Mack <daniel@zonque.org>
Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
 sound/soc/codecs/sta32x.c |   63 ++++++++++++++++++++++++++++++++++++++++++++-
 sound/soc/codecs/sta32x.h |    1 +
 2 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/sta32x.c b/sound/soc/codecs/sta32x.c
index bb82408..d2f3715 100644
--- a/sound/soc/codecs/sta32x.c
+++ b/sound/soc/codecs/sta32x.c
@@ -76,6 +76,8 @@ struct sta32x_priv {
 
 	unsigned int mclk;
 	unsigned int format;
+
+	u32 coef_shadow[STA32X_COEF_COUNT];
 };
 
 static const DECLARE_TLV_DB_SCALE(mvol_tlv, -12700, 50, 1);
@@ -227,6 +229,7 @@ static int sta32x_coefficient_put(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	struct sta32x_priv *sta32x = snd_soc_codec_get_drvdata(codec);
 	int numcoef = kcontrol->private_value >> 16;
 	int index = kcontrol->private_value & 0xffff;
 	unsigned int cfud;
@@ -239,6 +242,11 @@ static int sta32x_coefficient_put(struct snd_kcontrol *kcontrol,
 	snd_soc_write(codec, STA32X_CFUD, cfud);
 
 	snd_soc_write(codec, STA32X_CFADDR2, index);
+	for (i = 0; i < numcoef && (index + i < STA32X_COEF_COUNT); i++)
+		sta32x->coef_shadow[index + i] =
+			  (ucontrol->value.bytes.data[3 * i] << 16)
+			| (ucontrol->value.bytes.data[3 * i + 1] << 8)
+			| (ucontrol->value.bytes.data[3 * i + 2]);
 	for (i = 0; i < 3 * numcoef; i++)
 		snd_soc_write(codec, STA32X_B1CF1 + i,
 			      ucontrol->value.bytes.data[i]);
@@ -252,6 +260,48 @@ static int sta32x_coefficient_put(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+int sta32x_sync_coef_shadow(struct snd_soc_codec *codec)
+{
+	struct sta32x_priv *sta32x = snd_soc_codec_get_drvdata(codec);
+	unsigned int cfud;
+	int i;
+
+	/* preserve reserved bits in STA32X_CFUD */
+	cfud = snd_soc_read(codec, STA32X_CFUD) & 0xf0;
+
+	for (i = 0; i < STA32X_COEF_COUNT; i++) {
+		snd_soc_write(codec, STA32X_CFADDR2, i);
+		snd_soc_write(codec, STA32X_B1CF1,
+			      (sta32x->coef_shadow[i] >> 16) & 0xff);
+		snd_soc_write(codec, STA32X_B1CF2,
+			      (sta32x->coef_shadow[i] >> 8) & 0xff);
+		snd_soc_write(codec, STA32X_B1CF3,
+			      (sta32x->coef_shadow[i]) & 0xff);
+		/* chip documentation does not say if the bits are
+		 * self-clearing, so do it explicitly */
+		snd_soc_write(codec, STA32X_CFUD, cfud);
+		snd_soc_write(codec, STA32X_CFUD, cfud | 0x01);
+	}
+	return 0;
+}
+
+int sta32x_cache_sync(struct snd_soc_codec *codec)
+{
+	unsigned int mute;
+	int rc;
+
+	if (!codec->cache_sync)
+		return 0;
+
+	/* mute during register sync */
+	mute = snd_soc_read(codec, STA32X_MMUTE);
+	snd_soc_write(codec, STA32X_MMUTE, mute | STA32X_MMUTE_MMUTE);
+	sta32x_sync_coef_shadow(codec);
+	rc = snd_soc_cache_sync(codec);
+	snd_soc_write(codec, STA32X_MMUTE, mute);
+	return rc;
+}
+
 #define SINGLE_COEF(xname, index) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = sta32x_coefficient_info, \
@@ -661,7 +711,7 @@ static int sta32x_set_bias_level(struct snd_soc_codec *codec,
 				return ret;
 			}
 
-			snd_soc_cache_sync(codec);
+			sta32x_cache_sync(codec);
 		}
 
 		/* Power up to mute */
@@ -790,6 +840,17 @@ static int sta32x_probe(struct snd_soc_codec *codec)
 			    STA32X_CxCFG_OM_MASK,
 			    2 << STA32X_CxCFG_OM_SHIFT);
 
+	/* initialize coefficient shadow RAM with reset values */
+	for (i = 4; i <= 49; i += 5)
+		sta32x->coef_shadow[i] = 0x400000;
+	for (i = 50; i <= 54; i++)
+		sta32x->coef_shadow[i] = 0x7fffff;
+	sta32x->coef_shadow[55] = 0x5a9df7;
+	sta32x->coef_shadow[56] = 0x7fffff;
+	sta32x->coef_shadow[59] = 0x7fffff;
+	sta32x->coef_shadow[60] = 0x400000;
+	sta32x->coef_shadow[61] = 0x400000;
+
 	sta32x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 	/* Bias level configuration will have done an extra enable */
 	regulator_bulk_disable(ARRAY_SIZE(sta32x->supplies), sta32x->supplies);
diff --git a/sound/soc/codecs/sta32x.h b/sound/soc/codecs/sta32x.h
index b97ee5a..d8e32a6 100644
--- a/sound/soc/codecs/sta32x.h
+++ b/sound/soc/codecs/sta32x.h
@@ -19,6 +19,7 @@
 /* STA326 register addresses */
 
 #define STA32X_REGISTER_COUNT	0x2d
+#define STA32X_COEF_COUNT 62
 
 #define STA32X_CONFA	0x00
 #define STA32X_CONFB    0x01
-- 
1.7.7.2

Re: [PATCH 1/3] ASoC: sta32x: preserve coefficient RAM

[Mark Brown]

On Mon, Nov 14, 2011 at 05:23:16PM +0100, Johannes Stezenbach wrote:
> The coefficient RAM must be saved in a shadow so it can
> be restored when the codec is powered on using
> regulator_bulk_enable().

Applied, thanks.

[PATCH 2/3] ASoC: sta32x: add platform data definition

[Johannes Stezenbach]

Add a structure for platform specific configuration and use it,
thereby removing a few FIXMEs which marked hard-coded values.

Cc: Sven Neumann <s.neumann@raumfeld.com>
Cc: Daniel Mack <daniel@zonque.org>
Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
 include/sound/sta32x.h    |   34 ++++++++++++++++++++++++++++++++++
 sound/soc/codecs/sta32x.c |   30 +++++++++++++++++++++---------
 2 files changed, 55 insertions(+), 9 deletions(-)
 create mode 100644 include/sound/sta32x.h

diff --git a/include/sound/sta32x.h b/include/sound/sta32x.h
new file mode 100644
index 0000000..45d7477
--- /dev/null
+++ b/include/sound/sta32x.h
@@ -0,0 +1,34 @@
+/*
+ * Platform data for ST STA32x ASoC codec driver.
+ *
+ * Copyright: 2011 Raumfeld GmbH
+ * Author: Johannes Stezenbach <js@sig21.net>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+#ifndef __LINUX_SND__STA32X_H
+#define __LINUX_SND__STA32X_H
+
+#define STA32X_OCFG_2CH		0
+#define STA32X_OCFG_2_1CH	1
+#define STA32X_OCFG_1CH		3
+
+#define STA32X_OM_CH1		0
+#define STA32X_OM_CH2		1
+#define STA32X_OM_CH3		2
+
+#define STA32X_THERMAL_ADJUSTMENT_ENABLE	1
+#define STA32X_THERMAL_RECOVERY_ENABLE		2
+
+struct sta32x_platform_data {
+	int output_conf;
+	int ch1_output_mapping;
+	int ch2_output_mapping;
+	int ch3_output_mapping;
+	int thermal_conf;
+};
+
+#endif /* __LINUX_SND__STA32X_H */
diff --git a/sound/soc/codecs/sta32x.c b/sound/soc/codecs/sta32x.c
index d2f3715..97091e3 100644
--- a/sound/soc/codecs/sta32x.c
+++ b/sound/soc/codecs/sta32x.c
@@ -35,6 +35,7 @@
 #include <sound/initval.h>
 #include <sound/tlv.h>
 
+#include <sound/sta32x.h>
 #include "sta32x.h"
 
 #define STA32X_RATES (SNDRV_PCM_RATE_32000 | \
@@ -73,6 +74,7 @@ static const char *sta32x_supply_names[] = {
 struct sta32x_priv {
 	struct regulator_bulk_data supplies[ARRAY_SIZE(sta32x_supply_names)];
 	struct snd_soc_codec *codec;
+	struct sta32x_platform_data *pdata;
 
 	unsigned int mclk;
 	unsigned int format;
@@ -775,9 +777,10 @@ static int sta32x_resume(struct snd_soc_codec *codec)
 static int sta32x_probe(struct snd_soc_codec *codec)
 {
 	struct sta32x_priv *sta32x = snd_soc_codec_get_drvdata(codec);
-	int i, ret = 0;
+	int i, ret = 0, thermal = 0;
 
 	sta32x->codec = codec;
+	sta32x->pdata = dev_get_platdata(codec->dev);
 
 	/* regulators */
 	for (i = 0; i < ARRAY_SIZE(sta32x->supplies); i++)
@@ -820,25 +823,34 @@ static int sta32x_probe(struct snd_soc_codec *codec)
 	snd_soc_cache_write(codec, STA32X_AUTO3, 0x00);
 	snd_soc_cache_write(codec, STA32X_C3CFG, 0x40);
 
-	/* FIXME enable thermal warning adjustment and recovery  */
+	/* set thermal warning adjustment and recovery */
+	if (!(sta32x->pdata->thermal_conf & STA32X_THERMAL_ADJUSTMENT_ENABLE))
+		thermal |= STA32X_CONFA_TWAB;
+	if (!(sta32x->pdata->thermal_conf & STA32X_THERMAL_RECOVERY_ENABLE))
+		thermal |= STA32X_CONFA_TWRB;
 	snd_soc_update_bits(codec, STA32X_CONFA,
-			    STA32X_CONFA_TWAB | STA32X_CONFA_TWRB, 0);
+			    STA32X_CONFA_TWAB | STA32X_CONFA_TWRB,
+			    thermal);
 
-	/* FIXME select 2.1 mode  */
+	/* select output configuration  */
 	snd_soc_update_bits(codec, STA32X_CONFF,
 			    STA32X_CONFF_OCFG_MASK,
-			    1 << STA32X_CONFF_OCFG_SHIFT);
+			    sta32x->pdata->output_conf
+			    << STA32X_CONFF_OCFG_SHIFT);
 
-	/* FIXME channel to output mapping */
+	/* channel to output mapping */
 	snd_soc_update_bits(codec, STA32X_C1CFG,
 			    STA32X_CxCFG_OM_MASK,
-			    0 << STA32X_CxCFG_OM_SHIFT);
+			    sta32x->pdata->ch1_output_mapping
+			    << STA32X_CxCFG_OM_SHIFT);
 	snd_soc_update_bits(codec, STA32X_C2CFG,
 			    STA32X_CxCFG_OM_MASK,
-			    1 << STA32X_CxCFG_OM_SHIFT);
+			    sta32x->pdata->ch2_output_mapping
+			    << STA32X_CxCFG_OM_SHIFT);
 	snd_soc_update_bits(codec, STA32X_C3CFG,
 			    STA32X_CxCFG_OM_MASK,
-			    2 << STA32X_CxCFG_OM_SHIFT);
+			    sta32x->pdata->ch3_output_mapping
+			    << STA32X_CxCFG_OM_SHIFT);
 
 	/* initialize coefficient shadow RAM with reset values */
 	for (i = 4; i <= 49; i += 5)
-- 
1.7.7.2

Re: [PATCH 2/3] ASoC: sta32x: add platform data definition

[Mark Brown]

On Mon, Nov 14, 2011 at 05:23:17PM +0100, Johannes Stezenbach wrote:
> Add a structure for platform specific configuration and use it,
> thereby removing a few FIXMEs which marked hard-coded values.

Applied, thanks.

[PATCH 3/3] ASoC: sta32x: add workaround for ESD reset issue

[Johannes Stezenbach]

sta32x resets and loses all configuration during ESD test.
Work around by polling the CONFA register once a second
and restore all coeffcients and registers when CONFA
changes unexpectedly.

Cc: Sven Neumann <s.neumann@raumfeld.com>
Cc: Daniel Mack <daniel@zonque.org>
Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
 include/sound/sta32x.h    |    1 +
 sound/soc/codecs/sta32x.c |   50 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/include/sound/sta32x.h b/include/sound/sta32x.h
index 45d7477..8d93b03 100644
--- a/include/sound/sta32x.h
+++ b/include/sound/sta32x.h
@@ -29,6 +29,7 @@ struct sta32x_platform_data {
 	int ch2_output_mapping;
 	int ch3_output_mapping;
 	int thermal_conf;
+	int needs_esd_watchdog;
 };
 
 #endif /* __LINUX_SND__STA32X_H */
diff --git a/sound/soc/codecs/sta32x.c b/sound/soc/codecs/sta32x.c
index 97091e3..3b0deaf 100644
--- a/sound/soc/codecs/sta32x.c
+++ b/sound/soc/codecs/sta32x.c
@@ -27,6 +27,7 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -80,6 +81,8 @@ struct sta32x_priv {
 	unsigned int format;
 
 	u32 coef_shadow[STA32X_COEF_COUNT];
+	struct delayed_work watchdog_work;
+	int shutdown;
 };
 
 static const DECLARE_TLV_DB_SCALE(mvol_tlv, -12700, 50, 1);
@@ -304,6 +307,46 @@ int sta32x_cache_sync(struct snd_soc_codec *codec)
 	return rc;
 }
 
+/* work around ESD issue where sta32x resets and loses all configuration */
+static void sta32x_watchdog(struct work_struct *work)
+{
+	struct sta32x_priv *sta32x = container_of(work, struct sta32x_priv,
+						  watchdog_work.work);
+	struct snd_soc_codec *codec = sta32x->codec;
+	unsigned int confa, confa_cached;
+
+	/* check if sta32x has reset itself */
+	confa_cached = snd_soc_read(codec, STA32X_CONFA);
+	codec->cache_bypass = 1;
+	confa = snd_soc_read(codec, STA32X_CONFA);
+	codec->cache_bypass = 0;
+	if (confa != confa_cached) {
+		codec->cache_sync = 1;
+		sta32x_cache_sync(codec);
+	}
+
+	if (!sta32x->shutdown)
+		schedule_delayed_work(&sta32x->watchdog_work,
+				      round_jiffies_relative(HZ));
+}
+
+static void sta32x_watchdog_start(struct sta32x_priv *sta32x)
+{
+	if (sta32x->pdata->needs_esd_watchdog) {
+		sta32x->shutdown = 0;
+		schedule_delayed_work(&sta32x->watchdog_work,
+				      round_jiffies_relative(HZ));
+	}
+}
+
+static void sta32x_watchdog_stop(struct sta32x_priv *sta32x)
+{
+	if (sta32x->pdata->needs_esd_watchdog) {
+		sta32x->shutdown = 1;
+		cancel_delayed_work_sync(&sta32x->watchdog_work);
+	}
+}
+
 #define SINGLE_COEF(xname, index) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = sta32x_coefficient_info, \
@@ -714,6 +757,7 @@ static int sta32x_set_bias_level(struct snd_soc_codec *codec,
 			}
 
 			sta32x_cache_sync(codec);
+			sta32x_watchdog_start(sta32x);
 		}
 
 		/* Power up to mute */
@@ -730,7 +774,7 @@ static int sta32x_set_bias_level(struct snd_soc_codec *codec,
 				    STA32X_CONFF_PWDN | STA32X_CONFF_EAPD,
 				    STA32X_CONFF_PWDN);
 		msleep(300);
-
+		sta32x_watchdog_stop(sta32x);
 		regulator_bulk_disable(ARRAY_SIZE(sta32x->supplies),
 				       sta32x->supplies);
 		break;
@@ -863,6 +907,9 @@ static int sta32x_probe(struct snd_soc_codec *codec)
 	sta32x->coef_shadow[60] = 0x400000;
 	sta32x->coef_shadow[61] = 0x400000;
 
+	if (sta32x->pdata->needs_esd_watchdog)
+		INIT_DELAYED_WORK(&sta32x->watchdog_work, sta32x_watchdog);
+
 	sta32x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 	/* Bias level configuration will have done an extra enable */
 	regulator_bulk_disable(ARRAY_SIZE(sta32x->supplies), sta32x->supplies);
@@ -879,6 +926,7 @@ static int sta32x_remove(struct snd_soc_codec *codec)
 {
 	struct sta32x_priv *sta32x = snd_soc_codec_get_drvdata(codec);
 
+	sta32x_watchdog_stop(sta32x);
 	sta32x_set_bias_level(codec, SND_SOC_BIAS_OFF);
 	regulator_bulk_disable(ARRAY_SIZE(sta32x->supplies), sta32x->supplies);
 	regulator_bulk_free(ARRAY_SIZE(sta32x->supplies), sta32x->supplies);
-- 
1.7.7.2

Re: [PATCH 3/3] ASoC: sta32x: add workaround for ESD reset issue

[Mark Brown]

On Mon, Nov 14, 2011 at 05:23:18PM +0100, Johannes Stezenbach wrote:
> sta32x resets and loses all configuration during ESD test.
> Work around by polling the CONFA register once a second
> and restore all coeffcients and registers when CONFA
> changes unexpectedly.

Applied, though it would be nicer if idle_bias_off were also enabled so
that this only has an effect when audio is active (otherwise I can't
imagine anything cares about the reset).

Re: [PATCH 3/3] ASoC: sta32x: add workaround for ESD reset issue

[Johannes Stezenbach]

On Mon, Nov 14, 2011 at 09:37:17PM +0000, Mark Brown wrote:
> On Mon, Nov 14, 2011 at 05:23:18PM +0100, Johannes Stezenbach wrote:
> > sta32x resets and loses all configuration during ESD test.
> > Work around by polling the CONFA register once a second
> > and restore all coeffcients and registers when CONFA
> > changes unexpectedly.
> 
> Applied, though it would be nicer if idle_bias_off were also enabled so
> that this only has an effect when audio is active (otherwise I can't
> imagine anything cares about the reset).

OK, I must admit the meaning of this comment was not clear to me,
but after grepping in git log I found
a850260e4722706805fda3a0f6e5bc1539e83bac
Is that what you mean?

Thanks
Johannes

Re: [PATCH 3/3] ASoC: sta32x: add workaround for ESD reset issue

[Mark Brown]

On Tue, Nov 15, 2011 at 12:50:13PM +0100, Johannes Stezenbach wrote:
> On Mon, Nov 14, 2011 at 09:37:17PM +0000, Mark Brown wrote:

> > Applied, though it would be nicer if idle_bias_off were also enabled so
> > that this only has an effect when audio is active (otherwise I can't
> > imagine anything cares about the reset).

> OK, I must admit the meaning of this comment was not clear to me,
> but after grepping in git log I found
> a850260e4722706805fda3a0f6e5bc1539e83bac

Please always quote human readable text, not just git commit ids.

> Is that what you mean?

Yes.