Re: [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux