Several new properties to allow PWM fan working as a cooling device have been
combined into this single commit.

Signed-off-by: Lukasz Majewski <[email protected]>
---
Changes for v2:
- Rename cooling-pwm-values to cooling-levels
- Remove default-pulse-width property and stick to default hwmon policy
---
.../devicetree/bindings/hwmon/pwm-fan.txt | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
index 610757c..001446a 100644
--- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
@@ -3,10 +3,33 @@ Bindings for a fan connected to the PWM lines
Required properties:
- compatible : "pwm-fan"
- pwms : the PWM that is used to control the PWM fan
+- cooling-levels : PWM duty cycle values in a range from 0 to 255
+ which correspond to thermal cooling states
+
+Thorough description of the following bindings:
+ cooling-min-state = <0>;
+ cooling-max-state = <3>;
+ #cooling-cells = <2>;
+ thermal-zone {
+ cpu_thermal: cpu-thermal {
+ cooling-maps {
+ map0 {
+ trip = <&cpu_alert1>;
+ cooling-device = <&fan0 0 1>;
+ };
+ };
+ };
+
+for PWM FAN used as cooling device can be found at:
+./Documentation/devicetree/bindings/thermal/thermal.txt

Example:
pwm-fan {
compatible = "pwm-fan";
status = "okay";
pwms = <&pwm 0 10000 0>;
+ cooling-min-state = <0>;
+ cooling-max-state = <3>;
+ #cooling-cells = <2>;
+ cooling-levels = <0 102 170 255>;
};
--
2.0.0.rc2

2014-12-22 16:28:49

by Lukasz Majewski

[permalink] [raw]

On Fri, 6 Feb 2015 10:27:25 -0800
Guenter Roeck <[email protected]> wrote:

> On Fri, Feb 06, 2015 at 05:59:06PM +0100, Lukasz Majewski wrote:
> > It was necessary to decouple code handling writing to sysfs from
> > the one responsible for setting PWM of the fan.
> > Due to that, new __set_pwm() method was extracted, which is
> > responsible for only setting new PWM duty cycle.
> >
> > Signed-off-by: Lukasz Majewski <[email protected]>
> > ---
> > Changes for v2:
> > - None
> > Changes for v3:
> > - The commit headline has been reedited.
> > ---
> > drivers/hwmon/pwm-fan.c | 35 ++++++++++++++++++++++-------------
> > 1 file changed, 22 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index 1991d903..870e100 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -33,21 +33,15 @@ struct pwm_fan_ctx {
> > unsigned char pwm_value;
> > };
> >
> > -static ssize_t set_pwm(struct device *dev, struct device_attribute
> > *attr,
> > - const char *buf, size_t count)
> > +static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> > {
> > - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > - unsigned long pwm, duty;
> > - ssize_t ret;
> > -
> > - if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM)
> > - return -EINVAL;
> > -
> > - mutex_lock(&ctx->lock);
> > + unsigned long duty;
> > + int ret;
> >
> > if (ctx->pwm_value == pwm)
> > - goto exit_set_pwm_no_change;
> > + return 0;
> >
> Why did you move this check outside the lock ? With this change there
> is no guarantee that pwm_value wasn't changed while waiting for the
> lock.

Grrr. You are obviously right here. I will fix this. Thanks for spotting

Best regards,
Lukasz Majewski

>
> Guenter
>
> > + mutex_lock(&ctx->lock);
> > if (pwm == 0) {
> > pwm_disable(ctx->pwm);
> > goto exit_set_pwm;
> > @@ -66,13 +60,28 @@ static ssize_t set_pwm(struct device *dev,
> > struct device_attribute *attr,
> > exit_set_pwm:
> > ctx->pwm_value = pwm;
> > -exit_set_pwm_no_change:
> > - ret = count;
> > exit_set_pwm_err:
> > mutex_unlock(&ctx->lock);
> > return ret;
> > }
> >
> > +static ssize_t set_pwm(struct device *dev, struct device_attribute
> > *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > + unsigned long pwm;
> > + int ret;
> > +
> > + if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM)
> > + return -EINVAL;
> > +
> > + ret = __set_pwm(ctx, pwm);
> > + if (ret)
> > + return ret;
> > +
> > + return count;
> > +}
> > +
> > static ssize_t show_pwm(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > --
> > 2.0.0.rc2
> >

Attachments:

(No filename) (181.00 B)
OpenPGP digital signature

2015-02-08 21:47:12

Hey Lukasz,

On Wed, Feb 25, 2015 at 04:34:22PM +0100, Lukasz Majewski wrote:
> The PWM FAN device can now be used as a thermal cooling device. Necessary
> infrastructure has been added in this commit.
>
> Signed-off-by: Lukasz Majewski <[email protected]>
> ---
> Changes for v2:
> - Replace pwm_fan_cooling_states with pwm_fan_cooling_levels
> - Update ctx->pwm_fan_state when correct data from device tree is available
> - Using therma_cdev_update() when thermal is ready for controlling the fan
> Changes for v3:
> - Rename patch heading
> - pwm_fan_of_get_cooling_data() now returns -EINVAL when no "cooling-levels"
> property defined
> - register of cooling device only when proper cooling data is present
> Changes for v4:
> - None
> Changes for v5:
> - Check for IS_ENABLED(CONFIG_THERMAL) has been added to prevent from
> executing thermal_* specific functions
> ---
> drivers/hwmon/pwm-fan.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index e6ed353..2c1a8f0 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -24,6 +24,7 @@
> #include <linux/platform_device.h>
> #include <linux/pwm.h>
> #include <linux/sysfs.h>
> +#include <linux/thermal.h>
>
> #define MAX_PWM 255
>
> @@ -68,6 +69,17 @@ exit_set_pwm_err:
> return ret;
> }
>
> +static void pwm_fan_update_state(struct pwm_fan_ctx *ctx, unsigned long pwm)
> +{
> + int i;
> +
> + for (i = 0; i < ctx->pwm_fan_max_state; ++i)
> + if (pwm < ctx->pwm_fan_cooling_levels[i + 1])
> + break;
> +
> + ctx->pwm_fan_state = i;
> +}
> +
> static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> @@ -82,6 +94,7 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> if (ret)
> return ret;
>
> + pwm_fan_update_state(ctx, pwm);
> return count;
> }
>
> @@ -103,6 +116,59 @@ static struct attribute *pwm_fan_attrs[] = {
>
> ATTRIBUTE_GROUPS(pwm_fan);
>
> +/* thermal cooling device callbacks */
> +static int pwm_fan_get_max_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + struct pwm_fan_ctx *ctx = cdev->devdata;
> +

Why this call back is the only one you didn't add a check for ctx ==
NULL?

> + *state = ctx->pwm_fan_max_state;
> +
> + return 0;
> +}
> +
> +static int pwm_fan_get_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + struct pwm_fan_ctx *ctx = cdev->devdata;
> +
> + if (!ctx)
> + return -EINVAL;
> +
> + *state = ctx->pwm_fan_state;
> +
> + return 0;
> +}
> +
> +static int
> +pwm_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +{
> + struct pwm_fan_ctx *ctx = cdev->devdata;
> + int ret;
> +
> + if (!ctx || (state > ctx->pwm_fan_max_state))
> + return -EINVAL;
> +
> + if (state == ctx->pwm_fan_state)
> + return 0;
> +
> + ret = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
> + if (ret) {
> + dev_err(&cdev->device, "Cannot set pwm!\n");
> + return ret;
> + }
> +
> + ctx->pwm_fan_state = state;
> +
> + return ret;
> +}
> +
> +static const struct thermal_cooling_device_ops pwm_fan_cooling_ops = {
> + .get_max_state = pwm_fan_get_max_state,
> + .get_cur_state = pwm_fan_get_cur_state,
> + .set_cur_state = pwm_fan_set_cur_state,
> +};
> +
> int pwm_fan_of_get_cooling_data(struct device *dev, struct pwm_fan_ctx *ctx)
> {
> struct device_node *np = dev->of_node;
> @@ -145,8 +211,9 @@ int pwm_fan_of_get_cooling_data(struct device *dev, struct pwm_fan_ctx *ctx)
>
> static int pwm_fan_probe(struct platform_device *pdev)
> {
> - struct device *hwmon;
> + struct thermal_cooling_device *cdev;
> struct pwm_fan_ctx *ctx;
> + struct device *hwmon;
> int duty_cycle;
> int ret;
>
> @@ -193,6 +260,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + ctx->pwm_fan_state = ctx->pwm_fan_max_state;
> + if (IS_ENABLED(CONFIG_THERMAL)) {
> + cdev = thermal_of_cooling_device_register(pdev->dev.of_node,
> + "pwm-fan", ctx,
> + &pwm_fan_cooling_ops);

I know this is a PITA, but thermal subsystem still does not use devm
based helpers. That means, you need to be old school and unregister the
device yourself.

Please add the cdev to ctx structure:
+ ctx->cdev = cdev;

and add a call for:
thermal_cooling_device_unregister(ctx->cdev);

in static int pwm_fan_remove(struct platform_device *pdev)

despite the above two minor issues, you may add my
Acked-by: Eduardo Valentin <[email protected]>

> + if (IS_ERR(cdev)) {
> + dev_err(&pdev->dev,
> + "Failed to register pwm-fan as cooling device");
> + pwm_disable(ctx->pwm);
> + return PTR_ERR(cdev);
> + }
> + thermal_cdev_update(cdev);
> + }
> +
> return 0;
> }
>
> --
> 2.0.0.rc2
>

Attachments:

(No filename) (4.79 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-02-25 18:29:26

by Eduardo Valentin

[permalink] [raw]

Subject: Re: [PATCH v5 0/6] hwmon: thermal: Odroid U3: Provide support for Odroid U3 fan

Guenter,

On Wed, Feb 25, 2015 at 09:18:20AM -0800, Guenter Roeck wrote:
> On Wed, Feb 25, 2015 at 04:34:16PM +0100, Lukasz Majewski wrote:
> > Presented patches add support for Odroid's U3 optional CPU FAN, which uses PWM
> > subsystem for low level control.
> >
> > After successful probe it registers itself as a cooling device for thermal
> > subsystem.
> >
> > This driver also supports devices without DTS specified.
> >
> > To provide correct functionality, new properties to device tree description for
> > Exynos4412 and in particular Odroid U3 have been added.
> >
> > Those patches were tested at Exynos4412 - Odroid U3 board.
> >
> > Patches were applied on:
> > linux-soc-thermal/fixes branch (Linux v4.0-rc1)
> > SHA1: b71d399c7f2fe06b60b96155ec0b9ae167334e4a
> >
> > Kamil Debski (1):
> > ARM: dts: Add pwm-fan node to the Odroid-U3 board
> >
> > Lukasz Majewski (5):
> > Documentation: dts: Documentation entry to explain how to use PWM FAN
> > as a cooling device
> > ARM: dts: Add properties to use pwm-fan device as a cooling device in
> > Odroid U3
> > hwmon: pwm-fan: Extract __set_pwm() function to only modify PWM duty
> > cycle
> > hwmon: pwm-fan: Read PWM FAN configuration from device tree
> > hwmon: pwm-fan: Code for using PWM FAN as a cooling device
> >
> > .../devicetree/bindings/hwmon/pwm-fan.txt | 25 +++-
> > arch/arm/boot/dts/exynos4.dtsi | 2 +-
> > arch/arm/boot/dts/exynos4412-odroidu3.dts | 43 ++++++
> > drivers/hwmon/pwm-fan.c | 166 +++++++++++++++++++--
> > 4 files changed, 220 insertions(+), 16 deletions(-)
> >
> For the series:
>
> Acked-by: Guenter Roeck <[email protected]>
>
> Should I take it through hwmon ? Might make sense given the majority
> of the changes is in hwmon code.

I believe the series should go via your tree, yes. I had only minor
comments in the code added for the cooling device code, as it lacks the
unregistration call in the .remove callback.

Also, the DTS changes may generate conflicts with platform code. Lukasz
may probably ask Kukjin Kim to add them via the samsung tree.

BR,

Eduardo Valentin

>
> Thanks,
> Guenter

Attachments:

(No filename) (2.16 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-02-25 18:30:44

On Wed, Feb 25, 2015 at 04:34:19PM +0100, Lukasz Majewski wrote:
> With those bindings it is possible to use pwm-fan device available in
> Odroid U3 as a cooling device.
>
> Signed-off-by: Lukasz Majewski <[email protected]>

Acked-by: Eduardo Valentin <[email protected]>

> ---
> Changes for v2:
> - Rename cooling-pwm-values property to cooling-levels
> Changes for v3:
> - Change patch's topic to "ARM dts"
> - Reduce maximal cooling-level to 230 from 255
> Changes for v4:
> - None
> Changes for v5:
> - None
> ---
> arch/arm/boot/dts/exynos4412-odroidu3.dts | 33 ++++++++++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroidu3.dts b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> index 4c04837..abcfa3c 100644
> --- a/arch/arm/boot/dts/exynos4412-odroidu3.dts
> +++ b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> @@ -32,9 +32,40 @@
> };
> };
>
> - pwm-fan {
> + fan0: pwm-fan {
> compatible = "pwm-fan";
> pwms = <&pwm 0 10000 0>;
> + cooling-min-state = <0>;
> + cooling-max-state = <3>;
> + #cooling-cells = <2>;
> + cooling-levels = <0 102 170 230>;
> + };
> +
> + thermal-zones {
> + cpu_thermal: cpu-thermal {
> + cooling-maps {
> + map0 {
> + trip = <&cpu_alert1>;
> + cooling-device = <&cpu0 7 7>;
> + };
> + map1 {
> + trip = <&cpu_alert2>;
> + cooling-device = <&cpu0 13 13>;
> + };
> + map2 {
> + trip = <&cpu_alert0>;
> + cooling-device = <&fan0 0 1>;
> + };
> + map3 {
> + trip = <&cpu_alert1>;
> + cooling-device = <&fan0 1 2>;
> + };
> + map4 {
> + trip = <&cpu_alert2>;
> + cooling-device = <&fan0 2 3>;
> + };
> + };
> + };
> };
> };
>
> --
> 2.0.0.rc2
>

Attachments:

(No filename) (1.75 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-02-25 19:11:32

by Eduardo Valentin

[permalink] [raw]

Subject: Re: [PATCH v5 1/6] Documentation: dts: Documentation entry to explain how to use PWM FAN as a cooling device

On Wed, Feb 25, 2015 at 04:34:17PM +0100, Lukasz Majewski wrote:
> Explanation of several properties, which allow PWM fan working as a cooling
> device, have been embraced in this commit.
>
> Signed-off-by: Lukasz Majewski <[email protected]>
> ---
> Changes for v2:
> - Rename cooling-pwm-values to cooling-levels
> - Remove default-pulse-width property and stick to default hwmon policy
> Changes for v3:
> - Changing commit title from "hwmon: dts: Doc:" to "Documentation: dts"
> - Remove unnecessary properties
> - Set maximal cooling level to 230 instead of 255
> Changes for v4:
> - None
> Changes for v5:
> - Move thermal-zones description to example section
> - Extending example section
>
> ---
> .../devicetree/bindings/hwmon/pwm-fan.txt | 25 +++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> index 610757c..645251b 100644
> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> @@ -3,10 +3,33 @@ Bindings for a fan connected to the PWM lines
> Required properties:
> - compatible : "pwm-fan"
> - pwms : the PWM that is used to control the PWM fan
> +- cooling-levels : PWM duty cycle values in a range from 0 to 255
> + which correspond to thermal cooling states
>
> Example:
> - pwm-fan {
> + fan0: pwm-fan {
> compatible = "pwm-fan";
> status = "okay";
> pwms = <&pwm 0 10000 0>;
> + cooling-levels = <0 102 170 230>;
> + };
> +
> + thermal-zones {
> + cpu_thermal: cpu-thermal {
> + cooling-min-state = <0>;
> + cooling-max-state = <3>;
> + #cooling-cells = <2>;
> + trips {
> + cpu_alert1: cpu-alert1 {
> + temperature = <100000>; /* millicelsius */
> + hysteresis = <2000>; /* millicelsius */
> + type = "passive";
> + };
> + };
> + cooling-maps {
> + map0 {
> + trip = <&cpu_alert1>;
> + cooling-device = <&fan0 0 1>;
> + };
> + };

The above example has two issues:
(1) it is wrong, as the cooling device properties are in the thermal
zone node.
(2) has a misalignment / tabulation

Can you please fix that in your next version too?

> };
> --
> 2.0.0.rc2
>

Attachments:

(No filename) (2.22 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-02-25 19:13:13

Hi Guenter,

> On 02/26/2015 05:59 AM, Lukasz Majewski wrote:
> > The PWM FAN device can now be used as a thermal cooling device.
> > Necessary infrastructure has been added in this commit.
> >
> > Signed-off-by: Lukasz Majewski <[email protected]>
> > Acked-by: Eduardo Valentin <[email protected]>
> > ---
> > Changes for v2:
> > - Replace pwm_fan_cooling_states with pwm_fan_cooling_levels
> > - Update ctx->pwm_fan_state when correct data from device tree is
> > available
> > - Using therma_cdev_update() when thermal is ready for controlling
> > the fan Changes for v3:
> > - Rename patch heading
> > - pwm_fan_of_get_cooling_data() now returns -EINVAL when no
> > "cooling-levels" property defined
> > - register of cooling device only when proper cooling data is
> > present Changes for v4:
> > - None
> > Changes for v5:
> > - Check for IS_ENABLED(CONFIG_THERMAL) has been added to prevent
> > from executing thermal_* specific functions
> > Changes for v6:
> > - Adding missing ctx == NULL check in pwm_fan_get_max_state()
> > - Call to thermal_cooling_device_unregister(ctx->cdev); at
> > pwm_fan_remove()
> > - struct thermal_cooling_device *cdev; added to struct pwm_fan_ctx
> > ---
> > drivers/hwmon/pwm-fan.c | 89
> > ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed,
> > 88 insertions(+), 1 deletion(-)
> [ ... ]
> >
> > @@ -200,6 +286,7 @@ static int pwm_fan_remove(struct
> > platform_device *pdev) {
> > struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
> >
> > + thermal_cooling_device_unregister(ctx->cdev);
>
> Unfortunately there is still no prototype for this if CONFIG_THERMAL
> is not configured.

Yes... since Nishanth Menon's changes are now only in
linux-soc-thermal/fixes

>
> Two options: Yet another revision, or wait a week until the prototypes
> are (hopefully) available and submit a patch without the conditionals.

No problem, I will wait a bit and resend this patch set.

>
> Guenter
>

--
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group