* [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs
@ 2020-11-13 15:08 Paul Barker
2020-11-13 15:08 ` [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe Paul Barker
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Paul Barker @ 2020-11-13 15:08 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.
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
series:
https://lore.kernel.org/linux-devicetree/20200920180943.352526-1-pbarker@konsulko.com/
These changes can also be pulled from:
https://gitlab.com/pbarker.dev/staging/linux.git
tag: for-hwmon/pwm-fan-tachometers-v2_2020-11-13
Changes since v1:
* Split RPM calculation fix into a separate patch which has now been
accepted.
* Break the changes down into smaller patches so they're easier to
review.
* Rebased changes on hwmon-next.
Paul Barker (4):
hwmon: pwm-fan: Refactor pwm_fan_probe
hwmon: pwm-fan: Dynamically setup attribute groups
hwmon: pwm-fan: Store tach data separately
hwmon: pwm-fan: Support multiple fan tachometers
drivers/hwmon/pwm-fan.c | 155 ++++++++++++++++++++++++----------------
1 file changed, 95 insertions(+), 60 deletions(-)
base-commit: 414920a4a5d5613e4aa77c89944f9c1dc86b06c4
--
2.29.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe
2020-11-13 15:08 [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
@ 2020-11-13 15:08 ` Paul Barker
2020-11-26 1:45 ` Guenter Roeck
2020-11-13 15:08 ` [PATCH v2 2/4] hwmon: pwm-fan: Dynamically setup attribute groups Paul Barker
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Paul Barker @ 2020-11-13 15:08 UTC (permalink / raw)
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck
Cc: Paul Barker, linux-hwmon
Use platform_irq_count to determine the number of fan tachometer inputs
configured in the device tree. At this stage we support either 0 or 1
inputs.
Once we have this information we only need to read the
pulses-per-revolution value if a fan tachometer is actually configured
via an IRQ value.
Also add a debug print of the IRQ number and the pulses-per-revolution
value to aid in investigating issues.
Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
drivers/hwmon/pwm-fan.c | 50 +++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 1f63807c0399..efe2764f42d3 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -286,7 +286,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
struct device *hwmon;
int ret;
struct pwm_state state = { };
- u32 ppr = 2;
+ int tach_count;
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -300,10 +300,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)
@@ -339,20 +335,40 @@ 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;
- }
+ tach_count = platform_irq_count(pdev);
+ if (tach_count < 0)
+ return dev_err_probe(dev, tach_count,
+ "Could not get number of fan tachometer inputs\n");
+
+ if (tach_count > 0) {
+ u32 ppr = 2;
+
+ ctx->irq = platform_get_irq(pdev, 0);
+ if (ctx->irq == -EPROBE_DEFER)
+ return ctx->irq;
+ 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;
+ }
+ }
- 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;
+ 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;
}
+
+ dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
+ ctx->irq, ctx->pulses_per_revolution);
+
ctx->sample_start = ktime_get();
mod_timer(&ctx->rpm_timer, jiffies + HZ);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] hwmon: pwm-fan: Dynamically setup attribute groups
2020-11-13 15:08 [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
2020-11-13 15:08 ` [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe Paul Barker
@ 2020-11-13 15:08 ` Paul Barker
[not found] ` <20201126014500.GC111386@roeck-us.net>
2020-11-13 15:08 ` [PATCH v2 3/4] hwmon: pwm-fan: Store tach data separately Paul Barker
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Paul Barker @ 2020-11-13 15:08 UTC (permalink / raw)
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck
Cc: Paul Barker, linux-hwmon
Instead of implementing an is_visible function we can dynamically
populate the attribute group for the device based on whether a fan
tachometer input is configured or not.
Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
drivers/hwmon/pwm-fan.c | 55 +++++++++++++++++------------------------
1 file changed, 23 insertions(+), 32 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index efe2764f42d3..c4e0059ccaec 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -31,6 +31,8 @@ struct pwm_fan_ctx {
atomic_t pulses;
unsigned int rpm;
u8 pulses_per_revolution;
+ struct sensor_device_attribute sensor_attr;
+
ktime_t sample_start;
struct timer_list rpm_timer;
@@ -39,6 +41,9 @@ 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. */
@@ -138,36 +143,6 @@ static ssize_t rpm_show(struct device *dev,
}
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,
@@ -287,6 +262,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
int ret;
struct pwm_state state = { };
int tach_count;
+ size_t sz;
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -340,6 +316,13 @@ static int pwm_fan_probe(struct platform_device *pdev)
return dev_err_probe(dev, tach_count,
"Could not get number of fan tachometer inputs\n");
+ sz = (2 + tach_count) * sizeof(struct attribute *);
+ ctx->fan_group.attrs = devm_kzalloc(dev, sz, GFP_KERNEL);
+ if (!ctx->fan_group.attrs)
+ return -ENOMEM;
+
+ ctx->fan_group.attrs[0] = &sensor_dev_attr_pwm1.dev_attr.attr;
+
if (tach_count > 0) {
u32 ppr = 2;
@@ -366,6 +349,13 @@ static int pwm_fan_probe(struct platform_device *pdev)
return -EINVAL;
}
+ sysfs_attr_init(&ctx->sensor_attr.dev_attr.attr);
+
+ ctx->sensor_attr.dev_attr.attr.name = "fan1_input";
+ ctx->sensor_attr.dev_attr.attr.mode = 0444;
+ ctx->sensor_attr.dev_attr.show = rpm_show;
+ ctx->fan_group.attrs[1] = &ctx->sensor_attr.dev_attr.attr;
+
dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
ctx->irq, ctx->pulses_per_revolution);
@@ -373,8 +363,9 @@ static int pwm_fan_probe(struct platform_device *pdev)
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.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] hwmon: pwm-fan: Store tach data separately
2020-11-13 15:08 [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
2020-11-13 15:08 ` [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe Paul Barker
2020-11-13 15:08 ` [PATCH v2 2/4] hwmon: pwm-fan: Dynamically setup attribute groups Paul Barker
@ 2020-11-13 15:08 ` Paul Barker
2020-11-13 15:08 ` [PATCH v2 4/4] hwmon: pwm-fan: Support multiple fan tachometers Paul Barker
2020-11-25 16:45 ` [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
4 siblings, 0 replies; 9+ messages in thread
From: Paul Barker @ 2020-11-13 15:08 UTC (permalink / raw)
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck
Cc: Paul Barker, linux-hwmon
The data for the (optional) fan tachometer input is moved to a separate
structure which is only allocated if an input is actually configured.
After this change the pulse IRQ handler takes a pointer to the
tachometer data structure instead of the whole device context.
Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
drivers/hwmon/pwm-fan.c | 66 +++++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 26 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index c4e0059ccaec..7c75ce78b36a 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -22,17 +22,21 @@
#define MAX_PWM 255
-struct pwm_fan_ctx {
- struct mutex lock;
- struct pwm_device *pwm;
- struct regulator *reg_en;
-
+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;
+
+ struct pwm_fan_tach *tach;
ktime_t sample_start;
struct timer_list rpm_timer;
@@ -49,9 +53,9 @@ struct pwm_fan_ctx {
/* 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;
}
@@ -59,14 +63,15 @@ 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);
+ struct pwm_fan_tach *tach = ctx->tach;
unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start);
int pulses;
if (delta) {
- pulses = atomic_read(&ctx->pulses);
- atomic_sub(pulses, &ctx->pulses);
- ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
- (ctx->pulses_per_revolution * delta);
+ pulses = atomic_read(&tach->pulses);
+ atomic_sub(pulses, &tach->pulses);
+ tach->rpm = (unsigned int)(pulses * 1000 * 60) /
+ (tach->pulses_per_revolution * delta);
ctx->sample_start = ktime_get();
}
@@ -138,8 +143,9 @@ static ssize_t rpm_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+ struct pwm_fan_tach *tach = ctx->tach;
- return sprintf(buf, "%u\n", ctx->rpm);
+ return sprintf(buf, "%u\n", tach->rpm);
}
static SENSOR_DEVICE_ATTR_RW(pwm1, pwm, 0);
@@ -324,14 +330,22 @@ static int pwm_fan_probe(struct platform_device *pdev)
ctx->fan_group.attrs[0] = &sensor_dev_attr_pwm1.dev_attr.attr;
if (tach_count > 0) {
+ struct pwm_fan_tach *tach;
u32 ppr = 2;
- ctx->irq = platform_get_irq(pdev, 0);
- if (ctx->irq == -EPROBE_DEFER)
- return ctx->irq;
- if (ctx->irq > 0) {
- ret = devm_request_irq(dev, ctx->irq, pulse_handler, 0,
- pdev->name, ctx);
+ ctx->tach = devm_kzalloc(dev, sizeof(struct pwm_fan_tach),
+ GFP_KERNEL);
+ if (!ctx->tach)
+ return -ENOMEM;
+
+ tach = ctx->tach;
+
+ tach->irq = platform_get_irq(pdev, 0);
+ if (tach->irq == -EPROBE_DEFER)
+ return tach->irq;
+ 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",
@@ -343,21 +357,21 @@ static int pwm_fan_probe(struct platform_device *pdev)
of_property_read_u32(dev->of_node,
"pulses-per-revolution",
&ppr);
- ctx->pulses_per_revolution = ppr;
- if (!ctx->pulses_per_revolution) {
+ tach->pulses_per_revolution = ppr;
+ if (!tach->pulses_per_revolution) {
dev_err(dev, "pulses-per-revolution can't be zero.\n");
return -EINVAL;
}
- sysfs_attr_init(&ctx->sensor_attr.dev_attr.attr);
+ sysfs_attr_init(&tach->sensor_attr.dev_attr.attr);
- ctx->sensor_attr.dev_attr.attr.name = "fan1_input";
- ctx->sensor_attr.dev_attr.attr.mode = 0444;
- ctx->sensor_attr.dev_attr.show = rpm_show;
- ctx->fan_group.attrs[1] = &ctx->sensor_attr.dev_attr.attr;
+ tach->sensor_attr.dev_attr.attr.name = "fan1_input";
+ tach->sensor_attr.dev_attr.attr.mode = 0444;
+ tach->sensor_attr.dev_attr.show = rpm_show;
+ ctx->fan_group.attrs[1] = &tach->sensor_attr.dev_attr.attr;
dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
- ctx->irq, ctx->pulses_per_revolution);
+ tach->irq, tach->pulses_per_revolution);
ctx->sample_start = ktime_get();
mod_timer(&ctx->rpm_timer, jiffies + HZ);
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] hwmon: pwm-fan: Support multiple fan tachometers
2020-11-13 15:08 [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
` (2 preceding siblings ...)
2020-11-13 15:08 ` [PATCH v2 3/4] hwmon: pwm-fan: Store tach data separately Paul Barker
@ 2020-11-13 15:08 ` Paul Barker
2020-11-25 16:45 ` [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
4 siblings, 0 replies; 9+ messages in thread
From: Paul Barker @ 2020-11-13 15:08 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.
Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
drivers/hwmon/pwm-fan.c | 72 ++++++++++++++++++++++++-----------------
1 file changed, 43 insertions(+), 29 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7c75ce78b36a..03a3b57e2d99 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -36,7 +36,8 @@ struct pwm_fan_ctx {
struct pwm_device *pwm;
struct regulator *reg_en;
- struct pwm_fan_tach *tach;
+ int tach_count;
+ struct pwm_fan_tach *tachs;
ktime_t sample_start;
struct timer_list rpm_timer;
@@ -63,15 +64,19 @@ 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);
- struct pwm_fan_tach *tach = ctx->tach;
unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start);
- int pulses;
+ int i;
if (delta) {
- pulses = atomic_read(&tach->pulses);
- atomic_sub(pulses, &tach->pulses);
- tach->rpm = (unsigned int)(pulses * 1000 * 60) /
- (tach->pulses_per_revolution * delta);
+ for (i = 0; i < ctx->tach_count; i++) {
+ struct pwm_fan_tach *tach = &ctx->tachs[i];
+ int pulses;
+
+ pulses = atomic_read(&tach->pulses);
+ atomic_sub(pulses, &tach->pulses);
+ tach->rpm = (unsigned int)(pulses * 1000 * 60) /
+ (tach->pulses_per_revolution * delta);
+ }
ctx->sample_start = ktime_get();
}
@@ -143,7 +148,8 @@ static ssize_t rpm_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
- struct pwm_fan_tach *tach = ctx->tach;
+ 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", tach->rpm);
}
@@ -267,8 +273,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
struct device *hwmon;
int ret;
struct pwm_state state = { };
- int tach_count;
size_t sz;
+ int i;
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -317,30 +323,32 @@ static int pwm_fan_probe(struct platform_device *pdev)
if (ret)
return ret;
- tach_count = platform_irq_count(pdev);
- if (tach_count < 0)
- return dev_err_probe(dev, tach_count,
+ 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);
- sz = (2 + tach_count) * sizeof(struct attribute *);
+ 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;
ctx->fan_group.attrs[0] = &sensor_dev_attr_pwm1.dev_attr.attr;
- if (tach_count > 0) {
- struct pwm_fan_tach *tach;
- u32 ppr = 2;
-
- ctx->tach = devm_kzalloc(dev, sizeof(struct pwm_fan_tach),
- GFP_KERNEL);
- if (!ctx->tach)
+ if (ctx->tach_count > 0) {
+ sz = ctx->tach_count * sizeof(struct pwm_fan_tach);
+ ctx->tachs = devm_kzalloc(dev, sz, GFP_KERNEL);
+ if (!ctx->tachs)
return -ENOMEM;
+ }
- tach = ctx->tach;
+ 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, 0);
+ tach->irq = platform_get_irq(pdev, i);
if (tach->irq == -EPROBE_DEFER)
return tach->irq;
if (tach->irq > 0) {
@@ -354,9 +362,10 @@ static int pwm_fan_probe(struct platform_device *pdev)
}
}
- of_property_read_u32(dev->of_node,
- "pulses-per-revolution",
- &ppr);
+ 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");
@@ -365,14 +374,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
sysfs_attr_init(&tach->sensor_attr.dev_attr.attr);
- tach->sensor_attr.dev_attr.attr.name = "fan1_input";
+ 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;
- ctx->fan_group.attrs[1] = &tach->sensor_attr.dev_attr.attr;
+ tach->sensor_attr.index = i;
+ ctx->fan_group.attrs[i + 1] = &tach->sensor_attr.dev_attr.attr;
- dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
- tach->irq, tach->pulses_per_revolution);
+ dev_dbg(dev, "%s: irq=%d, pulses_per_revolution=%d\n",
+ name, tach->irq, tach->pulses_per_revolution);
+ }
+ if (ctx->tach_count > 0) {
ctx->sample_start = ktime_get();
mod_timer(&ctx->rpm_timer, jiffies + HZ);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs
2020-11-13 15:08 [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
` (3 preceding siblings ...)
2020-11-13 15:08 ` [PATCH v2 4/4] hwmon: pwm-fan: Support multiple fan tachometers Paul Barker
@ 2020-11-25 16:45 ` Paul Barker
2020-11-25 22:27 ` Guenter Roeck
4 siblings, 1 reply; 9+ messages in thread
From: Paul Barker @ 2020-11-25 16:45 UTC (permalink / raw)
To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck
Cc: linux-hwmon, Dongjin Kim
On Fri, 13 Nov 2020 at 15: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.
>
> 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
> series:
> https://lore.kernel.org/linux-devicetree/20200920180943.352526-1-pbarker@konsulko.com/
>
> These changes can also be pulled from:
>
> https://gitlab.com/pbarker.dev/staging/linux.git
> tag: for-hwmon/pwm-fan-tachometers-v2_2020-11-13
>
> Changes since v1:
>
> * Split RPM calculation fix into a separate patch which has now been
> accepted.
>
> * Break the changes down into smaller patches so they're easier to
> review.
>
> * Rebased changes on hwmon-next.
>
> Paul Barker (4):
> hwmon: pwm-fan: Refactor pwm_fan_probe
> hwmon: pwm-fan: Dynamically setup attribute groups
> hwmon: pwm-fan: Store tach data separately
> hwmon: pwm-fan: Support multiple fan tachometers
>
> drivers/hwmon/pwm-fan.c | 155 ++++++++++++++++++++++++----------------
> 1 file changed, 95 insertions(+), 60 deletions(-)
>
>
> base-commit: 414920a4a5d5613e4aa77c89944f9c1dc86b06c4
> --
> 2.29.2
>
Has anyone had a chance to look at this series? I see that the
corresponding dt-bindings change has been accepted into the hwmon-next
tree but I've not heard anything back on these patches.
I also see that a patch just got sent to this list which will conflict
with this series
(https://lore.kernel.org/linux-hwmon/20201125163242.GA1264232@paju/T/#u).
It'd be good to get feedback so that either myself, Dongjin Kim or
both of us can re-work our patches to be compatible.
Thanks,
--
Paul Barker
Konsulko Group
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs
2020-11-25 16:45 ` [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
@ 2020-11-25 22:27 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2020-11-25 22:27 UTC (permalink / raw)
To: Paul Barker
Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare,
linux-hwmon, Dongjin Kim
On Wed, Nov 25, 2020 at 04:45:27PM +0000, Paul Barker wrote:
> On Fri, 13 Nov 2020 at 15: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.
> >
> > 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
> > series:
> > https://lore.kernel.org/linux-devicetree/20200920180943.352526-1-pbarker@konsulko.com/
> >
> > These changes can also be pulled from:
> >
> > https://gitlab.com/pbarker.dev/staging/linux.git
> > tag: for-hwmon/pwm-fan-tachometers-v2_2020-11-13
> >
> > Changes since v1:
> >
> > * Split RPM calculation fix into a separate patch which has now been
> > accepted.
> >
> > * Break the changes down into smaller patches so they're easier to
> > review.
> >
> > * Rebased changes on hwmon-next.
> >
> > Paul Barker (4):
> > hwmon: pwm-fan: Refactor pwm_fan_probe
> > hwmon: pwm-fan: Dynamically setup attribute groups
> > hwmon: pwm-fan: Store tach data separately
> > hwmon: pwm-fan: Support multiple fan tachometers
> >
> > drivers/hwmon/pwm-fan.c | 155 ++++++++++++++++++++++++----------------
> > 1 file changed, 95 insertions(+), 60 deletions(-)
> >
> >
> > base-commit: 414920a4a5d5613e4aa77c89944f9c1dc86b06c4
> > --
> > 2.29.2
> >
>
> Has anyone had a chance to look at this series? I see that the
> corresponding dt-bindings change has been accepted into the hwmon-next
> tree but I've not heard anything back on these patches.
>
Still on my list. Sorry, I have been buried in work, and being stuck in
the middle of nowhere (aka Wyoming) last week with a broken charge port
on a Tesla didn't help.
> I also see that a patch just got sent to this list which will conflict
> with this series
> (https://lore.kernel.org/linux-hwmon/20201125163242.GA1264232@paju/T/#u).
> It'd be good to get feedback so that either myself, Dongjin Kim or
> both of us can re-work our patches to be compatible.
>
I have seen it. I hope I'll get to your series over the weekend.
Sorry for the delay.
Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe
2020-11-13 15:08 ` [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe Paul Barker
@ 2020-11-26 1:45 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2020-11-26 1:45 UTC (permalink / raw)
To: Paul Barker
Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, linux-hwmon
On Fri, Nov 13, 2020 at 03:08:50PM +0000, Paul Barker wrote:
> Use platform_irq_count to determine the number of fan tachometer inputs
> configured in the device tree. At this stage we support either 0 or 1
> inputs.
>
> Once we have this information we only need to read the
> pulses-per-revolution value if a fan tachometer is actually configured
> via an IRQ value.
>
> Also add a debug print of the IRQ number and the pulses-per-revolution
> value to aid in investigating issues.
>
> Signed-off-by: Paul Barker <pbarker@konsulko.com>
For my reference:
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/hwmon/pwm-fan.c | 50 +++++++++++++++++++++++++++--------------
> 1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 1f63807c0399..efe2764f42d3 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -286,7 +286,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
> struct device *hwmon;
> int ret;
> struct pwm_state state = { };
> - u32 ppr = 2;
> + int tach_count;
>
> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> @@ -300,10 +300,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)
> @@ -339,20 +335,40 @@ 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;
> - }
> + tach_count = platform_irq_count(pdev);
> + if (tach_count < 0)
> + return dev_err_probe(dev, tach_count,
> + "Could not get number of fan tachometer inputs\n");
> +
> + if (tach_count > 0) {
> + u32 ppr = 2;
> +
> + ctx->irq = platform_get_irq(pdev, 0);
> + if (ctx->irq == -EPROBE_DEFER)
> + return ctx->irq;
> + 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;
> + }
> + }
>
> - 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;
> + 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;
> }
> +
> + dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
> + ctx->irq, ctx->pulses_per_revolution);
> +
> ctx->sample_start = ktime_get();
> mod_timer(&ctx->rpm_timer, jiffies + HZ);
> }
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] hwmon: pwm-fan: Dynamically setup attribute groups
[not found] ` <20201126014500.GC111386@roeck-us.net>
@ 2020-11-26 9:07 ` Paul Barker
0 siblings, 0 replies; 9+ messages in thread
From: Paul Barker @ 2020-11-26 9:07 UTC (permalink / raw)
To: Guenter Roeck
Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, linux-hwmon
On Thu, 26 Nov 2020 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Nov 13, 2020 at 03:08:51PM +0000, Paul Barker wrote:
> > Instead of implementing an is_visible function we can dynamically
> > populate the attribute group for the device based on whether a fan
> > tachometer input is configured or not.
> >
> > Signed-off-by: Paul Barker <pbarker@konsulko.com>
>
> This will make it more difficult to ever convert the driver to use the
> devm_hwmon_device_register_with_info() API. I'd rather see a conversion
> to that API now instead of making it more difficult to implement in the
> future.
Ah I hadn't realised that was the preferred API. I'll take a look and
see if I can convert the driver, perhaps as a standalone patch so we
can get this right before I re-work my implementation of multiple
tachometer input support.
Thanks,
--
Paul Barker
Konsulko Group
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-26 9:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-13 15:08 [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
2020-11-13 15:08 ` [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe Paul Barker
2020-11-26 1:45 ` Guenter Roeck
2020-11-13 15:08 ` [PATCH v2 2/4] hwmon: pwm-fan: Dynamically setup attribute groups Paul Barker
[not found] ` <20201126014500.GC111386@roeck-us.net>
2020-11-26 9:07 ` Paul Barker
2020-11-13 15:08 ` [PATCH v2 3/4] hwmon: pwm-fan: Store tach data separately Paul Barker
2020-11-13 15:08 ` [PATCH v2 4/4] hwmon: pwm-fan: Support multiple fan tachometers Paul Barker
2020-11-25 16:45 ` [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs Paul Barker
2020-11-25 22:27 ` Guenter Roeck
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.