[PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Ajit Kumar Pandey]

Add "Playback Switch" mixer control to mute or unmute speaker
playback from ucm conf depend on use cases.

Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
---
 sound/soc/codecs/max98357a.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/sound/soc/codecs/max98357a.c b/sound/soc/codecs/max98357a.c
index 918812763884..9b2f16ab4498 100644
--- a/sound/soc/codecs/max98357a.c
+++ b/sound/soc/codecs/max98357a.c
@@ -73,6 +73,36 @@ static int max98357a_sdmode_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int speaker_mute_get(struct snd_kcontrol *kcontrol,
+			    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.enumerated.item[0] = max98357a->sdmode_switch;
+
+	return 0;
+}
+
+static int speaker_mute_put(struct snd_kcontrol *kcontrol,
+			    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
+	int mode = ucontrol->value.enumerated.item[0];
+
+	max98357a->sdmode_switch = mode;
+	gpiod_set_value_cansleep(max98357a->sdmode, mode);
+	dev_dbg(component->dev, "set sdmode to %d", mode);
+
+	return 0;
+}
+
+static const struct snd_kcontrol_new max98357a_snd_controls[] = {
+	SOC_SINGLE_BOOL_EXT("Playback Switch", 0,
+			    speaker_mute_get, speaker_mute_put),
+};
+
 static const struct snd_soc_dapm_widget max98357a_dapm_widgets[] = {
 	SND_SOC_DAPM_OUTPUT("Speaker"),
 	SND_SOC_DAPM_OUT_DRV_E("SD_MODE", SND_SOC_NOPM, 0, 0, NULL, 0,
@@ -86,6 +116,8 @@ static const struct snd_soc_dapm_route max98357a_dapm_routes[] = {
 };
 
 static const struct snd_soc_component_driver max98357a_component_driver = {
+	.controls		= max98357a_snd_controls,
+	.num_controls		= ARRAY_SIZE(max98357a_snd_controls),
 	.dapm_widgets		= max98357a_dapm_widgets,
 	.num_dapm_widgets	= ARRAY_SIZE(max98357a_dapm_widgets),
 	.dapm_routes		= max98357a_dapm_routes,
-- 
2.25.1

[PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Ajit Kumar Pandey]

Add "Playback Switch" mixer control to mute or unmute speaker
playback from ucm conf depend on use cases.

Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
---
 sound/soc/codecs/max98357a.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/sound/soc/codecs/max98357a.c b/sound/soc/codecs/max98357a.c
index 918812763884..9b2f16ab4498 100644
--- a/sound/soc/codecs/max98357a.c
+++ b/sound/soc/codecs/max98357a.c
@@ -73,6 +73,36 @@ static int max98357a_sdmode_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int speaker_mute_get(struct snd_kcontrol *kcontrol,
+			    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.enumerated.item[0] = max98357a->sdmode_switch;
+
+	return 0;
+}
+
+static int speaker_mute_put(struct snd_kcontrol *kcontrol,
+			    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
+	int mode = ucontrol->value.enumerated.item[0];
+
+	max98357a->sdmode_switch = mode;
+	gpiod_set_value_cansleep(max98357a->sdmode, mode);
+	dev_dbg(component->dev, "set sdmode to %d", mode);
+
+	return 0;
+}
+
+static const struct snd_kcontrol_new max98357a_snd_controls[] = {
+	SOC_SINGLE_BOOL_EXT("Playback Switch", 0,
+			    speaker_mute_get, speaker_mute_put),
+};
+
 static const struct snd_soc_dapm_widget max98357a_dapm_widgets[] = {
 	SND_SOC_DAPM_OUTPUT("Speaker"),
 	SND_SOC_DAPM_OUT_DRV_E("SD_MODE", SND_SOC_NOPM, 0, 0, NULL, 0,
@@ -86,6 +116,8 @@ static const struct snd_soc_dapm_route max98357a_dapm_routes[] = {
 };
 
 static const struct snd_soc_component_driver max98357a_component_driver = {
+	.controls		= max98357a_snd_controls,
+	.num_controls		= ARRAY_SIZE(max98357a_snd_controls),
 	.dapm_widgets		= max98357a_dapm_widgets,
 	.num_dapm_widgets	= ARRAY_SIZE(max98357a_dapm_widgets),
 	.dapm_routes		= max98357a_dapm_routes,
-- 
2.25.1

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On Thu, Dec 09, 2021 at 12:28:49AM +0530, Ajit Kumar Pandey wrote:

> +static int speaker_mute_put(struct snd_kcontrol *kcontrol,
> +			    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
> +	int mode = ucontrol->value.enumerated.item[0];
> +
> +	max98357a->sdmode_switch = mode;
> +	gpiod_set_value_cansleep(max98357a->sdmode, mode);
> +	dev_dbg(component->dev, "set sdmode to %d", mode);

This looks like it should just be a DAPM widget - it's just a generic
GPIO control, there's no connection with the CODEC that I can see so it
definitely shouldn't be in the CODEC driver.  Often trivial stuff like
this is done in the machine driver, though the simple-amplifier driver
is probably a good fit here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On Thu, Dec 09, 2021 at 12:28:49AM +0530, Ajit Kumar Pandey wrote:

> +static int speaker_mute_put(struct snd_kcontrol *kcontrol,
> +			    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
> +	int mode = ucontrol->value.enumerated.item[0];
> +
> +	max98357a->sdmode_switch = mode;
> +	gpiod_set_value_cansleep(max98357a->sdmode, mode);
> +	dev_dbg(component->dev, "set sdmode to %d", mode);

This looks like it should just be a DAPM widget - it's just a generic
GPIO control, there's no connection with the CODEC that I can see so it
definitely shouldn't be in the CODEC driver.  Often trivial stuff like
this is done in the machine driver, though the simple-amplifier driver
is probably a good fit here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Jaroslav Kysela]

On 08. 12. 21 19:58, Ajit Kumar Pandey wrote:
> Add "Playback Switch" mixer control to mute or unmute speaker
> playback from ucm conf depend on use cases.
> 
> Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

The "Playback Switch" is too short. It should be more specific (Headphone, 
Speaker etc.).

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Jaroslav Kysela]

On 08. 12. 21 19:58, Ajit Kumar Pandey wrote:
> Add "Playback Switch" mixer control to mute or unmute speaker
> playback from ucm conf depend on use cases.
> 
> Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

The "Playback Switch" is too short. It should be more specific (Headphone, 
Speaker etc.).

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

On Wed, Dec 08, 2021 at 09:25:04PM +0100, Jaroslav Kysela wrote:
> On 08. 12. 21 19:58, Ajit Kumar Pandey wrote:
> > Add "Playback Switch" mixer control to mute or unmute speaker
> > playback from ucm conf depend on use cases.

> The "Playback Switch" is too short. It should be more specific (Headphone,
> Speaker etc.).

The device is a speaker driver, it's likely to be getting a prefix added
as it's bound into the machine driver if there's any issues here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

On Wed, Dec 08, 2021 at 09:25:04PM +0100, Jaroslav Kysela wrote:
> On 08. 12. 21 19:58, Ajit Kumar Pandey wrote:
> > Add "Playback Switch" mixer control to mute or unmute speaker
> > playback from ucm conf depend on use cases.

> The "Playback Switch" is too short. It should be more specific (Headphone,
> Speaker etc.).

The device is a speaker driver, it's likely to be getting a prefix added
as it's bound into the machine driver if there's any issues here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Wed, Dec 08, 2021 at 08:21:31PM +0000, Mark Brown wrote:
> On Thu, Dec 09, 2021 at 12:28:49AM +0530, Ajit Kumar Pandey wrote:
> 
> > +static int speaker_mute_put(struct snd_kcontrol *kcontrol,
> > +			    struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> > +	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
> > +	int mode = ucontrol->value.enumerated.item[0];
> > +
> > +	max98357a->sdmode_switch = mode;
> > +	gpiod_set_value_cansleep(max98357a->sdmode, mode);
> > +	dev_dbg(component->dev, "set sdmode to %d", mode);
> 
> This looks like it should just be a DAPM widget - it's just a generic
> GPIO control, there's no connection with the CODEC that I can see so it
> definitely shouldn't be in the CODEC driver.  Often trivial stuff like
> this is done in the machine driver, though the simple-amplifier driver
> is probably a good fit here.

Actually now I look again the only control interface this driver has is
GPIOs but it does expose a digital interface with constraints as input
so doesn't fit within simple-amplifier.  However this is just powering
off the amplifier to achieve mute rather than a separate mute control so
it'd probably be better to use a SND_SOC_DAPM_PIN_SWITCH() for the
Speaker widget to do this, this would also end up addressing Jaroslav's
thing with the naming as a side effect.  Sorry about the confusion there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Wed, Dec 08, 2021 at 08:21:31PM +0000, Mark Brown wrote:
> On Thu, Dec 09, 2021 at 12:28:49AM +0530, Ajit Kumar Pandey wrote:
> 
> > +static int speaker_mute_put(struct snd_kcontrol *kcontrol,
> > +			    struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> > +	struct max98357a_priv *max98357a = snd_soc_component_get_drvdata(component);
> > +	int mode = ucontrol->value.enumerated.item[0];
> > +
> > +	max98357a->sdmode_switch = mode;
> > +	gpiod_set_value_cansleep(max98357a->sdmode, mode);
> > +	dev_dbg(component->dev, "set sdmode to %d", mode);
> 
> This looks like it should just be a DAPM widget - it's just a generic
> GPIO control, there's no connection with the CODEC that I can see so it
> definitely shouldn't be in the CODEC driver.  Often trivial stuff like
> this is done in the machine driver, though the simple-amplifier driver
> is probably a good fit here.

Actually now I look again the only control interface this driver has is
GPIOs but it does expose a digital interface with constraints as input
so doesn't fit within simple-amplifier.  However this is just powering
off the amplifier to achieve mute rather than a separate mute control so
it'd probably be better to use a SND_SOC_DAPM_PIN_SWITCH() for the
Speaker widget to do this, this would also end up addressing Jaroslav's
thing with the naming as a side effect.  Sorry about the confusion there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Ajit Kumar Pandey]

On 12/9/2021 2:10 AM, Mark Brown wrote:
> Actually now I look again the only control interface this driver has is
> GPIOs but it does expose a digital interface with constraints as input
> so doesn't fit within simple-amplifier.  However this is just powering
> off the amplifier to achieve mute rather than a separate mute control so
> it'd probably be better to use a SND_SOC_DAPM_PIN_SWITCH() for the
> Speaker widget to do this, this would also end up addressing Jaroslav's
> thing with the naming as a side effect.  Sorry about the confusion there.

Thanks for suggestion. We tried using SND_SOC_DAPM_PIN_SWITCH() for the 
speaker widget and it invoke dapm_event callback based on switch i.e 
max98357a_sdmode_event() but codec driver isn't enabling/disabling gpios 
in such event callback instead they are doing that in dai_ops trigger 
callback. In our platform single I2S controller instance (cpu-dai) is 
connected to two different endpoints with a single PCM device, hence we 
want to switch or enable/disable output based on Machine driver controls 
only.

Initially we thought to configure gpio within sdmode_event callback but
there was some pop noise issue reported in one platform with that change
hence reverted. Check 
https://patchwork.kernel.org/project/alsa-devel/patch/20200721114232.2812254-1-tzungbi@google.com/#23502085
So we thought of exposing a mixer control to enable/disable amp from UCM
in our platform without breaking existing functionality. Please let us
know any other alternative way if possible.

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Ajit Kumar Pandey]

On 12/9/2021 2:10 AM, Mark Brown wrote:
> Actually now I look again the only control interface this driver has is
> GPIOs but it does expose a digital interface with constraints as input
> so doesn't fit within simple-amplifier.  However this is just powering
> off the amplifier to achieve mute rather than a separate mute control so
> it'd probably be better to use a SND_SOC_DAPM_PIN_SWITCH() for the
> Speaker widget to do this, this would also end up addressing Jaroslav's
> thing with the naming as a side effect.  Sorry about the confusion there.

Thanks for suggestion. We tried using SND_SOC_DAPM_PIN_SWITCH() for the 
speaker widget and it invoke dapm_event callback based on switch i.e 
max98357a_sdmode_event() but codec driver isn't enabling/disabling gpios 
in such event callback instead they are doing that in dai_ops trigger 
callback. In our platform single I2S controller instance (cpu-dai) is 
connected to two different endpoints with a single PCM device, hence we 
want to switch or enable/disable output based on Machine driver controls 
only.

Initially we thought to configure gpio within sdmode_event callback but
there was some pop noise issue reported in one platform with that change
hence reverted. Check 
https://patchwork.kernel.org/project/alsa-devel/patch/20200721114232.2812254-1-tzungbi@google.com/#23502085
So we thought of exposing a mixer control to enable/disable amp from UCM
in our platform without breaking existing functionality. Please let us
know any other alternative way if possible.

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On Thu, Dec 16, 2021 at 05:54:45PM +0530, Ajit Kumar Pandey wrote:

> Thanks for suggestion. We tried using SND_SOC_DAPM_PIN_SWITCH() for the
> speaker widget and it invoke dapm_event callback based on switch i.e
> max98357a_sdmode_event() but codec driver isn't enabling/disabling gpios in
> such event callback instead they are doing that in dai_ops trigger callback.
> In our platform single I2S controller instance (cpu-dai) is connected to two
> different endpoints with a single PCM device, hence we want to switch or
> enable/disable output based on Machine driver controls only.

DAPM should cope perfectly fine with this setup...

> Initially we thought to configure gpio within sdmode_event callback but
> there was some pop noise issue reported in one platform with that change
> hence reverted. Check https://patchwork.kernel.org/project/alsa-devel/patch/20200721114232.2812254-1-tzungbi@google.com/#23502085
> So we thought of exposing a mixer control to enable/disable amp from UCM
> in our platform without breaking existing functionality. Please let us
> know any other alternative way if possible.

Whatever is going on this should be managed from the driver rather than
having a direct control, especially given the issues I mentioned with
there being zero coordination between this and the management that the
driver already does.  You could have DAPM controls set a variable and
coordinate with whatever you're doing in the pcm_ops, I'm not clear what
the use case is for having the manual control TBH.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On Thu, Dec 16, 2021 at 05:54:45PM +0530, Ajit Kumar Pandey wrote:

> Thanks for suggestion. We tried using SND_SOC_DAPM_PIN_SWITCH() for the
> speaker widget and it invoke dapm_event callback based on switch i.e
> max98357a_sdmode_event() but codec driver isn't enabling/disabling gpios in
> such event callback instead they are doing that in dai_ops trigger callback.
> In our platform single I2S controller instance (cpu-dai) is connected to two
> different endpoints with a single PCM device, hence we want to switch or
> enable/disable output based on Machine driver controls only.

DAPM should cope perfectly fine with this setup...

> Initially we thought to configure gpio within sdmode_event callback but
> there was some pop noise issue reported in one platform with that change
> hence reverted. Check https://patchwork.kernel.org/project/alsa-devel/patch/20200721114232.2812254-1-tzungbi@google.com/#23502085
> So we thought of exposing a mixer control to enable/disable amp from UCM
> in our platform without breaking existing functionality. Please let us
> know any other alternative way if possible.

Whatever is going on this should be managed from the driver rather than
having a direct control, especially given the issues I mentioned with
there being zero coordination between this and the management that the
driver already does.  You could have DAPM controls set a variable and
coordinate with whatever you're doing in the pcm_ops, I'm not clear what
the use case is for having the manual control TBH.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Ajit Kumar Pandey]

On 12/16/2021 7:00 PM, Mark Brown wrote:
> DAPM should cope perfectly fine with this setup...

Ok Thanks , we will re-look our DAPM graph and comeback.
We can drop this change for now.

Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker

[Ajit Kumar Pandey]

On 12/16/2021 7:00 PM, Mark Brown wrote:
> DAPM should cope perfectly fine with this setup...

Ok Thanks , we will re-look our DAPM graph and comeback.
We can drop this change for now.