[PATCH] drivers: thermal: tsens: Work with old DTBs

[PATCH] drivers: thermal: tsens: Work with old DTBs

[Amit Kucheria]

In order for the old DTBs to continue working, the new interrupt code
must not return an error if interrupts are not defined.

Fixes: 634e11d5b450a ("drivers: thermal: tsens: Add interrupt support")
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 015e7d2015985..d8f51067ed411 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -109,7 +109,7 @@ static int tsens_register(struct tsens_priv *priv)
 
 	irq = platform_get_irq_byname(pdev, "uplow");
 	if (irq < 0) {
-		ret = irq;
+		dev_warn(&pdev->dev, "Missing uplow irq in DT\n");
 		goto err_put_device;
 	}
 
@@ -118,7 +118,8 @@ static int tsens_register(struct tsens_priv *priv)
 					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
 					dev_name(&pdev->dev), priv);
 	if (ret) {
-		dev_err(&pdev->dev, "%s: failed to get irq\n", __func__);
+		dev_warn(&pdev->dev, "%s: failed to get uplow irq\n", __func__);
+		ret = 0;
 		goto err_put_device;
 	}
 
-- 
2.20.1

Re: [PATCH] drivers: thermal: tsens: Work with old DTBs

[Daniel Lezcano]

On 11/12/2019 10:58, Amit Kucheria wrote:
> In order for the old DTBs to continue working, the new interrupt code
> must not return an error if interrupts are not defined.
> 
> Fixes: 634e11d5b450a ("drivers: thermal: tsens: Add interrupt support")
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/qcom/tsens.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 015e7d2015985..d8f51067ed411 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -109,7 +109,7 @@ static int tsens_register(struct tsens_priv *priv)
>  
>  	irq = platform_get_irq_byname(pdev, "uplow");
>  	if (irq < 0) {
> -		ret = irq;

'ret' remains uninitialized here.

> +		dev_warn(&pdev->dev, "Missing uplow irq in DT\n");
>  		goto err_put_device;
>  	}
>  
> @@ -118,7 +118,8 @@ static int tsens_register(struct tsens_priv *priv)
>  					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>  					dev_name(&pdev->dev), priv);
>  	if (ret) {
> -		dev_err(&pdev->dev, "%s: failed to get irq\n", __func__);
> +		dev_warn(&pdev->dev, "%s: failed to get uplow irq\n", __func__);
> +		ret = 0;
>  		goto err_put_device;
>  	}

The code now is unable to make a distinction between an error in the DT
and the old DT :/

Why not version the DT?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] drivers: thermal: tsens: Work with old DTBs

[Amit Kucheria]

On Wed, Dec 11, 2019 at 9:42 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 11/12/2019 10:58, Amit Kucheria wrote:
> > In order for the old DTBs to continue working, the new interrupt code
> > must not return an error if interrupts are not defined.
> >
> > Fixes: 634e11d5b450a ("drivers: thermal: tsens: Add interrupt support")
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  drivers/thermal/qcom/tsens.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 015e7d2015985..d8f51067ed411 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -109,7 +109,7 @@ static int tsens_register(struct tsens_priv *priv)
> >
> >       irq = platform_get_irq_byname(pdev, "uplow");
> >       if (irq < 0) {
> > -             ret = irq;
>
> 'ret' remains uninitialized here.
>
> > +             dev_warn(&pdev->dev, "Missing uplow irq in DT\n");
> >               goto err_put_device;
> >       }
> >
> > @@ -118,7 +118,8 @@ static int tsens_register(struct tsens_priv *priv)
> >                                       IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> >                                       dev_name(&pdev->dev), priv);
> >       if (ret) {
> > -             dev_err(&pdev->dev, "%s: failed to get irq\n", __func__);
> > +             dev_warn(&pdev->dev, "%s: failed to get uplow irq\n", __func__);
> > +             ret = 0;
> >               goto err_put_device;
> >       }
>
> The code now is unable to make a distinction between an error in the DT
> and the old DT :/
>
> Why not version the DT?

Versioning the DT is probably overkill for this driver. Just checking
for ENXIO as suggested by Stephan seems to be enough. We don't lose
the error checking for devm_request_threaded_irq either.

Regards,
Amit

Re: [PATCH] drivers: thermal: tsens: Work with old DTBs

[Stephan Gerhold]

Hi Amit,

Thanks for the patch!

