| Message ID | 20211208185850.1555996-1-AjitKumar.Pandey@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: max98357a: Add mixer control to mute/unmute speaker | expand |
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 <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.
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-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.
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.
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 --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,
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(+)