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@xxxxxxxxxxxx>
> > Reviewed-by: Tzung-Bi Shih <tzungbi@xxxxxxxxxx>
> > ---
> > .../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
> >
> >
