The pwm-fan driver enables control of fans connected to PWM lines.
This driver uses the PWM framework, so it is compatible with all
PWM devices that provide drivers through the PWM framework.
Signed-off-by: Kamil Debski <[email protected]>
---
.../devicetree/bindings/hwmon/pwm-fan.txt | 12 ++
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/pwm-fan.c | 199 ++++++++++++++++++++
4 files changed, 221 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.txt
create mode 100644 drivers/hwmon/pwm-fan.c
diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
new file mode 100644
index 0000000..f534607
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
@@ -0,0 +1,12 @@
+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
+
+Example:
+ pwm-fan {
+ compatible = "pwm-fan";
+ status = "okay";
+ pwms = <&pwm 0 10000 0>;
+ };
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 08531a1..838ffb8 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1104,6 +1104,15 @@ config SENSORS_PCF8591
source drivers/hwmon/pmbus/Kconfig
+config SENSORS_PWM_FAN
+ tristate "PWM fan"
+ depends on PWM
+ help
+ If you say yes here you get support for fans connected to PWM lines.
+
+ This driver can also be built as a module. If so, the module
+ will be called pwm-fan.
+
config SENSORS_SHT15
tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
depends on GPIOLIB
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3dc0f02..3967ecf 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -120,6 +120,7 @@ obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
+obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
new file mode 100644
index 0000000..78fa627
--- /dev/null
+++ b/drivers/hwmon/pwm-fan.c
@@ -0,0 +1,199 @@
+/*
+ * pwm-fan.c - Hwmon driver for fans connected to PWM lines.
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * Author: Kamil Debski <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define MAX_PWM 255
+
+struct pwm_fan_ctx {
+ struct device *hwmon;
+ struct mutex lock;
+ struct platform_device *pdev;
+ struct pwm_device *pwm;
+ unsigned int duty_cycle;
+ unsigned char pwm_value;
+};
+
+static ssize_t show_pwm_enable(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ /* For the sake of simplicity only mode 1 (PWM) is supported */
+ return sprintf(buf, "1\n");
+}
+
+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, duty;
+
+ if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM)
+ return -EINVAL;
+
+ mutex_lock(&ctx->lock);
+
+ if (ctx->pwm_value == pwm)
+ goto exit_set_pwm;
+
+ duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
+ if (pwm_config(ctx->pwm, duty, ctx->pwm->period))
+ goto exit_set_pwm;
+
+ ctx->duty_cycle = duty;
+ ctx->pwm_value = pwm;
+
+
+exit_set_pwm:
+ mutex_unlock(&ctx->lock);
+
+ return count;
+}
+
+static ssize_t show_pwm(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", ctx->pwm_value);
+}
+
+
+static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
+ show_pwm_enable, NULL, 0);
+static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR,
+ show_pwm, set_pwm, 0);
+
+static struct attribute *pwm_fan_attributes[] = {
+ &sensor_dev_attr_pwm1_enable.dev_attr.attr,
+ &sensor_dev_attr_pwm1.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group pwm_fan_group = {
+ .attrs = pwm_fan_attributes,
+};
+
+static const struct attribute_group *pwm_fan_groups[] = {
+ &pwm_fan_group,
+ NULL
+};
+
+static int pwm_fan_probe(struct platform_device *pdev)
+{
+ struct pwm_fan_ctx *ctx;
+
+ ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx) {
+ dev_err(&pdev->dev, "Not enough memory for PWM fan device\n");
+ return -ENOMEM;
+ }
+
+ ctx->pdev = pdev;
+ mutex_init(&ctx->lock);
+
+ ctx->pwm = devm_of_pwm_get(&pdev->dev, pdev->dev.of_node, 0);
+ if (IS_ERR(ctx->pwm)) {
+ dev_err(&pdev->dev, "Could not get PWM");
+ return -EINVAL;
+ }
+
+ dev_set_drvdata(&pdev->dev, ctx);
+ platform_set_drvdata(pdev, ctx);
+
+ /* Set duty cycle to maximum allowed */
+ ctx->duty_cycle = ctx->pwm->period - 1;
+ ctx->pwm_value = MAX_PWM;
+ pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period);
+
+ ctx->hwmon = hwmon_device_register_with_groups(&pdev->dev, "pwmfan",
+ ctx, pwm_fan_groups);
+
+ if (IS_ERR(ctx->hwmon)) {
+ dev_err(&pdev->dev, "Failed to register hwmon device");
+ return PTR_ERR(ctx->hwmon);
+ }
+
+ /* Enbale PWM output */
+ pwm_enable(ctx->pwm);
+
+ return 0;
+}
+
+static int pwm_fan_remove(struct platform_device *pdev)
+{
+ struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
+
+ pwm_disable(ctx->pwm);
+ hwmon_device_unregister(ctx->hwmon);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pwm_fan_suspend(struct device *dev)
+{
+ struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+
+ return pwm_config(ctx->pwm, 0, ctx->pwm->period);
+}
+
+static int pwm_fan_resume(struct device *dev)
+{
+ struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+
+ return pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period);
+}
+
+static SIMPLE_DEV_PM_OPS(pwm_fan_pm, pwm_fan_suspend, pwm_fan_resume);
+#endif
+
+
+
+static struct of_device_id of_pwm_fan_match[] = {
+ { .compatible = "pwm-fan", },
+ {},
+};
+
+static struct platform_driver pwm_fan_driver = {
+ .probe = pwm_fan_probe,
+ .remove = pwm_fan_remove,
+ .driver = {
+ .name = "pwm-fan",
+#ifdef CONFIG_PM_SLEEP
+ .pm = &pwm_fan_pm,
+#endif
+ .of_match_table = of_match_ptr(of_pwm_fan_match),
+ },
+};
+
+module_platform_driver(pwm_fan_driver);
+
+MODULE_AUTHOR("Kamil Debski <[email protected]>");
+MODULE_ALIAS("platform:pwm-fan");
+MODULE_DESCRIPTION("PWM FAN driver");
+MODULE_LICENSE("GPL");
--
1.7.9.5
On Wed, Jul 09, 2014 at 04:53:20PM +0200, Kamil Debski wrote:
> The pwm-fan driver enables control of fans connected to PWM lines.
> This driver uses the PWM framework, so it is compatible with all
> PWM devices that provide drivers through the PWM framework.
>
> Signed-off-by: Kamil Debski <[email protected]>
> ---
> .../devicetree/bindings/hwmon/pwm-fan.txt | 12 ++
> drivers/hwmon/Kconfig | 9 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/pwm-fan.c | 199 ++++++++++++++++++++
> 4 files changed, 221 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> create mode 100644 drivers/hwmon/pwm-fan.c
Documentation/hwmon/pwm-fan is missing.
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> new file mode 100644
> index 0000000..f534607
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> @@ -0,0 +1,12 @@
> +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
> +
> +Example:
> + pwm-fan {
> + compatible = "pwm-fan";
> + status = "okay";
> + pwms = <&pwm 0 10000 0>;
> + };
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 08531a1..838ffb8 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1104,6 +1104,15 @@ config SENSORS_PCF8591
>
> source drivers/hwmon/pmbus/Kconfig
>
> +config SENSORS_PWM_FAN
> + tristate "PWM fan"
> + depends on PWM
> + help
> + If you say yes here you get support for fans connected to PWM lines.
> +
> + This driver can also be built as a module. If so, the module
> + will be called pwm-fan.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3dc0f02..3967ecf 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -120,6 +120,7 @@ obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> +obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> new file mode 100644
> index 0000000..78fa627
> --- /dev/null
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -0,0 +1,199 @@
> +/*
> + * pwm-fan.c - Hwmon driver for fans connected to PWM lines.
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + *
> + * Author: Kamil Debski <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
Please run the patch through checkpatch and ensure that all warnings and errors
are fixed.
> + */
> +
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
For new drivers, please list include files in alphabetic order.
> +
> +#define MAX_PWM 255
> +
> +struct pwm_fan_ctx {
> + struct device *hwmon;
> + struct mutex lock;
> + struct platform_device *pdev;
> + struct pwm_device *pwm;
> + unsigned int duty_cycle;
> + unsigned char pwm_value;
> +};
> +
> +static ssize_t show_pwm_enable(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + /* For the sake of simplicity only mode 1 (PWM) is supported */
> + return sprintf(buf, "1\n");
> +}
> +
> +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, duty;
> +
> + if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM)
> + return -EINVAL;
> +
> + mutex_lock(&ctx->lock);
> +
> + if (ctx->pwm_value == pwm)
> + goto exit_set_pwm;
> +
> + duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
Unnecessary double space.
> + if (pwm_config(ctx->pwm, duty, ctx->pwm->period))
> + goto exit_set_pwm;
Please return the error code.
> +
> + ctx->duty_cycle = duty;
> + ctx->pwm_value = pwm;
> +
> +
Single empty line only please.
> +exit_set_pwm:
> + mutex_unlock(&ctx->lock);
> +
> + return count;
> +}
> +
> +static ssize_t show_pwm(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", ctx->pwm_value);
pwm_value is unsigned, so please use %u.
> +}
> +
> +
Single empty line only please
> +static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
> + show_pwm_enable, NULL, 0);
Is this attribute really needed ?
> +static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR,
> + show_pwm, set_pwm, 0);
> +
> +static struct attribute *pwm_fan_attributes[] = {
> + &sensor_dev_attr_pwm1_enable.dev_attr.attr,
> + &sensor_dev_attr_pwm1.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group pwm_fan_group = {
> + .attrs = pwm_fan_attributes,
> +};
> +
> +static const struct attribute_group *pwm_fan_groups[] = {
> + &pwm_fan_group,
> + NULL
> +};
Please use the ATTRIBUTE_GROUPS macro.
> +
> +static int pwm_fan_probe(struct platform_device *pdev)
> +{
> + struct pwm_fan_ctx *ctx;
> +
> + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx) {
> + dev_err(&pdev->dev, "Not enough memory for PWM fan device\n");
Unnecessary error message.
> + return -ENOMEM;
> + }
> +
> + ctx->pdev = pdev;
> + mutex_init(&ctx->lock);
> +
> + ctx->pwm = devm_of_pwm_get(&pdev->dev, pdev->dev.of_node, 0);
Last argument is a string pointer. Please use NULL.
> + if (IS_ERR(ctx->pwm)) {
> + dev_err(&pdev->dev, "Could not get PWM");
> + return -EINVAL;
Return PTR_ERR(ctx->pwm).
> + }
> +
> + dev_set_drvdata(&pdev->dev, ctx);
> + platform_set_drvdata(pdev, ctx);
> +
> + /* Set duty cycle to maximum allowed */
> + ctx->duty_cycle = ctx->pwm->period - 1;
> + ctx->pwm_value = MAX_PWM;
> + pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period);
pwm_config() can return an error. Why not check it here ?
> +
> + ctx->hwmon = hwmon_device_register_with_groups(&pdev->dev, "pwmfan",
> + ctx, pwm_fan_groups);
Why not devm_hwmon_device_register_with_groups ?
> +
> + if (IS_ERR(ctx->hwmon)) {
> + dev_err(&pdev->dev, "Failed to register hwmon device");
> + return PTR_ERR(ctx->hwmon);
> + }
> +
> + /* Enbale PWM output */
> + pwm_enable(ctx->pwm);
> +
> + return 0;
> +}
> +
> +static int pwm_fan_remove(struct platform_device *pdev)
> +{
> + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
> +
> + pwm_disable(ctx->pwm);
Is this needed ? I don't see it used by the leds-pwm driver on cleanup.
> + hwmon_device_unregister(ctx->hwmon);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int pwm_fan_suspend(struct device *dev)
> +{
> + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +
> + return pwm_config(ctx->pwm, 0, ctx->pwm->period);
Wonder if this should be pwm_disable instead (and pwm_enable on resume).
Any thoughts ?
> +}
> +
> +static int pwm_fan_resume(struct device *dev)
> +{
> + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +
> + return pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pwm_fan_pm, pwm_fan_suspend, pwm_fan_resume);
> +#endif
> +
> +
> +
Single empty line only please
> +static struct of_device_id of_pwm_fan_match[] = {
> + { .compatible = "pwm-fan", },
> + {},
> +};
> +
> +static struct platform_driver pwm_fan_driver = {
> + .probe = pwm_fan_probe,
> + .remove = pwm_fan_remove,
> + .driver = {
> + .name = "pwm-fan",
> +#ifdef CONFIG_PM_SLEEP
> + .pm = &pwm_fan_pm,
> +#endif
> + .of_match_table = of_match_ptr(of_pwm_fan_match),
> + },
> +};
> +
> +module_platform_driver(pwm_fan_driver);
> +
> +MODULE_AUTHOR("Kamil Debski <[email protected]>");
> +MODULE_ALIAS("platform:pwm-fan");
> +MODULE_DESCRIPTION("PWM FAN driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
>
> _______________________________________________
> lm-sensors mailing list
> [email protected]
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
On 2014-07-09 at 16:53:20 +0200, Kamil Debski <[email protected]> wrote:
> The pwm-fan driver enables control of fans connected to PWM lines.
> This driver uses the PWM framework, so it is compatible with all
> PWM devices that provide drivers through the PWM framework.
>
> Signed-off-by: Kamil Debski <[email protected]>
> ---
> .../devicetree/bindings/hwmon/pwm-fan.txt | 12 ++
> drivers/hwmon/Kconfig | 9 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/pwm-fan.c | 199 ++++++++++++++++++++
> 4 files changed, 221 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> create mode 100644 drivers/hwmon/pwm-fan.c
[...]
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> new file mode 100644
> index 0000000..78fa627
> --- /dev/null
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -0,0 +1,199 @@
[...]
> +#ifdef CONFIG_PM_SLEEP
> +static int pwm_fan_suspend(struct device *dev)
> +{
> + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +
> + return pwm_config(ctx->pwm, 0, ctx->pwm->period);
> +}
> +
> +static int pwm_fan_resume(struct device *dev)
> +{
> + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +
> + return pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pwm_fan_pm, pwm_fan_suspend, pwm_fan_resume);
If you move this out of #ifdef CONFIG_PM_SLEEP, you won't need the
#ifdef around driver.pm below [1]. SIMPLE_DEV_PM_OPS will just define an
empty struct dev_pm_os if CONFIG_PM_LSEEP is not defined.
> +#endif
> +
> +
> +
> +static struct of_device_id of_pwm_fan_match[] = {
> + { .compatible = "pwm-fan", },
> + {},
> +};
> +
> +static struct platform_driver pwm_fan_driver = {
> + .probe = pwm_fan_probe,
> + .remove = pwm_fan_remove,
> + .driver = {
> + .name = "pwm-fan",
> +#ifdef CONFIG_PM_SLEEP
> + .pm = &pwm_fan_pm,
> +#endif
[1] #ifdef CONFIG_PM_SLEEP not needed with the above change.
> + .of_match_table = of_match_ptr(of_pwm_fan_match),
> + },
> +};
> +
> +module_platform_driver(pwm_fan_driver);
> +
> +MODULE_AUTHOR("Kamil Debski <[email protected]>");
> +MODULE_ALIAS("platform:pwm-fan");
> +MODULE_DESCRIPTION("PWM FAN driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
Hi Guenter,
Thank you for your review.
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck
> Sent: Wednesday, July 09, 2014 6:58 PM
>
> On Wed, Jul 09, 2014 at 04:53:20PM +0200, Kamil Debski wrote:
> > The pwm-fan driver enables control of fans connected to PWM lines.
> > This driver uses the PWM framework, so it is compatible with all PWM
> > devices that provide drivers through the PWM framework.
> >
> > Signed-off-by: Kamil Debski <[email protected]>
> > ---
> > .../devicetree/bindings/hwmon/pwm-fan.txt | 12 ++
> > drivers/hwmon/Kconfig | 9 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/pwm-fan.c | 199
> ++++++++++++++++++++
> > 4 files changed, 221 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > create mode 100644 drivers/hwmon/pwm-fan.c
>
> Documentation/hwmon/pwm-fan is missing.
Ok, I will add it.
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > new file mode 100644
> > index 0000000..f534607
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > @@ -0,0 +1,12 @@
> > +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
> > +
> > +Example:
> > + pwm-fan {
> > + compatible = "pwm-fan";
> > + status = "okay";
> > + pwms = <&pwm 0 10000 0>;
> > + };
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index
> > 08531a1..838ffb8 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1104,6 +1104,15 @@ config SENSORS_PCF8591
> >
> > source drivers/hwmon/pmbus/Kconfig
> >
> > +config SENSORS_PWM_FAN
> > + tristate "PWM fan"
> > + depends on PWM
> > + help
> > + If you say yes here you get support for fans connected to PWM
> lines.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called pwm-fan.
> > +
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and
> compat."
> > depends on GPIOLIB
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index
> > 3dc0f02..3967ecf 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -120,6 +120,7 @@ obj-$(CONFIG_SENSORS_NTC_THERMISTOR) +=
> ntc_thermistor.o
> > obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> > obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > +obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c new
> > file mode 100644 index 0000000..78fa627
> > --- /dev/null
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -0,0 +1,199 @@
> > +/*
> > + * pwm-fan.c - Hwmon driver for fans connected to PWM lines.
> > + *
> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> > + *
> > + * Author: Kamil Debski <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> > +02111-1307 USA
>
> Please run the patch through checkpatch and ensure that all warnings
> and errors are fixed.
I did checkpatch with -f by mistake, so it missed this error. Thanks for
pointing this out.
> > + */
> > +
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
>
> For new drivers, please list include files in alphabetic order.
Ok.
>
> > +
> > +#define MAX_PWM 255
> > +
> > +struct pwm_fan_ctx {
> > + struct device *hwmon;
> > + struct mutex lock;
> > + struct platform_device *pdev;
> > + struct pwm_device *pwm;
> > + unsigned int duty_cycle;
> > + unsigned char pwm_value;
> > +};
> > +
> > +static ssize_t show_pwm_enable(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + /* For the sake of simplicity only mode 1 (PWM) is supported */
> > + return sprintf(buf, "1\n");
> > +}
> > +
> > +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, duty;
> > +
> > + if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM)
> > + return -EINVAL;
> > +
> > + mutex_lock(&ctx->lock);
> > +
> > + if (ctx->pwm_value == pwm)
> > + goto exit_set_pwm;
> > +
> > + duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>
> Unnecessary double space.
Right.
>
> > + if (pwm_config(ctx->pwm, duty, ctx->pwm->period))
> > + goto exit_set_pwm;
>
> Please return the error code.
Right.
>
> > +
> > + ctx->duty_cycle = duty;
> > + ctx->pwm_value = pwm;
> > +
> > +
> Single empty line only please.
Ok.
>
> > +exit_set_pwm:
> > + mutex_unlock(&ctx->lock);
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t show_pwm(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", ctx->pwm_value);
>
> pwm_value is unsigned, so please use %u.
Ok.
>
> > +}
> > +
> > +
> Single empty line only please
Ok.
>
> > +static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
> > + show_pwm_enable, NULL, 0);
>
> Is this attribute really needed ?
I it can be omitted then I see no problem removing this.
>
> > +static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR,
> > + show_pwm, set_pwm, 0);
> > +
> > +static struct attribute *pwm_fan_attributes[] = {
> > + &sensor_dev_attr_pwm1_enable.dev_attr.attr,
> > + &sensor_dev_attr_pwm1.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group pwm_fan_group = {
> > + .attrs = pwm_fan_attributes,
> > +};
> > +
> > +static const struct attribute_group *pwm_fan_groups[] = {
> > + &pwm_fan_group,
> > + NULL
> > +};
>
> Please use the ATTRIBUTE_GROUPS macro.
Ok.
>
> > +
> > +static int pwm_fan_probe(struct platform_device *pdev) {
> > + struct pwm_fan_ctx *ctx;
> > +
> > + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx) {
> > + dev_err(&pdev->dev, "Not enough memory for PWM fan
> device\n");
>
> Unnecessary error message.
Ok.
>
> > + return -ENOMEM;
> > + }
> > +
> > + ctx->pdev = pdev;
> > + mutex_init(&ctx->lock);
> > +
> > + ctx->pwm = devm_of_pwm_get(&pdev->dev, pdev->dev.of_node, 0);
>
> Last argument is a string pointer. Please use NULL.
Ok.
>
> > + if (IS_ERR(ctx->pwm)) {
> > + dev_err(&pdev->dev, "Could not get PWM");
> > + return -EINVAL;
>
> Return PTR_ERR(ctx->pwm).
Right.
>
> > + }
> > +
> > + dev_set_drvdata(&pdev->dev, ctx);
> > + platform_set_drvdata(pdev, ctx);
> > +
> > + /* Set duty cycle to maximum allowed */
> > + ctx->duty_cycle = ctx->pwm->period - 1;
> > + ctx->pwm_value = MAX_PWM;
> > + pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period);
Guenter, I have a question regarding this. I see that gpio-fan reads
the state of gpios and uses this as the initial setting.
Led-pwm on init sets the brightness to LED_OFF.
What should be the default state after init? Should the fan be off or
at full power? I am thinking about adding an optional node to device
tree that could be used to set this. But the question remains - what
should be the default setting?
>
> pwm_config() can return an error. Why not check it here ?
Right.
>
> > +
> > + ctx->hwmon = hwmon_device_register_with_groups(&pdev->dev,
> "pwmfan",
> > + ctx, pwm_fan_groups);
>
> Why not devm_hwmon_device_register_with_groups ?
You're right.
>
> > +
> > + if (IS_ERR(ctx->hwmon)) {
> > + dev_err(&pdev->dev, "Failed to register hwmon device");
> > + return PTR_ERR(ctx->hwmon);
> > + }
> > +
> > + /* Enbale PWM output */
> > + pwm_enable(ctx->pwm);
> > +
> > + return 0;
> > +}
> > +
> > +static int pwm_fan_remove(struct platform_device *pdev) {
> > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
> > +
> > + pwm_disable(ctx->pwm);
>
> Is this needed ? I don't see it used by the leds-pwm driver on cleanup.
>
> > + hwmon_device_unregister(ctx->hwmon);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int pwm_fan_suspend(struct device *dev) {
> > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > +
> > + return pwm_config(ctx->pwm, 0, ctx->pwm->period);
>
> Wonder if this should be pwm_disable instead (and pwm_enable on resume).
> Any thoughts ?
I think this is a good idea. I had also looked on the leds-pwm, that you
mentioned, and the driver also runs pwm_disable for pwm setting of 0, which
seems like a good idea as well.
> > +}
> > +
> > +static int pwm_fan_resume(struct device *dev) {
> > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > +
> > + return pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period); }
> > +
> > +static SIMPLE_DEV_PM_OPS(pwm_fan_pm, pwm_fan_suspend,
> > +pwm_fan_resume); #endif
> > +
> > +
> > +
> Single empty line only please
Ok.
>
> > +static struct of_device_id of_pwm_fan_match[] = {
> > + { .compatible = "pwm-fan", },
> > + {},
> > +};
> > +
> > +static struct platform_driver pwm_fan_driver = {
> > + .probe = pwm_fan_probe,
> > + .remove = pwm_fan_remove,
> > + .driver = {
> > + .name = "pwm-fan",
> > +#ifdef CONFIG_PM_SLEEP
> > + .pm = &pwm_fan_pm,
> > +#endif
> > + .of_match_table = of_match_ptr(of_pwm_fan_match),
> > + },
> > +};
> > +
> > +module_platform_driver(pwm_fan_driver);
> > +
> > +MODULE_AUTHOR("Kamil Debski <[email protected]>");
> > +MODULE_ALIAS("platform:pwm-fan");
> > +MODULE_DESCRIPTION("PWM FAN driver"); MODULE_LICENSE("GPL");
> > --
> > 1.7.9.5
> >
> >
> > _______________________________________________
> > lm-sensors mailing list
> > [email protected]
> > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> >
Hi Tobias,
Thank you for your review.
> From: Tobias Klauser [mailto:[email protected]]
> Sent: Wednesday, July 09, 2014 7:50 PM
>
> On 2014-07-09 at 16:53:20 +0200, Kamil Debski <[email protected]>
> wrote:
> > The pwm-fan driver enables control of fans connected to PWM lines.
> > This driver uses the PWM framework, so it is compatible with all PWM
> > devices that provide drivers through the PWM framework.
> >
> > Signed-off-by: Kamil Debski <[email protected]>
> > ---
> > .../devicetree/bindings/hwmon/pwm-fan.txt | 12 ++
> > drivers/hwmon/Kconfig | 9 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/pwm-fan.c | 199
> ++++++++++++++++++++
> > 4 files changed, 221 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/hwmon/pwm-fan.txt
> > create mode 100644 drivers/hwmon/pwm-fan.c
>
> [...]
>
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c new
> > file mode 100644 index 0000000..78fa627
> > --- /dev/null
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -0,0 +1,199 @@
>
> [...]
>
> > +#ifdef CONFIG_PM_SLEEP
> > +static int pwm_fan_suspend(struct device *dev) {
> > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > +
> > + return pwm_config(ctx->pwm, 0, ctx->pwm->period); }
> > +
> > +static int pwm_fan_resume(struct device *dev) {
> > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > +
> > + return pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period); }
> > +
> > +static SIMPLE_DEV_PM_OPS(pwm_fan_pm, pwm_fan_suspend,
> > +pwm_fan_resume);
>
> If you move this out of #ifdef CONFIG_PM_SLEEP, you won't need the
> #ifdef around driver.pm below [1]. SIMPLE_DEV_PM_OPS will just define
> an empty struct dev_pm_os if CONFIG_PM_LSEEP is not defined.
Thanks for this suggestion.
>
> > +#endif
> > +
> > +
> > +
> > +static struct of_device_id of_pwm_fan_match[] = {
> > + { .compatible = "pwm-fan", },
> > + {},
> > +};
> > +
> > +static struct platform_driver pwm_fan_driver = {
> > + .probe = pwm_fan_probe,
> > + .remove = pwm_fan_remove,
> > + .driver = {
> > + .name = "pwm-fan",
> > +#ifdef CONFIG_PM_SLEEP
> > + .pm = &pwm_fan_pm,
> > +#endif
>
> [1] #ifdef CONFIG_PM_SLEEP not needed with the above change.
>
> > + .of_match_table = of_match_ptr(of_pwm_fan_match),
> > + },
> > +};
> > +
> > +module_platform_driver(pwm_fan_driver);
> > +
> > +MODULE_AUTHOR("Kamil Debski <[email protected]>");
> > +MODULE_ALIAS("platform:pwm-fan");
> > +MODULE_DESCRIPTION("PWM FAN driver"); MODULE_LICENSE("GPL");
> > --
> > 1.7.9.5
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
Hi Kamil,
On 07/10/2014 08:17 AM, Kamil Debski wrote:
>
>>
>>> + }
>>> +
>>> + dev_set_drvdata(&pdev->dev, ctx);
>>> + platform_set_drvdata(pdev, ctx);
>>> +
>>> + /* Set duty cycle to maximum allowed */
>>> + ctx->duty_cycle = ctx->pwm->period - 1;
>>> + ctx->pwm_value = MAX_PWM;
>>> + pwm_config(ctx->pwm, ctx->duty_cycle, ctx->pwm->period);
>
> Guenter, I have a question regarding this. I see that gpio-fan reads
> the state of gpios and uses this as the initial setting.
> Led-pwm on init sets the brightness to LED_OFF.
>
> What should be the default state after init? Should the fan be off or
> at full power? I am thinking about adding an optional node to device
> tree that could be used to set this. But the question remains - what
> should be the default setting?
>
Trick question ... I can not really answer it. you can try the devicetree
option, but be prepared to have it rejected - the usual argument is that
dt shall only describe the HW but not its configuration.
Ultimately, either way is fine with me.
Guenter
Hi,
I forgot to address one of the comments.
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck
> Sent: Wednesday, July 09, 2014 6:58 PM
> To: Kamil Debski
> Cc: [email protected]; [email protected]; lm-
> [email protected]; [email protected]; [email protected]
> Subject: Re: [lm-sensors] [PATCH] hwmon: pwm-fan: Add pwm-fan driver
>
> On Wed, Jul 09, 2014 at 04:53:20PM +0200, Kamil Debski wrote:
> > The pwm-fan driver enables control of fans connected to PWM lines.
> > This driver uses the PWM framework, so it is compatible with all PWM
> > devices that provide drivers through the PWM framework.
> >
> > Signed-off-by: Kamil Debski <[email protected]>
> > ---
> > .../devicetree/bindings/hwmon/pwm-fan.txt | 12 ++
> > drivers/hwmon/Kconfig | 9 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/pwm-fan.c | 199
<snip>
> > +static int pwm_fan_remove(struct platform_device *pdev) {
> > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
> > +
> > + pwm_disable(ctx->pwm);
>
> Is this needed ? I don't see it used by the leds-pwm driver on cleanup.
The pwm-samsung driver does not disable the PWM channel according to my
understanding of the code. But for example the pwm-renesas-tpu does
stop the PWM timer on free. What is the correct behavior?
Is the channel should be disabled on free then I agree that pwm_disable
is not necessary.
> > + hwmon_device_unregister(ctx->hwmon);
> > +
> > + return 0;
> > +}
> > +
<snip>
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland