[PATCH 0/2] ASoC: mediatek: Remove redundant code and add sample rate checker of MT7986 SoC

[PATCH 0/2] ASoC: mediatek: Remove redundant code and add sample rate checker of MT7986 SoC

[Maso Huang]

1. Remove redundant remove function.
2. Add sample rate checker.
Patches are based on broonie tree "for-next" branch.

Maso Huang (2):
  ASoC: mediatek: mt7986: remove redundant remove function
  ASoC: mediatek: mt7986: add sample rate checker

 sound/soc/mediatek/mt7986/mt7986-dai-etdm.c | 22 +++++++++--
 sound/soc/mediatek/mt7986/mt7986-wm8960.c   | 43 ++++++---------------
 2 files changed, 30 insertions(+), 35 deletions(-)

-- 
2.18.0

[PATCH 1/2] ASoC: mediatek: mt7986: remove redundant remove function

[Maso Huang]

Remove redundant remove function of mt7986-wm8960.

Signed-off-by: Maso Huang <maso.huang@mediatek.com>
---
 sound/soc/mediatek/mt7986/mt7986-wm8960.c | 43 +++++++----------------
 1 file changed, 12 insertions(+), 31 deletions(-)

diff --git a/sound/soc/mediatek/mt7986/mt7986-wm8960.c b/sound/soc/mediatek/mt7986/mt7986-wm8960.c
index 364d13b1c426..c1390b373410 100644
--- a/sound/soc/mediatek/mt7986/mt7986-wm8960.c
+++ b/sound/soc/mediatek/mt7986/mt7986-wm8960.c
@@ -12,11 +12,6 @@
 
 #include "mt7986-afe-common.h"
 
-struct mt7986_wm8960_priv {
-	struct device_node *platform_node;
-	struct device_node *codec_node;
-};
-
 static const struct snd_soc_dapm_widget mt7986_wm8960_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone", NULL),
 	SND_SOC_DAPM_MIC("AMIC", NULL),
@@ -92,20 +87,18 @@ static int mt7986_wm8960_machine_probe(struct platform_device *pdev)
 	struct snd_soc_card *card = &mt7986_wm8960_card;
 	struct snd_soc_dai_link *dai_link;
 	struct device_node *platform, *codec;
-	struct mt7986_wm8960_priv *priv;
+	struct device_node *platform_dai_node, *codec_dai_node;
 	int ret, i;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
