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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D9GH5V09WW47.358SY1F7LJ9ZV@cknow.org>
Date: Sat, 26 Apr 2025 11:49:13 +0200
From: "Diederik de Haas" <didi.debian@...ow.org>
To: "Nicolas Frattaroli" <nicolas.frattaroli@...labora.com>, "Rafael J.
 Wysocki" <rafael@...nel.org>, "Daniel Lezcano" <daniel.lezcano@...aro.org>,
 "Zhang Rui" <rui.zhang@...el.com>, "Lukasz Luba" <lukasz.luba@....com>,
 "Rob Herring" <robh@...nel.org>, "Krzysztof Kozlowski"
 <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>, "Heiko
 Stuebner" <heiko@...ech.de>, "Jonas Karlman" <jonas@...boo.se>
Cc: <devicetree@...r.kernel.org>, <linux-pm@...r.kernel.org>, "Sebastian
 Reichel" <sebastian.reichel@...labora.com>, <linux-kernel@...r.kernel.org>,
 <linux-rockchip@...ts.infradead.org>, <kernel@...labora.com>,
 <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 5/7] thermal: rockchip: support reading trim values
 from OTP

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)