[PATCH v1 0/2] Add optional dmic selection for two DMICs
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(+)
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>;
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.
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
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.
Hi Mark, Rob
To avoid confusion, I had submitted another one for process
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... 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
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 = {
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.
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.
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 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.
participants (4)
-
Ajye Huang -
Amadeusz Sławiński -
Mark Brown -
Rob Herring