2023-07-25 04:36:38

by Trevor Wu (吳文良)

[permalink] [raw]
Subject: [PATCH 0/3] ASoC: mt8188-mt6359: add SOF support

This series introduces dynamic pinctrl and adds support for the SOF on
the mt8188-mt6359 machine driver.

Trevor Wu (3):
ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl
ASoC: mediatek: common: revise SOF common code
ASoC: mediatek: mt8188-mt6359: add SOF support

.../soc/mediatek/common/mtk-dsp-sof-common.c | 106 ++++++--
.../soc/mediatek/common/mtk-dsp-sof-common.h | 8 +
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 238 +++++++++++++++++-
3 files changed, 325 insertions(+), 27 deletions(-)

--
2.18.0



2023-07-25 05:05:28

by Trevor Wu (吳文良)

[permalink] [raw]
Subject: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

To avoid power leakage, it is recommended to replace the default pinctrl
state with dynamic pinctrl since certain audio pinmux functions can
remain in a HIGH state even when audio is disabled. Linking pinctrl with
DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain in
GPIO mode by default and only switch to an audio function when necessary.

Signed-off-by: Trevor Wu <[email protected]>
---
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
index 7c9e08e6a4f5..667d79f33bf2 100644
--- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
+++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
@@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget mt8188_mt6359_widgets[] = {
SND_SOC_DAPM_MIC("Headset Mic", NULL),
SND_SOC_DAPM_SINK("HDMI"),
SND_SOC_DAPM_SINK("DP"),
+
+ /* dynamic pinctrl */
+ SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on", "aud_etdm_spk_off"),
+ SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on", "aud_etdm_hp_off"),
+ SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on", "aud_mtkaif_off"),
};

static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
@@ -267,6 +272,7 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
struct snd_soc_component *cmpnt_codec =
asoc_rtd_to_codec(rtd, 0)->component;
+ struct snd_soc_dapm_widget *pin_w = NULL, *w;
struct mtk_base_afe *afe;
struct mt8188_afe_private *afe_priv;
struct mtkaif_param *param;
@@ -306,6 +312,18 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
return 0;
}

+ for_each_card_widgets(rtd->card, w) {
+ if (!strcmp(w->name, "MTKAIF_PIN")) {
+ pin_w = w;
+ break;
+ }
+ }
+
+ if (!pin_w)
+ dev_dbg(afe->dev, "%s(), no pinmux widget, please check if default on\n", __func__);
+ else
+ dapm_pinctrl_event(pin_w, NULL, SND_SOC_DAPM_PRE_PMU);
+
pm_runtime_get_sync(afe->dev);
mt6359_mtkaif_calibration_enable(cmpnt_codec);

@@ -403,6 +421,9 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
for (i = 0; i < MT8188_MTKAIF_MISO_NUM; i++)
param->mtkaif_phase_cycle[i] = mtkaif_phase_cycle[i];

+ if (pin_w)
+ dapm_pinctrl_event(pin_w, NULL, SND_SOC_DAPM_POST_PMD);
+
dev_dbg(afe->dev, "%s(), end, calibration ok %d\n",
__func__, param->mtkaif_calibration_ok);

--
2.18.0


2023-07-25 05:39:40

by Trevor Wu (吳文良)

[permalink] [raw]
Subject: [PATCH 3/3] ASoC: mediatek: mt8188-mt6359: add SOF support

SOF is enabled when adsp phandle is assigned to "mediatek,adsp".
The required callback will be assigned when SOF is enabled.

Additionally, "mediatek,dai-link" is introduced to decide the supported
dai links for a project, so user can reuse the machine driver regardless
of dai link combination.

Signed-off-by: Trevor Wu <[email protected]>
---
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 217 ++++++++++++++++++++--
1 file changed, 205 insertions(+), 12 deletions(-)

diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
index 667d79f33bf2..e205de2899a9 100644
--- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
+++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
@@ -19,6 +19,8 @@
#include "../../codecs/mt6359.h"
#include "../common/mtk-afe-platform-driver.h"
#include "../common/mtk-soundcard-driver.h"
+#include "../common/mtk-dsp-sof-common.h"
+#include "../common/mtk-soc-card.h"

#define CKSYS_AUD_TOP_CFG 0x032c
#define RG_TEST_ON BIT(0)
@@ -45,6 +47,11 @@
*/
#define NAU8825_CODEC_DAI "nau8825-hifi"

+#define SOF_DMA_DL2 "SOF_DMA_DL2"
+#define SOF_DMA_DL3 "SOF_DMA_DL3"
+#define SOF_DMA_UL4 "SOF_DMA_UL4"
+#define SOF_DMA_UL5 "SOF_DMA_UL5"
+
/* FE */
SND_SOC_DAILINK_DEFS(playback2,
DAILINK_COMP_ARRAY(COMP_CPU("DL2")),
@@ -176,6 +183,49 @@ SND_SOC_DAILINK_DEFS(ul_src,
"dmic-hifi")),
DAILINK_COMP_ARRAY(COMP_EMPTY()));

