[PATCH 1/2 resend] ASoC: cs4271: switch to mute_stream

[PATCH 1/2 resend] ASoC: cs4271: switch to mute_stream

[Daniel Mack]

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 = {
-- 
1.8.1.4

[PATCH 2/2 resend] ASoC: cs4271: preserve "Master Playback Switch" setting

[Daniel Mack]

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 = {
-- 
1.8.1.4

Re: [PATCH 1/2 resend] ASoC: cs4271: switch to mute_stream

[Mark Brown]

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

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.

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

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

Re: [PATCH 2/2 resend] ASoC: cs4271: preserve "Master Playback Switch" setting

[Mark Brown]

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

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.

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

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

Re: [PATCH 2/2 resend] ASoC: cs4271: preserve "Master Playback Switch" setting

[Daniel Mack]

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

Re: [PATCH 2/2 resend] ASoC: cs4271: preserve "Master Playback Switch" setting

[Mark Brown]

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

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.

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

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

Re: [PATCH 2/2 resend] ASoC: cs4271: preserve "Master Playback Switch" setting

[Daniel Mack]

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

Re: [PATCH 2/2 resend] ASoC: cs4271: preserve "Master Playback Switch" setting

[Mark Brown]

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

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.

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

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

Re: [PATCH 2/2 resend] ASoC: cs4271: preserve "Master Playback Switch" setting

[Mark Brown]

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

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.

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

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