Re: [PATCH v3 1/1] hwmon: (mlxreg-fan) Add support for Mellanox FAN driver

Guenter Roeck
2018-07-02T16:14:18Z

On Mon, Jul 02, 2018 at 02:33:50PM +0000, Vadim Pasternak wrote:
> >
> > Also, the usage of "round" is "100 + (r)". A value of 0 is no problem.
>
> Hi Guenter,
>
> This is 15000000 / ((val) * (d) / 100 + (r))).
> Value is reading from the register ( >=0) , in default case should be:
> regval * 1132 / 100 + 500;
>
> > A value of -100 is problematic. Which makes me wonder - what is the point of
> > the offset ? And why "round" ? This looks like a fractional divider to me, where
> > d(real) = d / (100 + (r))
> > It might be useful to explain that somewhere, and use a better variable name
> > than 'round' to describe the fraction.
>
> I will change round to fraction and will add a comment before macros
> MLXREG_FAN_GET_RPM. Something like below:
> /*
> * FAN datasheet defines the formula for RPM calculations as RPM = 15/t-high.
> * The logic in a programmable device measures the time t-high by sampling the
> * tachometer every t-sample (with the default value 11.32 uS) and increment
> * a counter (N) as long as the pulse has not change:
> * RPM = 15 / (t-sample * (K + Regval)), where:
> * Regval: is the value read from the programmable device register;
> * - 0xff - represents tachometer fault;
> * - 0xfe - represents tachometer minimum value , which is 4444 RPM;
> * - 0x00 - represents tachometer maximum value , which is 300000 RPM;
> * K: is 44 and it represents the minimum allowed samples per pulse;
> * F: is equal K * t-sample (44 * 11.32 = ~500) and it represents a minimum
> * fraction in RPM calculation;
> * N: is equal K + Regval;
> * In order to calculate RPM from the register value the following formula is
> * used: RPM = 15 / ((Regval * 11.32 + F) * 10^(-6)), which in the default
> * case is modified to:
> * RPM = 15000000 / ((Regval * 1132) / 100 + 500);
> * - for Regval 0x00, RPM will be 15000000 / 500 = 30000;
> * - for Regval 0xfe, RPM will be 15000000 / ((254 * 1132) / 100 + 500) = 4444;
> * In common case the formula is modified to:
> * RPM = 15000000 / ((Regval * divider) / 100 + fraction).
> */
>
> Would it be OK?
>
If F = K * t-sample, and K == 44, F also includes t-sample and is thus partially
redundant. If K is a constant, you could write

rpm = 15000000 / DIV_ROUND_CLOSEST(((regval) + 44) * (d), 100);

If K is not constant, you could provide K as parameter.

rpm = 15000000 / DIV_ROUND_CLOSEST(((regval) + (K)) * (d), 100);

Using your examples:

15000000 / (((0 + 44) * 1132) / 100) = 30120
15000000 / (((254 + 44) * 1132) / 100) = 4447

You could also use

rpm = 15000000 * 100 / (((regval) + (K)) * (d)));
or
rpm = DIV_ROUND_CLOSEST(15000000 * 100, ((regval) + (K)) * (d));

which would probably generate even more accurate results and at the same time
simplify the equation.

Again with your examples:

rpm = 15000000 * 100 / ((0 + 44) * 1132) = 30115
rpm = 15000000 * 100 / ((254 + 44) * 1132) = 4446

Guenter