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

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

[Trevor Wu]

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

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

[Trevor Wu]

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 <trevor.wu@mediatek.com>
---
 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

[PATCH 2/3] ASoC: mediatek: common: revise SOF common code

[Trevor Wu]

Originally, normal dai link fixup callback is overwritten by sof fixup
callback on mtk_sof_card_late_probe and it relies on the mapping defined
on struct sof_conn_stream.

It's not flexible. When a new hardware connection is adopted, user needs
to update struct sof_conn_stream defined in machine driver which is used
to specify the mapping relationship of normal BE and SOF BE.

In the patch, mtk_sof_check_tplg_be_dai_link_fixup() is introduced for
all normal BEs. In mtk_sof_late_probe, back up normal BE fixup if it
exists and then overwrite be_hw_params_fixup by the new callback.

There are two cases for FE and BE connection.

case 1:
SOF FE -> normal BE
       -> SOF_BE

case 2:
normal FE -> normal BE

In the new fixup callback, it tries to find SOF_BE which connects to the
same FE, and then reuses the fixup of SOF_BE. If no SOF_BE exists,
it must be case 2, so rollback to the original fixup if it exists.

As a result, the predefined relation is not needed anymore. Hardware
connection can be controlled by the mixer control for AFE interconn.
Then, DPCM finds the BE mapping at runtime.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
---
 .../soc/mediatek/common/mtk-dsp-sof-common.c  | 106 +++++++++++++++---
 .../soc/mediatek/common/mtk-dsp-sof-common.h  |   8 ++
 2 files changed, 99 insertions(+), 15 deletions(-)

diff --git a/sound/soc/mediatek/common/mtk-dsp-sof-common.c b/sound/soc/mediatek/common/mtk-dsp-sof-common.c
index 6fef16306f74..ef49c2c0e253 100644
--- a/sound/soc/mediatek/common/mtk-dsp-sof-common.c
+++ b/sound/soc/mediatek/common/mtk-dsp-sof-common.c
@@ -54,6 +54,8 @@ int mtk_sof_card_probe(struct snd_soc_card *card)
 {
 	int i;
 	struct snd_soc_dai_link *dai_link;
+	struct mtk_soc_card_data *soc_card_data = snd_soc_card_get_drvdata(card);
+	struct mtk_sof_priv *sof_priv = soc_card_data->sof_priv;
 
 	/* Set stream_name to help sof bind widgets */
 	for_each_card_prelinks(card, i, dai_link) {
@@ -61,10 +63,74 @@ int mtk_sof_card_probe(struct snd_soc_card *card)
 			dai_link->stream_name = dai_link->name;
 	}
 
+	INIT_LIST_HEAD(&sof_priv->dai_link_list);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(mtk_sof_card_probe);
 
+/* fixup the BE DAI link to match any values from topology */
+static int mtk_sof_check_tplg_be_dai_link_fixup(struct snd_soc_pcm_runtime *rtd,
+						struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_card *card = rtd->card;
+	struct mtk_soc_card_data *soc_card_data = snd_soc_card_get_drvdata(card);
+	struct mtk_sof_priv *sof_priv = soc_card_data->sof_priv;
+	struct snd_soc_pcm_runtime *fe;
+	struct snd_soc_pcm_runtime *be;
+	struct snd_soc_dai_link *sof_dai_link = NULL;
+	struct mtk_dai_link *dai_link;
+	struct snd_soc_dpcm *dpcm;
+	int i, stream;
+	int ret = 0;
+
+	for_each_pcm_streams(stream) {
+		fe = NULL;
+		for_each_dpcm_fe(rtd, stream, dpcm) {
+			fe = dpcm->fe;
+			if (fe)
+				break;
+		}
+
+		if (!fe)
+			continue;
+
+		for_each_dpcm_be(fe, stream, dpcm) {
+			be = dpcm->be;
+			if (be == rtd)
+				continue;
+
+			for (i = 0; i < sof_priv->num_streams; i++) {
+				const struct sof_conn_stream *conn = &sof_priv->conn_streams[i];
+
+				if (!strcmp(be->dai_link->name, conn->sof_link)) {
+					sof_dai_link = be->dai_link;
+					goto FIXUP;
+				}
+			}
+		}
+	}
+
+FIXUP:
+	if (sof_dai_link) {
+		if (sof_priv->sof_dai_link_fixup)
+			ret = sof_priv->sof_dai_link_fixup(rtd, params);
+		else if (sof_dai_link->be_hw_params_fixup)
+			ret = sof_dai_link->be_hw_params_fixup(be, params);
+	} else {
+		list_for_each_entry(dai_link, &sof_priv->dai_link_list, list) {
+			if (strcmp(dai_link->name, rtd->dai_link->name) == 0) {
+				if (dai_link->be_hw_params_fixup)
+					ret = dai_link->be_hw_params_fixup(rtd, params);
+
+				break;
+			}
+		}
+	}
+
+	return ret;
+}
+
 int mtk_sof_card_late_probe(struct snd_soc_card *card)
 {
 	struct snd_soc_pcm_runtime *rtd;
@@ -72,6 +138,8 @@ int mtk_sof_card_late_probe(struct snd_soc_card *card)
 	struct mtk_soc_card_data *soc_card_data =
 		snd_soc_card_get_drvdata(card);
 	struct mtk_sof_priv *sof_priv = soc_card_data->sof_priv;
+	struct snd_soc_dai_link *dai_link;
+	struct mtk_dai_link *mtk_dai_link;
 	int i;
 
 	/* 1. find sof component */
@@ -86,25 +154,37 @@ int mtk_sof_card_late_probe(struct snd_soc_card *card)
 		return 0;
 	}
 
-	/* 2. add route path and fixup callback */
+	/* 2. overwrite all BE fixups, and backup the existing fixup */
+	for_each_card_prelinks(card, i, dai_link) {
+		if (dai_link->be_hw_params_fixup) {
+			mtk_dai_link = devm_kzalloc(card->dev,
+						    sizeof(*mtk_dai_link),
+						    GFP_KERNEL);
+			if (mtk_dai_link == NULL)
+				return -ENOMEM;
+
+			mtk_dai_link->be_hw_params_fixup = dai_link->be_hw_params_fixup;
+			mtk_dai_link->name = dai_link->name;
+
+			list_add(&mtk_dai_link->list, &sof_priv->dai_link_list);
+		}
+
+		if (dai_link->no_pcm)
+			dai_link->be_hw_params_fixup = mtk_sof_check_tplg_be_dai_link_fixup;
+	}
+
+	/* 3. add route path and SOF_BE fixup callback */
 	for (i = 0; i < sof_priv->num_streams; i++) {
 		const struct sof_conn_stream *conn = &sof_priv->conn_streams[i];
 		struct snd_soc_pcm_runtime *sof_rtd = NULL;
-		struct snd_soc_pcm_runtime *normal_rtd = NULL;
 
 		for_each_card_rtds(card, rtd) {
 			if (!strcmp(rtd->dai_link->name, conn->sof_link)) {
 				sof_rtd = rtd;
-				continue;
-			}
-			if (!strcmp(rtd->dai_link->name, conn->normal_link)) {
-				normal_rtd = rtd;
-				continue;
-			}
-			if (normal_rtd && sof_rtd)
 				break;
+			}
 		}
-		if (normal_rtd && sof_rtd) {
+		if (sof_rtd) {
 			int j;
 			struct snd_soc_dai *cpu_dai;
 
@@ -131,13 +211,9 @@ int mtk_sof_card_late_probe(struct snd_soc_card *card)
 				}
 			}
 
+			/* overwrite SOF BE fixup */
 			sof_rtd->dai_link->be_hw_params_fixup =
 				sof_comp->driver->be_hw_params_fixup;
-			if (sof_priv->sof_dai_link_fixup)
-				normal_rtd->dai_link->be_hw_params_fixup =
-					sof_priv->sof_dai_link_fixup;
-			else
-				normal_rtd->dai_link->be_hw_params_fixup = mtk_sof_dai_link_fixup;
 		}
 	}
 
