| Message ID | 20220324064511.10665-1-jiaxin.yu@mediatek.com |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: mt8192: support rt1015p_rt5682s | expand |
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 > >
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 > > > >
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.
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
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.
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 > > > > > > >
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?
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
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
From: Jiaxin Yu <jiaxin.yu@mediatek.corp-partner.google.com> The series reuses mt8192-mt6359-rt10150rt5682.c for supporting machine driver with rt1015p speaker amplifier and rt5682s headset codec. Changes from v6: - "speaker-codec" changes to "speaker-codecs" due to there may be two speaker codec. Changes from v5: - "mediatek,headset-codec" and "mediatek,speaker-codec" drop prefix and move to properties from patternProperties. Changes form v4: - split a large patch into three small patches for easy reviewing - correct coding style Changes from v3: - fix build error: too many arguments for format [-Werror-format-extra-args] Changes from v2: - fix build warnings such as "data argument not used by format string" Changes from v1: - uses the snd_soc_of_get_dai_link_codecs to complete the configuration of dai_link's codecs - uses definitions to simplifies card name and compatible name Jiaxin Yu (4): ASoC: dt-bindings: mt8192-mt6359: add new compatible and new properties ASoC: mediatek: mt8192: refactor for I2S3 DAI link of speaker ASoC: mediatek: mt8192: refactor for I2S8/I2S9 DAI links of headset ASoC: mediatek: mt8192: support rt1015p_rt5682s .../sound/mt8192-mt6359-rt1015-rt5682.yaml | 32 +++ sound/soc/mediatek/Kconfig | 1 + .../mt8192/mt8192-mt6359-rt1015-rt5682.c | 201 +++++++++++------- 3 files changed, 156 insertions(+), 78 deletions(-)