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

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

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