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

From: Guenter Roeck <linux@roeck-us.net>
To: Eliav Farber <farbere@amazon.com>
Cc: jdelvare@suse.com, robh+dt@kernel.org, mark.rutland@arm.com,
	linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, talel@amazon.com,
	hhhawa@amazon.com, jonnyc@amazon.com, hanochu@amazon.com,
	ronenk@amazon.com, itamark@amazon.com, shellykz@amazon.com,
	shorer@amazon.com, amitlavi@amazon.com, almogbs@amazon.com,
	dwmw@amazon.co.uk, rtanwar@maxlinear.com
Subject: Re: [PATCH v2 12/16] hwmon: (mr75203) modify the temperature equation
Date: Thu, 18 Aug 2022 13:23:24 -0700	[thread overview]
Message-ID: <20220818202324.GA3431316@roeck-us.net> (raw)
In-Reply-To: <20220817054321.6519-13-farbere@amazon.com>

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