Skip to contentSkip to conversation

ASoC: alc5623: Convert codec->hw_read to regmap_read

PATCH In review Axel Lin 2 participants 9 messages Oct 13, 2011

Subsystems (4)
Sound ASoC Wolfson microelectronics drivers Texas Instruments audio (ASoC/HDA) drivers
lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/3 ASoC: alc5623: Convert codec->hw_read to regmap_read

codec->hw_read is broken now, let's covert to regmap_read.

Signed-off-by Axel Lin <[email protected]>
---
 sound/soc/codecs/alc5623.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
sound/soc/codecs/alc5623.c +5 -2
index 557b3af..8c156cb 100644
@@ -51,10 +51,13 @@ static void alc5623_fill_cache(struct snd_soc_codec *codec)
 {
 	int i, step = codec->driver->reg_cache_step;
 	u16 *cache = codec->reg_cache;
+	unsigned int val;
 
 	/* not really efficient ... */
-	for (i = 0 ; i < codec->driver->reg_cache_size ; i += step)
-		cache[i] = codec->hw_read(codec, i);
+	for (i = 0 ; i < codec->driver->reg_cache_size ; i += step) {
+		regmap_read(codec->control_data, i, &val);
+		cache[i] = val;
+	}
 }
 
 static inline int alc5623_reset(struct snd_soc_codec *codec)
Signature · 1 line
1.7.4.1
author
#1 lore raw

PATCH 2/3 ASoC: tlv320aic3x: Convert codec->hw_read to regmap_read

codec->hw_read is broken now, let's covert to regmap_read.

Signed-off-by Axel Lin <[email protected]>
---
 sound/soc/codecs/tlv320aic3x.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
sound/soc/codecs/tlv320aic3x.c +7 -1
index be55b7f..8775d52 100644
@@ -131,13 +131,19 @@ static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
 		      u8 *value)
 {
 	u8 *cache = codec->reg_cache;
+	unsigned int val;
+	int ret;
 
 	if (codec->cache_only)
 		return -EINVAL;
 	if (reg >= AIC3X_CACHEREGNUM)
 		return -1;
 
-	*value = codec->hw_read(codec, reg);
+	ret = regmap_read(codec->control_data, reg, &val);
+	if (ret)
+		return ret;
+
+	*value = val;
 	cache[reg] = *value;
 
 	return 0;
Signature · 1 line
1.7.4.1
author
#2 lore raw

PATCH 3/3 ASoC: wm8961: Convert codec->hw_read to regmap_read

In reply to Axel Lin

codec->hw_read is broken now, let's covert to regmap_read.

Signed-off-by Axel Lin <[email protected]>
---
 sound/soc/codecs/wm8961.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
sound/soc/codecs/wm8961.c +4 -3
index cdee810..2fa1672 100644
@@ -960,6 +960,7 @@ static int wm8961_probe(struct snd_soc_codec *codec)
 	struct snd_soc_dapm_context *dapm = &codec->dapm;
 	int ret = 0;
 	u16 reg;
+	unsigned int val;
 
 	ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_I2C);
 	if (ret != 0) {

@@ -974,10 +975,10 @@ static int wm8961_probe(struct snd_soc_codec *codec)
 	}
 
 	/* This isn't volatile - readback doesn't correspond to write */
-	reg = codec->hw_read(codec, WM8961_RIGHT_INPUT_VOLUME);
+	regmap_read(codec->control_data, WM8961_RIGHT_INPUT_VOLUME, &val);
 	dev_info(codec->dev, "WM8961 family %d revision %c\n",
-		 (reg & WM8961_DEVICE_ID_MASK) >> WM8961_DEVICE_ID_SHIFT,
-		 ((reg & WM8961_CHIP_REV_MASK) >> WM8961_CHIP_REV_SHIFT)
+		 (val & WM8961_DEVICE_ID_MASK) >> WM8961_DEVICE_ID_SHIFT,
+		 ((val & WM8961_CHIP_REV_MASK) >> WM8961_CHIP_REV_SHIFT)
 		 + 'A');
 
 	ret = wm8961_reset(codec);
Signature · 1 line
1.7.4.1
#3 lore raw
In reply to Axel Lin
Quoted 3 lines · Axel Lin

codec->hw_read is broken now, let's covert to regmap_read.

Signed-off-by Axel Lin <[email protected]>

I'd rather not fix these like this because when we convert these drivers to use regmap directly it won't be quite so obvious what's going on and we'll easily introduce bugs where we start using the cached values from a regmap level cache.

Probably the best thing here is to convert all these drivers to use cache_bypass to read directly through the cache - the same thing will then work with direct regmap usage.

author
#4 lore raw

2011/10/14 Mark Brown <[email protected]>:

Quoted 5 lines · Axel Lin
Quoted 3 lines

codec->hw_read is broken now, let's covert to regmap_read.

Signed-off-by Axel Lin <[email protected]>
Quoted 9 lines

