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

[Jiaxin Yu <jiaxin.yu@mediatek.com>]

On Tue, 2022-03-29 at 18:30 -0400, Nícolas F. R. A. Prado wrote:
> 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

Hi Nícolas,

Thanks for your comment. Indeed I made hdmi-codec a required property,
because it is a must in this machine driver. I prefer to report errors
during the registration rather than during the use.

So I'd like to take your second suggestion. I need to update dt-binding 
that set hdmi-codec as required property.

"(Although I think this would technicallybreak the ABI?)"
==> I can't understand this question, could you help explain it in more
detail.

Thanks,
Jiaxin.Yu

> 
>  	}
> > -	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;
> > +	}
> >  
> 
snip...
> >  
> > -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
> > 
> >