Add "Playback Switch" mixer control to mute or unmute speaker
playback from ucm conf depend on use cases.
Signed-off-by: Ajit Kumar Pandey <[email protected]>
---
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
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.
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 <[email protected]>
The "Playback Switch" is too short. It should be more specific (Headphone,
Speaker etc.).
Jaroslav
--
Jaroslav Kysela <[email protected]>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
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.
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.
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/[email protected]/#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.
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/[email protected]/#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.
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.