* [PATCH] drivers: thermal: tsens: Work with old DTBs [not found] <cover.1576058136.git.amit.kucheria@linaro.org> @ 2019-12-11 9:58 ` Amit Kucheria 2019-12-11 15:09 ` Daniel Lezcano 2019-12-11 17:13 ` Stephan Gerhold 0 siblings, 2 replies; 5+ messages in thread From: Amit Kucheria @ 2019-12-11 9:58 UTC (permalink / raw) To: linux-kernel, linux-arm-msm, bjorn.andersson, agross, swboyd, stephan, olof, Daniel Lezcano Cc: linux-pm 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: thermal: tsens: Work with old DTBs 2019-12-11 9:58 ` [PATCH] drivers: thermal: tsens: Work with old DTBs Amit Kucheria @ 2019-12-11 15:09 ` Daniel Lezcano 2019-12-12 9:50 ` Amit Kucheria 2019-12-11 17:13 ` Stephan Gerhold 1 sibling, 1 reply; 5+ messages in thread From: Daniel Lezcano @ 2019-12-11 15:09 UTC (permalink / raw) To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson, agross, swboyd, stephan, olof Cc: linux-pm 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: thermal: tsens: Work with old DTBs 2019-12-11 15:09 ` Daniel Lezcano @ 2019-12-12 9:50 ` Amit Kucheria 0 siblings, 0 replies; 5+ messages in thread From: Amit Kucheria @ 2019-12-12 9:50 UTC (permalink / raw) To: Daniel Lezcano Cc: LKML, linux-arm-msm, Bjorn Andersson, Andy Gross, Stephen Boyd, Stephan Gerhold, Olof Johansson, Linux PM list 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: thermal: tsens: Work with old DTBs 2019-12-11 9:58 ` [PATCH] drivers: thermal: tsens: Work with old DTBs Amit Kucheria 2019-12-11 15:09 ` Daniel Lezcano @ 2019-12-11 17:13 ` Stephan Gerhold 2019-12-12 9:47 ` Amit Kucheria 1 sibling, 1 reply; 5+ messages in thread From: Stephan Gerhold @ 2019-12-11 17:13 UTC (permalink / raw) To: Amit Kucheria Cc: linux-kernel, linux-arm-msm, bjorn.andersson, agross, swboyd, olof, Daniel Lezcano, linux-pm 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: thermal: tsens: Work with old DTBs 2019-12-11 17:13 ` Stephan Gerhold @ 2019-12-12 9:47 ` Amit Kucheria 0 siblings, 0 replies; 5+ messages in thread From: Amit Kucheria @ 2019-12-12 9:47 UTC (permalink / raw) To: Stephan Gerhold Cc: LKML, linux-arm-msm, Bjorn Andersson, Andy Gross, Stephen Boyd, Olof Johansson, Daniel Lezcano, Linux PM list 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-12 9:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1576058136.git.amit.kucheria@linaro.org>
2019-12-11 9:58 ` [PATCH] drivers: thermal: tsens: Work with old DTBs Amit Kucheria
2019-12-11 15:09 ` Daniel Lezcano
2019-12-12 9:50 ` Amit Kucheria
2019-12-11 17:13 ` Stephan Gerhold
2019-12-12 9:47 ` Amit Kucheria
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox