[PATCH 0/2] thermal: qcom: tsens: fix temperature handling

[PATCH 0/2] thermal: qcom: tsens: fix temperature handling

[Priyansh Jain]

This series fixes multiple issues in the Qualcomm TSENS thermal driver
related to temperature sampling and trip threshold handling.

Patch 1 updates the temperature read path to atomically sample the
temperature value along with its valid bit, in accordance with hardware
programming guidelines. It also implements the recommended retry and
fallback behavior to avoid incorrect readings during transient hardware
update windows.

Patch 2 widens the software trip temperature limits to match the full
hardware-supported range. This prevents repeated threshold
reprogramming and interrupt storms when devices operate beyond the
previously clamped limits on newer chipsets, while preserving behavior
for platforms operating within the original range.

Priyansh Jain (2):
  thermal: qcom: tsens: atomic temperature read with hardware-guided
    retries
  thermal: qcom: tsens: widen temperature limits to match hardware range

 drivers/thermal/qcom/tsens-v1.c |   6 +-
 drivers/thermal/qcom/tsens-v2.c |  10 +--
 drivers/thermal/qcom/tsens.c    | 118 +++++++++++++++++++++-----------
 drivers/thermal/qcom/tsens.h    |  22 ++----
 4 files changed, 93 insertions(+), 63 deletions(-)

-- 
2.43.0

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

[Priyansh Jain]

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@oss.qualcomm.com>
---
 drivers/thermal/qcom/tsens-v1.c |   6 +-
 drivers/thermal/qcom/tsens-v2.c |   6 +-
 drivers/thermal/qcom/tsens.c    | 118 +++++++++++++++++++++-----------
 drivers/thermal/qcom/tsens.h    |  22 ++----
 4 files changed, 91 insertions(+), 61 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
index faa5d00788ca..2e0a01348c48 100644
--- a/drivers/thermal/qcom/tsens-v1.c
+++ b/drivers/thermal/qcom/tsens-v1.c
@@ -77,6 +77,9 @@ static struct tsens_features tsens_v1_feat = {
 	.max_sensors	= 11,
 	.trip_min_temp	= -40000,
 	.trip_max_temp	= 120000,
+	.valid_bit = BIT(14),
+	.last_temp_mask = 0x3FF,
+	.last_temp_resolution = 9,
 };
 
 static struct tsens_features tsens_v1_no_rpm_feat = {
@@ -132,8 +135,7 @@ static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
 	/* NO CRITICAL INTERRUPT SUPPORT on v1 */
 
 	/* Sn_STATUS */
-	REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  9),
-	REG_FIELD_FOR_EACH_SENSOR11(VALID,        TM_Sn_STATUS_OFF, 14, 14),
+	REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  14),
 	/* xxx_STATUS bits: 1 == threshold violated */
 	REG_FIELD_FOR_EACH_SENSOR11(MIN_STATUS,   TM_Sn_STATUS_OFF, 10, 10),
 	REG_FIELD_FOR_EACH_SENSOR11(LOWER_STATUS, TM_Sn_STATUS_OFF, 11, 11),
diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index 8d9698ea3ec4..814147735ba5 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -56,6 +56,9 @@ static struct tsens_features tsens_v2_feat = {
 	.max_sensors	= 16,
 	.trip_min_temp	= -40000,
 	.trip_max_temp	= 120000,
+	.valid_bit = BIT(21),
+	.last_temp_mask = 0xFFF,
+	.last_temp_resolution = 11,
 };
 
 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;
