Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <cover.1582048155.git.amit.kucheria@linaro.org>
 <4f5a4175371ac7973061cd4f9d19674ac308672c.1582048155.git.amit.kucheria@linaro.org>
 <158215713699.184098.4863049384855658604@swboyd.mtv.corp.google.com>
In-Reply-To: <158215713699.184098.4863049384855658604@swboyd.mtv.corp.google.com>
From:   Amit Kucheria <amit.kucheria@linaro.org>
Date:   Mon, 24 Feb 2020 18:14:02 +0530
Message-ID: <CAP245DUfcfcjMKixpJ9jkvN76nnAKdfBiDcNhSH6rSy0cUY=Bw@mail.gmail.com>
Subject: Re: [PATCH v5 5/8] drivers: thermal: tsens: Add critical interrupt support
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        sivaa@codeaurora.org, Amit Kucheria <amit.kucheria@verdurent.com>,
        Linux PM list <linux-pm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Feb 20, 2020 at 5:35 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Amit Kucheria (2020-02-18 10:12:09)
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 0e7cf5236932..5b003d598234 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -125,6 +125,28 @@ static int tsens_register(struct tsens_priv *priv)
> >                 goto err_put_device;
> >         }
> >
> > +       if (priv->feat->crit_int) {
> > +               irq_crit = platform_get_irq_byname(pdev, "critical");
> > +               if (irq_crit < 0) {
> > +                       ret = irq_crit;
> > +                       /* For old DTs with no IRQ defined */
> > +                       if (irq_crit == -ENXIO)
> > +                               ret = 0;
> > +                       goto err_crit_int;
> > +               }
> > +               ret = devm_request_threaded_irq(&pdev->dev, irq_crit,
> > +                                               NULL, tsens_critical_irq_thread,
> > +                                               IRQF_ONESHOT,
> > +                                               dev_name(&pdev->dev), priv);
> > +               if (ret) {
> > +                       dev_err(&pdev->dev, "%s: failed to get critical irq\n", __func__);
> > +                       goto err_crit_int;
> > +               }
> > +
> > +               enable_irq_wake(irq_crit);
> > +       }
> > +
> > +err_crit_int:
>
> Why use a goto? Can't this be done with if-else statements?
>
>        if (priv->feat->crit_int) {
>                irq_crit = platform_get_irq_byname(pdev, "critical");
>                if (irq_crit < 0) {
>                        ...
>                } else {
>                        ret = devm_request_threaded_irq(&pdev->dev, irq_crit,
>                                                        NULL, tsens_critical_irq_thread,
>                                                        IRQF_ONESHOT,
>                                                        dev_name(&pdev->dev), priv);
>                        if (ret)
>                                dev_err(&pdev->dev, "%s: failed to get critical irq\n", __func__);
>                        else
>                                enable_irq_wake(irq_crit);
>                }
>        }
>
> Or if the nesting is so deep that we need goto labels then perhaps it
> needs to be another function.

So the if-else form only slightly improved the readability. But moving
it to a function made it much better, IMO. So I went with that.

Thanks for the review.

Regards,
Amit

> >         enable_irq_wake(irq);
> >
> >  err_put_device: