On Sun, 4 Oct 2020 at 16:38, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > 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. Thanks for the feedback, I'll do some rework and re-submit. Probably just the calculation fix first and then the support for multiple fan tachometer inputs as a follow-up. -- Paul Barker Konsulko Group