On Wed, Dec 11, 2019 at 03:28:33PM +0530, Amit Kucheria wrote:
> In order for the old DTBs to continue working, the new interrupt code
> must not return an error if interrupts are not defined.
> 
> Fixes: 634e11d5b450a ("drivers: thermal: tsens: Add interrupt support")
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/qcom/tsens.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 015e7d2015985..d8f51067ed411 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -109,7 +109,7 @@ static int tsens_register(struct tsens_priv *priv)
>  
>  	irq = platform_get_irq_byname(pdev, "uplow");
>  	if (irq < 0) {
> -		ret = irq;
> +		dev_warn(&pdev->dev, "Missing uplow irq in DT\n");
>  		goto err_put_device;
>  	}

platform_get_irq_byname() already logs an error if the IRQ cannot be
found: qcom-tsens 4a9000.thermal-sensor: IRQ uplow not found

To replace that error with a warning (not sure if that is worth it),
we would need to replace the call with platform_get_irq_byname_optional().

>  
> @@ -118,7 +118,8 @@ static int tsens_register(struct tsens_priv *priv)
>  					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>  					dev_name(&pdev->dev), priv);
>  	if (ret) {
> -		dev_err(&pdev->dev, "%s: failed to get irq\n", __func__);
> +		dev_warn(&pdev->dev, "%s: failed to get uplow irq\n", __func__);
> +		ret = 0;
>  		goto err_put_device;

In case of the old DT, platform_get_irq_byname() will return -ENXIO,
because no interrupt is specified in the device tree.
So we should have already run into the error earlier,
and jumped to "err_put_device".

Is this hunk really necessary?

In other words, wouldn't it be enough to do something like

@@ -110,6 +110,8 @@ static int tsens_register(struct tsens_priv *priv)
 	irq = platform_get_irq_byname(pdev, "uplow");
 	if (irq < 0) {
 		ret = irq;
+		if (ret == -ENXIO)
+			ret = 0;
 		goto err_put_device;
 	}

... to essentially ignore only the "IRQ does not exist" condition
for old device trees?

Thanks,
Stephan

Re: [PATCH] drivers: thermal: tsens: Work with old DTBs

[Amit Kucheria]

On Wed, Dec 11, 2019 at 10:43 PM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> Hi Amit,
>
> Thanks for the patch!
>
> On Wed, Dec 11, 2019 at 03:28:33PM +0530, Amit Kucheria wrote:
> > In order for the old DTBs to continue working, the new interrupt code
> > must not return an error if interrupts are not defined.
> >
> > Fixes: 634e11d5b450a ("drivers: thermal: tsens: Add interrupt support")
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  drivers/thermal/qcom/tsens.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 015e7d2015985..d8f51067ed411 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -109,7 +109,7 @@ static int tsens_register(struct tsens_priv *priv)
> >
> >       irq = platform_get_irq_byname(pdev, "uplow");
> >       if (irq < 0) {
> > -             ret = irq;
> > +             dev_warn(&pdev->dev, "Missing uplow irq in DT\n");
> >               goto err_put_device;
> >       }
>
> platform_get_irq_byname() already logs an error if the IRQ cannot be
> found: qcom-tsens 4a9000.thermal-sensor: IRQ uplow not found
>
> To replace that error with a warning (not sure if that is worth it),
> we would need to replace the call with platform_get_irq_byname_optional().

I didn't know about optional, interesting.

> >
> > @@ -118,7 +118,8 @@ static int tsens_register(struct tsens_priv *priv)
> >                                       IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> >                                       dev_name(&pdev->dev), priv);
> >       if (ret) {
> > -             dev_err(&pdev->dev, "%s: failed to get irq\n", __func__);
> > +             dev_warn(&pdev->dev, "%s: failed to get uplow irq\n", __func__);
> > +             ret = 0;
> >               goto err_put_device;
>
> In case of the old DT, platform_get_irq_byname() will return -ENXIO,
> because no interrupt is specified in the device tree.
> So we should have already run into the error earlier,
> and jumped to "err_put_device".
>
> Is this hunk really necessary?

You're right. Just checking for ENXIO should be enough for old DTs.

> In other words, wouldn't it be enough to do something like
>
> @@ -110,6 +110,8 @@ static int tsens_register(struct tsens_priv *priv)
>         irq = platform_get_irq_byname(pdev, "uplow");
>         if (irq < 0) {
>                 ret = irq;
> +               if (ret == -ENXIO)
> +                       ret = 0;
>                 goto err_put_device;
>         }
>
> ... to essentially ignore only the "IRQ does not exist" condition
> for old device trees?

Thanks for the review.