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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Saturday, 26 April 2025 11:49:13 Central European Summer Time Diederik de Haas wrote:
> 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.

No. It is not possible to redefine the unit of these, because the values are 
factory programmed into these one-time programmable cells.

> And possibly define-ing the '30' so people don't need this commit
> message to figure out where that magic number comes from?

There is no constant of 30 in the code to define anywhere. This is what the
factory programmed values are referenced against.

> And also the '923' for ``.trim_slope``?

.trim_slope is an SoC specific value that does not need to be defined a second
time in a different place, it lives in the SoC data for that reason, much like
the code tables and which functions the driver should use for this chip. There
is nothing to be gained here from the indirection.

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

That seems like an additional change out of scope for this series, and does not
need to hold up this patch series for a v6.

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