* [PATCH 0/1] Document multiple fan tach support in pwm-fan driver
@ 2020-09-20 18:09 Paul Barker
2020-09-20 18:09 ` [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs Paul Barker
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Paul Barker @ 2020-09-20 18:09 UTC (permalink / raw
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
Guenter Roeck, Rob Herring
Cc: Paul Barker, linux-hwmon, devicetree
This patch updates the device tree bindings to match the changes I'm
about to send for the pwm-fan driver. Those changes will extend the
pwm-fan driver to support multiple fan tachometer inputs.
These changes can also be pulled from:
https://gitlab.com/pbarker.dev/staging/linux.git
tag: for-dt-next/pwm-fan_2020-09-20
Paul Barker (1):
dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs
.../devicetree/bindings/hwmon/pwm-fan.txt | 28 +++++++++++++------
1 file changed, 19 insertions(+), 9 deletions(-)
base-commit: 73f76a41c4ed7def5dc2ec7c33c7e9f94e601a20
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs
2020-09-20 18:09 [PATCH 0/1] Document multiple fan tach support in pwm-fan driver Paul Barker
@ 2020-09-20 18:09 ` Paul Barker
2020-09-29 17:21 ` Rob Herring
2020-11-22 15:29 ` Guenter Roeck
2020-09-20 18:09 ` [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc Paul Barker
` (2 subsequent siblings)
3 siblings, 2 replies; 10+ messages in thread
From: Paul Barker @ 2020-09-20 18:09 UTC (permalink / raw
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
Guenter Roeck, Rob Herring
Cc: Paul Barker, linux-hwmon, devicetree
Document and give an example of how to define multiple fan tachometer
inputs for the pwm-fan driver.
Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
.../devicetree/bindings/hwmon/pwm-fan.txt | 28 +++++++++++++------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
index 41b76762953a..4509e688623a 100644
--- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
@@ -8,15 +8,16 @@ Required properties:
Optional properties:
- fan-supply : phandle to the regulator that provides power to the fan
-- interrupts : This contains a single interrupt specifier which
- describes the tachometer output of the fan as an
- interrupt source. The output signal must generate a
- defined number of interrupts per fan revolution, which
- require that it must be self resetting edge interrupts.
- See interrupt-controller/interrupts.txt for the format.
-- pulses-per-revolution : define the tachometer pulses per fan revolution as
- an integer (default is 2 interrupts per revolution).
- The value must be greater than zero.
+- interrupts : This contains an interrupt specifier for each fan
+ tachometer output connected to an interrupt source.
+ The output signal must generate a defined number of
+ interrupts per fan revolution, which require that
+ it must be self resetting edge interrupts. See
+ interrupt-controller/interrupts.txt for the format.
+- pulses-per-revolution : define the number of pulses per fan revolution for
+ each tachometer input as an integer (default is 2
+ interrupts per revolution). The value must be
+ greater than zero.
Example:
fan0: pwm-fan {
@@ -55,3 +56,12 @@ Example 2:
interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
pulses-per-revolution = <2>;
};
+
+Example 3:
+ fan0: pwm-fan {
+ compatible = "pwm-fan";
+ pwms = <&pwm1 0 25000 0>;
+ interrupts-extended = <&gpio1 1 IRQ_TYPE_EDGE_FALLING>,
+ <&gpio2 5 IRQ_TYPE_EDGE_FALLING>;
+ pulses-per-revolution = <2>, <1>;
+ };
--
2.28.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc
2020-09-20 18:09 [PATCH 0/1] Document multiple fan tach support in pwm-fan driver Paul Barker
2020-09-20 18:09 ` [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs Paul Barker
@ 2020-09-20 18:09 ` Paul Barker
2020-10-04 8:49 ` Paul Barker
2020-09-20 18:09 ` [PATCH 1/2] hwmon: pwm-fan: Support multiple tachometer inputs Paul Barker
2020-09-20 18:09 ` [PATCH 2/2] hwmon: pwm-fan: Fix RPM calculation Paul Barker
3 siblings, 1 reply; 10+ messages in thread
From: Paul Barker @ 2020-09-20 18:09 UTC (permalink / raw
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
Guenter Roeck
Cc: Paul Barker, linux-hwmon
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
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] hwmon: pwm-fan: Support multiple tachometer inputs
2020-09-20 18:09 [PATCH 0/1] Document multiple fan tach support in pwm-fan driver Paul Barker
2020-09-20 18:09 ` [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs Paul Barker
2020-09-20 18:09 ` [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc Paul Barker
@ 2020-09-20 18:09 ` Paul Barker
2020-09-20 18:09 ` [PATCH 2/2] hwmon: pwm-fan: Fix RPM calculation Paul Barker
3 siblings, 0 replies; 10+ messages in thread
From: Paul Barker @ 2020-09-20 18:09 UTC (permalink / raw
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
Guenter Roeck
Cc: Paul Barker, linux-hwmon
The pwm-fan driver is extended to support multiple fan tachometer
signals connected to GPIO inputs. This is intended to support the case
where a single PWM output signal is routed to multiple fans, each of
which have a tachometer output connected back to a GPIO pin.
The number of fan tachometer inputs is determined by the number of
interrupt sources configured for the pwm-fan device. The number of
pulses-per-revolution entries should match the number of interrupt
sources so that each input has a value assigned.
The fan tachometer measurements are exposed as sysfs files fan1_input,
fan2_input, etc up to the number of configured inputs. If no inputs are
configured then no corresponding sysfs files will be created. This
avoids the need for an is_visible function to hide unused fanN_input
sysfs files.
Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
drivers/hwmon/pwm-fan.c | 165 ++++++++++++++++++++++++----------------
1 file changed, 101 insertions(+), 64 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index bdba2143021a..d7f8c11b4543 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -22,15 +22,22 @@
#define MAX_PWM 255
+struct pwm_fan_tach {
+ int irq;
+ atomic_t pulses;
+ unsigned int rpm;
+ u8 pulses_per_revolution;
+
+ struct sensor_device_attribute sensor_attr;
+};
+
struct pwm_fan_ctx {
struct mutex lock;
struct pwm_device *pwm;
struct regulator *reg_en;
- int irq;
- atomic_t pulses;
- unsigned int rpm;
- u8 pulses_per_revolution;
+ int tach_count;
+ struct pwm_fan_tach *tachs;
ktime_t sample_start;
struct timer_list rpm_timer;
@@ -39,14 +46,17 @@ struct pwm_fan_ctx {
unsigned int pwm_fan_max_state;
unsigned int *pwm_fan_cooling_levels;
struct thermal_cooling_device *cdev;
+
+ struct attribute_group fan_group;
+ struct attribute_group *fan_groups[2];
};
/* This handler assumes self resetting edge triggered interrupt. */
static irqreturn_t pulse_handler(int irq, void *dev_id)
{
- struct pwm_fan_ctx *ctx = dev_id;
+ struct pwm_fan_tach *tach = dev_id;
- atomic_inc(&ctx->pulses);
+ atomic_inc(&tach->pulses);
return IRQ_HANDLED;
}
@@ -54,16 +64,23 @@ static irqreturn_t pulse_handler(int irq, void *dev_id)
static void sample_timer(struct timer_list *t)
{
struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
- int pulses;
- u64 tmp;
+ ktime_t now = ktime_get();
+ int i;
- pulses = atomic_read(&ctx->pulses);
- atomic_sub(pulses, &ctx->pulses);
- tmp = (u64)pulses * ktime_ms_delta(ktime_get(), ctx->sample_start) * 60;
- do_div(tmp, ctx->pulses_per_revolution * 1000);
- ctx->rpm = tmp;
+ for (i = 0; i < ctx->tach_count; i++) {
+ struct pwm_fan_tach *tach;
+ int pulses;
+ u64 tmp;
+
+ tach = &ctx->tachs[i];
+ pulses = atomic_read(&tach->pulses);
+ atomic_sub(pulses, &tach->pulses);
+ tmp = (u64)pulses * ktime_ms_delta(now, ctx->sample_start) * 60;
+ do_div(tmp, tach->pulses_per_revolution * 1000);
+ tach->rpm = tmp;
+ }
- ctx->sample_start = ktime_get();
+ ctx->sample_start = now;
mod_timer(&ctx->rpm_timer, jiffies + HZ);
}
@@ -131,41 +148,13 @@ static ssize_t rpm_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ struct pwm_fan_tach *tach = &ctx->tachs[sensor_attr->index];
- return sprintf(buf, "%u\n", ctx->rpm);
+ return sprintf(buf, "%u\n", tach->rpm);
}
static SENSOR_DEVICE_ATTR_RW(pwm1, pwm, 0);
-static SENSOR_DEVICE_ATTR_RO(fan1_input, rpm, 0);
-
-static struct attribute *pwm_fan_attrs[] = {
- &sensor_dev_attr_pwm1.dev_attr.attr,
- &sensor_dev_attr_fan1_input.dev_attr.attr,
- NULL,
-};
-
-static umode_t pwm_fan_attrs_visible(struct kobject *kobj, struct attribute *a,
- int n)
-{
- struct device *dev = container_of(kobj, struct device, kobj);
- struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
-
- /* Hide fan_input in case no interrupt is available */
- if (n == 1 && ctx->irq <= 0)
- return 0;
-
- return a->mode;
-}
-
-static const struct attribute_group pwm_fan_group = {
- .attrs = pwm_fan_attrs,
- .is_visible = pwm_fan_attrs_visible,
-};
-
-static const struct attribute_group *pwm_fan_groups[] = {
- &pwm_fan_group,
- NULL,
-};
/* thermal cooling device callbacks */
static int pwm_fan_get_max_state(struct thermal_cooling_device *cdev,
@@ -284,7 +273,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
struct device *hwmon;
int ret;
struct pwm_state state = { };
- u32 ppr = 2;
+ size_t sz;
+ unsigned int i;
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -298,10 +288,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, ctx);
- ctx->irq = platform_get_irq_optional(pdev, 0);
- if (ctx->irq == -EPROBE_DEFER)
- return ctx->irq;
-
ctx->reg_en = devm_regulator_get_optional(dev, "fan");
if (IS_ERR(ctx->reg_en)) {
if (PTR_ERR(ctx->reg_en) != -ENODEV)
@@ -337,26 +323,77 @@ static int pwm_fan_probe(struct platform_device *pdev)
if (ret)
return ret;
- of_property_read_u32(dev->of_node, "pulses-per-revolution", &ppr);
- ctx->pulses_per_revolution = ppr;
- if (!ctx->pulses_per_revolution) {
- dev_err(dev, "pulses-per-revolution can't be zero.\n");
- return -EINVAL;
- }
+ ctx->tach_count = platform_irq_count(pdev);
+ if (ctx->tach_count < 0)
+ return dev_err_probe(dev, ctx->tach_count,
+ "Could not get number of fan tachometer inputs\n");
+ dev_dbg(dev, "%d fan tachometer inputs\n", ctx->tach_count);
- if (ctx->irq > 0) {
- ret = devm_request_irq(dev, ctx->irq, pulse_handler, 0,
- pdev->name, ctx);
- if (ret) {
- dev_err(dev, "Failed to request interrupt: %d\n", ret);
- return ret;
+ sz = (2 + ctx->tach_count) * sizeof(struct attribute *);
+ ctx->fan_group.attrs = devm_kzalloc(dev, sz, GFP_KERNEL);
+ if (!ctx->fan_group.attrs)
+ return -ENOMEM;
+
+ sz = ctx->tach_count * sizeof(struct pwm_fan_tach);
+ ctx->tachs = devm_kzalloc(dev, sz, GFP_KERNEL);
+ if (!ctx->tachs)
+ return -ENOMEM;
+
+ ctx->fan_group.attrs[0] = &sensor_dev_attr_pwm1.dev_attr.attr;
+ for (i = 0; i < ctx->tach_count; i++) {
+ struct pwm_fan_tach *tach = &ctx->tachs[i];
+ u32 ppr = 2;
+ char *name;
+
+ tach->irq = platform_get_irq(pdev, i);
+ if (tach->irq == -EPROBE_DEFER)
+ return tach->irq;
+
+ of_property_read_u32_index(dev->of_node,
+ "pulses-per-revolution",
+ i,
+ &ppr);
+ tach->pulses_per_revolution = ppr;
+ if (!tach->pulses_per_revolution) {
+ dev_err(dev, "pulses-per-revolution can't be zero.\n");
+ return -EINVAL;
}
+
+ if (tach->irq > 0) {
+ ret = devm_request_irq(dev, tach->irq, pulse_handler, 0,
+ pdev->name, tach);
+ if (ret) {
+ dev_err(dev,
+ "Failed to request interrupt: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
+ sysfs_attr_init(&tach->sensor_attr.dev_attr.attr);
+
+ name = devm_kzalloc(dev, 16, GFP_KERNEL);
+ snprintf(name, 16, "fan%d_input", i + 1);
+ tach->sensor_attr.dev_attr.attr.name = name;
+ tach->sensor_attr.dev_attr.attr.mode = 0444;
+ tach->sensor_attr.dev_attr.show = rpm_show;
+ tach->sensor_attr.index = i;
+ ctx->fan_group.attrs[i + 1] = &tach->sensor_attr.dev_attr.attr;
+
+ dev_dbg(dev, "%s: irq=%d, pulses_per_revolution=%d",
+ name, tach->irq, tach->pulses_per_revolution);
+ }
+
+ if (ctx->tach_count > 0) {
ctx->sample_start = ktime_get();
mod_timer(&ctx->rpm_timer, jiffies + HZ);
}
- hwmon = devm_hwmon_device_register_with_groups(dev, "pwmfan",
- ctx, pwm_fan_groups);
+ ctx->fan_groups[0] = &ctx->fan_group;
+
+ hwmon = devm_hwmon_device_register_with_groups(dev,
+ "pwmfan", ctx,
+ (const struct attribute_group **)ctx->fan_groups);
if (IS_ERR(hwmon)) {
dev_err(dev, "Failed to register hwmon device\n");
return PTR_ERR(hwmon);
--
2.28.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] hwmon: pwm-fan: Fix RPM calculation
2020-09-20 18:09 [PATCH 0/1] Document multiple fan tach support in pwm-fan driver Paul Barker
` (2 preceding siblings ...)
2020-09-20 18:09 ` [PATCH 1/2] hwmon: pwm-fan: Support multiple tachometer inputs Paul Barker
@ 2020-09-20 18:09 ` Paul Barker
3 siblings, 0 replies; 10+ messages in thread
From: Paul Barker @ 2020-09-20 18:09 UTC (permalink / raw
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
Guenter Roeck
Cc: Paul Barker, linux-hwmon
To convert the number of pulses counted into an RPM estimation, we need
to divide by the width of our measurement interval instead of
multiplying by it.
We also don't need to do 64-bit division, with 32-bits we can handle a
fan running at over 4 million RPM.
Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
drivers/hwmon/pwm-fan.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index d7f8c11b4543..2649f6bf1a26 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -65,19 +65,18 @@ static void sample_timer(struct timer_list *t)
{
struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
ktime_t now = ktime_get();
+ unsigned int delta = ktime_ms_delta(now, ctx->sample_start);
int i;
for (i = 0; i < ctx->tach_count; i++) {
struct pwm_fan_tach *tach;
- int pulses;
- u64 tmp;
+ unsigned int pulses;
tach = &ctx->tachs[i];
pulses = atomic_read(&tach->pulses);
atomic_sub(pulses, &tach->pulses);
- tmp = (u64)pulses * ktime_ms_delta(now, ctx->sample_start) * 60;
- do_div(tmp, tach->pulses_per_revolution * 1000);
- tach->rpm = tmp;
+ tach->rpm = (pulses * 1000 * 60) /
+ (tach->pulses_per_revolution * delta);
}
ctx->sample_start = now;
--
2.28.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs
2020-09-20 18:09 ` [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs Paul Barker
@ 2020-09-29 17:21 ` Rob Herring
2020-11-22 15:29 ` Guenter Roeck
1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2020-09-29 17:21 UTC (permalink / raw
To: Paul Barker
Cc: Guenter Roeck, linux-hwmon, Kamil Debski, Jean Delvare,
Rob Herring, Bartlomiej Zolnierkiewicz, devicetree
On Sun, 20 Sep 2020 19:09:40 +0100, Paul Barker wrote:
> Document and give an example of how to define multiple fan tachometer
> inputs for the pwm-fan driver.
>
> Signed-off-by: Paul Barker <pbarker@konsulko.com>
> ---
> .../devicetree/bindings/hwmon/pwm-fan.txt | 28 +++++++++++++------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc
2020-09-20 18:09 ` [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc Paul Barker
@ 2020-10-04 8:49 ` Paul Barker
2020-10-04 15:38 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Paul Barker @ 2020-10-04 8:49 UTC (permalink / raw
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
Guenter Roeck
Cc: linux-hwmon
On Sun, 20 Sep 2020 at 19:09, Paul Barker <pbarker@konsulko.com> 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.
Thanks,
--
Paul Barker
Konsulko Group
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc
2020-10-04 8:49 ` Paul Barker
@ 2020-10-04 15:38 ` Guenter Roeck
2020-10-04 22:13 ` Paul Barker
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2020-10-04 15:38 UTC (permalink / raw
To: Paul Barker, Kamil Debski, Bartlomiej Zolnierkiewicz,
Jean Delvare
Cc: linux-hwmon
On 10/4/20 1:49 AM, Paul Barker wrote:
> On Sun, 20 Sep 2020 at 19:09, Paul Barker <pbarker@konsulko.com> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc
2020-10-04 15:38 ` Guenter Roeck
@ 2020-10-04 22:13 ` Paul Barker
0 siblings, 0 replies; 10+ messages in thread
From: Paul Barker @ 2020-10-04 22:13 UTC (permalink / raw
To: Guenter Roeck
Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
linux-hwmon
On Sun, 4 Oct 2020 at 16:38, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/4/20 1:49 AM, Paul Barker wrote:
> > On Sun, 20 Sep 2020 at 19:09, Paul Barker <pbarker@konsulko.com> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs
2020-09-20 18:09 ` [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs Paul Barker
2020-09-29 17:21 ` Rob Herring
@ 2020-11-22 15:29 ` Guenter Roeck
1 sibling, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-11-22 15:29 UTC (permalink / raw
To: Paul Barker
Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
Rob Herring, linux-hwmon, devicetree
On Sun, Sep 20, 2020 at 07:09:40PM +0100, Paul Barker wrote:
> Document and give an example of how to define multiple fan tachometer
> inputs for the pwm-fan driver.
>
> Signed-off-by: Paul Barker <pbarker@konsulko.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
Applied.
Thanks,
Guenter
> ---
> .../devicetree/bindings/hwmon/pwm-fan.txt | 28 +++++++++++++------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> index 41b76762953a..4509e688623a 100644
> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> @@ -8,15 +8,16 @@ Required properties:
>
> Optional properties:
> - fan-supply : phandle to the regulator that provides power to the fan
> -- interrupts : This contains a single interrupt specifier which
> - describes the tachometer output of the fan as an
> - interrupt source. The output signal must generate a
> - defined number of interrupts per fan revolution, which
> - require that it must be self resetting edge interrupts.
> - See interrupt-controller/interrupts.txt for the format.
> -- pulses-per-revolution : define the tachometer pulses per fan revolution as
> - an integer (default is 2 interrupts per revolution).
> - The value must be greater than zero.
> +- interrupts : This contains an interrupt specifier for each fan
> + tachometer output connected to an interrupt source.
> + The output signal must generate a defined number of
> + interrupts per fan revolution, which require that
> + it must be self resetting edge interrupts. See
> + interrupt-controller/interrupts.txt for the format.
> +- pulses-per-revolution : define the number of pulses per fan revolution for
> + each tachometer input as an integer (default is 2
> + interrupts per revolution). The value must be
> + greater than zero.
>
> Example:
> fan0: pwm-fan {
> @@ -55,3 +56,12 @@ Example 2:
> interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
> pulses-per-revolution = <2>;
> };
> +
> +Example 3:
> + fan0: pwm-fan {
> + compatible = "pwm-fan";
> + pwms = <&pwm1 0 25000 0>;
> + interrupts-extended = <&gpio1 1 IRQ_TYPE_EDGE_FALLING>,
> + <&gpio2 5 IRQ_TYPE_EDGE_FALLING>;
> + pulses-per-revolution = <2>, <1>;
> + };
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-22 15:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-20 18:09 [PATCH 0/1] Document multiple fan tach support in pwm-fan driver Paul Barker
2020-09-20 18:09 ` [PATCH 1/1] dt-bindings: hwmon: pwm-fan: Support multiple fan tachometer inputs Paul Barker
2020-09-29 17:21 ` Rob Herring
2020-11-22 15:29 ` Guenter Roeck
2020-09-20 18:09 ` [PATCH 0/2] pwm-fan: Support multiple tach inputs & fix RPM calc Paul Barker
2020-10-04 8:49 ` Paul Barker
2020-10-04 15:38 ` Guenter Roeck
2020-10-04 22:13 ` Paul Barker
2020-09-20 18:09 ` [PATCH 1/2] hwmon: pwm-fan: Support multiple tachometer inputs Paul Barker
2020-09-20 18:09 ` [PATCH 2/2] hwmon: pwm-fan: Fix RPM calculation Paul Barker
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.