Contrary to the gpio-fan the pwm-fan driver isn't easy to setup
with pwmconfig/fancontrol because of the missing hwmon sysfs entry
for actual revolutions per minute. This series adds this feature.
This series based on the recently submitted regulator support.
Stefan Wahren (3):
dt-bindings: hwmon: Add tachometer interrupt to pwm-fan
Documentation: pwm-fan: Add description for RPM support
hwmon: pwm-fan: Add RPM support via external interrupt
.../devicetree/bindings/hwmon/pwm-fan.txt | 3 ++
Documentation/hwmon/pwm-fan | 4 ++
drivers/hwmon/pwm-fan.c | 55 ++++++++++++++++++++++
3 files changed, 62 insertions(+)
--
2.7.4
This adds a short description for the new RPM support of the pwm-fan
driver.
Signed-off-by: Stefan Wahren <[email protected]>
---
Documentation/hwmon/pwm-fan | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/hwmon/pwm-fan b/Documentation/hwmon/pwm-fan
index 18529d2..a1ba34c 100644
--- a/Documentation/hwmon/pwm-fan
+++ b/Documentation/hwmon/pwm-fan
@@ -15,3 +15,7 @@ The driver implements a simple interface for driving a fan connected to
a PWM output. It uses the generic PWM interface, thus it can be used with
a range of SoCs. The driver exposes the fan to the user space through
the hwmon's sysfs interface.
+
+The measured fan rotation speed returned via 'fan1_input' is derived
+from the sampled interrupts from the tachometer signal by assuming
+a 2 pulse-per-revolution fan.
--
2.7.4
This adds the tachometer interrupt to the pwm-fan binding, which is
necessary for RPM support.
Signed-off-by: Stefan Wahren <[email protected]>
---
Documentation/devicetree/bindings/hwmon/pwm-fan.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
index 49ca5d8..7f69b0b 100644
--- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
@@ -8,6 +8,9 @@ Required properties:
Optional properties:
- fan-supply : phandle to the regulator that provides power to the fan
+- interrupts : contains a single interrupt specifier which describes the
+ tachometer pin output of a 2 pulse-per-revolution fan.
+ See interrupt-controller/interrupts.txt for the format.
Example:
fan0: pwm-fan {
--
2.7.4
This adds RPM support to the pwm-fan driver in order to use with
fancontrol/pwmconfig. This feature is intended for 2 pulse-per-revolution
fans which provides a tachometer output signal.
Signed-off-by: Stefan Wahren <[email protected]>
---
drivers/hwmon/pwm-fan.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 9e0591e..731fdc6 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,13 +27,22 @@
#include <linux/regulator/consumer.h>
#include <linux/sysfs.h>
#include <linux/thermal.h>
+#include <linux/timer.h>
#define MAX_PWM 255
+#define SAMPLE_TIME 1 /* seconds */
+#define PULSE_PER_REVOLUTION 2
struct pwm_fan_ctx {
struct mutex lock;
struct pwm_device *pwm;
struct regulator *reg_en;
+
+ int irq;
+ unsigned int pulses;
+ unsigned int rpm;
+ struct timer_list sample_timer;
+
unsigned int pwm_value;
unsigned int pwm_fan_state;
unsigned int pwm_fan_max_state;
@@ -40,6 +50,27 @@ struct pwm_fan_ctx {
struct thermal_cooling_device *cdev;
};
+static irqreturn_t pulse_handler(int irq, void *dev_id)
+{
+ struct pwm_fan_ctx *ctx = dev_id;
+
+ if (ctx->pulses < INT_MAX / 2)
+ ctx->pulses++;
+
+ return IRQ_HANDLED;
+}
+
+static void sample_timer(struct timer_list *t)
+{
+ struct pwm_fan_ctx *ctx = from_timer(ctx, t, sample_timer);
+ unsigned int pulses;
+
+ pulses = ctx->pulses;
+ ctx->pulses -= pulses;
+ ctx->rpm = pulses * 60 / SAMPLE_TIME / PULSE_PER_REVOLUTION;
+ mod_timer(&ctx->sample_timer, jiffies + (HZ * SAMPLE_TIME));
+}
+
static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
{
unsigned long period;
@@ -100,11 +131,20 @@ 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,
};
@@ -263,6 +303,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
}
}
+ timer_setup(&ctx->sample_timer, sample_timer, 0);
+
+ ctx->irq = platform_get_irq(pdev, 0);
+ if (ctx->irq >= 0) {
+ 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");
+ return ret;
+ }
+ mod_timer(&ctx->sample_timer, jiffies + (HZ * SAMPLE_TIME));
+ }
+
hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, "pwmfan",
ctx, pwm_fan_groups);
if (IS_ERR(hwmon)) {
@@ -304,6 +357,8 @@ static int pwm_fan_remove(struct platform_device *pdev)
struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
thermal_cooling_device_unregister(ctx->cdev);
+ del_timer_sync(&ctx->sample_timer);
+
if (ctx->pwm_value)
pwm_disable(ctx->pwm);
--
2.7.4
On Wed, Jan 30, 2019 at 04:07:05PM +0100, Stefan Wahren wrote:
> This adds the tachometer interrupt to the pwm-fan binding, which is
> necessary for RPM support.
>
> Signed-off-by: Stefan Wahren <[email protected]>
> ---
> Documentation/devicetree/bindings/hwmon/pwm-fan.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> index 49ca5d8..7f69b0b 100644
> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> @@ -8,6 +8,9 @@ Required properties:
>
> Optional properties:
> - fan-supply : phandle to the regulator that provides power to the fan
> +- interrupts : contains a single interrupt specifier which describes the
> + tachometer pin output of a 2 pulse-per-revolution fan.
> + See interrupt-controller/interrupts.txt for the format.
So a hypothetical {1,4} pulse-per-revolution fan would explicitly not be
supported ? Why ?
Guenter
Hi Guenter,
> Guenter Roeck <[email protected]> hat am 30. Januar 2019 um 18:28 geschrieben:
>
>
> On Wed, Jan 30, 2019 at 04:07:05PM +0100, Stefan Wahren wrote:
> > This adds the tachometer interrupt to the pwm-fan binding, which is
> > necessary for RPM support.
> >
> > Signed-off-by: Stefan Wahren <[email protected]>
> > ---
> > Documentation/devicetree/bindings/hwmon/pwm-fan.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > index 49ca5d8..7f69b0b 100644
> > --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > @@ -8,6 +8,9 @@ Required properties:
> >
> > Optional properties:
> > - fan-supply : phandle to the regulator that provides power to the fan
> > +- interrupts : contains a single interrupt specifier which describes the
> > + tachometer pin output of a 2 pulse-per-revolution fan.
> > + See interrupt-controller/interrupts.txt for the format.
>
> So a hypothetical {1,4} pulse-per-revolution fan would explicitly not be
> supported ? Why ?
i could add an additional property to specify the pulse per revolution and use the 2 as default (according to the Intel spec for 4 pin pwm fan) which should fit in most cases.
>
> Guenter
On Wed, Jan 30, 2019 at 09:23:31PM +0100, Stefan Wahren wrote:
> Hi Guenter,
>
> > Guenter Roeck <[email protected]> hat am 30. Januar 2019 um 18:28 geschrieben:
> >
> >
> > On Wed, Jan 30, 2019 at 04:07:05PM +0100, Stefan Wahren wrote:
> > > This adds the tachometer interrupt to the pwm-fan binding, which is
> > > necessary for RPM support.
> > >
> > > Signed-off-by: Stefan Wahren <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/hwmon/pwm-fan.txt | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > > index 49ca5d8..7f69b0b 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > > +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > > @@ -8,6 +8,9 @@ Required properties:
> > >
> > > Optional properties:
> > > - fan-supply : phandle to the regulator that provides power to the fan
> > > +- interrupts : contains a single interrupt specifier which describes the
> > > + tachometer pin output of a 2 pulse-per-revolution fan.
> > > + See interrupt-controller/interrupts.txt for the format.
> >
> > So a hypothetical {1,4} pulse-per-revolution fan would explicitly not be
> > supported ? Why ?
>
> i could add an additional property to specify the pulse per revolution and use the 2 as default (according to the Intel spec for 4 pin pwm fan) which should fit in most cases.
>
That would be a possibility and make sense, but that is not
the point here. The "interrupts" property does not and should
not care how many pulses per revolution the fan provides.
Guenter
Hi Guenter,
Am 30.01.19 um 21:35 schrieb Guenter Roeck:
> On Wed, Jan 30, 2019 at 09:23:31PM +0100, Stefan Wahren wrote:
>> Hi Guenter,
>>
>>> Guenter Roeck <[email protected]> hat am 30. Januar 2019 um 18:28 geschrieben:
>>>
>>>
>>> On Wed, Jan 30, 2019 at 04:07:05PM +0100, Stefan Wahren wrote:
>>>> This adds the tachometer interrupt to the pwm-fan binding, which is
>>>> necessary for RPM support.
>>>>
>>>> Signed-off-by: Stefan Wahren <[email protected]>
>>>> ---
>>>> Documentation/devicetree/bindings/hwmon/pwm-fan.txt | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>>>> index 49ca5d8..7f69b0b 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>>>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>>>> @@ -8,6 +8,9 @@ Required properties:
>>>>
>>>> Optional properties:
>>>> - fan-supply : phandle to the regulator that provides power to the fan
>>>> +- interrupts : contains a single interrupt specifier which describes the
>>>> + tachometer pin output of a 2 pulse-per-revolution fan.
>>>> + See interrupt-controller/interrupts.txt for the format.
>>> So a hypothetical {1,4} pulse-per-revolution fan would explicitly not be
>>> supported ? Why ?
>> i could add an additional property to specify the pulse per revolution and use the 2 as default (according to the Intel spec for 4 pin pwm fan) which should fit in most cases.
>>
> That would be a possibility and make sense, but that is not
> the point here. The "interrupts" property does not and should
> not care how many pulses per revolution the fan provides.
sorry, i'm not sure what's the problem about. Do you want me to use a
GPIO instead of interrupt?
Or is it the wording here?
Suggestion:
contains a single interrupt specifier which is describes the tachometer
output of the fan as an input
Stefan
>
> Guenter
On Thu, Jan 31, 2019 at 09:02:33AM +0100, Stefan Wahren wrote:
> Hi Guenter,
>
> Am 30.01.19 um 21:35 schrieb Guenter Roeck:
> > On Wed, Jan 30, 2019 at 09:23:31PM +0100, Stefan Wahren wrote:
> >> Hi Guenter,
> >>
> >>> Guenter Roeck <[email protected]> hat am 30. Januar 2019 um 18:28 geschrieben:
> >>>
> >>>
> >>> On Wed, Jan 30, 2019 at 04:07:05PM +0100, Stefan Wahren wrote:
> >>>> This adds the tachometer interrupt to the pwm-fan binding, which is
> >>>> necessary for RPM support.
> >>>>
> >>>> Signed-off-by: Stefan Wahren <[email protected]>
> >>>> ---
> >>>> Documentation/devicetree/bindings/hwmon/pwm-fan.txt | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> >>>> index 49ca5d8..7f69b0b 100644
> >>>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> >>>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> >>>> @@ -8,6 +8,9 @@ Required properties:
> >>>>
> >>>> Optional properties:
> >>>> - fan-supply : phandle to the regulator that provides power to the fan
> >>>> +- interrupts : contains a single interrupt specifier which describes the
> >>>> + tachometer pin output of a 2 pulse-per-revolution fan.
> >>>> + See interrupt-controller/interrupts.txt for the format.
> >>> So a hypothetical {1,4} pulse-per-revolution fan would explicitly not be
> >>> supported ? Why ?
> >> i could add an additional property to specify the pulse per revolution and use the 2 as default (according to the Intel spec for 4 pin pwm fan) which should fit in most cases.
> >>
> > That would be a possibility and make sense, but that is not
> > the point here. The "interrupts" property does not and should
> > not care how many pulses per revolution the fan provides.
>
> sorry, i'm not sure what's the problem about. Do you want me to use a
> GPIO instead of interrupt?
>
> Or is it the wording here?
>
You wording limits the use of interrupts with pwm fans to fans with
2 pulses per revolution. You do not explain why that restriction would
be required or even make sense, or why it should be associated with
the 'interrupts' property.
Logically I assume that is because you expect an interrupt per pulse,
but that is not explained (or what the interrupt is expected to be
used for in the first place - it might, after all, be some kind of
error interrupt).
Guenter
On Wed, Jan 30, 2019 at 04:07:07PM +0100, Stefan Wahren wrote:
> This adds RPM support to the pwm-fan driver in order to use with
> fancontrol/pwmconfig. This feature is intended for 2 pulse-per-revolution
> fans which provides a tachometer output signal.
>
> Signed-off-by: Stefan Wahren <[email protected]>
> ---
> drivers/hwmon/pwm-fan.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 9e0591e..731fdc6 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,13 +27,22 @@
> #include <linux/regulator/consumer.h>
> #include <linux/sysfs.h>
> #include <linux/thermal.h>
> +#include <linux/timer.h>
>
> #define MAX_PWM 255
> +#define SAMPLE_TIME 1 /* seconds */
> +#define PULSE_PER_REVOLUTION 2
>
> struct pwm_fan_ctx {
> struct mutex lock;
> struct pwm_device *pwm;
> struct regulator *reg_en;
> +
> + int irq;
> + unsigned int pulses;
> + unsigned int rpm;
> + struct timer_list sample_timer;
> +
> unsigned int pwm_value;
> unsigned int pwm_fan_state;
> unsigned int pwm_fan_max_state;
> @@ -40,6 +50,27 @@ struct pwm_fan_ctx {
> struct thermal_cooling_device *cdev;
> };
>
> +static irqreturn_t pulse_handler(int irq, void *dev_id)
> +{
> + struct pwm_fan_ctx *ctx = dev_id;
> +
> + if (ctx->pulses < INT_MAX / 2)
> + ctx->pulses++;
Please explain the limit. With this code, the maximum number
for pulses is INT_MAX / 2. The calculation below multiplies
that with 60, which is well above UINT_MAX. Either use a
realistic limit or drop the if statement (it is kind of
unlikely to get that many interrupts per second).
> +
> + return IRQ_HANDLED;
This assumes a self resetting edge triggered interrupt ?
You might want to mention that somewhere.
> +}
> +
> +static void sample_timer(struct timer_list *t)
> +{
> + struct pwm_fan_ctx *ctx = from_timer(ctx, t, sample_timer);
> + unsigned int pulses;
> +
> + pulses = ctx->pulses;
> + ctx->pulses -= pulses;
This is racy; the substract operation is not atomic, and neither
is the addition in the interrupt handler.
> + ctx->rpm = pulses * 60 / SAMPLE_TIME / PULSE_PER_REVOLUTION;
The calculation assumes that sample_timer is called exactly after one
second. Have to tested how much variation this causes on a loaded system ?
> + mod_timer(&ctx->sample_timer, jiffies + (HZ * SAMPLE_TIME));
> +}
> +
> static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> {
> unsigned long period;
> @@ -100,11 +131,20 @@ 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,
This attribute is visible even if not supported, meaning it will
report a fan speed of 0 rpm in that case. Please make the attribute
conditional.
> NULL,
> };
>
> @@ -263,6 +303,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
> }
> }
>
> + timer_setup(&ctx->sample_timer, sample_timer, 0);
> +
> + ctx->irq = platform_get_irq(pdev, 0);
> + if (ctx->irq >= 0) {
> + 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");
> + return ret;
> + }
> + mod_timer(&ctx->sample_timer, jiffies + (HZ * SAMPLE_TIME));
> + }
> +
> hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, "pwmfan",
> ctx, pwm_fan_groups);
> if (IS_ERR(hwmon)) {
> @@ -304,6 +357,8 @@ static int pwm_fan_remove(struct platform_device *pdev)
> struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
>
> thermal_cooling_device_unregister(ctx->cdev);
> + del_timer_sync(&ctx->sample_timer);
> +
> if (ctx->pwm_value)
> pwm_disable(ctx->pwm);
>