[PATCH 1/2] ASoC: mediatek: mt8195: update control for RT5682 series

[PATCH 1/2] ASoC: mediatek: mt8195: update control for RT5682 series

[Trevor Wu]

Playback pop is observed and the root cause is the reference clock
provided by MT8195 is diabled before RT5682 finishes the control flow.

To ensure the reference clock supplied to RT5682 is disabled after RT5682
finishes all register controls. We replace BCLK with MCLK for RT5682
reference clock, and makes use of set_bias_level_post to handle MCLK
which guarantees MCLK is off after all RT5682 register access.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
---
 .../mt8195/mt8195-mt6359-rt1011-rt5682.c      | 66 +++++++++++++++-
 .../mt8195/mt8195-mt6359-rt1019-rt5682.c      | 76 ++++++++++++++++---
 2 files changed, 128 insertions(+), 14 deletions(-)

diff --git a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c
index 5cdbfaafd479..05359365f200 100644
--- a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c
+++ b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c
@@ -37,6 +37,7 @@ struct mt8195_mt6359_rt1011_rt5682_priv {
 	struct snd_soc_jack headset_jack;
 	struct snd_soc_jack dp_jack;
 	struct snd_soc_jack hdmi_jack;
+	struct clk *i2so1_mclk;
 };
 
 static const struct snd_soc_dapm_widget
