diff mbox series

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

Message ID 20220324064511.10665-3-jiaxin.yu@mediatek.com (mailing list archive)
State Superseded
Headers show
Series ASoC: mediatek: mt8192: support rt1015p_rt5682s | expand

Commit Message

Jiaxin Yu March 24, 2022, 6:45 a.m. UTC 
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(-)

Comments

Nícolas F. R. A. Prado March 29, 2022, 10:30 p.m. UTC | #1 
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
> 
>
Jiaxin Yu March 30, 2022, 2:33 a.m. UTC | #2 
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
> > 
> >
Mark Brown March 30, 2022, 12:30 p.m. UTC | #3 
On Wed, Mar 30, 2022 at 10:33:06AM +0800, Jiaxin Yu wrote:

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

Making a previously optional property required means that systems that
previously worked may stop working unless they update their DT, DTs may
be distributed separately to the kernel and perhaps even baked into
firmware or similar.
Jiaxin Yu March 30, 2022, 2:06 p.m. UTC | #4 
On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:
> On Wed, Mar 30, 2022 at 10:33:06AM +0800, Jiaxin Yu wrote:
> 
> > "(Although I think this would technicallybreak the ABI?)"
> > ==> I can't understand this question, could you help explain it in
> > more
> > detail.
> 
> Making a previously optional property required means that systems
> that
> previously worked may stop working unless they update their DT, DTs
> may
> be distributed separately to the kernel and perhaps even baked into
> firmware or similar.

Hi Mark,

Thank you for your detailed answer. I should keep the driver's behavior
consistent with the description of dt-bindings. The "mediatek,hdmi-
codec" needs to be set as the required property. Is my understanding
right?

Thanks,
Jiaxin.Yu
Mark Brown March 30, 2022, 2:24 p.m. UTC | #5 
On Wed, Mar 30, 2022 at 10:06:24PM +0800, Jiaxin Yu wrote:
> On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:

> > Making a previously optional property required means that systems
> > that
> > previously worked may stop working unless they update their DT, DTs
> > may
> > be distributed separately to the kernel and perhaps even baked into
> > firmware or similar.

> Thank you for your detailed answer. I should keep the driver's behavior
> consistent with the description of dt-bindings. The "mediatek,hdmi-
> codec" needs to be set as the required property. Is my understanding
> right?

The binding document and code should match so if one is changed the
other needs to be changed too.

In theory we should never change a previously optional property to
required which would mean that the code should be updated to reflect the
binding document, however sometimes the DT isn't actually used as a
stable intereface by anything for a given property or device type so we
can get away with changing things.
Nícolas F. R. A. Prado March 30, 2022, 3:20 p.m. UTC | #6 
On Wed, Mar 30, 2022 at 10:33:06AM +0800, Jiaxin Yu wrote:
> 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.

But what do you mean that it is required in this machine driver? The code checks
for presence of hdmi_codec and ignores it if it's not set, so it does really
seem optional to me. Also, I have tested this driver on mt8192-asurada-spherion
without hdmi-codec set in the DT and the speaker and headphone sound works just
fine.

Besides, there might be machines using this driver that don't support HDMI, and
requiring an hdmi-codec in the DT for them would not make any sense. So keeping
hdmi-codec as optional seems like the most sensible solution to me, really.

Thanks,
Nícolas

> 
> 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
> > > 
> > > 
>
Jiaxin Yu March 30, 2022, 3:48 p.m. UTC | #7 
On Wed, 2022-03-30 at 15:24 +0100, Mark Brown wrote:
> On Wed, Mar 30, 2022 at 10:06:24PM +0800, Jiaxin Yu wrote:
> > On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:
> > > Making a previously optional property required means that systems
> > > that
> > > previously worked may stop working unless they update their DT,
> > > DTs
> > > may
> > > be distributed separately to the kernel and perhaps even baked
> > > into
> > > firmware or similar.
> > Thank you for your detailed answer. I should keep the driver's
> > behavior
> > consistent with the description of dt-bindings. The "mediatek,hdmi-
> > codec" needs to be set as the required property. Is my
> > understanding
> > right?
> 
> The binding document and code should match so if one is changed the
> other needs to be changed too.
> 
> In theory we should never change a previously optional property to
> required which would mean that the code should be updated to reflect
> the
> binding document, however sometimes the DT isn't actually used as a
> stable intereface by anything for a given property or device type so
> we
> can get away with changing things.

"however sometimes the DT isn't actually used as a stable intereface by
anything for a given property or device type so we can get away with
changing things."

Sorry, I don't understand the real idea of this description. Does it
mean that dt-bindings in this series don't need to be updated, but the
driver?
Nícolas F. R. A. Prado March 30, 2022, 4:14 p.m. UTC | #8 
On Wed, Mar 30, 2022 at 11:48:01PM +0800, Jiaxin Yu wrote:
> On Wed, 2022-03-30 at 15:24 +0100, Mark Brown wrote:
> > On Wed, Mar 30, 2022 at 10:06:24PM +0800, Jiaxin Yu wrote:
> > > On Wed, 2022-03-30 at 13:30 +0100, Mark Brown wrote:
> > > > Making a previously optional property required means that systems
> > > > that
> > > > previously worked may stop working unless they update their DT,
> > > > DTs
> > > > may
> > > > be distributed separately to the kernel and perhaps even baked
> > > > into
> > > > firmware or similar.
> > > Thank you for your detailed answer. I should keep the driver's
> > > behavior
> > > consistent with the description of dt-bindings. The "mediatek,hdmi-
> > > codec" needs to be set as the required property. Is my
> > > understanding
> > > right?
> > 
> > The binding document and code should match so if one is changed the
> > other needs to be changed too.
> > 
> > In theory we should never change a previously optional property to
> > required which would mean that the code should be updated to reflect
> > the
> > binding document, however sometimes the DT isn't actually used as a
> > stable intereface by anything for a given property or device type so
> > we
> > can get away with changing things.
> 
> "however sometimes the DT isn't actually used as a stable intereface by
> anything for a given property or device type so we can get away with
> changing things."
> 
> Sorry, I don't understand the real idea of this description. Does it
> mean that dt-bindings in this series don't need to be updated, but the
> driver?

He means that usually the DT (and dt-binding) shouldn't be changed to avoid
incompatibilities, but sometimes it's OK to change them. For example if there
are no users of the DT yet.

But in any case, like I mentioned in my latest reply [1], I don't think changing
the dt-binding is the proper solution in this case. The driver should be changed
instead.

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20220330152026.6nuigsldx46lue44@notapiano
Jiaxin Yu March 30, 2022, 4:20 p.m. UTC | #9 
On Wed, 2022-03-30 at 11:20 -0400, Nícolas F. R. A. Prado wrote:

> > > >  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.
> 
> But what do you mean that it is required in this machine driver? The
> code checks
> for presence of hdmi_codec and ignores it if it's not set, so it does
> really
> seem optional to me. Also, I have tested this driver on mt8192-
> asurada-spherion
> without hdmi-codec set in the DT and the speaker and headphone sound
> works just
> fine.
> 
> Besides, there might be machines using this driver that don't support
> HDMI, and
> requiring an hdmi-codec in the DT for them would not make any sense.
> So keeping
> hdmi-codec as optional seems like the most sensible solution to me,
> really.
> 
> Thanks,
> Nícolas

Yes, I agree with you. In the past, if there was a new board without
HDMI audio, we would choose to add a new machine driver and a new dt-
bindings. But now, in order to simplify the code, we tend to share one
machine driver for boards that use similar codecs. And we are doing
this now.

Thanks,
Jiaxin.Yu
diff mbox series

Patch

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