Re: [PATCH v4 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Wolfram Sang <wsa-dev@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3368 bytes --]


> Would you please check the following from checkpatch too?

I saw them and chose to ignore them. I am not strict with those warnings
within the i2c subsystem as well. I can change the series if your mileage
varies, of course.

> WARNING: please write a paragraph that describes the config symbol fully
> #82: FILE: drivers/thermal/Kconfig:248:
> +config RCAR_GEN3_THERMAL

I can make up something.

> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

Is this mandatory?

> CHECK: struct mutex definition without comment
> #186: FILE: drivers/thermal/rcar_gen3_thermal.c:75:
> +	struct mutex lock;

Can change, but if there is only one lock, I don't really see much
benefit from this check.

> WARNING: line over 80 characters
> #204: FILE: drivers/thermal/rcar_gen3_thermal.c:93:
> +static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc, u32 reg)
> 
> CHECK: Alignment should match open parenthesis
> #210: FILE: drivers/thermal/rcar_gen3_thermal.c:99:
> +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> +				     u32 reg, u32 data)

I have those warnings (80 chars and open parens) ignored by default but
if you think it makes the code more readable, I'll change it.

> > +static void _linear_coefficient_calculation(struct rcar_gen3_thermal_tsc *tsc,
> > +					    int *ptat, int *thcode)
> > +{
> > +	int tj_2;
> > +	s64 a1, b1;
> > +	s64 a2, b2;
> > +	s64 a1_num, a1_den;
> > +	s64 a2_num, a2_den;
> > +
> > +	tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
> > +		/ (ptat[0] - ptat[2])) - CODETSD(41);
> > +
> > +	/* calculate coefficients for linear equation */
> > +	a1_num = CODETSD(thcode[1] - thcode[2]);
> > +	a1_den = tj_2 - TJ_3;
> > +	a1 = (10000 * a1_num) / a1_den;
> > +	b1 = (10000 * thcode[2]) - ((a1 * TJ_3) / 1000);
> > +
> > +	a2_num = CODETSD(thcode[1] - thcode[0]);
> > +	a2_den = tj_2 - TJ_1;
> > +	a2 = (10000 * a2_num) / a2_den;
> > +	b2 = (10000 * thcode[0]) - ((a2 * TJ_1) / 1000);
> > +
> > +	tsc->coef.a1 = DIV_ROUND_CLOSEST(a1, 10);
> > +	tsc->coef.b1 = DIV_ROUND_CLOSEST(b1, 10);
> > +	tsc->coef.a2 = DIV_ROUND_CLOSEST(a2, 10);
> > +	tsc->coef.b2 = DIV_ROUND_CLOSEST(b2, 10);
> 
> What is a a1, b1, a2, b2 typical values?
> 
> are you sure they do not fit into int? Looks like you start from pretty small values,
> but multiply by 10^3 on num and den to get better precision?

Typical values are a few thousand. a1_num uses CODETSD which multiplies
by 1000 and makes it a million. a1 then multiplies again by 10000 which
makes it 10 billion. No int.

I am quite sure the formulas can be rearranged to fit into an int. As
mentioned before, I hoped we could start with the already tested
formulas since documentation on them is sparse.

> > +static int _linear_temp_converter(struct equation_coefs *coef,
> > +					int temp_code)
> > +{
> > +	int temp, temp1, temp2;
> > +
> > +	temp1 = MCELSIUS((CODETSD(temp_code) - coef->b1)) / coef->a1;
> > +	temp2 = MCELSIUS((CODETSD(temp_code) - coef->b2)) / coef->a2;
> 
> aren't we overflowing the result of this 64 bit math assigned into an int?

The division ensures that we get an int. Hmmm, not very pretty, I agree.

Sigh, maybe it is better to refactor the formulas before submitting
upstream :/

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]