[PATCH 0/3] make the sgtl5000-codec work

[PATCH 0/3] make the sgtl5000-codec work

[Wolfram Sang]

These are three patches I developed while trying to make the sgtl5000 work on
an mx28evk with the mxs-support provided by Dong Aisheng. The chip used to
lock up before, because a wrong value was written to the power control.

The first patch removes code duplication
The second one is for user-comfort
The third one makes the device work

Aisheng: This should make your hack for the sgtl5000 obsolete. Could you please
test it and add your Tested-by-tag if it also works for you? (Or Reviewed-by if
you want to review the code).

Zeng Zhaoming: Did this code really work for you?

Regards,

   Wolfram

Wolfram Sang (3):
  ASoC: sgtl5000: refactor registering internal ldo
  ASoC: sgtl5000: guide user when regulator support is needed
  ASoC: sgtl5000: fix cache handling

 sound/soc/codecs/sgtl5000.c |  198 +++++++++++++++----------------------------
 1 files changed, 67 insertions(+), 131 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] ASoC: sgtl5000: refactor registering internal ldo

[Wolfram Sang]

The code for registering the internal ldo was present twice. Turn it
into a function instead. Also, inform the user if LDO is used now.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Dong Aisheng <b29396@freescale.com>
Cc: Zeng Zhaoming <b32542@freescale.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/sgtl5000.c |   69 +++++++++++++++++++-----------------------
 1 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index ff29380..17af336 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1218,6 +1218,34 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
 	return 0;
 }
 
+static int sgtl5000_replace_vddd_with_ldo(struct snd_soc_codec *codec)
+{
+	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	/* set internal ldo to 1.2v */
+	ret = ldo_regulator_register(codec, &ldo_init_data, LDO_VOLTAGE);
+	if (ret) {
+		dev_err(codec->dev,
+			"Failed to register vddd internal supplies: %d\n", ret);
+		return ret;
+	}
+
+	sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
+
+	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
+			sgtl5000->supplies);
+
+	if (ret) {
+		ldo_regulator_remove(codec);
+		dev_err(codec->dev, "Failed to request supplies: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(codec->dev, "Using internal LDO instead of VDDD\n");
+	return 0;
+}
+
 static int sgtl5000_enable_regulators(struct snd_soc_codec *codec)
 {
 	u16 reg;
@@ -1235,30 +1263,9 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec)
 	if (!ret)
 		external_vddd = 1;
 	else {
-		/* set internal ldo to 1.2v */
-		int voltage = LDO_VOLTAGE;
-
-		ret = ldo_regulator_register(codec, &ldo_init_data, voltage);
-		if (ret) {
-			dev_err(codec->dev,
-			"Failed to register vddd internal supplies: %d\n",
-				ret);
-			return ret;
-		}
-
-		sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
-
-		ret = regulator_bulk_get(codec->dev,
-				ARRAY_SIZE(sgtl5000->supplies),
-				sgtl5000->supplies);
-
-		if (ret) {
-			ldo_regulator_remove(codec);
-			dev_err(codec->dev,
-				"Failed to request supplies: %d\n", ret);
-
+		ret = sgtl5000_replace_vddd_with_ldo(codec);
+		if (ret)
 			return ret;
-		}
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
@@ -1287,7 +1294,6 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec)
 	 * roll back to use internal LDO
 	 */
 	if (external_vddd && rev >= 0x11) {
-		int voltage = LDO_VOLTAGE;
 		/* disable all regulator first */
 		regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
 					sgtl5000->supplies);
@@ -1295,23 +1301,10 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec)
 		regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies),
 					sgtl5000->supplies);
 
-		ret = ldo_regulator_register(codec, &ldo_init_data, voltage);
+		ret = sgtl5000_replace_vddd_with_ldo(codec);
 		if (ret)
 			return ret;
 
