| Message ID | 1554214910-29925-4-git-send-email-stefan.wahren@i2se.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: pwm-fan: Add RPM support | expand |
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@i2se.com> > --- > 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. [ 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; > + } > + ctx->sample_start = ktime_get(); > + mod_timer(&ctx->rpm_timer, jiffies + HZ); > + } > + > hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, "pwmfan", > ctx, pwm_fan_groups); > if (IS_ERR(hwmon)) { > dev_err(&pdev->dev, "Failed to register hwmon device\n"); > ret = PTR_ERR(hwmon); > - goto err_pwm_disable; > + goto err_del_timer; > } > > ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx); > if (ret) > - return ret; > + goto err_del_timer; Outch. This is buggy and should have been "goto err_pwm_disable;". It needs to be fixed with a separate patch, and first, so we can backport it. Can you do that ? > > ctx->pwm_fan_state = ctx->pwm_fan_max_state; > if (IS_ENABLED(CONFIG_THERMAL)) { > @@ -282,7 +380,7 @@ static int pwm_fan_probe(struct platform_device *pdev) > dev_err(&pdev->dev, > "Failed to register pwm-fan as cooling device"); > ret = PTR_ERR(cdev); > - goto err_pwm_disable; > + goto err_del_timer; > } > ctx->cdev = cdev; > thermal_cdev_update(cdev); > @@ -290,6 +388,9 @@ static int pwm_fan_probe(struct platform_device *pdev) > > return 0; > > +err_del_timer: > + del_timer_sync(&ctx->rpm_timer); > + > err_pwm_disable: > state.enabled = false; > pwm_apply_state(ctx->pwm, &state); > @@ -306,6 +407,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->rpm_timer); > + > if (ctx->pwm_value) > pwm_disable(ctx->pwm); > > -- > 2.7.4 >
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@i2se.com> >> --- >> 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 > > [ 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; >> + } >> + ctx->sample_start = ktime_get(); >> + mod_timer(&ctx->rpm_timer, jiffies + HZ); >> + } >> + >> hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, "pwmfan", >> ctx, pwm_fan_groups); >> if (IS_ERR(hwmon)) { >> dev_err(&pdev->dev, "Failed to register hwmon device\n"); >> ret = PTR_ERR(hwmon); >> - goto err_pwm_disable; >> + goto err_del_timer; >> } >> >> ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx); >> if (ret) >> - return ret; >> + goto err_del_timer; > Outch. This is buggy and should have been "goto err_pwm_disable;". > It needs to be fixed with a separate patch, and first, so we can > backport it. Can you do that ? Sure Stefan
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@i2se.com> >>> --- >>> 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; >> >> [ 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? Robin. >>> + } >>> + ctx->sample_start = ktime_get(); >>> + mod_timer(&ctx->rpm_timer, jiffies + HZ); >>> + } >>> + >>> hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, "pwmfan", >>> ctx, pwm_fan_groups); >>> if (IS_ERR(hwmon)) { >>> dev_err(&pdev->dev, "Failed to register hwmon device\n"); >>> ret = PTR_ERR(hwmon); >>> - goto err_pwm_disable; >>> + goto err_del_timer; >>> } >>> >>> ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx); >>> if (ret) >>> - return ret; >>> + goto err_del_timer; >> Outch. This is buggy and should have been "goto err_pwm_disable;". >> It needs to be fixed with a separate patch, and first, so we can >> backport it. Can you do that ? > > Sure > > Stefan >
On Wed, Apr 03, 2019 at 04:59:35PM +0100, Robin Murphy wrote: > 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@i2se.com> > >>>--- > >>> 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; > +1 Thanks, Guenter > >> > >>[ 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? > > Robin. > > >>>+ } > >>>+ ctx->sample_start = ktime_get(); > >>>+ mod_timer(&ctx->rpm_timer, jiffies + HZ); > >>>+ } > >>>+ > >>> hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, "pwmfan", > >>> ctx, pwm_fan_groups); > >>> if (IS_ERR(hwmon)) { > >>> dev_err(&pdev->dev, "Failed to register hwmon device\n"); > >>> ret = PTR_ERR(hwmon); > >>>- goto err_pwm_disable; > >>>+ goto err_del_timer; > >>> } > >>> ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx); > >>> if (ret) > >>>- return ret; > >>>+ goto err_del_timer; > >>Outch. This is buggy and should have been "goto err_pwm_disable;". > >>It needs to be fixed with a separate patch, and first, so we can > >>backport it. Can you do that ? > > > >Sure > > > >Stefan > >
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@i2se.com> >>>> --- >>>> 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
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; + } + + 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", + &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; + } else 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"); + goto err_pwm_disable; + } + ctx->sample_start = ktime_get(); + mod_timer(&ctx->rpm_timer, jiffies + HZ); + } + hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, "pwmfan", ctx, pwm_fan_groups); if (IS_ERR(hwmon)) { dev_err(&pdev->dev, "Failed to register hwmon device\n"); ret = PTR_ERR(hwmon); - goto err_pwm_disable; + goto err_del_timer; } ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx); if (ret) - return ret; + goto err_del_timer; ctx->pwm_fan_state = ctx->pwm_fan_max_state; if (IS_ENABLED(CONFIG_THERMAL)) { @@ -282,7 +380,7 @@ static int pwm_fan_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Failed to register pwm-fan as cooling device"); ret = PTR_ERR(cdev); - goto err_pwm_disable; + goto err_del_timer; } ctx->cdev = cdev; thermal_cdev_update(cdev); @@ -290,6 +388,9 @@ static int pwm_fan_probe(struct platform_device *pdev) return 0; +err_del_timer: + del_timer_sync(&ctx->rpm_timer); + err_pwm_disable: state.enabled = false; pwm_apply_state(ctx->pwm, &state); @@ -306,6 +407,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->rpm_timer); + if (ctx->pwm_value) pwm_disable(ctx->pwm);
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@i2se.com> --- drivers/hwmon/pwm-fan.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 107 insertions(+), 4 deletions(-)