[alsa-devel] [PATCH 1/2 resend] ASoC: cs4271: switch to mute_stream
Use the newly introduced mute_stream DAI operation, and don't mute the codec if it's called for the _CAPTURE stream.
Signed-off-by: Daniel Mack zonque@gmail.com Acked-by: Alexander Sverdlin subaparts@yandex.ru --- sound/soc/codecs/cs4271.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c index ac0d3b4..03036b3 100644 --- a/sound/soc/codecs/cs4271.c +++ b/sound/soc/codecs/cs4271.c @@ -388,7 +388,7 @@ static int cs4271_hw_params(struct snd_pcm_substream *substream, return cs4271_set_deemph(codec); }
-static int cs4271_digital_mute(struct snd_soc_dai *dai, int mute) +static int cs4271_mute_stream(struct snd_soc_dai *dai, int mute, int stream) { struct snd_soc_codec *codec = dai->codec; struct cs4271_private *cs4271 = snd_soc_codec_get_drvdata(codec); @@ -396,6 +396,9 @@ static int cs4271_digital_mute(struct snd_soc_dai *dai, int mute) int val_a = 0; int val_b = 0;
+ if (stream != SNDRV_PCM_STREAM_PLAYBACK) + return 0; + if (mute) { val_a = CS4271_VOLA_MUTE; val_b = CS4271_VOLB_MUTE; @@ -442,7 +445,7 @@ static const struct snd_soc_dai_ops cs4271_dai_ops = { .hw_params = cs4271_hw_params, .set_sysclk = cs4271_set_dai_sysclk, .set_fmt = cs4271_set_dai_fmt, - .digital_mute = cs4271_digital_mute, + .mute_stream = cs4271_mute_stream, };
static struct snd_soc_dai_driver cs4271_dai = {
Currently, both the ALSA control for "Digital Master Playback Switch" and the ALSA core (by calling dai_ops->mute_stream()) control the same bits in the CS4271_VOL[AB]_MUTE registers.
That's a problem for applications which intentionally want to keep the flag switched off from userspace, even though the stream is already playing.
Fix this by keeping track of the states on both sides - the ALSA control and the ASoC core - and actually mute the Codec if either one of the two flags is set.
Signed-off-by: Daniel Mack zonque@gmail.com Reported-by: Michael Hirsch m.hirsch@raumfeld.com Acked-by: Alexander Sverdlin subaparts@yandex.ru --- sound/soc/codecs/cs4271.c | 74 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 21 deletions(-)
diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c index 03036b3..3a2ab48 100644 --- a/sound/soc/codecs/cs4271.c +++ b/sound/soc/codecs/cs4271.c @@ -171,6 +171,9 @@ struct cs4271_private { int gpio_disable; /* enable soft reset workaround */ bool enable_soft_reset; + /* keep track of playback mute flags */ + bool mute_control; + bool digital_mute; };
/* @@ -281,6 +284,51 @@ static int cs4271_put_deemph(struct snd_kcontrol *kcontrol, return cs4271_set_deemph(codec); }
+static int cs4271_set_mute(struct snd_soc_codec *codec) +{ + struct cs4271_private *cs4271 = snd_soc_codec_get_drvdata(codec); + int val_a = 0; + int val_b = 0; + int ret; + + if (cs4271->digital_mute || cs4271->mute_control) { + val_a = CS4271_VOLA_MUTE; + val_b = CS4271_VOLB_MUTE; + } + + ret = regmap_update_bits(cs4271->regmap, CS4271_VOLA, + CS4271_VOLA_MUTE, val_a); + if (ret < 0) + return ret; + + ret = regmap_update_bits(cs4271->regmap, CS4271_VOLB, + CS4271_VOLB_MUTE, val_b); + if (ret < 0) + return ret; + + return 0; +} + +static int cs4271_get_playback_switch(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct cs4271_private *cs4271 = snd_soc_codec_get_drvdata(codec); + + ucontrol->value.enumerated.item[0] = !cs4271->mute_control; + return 0; +} + +static int cs4271_put_playback_switch(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct cs4271_private *cs4271 = snd_soc_codec_get_drvdata(codec); + + cs4271->mute_control = !ucontrol->value.enumerated.item[0]; + return cs4271_set_mute(codec); +} + struct cs4271_clk_cfg { bool master; /* codec mode */ u8 speed_mode; /* codec speed mode: 1x, 2x, 4x */ @@ -392,28 +440,12 @@ static int cs4271_mute_stream(struct snd_soc_dai *dai, int mute, int stream) { struct snd_soc_codec *codec = dai->codec; struct cs4271_private *cs4271 = snd_soc_codec_get_drvdata(codec); - int ret; - int val_a = 0; - int val_b = 0;
- if (stream != SNDRV_PCM_STREAM_PLAYBACK) - return 0; - - if (mute) { - val_a = CS4271_VOLA_MUTE; - val_b = CS4271_VOLB_MUTE; + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + cs4271->digital_mute = mute; + return cs4271_set_mute(codec); }
- ret = regmap_update_bits(cs4271->regmap, CS4271_VOLA, - CS4271_VOLA_MUTE, val_a); - if (ret < 0) - return ret; - - ret = regmap_update_bits(cs4271->regmap, CS4271_VOLB, - CS4271_VOLB_MUTE, val_b); - if (ret < 0) - return ret; - return 0; }
@@ -437,8 +469,8 @@ static const struct snd_kcontrol_new cs4271_snd_controls[] = { SOC_DOUBLE("Master Capture Switch", CS4271_ADCCTL, 3, 2, 1, 1), SOC_SINGLE("Dither 16-Bit Data Switch", CS4271_ADCCTL, 5, 1, 0), SOC_DOUBLE("High Pass Filter Switch", CS4271_ADCCTL, 1, 0, 1, 1), - SOC_DOUBLE_R("Master Playback Switch", CS4271_VOLA, CS4271_VOLB, - 7, 1, 1), + SOC_SINGLE_BOOL_EXT("Master Playback Switch", 0, + cs4271_get_playback_switch, cs4271_put_playback_switch), };
static const struct snd_soc_dai_ops cs4271_dai_ops = {
On Thu, Mar 21, 2013 at 08:43:55PM +0100, Daniel Mack wrote:
Currently, both the ALSA control for "Digital Master Playback Switch" and the ALSA core (by calling dai_ops->mute_stream()) control the same bits in the CS4271_VOL[AB]_MUTE registers.
That's a problem for applications which intentionally want to keep the flag switched off from userspace, even though the stream is already playing.
Fix this by keeping track of the states on both sides - the ALSA control and the ASoC core - and actually mute the Codec if either one of the two flags is set.
The usual fix for this is to just not have the mute operation if it's important. Otherwise this seems like something we ought to implement in the core, it's not a silly feature by any stretch of the imagination but it applies to any device with a DAI mute unless there's some device specific thing I'm missing here.
Hi Mark,
On 22.03.2013 11:18, Mark Brown wrote:
On Thu, Mar 21, 2013 at 08:43:55PM +0100, Daniel Mack wrote:
Currently, both the ALSA control for "Digital Master Playback Switch" and the ALSA core (by calling dai_ops->mute_stream()) control the same bits in the CS4271_VOL[AB]_MUTE registers.
That's a problem for applications which intentionally want to keep the flag switched off from userspace, even though the stream is already playing.
Fix this by keeping track of the states on both sides - the ALSA control and the ASoC core - and actually mute the Codec if either one of the two flags is set.
The usual fix for this is to just not have the mute operation if it's important. Otherwise this seems like something we ought to implement in the core, it's not a silly feature by any stretch of the imagination but it applies to any device with a DAI mute unless there's some device specific thing I'm missing here.
Well, the thing is about different use cases. If you leave everything untouched as of the ALSA controls (unmuted), the codec will enter its mute state just as expected by the core. The register bit also has implications to external mute stages btw, which board designers usually implement with MOSFETs.
It's just that if the user intentionally mutes the stream via the ALSA control, the core will override it, thus changing the state of the actual control unexpectedly.
So I'd say having a simple OR logic in this case is reasonable and serves all the needs I can think of, no?
Thanks, Daniel
On Fri, Mar 22, 2013 at 11:23:32AM +0100, Daniel Mack wrote:
On 22.03.2013 11:18, Mark Brown wrote:
The usual fix for this is to just not have the mute operation if it's important. Otherwise this seems like something we ought to implement in the core, it's not a silly feature by any stretch of the imagination but it applies to any device with a DAI mute unless there's some device specific thing I'm missing here.
So I'd say having a simple OR logic in this case is reasonable and serves all the needs I can think of, no?
You're not really responding to what I'm saying here. What I'm saying is that if this is useful it should be implemented in the core unless there's some reason I'm missing why this is specific to this driver.
On 22.03.2013 11:32, Mark Brown wrote:
On Fri, Mar 22, 2013 at 11:23:32AM +0100, Daniel Mack wrote:
On 22.03.2013 11:18, Mark Brown wrote:
The usual fix for this is to just not have the mute operation if it's important. Otherwise this seems like something we ought to implement in the core, it's not a silly feature by any stretch of the imagination but it applies to any device with a DAI mute unless there's some device specific thing I'm missing here.
So I'd say having a simple OR logic in this case is reasonable and serves all the needs I can think of, no?
You're not really responding to what I'm saying here. What I'm saying is that if this is useful it should be implemented in the core unless there's some reason I'm missing why this is specific to this driver.
Ok, point taken. It's just not really a drop-in equivalent, given that both the numid and the name string will change then.
I'll see when I'll find some time to do that. Your idea is to have a "Master Playback Switch" for both the CPU and the Codec on each link, if the driver implements either ->mute_stream or ->digital_mute, right? And the control would default to "on"?
Thanks, Daniel
On Fri, Mar 22, 2013 at 12:04:54PM +0100, Daniel Mack wrote:
On 22.03.2013 11:32, Mark Brown wrote:
You're not really responding to what I'm saying here. What I'm saying is that if this is useful it should be implemented in the core unless there's some reason I'm missing why this is specific to this driver.
Ok, point taken. It's just not really a drop-in equivalent, given that both the numid and the name string will change then.
Well, we could probably arrange something for the name string (and I'd think we'd want to).
I'll see when I'll find some time to do that. Your idea is to have a "Master Playback Switch" for both the CPU and the Codec on each link, if the driver implements either ->mute_stream or ->digital_mute, right? And the control would default to "on"?
Yes, something like that. Providing a way to set the name of the control would be good but it could be done as a stage two. I'm still swithering about applying the patch, it's driver local and won't really interfere with any generic support that comes along.
On Thu, Mar 21, 2013 at 08:43:55PM +0100, Daniel Mack wrote:
Currently, both the ALSA control for "Digital Master Playback Switch" and the ALSA core (by calling dai_ops->mute_stream()) control the same bits in the CS4271_VOL[AB]_MUTE registers.
Applied, thanks.
On Thu, Mar 21, 2013 at 08:43:54PM +0100, Daniel Mack wrote:
Use the newly introduced mute_stream DAI operation, and don't mute the codec if it's called for the _CAPTURE stream.
Applied, thanks.
participants (2)
-
Daniel Mack -
Mark Brown