diff --git a/sound/soc/mediatek/common/mtk-dsp-sof-common.h b/sound/soc/mediatek/common/mtk-dsp-sof-common.h
index dd38c4a93574..4bc5e1c0c8ed 100644
--- a/sound/soc/mediatek/common/mtk-dsp-sof-common.h
+++ b/sound/soc/mediatek/common/mtk-dsp-sof-common.h
@@ -18,11 +18,19 @@ struct sof_conn_stream {
 	int stream_dir;
 };
 
+struct mtk_dai_link {
+	const char *name;
+	int (*be_hw_params_fixup)(struct snd_soc_pcm_runtime *rtd,
+				  struct snd_pcm_hw_params *params);
+	struct list_head list;
+};
+
 struct mtk_sof_priv {
 	const struct sof_conn_stream *conn_streams;
 	int num_streams;
 	int (*sof_dai_link_fixup)(struct snd_soc_pcm_runtime *rtd,
 				  struct snd_pcm_hw_params *params);
+	struct list_head dai_link_list;
 };
 
 int mtk_sof_dai_link_fixup(struct snd_soc_pcm_runtime *rtd,
-- 
2.18.0

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

[Trevor Wu]

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 <trevor.wu@mediatek.com>
---
 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

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

[AngeloGioacchino Del Regno]

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 <trevor.wu@mediatek.com>
> ---
>   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

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

[AngeloGioacchino Del Regno]

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 <trevor.wu@mediatek.com>
> ---
>   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

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

[Trevor Wu (吳文良)]

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 <trevor.wu@mediatek.com>
> > ---
> >   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

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

[Trevor Wu (吳文良)]

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 <trevor.wu@mediatek.com>
> > ---
> >   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

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

[AngeloGioacchino Del Regno]

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 <trevor.wu@mediatek.com>
>>> ---
>>>    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

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

[Trevor Wu (吳文良)]

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 <trevor.wu@mediatek.com>
> > > > ---
> > > >    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

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

[AngeloGioacchino Del Regno]

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 <trevor.wu@mediatek.com>
>>>>> ---
>>>>>     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

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

[Trevor Wu (吳文良)]

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

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

[AngeloGioacchino Del Regno]

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 <trevor.wu@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>