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