-		sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
-
-		ret = regulator_bulk_get(codec->dev,
-				ARRAY_SIZE(sgtl5000->supplies),
-				sgtl5000->supplies);
-		if (ret) {
-			ldo_regulator_remove(codec);
-			dev_err(codec->dev,
-				"Failed to request supplies: %d\n", ret);
-
-			return ret;
-		}
-
 		ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
 						sgtl5000->supplies);
 		if (ret)
-- 
1.7.5.4

Re: [PATCH 1/3] ASoC: sgtl5000: refactor registering internal ldo

[Mark Brown]

On Mon, Jul 18, 2011 at 05:53:03PM +0200, Wolfram Sang wrote:
> The code for registering the internal ldo was present twice. Turn it
> into a function instead. Also, inform the user if LDO is used now.

Applied, thanks.

[PATCH 2/3] ASoC: sgtl5000: guide user when regulator support is needed

[Wolfram Sang]

Print a hint when the user has a setup where CONFIG_REGULATOR is really
needed to make the driver work.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Dong Aisheng <b29396@freescale.com>
Cc: Zeng Zhaoming <b32542@freescale.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/sgtl5000.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 17af336..76258f2 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -907,6 +907,7 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
 				struct regulator_init_data *init_data,
 				int voltage)
 {
+	dev_err(codec->dev, "this setup needs regulator support in the kernel\n");
 	return -EINVAL;
 }
 
-- 
1.7.5.4

[PATCH 3/3] ASoC: sgtl5000: fix cache handling

[Wolfram Sang]

Cache handling in this driver is seriously broken. The chip has 16-bit
registers, yet their register numbering also increases by 2 per register, i.e.
there are only even-numbered registers. The default cache in this driver,
though, simply increments register numbers, so it does need some mapping as
seen in sgtl5000_restore_regs(), note the '>> 1':

	snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL,
                        cache[SGTL5000_CHIP_LINREG_CTRL >> 1]);

That, of course, won't work with snd_soc_update_bits(). (Thus, we won't even
notice the missing register 0x1c in the default regs which shifted all follwing
registers to wrong values.) Noticed on the MX28EVK where enabling the regulators
simply locked up the chip.

Refactor the routines and use a properly sized default_regs array which matches
the register layout of the underlying chip. Also saves some code which should
make up for the bigger array a little.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Dong Aisheng <b29396@freescale.com>
Cc: Zeng Zhaoming <b32542@freescale.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/sgtl5000.c |  128 ++++++++++++-------------------------------
 1 files changed, 35 insertions(+), 93 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 76258f2..7e4066e 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -33,73 +33,31 @@
 #define SGTL5000_DAP_REG_OFFSET	0x0100
 #define SGTL5000_MAX_REG_OFFSET	0x013A
 
