| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 30 Mar 2022 11:20:26 -0400
From: Nícolas F. R. A. Prado
<nfraprado@...labora.com>
To: Jiaxin Yu <jiaxin.yu@...iatek.com>
Cc: broonie@...nel.org, robh+dt@...nel.org, tzungbi@...gle.com,
angelogioacchino.delregno@...labora.com, aaronyu@...gle.com,
matthias.bgg@...il.com, trevor.wu@...iatek.com, linmq006@...il.com,
alsa-devel@...a-project.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
Project_Global_Chrome_Upstream_Group@...iatek.com,
Tzung-Bi Shih <tzungbi@...nel.org>
Subject: 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@...iatek.com>
> > > Reviewed-by: Tzung-Bi Shih <tzungbi@...nel.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
> > >
> > >
>
Powered by blists - more mailing lists