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

[Konrad Dybcio]

On 5/13/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 yields 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; if neither pair matches, -EAGAIN 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>
> 
> ---

Leaving a couple nits below, feel free to apply my tag with or
without them:

Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>

Konrad


diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index b1b938312723..96c1ff9e3133 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -330,7 +330,7 @@ static int tsens_read_temp(const struct tsens_sensor *s, int field, int *temp)
 {
        struct tsens_priv *priv = s->priv;
        int temp_val[MAX_READ_RETRY] = {0};
-       u32 status = 0;
+       u32 status;
        int ret;
        u32 last_temp_mask = GENMASK(priv->fields[LAST_TEMP_0].msb,
                                        priv->fields[LAST_TEMP_0].lsb);
@@ -341,7 +341,7 @@ static int tsens_read_temp(const struct tsens_sensor *s, int field, int *temp)
                if (ret)
                        return ret;
 
-               /* VER_0 doesn't have VALID bit */
+               /* VER_0 doesn't have a VALID bit */
                if (!valid_bit) {
                        *temp = status & last_temp_mask;
                        return 0;
@@ -355,7 +355,8 @@ static int tsens_read_temp(const struct tsens_sensor *s, int field, int *temp)
                }
        }
 
-       /* As per the HW guidelines, if none of the attempts observe a
+       /*
+        * As per the HW guidelines, 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
@@ -576,13 +577,17 @@ static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id,
                d->crit_irq_mask = 0;
                d->crit_thresh = 0;
        }
+
        ret = regmap_field_read(priv->rf[UP_THRESH_0 + hw_id], &d->up_thresh);
        if (ret)
                return ret;
+
        d->up_thresh = tsens_hw_to_mC(s, d->up_thresh);
+
        ret = regmap_field_read(priv->rf[LOW_THRESH_0 + hw_id], &d->low_thresh);
        if (ret)
                return ret;
+
        d->low_thresh = tsens_hw_to_mC(s, d->low_thresh);
 
        dev_dbg(priv->dev, "[%u] %s%s: status(%u|%u|%u) | clr(%u|%u|%u) | mask(%u|%u|%u)\n",
@@ -807,12 +812,11 @@ static void tsens_disable_irq(struct tsens_priv *priv)
 
 int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
 {
-       int ret;
        int hw_id = s->hw_id;
        u32 temp_idx = LAST_TEMP_0 + hw_id;
+       int ret;
 
        ret = tsens_read_temp(s, temp_idx, temp);
-
        if (!ret)
                *temp = tsens_hw_to_mC(s, *temp);