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

[Priyansh Jain <priyansh.jain@oss.qualcomm.com>]

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

>