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

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