+	card->dev = &pdev->dev;
 
 	platform = of_get_child_by_name(pdev->dev.of_node, "platform");
 
 	if (platform) {
-		priv->platform_node = of_parse_phandle(platform, "sound-dai", 0);
+		platform_dai_node = of_parse_phandle(platform, "sound-dai", 0);
 		of_node_put(platform);
 
-		if (!priv->platform_node) {
+		if (!platform_dai_node) {
 			dev_err(&pdev->dev, "Failed to parse platform/sound-dai property\n");
 			return -EINVAL;
 		}
@@ -117,24 +110,22 @@ static int mt7986_wm8960_machine_probe(struct platform_device *pdev)
 	for_each_card_prelinks(card, i, dai_link) {
 		if (dai_link->platforms->name)
 			continue;
-		dai_link->platforms->of_node = priv->platform_node;
+		dai_link->platforms->of_node = platform_dai_node;
 	}
 
-	card->dev = &pdev->dev;
-
 	codec = of_get_child_by_name(pdev->dev.of_node, "codec");
 
 	if (codec) {
-		priv->codec_node = of_parse_phandle(codec, "sound-dai", 0);
+		codec_dai_node = of_parse_phandle(codec, "sound-dai", 0);
 		of_node_put(codec);
 
-		if (!priv->codec_node) {
-			of_node_put(priv->platform_node);
+		if (!codec_dai_node) {
+			of_node_put(platform_dai_node);
 			dev_err(&pdev->dev, "Failed to parse codec/sound-dai property\n");
 			return -EINVAL;
 		}
 	} else {
-		of_node_put(priv->platform_node);
+		of_node_put(platform_dai_node);
 		dev_err(&pdev->dev, "Property 'codec' missing or invalid\n");
 		return -EINVAL;
 	}
@@ -142,7 +133,7 @@ static int mt7986_wm8960_machine_probe(struct platform_device *pdev)
 	for_each_card_prelinks(card, i, dai_link) {
 		if (dai_link->codecs->name)
 			continue;
-		dai_link->codecs->of_node = priv->codec_node;
+		dai_link->codecs->of_node = codec_dai_node;
 	}
 
 	ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
@@ -158,20 +149,11 @@ static int mt7986_wm8960_machine_probe(struct platform_device *pdev)
 	}
 
 err_of_node_put:
-	of_node_put(priv->codec_node);
-	of_node_put(priv->platform_node);
+	of_node_put(platform_dai_node);
+	of_node_put(codec_dai_node);
 	return ret;
 }
 
-static void mt7986_wm8960_machine_remove(struct platform_device *pdev)
-{
-	struct snd_soc_card *card = platform_get_drvdata(pdev);
-	struct mt7986_wm8960_priv *priv = snd_soc_card_get_drvdata(card);
-
-	of_node_put(priv->codec_node);
-	of_node_put(priv->platform_node);
-}
-
 static const struct of_device_id mt7986_wm8960_machine_dt_match[] = {
 	{.compatible = "mediatek,mt7986-wm8960-sound"},
 	{ /* sentinel */ }
@@ -184,7 +166,6 @@ static struct platform_driver mt7986_wm8960_machine = {
 		.of_match_table = mt7986_wm8960_machine_dt_match,
 	},
 	.probe = mt7986_wm8960_machine_probe,
-	.remove_new = mt7986_wm8960_machine_remove,
 };
 
 module_platform_driver(mt7986_wm8960_machine);
-- 
2.18.0

Re: [PATCH 1/2] ASoC: mediatek: mt7986: remove redundant remove function

[AngeloGioacchino Del Regno]

Il 23/10/23 11:54, Maso Huang ha scritto:
> Remove redundant remove function of mt7986-wm8960.
> 
> Signed-off-by: Maso Huang <maso.huang@mediatek.com>

You're doing more than what you're advertising; you are:

1. Dropping the remove callback; and
2. Removing the mt7986_wm8960_priv structure

You can do that in one single commit if you really want to, but you have to
use the appropriate commit title and description, saying exactly what you're doing.

Regards,
Angelo

Re: [PATCH 1/2] ASoC: mediatek: mt7986: remove redundant remove function

[Maso Huang (黃加竹)]

On Mon, 2023-10-23 at 13:07 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 11:54, Maso Huang ha scritto:
> > Remove redundant remove function of mt7986-wm8960.
> > 
> > Signed-off-by: Maso Huang <maso.huang@mediatek.com>
> 
> You're doing more than what you're advertising; you are:
> 
> 1. Dropping the remove callback; and
> 2. Removing the mt7986_wm8960_priv structure
> 
> You can do that in one single commit if you really want to, but you
> have to
> use the appropriate commit title and description, saying exactly what
> you're doing.
> 
> Regards,
> Angelo
> 

Hi Angelo,

Thanks for your review!
I'll devide into two with appropriate title and description in v2.

Best regards,
Maso

[PATCH 2/2] ASoC: mediatek: mt7986: add sample rate checker

[Maso Huang]

mt7986 only supports 8/12/16/24/32/48/96/192 kHz

Signed-off-by: Maso Huang <maso.huang@mediatek.com>
---
 sound/soc/mediatek/mt7986/mt7986-dai-etdm.c | 22 +++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c b/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
index e523d33846fe..270852ce3dd9 100644
--- a/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
+++ b/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
@@ -237,12 +237,26 @@ static int mtk_dai_etdm_hw_params(struct snd_pcm_substream *substream,
 				  struct snd_pcm_hw_params *params,
 				  struct snd_soc_dai *dai)
 {
+	unsigned int rate = params_rate(params);
 	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
 
-	mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_PLAYBACK);
-	mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_CAPTURE);
-
-	return 0;
+	switch (rate) {
+	case 8000:
+	case 12000:
+	case 16000:
+	case 24000:
+	case 32000:
+	case 48000:
+	case 96000:
+	case 192000:
+		mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_PLAYBACK);
+		mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_CAPTURE);
+		return 0;
+	default:
+		dev_warn(afe->dev, "%s(), sample rate: %d is not supported\n", __func__, rate);
+		dev_warn(afe->dev, "%s(), only support 8/12/16/24/32/48/96/192 kHz\n", __func__);
+		return -EINVAL;
+	}
 }
 
 static int mtk_dai_etdm_trigger(struct snd_pcm_substream *substream, int cmd,
-- 
2.18.0

Re: [PATCH 2/2] ASoC: mediatek: mt7986: add sample rate checker

[AngeloGioacchino Del Regno]

Il 23/10/23 11:54, Maso Huang ha scritto:
> mt7986 only supports 8/12/16/24/32/48/96/192 kHz
> 
> Signed-off-by: Maso Huang <maso.huang@mediatek.com>
> ---
>   sound/soc/mediatek/mt7986/mt7986-dai-etdm.c | 22 +++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c b/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
> index e523d33846fe..270852ce3dd9 100644
> --- a/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
> +++ b/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
> @@ -237,12 +237,26 @@ static int mtk_dai_etdm_hw_params(struct snd_pcm_substream *substream,
>   				  struct snd_pcm_hw_params *params,
>   				  struct snd_soc_dai *dai)
>   {
> +	unsigned int rate = params_rate(params);
>   	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
>   
> -	mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_PLAYBACK);
> -	mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_CAPTURE);
> -
> -	return 0;
> +	switch (rate) {
> +	case 8000:
> +	case 12000:
> +	case 16000:
> +	case 24000:
> +	case 32000:
> +	case 48000:
> +	case 96000:
> +	case 192000:
> +		mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_PLAYBACK);
> +		mtk_dai_etdm_config(afe, params, dai, SNDRV_PCM_STREAM_CAPTURE);
> +		return 0;
> +	default:
> +		dev_warn(afe->dev, "%s(), sample rate: %d is not supported\n", __func__, rate);
> +		dev_warn(afe->dev, "%s(), only support 8/12/16/24/32/48/96/192 kHz\n", __func__);

