[PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
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,
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 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/20200721114232.2812254... 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/20200721114232.2812254... 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 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
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.
participants (3)
-
Ajit Kumar Pandey -
Jaroslav Kysela -
Mark Brown