On 10/4/20 1:49 AM, Paul Barker wrote: > On Sun, 20 Sep 2020 at 19:09, Paul Barker <pbarker@xxxxxxxxxxxx> wrote: >> >> These changes were made to support a custom board where one PWM output >> is routed to two fans, each of which has a tachometer signal routed to a >> GPIO input on the SoC. While debugging on this board it was found that >> the RPM readings were being overestimated due to a bug in the >> calculation code so I've included a fix for that. >> >> As the custom board doesn't currently support the latest mainline kernel >> I've tested these changes on a SanCloud BeagleBone Enhanced using an >> oscilloscope to check the PWM output and a signal generator to simulate >> the fan tachometer signals. I've tested variants of the device tree with >> 0, 1 and 2 fan tachometer inputs configured to ensure the logic in the >> probe function is correct. >> >> The device tree bindings changes have been submitted in a separate >> patch. >> >> These changes can also be pulled from: >> >> https://gitlab.com/pbarker.dev/staging/linux.git >> tag: for-hwmon/pwm-fan_2020-09-20 >> >> Paul Barker (2): >> hwmon: pwm-fan: Support multiple tachometer inputs >> hwmon: pwm-fan: Fix RPM calculation >> >> drivers/hwmon/pwm-fan.c | 164 ++++++++++++++++++++++++---------------- >> 1 file changed, 100 insertions(+), 64 deletions(-) >> >> >> base-commit: 2835d860d3fcc3e4829e96987544e811d35dee48 > > Ping on these, I've not had any feedback for 2 weeks. > Unfortunately the patches are a bit difficult to understand, and I did not have the time to fully analyze them. One thing I can say is that the order should be reversed - the bug fix should come first. Also, patch 2 adds a potential divide by 0 problem, if 'delta' happens to be 0. There are little details, such as replacing 'int pulses' with 'unsigned int pulses', which make things more difficult to review. atomic_t is int. Assigning it to an unsigned int _should_ not matter here, but again such little changes make it more difficult to review a patch because they require extra scrutiny. Guenter
