[PATCH] ASoC: wm8731: initialize the hardware when loading the codec driver; use regmap for I/O

[PATCH] ASoC: wm8731: initialize the hardware when loading the codec driver; use regmap for I/O

[Sergey Kiselev]

Hi,

This patch moves the request supplies and hardware reset and initialization
from wm8731_probe to wm8731_i2c_probe and wm8731_spi_probe. So that the
codec hardware is initialized when loading the codec driver, and not when
loading the machine driver.
The I/O is converted to regmap_write and regmap_update_bits. (Mostly as a side
effect, since snd_soc_write and friends won't work before snd_soc_register_codec
is called)

Signed-off-by: Sergey Kiselev <sergey.kiselev@intel.com>
---
 wm8731.c |  107 ++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 72 insertions(+), 35 deletions(-)

diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 2245b6a..05fa4d6 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -84,7 +84,7 @@ static bool wm8731_writeable(struct device *dev, unsigned int reg)
 	return reg <= WM8731_RESET;
 }
 
-#define wm8731_reset(c)	snd_soc_write(c, WM8731_RESET, 0)
+#define wm8731_reset(m)	regmap_write(m, WM8731_RESET, 0)
 
 static const char *wm8731_input_select[] = {"Line In", "Mic"};
 
@@ -118,7 +118,7 @@ static int wm8731_set_deemph(struct snd_soc_codec *codec)
 	dev_dbg(codec->dev, "Set deemphasis %d (%dHz)\n",
 		best, wm8731_deemph[best]);
 
-	return snd_soc_update_bits(codec, WM8731_APDIGI, 0x6, val);
+	return regmap_update_bits(wm8731->regmap, WM8731_APDIGI, 0x6, val);
 }
 
 static int wm8731_get_deemph(struct snd_kcontrol *kcontrol,
@@ -344,7 +344,7 @@ static int wm8731_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_codec *codec = dai->codec;
 	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
-	u16 iface = snd_soc_read(codec, WM8731_IFACE) & 0xfff3;
+	unsigned int format = 0;
 	int i = get_coeff(wm8731->sysclk, params_rate(params));
 	u16 srate = (coeff_div[i].sr << 2) |
 		(coeff_div[i].bosr << 1) | coeff_div[i].usb;
@@ -358,28 +358,27 @@ static int wm8731_hw_params(struct snd_pcm_substream *substream,
 	case 16:
 		break;
 	case 20:
-		iface |= 0x0004;
+		format = 0x4;
 		break;
 	case 24:
-		iface |= 0x0008;
+		format = 0x8;
 		break;
 	}
 
 	wm8731_set_deemph(codec);
 
-	snd_soc_write(codec, WM8731_IFACE, iface);
-	return 0;
+	return regmap_update_bits(wm8731->regmap, WM8731_IFACE, 0x0c, format);
 }
 
 static int wm8731_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_codec *codec = dai->codec;
-	u16 mute_reg = snd_soc_read(codec, WM8731_APDIGI) & 0xfff7;
+	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
 
 	if (mute)
-		snd_soc_write(codec, WM8731_APDIGI, mute_reg | 0x8);
+		regmap_update_bits(wm8731->regmap, WM8731_APDIGI, 0x8, 0x8);
 	else
-		snd_soc_write(codec, WM8731_APDIGI, mute_reg);
+		regmap_update_bits(wm8731->regmap, WM8731_APDIGI, 0x8, 0);
 	return 0;
 }
 
@@ -431,6 +430,7 @@ static int wm8731_set_dai_fmt(struct snd_soc_dai *codec_dai,
 		unsigned int fmt)
 {
 	struct snd_soc_codec *codec = codec_dai->codec;
+	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
 	u16 iface = 0;
 
 	/* set master/slave audio interface */
@@ -482,7 +482,7 @@ static int wm8731_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	}
 
 	/* set iface */
-	snd_soc_write(codec, WM8731_IFACE, iface);
+	regmap_write(wm8731->regmap, WM8731_IFACE, iface);
 	return 0;
 }
 
