Re: [PATCH v2 12/16] hwmon: (mr75203) modify the temperature equation

Guenter Roeck
2022-08-18T20:24:28Z

On Wed, Aug 17, 2022 at 05:43:17AM +0000, Eliav Farber wrote:
> Modify the equation and coefficients to convert the digital output to
> temperature according to series 5 of the Moortec Embedded Temperature
> Sensor (METS) datasheet:
> T = G + H * (n / cal5 - 0.5) + J * F
>
> The G, H and J coefficients are multiplied by 1000 to get the temperature
> in milli-Celsius.
>

This is, at the very least, confusing. It doesn't explain the discrepancy
to the old code nor the change in constant values. I have no idea if this
change would result in erroneous readings on some other system where
the existing calculation may be the correct one.

On top of that, it seems overflow-prune in 32 bit systems.

Guenter

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> drivers/hwmon/mr75203.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index e3191f590167..e500897585e4 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -102,9 +102,10 @@
>
> #define PVT_POLL_DELAY_US 20
> #define PVT_POLL_TIMEOUT_US 20000
> -#define PVT_H_CONST 100000
> -#define PVT_CAL5_CONST 2047
> -#define PVT_G_CONST 40000
> +#define PVT_H_CONST 60000
> +#define PVT_G_CONST 200000
> +#define PVT_J_CONST -100
> +#define PVT_CAL5_CONST 4094
> #define PVT_CONV_BITS 10
> #define PVT_N_CONST 90
> #define PVT_R_CONST 245805
> @@ -158,7 +159,6 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
> struct regmap *t_map = pvt->t_map;
> u32 stat, nbs;
> int ret;
> - u64 tmp;
>
> switch (attr) {
> case hwmon_temp_input:
> @@ -176,12 +176,13 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
> nbs &= SAMPLE_DATA_MSK;
>
> /*
> - * Convert the register value to
> - * degrees centigrade temperature
> + * Convert the register value to degrees centigrade temperature:
> + * T = G + H * (n / cal5 - 0.5) + J * F
> */
> - tmp = nbs * PVT_H_CONST;
> - do_div(tmp, PVT_CAL5_CONST);
> - *val = tmp - PVT_G_CONST - pvt->ip_freq;
> + *val = PVT_G_CONST;
> + *val += PVT_H_CONST * nbs / PVT_CAL5_CONST;
> + *val -= PVT_H_CONST / 2;
> + *val += PVT_J_CONST * pvt->ip_freq / HZ_PER_MHZ;
>
> return 0;
> default:
> @@ -313,7 +314,7 @@ static int pvt_init(struct pvt_device *pvt)
> (key >> 1) << CLK_SYNTH_HI_SFT |
> (key >> 1) << CLK_SYNTH_HOLD_SFT | CLK_SYNTH_EN;
>
> - pvt->ip_freq = sys_freq * 100 / (key + 2);
> + pvt->ip_freq = clk_get_rate(pvt->clk) / (key + 2);
>
> if (t_num) {
> ret = regmap_write(t_map, SDIF_SMPL_CTRL, 0x0);