Re: [PATCH v5 5/7] thermal: rockchip: support reading trim values from OTP

["Diederik de Haas" <didi.debian@cknow.org>]

[-- Attachment #1: Type: text/plain, Size: 4034 bytes --]

Hi Nicolas,

On Fri Apr 25, 2025 at 9:34 PM CEST, Nicolas Frattaroli wrote:
> Many of the Rockchip SoCs support storing trim values for the sensors in
> factory programmable memory. These values specify a fixed offset from
> the sensor's returned temperature to get a more accurate picture of what
> temperature the silicon is actually at.
>
> The way this is implemented is with various OTP cells, which may be
> absent. There may both be whole-TSADC trim values, as well as per-sensor
> trim values.
>
> In the downstream driver, whole-chip trim values override the per-sensor
> trim values. This rewrite of the functionality changes the semantics to
> something I see as slightly more useful: allow the whole-chip trim
> values to serve as a fallback for lacking per-sensor trim values,
> instead of overriding already present sensor trim values.
>
> Additionally, the chip may specify an offset (trim_base, trim_base_frac)
> in degrees celsius and degrees decicelsius respectively which defines
> what the basis is from which the trim, if any, should be calculated
> from. By default, this is 30 degrees Celsius, but the chip can once
> again specify a different value through OTP cells.

Would it be useful to define all the values in the same unit?
Having celsius and decicelsius and millicelsius sounds like a recipe for
(future) off-by-10/-100/-1000 errors.

And possibly define-ing the '30' so people don't need this commit
message to figure out where that magic number comes from?
And also the '923' for ``.trim_slope``?

> The implementation of these trim calculations have been tested
> extensively on an RK3576, where it was confirmed to get rid of pesky 1.8
> degree Celsius offsets between certain sensors.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/thermal/rockchip_thermal.c | 221 +++++++++++++++++++++++++++++++++----
>  1 file changed, 202 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 89e3180667e2a8f0ef5542b0db4d9e19a21a24d3..3beff9b6fac3abe8948b56132b618ff1bed57217 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -9,6 +9,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> @@ -69,16 +70,18 @@ struct chip_tsadc_table {
>   * struct rockchip_tsadc_chip - hold the private data of tsadc chip
>   * @chn_offset: the channel offset of the first channel
>   * @chn_num: the channel number of tsadc chip
> - * @tshut_temp: the hardware-controlled shutdown temperature value
> + * @trim_slope: used to convert the trim code to a temperature in millicelsius
> + * @tshut_temp: the hardware-controlled shutdown temperature value, with no trim

Having the same units used everywhere would also avoid possible
confusion here as ``trim_slope`` explicitly mentions millicelsius, but
``tshut_temp`` does not, but AFAIK it's also in millicelsius.

Cheers,
  Diederik

>   * @tshut_mode: the hardware-controlled shutdown mode (0:CRU 1:GPIO)
>   * @tshut_polarity: the hardware-controlled active polarity (0:LOW 1:HIGH)
>   * @initialize: SoC special initialize tsadc controller method
>   * @irq_ack: clear the interrupt
>   * @control: enable/disable method for the tsadc controller
> - * @get_temp: get the temperature
> + * @get_temp: get the raw temperature, unadjusted by trim
>   * @set_alarm_temp: set the high temperature interrupt
>   * @set_tshut_temp: set the hardware-controlled shutdown temperature
>   * @set_tshut_mode: set the hardware-controlled shutdown mode
> + * @get_trim_code: convert a hardware temperature code to one adjusted for by trim
>   * @table: the chip-specific conversion table
>   */
>  struct rockchip_tsadc_chip {
> @@ -86,6 +89,9 @@ struct rockchip_tsadc_chip {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]