Re: [PATCH 1/3] drivers: thermal: tsens: Add critical interrupt support

[Bjorn Andersson]

On Thu 28 Nov 10:46 PST 2019, Amit Kucheria wrote:

> On Wed, Nov 13, 2019 at 1:08 AM Bjorn Andersson
> <bjorn.andersson@xxxxxxxxxx> wrote:
> >
> > On Mon 11 Nov 11:21 PST 2019, Amit Kucheria wrote:
> >
> > > TSENS IP v2.x adds critical threshold interrupt support for each sensor
> > > in addition to the upper/lower threshold interrupt. Add support in the
> > > driver.
> > >
> > > Signed-off-by: Amit Kucheria <amit.kucheria@xxxxxxxxxx>
> > > ---
> > >  drivers/thermal/qcom/tsens-common.c | 129 ++++++++++++++++++++++++++--
> > >  drivers/thermal/qcom/tsens-v2.c     |   8 +-
> > >  drivers/thermal/qcom/tsens.c        |  21 +++++
> > >  drivers/thermal/qcom/tsens.h        |  73 ++++++++++++++++
> > >  4 files changed, 220 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> > > index 4359a4247ac3..2989cb952cdb 100644
> > > --- a/drivers/thermal/qcom/tsens-common.c
> > > +++ b/drivers/thermal/qcom/tsens-common.c
> > > @@ -23,6 +23,10 @@
> > >   * @low_thresh:     lower threshold temperature value
> > >   * @low_irq_mask:   mask register for lower threshold irqs
> > >   * @low_irq_clear:  clear register for lower threshold irqs
> > > + * @crit_viol:      critical threshold violated
> >
> > "violated" as in "temperature is above crit_thresh"?
> 
> Yes.
> 
> >
> > > + * @crit_thresh:    critical threshold temperature value
> > > + * @crit_irq_mask:  mask register for critical threshold irqs
> > > + * @crit_irq_clear: clear register for critical threshold irqs
> > >   *
> > [..]
> > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > > index 7d317660211e..784c4976c4f9 100644
> > > --- a/drivers/thermal/qcom/tsens.c
> > > +++ b/drivers/thermal/qcom/tsens.c
> > > @@ -121,6 +121,27 @@ static int tsens_register(struct tsens_priv *priv)
> > >
> > >       enable_irq_wake(irq);
> > >
> > > +     if (tsens_version(priv) > VER_1_X) {
> > > +             irq = platform_get_irq_byname(pdev, "critical");
> > > +             if (irq < 0) {
> >
> > Treating this as a fatal error breaks backwards compatibility with
> > current devicetree; and even within your patch series, tsens should fail
> > to probe between this patch and the application of patch 3.
> 
> Good catch.
> 
> > Please flip this around and do:
> >
> > irq = platform_get_irq_byname(pdev, "critical");
> > if (irq >= 0 && tsens_version(priv) > VER_1_X) {
> >         request_irq()...
> > }
> 
> Won't this still break with current devicetree since irq < 0 until
> patch 3? Or are you saying we shouldn't check for
> platform_get_irq_byname() failure?
> 

I'm trying to say that dtsi without "critical" defined should cause the
driver to simply skip this segment, not fail to initialize.

> I can see two ways out:
> 1. We patch the dtsi before the code change.

You're expected to maintain backwards compatibility with existing dtb
files out there. The support for critical interrupt is an additional
feature, so you should be able to do this by detecting if "critical" is
defined (e.g. by checking the return value of
platform_get_irq_byname()).

> 2. We make critical interrupt failure non-fatal by just printing some
> messages and still returning success.
> 

Try to make it as specific as possible (without adding a bunch of code)
and throw in a dev_info() if no "critical" is found.

Regards,
Bjorn

> Regards,
> Amit
> 
> 
> > > +                     ret = irq;
> > > +                     goto err_put_device;
> > > +             }
> > > +
> > > +             ret = devm_request_threaded_irq(&pdev->dev, irq,
> > > +                                             NULL, tsens_critical_irq_thread,
> > > +                                             IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > > +                                             dev_name(&pdev->dev), priv);
> > > +             if (ret) {
> > > +                     dev_err(&pdev->dev, "%s: failed to get critical irq\n", __func__);
> > > +                     goto err_put_device;
> > > +             }
> > > +
> > > +             enable_irq_wake(irq);
> > > +     }
> > > +
> > > +     return 0;
> > > +
> > >  err_put_device:
> > >       put_device(&pdev->dev);
> > >       return ret;
> >
> > Regards,
> > Bjorn