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

[Priyansh Jain]

On 04-05-2026 03:29 pm, Konrad Dybcio wrote:
> On 5/4/26 11:42 AM, Priyansh Jain wrote:
> > On 30-04-2026 09:21 pm, Konrad Dybcio wrote:
> > > On 4/30/26 7:44 AM, Priyansh Jain wrote:
> > > > The existing TSENS temperature read logic polls the valid bit and then
> > > > reads the temperature register. When temperature reads are triggered
> > > > at very short intervals, this can race with hardware updates and allow
> > > > the temperature field to be read while it is still being updated.
> > > >
> > > > In this case, the valid bit may already be asserted even though the
> > > > temperature value is transitioning, resulting in an incorrect reading.
> > > >
> > > > Hardware programming guidelines require the temperature value and the
> > > > valid bit to be sampled atomically in the same read transaction. A
> > > > reading is considered valid only if the valid bit is observed set in
> > > > that same sample.
> > > >
> > > > The guidelines further specify that software should attempt the
> > > > temperature read up to three times to account for transient update
> > > > windows. If none of the attempts observe a valid sample, a stable
> > > > fallback value must be returned: if the first and second samples match,
> > > > the second value is returned; otherwise, if the second and third
> > > > samples match, the third value is returned.
> > > >
> > > > Update the TSENS sensor read logic to implement atomic sampling along
> > > > with the recommended retry-and-compare fallback behavior. This removes
> > > > the race window and ensures deterministic temperature values in
> > > > accordance with hardware requirements.
> > > >
> > > > Signed-off-by: Priyansh Jain<priyansh.jain@xxxxxxxxxxxxxxxx>
> > > > ---
>
> [...]
>
> > > > @@ -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),
> > > ..this change feels rather odd - the existing regfields seem like a good
> > > place to handle this register map difference
> > This change is required to ensure that both the VALID bit and LAST_TEMP
> > are read as part of a single transaction, in order to avoid a race condition where
> > the VALID bit may already be asserted while the temperature value is still transitioning,
> > potentially resulting in an incorrect reading. If needed, I can rename the field from
> > LAST_TEMP to something that more clearly reflects a combined representation
> > of the temperature value and the VALID bit.
> >
> > Let me know you thoughts on this.
>
> Hm, I somehow managed to skip the connection between the two..
>
> I think we could retain the current (pretty good) containment of all
> register differences between the versions as-is, with something like
> this diff:
>
> -	ret = regmap_field_read(priv->rf[field], &status);
> +	/* Fields within the STATUS register are only valid if read atomically */
> +	ret = regmap_read(priv->rf[field].reg, &status);
> 	if (ret)
> 		return ret;
>
> and then falling back to the prior definitions of the VALID mask etc.

Thanks for this feedback , this seems correct approach, i will update as 
per the suggestion provided

Thanks,
Priyansh

> Konrad