-/* default value of sgtl5000 registers except DAP */
-static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET >> 1] =  {
-	0xa011, /* 0x0000, CHIP_ID. 11 stand for revison 17 */
-	0x0000, /* 0x0002, CHIP_DIG_POWER. */
-	0x0008, /* 0x0004, CHIP_CKL_CTRL */
-	0x0010, /* 0x0006, CHIP_I2S_CTRL */
-	0x0000, /* 0x0008, reserved */
-	0x0008, /* 0x000A, CHIP_SSS_CTRL */
-	0x0000, /* 0x000C, reserved */
-	0x020c, /* 0x000E, CHIP_ADCDAC_CTRL */
-	0x3c3c, /* 0x0010, CHIP_DAC_VOL */
-	0x0000, /* 0x0012, reserved */
-	0x015f, /* 0x0014, CHIP_PAD_STRENGTH */
-	0x0000, /* 0x0016, reserved */
-	0x0000, /* 0x0018, reserved */
-	0x0000, /* 0x001A, reserved */
-	0x0000, /* 0x001E, reserved */
-	0x0000, /* 0x0020, CHIP_ANA_ADC_CTRL */
-	0x1818, /* 0x0022, CHIP_ANA_HP_CTRL */
-	0x0111, /* 0x0024, CHIP_ANN_CTRL */
-	0x0000, /* 0x0026, CHIP_LINREG_CTRL */
-	0x0000, /* 0x0028, CHIP_REF_CTRL */
-	0x0000, /* 0x002A, CHIP_MIC_CTRL */
-	0x0000, /* 0x002C, CHIP_LINE_OUT_CTRL */
-	0x0404, /* 0x002E, CHIP_LINE_OUT_VOL */
-	0x7060, /* 0x0030, CHIP_ANA_POWER */
-	0x5000, /* 0x0032, CHIP_PLL_CTRL */
-	0x0000, /* 0x0034, CHIP_CLK_TOP_CTRL */
-	0x0000, /* 0x0036, CHIP_ANA_STATUS */
-	0x0000, /* 0x0038, reserved */
-	0x0000, /* 0x003A, CHIP_ANA_TEST2 */
-	0x0000, /* 0x003C, CHIP_SHORT_CTRL */
-	0x0000, /* reserved */
-};
-
-/* default value of dap registers */
-static const u16 sgtl5000_dap_regs[] = {
-	0x0000, /* 0x0100, DAP_CONTROL */
-	0x0000, /* 0x0102, DAP_PEQ */
-	0x0040, /* 0x0104, DAP_BASS_ENHANCE */
-	0x051f, /* 0x0106, DAP_BASS_ENHANCE_CTRL */
-	0x0000, /* 0x0108, DAP_AUDIO_EQ */
-	0x0040, /* 0x010A, DAP_SGTL_SURROUND */
-	0x0000, /* 0x010C, DAP_FILTER_COEF_ACCESS */
-	0x0000, /* 0x010E, DAP_COEF_WR_B0_MSB */
-	0x0000, /* 0x0110, DAP_COEF_WR_B0_LSB */
-	0x0000, /* 0x0112, reserved */
-	0x0000, /* 0x0114, reserved */
-	0x002f, /* 0x0116, DAP_AUDIO_EQ_BASS_BAND0 */
-	0x002f, /* 0x0118, DAP_AUDIO_EQ_BAND0 */
-	0x002f, /* 0x011A, DAP_AUDIO_EQ_BAND2 */
-	0x002f, /* 0x011C, DAP_AUDIO_EQ_BAND3 */
-	0x002f, /* 0x011E, DAP_AUDIO_EQ_TREBLE_BAND4 */
-	0x8000, /* 0x0120, DAP_MAIN_CHAN */
-	0x0000, /* 0x0122, DAP_MIX_CHAN */
-	0x0510, /* 0x0124, DAP_AVC_CTRL */
-	0x1473, /* 0x0126, DAP_AVC_THRESHOLD */
-	0x0028, /* 0x0128, DAP_AVC_ATTACK */
-	0x0050, /* 0x012A, DAP_AVC_DECAY */
-	0x0000, /* 0x012C, DAP_COEF_WR_B1_MSB */
-	0x0000, /* 0x012E, DAP_COEF_WR_B1_LSB */
-	0x0000, /* 0x0130, DAP_COEF_WR_B2_MSB */
-	0x0000, /* 0x0132, DAP_COEF_WR_B2_LSB */
-	0x0000, /* 0x0134, DAP_COEF_WR_A1_MSB */
-	0x0000, /* 0x0136, DAP_COEF_WR_A1_LSB */
-	0x0000, /* 0x0138, DAP_COEF_WR_A2_MSB */
-	0x0000, /* 0x013A, DAP_COEF_WR_A2_LSB */
+/* default value of sgtl5000 registers */
+static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET] =  {
+	[SGTL5000_CHIP_CLK_CTRL] = 0x0008,
+	[SGTL5000_CHIP_I2S_CTRL] = 0x0010,
+	[SGTL5000_CHIP_SSS_CTRL] = 0x0008,
+	[SGTL5000_CHIP_DAC_VOL] = 0x3c3c,
+	[SGTL5000_CHIP_PAD_STRENGTH] = 0x015f,
+	[SGTL5000_CHIP_ANA_HP_CTRL] = 0x1818,
+	[SGTL5000_CHIP_ANA_CTRL] = 0x0111,
+	[SGTL5000_CHIP_LINE_OUT_VOL] = 0x0404,
+	[SGTL5000_CHIP_ANA_POWER] = 0x7060,
+	[SGTL5000_CHIP_PLL_CTRL] = 0x5000,
+	[SGTL5000_DAP_BASS_ENHANCE] = 0x0040,
+	[SGTL5000_DAP_BASS_ENHANCE_CTRL] = 0x051f,
+	[SGTL5000_DAP_SURROUND] = 0x0040,
+	[SGTL5000_DAP_EQ_BASS_BAND0] = 0x002f,
+	[SGTL5000_DAP_EQ_BASS_BAND1] = 0x002f,
+	[SGTL5000_DAP_EQ_BASS_BAND2] = 0x002f,
+	[SGTL5000_DAP_EQ_BASS_BAND3] = 0x002f,
+	[SGTL5000_DAP_EQ_BASS_BAND4] = 0x002f,
+	[SGTL5000_DAP_MAIN_CHAN] = 0x8000,
+	[SGTL5000_DAP_AVC_CTRL] = 0x0510,
+	[SGTL5000_DAP_AVC_THRESHOLD] = 0x1473,
+	[SGTL5000_DAP_AVC_ATTACK] = 0x0028,
+	[SGTL5000_DAP_AVC_DECAY] = 0x0050,
 };
 
 /* regulator supplies for sgtl5000, VDDD is an optional external supply */