+SND_SOC_DAILINK_DEFS(AFE_SOF_DL2,
+ DAILINK_COMP_ARRAY(COMP_CPU("SOF_DL2")),
+ DAILINK_COMP_ARRAY(COMP_DUMMY()),
+ DAILINK_COMP_ARRAY(COMP_EMPTY()));
+
+SND_SOC_DAILINK_DEFS(AFE_SOF_DL3,
+ DAILINK_COMP_ARRAY(COMP_CPU("SOF_DL3")),
+ DAILINK_COMP_ARRAY(COMP_DUMMY()),
+ DAILINK_COMP_ARRAY(COMP_EMPTY()));
+
+SND_SOC_DAILINK_DEFS(AFE_SOF_UL4,
+ DAILINK_COMP_ARRAY(COMP_CPU("SOF_UL4")),
+ DAILINK_COMP_ARRAY(COMP_DUMMY()),
+ DAILINK_COMP_ARRAY(COMP_EMPTY()));
+
+SND_SOC_DAILINK_DEFS(AFE_SOF_UL5,
+ DAILINK_COMP_ARRAY(COMP_CPU("SOF_UL5")),
+ DAILINK_COMP_ARRAY(COMP_DUMMY()),
+ DAILINK_COMP_ARRAY(COMP_EMPTY()));
+
+static const struct sof_conn_stream g_sof_conn_streams[] = {
+ {
+ .sof_link = "AFE_SOF_DL2",
+ .sof_dma = SOF_DMA_DL2,
+ .stream_dir = SNDRV_PCM_STREAM_PLAYBACK
+ },
+ {
+ .sof_link = "AFE_SOF_DL3",
+ .sof_dma = SOF_DMA_DL3,
+ .stream_dir = SNDRV_PCM_STREAM_PLAYBACK
+ },
+ {
+ .sof_link = "AFE_SOF_UL4",
+ .sof_dma = SOF_DMA_UL4,
+ .stream_dir = SNDRV_PCM_STREAM_CAPTURE
+ },
+ {
+ .sof_link = "AFE_SOF_UL5",
+ .sof_dma = SOF_DMA_UL5,
+ .stream_dir = SNDRV_PCM_STREAM_CAPTURE
+ },
+};
+
struct mt8188_mt6359_priv {
struct snd_soc_jack dp_jack;
struct snd_soc_jack hdmi_jack;
@@ -246,6 +296,10 @@ static const struct snd_soc_dapm_widget mt8188_mt6359_widgets[] = {
SND_SOC_DAPM_MIC("Headset Mic", NULL),
SND_SOC_DAPM_SINK("HDMI"),
SND_SOC_DAPM_SINK("DP"),
+ SND_SOC_DAPM_MIXER(SOF_DMA_DL2, SND_SOC_NOPM, 0, 0, NULL, 0),
+ SND_SOC_DAPM_MIXER(SOF_DMA_DL3, SND_SOC_NOPM, 0, 0, NULL, 0),
+ SND_SOC_DAPM_MIXER(SOF_DMA_UL4, SND_SOC_NOPM, 0, 0, NULL, 0),
+ SND_SOC_DAPM_MIXER(SOF_DMA_UL5, SND_SOC_NOPM, 0, 0, NULL, 0),

/* dynamic pinctrl */
SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on", "aud_etdm_spk_off"),
@@ -266,6 +320,19 @@ static const struct snd_kcontrol_new mt8188_nau8825_controls[] = {
SOC_DAPM_PIN_SWITCH("Headphone Jack"),
};

+static const struct snd_soc_dapm_route mt8188_mt6359_routes[] = {
+ /* SOF Uplink */
+ {SOF_DMA_UL4, NULL, "O034"},
+ {SOF_DMA_UL4, NULL, "O035"},
+ {SOF_DMA_UL5, NULL, "O036"},
+ {SOF_DMA_UL5, NULL, "O037"},
+ /* SOF Downlink */
+ {"I070", NULL, SOF_DMA_DL2},
+ {"I071", NULL, SOF_DMA_DL2},
+ {"I020", NULL, SOF_DMA_DL3},
+ {"I021", NULL, SOF_DMA_DL3},
+};
+
static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
{
struct snd_soc_component *cmpnt_afe =
@@ -471,8 +538,17 @@ enum {
DAI_LINK_ETDM3_OUT_BE,
DAI_LINK_PCM1_BE,
DAI_LINK_UL_SRC_BE,
+ DAI_LINK_REGULAR_LAST = DAI_LINK_UL_SRC_BE,
+ DAI_LINK_SOF_START,
+ DAI_LINK_SOF_DL2_BE = DAI_LINK_SOF_START,
+ DAI_LINK_SOF_DL3_BE,
+ DAI_LINK_SOF_UL4_BE,
+ DAI_LINK_SOF_UL5_BE,
+ DAI_LINK_SOF_END = DAI_LINK_SOF_UL5_BE,
};

+#define DAI_LINK_REGULAR_NUM (DAI_LINK_REGULAR_LAST + 1)
+
static int mt8188_dptx_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
@@ -503,7 +579,8 @@ static int mt8188_dptx_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,

static int mt8188_hdmi_codec_init(struct snd_soc_pcm_runtime *rtd)
{
- struct mt8188_mt6359_priv *priv = snd_soc_card_get_drvdata(rtd->card);
+ struct mtk_soc_card_data *soc_card_data = snd_soc_card_get_drvdata(rtd->card);
+ struct mt8188_mt6359_priv *priv = soc_card_data->mach_priv;
struct snd_soc_component *component = asoc_rtd_to_codec(rtd, 0)->component;
int ret = 0;

@@ -528,7 +605,8 @@ static int mt8188_hdmi_codec_init(struct snd_soc_pcm_runtime *rtd)

static int mt8188_dptx_codec_init(struct snd_soc_pcm_runtime *rtd)
{
- struct mt8188_mt6359_priv *priv = snd_soc_card_get_drvdata(rtd->card);
+ struct mtk_soc_card_data *soc_card_data = snd_soc_card_get_drvdata(rtd->card);
+ struct mt8188_mt6359_priv *priv = soc_card_data->mach_priv;
struct snd_soc_component *component = asoc_rtd_to_codec(rtd, 0)->component;
int ret = 0;

@@ -648,7 +726,8 @@ static int mt8188_max98390_codec_init(struct snd_soc_pcm_runtime *rtd)
static int mt8188_nau8825_codec_init(struct snd_soc_pcm_runtime *rtd)
{
struct snd_soc_card *card = rtd->card;
- struct mt8188_mt6359_priv *priv = snd_soc_card_get_drvdata(card);
+ struct mtk_soc_card_data *soc_card_data = snd_soc_card_get_drvdata(card);
+ struct mt8188_mt6359_priv *priv = soc_card_data->mach_priv;
struct snd_soc_component *component = asoc_rtd_to_codec(rtd, 0)->component;
struct snd_soc_jack *jack = &priv->headset_jack;
int ret;
@@ -733,6 +812,33 @@ static int mt8188_nau8825_hw_params(struct snd_pcm_substream *substream,
static const struct snd_soc_ops mt8188_nau8825_ops = {
.hw_params = mt8188_nau8825_hw_params,
};
+
+static int mt8188_sof_be_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+ struct snd_soc_component *cmpnt_afe = NULL;
+ struct snd_soc_pcm_runtime *runtime;
+
+ /* find afe component */
+ for_each_card_rtds(rtd->card, runtime) {
+ cmpnt_afe = snd_soc_rtdcom_lookup(runtime, AFE_PCM_NAME);
+ if (cmpnt_afe)
+ break;
+ }
+
+ if (cmpnt_afe && !pm_runtime_active(cmpnt_afe->dev)) {
+ dev_err(rtd->dev, "afe pm runtime is not active!!\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct snd_soc_ops mt8188_sof_be_ops = {
+ .hw_params = mt8188_sof_be_hw_params,
+};
+
static struct snd_soc_dai_link mt8188_mt6359_dai_links[] = {
/* FE */
[DAI_LINK_DL2_FE] = {
@@ -1003,6 +1109,36 @@ static struct snd_soc_dai_link mt8188_mt6359_dai_links[] = {
.dpcm_capture = 1,
SND_SOC_DAILINK_REG(ul_src),
},
+
+ /* SOF BE */
+ [DAI_LINK_SOF_DL2_BE] = {
+ .name = "AFE_SOF_DL2",
+ .no_pcm = 1,
+ .dpcm_playback = 1,
+ .ops = &mt8188_sof_be_ops,
+ SND_SOC_DAILINK_REG(AFE_SOF_DL2),
+ },
+ [DAI_LINK_SOF_DL3_BE] = {
+ .name = "AFE_SOF_DL3",
+ .no_pcm = 1,
+ .dpcm_playback = 1,
+ .ops = &mt8188_sof_be_ops,
+ SND_SOC_DAILINK_REG(AFE_SOF_DL3),
+ },
+ [DAI_LINK_SOF_UL4_BE] = {
+ .name = "AFE_SOF_UL4",
+ .no_pcm = 1,
+ .dpcm_capture = 1,
+ .ops = &mt8188_sof_be_ops,
+ SND_SOC_DAILINK_REG(AFE_SOF_UL4),
+ },
+ [DAI_LINK_SOF_UL5_BE] = {
+ .name = "AFE_SOF_UL5",
+ .no_pcm = 1,
+ .dpcm_capture = 1,
+ .ops = &mt8188_sof_be_ops,
+ SND_SOC_DAILINK_REG(AFE_SOF_UL5),
+ },
};

static struct snd_kcontrol *ctl_find(struct snd_card *card, const char *name)
@@ -1017,7 +1153,8 @@ static struct snd_kcontrol *ctl_find(struct snd_card *card, const char *name)

static void mt8188_fixup_controls(struct snd_soc_card *card)
{
- struct mt8188_mt6359_priv *priv = snd_soc_card_get_drvdata(card);
+ struct mtk_soc_card_data *soc_card_data = snd_soc_card_get_drvdata(card);
+ struct mt8188_mt6359_priv *priv = soc_card_data->mach_priv;
struct mt8188_card_data *card_data = (struct mt8188_card_data *)priv->private_data;
struct snd_kcontrol *kctl;

@@ -1045,6 +1182,8 @@ static struct snd_soc_card mt8188_mt6359_soc_card = {
.num_links = ARRAY_SIZE(mt8188_mt6359_dai_links),
.dapm_widgets = mt8188_mt6359_widgets,
.num_dapm_widgets = ARRAY_SIZE(mt8188_mt6359_widgets),
+ .dapm_routes = mt8188_mt6359_routes,
+ .num_dapm_routes = ARRAY_SIZE(mt8188_mt6359_routes),
.controls = mt8188_mt6359_controls,
.num_controls = ARRAY_SIZE(mt8188_mt6359_controls),
.fixup_controls = mt8188_fixup_controls,
@@ -1054,6 +1193,8 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
{
struct snd_soc_card *card = &mt8188_mt6359_soc_card;
struct device_node *platform_node;
+ struct device_node *adsp_node;
+ struct mtk_soc_card_data *soc_card_data;
struct mt8188_mt6359_priv *priv;
struct mt8188_card_data *card_data;
struct snd_soc_dai_link *dai_link;
@@ -1074,21 +1215,64 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
if (!card->name)
card->name = card_data->name;

- priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
if (of_property_read_bool(pdev->dev.of_node, "audio-routing")) {
ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
if (ret)
return ret;
}

+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ soc_card_data = devm_kzalloc(&pdev->dev, sizeof(*card_data), GFP_KERNEL);
+ if (!soc_card_data)
+ return -ENOMEM;
+
+ soc_card_data->mach_priv = priv;
+
+ adsp_node = of_parse_phandle(pdev->dev.of_node, "mediatek,adsp", 0);
+ if (adsp_node) {
+ struct mtk_sof_priv *sof_priv;
+
+ sof_priv = devm_kzalloc(&pdev->dev, sizeof(*sof_priv), GFP_KERNEL);
+ if (!sof_priv) {
+ ret = -ENOMEM;
+ goto err_adsp_node;
+ }
+ sof_priv->conn_streams = g_sof_conn_streams;
+ sof_priv->num_streams = ARRAY_SIZE(g_sof_conn_streams);
+ soc_card_data->sof_priv = sof_priv;
+ card->probe = mtk_sof_card_probe;
+ card->late_probe = mtk_sof_card_late_probe;
+ if (!card->topology_shortname_created) {
+ snprintf(card->topology_shortname, 32, "sof-%s", card->name);
+ card->topology_shortname_created = true;
+ }
+ card->name = card->topology_shortname;
+ }
+
+ if (of_property_read_bool(pdev->dev.of_node, "mediatek,dai-link")) {
+ ret = mtk_sof_dailink_parse_of(card, pdev->dev.of_node,
+ "mediatek,dai-link",
+ mt8188_mt6359_dai_links,
+ ARRAY_SIZE(mt8188_mt6359_dai_links));
+ if (ret) {
+ dev_err_probe(&pdev->dev, ret, "Parse dai-link fail\n");
+ goto err_adsp_node;
+ }
+ } else {
+ if (!adsp_node)
+ card->num_links = DAI_LINK_REGULAR_NUM;
+ }
+
platform_node = of_parse_phandle(pdev->dev.of_node,
"mediatek,platform", 0);
if (!platform_node) {
ret = -EINVAL;
- return dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n");
+ dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n");
+ goto err_platform_node;
+
}

ret = parse_dai_link_info(card);
@@ -1096,8 +1280,12 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
goto err;

for_each_card_prelinks(card, i, dai_link) {
- if (!dai_link->platforms->name)
- dai_link->platforms->of_node = platform_node;
+ if (!dai_link->platforms->name) {
+ if (!strncmp(dai_link->name, "AFE_SOF", strlen("AFE_SOF")) && adsp_node)
+ dai_link->platforms->of_node = adsp_node;
+ else
+ dai_link->platforms->of_node = platform_node;
+ }

if (strcmp(dai_link->name, "DPTX_BE") == 0) {
if (strcmp(dai_link->codecs->dai_name, "snd-soc-dummy-dai"))
@@ -1140,7 +1328,7 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
}

priv->private_data = card_data;
- snd_soc_card_set_drvdata(card, priv);
+ snd_soc_card_set_drvdata(card, soc_card_data);

ret = devm_snd_soc_register_card(&pdev->dev, card);
if (ret)
@@ -1149,6 +1337,11 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
err:
of_node_put(platform_node);
clean_card_reference(card);
+
+err_adsp_node:
+err_platform_node:
+ of_node_put(adsp_node);
+
return ret;
}

--
2.18.0


Subject: Re: [PATCH 3/3] ASoC: mediatek: mt8188-mt6359: add SOF support

Il 25/07/23 05:53, Trevor Wu ha scritto:
> SOF is enabled when adsp phandle is assigned to "mediatek,adsp".
> The required callback will be assigned when SOF is enabled.
>
> Additionally, "mediatek,dai-link" is introduced to decide the supported
> dai links for a project, so user can reuse the machine driver regardless
> of dai link combination.
>
> Signed-off-by: Trevor Wu <[email protected]>
> ---
> sound/soc/mediatek/mt8188/mt8188-mt6359.c | 217 ++++++++++++++++++++--
> 1 file changed, 205 insertions(+), 12 deletions(-)
>
> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> index 667d79f33bf2..e205de2899a9 100644
> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c

..snip..

> @@ -1074,21 +1215,64 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
> if (!card->name)
> card->name = card_data->name;
>
> - priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> - if (!priv)
> - return -ENOMEM;
> -
> if (of_property_read_bool(pdev->dev.of_node, "audio-routing")) {
> ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
> if (ret)
> return ret;
> }
>
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + soc_card_data = devm_kzalloc(&pdev->dev, sizeof(*card_data), GFP_KERNEL);
> + if (!soc_card_data)
> + return -ENOMEM;
> +
> + soc_card_data->mach_priv = priv;
> +
> + adsp_node = of_parse_phandle(pdev->dev.of_node, "mediatek,adsp", 0);
> + if (adsp_node) {
> + struct mtk_sof_priv *sof_priv;
> +
> + sof_priv = devm_kzalloc(&pdev->dev, sizeof(*sof_priv), GFP_KERNEL);
> + if (!sof_priv) {
> + ret = -ENOMEM;
> + goto err_adsp_node;
> + }
> + sof_priv->conn_streams = g_sof_conn_streams;
> + sof_priv->num_streams = ARRAY_SIZE(g_sof_conn_streams);
> + soc_card_data->sof_priv = sof_priv;
> + card->probe = mtk_sof_card_probe;
> + card->late_probe = mtk_sof_card_late_probe;
> + if (!card->topology_shortname_created) {
> + snprintf(card->topology_shortname, 32, "sof-%s", card->name);
> + card->topology_shortname_created = true;
> + }
> + card->name = card->topology_shortname;
> + }
> +
> + if (of_property_read_bool(pdev->dev.of_node, "mediatek,dai-link")) {
> + ret = mtk_sof_dailink_parse_of(card, pdev->dev.of_node,
> + "mediatek,dai-link",
> + mt8188_mt6359_dai_links,
> + ARRAY_SIZE(mt8188_mt6359_dai_links));
> + if (ret) {
> + dev_err_probe(&pdev->dev, ret, "Parse dai-link fail\n");
> + goto err_adsp_node;
> + }
> + } else {
> + if (!adsp_node)
> + card->num_links = DAI_LINK_REGULAR_NUM;
> + }
> +
> platform_node = of_parse_phandle(pdev->dev.of_node,
> "mediatek,platform", 0);
> if (!platform_node) {
> ret = -EINVAL;
> - return dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n");
> + dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n");

We could shorten this with

ret = dev_err_probe(&pdev->dev, -EINVAL, "Property ....");
goto err_adsp_node;

...but I don't have strong opinions about that, it's your choice.

> + goto err_platform_node;
> +
> }
>
> ret = parse_dai_link_info(card);
> @@ -1096,8 +1280,12 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
> goto err;
>
> for_each_card_prelinks(card, i, dai_link) {
> - if (!dai_link->platforms->name)
> - dai_link->platforms->of_node = platform_node;
> + if (!dai_link->platforms->name) {
> + if (!strncmp(dai_link->name, "AFE_SOF", strlen("AFE_SOF")) && adsp_node)
> + dai_link->platforms->of_node = adsp_node;
> + else
> + dai_link->platforms->of_node = platform_node;
> + }
>
> if (strcmp(dai_link->name, "DPTX_BE") == 0) {
> if (strcmp(dai_link->codecs->dai_name, "snd-soc-dummy-dai"))
> @@ -1140,7 +1328,7 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
> }
>
> priv->private_data = card_data;
> - snd_soc_card_set_drvdata(card, priv);
> + snd_soc_card_set_drvdata(card, soc_card_data);
>
> ret = devm_snd_soc_register_card(&pdev->dev, card);
> if (ret)
> @@ -1149,6 +1337,11 @@ static int mt8188_mt6359_dev_probe(struct platform_device *pdev)
> err:
> of_node_put(platform_node);
> clean_card_reference(card);
> +
> +err_adsp_node:
> +err_platform_node:

Just one label is enough. Keep err_adsp_node, remove err_platform_node.

Regards,
Angelo



Subject: Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Il 25/07/23 05:53, Trevor Wu ha scritto:
> To avoid power leakage, it is recommended to replace the default pinctrl
> state with dynamic pinctrl since certain audio pinmux functions can
> remain in a HIGH state even when audio is disabled. Linking pinctrl with
> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain in
> GPIO mode by default and only switch to an audio function when necessary.
>
> Signed-off-by: Trevor Wu <[email protected]>
> ---
> sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> index 7c9e08e6a4f5..667d79f33bf2 100644
> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget mt8188_mt6359_widgets[] = {
> SND_SOC_DAPM_MIC("Headset Mic", NULL),
> SND_SOC_DAPM_SINK("HDMI"),
> SND_SOC_DAPM_SINK("DP"),
> +
> + /* dynamic pinctrl */
> + SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on", "aud_etdm_spk_off"),
> + SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on", "aud_etdm_hp_off"),
> + SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on", "aud_mtkaif_off"),
> };
>
> static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
> @@ -267,6 +272,7 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
> snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
> struct snd_soc_component *cmpnt_codec =
> asoc_rtd_to_codec(rtd, 0)->component;
> + struct snd_soc_dapm_widget *pin_w = NULL, *w;
> struct mtk_base_afe *afe;
> struct mt8188_afe_private *afe_priv;
> struct mtkaif_param *param;
> @@ -306,6 +312,18 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
> return 0;
> }
>
> + for_each_card_widgets(rtd->card, w) {
> + if (!strcmp(w->name, "MTKAIF_PIN")) {

if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
pin_w = w;
break;
}

That's safer.

> + pin_w = w;
> + break;
> + }
> + }
> +
> + if (!pin_w)

Just a nitpick: you're checking for `if (pin_w)` later in this function, so
to increase readability please do the same here.

if (pin_w)
dapm_pinctrl_event(...)
else
dev_dbg(...)

Regards,
Angelo

2023-07-26 03:01:11

by Trevor Wu (吳文良)

[permalink] [raw]
Subject: Re: [PATCH 3/3] ASoC: mediatek: mt8188-mt6359: add SOF support

On Tue, 2023-07-25 at 09:11 +0200, AngeloGioacchino Del Regno wrote:
> Il 25/07/23 05:53, Trevor Wu ha scritto:
> > SOF is enabled when adsp phandle is assigned to "mediatek,adsp".
> > The required callback will be assigned when SOF is enabled.
> >
> > Additionally, "mediatek,dai-link" is introduced to decide the
> > supported
> > dai links for a project, so user can reuse the machine driver
> > regardless
> > of dai link combination.
> >
> > Signed-off-by: Trevor Wu <[email protected]>
> > ---
> > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 217
> > ++++++++++++++++++++--
> > 1 file changed, 205 insertions(+), 12 deletions(-)
> >
> > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > index 667d79f33bf2..e205de2899a9 100644
> > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>
> ..snip..
>
> > @@ -1074,21 +1215,64 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> > if (!card->name)
> > card->name = card_data->name;
> >
> > - priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > - if (!priv)
> > - return -ENOMEM;
> > -
> > if (of_property_read_bool(pdev->dev.of_node, "audio-routing"))
> > {
> > ret = snd_soc_of_parse_audio_routing(card, "audio-
> > routing");
> > if (ret)
> > return ret;
> > }
> >
> > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + soc_card_data = devm_kzalloc(&pdev->dev, sizeof(*card_data),
> > GFP_KERNEL);
> > + if (!soc_card_data)
> > + return -ENOMEM;
> > +
> > + soc_card_data->mach_priv = priv;
> > +
> > + adsp_node = of_parse_phandle(pdev->dev.of_node,
> > "mediatek,adsp", 0);
> > + if (adsp_node) {
> > + struct mtk_sof_priv *sof_priv;
> > +
> > + sof_priv = devm_kzalloc(&pdev->dev, sizeof(*sof_priv),
> > GFP_KERNEL);
> > + if (!sof_priv) {
> > + ret = -ENOMEM;
> > + goto err_adsp_node;
> > + }
> > + sof_priv->conn_streams = g_sof_conn_streams;
> > + sof_priv->num_streams = ARRAY_SIZE(g_sof_conn_streams);
> > + soc_card_data->sof_priv = sof_priv;
> > + card->probe = mtk_sof_card_probe;
> > + card->late_probe = mtk_sof_card_late_probe;
> > + if (!card->topology_shortname_created) {
> > + snprintf(card->topology_shortname, 32, "sof-
> > %s", card->name);
> > + card->topology_shortname_created = true;
> > + }
> > + card->name = card->topology_shortname;
> > + }
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "mediatek,dai-
> > link")) {
> > + ret = mtk_sof_dailink_parse_of(card, pdev->dev.of_node,
> > + "mediatek,dai-link",
> > + mt8188_mt6359_dai_links,
> > + ARRAY_SIZE(mt8188_mt6359
> > _dai_links));
> > + if (ret) {
> > + dev_err_probe(&pdev->dev, ret, "Parse dai-link
> > fail\n");
> > + goto err_adsp_node;
> > + }
> > + } else {
> > + if (!adsp_node)
> > + card->num_links = DAI_LINK_REGULAR_NUM;
> > + }
> > +
> > platform_node = of_parse_phandle(pdev->dev.of_node,
> > "mediatek,platform", 0);
> > if (!platform_node) {
> > ret = -EINVAL;
> > - return dev_err_probe(&pdev->dev, ret, "Property
> > 'platform' missing or invalid\n");
> > + dev_err_probe(&pdev->dev, ret, "Property 'platform'
> > missing or invalid\n");
>
> We could shorten this with
>
> ret = dev_err_probe(&pdev->dev, -EINVAL, "Property ....");
> goto err_adsp_node;
>
> ...but I don't have strong opinions about that, it's your choice.
>

It is concise. I will apply it in v2.

> > + goto err_platform_node;
> > +
> > }
> >
> > ret = parse_dai_link_info(card);
> > @@ -1096,8 +1280,12 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> > goto err;
> >
> > for_each_card_prelinks(card, i, dai_link) {
> > - if (!dai_link->platforms->name)
> > - dai_link->platforms->of_node = platform_node;
> > + if (!dai_link->platforms->name) {
> > + if (!strncmp(dai_link->name, "AFE_SOF",
> > strlen("AFE_SOF")) && adsp_node)
> > + dai_link->platforms->of_node =
> > adsp_node;
> > + else
> > + dai_link->platforms->of_node =
> > platform_node;
> > + }
> >
> > if (strcmp(dai_link->name, "DPTX_BE") == 0) {
> > if (strcmp(dai_link->codecs->dai_name, "snd-
> > soc-dummy-dai"))
> > @@ -1140,7 +1328,7 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> > }
> >
> > priv->private_data = card_data;
> > - snd_soc_card_set_drvdata(card, priv);
> > + snd_soc_card_set_drvdata(card, soc_card_data);
> >
> > ret = devm_snd_soc_register_card(&pdev->dev, card);
> > if (ret)
> > @@ -1149,6 +1337,11 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> > err:
> > of_node_put(platform_node);
> > clean_card_reference(card);
> > +
> > +err_adsp_node:
> > +err_platform_node:
>
> Just one label is enough. Keep err_adsp_node, remove
> err_platform_node.
>
OK. I will remove one label in v2.

Thanks,
Trevor

2023-07-26 03:30:52

by Trevor Wu (吳文良)

[permalink] [raw]
Subject: Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno wrote:
> Il 25/07/23 05:53, Trevor Wu ha scritto:
> > To avoid power leakage, it is recommended to replace the default
> > pinctrl
> > state with dynamic pinctrl since certain audio pinmux functions can
> > remain in a HIGH state even when audio is disabled. Linking pinctrl
> > with
> > DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain
> > in
> > GPIO mode by default and only switch to an audio function when
> > necessary.
> >
> > Signed-off-by: Trevor Wu <[email protected]>
> > ---
> > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
> > +++++++++++++++++++++
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > index 7c9e08e6a4f5..667d79f33bf2 100644
> > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
> > mt8188_mt6359_widgets[] = {
> > SND_SOC_DAPM_MIC("Headset Mic", NULL),
> > SND_SOC_DAPM_SINK("HDMI"),
> > SND_SOC_DAPM_SINK("DP"),
> > +
> > + /* dynamic pinctrl */
> > + SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
> > "aud_etdm_spk_off"),
> > + SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
> > "aud_etdm_hp_off"),
> > + SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
> > "aud_mtkaif_off"),
> > };
> >
> > static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
> > @@ -267,6 +272,7 @@ static int
> > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
> > snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
> > struct snd_soc_component *cmpnt_codec =
> > asoc_rtd_to_codec(rtd, 0)->component;
> > + struct snd_soc_dapm_widget *pin_w = NULL, *w;
> > struct mtk_base_afe *afe;
> > struct mt8188_afe_private *afe_priv;
> > struct mtkaif_param *param;
> > @@ -306,6 +312,18 @@ static int
> > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
> > return 0;
> > }
> >
> > + for_each_card_widgets(rtd->card, w) {
> > + if (!strcmp(w->name, "MTKAIF_PIN")) {
>
> if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
> pin_w = w;
> break;
> }
>
> That's safer.
>

If w->name is MTKAIF, the strncmp expression will return 0. However,
the result is not expected. I prefer to keep strcmp here.

> > + pin_w = w;
> > + break;
> > + }
> > + }
> > +
> > + if (!pin_w)
>
> Just a nitpick: you're checking for `if (pin_w)` later in this
> function, so
> to increase readability please do the same here.
>
> if (pin_w)
> dapm_pinctrl_event(...)
> else
> dev_dbg(...)
>

OK. I will update it in v2.

Thanks,
Trevor

Subject: Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Il 26/07/23 04:19, Trevor Wu (吳文良) ha scritto:
> On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno wrote:
>> Il 25/07/23 05:53, Trevor Wu ha scritto:
>>> To avoid power leakage, it is recommended to replace the default
>>> pinctrl
>>> state with dynamic pinctrl since certain audio pinmux functions can
>>> remain in a HIGH state even when audio is disabled. Linking pinctrl
>>> with
>>> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain
>>> in
>>> GPIO mode by default and only switch to an audio function when
>>> necessary.
>>>
>>> Signed-off-by: Trevor Wu <[email protected]>
>>> ---
>>> sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
>>> +++++++++++++++++++++
>>> 1 file changed, 21 insertions(+)
>>>
>>> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> index 7c9e08e6a4f5..667d79f33bf2 100644
>>> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
>>> mt8188_mt6359_widgets[] = {
>>> SND_SOC_DAPM_MIC("Headset Mic", NULL),
>>> SND_SOC_DAPM_SINK("HDMI"),
>>> SND_SOC_DAPM_SINK("DP"),
>>> +
>>> + /* dynamic pinctrl */
>>> + SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
>>> "aud_etdm_spk_off"),
>>> + SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
>>> "aud_etdm_hp_off"),
>>> + SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
>>> "aud_mtkaif_off"),
>>> };
>>>
>>> static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
>>> @@ -267,6 +272,7 @@ static int
>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
>>> snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
>>> struct snd_soc_component *cmpnt_codec =
>>> asoc_rtd_to_codec(rtd, 0)->component;
>>> + struct snd_soc_dapm_widget *pin_w = NULL, *w;
>>> struct mtk_base_afe *afe;
>>> struct mt8188_afe_private *afe_priv;
>>> struct mtkaif_param *param;
>>> @@ -306,6 +312,18 @@ static int
>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
>>> return 0;
>>> }
>>>
>>> + for_each_card_widgets(rtd->card, w) {
>>> + if (!strcmp(w->name, "MTKAIF_PIN")) {
>>
>> if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
>> pin_w = w;
>> break;
>> }
>>
>> That's safer.
>>
>
> If w->name is MTKAIF, the strncmp expression will return 0. However,
> the result is not expected. I prefer to keep strcmp here.
>

You could also do, instead

if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))

...solving your concern.

Regards,
Angelo

>>> + pin_w = w;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (!pin_w)
>>
>> Just a nitpick: you're checking for `if (pin_w)` later in this
>> function, so
>> to increase readability please do the same here.
>>
>> if (pin_w)
>> dapm_pinctrl_event(...)
>> else
>> dev_dbg(...)
>>
>
> OK. I will update it in v2.
>
> Thanks,
> Trevor



2023-07-26 12:06:58

by Trevor Wu (吳文良)

[permalink] [raw]
Subject: Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

On Wed, 2023-07-26 at 08:43 +0200, AngeloGioacchino Del Regno wrote:
> Il 26/07/23 04:19, Trevor Wu (吳文良) ha scritto:
> > On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 25/07/23 05:53, Trevor Wu ha scritto:
> > > > To avoid power leakage, it is recommended to replace the
> > > > default
> > > > pinctrl
> > > > state with dynamic pinctrl since certain audio pinmux functions
> > > > can
> > > > remain in a HIGH state even when audio is disabled. Linking
> > > > pinctrl
> > > > with
> > > > DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins
> > > > remain
> > > > in
> > > > GPIO mode by default and only switch to an audio function when
> > > > necessary.
> > > >
> > > > Signed-off-by: Trevor Wu <[email protected]>
> > > > ---
> > > > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
> > > > +++++++++++++++++++++
> > > > 1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > index 7c9e08e6a4f5..667d79f33bf2 100644
> > > > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
> > > > mt8188_mt6359_widgets[] = {
> > > > SND_SOC_DAPM_MIC("Headset Mic", NULL),
> > > > SND_SOC_DAPM_SINK("HDMI"),
> > > > SND_SOC_DAPM_SINK("DP"),
> > > > +
> > > > + /* dynamic pinctrl */
> > > > + SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
> > > > "aud_etdm_spk_off"),
> > > > + SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
> > > > "aud_etdm_hp_off"),
> > > > + SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
> > > > "aud_mtkaif_off"),
> > > > };
> > > >
> > > > static const struct snd_kcontrol_new
> > > > mt8188_mt6359_controls[] = {
> > > > @@ -267,6 +272,7 @@ static int
> > > > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
> > > > *rtd)
> > > > snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
> > > > struct snd_soc_component *cmpnt_codec =
> > > > asoc_rtd_to_codec(rtd, 0)->component;
> > > > + struct snd_soc_dapm_widget *pin_w = NULL, *w;
> > > > struct mtk_base_afe *afe;
> > > > struct mt8188_afe_private *afe_priv;
> > > > struct mtkaif_param *param;
> > > > @@ -306,6 +312,18 @@ static int
> > > > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
> > > > *rtd)
> > > > return 0;
> > > > }
> > > >
> > > > + for_each_card_widgets(rtd->card, w) {
> > > > + if (!strcmp(w->name, "MTKAIF_PIN")) {
> > >
> > > if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
> > > pin_w = w;
> > > break;
> > > }
> > >
> > > That's safer.
> > >
> >
> > If w->name is MTKAIF, the strncmp expression will return 0.
> > However,
> > the result is not expected. I prefer to keep strcmp here.
> >
>
> You could also do, instead
>
> if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))
>
> ...solving your concern.
>
>

From my understanding, strncmp is utilized to determine a string begins
with a particular prefix while strcmp is used to compare a whole
string. In this scenario, I wish to verify if the widget name is
exactly 'MTKAIF_PIN', so I believe using strcmp would be more
appropriate.

Using either strlen(w->name) or strlen("MTKAIF_PIN") may lead to
incorrect results when w->name is either MTKAIF or MTKAIF_PIN1.

Thanks,
Trevor

Subject: Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Il 26/07/23 13:36, Trevor Wu (吳文良) ha scritto:
> On Wed, 2023-07-26 at 08:43 +0200, AngeloGioacchino Del Regno wrote:
>> Il 26/07/23 04:19, Trevor Wu (吳文良) ha scritto:
>>> On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>> Il 25/07/23 05:53, Trevor Wu ha scritto:
>>>>> To avoid power leakage, it is recommended to replace the
>>>>> default
>>>>> pinctrl
>>>>> state with dynamic pinctrl since certain audio pinmux functions
>>>>> can
>>>>> remain in a HIGH state even when audio is disabled. Linking
>>>>> pinctrl
>>>>> with
>>>>> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins
>>>>> remain
>>>>> in
>>>>> GPIO mode by default and only switch to an audio function when
>>>>> necessary.
>>>>>
>>>>> Signed-off-by: Trevor Wu <[email protected]>
>>>>> ---
>>>>> sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
>>>>> +++++++++++++++++++++
>>>>> 1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> index 7c9e08e6a4f5..667d79f33bf2 100644
>>>>> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
>>>>> mt8188_mt6359_widgets[] = {
>>>>> SND_SOC_DAPM_MIC("Headset Mic", NULL),
>>>>> SND_SOC_DAPM_SINK("HDMI"),
>>>>> SND_SOC_DAPM_SINK("DP"),
>>>>> +
>>>>> + /* dynamic pinctrl */
>>>>> + SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
>>>>> "aud_etdm_spk_off"),
>>>>> + SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
>>>>> "aud_etdm_hp_off"),
>>>>> + SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
>>>>> "aud_mtkaif_off"),
>>>>> };
>>>>>
>>>>> static const struct snd_kcontrol_new
>>>>> mt8188_mt6359_controls[] = {
>>>>> @@ -267,6 +272,7 @@ static int
>>>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
>>>>> *rtd)
>>>>> snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
>>>>> struct snd_soc_component *cmpnt_codec =
>>>>> asoc_rtd_to_codec(rtd, 0)->component;
>>>>> + struct snd_soc_dapm_widget *pin_w = NULL, *w;
>>>>> struct mtk_base_afe *afe;
>>>>> struct mt8188_afe_private *afe_priv;
>>>>> struct mtkaif_param *param;
>>>>> @@ -306,6 +312,18 @@ static int
>>>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
>>>>> *rtd)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> + for_each_card_widgets(rtd->card, w) {
>>>>> + if (!strcmp(w->name, "MTKAIF_PIN")) {
>>>>
>>>> if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
>>>> pin_w = w;
>>>> break;
>>>> }
>>>>
>>>> That's safer.
>>>>
>>>
>>> If w->name is MTKAIF, the strncmp expression will return 0.
>>> However,
>>> the result is not expected. I prefer to keep strcmp here.
>>>
>>
>> You could also do, instead
>>
>> if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))
>>
>> ...solving your concern.
>>
>>
>
> From my understanding, strncmp is utilized to determine a string begins
> with a particular prefix while strcmp is used to compare a whole
> string. In this scenario, I wish to verify if the widget name is
> exactly 'MTKAIF_PIN', so I believe using strcmp would be more
> appropriate.
>
> Using either strlen(w->name) or strlen("MTKAIF_PIN") may lead to
> incorrect results when w->name is either MTKAIF or MTKAIF_PIN1.
>
> Thanks,
> Trevor

