2019-12-11 09:59:43

by Amit Kucheria

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

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 <[email protected]>
---
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

2019-12-11 16:13:13

by Daniel Lezcano

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

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 <[email protected]>
> ---
> 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

2019-12-11 17:15:41

by Stephan Gerhold

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

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 <[email protected]>
> ---
> 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

2019-12-12 09:48:36

by Amit Kucheria

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

On Wed, Dec 11, 2019 at 10:43 PM Stephan Gerhold <[email protected]> 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 <[email protected]>
> > ---
> > 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.

2019-12-12 09:53:11

by Amit Kucheria

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

On Wed, Dec 11, 2019 at 9:42 PM Daniel Lezcano
<[email protected]> 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 <[email protected]>
> > ---
> > 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