@@ -1023,12 +981,10 @@ static int sgtl5000_suspend(struct snd_soc_codec *codec, pm_message_t state)
 static int sgtl5000_restore_regs(struct snd_soc_codec *codec)
 {
 	u16 *cache = codec->reg_cache;
-	int i;
-	int regular_regs = SGTL5000_CHIP_SHORT_CTRL >> 1;
+	u16 reg;
 
 	/* restore regular registers */
-	for (i = 0; i < regular_regs; i++) {
-		int reg = i << 1;
+	for (reg = 0; reg <= SGTL5000_CHIP_SHORT_CTRL; reg += 2) {
 
 		/* this regs depends on the others */
 		if (reg == SGTL5000_CHIP_ANA_POWER ||
@@ -1038,35 +994,31 @@ static int sgtl5000_restore_regs(struct snd_soc_codec *codec)
 			reg == SGTL5000_CHIP_CLK_CTRL)
 			continue;
 
-		snd_soc_write(codec, reg, cache[i]);
+		snd_soc_write(codec, reg, cache[reg]);
 	}
 
 	/* restore dap registers */
-	for (i = SGTL5000_DAP_REG_OFFSET >> 1;
-			i < SGTL5000_MAX_REG_OFFSET >> 1; i++) {
-		int reg = i << 1;
-
-		snd_soc_write(codec, reg, cache[i]);
-	}
+	for (reg = SGTL5000_DAP_REG_OFFSET; reg < SGTL5000_MAX_REG_OFFSET; reg += 2)
+		snd_soc_write(codec, reg, cache[reg]);
 
 	/*
 	 * restore power and other regs according
 	 * to set_power() and set_clock()
 	 */
 	snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL,
-			cache[SGTL5000_CHIP_LINREG_CTRL >> 1]);
+			cache[SGTL5000_CHIP_LINREG_CTRL]);
 
 	snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER,
-			cache[SGTL5000_CHIP_ANA_POWER >> 1]);
+			cache[SGTL5000_CHIP_ANA_POWER]);
 
 	snd_soc_write(codec, SGTL5000_CHIP_CLK_CTRL,
-			cache[SGTL5000_CHIP_CLK_CTRL >> 1]);
+			cache[SGTL5000_CHIP_CLK_CTRL]);
 
 	snd_soc_write(codec, SGTL5000_CHIP_REF_CTRL,
-			cache[SGTL5000_CHIP_REF_CTRL >> 1]);
+			cache[SGTL5000_CHIP_REF_CTRL]);
 
 	snd_soc_write(codec, SGTL5000_CHIP_LINE_OUT_CTRL,
-			cache[SGTL5000_CHIP_LINE_OUT_CTRL >> 1]);
+			cache[SGTL5000_CHIP_LINE_OUT_CTRL]);
 	return 0;
 }
 
