[alsa-devel] [PATCH 1/2] ASoC: wm8804: Fix small issues in probe error path
The regulator notifiers were not being cleared up on the error path in wm8804_probe and the nothing was being cleared up if snd_soc_register_codec failed. This patch fixes these issues.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com --- sound/soc/codecs/wm8804.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/wm8804.c b/sound/soc/codecs/wm8804.c index 1bd4ace..6131c2a 100644 --- a/sound/soc/codecs/wm8804.c +++ b/sound/soc/codecs/wm8804.c @@ -607,6 +607,7 @@ int wm8804_probe(struct device *dev, struct regmap *regmap) dev_err(dev, "Failed to register regulator notifier: %d\n", ret); + return ret; } }
@@ -614,7 +615,7 @@ int wm8804_probe(struct device *dev, struct regmap *regmap) wm8804->supplies); if (ret) { dev_err(dev, "Failed to enable supplies: %d\n", ret); - goto err_reg_enable; + goto err_reg_notify; }
ret = regmap_read(regmap, WM8804_RST_DEVID1, &id1); @@ -651,11 +652,22 @@ int wm8804_probe(struct device *dev, struct regmap *regmap) goto err_reg_enable; }
- return snd_soc_register_codec(dev, &soc_codec_dev_wm8804, - &wm8804_dai, 1); + ret = snd_soc_register_codec(dev, &soc_codec_dev_wm8804, + &wm8804_dai, 1); + if (ret < 0) { + dev_err(dev, "Failed to register CODEC: %d\n", ret); + goto err_reg_enable; + } + + return 0;
err_reg_enable: regulator_bulk_disable(ARRAY_SIZE(wm8804->supplies), wm8804->supplies); +err_reg_notify: + for (i = 0; i < ARRAY_SIZE(wm8804->supplies); ++i) + regulator_unregister_notifier(wm8804->supplies[i].consumer, + &wm8804->disable_nb[i]); + return ret; } EXPORT_SYMBOL_GPL(wm8804_probe);
From: Sapthagiri Baratam sapthagiri.baratam@incubesol.com
Signed-off-by: Sapthagiri Baratam sapthagiri.baratam@incubesol.com Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com ---
This does cause some changes to the ALSA controls I wanted to get peoples opinion on. I think there are a couple of users for this driver.
Firstly, 'Input Source' changes names to 'Tx Source' this is a lot clearer, but perhaps we shouldn't change this, as it doesn't have to change?
Secondly, the patch removes the controls 'TX Playback Switch' and 'AIF Playback Switch' as these bits are controlled by DAPM now. I wasn't sure if it would be preferrable to leave dummy controls that do nothing?
Thanks, Charles
sound/soc/codecs/wm8804.c | 140 ++++++++++++++++++++++++++------------------ 1 files changed, 83 insertions(+), 57 deletions(-)
diff --git a/sound/soc/codecs/wm8804.c b/sound/soc/codecs/wm8804.c index 6131c2a..7a04dc4 100644 --- a/sound/soc/codecs/wm8804.c +++ b/sound/soc/codecs/wm8804.c @@ -24,6 +24,7 @@ #include <sound/soc.h> #include <sound/initval.h> #include <sound/tlv.h> +#include <sound/soc-dapm.h>
#include "wm8804.h"
@@ -61,14 +62,16 @@ struct wm8804_priv { struct regulator_bulk_data supplies[WM8804_NUM_SUPPLIES]; struct notifier_block disable_nb[WM8804_NUM_SUPPLIES]; int mclk_div; -};
-static int txsrc_get(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol); + int aif_pwr; +};
static int txsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol);
+static int wm8804_aif_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event); + /* * We can't use the same notifier block for more than one supply and * there's no way I can see to get from a callback to the caller @@ -90,26 +93,62 @@ WM8804_REGULATOR_EVENT(0) WM8804_REGULATOR_EVENT(1)
static const char *txsrc_text[] = { "S/PDIF RX", "AIF" }; -static SOC_ENUM_SINGLE_EXT_DECL(txsrc, txsrc_text); +static const SOC_ENUM_SINGLE_DECL(txsrc, WM8804_SPDTX4, 6, txsrc_text);
-static const struct snd_kcontrol_new wm8804_snd_controls[] = { - SOC_ENUM_EXT("Input Source", txsrc, txsrc_get, txsrc_put), - SOC_SINGLE("TX Playback Switch", WM8804_PWRDN, 2, 1, 1), - SOC_SINGLE("AIF Playback Switch", WM8804_PWRDN, 4, 1, 1) +static const struct snd_kcontrol_new wm8804_tx_source_mux[] = { + SOC_DAPM_ENUM_EXT("Tx Source", txsrc, + snd_soc_dapm_get_enum_double, txsrc_put), };
-static int txsrc_get(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct snd_soc_codec *codec; - unsigned int src; +static const struct snd_soc_dapm_widget wm8804_dapm_widgets[] = { +SND_SOC_DAPM_OUTPUT("SPDIF Out"), +SND_SOC_DAPM_INPUT("SPDIF In"), + +SND_SOC_DAPM_PGA("SPDIFTX", WM8804_PWRDN, 2, 1, NULL, 0), +SND_SOC_DAPM_PGA("SPDIFRX", WM8804_PWRDN, 1, 1, NULL, 0), + +SND_SOC_DAPM_MUX("Tx Source", SND_SOC_NOPM, 6, 0, wm8804_tx_source_mux), + +SND_SOC_DAPM_AIF_OUT_E("AIFTX", NULL, 0, SND_SOC_NOPM, 0, 0, wm8804_aif_event, + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), +SND_SOC_DAPM_AIF_IN_E("AIFRX", NULL, 0, SND_SOC_NOPM, 0, 0, wm8804_aif_event, + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), +}; + +static const struct snd_soc_dapm_route wm8804_dapm_routes[] = { + { "AIFRX", NULL, "Playback" }, + { "Tx Source", "AIF", "AIFRX" }, + + { "SPDIFRX", NULL, "SPDIF In" }, + { "Tx Source", "S/PDIF RX", "SPDIFRX" }, + + { "SPDIFTX", NULL, "Tx Source" }, + { "SPDIF Out", NULL, "SPDIFTX" },
- codec = snd_soc_kcontrol_codec(kcontrol); - src = snd_soc_read(codec, WM8804_SPDTX4); - if (src & 0x40) - ucontrol->value.integer.value[0] = 1; - else - ucontrol->value.integer.value[0] = 0; + { "AIFTX", NULL, "SPDIFRX" }, + { "Capture", NULL, "AIFTX" }, +}; + +static int wm8804_aif_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm); + struct wm8804_priv *wm8804 = snd_soc_codec_get_drvdata(codec); + + switch (event) { + case SND_SOC_DAPM_POST_PMU: + /* power up the aif */ + if (!wm8804->aif_pwr) + snd_soc_update_bits(codec, WM8804_PWRDN, 0x10, 0x0); + wm8804->aif_pwr++; + break; + case SND_SOC_DAPM_POST_PMD: + /* power down only both paths are disabled */ + wm8804->aif_pwr--; + if (!wm8804->aif_pwr) + snd_soc_update_bits(codec, WM8804_PWRDN, 0x10, 0x10); + break; + }
return 0; } @@ -117,48 +156,33 @@ static int txsrc_get(struct snd_kcontrol *kcontrol, static int txsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { - struct snd_soc_codec *codec; - unsigned int src, txpwr; + struct snd_soc_codec *codec = snd_soc_dapm_kcontrol_codec(kcontrol); + struct snd_soc_dapm_context *dapm = &codec->dapm; + struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; + unsigned int val = ucontrol->value.enumerated.item[0] << e->shift_l; + unsigned int mask = 1 << e->shift_l; + unsigned int txpwr; + + if (val != 0 && val != mask) + return -EINVAL;
- codec = snd_soc_kcontrol_codec(kcontrol); + snd_soc_dapm_mutex_lock(dapm);
- if (ucontrol->value.integer.value[0] != 0 - && ucontrol->value.integer.value[0] != 1) - return -EINVAL; + if (snd_soc_test_bits(codec, e->reg, mask, val)) { + /* save the current power state of the transmitter */ + txpwr = snd_soc_read(codec, WM8804_PWRDN) & 0x4;
- src = snd_soc_read(codec, WM8804_SPDTX4); - switch ((src & 0x40) >> 6) { - case 0: - if (!ucontrol->value.integer.value[0]) - return 0; - break; - case 1: - if (ucontrol->value.integer.value[1]) - return 0; - break; - } + /* power down the transmitter */ + snd_soc_update_bits(codec, WM8804_PWRDN, 0x4, 0x4);
- /* save the current power state of the transmitter */ - txpwr = snd_soc_read(codec, WM8804_PWRDN) & 0x4; - /* power down the transmitter */ - snd_soc_update_bits(codec, WM8804_PWRDN, 0x4, 0x4); - /* set the tx source */ - snd_soc_update_bits(codec, WM8804_SPDTX4, 0x40, - ucontrol->value.integer.value[0] << 6); - - if (ucontrol->value.integer.value[0]) { - /* power down the receiver */ - snd_soc_update_bits(codec, WM8804_PWRDN, 0x2, 0x2); - /* power up the AIF */ - snd_soc_update_bits(codec, WM8804_PWRDN, 0x10, 0); - } else { - /* don't power down the AIF -- may be used as an output */ - /* power up the receiver */ - snd_soc_update_bits(codec, WM8804_PWRDN, 0x2, 0); + /* set the tx source */ + snd_soc_update_bits(codec, e->reg, mask, val); + + /* restore the transmitter's configuration */ + snd_soc_update_bits(codec, WM8804_PWRDN, 0x4, txpwr); }
- /* restore the transmitter's configuration */ - snd_soc_update_bits(codec, WM8804_PWRDN, 0x4, txpwr); + snd_soc_dapm_mutex_unlock(dapm);
return 0; } @@ -555,8 +579,10 @@ static const struct snd_soc_codec_driver soc_codec_dev_wm8804 = { .set_bias_level = wm8804_set_bias_level, .idle_bias_off = true,
- .controls = wm8804_snd_controls, - .num_controls = ARRAY_SIZE(wm8804_snd_controls), + .dapm_widgets = wm8804_dapm_widgets, + .num_dapm_widgets = ARRAY_SIZE(wm8804_dapm_widgets), + .dapm_routes = wm8804_dapm_routes, + .num_dapm_routes = ARRAY_SIZE(wm8804_dapm_routes), };
const struct regmap_config wm8804_regmap_config = {
On Mon, Mar 02, 2015 at 05:57:55PM +0000, Charles Keepax wrote:
Firstly, 'Input Source' changes names to 'Tx Source' this is a lot clearer, but perhaps we shouldn't change this, as it doesn't have to change?
Yes, leave it be. If you were renaming this should be in the changelog with an explanation as to why.
Secondly, the patch removes the controls 'TX Playback Switch' and 'AIF Playback Switch' as these bits are controlled by DAPM now. I wasn't sure if it would be preferrable to leave dummy controls that do nothing?
This sounds like you need a more detailed changelog entry explaining why it's a good idea to do this in the first place, never mind the proxy control - what is the purpose of this change and what benefit will it bring? Given that this is a S/PDIF bridge it's not at all obvious that the chip will be used in applications where the sort of power savings you're likely to see from powering down one of the links (as opposed to cutting the power to the chip) are likely to register. Why move those bits to DAPM control at all?
I also note that this patch is apparently written by but not CCed to Sapthagiri Baratam.
On Mon, Mar 02, 2015 at 06:16:13PM +0000, Mark Brown wrote:
On Mon, Mar 02, 2015 at 05:57:55PM +0000, Charles Keepax wrote:
Firstly, 'Input Source' changes names to 'Tx Source' this is a lot clearer, but perhaps we shouldn't change this, as it doesn't have to change?
Yes, leave it be. If you were renaming this should be in the changelog with an explanation as to why.
Cool can do.
Secondly, the patch removes the controls 'TX Playback Switch' and 'AIF Playback Switch' as these bits are controlled by DAPM now. I wasn't sure if it would be preferrable to leave dummy controls that do nothing?
This sounds like you need a more detailed changelog entry explaining why it's a good idea to do this in the first place, never mind the proxy control - what is the purpose of this change and what benefit will it bring? Given that this is a S/PDIF bridge it's not at all obvious that the chip will be used in applications where the sort of power savings you're likely to see from powering down one of the links (as opposed to cutting the power to the chip) are likely to register. Why move those bits to DAPM control at all?
Hmm... I hadn't really thought about it like that, mostly I had viewed this somewhat as a nice modernization of the driver. I am pretty sure there are some benefits around the clocking from disabling the RX path (which is never disabled currently) as this exercises some automatic control over the PLL whilst active, which can be difficult to handle. But I guess that could also just be handled by adding a control to disable it. Let us try redrafting the commit message and we can see where that takes us I guess.
I also note that this patch is apparently written by but not CCed to Sapthagiri Baratam.
He is on the patches alias, but yes that was rather remiss of me not to directly CC him.
Thanks, Charles
On Mon, Mar 02, 2015 at 05:57:54PM +0000, Charles Keepax wrote:
The regulator notifiers were not being cleared up on the error path in wm8804_probe and the nothing was being cleared up if snd_soc_register_codec failed. This patch fixes these issues.
Why not fix this at source by adding a devm_ notifier registration?
On Mon, Mar 02, 2015 at 06:03:51PM +0000, Mark Brown wrote:
On Mon, Mar 02, 2015 at 05:57:54PM +0000, Charles Keepax wrote:
The regulator notifiers were not being cleared up on the error path in wm8804_probe and the nothing was being cleared up if snd_soc_register_codec failed. This patch fixes these issues.
Why not fix this at source by adding a devm_ notifier registration?
Oops.. yeah that is almost certainly better. I will respin for that.
Thanks, Charles
participants (2)
-
Charles Keepax -
Mark Brown