diff mbox series

ASoC: max98357a: Add mixer control to mute/unmute speaker

Message ID 20211208185850.1555996-1-AjitKumar.Pandey@amd.com
State New
Headers show
Series ASoC: max98357a: Add mixer control to mute/unmute speaker | expand

Commit Message

Ajit Kumar Pandey Dec. 8, 2021, 6:58 p.m. UTC
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(+)

Comments

Mark Brown Dec. 8, 2021, 8:21 p.m. UTC | #1
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.
Mark Brown Dec. 8, 2021, 8:33 p.m. UTC | #2
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.
Ajit Kumar Pandey Dec. 16, 2021, 12:24 p.m. UTC | #3
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.
Mark Brown Dec. 16, 2021, 1:30 p.m. UTC | #4
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.
Ajit Kumar Pandey Dec. 17, 2021, 1:58 p.m. UTC | #5
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.
diff mbox series

Patch

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,