[PATCH v1 0/2] Add optional dmic selection for two DMICs

[PATCH v1 0/2] Add optional dmic selection for two DMICs

[Ajye Huang]

v2:
- Documentation: Add dmic_sel-gpios optional prop for two DMICs.
- dmic codec driver: 
  - "dmic_sel-gpios" property is used for a mixer control to switch
     the dmic signal source between the Front and Rear Dmic.

Thanks for the review!

Ajye Huang (2):
  ASoC: dt-bindings: Document dmic_sel-gpios optional prop for two DMICs
    case
  ASoC: dmic: Add optional dmic selection

 .../devicetree/bindings/sound/dmic-codec.yaml |  5 ++
 sound/soc/codecs/dmic.c                       | 52 +++++++++++++++++++
 2 files changed, 57 insertions(+)

-- 
2.25.1

[PATCH v1 1/2] ASoC: dt-bindings: Document dmic_sel-gpios optional prop for two DMICs case

[Ajye Huang]

Document dmic_sel-gpios optional prop for switching between two DMICs.
Ex, the GPIO can control a MUX to select Front or Rear dmic.

Signed-off-by: Ajye Huang <ajye_huang@compal.corp-partner.google.com>
---
 Documentation/devicetree/bindings/sound/dmic-codec.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/dmic-codec.yaml b/Documentation/devicetree/bindings/sound/dmic-codec.yaml
index bba27c985996..4013af65f3e7 100644
--- a/Documentation/devicetree/bindings/sound/dmic-codec.yaml
+++ b/Documentation/devicetree/bindings/sound/dmic-codec.yaml
@@ -36,6 +36,10 @@ properties:
   wakeup-delay-ms:
     description: Delay (in ms) after enabling the DMIC
 
+  dmic_sel-gpios:
+    maxItems: 1
+    description: GPIO for switching between DMICs, ex Front/Rear dmic
+
 required:
   - compatible
 
@@ -48,6 +52,7 @@ examples:
     dmic {
         compatible = "dmic-codec";
         dmicen-gpios = <&gpio4 3 GPIO_ACTIVE_HIGH>;
+        dmic_sel-gpios = <&pio 23 GPIO_ACTIVE_HIGH>;
         num-channels = <1>;
         wakeup-delay-ms = <50>;
         modeswitch-delay-ms = <35>;
-- 
2.25.1

Re: [PATCH v1 1/2] ASoC: dt-bindings: Document dmic_sel-gpios optional prop for two DMICs case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

On Fri, Oct 28, 2022 at 06:24:49PM +0800, Ajye Huang wrote:
> Document dmic_sel-gpios optional prop for switching between two DMICs.
> Ex, the GPIO can control a MUX to select Front or Rear dmic.

> +  dmic_sel-gpios:
> +    maxItems: 1
> +    description: GPIO for switching between DMICs, ex Front/Rear dmic
> +

If we're going to do this we should also allow the bindings to label the
mics appropriately so that the control presented can reflect the actual
hardware.  It does feel like it might fit better to do this separately
to the DMIC driver as a mux between the DMIC and the DAI it's connected
to but equally with the way things are at the minute that feels like
it's probably disproportionate effort.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] ASoC: dt-bindings: Document dmic_sel-gpios optional prop for two DMICs case

[Rob Herring]

On Fri, Oct 28, 2022 at 12:56:16PM +0100, Mark Brown wrote:
> On Fri, Oct 28, 2022 at 06:24:49PM +0800, Ajye Huang wrote:
> > Document dmic_sel-gpios optional prop for switching between two DMICs.
> > Ex, the GPIO can control a MUX to select Front or Rear dmic.
> 
> > +  dmic_sel-gpios:

s/_/-/

> > +    maxItems: 1
> > +    description: GPIO for switching between DMICs, ex Front/Rear dmic
> > +
> 
> If we're going to do this we should also allow the bindings to label the
> mics appropriately so that the control presented can reflect the actual
> hardware.  It does feel like it might fit better to do this separately
> to the DMIC driver as a mux between the DMIC and the DAI it's connected
> to but equally with the way things are at the minute that feels like
> it's probably disproportionate effort.

Are there other needs for DAI muxes? We already have a mux binding, so 
defining a DAI mux would work for any type of muxing control, not just 
GPIO.

Rob

Re: [PATCH v1 1/2] ASoC: dt-bindings: Document dmic_sel-gpios optional prop for two DMICs case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

On Mon, Oct 31, 2022 at 01:43:43PM -0500, Rob Herring wrote:
> On Fri, Oct 28, 2022 at 12:56:16PM +0100, Mark Brown wrote:

> > If we're going to do this we should also allow the bindings to label the
> > mics appropriately so that the control presented can reflect the actual
> > hardware.  It does feel like it might fit better to do this separately
> > to the DMIC driver as a mux between the DMIC and the DAI it's connected
> > to but equally with the way things are at the minute that feels like
> > it's probably disproportionate effort.

> Are there other needs for DAI muxes? We already have a mux binding, so 
> defining a DAI mux would work for any type of muxing control, not just 
> GPIO.

I suspect that anything that is more complex than a GPIO should be a
full fledged CODEC with the muxing internal to the CODEC and just
described that way.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] ASoC: dt-bindings: Document dmic_sel-gpios optional prop for two DMICs case

[Ajye Huang]

Hi Mark, Rob

To avoid confusion, I had submitted another one for process

Re: [PATCH v1 1/2] ASoC: dt-bindings: Document dmic_sel-gpios optional prop for two DMICs case

[Ajye Huang]

Hi Mark, Rob

I submitted another one to process the kcontrol in the audio machine
driver instead,  sorry about that previous non-completed mail.
 https://patchwork.kernel.org/project/alsa-devel/patch/20221031122224.1846221-2-ajye_huang@compal.corp-partner.google.com/
thanks

On Tue, Nov 1, 2022 at 9:04 AM Ajye Huang
<ajye_huang@compal.corp-partner.google.com> wrote:
>
> Hi Mark, Rob
>
> To avoid confusion, I had submitted another one for process

[PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Ajye Huang]

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(+)

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 = {
-- 
2.25.1

Re: [PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Amadeusz Sławiński]

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.

Re: [PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Ajye Huang]

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.
>

Re: [PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Ajye Huang]

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.

Re: [PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/2] ASoC: dmic: Add optional dmic selection

[Ajye Huang]

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.