Re: [PATCH v4] thermal/drivers/tsens: Add suspend to RAM support for tsens

[Priyansh Jain]

On 3/26/2024 4:30 PM, Daniel Lezcano wrote:
> On 26/03/2024 08:40, Priyansh Jain wrote:
> > As part of suspend to RAM, tsens hardware will be turned off.
> > While resume callback, re-initialize tsens hardware.
> >
> > Signed-off-by: Priyansh Jain <quic_priyjain@xxxxxxxxxxx>
> > ---
> > V3 -> V4: Make tsens_reinit function specific to tsens v2. Add
> > NULL resume callback support for platform whose versions < ver_2_x
> > in tsens ops.
> > V2 -> V3: Remove suspend callback & interrupt enablement part from
> > resume callback.
> > V1 -> V2: Update commit text to explain the necessity of this patch
> >
> >   drivers/thermal/qcom/tsens-v0_1.c |  6 +++++
> >   drivers/thermal/qcom/tsens-v1.c   |  3 +++
> >   drivers/thermal/qcom/tsens-v2.c   |  1 +
> >   drivers/thermal/qcom/tsens.c      | 37 +++++++++++++++++++++++++++++++
> >   drivers/thermal/qcom/tsens.h      |  5 +++++
> >   5 files changed, 52 insertions(+)
> >
> > diff --git a/drivers/thermal/qcom/tsens-v0_1.c 
> > b/drivers/thermal/qcom/tsens-v0_1.c
> > index 32d2d3e33287..7ed85379247b 100644
> > --- a/drivers/thermal/qcom/tsens-v0_1.c
> > +++ b/drivers/thermal/qcom/tsens-v0_1.c
> > @@ -329,6 +329,7 @@ static const struct tsens_ops ops_8226 = {
> >       .init        = init_8226,
> >       .calibrate    = tsens_calibrate_common,
> >       .get_temp    = get_temp_common,
> > +    .resume        = NULL,
>
> As a static variable it is already set to NULL. Why do you need to 
> explicitly set them everywhere ?
It was asked in last version to explicitly add (.resume = NULL). So 
added this for all the tsens platforms for which resume callback is not
validated.

> [ ... ]
>
> > +#ifdef CONFIG_SUSPEND
> > +static int tsens_reinit(struct tsens_priv *priv)
> > +{
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&priv->ul_lock, flags);
>
> What this lock is protecting ?
Yes this lock can be removed, will remove this in next patch.

> > +    if (tsens_version(priv) >= VER_2_X) {
>
> May be move this test before the lock ?
>
> > +        /*
> > +         * Re-enable the watchdog, unmask the bark.
> > +         * Disable cycle completion monitoring
> > +         */
> > +        if (priv->feat->has_watchdog) {
> > +            regmap_field_write(priv->rf[WDOG_BARK_MASK], 0);
> > +            regmap_field_write(priv->rf[CC_MON_MASK], 1);
> > +        }
> > +
> > +        /* Re-enable interrupts */
> > +        tsens_enable_irq(priv);
> > +    }
> > +
> > +    spin_unlock_irqrestore(&priv->ul_lock, flags);
> > +
> > +    return 0;
> > +}
> > +
> > +int tsens_resume_common(struct tsens_priv *priv)
> > +{
> > +    if (pm_suspend_target_state == PM_SUSPEND_MEM)
> > +        tsens_reinit(priv);
> > +
> > +    return 0;
> > +}
> > +
> > +#endif /* !CONFIG_SUSPEND */