* [PATCH 4/5] hwmon: DNS323 rev C1 fan support
@ 2010-05-22 10:54 Benjamin Herrenschmidt
2010-05-22 11:00 ` Benjamin Herrenschmidt
2010-10-13 11:59 ` Simon Guinot
0 siblings, 2 replies; 33+ messages in thread
From: Benjamin Herrenschmidt @ 2010-05-22 10:54 UTC (permalink / raw)
To: linux-arm-kernel
The hardware only supports 3 settings: off, slow and fast.
In order to have a chance to work with existing fan control systems,
we emulate a PWM device with the following mapping:
0.. 15 off 0 RPM input
16..127 slow 100 RPM input
128..255 fast 2000 RPM input
This provides something more/less working with fancontrol, though
it does have a tendency to work by doing short bursts of "slow"
speed every half a minute as it settles around my min temp. Not
a big deal a specialized script could probably do better, or even
tweaks to fancontrol config. At leats it should be safe.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: lm-sensors at lm-sensors.org
---
drivers/hwmon/Kconfig | 12 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/dns323c-fan.c | 271 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 284 insertions(+), 0 deletions(-)
create mode 100644 drivers/hwmon/dns323c-fan.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 9be8e17..5f55735 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1087,6 +1087,18 @@ config SENSORS_MC13783_ADC
help
Support for the A/D converter on MC13783 PMIC.
+config SENSORS_DNS323C_FAN
+ tristate "D-Link DNS323 rev C1 Fan"
+ depends on MACH_DNS323
+ help
+ Support for the GPIO based fan control on the D-Link DNS323
+ HW revision C1. This exposes a pseudo pwm device with the
+ following values supported:
+
+ 0..15 : Fan off
+ 16..127 : Fan on low speed
+ 128..255 : Fan on high speed
+
if ACPI
comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 4aa1a3d..15bcdef 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o
obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o
obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
+obj-$(CONFIG_SENSORS_DNS323C_FAN)+= dns323c-fan.o
ifeq ($(CONFIG_HWMON_DEBUG_CHIP),y)
EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/hwmon/dns323c-fan.c b/drivers/hwmon/dns323c-fan.c
new file mode 100644
index 0000000..4ae18d0
--- /dev/null
+++ b/drivers/hwmon/dns323c-fan.c
@@ -0,0 +1,271 @@
+/*
+ * dns323c_fan - Driver for the D-LINK DNS-323 rev C1 fan control
+ *
+ * Copyright 2010 Benjamin Herrenschmidt
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/gpio.h>
+#include <linux/hwmon.h>
+
+#include <linux/hwmon-sysfs.h>
+#include <linux/gfp.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/sysfs.h>
+#include <linux/platform_device.h>
+
+enum fan_speed {
+ FAN_OFF,
+ FAN_LOW ,
+ FAN_FAST,
+ FAN_FAST_LOCK,
+};
+
+#define DNS323C_GPIO_FAN_BIT1 18
+#define DNS323C_GPIO_FAN_BIT0 19
+
+struct dns323c_fan {
+ struct device *hwmon;
+ struct mutex lock;
+ enum fan_speed speed;
+};
+
+static void __set_fan_speed(struct dns323c_fan *fan, enum fan_speed speed)
+{
+ if (speed == fan->speed)
+ return;
+
+ switch(speed) {
+ case FAN_OFF:
+ gpio_set_value(DNS323C_GPIO_FAN_BIT1, 0);
+ gpio_set_value(DNS323C_GPIO_FAN_BIT0, 0);
+ break;
+ case FAN_LOW:
+ gpio_set_value(DNS323C_GPIO_FAN_BIT1, 0);
+ gpio_set_value(DNS323C_GPIO_FAN_BIT0, 1);
+ break;
+ default:
+ gpio_set_value(DNS323C_GPIO_FAN_BIT0, 0);
+ gpio_set_value(DNS323C_GPIO_FAN_BIT1, 1);
+ };
+ fan->speed = speed;
+}
+
+static ssize_t show_name(struct device *dev, struct device_attribute *da,
+ char *buf)
+{
+ return sprintf(buf, "dns323c-fan\n");
+}
+
+static ssize_t show_pwm(struct device *dev, struct device_attribute *da,
+ char *buf)
+{
+ struct dns323c_fan *fan = dev_get_drvdata(dev);
+ int pseudo_pwm;
+
+ switch(fan->speed) {
+ case FAN_OFF:
+ pseudo_pwm = 0;
+ break;
+ case FAN_LOW:
+ pseudo_pwm = 63;
+ break;
+ default:
+ pseudo_pwm = 255;
+ }
+ return sprintf(buf, "%d\n", pseudo_pwm);
+}
+
+static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
+ const char *buf, size_t count)
+{
+ struct dns323c_fan *fan = dev_get_drvdata(dev);
+ enum fan_speed speed;
+ unsigned long val;
+
+ if (strict_strtoul(buf, 10, &val))
+ return -EINVAL;
+ if (fan->speed == FAN_FAST_LOCK)
+ return count;
+
+ mutex_lock(&fan->lock);
+ if (val < 16)
+ speed = FAN_OFF;
+ else if (val < 128)
+ speed = FAN_LOW;
+ else
+ speed = FAN_FAST;
+ __set_fan_speed(fan, speed);
+ mutex_unlock(&fan->lock);
+
+ return count;
+}
+
+static ssize_t show_pwm_en(struct device *dev, struct device_attribute *da,
+ char *buf)
+{
+ struct dns323c_fan *fan = dev_get_drvdata(dev);
+
+ if (fan->speed == FAN_FAST_LOCK)
+ return sprintf(buf, "0\n");
+ else
+ return sprintf(buf, "1\n");
+}
+
+static ssize_t set_pwm_en(struct device *dev, struct device_attribute *da,
+ const char *buf, size_t count)
+{
+ struct dns323c_fan *fan = dev_get_drvdata(dev);
+ enum fan_speed speed;
+ unsigned long val;
+
+ if (strict_strtoul(buf, 10, &val))
+ return -EINVAL;
+ if (val != 0 && val != 1)
+ return -EINVAL;
+
+ mutex_lock(&fan->lock);
+ if (val == 0 && fan->speed != FAN_FAST_LOCK)
+ speed = FAN_FAST_LOCK;
+ else if (val != 0 && fan->speed == FAN_FAST_LOCK)
+ speed = FAN_FAST;
+ else
+ speed = fan->speed;
+ __set_fan_speed(fan, speed);
+ mutex_unlock(&fan->lock);
+
+ return count;
+}
+
+static ssize_t show_fake_rpm(struct device *dev, struct device_attribute *da,
+ char *buf)
+{
+ struct dns323c_fan *fan = dev_get_drvdata(dev);
+ int pseudo_rpm;
+
+ switch(fan->speed) {
+ case FAN_OFF:
+ pseudo_rpm = 0;
+ break;
+ case FAN_LOW:
+ pseudo_rpm = 400;
+ break;
+ default:
+ pseudo_rpm = 2000;
+ }
+ return sprintf(buf, "%d\n", pseudo_rpm);
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+static DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm);
+static DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR, show_pwm_en, set_pwm_en);
+static DEVICE_ATTR(fan1_input, S_IRUGO, show_fake_rpm, NULL);
+
+static int dns323c_fan_probe(struct platform_device *pdev)
+{
+ struct dns323c_fan *fan = NULL;
+ int ret = -ENXIO;
+
+ /* Get the GPIOs */
+ if (gpio_request(DNS323C_GPIO_FAN_BIT0, "FAN0") != 0) {
+ pr_err("dns323c_fan: Failed to request fan GPIO 0 !\n");
+ return -ENXIO;
+ }
+ if (gpio_request(DNS323C_GPIO_FAN_BIT1, "FAN1") != 0) {
+ pr_err("dns323c_fan: Failed to request fan GPIO 1 !\n");
+ goto err_gpio;
+ }
+
+ /* Set directions to output and medium speed. We write bit 1 first
+ * since it contains 0 to avoid having a transitory 11 state which
+ * isn't supported
+ */
+ gpio_direction_output(DNS323C_GPIO_FAN_BIT1, 0);
+ gpio_direction_output(DNS323C_GPIO_FAN_BIT0, 1);
+
+ /* Grab some memory for our state */
+ fan = kzalloc(sizeof(struct dns323c_fan), GFP_KERNEL);
+ if (!fan) {
+ ret = -ENOMEM;
+ goto err_alloc;
+ }
+ fan->speed = FAN_LOW;
+ mutex_init(&fan->lock);
+ platform_set_drvdata(pdev, fan);
+
+ ret = device_create_file(&pdev->dev, &dev_attr_name);
+ ret |= device_create_file(&pdev->dev, &dev_attr_pwm1);
+ ret |= device_create_file(&pdev->dev, &dev_attr_pwm1_enable);
+ ret |= device_create_file(&pdev->dev, &dev_attr_fan1_input);
+ if (ret)
+ goto err_file;
+
+ fan->hwmon = hwmon_device_register(&pdev->dev);
+ if (IS_ERR(fan->hwmon)) {
+ ret = PTR_ERR(fan->hwmon);
+ goto err_dev;
+ }
+ return 0;
+
+ err_dev:
+ device_remove_file(&pdev->dev, &dev_attr_name);
+ device_remove_file(&pdev->dev, &dev_attr_pwm1);
+ device_remove_file(&pdev->dev, &dev_attr_pwm1_enable);
+ device_remove_file(&pdev->dev, &dev_attr_fan1_input);
+ err_file:
+ kfree(fan);
+ err_alloc:
+ gpio_free(DNS323C_GPIO_FAN_BIT1);
+ err_gpio:
+ gpio_free(DNS323C_GPIO_FAN_BIT0);
+ return ret;
+}
+
+static int __devexit dns323c_fan_remove(struct platform_device *pdev)
+{
+ struct dns323c_fan *fan = platform_get_drvdata(pdev);
+
+ hwmon_device_unregister(fan->hwmon);
+ device_remove_file(&pdev->dev, &dev_attr_name);
+ device_remove_file(&pdev->dev, &dev_attr_pwm1);
+ device_remove_file(&pdev->dev, &dev_attr_pwm1_enable);
+ device_remove_file(&pdev->dev, &dev_attr_fan1_input);
+ kfree(fan);
+ gpio_free(DNS323C_GPIO_FAN_BIT1);
+ gpio_free(DNS323C_GPIO_FAN_BIT0);
+ return 0;
+}
+
+static struct platform_driver dns323c_fan_driver = {
+ .probe = dns323c_fan_probe,
+ .remove = __devexit_p(dns323c_fan_remove),
+ .driver = {
+ .name = "dns323c-fan",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init dns323c_fan_init(void)
+{
+ return platform_driver_register(&dns323c_fan_driver);
+}
+
+static void __exit dns323c_fan_exit(void)
+{
+ platform_driver_unregister(&dns323c_fan_driver);
+}
+
+MODULE_AUTHOR("Benjamin Herrenschmidt <benh@kernel.crashing.org>");
+MODULE_DESCRIPTION("DNS323 RevC1 Fan control");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:dns323c-fan");
+
+module_init(dns323c_fan_init);
+module_exit(dns323c_fan_exit);
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 4/5] hwmon: DNS323 rev C1 fan support
2010-05-22 10:54 [PATCH 4/5] hwmon: DNS323 rev C1 fan support Benjamin Herrenschmidt
@ 2010-05-22 11:00 ` Benjamin Herrenschmidt
2010-10-13 11:59 ` Simon Guinot
1 sibling, 0 replies; 33+ messages in thread
From: Benjamin Herrenschmidt @ 2010-05-22 11:00 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, 2010-05-22 at 20:54 +1000, Benjamin Herrenschmidt wrote:
> The hardware only supports 3 settings: off, slow and fast.
>
> In order to have a chance to work with existing fan control systems,
> we emulate a PWM device with the following mapping:
>
> 0.. 15 off 0 RPM input
> 16..127 slow 100 RPM input
^^^
Little typo in the changeset comment, I actually fake 400 RPM, which
sounds more realistic from a gut feeling of how the fan is going but
I haven't actually measured.
Cheers,
Ben.
> 128..255 fast 2000 RPM input
>
> This provides something more/less working with fancontrol, though
> it does have a tendency to work by doing short bursts of "slow"
> speed every half a minute as it settles around my min temp. Not
> a big deal a specialized script could probably do better, or even
> tweaks to fancontrol config. At leats it should be safe.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: lm-sensors at lm-sensors.org
> ---
> drivers/hwmon/Kconfig | 12 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/dns323c-fan.c | 271 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 284 insertions(+), 0 deletions(-)
> create mode 100644 drivers/hwmon/dns323c-fan.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 9be8e17..5f55735 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1087,6 +1087,18 @@ config SENSORS_MC13783_ADC
> help
> Support for the A/D converter on MC13783 PMIC.
>
> +config SENSORS_DNS323C_FAN
> + tristate "D-Link DNS323 rev C1 Fan"
> + depends on MACH_DNS323
> + help
> + Support for the GPIO based fan control on the D-Link DNS323
> + HW revision C1. This exposes a pseudo pwm device with the
> + following values supported:
> +
> + 0..15 : Fan off
> + 16..127 : Fan on low speed
> + 128..255 : Fan on high speed
> +
> if ACPI
>
> comment "ACPI drivers"
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 4aa1a3d..15bcdef 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o
> obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o
> obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
> +obj-$(CONFIG_SENSORS_DNS323C_FAN)+= dns323c-fan.o
>
> ifeq ($(CONFIG_HWMON_DEBUG_CHIP),y)
> EXTRA_CFLAGS += -DDEBUG
> diff --git a/drivers/hwmon/dns323c-fan.c b/drivers/hwmon/dns323c-fan.c
> new file mode 100644
> index 0000000..4ae18d0
> --- /dev/null
> +++ b/drivers/hwmon/dns323c-fan.c
> @@ -0,0 +1,271 @@
> +/*
> + * dns323c_fan - Driver for the D-LINK DNS-323 rev C1 fan control
> + *
> + * Copyright 2010 Benjamin Herrenschmidt
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/gpio.h>
> +#include <linux/hwmon.h>
> +
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/gfp.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/sysfs.h>
> +#include <linux/platform_device.h>
> +
> +enum fan_speed {
> + FAN_OFF,
> + FAN_LOW ,
> + FAN_FAST,
> + FAN_FAST_LOCK,
> +};
> +
> +#define DNS323C_GPIO_FAN_BIT1 18
> +#define DNS323C_GPIO_FAN_BIT0 19
> +
> +struct dns323c_fan {
> + struct device *hwmon;
> + struct mutex lock;
> + enum fan_speed speed;
> +};
> +
> +static void __set_fan_speed(struct dns323c_fan *fan, enum fan_speed speed)
> +{
> + if (speed == fan->speed)
> + return;
> +
> + switch(speed) {
> + case FAN_OFF:
> + gpio_set_value(DNS323C_GPIO_FAN_BIT1, 0);
> + gpio_set_value(DNS323C_GPIO_FAN_BIT0, 0);
> + break;
> + case FAN_LOW:
> + gpio_set_value(DNS323C_GPIO_FAN_BIT1, 0);
> + gpio_set_value(DNS323C_GPIO_FAN_BIT0, 1);
> + break;
> + default:
> + gpio_set_value(DNS323C_GPIO_FAN_BIT0, 0);
> + gpio_set_value(DNS323C_GPIO_FAN_BIT1, 1);
> + };
> + fan->speed = speed;
> +}
> +
> +static ssize_t show_name(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + return sprintf(buf, "dns323c-fan\n");
> +}
> +
> +static ssize_t show_pwm(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + struct dns323c_fan *fan = dev_get_drvdata(dev);
> + int pseudo_pwm;
> +
> + switch(fan->speed) {
> + case FAN_OFF:
> + pseudo_pwm = 0;
> + break;
> + case FAN_LOW:
> + pseudo_pwm = 63;
> + break;
> + default:
> + pseudo_pwm = 255;
> + }
> + return sprintf(buf, "%d\n", pseudo_pwm);
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
> + const char *buf, size_t count)
> +{
> + struct dns323c_fan *fan = dev_get_drvdata(dev);
> + enum fan_speed speed;
> + unsigned long val;
> +
> + if (strict_strtoul(buf, 10, &val))
> + return -EINVAL;
> + if (fan->speed == FAN_FAST_LOCK)
> + return count;
> +
> + mutex_lock(&fan->lock);
> + if (val < 16)
> + speed = FAN_OFF;
> + else if (val < 128)
> + speed = FAN_LOW;
> + else
> + speed = FAN_FAST;
> + __set_fan_speed(fan, speed);
> + mutex_unlock(&fan->lock);
> +
> + return count;
> +}
> +
> +static ssize_t show_pwm_en(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + struct dns323c_fan *fan = dev_get_drvdata(dev);
> +
> + if (fan->speed == FAN_FAST_LOCK)
> + return sprintf(buf, "0\n");
> + else
> + return sprintf(buf, "1\n");
> +}
> +
> +static ssize_t set_pwm_en(struct device *dev, struct device_attribute *da,
> + const char *buf, size_t count)
> +{
> + struct dns323c_fan *fan = dev_get_drvdata(dev);
> + enum fan_speed speed;
> + unsigned long val;
> +
> + if (strict_strtoul(buf, 10, &val))
> + return -EINVAL;
> + if (val != 0 && val != 1)
> + return -EINVAL;
> +
> + mutex_lock(&fan->lock);
> + if (val == 0 && fan->speed != FAN_FAST_LOCK)
> + speed = FAN_FAST_LOCK;
> + else if (val != 0 && fan->speed == FAN_FAST_LOCK)
> + speed = FAN_FAST;
> + else
> + speed = fan->speed;
> + __set_fan_speed(fan, speed);
> + mutex_unlock(&fan->lock);
> +
> + return count;
> +}
> +
> +static ssize_t show_fake_rpm(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + struct dns323c_fan *fan = dev_get_drvdata(dev);
> + int pseudo_rpm;
> +
> + switch(fan->speed) {
> + case FAN_OFF:
> + pseudo_rpm = 0;
> + break;
> + case FAN_LOW:
> + pseudo_rpm = 400;
> + break;
> + default:
> + pseudo_rpm = 2000;
> + }
> + return sprintf(buf, "%d\n", pseudo_rpm);
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +static DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm);
> +static DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR, show_pwm_en, set_pwm_en);
> +static DEVICE_ATTR(fan1_input, S_IRUGO, show_fake_rpm, NULL);
> +
> +static int dns323c_fan_probe(struct platform_device *pdev)
> +{
> + struct dns323c_fan *fan = NULL;
> + int ret = -ENXIO;
> +
> + /* Get the GPIOs */
> + if (gpio_request(DNS323C_GPIO_FAN_BIT0, "FAN0") != 0) {
> + pr_err("dns323c_fan: Failed to request fan GPIO 0 !\n");
> + return -ENXIO;
> + }
> + if (gpio_request(DNS323C_GPIO_FAN_BIT1, "FAN1") != 0) {
> + pr_err("dns323c_fan: Failed to request fan GPIO 1 !\n");
> + goto err_gpio;
> + }
> +
> + /* Set directions to output and medium speed. We write bit 1 first
> + * since it contains 0 to avoid having a transitory 11 state which
> + * isn't supported
> + */
> + gpio_direction_output(DNS323C_GPIO_FAN_BIT1, 0);
> + gpio_direction_output(DNS323C_GPIO_FAN_BIT0, 1);
> +
> + /* Grab some memory for our state */
> + fan = kzalloc(sizeof(struct dns323c_fan), GFP_KERNEL);
> + if (!fan) {
> + ret = -ENOMEM;
> + goto err_alloc;
> + }
> + fan->speed = FAN_LOW;
> + mutex_init(&fan->lock);
> + platform_set_drvdata(pdev, fan);
> +
> + ret = device_create_file(&pdev->dev, &dev_attr_name);
> + ret |= device_create_file(&pdev->dev, &dev_attr_pwm1);
> + ret |= device_create_file(&pdev->dev, &dev_attr_pwm1_enable);
> + ret |= device_create_file(&pdev->dev, &dev_attr_fan1_input);
> + if (ret)
> + goto err_file;
> +
> + fan->hwmon = hwmon_device_register(&pdev->dev);
> + if (IS_ERR(fan->hwmon)) {
> + ret = PTR_ERR(fan->hwmon);
> + goto err_dev;
> + }
> + return 0;
> +
> + err_dev:
> + device_remove_file(&pdev->dev, &dev_attr_name);
> + device_remove_file(&pdev->dev, &dev_attr_pwm1);
> + device_remove_file(&pdev->dev, &dev_attr_pwm1_enable);
> + device_remove_file(&pdev->dev, &dev_attr_fan1_input);
> + err_file:
> + kfree(fan);
> + err_alloc:
> + gpio_free(DNS323C_GPIO_FAN_BIT1);
> + err_gpio:
> + gpio_free(DNS323C_GPIO_FAN_BIT0);
> + return ret;
> +}
> +
> +static int __devexit dns323c_fan_remove(struct platform_device *pdev)
> +{
> + struct dns323c_fan *fan = platform_get_drvdata(pdev);
> +
> + hwmon_device_unregister(fan->hwmon);
> + device_remove_file(&pdev->dev, &dev_attr_name);
> + device_remove_file(&pdev->dev, &dev_attr_pwm1);
> + device_remove_file(&pdev->dev, &dev_attr_pwm1_enable);
> + device_remove_file(&pdev->dev, &dev_attr_fan1_input);
> + kfree(fan);
> + gpio_free(DNS323C_GPIO_FAN_BIT1);
> + gpio_free(DNS323C_GPIO_FAN_BIT0);
> + return 0;
> +}
> +
> +static struct platform_driver dns323c_fan_driver = {
> + .probe = dns323c_fan_probe,
> + .remove = __devexit_p(dns323c_fan_remove),
> + .driver = {
> + .name = "dns323c-fan",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init dns323c_fan_init(void)
> +{
> + return platform_driver_register(&dns323c_fan_driver);
> +}
> +
> +static void __exit dns323c_fan_exit(void)
> +{
> + platform_driver_unregister(&dns323c_fan_driver);
> +}
> +
> +MODULE_AUTHOR("Benjamin Herrenschmidt <benh@kernel.crashing.org>");
> +MODULE_DESCRIPTION("DNS323 RevC1 Fan control");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:dns323c-fan");
> +
> +module_init(dns323c_fan_init);
> +module_exit(dns323c_fan_exit);
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 4/5] hwmon: DNS323 rev C1 fan support
2010-05-22 10:54 [PATCH 4/5] hwmon: DNS323 rev C1 fan support Benjamin Herrenschmidt
2010-05-22 11:00 ` Benjamin Herrenschmidt
@ 2010-10-13 11:59 ` Simon Guinot
2010-10-13 16:34 ` [lm-sensors] " Guenter Roeck
1 sibling, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-13 11:59 UTC (permalink / raw)
To: linux-arm-kernel
Hi Benjamin,
What is the status for the DNS323 fan support ?
Have this patch been merged yet ?
I am currently looking to add hwmon support for the GPIO fan found
on Network Space Max v2 boards. Obviously, some attributes are shared
with the DNS323 fan. Maybe there is some room for a generic GPIO fan
driver ?
Platform data could provide the board specific GPIO pinout and a speed
conversion array (rpm from/to GPIO value). Here is a proposal for this
platform data interface:
struct gpio_fan {
const char *name;
unsigned gpio;
unsigned active_low;
};
struct gpio_fan_speed {
int value;
int rpm;
};
struct gpio_fan_platform_data {
struct gpio_fan *alarm; /* fan alarm GPIO. */
struct gpio_fan *ctrl; /* fan control GPIOs. */
int num_ctrl;
/*
* Speed conversion array: rpm from/to GPIO bit field.
* This array _must_ be sorted in ascending rpm order.
*/
struct gpio_fan_speed *speed;
int num_speed;
};
Based on this informations the GPIO fan driver could perform the
speed conversions (pwm, rpm, GPIO value) and then provide a hwmon
interface.
Thanks for advice.
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101013/4327f1b5/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [lm-sensors] [PATCH 4/5] hwmon: DNS323 rev C1 fan support
2010-10-13 11:59 ` Simon Guinot
@ 2010-10-13 16:34 ` Guenter Roeck
2010-10-17 15:40 ` Simon Guinot
0 siblings, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-13 16:34 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 2010-10-13 at 07:59 -0400, Simon Guinot wrote:
> Hi Benjamin,
>
> What is the status for the DNS323 fan support ?
> Have this patch been merged yet ?
>
The most recent version I found on the web (dated June 13) has problems
with error detection and reporting, and fails to remove sysfs files in
some error conditions. So I would be surprised if it was merged, unless
there is a more recent version which I missed.
On a side note, there are several versions of the patch on the web, none
indicating what has changed between versions. Would be great to have
such data attached to the various patch versions.
> I am currently looking to add hwmon support for the GPIO fan found
> on Network Space Max v2 boards. Obviously, some attributes are shared
> with the DNS323 fan. Maybe there is some room for a generic GPIO fan
> driver ?
>
> Platform data could provide the board specific GPIO pinout and a speed
> conversion array (rpm from/to GPIO value). Here is a proposal for this
> platform data interface:
>
> struct gpio_fan {
> const char *name;
> unsigned gpio;
> unsigned active_low;
> };
>
> struct gpio_fan_speed {
> int value;
> int rpm;
> };
>
> struct gpio_fan_platform_data {
> struct gpio_fan *alarm; /* fan alarm GPIO. */
> struct gpio_fan *ctrl; /* fan control GPIOs. */
> int num_ctrl;
> /*
> * Speed conversion array: rpm from/to GPIO bit field.
> * This array _must_ be sorted in ascending rpm order.
> */
> struct gpio_fan_speed *speed;
> int num_speed;
> };
>
> Based on this informations the GPIO fan driver could perform the
> speed conversions (pwm, rpm, GPIO value) and then provide a hwmon
> interface.
>
Sounds like a good idea to me.
Couple of comments.
Personally, I prefer to see num_XXX variables before the actual objects,
but maybe that is just a personal preference.
I am not sure what one would do with "active_low". Would that be used
for the alarm ?
Adding a "fault" object might make sense.
It seems that bit write order is missing. Since it is not always the
same, as the proposed dns323 driver indicates, that might be a tricky
problem to solve. You would have to come up with an at least somewhat
generic solution for that.
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [lm-sensors] [PATCH 4/5] hwmon: DNS323 rev C1 fan support
2010-10-13 16:34 ` [lm-sensors] " Guenter Roeck
@ 2010-10-17 15:40 ` Simon Guinot
2010-10-17 15:50 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Simon Guinot
0 siblings, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-17 15:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
Thanks for your comments.
On Wed, Oct 13, 2010 at 09:34:10AM -0700, Guenter Roeck wrote:
> On Wed, 2010-10-13 at 07:59 -0400, Simon Guinot wrote:
> > Hi Benjamin,
> >
> > What is the status for the DNS323 fan support ?
> > Have this patch been merged yet ?
> >
> The most recent version I found on the web (dated June 13) has problems
> with error detection and reporting, and fails to remove sysfs files in
> some error conditions. So I would be surprised if it was merged, unless
> there is a more recent version which I missed.
>
> On a side note, there are several versions of the patch on the web, none
> indicating what has changed between versions. Would be great to have
> such data attached to the various patch versions.
>
> > I am currently looking to add hwmon support for the GPIO fan found
> > on Network Space Max v2 boards. Obviously, some attributes are shared
> > with the DNS323 fan. Maybe there is some room for a generic GPIO fan
> > driver ?
> >
> > Platform data could provide the board specific GPIO pinout and a speed
> > conversion array (rpm from/to GPIO value). Here is a proposal for this
> > platform data interface:
> >
> > struct gpio_fan {
> > const char *name;
> > unsigned gpio;
> > unsigned active_low;
> > };
> >
> > struct gpio_fan_speed {
> > int value;
> > int rpm;
> > };
> >
> > struct gpio_fan_platform_data {
> > struct gpio_fan *alarm; /* fan alarm GPIO. */
> > struct gpio_fan *ctrl; /* fan control GPIOs. */
> > int num_ctrl;
> > /*
> > * Speed conversion array: rpm from/to GPIO bit field.
> > * This array _must_ be sorted in ascending rpm order.
> > */
> > struct gpio_fan_speed *speed;
> > int num_speed;
> > };
> >
> > Based on this informations the GPIO fan driver could perform the
> > speed conversions (pwm, rpm, GPIO value) and then provide a hwmon
> > interface.
> >
> Sounds like a good idea to me.
>
> Couple of comments.
>
> Personally, I prefer to see num_XXX variables before the actual objects,
> but maybe that is just a personal preference.
Ok.
>
> I am not sure what one would do with "active_low". Would that be used
> for the alarm ?
Yes.
>
> Adding a "fault" object might make sense.
Yes.
>
> It seems that bit write order is missing. Since it is not always the
> same, as the proposed dns323 driver indicates, that might be a tricky
> problem to solve. You would have to come up with an at least somewhat
> generic solution for that.
Prevent from writing illegal GPIO values during some transitional states
is really a painful task. This workaround looks really due to a hardware
specific limitation. A different GPIO fan hardware could provide
different limitations. As an example, enabling a latch could be needed
to set the fan speed...
No one can predict all the possible hardware designs for a GPIO fan :)
So, rather than dealing with the specific issues within the driver, we
could simply allow board-setup code to override the "generic"
{get,set}_fan_control functions.
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101017/505faae0/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-17 15:40 ` Simon Guinot
@ 2010-10-17 15:50 ` Simon Guinot
2010-10-17 15:50 ` [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2 Simon Guinot
` (2 more replies)
0 siblings, 3 replies; 33+ messages in thread
From: Simon Guinot @ 2010-10-17 15:50 UTC (permalink / raw)
To: linux-arm-kernel
From: Simon Guinot <sguinot@lacie.com>
This patch adds hwmon support for the GPIO connected fan.
Platform specific informations as GPIO pinout and speed conversion array
(rpm from/to GPIO value) are passed to the driver via platform_data.
Signed-off-by: Simon Guinot <sguinot@lacie.com>
---
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/gpio-fan.c | 508 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/gpio-fan.h | 43 ++++
4 files changed, 561 insertions(+), 0 deletions(-)
create mode 100644 drivers/hwmon/gpio-fan.c
create mode 100644 include/linux/gpio-fan.h
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4d4d09b..1dc57c1 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -399,6 +399,15 @@ config SENSORS_GL520SM
This driver can also be built as a module. If so, the module
will be called gl520sm.
+config SENSORS_GPIO_FAN
+ tristate "GPIO fan"
+ depends on GENERIC_GPIO
+ help
+ If you say yes here you get support for the GPIO connected fan.
+
+ This driver can also be built as a module. If so, the module
+ will be called gpio-fan.
+
config SENSORS_CORETEMP
tristate "Intel Core/Core2/Atom temperature sensor"
depends on X86 && PCI && EXPERIMENTAL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e3c2484..a793e28 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_FSCHMD) += fschmd.o
obj-$(CONFIG_SENSORS_G760A) += g760a.o
obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
+obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o
diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
new file mode 100644
index 0000000..6cc8205
--- /dev/null
+++ b/drivers/hwmon/gpio-fan.c
@@ -0,0 +1,508 @@
+/*
+ * gpio-fan.c - Hwmon driver for GPIO connected fan.
+ *
+ * Copyright (C) 2010 LaCie
+ *
+ * Author: Simon Guinot <sguinot@lacie.com>
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/hwmon.h>
+#include <linux/gpio.h>
+#include <linux/gpio-fan.h>
+
+struct gpio_fan_data {
+ struct platform_device *pdev;
+ struct device *hwmon_dev;
+ struct mutex lock; /* lock GPIOs operations. */
+ int num_ctrl;
+ unsigned *ctrl;
+ int ctrl_val;
+ int num_speed;
+ struct gpio_fan_speed *speed;
+ int pwm_enable;
+ struct gpio_fan_alarm *alarm;
+ struct work_struct alarm_work;
+ void (*platform_set_ctrl)(int num_ctrl, unsigned *ctrl, int ctrl_val);
+};
+
+/*
+ * Alarm GPIO.
+ */
+
+static void fan_alarm_notify(struct work_struct *ws)
+{
+ struct gpio_fan_data *fan_data =
+ container_of(ws, struct gpio_fan_data, alarm_work);
+
+ sysfs_notify(&fan_data->pdev->dev.kobj, NULL, "fan1_alarm");
+ kobject_uevent(&fan_data->pdev->dev.kobj, KOBJ_CHANGE);
+}
+
+static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id)
+{
+ struct gpio_fan_data *fan_data = dev_id;
+
+ schedule_work(&fan_data->alarm_work);
+
+ return IRQ_HANDLED;
+}
+
+static ssize_t show_fan_alarm(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+ struct gpio_fan_alarm *alarm = fan_data->alarm;
+ int value = gpio_get_value(alarm->gpio);
+
+ if (alarm->active_low)
+ value = !value;
+
+ return sprintf(buf, "%d\n", value);
+}
+
+static DEVICE_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL);
+
+static int __devinit
+fan_alarm_init(struct gpio_fan_data *fan_data, struct gpio_fan_alarm *alarm)
+{
+ int err;
+ int alarm_irq;
+ struct platform_device *pdev = fan_data->pdev;
+
+ fan_data->alarm = alarm;
+
+ err = gpio_request(alarm->gpio, "GPIO fan alarm");
+ if (err)
+ return err;
+
+ err = gpio_direction_input(alarm->gpio);
+ if (err)
+ goto err_free_gpio;
+
+ err = device_create_file(&pdev->dev, &dev_attr_fan1_alarm);
+ if (err)
+ goto err_free_gpio;
+
+ INIT_WORK(&fan_data->alarm_work, fan_alarm_notify);
+ alarm_irq = gpio_to_irq(alarm->gpio);
+ set_irq_type(alarm_irq, IRQ_TYPE_EDGE_BOTH);
+ err = request_irq(alarm_irq, fan_alarm_irq_handler, 0,
+ "GPIO fan alarm", fan_data);
+ if (err)
+ goto err_free_sysfs;
+
+ return 0;
+
+err_free_sysfs:
+ device_remove_file(&pdev->dev, &dev_attr_fan1_alarm);
+err_free_gpio:
+ gpio_free(alarm->gpio);
+
+ return err;
+}
+
+static void __devexit fan_alarm_free(struct gpio_fan_data *fan_data)
+{
+ struct platform_device *pdev = fan_data->pdev;
+
+ free_irq(gpio_to_irq(fan_data->alarm->gpio), fan_data);
+ device_remove_file(&pdev->dev, &dev_attr_fan1_alarm);
+ gpio_free(fan_data->alarm->gpio);
+}
+
+/*
+ * Control GPIOs.
+ */
+
+static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
+{
+ int i;
+ int ctrl_val = 0;
+
+ for (i = 0; i < fan_data->num_ctrl; i++) {
+ int value;
+
+ value = gpio_get_value(fan_data->ctrl[i]);
+ ctrl_val |= (value << i);
+ }
+ return ctrl_val;
+}
+
+static void __set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
+{
+ int i;
+
+ for (i = 0; i < fan_data->num_ctrl; i++) {
+ int value = !!(ctrl_val & (1 << i));
+
+ gpio_set_value(fan_data->ctrl[i], value);
+ }
+}
+
+static void set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
+{
+ if (fan_data->platform_set_ctrl)
+ fan_data->platform_set_ctrl(fan_data->num_ctrl, fan_data->ctrl,
+ ctrl_val);
+ else
+ __set_fan_ctrl(fan_data, ctrl_val);
+
+ fan_data->ctrl_val = ctrl_val;
+}
+
+static int rpm_to_ctrl(struct gpio_fan_data *fan_data, int rpm)
+{
+ struct gpio_fan_speed *speed = fan_data->speed;
+ int num_speed = fan_data->num_speed;
+ int ctrl_val = speed[num_speed - 1].ctrl_val; /* Maximum speed */
+ int i;
+
+ for (i = 0; i < num_speed; i++) {
+ if (speed[i].rpm >= rpm) {
+ ctrl_val = speed[i].ctrl_val;
+ break;
+ }
+ }
+ return ctrl_val;
+}
+
+static int ctrl_to_rpm(struct gpio_fan_data *fan_data, int ctrl_val, int *rpm)
+{
+ struct gpio_fan_speed *speed = fan_data->speed;
+ int num_speed = fan_data->num_speed;
+ int i;
+
+ for (i = 0; i < num_speed; i++) {
+ if (speed[i].ctrl_val == ctrl_val) {
+ *rpm = speed[i].rpm;
+ return 0;
+ }
+ }
+ return -EINVAL;
+}
+
+static int pwm_to_rpm(struct gpio_fan_data *fan_data, u8 pwm)
+{
+ int rpm_min = fan_data->speed[0].rpm;
+ int rpm_max = fan_data->speed[fan_data->num_speed - 1].rpm;
+
+ return rpm_min + DIV_ROUND_UP((rpm_max - rpm_min), 255) * pwm;
+}
+
+static u8 rpm_to_pwm(struct gpio_fan_data *fan_data, int rpm)
+{
+ int rpm_min = fan_data->speed[0].rpm;
+ int rpm_max = fan_data->speed[fan_data->num_speed - 1].rpm;
+
+ return (rpm - rpm_min) / DIV_ROUND_UP((rpm_max - rpm_min), 255);
+}
+
+static ssize_t show_pwm(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+ int rpm;
+ int ret;
+
+ ret = ctrl_to_rpm(fan_data, fan_data->ctrl_val, &rpm);
+ if (ret)
+ return ret;
+
+ return sprintf(buf, "%d\n", rpm_to_pwm(fan_data, rpm));
+}
+
+static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+ unsigned long pwm;
+ int ctrl_val;
+ int ret = count;
+
+ if (strict_strtoul(buf, 10, &pwm) || pwm > 255)
+ return -EINVAL;
+
+ mutex_lock(&fan_data->lock);
+
+ if (fan_data->pwm_enable != 1) {
+ ret = -EPERM;
+ goto exit_unlock;
+ }
+
+ ctrl_val = rpm_to_ctrl(fan_data, pwm_to_rpm(fan_data, (u8) pwm));
+ if (ctrl_val != fan_data->ctrl_val)
+ set_fan_ctrl(fan_data, ctrl_val);
+
+exit_unlock:
+ mutex_unlock(&fan_data->lock);
+
+ return ret;
+}
+
+static ssize_t show_pwm_enable(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", fan_data->pwm_enable);
+}
+
+static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+ unsigned long val;
+ int ctrl_val;
+
+ if (strict_strtoul(buf, 10, &val) || val > 1)
+ return -EINVAL;
+
+ if (fan_data->pwm_enable == val)
+ return count;
+
+ mutex_lock(&fan_data->lock);
+
+ fan_data->pwm_enable = val;
+
+ /* Disable manual control mode: set fan at full speed. */
+ if (val == 0) {
+ ctrl_val = fan_data->speed[fan_data->num_speed - 1].ctrl_val;
+ if (ctrl_val != fan_data->ctrl_val)
+ set_fan_ctrl(fan_data, ctrl_val);
+ }
+
+ mutex_unlock(&fan_data->lock);
+
+ return count;
+}
+
+static ssize_t show_pwm_mode(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "0\n");
+}
+
+static ssize_t show_rpm(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+ int ret;
+ int rpm;
+
+ ret = ctrl_to_rpm(fan_data, fan_data->ctrl_val, &rpm);
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%d\n", rpm);
+}
+
+static DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm);
+static DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
+ show_pwm_enable, set_pwm_enable);
+static DEVICE_ATTR(pwm1_mode, S_IRUGO, show_pwm_mode, NULL);
+static DEVICE_ATTR(fan1_input, S_IRUGO, show_rpm, NULL);
+
+static struct attribute *gpio_fan_ctrl_attributes[] = {
+ &dev_attr_pwm1.attr,
+ &dev_attr_pwm1_enable.attr,
+ &dev_attr_pwm1_mode.attr,
+ &dev_attr_fan1_input.attr,
+ NULL
+};
+
+static const struct attribute_group gpio_fan_ctrl_group = {
+ .attrs = gpio_fan_ctrl_attributes,
+};
+
+static int __devinit fan_ctrl_init(struct gpio_fan_data *fan_data,
+ struct gpio_fan_platform_data *pdata)
+{
+ struct platform_device *pdev = fan_data->pdev;
+ int num_ctrl = pdata->num_ctrl;
+ unsigned *ctrl = pdata->ctrl;
+ int i, err;
+
+ for (i = 0; i < num_ctrl; i++) {
+ err = gpio_request(ctrl[i], "GPIO fan control");
+ if (err)
+ goto err_free_gpio;
+
+ err = gpio_direction_output(ctrl[i], gpio_get_value(ctrl[i]));
+ if (err) {
+ gpio_free(ctrl[i]);
+ goto err_free_gpio;
+ }
+ }
+
+ err = sysfs_create_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
+ if (err)
+ goto err_free_gpio;
+
+ fan_data->num_ctrl = num_ctrl;
+ fan_data->ctrl = ctrl;
+ fan_data->platform_set_ctrl = pdata->set_ctrl;
+ fan_data->num_speed = pdata->num_speed;
+ fan_data->speed = pdata->speed;
+ fan_data->pwm_enable = 1; /* Enable manual fan speed control. */
+ if (pdata->get_ctrl)
+ fan_data->ctrl_val = pdata->get_ctrl(num_ctrl, ctrl);
+ else
+ fan_data->ctrl_val = __get_fan_ctrl(fan_data);
+
+ return 0;
+
+err_free_gpio:
+ for (i = i - 1; i >= 0; i--)
+ gpio_free(ctrl[i]);
+
+ return err;
+}
+
+static void __devexit fan_ctrl_free(struct gpio_fan_data *fan_data)
+{
+ struct platform_device *pdev = fan_data->pdev;
+ int i;
+
+ sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
+ for (i = 0; i < fan_data->num_ctrl; i++)
+ gpio_free(fan_data->ctrl[i]);
+}
+
+/*
+ * Platform driver.
+ */
+
+static ssize_t show_name(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "gpio-fan\n");
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static int __devinit gpio_fan_probe(struct platform_device *pdev)
+{
+ int err = 0;
+ struct gpio_fan_data *fan_data;
+ struct gpio_fan_platform_data *pdata = pdev->dev.platform_data;
+
+ if (!pdata)
+ return -EINVAL;
+
+ fan_data = kzalloc(sizeof(struct gpio_fan_data), GFP_KERNEL);
+ if (!fan_data)
+ return -ENOMEM;
+
+ fan_data->pdev = pdev;
+ platform_set_drvdata(pdev, fan_data);
+ mutex_init(&fan_data->lock);
+
+ /* Configure alarm GPIO if available. */
+ if (pdata->alarm) {
+ err = fan_alarm_init(fan_data, pdata->alarm);
+ if (err)
+ goto err_free_data;
+ }
+
+ /* Configure control GPIOs if available. */
+ if (pdata->ctrl && pdata->num_ctrl) {
+ if (!pdata->num_speed || !pdata->speed) {
+ err = -EINVAL;
+ goto err_free_alarm;
+ }
+ err = fan_ctrl_init(fan_data, pdata);
+ if (err)
+ goto err_free_alarm;
+ }
+
+ err = device_create_file(&pdev->dev, &dev_attr_name);
+ if (err)
+ goto err_free_ctrl;
+
+ /* Make this driver part of hwmon class. */
+ fan_data->hwmon_dev = hwmon_device_register(&pdev->dev);
+ if (IS_ERR(fan_data->hwmon_dev)) {
+ err = PTR_ERR(fan_data->hwmon_dev);
+ goto err_remove_name;
+ }
+
+ dev_info(&pdev->dev, "GPIO fan initialized\n");
+
+ return 0;
+
+err_remove_name:
+ device_remove_file(&pdev->dev, &dev_attr_name);
+err_free_ctrl:
+ fan_ctrl_free(fan_data);
+err_free_alarm:
+ fan_alarm_free(fan_data);
+err_free_data:
+ platform_set_drvdata(pdev, NULL);
+ kfree(fan_data);
+
+ return err;
+}
+
+static int __devexit gpio_fan_remove(struct platform_device *pdev)
+{
+ struct gpio_fan_data *fan_data = platform_get_drvdata(pdev);
+
+ hwmon_device_unregister(fan_data->hwmon_dev);
+ device_remove_file(&pdev->dev, &dev_attr_name);
+ if (fan_data->alarm)
+ fan_alarm_free(fan_data);
+ if (fan_data->ctrl)
+ fan_ctrl_free(fan_data);
+ kfree(fan_data);
+
+ return 0;
+}
+
+static struct platform_driver gpio_fan_driver = {
+ .probe = gpio_fan_probe,
+ .remove = __devexit_p(gpio_fan_remove),
+ .driver = {
+ .name = "gpio-fan",
+ },
+};
+
+static int __init gpio_fan_init(void)
+{
+ return platform_driver_register(&gpio_fan_driver);
+}
+
+static void __exit gpio_fan_exit(void)
+{
+ platform_driver_unregister(&gpio_fan_driver);
+ return;
+}
+
+module_init(gpio_fan_init);
+module_exit(gpio_fan_exit);
+
+MODULE_AUTHOR("Simon Guinot <sguinot@lacie.com>");
+MODULE_DESCRIPTION("GPIO FAN driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-fan");
diff --git a/include/linux/gpio-fan.h b/include/linux/gpio-fan.h
new file mode 100644
index 0000000..58f0930
--- /dev/null
+++ b/include/linux/gpio-fan.h
@@ -0,0 +1,43 @@
+/*
+ * include/linux/gpio-fan.h
+ *
+ * Platform data structure for GPIO fan driver
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __LINUX_GPIO_FAN_H
+#define __LINUX_GPIO_FAN_H
+
+struct gpio_fan_alarm {
+ unsigned gpio;
+ unsigned active_low;
+};
+
+struct gpio_fan_speed {
+ int rpm;
+ int ctrl_val;
+};
+
+struct gpio_fan_platform_data {
+ int num_ctrl;
+ unsigned *ctrl; /* fan control GPIOs. */
+ struct gpio_fan_alarm *alarm; /* fan alarm GPIO. */
+ /*
+ * Speed conversion array: rpm from/to GPIO bit field.
+ * This array _must_ be sorted in ascending rpm order.
+ */
+ int num_speed;
+ struct gpio_fan_speed *speed;
+ /*
+ * This functions can be supplied if some specific operations are
+ * required to get/set a fan control (handle a latch enable GPIO,
+ * prevent from writing some transitional control value, etc...)
+ */
+ int (*get_ctrl)(int num_ctrl, unsigned *ctrl);
+ void (*set_ctrl)(int num_ctrl, unsigned *ctrl, int ctrl_val);
+};
+
+#endif /* __LINUX_GPIO_FAN_H */
--
1.6.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-17 15:50 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Simon Guinot
@ 2010-10-17 15:50 ` Simon Guinot
2010-10-22 1:53 ` Guenter Roeck
2010-10-18 16:08 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Guenter Roeck
2010-10-19 22:03 ` Guenter Roeck
2 siblings, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-17 15:50 UTC (permalink / raw)
To: linux-arm-kernel
From: Simon Guinot <sguinot@lacie.com>
Signed-off-by: Simon Guinot <sguinot@lacie.com>
---
arch/arm/mach-kirkwood/netspace_v2-setup.c | 43 ++++++++++++++++++++++++++++
1 files changed, 43 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-kirkwood/netspace_v2-setup.c b/arch/arm/mach-kirkwood/netspace_v2-setup.c
index fed264d..8e14147 100644
--- a/arch/arm/mach-kirkwood/netspace_v2-setup.c
+++ b/arch/arm/mach-kirkwood/netspace_v2-setup.c
@@ -30,6 +30,7 @@
#include <linux/gpio.h>
#include <linux/gpio_keys.h>
#include <linux/leds.h>
+#include <linux/gpio-fan.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
#include <mach/kirkwood.h>
@@ -137,6 +138,46 @@ static struct platform_device netspace_v2_leds = {
};
/*****************************************************************************
+ * GPIO fan
+ ****************************************************************************/
+
+/* Designed for fan 40x40x16: ADDA AD0412LB-D50 6000rpm at 12v */
+static struct gpio_fan_speed netspace_max_v2_fan_speed[] = {
+ { 0, 0 },
+ { 1500, 15 },
+ { 1700, 14 },
+ { 1800, 13 },
+ { 2100, 12 },
+ { 3100, 11 },
+ { 3300, 10 },
+ { 4300, 9 },
+ { 5500, 8 },
+};
+
+static unsigned netspace_max_v2_fan_ctrl[] = { 22, 7, 33, 23 };
+
+static struct gpio_fan_alarm netspace_max_v2_fan_alarm = {
+ .gpio = 25,
+ .active_low = 1,
+};
+
+static struct gpio_fan_platform_data netspace_max_v2_fan_data = {
+ .num_ctrl = ARRAY_SIZE(netspace_max_v2_fan_ctrl),
+ .ctrl = netspace_max_v2_fan_ctrl,
+ .alarm = &netspace_max_v2_fan_alarm,
+ .num_speed = ARRAY_SIZE(netspace_max_v2_fan_speed),
+ .speed = netspace_max_v2_fan_speed,
+};
+
+static struct platform_device netspace_max_v2_gpio_fan = {
+ .name = "gpio-fan",
+ .id = -1,
+ .dev = {
+ .platform_data = &netspace_max_v2_fan_data,
+ },
+};
+
+/*****************************************************************************
* General Setup
****************************************************************************/
@@ -205,6 +246,8 @@ static void __init netspace_v2_init(void)
platform_device_register(&netspace_v2_leds);
platform_device_register(&netspace_v2_gpio_leds);
platform_device_register(&netspace_v2_gpio_buttons);
+ if (machine_is_netspace_max_v2())
+ platform_device_register(&netspace_max_v2_gpio_fan);
if (gpio_request(NETSPACE_V2_GPIO_POWER_OFF, "power-off") == 0 &&
gpio_direction_output(NETSPACE_V2_GPIO_POWER_OFF, 0) == 0)
--
1.6.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-17 15:50 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Simon Guinot
2010-10-17 15:50 ` [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2 Simon Guinot
@ 2010-10-18 16:08 ` Guenter Roeck
2010-10-18 18:00 ` Chris Moore
` (2 more replies)
2010-10-19 22:03 ` Guenter Roeck
2 siblings, 3 replies; 33+ messages in thread
From: Guenter Roeck @ 2010-10-18 16:08 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Oct 17, 2010 at 11:50:11AM -0400, Simon Guinot wrote:
> From: Simon Guinot <sguinot@lacie.com>
>
> This patch adds hwmon support for the GPIO connected fan.
>
> Platform specific informations as GPIO pinout and speed conversion array
> (rpm from/to GPIO value) are passed to the driver via platform_data.
>
> Signed-off-by: Simon Guinot <sguinot@lacie.com>
Hi Simon,
good start. Bunch of comments inline. Our mailer was nice enough to replace tabs
with blanks, thanks to our friends at MicroSomething, so I could not run the patch
through checkpatch.pl. Hope you did that.
> ---
> drivers/hwmon/Kconfig | 9 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/gpio-fan.c | 508 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/gpio-fan.h | 43 ++++
> 4 files changed, 561 insertions(+), 0 deletions(-)
> create mode 100644 drivers/hwmon/gpio-fan.c
> create mode 100644 include/linux/gpio-fan.h
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 4d4d09b..1dc57c1 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -399,6 +399,15 @@ config SENSORS_GL520SM
> This driver can also be built as a module. If so, the module
> will be called gl520sm.
>
> +config SENSORS_GPIO_FAN
> + tristate "GPIO fan"
> + depends on GENERIC_GPIO
> + help
> + If you say yes here you get support for the GPIO connected fan.
> +
> + This driver can also be built as a module. If so, the module
> + will be called gpio-fan.
> +
Can you move this up a bit, ahead of SENSORS_FSCHMD ? Trying to stay in sequence.
> config SENSORS_CORETEMP
> tristate "Intel Core/Core2/Atom temperature sensor"
> depends on X86 && PCI && EXPERIMENTAL
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e3c2484..a793e28 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_FSCHMD) += fschmd.o
> obj-$(CONFIG_SENSORS_G760A) += g760a.o
> obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> +obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
> obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
> obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
> obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> new file mode 100644
> index 0000000..6cc8205
> --- /dev/null
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -0,0 +1,508 @@
> +/*
> + * gpio-fan.c - Hwmon driver for GPIO connected fan.
> + *
> + * Copyright (C) 2010 LaCie
> + *
> + * Author: Simon Guinot <sguinot@lacie.com>
> + *
> + * 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/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/hwmon.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio-fan.h>
> +
> +struct gpio_fan_data {
> + struct platform_device *pdev;
> + struct device *hwmon_dev;
> + struct mutex lock; /* lock GPIOs operations. */
> + int num_ctrl;
> + unsigned *ctrl;
> + int ctrl_val;
> + int num_speed;
> + struct gpio_fan_speed *speed;
> + int pwm_enable;
> + struct gpio_fan_alarm *alarm;
> + struct work_struct alarm_work;
> + void (*platform_set_ctrl)(int num_ctrl, unsigned *ctrl, int ctrl_val);
> +};
> +
> +/*
> + * Alarm GPIO.
> + */
> +
> +static void fan_alarm_notify(struct work_struct *ws)
> +{
> + struct gpio_fan_data *fan_data =
> + container_of(ws, struct gpio_fan_data, alarm_work);
> +
> + sysfs_notify(&fan_data->pdev->dev.kobj, NULL, "fan1_alarm");
> + kobject_uevent(&fan_data->pdev->dev.kobj, KOBJ_CHANGE);
> +}
> +
> +static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id)
> +{
> + struct gpio_fan_data *fan_data = dev_id;
> +
> + schedule_work(&fan_data->alarm_work);
> +
> + return IRQ_HANDLED;
> +}
You are assuming that the irq does not have to be shared, and
will always be handled. This may interfer with other drivers
using the same IRQ for other GPIO pins.
> +
> +static ssize_t show_fan_alarm(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> + struct gpio_fan_alarm *alarm = fan_data->alarm;
> + int value = gpio_get_value(alarm->gpio);
> +
> + if (alarm->active_low)
> + value = !value;
> +
> + return sprintf(buf, "%d\n", value);
> +}
> +
> +static DEVICE_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL);
> +
> +static int __devinit
> +fan_alarm_init(struct gpio_fan_data *fan_data, struct gpio_fan_alarm *alarm)
> +{
> + int err;
> + int alarm_irq;
> + struct platform_device *pdev = fan_data->pdev;
> +
> + fan_data->alarm = alarm;
> +
> + err = gpio_request(alarm->gpio, "GPIO fan alarm");
> + if (err)
> + return err;
> +
> + err = gpio_direction_input(alarm->gpio);
> + if (err)
> + goto err_free_gpio;
> +
> + err = device_create_file(&pdev->dev, &dev_attr_fan1_alarm);
> + if (err)
> + goto err_free_gpio;
> +
> + INIT_WORK(&fan_data->alarm_work, fan_alarm_notify);
> + alarm_irq = gpio_to_irq(alarm->gpio);
gpio_to_irq() can return an error. Please check (I see that other drivers
often don't check the return value, but that may be platform specific knowledge).
Also, what happens if the gpio pin does not support interrupts in the first place ?
Still need to be able to support alarms that case.
> + set_irq_type(alarm_irq, IRQ_TYPE_EDGE_BOTH);
> + err = request_irq(alarm_irq, fan_alarm_irq_handler, 0,
> + "GPIO fan alarm", fan_data);
Why not IRQF_SHARED ?
> + if (err)
> + goto err_free_sysfs;
> +
> + return 0;
> +
> +err_free_sysfs:
> + device_remove_file(&pdev->dev, &dev_attr_fan1_alarm);
> +err_free_gpio:
> + gpio_free(alarm->gpio);
> +
> + return err;
> +}
> +
> +static void __devexit fan_alarm_free(struct gpio_fan_data *fan_data)
> +{
> + struct platform_device *pdev = fan_data->pdev;
> +
> + free_irq(gpio_to_irq(fan_data->alarm->gpio), fan_data);
Please check for gpio_to_irq() error return.
> + device_remove_file(&pdev->dev, &dev_attr_fan1_alarm);
> + gpio_free(fan_data->alarm->gpio);
> +}
> +
> +/*
> + * Control GPIOs.
> + */
> +
> +static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
> +{
> + int i;
> + int ctrl_val = 0;
> +
> + for (i = 0; i < fan_data->num_ctrl; i++) {
> + int value;
> +
> + value = gpio_get_value(fan_data->ctrl[i]);
> + ctrl_val |= (value << i);
> + }
> + return ctrl_val;
> +}
> +
> +static void __set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
> +{
> + int i;
> +
> + for (i = 0; i < fan_data->num_ctrl; i++) {
> + int value = !!(ctrl_val & (1 << i));
> +
value = (ctrl_val & (1 >> i));
would be much simpler and easier to understand.
Actually, you would not need 'value' in the first place.
> + gpio_set_value(fan_data->ctrl[i], value);
> + }
> +}
> +
> +static void set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
> +{
> + if (fan_data->platform_set_ctrl)
> + fan_data->platform_set_ctrl(fan_data->num_ctrl, fan_data->ctrl,
> + ctrl_val);
> + else
> + __set_fan_ctrl(fan_data, ctrl_val);
> +
> + fan_data->ctrl_val = ctrl_val;
> +}
> +
> +static int rpm_to_ctrl(struct gpio_fan_data *fan_data, int rpm)
> +{
> + struct gpio_fan_speed *speed = fan_data->speed;
> + int num_speed = fan_data->num_speed;
> + int ctrl_val = speed[num_speed - 1].ctrl_val; /* Maximum speed */
> + int i;
> +
> + for (i = 0; i < num_speed; i++) {
{ } not needed here.
> + if (speed[i].rpm >= rpm) {
> + ctrl_val = speed[i].ctrl_val;
> + break;
> + }
> + }
> + return ctrl_val;
> +}
> +
> +static int ctrl_to_rpm(struct gpio_fan_data *fan_data, int ctrl_val, int *rpm)
> +{
Since rpm is presumably always positive, you don't need a pointer to rpm.
Return value can be rpm or error.
> + struct gpio_fan_speed *speed = fan_data->speed;
> + int num_speed = fan_data->num_speed;
> + int i;
> +
> + for (i = 0; i < num_speed; i++) {
{ } not needed here.
> + if (speed[i].ctrl_val == ctrl_val) {
> + *rpm = speed[i].rpm;
> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
> +
> +static int pwm_to_rpm(struct gpio_fan_data *fan_data, u8 pwm)
> +{
> + int rpm_min = fan_data->speed[0].rpm;
> + int rpm_max = fan_data->speed[fan_data->num_speed - 1].rpm;
> +
> + return rpm_min + DIV_ROUND_UP((rpm_max - rpm_min), 255) * pwm;
> +}
> +
> +static u8 rpm_to_pwm(struct gpio_fan_data *fan_data, int rpm)
> +{
> + int rpm_min = fan_data->speed[0].rpm;
> + int rpm_max = fan_data->speed[fan_data->num_speed - 1].rpm;
> +
> + return (rpm - rpm_min) / DIV_ROUND_UP((rpm_max - rpm_min), 255);
> +}
> +
> +static ssize_t show_pwm(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> + int rpm;
> + int ret;
> +
> + ret = ctrl_to_rpm(fan_data, fan_data->ctrl_val, &rpm);
> + if (ret)
> + return ret;
> +
> + return sprintf(buf, "%d\n", rpm_to_pwm(fan_data, rpm));
> +}
> +
I don't really understand the value of supporting pwm attributes,
since you have to convert those to rpm anyway. Why not just stick
with fan1_input and fan1_target ? This would simplify the code a lot.
fan1_min and fan1_max should be supported as well, especially
since the information is provided anyway.
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> + unsigned long pwm;
> + int ctrl_val;
> + int ret = count;
> +
> + if (strict_strtoul(buf, 10, &pwm) || pwm > 255)
> + return -EINVAL;
> +
> + mutex_lock(&fan_data->lock);
> +
> + if (fan_data->pwm_enable != 1) {
> + ret = -EPERM;
> + goto exit_unlock;
> + }
> +
> + ctrl_val = rpm_to_ctrl(fan_data, pwm_to_rpm(fan_data, (u8) pwm));
> + if (ctrl_val != fan_data->ctrl_val)
> + set_fan_ctrl(fan_data, ctrl_val);
> +
> +exit_unlock:
> + mutex_unlock(&fan_data->lock);
> +
> + return ret;
> +}
> +
> +static ssize_t show_pwm_enable(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", fan_data->pwm_enable);
> +}
> +
> +static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> + unsigned long val;
> + int ctrl_val;
> +
> + if (strict_strtoul(buf, 10, &val) || val > 1)
> + return -EINVAL;
> +
> + if (fan_data->pwm_enable == val)
> + return count;
> +
> + mutex_lock(&fan_data->lock);
> +
> + fan_data->pwm_enable = val;
> +
> + /* Disable manual control mode: set fan at full speed. */
> + if (val == 0) {
> + ctrl_val = fan_data->speed[fan_data->num_speed - 1].ctrl_val;
> + if (ctrl_val != fan_data->ctrl_val)
> + set_fan_ctrl(fan_data, ctrl_val);
> + }
> +
> + mutex_unlock(&fan_data->lock);
> +
> + return count;
> +}
> +
> +static ssize_t show_pwm_mode(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t show_rpm(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> + int ret;
> + int rpm;
> +
> + ret = ctrl_to_rpm(fan_data, fan_data->ctrl_val, &rpm);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", rpm);
> +}
> +
> +static DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm);
> +static DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
> + show_pwm_enable, set_pwm_enable);
> +static DEVICE_ATTR(pwm1_mode, S_IRUGO, show_pwm_mode, NULL);
> +static DEVICE_ATTR(fan1_input, S_IRUGO, show_rpm, NULL);
> +
> +static struct attribute *gpio_fan_ctrl_attributes[] = {
> + &dev_attr_pwm1.attr,
> + &dev_attr_pwm1_enable.attr,
> + &dev_attr_pwm1_mode.attr,
> + &dev_attr_fan1_input.attr,
> + NULL
> +};
> +
> +static const struct attribute_group gpio_fan_ctrl_group = {
> + .attrs = gpio_fan_ctrl_attributes,
> +};
> +
> +static int __devinit fan_ctrl_init(struct gpio_fan_data *fan_data,
> + struct gpio_fan_platform_data *pdata)
> +{
> + struct platform_device *pdev = fan_data->pdev;
> + int num_ctrl = pdata->num_ctrl;
> + unsigned *ctrl = pdata->ctrl;
> + int i, err;
> +
> + for (i = 0; i < num_ctrl; i++) {
> + err = gpio_request(ctrl[i], "GPIO fan control");
> + if (err)
> + goto err_free_gpio;
> +
> + err = gpio_direction_output(ctrl[i], gpio_get_value(ctrl[i]));
> + if (err) {
> + gpio_free(ctrl[i]);
> + goto err_free_gpio;
> + }
> + }
> +
> + err = sysfs_create_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> + if (err)
> + goto err_free_gpio;
> +
> + fan_data->num_ctrl = num_ctrl;
> + fan_data->ctrl = ctrl;
> + fan_data->platform_set_ctrl = pdata->set_ctrl;
> + fan_data->num_speed = pdata->num_speed;
> + fan_data->speed = pdata->speed;
> + fan_data->pwm_enable = 1; /* Enable manual fan speed control. */
> + if (pdata->get_ctrl)
> + fan_data->ctrl_val = pdata->get_ctrl(num_ctrl, ctrl);
> + else
> + fan_data->ctrl_val = __get_fan_ctrl(fan_data);
> +
> + return 0;
> +
> +err_free_gpio:
> + for (i = i - 1; i >= 0; i--)
> + gpio_free(ctrl[i]);
> +
This misses the most recently allocated gpio pin if gpio_direction_output() failed.
> + return err;
> +}
> +
> +static void __devexit fan_ctrl_free(struct gpio_fan_data *fan_data)
> +{
> + struct platform_device *pdev = fan_data->pdev;
> + int i;
> +
> + sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> + for (i = 0; i < fan_data->num_ctrl; i++)
> + gpio_free(fan_data->ctrl[i]);
> +}
> +
> +/*
> + * Platform driver.
> + */
> +
> +static ssize_t show_name(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "gpio-fan\n");
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +
> +static int __devinit gpio_fan_probe(struct platform_device *pdev)
> +{
> + int err = 0;
> + struct gpio_fan_data *fan_data;
> + struct gpio_fan_platform_data *pdata = pdev->dev.platform_data;
> +
> + if (!pdata)
> + return -EINVAL;
> +
> + fan_data = kzalloc(sizeof(struct gpio_fan_data), GFP_KERNEL);
> + if (!fan_data)
> + return -ENOMEM;
> +
> + fan_data->pdev = pdev;
> + platform_set_drvdata(pdev, fan_data);
> + mutex_init(&fan_data->lock);
> +
> + /* Configure alarm GPIO if available. */
> + if (pdata->alarm) {
> + err = fan_alarm_init(fan_data, pdata->alarm);
> + if (err)
> + goto err_free_data;
> + }
> +
> + /* Configure control GPIOs if available. */
> + if (pdata->ctrl && pdata->num_ctrl) {
> + if (!pdata->num_speed || !pdata->speed) {
> + err = -EINVAL;
> + goto err_free_alarm;
> + }
> + err = fan_ctrl_init(fan_data, pdata);
> + if (err)
> + goto err_free_alarm;
> + }
> +
> + err = device_create_file(&pdev->dev, &dev_attr_name);
> + if (err)
> + goto err_free_ctrl;
> +
> + /* Make this driver part of hwmon class. */
> + fan_data->hwmon_dev = hwmon_device_register(&pdev->dev);
> + if (IS_ERR(fan_data->hwmon_dev)) {
> + err = PTR_ERR(fan_data->hwmon_dev);
> + goto err_remove_name;
> + }
> +
> + dev_info(&pdev->dev, "GPIO fan initialized\n");
> +
Might be a good idea to add a notion of which fan was initialized.
After all, there could be more than one.
> + return 0;
> +
> +err_remove_name:
> + device_remove_file(&pdev->dev, &dev_attr_name);
> +err_free_ctrl:
> + fan_ctrl_free(fan_data);
Might want to check for fan_data->ctrl to be consistent with the code below.
> +err_free_alarm:
> + fan_alarm_free(fan_data);
This will crash if fan_data->alarm is NULL.
> +err_free_data:
> + platform_set_drvdata(pdev, NULL);
> + kfree(fan_data);
> +
> + return err;
> +}
> +
> +static int __devexit gpio_fan_remove(struct platform_device *pdev)
> +{
> + struct gpio_fan_data *fan_data = platform_get_drvdata(pdev);
> +
> + hwmon_device_unregister(fan_data->hwmon_dev);
> + device_remove_file(&pdev->dev, &dev_attr_name);
> + if (fan_data->alarm)
> + fan_alarm_free(fan_data);
> + if (fan_data->ctrl)
> + fan_ctrl_free(fan_data);
> + kfree(fan_data);
> +
> + return 0;
> +}
> +
> +static struct platform_driver gpio_fan_driver = {
> + .probe = gpio_fan_probe,
> + .remove = __devexit_p(gpio_fan_remove),
> + .driver = {
> + .name = "gpio-fan",
> + },
> +};
> +
> +static int __init gpio_fan_init(void)
> +{
> + return platform_driver_register(&gpio_fan_driver);
> +}
> +
> +static void __exit gpio_fan_exit(void)
> +{
> + platform_driver_unregister(&gpio_fan_driver);
> + return;
> +}
> +
> +module_init(gpio_fan_init);
> +module_exit(gpio_fan_exit);
> +
> +MODULE_AUTHOR("Simon Guinot <sguinot@lacie.com>");
> +MODULE_DESCRIPTION("GPIO FAN driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:gpio-fan");
> diff --git a/include/linux/gpio-fan.h b/include/linux/gpio-fan.h
> new file mode 100644
> index 0000000..58f0930
> --- /dev/null
> +++ b/include/linux/gpio-fan.h
> @@ -0,0 +1,43 @@
> +/*
> + * include/linux/gpio-fan.h
> + *
> + * Platform data structure for GPIO fan driver
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __LINUX_GPIO_FAN_H
> +#define __LINUX_GPIO_FAN_H
> +
> +struct gpio_fan_alarm {
> + unsigned gpio;
> + unsigned active_low;
> +};
> +
> +struct gpio_fan_speed {
> + int rpm;
> + int ctrl_val;
> +};
> +
> +struct gpio_fan_platform_data {
> + int num_ctrl;
> + unsigned *ctrl; /* fan control GPIOs. */
> + struct gpio_fan_alarm *alarm; /* fan alarm GPIO. */
> + /*
> + * Speed conversion array: rpm from/to GPIO bit field.
> + * This array _must_ be sorted in ascending rpm order.
> + */
> + int num_speed;
> + struct gpio_fan_speed *speed;
> + /*
> + * This functions can be supplied if some specific operations are
> + * required to get/set a fan control (handle a latch enable GPIO,
> + * prevent from writing some transitional control value, etc...)
> + */
> + int (*get_ctrl)(int num_ctrl, unsigned *ctrl);
> + void (*set_ctrl)(int num_ctrl, unsigned *ctrl, int ctrl_val);
Not sure if this is a good/useful API. It expects that the called function
identifies the fan from the ctrl[] array. Not sure how clumsy the resulting code
might be. It may be better to leave the API out for now until someone actually
writes a driver (hint, hint, dns323) using it.
> +};
> +
> +#endif /* __LINUX_GPIO_FAN_H */
> --
> 1.6.3.1
>
--
Guenter Roeck
Distinguished Engineer
PDU IP Systems
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-18 16:08 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Guenter Roeck
@ 2010-10-18 18:00 ` Chris Moore
2010-10-18 18:35 ` Guenter Roeck
2010-10-18 20:36 ` Simon Guinot
2010-10-21 20:07 ` Simon Guinot
2 siblings, 1 reply; 33+ messages in thread
From: Chris Moore @ 2010-10-18 18:00 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Le 18/10/2010 18:08, Guenter Roeck a ?crit :
> On Sun, Oct 17, 2010 at 11:50:11AM -0400, Simon Guinot wrote:
>
>> +static void __set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
>> +{
>> + int i;
>> +
>> + for (i = 0; i< fan_data->num_ctrl; i++) {
>> + int value = !!(ctrl_val& (1<< i));
>> +
> value = (ctrl_val& (1>> i));
> would be much simpler and easier to understand.
IMHO you mean:
value = (ctrl_val>> i)& 1;
Cheers,
Chris
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-18 18:00 ` Chris Moore
@ 2010-10-18 18:35 ` Guenter Roeck
0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2010-10-18 18:35 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 18, 2010 at 02:00:27PM -0400, Chris Moore wrote:
>
> Hi,
>
> Le 18/10/2010 18:08, Guenter Roeck a ?crit :
> > On Sun, Oct 17, 2010 at 11:50:11AM -0400, Simon Guinot wrote:
> >
> >> +static void __set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i< fan_data->num_ctrl; i++) {
> >> + int value = !!(ctrl_val& (1<< i));
> >> +
> > value = (ctrl_val& (1>> i));
> > would be much simpler and easier to understand.
>
> IMHO you mean:
>
> value = (ctrl_val>> i)& 1;
>
yes...
thanks for the correction.
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-18 16:08 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Guenter Roeck
2010-10-18 18:00 ` Chris Moore
@ 2010-10-18 20:36 ` Simon Guinot
2010-10-18 20:50 ` Guenter Roeck
2010-10-19 6:52 ` Guenter Roeck
2010-10-21 20:07 ` Simon Guinot
2 siblings, 2 replies; 33+ messages in thread
From: Simon Guinot @ 2010-10-18 20:36 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
Thanks for your review.
On Mon, Oct 18, 2010 at 09:08:30AM -0700, Guenter Roeck wrote:
> On Sun, Oct 17, 2010 at 11:50:11AM -0400, Simon Guinot wrote:
> > From: Simon Guinot <sguinot@lacie.com>
> >
> > This patch adds hwmon support for the GPIO connected fan.
> >
> > Platform specific informations as GPIO pinout and speed conversion array
> > (rpm from/to GPIO value) are passed to the driver via platform_data.
> >
> > Signed-off-by: Simon Guinot <sguinot@lacie.com>
>
> Hi Simon,
>
> good start. Bunch of comments inline. Our mailer was nice enough to replace tabs
> with blanks, thanks to our friends at MicroSomething, so I could not run the patch
> through checkpatch.pl. Hope you did that.
Yes, I did.
>
> > ---
> > drivers/hwmon/Kconfig | 9 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/gpio-fan.c | 508 ++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/gpio-fan.h | 43 ++++
> > 4 files changed, 561 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/hwmon/gpio-fan.c
> > create mode 100644 include/linux/gpio-fan.h
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 4d4d09b..1dc57c1 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -399,6 +399,15 @@ config SENSORS_GL520SM
> > This driver can also be built as a module. If so, the module
> > will be called gl520sm.
> >
> > +config SENSORS_GPIO_FAN
> > + tristate "GPIO fan"
> > + depends on GENERIC_GPIO
> > + help
> > + If you say yes here you get support for the GPIO connected fan.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called gpio-fan.
> > +
>
> Can you move this up a bit, ahead of SENSORS_FSCHMD ? Trying to stay in sequence.
Ok.
>
> > config SENSORS_CORETEMP
> > tristate "Intel Core/Core2/Atom temperature sensor"
> > depends on X86 && PCI && EXPERIMENTAL
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e3c2484..a793e28 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_FSCHMD) += fschmd.o
> > obj-$(CONFIG_SENSORS_G760A) += g760a.o
> > obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> > obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> > +obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
> > obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
> > obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
> > obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o
> > diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> > new file mode 100644
> > index 0000000..6cc8205
> > --- /dev/null
> > +++ b/drivers/hwmon/gpio-fan.c
> > @@ -0,0 +1,508 @@
> > +/*
> > + * gpio-fan.c - Hwmon driver for GPIO connected fan.
> > + *
> > + * Copyright (C) 2010 LaCie
> > + *
> > + * Author: Simon Guinot <sguinot@lacie.com>
> > + *
> > + * 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/module.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/err.h>
> > +#include <linux/mutex.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio-fan.h>
> > +
> > +struct gpio_fan_data {
> > + struct platform_device *pdev;
> > + struct device *hwmon_dev;
> > + struct mutex lock; /* lock GPIOs operations. */
> > + int num_ctrl;
> > + unsigned *ctrl;
> > + int ctrl_val;
> > + int num_speed;
> > + struct gpio_fan_speed *speed;
> > + int pwm_enable;
> > + struct gpio_fan_alarm *alarm;
> > + struct work_struct alarm_work;
> > + void (*platform_set_ctrl)(int num_ctrl, unsigned *ctrl, int ctrl_val);
> > +};
> > +
> > +/*
> > + * Alarm GPIO.
> > + */
> > +
> > +static void fan_alarm_notify(struct work_struct *ws)
> > +{
> > + struct gpio_fan_data *fan_data =
> > + container_of(ws, struct gpio_fan_data, alarm_work);
> > +
> > + sysfs_notify(&fan_data->pdev->dev.kobj, NULL, "fan1_alarm");
> > + kobject_uevent(&fan_data->pdev->dev.kobj, KOBJ_CHANGE);
> > +}
> > +
> > +static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id)
> > +{
> > + struct gpio_fan_data *fan_data = dev_id;
> > +
> > + schedule_work(&fan_data->alarm_work);
> > +
> > + return IRQ_HANDLED;
> > +}
> You are assuming that the irq does not have to be shared, and
> will always be handled. This may interfer with other drivers
> using the same IRQ for other GPIO pins.
Yes, you are right.
>
> > +
> > +static ssize_t show_fan_alarm(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> > + struct gpio_fan_alarm *alarm = fan_data->alarm;
> > + int value = gpio_get_value(alarm->gpio);
> > +
> > + if (alarm->active_low)
> > + value = !value;
> > +
> > + return sprintf(buf, "%d\n", value);
> > +}
> > +
> > +static DEVICE_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL);
> > +
> > +static int __devinit
> > +fan_alarm_init(struct gpio_fan_data *fan_data, struct gpio_fan_alarm *alarm)
> > +{
> > + int err;
> > + int alarm_irq;
> > + struct platform_device *pdev = fan_data->pdev;
> > +
> > + fan_data->alarm = alarm;
> > +
> > + err = gpio_request(alarm->gpio, "GPIO fan alarm");
> > + if (err)
> > + return err;
> > +
> > + err = gpio_direction_input(alarm->gpio);
> > + if (err)
> > + goto err_free_gpio;
> > +
> > + err = device_create_file(&pdev->dev, &dev_attr_fan1_alarm);
> > + if (err)
> > + goto err_free_gpio;
> > +
> > + INIT_WORK(&fan_data->alarm_work, fan_alarm_notify);
> > + alarm_irq = gpio_to_irq(alarm->gpio);
>
> gpio_to_irq() can return an error. Please check (I see that other drivers
> often don't check the return value, but that may be platform specific knowledge).
Yes, I have to check the returned value.
>
> Also, what happens if the gpio pin does not support interrupts in the first place ?
> Still need to be able to support alarms that case.
Good idea.
>
> > + set_irq_type(alarm_irq, IRQ_TYPE_EDGE_BOTH);
> > + err = request_irq(alarm_irq, fan_alarm_irq_handler, 0,
> > + "GPIO fan alarm", fan_data);
>
> Why not IRQF_SHARED ?
Simply because I have missed that.
>
> > + if (err)
> > + goto err_free_sysfs;
> > +
> > + return 0;
> > +
> > +err_free_sysfs:
> > + device_remove_file(&pdev->dev, &dev_attr_fan1_alarm);
> > +err_free_gpio:
> > + gpio_free(alarm->gpio);
> > +
> > + return err;
> > +}
> > +
> > +static void __devexit fan_alarm_free(struct gpio_fan_data *fan_data)
> > +{
> > + struct platform_device *pdev = fan_data->pdev;
> > +
> > + free_irq(gpio_to_irq(fan_data->alarm->gpio), fan_data);
>
> Please check for gpio_to_irq() error return.
Yes.
>
> > + device_remove_file(&pdev->dev, &dev_attr_fan1_alarm);
> > + gpio_free(fan_data->alarm->gpio);
> > +}
> > +
> > +/*
> > + * Control GPIOs.
> > + */
> > +
> > +static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
> > +{
> > + int i;
> > + int ctrl_val = 0;
> > +
> > + for (i = 0; i < fan_data->num_ctrl; i++) {
> > + int value;
> > +
> > + value = gpio_get_value(fan_data->ctrl[i]);
> > + ctrl_val |= (value << i);
> > + }
> > + return ctrl_val;
> > +}
> > +
> > +static void __set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < fan_data->num_ctrl; i++) {
> > + int value = !!(ctrl_val & (1 << i));
> > +
> value = (ctrl_val & (1 >> i));
> would be much simpler and easier to understand.
> Actually, you would not need 'value' in the first place.
I will simplify this function.
>
> > + gpio_set_value(fan_data->ctrl[i], value);
> > + }
> > +}
> > +
> > +static void set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
> > +{
> > + if (fan_data->platform_set_ctrl)
> > + fan_data->platform_set_ctrl(fan_data->num_ctrl, fan_data->ctrl,
> > + ctrl_val);
> > + else
> > + __set_fan_ctrl(fan_data, ctrl_val);
> > +
> > + fan_data->ctrl_val = ctrl_val;
> > +}
> > +
> > +static int rpm_to_ctrl(struct gpio_fan_data *fan_data, int rpm)
> > +{
> > + struct gpio_fan_speed *speed = fan_data->speed;
> > + int num_speed = fan_data->num_speed;
> > + int ctrl_val = speed[num_speed - 1].ctrl_val; /* Maximum speed */
> > + int i;
> > +
> > + for (i = 0; i < num_speed; i++) {
>
> { } not needed here.
Ok.
>
> > + if (speed[i].rpm >= rpm) {
> > + ctrl_val = speed[i].ctrl_val;
> > + break;
> > + }
> > + }
> > + return ctrl_val;
> > +}
> > +
> > +static int ctrl_to_rpm(struct gpio_fan_data *fan_data, int ctrl_val, int *rpm)
> > +{
>
> Since rpm is presumably always positive, you don't need a pointer to rpm.
> Return value can be rpm or error.
Yes.
>
> > + struct gpio_fan_speed *speed = fan_data->speed;
> > + int num_speed = fan_data->num_speed;
> > + int i;
> > +
> > + for (i = 0; i < num_speed; i++) {
>
> { } not needed here.
Yes.
>
> > + if (speed[i].ctrl_val == ctrl_val) {
> > + *rpm = speed[i].rpm;
> > + return 0;
> > + }
> > + }
> > + return -EINVAL;
> > +}
> > +
> > +static int pwm_to_rpm(struct gpio_fan_data *fan_data, u8 pwm)
> > +{
> > + int rpm_min = fan_data->speed[0].rpm;
> > + int rpm_max = fan_data->speed[fan_data->num_speed - 1].rpm;
> > +
> > + return rpm_min + DIV_ROUND_UP((rpm_max - rpm_min), 255) * pwm;
> > +}
> > +
> > +static u8 rpm_to_pwm(struct gpio_fan_data *fan_data, int rpm)
> > +{
> > + int rpm_min = fan_data->speed[0].rpm;
> > + int rpm_max = fan_data->speed[fan_data->num_speed - 1].rpm;
> > +
> > + return (rpm - rpm_min) / DIV_ROUND_UP((rpm_max - rpm_min), 255);
> > +}
> > +
> > +static ssize_t show_pwm(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> > + int rpm;
> > + int ret;
> > +
> > + ret = ctrl_to_rpm(fan_data, fan_data->ctrl_val, &rpm);
> > + if (ret)
> > + return ret;
> > +
> > + return sprintf(buf, "%d\n", rpm_to_pwm(fan_data, rpm));
> > +}
> > +
> I don't really understand the value of supporting pwm attributes,
> since you have to convert those to rpm anyway. Why not just stick
> with fan1_input and fan1_target ? This would simplify the code a lot.
I don't know very well the hwmon API. I have simply been fooled by the
sysfs-interface document which claim that fan[1-*]_target only make
sense for a closed-loop fan. Moreover, I was expecting gpio-fan to be
compliant with the fancontrol shell script...
But anyway, you are right. I just don't want the pwm interface.
>
> fan1_min and fan1_max should be supported as well, especially
> since the information is provided anyway.
Yes.
>
> > +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> > + unsigned long pwm;
> > + int ctrl_val;
> > + int ret = count;
> > +
> > + if (strict_strtoul(buf, 10, &pwm) || pwm > 255)
> > + return -EINVAL;
> > +
> > + mutex_lock(&fan_data->lock);
> > +
> > + if (fan_data->pwm_enable != 1) {
> > + ret = -EPERM;
> > + goto exit_unlock;
> > + }
> > +
> > + ctrl_val = rpm_to_ctrl(fan_data, pwm_to_rpm(fan_data, (u8) pwm));
> > + if (ctrl_val != fan_data->ctrl_val)
> > + set_fan_ctrl(fan_data, ctrl_val);
> > +
> > +exit_unlock:
> > + mutex_unlock(&fan_data->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t show_pwm_enable(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", fan_data->pwm_enable);
> > +}
> > +
> > +static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> > + unsigned long val;
> > + int ctrl_val;
> > +
> > + if (strict_strtoul(buf, 10, &val) || val > 1)
> > + return -EINVAL;
> > +
> > + if (fan_data->pwm_enable == val)
> > + return count;
> > +
> > + mutex_lock(&fan_data->lock);
> > +
> > + fan_data->pwm_enable = val;
> > +
> > + /* Disable manual control mode: set fan at full speed. */
> > + if (val == 0) {
> > + ctrl_val = fan_data->speed[fan_data->num_speed - 1].ctrl_val;
> > + if (ctrl_val != fan_data->ctrl_val)
> > + set_fan_ctrl(fan_data, ctrl_val);
> > + }
> > +
> > + mutex_unlock(&fan_data->lock);
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t show_pwm_mode(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + return sprintf(buf, "0\n");
> > +}
> > +
> > +static ssize_t show_rpm(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
> > + int ret;
> > + int rpm;
> > +
> > + ret = ctrl_to_rpm(fan_data, fan_data->ctrl_val, &rpm);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return sprintf(buf, "%d\n", rpm);
> > +}
> > +
> > +static DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm);
> > +static DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
> > + show_pwm_enable, set_pwm_enable);
> > +static DEVICE_ATTR(pwm1_mode, S_IRUGO, show_pwm_mode, NULL);
> > +static DEVICE_ATTR(fan1_input, S_IRUGO, show_rpm, NULL);
> > +
> > +static struct attribute *gpio_fan_ctrl_attributes[] = {
> > + &dev_attr_pwm1.attr,
> > + &dev_attr_pwm1_enable.attr,
> > + &dev_attr_pwm1_mode.attr,
> > + &dev_attr_fan1_input.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group gpio_fan_ctrl_group = {
> > + .attrs = gpio_fan_ctrl_attributes,
> > +};
> > +
> > +static int __devinit fan_ctrl_init(struct gpio_fan_data *fan_data,
> > + struct gpio_fan_platform_data *pdata)
> > +{
> > + struct platform_device *pdev = fan_data->pdev;
> > + int num_ctrl = pdata->num_ctrl;
> > + unsigned *ctrl = pdata->ctrl;
> > + int i, err;
> > +
> > + for (i = 0; i < num_ctrl; i++) {
> > + err = gpio_request(ctrl[i], "GPIO fan control");
> > + if (err)
> > + goto err_free_gpio;
> > +
> > + err = gpio_direction_output(ctrl[i], gpio_get_value(ctrl[i]));
> > + if (err) {
> > + gpio_free(ctrl[i]);
> > + goto err_free_gpio;
> > + }
> > + }
> > +
> > + err = sysfs_create_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> > + if (err)
> > + goto err_free_gpio;
> > +
> > + fan_data->num_ctrl = num_ctrl;
> > + fan_data->ctrl = ctrl;
> > + fan_data->platform_set_ctrl = pdata->set_ctrl;
> > + fan_data->num_speed = pdata->num_speed;
> > + fan_data->speed = pdata->speed;
> > + fan_data->pwm_enable = 1; /* Enable manual fan speed control. */
> > + if (pdata->get_ctrl)
> > + fan_data->ctrl_val = pdata->get_ctrl(num_ctrl, ctrl);
> > + else
> > + fan_data->ctrl_val = __get_fan_ctrl(fan_data);
> > +
> > + return 0;
> > +
> > +err_free_gpio:
> > + for (i = i - 1; i >= 0; i--)
> > + gpio_free(ctrl[i]);
> > +
> This misses the most recently allocated gpio pin if gpio_direction_output() failed.
The gpio is freed above while handling the gpio_direction_output() error.
>
> > + return err;
> > +}
> > +
> > +static void __devexit fan_ctrl_free(struct gpio_fan_data *fan_data)
> > +{
> > + struct platform_device *pdev = fan_data->pdev;
> > + int i;
> > +
> > + sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> > + for (i = 0; i < fan_data->num_ctrl; i++)
> > + gpio_free(fan_data->ctrl[i]);
> > +}
> > +
> > +/*
> > + * Platform driver.
> > + */
> > +
> > +static ssize_t show_name(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + return sprintf(buf, "gpio-fan\n");
> > +}
> > +
> > +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> > +
> > +static int __devinit gpio_fan_probe(struct platform_device *pdev)
> > +{
> > + int err = 0;
> > + struct gpio_fan_data *fan_data;
> > + struct gpio_fan_platform_data *pdata = pdev->dev.platform_data;
> > +
> > + if (!pdata)
> > + return -EINVAL;
> > +
> > + fan_data = kzalloc(sizeof(struct gpio_fan_data), GFP_KERNEL);
> > + if (!fan_data)
> > + return -ENOMEM;
> > +
> > + fan_data->pdev = pdev;
> > + platform_set_drvdata(pdev, fan_data);
> > + mutex_init(&fan_data->lock);
> > +
> > + /* Configure alarm GPIO if available. */
> > + if (pdata->alarm) {
> > + err = fan_alarm_init(fan_data, pdata->alarm);
> > + if (err)
> > + goto err_free_data;
> > + }
> > +
> > + /* Configure control GPIOs if available. */
> > + if (pdata->ctrl && pdata->num_ctrl) {
> > + if (!pdata->num_speed || !pdata->speed) {
> > + err = -EINVAL;
> > + goto err_free_alarm;
> > + }
> > + err = fan_ctrl_init(fan_data, pdata);
> > + if (err)
> > + goto err_free_alarm;
> > + }
> > +
> > + err = device_create_file(&pdev->dev, &dev_attr_name);
> > + if (err)
> > + goto err_free_ctrl;
> > +
> > + /* Make this driver part of hwmon class. */
> > + fan_data->hwmon_dev = hwmon_device_register(&pdev->dev);
> > + if (IS_ERR(fan_data->hwmon_dev)) {
> > + err = PTR_ERR(fan_data->hwmon_dev);
> > + goto err_remove_name;
> > + }
> > +
> > + dev_info(&pdev->dev, "GPIO fan initialized\n");
> > +
> Might be a good idea to add a notion of which fan was initialized.
> After all, there could be more than one.
dev_info() don't do it for me ? anyway, I will check this too.
>
> > + return 0;
> > +
> > +err_remove_name:
> > + device_remove_file(&pdev->dev, &dev_attr_name);
> > +err_free_ctrl:
> > + fan_ctrl_free(fan_data);
>
> Might want to check for fan_data->ctrl to be consistent with the code below.
Of course...
>
> > +err_free_alarm:
> > + fan_alarm_free(fan_data);
>
> This will crash if fan_data->alarm is NULL.
...
>
> > +err_free_data:
> > + platform_set_drvdata(pdev, NULL);
> > + kfree(fan_data);
> > +
> > + return err;
> > +}
> > +
> > +static int __devexit gpio_fan_remove(struct platform_device *pdev)
> > +{
> > + struct gpio_fan_data *fan_data = platform_get_drvdata(pdev);
> > +
> > + hwmon_device_unregister(fan_data->hwmon_dev);
> > + device_remove_file(&pdev->dev, &dev_attr_name);
> > + if (fan_data->alarm)
> > + fan_alarm_free(fan_data);
> > + if (fan_data->ctrl)
> > + fan_ctrl_free(fan_data);
> > + kfree(fan_data);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver gpio_fan_driver = {
> > + .probe = gpio_fan_probe,
> > + .remove = __devexit_p(gpio_fan_remove),
> > + .driver = {
> > + .name = "gpio-fan",
> > + },
> > +};
> > +
> > +static int __init gpio_fan_init(void)
> > +{
> > + return platform_driver_register(&gpio_fan_driver);
> > +}
> > +
> > +static void __exit gpio_fan_exit(void)
> > +{
> > + platform_driver_unregister(&gpio_fan_driver);
> > + return;
> > +}
> > +
> > +module_init(gpio_fan_init);
> > +module_exit(gpio_fan_exit);
> > +
> > +MODULE_AUTHOR("Simon Guinot <sguinot@lacie.com>");
> > +MODULE_DESCRIPTION("GPIO FAN driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:gpio-fan");
> > diff --git a/include/linux/gpio-fan.h b/include/linux/gpio-fan.h
> > new file mode 100644
> > index 0000000..58f0930
> > --- /dev/null
> > +++ b/include/linux/gpio-fan.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * include/linux/gpio-fan.h
> > + *
> > + * Platform data structure for GPIO fan driver
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#ifndef __LINUX_GPIO_FAN_H
> > +#define __LINUX_GPIO_FAN_H
> > +
> > +struct gpio_fan_alarm {
> > + unsigned gpio;
> > + unsigned active_low;
> > +};
> > +
> > +struct gpio_fan_speed {
> > + int rpm;
> > + int ctrl_val;
> > +};
> > +
> > +struct gpio_fan_platform_data {
> > + int num_ctrl;
> > + unsigned *ctrl; /* fan control GPIOs. */
> > + struct gpio_fan_alarm *alarm; /* fan alarm GPIO. */
> > + /*
> > + * Speed conversion array: rpm from/to GPIO bit field.
> > + * This array _must_ be sorted in ascending rpm order.
> > + */
> > + int num_speed;
> > + struct gpio_fan_speed *speed;
> > + /*
> > + * This functions can be supplied if some specific operations are
> > + * required to get/set a fan control (handle a latch enable GPIO,
> > + * prevent from writing some transitional control value, etc...)
> > + */
> > + int (*get_ctrl)(int num_ctrl, unsigned *ctrl);
> > + void (*set_ctrl)(int num_ctrl, unsigned *ctrl, int ctrl_val);
>
> Not sure if this is a good/useful API. It expects that the called function
> identifies the fan from the ctrl[] array. Not sure how clumsy the resulting code
> might be. It may be better to leave the API out for now until someone actually
> writes a driver (hint, hint, dns323) using it.
I agree and I will happily throw away the clumsy API :)
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101018/93954f42/attachment-0001.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-18 20:36 ` Simon Guinot
@ 2010-10-18 20:50 ` Guenter Roeck
2010-10-19 11:46 ` Simon Guinot
2010-10-19 6:52 ` Guenter Roeck
1 sibling, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-18 20:50 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 18, 2010 at 04:36:10PM -0400, Simon Guinot wrote:
[ ... ]
> > I don't really understand the value of supporting pwm attributes,
> > since you have to convert those to rpm anyway. Why not just stick
> > with fan1_input and fan1_target ? This would simplify the code a lot.
>
> I don't know very well the hwmon API. I have simply been fooled by the
> sysfs-interface document which claim that fan[1-*]_target only make
> sense for a closed-loop fan. Moreover, I was expecting gpio-fan to be
> compliant with the fancontrol shell script...
>
> But anyway, you are right. I just don't want the pwm interface.
>
If the fancontrol script doesn't support fanX_target, and a given fan
doesn't support pwm, it might make sense to update it.
Jean, any comments ?
> > > +
> > > +err_free_gpio:
> > > + for (i = i - 1; i >= 0; i--)
> > > + gpio_free(ctrl[i]);
> > > +
> > This misses the most recently allocated gpio pin if gpio_direction_output() failed.
>
> The gpio is freed above while handling the gpio_direction_output() error.
>
I missed that. Thatks for the clarification.
> > > +
> > > + dev_info(&pdev->dev, "GPIO fan initialized\n");
> > > +
> > Might be a good idea to add a notion of which fan was initialized.
> > After all, there could be more than one.
>
> dev_info() don't do it for me ? anyway, I will check this too.
>
It probably does. Wonder what it actually shows. Can you check ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-18 20:36 ` Simon Guinot
2010-10-18 20:50 ` Guenter Roeck
@ 2010-10-19 6:52 ` Guenter Roeck
2010-10-19 8:36 ` Simon Guinot
1 sibling, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-19 6:52 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 18, 2010 at 04:36:10PM -0400, Simon Guinot wrote:
[ ... ]
> > I don't really understand the value of supporting pwm attributes,
> > since you have to convert those to rpm anyway. Why not just stick
> > with fan1_input and fan1_target ? This would simplify the code a lot.
>
> I don't know very well the hwmon API. I have simply been fooled by the
> sysfs-interface document which claim that fan[1-*]_target only make
> sense for a closed-loop fan. Moreover, I was expecting gpio-fan to be
> compliant with the fancontrol shell script...
>
> But anyway, you are right. I just don't want the pwm interface.
>
Thinking more about this, another option might be to keep using
the pwm interface (for fancontrol), but simplify your code.
Your transitions are currently pwm -> rpm -> ctrl and vice versa.
However, direct conversion pwm -> ctrl should also be possible
and would be much simpler than the two-stage conversion.
To do that, you could map num_speed directly to the pwm range of (0..255).
Something like
pwm = DIV_ROUND_CLOSEST(speed_index * 255, num_speed - 1);
and
speed_index = pwm * (num_speed - 1) / 255;
Then use speed_index to get control value and rpm from the speed table.
Would that make sense ? You could then also provide fan1_target
for direct fan speed control.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-19 6:52 ` Guenter Roeck
@ 2010-10-19 8:36 ` Simon Guinot
2010-10-19 15:15 ` Guenter Roeck
0 siblings, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-19 8:36 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
On Mon, Oct 18, 2010 at 11:52:21PM -0700, Guenter Roeck wrote:
> On Mon, Oct 18, 2010 at 04:36:10PM -0400, Simon Guinot wrote:
> [ ... ]
> > > I don't really understand the value of supporting pwm attributes,
> > > since you have to convert those to rpm anyway. Why not just stick
> > > with fan1_input and fan1_target ? This would simplify the code a lot.
> >
> > I don't know very well the hwmon API. I have simply been fooled by the
> > sysfs-interface document which claim that fan[1-*]_target only make
> > sense for a closed-loop fan. Moreover, I was expecting gpio-fan to be
> > compliant with the fancontrol shell script...
> >
> > But anyway, you are right. I just don't want the pwm interface.
> >
> Thinking more about this, another option might be to keep using
> the pwm interface (for fancontrol), but simplify your code.
> Your transitions are currently pwm -> rpm -> ctrl and vice versa.
> However, direct conversion pwm -> ctrl should also be possible
> and would be much simpler than the two-stage conversion.
>
> To do that, you could map num_speed directly to the pwm range of (0..255).
> Something like
>
> pwm = DIV_ROUND_CLOSEST(speed_index * 255, num_speed - 1);
> and
> speed_index = pwm * (num_speed - 1) / 255;
>
> Then use speed_index to get control value and rpm from the speed table.
>
> Would that make sense ? You could then also provide fan1_target
> for direct fan speed control.
Unfortunately, I believe this approximation is not possible. Take a look
at the Network Space Max fan speed array:
/* Designed for fan 40x40x16: ADDA AD0412LB-D50 6000rpm at 12v */
static struct gpio_fan_speed netspace_max_v2_fan_speed[] = {
{ 0, 0 },
{ 1500, 15 },
{ 1700, 14 },
{ 1800, 13 },
{ 2100, 12 },
{ 3100, 11 },
{ 3300, 10 },
{ 4300, 9 },
{ 5500, 8 },
};
The function rpm_speed(index) is really not linear. I am not sure about
the resulting behaviour when this will be used by a userland fan control
process...
If the pwm interface must be exported, I think we have to respect the
fan speed array. But as you said, maybe the work is rather on the
fancontrol script side.
As a first step, we could keep the heavy pwm interface. Once the
fancontrol script (or something else) will be able to handle the rpm
attributes, we will drop the pwm compatibility.
Simply remove the gpio-fan pwm interface seems a good option too...
Both make sense for me. Let me know your favourite way.
Thanks,
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101019/bb23fe5a/attachment-0001.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-18 20:50 ` Guenter Roeck
@ 2010-10-19 11:46 ` Simon Guinot
2010-10-19 14:52 ` Guenter Roeck
0 siblings, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-19 11:46 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
On Mon, Oct 18, 2010 at 01:50:34PM -0700, Guenter Roeck wrote:
> On Mon, Oct 18, 2010 at 04:36:10PM -0400, Simon Guinot wrote:
> [ ... ]
>
> > > I don't really understand the value of supporting pwm attributes,
> > > since you have to convert those to rpm anyway. Why not just stick
> > > with fan1_input and fan1_target ? This would simplify the code a lot.
> >
> > I don't know very well the hwmon API. I have simply been fooled by the
> > sysfs-interface document which claim that fan[1-*]_target only make
> > sense for a closed-loop fan. Moreover, I was expecting gpio-fan to be
> > compliant with the fancontrol shell script...
> >
> > But anyway, you are right. I just don't want the pwm interface.
> >
> If the fancontrol script doesn't support fanX_target, and a given fan
> doesn't support pwm, it might make sense to update it.
>
> Jean, any comments ?
>
> > > > +
> > > > +err_free_gpio:
> > > > + for (i = i - 1; i >= 0; i--)
> > > > + gpio_free(ctrl[i]);
> > > > +
> > > This misses the most recently allocated gpio pin if gpio_direction_output() failed.
> >
> > The gpio is freed above while handling the gpio_direction_output() error.
> >
> I missed that. Thatks for the clarification.
>
> > > > +
> > > > + dev_info(&pdev->dev, "GPIO fan initialized\n");
> > > > +
> > > Might be a good idea to add a notion of which fan was initialized.
> > > After all, there could be more than one.
> >
> > dev_info() don't do it for me ? anyway, I will check this too.
> >
> It probably does. Wonder what it actually shows. Can you check ?
If CONFIG_PRINTK is enabled, dev_info() end up in a dev_printk() call
with the level argument set to KERN_INFO.
The message format is:
${driver_name} ${device_name}: ${message}
Note that the device id number is part of the device name. So, if two
gpio-fan devices are initialized, you will see:
gpio-fan gpio-fan.0: GPIO fan initialized
gpio-fan gpio-fan.1: GPIO fan initialized
For a single fan device, you will see:
gpio-fan gpio-fan: GPIO fan initialized
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101019/00391386/attachment-0001.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-19 11:46 ` Simon Guinot
@ 2010-10-19 14:52 ` Guenter Roeck
0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2010-10-19 14:52 UTC (permalink / raw)
To: linux-arm-kernel
Hi Simon.
On Tue, Oct 19, 2010 at 07:46:36AM -0400, Simon Guinot wrote:
> Hi Guenter,
[ ... ]
> > It probably does. Wonder what it actually shows. Can you check ?
>
> If CONFIG_PRINTK is enabled, dev_info() end up in a dev_printk() call
> with the level argument set to KERN_INFO.
> The message format is:
> ${driver_name} ${device_name}: ${message}
>
> Note that the device id number is part of the device name. So, if two
> gpio-fan devices are initialized, you will see:
>
> gpio-fan gpio-fan.0: GPIO fan initialized
> gpio-fan gpio-fan.1: GPIO fan initialized
>
> For a single fan device, you will see:
>
> gpio-fan gpio-fan: GPIO fan initialized
>
Ok, guess that is all we can ask for.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-19 8:36 ` Simon Guinot
@ 2010-10-19 15:15 ` Guenter Roeck
2010-10-19 19:30 ` Simon Guinot
0 siblings, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-19 15:15 UTC (permalink / raw)
To: linux-arm-kernel
Hi Simon,
On Tue, Oct 19, 2010 at 04:36:56AM -0400, Simon Guinot wrote:
[ ... ]
> /* Designed for fan 40x40x16: ADDA AD0412LB-D50 6000rpm at 12v */
> static struct gpio_fan_speed netspace_max_v2_fan_speed[] = {
> { 0, 0 },
> { 1500, 15 },
> { 1700, 14 },
> { 1800, 13 },
> { 2100, 12 },
> { 3100, 11 },
> { 3300, 10 },
> { 4300, 9 },
> { 5500, 8 },
> };
>
> The function rpm_speed(index) is really not linear. I am not sure about
> the resulting behaviour when this will be used by a userland fan control
> process...
>
> If the pwm interface must be exported, I think we have to respect the
> fan speed array. But as you said, maybe the work is rather on the
> fancontrol script side.
>
Not sure if there is an expectation that the mapping pwm->speed has
to be linear. One might as well argue that steps should be equally
far apart to ensure that results are better predictable.
> As a first step, we could keep the heavy pwm interface. Once the
> fancontrol script (or something else) will be able to handle the rpm
> attributes, we will drop the pwm compatibility.
> Simply remove the gpio-fan pwm interface seems a good option too...
>
I think we should keep the pwm interface after all. Changing fancontrol
is a possibility, but the change would take a while to make it upstream.
As for the actual code, I would prefer the simpler implementation, but I am willing
to go with the complex one if you feel wtrongly about it. Only thing I would ask you
to do is to add an explanation of what is done and why.
Reminds me - you'll also have to provide Documentation/hwmon/gpio-fan.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-19 15:15 ` Guenter Roeck
@ 2010-10-19 19:30 ` Simon Guinot
0 siblings, 0 replies; 33+ messages in thread
From: Simon Guinot @ 2010-10-19 19:30 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
On Tue, Oct 19, 2010 at 08:15:12AM -0700, Guenter Roeck wrote:
> Hi Simon,
>
> On Tue, Oct 19, 2010 at 04:36:56AM -0400, Simon Guinot wrote:
>
> [ ... ]
>
> > /* Designed for fan 40x40x16: ADDA AD0412LB-D50 6000rpm at 12v */
> > static struct gpio_fan_speed netspace_max_v2_fan_speed[] = {
> > { 0, 0 },
> > { 1500, 15 },
> > { 1700, 14 },
> > { 1800, 13 },
> > { 2100, 12 },
> > { 3100, 11 },
> > { 3300, 10 },
> > { 4300, 9 },
> > { 5500, 8 },
> > };
> >
> > The function rpm_speed(index) is really not linear. I am not sure about
> > the resulting behaviour when this will be used by a userland fan control
> > process...
> >
> > If the pwm interface must be exported, I think we have to respect the
> > fan speed array. But as you said, maybe the work is rather on the
> > fancontrol script side.
> >
> Not sure if there is an expectation that the mapping pwm->speed has
> to be linear. One might as well argue that steps should be equally
> far apart to ensure that results are better predictable.
>
> > As a first step, we could keep the heavy pwm interface. Once the
> > fancontrol script (or something else) will be able to handle the rpm
> > attributes, we will drop the pwm compatibility.
> > Simply remove the gpio-fan pwm interface seems a good option too...
> >
> I think we should keep the pwm interface after all. Changing fancontrol
> is a possibility, but the change would take a while to make it upstream.
Yes, I understand. I will probably look soon into fancontrol.
Some of the cheapest LaCie board have a fan and no temperature sensors.
The embedded system get a more or less accurate temperature from the
hard drive sensor via SMART commands :)
I'd like to have some userspace fan support for this boards too.
>
> As for the actual code, I would prefer the simpler implementation, but I am willing
> to go with the complex one if you feel wtrongly about it. Only thing I would ask you
> to do is to add an explanation of what is done and why.
Looking at the pwm_to_rpm() and rpm_to_pwm() functions, I can't feel
strongly a such implementation (but maybe wrongly).
So... I have changed my mind. The simpler implementation make sense:
the pwm value will simply map the speed index.
Moreover it appear to be very usable from a userspace point.
>
> Reminds me - you'll also have to provide Documentation/hwmon/gpio-fan.
Ok.
Thanks,
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101019/ebed0881/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-17 15:50 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Simon Guinot
2010-10-17 15:50 ` [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2 Simon Guinot
2010-10-18 16:08 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Guenter Roeck
@ 2010-10-19 22:03 ` Guenter Roeck
2010-10-20 0:19 ` Simon Guinot
2 siblings, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-19 22:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Simon,
one more comment ...
On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
[ ... ]
> +static int __devinit
> +fan_alarm_init(struct gpio_fan_data *fan_data, struct gpio_fan_alarm *alarm)
> +{
__devinit and __devexit can cause problems if this driver is built into
the kernel, but the underlying i2c master driver is built as module.
So please remove __devinit and __devexit.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-19 22:03 ` Guenter Roeck
@ 2010-10-20 0:19 ` Simon Guinot
2010-10-20 0:50 ` Guenter Roeck
0 siblings, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-20 0:19 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
On Tue, Oct 19, 2010 at 03:03:45PM -0700, Guenter Roeck wrote:
> Hi Simon,
>
> one more comment ...
>
> On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> [ ... ]
> > +static int __devinit
> > +fan_alarm_init(struct gpio_fan_data *fan_data, struct gpio_fan_alarm *alarm)
> > +{
>
> __devinit and __devexit can cause problems if this driver is built into
> the kernel, but the underlying i2c master driver is built as module.
I am not sure I understand this. Are you talking about a I2C GPIO
expander ? or about the hwmon registration process ?
For the first case, no I2C mean no GPIOs and the gpio-fan initialization
will fail (because no GPIO chip available). There is nothing wrong with
freeing the useless init functions.
For the second case, let me know...
Not really related, but there is too much __devinit and __devexit
declarations in the first patch version. Thanks to the compiler, all
this functions are inlined in the good place.
>
> So please remove __devinit and __devexit.
I will if you tell me why :)
Thanks,
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101020/f0e121e2/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-20 0:19 ` Simon Guinot
@ 2010-10-20 0:50 ` Guenter Roeck
2010-10-20 7:59 ` Simon Guinot
0 siblings, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-20 0:50 UTC (permalink / raw)
To: linux-arm-kernel
Hi Simon,
On Tue, 2010-10-19 at 20:19 -0400, Simon Guinot wrote:
> Hi Guenter,
>
> On Tue, Oct 19, 2010 at 03:03:45PM -0700, Guenter Roeck wrote:
> > Hi Simon,
> >
> > one more comment ...
> >
> > On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > [ ... ]
> > > +static int __devinit
> > > +fan_alarm_init(struct gpio_fan_data *fan_data, struct gpio_fan_alarm *alarm)
> > > +{
> >
> > __devinit and __devexit can cause problems if this driver is built into
> > the kernel, but the underlying i2c master driver is built as module.
>
> I am not sure I understand this. Are you talking about a I2C GPIO
> expander ? or about the hwmon registration process ?
Not related to I2C specifically, since you don't use I2C. No idea why I
mentioned I2C, really. Probably because I had I2C in my mind.
Anyway, there are gpio drivers which can be loaded and unloaded. GPIO
pin(s) on one of those could point to the gpio-fan driver. If gpio-fan
is built into the kernel, but the underlying gpio driver is built as
module, loading the gpio driver might cause the gpio-fan probe function
to be called - but that function no longer exists if it is tagged as
__devinit.
Browsing through the gpio code, it seems the problem doesn't really
exist, since the above scenario does not happen (unless I am missing
something, of course). Also, other drivers using gpio calls also use
__devinit and __devexit. So just ignore what I said.
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-20 0:50 ` Guenter Roeck
@ 2010-10-20 7:59 ` Simon Guinot
0 siblings, 0 replies; 33+ messages in thread
From: Simon Guinot @ 2010-10-20 7:59 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
On Tue, Oct 19, 2010 at 05:50:26PM -0700, Guenter Roeck wrote:
> Hi Simon,
>
> On Tue, 2010-10-19 at 20:19 -0400, Simon Guinot wrote:
> > Hi Guenter,
> >
> > On Tue, Oct 19, 2010 at 03:03:45PM -0700, Guenter Roeck wrote:
> > > Hi Simon,
> > >
> > > one more comment ...
> > >
> > > On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > > [ ... ]
> > > > +static int __devinit
> > > > +fan_alarm_init(struct gpio_fan_data *fan_data, struct gpio_fan_alarm *alarm)
> > > > +{
> > >
> > > __devinit and __devexit can cause problems if this driver is built into
> > > the kernel, but the underlying i2c master driver is built as module.
> >
> > I am not sure I understand this. Are you talking about a I2C GPIO
> > expander ? or about the hwmon registration process ?
>
> Not related to I2C specifically, since you don't use I2C. No idea why I
> mentioned I2C, really. Probably because I had I2C in my mind.
gpio-fan could use I2C implicitly if the GPIOs are provided by a GPIO
I2C expander.
>
> Anyway, there are gpio drivers which can be loaded and unloaded. GPIO
> pin(s) on one of those could point to the gpio-fan driver. If gpio-fan
> is built into the kernel, but the underlying gpio driver is built as
> module, loading the gpio driver might cause the gpio-fan probe function
> to be called - but that function no longer exists if it is tagged as
> __devinit.
IMHO, the gpio-fan probe function will be called anyway if a "gpio-fan"
platform device is available. If the needed GPIOs are not registered at
this time, gpio_request() will fail and the gpio-fan probe function will
exit.
Later, even if the GPIOs are registered (via gpiochip_add()), this will
not trigger a gpio-fan probe.
>
> Browsing through the gpio code, it seems the problem doesn't really
> exist, since the above scenario does not happen (unless I am missing
> something, of course). Also, other drivers using gpio calls also use
> __devinit and __devexit. So just ignore what I said.
Ok.
Thanks,
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101020/62a4f0e4/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-18 16:08 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Guenter Roeck
2010-10-18 18:00 ` Chris Moore
2010-10-18 20:36 ` Simon Guinot
@ 2010-10-21 20:07 ` Simon Guinot
2010-10-21 20:26 ` Guenter Roeck
2 siblings, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-21 20:07 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
On Mon, Oct 18, 2010 at 09:08:30AM -0700, Guenter Roeck wrote:
> On Sun, Oct 17, 2010 at 11:50:11AM -0400, Simon Guinot wrote:
> > From: Simon Guinot <sguinot@lacie.com>
> >
> > This patch adds hwmon support for the GPIO connected fan.
> >
> > Platform specific informations as GPIO pinout and speed conversion array
> > (rpm from/to GPIO value) are passed to the driver via platform_data.
> >
> > Signed-off-by: Simon Guinot <sguinot@lacie.com>
>
> Hi Simon,
>
> good start. Bunch of comments inline. Our mailer was nice enough to replace tabs
> with blanks, thanks to our friends at MicroSomething, so I could not run the patch
> through checkpatch.pl. Hope you did that.
>
> > ---
> > drivers/hwmon/Kconfig | 9 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/gpio-fan.c | 508 ++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/gpio-fan.h | 43 ++++
> > 4 files changed, 561 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/hwmon/gpio-fan.c
> > create mode 100644 include/linux/gpio-fan.h
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 4d4d09b..1dc57c1 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -399,6 +399,15 @@ config SENSORS_GL520SM
> > This driver can also be built as a module. If so, the module
> > will be called gl520sm.
> >
> > +config SENSORS_GPIO_FAN
> > + tristate "GPIO fan"
> > + depends on GENERIC_GPIO
> > + help
> > + If you say yes here you get support for the GPIO connected fan.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called gpio-fan.
> > +
>
> Can you move this up a bit, ahead of SENSORS_FSCHMD ? Trying to stay in sequence.
Ok. Let's try to figure out where is the right location :)
After SENSORS_GL520SM or not ?
Thanks,
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101021/8ce2d545/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] hwmon: add generic GPIO fan driver
2010-10-21 20:07 ` Simon Guinot
@ 2010-10-21 20:26 ` Guenter Roeck
0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2010-10-21 20:26 UTC (permalink / raw)
To: linux-arm-kernel
Hi Simon,
On Thu, Oct 21, 2010 at 04:07:10PM -0400, Simon Guinot wrote:
[ ... ]
> > > @@ -399,6 +399,15 @@ config SENSORS_GL520SM
> > > This driver can also be built as a module. If so, the module
> > > will be called gl520sm.
> > >
> > > +config SENSORS_GPIO_FAN
> > > + tristate "GPIO fan"
> > > + depends on GENERIC_GPIO
> > > + help
> > > + If you say yes here you get support for the GPIO connected fan.
> > > +
> > > + This driver can also be built as a module. If so, the module
> > > + will be called gpio-fan.
> > > +
> >
> > Can you move this up a bit, ahead of SENSORS_FSCHMD ? Trying to stay in sequence.
>
> Ok. Let's try to figure out where is the right location :)
> After SENSORS_GL520SM or not ?
>
My brain must have told me that "G" comes before "F". Or so. ABCDEGF, isn't it ?
After SENSORS_GL520SM. Sorry. Or whereever it fits in the alphabet,
if I got that wrong again ;).
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-17 15:50 ` [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2 Simon Guinot
@ 2010-10-22 1:53 ` Guenter Roeck
2010-10-22 2:08 ` Nicolas Pitre
0 siblings, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-22 1:53 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> From: Simon Guinot <sguinot@lacie.com>
>
> Signed-off-by: Simon Guinot <sguinot@lacie.com>
Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
I assume this will get pushed through the ARM tree, right ?
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 1:53 ` Guenter Roeck
@ 2010-10-22 2:08 ` Nicolas Pitre
2010-10-22 4:30 ` Guenter Roeck
2010-10-22 8:27 ` [PATCH 2/2] " Simon Guinot
0 siblings, 2 replies; 33+ messages in thread
From: Nicolas Pitre @ 2010-10-22 2:08 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 21 Oct 2010, Guenter Roeck wrote:
> On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > From: Simon Guinot <sguinot@lacie.com>
> >
> > Signed-off-by: Simon Guinot <sguinot@lacie.com>
>
> Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
>
> I assume this will get pushed through the ARM tree, right ?
Might be simpler if this is routed along the same path as the core code
it relies upon to avoid dependency ordering issues. There is nothing
particularly ARM specific here anyway.
Nicolas
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 2:08 ` Nicolas Pitre
@ 2010-10-22 4:30 ` Guenter Roeck
2010-10-22 9:29 ` [PATCH] " Simon Guinot
2010-10-22 8:27 ` [PATCH 2/2] " Simon Guinot
1 sibling, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-22 4:30 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Oct 21, 2010 at 10:08:01PM -0400, Nicolas Pitre wrote:
> On Thu, 21 Oct 2010, Guenter Roeck wrote:
>
> > On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > > From: Simon Guinot <sguinot@lacie.com>
> > >
> > > Signed-off-by: Simon Guinot <sguinot@lacie.com>
> >
> > Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
> >
> > I assume this will get pushed through the ARM tree, right ?
>
> Might be simpler if this is routed along the same path as the core code
> it relies upon to avoid dependency ordering issues. There is nothing
> particularly ARM specific here anyway.
>
Fine with me, as long as no one objects. Simon, can you reparent this
on top of your patch v3 (or maybe on top of 2.6.36) and re-send it ?
For some reason git refuses to apply it for me.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 2:08 ` Nicolas Pitre
2010-10-22 4:30 ` Guenter Roeck
@ 2010-10-22 8:27 ` Simon Guinot
2010-10-22 9:58 ` Guenter Roeck
2010-10-22 18:39 ` Guenter Roeck
1 sibling, 2 replies; 33+ messages in thread
From: Simon Guinot @ 2010-10-22 8:27 UTC (permalink / raw)
To: linux-arm-kernel
Hi Nicolas,
On Thu, Oct 21, 2010 at 10:08:01PM -0400, Nicolas Pitre wrote:
> On Thu, 21 Oct 2010, Guenter Roeck wrote:
>
> > On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > > From: Simon Guinot <sguinot@lacie.com>
> > >
> > > Signed-off-by: Simon Guinot <sguinot@lacie.com>
> >
> > Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
> >
> > I assume this will get pushed through the ARM tree, right ?
>
> Might be simpler if this is routed along the same path as the core code
> it relies upon to avoid dependency ordering issues. There is nothing
> particularly ARM specific here anyway.
There is some changes, related to the netspace_v2-setup.c file, stagged
in your Orion tree. At some point, a merge will be needed.
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101022/0fa7f33d/attachment-0001.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 4:30 ` Guenter Roeck
@ 2010-10-22 9:29 ` Simon Guinot
2010-10-22 9:59 ` Guenter Roeck
0 siblings, 1 reply; 33+ messages in thread
From: Simon Guinot @ 2010-10-22 9:29 UTC (permalink / raw)
To: linux-arm-kernel
From: Simon Guinot <sguinot@lacie.com>
Signed-off-by: Simon Guinot <sguinot@lacie.com>
---
arch/arm/mach-kirkwood/netspace_v2-setup.c | 43 ++++++++++++++++++++++++++++
1 files changed, 43 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-kirkwood/netspace_v2-setup.c b/arch/arm/mach-kirkwood/netspace_v2-setup.c
index d26bf32..9e27115 100644
--- a/arch/arm/mach-kirkwood/netspace_v2-setup.c
+++ b/arch/arm/mach-kirkwood/netspace_v2-setup.c
@@ -35,6 +35,7 @@
#include <linux/gpio.h>
#include <linux/gpio_keys.h>
#include <linux/leds.h>
+#include <linux/gpio-fan.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
#include <asm/mach/time.h>
@@ -224,6 +225,46 @@ static struct platform_device netspace_v2_leds = {
};
/*****************************************************************************
+ * GPIO fan
+ ****************************************************************************/
+
+/* Designed for fan 40x40x16: ADDA AD0412LB-D50 6000rpm at 12v */
+static struct gpio_fan_speed netspace_max_v2_fan_speed[] = {
+ { 0, 0 },
+ { 1500, 15 },
+ { 1700, 14 },
+ { 1800, 13 },
+ { 2100, 12 },
+ { 3100, 11 },
+ { 3300, 10 },
+ { 4300, 9 },
+ { 5500, 8 },
+};
+
+static unsigned netspace_max_v2_fan_ctrl[] = { 22, 7, 33, 23 };
+
+static struct gpio_fan_alarm netspace_max_v2_fan_alarm = {
+ .gpio = 25,
+ .active_low = 1,
+};
+
+static struct gpio_fan_platform_data netspace_max_v2_fan_data = {
+ .num_ctrl = ARRAY_SIZE(netspace_max_v2_fan_ctrl),
+ .ctrl = netspace_max_v2_fan_ctrl,
+ .alarm = &netspace_max_v2_fan_alarm,
+ .num_speed = ARRAY_SIZE(netspace_max_v2_fan_speed),
+ .speed = netspace_max_v2_fan_speed,
+};
+
+static struct platform_device netspace_max_v2_gpio_fan = {
+ .name = "gpio-fan",
+ .id = -1,
+ .dev = {
+ .platform_data = &netspace_max_v2_fan_data,
+ },
+};
+
+/*****************************************************************************
* Timer
****************************************************************************/
@@ -307,6 +348,8 @@ static void __init netspace_v2_init(void)
platform_device_register(&netspace_v2_leds);
platform_device_register(&netspace_v2_gpio_leds);
platform_device_register(&netspace_v2_gpio_buttons);
+ if (machine_is_netspace_max_v2())
+ platform_device_register(&netspace_max_v2_gpio_fan);
if (gpio_request(NETSPACE_V2_GPIO_POWER_OFF, "power-off") == 0 &&
gpio_direction_output(NETSPACE_V2_GPIO_POWER_OFF, 0) == 0)
--
1.6.3.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 8:27 ` [PATCH 2/2] " Simon Guinot
@ 2010-10-22 9:58 ` Guenter Roeck
2010-10-22 18:39 ` Guenter Roeck
1 sibling, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2010-10-22 9:58 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Oct 22, 2010 at 04:27:12AM -0400, Simon Guinot wrote:
> Hi Nicolas,
>
> On Thu, Oct 21, 2010 at 10:08:01PM -0400, Nicolas Pitre wrote:
> > On Thu, 21 Oct 2010, Guenter Roeck wrote:
> >
> > > On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > > > From: Simon Guinot <sguinot@lacie.com>
> > > >
> > > > Signed-off-by: Simon Guinot <sguinot@lacie.com>
> > >
> > > Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > >
> > > I assume this will get pushed through the ARM tree, right ?
> >
> > Might be simpler if this is routed along the same path as the core code
> > it relies upon to avoid dependency ordering issues. There is nothing
> > particularly ARM specific here anyway.
>
> There is some changes, related to the netspace_v2-setup.c file, stagged
> in your Orion tree. At some point, a merge will be needed.
>
We'll see if it causes any conflicts in the -next tree after I apply it.
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 9:29 ` [PATCH] " Simon Guinot
@ 2010-10-22 9:59 ` Guenter Roeck
0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2010-10-22 9:59 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Oct 22, 2010 at 05:29:18AM -0400, Simon Guinot wrote:
> From: Simon Guinot <sguinot@lacie.com>
>
> Signed-off-by: Simon Guinot <sguinot@lacie.com>
Applied, thanks.
I'll let it sit in my next tree for a couple of days to see if there are any
resulting conflicts.
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 8:27 ` [PATCH 2/2] " Simon Guinot
2010-10-22 9:58 ` Guenter Roeck
@ 2010-10-22 18:39 ` Guenter Roeck
2010-10-22 18:50 ` Nicolas Pitre
1 sibling, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2010-10-22 18:39 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 2010-10-22 at 04:27 -0400, Simon Guinot wrote:
> Hi Nicolas,
>
> On Thu, Oct 21, 2010 at 10:08:01PM -0400, Nicolas Pitre wrote:
> > On Thu, 21 Oct 2010, Guenter Roeck wrote:
> >
> > > On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > > > From: Simon Guinot <sguinot@lacie.com>
> > > >
> > > > Signed-off-by: Simon Guinot <sguinot@lacie.com>
> > >
> > > Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > >
> > > I assume this will get pushed through the ARM tree, right ?
> >
> > Might be simpler if this is routed along the same path as the core code
> > it relies upon to avoid dependency ordering issues. There is nothing
> > particularly ARM specific here anyway.
>
> There is some changes, related to the netspace_v2-setup.c file, stagged
> in your Orion tree. At some point, a merge will be needed.
>
I rebased to Linus' latest tree which had some changes in
netspace_v2-setup.c. We'll see if that solves the merge problem.
Nicolas, I assume I have your implied Ack for this commit. If not,
please let me know.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2
2010-10-22 18:39 ` Guenter Roeck
@ 2010-10-22 18:50 ` Nicolas Pitre
0 siblings, 0 replies; 33+ messages in thread
From: Nicolas Pitre @ 2010-10-22 18:50 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 22 Oct 2010, Guenter Roeck wrote:
> On Fri, 2010-10-22 at 04:27 -0400, Simon Guinot wrote:
> > Hi Nicolas,
> >
> > On Thu, Oct 21, 2010 at 10:08:01PM -0400, Nicolas Pitre wrote:
> > > On Thu, 21 Oct 2010, Guenter Roeck wrote:
> > >
> > > > On Sun, 2010-10-17 at 11:50 -0400, Simon Guinot wrote:
> > > > > From: Simon Guinot <sguinot@lacie.com>
> > > > >
> > > > > Signed-off-by: Simon Guinot <sguinot@lacie.com>
> > > >
> > > > Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > >
> > > > I assume this will get pushed through the ARM tree, right ?
> > >
> > > Might be simpler if this is routed along the same path as the core code
> > > it relies upon to avoid dependency ordering issues. There is nothing
> > > particularly ARM specific here anyway.
> >
> > There is some changes, related to the netspace_v2-setup.c file, stagged
> > in your Orion tree. At some point, a merge will be needed.
> >
> I rebased to Linus' latest tree which had some changes in
> netspace_v2-setup.c. We'll see if that solves the merge problem.
>
> Nicolas, I assume I have your implied Ack for this commit. If not,
> please let me know.
Yes, you do.
Nicolas
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2010-10-22 18:50 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-22 10:54 [PATCH 4/5] hwmon: DNS323 rev C1 fan support Benjamin Herrenschmidt
2010-05-22 11:00 ` Benjamin Herrenschmidt
2010-10-13 11:59 ` Simon Guinot
2010-10-13 16:34 ` [lm-sensors] " Guenter Roeck
2010-10-17 15:40 ` Simon Guinot
2010-10-17 15:50 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Simon Guinot
2010-10-17 15:50 ` [PATCH 2/2] [ARM] Kirkwood: add fan support for Network Space Max v2 Simon Guinot
2010-10-22 1:53 ` Guenter Roeck
2010-10-22 2:08 ` Nicolas Pitre
2010-10-22 4:30 ` Guenter Roeck
2010-10-22 9:29 ` [PATCH] " Simon Guinot
2010-10-22 9:59 ` Guenter Roeck
2010-10-22 8:27 ` [PATCH 2/2] " Simon Guinot
2010-10-22 9:58 ` Guenter Roeck
2010-10-22 18:39 ` Guenter Roeck
2010-10-22 18:50 ` Nicolas Pitre
2010-10-18 16:08 ` [PATCH 1/2] hwmon: add generic GPIO fan driver Guenter Roeck
2010-10-18 18:00 ` Chris Moore
2010-10-18 18:35 ` Guenter Roeck
2010-10-18 20:36 ` Simon Guinot
2010-10-18 20:50 ` Guenter Roeck
2010-10-19 11:46 ` Simon Guinot
2010-10-19 14:52 ` Guenter Roeck
2010-10-19 6:52 ` Guenter Roeck
2010-10-19 8:36 ` Simon Guinot
2010-10-19 15:15 ` Guenter Roeck
2010-10-19 19:30 ` Simon Guinot
2010-10-21 20:07 ` Simon Guinot
2010-10-21 20:26 ` Guenter Roeck
2010-10-19 22:03 ` Guenter Roeck
2010-10-20 0:19 ` Simon Guinot
2010-10-20 0:50 ` Guenter Roeck
2010-10-20 7:59 ` Simon Guinot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).