Re: [PATCH/RFT v2 2/3] thermal: rcar_gen3_thermal: Update calculation formula of IRQTEMP

From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Cc: linux-pm@vger.kernel.org, Zhang Rui <rui.zhang@intel.com>,
	Eduardo Valentin <edubezval@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Simon Horman <horms@verge.net.au>,
	Magnus Damm <magnus.damm@gmail.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH/RFT v2 2/3] thermal: rcar_gen3_thermal: Update calculation formula of IRQTEMP
Date: Thu, 18 Apr 2019 10:53:46 +0200	[thread overview]
Message-ID: <20190418085346.GX28515@bigcity.dyn.berto.se> (raw)
In-Reply-To: <1555436655-5262-3-git-send-email-ykaneko0929@gmail.com>

Hi Kaneko-san,

Thanks for your work.

On 2019-04-17 02:44:14 +0900, Yoshihiro Kaneko wrote:
> Update the formula to calculate CTEMP:
> Currently, the CTEMP is average of val1 (is calculated by
> formula 1) and val2 (is calculated by formula 2). But,
> as description in HWM (chapter 10A.3.1.1 Setting of Normal Mode)
> 
> If (STEMP < Tj_T) CTEMP value should be val1.
> If (STEMP > Tj_T) CTEMP value should be val2.
> 
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index a2fd0fd..97cf3cd 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -77,6 +77,7 @@ struct rcar_gen3_thermal_tsc {
>  	struct equation_coefs coef;
>  	int low;
>  	int high;
> +	int tj_2;
>  };
>  
>  struct rcar_gen3_thermal_priv {
> @@ -126,28 +127,27 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
>  /* no idea where these constants come from */
>  #define TJ_3 -41
>  
> -static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> +
> +static void rcar_gen3_thermal_calc_coefs(struct rcar_gen3_thermal_tsc *tsc,
>  					 int *ptat, int *thcode,
>  					 unsigned int ths_tj_1)
>  {
> -	int tj_2;
> -
>  	/* TODO: Find documentation and document constant calculation formula */

Maybe you cold also fix this todo now that the datasheet have been 
updated with 10A.3.1.1 ? This might also impact the equation_coefs 
structure.

>  
>  	/*
>  	 * Division is not scaled in BSP and if scaled it might overflow
>  	 * the dividend (4095 * 4095 << 14 > INT_MAX) so keep it unscaled
>  	 */
> -	tj_2 = (FIXPT_INT((ptat[1] - ptat[2]) * 157)
> -		/ (ptat[0] - ptat[2])) + FIXPT_INT(TJ_3);
> +	tsc->tj_2 = (FIXPT_INT((ptat[1] - ptat[2]) * 157)
> +		     / (ptat[0] - ptat[2])) + FIXPT_INT(TJ_3);

I think tj_2 should be renamed to match the names in the datasheet, it's 
Tj_T from 10A.3.1.1 right?

>  
> -	coef->a1 = FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[2]),
> -			     tj_2 - FIXPT_INT(TJ_3));
> -	coef->b1 = FIXPT_INT(thcode[2]) - coef->a1 * TJ_3;
> +	tsc->coef.a1 = FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[2]),
> +				 tsc->tj_2 - FIXPT_INT(TJ_3));
> +	tsc->coef.b1 = FIXPT_INT(thcode[2]) - tsc->coef.a1 * TJ_3;
>  
> -	coef->a2 = FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[0]),
> -			     tj_2 - FIXPT_INT(ths_tj_1));
> -	coef->b2 = FIXPT_INT(thcode[0]) - coef->a2 * ths_tj_1;
> +	tsc->coef.a2 = FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[0]),
> +				 tsc->tj_2 - FIXPT_INT(ths_tj_1));
> +	tsc->coef.b2 = FIXPT_INT(thcode[0]) - tsc->coef.a2 * ths_tj_1;
>  }
>  
>  static int rcar_gen3_thermal_round(int temp)
> @@ -186,13 +186,15 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
>  static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
>  					      int mcelsius)
>  {
> -	int celsius, val1, val2;
> +	int celsius, val;
>  
>  	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
> -	val1 = celsius * tsc->coef.a1 + tsc->coef.b1;
> -	val2 = celsius * tsc->coef.a2 + tsc->coef.b2;
> +	if (celsius <= INT_FIXPT(tsc->tj_2))
> +		val = celsius * tsc->coef.a1 + tsc->coef.b1;
> +	else
> +		val = celsius * tsc->coef.a2 + tsc->coef.b2;
>  
> -	return INT_FIXPT((val1 + val2) / 2);
> +	return INT_FIXPT(val);
>  }
>  
>  static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
> @@ -440,7 +442,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  		priv->tscs[i] = tsc;
>  
>  		priv->thermal_init(tsc);
> -		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i],
> +		rcar_gen3_thermal_calc_coefs(tsc, ptat, thcode[i],
>  					     *rcar_gen3_ths_tj_1);
>  
>  		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> -- 
> 1.9.1
> 

-- 
Regards,
Niklas Söderlund