| Message ID | 39d6b8e4b2cc5836839cfae7cdf0ee3470653b64.1576058136.git.amit.kucheria@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | drivers: thermal: tsens: Work with old DTBs | expand |
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?
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
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.
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
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; }
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(-)