| Message ID | 20221028102450.1161382-3-ajye_huang@compal.corp-partner.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add optional dmic selection for two DMICs | expand |
On 10/28/2022 12:24 PM, Ajye Huang wrote: > Having two DMICs, a front DMIC and a rear DMIC, > but only host audio input AUX port0 is used for these two Dmics. > A "dmic_sel-gpios" property is used for a mixer control to switch > the dmic signal source between the Front and Rear Dmic. > > usage: amixer -c0 cset name='Dmic Mux' 'FrontMic' > usage: amixer -c0 cset name='Dmic Mux' 'RearMic' > > Refer to this one as an example, > commit 3cfbf07c6d27 > ("ASoC: qcom: sc7180: Modify machine driver for 2mic") > > Signed-off-by: Ajye Huang <ajye_huang@compal.corp-partner.google.com> > --- I'm very suspicious of this patchset. As it is you add kcontrol which won't take effect on most platforms making use of DMIC. It feels to me that it is something you want to handle on machine driver side instead.
On Fri, Oct 28, 2022 at 06:24:50PM +0800, Ajye Huang wrote: > + dmic->dmic_sel = devm_gpiod_get_optional(component->dev, > + "dmic_sel", GPIOD_OUT_LOW); > + if (IS_ERR(dmic->dmic_sel)) > + return PTR_ERR(dmic->dmic_sel); > + > snd_soc_component_set_drvdata(component, dmic); > > return 0; > @@ -125,10 +172,15 @@ static const struct snd_soc_dapm_widget dmic_dapm_widgets[] = { > SND_SOC_NOPM, 0, 0, dmic_aif_event, > SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), > SND_SOC_DAPM_INPUT("DMic"), > + SND_SOC_DAPM_MIC("DMIC", NULL), > + SND_SOC_DAPM_MUX("Dmic Mux", SND_SOC_NOPM, 0, 0, &dmic_mux_control), If we are doing this then adding the mux needs to be conditional on having the GPIO, without the GPIO the control is at best confusing to users.
Hi Amadeusz, Yes, the original version I tried the implementation on audio machine driver, but one person gave me an idea for this dmic.c Do you think it is appropriate on dmic.c? If it isn't, I will add kcontrol into audio machine driver. thanks On Fri, Oct 28, 2022 at 7:44 PM Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> wrote: > > On 10/28/2022 12:24 PM, Ajye Huang wrote: > > Having two DMICs, a front DMIC and a rear DMIC, > > but only host audio input AUX port0 is used for these two Dmics. > > A "dmic_sel-gpios" property is used for a mixer control to switch > > the dmic signal source between the Front and Rear Dmic. > > > > usage: amixer -c0 cset name='Dmic Mux' 'FrontMic' > > usage: amixer -c0 cset name='Dmic Mux' 'RearMic' > > > > Refer to this one as an example, > > commit 3cfbf07c6d27 > > ("ASoC: qcom: sc7180: Modify machine driver for 2mic") > > > > Signed-off-by: Ajye Huang <ajye_huang@compal.corp-partner.google.com> > > --- > > > I'm very suspicious of this patchset. As it is you add kcontrol which > won't take effect on most platforms making use of DMIC. It feels to me > that it is something you want to handle on machine driver side instead. >
On Fri, Oct 28, 2022 at 08:54:31PM +0800, Ajye Huang wrote: > Yes, the original version I tried the implementation on audio machine > driver, but one person gave me an idea for this dmic.c > Do you think it is appropriate on dmic.c? > If it isn't, I will add kcontrol into audio machine driver. thanks It's definitely a better fit somewhere else - like I said in reply to the DT binding it's really a mux that sits between the DMICs and the DAI so that's probably where a generic version should be implemented. Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed.
Hi Mark Brown,
Thank you for review,
I think it is appropriate to implement on audio machine side, like
this I did before,
commit 3cfbf07c6d27
("ASoC: qcom: sc7180: Modify machine driver for 2mic")
What is your suggestion? Thank you.
On Fri, Oct 28, 2022 at 7:58 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 28, 2022 at 06:24:50PM +0800, Ajye Huang wrote:
>
> > + dmic->dmic_sel = devm_gpiod_get_optional(component->dev,
> > + "dmic_sel", GPIOD_OUT_LOW);
> > + if (IS_ERR(dmic->dmic_sel))
> > + return PTR_ERR(dmic->dmic_sel);
> > +
> > snd_soc_component_set_drvdata(component, dmic);
> >
> > return 0;
> > @@ -125,10 +172,15 @@ static const struct snd_soc_dapm_widget dmic_dapm_widgets[] = {
> > SND_SOC_NOPM, 0, 0, dmic_aif_event,
> > SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
> > SND_SOC_DAPM_INPUT("DMic"),
> > + SND_SOC_DAPM_MIC("DMIC", NULL),
> > + SND_SOC_DAPM_MUX("Dmic Mux", SND_SOC_NOPM, 0, 0, &dmic_mux_control),
>
> If we are doing this then adding the mux needs to be conditional on
> having the GPIO, without the GPIO the control is at best confusing to
> users.
On Fri, Oct 28, 2022 at 08:59:54PM +0800, Ajye Huang wrote: > Thank you for review, > I think it is appropriate to implement on audio machine side, like > this I did before, > commit 3cfbf07c6d27 > ("ASoC: qcom: sc7180: Modify machine driver for 2mic") > What is your suggestion? Thank you. Doing that seems fine. Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed.
Hi Mark Brown I need to abandon this one, I will send another new patch, thank you so much. On Fri, Oct 28, 2022 at 9:01 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Oct 28, 2022 at 08:59:54PM +0800, Ajye Huang wrote: > > > Thank you for review, > > I think it is appropriate to implement on audio machine side, like > > this I did before, > > commit 3cfbf07c6d27 > > ("ASoC: qcom: sc7180: Modify machine driver for 2mic") > > > What is your suggestion? Thank you. > > Doing that seems fine. > > Please don't top post, reply in line with needed context. This allows > readers to readily follow the flow of conversation and understand what > you are talking about and also helps ensure that everything in the > discussion is being addressed.
diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c index 4fd6f97e5a49..5c56fbcdb3e6 100644 --- a/sound/soc/codecs/dmic.c +++ b/sound/soc/codecs/dmic.c @@ -28,8 +28,50 @@ struct dmic { int wakeup_delay; /* Delay after DMIC mode switch */ int modeswitch_delay; + struct gpio_desc *dmic_sel; + int dmic_switch; }; +static int dmic_sel_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kcontrol); + struct snd_soc_component *component = snd_soc_dapm_to_component(dapm); + struct dmic *dmic = snd_soc_component_get_drvdata(component); + + ucontrol->value.integer.value[0] = dmic->dmic_switch; + return 0; +} + +static int dmic_sel_set(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kcontrol); + struct snd_soc_component *component = snd_soc_dapm_to_component(dapm); + struct dmic *dmic = snd_soc_component_get_drvdata(component); + + dmic->dmic_switch = ucontrol->value.integer.value[0]; + if (dmic->dmic_sel) { + gpiod_set_value(dmic->dmic_sel, dmic->dmic_switch); + dev_info(dapm->dev, "%s value %d\n", __func__, dmic->dmic_switch); + } else + dev_info(dapm->dev, "%s without dmic_sel-gpios\n", __func__); + + return 0; +} + +static const char * const dmic_mux_text[] = { + "FrontMic", + "RearMic", +}; + +static SOC_ENUM_SINGLE_DECL(dmic_enum, + SND_SOC_NOPM, 0, dmic_mux_text); + +static const struct snd_kcontrol_new dmic_mux_control = + SOC_DAPM_ENUM_EXT("DMIC Select Mux", dmic_enum, + dmic_sel_get, dmic_sel_set); + static int dmic_daiops_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { @@ -115,6 +157,11 @@ static int dmic_component_probe(struct snd_soc_component *component) if (dmic->modeswitch_delay > MAX_MODESWITCH_DELAY) dmic->modeswitch_delay = MAX_MODESWITCH_DELAY; + dmic->dmic_sel = devm_gpiod_get_optional(component->dev, + "dmic_sel", GPIOD_OUT_LOW); + if (IS_ERR(dmic->dmic_sel)) + return PTR_ERR(dmic->dmic_sel); + snd_soc_component_set_drvdata(component, dmic); return 0; @@ -125,10 +172,15 @@ static const struct snd_soc_dapm_widget dmic_dapm_widgets[] = { SND_SOC_NOPM, 0, 0, dmic_aif_event, SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_INPUT("DMic"), + SND_SOC_DAPM_MIC("DMIC", NULL), + SND_SOC_DAPM_MUX("Dmic Mux", SND_SOC_NOPM, 0, 0, &dmic_mux_control), }; static const struct snd_soc_dapm_route intercon[] = { {"DMIC AIF", NULL, "DMic"}, + /* digital mics */ + {"Dmic Mux", "FrontMic", "DMIC"}, + {"Dmic Mux", "RearMic", "DMIC"}, }; static const struct snd_soc_component_driver soc_dmic = {
Having two DMICs, a front DMIC and a rear DMIC, but only host audio input AUX port0 is used for these two Dmics. A "dmic_sel-gpios" property is used for a mixer control to switch the dmic signal source between the Front and Rear Dmic. usage: amixer -c0 cset name='Dmic Mux' 'FrontMic' usage: amixer -c0 cset name='Dmic Mux' 'RearMic' Refer to this one as an example, commit 3cfbf07c6d27 ("ASoC: qcom: sc7180: Modify machine driver for 2mic") Signed-off-by: Ajye Huang <ajye_huang@compal.corp-partner.google.com> --- sound/soc/codecs/dmic.c | 52 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)