strcmp() and strncmp() are the same; except strncmp() compares *at most* `n` bytes,
where `n` is my `strlen("MTKAIF_PIN")`.

From Linux man pages....
https://www.man7.org/linux/man-pages/man3/strcmp.3.html

Regards,
Angelo

2023-07-27 02:38:03

by Trevor Wu (吳文良)

[permalink] [raw]
Subject: Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

On Wed, 2023-07-26 at 13:54 +0200, AngeloGioacchino Del Regno wrote:

..snip..

> > > > > >
> > > > > > @@ -306,6 +312,18 @@ static int
> > > > > > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
> > > > > > *rtd)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > + for_each_card_widgets(rtd->card, w) {
> > > > > > + if (!strcmp(w->name, "MTKAIF_PIN")) {
> > > > >
> > > > > if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
> > > > > pin_w = w;
> > > > > break;
> > > > > }
> > > > >
> > > > > That's safer.
> > > > >
> > > >
> > > > If w->name is MTKAIF, the strncmp expression will return 0.
> > > > However,
> > > > the result is not expected. I prefer to keep strcmp here.
> > > >
> > >
> > > You could also do, instead
> > >
> > > if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))
> > >
> > > ...solving your concern.
> > >
> > >
> >
> > From my understanding, strncmp is utilized to determine a string
> > begins
> > with a particular prefix while strcmp is used to compare a whole
> > string. In this scenario, I wish to verify if the widget name is
> > exactly 'MTKAIF_PIN', so I believe using strcmp would be more
> > appropriate.
> >
> > Using either strlen(w->name) or strlen("MTKAIF_PIN") may lead to
> > incorrect results when w->name is either MTKAIF or MTKAIF_PIN1.
> >
> > Thanks,
> > Trevor
>
> strcmp() and strncmp() are the same; except strncmp() compares *at
> most* `n` bytes,
> where `n` is my `strlen("MTKAIF_PIN")`.
>
> From Linux man pages....
>
https://urldefense.com/v3/__https://www.man7.org/linux/man-pages/man3/strcmp.3.html__;!!CTRNKA9wMg0ARbw!n-X-ckbkVYv4cbrhpwb2f7NH6sQkxx1czmCU2-q5BtSOhQU0C68wj9JNcu47YfbYeTpVEzjYQVXGuQb5ulpeWwKjnVMdZpg$ 
>
>
>

Hi Angelo,

My concern is that strncmp() compares at most `n` bytes, where `n` is
the length of the string 'MTKAIF_PIN'. For instance, if both
'MTKAIF_PIN' and 'MTKAIF_PINMUX' exist in the widget list, they will
both enter execute_something() function below when executing this code:

if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN")) == 0) {
execute_something();
}.

This is not my expected scenario. To prevent this problem, we can use
strcmp() instead of strncmp(). strcmp() compares two strings until it
finds a difference or reaches the end of one of them. Therefore, it
will compare the entire string 'MTKAIF_PIN' with w->name and make sure
that only do execute_something() when w->names is the same as
'MTKAIF_PIN'.

Thanks,
Trevor

Subject: Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Il 25/07/23 05:53, Trevor Wu ha scritto:
> To avoid power leakage, it is recommended to replace the default pinctrl
> state with dynamic pinctrl since certain audio pinmux functions can
> remain in a HIGH state even when audio is disabled. Linking pinctrl with
> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain in
> GPIO mode by default and only switch to an audio function when necessary.
>
> Signed-off-by: Trevor Wu <[email protected]>

Reviewed-by: AngeloGioacchino Del Regno <[email protected]>