| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fc9863ef-8781-46a0-ba8c-6dabd9fb6cdf@notapiano>
Date: Tue, 4 Mar 2025 17:16:45 -0300
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Trevor Wu <trevor.wu@...iatek.com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
kernel@...labora.com, linux-sound@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org,
Zoran Zhan <zoran.zhan@...iatek.com>
Subject: Re: [PATCH 3/3] ASoC: mediatek: mt8188-mt6359: Add headset jack
detect support
On Tue, Mar 04, 2025 at 04:39:33PM +0100, AngeloGioacchino Del Regno wrote:
> Il 14/02/25 16:14, Nícolas F. R. A. Prado ha scritto:
> > Enable headset jack detection for MT8188 platforms using the MT6359
> > ACCDET block for it.
> >
> > Co-developed-by: Zoran Zhan <zoran.zhan@...iatek.com>
> > Signed-off-by: Zoran Zhan <zoran.zhan@...iatek.com>
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > ---
> > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 43 +++++++++++++++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> >
> > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > index 2d0d04e0232da07ba43a030b14853322427d55e7..4e19e6cfad1e1f42863b2e2f27131f880c5883bf 100644
> > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > @@ -17,6 +17,7 @@
> > #include "mt8188-afe-common.h"
> > #include "../../codecs/nau8825.h"
> > #include "../../codecs/mt6359.h"
> > +#include "../../codecs/mt6359-accdet.h"
> > #include "../../codecs/rt5682.h"
> > #include "../common/mtk-afe-platform-driver.h"
> > #include "../common/mtk-soundcard-driver.h"
> > @@ -266,6 +267,17 @@ static struct snd_soc_jack_pin nau8825_jack_pins[] = {
> > },
> > };
> > +static struct snd_soc_jack_pin mt8188_headset_jack_pins[] = {
>
> This is the same as nau8825_jack_pins... perhaps we could reuse that?
The difference is the pin name: "Headphone Jack", which results in a difference
in the widget's name. I remember you wanted to not have the "Jack" in the
widget's name to standardize it among other MTK platforms, and we could do that
here by dropping it from the nau8825_jack_pins. But since that name is also used
on the audio routes in a few DTs, those would need updating as well:
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku0.dts
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku1.dts
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku3.dts
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku2.dts
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku5.dts
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku7.dts
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku4.dts
arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku6.dts
The name is not used in upstream alsa-ucm-conf though, so it should be safe to
update.
In any case, I didn't want to expand the scope of this series to that unrelated
change, so that's why I introduced this separate struct, thinking we could
commonize as a later step.
>
> > + {
> > + .pin = "Headphone",
> > + .mask = SND_JACK_HEADPHONE,
> > + },
> > + {
> > + .pin = "Headset Mic",
> > + .mask = SND_JACK_MICROPHONE,
> > + },
> > +};
> > +
> > static const struct snd_kcontrol_new mt8188_dumb_spk_controls[] = {
> > SOC_DAPM_PIN_SWITCH("Ext Spk"),
> > };
> > @@ -500,6 +512,35 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
> > return 0;
> > }
> > +static int mt8188_mt6359_accdet_init(struct snd_soc_pcm_runtime *rtd)
> > +{
> > + struct mtk_soc_card_data *soc_card_data = snd_soc_card_get_drvdata(rtd->card);
> > + struct snd_soc_jack *jack = &soc_card_data->card_data->jacks[MT8188_JACK_HEADSET];
> > + int ret;
> > +
> > + if (!soc_card_data->accdet)
> > + return 0;
>
> I'm not sure... if we have mediatek,accdet (so accdet is present here), but we also
> have a NAU8825, or RT5682S, or ES8326 codec, this function will create a headset
> jack for MT6359, but then mt8188_headset_codec_init() will do the same again!
>
> I think we should find a way to avoid that situation, as I'm mostly sure that this
> will give issues in the long run.
>
> Even if it wouldn't, having two headset jacks exposed, of which one doesn't work
> because it doesn't exist on the physical board... would be confusing for the user.
IMO that situation happening would mean the DT is wrong. If the board has the
headset jack pins wired to a headset codec, and that's responsible for jack
detection, then those pins won't be wired to the ACCDET block, and therefore the
board DT shouldn't have the mediatek,accdet property.
I think you're imagining having the mediatek,accdet property always present, and
the logic you propose would allow specifying a headset codec to essentially
override it (making the mediatek,accdet property useless in this case). But
there can also be boards with no jacks at all, and in those cases we don't want
jack widgets exposed to userspace, so the mediatek,accdet property should be
omitted for those boards, and only present on boards where the ACCDET is
actually wired to.
>
> I guess that the best option here would be:
> - Let the `for_each_card_prelinks()` loop finish
> - Check if any of the external codecs are providing 3.5mm jack
> - External codec providing jack means that the detection should be performed
> by the external codec, as the jack should not be physically routed to the
> MediaTek ACCDET related PMIC pins (right?)
> - No external codec means that the accessory detection can only be performed
> by the MediaTek ACCDET IP
> - If external codec manages 3.5mm jack: do nothing
> - If no external codec managing 3.5mm jack: check if accdet provided in DT and
> initialize it
>
> ....unless I'm wrong - and if I am, please explain why (and also add explanation
> to the commit description).
I can add to the commit message:
Only boards that have jack detection managed by the MT6359 ACCDET block will
have the mediatek,accdet property present.
Thanks,
Nícolas
>
> Cheers,
> Angelo
>
> > +
> > + ret = snd_soc_card_jack_new_pins(rtd->card, "Headset Jack",
> > + SND_JACK_HEADSET | SND_JACK_BTN_0 |
> > + SND_JACK_BTN_1 | SND_JACK_BTN_2 |
> > + SND_JACK_BTN_3,
> > + jack, mt8188_headset_jack_pins,
> > + ARRAY_SIZE(mt8188_headset_jack_pins));
> > + if (ret) {
> > + dev_err(rtd->dev, "Headset Jack create failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = mt6359_accdet_enable_jack_detect(soc_card_data->accdet, jack);
> > + if (ret) {
> > + dev_err(rtd->dev, "Headset Jack enable failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mt8188_mt6359_init(struct snd_soc_pcm_runtime *rtd)
> > {
> > struct snd_soc_component *cmpnt_codec =
> > @@ -512,6 +553,8 @@ static int mt8188_mt6359_init(struct snd_soc_pcm_runtime *rtd)
> > /* mtkaif calibration */
> > mt8188_mt6359_mtkaif_calibration(rtd);
> > + mt8188_mt6359_accdet_init(rtd);
> > +
> > return 0;
> > }
> >
Powered by blists - more mailing lists