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

From: "Nícolas F. R. A. Prado" <nfraprado@collabora.com>
To: Jiaxin Yu <jiaxin.yu@mediatek.com>
Cc: broonie@kernel.org, robh+dt@kernel.org, tzungbi@google.com,
	angelogioacchino.delregno@collabora.com, aaronyu@google.com,
	matthias.bgg@gmail.com, trevor.wu@mediatek.com,
	linmq006@gmail.com, alsa-devel@alsa-project.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	Project_Global_Chrome_Upstream_Group@mediatek.com,
	Tzung-Bi Shih <tzungbi@kernel.org>
Subject: Re: [v7 2/4] ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker
Date: Tue, 29 Mar 2022 18:30:02 -0400	[thread overview]
Message-ID: <20220329223002.uo7kiemopkh7ak4x@notapiano> (raw)
In-Reply-To: <20220324064511.10665-3-jiaxin.yu@mediatek.com>

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
> 
> 