@@ -1454,16 +1406,6 @@ static __devinit int sgtl5000_i2c_probe(struct i2c_client *client,
 	if (!sgtl5000)
 		return -ENOMEM;
 
-	/*
-	 * copy DAP default values to default value array.
-	 * sgtl5000 register space has a big hole, merge it
-	 * at init phase makes life easy.
-	 * FIXME: should we drop 'const' of sgtl5000_regs?
-	 */
-	memcpy((void *)(&sgtl5000_regs[0] + (SGTL5000_DAP_REG_OFFSET >> 1)),
-			sgtl5000_dap_regs,
-			SGTL5000_MAX_REG_OFFSET - SGTL5000_DAP_REG_OFFSET);
-
 	i2c_set_clientdata(client, sgtl5000);
 
 	ret = snd_soc_register_codec(&client->dev,
-- 
1.7.5.4

Re: [PATCH 0/3] make the sgtl5000-codec work

[Zeng Zhaoming]

On Mon 2011-07-18 17:53:02, Wolfram Sang wrote:
> These are three patches I developed while trying to make the sgtl5000 work on
> an mx28evk with the mxs-support provided by Dong Aisheng. The chip used to
> lock up before, because a wrong value was written to the power control.
> 
> The first patch removes code duplication
> The second one is for user-comfort
> The third one makes the device work
> 
> Aisheng: This should make your hack for the sgtl5000 obsolete. Could you please
> test it and add your Tested-by-tag if it also works for you? (Or Reviewed-by if
> you want to review the code).
> 
> Zeng Zhaoming: Did this code really work for you?
>
Hi, Wolfram:
    Thanks to CC me.
    pls check http://comments.gmane.org/gmane.linux.alsa.devel/83781

    I think Mark is right, sgtl5000 already declared its register_step is 2, soc_cache.c
    should generate a dense cache layout instead of padding it by driver.

    But after check soc_cache.c, ASoC mix up index and register address to index cache,
    and many places need to modify to correct it, so I think we should re-consider
    if register_step != 1 is not a common case.
    
> Regards,
> 
>    Wolfram
>
  Thanks.

> Wolfram Sang (3):
>   ASoC: sgtl5000: refactor registering internal ldo
>   ASoC: sgtl5000: guide user when regulator support is needed
>   ASoC: sgtl5000: fix cache handling
> 
>  sound/soc/codecs/sgtl5000.c |  198 +++++++++++++++----------------------------
>  1 files changed, 67 insertions(+), 131 deletions(-)
> 
> -- 
> 1.7.5.4
> 

Re: [PATCH 0/3] make the sgtl5000-codec work

[Wolfram Sang]

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

Hi,

>     pls check http://comments.gmane.org/gmane.linux.alsa.devel/83781
> 
>     I think Mark is right, sgtl5000 already declared its register_step is 2, soc_cache.c
>     should generate a dense cache layout instead of padding it by driver.

Thanks for the pointer. I can follow Mark's reasoning. In fact, I was
wondering why ASoC does not consider the step, but I assumed it was
intentional. With my holidays coming along, nothing I am going to
tackle in the next time, though ;)

>     But after check soc_cache.c, ASoC mix up index and register address to index cache,
>     and many places need to modify to correct it, so I think we should re-consider
>     if register_step != 1 is not a common case.

I think it should be properly fixed in ASoC. It should be carefully
done, but otherwise not be a major task IMO. Note that sgtl5000 will
need adaptions nonetheless. Which brings me back to the question: In
what setup did the driver work for you? Did you have regulators on that
board? I am still trying to understand the side-effects of what I am
seeing...

Mark: Will you pick up patches 1 and 2 nonetheless?

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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

Re: [PATCH 0/3] make the sgtl5000-codec work

[Liam Girdwood]

On Tue, 2011-07-19 at 11:04 +0200, Wolfram Sang wrote:
> Hi,
> 
> >     pls check http://comments.gmane.org/gmane.linux.alsa.devel/83781
> > 
> >     I think Mark is right, sgtl5000 already declared its register_step is 2, soc_cache.c
> >     should generate a dense cache layout instead of padding it by driver.
> 
> Thanks for the pointer. I can follow Mark's reasoning. In fact, I was
> wondering why ASoC does not consider the step, but I assumed it was
> intentional. With my holidays coming along, nothing I am going to
> tackle in the next time, though ;)
> 
> >     But after check soc_cache.c, ASoC mix up index and register address to index cache,
> >     and many places need to modify to correct it, so I think we should re-consider
> >     if register_step != 1 is not a common case.
> 
> I think it should be properly fixed in ASoC. It should be carefully
> done, but otherwise not be a major task IMO. Note that sgtl5000 will
> need adaptions nonetheless. Which brings me back to the question: In
> what setup did the driver work for you? Did you have regulators on that
> board? I am still trying to understand the side-effects of what I am
> seeing...
> 
> Mark: Will you pick up patches 1 and 2 nonetheless?

