* [PATCH V4 0/3] hwmon: pwm-fan: Add RPM support @ 2019-04-02 14:21 Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 1/3] dt-bindings: hwmon: Add tachometer interrupt to pwm-fan Stefan Wahren ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Stefan Wahren @ 2019-04-02 14:21 UTC (permalink / raw) To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck, Rob Herring, Mark Rutland, Robin Murphy Cc: linux-hwmon, devicetree, linux-kernel, Stefan Wahren 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. Changes in V4: - remove copy & paste artifact in dt-binding example - switch to u64 for rpm calculation - drop overflow handling and init atomic - handle 0 not as valid irq - reduce range of pulses_per_revolution - handle probe defer of platform_get_irq - delete timer properly in bail-out path Changes in V3: - rename property interrupt-ratio to pulses-per-revolution to avoid confusion with interrupt binding - handle error case pulses-per-revolution = 0 - bail out properly in case we are unable to request the irq Changes in V2: - address Guenter's comments: - improve description of interrupts - use atomic_t to avoid races of the pulse counter - measure sample time to make rpm more reliable under load - make sysfs entry fan1_input conditional - add dt-property to define interrupts per fan revolution - example for fan with RPM 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 | 21 +++- Documentation/hwmon/pwm-fan | 3 + drivers/hwmon/pwm-fan.c | 111 ++++++++++++++++++++- 3 files changed, 130 insertions(+), 5 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V4 1/3] dt-bindings: hwmon: Add tachometer interrupt to pwm-fan 2019-04-02 14:21 [PATCH V4 0/3] hwmon: pwm-fan: Add RPM support Stefan Wahren @ 2019-04-02 14:21 ` Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 2/3] Documentation: pwm-fan: Add description for RPM support Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt Stefan Wahren 2 siblings, 0 replies; 9+ messages in thread From: Stefan Wahren @ 2019-04-02 14:21 UTC (permalink / raw) To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck, Rob Herring, Mark Rutland, Robin Murphy Cc: linux-hwmon, devicetree, linux-kernel, Stefan Wahren This adds the tachometer interrupt to the pwm-fan binding, which is necessary for RPM support. Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> Reviewed-by: Rob Herring <robh@kernel.org> --- Documentation/devicetree/bindings/hwmon/pwm-fan.txt | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index 49ca5d8..6ced829b 100644 --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -7,7 +7,16 @@ Required properties: which correspond to thermal cooling states Optional properties: -- fan-supply : phandle to the regulator that provides power to the fan +- fan-supply : phandle to the regulator that provides power to the fan +- interrupts : This contains a single interrupt specifier which + describes the tachometer output of the fan as an + interrupt source. The output signal must generate a + defined number of interrupts per fan revolution, which + require that it must be self resetting edge interrupts. + See interrupt-controller/interrupts.txt for the format. +- pulses-per-revolution : define the tachometer pulses per fan revolution as + an integer (default is 2 interrupts per revolution). + The value must be greater than zero. Example: fan0: pwm-fan { @@ -38,3 +47,13 @@ Example: }; }; }; + +Example 2: + fan0: pwm-fan { + compatible = "pwm-fan"; + pwms = <&pwm 0 40000 0>; + fan-supply = <®_fan>; + interrupt-parent = <&gpio5>; + interrupts = <1 IRQ_TYPE_EDGE_FALLING>; + pulses-per-revolution = <2>; + }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH V4 2/3] Documentation: pwm-fan: Add description for RPM support 2019-04-02 14:21 [PATCH V4 0/3] hwmon: pwm-fan: Add RPM support Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 1/3] dt-bindings: hwmon: Add tachometer interrupt to pwm-fan Stefan Wahren @ 2019-04-02 14:21 ` Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt Stefan Wahren 2 siblings, 0 replies; 9+ messages in thread From: Stefan Wahren @ 2019-04-02 14:21 UTC (permalink / raw) To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck, Rob Herring, Mark Rutland, Robin Murphy Cc: linux-hwmon, devicetree, linux-kernel, Stefan Wahren This adds a short description for the new RPM support of the pwm-fan driver. Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- Documentation/hwmon/pwm-fan | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/hwmon/pwm-fan b/Documentation/hwmon/pwm-fan index 18529d2..82fe967 100644 --- a/Documentation/hwmon/pwm-fan +++ b/Documentation/hwmon/pwm-fan @@ -15,3 +15,6 @@ 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 fan rotation speed returned via the optional 'fan1_input' is extrapolated +from the sampled interrupts from the tachometer signal within 1 second. -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt 2019-04-02 14:21 [PATCH V4 0/3] hwmon: pwm-fan: Add RPM support Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 1/3] dt-bindings: hwmon: Add tachometer interrupt to pwm-fan Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 2/3] Documentation: pwm-fan: Add description for RPM support Stefan Wahren @ 2019-04-02 14:21 ` Stefan Wahren 2019-04-02 20:55 ` Guenter Roeck 2 siblings, 1 reply; 9+ messages in thread From: Stefan Wahren @ 2019-04-02 14:21 UTC (permalink / raw) To: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck, Rob Herring, Mark Rutland, Robin Murphy Cc: linux-hwmon, devicetree, linux-kernel, Stefan Wahren 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; + } + + 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt 2019-04-02 14:21 ` [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt Stefan Wahren @ 2019-04-02 20:55 ` Guenter Roeck 2019-04-03 9:55 ` Stefan Wahren 0 siblings, 1 reply; 9+ messages in thread From: Guenter Roeck @ 2019-04-02 20:55 UTC (permalink / raw) To: Stefan Wahren Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Rob Herring, Mark Rutland, Robin Murphy, linux-hwmon, devicetree, linux-kernel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt 2019-04-02 20:55 ` Guenter Roeck @ 2019-04-03 9:55 ` Stefan Wahren 2019-04-03 15:59 ` Robin Murphy 0 siblings, 1 reply; 9+ messages in thread From: Stefan Wahren @ 2019-04-03 9:55 UTC (permalink / raw) To: Guenter Roeck Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Rob Herring, Mark Rutland, Robin Murphy, linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt 2019-04-03 9:55 ` Stefan Wahren @ 2019-04-03 15:59 ` Robin Murphy 2019-04-03 16:11 ` Guenter Roeck 2019-04-03 16:23 ` Stefan Wahren 0 siblings, 2 replies; 9+ messages in thread From: Robin Murphy @ 2019-04-03 15:59 UTC (permalink / raw) To: Stefan Wahren, Guenter Roeck Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Rob Herring, Mark Rutland, linux-hwmon, devicetree, linux-kernel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt 2019-04-03 15:59 ` Robin Murphy @ 2019-04-03 16:11 ` Guenter Roeck 2019-04-03 16:23 ` Stefan Wahren 1 sibling, 0 replies; 9+ messages in thread From: Guenter Roeck @ 2019-04-03 16:11 UTC (permalink / raw) To: Robin Murphy Cc: Stefan Wahren, Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Rob Herring, Mark Rutland, linux-hwmon, devicetree, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt 2019-04-03 15:59 ` Robin Murphy 2019-04-03 16:11 ` Guenter Roeck @ 2019-04-03 16:23 ` Stefan Wahren 1 sibling, 0 replies; 9+ messages in thread From: Stefan Wahren @ 2019-04-03 16:23 UTC (permalink / raw) To: Robin Murphy, Guenter Roeck Cc: Kamil Debski, Bartlomiej Zolnierkiewicz, Jean Delvare, Rob Herring, Mark Rutland, linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-04-03 16:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-02 14:21 [PATCH V4 0/3] hwmon: pwm-fan: Add RPM support Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 1/3] dt-bindings: hwmon: Add tachometer interrupt to pwm-fan Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 2/3] Documentation: pwm-fan: Add description for RPM support Stefan Wahren 2019-04-02 14:21 ` [PATCH V4 3/3] hwmon: pwm-fan: Add RPM support via external interrupt Stefan Wahren 2019-04-02 20:55 ` Guenter Roeck 2019-04-03 9:55 ` Stefan Wahren 2019-04-03 15:59 ` Robin Murphy 2019-04-03 16:11 ` Guenter Roeck 2019-04-03 16:23 ` Stefan Wahren
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.