Re: [PATCH 1/2] thermal: qcom: tsens: atomic temperature read with hardware-guided retries

[Daniel Lezcano]

On 5/5/26 08:11, Priyansh Jain wrote:

[ ... ]

> > > +    .valid_bit = BIT(14),
> > > +    .last_temp_mask = 0x3FF,
> >
> > This is GENMASK(9, 0)
> >
> > > +    .last_temp_resolution = 9,
> >
> > Please comply with the SSOT, in the init function compute the mask with:
> >
> >      ->last_temp_mask = GENMASK(9, 0);
> >
> > and remove the initialization here
> Thanks for pointing this out — yes, this approach looks better.
> If I understand correctly, you’re suggesting that the mask should simply 
> be defined in the init function as follows:
> priv->feat->last_temp_mask = GENMASK(priv->feat->last_temp_resolution, 0);
> ?

Yes, that's correct


> > >   };
> > >   static struct tsens_features ipq8074_feat = {
> > > @@ -125,8 +128,7 @@ static const struct reg_field 
> > > tsens_v2_regfields[MAX_REGFIELDS] = {
> > >       [WDOG_BARK_COUNT]  = REG_FIELD(TM_WDOG_LOG_OFF,             0,  
> > > 7),
> > >       /* Sn_STATUS */
> > > -    REG_FIELD_FOR_EACH_SENSOR16(LAST_TEMP,       TM_Sn_STATUS_OFF, 
> > > 0,  11),
> > > -    REG_FIELD_FOR_EACH_SENSOR16(VALID,           TM_Sn_STATUS_OFF, 
> > > 21,  21),
> > > +    REG_FIELD_FOR_EACH_SENSOR16(LAST_TEMP,       TM_Sn_STATUS_OFF, 
> > > 0,  21),
> > >       /* xxx_STATUS bits: 1 == threshold violated */
> > >       REG_FIELD_FOR_EACH_SENSOR16(MIN_STATUS,      TM_Sn_STATUS_OFF, 
> > > 16,  16),
> > >       REG_FIELD_FOR_EACH_SENSOR16(LOWER_STATUS,    TM_Sn_STATUS_OFF, 
> > > 17,  17),
> > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > > index a2422ebee816..15392a17ef41 100644
> > > --- a/drivers/thermal/qcom/tsens.c
> > > +++ b/drivers/thermal/qcom/tsens.c
> > > @@ -315,10 +315,66 @@ static inline int code_to_degc(u32 adc_code, 
> > > const struct tsens_sensor *s)
> > >       return degc;
> > >   }
> > > +static inline enum tsens_ver tsens_version(struct tsens_priv *priv)
> > > +{
> > > +    return priv->feat->ver_major;
> > > +}
> >
> > I agree putting accessor functions is a good practice but here as it 
> > results in duplicating the function, the benefit is discutable.
> I did not introduce this new function; it was already present and I only 
> moved it from the bottom of the file to the top since it was being used 
> in tsens_read_temp().
> However, this change is no longer required as I am removing the use of 
> tsens_version() in tsens_read_temp(). As discussed earlier with Konrad, 
> it makes more sense to check for valid‑bit support rather than relying 
> on the TSENS version check in tsens_read_temp().

Ah yes, makes sense

[ ... ]

> > > +    }
> > > +
> > > +    if (temp_val[0] == temp_val[1])
> > > +        *temp = temp_val[1];
> > > +    else if (temp_val[1] == temp_val[2])
> > > +        *temp = temp_val[2];
> > > +    else
> > > +        return -EAGAIN;
> >
> > We have a, b and c.
> >
> > if a == b, then return b
> > else b == c, then return c
> > else return -EAGAIN
> >
> > It is like we have two consecutives successful read. IMO that could be 
> > simplified to:
> >
> > int prev = INTMAX;
> >
> > /*
> >   * An explanation ...
> >   */
> >
> > for (i = 0; i < max_retry; i++) {
> >
> >      int value, valid;
> >
> >      ret = regmap_field_read(priv->rf[field], &status);
> >      if (ret)
> >          return ret;
> >
> >      value = FIELD_GET(priv->feat->last_temp_mask, status);
> >
> >      valid = FIELD_GET(priv->feat->valid_bit, status)
> >      if (valid)
> >          return value;
> >
> >      if (value == prev)
> >          return value;
> >
> >      prev = value;
> > }
> >
> > return -EAGAIN;
> >
> > (Not tested)
> This approach has some misalignment with the HW recommendations.
> As per the HW guidelines, 3 back‑to‑back reads must be performed until a 
> valid read is observed.
> b or c should be returned only if none of the three reads(a,b,c) report 
> the valid bit not set.

Right I missed the point the HW recommendations is to read 3 times in 
any case. Maybe replace if (value == prev) continue; ?