* [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
@ 2021-12-08 18:58 Ajit Kumar Pandey
2021-12-08 20:21 ` Mark Brown
2021-12-08 20:25 ` Jaroslav Kysela
0 siblings, 2 replies; 8+ messages in thread
From: Ajit Kumar Pandey @ 2021-12-08 18:58 UTC (permalink / raw)
To: broonie, alsa-devel
Cc: Vijendar.Mukunda, Alexander.Deucher, Basavaraj.Hiregoudar,
Sunil-kumar.Dommati, Ajit Kumar Pandey, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, open list
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
2021-12-08 18:58 [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker Ajit Kumar Pandey
@ 2021-12-08 20:21 ` Mark Brown
2021-12-08 20:40 ` Mark Brown
2021-12-08 20:25 ` Jaroslav Kysela
1 sibling, 1 reply; 8+ messages in thread
From: Mark Brown @ 2021-12-08 20:21 UTC (permalink / raw)
To: Ajit Kumar Pandey
Cc: alsa-devel, Vijendar.Mukunda, Alexander.Deucher,
Basavaraj.Hiregoudar, Sunil-kumar.Dommati, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, open list
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
2021-12-08 18:58 [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker Ajit Kumar Pandey
2021-12-08 20:21 ` Mark Brown
@ 2021-12-08 20:25 ` Jaroslav Kysela
2021-12-08 20:33 ` Mark Brown
1 sibling, 1 reply; 8+ messages in thread
From: Jaroslav Kysela @ 2021-12-08 20:25 UTC (permalink / raw)
To: Ajit Kumar Pandey, broonie, alsa-devel
Cc: Vijendar.Mukunda, Alexander.Deucher, Basavaraj.Hiregoudar,
Sunil-kumar.Dommati, Liam Girdwood, Takashi Iwai, open list
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
2021-12-08 20:25 ` Jaroslav Kysela
@ 2021-12-08 20:33 ` Mark Brown
0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2021-12-08 20:33 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: Ajit Kumar Pandey, alsa-devel, Vijendar.Mukunda,
Alexander.Deucher, Basavaraj.Hiregoudar, Sunil-kumar.Dommati,
Liam Girdwood, Takashi Iwai, open list
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
2021-12-08 20:21 ` Mark Brown
@ 2021-12-08 20:40 ` Mark Brown
2021-12-16 12:24 ` Ajit Kumar Pandey
0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2021-12-08 20:40 UTC (permalink / raw)
To: Ajit Kumar Pandey
Cc: alsa-devel, Vijendar.Mukunda, Alexander.Deucher,
Basavaraj.Hiregoudar, Sunil-kumar.Dommati, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, open list
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
2021-12-08 20:40 ` Mark Brown
@ 2021-12-16 12:24 ` Ajit Kumar Pandey
2021-12-16 13:30 ` Mark Brown
0 siblings, 1 reply; 8+ messages in thread
From: Ajit Kumar Pandey @ 2021-12-16 12:24 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, Vijendar.Mukunda, Alexander.Deucher,
Basavaraj.Hiregoudar, Sunil-kumar.Dommati, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, open list
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
2021-12-16 12:24 ` Ajit Kumar Pandey
@ 2021-12-16 13:30 ` Mark Brown
2021-12-17 13:58 ` Ajit Kumar Pandey
0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2021-12-16 13:30 UTC (permalink / raw)
To: Ajit Kumar Pandey
Cc: alsa-devel, Vijendar.Mukunda, Alexander.Deucher,
Basavaraj.Hiregoudar, Sunil-kumar.Dommati, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, open list
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
2021-12-16 13:30 ` Mark Brown
@ 2021-12-17 13:58 ` Ajit Kumar Pandey
0 siblings, 0 replies; 8+ messages in thread
From: Ajit Kumar Pandey @ 2021-12-17 13:58 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, Vijendar.Mukunda, Alexander.Deucher,
Basavaraj.Hiregoudar, Sunil-kumar.Dommati, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, open list
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-12-17 13:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-08 18:58 [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker Ajit Kumar Pandey
2021-12-08 20:21 ` Mark Brown
2021-12-08 20:40 ` Mark Brown
2021-12-16 12:24 ` Ajit Kumar Pandey
2021-12-16 13:30 ` Mark Brown
2021-12-17 13:58 ` Ajit Kumar Pandey
2021-12-08 20:25 ` Jaroslav Kysela
2021-12-08 20:33 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).