Please CC me on all ASoC patches. 1 and 2 look OK.

Acked-by: Liam Girdwood <lrg@ti.com>

Thanks

Liam

Re: [PATCH 0/3] make the sgtl5000-codec work

[Mark Brown]

On Tue, Jul 19, 2011 at 11:04:04AM +0200, Wolfram Sang wrote:

> >     pls check http://comments.gmane.org/gmane.linux.alsa.devel/83781
> > 
> >     I think Mark is right, sgtl5000 already declared its register_step is 2, soc_cache.c
> >     should generate a dense cache layout instead of padding it by driver.

> Thanks for the pointer. I can follow Mark's reasoning. In fact, I was
> wondering why ASoC does not consider the step, but I assumed it was
> intentional. With my holidays coming along, nothing I am going to
> tackle in the next time, though ;)

The only reason steps are ever used is for AC'97 devices which don't use
the generic cache code as they're pretty much obsolete so the framework
support is mostly just carried around as legacy.  The step size is used
in the cache display code which does use it.  If someone was interested
enough to convert AC'97 over that'd be nice.

Re: [PATCH 0/3] make the sgtl5000-codec work

[Wolfram Sang]

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

On Tue, Jul 19, 2011 at 04:13:37PM +0100, Mark Brown wrote:
> On Tue, Jul 19, 2011 at 11:04:04AM +0200, Wolfram Sang wrote:
> 
> > >     pls check http://comments.gmane.org/gmane.linux.alsa.devel/83781
> > > 
> > >     I think Mark is right, sgtl5000 already declared its register_step is 2, soc_cache.c
> > >     should generate a dense cache layout instead of padding it by driver.
> 
> > Thanks for the pointer. I can follow Mark's reasoning. In fact, I was
> > wondering why ASoC does not consider the step, but I assumed it was
> > intentional. With my holidays coming along, nothing I am going to
> > tackle in the next time, though ;)
> 
> The only reason steps are ever used is for AC'97 devices which don't use
> the generic cache code as they're pretty much obsolete so the framework
> support is mostly just carried around as legacy.  The step size is used
> in the cache display code which does use it.  If someone was interested
> enough to convert AC'97 over that'd be nice.

I am a bit confused now: Do you want to retire the step size in the long run?
Then my sgtl-patch might be the right approach after all.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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

Re: [PATCH 0/3] make the sgtl5000-codec work

[Mark Brown]

On Tue, Jul 19, 2011 at 06:46:39PM +0200, Wolfram Sang wrote:
> On Tue, Jul 19, 2011 at 04:13:37PM +0100, Mark Brown wrote:

> > The only reason steps are ever used is for AC'97 devices which don't use
> > the generic cache code as they're pretty much obsolete so the framework
> > support is mostly just carried around as legacy.  The step size is used
> > in the cache display code which does use it.  If someone was interested
> > enough to convert AC'97 over that'd be nice.

