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

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.