+}
+
+/**
+ * tsens_read_temp - To read temperature from hw in deciCelsius.
+ * @s:     Pointer to sensor struct
+ * @field: Index into regmap_field array pointing to temperature data
+ * @temp: temperature in deciCelsius to be read from hardware
+ *
+ * This function handles temperature returned in ADC code or deciCelsius
+ * depending on IP version.
+ *
+ * Return: 0 on success, a negative errno will be returned in error cases
+ */
+static int tsens_read_temp(const struct tsens_sensor *s, int field, int *temp)
+{
+	struct tsens_priv *priv = s->priv;
+	int temp_val[3] = {0};
+	unsigned int status = 0;
+	int ret = 0, i;
+	int max_retry = 3;
+
+	ret = regmap_field_read(priv->rf[field], &status);
+	if (ret)
+		return ret;
+
+	/* VER_0 doesn't have VALID bit */
+	if (tsens_version(priv) == VER_0) {
+		*temp = status;
+		return ret;
+	}
+
+	for (i = 0; i < max_retry; i++) {
+		temp_val[i] = status & priv->feat->last_temp_mask;
+		if (status & priv->feat->valid_bit) {
+			*temp = temp_val[i];
+			return ret;
+		}
+		ret = regmap_field_read(priv->rf[field], &status);
+		if (ret)
+			return ret;
+	}
+
+	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;
+
+	return ret;
+}
+
 /**
  * tsens_hw_to_mC - Return sign-extended temperature in mCelsius.
  * @s:     Pointer to sensor struct
  * @field: Index into regmap_field array pointing to temperature data
+ * @temp: temperature in milliCelsius to be read from hardware
  *
  * This function handles temperature returned in ADC code or deciCelsius
  * depending on IP version.
@@ -326,19 +382,12 @@ static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
  * Return: Temperature in milliCelsius on success, a negative errno will
  * be returned in error cases
  */
-static int tsens_hw_to_mC(const struct tsens_sensor *s, int field)
+static int tsens_hw_to_mC(const struct tsens_sensor *s, int temp)
 {
 	struct tsens_priv *priv = s->priv;
 	u32 resolution;
-	u32 temp = 0;
-	int ret;
 
-	resolution = priv->fields[LAST_TEMP_0].msb -
-		priv->fields[LAST_TEMP_0].lsb;
-
-	ret = regmap_field_read(priv->rf[field], &temp);
-	if (ret)
-		return ret;
+	resolution = priv->feat->last_temp_resolution;
 
 	/* Convert temperature from ADC code to milliCelsius */
 	if (priv->feat->adc)
@@ -370,11 +419,6 @@ static int tsens_mC_to_hw(const struct tsens_sensor *s, int temp)
 	return temp / 100;
 }
 
-static inline enum tsens_ver tsens_version(struct tsens_priv *priv)
-{
-	return priv->feat->ver_major;
-}
-
 static void tsens_set_interrupt_v1(struct tsens_priv *priv, u32 hw_id,
 				   enum tsens_irq_type irq_type, bool enable)
 {
@@ -514,8 +558,12 @@ static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id,
 					&d->crit_irq_mask);
 		if (ret)
 			return ret;
-
-		d->crit_thresh = tsens_hw_to_mC(s, CRIT_THRESH_0 + hw_id);
+		ret = regmap_field_read(priv->rf[CRIT_THRESH_0 + hw_id], &d->crit_thresh);
+		if (ret)
+			return ret;
+		d->crit_thresh = tsens_hw_to_mC(s, d->crit_thresh);
+		if (ret)
+			return ret;
 	} else {
 		/* No mask register on older TSENS */
 		d->up_irq_mask = 0;
@@ -524,9 +572,14 @@ static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id,
 		d->crit_irq_mask = 0;
 		d->crit_thresh = 0;
 	}
-
-	d->up_thresh  = tsens_hw_to_mC(s, UP_THRESH_0 + hw_id);
-	d->low_thresh = tsens_hw_to_mC(s, LOW_THRESH_0 + hw_id);
+	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",
 		hw_id, __func__,