@@ -491,7 +491,6 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
 {
 	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
 	int ret;
-	u16 reg;
 
 	switch (level) {
 	case SND_SOC_BIAS_ON:
@@ -511,13 +510,12 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
 		}
 
 		/* Clear PWROFF, gate CLKOUT, everything else as-is */
-		reg = snd_soc_read(codec, WM8731_PWR) & 0xff7f;
-		snd_soc_write(codec, WM8731_PWR, reg | 0x0040);
+		regmap_update_bits(wm8731->regmap, WM8731_PWR, 0xc0, 0x40);
 		break;
 	case SND_SOC_BIAS_OFF:
 		if (wm8731->mclk)
 			clk_disable_unprepare(wm8731->mclk);
-		snd_soc_write(codec, WM8731_PWR, 0xffff);
+		regmap_write(wm8731->regmap, WM8731_PWR, 0xff);
 		regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
 				       wm8731->supplies);
 		regcache_mark_dirty(wm8731->regmap);
@@ -573,42 +571,53 @@ static struct snd_soc_dai_driver wm8731_dai = {
 
 static int wm8731_probe(struct snd_soc_codec *codec)
 {
-	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
-	int ret = 0, i;
+	wm8731_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
+	return 0;
+}
+
+static int wm8731_request_supplies(struct device *dev,
+		struct wm8731_priv *wm8731)
+{
+	int ret = 0, i ;
 
 	for (i = 0; i < ARRAY_SIZE(wm8731->supplies); i++)
 		wm8731->supplies[i].supply = wm8731_supply_names[i];
 
-	ret = devm_regulator_bulk_get(codec->dev, ARRAY_SIZE(wm8731->supplies),
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(wm8731->supplies),
 				 wm8731->supplies);
 	if (ret != 0) {
-		dev_err(codec->dev, "Failed to request supplies: %d\n", ret);
+		dev_err(dev, "Failed to request supplies: %d\n", ret);
 		return ret;
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(wm8731->supplies),
 				    wm8731->supplies);
 	if (ret != 0) {
-		dev_err(codec->dev, "Failed to enable supplies: %d\n", ret);
+		dev_err(dev, "Failed to enable supplies: %d\n", ret);
 		return ret;
 	}
 
-	ret = wm8731_reset(codec);
+	return 0;
+}
+
+static int wm8731_hw_init(struct device *dev, struct wm8731_priv *wm8731)
+{
+	int ret = 0;
+
+	ret = wm8731_reset(wm8731->regmap);
 	if (ret < 0) {
-		dev_err(codec->dev, "Failed to issue reset: %d\n", ret);
+		dev_err(dev, "Failed to issue reset: %d\n", ret);
 		goto err_regulator_enable;
 	}
 
-	wm8731_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
-
-	/* Latch the update bits */
-	snd_soc_update_bits(codec, WM8731_LOUT1V, 0x100, 0);
-	snd_soc_update_bits(codec, WM8731_ROUT1V, 0x100, 0);
-	snd_soc_update_bits(codec, WM8731_LINVOL, 0x100, 0);
-	snd_soc_update_bits(codec, WM8731_RINVOL, 0x100, 0);
+	/* Clear POWEROFF, keep everything else disabled */
+	regmap_write(wm8731->regmap, WM8731_PWR, 0x7f);
 
 	/* Disable bypass path by default */
-	snd_soc_update_bits(codec, WM8731_APANA, 0x8, 0);
+	regmap_update_bits(wm8731->regmap, WM8731_APANA, 0x8, 0);
+
+	regcache_mark_dirty(wm8731->regmap);
 
 	/* Regulators will have been enabled by bias management */
 	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
@@ -624,9 +633,7 @@ err_regulator_enable:
 /* power down chip */
 static int wm8731_remove(struct snd_soc_codec *codec)
 {
-	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
-
-	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
+	wm8731_set_bias_level(codec, SND_SOC_BIAS_OFF);
 
 	return 0;
 }
@@ -690,6 +697,13 @@ static int wm8731_spi_probe(struct spi_device *spi)
 
 	mutex_init(&wm8731->lock);
 
+	spi_set_drvdata(spi, wm8731);
+
+	ret = wm8731_request_supplies(&spi->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
+
 	wm8731->regmap = devm_regmap_init_spi(spi, &wm8731_regmap);
 	if (IS_ERR(wm8731->regmap)) {
 		ret = PTR_ERR(wm8731->regmap);
@@ -698,7 +712,10 @@ static int wm8731_spi_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	spi_set_drvdata(spi, wm8731);
+	ret = wm8731_hw_init(&spi->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
 
 	ret = snd_soc_register_codec(&spi->dev,
 			&soc_codec_dev_wm8731, &wm8731_dai, 1);
@@ -712,7 +729,12 @@ static int wm8731_spi_probe(struct spi_device *spi)
 
 static int wm8731_spi_remove(struct spi_device *spi)
 {
+	struct wm8731_priv *wm8731 = spi_get_drvdata(spi);
+
 	snd_soc_unregister_codec(&spi->dev);
+
+	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
+
 	return 0;
 }
 
@@ -754,6 +776,13 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
 
 	mutex_init(&wm8731->lock);
 
+	i2c_set_clientdata(i2c, wm8731);
+
+	ret = wm8731_request_supplies(&i2c->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
+
 	wm8731->regmap = devm_regmap_init_i2c(i2c, &wm8731_regmap);
 	if (IS_ERR(wm8731->regmap)) {
 		ret = PTR_ERR(wm8731->regmap);
@@ -762,7 +791,10 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	i2c_set_clientdata(i2c, wm8731);
+	ret = wm8731_hw_init(&i2c->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
 
 	ret = snd_soc_register_codec(&i2c->dev,
 			&soc_codec_dev_wm8731, &wm8731_dai, 1);
@@ -776,7 +808,12 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
 
 static int wm8731_i2c_remove(struct i2c_client *client)
 {
+	struct wm8731_priv *wm8731 = i2c_get_clientdata(client);
+
 	snd_soc_unregister_codec(&client->dev);
+
+	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
+
 	return 0;
 }
 

--
Sergey

Re: [PATCH] ASoC: wm8731: initialize the hardware when loading the codec driver; use regmap for I/O

[Lars-Peter Clausen]

On 05/30/2015 09:34 AM, Sergey Kiselev wrote:
> Hi,
>
> This patch moves the request supplies and hardware reset and initialization
> from wm8731_probe to wm8731_i2c_probe and wm8731_spi_probe. So that the
> codec hardware is initialized when loading the codec driver, and not when
> loading the machine driver.
> The I/O is converted to regmap_write and regmap_update_bits. (Mostly as a side
> effect, since snd_soc_write and friends won't work before snd_soc_register_codec
> is called)
>
> Signed-off-by: Sergey Kiselev <sergey.kiselev@intel.com>

Hi,

It looks like the patch has been made against an older tree. It will not 
apply on-top of ASoC for-next branch.

> ---
[...]
>   static const char *wm8731_input_select[] = {"Line In", "Mic"};
>
> @@ -118,7 +118,7 @@ static int wm8731_set_deemph(struct snd_soc_codec *codec)
>   	dev_dbg(codec->dev, "Set deemphasis %d (%dHz)\n",
>   		best, wm8731_deemph[best]);
>
> -	return snd_soc_update_bits(codec, WM8731_APDIGI, 0x6, val);
> +	return regmap_update_bits(wm8731->regmap, WM8731_APDIGI, 0x6, val);

There is no need to replace all the snd_soc_${io}() calls with 
regmap_${io}(). The snd_soc methods will just call the corresponding regmap 
method internally. But if you really want to do it this should be done in a 
separate patch.

>   }
>
[...]
> @@ -573,42 +571,53 @@ static struct snd_soc_dai_driver wm8731_dai = {
>
>   static int wm8731_probe(struct snd_soc_codec *codec)
>   {
> -	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
> -	int ret = 0, i;
> +	wm8731_set_bias_level(codec, SND_SOC_BIAS_STANDBY);

This is not necessary, the core will put the CODEC into BIAS_STANDBY mode, 
after the probe callback has been called. So the whole probe function can be 
dropped.

> +
> +	return 0;
> +}
[...]
>   static int wm8731_remove(struct snd_soc_codec *codec)
>   {
> -	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
> -
> -	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
> +	wm8731_set_bias_level(codec, SND_SOC_BIAS_OFF);

Same here, the core already puts the CODEC into BIAS_OFF mode before the 
remove callback is called.

>
>   	return 0;
>   }

Re: [PATCH] ASoC: wm8731: initialize the hardware when loading the codec driver; use regmap for I/O

[Sergey Kiselev]

Hi,

I generated a new patch that fixes the issues indicated by Lars.

On Mon, 1 Jun 2015 11:03:09 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:
> It looks like the patch has been made against an older tree. It will 
> not apply on-top of ASoC for-next branch.
> 

Rebased.

> There is no need to replace all the snd_soc_${io}() calls with 
> regmap_${io}(). The snd_soc methods will just call the corresponding 
> regmap method internally. But if you really want to do it this should 
> be done in a separate patch.
> 

I kept the snd_soc_${io} except of reset and wm8731_hw_init, that are called before codec is registered (so snd_soc_${io} can't be used there). I'll generate a separate patch for regmap transistion.

> This is not necessary, the core will put the CODEC into BIAS_STANDBY 
> mode, after the probe callback has been called. So the whole probe 
> function can be dropped.
> 
> 
> Same here, the core already puts the CODEC into BIAS_OFF mode before 
> the remove callback is called.
> 

wm8731_probe and wm8731_remove callbacks removed.

This patch moves the request supplies and hardware reset and initialization from wm8731_probe to wm8731_i2c_probe and wm8731_spi_probe. So that the codec hardware is initialized when loading the codec driver, and not when loading the machine driver.

Signed-off-by: Sergey Kiselev <sergey.kiselev@intel.com>
---
 wm8731.c |   82 +++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 30 deletions(-)

diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 915ea11..8b19205 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -84,7 +84,7 @@ static bool wm8731_writeable(struct device *dev, unsigned int reg)
 	return reg <= WM8731_RESET;
 }
 
-#define wm8731_reset(c)	snd_soc_write(c, WM8731_RESET, 0)
+#define wm8731_reset(m)	regmap_write(m, WM8731_RESET, 0)
 
 static const char *wm8731_input_select[] = {"Line In", "Mic"};
 
@@ -571,44 +571,48 @@ static struct snd_soc_dai_driver wm8731_dai = {
 	.symmetric_rates = 1,
 };
 
-static int wm8731_probe(struct snd_soc_codec *codec)
+static int wm8731_request_supplies(struct device *dev,
+		struct wm8731_priv *wm8731)
 {
-	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
 	int ret = 0, i;
 
 	for (i = 0; i < ARRAY_SIZE(wm8731->supplies); i++)
 		wm8731->supplies[i].supply = wm8731_supply_names[i];
 
-	ret = devm_regulator_bulk_get(codec->dev, ARRAY_SIZE(wm8731->supplies),
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(wm8731->supplies),
 				 wm8731->supplies);
 	if (ret != 0) {
-		dev_err(codec->dev, "Failed to request supplies: %d\n", ret);
+		dev_err(dev, "Failed to request supplies: %d\n", ret);
 		return ret;
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(wm8731->supplies),
 				    wm8731->supplies);
 	if (ret != 0) {
-		dev_err(codec->dev, "Failed to enable supplies: %d\n", ret);
+		dev_err(dev, "Failed to enable supplies: %d\n", ret);
 		return ret;
 	}
 
-	ret = wm8731_reset(codec);
+	return 0;
+}
+
+static int wm8731_hw_init(struct device *dev, struct wm8731_priv *wm8731)
+{
+	int ret = 0;
+
+	ret = wm8731_reset(wm8731->regmap);
 	if (ret < 0) {
-		dev_err(codec->dev, "Failed to issue reset: %d\n", ret);
+		dev_err(dev, "Failed to issue reset: %d\n", ret);
 		goto err_regulator_enable;
 	}
 
-	snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_STANDBY);
-
-	/* Latch the update bits */
-	snd_soc_update_bits(codec, WM8731_LOUT1V, 0x100, 0);
-	snd_soc_update_bits(codec, WM8731_ROUT1V, 0x100, 0);
-	snd_soc_update_bits(codec, WM8731_LINVOL, 0x100, 0);
-	snd_soc_update_bits(codec, WM8731_RINVOL, 0x100, 0);
+	/* Clear POWEROFF, keep everything else disabled */
+	regmap_write(wm8731->regmap, WM8731_PWR, 0x7f);
 
 	/* Disable bypass path by default */
-	snd_soc_update_bits(codec, WM8731_APANA, 0x8, 0);
+	regmap_update_bits(wm8731->regmap, WM8731_APANA, 0x8, 0);
+
+	regcache_mark_dirty(wm8731->regmap);
 
 	/* Regulators will have been enabled by bias management */
 	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
@@ -621,19 +625,7 @@ err_regulator_enable:
 	return ret;
 }
 
-/* power down chip */
-static int wm8731_remove(struct snd_soc_codec *codec)
-{
-	struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
-
-	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
-
-	return 0;
-}
-
 static struct snd_soc_codec_driver soc_codec_dev_wm8731 = {
-	.probe =	wm8731_probe,
-	.remove =	wm8731_remove,
 	.set_bias_level = wm8731_set_bias_level,
 	.suspend_bias_off = true,
 
@@ -690,6 +682,13 @@ static int wm8731_spi_probe(struct spi_device *spi)
 
 	mutex_init(&wm8731->lock);
 
+	spi_set_drvdata(spi, wm8731);
+
+	ret = wm8731_request_supplies(&spi->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
+
 	wm8731->regmap = devm_regmap_init_spi(spi, &wm8731_regmap);
 	if (IS_ERR(wm8731->regmap)) {
 		ret = PTR_ERR(wm8731->regmap);
@@ -698,7 +697,10 @@ static int wm8731_spi_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	spi_set_drvdata(spi, wm8731);
+	ret = wm8731_hw_init(&spi->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
 
 	ret = snd_soc_register_codec(&spi->dev,
 			&soc_codec_dev_wm8731, &wm8731_dai, 1);
@@ -712,7 +714,12 @@ static int wm8731_spi_probe(struct spi_device *spi)
 
 static int wm8731_spi_remove(struct spi_device *spi)
 {
+	struct wm8731_priv *wm8731 = spi_get_drvdata(spi);
+
 	snd_soc_unregister_codec(&spi->dev);
+
+	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
+
 	return 0;
 }
 
@@ -754,6 +761,13 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
 
 	mutex_init(&wm8731->lock);
 
+	i2c_set_clientdata(i2c, wm8731);
+
+	ret = wm8731_request_supplies(&i2c->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
+
 	wm8731->regmap = devm_regmap_init_i2c(i2c, &wm8731_regmap);
 	if (IS_ERR(wm8731->regmap)) {
 		ret = PTR_ERR(wm8731->regmap);
@@ -762,7 +776,10 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	i2c_set_clientdata(i2c, wm8731);
+	ret = wm8731_hw_init(&i2c->dev, wm8731);
+	if (ret != 0) {
+		return ret;
+	}
 
 	ret = snd_soc_register_codec(&i2c->dev,
 			&soc_codec_dev_wm8731, &wm8731_dai, 1);
@@ -776,7 +793,12 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
 
 static int wm8731_i2c_remove(struct i2c_client *client)
 {
+	struct wm8731_priv *wm8731 = i2c_get_clientdata(client);
+
 	snd_soc_unregister_codec(&client->dev);
+
+	regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies), wm8731->supplies);
+
 	return 0;
 }
 
--
Sergey

Re: [PATCH] ASoC: wm8731: initialize the hardware when loading the codec driver; use regmap for I/O

[Mark Brown]

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

On Tue, Jun 02, 2015 at 11:33:18AM -0700, Sergey Kiselev wrote:
> Hi,
> 
> I generated a new patch that fixes the issues indicated by Lars.

Don't top post, and if you want me to review this patch you're going to
need to send it as covered in SubmittingPatches.

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

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