linux-kernel - Re: [PATCH 3/3] ASoC: fsl-asoc-card: Support Headphone and Microphone Jack detection

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAA+D8ANQ_B9jJUhLYQnKxKJcVrmvakxPo58h433QqFhdu2nRPA@mail.gmail.com>
Date:   Wed, 15 Jul 2020 12:14:01 +0800
From:   Shengjiu Wang <shengjiu.wang@...il.com>
To:     Nicolin Chen <nicoleotsuka@...il.com>
Cc:     Shengjiu Wang <shengjiu.wang@....com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux-ALSA <alsa-devel@...a-project.org>,
        linuxppc-dev@...ts.ozlabs.org, Timur Tabi <timur@...nel.org>,
        kuninori.morimoto.gx@...esas.com, samuel@...lland.org,
        katsuhiro@...suster.net,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Xiubo Li <Xiubo.Lee@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Takashi Iwai <tiwai@...e.com>, Mark Brown <broonie@...nel.org>,
        Fabio Estevam <festevam@...il.com>
Subject: Re: [PATCH 3/3] ASoC: fsl-asoc-card: Support Headphone and Microphone
 Jack detection

On Wed, Jul 15, 2020 at 5:16 AM Nicolin Chen <nicoleotsuka@...il.com> wrote:
>
> Hi Shengjiu,
>
> The whole series looks good to me. Just a couple of small
> questions inline:
>
> On Tue, Jul 14, 2020 at 05:05:36PM +0800, Shengjiu Wang wrote:
> > Use asoc_simple_init_jack function from simple card to implement
> > the Headphone and Microphone detection.
> > Register notifier to disable Speaker when Headphone is plugged in
> > and enable Speaker when Headphone is unplugged.
> > Register notifier to disable Digital Microphone when Analog Microphone
> > is plugged in and enable DMIC when Analog Microphone is unplugged.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@....com>
> > ---
> >  sound/soc/fsl/Kconfig         |  1 +
> >  sound/soc/fsl/fsl-asoc-card.c | 69 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 68 insertions(+), 2 deletions(-)
>
> >  static int fsl_asoc_card_late_probe(struct snd_soc_card *card)
> >  {
> >       struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(card);
> > @@ -745,8 +789,29 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
> >       snd_soc_card_set_drvdata(&priv->card, priv);
> >
> >       ret = devm_snd_soc_register_card(&pdev->dev, &priv->card);
> > -     if (ret && ret != -EPROBE_DEFER)
> > -             dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
> > +     if (ret) {
> > +             if (ret != -EPROBE_DEFER)
> > +                     dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
>
> I think we may move this EPROBE_DEFER to the asrc_fail label.

If we move this to asrc_fail label, then it will be hard to define the
error message.
There are many places that goto asrc_fail.

>
> > +             goto asrc_fail;
> > +     }
> > +
> > +     if (of_property_read_bool(np, "hp-det-gpio")) {
>
> Could we move this check inside asoc_simple_init_jack? There's no
> problem with doing it here though, yet I got a bit confused by it
> as I thought it's a boolean type property, which would be against
> the DT bindings until I saw asoc_simple_init_jack() uses the same
> string to get the GPIO. Just it probably would be a bit tricky as
> we need it to be optional here.
>
> Otherwise, I think we may add a line of comments to indicate that
> the API would use the same string to get the GPIO.

In asoc_simple_init_jack, gpio_is_valid() will be invalid when there is
no "hp-det-gpio" property, and asoc_simple_init_jack will return 0.

The reason why I add a check here is mostly for
snd_soc_jack_notifier_register().
when there is no jack created, there will be a kernel dump.

or I can use this code:

-       if (of_property_read_bool(np, "hp-det-gpio")) {
-               ret = asoc_simple_init_jack(&priv->card, &priv->hp_jack,
-                                           1, NULL, "Headphone Jack");
-               if (ret)
-                       goto asrc_fail;
+       ret = asoc_simple_init_jack(&priv->card, &priv->hp_jack,
+                                   1, NULL, "Headphone Jack");
+       if (ret)
+               goto asrc_fail;

+       if (priv->hp_jack.jack.jack)
                snd_soc_jack_notifier_register(&priv->hp_jack.jack,
&hp_jack_nb);
-       }

what do you think?

best regards
wang shengjiu