That's better:

dev_err(afe->dev,
	"Sample rate %d invalid. Supported rates: 8/12/16/24/32/48/96/192 kHz\n",
	rate, __func__);

after which,

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

Regards,
Angelo

Re: [PATCH 2/2] ASoC: mediatek: mt7986: add sample rate checker

[Maso Huang (黃加竹)]

On Mon, 2023-10-23 at 13:07 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 11:54, Maso Huang ha scritto:
> > mt7986 only supports 8/12/16/24/32/48/96/192 kHz
> > 
> > Signed-off-by: Maso Huang <maso.huang@mediatek.com>
> > ---
> >   sound/soc/mediatek/mt7986/mt7986-dai-etdm.c | 22
> > +++++++++++++++++----
> >   1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
> > b/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
> > index e523d33846fe..270852ce3dd9 100644
> > --- a/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
> > +++ b/sound/soc/mediatek/mt7986/mt7986-dai-etdm.c
> > @@ -237,12 +237,26 @@ static int mtk_dai_etdm_hw_params(struct
> > snd_pcm_substream *substream,
> >   				  struct snd_pcm_hw_params *params,
> >   				  struct snd_soc_dai *dai)
> >   {
> > +	unsigned int rate = params_rate(params);
> >   	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> >   
> > -	mtk_dai_etdm_config(afe, params, dai,
> > SNDRV_PCM_STREAM_PLAYBACK);
> > -	mtk_dai_etdm_config(afe, params, dai,
> > SNDRV_PCM_STREAM_CAPTURE);
> > -
> > -	return 0;
> > +	switch (rate) {
> > +	case 8000:
> > +	case 12000:
> > +	case 16000:
> > +	case 24000:
> > +	case 32000:
> > +	case 48000:
> > +	case 96000:
> > +	case 192000:
> > +		mtk_dai_etdm_config(afe, params, dai,
> > SNDRV_PCM_STREAM_PLAYBACK);
> > +		mtk_dai_etdm_config(afe, params, dai,
> > SNDRV_PCM_STREAM_CAPTURE);
> > +		return 0;
> > +	default:
> > +		dev_warn(afe->dev, "%s(), sample rate: %d is not
> > supported\n", __func__, rate);
> > +		dev_warn(afe->dev, "%s(), only support
> > 8/12/16/24/32/48/96/192 kHz\n", __func__);
> 
> That's better:
> 
> dev_err(afe->dev,
> 	"Sample rate %d invalid. Supported rates:
> 8/12/16/24/32/48/96/192 kHz\n",
> 	rate, __func__);
> 
> after which,
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> 
> Regards,
> Angelo
> 
> 

Hi Angelo,

Thanks for your review!
I'll refine it in v2.

Best regards,
Maso