I'd rather not fix these like this because when we convert these drivers to use regmap directly it won't be quite so obvious what's going on and we'll easily introduce bugs where we start using the cached values from a regmap level cache.

Probably the best thing here is to convert all these drivers to use cache_bypass to read directly through the cache - the same thing will then work with direct regmap usage.

I don't complete understand "use cache_bypass to read directly through the cache".
Can you illustrate it?
The purpose of original implementation is to read h/w then fill the cache. The cache is zeroed before we call alc5623_fill_cache().

Or do you mean changes like below instead of calling regmap_read directly?

sound/soc/codecs/alc5623.c +7 -2
index 557b3af..43006b3 100644
@@ -51,10 +51,15 @@ static void alc5623_fill_cache(struct snd_soc_codec *codec)
 {
        int i, step = codec->driver->reg_cache_step;
        u16 *cache = codec->reg_cache;
+       unsigned int val;
 
        /* not really efficient ... */
-       for (i = 0 ; i < codec->driver->reg_cache_size ; i += step)
-               cache[i] = codec->hw_read(codec, i);
+       codec->cache_bypass = 1;
+       for (i = 0 ; i < codec->driver->reg_cache_size ; i += step) {
+               snd_soc_read(codec, i);
+               cache[i] = val;
+       }
+       codec->cache_bypass = 0;
 }
 
 static inline int alc5623_reset(struct snd_soc_codec *codec)

Thanks,
Axel

#5 lore raw
Quoted 8 lines · Axel Lin

2011/10/14 Mark Brown <[email protected]>:

Quoted 3 lines

Probably the best thing here is to convert all these drivers to use cache_bypass to read directly through the cache - the same thing will then work with direct regmap usage.

I don't complete understand "use cache_bypass to read directly through the cache".

Set the cache_bypass flag in the device struct.

Quoted 1 line

Or do you mean changes like below instead of calling regmap_read directly?

No, this isn't going to work. snd_soc_read() will use the cache.

author
#6 lore raw

2011/10/14 Mark Brown <[email protected]>:

Quoted 12 lines · Axel Lin
Quoted 8 lines

2011/10/14 Mark Brown <[email protected]>:

Quoted 3 lines

Probably the best thing here is to convert all these drivers to use cache_bypass to read directly through the cache - the same thing will then work with direct regmap usage.

I don't complete understand "use cache_bypass to read directly through the cache".

Set the cache_bypass flag in the device struct.

Sorry. I still don't get it.

I check the regmap code, it seems cache_bypsss is only set if map->cache_type == REGCACHE_NONE or by regcache_cache_bypass().

I'm wondering if we can fix it this way:

sound/soc/codecs/alc5623.c +7 -2
index 557b3af..9e43bb9 100644
@@ -51,10 +51,15 @@ static void alc5623_fill_cache(struct snd_soc_codec *codec)
 {
        int i, step = codec->driver->reg_cache_step;
        u16 *cache = codec->reg_cache;
+       unsigned int val;
 
        /* not really efficient ... */
-       for (i = 0 ; i < codec->driver->reg_cache_size ; i += step)
-               cache[i] = codec->hw_read(codec, i);
+       regcache_cache_bypass(codec->control_data, true);
+       for (i = 0 ; i < codec->driver->reg_cache_size ; i += step) {
+               regmap_read(codec->control_data, i, &val);
+               cache[i] = val;
+       }
+       regcache_cache_bypass(codec->control_data, false);
 }
 
 static inline int alc5623_reset(struct snd_soc_codec *codec)
#7 lore raw
Quoted 2 lines · Axel Lin

I check the regmap code, it seems cache_bypsss is only set if map->cache_type == REGCACHE_NONE or by regcache_cache_bypass().

That's in regmap. Any ASoC drivers which haven't been converted to directly use regmap will still use the ASoC copy of the cache code.

sound/soc/codecs/alc5623.c static void alc5623_fill_cache(struct snd_soc_codec *codec) · line 62

Quoted 6 lines
sound/soc/codecs/alc5623.c +6 -0
+       regcache_cache_bypass(codec->control_data, true);
+       for (i = 0 ; i < codec->driver->reg_cache_size ; i += step) {
+               regmap_read(codec->control_data, i, &val);
+               cache[i] = val;
+       }
+       regcache_cache_bypass(codec->control_data, false);

This is what a driver which uses regmap directly should do.

#8 lore raw
In reply to Axel Lin

sound/soc/codecs/alc5623.c static void alc5623_fill_cache(struct snd_soc_codec *codec) · line 62

Quoted 6 lines · Axel Lin
sound/soc/codecs/alc5623.c +6 -0
+       codec->cache_bypass = 1;
+       for (i = 0 ; i < codec->driver->reg_cache_size ; i += step) {
+               snd_soc_read(codec, i);
+               cache[i] = val;
+       }
+       codec->cache_bypass = 0;

Oh, sorry - I misread this and this is what I'm expecting.