Re: [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt
From: Stefan Wahren
Date: Wed Apr 03 2019 - 12:23:50 EST
Am 03.04.2019 um 17:59 schrieb Robin Murphy:
On 03/04/2019 10:55, Stefan Wahren wrote:
Hi Guenter,
Am 02.04.19 um 22:55 schrieb Guenter Roeck:
On Tue, Apr 02, 2019 at 04:21:50PM +0200, Stefan Wahren wrote:
This adds RPM support to the pwm-fan driver in order to use with
fancontrol/pwmconfig. This feature is intended for fans with a
tachometer
output signal, which generate a defined number of pulses per
revolution.
Signed-off-by: Stefan Wahren <stefan.wahren@xxxxxxxx>
---
 drivers/hwmon/pwm-fan.c | 111
++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 167221c..3245a49 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -18,6 +18,7 @@
  #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
@@ -26,6 +27,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/sysfs.h>
 #include <linux/thermal.h>
+#include <linux/timer.h>
  #define MAX_PWM 255
 @@ -33,6 +35,14 @@ 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;
+ÂÂÂ ktime_t sample_start;
+ÂÂÂ struct timer_list rpm_timer;
+
ÂÂÂÂÂ unsigned int pwm_value;
ÂÂÂÂÂ unsigned int pwm_fan_state;
ÂÂÂÂÂ unsigned int pwm_fan_max_state;
@@ -40,6 +50,32 @@ struct pwm_fan_ctx {
ÂÂÂÂÂ struct thermal_cooling_device *cdev;
 };
 +/* 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;
+
+ÂÂÂ atomic_inc(&ctx->pulses);
+
+ÂÂÂ return IRQ_HANDLED;
+}
+
+static void sample_timer(struct timer_list *t)
+{
+ÂÂÂ struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
+ÂÂÂ int pulses;
+ÂÂÂ u64 tmp;
+
+ÂÂÂ 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;
+
+ÂÂÂ ctx->sample_start = ktime_get();
+ÂÂÂ mod_timer(&ctx->rpm_timer, jiffies + HZ);
+}
+
 static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 {
ÂÂÂÂÂ unsigned long period;
@@ -100,15 +136,49 @@ static ssize_t pwm_show(struct device *dev,
struct device_attribute *attr,
ÂÂÂÂÂ return sprintf(buf, "%u\n", ctx->pwm_value);
 }
 +static ssize_t rpm_show(struct device *dev,
+ÂÂÂÂÂÂÂÂÂÂÂ struct device_attribute *attr, char *buf)
+{
+ÂÂÂ struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+
+ÂÂÂ return sprintf(buf, "%u\n", ctx->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,
 };
 -ATTRIBUTE_GROUPS(pwm_fan);
+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);
+ÂÂÂ struct device_attribute *devattr;
+
+ /* Hide fan_input in case no interrupt is available */
+ÂÂÂ devattr = container_of(a, struct device_attribute, attr);
+ÂÂÂ if (devattr == &sensor_dev_attr_fan1_input.dev_attr) {
+ÂÂÂÂÂÂÂ if (ctx->irq <= 0)
+ÂÂÂÂÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
Side note: This can be easier written as
ÂÂÂÂif (n == 1 && ctx->irq <= 0)
ÂÂÂÂÂÂÂ return 0;
Not that it matters much.
+
+ÂÂÂ 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,
@@ -261,17 +331,45 @@ static int pwm_fan_probe(struct
platform_device *pdev)
ÂÂÂÂÂÂÂÂÂ goto err_reg_disable;
ÂÂÂÂÂ }
 + timer_setup(&ctx->rpm_timer, sample_timer, 0);
+
+ÂÂÂ if (of_property_read_u8(pdev->dev.of_node,
"pulses-per-revolution",
This does not work: The property is not defined as u8. You have to
either
use of_property_read_u32() or declare the property as u8.
pulses_per_revolution is defined as u8 since this version
The variable might be, but the "pulses-per-revolution" property itself
is not being defined with the appropriate DT type ("/bits/ 8") in the
binding, and thus will be stored as a regular 32-bit cell, for which
reading it as a u8 array may or may not work correctly depending on
endianness.
TBH, unless there's a real need for a specific binary format in the
FDT, I don't think it's usually worth the bother of using irregular DT
types, especially when the practical impact amounts to possibly saving
up to 3 bytes for a property which usually won't need to be specified
anyway. I'd just do something like:
ÂÂÂÂu32 ppr = 2;
ÂÂÂÂof_property_read_u32(np, "pulses-per-revolution", &ppr);
ÂÂÂÂctx->pulses_per_revolution = ppr;
My intention was to avoid another overflow in case the device tree
provides unrealistic values ( my expected range 1 - 10 ). Saving space
would be a benefit, but i'm okay with this suggestion.
[ Sorry, I didn't know until recently that this is necessary ]
+ &ctx->pulses_per_revolution)) {
+ÂÂÂÂÂÂÂ ctx->pulses_per_revolution = 2;
+ÂÂÂ }
+
+ÂÂÂ if (!ctx->pulses_per_revolution) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "pulses-per-revolution can't be
zero.\n");
+ÂÂÂÂÂÂÂ ret = -EINVAL;
+ÂÂÂÂÂÂÂ goto err_pwm_disable;
+ÂÂÂ }
+
+ÂÂÂ ctx->irq = platform_get_irq(pdev, 0);
+ÂÂÂ if (ctx->irq == -EPROBE_DEFER) {
+ÂÂÂÂÂÂÂ ret = ctx->irq;
+ÂÂÂÂÂÂÂ goto err_pwm_disable;
It might be better to call platform_get_irq() and to do do this check
first, before enabling the regulator (in practice before calling
devm_regulator_get_optional). It doesn't make sense to enable the
regulator only to disable it because the irq is not yet available.
+ÂÂÂ } else if (ctx->irq > 0) {
As written, this else is unnecessary, and static checkers will complain
about it.
+ÂÂÂÂÂÂÂ ret = devm_request_irq(&pdev->dev, ctx->irq,
pulse_handler, 0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pdev->name, ctx);
+ÂÂÂÂÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Can't get interrupt working.\n");
+ÂÂÂÂÂÂÂÂÂÂÂ goto err_pwm_disable;
We could still continue without RPM support at this point, couldn't
we? Or is this a deliberate "if that failed, then who knows how messed
up the system is..." kind of thing?
In case someone specified an interrupt, the user expect it to work. This
helps to identify broken DT faster.
The gpio-fan also have optional irq support and also bail out if
devm_request_irq fails.
Btw i will add the return code into the error message.
Stefan