@@ -750,33 +803,16 @@ static void tsens_disable_irq(struct tsens_priv *priv)
 
 int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
 {
-	struct tsens_priv *priv = s->priv;
+	int ret;
 	int hw_id = s->hw_id;
 	u32 temp_idx = LAST_TEMP_0 + hw_id;
-	u32 valid_idx = VALID_0 + hw_id;
-	u32 valid;
-	int ret;
-
-	/* VER_0 doesn't have VALID bit */
-	if (tsens_version(priv) == VER_0)
-		goto get_temp;
 
-	/* Valid bit is 0 for 6 AHB clock cycles.
-	 * At 19.2MHz, 1 AHB clock is ~60ns.
-	 * We should enter this loop very, very rarely.
-	 * Wait 1 us since it's the min of poll_timeout macro.
-	 * Old value was 400 ns.
-	 */
-	ret = regmap_field_read_poll_timeout(priv->rf[valid_idx], valid,
-					     valid, 1, 20 * USEC_PER_MSEC);
-	if (ret)
-		return ret;
+	ret = tsens_read_temp(s, temp_idx, temp);
 
-get_temp:
-	/* Valid bit is set, OK to read the temperature */
-	*temp = tsens_hw_to_mC(s, temp_idx);
+	if (!ret)
+		*temp = tsens_hw_to_mC(s, *temp);
 
-	return 0;
+		return ret;
 }
 
 int get_temp_common(const struct tsens_sensor *s, int *temp)
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 2a7afa4c899b..f346557b0a8a 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -194,22 +194,6 @@ enum regfield_ids {
 	LAST_TEMP_13,
 	LAST_TEMP_14,
 	LAST_TEMP_15,
-	VALID_0,		/* VALID reading or not */
-	VALID_1,
-	VALID_2,
-	VALID_3,
-	VALID_4,
-	VALID_5,
-	VALID_6,
-	VALID_7,
-	VALID_8,
-	VALID_9,
-	VALID_10,
-	VALID_11,
-	VALID_12,
-	VALID_13,
-	VALID_14,
-	VALID_15,
 	LOWER_STATUS_0,	/* LOWER threshold violated */
 	LOWER_STATUS_1,
 	LOWER_STATUS_2,
@@ -511,6 +495,9 @@ enum regfield_ids {
  * @max_sensors: maximum sensors supported by this version of the IP
  * @trip_min_temp: minimum trip temperature supported by this version of the IP
  * @trip_max_temp: maximum trip temperature supported by this version of the IP
+ * @valid_bit: validate if read temperature is valid or not?
+ * @last_temp_mask: mask register for last temperature
+ * @last_temp_resolution: last temperarure sign bit resolution
  */
 struct tsens_features {
 	unsigned int ver_major;
@@ -522,6 +509,9 @@ struct tsens_features {
 	unsigned int max_sensors;
 	int trip_min_temp;
 	int trip_max_temp;
+	int valid_bit;
+	int last_temp_mask;
+	u32 last_temp_resolution;
 };
 
 /**
-- 
2.43.0

[PATCH 2/2] thermal: qcom: tsens: widen temperature limits to match hardware range

[Priyansh Jain]

The TSENS v2 software driver currently clamps trip_min_temp and
trip_max_temp to -40°C and 120°C respectively. However, the
TSENS v2 hardware temperature threshold registers support a wider
programmable range from -204°C to +204°C.

On newer chipsets using TSENS v2, devices may legitimately operate
beyond the existing software limits (for example, up to 130°C). When a
trip temperature is programmed outside the software clamped range, it is
constrained to 120°C on the upper end or -40°C on the lower end.
If the actual temperature continues to exceed this clamped limit, the
threshold is immediately violated again, which can result in a
continuous interrupt storm.

Expand the TSENS v2 software trip temperature limits to match the full
hardware supported range (-204°C to +204°C). This avoids repeated
threshold reprogramming and ensures correct trip handling on TSENS v2
based platforms.

Signed-off-by: Priyansh Jain <priyansh.jain@oss.qualcomm.com>
---
 drivers/thermal/qcom/tsens-v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index 814147735ba5..3bd654c8dd6e 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -54,8 +54,8 @@ static struct tsens_features tsens_v2_feat = {
 	.adc		= 0,
 	.srot_split	= 1,
 	.max_sensors	= 16,
-	.trip_min_temp	= -40000,
-	.trip_max_temp	= 120000,
+	.trip_min_temp	= -204000,
+	.trip_max_temp	= 204000,
 	.valid_bit = BIT(21),
 	.last_temp_mask = 0xFFF,
 	.last_temp_resolution = 11,
-- 
2.43.0

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

[Konrad Dybcio]

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@oss.qualcomm.com>
> ---

[...]

>  static struct tsens_features tsens_v1_no_rpm_feat = {

This struct also needs the same adjustment

[...]

>  static struct tsens_features ipq8074_feat = {

And other structs in tsens-v2.c, but..

> @@ -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

[...]

> +static int tsens_read_temp(const struct tsens_sensor *s, int field, int *temp)
> +{
> +	struct tsens_priv *priv = s->priv;
> +	int temp_val[3] = {0};
> +	unsigned int status = 0;
> +	int ret = 0, i;
> +	int max_retry = 3;
> +
> +	ret = regmap_field_read(priv->rf[field], &status);
> +	if (ret)
> +		return ret;
> +
> +	/* VER_0 doesn't have VALID bit */
> +	if (tsens_version(priv) == VER_0) {

Then, we can check for if (!priv->rf[field]) instead of checking the ver
explicitly

Konrad

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

[Konrad Dybcio]

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@oss.qualcomm.com>
> ---

[...]

> +/**
> + * tsens_read_temp - To read temperature from hw in deciCelsius.

"Retrieve temperature readings from the hardware"

> + * @s:     Pointer to sensor struct
> + * @field: Index into regmap_field array pointing to temperature data
> + * @temp: temperature in deciCelsius to be read from hardware
> + *
> + * This function handles temperature returned in ADC code or deciCelsius
> + * depending on IP version.
> + *
> + * Return: 0 on success, a negative errno will be returned in error cases
> + */
> +static int tsens_read_temp(const struct tsens_sensor *s, int field, int *temp)
> +{
> +	struct tsens_priv *priv = s->priv;
> +	int temp_val[3] = {0};
> +	unsigned int status = 0;

This should be a u32 to reflect this is a known-size register

> +	int ret = 0, i;

You can declare the iterator inside the loop

'ret' doesn't need to be initialized, as it's immediately overwritten

> +	int max_retry = 3;

This should be a constant, perhaps a #defined one, reused for the size
of temp_val

> +
> +	ret = regmap_field_read(priv->rf[field], &status);
> +	if (ret)
> +		return ret;
> +
> +	/* VER_0 doesn't have VALID bit */
> +	if (tsens_version(priv) == VER_0) {
> +		*temp = status;
> +		return ret;

Ret will always be a '0' here (and in below cases)

> +	}
> +
> +	for (i = 0; i < max_retry; i++) {
> +		temp_val[i] = status & priv->feat->last_temp_mask;
> +		if (status & priv->feat->valid_bit) {
> +			*temp = temp_val[i];
> +			return ret;
> +		}
> +		ret = regmap_field_read(priv->rf[field], &status);

Please add a \n above for readability> +		if (ret)
> +			return ret;
> +	}
> +
> +	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;

This deserves a comment. Your description in the commit message is good,
but the reasoning behind this logic is not obvious and will get lost in
time, as 'git blame' will be bloated by cleanups

Konrad

Re: [PATCH 2/2] thermal: qcom: tsens: widen temperature limits to match hardware range

[Konrad Dybcio]

On 4/30/26 7:44 AM, Priyansh Jain wrote:
> The TSENS v2 software driver currently clamps trip_min_temp and
> trip_max_temp to -40°C and 120°C respectively. However, the
> TSENS v2 hardware temperature threshold registers support a wider
> programmable range from -204°C to +204°C.

Does this hold true for all tsens_v2 implementations, starting with
the ancient msm8996 and msm8953?

Konrad

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

[Konrad Dybcio]

On 5/4/26 11:42 AM, Priyansh Jain wrote:
> 
> On 30-04-2026 09:30 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@oss.qualcomm.com>
>>> ---


>>> +    ret = regmap_field_read(priv->rf[field], &status);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /* VER_0 doesn't have VALID bit */
>>> +    if (tsens_version(priv) == VER_0) {
>>> +        *temp = status;
>>> +        return ret;
>> Ret will always be a '0' here (and in below cases)
> Yes correct it will be '0' , so you are suggesting to return 0 instead of ret ?

Yes, this makes things more obvious

Konrad

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

[Konrad Dybcio]

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@oss.qualcomm.com>
>>> ---

[...]

>>> @@ -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.

Konrad

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@oss.qualcomm.com>
>>>> ---
> 
> [...]
> 
>>>> @@ -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

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

[Daniel Lezcano]

On 4/30/26 07:44, 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@oss.qualcomm.com>
> ---
>   drivers/thermal/qcom/tsens-v1.c |   6 +-
>   drivers/thermal/qcom/tsens-v2.c |   6 +-
>   drivers/thermal/qcom/tsens.c    | 118 +++++++++++++++++++++-----------
>   drivers/thermal/qcom/tsens.h    |  22 ++----
>   4 files changed, 91 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
> index faa5d00788ca..2e0a01348c48 100644
> --- a/drivers/thermal/qcom/tsens-v1.c
> +++ b/drivers/thermal/qcom/tsens-v1.c
> @@ -77,6 +77,9 @@ static struct tsens_features tsens_v1_feat = {
>   	.max_sensors	= 11,
>   	.trip_min_temp	= -40000,
>   	.trip_max_temp	= 120000,
> +	.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

>   };
>   
>   static struct tsens_features tsens_v1_no_rpm_feat = {
> @@ -132,8 +135,7 @@ static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
>   	/* NO CRITICAL INTERRUPT SUPPORT on v1 */
>   
>   	/* Sn_STATUS */
> -	REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  9),
> -	REG_FIELD_FOR_EACH_SENSOR11(VALID,        TM_Sn_STATUS_OFF, 14, 14),
> +	REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  14),
>   	/* xxx_STATUS bits: 1 == threshold violated */
>   	REG_FIELD_FOR_EACH_SENSOR11(MIN_STATUS,   TM_Sn_STATUS_OFF, 10, 10),
>   	REG_FIELD_FOR_EACH_SENSOR11(LOWER_STATUS, TM_Sn_STATUS_OFF, 11, 11),
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 8d9698ea3ec4..814147735ba5 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -56,6 +56,9 @@ static struct tsens_features tsens_v2_feat = {
>   	.max_sensors	= 16,
>   	.trip_min_temp	= -40000,
>   	.trip_max_temp	= 120000,
> +	.valid_bit = BIT(21),
> +	.last_temp_mask = 0xFFF,
> +	.last_temp_resolution = 11,

Ditto

>   };
>   
>   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.

> +/**
> + * tsens_read_temp - To read temperature from hw in deciCelsius.
> + * @s:     Pointer to sensor struct
> + * @field: Index into regmap_field array pointing to temperature data
> + * @temp: temperature in deciCelsius to be read from hardware
> + *
> + * This function handles temperature returned in ADC code or deciCelsius
> + * depending on IP version.
> + *
> + * Return: 0 on success, a negative errno will be returned in error cases
> + */
> +static int tsens_read_temp(const struct tsens_sensor *s, int field, int *temp)
> +{
> +	struct tsens_priv *priv = s->priv;
> +	int temp_val[3] = {0};
> +	unsigned int status = 0;
> +	int ret = 0, i;
> +	int max_retry = 3;

Please avoid litterals. Add a macro for max number of retries. As the 
value 3 is not an arbitrary value but a documented value, add a small 
comment to tell it is a hardware requirement.

> +	ret = regmap_field_read(priv->rf[field], &status);
> +	if (ret)
> +		return ret;
> +
> +	/* VER_0 doesn't have VALID bit */
> +	if (tsens_version(priv) == VER_0) {
> +		*temp = status;
> +		return ret;
> +	}

Please use a callback for v0 and v1. Set it at probe time, so the 
version does not have to be checked at very read.

> +	for (i = 0; i < max_retry; i++) {
> +		temp_val[i] = status & priv->feat->last_temp_mask;
> +		if (() {
> +			*temp = temp_val[i];
> +			return ret;
> +		}
> +		ret = regmap_field_read(priv->rf[field], &status);
> +		if (ret)
> +			return ret;

It looks like more than max_retry is happening. One time before the 
loop, then 3 times in loop. So 4 times in total.

> +	}
> +
> +	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)

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

[Priyansh Jain]

On 04-05-2026 10:59 pm, Daniel Lezcano wrote:
> On 4/30/26 07:44, 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@oss.qualcomm.com>
>> ---
>>   drivers/thermal/qcom/tsens-v1.c |   6 +-
>>   drivers/thermal/qcom/tsens-v2.c |   6 +-
>>   drivers/thermal/qcom/tsens.c    | 118 +++++++++++++++++++++-----------
>>   drivers/thermal/qcom/tsens.h    |  22 ++----
>>   4 files changed, 91 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/ 
>> tsens-v1.c
>> index faa5d00788ca..2e0a01348c48 100644
>> --- a/drivers/thermal/qcom/tsens-v1.c
>> +++ b/drivers/thermal/qcom/tsens-v1.c
>> @@ -77,6 +77,9 @@ static struct tsens_features tsens_v1_feat = {
>>       .max_sensors    = 11,
>>       .trip_min_temp    = -40000,
>>       .trip_max_temp    = 120000,
>> +    .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);
?
> 
>>   };
>>   static struct tsens_features tsens_v1_no_rpm_feat = {
>> @@ -132,8 +135,7 @@ static const struct reg_field 
>> tsens_v1_regfields[MAX_REGFIELDS] = {
>>       /* NO CRITICAL INTERRUPT SUPPORT on v1 */
>>       /* Sn_STATUS */
>> -    REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  9),
>> -    REG_FIELD_FOR_EACH_SENSOR11(VALID,        TM_Sn_STATUS_OFF, 14, 14),
>> +    REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  
>> 14),
>>       /* xxx_STATUS bits: 1 == threshold violated */
>>       REG_FIELD_FOR_EACH_SENSOR11(MIN_STATUS,   TM_Sn_STATUS_OFF, 10, 
>> 10),
>>       REG_FIELD_FOR_EACH_SENSOR11(LOWER_STATUS, TM_Sn_STATUS_OFF, 11, 
>> 11),
>> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/ 
>> tsens-v2.c
>> index 8d9698ea3ec4..814147735ba5 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -56,6 +56,9 @@ static struct tsens_features tsens_v2_feat = {
>>       .max_sensors    = 16,
>>       .trip_min_temp    = -40000,
>>       .trip_max_temp    = 120000,
>> +    .valid_bit = BIT(21),
>> +    .last_temp_mask = 0xFFF,
>> +    .last_temp_resolution = 11,
> 
> Ditto
ACK
> 
>>   };
>>   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().
>> +/**
>> + * tsens_read_temp - To read temperature from hw in deciCelsius.
>> + * @s:     Pointer to sensor struct
>> + * @field: Index into regmap_field array pointing to temperature data
>> + * @temp: temperature in deciCelsius to be read from hardware
>> + *
>> + * This function handles temperature returned in ADC code or deciCelsius
>> + * depending on IP version.
>> + *
>> + * Return: 0 on success, a negative errno will be returned in error 
>> cases
>> + */
>> +static int tsens_read_temp(const struct tsens_sensor *s, int field, 
>> int *temp)
>> +{
>> +    struct tsens_priv *priv = s->priv;
>> +    int temp_val[3] = {0};
>> +    unsigned int status = 0;
>> +    int ret = 0, i;
>> +    int max_retry = 3;
> 
> Please avoid litterals. Add a macro for max number of retries. As the 
> value 3 is not an arbitrary value but a documented value, add a small 
> comment to tell it is a hardware requirement.
> 
ACK
>> +    ret = regmap_field_read(priv->rf[field], &status);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* VER_0 doesn't have VALID bit */
>> +    if (tsens_version(priv) == VER_0) {
>> +        *temp = status;
>> +        return ret;
>> +    }
> 
> Please use a callback for v0 and v1. Set it at probe time, so the 
> version does not have to be checked at very read.
> 
Yes i am removing version check, instead adding valid bit check as
discussed with Konrad earlier.

>> +    for (i = 0; i < max_retry; i++) {
>> +        temp_val[i] = status & priv->feat->last_temp_mask;
>> +        if (() {
>> +            *temp = temp_val[i];
>> +            return ret;
>> +        }
>> +        ret = regmap_field_read(priv->rf[field], &status);
>> +        if (ret)
>> +            return ret;
> 
> It looks like more than max_retry is happening. One time before the 
> loop, then 3 times in loop. So 4 times in total.

Thanks for pointing this out, Yes correct read will happen 4 times will
update the logic.

> 
>> +    }
>> +
>> +    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.

If a == b, return b
Else if b == c, return c
Else return -EAGAIN

Regards,
Priyansh
> 
> 
> 

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; ?

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

[Priyansh Jain]

On 05-05-2026 01:13 pm, Daniel Lezcano wrote:
> 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
> 
ACK
> 
>>>>   };
>>>>   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; ?
> 
We need to store all three readings because, if all of them are invalid, 
we must compare the first, second, and third reads using the following 
logic:

if a == b, return b
else if b == c, return c
else return -EAGAIN

Given this requirement, comparing (value == prev) inside the read loop 
would not be correct, as it does not preserve all three samples for the 
final comparison.

Thanks,
Priyansh

> 

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

[Daniel Lezcano]

On 5/5/26 10:48, Priyansh Jain wrote:

[ ... ]

>>>>
>>>> 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; ?
>>
> We need to store all three readings because, if all of them are invalid, 
> we must compare the first, second, and third reads using the following 
> logic:
> 
> if a == b, return b
> else if b == c, return c
> else return -EAGAIN
> 
> Given this requirement, comparing (value == prev) inside the read loop 
> would not be correct, as it does not preserve all three samples for the 
> final comparison.

I tried the different combinations and comparing inside the loop should 
work. But the optimization introduces an implicit inference not helping 
for the clarity of the code and probably prone to errors in case of 
changes. So probably simpler to keep your approach. Please add a comment 
above the if a == b return b else ...

Thanks

   -- Daniel

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

[Priyansh Jain]

On 05-05-2026 03:05 pm, Daniel Lezcano wrote:
> On 5/5/26 10:48, Priyansh Jain wrote:
> 
> [ ... ]
> 
>>>>>
>>>>> 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; ?
>>>
>> We need to store all three readings because, if all of them are 
>> invalid, we must compare the first, second, and third reads using the 
>> following logic:
>>
>> if a == b, return b
>> else if b == c, return c
>> else return -EAGAIN
>>
>> Given this requirement, comparing (value == prev) inside the read loop 
>> would not be correct, as it does not preserve all three samples for 
>> the final comparison.
> 
> I tried the different combinations and comparing inside the loop should 
> work. But the optimization introduces an implicit inference not helping 
> for the clarity of the code and probably prone to errors in case of 
> changes. So probably simpler to keep your approach. Please add a comment 
> above the if a == b return b else ...
> 
> Thanks

Thanks , will go ahead with my approach and will add a comment before 
comparison code .

Thanks,
Priyansh

> 
>    -- Daniel