> I am a bit confused now: Do you want to retire the step size in the long run?
> Then my sgtl-patch might be the right approach after all.

Err, no.  I'm saying there's no current use of step size in the
soc-cache code because no modern hardware has any need for it.

Re: [PATCH 0/3] make the sgtl5000-codec work

[Dong Aisheng]

2011/7/18 Wolfram Sang <w.sang@pengutronix.de>:
> These are three patches I developed while trying to make the sgtl5000 work on
> an mx28evk with the mxs-support provided by Dong Aisheng. The chip used to
> lock up before, because a wrong value was written to the power control.
>
> The first patch removes code duplication
> The second one is for user-comfort
> The third one makes the device work
>
> Aisheng: This should make your hack for the sgtl5000 obsolete. Could you please
> test it and add your Tested-by-tag if it also works for you? (Or Reviewed-by if
> you want to review the code).
I'm willing to do that.
I'll give the result soon.

Regards
Dong Aisheng

Re: [PATCH 0/3] make the sgtl5000-codec work

[Shawn Guo]

On Mon, Jul 18, 2011 at 05:53:02PM +0200, Wolfram Sang wrote:
> These are three patches I developed while trying to make the sgtl5000 work on
> an mx28evk with the mxs-support provided by Dong Aisheng. The chip used to
> lock up before, because a wrong value was written to the power control.
> 
> The first patch removes code duplication
> The second one is for user-comfort
> The third one makes the device work
> 
> Aisheng: This should make your hack for the sgtl5000 obsolete. Could you please
> test it and add your Tested-by-tag if it also works for you? (Or Reviewed-by if
> you want to review the code).
> 
> Zeng Zhaoming: Did this code really work for you?
> 
> Regards,
> 
>    Wolfram
> 
> Wolfram Sang (3):
>   ASoC: sgtl5000: refactor registering internal ldo
>   ASoC: sgtl5000: guide user when regulator support is needed
>   ASoC: sgtl5000: fix cache handling
> 
>  sound/soc/codecs/sgtl5000.c |  198 +++++++++++++++----------------------------
>  1 files changed, 67 insertions(+), 131 deletions(-)
> 
On i.mx51 babbage,

Tested-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

Re: [PATCH 0/3] make the sgtl5000-codec work

[Dong Aisheng]

2011/7/18 Wolfram Sang <w.sang@pengutronix.de>:
> These are three patches I developed while trying to make the sgtl5000 work on
> an mx28evk with the mxs-support provided by Dong Aisheng. The chip used to
> lock up before, because a wrong value was written to the power control.
>
> The first patch removes code duplication
> The second one is for user-comfort
> The third one makes the device work
>
> Aisheng: This should make your hack for the sgtl5000 obsolete. Could you please
> test it and add your Tested-by-tag if it also works for you? (Or Reviewed-by if
> you want to review the code).
It works on MX28EVK.

Tested-by: Dong Aisheng <b29396@freescale.com>

Re: [PATCH 0/3] make the sgtl5000-codec work

[Mark Brown]

On Mon, Jul 18, 2011 at 05:53:02PM +0200, Wolfram Sang wrote:
> These are three patches I developed while trying to make the sgtl5000 work on
> an mx28evk with the mxs-support provided by Dong Aisheng. The chip used to
> lock up before, because a wrong value was written to the power control.

> The first patch removes code duplication
> The second one is for user-comfort
> The third one makes the device work

Shouldn't we be restructuring this series so that the last change goes
in as a critical bugfix for stable and the other two code cleanups then
go on top of it?

Re: [PATCH 0/3] make the sgtl5000-codec work

[Wolfram Sang]

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


> > The first patch removes code duplication
> > The second one is for user-comfort
> > The third one makes the device work
> 
> Shouldn't we be restructuring this series so that the last change goes
> in as a critical bugfix for stable and the other two code cleanups then
> go on top of it?

Yes. That was the chronological order; I later realized this is suboptimal
here, sorry (luckily the patches are independent enough). Will take care next
time.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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