On Wed, Dec 07, 2016 at 10:06:27AM +0100, Wolfram Sang wrote:
>
> > 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.
at least those that make sense.
>
> > 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.
cool
>
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>
> Is this mandatory?
not really.
>
> > 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.
>
still good to explicitly write it down to prevent future changes to
abuse the lock.
> > 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.
>
I typically ask to keep checkpatch as clean as possible. The above can
easily be avoided by returning right after static inline.
> > > +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 :/
>
cool, I am checking on the next version.
> Regards,
>
> Wolfram
>
BR,
Eduardo Valentin
