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

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.