On Thu, Feb 20, 2020 at 5:35 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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:
