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

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