| Message ID | 20200204102016.I73b26b5e319de173d05823e79f5861bf1826261c@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: max98357a: add speaker switch | expand |
On Tue, Feb 04, 2020 at 11:04:06AM +0800, Tzung-Bi Shih wrote: > Some machine may share the same I2S lines for multiple codecs. For > example, mediatek/mt8183/mt8183-da7219-max98357 shares the same lines > between max98357a and da7219. When writing audio data through the I2S > lines, all codecs on the lines would try to generate sound if they > accepts DO line. As a result, multiple codecs generate sound at a > time. Rather than adding this in the driver it would be better to add some _PIN_SWITCH() widgets to the speaker outputs, those exist for essentially this purpose. > + max98357a->spk_switch = ucontrol->value.integer.value[0]; > + dev_info(component->dev, > + "put speaker switch: %d\n", max98357a->spk_switch); These _info() prints are too noisy.
On Tue, Feb 11, 2020 at 2:38 AM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 04, 2020 at 11:04:06AM +0800, Tzung-Bi Shih wrote: > > > Some machine may share the same I2S lines for multiple codecs. For > > example, mediatek/mt8183/mt8183-da7219-max98357 shares the same lines > > between max98357a and da7219. When writing audio data through the I2S > > lines, all codecs on the lines would try to generate sound if they > > accepts DO line. As a result, multiple codecs generate sound at a > > time. > > Rather than adding this in the driver it would be better to add some > _PIN_SWITCH() widgets to the speaker outputs, those exist for > essentially this purpose. (We take rockchip/rk3399_gru_sound.c as a reference to use SOC_DAPM_PIN_SWITCH.) Did you mean (in machine driver): - Add SND_SOC_DAPM_SPK("Speakers", NULL) - Add SOC_DAPM_PIN_SWITCH("Speakers") - Add DAPM route "Speaker" (from max98357a) to "Speakers" User space program controls "Speakers Switch" to toggle the switch? We found the method cannot avoid max98357a->sdmode being set. As a result, max98357a consumes power even if we don't switch on it.
On Tue, Feb 11, 2020 at 06:32:20PM +0800, Tzung-Bi Shih wrote: > Did you mean (in machine driver): > - Add SND_SOC_DAPM_SPK("Speakers", NULL) > - Add SOC_DAPM_PIN_SWITCH("Speakers") > - Add DAPM route "Speaker" (from max98357a) to "Speakers" > User space program controls "Speakers Switch" to toggle the switch? Yes. > We found the method cannot avoid max98357a->sdmode being set. As a > result, max98357a consumes power even if we don't switch on it. That seems like something that you can address by improving integration with DAPM.
diff --git a/sound/soc/codecs/max98357a.c b/sound/soc/codecs/max98357a.c index 16313b973eaa..a2c3be69a0ee 100644 --- a/sound/soc/codecs/max98357a.c +++ b/sound/soc/codecs/max98357a.c @@ -22,6 +22,7 @@ struct max98357a_priv { struct gpio_desc *sdmode; unsigned int sdmode_delay; + int spk_switch; }; static int max98357a_daiops_trigger(struct snd_pcm_substream *substream, @@ -29,7 +30,7 @@ static int max98357a_daiops_trigger(struct snd_pcm_substream *substream, { struct max98357a_priv *max98357a = snd_soc_dai_get_drvdata(dai); - if (!max98357a->sdmode) + if (!max98357a->sdmode || !max98357a->spk_switch) return 0; switch (cmd) { @@ -49,6 +50,37 @@ static int max98357a_daiops_trigger(struct snd_pcm_substream *substream, return 0; } +static int max98357a_get_spk_switch(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.integer.value[0] = max98357a->spk_switch; + return 0; +} + +static int max98357a_put_spk_switch(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); + + max98357a->spk_switch = ucontrol->value.integer.value[0]; + dev_info(component->dev, + "put speaker switch: %d\n", max98357a->spk_switch); + return 0; +} + +static const struct snd_kcontrol_new max98357a_snd_controls[] = { + SOC_SINGLE_BOOL_EXT("Speaker Switch", 0, + max98357a_get_spk_switch, max98357a_put_spk_switch), +}; + static const struct snd_soc_dapm_widget max98357a_dapm_widgets[] = { SND_SOC_DAPM_OUTPUT("Speaker"), }; @@ -58,6 +90,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, @@ -117,6 +151,9 @@ static int max98357a_platform_probe(struct platform_device *pdev) "default: no delay\n"); } + /* For drivers who are not aware of the switch, default set to on. */ + max98357a->spk_switch = 1; + dev_set_drvdata(&pdev->dev, max98357a); return devm_snd_soc_register_component(&pdev->dev,
Some machine may share the same I2S lines for multiple codecs. For example, mediatek/mt8183/mt8183-da7219-max98357 shares the same lines between max98357a and da7219. When writing audio data through the I2S lines, all codecs on the lines would try to generate sound if they accepts DO line. As a result, multiple codecs generate sound at a time. Adds a separate switch to max98357a. Userspace program has choices to turn on or off the switch. Note that, userspace program should change the switch before opening the stream. The switch won't take effects if the stream is already there. Default value of the switch is on to not break existing driver usages (who are unlikely aware of existence of the switch). Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- sound/soc/codecs/max98357a.c | 39 +++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)