@@ -87,8 +88,8 @@ static int mt8195_rt5682_etdm_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
-	ret = snd_soc_dai_set_pll(codec_dai, RT5682_PLL1, RT5682_PLL1_S_BCLK1,
-				  rate * 64, rate * 512);
+	ret = snd_soc_dai_set_pll(codec_dai, RT5682_PLL1, RT5682_PLL1_S_MCLK,
+				  rate * 256, rate * 512);
 	if (ret) {
 		dev_err(card->dev, "failed to set pll\n");
 		return ret;
@@ -101,7 +102,7 @@ static int mt8195_rt5682_etdm_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
-	return snd_soc_dai_set_sysclk(cpu_dai, 0, rate * 128,
+	return snd_soc_dai_set_sysclk(cpu_dai, 0, rate * 256,
 				      SND_SOC_CLOCK_OUT);
 }
 
@@ -565,6 +566,51 @@ static const struct snd_soc_ops mt8195_capture_ops = {
 	.startup = mt8195_capture_startup,
 };
 
+static int mt8195_set_bias_level_post(struct snd_soc_card *card,
+	struct snd_soc_dapm_context *dapm, enum snd_soc_bias_level level)
+{
+	struct snd_soc_component *component = dapm->component;
+	struct mt8195_mt6359_rt1011_rt5682_priv *priv =
+		snd_soc_card_get_drvdata(card);
+	int ret = 0;
+
+	/*
+	 * It's required to control mclk directly in the set_bias_level_post
+	 * function for rt5682 and rt5682s codec, or the unexpected pop happens
+	 * at the end of playback.
+	 */
+	if (!component ||
+	    (strcmp(component->name, RT5682_DEV0_NAME) &&
+	    strcmp(component->name, RT5682S_DEV0_NAME)))
+		return 0;
+
+	if (IS_ERR(priv->i2so1_mclk))
+		return 0;
+
+	switch (level) {
+	case SND_SOC_BIAS_OFF:
+		if (!__clk_is_enabled(priv->i2so1_mclk))
+			return 0;
+
+		dev_dbg(card->dev, "Disable i2so1");
+		clk_disable_unprepare(priv->i2so1_mclk);
+		break;
+	case SND_SOC_BIAS_ON:
+		dev_dbg(card->dev, "Enable i2so1");
+		ret = clk_prepare_enable(priv->i2so1_mclk);
+		if (ret) {
+			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
+			return ret;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+
 enum {
 	DAI_LINK_DL2_FE,
 	DAI_LINK_DL3_FE,
@@ -1040,6 +1086,7 @@ static struct snd_soc_card mt8195_mt6359_rt1011_rt5682_soc_card = {
 	.num_dapm_routes = ARRAY_SIZE(mt8195_mt6359_rt1011_rt5682_routes),
 	.codec_conf = rt1011_amp_conf,
 	.num_configs = ARRAY_SIZE(rt1011_amp_conf),
+	.set_bias_level_post = mt8195_set_bias_level_post,
 };
 
 static int mt8195_mt6359_rt1011_rt5682_dev_probe(struct platform_device *pdev)
@@ -1072,6 +1119,19 @@ static int mt8195_mt6359_rt1011_rt5682_dev_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	priv->i2so1_mclk = devm_clk_get(&pdev->dev, "i2so1_mclk");
+	if (IS_ERR(priv->i2so1_mclk)) {
+		ret = PTR_ERR(priv->i2so1_mclk);
+		if (ret == -ENOENT) {
+			dev_dbg(&pdev->dev,
+				"Failed to get i2so1_mclk, defer probe\n");
+			return -EPROBE_DEFER;
+		}
+
+		dev_err(&pdev->dev, "Failed to get i2so1_mclk, err:%d\n", ret);
+		return ret;
+	}
+
 	for_each_card_prelinks(card, i, dai_link) {
 		if (!dai_link->platforms->name)
 			dai_link->platforms->of_node = priv->platform_node;
diff --git a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c
index fa50a31e9718..b2c3b57da9c4 100644
--- a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c
+++ b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c
@@ -50,6 +50,7 @@ struct mt8195_mt6359_rt1019_rt5682_priv {
 	struct snd_soc_jack headset_jack;
 	struct snd_soc_jack dp_jack;
 	struct snd_soc_jack hdmi_jack;
+	struct clk *i2so1_mclk;
 };
 
 static const struct snd_soc_dapm_widget
@@ -96,8 +97,6 @@ static int mt8195_rt5682_etdm_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
 	unsigned int rate = params_rate(params);
-	unsigned int mclk_fs_ratio = 128;
-	unsigned int mclk_fs = rate * mclk_fs_ratio;
 	int bitwidth;
 	int ret;
 
@@ -113,25 +112,22 @@ static int mt8195_rt5682_etdm_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
-	ret = snd_soc_dai_set_pll(codec_dai, RT5682_PLL1,
-				  RT5682_PLL1_S_BCLK1,
-				  params_rate(params) * 64,
-				  params_rate(params) * 512);
+	ret = snd_soc_dai_set_pll(codec_dai, RT5682_PLL1, RT5682_PLL1_S_MCLK,
+				  rate * 256, rate * 512);
 	if (ret) {
 		dev_err(card->dev, "failed to set pll\n");
 		return ret;
 	}
 
-	ret = snd_soc_dai_set_sysclk(codec_dai,
-				     RT5682_SCLK_S_PLL1,
-				     params_rate(params) * 512,
-				     SND_SOC_CLOCK_IN);
+	ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1,
+				     rate * 512, SND_SOC_CLOCK_IN);
 	if (ret) {
 		dev_err(card->dev, "failed to set sysclk\n");
 		return ret;
 	}
 
-	return snd_soc_dai_set_sysclk(cpu_dai, 0, mclk_fs, SND_SOC_CLOCK_OUT);
+	return snd_soc_dai_set_sysclk(cpu_dai, 0, rate * 256,
+				      SND_SOC_CLOCK_OUT);
 }
 
 static const struct snd_soc_ops mt8195_rt5682_etdm_ops = {
@@ -564,6 +560,50 @@ static const struct snd_soc_ops mt8195_capture_ops = {
 	.startup = mt8195_capture_startup,
 };
 
+static int mt8195_set_bias_level_post(struct snd_soc_card *card,
+	struct snd_soc_dapm_context *dapm, enum snd_soc_bias_level level)
+{
+	struct snd_soc_component *component = dapm->component;
+	struct mt8195_mt6359_rt1019_rt5682_priv *priv =
+		snd_soc_card_get_drvdata(card);
+	int ret = 0;
+
+	/*
+	 * It's required to control mclk directly in the set_bias_level_post
+	 * function for rt5682 and rt5682s codec, or the unexpected pop happens
+	 * at the end of playback.
+	 */
+	if (!component ||
+	    (strcmp(component->name, RT5682_DEV0_NAME) &&
+	    strcmp(component->name, RT5682S_DEV0_NAME)))
+		return 0;
+
+	if (IS_ERR(priv->i2so1_mclk))
+		return 0;
+
+	switch (level) {
+	case SND_SOC_BIAS_OFF:
+		if (!__clk_is_enabled(priv->i2so1_mclk))
+			return 0;
+
+		dev_dbg(card->dev, "Disable i2so1");
+		clk_disable_unprepare(priv->i2so1_mclk);
+		break;
+	case SND_SOC_BIAS_ON:
+		dev_dbg(card->dev, "Enable i2so1");
+		ret = clk_prepare_enable(priv->i2so1_mclk);
+		if (ret) {
+			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
+			return ret;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
 enum {
 	DAI_LINK_DL2_FE,
 	DAI_LINK_DL3_FE,
@@ -1203,6 +1243,7 @@ static struct snd_soc_card mt8195_mt6359_rt1019_rt5682_soc_card = {
 	.num_dapm_widgets = ARRAY_SIZE(mt8195_mt6359_rt1019_rt5682_widgets),
 	.dapm_routes = mt8195_mt6359_rt1019_rt5682_routes,
 	.num_dapm_routes = ARRAY_SIZE(mt8195_mt6359_rt1019_rt5682_routes),
+	.set_bias_level_post = mt8195_set_bias_level_post,
 };
 
 static int mt8195_dailink_parse_of(struct snd_soc_card *card, struct device_node *np,
@@ -1285,6 +1326,19 @@ static int mt8195_mt6359_rt1019_rt5682_dev_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	priv->i2so1_mclk = devm_clk_get(&pdev->dev, "i2so1_mclk");
+	if (IS_ERR(priv->i2so1_mclk)) {
+		ret = PTR_ERR(priv->i2so1_mclk);
+		if (ret == -ENOENT) {
+			dev_dbg(&pdev->dev,
+				"Failed to get i2so1_mclk, defer probe\n");
+			return -EPROBE_DEFER;
+		}
+
+		dev_err(&pdev->dev, "Failed to get i2so1_mclk, err:%d\n", ret);
+		return ret;
+	}
+
 	/* dai link */
 	priv->adsp_node = of_parse_phandle(pdev->dev.of_node,
 					   "mediatek,adsp", 0);
-- 
2.18.0


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 2/2] dt-bindings: mediatek: mt8195: add clock property to sound node

[Trevor Wu]

clocks and clock-names are added to provide MCLK phandle for sound card.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
---
 .../bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml  | 12 ++++++++++++
 .../bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml  | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml
index cf6ad7933e23..b57c856d0cf3 100644
--- a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml
+++ b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml
@@ -32,11 +32,21 @@ properties:
     $ref: "/schemas/types.yaml#/definitions/phandle"
     description: The phandle of MT8195 HDMI codec node.
 
+  clocks:
+    items:
+      - description: phandle and clock specifier for codec MCLK.
+
+  clock-names:
+    items:
+      - const: i2so1_mclk
+
 additionalProperties: false
 
 required:
   - compatible
   - mediatek,platform
+  - clocks
+  - clock-names
 
 examples:
   - |
@@ -44,6 +54,8 @@ examples:
     sound: mt8195-sound {
         compatible = "mediatek,mt8195_mt6359_rt1011_rt5682";
         mediatek,platform = <&afe>;
+        clocks = <&topckgen 235>; //CLK_TOP_APLL12_DIV2
+        clock-names = "i2so1_mclk";
         pinctrl-names = "default";
         pinctrl-0 = <&aud_pins_default>;
     };
diff --git a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml
index 8f177e02ad35..e4720f76f66b 100644
--- a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml
+++ b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml
@@ -42,11 +42,21 @@ properties:
       A list of the desired dai-links in the sound card. Each entry is a
       name defined in the machine driver.
 
+  clocks:
+    items:
+      - description: phandle and clock specifier for codec MCLK.
+
+  clock-names:
+    items:
+      - const: i2so1_mclk
+
 additionalProperties: false
 
 required:
   - compatible
   - mediatek,platform
+  - clocks
+  - clock-names
 
 examples:
   - |
@@ -54,6 +64,8 @@ examples:
     sound: mt8195-sound {
         compatible = "mediatek,mt8195_mt6359_rt1019_rt5682";
         mediatek,platform = <&afe>;
+        clocks = <&topckgen 235>; //CLK_TOP_APLL12_DIV2
+        clock-names = "i2so1_mclk";
         pinctrl-names = "default";
         pinctrl-0 = <&aud_pins_default>;
     };
-- 
2.18.0


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 1/2] ASoC: mediatek: mt8195: update control for RT5682 series

[Tzung-Bi Shih]

On Wed, Dec 15, 2021 at 02:58:34PM +0800, Trevor Wu wrote:
> --- a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c
> +++ b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c
[...]
> +static int mt8195_set_bias_level_post(struct snd_soc_card *card,
> +	struct snd_soc_dapm_context *dapm, enum snd_soc_bias_level level)
> +{
> +	struct snd_soc_component *component = dapm->component;
> +	struct mt8195_mt6359_rt1011_rt5682_priv *priv =
> +		snd_soc_card_get_drvdata(card);
> +	int ret = 0;

ret doesn't need to be initialized.

> +	/*
> +	 * It's required to control mclk directly in the set_bias_level_post
> +	 * function for rt5682 and rt5682s codec, or the unexpected pop happens
> +	 * at the end of playback.
> +	 */
> +	if (!component ||
> +	    (strcmp(component->name, RT5682_DEV0_NAME) &&
> +	    strcmp(component->name, RT5682S_DEV0_NAME)))
> +		return 0;
> +
> +	if (IS_ERR(priv->i2so1_mclk))
> +		return 0;

I doubt if it needs to check priv->i2so1_mclk.  In other words, if IS_ERR(priv->i2so1_mclk) is true in _probe, does mt8195_set_bias_level_post() get called?

> +	switch (level) {
> +	case SND_SOC_BIAS_OFF:
> +		if (!__clk_is_enabled(priv->i2so1_mclk))
> +			return 0;
> +
> +		dev_dbg(card->dev, "Disable i2so1");
> +		clk_disable_unprepare(priv->i2so1_mclk);

I would suggest move dev_dbg() later than clk_disable_unprepare() which means "Disable i2so1" is done.

> +		break;
> +	case SND_SOC_BIAS_ON:
> +		dev_dbg(card->dev, "Enable i2so1");
> +		ret = clk_prepare_enable(priv->i2so1_mclk);
> +		if (ret) {
> +			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);

The error message can be more specific.  "Cannot enable i2so1" for example.

> +			return ret;
> +		}

Also, I would suggest move dev_dbg() later than clk_prepare_enable().  Otherwise, it could fail to prepare or enable but still can see "Enable i2so1" message.

> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;

The function doesn't use any gotos.  To be concise, "return 0;".

> @@ -1072,6 +1119,19 @@ static int mt8195_mt6359_rt1011_rt5682_dev_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	priv->i2so1_mclk = devm_clk_get(&pdev->dev, "i2so1_mclk");
> +	if (IS_ERR(priv->i2so1_mclk)) {
> +		ret = PTR_ERR(priv->i2so1_mclk);
> +		if (ret == -ENOENT) {
> +			dev_dbg(&pdev->dev,
> +				"Failed to get i2so1_mclk, defer probe\n");
> +			return -EPROBE_DEFER;
> +		}

Does devm_clk_get_optional() could make the block more concise?

> +
> +		dev_err(&pdev->dev, "Failed to get i2so1_mclk, err:%d\n", ret);

If devm_clk_get() is possible to return -EPROBE_DEFER too, use dev_err_probe().

> --- a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c
> +++ b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c
[...]
> +static int mt8195_set_bias_level_post(struct snd_soc_card *card,
> +	struct snd_soc_dapm_context *dapm, enum snd_soc_bias_level level)
> +{
> +	struct snd_soc_component *component = dapm->component;
> +	struct mt8195_mt6359_rt1019_rt5682_priv *priv =
> +		snd_soc_card_get_drvdata(card);
> +	int ret = 0;

Ditto, see comments above.

> +
> +	/*
> +	 * It's required to control mclk directly in the set_bias_level_post
> +	 * function for rt5682 and rt5682s codec, or the unexpected pop happens
> +	 * at the end of playback.
> +	 */
> +	if (!component ||
> +	    (strcmp(component->name, RT5682_DEV0_NAME) &&
> +	    strcmp(component->name, RT5682S_DEV0_NAME)))
> +		return 0;
> +
> +	if (IS_ERR(priv->i2so1_mclk))
> +		return 0;

Ditto, see comments above.

> +
> +	switch (level) {
> +	case SND_SOC_BIAS_OFF:
> +		if (!__clk_is_enabled(priv->i2so1_mclk))
> +			return 0;
> +
> +		dev_dbg(card->dev, "Disable i2so1");
> +		clk_disable_unprepare(priv->i2so1_mclk);
> +		break;
> +	case SND_SOC_BIAS_ON:
> +		dev_dbg(card->dev, "Enable i2so1");
> +		ret = clk_prepare_enable(priv->i2so1_mclk);
> +		if (ret) {
> +			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
> +			return ret;
> +		}
> +		break;
> +	default:
> +		break;
> +	}

Ditto, see comments above for the block.

> +
> +	return ret;

Ditto, see comments above.

> @@ -1285,6 +1326,19 @@ static int mt8195_mt6359_rt1019_rt5682_dev_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	priv->i2so1_mclk = devm_clk_get(&pdev->dev, "i2so1_mclk");
> +	if (IS_ERR(priv->i2so1_mclk)) {
> +		ret = PTR_ERR(priv->i2so1_mclk);
> +		if (ret == -ENOENT) {
> +			dev_dbg(&pdev->dev,
> +				"Failed to get i2so1_mclk, defer probe\n");
> +			return -EPROBE_DEFER;
> +		}
> +
> +		dev_err(&pdev->dev, "Failed to get i2so1_mclk, err:%d\n", ret);
> +		return ret;
> +	}

Ditto, see comments above for the block.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 1/2] ASoC: mediatek: mt8195: update control for RT5682 series

[Trevor Wu]

Hi Tzung-Bi,
Thanks for your reviewing,

On Wed, 2021-12-15 at 16:20 +0800, Tzung-Bi Shih wrote:
> On Wed, Dec 15, 2021 at 02:58:34PM +0800, Trevor Wu wrote:
> > --- a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c
> > +++ b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c
> 
> [...]
> > +static int mt8195_set_bias_level_post(struct snd_soc_card *card,
> > +	struct snd_soc_dapm_context *dapm, enum snd_soc_bias_level
> > level)
> > +{
> > +	struct snd_soc_component *component = dapm->component;
> > +	struct mt8195_mt6359_rt1011_rt5682_priv *priv =
> > +		snd_soc_card_get_drvdata(card);
> > +	int ret = 0;
> 
> ret doesn't need to be initialized.
> 
Originally, I initailize "ret", because it will be returned at the end
of the function. To be concise, I will return 0 directly and remove the
initialization.

> > +	/*
> > +	 * It's required to control mclk directly in the
> > set_bias_level_post
> > +	 * function for rt5682 and rt5682s codec, or the unexpected pop
> > happens
> > +	 * at the end of playback.
> > +	 */
> > +	if (!component ||
> > +	    (strcmp(component->name, RT5682_DEV0_NAME) &&
> > +	    strcmp(component->name, RT5682S_DEV0_NAME)))
> > +		return 0;
> > +
> > +	if (IS_ERR(priv->i2so1_mclk))
> > +		return 0;
> 
> I doubt if it needs to check priv->i2so1_mclk.  In other words, if
> IS_ERR(priv->i2so1_mclk) is true in _probe, does
> mt8195_set_bias_level_post() get called?
> 

Now, i2so1_mclk is a required property.
I will remove the condition in v2.

> > +	switch (level) {
> > +	case SND_SOC_BIAS_OFF:
> > +		if (!__clk_is_enabled(priv->i2so1_mclk))
> > +			return 0;
> > +
> > +		dev_dbg(card->dev, "Disable i2so1");
> > +		clk_disable_unprepare(priv->i2so1_mclk);
> 
> I would suggest move dev_dbg() later than clk_disable_unprepare()
> which means "Disable i2so1" is done.
> 
OK.

> > +		break;
> > +	case SND_SOC_BIAS_ON:
> > +		dev_dbg(card->dev, "Enable i2so1");
> > +		ret = clk_prepare_enable(priv->i2so1_mclk);
> > +		if (ret) {
> > +			dev_err(card->dev, "Can't enable mclk, err:
> > %d\n", ret);
> 
> The error message can be more specific.  "Cannot enable i2so1" for
> example.
> 
OK.

> > +			return ret;
> > +		}
> 
> Also, I would suggest move dev_dbg() later than
> clk_prepare_enable().  Otherwise, it could fail to prepare or enable
> but still can see "Enable i2so1" message.
> 
Yes, that's correct.
I will move it to a proper position.

> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return ret;
> 
> The function doesn't use any gotos.  To be concise, "return 0;".
OK.

> 
> > @@ -1072,6 +1119,19 @@ static int
> > mt8195_mt6359_rt1011_rt5682_dev_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > +	priv->i2so1_mclk = devm_clk_get(&pdev->dev, "i2so1_mclk");
> > +	if (IS_ERR(priv->i2so1_mclk)) {
> > +		ret = PTR_ERR(priv->i2so1_mclk);
> > +		if (ret == -ENOENT) {
> > +			dev_dbg(&pdev->dev,
> > +				"Failed to get i2so1_mclk, defer
> > probe\n");
> > +			return -EPROBE_DEFER;
> > +		}
> 
> Does devm_clk_get_optional() could make the block more concise?

Even though we use devm_clk_get_optional, we still have to handle the
(-ENOENT) case in probe function. In my opinion, original
implementation could be kept.

> 
> > +
> > +		dev_err(&pdev->dev, "Failed to get i2so1_mclk,
> > err:%d\n", ret);
> 
> If devm_clk_get() is possible to return -EPROBE_DEFER too, use
> dev_err_probe().
Ok.

> 
> > --- a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c
> > +++ b/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c
> 
> [...]
> > +static int mt8195_set_bias_level_post(struct snd_soc_card *card,
> > +	struct snd_soc_dapm_context *dapm, enum snd_soc_bias_level
> > level)
> > +{
> > +	struct snd_soc_component *component = dapm->component;
> > +	struct mt8195_mt6359_rt1019_rt5682_priv *priv =
> > +		snd_soc_card_get_drvdata(card);
> > +	int ret = 0;
> 
> Ditto, see comments above.
> 
OK.

> > +
> > +	/*
> > +	 * It's required to control mclk directly in the
> > set_bias_level_post
> > +	 * function for rt5682 and rt5682s codec, or the unexpected pop
> > happens
> > +	 * at the end of playback.
> > +	 */
> > +	if (!component ||
> > +	    (strcmp(component->name, RT5682_DEV0_NAME) &&
> > +	    strcmp(component->name, RT5682S_DEV0_NAME)))
> > +		return 0;
> > +
> > +	if (IS_ERR(priv->i2so1_mclk))
> > +		return 0;
> 
> Ditto, see comments above.
> 
OK.

> > +
> > +	switch (level) {
> > +	case SND_SOC_BIAS_OFF:
> > +		if (!__clk_is_enabled(priv->i2so1_mclk))
> > +			return 0;
> > +
> > +		dev_dbg(card->dev, "Disable i2so1");
> > +		clk_disable_unprepare(priv->i2so1_mclk);
> > +		break;
> > +	case SND_SOC_BIAS_ON:
> > +		dev_dbg(card->dev, "Enable i2so1");
> > +		ret = clk_prepare_enable(priv->i2so1_mclk);
> > +		if (ret) {
> > +			dev_err(card->dev, "Can't enable mclk, err:
> > %d\n", ret);
> > +			return ret;
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> 
> Ditto, see comments above for the block.
> 
OK.

> > +
> > +	return ret;
> 
> Ditto, see comments above.
> 
OK.

> > @@ -1285,6 +1326,19 @@ static int
> > mt8195_mt6359_rt1019_rt5682_dev_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > +	priv->i2so1_mclk = devm_clk_get(&pdev->dev, "i2so1_mclk");
> > +	if (IS_ERR(priv->i2so1_mclk)) {
> > +		ret = PTR_ERR(priv->i2so1_mclk);
> > +		if (ret == -ENOENT) {
> > +			dev_dbg(&pdev->dev,
> > +				"Failed to get i2so1_mclk, defer
> > probe\n");
> > +			return -EPROBE_DEFER;
> > +		}
> > +
> > +		dev_err(&pdev->dev, "Failed to get i2so1_mclk,
> > err:%d\n", ret);
> > +		return ret;
> > +	}
> 
> Ditto, see comments above for the block.
OK.



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 1/2] ASoC: mediatek: mt8195: update control for RT5682 series

[Tzung-Bi Shih]

On Thu, Dec 16, 2021 at 11:37:34AM +0800, Trevor Wu wrote:
> On Wed, 2021-12-15 at 16:20 +0800, Tzung-Bi Shih wrote:
> > On Wed, Dec 15, 2021 at 02:58:34PM +0800, Trevor Wu wrote:
> > > @@ -1072,6 +1119,19 @@ static int
> > > mt8195_mt6359_rt1011_rt5682_dev_probe(struct platform_device *pdev)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	priv->i2so1_mclk = devm_clk_get(&pdev->dev, "i2so1_mclk");
> > > +	if (IS_ERR(priv->i2so1_mclk)) {
> > > +		ret = PTR_ERR(priv->i2so1_mclk);
> > > +		if (ret == -ENOENT) {
> > > +			dev_dbg(&pdev->dev,
> > > +				"Failed to get i2so1_mclk, defer
> > > probe\n");
> > > +			return -EPROBE_DEFER;
> > > +		}
> > 
> > Does devm_clk_get_optional() could make the block more concise?
> 
> Even though we use devm_clk_get_optional, we still have to handle the
> (-ENOENT) case in probe function. In my opinion, original
> implementation could be kept.

I am neutral to my original suggestion but devm_clk_get_optional() returns NULL if -ENONENT.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] dt-bindings: mediatek: mt8195: add clock property to sound node

[Rob Herring]

On Wed, Dec 15, 2021 at 02:58:35PM +0800, Trevor Wu wrote:
> clocks and clock-names are added to provide MCLK phandle for sound card.
> 
> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> ---
>  .../bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml  | 12 ++++++++++++
>  .../bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml  | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml
> index cf6ad7933e23..b57c856d0cf3 100644
> --- a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml
> +++ b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml
> @@ -32,11 +32,21 @@ properties:
>      $ref: "/schemas/types.yaml#/definitions/phandle"
>      description: The phandle of MT8195 HDMI codec node.
>  
> +  clocks:
> +    items:
> +      - description: phandle and clock specifier for codec MCLK.
> +
> +  clock-names:
> +    items:
> +      - const: i2so1_mclk
> +
>  additionalProperties: false
>  
>  required:
>    - compatible
>    - mediatek,platform
> +  - clocks
> +  - clock-names
>  
>  examples:
>    - |
> @@ -44,6 +54,8 @@ examples:
>      sound: mt8195-sound {
>          compatible = "mediatek,mt8195_mt6359_rt1011_rt5682";
>          mediatek,platform = <&afe>;
> +        clocks = <&topckgen 235>; //CLK_TOP_APLL12_DIV2
> +        clock-names = "i2so1_mclk";
>          pinctrl-names = "default";
>          pinctrl-0 = <&aud_pins_default>;
>      };
> diff --git a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml
> index 8f177e02ad35..e4720f76f66b 100644
> --- a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml
> +++ b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml
> @@ -42,11 +42,21 @@ properties:
>        A list of the desired dai-links in the sound card. Each entry is a
>        name defined in the machine driver.
>  
> +  clocks:
> +    items:
> +      - description: phandle and clock specifier for codec MCLK.
> +
> +  clock-names:
> +    items:
> +      - const: i2so1_mclk
> +
>  additionalProperties: false
>  
>  required:
>    - compatible
>    - mediatek,platform
> +  - clocks
> +  - clock-names
>  
>  examples:
>    - |
> @@ -54,6 +64,8 @@ examples:
>      sound: mt8195-sound {
>          compatible = "mediatek,mt8195_mt6359_rt1019_rt5682";
>          mediatek,platform = <&afe>;
> +        clocks = <&topckgen 235>; //CLK_TOP_APLL12_DIV2
> +        clock-names = "i2so1_mclk";

Being a virtual node, how does it have clocks? This belongs in the h/w 
device that consumes the clock.

>          pinctrl-names = "default";
>          pinctrl-0 = <&aud_pins_default>;
>      };
> -- 
> 2.18.0
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] dt-bindings: mediatek: mt8195: add clock property to sound node

[Trevor Wu]

On Thu, 2021-12-16 at 13:06 -0600, Rob Herring wrote:
> On Wed, Dec 15, 2021 at 02:58:35PM +0800, Trevor Wu wrote:
> > clocks and clock-names are added to provide MCLK phandle for sound
> > card.
> > 
> > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> > ---
> >  .../bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml  | 12
> > ++++++++++++
> >  .../bindings/sound/mt8195-mt6359-rt1019-rt5682.yaml  | 12
> > ++++++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/mt8195-mt6359-
> > rt1011-rt5682.yaml
> > b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-
> > rt5682.yaml
> > index cf6ad7933e23..b57c856d0cf3 100644
> > --- a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-
> > rt5682.yaml
> > +++ b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-
> > rt5682.yaml
> > @@ -32,11 +32,21 @@ properties:
> >      $ref: "/schemas/types.yaml#/definitions/phandle"
> >      description: The phandle of MT8195 HDMI codec node.
> >  
> > +  clocks:
> > +    items:
> > +      - description: phandle and clock specifier for codec MCLK.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: i2so1_mclk
> > +
> >  additionalProperties: false
> >  
> >  required:
> >    - compatible
> >    - mediatek,platform
> > +  - clocks
> > +  - clock-names
> >  
> >  examples:
> >    - |
> > @@ -44,6 +54,8 @@ examples:
> >      sound: mt8195-sound {
> >          compatible = "mediatek,mt8195_mt6359_rt1011_rt5682";
> >          mediatek,platform = <&afe>;
> > +        clocks = <&topckgen 235>; //CLK_TOP_APLL12_DIV2
> > +        clock-names = "i2so1_mclk";
> >          pinctrl-names = "default";
> >          pinctrl-0 = <&aud_pins_default>;
> >      };
> > diff --git a/Documentation/devicetree/bindings/sound/mt8195-mt6359-
> > rt1019-rt5682.yaml
> > b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-
> > rt5682.yaml
> > index 8f177e02ad35..e4720f76f66b 100644
> > --- a/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-
> > rt5682.yaml
> > +++ b/Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1019-
> > rt5682.yaml
> > @@ -42,11 +42,21 @@ properties:
> >        A list of the desired dai-links in the sound card. Each
> > entry is a
> >        name defined in the machine driver.
> >  
> > +  clocks:
> > +    items:
> > +      - description: phandle and clock specifier for codec MCLK.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: i2so1_mclk
> > +
> >  additionalProperties: false
> >  
> >  required:
> >    - compatible
> >    - mediatek,platform
> > +  - clocks
> > +  - clock-names
> >  
> >  examples:
> >    - |
> > @@ -54,6 +64,8 @@ examples:
> >      sound: mt8195-sound {
> >          compatible = "mediatek,mt8195_mt6359_rt1019_rt5682";
> >          mediatek,platform = <&afe>;
> > +        clocks = <&topckgen 235>; //CLK_TOP_APLL12_DIV2
> > +        clock-names = "i2so1_mclk";
> 
> Being a virtual node, how does it have clocks? This belongs in the
> h/w device that consumes the clock.

Hi Rob,

I also found the same usages in some bindings from nvidia, like[1].

Based on my understanding, it seems not to be recommended now and 
clocks should only be used for a real hw node, is it correct?

If yes, I will try other way to get the clock I need in alsa machine
driver.

[1] 
https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5640.txt

Thanks,
Trevor

> >          pinctrl-0 = <&aud_pins_default>;
> >      };
> > -- 
> > 2.18.0
> > 
> > 


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek