[v7 0/4] ASoC: mediatek: mt8192: support rt1015p_rt5682s

[v7 0/4] ASoC: mediatek: mt8192: support rt1015p_rt5682s

[Jiaxin Yu]

From: Jiaxin Yu <jiaxin.yu@mediatek.corp-partner.google.com>

The series reuses mt8192-mt6359-rt10150rt5682.c for supporting machine
driver with rt1015p speaker amplifier and rt5682s headset codec.

Changes from v6:
  - "speaker-codec" changes to "speaker-codecs" due to there may be two
    speaker codec.

Changes from v5:
  - "mediatek,headset-codec" and "mediatek,speaker-codec" drop prefix
    and move to properties from patternProperties.

Changes form v4:
  - split a large patch into three small patches for easy reviewing
  - correct coding style

Changes from v3:
  - fix build error: too many arguments for format
    [-Werror-format-extra-args]

Changes from v2:
  - fix build warnings such as "data argument not used by format string"

Changes from v1:
  - uses the snd_soc_of_get_dai_link_codecs to complete the
  configuration of dai_link's codecs
  - uses definitions to simplifies card name and compatible name

Jiaxin Yu (4):
  ASoC: dt-bindings: mt8192-mt6359: add new compatible and new
    properties
  ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker
  ASoC: mediatek: mt8192: refactor for I2S8/I2S9 DAI links of headset
  ASoC: mediatek: mt8192: support rt1015p_rt5682s

 .../sound/mt8192-mt6359-rt1015-rt5682.yaml    |  32 +++
 sound/soc/mediatek/Kconfig                    |   1 +
 .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 201 +++++++++++-------
 3 files changed, 156 insertions(+), 78 deletions(-)

-- 
2.18.0

[v7 1/4] ASoC: dt-bindings: mt8192-mt6359: add new compatible and new properties

[Jiaxin Yu]

1. Adds new compatible string "mt8192_mt6359_rt1015p_rt5682s" for machine
with rt1015p and rt5682s.
2. Adds new property "headset-codec" for getting headset codec.
3. Adds new property "speaker-codecs" for getting speaker codecs.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
---
 .../sound/mt8192-mt6359-rt1015-rt5682.yaml    | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
index a781e7aaaa38..385fcf5dd40f 100644
--- a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
+++ b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - mediatek,mt8192_mt6359_rt1015_rt5682
       - mediatek,mt8192_mt6359_rt1015p_rt5682
+      - mediatek,mt8192_mt6359_rt1015p_rt5682s
 
   mediatek,platform:
     $ref: "/schemas/types.yaml#/definitions/phandle"
@@ -27,11 +28,33 @@ properties:
     $ref: "/schemas/types.yaml#/definitions/phandle"
     description: The phandle of HDMI codec.
 
+  headset-codec:
+    type: object
+    properties:
+      sound-dai:
+        $ref: /schemas/types.yaml#/definitions/phandle
+    required:
+      - sound-dai
+
+  speaker-codecs:
+    type: object
+    properties:
+      sound-dai:
+        minItems: 1
+        maxItems: 2
+        items:
+          maxItems: 1
+        $ref: /schemas/types.yaml#/definitions/phandle-array
+    required:
+      - sound-dai
+
 additionalProperties: false
 
 required:
   - compatible
   - mediatek,platform
+  - headset-codec
+  - speaker-codecs
 
 examples:
   - |
@@ -44,6 +67,15 @@ examples:
                         "aud_clk_mosi_on";
         pinctrl-0 = <&aud_clk_mosi_off>;
         pinctrl-1 = <&aud_clk_mosi_on>;
+
+        headset-codec {
+            sound-dai = <&rt5682>;
+        };
+
+        speaker-codecs {
+            sound-dai = <&rt1015_l>,
+                        <&rt1015_r>;
+        };
     };
 
 ...
-- 
2.18.0

[v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Jiaxin Yu]

MT8192 platform will use rt1015 or rt105p codec, so through the
snd_soc_of_get_dai_link_codecs() to complete the configuration
of dai_link's codecs.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 108 ++++++++++--------
 1 file changed, 59 insertions(+), 49 deletions(-)

diff --git a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
index ee91569c0911..837c2ccd5b3d 100644
--- a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
+++ b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
@@ -604,17 +604,9 @@ SND_SOC_DAILINK_DEFS(i2s2,
 		     DAILINK_COMP_ARRAY(COMP_DUMMY()),
 		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
 
-SND_SOC_DAILINK_DEFS(i2s3_rt1015,
+SND_SOC_DAILINK_DEFS(i2s3,
 		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
-		     DAILINK_COMP_ARRAY(COMP_CODEC(RT1015_DEV0_NAME,
-						   RT1015_CODEC_DAI),
-					COMP_CODEC(RT1015_DEV1_NAME,
-						   RT1015_CODEC_DAI)),
-		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
-
-SND_SOC_DAILINK_DEFS(i2s3_rt1015p,
-		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
-		     DAILINK_COMP_ARRAY(COMP_CODEC("rt1015p", "HiFi")),
+		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
 		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
 
 SND_SOC_DAILINK_DEFS(i2s5,
@@ -929,6 +921,7 @@ static struct snd_soc_dai_link mt8192_mt6359_dai_links[] = {
 		.dpcm_playback = 1,
 		.ignore_suspend = 1,
 		.be_hw_params_fixup = mt8192_i2s_hw_params_fixup,
+		SND_SOC_DAILINK_REG(i2s3),
 	},
 	{
 		.name = "I2S5",
@@ -1100,55 +1093,64 @@ static struct snd_soc_card mt8192_mt6359_rt1015p_rt5682_card = {
 	.num_dapm_routes = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_routes),
 };
 
+static int mt8192_mt6359_card_set_be_link(struct snd_soc_card *card,
+					  struct snd_soc_dai_link *link,
+					  struct device_node *node,
+					  char *link_name)
+{
+	int ret;
+
+	if (node && strcmp(link->name, link_name) == 0) {
+		ret = snd_soc_of_get_dai_link_codecs(card->dev, node, link);
+		if (ret < 0) {
+			dev_err_probe(card->dev, ret, "get dai link codecs fail\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card;
-	struct device_node *platform_node, *hdmi_codec;
+	struct device_node *platform_node, *hdmi_codec, *speaker_codec;
 	int ret, i;
 	struct snd_soc_dai_link *dai_link;
 	struct mt8192_mt6359_priv *priv;
 
-	platform_node = of_parse_phandle(pdev->dev.of_node,
-					 "mediatek,platform", 0);
-	if (!platform_node) {
-		dev_err(&pdev->dev, "Property 'platform' missing or invalid\n");
+	card = (struct snd_soc_card *)of_device_get_match_data(&pdev->dev);
+	if (!card)
 		return -EINVAL;
+	card->dev = &pdev->dev;
+
+	platform_node = of_parse_phandle(pdev->dev.of_node, "mediatek,platform", 0);
+	if (!platform_node) {
+		ret = -EINVAL;
+		dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n");
+		goto err_platform_node;
 	}
 
-	card = (struct snd_soc_card *)of_device_get_match_data(&pdev->dev);
-	if (!card) {
+	hdmi_codec = of_parse_phandle(pdev->dev.of_node, "mediatek,hdmi-codec", 0);
+	if (!hdmi_codec) {
 		ret = -EINVAL;
-		goto put_platform_node;
+		dev_err_probe(&pdev->dev, ret, "Property 'hdmi-codec' missing or invalid\n");
+		goto err_hdmi_codec;
 	}
-	card->dev = &pdev->dev;
 
-	hdmi_codec = of_parse_phandle(pdev->dev.of_node,
-				      "mediatek,hdmi-codec", 0);
+	speaker_codec = of_get_child_by_name(pdev->dev.of_node, "speaker-codecs");
+	if (!speaker_codec) {
+		ret = -EINVAL;
+		dev_err_probe(&pdev->dev, ret, "Property 'speaker-codecs' missing or invalid\n");
+		goto err_speaker_codec;
+	}
 
 	for_each_card_prelinks(card, i, dai_link) {
-		if (strcmp(dai_link->name, "I2S3") == 0) {
-			if (card == &mt8192_mt6359_rt1015_rt5682_card) {
-				dai_link->ops = &mt8192_rt1015_i2s_ops;
-				dai_link->cpus = i2s3_rt1015_cpus;
-				dai_link->num_cpus =
-					ARRAY_SIZE(i2s3_rt1015_cpus);
-				dai_link->codecs = i2s3_rt1015_codecs;
-				dai_link->num_codecs =
-					ARRAY_SIZE(i2s3_rt1015_codecs);
-				dai_link->platforms = i2s3_rt1015_platforms;
-				dai_link->num_platforms =
-					ARRAY_SIZE(i2s3_rt1015_platforms);
-			} else if (card == &mt8192_mt6359_rt1015p_rt5682_card) {
-				dai_link->cpus = i2s3_rt1015p_cpus;
-				dai_link->num_cpus =
-					ARRAY_SIZE(i2s3_rt1015p_cpus);
-				dai_link->codecs = i2s3_rt1015p_codecs;
-				dai_link->num_codecs =
-					ARRAY_SIZE(i2s3_rt1015p_codecs);
-				dai_link->platforms = i2s3_rt1015p_platforms;
-				dai_link->num_platforms =
-					ARRAY_SIZE(i2s3_rt1015p_platforms);
-			}
+		ret = mt8192_mt6359_card_set_be_link(card, dai_link, speaker_codec, "I2S3");
+		if (ret) {
+			dev_err_probe(&pdev->dev, ret, "%s set speaker_codec fail\n",
+				      dai_link->name);
+			goto err_probe;
 		}
 
 		if (hdmi_codec && strcmp(dai_link->name, "TDM") == 0) {
@@ -1156,6 +1158,9 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 			dai_link->ignore = 0;
 		}
 
+		if (strcmp(dai_link->codecs[0].dai_name, RT1015_CODEC_DAI) == 0)
+			dai_link->ops = &mt8192_rt1015_i2s_ops;
+
 		if (!dai_link->platforms->name)
 			dai_link->platforms->of_node = platform_node;
 	}
@@ -1163,22 +1168,27 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
-		goto put_hdmi_codec;
+		goto err_probe;
 	}
 	snd_soc_card_set_drvdata(card, priv);
 
 	ret = mt8192_afe_gpio_init(&pdev->dev);
 	if (ret) {
-		dev_err(&pdev->dev, "init gpio error %d\n", ret);
-		goto put_hdmi_codec;
+		dev_err_probe(&pdev->dev, ret, "%s init gpio error\n", __func__);
+		goto err_probe;
 	}
 
 	ret = devm_snd_soc_register_card(&pdev->dev, card);
+	if (ret)
+		dev_err_probe(&pdev->dev, ret, "%s snd_soc_register_card fail\n", __func__);
 
-put_hdmi_codec:
+err_probe:
+	of_node_put(speaker_codec);
+err_speaker_codec:
 	of_node_put(hdmi_codec);
-put_platform_node:
+err_hdmi_codec:
 	of_node_put(platform_node);
+err_platform_node:
 	return ret;
 }
 
-- 
2.18.0

[v7 3/4] ASoC: mediatek: mt8192: refactor for I2S8/I2S9 DAI links of headset

[Jiaxin Yu]

MT8192 platform use rt5682 codec, so through the snd_soc_of_get_dai_link_codecs()
to complete the configuration of I2S8/I2S9 dai_link's codecs.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 34 ++++++++++++++-----
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
index 837c2ccd5b3d..1d2821d484ff 100644
--- a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
+++ b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
@@ -28,9 +28,6 @@
 #define RT1015_DEV0_NAME	"rt1015.1-0028"
 #define RT1015_DEV1_NAME	"rt1015.1-0029"
 
-#define RT5682_CODEC_DAI	"rt5682-aif1"
-#define RT5682_DEV0_NAME	"rt5682.1-001a"
-
 struct mt8192_mt6359_priv {
 	struct snd_soc_jack headset_jack;
 	struct snd_soc_jack hdmi_jack;
@@ -626,14 +623,12 @@ SND_SOC_DAILINK_DEFS(i2s7,
 
 SND_SOC_DAILINK_DEFS(i2s8,
 		     DAILINK_COMP_ARRAY(COMP_CPU("I2S8")),
-		     DAILINK_COMP_ARRAY(COMP_CODEC(RT5682_DEV0_NAME,
-						   RT5682_CODEC_DAI)),
+		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
 		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
 
 SND_SOC_DAILINK_DEFS(i2s9,
 		     DAILINK_COMP_ARRAY(COMP_CPU("I2S9")),
-		     DAILINK_COMP_ARRAY(COMP_CODEC(RT5682_DEV0_NAME,
-						   RT5682_CODEC_DAI)),
+		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
 		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
 
 SND_SOC_DAILINK_DEFS(connsys_i2s,
@@ -1114,7 +1109,7 @@ static int mt8192_mt6359_card_set_be_link(struct snd_soc_card *card,
 static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card;
-	struct device_node *platform_node, *hdmi_codec, *speaker_codec;
+	struct device_node *platform_node, *hdmi_codec, *headset_codec, *speaker_codec;
 	int ret, i;
 	struct snd_soc_dai_link *dai_link;
 	struct mt8192_mt6359_priv *priv;
@@ -1145,6 +1140,13 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 		goto err_speaker_codec;
 	}
 
+	headset_codec = of_get_child_by_name(pdev->dev.of_node, "headset-codec");
+	if (!headset_codec) {
+		ret = -EINVAL;
+		dev_err_probe(&pdev->dev, ret, "Property 'headset-codec' missing or invalid\n");
+		goto err_headset_codec;
+	}
+
 	for_each_card_prelinks(card, i, dai_link) {
 		ret = mt8192_mt6359_card_set_be_link(card, dai_link, speaker_codec, "I2S3");
 		if (ret) {
@@ -1153,6 +1155,20 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 			goto err_probe;
 		}
 
+		ret = mt8192_mt6359_card_set_be_link(card, dai_link, headset_codec, "I2S8");
+		if (ret) {
+			dev_err_probe(&pdev->dev, ret, "%s set headset_codec fail\n",
+				      dai_link->name);
+			goto err_probe;
+		}
+
+		ret = mt8192_mt6359_card_set_be_link(card, dai_link, headset_codec, "I2S9");
+		if (ret) {
+			dev_err_probe(&pdev->dev, ret, "%s set headset_codec fail\n",
+				      dai_link->name);
+			goto err_probe;
+		}
+
 		if (hdmi_codec && strcmp(dai_link->name, "TDM") == 0) {
 			dai_link->codecs->of_node = hdmi_codec;
 			dai_link->ignore = 0;
@@ -1183,6 +1199,8 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 		dev_err_probe(&pdev->dev, ret, "%s snd_soc_register_card fail\n", __func__);
 
 err_probe:
+	of_node_put(headset_codec);
+err_headset_codec:
 	of_node_put(speaker_codec);
 err_speaker_codec:
 	of_node_put(hdmi_codec);
-- 
2.18.0

[v7 4/4] ASoC: mediatek: mt8192: support rt1015p_rt5682s

[Jiaxin Yu]

To support machine that only choose one of the rt5682s and rt5682 as
headset codec, adds new compatible string "mt8192_mt6359_rt1015p_rt5682s".
Meanwhile, using macros to simplifies card name and compatible name.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 sound/soc/mediatek/Kconfig                    |  1 +
 .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 61 ++++++++++++-------
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
index d515613a79da..cacfbab4262d 100644
--- a/sound/soc/mediatek/Kconfig
+++ b/sound/soc/mediatek/Kconfig
@@ -176,6 +176,7 @@ config SND_SOC_MT8192_MT6359_RT1015_RT5682
 	select SND_SOC_RT1015
 	select SND_SOC_RT1015P
 	select SND_SOC_RT5682_I2C
+	select SND_SOC_RT5682S
 	select SND_SOC_DMIC
 	help
 	  This adds ASoC driver for Mediatek MT8192 boards
diff --git a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
index 1d2821d484ff..fcb6e3da8ef5 100644
--- a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
+++ b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
@@ -28,6 +28,14 @@
 #define RT1015_DEV0_NAME	"rt1015.1-0028"
 #define RT1015_DEV1_NAME	"rt1015.1-0029"
 
+#define RT1015_RT5682_CARD_NAME "mt8192_mt6359_rt1015_rt5682"
+#define RT1015P_RT5682_CARD_NAME "mt8192_mt6359_rt1015p_rt5682"
+#define RT1015P_RT5682S_CARD_NAME "mt8192_mt6359_rt1015p_rt5682s"
+
+#define RT1015_RT5682_OF_NAME "mediatek,mt8192_mt6359_rt1015_rt5682"
+#define RT1015P_RT5682_OF_NAME "mediatek,mt8192_mt6359_rt1015p_rt5682"
+#define RT1015P_RT5682S_OF_NAME "mediatek,mt8192_mt6359_rt1015p_rt5682s"
+
 struct mt8192_mt6359_priv {
 	struct snd_soc_jack headset_jack;
 	struct snd_soc_jack hdmi_jack;
@@ -68,8 +76,8 @@ static int mt8192_rt1015_i2s_hw_params(struct snd_pcm_substream *substream,
 	return snd_soc_dai_set_sysclk(cpu_dai, 0, mclk_fs, SND_SOC_CLOCK_OUT);
 }
 
-static int mt8192_rt5682_i2s_hw_params(struct snd_pcm_substream *substream,
-				       struct snd_pcm_hw_params *params)
+static int mt8192_rt5682x_i2s_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_card *card = rtd->card;
@@ -118,8 +126,8 @@ static const struct snd_soc_ops mt8192_rt1015_i2s_ops = {
 	.hw_params = mt8192_rt1015_i2s_hw_params,
 };
 
-static const struct snd_soc_ops mt8192_rt5682_i2s_ops = {
-	.hw_params = mt8192_rt5682_i2s_hw_params,
+static const struct snd_soc_ops mt8192_rt5682x_i2s_ops = {
+	.hw_params = mt8192_rt5682x_i2s_hw_params,
 };
 
 static int mt8192_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
@@ -950,7 +958,7 @@ static struct snd_soc_dai_link mt8192_mt6359_dai_links[] = {
 		.init = mt8192_rt5682_init,
 		.be_hw_params_fixup = mt8192_i2s_hw_params_fixup,
 		SND_SOC_DAILINK_REG(i2s8),
-		.ops = &mt8192_rt5682_i2s_ops,
+		.ops = &mt8192_rt5682x_i2s_ops,
 	},
 	{
 		.name = "I2S9",
@@ -959,7 +967,7 @@ static struct snd_soc_dai_link mt8192_mt6359_dai_links[] = {
 		.ignore_suspend = 1,
 		.be_hw_params_fixup = mt8192_i2s_hw_params_fixup,
 		SND_SOC_DAILINK_REG(i2s9),
-		.ops = &mt8192_rt5682_i2s_ops,
+		.ops = &mt8192_rt5682x_i2s_ops,
 	},
 	{
 		.name = "CONNSYS_I2S",
@@ -1039,7 +1047,7 @@ static struct snd_soc_codec_conf rt1015_amp_conf[] = {
 };
 
 static struct snd_soc_card mt8192_mt6359_rt1015_rt5682_card = {
-	.name = "mt8192_mt6359_rt1015_rt5682",
+	.name = RT1015_RT5682_CARD_NAME,
 	.owner = THIS_MODULE,
 	.dai_link = mt8192_mt6359_dai_links,
 	.num_links = ARRAY_SIZE(mt8192_mt6359_dai_links),
@@ -1053,14 +1061,13 @@ static struct snd_soc_card mt8192_mt6359_rt1015_rt5682_card = {
 	.num_configs = ARRAY_SIZE(rt1015_amp_conf),
 };
 
-static const struct snd_soc_dapm_widget
-mt8192_mt6359_rt1015p_rt5682_widgets[] = {
+static const struct snd_soc_dapm_widget mt8192_mt6359_rt1015p_rt5682x_widgets[] = {
 	SND_SOC_DAPM_SPK("Speakers", NULL),
 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
 };
 
-static const struct snd_soc_dapm_route mt8192_mt6359_rt1015p_rt5682_routes[] = {
+static const struct snd_soc_dapm_route mt8192_mt6359_rt1015p_rt5682x_routes[] = {
 	/* speaker */
 	{ "Speakers", NULL, "Speaker" },
 	/* headset */
@@ -1069,23 +1076,22 @@ static const struct snd_soc_dapm_route mt8192_mt6359_rt1015p_rt5682_routes[] = {
 	{ "IN1P", NULL, "Headset Mic" },
 };
 
-static const struct snd_kcontrol_new mt8192_mt6359_rt1015p_rt5682_controls[] = {
+static const struct snd_kcontrol_new mt8192_mt6359_rt1015p_rt5682x_controls[] = {
 	SOC_DAPM_PIN_SWITCH("Speakers"),
 	SOC_DAPM_PIN_SWITCH("Headphone Jack"),
 	SOC_DAPM_PIN_SWITCH("Headset Mic"),
 };
 
-static struct snd_soc_card mt8192_mt6359_rt1015p_rt5682_card = {
-	.name = "mt8192_mt6359_rt1015p_rt5682",
+static struct snd_soc_card mt8192_mt6359_rt1015p_rt5682x_card = {
 	.owner = THIS_MODULE,
 	.dai_link = mt8192_mt6359_dai_links,
 	.num_links = ARRAY_SIZE(mt8192_mt6359_dai_links),
-	.controls = mt8192_mt6359_rt1015p_rt5682_controls,
-	.num_controls = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_controls),
-	.dapm_widgets = mt8192_mt6359_rt1015p_rt5682_widgets,
-	.num_dapm_widgets = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_widgets),
-	.dapm_routes = mt8192_mt6359_rt1015p_rt5682_routes,
-	.num_dapm_routes = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_routes),
+	.controls = mt8192_mt6359_rt1015p_rt5682x_controls,
+	.num_controls = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682x_controls),
+	.dapm_widgets = mt8192_mt6359_rt1015p_rt5682x_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682x_widgets),
+	.dapm_routes = mt8192_mt6359_rt1015p_rt5682x_routes,
+	.num_dapm_routes = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682x_routes),
 };
 
 static int mt8192_mt6359_card_set_be_link(struct snd_soc_card *card,
@@ -1119,6 +1125,13 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 		return -EINVAL;
 	card->dev = &pdev->dev;
 
+	if (of_device_is_compatible(pdev->dev.of_node, RT1015P_RT5682_OF_NAME))
+		card->name = RT1015P_RT5682_CARD_NAME;
+	else if (of_device_is_compatible(pdev->dev.of_node, RT1015P_RT5682S_OF_NAME))
+		card->name = RT1015P_RT5682S_CARD_NAME;
+	else
+		dev_dbg(&pdev->dev, "No need to set card name\n");
+
 	platform_node = of_parse_phandle(pdev->dev.of_node, "mediatek,platform", 0);
 	if (!platform_node) {
 		ret = -EINVAL;
@@ -1213,12 +1226,16 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
 #ifdef CONFIG_OF
 static const struct of_device_id mt8192_mt6359_dt_match[] = {
 	{
-		.compatible = "mediatek,mt8192_mt6359_rt1015_rt5682",
+		.compatible = RT1015_RT5682_OF_NAME,
 		.data = &mt8192_mt6359_rt1015_rt5682_card,
 	},
 	{
-		.compatible = "mediatek,mt8192_mt6359_rt1015p_rt5682",
-		.data = &mt8192_mt6359_rt1015p_rt5682_card,
+		.compatible = RT1015P_RT5682_OF_NAME,
+		.data = &mt8192_mt6359_rt1015p_rt5682x_card,
+	},
+	{
+		.compatible = RT1015P_RT5682S_OF_NAME,
+		.data = &mt8192_mt6359_rt1015p_rt5682x_card,
 	},
 	{}
 };
-- 
2.18.0

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Nícolas F. R. A. Prado]

Hi Jiaxin,

On Thu, Mar 24, 2022 at 02:45:09PM +0800, Jiaxin Yu wrote:
> MT8192 platform will use rt1015 or rt105p codec, so through the
> snd_soc_of_get_dai_link_codecs() to complete the configuration
> of dai_link's codecs.
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 108 ++++++++++--------
>  1 file changed, 59 insertions(+), 49 deletions(-)
> 
> diff --git a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> index ee91569c0911..837c2ccd5b3d 100644
> --- a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> +++ b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> @@ -604,17 +604,9 @@ SND_SOC_DAILINK_DEFS(i2s2,
>  		     DAILINK_COMP_ARRAY(COMP_DUMMY()),
>  		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
>  
> -SND_SOC_DAILINK_DEFS(i2s3_rt1015,
> +SND_SOC_DAILINK_DEFS(i2s3,
>  		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
> -		     DAILINK_COMP_ARRAY(COMP_CODEC(RT1015_DEV0_NAME,
> -						   RT1015_CODEC_DAI),
> -					COMP_CODEC(RT1015_DEV1_NAME,
> -						   RT1015_CODEC_DAI)),
> -		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> -
> -SND_SOC_DAILINK_DEFS(i2s3_rt1015p,
> -		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
> -		     DAILINK_COMP_ARRAY(COMP_CODEC("rt1015p", "HiFi")),
> +		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
>  		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
>  
>  SND_SOC_DAILINK_DEFS(i2s5,
> @@ -929,6 +921,7 @@ static struct snd_soc_dai_link mt8192_mt6359_dai_links[] = {
>  		.dpcm_playback = 1,
>  		.ignore_suspend = 1,
>  		.be_hw_params_fixup = mt8192_i2s_hw_params_fixup,
> +		SND_SOC_DAILINK_REG(i2s3),
>  	},
>  	{
>  		.name = "I2S5",
> @@ -1100,55 +1093,64 @@ static struct snd_soc_card mt8192_mt6359_rt1015p_rt5682_card = {
>  	.num_dapm_routes = ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_routes),
>  };
>  
> +static int mt8192_mt6359_card_set_be_link(struct snd_soc_card *card,
> +					  struct snd_soc_dai_link *link,
> +					  struct device_node *node,
> +					  char *link_name)
> +{
> +	int ret;
> +
> +	if (node && strcmp(link->name, link_name) == 0) {
> +		ret = snd_soc_of_get_dai_link_codecs(card->dev, node, link);
> +		if (ret < 0) {
> +			dev_err_probe(card->dev, ret, "get dai link codecs fail\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
>  {
>  	struct snd_soc_card *card;
> -	struct device_node *platform_node, *hdmi_codec;
> +	struct device_node *platform_node, *hdmi_codec, *speaker_codec;
>  	int ret, i;
>  	struct snd_soc_dai_link *dai_link;
>  	struct mt8192_mt6359_priv *priv;
>  
> -	platform_node = of_parse_phandle(pdev->dev.of_node,
> -					 "mediatek,platform", 0);
> -	if (!platform_node) {
> -		dev_err(&pdev->dev, "Property 'platform' missing or invalid\n");
> +	card = (struct snd_soc_card *)of_device_get_match_data(&pdev->dev);
> +	if (!card)
>  		return -EINVAL;
> +	card->dev = &pdev->dev;
> +
> +	platform_node = of_parse_phandle(pdev->dev.of_node, "mediatek,platform", 0);
> +	if (!platform_node) {
> +		ret = -EINVAL;
> +		dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n");
> +		goto err_platform_node;
>  	}
>  
> -	card = (struct snd_soc_card *)of_device_get_match_data(&pdev->dev);
> -	if (!card) {
> +	hdmi_codec = of_parse_phandle(pdev->dev.of_node, "mediatek,hdmi-codec", 0);
> +	if (!hdmi_codec) {
>  		ret = -EINVAL;
> -		goto put_platform_node;
> +		dev_err_probe(&pdev->dev, ret, "Property 'hdmi-codec' missing or invalid\n");
> +		goto err_hdmi_codec;

You're making hdmi-codec a required property, since now the driver fails to
probe without it. Is it really required though? The driver code still checks for
the presence of hdmi_codec before using it, so shouldn't it be fine to let it be
optional?

If it is really required now though, then I guess at least the dt-binding should
be updated accordingly. (Although I think this would technically break the ABI?)

Thanks,
Nícolas

>  	}
> -	card->dev = &pdev->dev;
>  
> -	hdmi_codec = of_parse_phandle(pdev->dev.of_node,
> -				      "mediatek,hdmi-codec", 0);
> +	speaker_codec = of_get_child_by_name(pdev->dev.of_node, "speaker-codecs");
> +	if (!speaker_codec) {
> +		ret = -EINVAL;
> +		dev_err_probe(&pdev->dev, ret, "Property 'speaker-codecs' missing or invalid\n");
> +		goto err_speaker_codec;
> +	}
>  
>  	for_each_card_prelinks(card, i, dai_link) {
> -		if (strcmp(dai_link->name, "I2S3") == 0) {
> -			if (card == &mt8192_mt6359_rt1015_rt5682_card) {
> -				dai_link->ops = &mt8192_rt1015_i2s_ops;
> -				dai_link->cpus = i2s3_rt1015_cpus;
> -				dai_link->num_cpus =
> -					ARRAY_SIZE(i2s3_rt1015_cpus);
> -				dai_link->codecs = i2s3_rt1015_codecs;
> -				dai_link->num_codecs =
> -					ARRAY_SIZE(i2s3_rt1015_codecs);
> -				dai_link->platforms = i2s3_rt1015_platforms;
> -				dai_link->num_platforms =
> -					ARRAY_SIZE(i2s3_rt1015_platforms);
> -			} else if (card == &mt8192_mt6359_rt1015p_rt5682_card) {
> -				dai_link->cpus = i2s3_rt1015p_cpus;
> -				dai_link->num_cpus =
> -					ARRAY_SIZE(i2s3_rt1015p_cpus);
> -				dai_link->codecs = i2s3_rt1015p_codecs;
> -				dai_link->num_codecs =
> -					ARRAY_SIZE(i2s3_rt1015p_codecs);
> -				dai_link->platforms = i2s3_rt1015p_platforms;
> -				dai_link->num_platforms =
> -					ARRAY_SIZE(i2s3_rt1015p_platforms);
> -			}
> +		ret = mt8192_mt6359_card_set_be_link(card, dai_link, speaker_codec, "I2S3");
> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret, "%s set speaker_codec fail\n",
> +				      dai_link->name);
> +			goto err_probe;
>  		}
>  
>  		if (hdmi_codec && strcmp(dai_link->name, "TDM") == 0) {
> @@ -1156,6 +1158,9 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
>  			dai_link->ignore = 0;
>  		}
>  
> +		if (strcmp(dai_link->codecs[0].dai_name, RT1015_CODEC_DAI) == 0)
> +			dai_link->ops = &mt8192_rt1015_i2s_ops;
> +
>  		if (!dai_link->platforms->name)
>  			dai_link->platforms->of_node = platform_node;
>  	}
> @@ -1163,22 +1168,27 @@ static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv) {
>  		ret = -ENOMEM;
> -		goto put_hdmi_codec;
> +		goto err_probe;
>  	}
>  	snd_soc_card_set_drvdata(card, priv);
>  
>  	ret = mt8192_afe_gpio_init(&pdev->dev);
>  	if (ret) {
> -		dev_err(&pdev->dev, "init gpio error %d\n", ret);
> -		goto put_hdmi_codec;
> +		dev_err_probe(&pdev->dev, ret, "%s init gpio error\n", __func__);
> +		goto err_probe;
>  	}
>  
>  	ret = devm_snd_soc_register_card(&pdev->dev, card);
> +	if (ret)
> +		dev_err_probe(&pdev->dev, ret, "%s snd_soc_register_card fail\n", __func__);
>  
> -put_hdmi_codec:
> +err_probe:
> +	of_node_put(speaker_codec);
> +err_speaker_codec:
>  	of_node_put(hdmi_codec);
> -put_platform_node:
> +err_hdmi_codec:
>  	of_node_put(platform_node);
> +err_platform_node:
>  	return ret;
>  }
>  
> -- 
> 2.18.0
> 
> 

Re: [v7 1/4] ASoC: dt-bindings: mt8192-mt6359: add new compatible and new properties

[Rob Herring]

On Thu, 24 Mar 2022 14:45:08 +0800, Jiaxin Yu wrote:
> 1. Adds new compatible string "mt8192_mt6359_rt1015p_rt5682s" for machine
> with rt1015p and rt5682s.
> 2. Adds new property "headset-codec" for getting headset codec.
> 3. Adds new property "speaker-codecs" for getting speaker codecs.
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> ---
>  .../sound/mt8192-mt6359-rt1015-rt5682.yaml    | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Jiaxin Yu]

On Tue, 2022-03-29 at 18:30 -0400, Nícolas F. R. A. Prado wrote:
> Hi Jiaxin,
> 
> On Thu, Mar 24, 2022 at 02:45:09PM +0800, Jiaxin Yu wrote:
> > MT8192 platform will use rt1015 or rt105p codec, so through the
> > snd_soc_of_get_dai_link_codecs() to complete the configuration
> > of dai_link's codecs.
> > 
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > ---
> >  .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 108 ++++++++++--
> > ------
> >  1 file changed, 59 insertions(+), 49 deletions(-)
> > 
> > diff --git a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-
> > rt5682.c b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> > index ee91569c0911..837c2ccd5b3d 100644
> > --- a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> > +++ b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> > @@ -604,17 +604,9 @@ SND_SOC_DAILINK_DEFS(i2s2,
> >  		     DAILINK_COMP_ARRAY(COMP_DUMMY()),
> >  		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> >  
> > -SND_SOC_DAILINK_DEFS(i2s3_rt1015,
> > +SND_SOC_DAILINK_DEFS(i2s3,
> >  		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
> > -		     DAILINK_COMP_ARRAY(COMP_CODEC(RT1015_DEV0_NAME,
> > -						   RT1015_CODEC_DAI),
> > -					COMP_CODEC(RT1015_DEV1_NAME,
> > -						   RT1015_CODEC_DAI)),
> > -		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> > -
> > -SND_SOC_DAILINK_DEFS(i2s3_rt1015p,
> > -		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
> > -		     DAILINK_COMP_ARRAY(COMP_CODEC("rt1015p", "HiFi")),
> > +		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
> >  		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> >  
> >  SND_SOC_DAILINK_DEFS(i2s5,
> > @@ -929,6 +921,7 @@ static struct snd_soc_dai_link
> > mt8192_mt6359_dai_links[] = {
> >  		.dpcm_playback = 1,
> >  		.ignore_suspend = 1,
> >  		.be_hw_params_fixup = mt8192_i2s_hw_params_fixup,
> > +		SND_SOC_DAILINK_REG(i2s3),
> >  	},
> >  	{
> >  		.name = "I2S5",
> > @@ -1100,55 +1093,64 @@ static struct snd_soc_card
> > mt8192_mt6359_rt1015p_rt5682_card = {
> >  	.num_dapm_routes =
> > ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_routes),
> >  };
> >  
> > +static int mt8192_mt6359_card_set_be_link(struct snd_soc_card
> > *card,
> > +					  struct snd_soc_dai_link
> > *link,
> > +					  struct device_node *node,
> > +					  char *link_name)
> > +{
> > +	int ret;
> > +
> > +	if (node && strcmp(link->name, link_name) == 0) {
> > +		ret = snd_soc_of_get_dai_link_codecs(card->dev, node,
> > link);
> > +		if (ret < 0) {
> > +			dev_err_probe(card->dev, ret, "get dai link
> > codecs fail\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
> >  {
> >  	struct snd_soc_card *card;
> > -	struct device_node *platform_node, *hdmi_codec;
> > +	struct device_node *platform_node, *hdmi_codec, *speaker_codec;
> >  	int ret, i;
> >  	struct snd_soc_dai_link *dai_link;
> >  	struct mt8192_mt6359_priv *priv;
> >  
> > -	platform_node = of_parse_phandle(pdev->dev.of_node,
> > -					 "mediatek,platform", 0);
> > -	if (!platform_node) {
> > -		dev_err(&pdev->dev, "Property 'platform' missing or
> > invalid\n");
> > +	card = (struct snd_soc_card *)of_device_get_match_data(&pdev-
> > >dev);
> > +	if (!card)
> >  		return -EINVAL;
> > +	card->dev = &pdev->dev;
> > +
> > +	platform_node = of_parse_phandle(pdev->dev.of_node,
> > "mediatek,platform", 0);
> > +	if (!platform_node) {
> > +		ret = -EINVAL;
> > +		dev_err_probe(&pdev->dev, ret, "Property 'platform'
> > missing or invalid\n");
> > +		goto err_platform_node;
> >  	}
> >  
> > -	card = (struct snd_soc_card *)of_device_get_match_data(&pdev-
> > >dev);
> > -	if (!card) {
> > +	hdmi_codec = of_parse_phandle(pdev->dev.of_node,
> > "mediatek,hdmi-codec", 0);
> > +	if (!hdmi_codec) {
> >  		ret = -EINVAL;
> > -		goto put_platform_node;
> > +		dev_err_probe(&pdev->dev, ret, "Property 'hdmi-codec'
> > missing or invalid\n");
> > +		goto err_hdmi_codec;
> 
> You're making hdmi-codec a required property, since now the driver
> fails to
> probe without it. Is it really required though? The driver code still
> checks for
> the presence of hdmi_codec before using it, so shouldn't it be fine
> to let it be
> optional?
> 
> If it is really required now though, then I guess at least the dt-
> binding should
> be updated accordingly. (Although I think this would technically
> break the ABI?)
> 
> Thanks,
> Nícolas

Hi Nícolas,

Thanks for your comment. Indeed I made hdmi-codec a required property,
because it is a must in this machine driver. I prefer to report errors
during the registration rather than during the use.

So I'd like to take your second suggestion. I need to update dt-binding 
that set hdmi-codec as required property.

"(Although I think this would technicallybreak the ABI?)"
==> I can't understand this question, could you help explain it in more
detail.

Thanks,
Jiaxin.Yu

> 
>  	}
> > -	card->dev = &pdev->dev;
> >  
> > -	hdmi_codec = of_parse_phandle(pdev->dev.of_node,
> > -				      "mediatek,hdmi-codec", 0);
> > +	speaker_codec = of_get_child_by_name(pdev->dev.of_node,
> > "speaker-codecs");
> > +	if (!speaker_codec) {
> > +		ret = -EINVAL;
> > +		dev_err_probe(&pdev->dev, ret, "Property 'speaker-
> > codecs' missing or invalid\n");
> > +		goto err_speaker_codec;
> > +	}
> >  
> 
snip...
> >  
> > -put_hdmi_codec:
> > +err_probe:
> > +	of_node_put(speaker_codec);
> > +err_speaker_codec:
> >  	of_node_put(hdmi_codec);
> > -put_platform_node:
> > +err_hdmi_codec:
> >  	of_node_put(platform_node);
> > +err_platform_node:
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.18.0
> > 
> > 

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Mark Brown]

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

On Wed, Mar 30, 2022 at 10:33:06AM +0800, Jiaxin Yu wrote:

> "(Although I think this would technicallybreak the ABI?)"
> ==> I can't understand this question, could you help explain it in more
> detail.

Making a previously optional property required means that systems that
previously worked may stop working unless they update their DT, DTs may
be distributed separately to the kernel and perhaps even baked into
firmware or similar.

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

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Jiaxin Yu]

On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:
> On Wed, Mar 30, 2022 at 10:33:06AM +0800, Jiaxin Yu wrote:
> 
> > "(Although I think this would technicallybreak the ABI?)"
> > ==> I can't understand this question, could you help explain it in
> > more
> > detail.
> 
> Making a previously optional property required means that systems
> that
> previously worked may stop working unless they update their DT, DTs
> may
> be distributed separately to the kernel and perhaps even baked into
> firmware or similar.

Hi Mark,

Thank you for your detailed answer. I should keep the driver's behavior
consistent with the description of dt-bindings. The "mediatek,hdmi-
codec" needs to be set as the required property. Is my understanding
right?

Thanks,
Jiaxin.Yu

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Mark Brown]

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

On Wed, Mar 30, 2022 at 10:06:24PM +0800, Jiaxin Yu wrote:
> On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:

> > Making a previously optional property required means that systems
> > that
> > previously worked may stop working unless they update their DT, DTs
> > may
> > be distributed separately to the kernel and perhaps even baked into
> > firmware or similar.

> Thank you for your detailed answer. I should keep the driver's behavior
> consistent with the description of dt-bindings. The "mediatek,hdmi-
> codec" needs to be set as the required property. Is my understanding
> right?

The binding document and code should match so if one is changed the
other needs to be changed too.

In theory we should never change a previously optional property to
required which would mean that the code should be updated to reflect the
binding document, however sometimes the DT isn't actually used as a
stable intereface by anything for a given property or device type so we
can get away with changing things.

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

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Nícolas F. R. A. Prado]

On Wed, Mar 30, 2022 at 10:33:06AM +0800, Jiaxin Yu wrote:
> On Tue, 2022-03-29 at 18:30 -0400, Nícolas F. R. A. Prado wrote:
> > Hi Jiaxin,
> > 
> > On Thu, Mar 24, 2022 at 02:45:09PM +0800, Jiaxin Yu wrote:
> > > MT8192 platform will use rt1015 or rt105p codec, so through the
> > > snd_soc_of_get_dai_link_codecs() to complete the configuration
> > > of dai_link's codecs.
> > > 
> > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > > ---
> > >  .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 108 ++++++++++--
> > > ------
> > >  1 file changed, 59 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-
> > > rt5682.c b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> > > index ee91569c0911..837c2ccd5b3d 100644
> > > --- a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> > > +++ b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> > > @@ -604,17 +604,9 @@ SND_SOC_DAILINK_DEFS(i2s2,
> > >  		     DAILINK_COMP_ARRAY(COMP_DUMMY()),
> > >  		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> > >  
> > > -SND_SOC_DAILINK_DEFS(i2s3_rt1015,
> > > +SND_SOC_DAILINK_DEFS(i2s3,
> > >  		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
> > > -		     DAILINK_COMP_ARRAY(COMP_CODEC(RT1015_DEV0_NAME,
> > > -						   RT1015_CODEC_DAI),
> > > -					COMP_CODEC(RT1015_DEV1_NAME,
> > > -						   RT1015_CODEC_DAI)),
> > > -		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> > > -
> > > -SND_SOC_DAILINK_DEFS(i2s3_rt1015p,
> > > -		     DAILINK_COMP_ARRAY(COMP_CPU("I2S3")),
> > > -		     DAILINK_COMP_ARRAY(COMP_CODEC("rt1015p", "HiFi")),
> > > +		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
> > >  		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> > >  
> > >  SND_SOC_DAILINK_DEFS(i2s5,
> > > @@ -929,6 +921,7 @@ static struct snd_soc_dai_link
> > > mt8192_mt6359_dai_links[] = {
> > >  		.dpcm_playback = 1,
> > >  		.ignore_suspend = 1,
> > >  		.be_hw_params_fixup = mt8192_i2s_hw_params_fixup,
> > > +		SND_SOC_DAILINK_REG(i2s3),
> > >  	},
> > >  	{
> > >  		.name = "I2S5",
> > > @@ -1100,55 +1093,64 @@ static struct snd_soc_card
> > > mt8192_mt6359_rt1015p_rt5682_card = {
> > >  	.num_dapm_routes =
> > > ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_routes),
> > >  };
> > >  
> > > +static int mt8192_mt6359_card_set_be_link(struct snd_soc_card
> > > *card,
> > > +					  struct snd_soc_dai_link
> > > *link,
> > > +					  struct device_node *node,
> > > +					  char *link_name)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (node && strcmp(link->name, link_name) == 0) {
> > > +		ret = snd_soc_of_get_dai_link_codecs(card->dev, node,
> > > link);
> > > +		if (ret < 0) {
> > > +			dev_err_probe(card->dev, ret, "get dai link
> > > codecs fail\n");
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int mt8192_mt6359_dev_probe(struct platform_device *pdev)
> > >  {
> > >  	struct snd_soc_card *card;
> > > -	struct device_node *platform_node, *hdmi_codec;
> > > +	struct device_node *platform_node, *hdmi_codec, *speaker_codec;
> > >  	int ret, i;
> > >  	struct snd_soc_dai_link *dai_link;
> > >  	struct mt8192_mt6359_priv *priv;
> > >  
> > > -	platform_node = of_parse_phandle(pdev->dev.of_node,
> > > -					 "mediatek,platform", 0);
> > > -	if (!platform_node) {
> > > -		dev_err(&pdev->dev, "Property 'platform' missing or
> > > invalid\n");
> > > +	card = (struct snd_soc_card *)of_device_get_match_data(&pdev-
> > > >dev);
> > > +	if (!card)
> > >  		return -EINVAL;
> > > +	card->dev = &pdev->dev;
> > > +
> > > +	platform_node = of_parse_phandle(pdev->dev.of_node,
> > > "mediatek,platform", 0);
> > > +	if (!platform_node) {
> > > +		ret = -EINVAL;
> > > +		dev_err_probe(&pdev->dev, ret, "Property 'platform'
> > > missing or invalid\n");
> > > +		goto err_platform_node;
> > >  	}
> > >  
> > > -	card = (struct snd_soc_card *)of_device_get_match_data(&pdev-
> > > >dev);
> > > -	if (!card) {
> > > +	hdmi_codec = of_parse_phandle(pdev->dev.of_node,
> > > "mediatek,hdmi-codec", 0);
> > > +	if (!hdmi_codec) {
> > >  		ret = -EINVAL;
> > > -		goto put_platform_node;
> > > +		dev_err_probe(&pdev->dev, ret, "Property 'hdmi-codec'
> > > missing or invalid\n");
> > > +		goto err_hdmi_codec;
> > 
> > You're making hdmi-codec a required property, since now the driver
> > fails to
> > probe without it. Is it really required though? The driver code still
> > checks for
> > the presence of hdmi_codec before using it, so shouldn't it be fine
> > to let it be
> > optional?
> > 
> > If it is really required now though, then I guess at least the dt-
> > binding should
> > be updated accordingly. (Although I think this would technically
> > break the ABI?)
> > 
> > Thanks,
> > Nícolas
> 
> Hi Nícolas,
> 
> Thanks for your comment. Indeed I made hdmi-codec a required property,
> because it is a must in this machine driver. I prefer to report errors
> during the registration rather than during the use.

But what do you mean that it is required in this machine driver? The code checks
for presence of hdmi_codec and ignores it if it's not set, so it does really
seem optional to me. Also, I have tested this driver on mt8192-asurada-spherion
without hdmi-codec set in the DT and the speaker and headphone sound works just
fine.

Besides, there might be machines using this driver that don't support HDMI, and
requiring an hdmi-codec in the DT for them would not make any sense. So keeping
hdmi-codec as optional seems like the most sensible solution to me, really.

Thanks,
Nícolas

> 
> So I'd like to take your second suggestion. I need to update dt-binding 
> that set hdmi-codec as required property.
> 
> "(Although I think this would technicallybreak the ABI?)"
> ==> I can't understand this question, could you help explain it in more
> detail.
> 
> Thanks,
> Jiaxin.Yu
> 
> > 
> >  	}
> > > -	card->dev = &pdev->dev;
> > >  
> > > -	hdmi_codec = of_parse_phandle(pdev->dev.of_node,
> > > -				      "mediatek,hdmi-codec", 0);
> > > +	speaker_codec = of_get_child_by_name(pdev->dev.of_node,
> > > "speaker-codecs");
> > > +	if (!speaker_codec) {
> > > +		ret = -EINVAL;
> > > +		dev_err_probe(&pdev->dev, ret, "Property 'speaker-
> > > codecs' missing or invalid\n");
> > > +		goto err_speaker_codec;
> > > +	}
> > >  
> > 
> snip...
> > >  
> > > -put_hdmi_codec:
> > > +err_probe:
> > > +	of_node_put(speaker_codec);
> > > +err_speaker_codec:
> > >  	of_node_put(hdmi_codec);
> > > -put_platform_node:
> > > +err_hdmi_codec:
> > >  	of_node_put(platform_node);
> > > +err_platform_node:
> > >  	return ret;
> > >  }
> > >  
> > > -- 
> > > 2.18.0
> > > 
> > > 
> 

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Jiaxin Yu]

On Wed, 2022-03-30 at 15:24 +0100, Mark Brown wrote:
> On Wed, Mar 30, 2022 at 10:06:24PM +0800, Jiaxin Yu wrote:
> > On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:
> > > Making a previously optional property required means that systems
> > > that
> > > previously worked may stop working unless they update their DT,
> > > DTs
> > > may
> > > be distributed separately to the kernel and perhaps even baked
> > > into
> > > firmware or similar.
> > Thank you for your detailed answer. I should keep the driver's
> > behavior
> > consistent with the description of dt-bindings. The "mediatek,hdmi-
> > codec" needs to be set as the required property. Is my
> > understanding
> > right?
> 
> The binding document and code should match so if one is changed the
> other needs to be changed too.
> 
> In theory we should never change a previously optional property to
> required which would mean that the code should be updated to reflect
> the
> binding document, however sometimes the DT isn't actually used as a
> stable intereface by anything for a given property or device type so
> we
> can get away with changing things.

"however sometimes the DT isn't actually used as a stable intereface by
anything for a given property or device type so we can get away with
changing things."

Sorry, I don't understand the real idea of this description. Does it
mean that dt-bindings in this series don't need to be updated, but the
driver?

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Nícolas F. R. A. Prado]

On Wed, Mar 30, 2022 at 11:48:01PM +0800, Jiaxin Yu wrote:
> On Wed, 2022-03-30 at 15:24 +0100, Mark Brown wrote:
> > On Wed, Mar 30, 2022 at 10:06:24PM +0800, Jiaxin Yu wrote:
> > > On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:
> > > > Making a previously optional property required means that systems
> > > > that
> > > > previously worked may stop working unless they update their DT,
> > > > DTs
> > > > may
> > > > be distributed separately to the kernel and perhaps even baked
> > > > into
> > > > firmware or similar.
> > > Thank you for your detailed answer. I should keep the driver's
> > > behavior
> > > consistent with the description of dt-bindings. The "mediatek,hdmi-
> > > codec" needs to be set as the required property. Is my
> > > understanding
> > > right?
> > 
> > The binding document and code should match so if one is changed the
> > other needs to be changed too.
> > 
> > In theory we should never change a previously optional property to
> > required which would mean that the code should be updated to reflect
> > the
> > binding document, however sometimes the DT isn't actually used as a
> > stable intereface by anything for a given property or device type so
> > we
> > can get away with changing things.
> 
> "however sometimes the DT isn't actually used as a stable intereface by
> anything for a given property or device type so we can get away with
> changing things."
> 
> Sorry, I don't understand the real idea of this description. Does it
> mean that dt-bindings in this series don't need to be updated, but the
> driver?

He means that usually the DT (and dt-binding) shouldn't be changed to avoid
incompatibilities, but sometimes it's OK to change them. For example if there
are no users of the DT yet.

But in any case, like I mentioned in my latest reply [1], I don't think changing
the dt-binding is the proper solution in this case. The driver should be changed
instead.

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20220330152026.6nuigsldx46lue44@notapiano

Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker

[Jiaxin Yu]

On Wed, 2022-03-30 at 11:20 -0400, Nícolas F. R. A. Prado wrote:

> > > >  static int mt8192_mt6359_dev_probe(struct platform_device
> > > > *pdev)
> > > >  {
> > > >  	struct snd_soc_card *card;
> > > > -	struct device_node *platform_node, *hdmi_codec;
> > > > +	struct device_node *platform_node, *hdmi_codec,
> > > > *speaker_codec;
> > > >  	int ret, i;
> > > >  	struct snd_soc_dai_link *dai_link;
> > > >  	struct mt8192_mt6359_priv *priv;
> > > >  
> > > > -	platform_node = of_parse_phandle(pdev->dev.of_node,
> > > > -					 "mediatek,platform",
> > > > 0);
> > > > -	if (!platform_node) {
> > > > -		dev_err(&pdev->dev, "Property 'platform'
> > > > missing or
> > > > invalid\n");
> > > > +	card = (struct snd_soc_card
> > > > *)of_device_get_match_data(&pdev-
> > > > > dev);
> > > > 
> > > > +	if (!card)
> > > >  		return -EINVAL;
> > > > +	card->dev = &pdev->dev;
> > > > +
> > > > +	platform_node = of_parse_phandle(pdev->dev.of_node,
> > > > "mediatek,platform", 0);
> > > > +	if (!platform_node) {
> > > > +		ret = -EINVAL;
> > > > +		dev_err_probe(&pdev->dev, ret, "Property
> > > > 'platform'
> > > > missing or invalid\n");
> > > > +		goto err_platform_node;
> > > >  	}
> > > >  
> > > > -	card = (struct snd_soc_card
> > > > *)of_device_get_match_data(&pdev-
> > > > > dev);
> > > > 
> > > > -	if (!card) {
> > > > +	hdmi_codec = of_parse_phandle(pdev->dev.of_node,
> > > > "mediatek,hdmi-codec", 0);
> > > > +	if (!hdmi_codec) {
> > > >  		ret = -EINVAL;
> > > > -		goto put_platform_node;
> > > > +		dev_err_probe(&pdev->dev, ret, "Property 'hdmi-
> > > > codec'
> > > > missing or invalid\n");
> > > > +		goto err_hdmi_codec;
> > > 
> > > You're making hdmi-codec a required property, since now the
> > > driver
> > > fails to
> > > probe without it. Is it really required though? The driver code
> > > still
> > > checks for
> > > the presence of hdmi_codec before using it, so shouldn't it be
> > > fine
> > > to let it be
> > > optional?
> > > 
> > > If it is really required now though, then I guess at least the
> > > dt-
> > > binding should
> > > be updated accordingly. (Although I think this would technically
> > > break the ABI?)
> > > 
> > > Thanks,
> > > Nícolas
> > 
> > Hi Nícolas,
> > 
> > Thanks for your comment. Indeed I made hdmi-codec a required
> > property,
> > because it is a must in this machine driver. I prefer to report
> > errors
> > during the registration rather than during the use.
> 
> But what do you mean that it is required in this machine driver? The
> code checks
> for presence of hdmi_codec and ignores it if it's not set, so it does
> really
> seem optional to me. Also, I have tested this driver on mt8192-
> asurada-spherion
> without hdmi-codec set in the DT and the speaker and headphone sound
> works just
> fine.
> 
> Besides, there might be machines using this driver that don't support
> HDMI, and
> requiring an hdmi-codec in the DT for them would not make any sense.
> So keeping
> hdmi-codec as optional seems like the most sensible solution to me,
> really.
> 
> Thanks,
> Nícolas

Yes, I agree with you. In the past, if there was a new board without
HDMI audio, we would choose to add a new machine driver and a new dt-
bindings. But now, in order to simplify the code, we tend to share one
machine driver for boards that use similar codecs. And we are doing
this now.

Thanks,
Jiaxin.Yu