[PATCHv2 0/3] Add G762/G763 PWM fan controller

[PATCHv2 0/3] Add G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi,

This series adds support for GMT G762/G763. This work is based on a
basic version for 2.6.31 kernel developed Olivier Mouchet for LaCie
NAS. Updates have been performed to run on recent kernels. Support has
been completed and additional features added: ability to configure
various characteristics from .dts file, better initialization, alarms
and error reporting support, gear mode, polarity, fan pulse per
revolution, fan startup voltage control. The following detailed
datasheet has been used as a basis for this work:

  http://natisbad.org/NAS/refs/GMT_EDS-762_763-080710-0.2.pdf

The patch was developed for and tested against the GMT G762 fan
controller used in a Netgear ReadyNAS Duo v2 (kirkwood 88F6282-based
NAS). This is the main reason for the device tree bindings provided in
first patch. The driver also support init via board file. The patches
are against current ARM tree; tell me if you need me to rebase it
against something else. Patch 2 and 3 provides documentation for the
driver and DT bindings, respectively.

I think most of the comments provided on v0 and v1 have been taken into
account. A detailed list of changes is provided below.

Nonetheless, some remarks made during previous review need some specific
feedback:

 Full speed mode: I started looking how I could emulate full speed mode
 (i.e. sysfs value 0 for "pwm_enable") but I found no easy way to do it
 w/o making things quite complex because the feature is orthogonal to
 fan loop mode (closed or open). This would imply setting *both*
 G762_REG_SET_CNT and G762_REG_SET_OUT registers (respectively for
 closed-loop and open-loop) to their max value (so that a loop mode
 change does not reset full speed), and also keep an internal flag to
 record the fact we are in full speed mode instead of reading current
 mode (PWM or DC) from the device register value. I took a look at some
 other drivers (w83793.c, w83l786ng.c, etc) to see how those emulate the
 feature and they just do not. What do you think?

 Support for board init code: in order to support setting hardware
 characteristics via board init file, I provided a (static const)
 init structure with all the fields of struct g762_platform_data set to
 a specific value (G762_DEFAULT_NO_OVERLOAD). The structure can be used in
 a board init file to initialize a local structure for which specific
 fields can then be given meaningful values. Those which are not
 overloaded will not be pushed to the chip. This is in sync with the
 common policy for hwmon drivers not to change the device configuration
 by default. If you see a more proper way to do it - other than the
 static const struct - I'd be happy to make the change.
 
 Setting mode before fan target value: Guenter, you made a remark about
 the undesirable context to have to set the mode (closed-loop) before 
 setting the fan target. I can change the code and accept the fan target
 value in all cases but this will have no impact before the mode is
 set to closed-loop. At the moment, the user gets a feedback it should
 set the mode before using fan1_target (i.e. invalid value). What do you
 think?

Comments welcome,

Cheers,

a+

Changes since v1
    Changed author
    removed bad tabs
    Provide datasheet link w/o fud in g762 documentation
    removed documentation for removed fan_gear_mode sysfs entry
    removed tested-by from patch
    removed FSF address in header file
    removed useless include of <linux/slab.h>
    removed useless parenthesis against HZ in define
    use spaces around binary operators
    use i2c_smbus_{read,write}_byte_data() instead of g762_{read,write}_value()
    use return value of i2c_smbus_write_byte_data()
    use true for initializing boolean
    removed useless blank lines
    do not enforce single return point rule where less readable
    use dev_err() and dev_dbg() instead of dev_info() when it makes sense
    remove leading '&' for function passed as pointers
    allow passing parameter via platform_data struct for non-DT enabled boards
    set data->valid to false when config is modified
    s/linear/DC/ for mode (g762 datasheet uses linear)
    more tests on rpm_from_cnt() and cnt_from_rpm() formula
    dont overload     

Changes since v0
    removed forward declaration 
    use bool for valid field instead of bit field.
    protect macro args
    fixed typo in subject line
    Added mention for G763 support in Kconfig
    fixed typo in driver name in Kconfig
    do not use DRVNAME in i2c_device_id g762_id[] 
    Following discussions, kept DEVICE_ATTR (i.e. no switch to SENSOR_DEVICE_ATTR)
    removed useless casts when flipping bit values
    Sanity check user input value (e.g. to prevent 256 to silenty become 0)
    Added extra lines for multi line comments when needed
    removed various testing knobs
    make removed knobs available via DT
    passed checkpatch script on the patch
    removed useless lock protection againt clk setting
    moved all setter at the beginning of the file
    removed bad (u16) casts in g762_write_value() calls


Arnaud Ebalard (3):
  Add support for GMT G762/G763 PWM fan controller
  Add documentation for g762 driver
  Add DT bindings documentation for g762 driver

 Documentation/devicetree/bindings/hwmon/g762.txt |   57 ++
 Documentation/hwmon/g762                         |   64 ++
 drivers/hwmon/Kconfig                            |   10 +
 drivers/hwmon/Makefile                           |    1 +
 drivers/hwmon/g762.c                             | 1012 ++++++++++++++++++++++
 include/linux/platform_data/g762.h               |   54 ++
 6 files changed, 1198 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/g762.txt
 create mode 100644 Documentation/hwmon/g762
 create mode 100644 drivers/hwmon/g762.c
 create mode 100644 include/linux/platform_data/g762.h

-- 
1.7.10.4

[PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 drivers/hwmon/Kconfig              |   10 +
 drivers/hwmon/Makefile             |    1 +
 drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
 include/linux/platform_data/g762.h |   54 ++
 4 files changed, 1077 insertions(+)
 create mode 100644 drivers/hwmon/g762.c
 create mode 100644 include/linux/platform_data/g762.h

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0428e8a..142bdf8 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -456,6 +456,16 @@ config SENSORS_G760A
 	  This driver can also be built as a module.  If so, the module
 	  will be called g760a.
 
+config SENSORS_G762
+	tristate "GMT G762 and G763"
+	depends on I2C
+	help
+	  If you say yes here you get support for Global Mixed-mode
+	  Technology Inc G762 and G763 fan speed PWM controller chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called g762.
+
 config SENSORS_GL518SM
 	tristate "Genesys Logic GL518SM"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d17d3e6..4f0fb52 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
 obj-$(CONFIG_SENSORS_FAM15H_POWER) += fam15h_power.o
 obj-$(CONFIG_SENSORS_FSCHMD)	+= fschmd.o
 obj-$(CONFIG_SENSORS_G760A)	+= g760a.o
+obj-$(CONFIG_SENSORS_G762)	+= g762.o
 obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
diff --git a/drivers/hwmon/g762.c b/drivers/hwmon/g762.c
new file mode 100644
index 0000000..d21ec24
--- /dev/null
+++ b/drivers/hwmon/g762.c
@@ -0,0 +1,1012 @@
+/*
+ * g762 - Driver for the Global Mixed-mode Technology Inc. fan speed
+ *        PWM controller chips from G762 family, i.e. G762 and G763
+ *
+ * Copyright (C) 2013, Arnaud EBALARD <arno@natisbad.org>
+ *
+ * This work is based on a basic version for 2.6.31 kernel developed
+ * by Olivier Mouchet for LaCie. Updates and correction have been
+ * performed to run on recent kernels. Additional features, like the
+ * ability to configure various characteristics via .dts file, have
+ * been added. Detailed datasheet on which this development is based
+ * is available here:
+ *
+ *  http://natisbad.org/NAS/refs/GMT_EDS-762_763-080710-0.2.pdf
+ *
+ * Headers from previous developments have been kept below:
+ *
+ * Copyright (c) 2009 LaCie
+ *
+ * Author: Olivier Mouchet <olivier.mouchet@gmail.com>
+ *
+ * based on g760a code written by Herbert Valerio Riedel <hvr@gnu.org>
+ * Copyright (C) 2007  Herbert Valerio Riedel <hvr@gnu.org>
+ *
+ * g762: minimal datasheet available at:
+ *       http://www.gmt.com.tw/product/datasheet/EDS-762_3.pdf
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_data/g762.h>
+
+#define DRVNAME "g762"
+
+static const struct i2c_device_id g762_id[] = {
+	{ "g762", 0 },
+	{ "g763", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, g762_id);
+
+enum g762_regs {
+	G762_REG_SET_CNT  = 0x00,
+	G762_REG_ACT_CNT  = 0x01,
+	G762_REG_FAN_STA  = 0x02,
+	G762_REG_SET_OUT  = 0x03,
+	G762_REG_FAN_CMD1 = 0x04,
+	G762_REG_FAN_CMD2 = 0x05,
+};
+
+/* Config register bits */
+#define G762_REG_FAN_CMD1_DET_FAN_FAIL  0x80 /* enable fan_fail signal */
+#define G762_REG_FAN_CMD1_DET_FAN_OOC   0x40 /* enable fan_out_of_control */
+#define G762_REG_FAN_CMD1_OUT_MODE      0x20 /* out mode, PWM or DC */
+#define G762_REG_FAN_CMD1_FAN_MODE      0x10 /* fan mode: closed/open loop */
+#define G762_REG_FAN_CMD1_CLK_DIV_ID1   0x08 /* clock divisor value */
+#define G762_REG_FAN_CMD1_CLK_DIV_ID0   0x04
+#define G762_REG_FAN_CMD1_PWM_POLARITY  0x02 /* PWM polarity */
+#define G762_REG_FAN_CMD1_PULSE_PER_REV 0x01 /* pulse per fan revolution */
+
+#define G762_REG_FAN_CMD2_GEAR_MODE_1   0x08 /* fan gear mode */
+#define G762_REG_FAN_CMD2_GEAR_MODE_0   0x04
+#define G762_REG_FAN_CMD2_FAN_STARTV_1  0x02 /* fan startup voltage */
+#define G762_REG_FAN_CMD2_FAN_STARTV_0  0x01
+
+#define G762_REG_FAN_STA_FAIL           0x02 /* fan fail */
+#define G762_REG_FAN_STA_OOC            0x01 /* fan out of control */
+
+/* config register values */
+#define OUT_MODE_PWM            1
+#define OUT_MODE_DC             0
+
+#define FAN_MODE_CLOSED_LOOP    2
+#define FAN_MODE_OPEN_LOOP      1
+
+/* register data is read (and cached) at most once per second */
+#define G762_UPDATE_INTERVAL    HZ
+
+/*
+ * extract pulse count per fan revolution value (2 or 4) from given
+ * FAN_CMD1 register value
+ */
+#define PULSE_FROM_REG(reg) \
+	((((reg) & G762_REG_FAN_CMD1_PULSE_PER_REV)+1) << 1)
+
+/*
+ * extract fan clock divisor (1, 2, 4 or 8) from given FAN_CMD1
+ * register value
+ */
+#define CLKDIV_FROM_REG(reg) \
+	(1 << (((reg) & (G762_REG_FAN_CMD1_CLK_DIV_ID0 |	\
+			 G762_REG_FAN_CMD1_CLK_DIV_ID1)) >> 2))
+
+/*
+ * extract fan gear mode multiplier value (0, 2 or 4) from given
+ * FAN_CMD2 register value
+ */
+#define GEARMULT_FROM_REG(reg) \
+	(1 << (((reg) & (G762_REG_FAN_CMD2_GEAR_MODE_0 |	\
+			 G762_REG_FAN_CMD2_GEAR_MODE_1)) >> 2))
+
+struct g762_data {
+	struct i2c_client *client;
+	struct device *hwmon_dev;
+
+	/* update mutex */
+	struct mutex update_lock;
+
+	/* board specific parameters. */
+	u32 clk;
+
+	/* g762 register cache */
+	bool valid;
+	unsigned long last_updated; /* in jiffies */
+
+	u8 set_cnt;  /* controls fan rotation speed in closed-loop mode */
+	u8 act_cnt;  /* provides access to current fan RPM value */
+	u8 fan_sta;  /* bit 0: set when actual fan speed is more than
+		      *        25% outside requested fan speed
+		      * bit 1: set when no transition occurs on fan
+		      *        pin for 0.7s
+		      */
+	u8 set_out;  /* controls fan rotation speed in open-loop mode */
+	u8 fan_cmd1; /*   0: FG_PLS_ID0 FG pulses count per revolution
+		      *      0: 2 counts per revolution
+		      *      1: 4 counts per revolution
+		      *   1: PWM_POLARITY 1: negative_duty
+		      *                   0: positive_duty
+		      * 2,3: [FG_CLOCK_ID0, FG_CLK_ID1]
+		      *         00: Divide fan clock by 1
+		      *         01: Divide fan clock by 2
+		      *         10: Divide fan clock by 4
+		      *         11: Divide fan clock by 8
+		      *   4: FAN_MODE 1:closed-loop, 0:open-loop
+		      *   5: OUT_MODE 1:PWM, 0:DC
+		      *   6: DET_FAN_OOC enable "fan ooc" status
+		      *   7: DET_FAN_FAIL enable "fan fail" status
+		      */
+	u8 fan_cmd2; /* 0,1: FAN_STARTV 0,1,2,3 -> 0,32,64,96 dac_code
+		      * 2,3: FG_GEAR_MODE
+		      *         00: multiplier = 1
+		      *         01: multiplier = 2
+		      *         10: multiplier = 4
+		      *   4: Mask ALERT# (g763 only)
+		      */
+};
+
+/*
+ * sysfs PWM interface uses value from 0 to 255 when g762 FAN_SET_CNT register
+ * uses values from 255 (off) to 0 (full speed). Note that FAN_SET_OUT register
+ * uses values from 0 (off) to 255 (full speed), i.e. does not need translation.
+ */
+#define PWM_FROM_CNT(cnt)       (0xff - (cnt))
+#define PWM_TO_CNT(pwm)         (0xff - (pwm))
+
+/*
+ * Convert count value from fan controller registers (FAN_SET_CNT/FAN_ACT_CNT)
+ * into fan speed RPM value. Note that the datasheet documents a basic formula.
+ * Influence of additional parameters (fan clock divisor, fan gear mode) have
+ * been infered from examples in the datasheet and tests.
+ */
+static inline unsigned int rpm_from_cnt(u8 cnt, u32 clk, u16 p,
+					u8 clk_div, u8 gear_mult)
+{
+	if (cnt == 0xff)  /* cnt to 255 stops the fan */
+		return 0;
+
+	return (clk*30*gear_mult)/((cnt ? cnt : 1)*p*clk_div);
+}
+
+/* Convert fan RPM value from sysfs into count value for fan controller register
+ * (FAN_SET_CNT). */
+static inline unsigned char cnt_from_rpm(u32 rpm, u32 clk, u16 p,
+					 u8 clk_div, u8 gear_mult)
+{
+	if (!rpm)         /* to stop the fan, set cnt to 255 */
+		return 0xff;
+
+	return clamp_val(((clk*30*gear_mult)/(rpm*p*clk_div)), 0, 255);
+}
+
+/* helper to grab and cache data, at most one time per second */
+static struct g762_data *g762_update_client(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+	if (time_before(jiffies, data->last_updated + G762_UPDATE_INTERVAL) &&
+	    likely(data->valid))
+		goto out;
+
+	data->set_cnt =  i2c_smbus_read_byte_data(client, G762_REG_SET_CNT);
+	data->act_cnt =  i2c_smbus_read_byte_data(client, G762_REG_ACT_CNT);
+	data->fan_sta =  i2c_smbus_read_byte_data(client, G762_REG_FAN_STA);
+	data->set_out =  i2c_smbus_read_byte_data(client, G762_REG_SET_OUT);
+	data->fan_cmd1 = i2c_smbus_read_byte_data(client, G762_REG_FAN_CMD1);
+	data->fan_cmd2 = i2c_smbus_read_byte_data(client, G762_REG_FAN_CMD2);
+
+	data->last_updated = jiffies;
+	data->valid = true;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+/*
+ * helpers for passing hardware characteristics via DT. Some of those
+ * are also used by sysfs handlers (write functions) later in the file.
+ */
+
+/* Set pwm mode. Accepts either 0 (PWM mode) or 1 (DC mode) */
+static int do_set_pwm_mode(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (val) {
+	case OUT_MODE_PWM:
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_OUT_MODE;
+		break;
+	case OUT_MODE_DC:
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_OUT_MODE;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
+					data->fan_cmd1);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/*
+ * Set reference clock. Accepted values are between 0 and 0xffffff.
+ * Note that this is a characteristics of the system but an internal
+ * parameter, i.e. value is not passed to the device.
+ */
+static int do_set_pwm_freq(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = i2c_get_clientdata(client);
+
+	if (val > 0xffffff)
+		return -EINVAL;
+
+	data->clk = val;
+
+	return 0;
+}
+
+/* Set fan clock divisor. Accepted values are 1, 2, 4 and 8. */
+static int do_set_fan_div(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (val) {
+	case 1:
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID0;
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID1;
+		break;
+	case 2:
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID0;
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID1;
+		break;
+	case 4:
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID0;
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID1;
+		break;
+	case 8:
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID0;
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID1;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
+					data->fan_cmd1);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Set fan gear mode. Accepted values are either 0, 1 or 2. */
+static int do_set_fan_gear_mode(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (val) {
+	case 0:
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_0;
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_1;
+		break;
+	case 1:
+		data->fan_cmd2 |=  G762_REG_FAN_CMD2_GEAR_MODE_0;
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_1;
+		break;
+	case 2:
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_0;
+		data->fan_cmd2 |=  G762_REG_FAN_CMD2_GEAR_MODE_1;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD2,
+					data->fan_cmd2);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Set pulse per revolution value. Accepts either 2 or 4. */
+static int do_set_fan_pulses(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (val) {
+	case 2:
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_PULSE_PER_REV;
+		break;
+	case 4:
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_PULSE_PER_REV;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
+					data->fan_cmd1);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Set fan mode. Accepts either 1 (open-loop) or 2 (closed-loop). */
+static int do_set_pwm_enable(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (val) {
+	case FAN_MODE_OPEN_LOOP:
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_FAN_MODE;
+		break;
+	case FAN_MODE_CLOSED_LOOP:
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_FAN_MODE;
+		break;
+	default:
+		goto out;
+	}
+
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
+					data->fan_cmd1);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Set PWM polarity (0 for negative duty, 1 for positive duty) */
+static int do_set_pwm_polarity(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (val) {
+	case 0: /* i.e. negative duty */
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_PWM_POLARITY;
+		break;
+	case 1: /* i.e. positive duty */
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_PWM_POLARITY;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
+					data->fan_cmd1);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/*
+ * Set pwm value. Accepted values are between 0 (stops the fan) and 255 (full
+ * speed). Note that the chip register used for setting the value depends on
+ * current fan mode of the device (closed or open-loop).
+ */
+static int do_set_pwm(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret;
+	u8 reg;
+
+	if (val > 255)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
+		val = PWM_TO_CNT(val);
+		data->set_cnt = val;
+		reg = G762_REG_SET_CNT;
+	} else {                                           /* open-loop */
+		data->set_out = val;
+		reg = G762_REG_SET_OUT;
+	}
+	ret = i2c_smbus_write_byte_data(client, reg, (u8)val);
+	data->valid = false;
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Set fan RPM value. This only makes sense in closed-loop mode. */
+static int do_set_fan_target(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
+		data->set_cnt = cnt_from_rpm(val, data->clk,
+					     PULSE_FROM_REG(data->fan_cmd1),
+					     CLKDIV_FROM_REG(data->fan_cmd1),
+					     GEARMULT_FROM_REG(data->fan_cmd2));
+		ret = i2c_smbus_write_byte_data(client, G762_REG_SET_CNT,
+						data->set_cnt);
+		data->valid = false;
+	}
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Enable/disable fan failure detection. Accepted values are 1 and 0. */
+static int do_fan_failure_detection_toggle(struct device *dev,
+					   unsigned long enable)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (enable) {
+	case 0:
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_DET_FAN_FAIL;
+		break;
+	case 1:
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_DET_FAN_FAIL;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
+					data->fan_cmd1);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Enable/disable fan out of control detection. Accepted values are 1 and 0 */
+static int do_fan_ooc_detection_toggle(struct device *dev, unsigned int enable)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (enable) {
+	case 0:
+		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_DET_FAN_OOC;
+		break;
+	case 1:
+		data->fan_cmd1 |=  G762_REG_FAN_CMD1_DET_FAN_OOC;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
+					data->fan_cmd1);
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/* Set fan startup voltage. Accepted values are either 0, 1, 2 or 3. */
+static int do_set_fan_startv(struct device *dev, unsigned long val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct g762_data *data = g762_update_client(dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	switch (val) {
+	case 0:
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_0;
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_1;
+		data->valid = false;
+		break;
+	case 1:
+		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_0;
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_1;
+		data->valid = false;
+		break;
+	case 2:
+		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_0;
+		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_1;
+		data->valid = false;
+		break;
+	case 3:
+		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_0;
+		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_1;
+		data->valid = false;
+		break;
+	default:
+		goto out;
+	}
+	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD2,
+					data->fan_cmd2);
+	data->valid = false;
+ out:
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+/*
+ * Configuration-related definitions
+ */
+
+static inline void g762_platform_data_init(struct g762_platform_data *pdata)
+{
+	*pdata = G762_DEFAULT_PDATA;
+}
+
+static inline int g762_one_prop_commit(struct i2c_client *client,
+				       u32 pval, const char *pname,
+				       int (*psetter)(struct device *dev,
+						      unsigned long val))
+{
+	int ret = 0;
+
+	if ((pval != G762_DEFAULT_NO_OVERLOAD) &&
+	    (*psetter)(&client->dev, pval)) {
+		dev_err(&client->dev, "unable to set %s (%d)\n", pname, pval);
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int g762_platform_data_commit(struct i2c_client *client,
+				     struct g762_platform_data *pdata)
+{
+	int ret = 0;
+
+	if (g762_one_prop_commit(client, pdata->fan_startv,
+				 "fan_startv", do_set_fan_startv) ||
+	    g762_one_prop_commit(client, pdata->fan_div,
+				 "fan_div", do_set_fan_div) ||
+	    g762_one_prop_commit(client, pdata->fan_pulses,
+				 "fan_pulses", do_set_fan_pulses) ||
+	    g762_one_prop_commit(client, pdata->pwm_freq,
+				 "pwm_freq", do_set_pwm_freq) ||
+	    g762_one_prop_commit(client, pdata->fan_gear_mode,
+				 "fan_gear_mode", do_set_fan_gear_mode) ||
+	    g762_one_prop_commit(client, pdata->pwm_polarity,
+				 "pwm_polarity", do_set_pwm_polarity) ||
+	    g762_one_prop_commit(client, pdata->pwm_mode,
+				 "pwm_mode", do_set_pwm_mode) ||
+	    g762_one_prop_commit(client, pdata->pwm_enable,
+				 "pwm_enable", do_set_pwm_enable) ||
+	    g762_one_prop_commit(client, pdata->fan_target,
+				 "fan_target", do_set_fan_target)) {
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+/*
+ * Helpers to import hardware characteristics from .dts file and overload
+ * default platform data values.
+ */
+
+#ifdef CONFIG_OF
+static struct of_device_id g762_dt_match[] = {
+	{ .compatible = "gmt,g762" },
+	{ .compatible = "gmt,g763" },
+	{ },
+};
+
+static inline void g762_of_import_one_prop(struct i2c_client *client,
+					   u32 *dest, const char *pname)
+{
+	const __be32 *prop;
+	int len;
+
+	prop = of_get_property(client->dev.of_node, pname, &len);
+	if (prop && len == sizeof(u32)) {
+		*dest = be32_to_cpu(prop[0]);
+		dev_dbg(&client->dev, "found %s (%d)\n", pname, *dest);
+	}
+}
+
+static void g762_platform_data_of_overload(struct i2c_client *client,
+					   struct g762_platform_data *pdata)
+{
+	if (!client->dev.of_node)
+		return;
+
+	g762_of_import_one_prop(client, &pdata->fan_gear_mode, "fan_gear_mode");
+	g762_of_import_one_prop(client, &pdata->pwm_polarity, "pwm_polarity");
+	g762_of_import_one_prop(client, &pdata->fan_startv, "fan_startv");
+	g762_of_import_one_prop(client, &pdata->pwm_freq, "pwm_freq");
+	g762_of_import_one_prop(client, &pdata->fan_div, "fan_div");
+	g762_of_import_one_prop(client, &pdata->fan_pulses, "fan_pulses");
+	g762_of_import_one_prop(client, &pdata->pwm_mode, "pwm_mode");
+	g762_of_import_one_prop(client, &pdata->fan_target, "fan_target");
+	g762_of_import_one_prop(client, &pdata->pwm_enable, "pwm_enable");
+}
+#else
+static void g762_platform_data_of_overload(struct i2c_client *client,
+				    struct g762_platform_data *pdata) { }
+#endif
+
+/*
+ * helper to overload driver parameters from board init file for those
+ * not already converted to device tree
+ */
+static void g762_platform_data_overload(struct i2c_client *client,
+					struct g762_platform_data *pdata)
+{
+	struct g762_platform_data *opdata = client->dev.platform_data;
+
+	if (opdata)
+		*pdata = *opdata;
+}
+
+/*
+ * sysfs attributes
+ */
+
+/*
+ * Read function for fan1_input sysfs file. Return current fan RPM value, or
+ * 0 if fan is out of control. The function is available in both closed and
+ * open-loop mode.
+ */
+static ssize_t get_fan_rpm(struct device *dev, struct device_attribute *da,
+			   char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+	unsigned int rpm = 0;
+
+	mutex_lock(&data->update_lock);
+	/* reverse logic: fan out of control reporting is enabled low */
+	if (data->fan_sta & G762_REG_FAN_STA_OOC) {
+		rpm = rpm_from_cnt(data->act_cnt, data->clk,
+				   PULSE_FROM_REG(data->fan_cmd1),
+				   CLKDIV_FROM_REG(data->fan_cmd1),
+				   GEARMULT_FROM_REG(data->fan_cmd2));
+	}
+	mutex_unlock(&data->update_lock);
+
+	return sprintf(buf, "%u\n", rpm);
+}
+
+/*
+ * Read and write functions for pwm1_mode sysfs file. Get and set fan speed
+ * control mode i.e. PWM (1) or DC (0).
+ */
+static ssize_t get_pwm_mode(struct device *dev, struct device_attribute *da,
+			    char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+
+	return sprintf(buf, "%d\n",
+		       !!(data->fan_cmd1 & G762_REG_FAN_CMD1_OUT_MODE));
+}
+
+static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *da,
+			    const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val) || do_set_pwm_mode(dev, val))
+		return -EINVAL;
+
+	return count;
+}
+
+/*
+ * Read and write functions for fan1_div sysfs file. Get and set fan
+ * controller prescaler value
+ */
+static ssize_t get_fan_div(struct device *dev,
+			       struct device_attribute *da, char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+
+	return sprintf(buf, "%d\n", CLKDIV_FROM_REG(data->fan_cmd1));
+}
+
+static ssize_t set_fan_div(struct device *dev,
+			       struct device_attribute *da,
+			       const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val) || do_set_fan_div(dev, val))
+		return -EINVAL;
+
+	return count;
+}
+
+/*
+ * Read and write functions for pwm1_enable. Get and set the fan speed control
+ * mode (i.e. closed or open-loop).
+ *
+ * Following documentation about hwmon's sysfs interface, a pwm1_enable node
+ * should accept followings:
+ *
+ *  0 : no fan speed control (i.e. fan at full speed)
+ *  1 : manual fan speed control enabled (use pwm[1-*]) (open-loop)
+ *  2+: automatic fan speed control enabled (use fan[1-*]_target) (closed-loop)
+ *
+ * but we do not accept 0 as "no-control" mode is not supported by g762,
+ * -EINVAL is returned in this case.
+ */
+static ssize_t get_pwm_enable(struct device *dev,
+			      struct device_attribute *da, char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+
+	return sprintf(buf, "%d\n",
+		       (!!(data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE)) + 1);
+}
+
+static ssize_t set_pwm_enable(struct device *dev,
+			      struct device_attribute *da,
+			      const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val) || do_set_pwm_enable(dev, val))
+		return -EINVAL;
+
+	return count;
+}
+
+/*
+ * Read and write functions for pwm1 sysfs file. Get and set the fan speed
+ * in both open and closed-loop mode. Speed is given as a relative value
+ * between 0 and 255, where 255 is maximum speed. Note that due to rounding
+ * errors it is possible that you don't read back exactly the value you have
+ * set.
+ */
+static ssize_t get_pwm(struct device *dev, struct device_attribute *da,
+		       char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+	int val;
+
+	mutex_lock(&data->update_lock);
+	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
+		val = PWM_FROM_CNT(data->set_cnt);
+	} else {                                           /* open-loop */
+		val = data->set_out;
+	}
+	mutex_unlock(&data->update_lock);
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
+		       const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val) || do_set_pwm(dev, val))
+		return -EINVAL;
+
+	return count;
+}
+
+/*
+ * Read and write function for fan1_target sysfs file. Get/set the fan speed in
+ * closed-loop mode. Speed is given as a RPM value; then the chip will regulate
+ * the fan speed using pulses from fan tachometer. The functions both return
+ * -EINVAL when called in open-loop mode.
+ *
+ * Refer to rpm_from_cnt() implementation above to get info about count number
+ * calculation.
+ *
+ * Also note that due to rounding errors it is possible that you don't read
+ * back exactly the value you have set.
+ */
+static ssize_t get_fan_target(struct device *dev, struct device_attribute *da,
+			      char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+	unsigned int rpm;
+	ssize_t ret = -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
+		rpm = rpm_from_cnt(data->set_cnt, data->clk,
+				   PULSE_FROM_REG(data->fan_cmd1),
+				   CLKDIV_FROM_REG(data->fan_cmd1),
+				   GEARMULT_FROM_REG(data->fan_cmd2));
+		ret = sprintf(buf, "%u\n", rpm);
+	}
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+static ssize_t set_fan_target(struct device *dev, struct device_attribute *da,
+			      const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val) || do_set_fan_target(dev, val))
+		return -EINVAL;
+
+	return count;
+}
+
+/* read function for fan1_fault sysfs file. */
+static ssize_t get_fan_failure(struct device *dev, struct device_attribute *da,
+			       char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+
+	return sprintf(buf, "%u\n", !!(data->fan_sta & G762_REG_FAN_STA_FAIL));
+}
+
+/*
+ * read function for fan1_alarm sysfs file. Note that OOC confition is
+ * enabled low
+ */
+static ssize_t get_fan_ooc(struct device *dev, struct device_attribute *da,
+			   char *buf)
+{
+	struct g762_data *data = g762_update_client(dev);
+
+	return sprintf(buf, "%u\n", !(data->fan_sta & G762_REG_FAN_STA_OOC));
+}
+
+static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, get_pwm, set_pwm);
+static DEVICE_ATTR(pwm1_mode, S_IWUSR | S_IRUGO, get_pwm_mode, set_pwm_mode);
+static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
+		   get_pwm_enable, set_pwm_enable);
+static DEVICE_ATTR(fan1_input, S_IRUGO, get_fan_rpm, NULL);
+static DEVICE_ATTR(fan1_alarm, S_IRUGO, get_fan_ooc, NULL);
+static DEVICE_ATTR(fan1_fault, S_IRUGO, get_fan_failure, NULL);
+static DEVICE_ATTR(fan1_target, S_IWUSR | S_IRUGO,
+		   get_fan_target, set_fan_target);
+static DEVICE_ATTR(fan1_div, S_IWUSR | S_IRUGO, get_fan_div, set_fan_div);
+
+/* Driver data */
+static struct attribute *g762_attributes[] = {
+	&dev_attr_fan1_input.attr,
+	&dev_attr_fan1_alarm.attr,
+	&dev_attr_fan1_fault.attr,
+	&dev_attr_fan1_target.attr,
+	&dev_attr_fan1_div.attr,
+	&dev_attr_pwm1.attr,
+	&dev_attr_pwm1_mode.attr,
+	&dev_attr_pwm1_enable.attr,
+	NULL
+};
+
+static const struct attribute_group g762_group = {
+	.attrs = g762_attributes,
+};
+
+static int g762_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct g762_data *data;
+	struct g762_platform_data pdata;
+	int err;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	data = devm_kzalloc(&client->dev, sizeof(struct g762_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, data);
+
+	data->client = client;
+	mutex_init(&data->update_lock);
+
+	/* Enable fan protection and fan fail detection by default */
+	do_fan_ooc_detection_toggle(&client->dev, 1);
+	do_fan_failure_detection_toggle(&client->dev, 1);
+
+	/*
+	 * Set default configuration values before passing the structure
+	 * first to overload routine used by boards non converted to DT and
+	 * then to OF helper (to overload them using those provided by .dts
+	 * file, if any). Final result is then commited.
+	 */
+	g762_platform_data_init(&pdata);
+	g762_platform_data_overload(client, &pdata);
+	g762_platform_data_of_overload(client, &pdata);
+	err = g762_platform_data_commit(client, &pdata);
+	if (err)
+		return err;
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&client->dev.kobj, &g762_group);
+	if (err)
+		return err;
+
+	data->hwmon_dev = (struct device *)hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		goto err_out;
+	}
+
+	return 0;
+
+ err_out:
+	sysfs_remove_group(&client->dev.kobj, &g762_group);
+	return err;
+}
+
+static int g762_remove(struct i2c_client *client)
+{
+	struct g762_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &g762_group);
+
+	return 0;
+}
+
+static struct i2c_driver g762_driver = {
+	.driver = {
+		.name = DRVNAME,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(g762_dt_match),
+	},
+	.probe	  = g762_probe,
+	.remove	  = g762_remove,
+	.id_table = g762_id,
+};
+
+module_i2c_driver(g762_driver);
+
+MODULE_AUTHOR("Arnaud EBALARD <arno@natisbad.org>");
+MODULE_DESCRIPTION("GMT G762/G763 driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_data/g762.h b/include/linux/platform_data/g762.h
new file mode 100644
index 0000000..29e3934
--- /dev/null
+++ b/include/linux/platform_data/g762.h
@@ -0,0 +1,54 @@
+/*
+ * Platform data structure for g762 fan controller driver
+ *
+ * Copyright (C) 2013, Arnaud EBALARD <arno@natisbad.org>
+ *
+ * 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
+ */
+#ifndef __LINUX_PLATFORM_DATA_G762_H__
+#define __LINUX_PLATFORM_DATA_G762_H__
+
+#define G762_DEFAULT_NO_OVERLOAD 0xffffffff
+
+struct g762_platform_data {
+	u32 fan_startv;
+	u32 fan_gear_mode;
+	u32 fan_div;
+	u32 fan_pulses;
+	u32 fan_target;
+	u32 pwm_polarity;
+	u32 pwm_enable;
+	u32 pwm_freq;
+	u32 pwm_mode;
+};
+
+/* When filling a g762_platform_data structure to be passed during platform
+ * init to the driver, it needs to be initialized to the following default
+ * value before changing specific fields. This is needed to avoid a sparse
+ * initialization to have current values replaced by 0 */
+
+static const struct g762_platform_data G762_DEFAULT_PDATA = {
+	.fan_startv = G762_DEFAULT_NO_OVERLOAD,
+	.fan_gear_mode = G762_DEFAULT_NO_OVERLOAD,
+	.fan_div = G762_DEFAULT_NO_OVERLOAD,
+	.fan_pulses = G762_DEFAULT_NO_OVERLOAD,
+	.fan_target = G762_DEFAULT_NO_OVERLOAD,
+	.pwm_polarity = G762_DEFAULT_NO_OVERLOAD,
+	.pwm_enable = G762_DEFAULT_NO_OVERLOAD,
+	.pwm_freq = G762_DEFAULT_NO_OVERLOAD,
+	.pwm_mode = G762_DEFAULT_NO_OVERLOAD,
+};
+
+#endif /* __LINUX_PLATFORM_DATA_G762_H__ */
-- 
1.7.10.4

[PATCHv2 2/3] Add documentation for g762 driver

[Arnaud Ebalard]

Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 Documentation/hwmon/g762 |   64 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/hwmon/g762

diff --git a/Documentation/hwmon/g762 b/Documentation/hwmon/g762
new file mode 100644
index 0000000..b749fe1
--- /dev/null
+++ b/Documentation/hwmon/g762
@@ -0,0 +1,64 @@
+Kernel driver g762
+==================
+
+The GMT G762 Fan Speed PWM Controller is connected directly to a fan
+and performs closed-loop or open-loop control of the fan speed. Two
+modes - PWM or DC - are supported by the device.
+
+For additional information, a detailed datasheet is available at
+http://natisbad.org/NAS/ref/GMT_EDS-762_763-080710-0.2.pdf. sysfs
+bindings are described in Documentation/hwmon/sysfs-interface.
+
+The following entries are available to the user in a subdirectory of
+/sys/bus/i2c/drivers/g762/ to control the operation of the device.
+This can be done manually using the following entries but is usually
+done via a userland daemon like fancontrol.
+
+Note that those entries do not provide ways to setup the specific
+hardware characteristics of the system (reference clock, pulses per
+fan revolution, ...); Those can be modified via devicetree bindings
+documented in Documentation/devicetree/bindings/hwmon/g762.txt.
+
+
+  fan1_target: set desired fan speed. This only makes sense in closed-loop
+            fan speed control (i.e. when pwm1_enable is set to 2).
+
+  fan1_input: provide current fan rotation value in RPM as reported by
+            the fan to the device.
+
+  fan1_div: fan clock divisor. Supported value are 1, 2, 4 and 8.
+
+  fan1_fault: reports fan failure, i.e. no transition on fan gear pin for
+            about 0.7s (if the fan is not voluntarily set off).
+
+  fan1_alarm: in closed-loop control mode, if fan RPM value is 25% out
+            of the programmed value for over 6 seconds 'fan1_alarm' is
+            set to 1.
+
+  pwm1_enable: set current fan speed control mode i.e. 1 for manual fan
+            speed control (open-loop) via pwm1 described below, 2 for
+            automatic fan speed control (closed-loop) via fan1_target
+            above (pwm1 is also usable).
+
+  pwm1_mode: set or get fan driving mode: 1 for PWM mode, 0 for DC mode.
+
+  pwm1: get or set PWM fan control value. This is an integer value
+            between 0 and 255. 0 stops the fan, 255 makes it run at
+            full speed. Note that the fan speed control mode (open vs
+            closed-loop) must be set prior via pwm1_enable prior to
+            passing a value via pwm1.
+
+
+Both in PWM mode ('pwm1_mode' set to 1) and DC mode ('pwm1_mode' set to 0),
+when current fan speed control mode is open-loop ('pwm1_enable' set to 1),
+the fan speed is programmed by setting a value between 0 and 255 via 'pwm1'
+entry (0 stops the fan, 255 makes it run at full speed). In closed-loop mode
+('pwm1_enable' set to 2), the expected rotation speed in RPM can be passed to
+the chip via 'fan1_target'. In closed-loop mode, the target speed is compared
+with current speed (available via 'fan1_input') by the device and a feedback
+is performed to match that target value. The fan speed value is computed
+based on the parameters associated with the physical characteristics of the
+system: a reference clock source frequency, a number of pulses per fan
+revolution, etc.
+
+Note that the driver will update its values at most once per second.
-- 
1.7.10.4

[PATCHv2 3/3] Add DT bindings documentation for g762 driver

[Arnaud Ebalard]

Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
---
 Documentation/devicetree/bindings/hwmon/g762.txt |   57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/g762.txt

diff --git a/Documentation/devicetree/bindings/hwmon/g762.txt b/Documentation/devicetree/bindings/hwmon/g762.txt
new file mode 100644
index 0000000..f7ac01a
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/g762.txt
@@ -0,0 +1,57 @@
+GMT G762/G763 PWM Fan controller
+
+Required node properties:
+
+ - "compatible": must be either "gmt,g762" or "gmt,g763"
+ - "reg": I2C bus address of the device
+
+Optional properties:
+
+ - "pwm_mode": fan driving mode. 1 for PWM mode, 0 for linear.
+
+ - "pwm_enable": fan speed control. 1 for open-loop, 2 for closed-loop.
+
+ - "pwm_freq": reference clock frequency for PWM mode in Hz.
+
+ - "fan_pulses": number of pulses per fan revolution. Supported values
+               are 2 and 4.
+
+ - "fan_div": fan clock frequency divisor value. Supported values are 1,
+               2, 4 and 8. Default is 1.
+
+ - "fan_target": initial target fan speed in RPM. Only works in closed-loop
+               fan speed control, i.e. when pwm_enable has already been set
+               to 2.
+
+ - "fan_startv": fan startup voltage. Accepted values are 0, 1, 2 and 3.
+               The higher the more.
+
+ - "pwm_polarity": pwm polarity. Accepted values are 0 (positive duty)
+               and 1 (negative duty).
+
+ - "fan_gear_mode": fan gear mode. Supported values are 0, 1 and 2.
+
+If an optional property is not set in .dts file, then current value is kept
+unmodified (e.g. u-boot installed value).
+
+
+Additional information on operational parameters for the device is available
+in Documentation/hwmon/g762. A detailed datasheet for the device is available
+at http://natisbad.org/NAS/refs/GMT_EDS-762_763-080710-0.2.pdf.
+
+Example g762 node:
+
+   g762: g762@3e {
+	compatible = "gmt,g762";
+	reg = <0x3e>;
+	pwm_mode = <1>;      /* closed-loop control */
+	pwm_enable = <2>;    /* PWM mode */
+	pwm_freq = <8192>;   /* PWM reference clock freq */
+	fan_pulses = <2>;    /* 2 pulses per rev */
+	fan_div = <2>;       /* fan clock divisor */
+	fan_target = <2000>; /* target fan speed at 2000 RPM */
+	fan_gear_mode = <0>; /* chip default */
+	fan_startv = <1>;    /* chip default */
+	pwm_polarity = <0>;  /* chip default */
+   };
+
-- 
1.7.10.4

Re: [PATCHv2 0/3] Add G762/G763 PWM fan controller

[Arnd Bergmann]

On Tuesday 28 May 2013 00:02:29 Arnaud Ebalard wrote:
> 
> This series adds support for GMT G762/G763. This work is based on a
> basic version for 2.6.31 kernel developed Olivier Mouchet for LaCie
> NAS. Updates have been performed to run on recent kernels. Support has
> been completed and additional features added: ability to configure
> various characteristics from .dts file, better initialization, alarms
> and error reporting support, gear mode, polarity, fan pulse per
> revolution, fan startup voltage control.

I wonder if this could be split into two separate drivers, one for
the pwm subsystem, and one for a hardware-independent fan controller
based on the pwm interfaces.

	Arnd

Re: [PATCHv2 0/3] Add G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi Arnd,

Arnd Bergmann <arnd@arndb.de> writes:

> On Tuesday 28 May 2013 00:02:29 Arnaud Ebalard wrote:
>> 
>> This series adds support for GMT G762/G763. This work is based on a
>> basic version for 2.6.31 kernel developed Olivier Mouchet for LaCie
>> NAS. Updates have been performed to run on recent kernels. Support has
>> been completed and additional features added: ability to configure
>> various characteristics from .dts file, better initialization, alarms
>> and error reporting support, gear mode, polarity, fan pulse per
>> revolution, fan startup voltage control.
>
> I wonder if this could be split into two separate drivers, one for
> the pwm subsystem, and one for a hardware-independent fan controller
> based on the pwm interfaces.

To be honest, I wouldn't even know how to start in order to do that.
Additionally, it would be worth the effort if other drivers could be
easily refactored using this idea. I'll let hwmon people provide some
feedback and try and comply with their directions (based on the cpu
cycle I can spend on this).

Cheers,

a+

Re: [PATCHv2 0/3] Add G762/G763 PWM fan controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

On Tue, May 28, 2013 at 12:15:04PM +0200, Arnaud Ebalard wrote:
> Hi Arnd,
> 
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > On Tuesday 28 May 2013 00:02:29 Arnaud Ebalard wrote:
> >> 
> >> This series adds support for GMT G762/G763. This work is based on a
> >> basic version for 2.6.31 kernel developed Olivier Mouchet for LaCie
> >> NAS. Updates have been performed to run on recent kernels. Support has
> >> been completed and additional features added: ability to configure
> >> various characteristics from .dts file, better initialization, alarms
> >> and error reporting support, gear mode, polarity, fan pulse per
> >> revolution, fan startup voltage control.
> >
> > I wonder if this could be split into two separate drivers, one for
> > the pwm subsystem, and one for a hardware-independent fan controller
> > based on the pwm interfaces.
> 
> To be honest, I wouldn't even know how to start in order to do that.
> Additionally, it would be worth the effort if other drivers could be
> easily refactored using this idea. I'll let hwmon people provide some
> feedback and try and comply with their directions (based on the cpu
> cycle I can spend on this).

What Arnd proposes doesn't sounds like a very good idea. PWM-controlled
fans should be able to work just fine using a generic driver that uses
the PWM framework, similar to the PWM backlight and LED drivers.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 0/3] Add G762/G763 PWM fan controller

[Guenter Roeck]

On Tue, May 28, 2013 at 01:19:23PM +0200, Thierry Reding wrote:
> On Tue, May 28, 2013 at 12:15:04PM +0200, Arnaud Ebalard wrote:
> > Hi Arnd,
> > 
> > Arnd Bergmann <arnd@arndb.de> writes:
> > 
> > > On Tuesday 28 May 2013 00:02:29 Arnaud Ebalard wrote:
> > >> 
> > >> This series adds support for GMT G762/G763. This work is based on a
> > >> basic version for 2.6.31 kernel developed Olivier Mouchet for LaCie
> > >> NAS. Updates have been performed to run on recent kernels. Support has
> > >> been completed and additional features added: ability to configure
> > >> various characteristics from .dts file, better initialization, alarms
> > >> and error reporting support, gear mode, polarity, fan pulse per
> > >> revolution, fan startup voltage control.
> > >
> > > I wonder if this could be split into two separate drivers, one for
> > > the pwm subsystem, and one for a hardware-independent fan controller
> > > based on the pwm interfaces.
> > 
> > To be honest, I wouldn't even know how to start in order to do that.
> > Additionally, it would be worth the effort if other drivers could be
> > easily refactored using this idea. I'll let hwmon people provide some
> > feedback and try and comply with their directions (based on the cpu
> > cycle I can spend on this).
> 
> What Arnd proposes doesn't sounds like a very good idea. PWM-controlled
> fans should be able to work just fine using a generic driver that uses
> the PWM framework, similar to the PWM backlight and LED drivers.
> 
Isn't that exactly what Arnd proposed ?

Guenter

Re: [PATCHv2 0/3] Add G762/G763 PWM fan controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

On Tue, May 28, 2013 at 05:29:40AM -0700, Guenter Roeck wrote:
> On Tue, May 28, 2013 at 01:19:23PM +0200, Thierry Reding wrote:
> > On Tue, May 28, 2013 at 12:15:04PM +0200, Arnaud Ebalard wrote:
> > > Hi Arnd,
> > > 
> > > Arnd Bergmann <arnd@arndb.de> writes:
> > > 
> > > > On Tuesday 28 May 2013 00:02:29 Arnaud Ebalard wrote:
> > > >> 
> > > >> This series adds support for GMT G762/G763. This work is based on a
> > > >> basic version for 2.6.31 kernel developed Olivier Mouchet for LaCie
> > > >> NAS. Updates have been performed to run on recent kernels. Support has
> > > >> been completed and additional features added: ability to configure
> > > >> various characteristics from .dts file, better initialization, alarms
> > > >> and error reporting support, gear mode, polarity, fan pulse per
> > > >> revolution, fan startup voltage control.
> > > >
> > > > I wonder if this could be split into two separate drivers, one for
> > > > the pwm subsystem, and one for a hardware-independent fan controller
> > > > based on the pwm interfaces.
> > > 
> > > To be honest, I wouldn't even know how to start in order to do that.
> > > Additionally, it would be worth the effort if other drivers could be
> > > easily refactored using this idea. I'll let hwmon people provide some
> > > feedback and try and comply with their directions (based on the cpu
> > > cycle I can spend on this).
> > 
> > What Arnd proposes doesn't sounds like a very good idea. PWM-controlled
> > fans should be able to work just fine using a generic driver that uses
> > the PWM framework, similar to the PWM backlight and LED drivers.
> > 
> Isn't that exactly what Arnd proposed ?

Oh, right. I meant to say: "What Arnd proposes sounds like a very good
idea." =) Sorry for the confusion.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 0/3] Add G762/G763 PWM fan controller

[Guenter Roeck]

On Tue, May 28, 2013 at 03:47:49PM +0200, Thierry Reding wrote:
> On Tue, May 28, 2013 at 05:29:40AM -0700, Guenter Roeck wrote:
> > On Tue, May 28, 2013 at 01:19:23PM +0200, Thierry Reding wrote:
> > > On Tue, May 28, 2013 at 12:15:04PM +0200, Arnaud Ebalard wrote:
> > > > Hi Arnd,
> > > > 
> > > > Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> writes:
> > > > 
> > > > > On Tuesday 28 May 2013 00:02:29 Arnaud Ebalard wrote:
> > > > >> 
> > > > >> This series adds support for GMT G762/G763. This work is based on a
> > > > >> basic version for 2.6.31 kernel developed Olivier Mouchet for LaCie
> > > > >> NAS. Updates have been performed to run on recent kernels. Support has
> > > > >> been completed and additional features added: ability to configure
> > > > >> various characteristics from .dts file, better initialization, alarms
> > > > >> and error reporting support, gear mode, polarity, fan pulse per
> > > > >> revolution, fan startup voltage control.
> > > > >
> > > > > I wonder if this could be split into two separate drivers, one for
> > > > > the pwm subsystem, and one for a hardware-independent fan controller
> > > > > based on the pwm interfaces.
> > > > 
> > > > To be honest, I wouldn't even know how to start in order to do that.
> > > > Additionally, it would be worth the effort if other drivers could be
> > > > easily refactored using this idea. I'll let hwmon people provide some
> > > > feedback and try and comply with their directions (based on the cpu
> > > > cycle I can spend on this).
> > > 
> > > What Arnd proposes doesn't sounds like a very good idea. PWM-controlled
> > > fans should be able to work just fine using a generic driver that uses
> > > the PWM framework, similar to the PWM backlight and LED drivers.
> > > 
> > Isn't that exactly what Arnd proposed ?
> 
> Oh, right. I meant to say: "What Arnd proposes sounds like a very good
> idea." =) Sorry for the confusion.
> 
I looked into it, and frankly I don't know how to make it work either. The chip
is dedicated to fan control and supports both linear and pwm mode. Both modes
are controlled differently. The available configuration options are very fan
control specific.

While I agree that a generic fan control driver for pwm chips would be great,
problem here is that this is not a generic pwm controller. 

Thanks,
Guenter

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 4415 bytes --]

On Tue, May 28, 2013 at 12:03:14AM +0200, Arnaud Ebalard wrote:
> 
> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
> ---
>  drivers/hwmon/Kconfig              |   10 +
>  drivers/hwmon/Makefile             |    1 +
>  drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
>  include/linux/platform_data/g762.h |   54 ++
>  4 files changed, 1077 insertions(+)
>  create mode 100644 drivers/hwmon/g762.c
>  create mode 100644 include/linux/platform_data/g762.h

Hi Arnaud,

After more tests on my 2Big Network v2 board, it appears that the fan
doesn't rotate when PWM mode (the preferred operating mode for this
board) is selected. Nevertheless, DC mode is usable (even if not ideal
given the hardware). After some investigations I noticed that an extra
initialization is needed to enable PWM mode on my board: the set_cnt
register must be set to 0 while the default value is 0xff. Is that
specific to my hardware ? Is PWM mode working on your ReadyNAS with
the default set_cnt value ?

I haven't noticed this issue while testing your v1 patch series because
at the time I was using a board with an U-Boot modified by LaCie. This
last sets the set_cnt register to 0 while U-Boot mainline don't.
Actually, the 2Big fan is not configured nor even started by U-Boot
mainline. I have to fix this but maybe that something can be done in
the g762 Linux driver too ?

> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0428e8a..142bdf8 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig

Snip

> diff --git a/include/linux/platform_data/g762.h b/include/linux/platform_data/g762.h
> new file mode 100644
> index 0000000..29e3934
> --- /dev/null
> +++ b/include/linux/platform_data/g762.h
> @@ -0,0 +1,54 @@
> +/*
> + * Platform data structure for g762 fan controller driver
> + *
> + * Copyright (C) 2013, Arnaud EBALARD <arno@natisbad.org>
> + *
> + * 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
> + */
> +#ifndef __LINUX_PLATFORM_DATA_G762_H__
> +#define __LINUX_PLATFORM_DATA_G762_H__
> +
> +#define G762_DEFAULT_NO_OVERLOAD 0xffffffff
> +
> +struct g762_platform_data {
> +	u32 fan_startv;
> +	u32 fan_gear_mode;
> +	u32 fan_div;
> +	u32 fan_pulses;
> +	u32 fan_target;
> +	u32 pwm_polarity;
> +	u32 pwm_enable;
> +	u32 pwm_freq;
> +	u32 pwm_mode;
> +};
> +
> +/* When filling a g762_platform_data structure to be passed during platform
> + * init to the driver, it needs to be initialized to the following default
> + * value before changing specific fields. This is needed to avoid a sparse
> + * initialization to have current values replaced by 0 */

In other places, you use this multi-line comment format:

/*
 * ...
 * ...
 */

> +
> +static const struct g762_platform_data G762_DEFAULT_PDATA = {
> +	.fan_startv = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_gear_mode = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_div = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_pulses = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_target = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_polarity = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_enable = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_freq = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_mode = G762_DEFAULT_NO_OVERLOAD,
> +};

Are you sure that all this settings are needed ? IMHO the g762 platform
data should only holds the platform specific configuration: fan_startv,
fan_gear_mode, fan_div and fan_pulses. The other settings are not really
related with the platform but more with the operating fan configuration.
For my part, I am happy enough with the sysfs hwmon interface.

The same comment applies to the g762 DT binding properties.

Regards,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Guenter Roeck]

On Tue, May 28, 2013 at 12:03:14AM +0200, Arnaud Ebalard wrote:
> 
> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
> ---
>  drivers/hwmon/Kconfig              |   10 +
>  drivers/hwmon/Makefile             |    1 +
>  drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
>  include/linux/platform_data/g762.h |   54 ++
>  4 files changed, 1077 insertions(+)
>  create mode 100644 drivers/hwmon/g762.c
>  create mode 100644 include/linux/platform_data/g762.h
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0428e8a..142bdf8 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -456,6 +456,16 @@ config SENSORS_G760A
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called g760a.
>  
> +config SENSORS_G762
> +	tristate "GMT G762 and G763"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Global Mixed-mode
> +	  Technology Inc G762 and G763 fan speed PWM controller chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called g762.
> +
>  config SENSORS_GL518SM
>  	tristate "Genesys Logic GL518SM"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index d17d3e6..4f0fb52 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
>  obj-$(CONFIG_SENSORS_FAM15H_POWER) += fam15h_power.o
>  obj-$(CONFIG_SENSORS_FSCHMD)	+= fschmd.o
>  obj-$(CONFIG_SENSORS_G760A)	+= g760a.o
> +obj-$(CONFIG_SENSORS_G762)	+= g762.o
>  obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
>  obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
>  obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
> diff --git a/drivers/hwmon/g762.c b/drivers/hwmon/g762.c
> new file mode 100644
> index 0000000..d21ec24
> --- /dev/null
> +++ b/drivers/hwmon/g762.c
> @@ -0,0 +1,1012 @@
> +/*
> + * g762 - Driver for the Global Mixed-mode Technology Inc. fan speed
> + *        PWM controller chips from G762 family, i.e. G762 and G763
> + *
> + * Copyright (C) 2013, Arnaud EBALARD <arno@natisbad.org>
> + *
> + * This work is based on a basic version for 2.6.31 kernel developed
> + * by Olivier Mouchet for LaCie. Updates and correction have been
> + * performed to run on recent kernels. Additional features, like the
> + * ability to configure various characteristics via .dts file, have
> + * been added. Detailed datasheet on which this development is based
> + * is available here:
> + *
> + *  http://natisbad.org/NAS/refs/GMT_EDS-762_763-080710-0.2.pdf
> + *
> + * Headers from previous developments have been kept below:
> + *
> + * Copyright (c) 2009 LaCie
> + *
> + * Author: Olivier Mouchet <olivier.mouchet@gmail.com>
> + *
> + * based on g760a code written by Herbert Valerio Riedel <hvr@gnu.org>
> + * Copyright (C) 2007  Herbert Valerio Riedel <hvr@gnu.org>
> + *
> + * g762: minimal datasheet available at:
> + *       http://www.gmt.com.tw/product/datasheet/EDS-762_3.pdf
> + *
> + * 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.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_data/g762.h>
> +
> +#define DRVNAME "g762"
> +
> +static const struct i2c_device_id g762_id[] = {
> +	{ "g762", 0 },
> +	{ "g763", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, g762_id);
> +
> +enum g762_regs {
> +	G762_REG_SET_CNT  = 0x00,
> +	G762_REG_ACT_CNT  = 0x01,
> +	G762_REG_FAN_STA  = 0x02,
> +	G762_REG_SET_OUT  = 0x03,
> +	G762_REG_FAN_CMD1 = 0x04,
> +	G762_REG_FAN_CMD2 = 0x05,
> +};
> +
> +/* Config register bits */
> +#define G762_REG_FAN_CMD1_DET_FAN_FAIL  0x80 /* enable fan_fail signal */
> +#define G762_REG_FAN_CMD1_DET_FAN_OOC   0x40 /* enable fan_out_of_control */
> +#define G762_REG_FAN_CMD1_OUT_MODE      0x20 /* out mode, PWM or DC */
> +#define G762_REG_FAN_CMD1_FAN_MODE      0x10 /* fan mode: closed/open loop */
> +#define G762_REG_FAN_CMD1_CLK_DIV_ID1   0x08 /* clock divisor value */
> +#define G762_REG_FAN_CMD1_CLK_DIV_ID0   0x04
> +#define G762_REG_FAN_CMD1_PWM_POLARITY  0x02 /* PWM polarity */
> +#define G762_REG_FAN_CMD1_PULSE_PER_REV 0x01 /* pulse per fan revolution */
> +
> +#define G762_REG_FAN_CMD2_GEAR_MODE_1   0x08 /* fan gear mode */
> +#define G762_REG_FAN_CMD2_GEAR_MODE_0   0x04
> +#define G762_REG_FAN_CMD2_FAN_STARTV_1  0x02 /* fan startup voltage */
> +#define G762_REG_FAN_CMD2_FAN_STARTV_0  0x01
> +
> +#define G762_REG_FAN_STA_FAIL           0x02 /* fan fail */
> +#define G762_REG_FAN_STA_OOC            0x01 /* fan out of control */
> +
> +/* config register values */
> +#define OUT_MODE_PWM            1
> +#define OUT_MODE_DC             0
> +
> +#define FAN_MODE_CLOSED_LOOP    2
> +#define FAN_MODE_OPEN_LOOP      1
> +
> +/* register data is read (and cached) at most once per second */
> +#define G762_UPDATE_INTERVAL    HZ
> +
> +/*
> + * extract pulse count per fan revolution value (2 or 4) from given
> + * FAN_CMD1 register value
> + */
> +#define PULSE_FROM_REG(reg) \
> +	((((reg) & G762_REG_FAN_CMD1_PULSE_PER_REV)+1) << 1)
> +
> +/*
> + * extract fan clock divisor (1, 2, 4 or 8) from given FAN_CMD1
> + * register value
> + */
> +#define CLKDIV_FROM_REG(reg) \
> +	(1 << (((reg) & (G762_REG_FAN_CMD1_CLK_DIV_ID0 |	\
> +			 G762_REG_FAN_CMD1_CLK_DIV_ID1)) >> 2))
> +
> +/*
> + * extract fan gear mode multiplier value (0, 2 or 4) from given
> + * FAN_CMD2 register value
> + */
> +#define GEARMULT_FROM_REG(reg) \
> +	(1 << (((reg) & (G762_REG_FAN_CMD2_GEAR_MODE_0 |	\
> +			 G762_REG_FAN_CMD2_GEAR_MODE_1)) >> 2))
> +
> +struct g762_data {
> +	struct i2c_client *client;
> +	struct device *hwmon_dev;
> +
> +	/* update mutex */
> +	struct mutex update_lock;
> +
> +	/* board specific parameters. */
> +	u32 clk;
> +
> +	/* g762 register cache */
> +	bool valid;
> +	unsigned long last_updated; /* in jiffies */
> +
> +	u8 set_cnt;  /* controls fan rotation speed in closed-loop mode */
> +	u8 act_cnt;  /* provides access to current fan RPM value */
> +	u8 fan_sta;  /* bit 0: set when actual fan speed is more than
> +		      *        25% outside requested fan speed
> +		      * bit 1: set when no transition occurs on fan
> +		      *        pin for 0.7s
> +		      */
> +	u8 set_out;  /* controls fan rotation speed in open-loop mode */
> +	u8 fan_cmd1; /*   0: FG_PLS_ID0 FG pulses count per revolution
> +		      *      0: 2 counts per revolution
> +		      *      1: 4 counts per revolution
> +		      *   1: PWM_POLARITY 1: negative_duty
> +		      *                   0: positive_duty
> +		      * 2,3: [FG_CLOCK_ID0, FG_CLK_ID1]
> +		      *         00: Divide fan clock by 1
> +		      *         01: Divide fan clock by 2
> +		      *         10: Divide fan clock by 4
> +		      *         11: Divide fan clock by 8
> +		      *   4: FAN_MODE 1:closed-loop, 0:open-loop
> +		      *   5: OUT_MODE 1:PWM, 0:DC
> +		      *   6: DET_FAN_OOC enable "fan ooc" status
> +		      *   7: DET_FAN_FAIL enable "fan fail" status
> +		      */
> +	u8 fan_cmd2; /* 0,1: FAN_STARTV 0,1,2,3 -> 0,32,64,96 dac_code
> +		      * 2,3: FG_GEAR_MODE
> +		      *         00: multiplier = 1
> +		      *         01: multiplier = 2
> +		      *         10: multiplier = 4
> +		      *   4: Mask ALERT# (g763 only)
> +		      */
> +};
> +
> +/*
> + * sysfs PWM interface uses value from 0 to 255 when g762 FAN_SET_CNT register
> + * uses values from 255 (off) to 0 (full speed). Note that FAN_SET_OUT register
> + * uses values from 0 (off) to 255 (full speed), i.e. does not need translation.
> + */
> +#define PWM_FROM_CNT(cnt)       (0xff - (cnt))
> +#define PWM_TO_CNT(pwm)         (0xff - (pwm))
> +
> +/*
> + * Convert count value from fan controller registers (FAN_SET_CNT/FAN_ACT_CNT)
> + * into fan speed RPM value. Note that the datasheet documents a basic formula.
> + * Influence of additional parameters (fan clock divisor, fan gear mode) have
> + * been infered from examples in the datasheet and tests.
> + */
> +static inline unsigned int rpm_from_cnt(u8 cnt, u32 clk, u16 p,
> +					u8 clk_div, u8 gear_mult)
> +{
> +	if (cnt == 0xff)  /* cnt to 255 stops the fan */
> +		return 0;
> +
> +	return (clk*30*gear_mult)/((cnt ? cnt : 1)*p*clk_div);

Please follow CodingStyle and add spaces between operators (not just here).

> +}
> +
> +/* Convert fan RPM value from sysfs into count value for fan controller register
> + * (FAN_SET_CNT). */
> +static inline unsigned char cnt_from_rpm(u32 rpm, u32 clk, u16 p,
> +					 u8 clk_div, u8 gear_mult)
> +{
> +	if (!rpm)         /* to stop the fan, set cnt to 255 */
> +		return 0xff;
> +
> +	return clamp_val(((clk*30*gear_mult)/(rpm*p*clk_div)), 0, 255);
> +}
> +
> +/* helper to grab and cache data, at most one time per second */
> +static struct g762_data *g762_update_client(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->update_lock);
> +	if (time_before(jiffies, data->last_updated + G762_UPDATE_INTERVAL) &&
> +	    likely(data->valid))
> +		goto out;
> +
> +	data->set_cnt =  i2c_smbus_read_byte_data(client, G762_REG_SET_CNT);
> +	data->act_cnt =  i2c_smbus_read_byte_data(client, G762_REG_ACT_CNT);
> +	data->fan_sta =  i2c_smbus_read_byte_data(client, G762_REG_FAN_STA);
> +	data->set_out =  i2c_smbus_read_byte_data(client, G762_REG_SET_OUT);
> +	data->fan_cmd1 = i2c_smbus_read_byte_data(client, G762_REG_FAN_CMD1);
> +	data->fan_cmd2 = i2c_smbus_read_byte_data(client, G762_REG_FAN_CMD2);
> +
Failed i2c reads (returning negative values) not a problem ? Driver operation
will be more or less random in that case, and may result in erratic behavior.

> +	data->last_updated = jiffies;
> +	data->valid = true;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +/*
> + * helpers for passing hardware characteristics via DT. Some of those
> + * are also used by sysfs handlers (write functions) later in the file.
> + */
> +
> +/* Set pwm mode. Accepts either 0 (PWM mode) or 1 (DC mode) */
> +static int do_set_pwm_mode(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (val) {
> +	case OUT_MODE_PWM:
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_OUT_MODE;
> +		break;
> +	case OUT_MODE_DC:
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_OUT_MODE;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
> +					data->fan_cmd1);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * Set reference clock. Accepted values are between 0 and 0xffffff.
> + * Note that this is a characteristics of the system but an internal
> + * parameter, i.e. value is not passed to the device.
> + */
> +static int do_set_pwm_freq(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = i2c_get_clientdata(client);
> +
> +	if (val > 0xffffff)
> +		return -EINVAL;
> +
> +	data->clk = val;
> +
> +	return 0;
> +}
> +
> +/* Set fan clock divisor. Accepted values are 1, 2, 4 and 8. */
> +static int do_set_fan_div(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (val) {
> +	case 1:
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID0;
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID1;
> +		break;
> +	case 2:
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID0;
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID1;
> +		break;
> +	case 4:
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_CLK_DIV_ID0;
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID1;
> +		break;
> +	case 8:
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID0;
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_CLK_DIV_ID1;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
> +					data->fan_cmd1);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Set fan gear mode. Accepted values are either 0, 1 or 2. */
> +static int do_set_fan_gear_mode(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (val) {
> +	case 0:
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_0;
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_1;
> +		break;
> +	case 1:
> +		data->fan_cmd2 |=  G762_REG_FAN_CMD2_GEAR_MODE_0;
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_1;
> +		break;
> +	case 2:
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_GEAR_MODE_0;
> +		data->fan_cmd2 |=  G762_REG_FAN_CMD2_GEAR_MODE_1;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD2,
> +					data->fan_cmd2);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Set pulse per revolution value. Accepts either 2 or 4. */
> +static int do_set_fan_pulses(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (val) {
> +	case 2:
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_PULSE_PER_REV;
> +		break;
> +	case 4:
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_PULSE_PER_REV;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
> +					data->fan_cmd1);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Set fan mode. Accepts either 1 (open-loop) or 2 (closed-loop). */
> +static int do_set_pwm_enable(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (val) {
> +	case FAN_MODE_OPEN_LOOP:
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_FAN_MODE;
> +		break;
> +	case FAN_MODE_CLOSED_LOOP:
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_FAN_MODE;
> +		break;
> +	default:
> +		goto out;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
> +					data->fan_cmd1);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Set PWM polarity (0 for negative duty, 1 for positive duty) */
> +static int do_set_pwm_polarity(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (val) {
> +	case 0: /* i.e. negative duty */
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_PWM_POLARITY;
> +		break;
> +	case 1: /* i.e. positive duty */
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_PWM_POLARITY;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
> +					data->fan_cmd1);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * Set pwm value. Accepted values are between 0 (stops the fan) and 255 (full
> + * speed). Note that the chip register used for setting the value depends on
> + * current fan mode of the device (closed or open-loop).
> + */
> +static int do_set_pwm(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret;
> +	u8 reg;
> +
> +	if (val > 255)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
> +		val = PWM_TO_CNT(val);
> +		data->set_cnt = val;
> +		reg = G762_REG_SET_CNT;
> +	} else {                                           /* open-loop */
> +		data->set_out = val;
> +		reg = G762_REG_SET_OUT;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, reg, (u8)val);
> +	data->valid = false;
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Set fan RPM value. This only makes sense in closed-loop mode. */
> +static int do_set_fan_target(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
> +		data->set_cnt = cnt_from_rpm(val, data->clk,
> +					     PULSE_FROM_REG(data->fan_cmd1),
> +					     CLKDIV_FROM_REG(data->fan_cmd1),
> +					     GEARMULT_FROM_REG(data->fan_cmd2));
> +		ret = i2c_smbus_write_byte_data(client, G762_REG_SET_CNT,
> +						data->set_cnt);

In closed loop mode you have two paths to set the same register. Given that,
it doesn't make sense to me to set the fan target with set_pwm as well.
If you use one attribute (set_pwm) for setting set_out, and the other for
set_cnt, your problems with conditional command acceptance should be solved.

> +		data->valid = false;
> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Enable/disable fan failure detection. Accepted values are 1 and 0. */
> +static int do_fan_failure_detection_toggle(struct device *dev,
> +					   unsigned long enable)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (enable) {
> +	case 0:
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_DET_FAN_FAIL;
> +		break;
> +	case 1:
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_DET_FAN_FAIL;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
> +					data->fan_cmd1);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Enable/disable fan out of control detection. Accepted values are 1 and 0 */
> +static int do_fan_ooc_detection_toggle(struct device *dev, unsigned int enable)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (enable) {
> +	case 0:
> +		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_DET_FAN_OOC;
> +		break;
> +	case 1:
> +		data->fan_cmd1 |=  G762_REG_FAN_CMD1_DET_FAN_OOC;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD1,
> +					data->fan_cmd1);
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/* Set fan startup voltage. Accepted values are either 0, 1, 2 or 3. */
> +static int do_set_fan_startv(struct device *dev, unsigned long val)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct g762_data *data = g762_update_client(dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	switch (val) {
> +	case 0:
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_0;
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_1;
> +		data->valid = false;
> +		break;
> +	case 1:
> +		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_0;
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_1;
> +		data->valid = false;
> +		break;
> +	case 2:
> +		data->fan_cmd2 &= ~G762_REG_FAN_CMD2_FAN_STARTV_0;
> +		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_1;
> +		data->valid = false;
> +		break;
> +	case 3:
> +		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_0;
> +		data->fan_cmd2 |=  G762_REG_FAN_CMD2_FAN_STARTV_1;
> +		data->valid = false;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	ret = i2c_smbus_write_byte_data(client, G762_REG_FAN_CMD2,
> +					data->fan_cmd2);
> +	data->valid = false;
> + out:
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * Configuration-related definitions
> + */
> +
> +static inline void g762_platform_data_init(struct g762_platform_data *pdata)
> +{
> +	*pdata = G762_DEFAULT_PDATA;
> +}
> +
> +static inline int g762_one_prop_commit(struct i2c_client *client,
> +				       u32 pval, const char *pname,
> +				       int (*psetter)(struct device *dev,
> +						      unsigned long val))
> +{
> +	int ret = 0;
> +
> +	if ((pval != G762_DEFAULT_NO_OVERLOAD) &&
> +	    (*psetter)(&client->dev, pval)) {
> +		dev_err(&client->dev, "unable to set %s (%d)\n", pname, pval);
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int g762_platform_data_commit(struct i2c_client *client,
> +				     struct g762_platform_data *pdata)
> +{
> +	int ret = 0;
> +
> +	if (g762_one_prop_commit(client, pdata->fan_startv,
> +				 "fan_startv", do_set_fan_startv) ||
> +	    g762_one_prop_commit(client, pdata->fan_div,
> +				 "fan_div", do_set_fan_div) ||
> +	    g762_one_prop_commit(client, pdata->fan_pulses,
> +				 "fan_pulses", do_set_fan_pulses) ||
> +	    g762_one_prop_commit(client, pdata->pwm_freq,
> +				 "pwm_freq", do_set_pwm_freq) ||
> +	    g762_one_prop_commit(client, pdata->fan_gear_mode,
> +				 "fan_gear_mode", do_set_fan_gear_mode) ||
> +	    g762_one_prop_commit(client, pdata->pwm_polarity,
> +				 "pwm_polarity", do_set_pwm_polarity) ||
> +	    g762_one_prop_commit(client, pdata->pwm_mode,
> +				 "pwm_mode", do_set_pwm_mode) ||
> +	    g762_one_prop_commit(client, pdata->pwm_enable,
> +				 "pwm_enable", do_set_pwm_enable) ||
> +	    g762_one_prop_commit(client, pdata->fan_target,
> +				 "fan_target", do_set_fan_target)) {
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Helpers to import hardware characteristics from .dts file and overload
> + * default platform data values.
> + */
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id g762_dt_match[] = {
> +	{ .compatible = "gmt,g762" },
> +	{ .compatible = "gmt,g763" },
> +	{ },
> +};
> +
> +static inline void g762_of_import_one_prop(struct i2c_client *client,
> +					   u32 *dest, const char *pname)
> +{
> +	const __be32 *prop;
> +	int len;
> +
> +	prop = of_get_property(client->dev.of_node, pname, &len);
> +	if (prop && len == sizeof(u32)) {
> +		*dest = be32_to_cpu(prop[0]);
> +		dev_dbg(&client->dev, "found %s (%d)\n", pname, *dest);
> +	}
> +}
> +
> +static void g762_platform_data_of_overload(struct i2c_client *client,
> +					   struct g762_platform_data *pdata)
> +{
> +	if (!client->dev.of_node)
> +		return;
> +
> +	g762_of_import_one_prop(client, &pdata->fan_gear_mode, "fan_gear_mode");
> +	g762_of_import_one_prop(client, &pdata->pwm_polarity, "pwm_polarity");
> +	g762_of_import_one_prop(client, &pdata->fan_startv, "fan_startv");
> +	g762_of_import_one_prop(client, &pdata->pwm_freq, "pwm_freq");
> +	g762_of_import_one_prop(client, &pdata->fan_div, "fan_div");
> +	g762_of_import_one_prop(client, &pdata->fan_pulses, "fan_pulses");
> +	g762_of_import_one_prop(client, &pdata->pwm_mode, "pwm_mode");
> +	g762_of_import_one_prop(client, &pdata->fan_target, "fan_target");
> +	g762_of_import_one_prop(client, &pdata->pwm_enable, "pwm_enable");

I second the other comments ... why not use the sysfs interface to set the
values it supports ?

> +}
> +#else
> +static void g762_platform_data_of_overload(struct i2c_client *client,
> +				    struct g762_platform_data *pdata) { }
> +#endif
> +
> +/*
> + * helper to overload driver parameters from board init file for those
> + * not already converted to device tree
> + */
> +static void g762_platform_data_overload(struct i2c_client *client,
> +					struct g762_platform_data *pdata)
> +{
> +	struct g762_platform_data *opdata = client->dev.platform_data;
> +
> +	if (opdata)
> +		*pdata = *opdata;
> +}
> +
> +/*
> + * sysfs attributes
> + */
> +
> +/*
> + * Read function for fan1_input sysfs file. Return current fan RPM value, or
> + * 0 if fan is out of control. The function is available in both closed and
> + * open-loop mode.
> + */
> +static ssize_t get_fan_rpm(struct device *dev, struct device_attribute *da,
> +			   char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +	unsigned int rpm = 0;
> +
> +	mutex_lock(&data->update_lock);
> +	/* reverse logic: fan out of control reporting is enabled low */
> +	if (data->fan_sta & G762_REG_FAN_STA_OOC) {
> +		rpm = rpm_from_cnt(data->act_cnt, data->clk,
> +				   PULSE_FROM_REG(data->fan_cmd1),
> +				   CLKDIV_FROM_REG(data->fan_cmd1),
> +				   GEARMULT_FROM_REG(data->fan_cmd2));
> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return sprintf(buf, "%u\n", rpm);
> +}
> +
> +/*
> + * Read and write functions for pwm1_mode sysfs file. Get and set fan speed
> + * control mode i.e. PWM (1) or DC (0).
> + */
> +static ssize_t get_pwm_mode(struct device *dev, struct device_attribute *da,
> +			    char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +
> +	return sprintf(buf, "%d\n",
> +		       !!(data->fan_cmd1 & G762_REG_FAN_CMD1_OUT_MODE));
> +}
> +
> +static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *da,
> +			    const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 10, &val) || do_set_pwm_mode(dev, val))
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +/*
> + * Read and write functions for fan1_div sysfs file. Get and set fan
> + * controller prescaler value
> + */
> +static ssize_t get_fan_div(struct device *dev,
> +			       struct device_attribute *da, char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +
> +	return sprintf(buf, "%d\n", CLKDIV_FROM_REG(data->fan_cmd1));
> +}
> +
> +static ssize_t set_fan_div(struct device *dev,
> +			       struct device_attribute *da,
> +			       const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 10, &val) || do_set_fan_div(dev, val))
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +/*
> + * Read and write functions for pwm1_enable. Get and set the fan speed control
> + * mode (i.e. closed or open-loop).
> + *
> + * Following documentation about hwmon's sysfs interface, a pwm1_enable node
> + * should accept followings:
> + *
> + *  0 : no fan speed control (i.e. fan at full speed)
> + *  1 : manual fan speed control enabled (use pwm[1-*]) (open-loop)
> + *  2+: automatic fan speed control enabled (use fan[1-*]_target) (closed-loop)
> + *
> + * but we do not accept 0 as "no-control" mode is not supported by g762,
> + * -EINVAL is returned in this case.
> + */
> +static ssize_t get_pwm_enable(struct device *dev,
> +			      struct device_attribute *da, char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +
> +	return sprintf(buf, "%d\n",
> +		       (!!(data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE)) + 1);
> +}
> +
> +static ssize_t set_pwm_enable(struct device *dev,
> +			      struct device_attribute *da,
> +			      const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 10, &val) || do_set_pwm_enable(dev, val))
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +/*
> + * Read and write functions for pwm1 sysfs file. Get and set the fan speed
> + * in both open and closed-loop mode. Speed is given as a relative value
> + * between 0 and 255, where 255 is maximum speed. Note that due to rounding
> + * errors it is possible that you don't read back exactly the value you have
> + * set.
> + */
> +static ssize_t get_pwm(struct device *dev, struct device_attribute *da,
> +		       char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +	int val;
> +
> +	mutex_lock(&data->update_lock);
> +	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
> +		val = PWM_FROM_CNT(data->set_cnt);
> +	} else {                                           /* open-loop */
> +		val = data->set_out;
> +	}

Again, this is not very consistent, as set_cnt is pretty much reported twice
(as pwm and as target speed) in closed loop mode. Just report set_out here.

Side note: { } is not needed in single-statement conditionals.

> +	mutex_unlock(&data->update_lock);
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
> +		       const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 10, &val) || do_set_pwm(dev, val))
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +/*
> + * Read and write function for fan1_target sysfs file. Get/set the fan speed in
> + * closed-loop mode. Speed is given as a RPM value; then the chip will regulate
> + * the fan speed using pulses from fan tachometer. The functions both return
> + * -EINVAL when called in open-loop mode.
> + *
> + * Refer to rpm_from_cnt() implementation above to get info about count number
> + * calculation.
> + *
> + * Also note that due to rounding errors it is possible that you don't read
> + * back exactly the value you have set.
> + */
> +static ssize_t get_fan_target(struct device *dev, struct device_attribute *da,
> +			      char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +	unsigned int rpm;
> +	ssize_t ret = -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
> +		rpm = rpm_from_cnt(data->set_cnt, data->clk,
> +				   PULSE_FROM_REG(data->fan_cmd1),
> +				   CLKDIV_FROM_REG(data->fan_cmd1),
> +				   GEARMULT_FROM_REG(data->fan_cmd2));
> +		ret = sprintf(buf, "%u\n", rpm);
> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t set_fan_target(struct device *dev, struct device_attribute *da,
> +			      const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 10, &val) || do_set_fan_target(dev, val))
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +/* read function for fan1_fault sysfs file. */
> +static ssize_t get_fan_failure(struct device *dev, struct device_attribute *da,
> +			       char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +
> +	return sprintf(buf, "%u\n", !!(data->fan_sta & G762_REG_FAN_STA_FAIL));
> +}
> +
> +/*
> + * read function for fan1_alarm sysfs file. Note that OOC confition is
> + * enabled low
> + */
> +static ssize_t get_fan_ooc(struct device *dev, struct device_attribute *da,
> +			   char *buf)
> +{
> +	struct g762_data *data = g762_update_client(dev);
> +
> +	return sprintf(buf, "%u\n", !(data->fan_sta & G762_REG_FAN_STA_OOC));
> +}
> +
> +static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, get_pwm, set_pwm);
> +static DEVICE_ATTR(pwm1_mode, S_IWUSR | S_IRUGO, get_pwm_mode, set_pwm_mode);
> +static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
> +		   get_pwm_enable, set_pwm_enable);
> +static DEVICE_ATTR(fan1_input, S_IRUGO, get_fan_rpm, NULL);
> +static DEVICE_ATTR(fan1_alarm, S_IRUGO, get_fan_ooc, NULL);
> +static DEVICE_ATTR(fan1_fault, S_IRUGO, get_fan_failure, NULL);
> +static DEVICE_ATTR(fan1_target, S_IWUSR | S_IRUGO,
> +		   get_fan_target, set_fan_target);
> +static DEVICE_ATTR(fan1_div, S_IWUSR | S_IRUGO, get_fan_div, set_fan_div);
> +
> +/* Driver data */
> +static struct attribute *g762_attributes[] = {
> +	&dev_attr_fan1_input.attr,
> +	&dev_attr_fan1_alarm.attr,
> +	&dev_attr_fan1_fault.attr,
> +	&dev_attr_fan1_target.attr,
> +	&dev_attr_fan1_div.attr,
> +	&dev_attr_pwm1.attr,
> +	&dev_attr_pwm1_mode.attr,
> +	&dev_attr_pwm1_enable.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group g762_group = {
> +	.attrs = g762_attributes,
> +};
> +
> +static int g762_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct g762_data *data;
> +	struct g762_platform_data pdata;
> +	int err;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct g762_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	data->client = client;
> +	mutex_init(&data->update_lock);
> +
> +	/* Enable fan protection and fan fail detection by default */
> +	do_fan_ooc_detection_toggle(&client->dev, 1);
> +	do_fan_failure_detection_toggle(&client->dev, 1);
> +
> +	/*
> +	 * Set default configuration values before passing the structure
> +	 * first to overload routine used by boards non converted to DT and
> +	 * then to OF helper (to overload them using those provided by .dts
> +	 * file, if any). Final result is then commited.
> +	 */
> +	g762_platform_data_init(&pdata);
> +	g762_platform_data_overload(client, &pdata);
> +	g762_platform_data_of_overload(client, &pdata);
> +	err = g762_platform_data_commit(client, &pdata);
> +	if (err)
> +		return err;
> +
> +	/* Register sysfs hooks */
> +	err = sysfs_create_group(&client->dev.kobj, &g762_group);
> +	if (err)
> +		return err;
> +
> +	data->hwmon_dev = (struct device *)hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		err = PTR_ERR(data->hwmon_dev);
> +		goto err_out;
> +	}
> +
> +	return 0;
> +
> + err_out:
> +	sysfs_remove_group(&client->dev.kobj, &g762_group);
> +	return err;
> +}
> +
> +static int g762_remove(struct i2c_client *client)
> +{
> +	struct g762_data *data = i2c_get_clientdata(client);
> +
> +	hwmon_device_unregister(data->hwmon_dev);
> +	sysfs_remove_group(&client->dev.kobj, &g762_group);
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver g762_driver = {
> +	.driver = {
> +		.name = DRVNAME,
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(g762_dt_match),
> +	},
> +	.probe	  = g762_probe,
> +	.remove	  = g762_remove,
> +	.id_table = g762_id,
> +};
> +
> +module_i2c_driver(g762_driver);
> +
> +MODULE_AUTHOR("Arnaud EBALARD <arno@natisbad.org>");
> +MODULE_DESCRIPTION("GMT G762/G763 driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/g762.h b/include/linux/platform_data/g762.h
> new file mode 100644
> index 0000000..29e3934
> --- /dev/null
> +++ b/include/linux/platform_data/g762.h
> @@ -0,0 +1,54 @@
> +/*
> + * Platform data structure for g762 fan controller driver
> + *
> + * Copyright (C) 2013, Arnaud EBALARD <arno@natisbad.org>
> + *
> + * 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
> + */
> +#ifndef __LINUX_PLATFORM_DATA_G762_H__
> +#define __LINUX_PLATFORM_DATA_G762_H__
> +
> +#define G762_DEFAULT_NO_OVERLOAD 0xffffffff
> +
> +struct g762_platform_data {
> +	u32 fan_startv;
> +	u32 fan_gear_mode;
> +	u32 fan_div;
> +	u32 fan_pulses;
> +	u32 fan_target;
> +	u32 pwm_polarity;
> +	u32 pwm_enable;
> +	u32 pwm_freq;
> +	u32 pwm_mode;
> +};
> +
> +/* When filling a g762_platform_data structure to be passed during platform
> + * init to the driver, it needs to be initialized to the following default
> + * value before changing specific fields. This is needed to avoid a sparse
> + * initialization to have current values replaced by 0 */
> +
comment style

> +static const struct g762_platform_data G762_DEFAULT_PDATA = {
> +	.fan_startv = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_gear_mode = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_div = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_pulses = G762_DEFAULT_NO_OVERLOAD,
> +	.fan_target = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_polarity = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_enable = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_freq = G762_DEFAULT_NO_OVERLOAD,
> +	.pwm_mode = G762_DEFAULT_NO_OVERLOAD,
> +};
> +
A constant array in an include file ? Please not; this is completely
unacceptable. You can pass platform data to the driver like every other
driver does. The entire approach with G762_DEFAULT_NO_OVERLOAD is odd anyway;
it would be great if you could come up with another mechanism to pass platform
data. It is quite common to use '0' for 'default' to avoid having to initialize
all default fields.

Thanks,
Guenter

> +#endif /* __LINUX_PLATFORM_DATA_G762_H__ */
> -- 
> 1.7.10.4
> 
> 

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi Simon and Guenter,

Simon Guinot <simon.guinot@sequanux.org> writes:

> On Tue, May 28, 2013 at 12:03:14AM +0200, Arnaud Ebalard wrote:
>> 
>> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
>> ---
>>  drivers/hwmon/Kconfig              |   10 +
>>  drivers/hwmon/Makefile             |    1 +
>>  drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
>>  include/linux/platform_data/g762.h |   54 ++
>>  4 files changed, 1077 insertions(+)
>>  create mode 100644 drivers/hwmon/g762.c
>>  create mode 100644 include/linux/platform_data/g762.h
>
> Hi Arnaud,
>
> After more tests on my 2Big Network v2 board, it appears that the fan
> doesn't rotate when PWM mode (the preferred operating mode for this
> board) is selected. Nevertheless, DC mode is usable (even if not ideal
> given the hardware). After some investigations I noticed that an extra
> initialization is needed to enable PWM mode on my board: the set_cnt
> register must be set to 0 while the default value is 0xff. Is that
> specific to my hardware ? Is PWM mode working on your ReadyNAS with
> the default set_cnt value ?

First, thanks for testing this!

Regarding your problem, I first started by booting current version of my
driver on the Duo v2 w/o touching chip registers (only clock reference
value). This way, I inherit the values installed by (NETGEAR's) u-boot:

$ for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
fan1_alarm:0
fan1_div:2
fan1_fault:0
fan1_input:1807
fan1_target:1807
pwm1:221
pwm1_enable:2        /* closed-loop, i.e. config is done via set_cnt */
pwm1_mode:1          /* PWM mode */ 

If G762 datasheet is correct, NETGEAR's u-boot alters fan divisor ('01'
i.e. 2 instead of '00' i.e. 1). It also changes the default mode from
linear to pwm and sets set_cnt to 34 (255-221). 

Then, I grabbed the GPL sources (5.3.7) of NETGEAR's u-boot and found
the following in a boot function:

 /* Turn G76x(FAN controller, i2c address 0x3e) on.
  * The FAN_SET_CNT register's offset is 0x0.
  * Set [1300(rpm) == 65% == 5a(FAN_SET_CNT)] as default.
  */
 unsigned char byte=0x5a;
 if(i2c_write(0, 0x3e, 0x0, 1, &byte, 1) != 0) 
   puts ("Error writing the i2c chip : G76x(Fan controller).\n");

AFAICT, this is the only location in the whole source tarball where 0x3e
reg is modified in an i2c function. 


Then, I grabbed the u-boot source code from NETGEAR for ReadyNAS 102
(Armada-370 based NAS, also using a G762) and it has the following:

 /* Turn G76x(FAN controller, i2c address 0x3e) on.
  * The FAN_SET_CNT register's offset is 0x0.
  * Set [1300(rpm) == 65% == 5a(FAN_SET_CNT)] as default.
  */
 MV_U8 byte=0x5a;
 if(i2c_write(0x3e, 0x0, 1, &byte, 1) != 0)
 	puts ("Error writing the i2c chip : G76x(Fan controller).\n");
 /* Tune the FAN_STARTV */
 if (i2c_read(0x3e, 0x5, 1, &byte, 1) != 0)
 	puts ("Error reading the i2c chip : G76x(Fan controller).\n");
 byte |= 0x3;
 if(i2c_write(0x3e, 0x5, 1, &byte, 1) != 0)
 	puts ("Error writing the i2c chip : G76x(Fan controller).\n");
 
In the end, it does not seem NETGEAR's u-boot does some specific things
except setting a non-zero value to SET_CNT register for the fan to start
rotating.

BTW, if you wonder why some attributes reported at the beginning have
different values than the ones in the datasheet (0x5a set in u-boot,
reading 0x22) although NETGEAR's u-boot code does not seem to tamper
those, I don't have the answer. 


> I haven't noticed this issue while testing your v1 patch series because
> at the time I was using a board with an U-Boot modified by LaCie. This
> last sets the set_cnt register to 0 while U-Boot mainline don't.
> Actually, the 2Big fan is not configured nor even started by U-Boot
> mainline. I have to fix this but maybe that something can be done in
> the g762 Linux driver too ?

You mean like reading set_cnt, setting it to 0 and then setting it back
to previous value? I guess it would be good to confirm that the G762
needs that to start operating on all platforms. The datasheet does not
provide info and I cannot confirm/infirm that on ReadyNAS Duo v2 and
102 platforms.


>> +/* When filling a g762_platform_data structure to be passed during platform
>> + * init to the driver, it needs to be initialized to the following default
>> + * value before changing specific fields. This is needed to avoid a sparse
>> + * initialization to have current values replaced by 0 */
>
> In other places, you use this multi-line comment format:
>
> /*
>  * ...
>  * ...
>  */

I missed than one, thanks.


>> +static const struct g762_platform_data G762_DEFAULT_PDATA = {
>> +	.fan_startv = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_gear_mode = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_div = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_pulses = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_target = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_polarity = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_enable = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_freq = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_mode = G762_DEFAULT_NO_OVERLOAD,
>> +};
>
> Are you sure that all this settings are needed ? IMHO the g762 platform
> data should only holds the platform specific configuration: fan_startv,
> fan_gear_mode, fan_div and fan_pulses. The other settings are not really
> related with the platform but more with the operating fan configuration.
> For my part, I am happy enough with the sysfs hwmon interface.
>
> The same comment applies to the g762 DT binding properties.

Reading Guenter's mail, it seems he has the same opinion. He also agrees
with you that the array is not a good idea in a header.

Regarding the former, I will comply to what Guenter decides but here are
the reasons why I wanted to expose all those attributes. 

I think choosing PWM mode and closed-loop is also platform related. For
instance, on my ReadyNAS Duo v2, the fan is a 3 wire one with a
tachometer and it makes sense to set the chip in closed-loop
mode using previous knobs. Setting a minimal fan_target is also
useful to prevent someone booting a kernel to just have no cooling. One
could argue that the u-boot provided by the manufacturer has already
taken care of those but:

 - the user may install a stock u-boot
 - the manufacturer may have performed this config in the kernel and not
   using u-boot

Regarding the latter, I will try and come with an other approach,
probably using a function instead of an array if it is
acceptable. My initial idea was that the platform maintainer would be
able to to do sth like:  

  struct g762_platform_data pdata = G762_DEFAULT_PDATA;
  pdata.pwm_freq = 8192;

and then pass the structure to the i2c init routine if he only wants to
give a reference clock frequency. Other attributes would not be modified.

Additionally, the problem with using 0 as a "use default value" is that
0 is used for linear mode (sysfs interface for pwm_mode), 0 is also a
valid value for fan_startv. Same for pwm_polarity and fan_gear_mode.
Hence my decision to use something else.  


Guenter, I will work on a v3 this evening based on your comments.

Cheers,

a+

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi Guenter,

apologies for the looong email ...

Guenter Roeck <linux@roeck-us.net> writes:

>> +	return (clk*30*gear_mult)/((cnt ? cnt : 1)*p*clk_div);
>
> Please follow CodingStyle and add spaces between operators (not just
> here).

done.


>> +static struct g762_data *g762_update_client(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct g762_data *data = i2c_get_clientdata(client);
>> +
>> +	mutex_lock(&data->update_lock);
>> +	if (time_before(jiffies, data->last_updated + G762_UPDATE_INTERVAL) &&
>> +	    likely(data->valid))
>> +		goto out;
>> +
>> +	data->set_cnt =  i2c_smbus_read_byte_data(client, G762_REG_SET_CNT);
>> +	data->act_cnt =  i2c_smbus_read_byte_data(client, G762_REG_ACT_CNT);
>> +	data->fan_sta =  i2c_smbus_read_byte_data(client, G762_REG_FAN_STA);
>> +	data->set_out =  i2c_smbus_read_byte_data(client, G762_REG_SET_OUT);
>> +	data->fan_cmd1 = i2c_smbus_read_byte_data(client, G762_REG_FAN_CMD1);
>> +	data->fan_cmd2 = i2c_smbus_read_byte_data(client, G762_REG_FAN_CMD2);
>> +
> Failed i2c reads (returning negative values) not a problem ? Driver
> operation will be more or less random in that case, and may result in
> erratic behavior.

Now checking return value of both i2c_smbus_{read,write}_byte_data(). I
also created wrappers for both functions to report some info upon error.


>> +/* Set fan RPM value. This only makes sense in closed-loop mode. */
>> +static int do_set_fan_target(struct device *dev, unsigned long val)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct g762_data *data = g762_update_client(dev);
>> +	int ret = -EINVAL;
>> +
>> +	mutex_lock(&data->update_lock);
>> +	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
>> +		data->set_cnt = cnt_from_rpm(val, data->clk,
>> +					     PULSE_FROM_REG(data->fan_cmd1),
>> +					     CLKDIV_FROM_REG(data->fan_cmd1),
>> +					     GEARMULT_FROM_REG(data->fan_cmd2));
>> +		ret = i2c_smbus_write_byte_data(client, G762_REG_SET_CNT,
>> +						data->set_cnt);
>
> In closed loop mode you have two paths to set the same register. Given that,
> it doesn't make sense to me to set the fan target with set_pwm as
> well.

It does not seems logical to me to remove access to pwm1 in closed-loop
mode, i.e. prevent the user to have access to the 0-255 interface now
that she can pass RPM values.

I am not an expert but utilities like fancontrol/pwmconfig work with
pwm1 sysfs entry, not fan1_target. AFAICT from sysfs doc, this simple
and agnostic interface (0 to 255 value) should work for all
combinations: DC vs PWM, closed vs open-loop. If I make pwm1 unavailable
in closed-loop mode to keep only fan1_target as a unique path to control
the speed, then this goes against least expectation principle and
requires hacks from user or .dts writer: one of those will have to force
open-loop to change the value. On ReadyNAS Duo v2 and 102, if I do not
provide a .dts file which changes the default value of the chip (not 
modified by u-boot), pwmconfig/fancontrol will not work by default.

As I do not have your background on the topic, I'll let you decide but
here is what seems logical to me (considering ReadyNAS hardware and
default values, how the chip works and the experience I had w/
fancontrol and pwmconfig):   

 - let pwm1 available in all cases: DC/PWM and closed/open-loop. This
   makes fancontrol usable in all cases w/o any user interaction.

 - provide fan_target in closed-loop mode only and do not accept values
   before the mode is indeed closed-loop. I think we can expect the
   user who uses this interface to have read some documentation. For
   instance, you need to know which value makes sense on your platform,
   which is not that obvious.

Guenter, if you still think it is a good idea to make pwm1 unavailable
in closed-loop mode, I will make the change and stop arguing.


> If you use one attribute (set_pwm) for setting set_out, and the other for
> set_cnt, your problems with conditional command acceptance should be solved.

I do not have problems with current logic: in practice, for a given
board, mode will be set (by u-boot or driver) before the speed related
registers are modified. I even take care in pdata attributes import to
set fan_target after everything else.


>> +static void g762_platform_data_of_overload(struct i2c_client *client,
>> +					   struct g762_platform_data *pdata)
>> +{
>> +	if (!client->dev.of_node)
>> +		return;
>> +
>> +	g762_of_import_one_prop(client, &pdata->fan_gear_mode, "fan_gear_mode");
>> +	g762_of_import_one_prop(client, &pdata->pwm_polarity, "pwm_polarity");
>> +	g762_of_import_one_prop(client, &pdata->fan_startv, "fan_startv");
>> +	g762_of_import_one_prop(client, &pdata->pwm_freq, "pwm_freq");
>> +	g762_of_import_one_prop(client, &pdata->fan_div, "fan_div");
>> +	g762_of_import_one_prop(client, &pdata->fan_pulses, "fan_pulses");
>> +	g762_of_import_one_prop(client, &pdata->pwm_mode, "pwm_mode");
>> +	g762_of_import_one_prop(client, &pdata->fan_target, "fan_target");
>> +	g762_of_import_one_prop(client, &pdata->pwm_enable, "pwm_enable");
>
> I second the other comments ... why not use the sysfs interface to set the
> values it supports ?

answer in previous email to Simon.


>> +/*
>> + * Read and write functions for pwm1 sysfs file. Get and set the fan speed
>> + * in both open and closed-loop mode. Speed is given as a relative value
>> + * between 0 and 255, where 255 is maximum speed. Note that due to rounding
>> + * errors it is possible that you don't read back exactly the value you have
>> + * set.
>> + */
>> +static ssize_t get_pwm(struct device *dev, struct device_attribute *da,
>> +		       char *buf)
>> +{
>> +	struct g762_data *data = g762_update_client(dev);
>> +	int val;
>> +
>> +	mutex_lock(&data->update_lock);
>> +	if (data->fan_cmd1 & G762_REG_FAN_CMD1_FAN_MODE) { /* closed-loop */
>> +		val = PWM_FROM_CNT(data->set_cnt);
>> +	} else {                                           /* open-loop */
>> +		val = data->set_out;
>> +	}
>
> Again, this is not very consistent, as set_cnt is pretty much reported twice
> (as pwm and as target speed) in closed loop mode. Just report set_out here.

will do if you decide I should kill pwm1 in closed-loop mode. 


> Side note: { } is not needed in single-statement conditionals.

Will be fixed in v3.


>> +/* When filling a g762_platform_data structure to be passed during platform
>> + * init to the driver, it needs to be initialized to the following default
>> + * value before changing specific fields. This is needed to avoid a sparse
>> + * initialization to have current values replaced by 0 */
>> +
> comment style

Will be fixed in v3.


>> +static const struct g762_platform_data G762_DEFAULT_PDATA = {
>> +	.fan_startv = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_gear_mode = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_div = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_pulses = G762_DEFAULT_NO_OVERLOAD,
>> +	.fan_target = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_polarity = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_enable = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_freq = G762_DEFAULT_NO_OVERLOAD,
>> +	.pwm_mode = G762_DEFAULT_NO_OVERLOAD,
>> +};
>> +
> A constant array in an include file ? Please not; this is completely
> unacceptable. You can pass platform data to the driver like every other
> driver does. The entire approach with G762_DEFAULT_NO_OVERLOAD is odd anyway;
> it would be great if you could come up with another mechanism to pass platform
> data. It is quite common to use '0' for 'default' to avoid having to initialize
> all default fields.

My life would be easier if I could use 0 and just let the developer pass
a sparse g762_platform_data, initialized only with the value she has
interest in, but as discussed in previous email to Simon, 0 is a valid
value for some attributes (fan_startv, fan_gear_mode, pwm_polarity,
pwm_mode at least) which prevents using it as a 'default' value. If a
good pattern exists for that problem I would happy to learn it.

In a first version of v3, I first replaced G762_DEFAULT_PDATA by the
declaration of a function which sets all the fields of the structure one
bye one to G762_DEFAULT_NO_OVERLOAD. The header looked better but I don't
think the whole approach did.

So I changed my mind and went for the following: a wrapper for passing
attribute values, which sets MSB of passed value to 1. This allows to
spot initialized values in a sparse init. I put some info and an example
at the beginning of the header.

I am currently rereading v3 and will post it soon.

Cheers,

a+

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi,

>> I haven't noticed this issue while testing your v1 patch series because
>> at the time I was using a board with an U-Boot modified by LaCie. This
>> last sets the set_cnt register to 0 while U-Boot mainline don't.
>> Actually, the 2Big fan is not configured nor even started by U-Boot
>> mainline. I have to fix this but maybe that something can be done in
>> the g762 Linux driver too ?

At the moment, DT bindings and platform_data structure both allow
setting an initial fan_target value. I could add a 'pwm' binding for DT  
and an assoicated field in platform_data to allow passing an initial
value so that maintainers of platforms with a 2 wire fan (open loop) can
still make the fan somewhat rotate at boot.

This is related but I guess this would not completely solve your issue
though. 

Cheers,

a+

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Guenter Roeck]

On Sun, Jun 02, 2013 at 05:45:58PM +0200, Arnaud Ebalard wrote:
> Hi,
> 
> >> I haven't noticed this issue while testing your v1 patch series because
> >> at the time I was using a board with an U-Boot modified by LaCie. This
> >> last sets the set_cnt register to 0 while U-Boot mainline don't.
> >> Actually, the 2Big fan is not configured nor even started by U-Boot
> >> mainline. I have to fix this but maybe that something can be done in
> >> the g762 Linux driver too ?
> 
> At the moment, DT bindings and platform_data structure both allow
> setting an initial fan_target value. I could add a 'pwm' binding for DT  
> and an assoicated field in platform_data to allow passing an initial
> value so that maintainers of platforms with a 2 wire fan (open loop) can
> still make the fan somewhat rotate at boot.
> 
Devicetree properties are supposed to describe the system, not its configuration.
There are exceptions, but I don't recall seeing a case where a parameter
can be set both with devicetree properties and sysfs attributes.

Before going further on this, we will need to have an ack from devicetree
maintainers which specifically permits your dual-configuration approach.
Even with that, you'll still have convince me that you need both (while
so far no other driver needed it).

Thanks,
Guenter

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi Guenter,

Guenter Roeck <linux@roeck-us.net> writes:

> On Sun, Jun 02, 2013 at 05:45:58PM +0200, Arnaud Ebalard wrote:
>> Hi,
>> 
>> >> I haven't noticed this issue while testing your v1 patch series because
>> >> at the time I was using a board with an U-Boot modified by LaCie. This
>> >> last sets the set_cnt register to 0 while U-Boot mainline don't.
>> >> Actually, the 2Big fan is not configured nor even started by U-Boot
>> >> mainline. I have to fix this but maybe that something can be done in
>> >> the g762 Linux driver too ?
>> 
>> At the moment, DT bindings and platform_data structure both allow
>> setting an initial fan_target value. I could add a 'pwm' binding for DT  
>> and an assoicated field in platform_data to allow passing an initial
>> value so that maintainers of platforms with a 2 wire fan (open loop) can
>> still make the fan somewhat rotate at boot.
>> 
> Devicetree properties are supposed to describe the system, not its configuration.
> There are exceptions, but I don't recall seeing a case where a parameter
> can be set both with devicetree properties and sysfs attributes.
>
> Before going further on this, we will need to have an ack from devicetree
> maintainers which specifically permits your dual-configuration approach.
> Even with that, you'll still have convince me that you need both (while
> so far no other driver needed it).

If you are not convinced yet, what I can do is remove the code in question
in a v4 and work on remaining issues (e.g. platform data init, fan_target
and pwm1 access for closed-loop) based on your feedback. If during the
process or after inclusion of this minimal version someone brings new
arguments to add the code back, this should be fairly easy to do.

Thanks for your time and patience dealing with me.

Cheers,

a+

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 9341 bytes --]

On Sat, Jun 01, 2013 at 07:26:54PM +0200, Arnaud Ebalard wrote:
> Hi Simon and Guenter,
> 
> Simon Guinot <simon.guinot@sequanux.org> writes:
> 
> > On Tue, May 28, 2013 at 12:03:14AM +0200, Arnaud Ebalard wrote:
> >> 
> >> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
> >> ---
> >>  drivers/hwmon/Kconfig              |   10 +
> >>  drivers/hwmon/Makefile             |    1 +
> >>  drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
> >>  include/linux/platform_data/g762.h |   54 ++
> >>  4 files changed, 1077 insertions(+)
> >>  create mode 100644 drivers/hwmon/g762.c
> >>  create mode 100644 include/linux/platform_data/g762.h
> >
> > Hi Arnaud,
> >
> > After more tests on my 2Big Network v2 board, it appears that the fan
> > doesn't rotate when PWM mode (the preferred operating mode for this
> > board) is selected. Nevertheless, DC mode is usable (even if not ideal
> > given the hardware). After some investigations I noticed that an extra
> > initialization is needed to enable PWM mode on my board: the set_cnt
> > register must be set to 0 while the default value is 0xff. Is that
> > specific to my hardware ? Is PWM mode working on your ReadyNAS with
> > the default set_cnt value ?
> 
> First, thanks for testing this!
> 
> Regarding your problem, I first started by booting current version of my
> driver on the Duo v2 w/o touching chip registers (only clock reference
> value). This way, I inherit the values installed by (NETGEAR's) u-boot:
> 
> $ for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
> fan1_alarm:0
> fan1_div:2
> fan1_fault:0
> fan1_input:1807
> fan1_target:1807
> pwm1:221
> pwm1_enable:2        /* closed-loop, i.e. config is done via set_cnt */
> pwm1_mode:1          /* PWM mode */ 

Sorry, I realize that I have not been very accurate in my description of
the problem: The fan doesn't rotate when PWM _and_ open-loop control are
selected. On the 2Big2 board, 2 wires are used to drive the fan.

Then with pwm1_mode=1, pwm1_enable=1 and set_cnt=0xff (default value),
nothing happen whatever the value I write in pwm1.

By testing, I have discovered that writing 0 into set_cnt allows to work
around the issue. I can do this by using the closed-loop control:
pwm1_mode=1, pwm1_enable=2 and pwm=255. Now, set_cnt worths 0 and if I
switch back to open-loop control, all works as expected.

Can you try open-loop control and PWM mode with your board ? I wonder if
this issue is specific to the 2Big2 hardware.

> 
> If G762 datasheet is correct, NETGEAR's u-boot alters fan divisor ('01'
> i.e. 2 instead of '00' i.e. 1). It also changes the default mode from
> linear to pwm and sets set_cnt to 34 (255-221). 
> 
> Then, I grabbed the GPL sources (5.3.7) of NETGEAR's u-boot and found
> the following in a boot function:
> 
>  /* Turn G76x(FAN controller, i2c address 0x3e) on.
>   * The FAN_SET_CNT register's offset is 0x0.
>   * Set [1300(rpm) == 65% == 5a(FAN_SET_CNT)] as default.
>   */
>  unsigned char byte=0x5a;
>  if(i2c_write(0, 0x3e, 0x0, 1, &byte, 1) != 0) 
>    puts ("Error writing the i2c chip : G76x(Fan controller).\n");
> 
> AFAICT, this is the only location in the whole source tarball where 0x3e
> reg is modified in an i2c function. 
> 
> 
> Then, I grabbed the u-boot source code from NETGEAR for ReadyNAS 102
> (Armada-370 based NAS, also using a G762) and it has the following:
> 
>  /* Turn G76x(FAN controller, i2c address 0x3e) on.
>   * The FAN_SET_CNT register's offset is 0x0.
>   * Set [1300(rpm) == 65% == 5a(FAN_SET_CNT)] as default.
>   */
>  MV_U8 byte=0x5a;
>  if(i2c_write(0x3e, 0x0, 1, &byte, 1) != 0)
>  	puts ("Error writing the i2c chip : G76x(Fan controller).\n");
>  /* Tune the FAN_STARTV */
>  if (i2c_read(0x3e, 0x5, 1, &byte, 1) != 0)
>  	puts ("Error reading the i2c chip : G76x(Fan controller).\n");
>  byte |= 0x3;
>  if(i2c_write(0x3e, 0x5, 1, &byte, 1) != 0)
>  	puts ("Error writing the i2c chip : G76x(Fan controller).\n");
>  
> In the end, it does not seem NETGEAR's u-boot does some specific things
> except setting a non-zero value to SET_CNT register for the fan to start
> rotating.

In closed-loop mode, this is perfectly understandable :)

> 
> BTW, if you wonder why some attributes reported at the beginning have
> different values than the ones in the datasheet (0x5a set in u-boot,
> reading 0x22) although NETGEAR's u-boot code does not seem to tamper
> those, I don't have the answer. 
> 
> 
> > I haven't noticed this issue while testing your v1 patch series because
> > at the time I was using a board with an U-Boot modified by LaCie. This
> > last sets the set_cnt register to 0 while U-Boot mainline don't.
> > Actually, the 2Big fan is not configured nor even started by U-Boot
> > mainline. I have to fix this but maybe that something can be done in
> > the g762 Linux driver too ?
> 
> You mean like reading set_cnt, setting it to 0 and then setting it back
> to previous value? I guess it would be good to confirm that the G762
> needs that to start operating on all platforms. The datasheet does not
> provide info and I cannot confirm/infirm that on ReadyNAS Duo v2 and
> 102 platforms.

No, I simply mean setting set_cnt to 0 when open-loop mode is selected.

> 
> 
> >> +/* When filling a g762_platform_data structure to be passed during platform
> >> + * init to the driver, it needs to be initialized to the following default
> >> + * value before changing specific fields. This is needed to avoid a sparse
> >> + * initialization to have current values replaced by 0 */
> >
> > In other places, you use this multi-line comment format:
> >
> > /*
> >  * ...
> >  * ...
> >  */
> 
> I missed than one, thanks.
> 
> 
> >> +static const struct g762_platform_data G762_DEFAULT_PDATA = {
> >> +	.fan_startv = G762_DEFAULT_NO_OVERLOAD,
> >> +	.fan_gear_mode = G762_DEFAULT_NO_OVERLOAD,
> >> +	.fan_div = G762_DEFAULT_NO_OVERLOAD,
> >> +	.fan_pulses = G762_DEFAULT_NO_OVERLOAD,
> >> +	.fan_target = G762_DEFAULT_NO_OVERLOAD,
> >> +	.pwm_polarity = G762_DEFAULT_NO_OVERLOAD,
> >> +	.pwm_enable = G762_DEFAULT_NO_OVERLOAD,
> >> +	.pwm_freq = G762_DEFAULT_NO_OVERLOAD,
> >> +	.pwm_mode = G762_DEFAULT_NO_OVERLOAD,
> >> +};
> >
> > Are you sure that all this settings are needed ? IMHO the g762 platform
> > data should only holds the platform specific configuration: fan_startv,
> > fan_gear_mode, fan_div and fan_pulses. The other settings are not really
> > related with the platform but more with the operating fan configuration.
> > For my part, I am happy enough with the sysfs hwmon interface.
> >
> > The same comment applies to the g762 DT binding properties.
> 
> Reading Guenter's mail, it seems he has the same opinion. He also agrees
> with you that the array is not a good idea in a header.
> 
> Regarding the former, I will comply to what Guenter decides but here are
> the reasons why I wanted to expose all those attributes. 
> 
> I think choosing PWM mode and closed-loop is also platform related. For
> instance, on my ReadyNAS Duo v2, the fan is a 3 wire one with a
> tachometer and it makes sense to set the chip in closed-loop
> mode using previous knobs. Setting a minimal fan_target is also
> useful to prevent someone booting a kernel to just have no cooling. One
> could argue that the u-boot provided by the manufacturer has already
> taken care of those but:
> 
>  - the user may install a stock u-boot

I assume you mean mainline u-boot here. Mainline u-boot should also
take care of the initial fan configuration.

>  - the manufacturer may have performed this config in the kernel and not
>    using u-boot

A userland program is needed anyway to control the fan speed. Even if
the bootloader missed that (and it should not), then the device will
have cooling soon enough.

Now if we really need to configure the initial values for some hwmon
global attributes (pwm1_*, fan1_*), then a end-driver is probably not
the right place to do that. Several hwmon drivers may also want to have
this DT attributes. Then, to avoid code duplication, it may be better to
handle them from the hwmon core code.

Regards,

Simon

> 
> Regarding the latter, I will try and come with an other approach,
> probably using a function instead of an array if it is
> acceptable. My initial idea was that the platform maintainer would be
> able to to do sth like:  
> 
>   struct g762_platform_data pdata = G762_DEFAULT_PDATA;
>   pdata.pwm_freq = 8192;
> 
> and then pass the structure to the i2c init routine if he only wants to
> give a reference clock frequency. Other attributes would not be modified.
> 
> Additionally, the problem with using 0 as a "use default value" is that
> 0 is used for linear mode (sysfs interface for pwm_mode), 0 is also a
> valid value for fan_startv. Same for pwm_polarity and fan_gear_mode.
> Hence my decision to use something else.  
> 
> 
> Guenter, I will work on a v3 this evening based on your comments.
> 
> Cheers,
> 
> a+
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi Simon,

arno@natisbad.org (Arnaud Ebalard) writes:

> Regarding your problem, I first started by booting current version of my
> driver on the Duo v2 w/o touching chip registers (only clock reference
> value). This way, I inherit the values installed by (NETGEAR's) u-boot:
>
> $ for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
> fan1_alarm:0
> fan1_div:2
> fan1_fault:0
> fan1_input:1807
> fan1_target:1807
> pwm1:221
> pwm1_enable:2        /* closed-loop, i.e. config is done via set_cnt */
> pwm1_mode:1          /* PWM mode */ 
>
> If G762 datasheet is correct, NETGEAR's u-boot alters fan divisor ('01'
> i.e. 2 instead of '00' i.e. 1). It also changes the default mode from
> linear to pwm and sets set_cnt to 34 (255-221). 
>
> Then, I grabbed the GPL sources (5.3.7) of NETGEAR's u-boot and found
> the following in a boot function:
>
>  /* Turn G76x(FAN controller, i2c address 0x3e) on.
>   * The FAN_SET_CNT register's offset is 0x0.
>   * Set [1300(rpm) == 65% == 5a(FAN_SET_CNT)] as default.
>   */
>  unsigned char byte=0x5a;
>  if(i2c_write(0, 0x3e, 0x0, 1, &byte, 1) != 0) 
>    puts ("Error writing the i2c chip : G76x(Fan controller).\n");
>
> AFAICT, this is the only location in the whole source tarball where 0x3e
> reg is modified in an i2c function. 
>
>
> Then, I grabbed the u-boot source code from NETGEAR for ReadyNAS 102
> (Armada-370 based NAS, also using a G762) and it has the following:
>
>  /* Turn G76x(FAN controller, i2c address 0x3e) on.
>   * The FAN_SET_CNT register's offset is 0x0.
>   * Set [1300(rpm) == 65% == 5a(FAN_SET_CNT)] as default.
>   */
>  MV_U8 byte=0x5a;
>  if(i2c_write(0x3e, 0x0, 1, &byte, 1) != 0)
>  	puts ("Error writing the i2c chip : G76x(Fan controller).\n");
>  /* Tune the FAN_STARTV */
>  if (i2c_read(0x3e, 0x5, 1, &byte, 1) != 0)
>  	puts ("Error reading the i2c chip : G76x(Fan controller).\n");
>  byte |= 0x3;
>  if(i2c_write(0x3e, 0x5, 1, &byte, 1) != 0)
>  	puts ("Error writing the i2c chip : G76x(Fan controller).\n");
>  
> In the end, it does not seem NETGEAR's u-boot does some specific things
> except setting a non-zero value to SET_CNT register for the fan to start
> rotating.
>
> BTW, if you wonder why some attributes reported at the beginning have
> different values than the ones in the datasheet (0x5a set in u-boot,
> reading 0x22) although NETGEAR's u-boot code does not seem to tamper
> those, I don't have the answer. 

I was not happy with previous result and did some additional tests.
Between each tests, I did a full power off (halt and removal of power
cable) of the device instead of a simple reboot. With a power off, I
get the following:

fan1_alarm:0
fan1_div:1
fan1_fault:0
fan1_input:1365
fan1_target:1365
pwm1:0
pwm1_enable:2
pwm1_mode:0

Note that fan1_target reflects the fact that set_cnt is 165 (i.e. 255 -
0x5a), which is exactly what NETGEAR's u-boot sets for this reg.

Now, if I do a simple restart, there is some hysteresis which explains
the weird difference in my first test.

Another message follows w/ additional answers to your problem.

Cheers,

a+

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi Simon,

Simon Guinot <simon.guinot@sequanux.org> writes:

> On Sat, Jun 01, 2013 at 07:26:54PM +0200, Arnaud Ebalard wrote:
>> Hi Simon and Guenter,
>> 
>> Simon Guinot <simon.guinot@sequanux.org> writes:
>> 
>> > On Tue, May 28, 2013 at 12:03:14AM +0200, Arnaud Ebalard wrote:
>> >> 
>> >> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
>> >> ---
>> >>  drivers/hwmon/Kconfig              |   10 +
>> >>  drivers/hwmon/Makefile             |    1 +
>> >>  drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
>> >>  include/linux/platform_data/g762.h |   54 ++
>> >>  4 files changed, 1077 insertions(+)
>> >>  create mode 100644 drivers/hwmon/g762.c
>> >>  create mode 100644 include/linux/platform_data/g762.h
>> >
>> > Hi Arnaud,
>> >
>> > After more tests on my 2Big Network v2 board, it appears that the fan
>> > doesn't rotate when PWM mode (the preferred operating mode for this
>> > board) is selected. Nevertheless, DC mode is usable (even if not ideal
>> > given the hardware). After some investigations I noticed that an extra
>> > initialization is needed to enable PWM mode on my board: the set_cnt
>> > register must be set to 0 while the default value is 0xff. Is that
>> > specific to my hardware ? Is PWM mode working on your ReadyNAS with
>> > the default set_cnt value ?
>> 
>> First, thanks for testing this!
>> 
>> Regarding your problem, I first started by booting current version of my
>> driver on the Duo v2 w/o touching chip registers (only clock reference
>> value). This way, I inherit the values installed by (NETGEAR's) u-boot:
>> 
>> $ for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
>> fan1_alarm:0
>> fan1_div:2
>> fan1_fault:0
>> fan1_input:1807
>> fan1_target:1807
>> pwm1:221
>> pwm1_enable:2        /* closed-loop, i.e. config is done via set_cnt */
>> pwm1_mode:1          /* PWM mode */ 
>
> Sorry, I realize that I have not been very accurate in my description of
> the problem: The fan doesn't rotate when PWM _and_ open-loop control are
> selected. On the 2Big2 board, 2 wires are used to drive the fan.
>
> Then with pwm1_mode=1, pwm1_enable=1 and set_cnt=0xff (default value),
> nothing happen whatever the value I write in pwm1.

After boot, with set_cnt and set_out untouched by me (set_cnt set to
0x5a by u-boot, i.e. 255-0x5a read on pwm1):

fan1_alarm:0
fan1_div:1
fan1_fault:0
fan1_input:1365
fan1_target:1365
pwm1:0                  /* set_out is 0 (considering fan1_target,
                           set_cnt is 0x5a)*/
pwm1_enable:2           /* closed-loop */
pwm1_mode:0             /* DC mode */

# echo 1 > pwm1_mode    /* PWM mode */       # fan rotates 
# echo 1 > pwm1_enable  /* open-loop */      # fan stops rotating

At that point we have:

fan1_alarm:0
fan1_div:1
fan1_fault:0
fan1_input:0
fan1_target:1365
pwm1:0                  /* set_out is 0, set_cnt is still 0x5a */
pwm1_enable:1
pwm1_mode:1

Then:

# echo 100 > pwm1                            # fan rotates


> By testing, I have discovered that writing 0 into set_cnt allows to work
> around the issue. I can do this by using the closed-loop control:
> pwm1_mode=1, pwm1_enable=2 and pwm=255. Now, set_cnt worths 0 and if I
> switch back to open-loop control, all works as expected.
>
> Can you try open-loop control and PWM mode with your board ? I wonder if
> this issue is specific to the 2Big2 hardware.

AFAICT, set_cnt has never been set to 0 in my test and PWM+open-loop
works as expected, i.e. just by setting a non-zero value in set_out.
Tell me if I missed something or if you want me to perform another
test.

Cheers,

a+

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 4832 bytes --]

On Tue, Jun 04, 2013 at 08:52:12AM +0200, Arnaud Ebalard wrote:
> Hi Simon,
> 
> Simon Guinot <simon.guinot@sequanux.org> writes:
> 
> > On Sat, Jun 01, 2013 at 07:26:54PM +0200, Arnaud Ebalard wrote:
> >> Hi Simon and Guenter,
> >> 
> >> Simon Guinot <simon.guinot@sequanux.org> writes:
> >> 
> >> > On Tue, May 28, 2013 at 12:03:14AM +0200, Arnaud Ebalard wrote:
> >> >> 
> >> >> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
> >> >> ---
> >> >>  drivers/hwmon/Kconfig              |   10 +
> >> >>  drivers/hwmon/Makefile             |    1 +
> >> >>  drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
> >> >>  include/linux/platform_data/g762.h |   54 ++
> >> >>  4 files changed, 1077 insertions(+)
> >> >>  create mode 100644 drivers/hwmon/g762.c
> >> >>  create mode 100644 include/linux/platform_data/g762.h
> >> >
> >> > Hi Arnaud,
> >> >
> >> > After more tests on my 2Big Network v2 board, it appears that the fan
> >> > doesn't rotate when PWM mode (the preferred operating mode for this
> >> > board) is selected. Nevertheless, DC mode is usable (even if not ideal
> >> > given the hardware). After some investigations I noticed that an extra
> >> > initialization is needed to enable PWM mode on my board: the set_cnt
> >> > register must be set to 0 while the default value is 0xff. Is that
> >> > specific to my hardware ? Is PWM mode working on your ReadyNAS with
> >> > the default set_cnt value ?
> >> 
> >> First, thanks for testing this!
> >> 
> >> Regarding your problem, I first started by booting current version of my
> >> driver on the Duo v2 w/o touching chip registers (only clock reference
> >> value). This way, I inherit the values installed by (NETGEAR's) u-boot:
> >> 
> >> $ for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
> >> fan1_alarm:0
> >> fan1_div:2
> >> fan1_fault:0
> >> fan1_input:1807
> >> fan1_target:1807
> >> pwm1:221
> >> pwm1_enable:2        /* closed-loop, i.e. config is done via set_cnt */
> >> pwm1_mode:1          /* PWM mode */ 
> >
> > Sorry, I realize that I have not been very accurate in my description of
> > the problem: The fan doesn't rotate when PWM _and_ open-loop control are
> > selected. On the 2Big2 board, 2 wires are used to drive the fan.
> >
> > Then with pwm1_mode=1, pwm1_enable=1 and set_cnt=0xff (default value),
> > nothing happen whatever the value I write in pwm1.
> 
> After boot, with set_cnt and set_out untouched by me (set_cnt set to
> 0x5a by u-boot, i.e. 255-0x5a read on pwm1):
> 
> fan1_alarm:0
> fan1_div:1
> fan1_fault:0
> fan1_input:1365
> fan1_target:1365
> pwm1:0                  /* set_out is 0 (considering fan1_target,
>                            set_cnt is 0x5a)*/
> pwm1_enable:2           /* closed-loop */
> pwm1_mode:0             /* DC mode */
> 
> # echo 1 > pwm1_mode    /* PWM mode */       # fan rotates 
> # echo 1 > pwm1_enable  /* open-loop */      # fan stops rotating
> 
> At that point we have:
> 
> fan1_alarm:0
> fan1_div:1
> fan1_fault:0
> fan1_input:0
> fan1_target:1365
> pwm1:0                  /* set_out is 0, set_cnt is still 0x5a */
> pwm1_enable:1
> pwm1_mode:1
> 
> Then:
> 
> # echo 100 > pwm1                            # fan rotates

OK you are lucky. Your bootloader initialize set_cnt with 0x5a. Mine
don't and it is precisely the issue: set_cnt is still 0xff (the default
value) when the driver controls the fan.

Please, could you try open-loop mode with set_cnt set to 0xff ?  Maybe
you can enforce this value for the test purpose ?

If you observe the same behaviour than me, then could modify the driver
to ensure that set_cnt is not 0xff when open-loop mode is selected ?
Maybe by systematically setting a different value (as 0) ?

> 
> 
> > By testing, I have discovered that writing 0 into set_cnt allows to work
> > around the issue. I can do this by using the closed-loop control:
> > pwm1_mode=1, pwm1_enable=2 and pwm=255. Now, set_cnt worths 0 and if I
> > switch back to open-loop control, all works as expected.
> >
> > Can you try open-loop control and PWM mode with your board ? I wonder if
> > this issue is specific to the 2Big2 hardware.
> 
> AFAICT, set_cnt has never been set to 0 in my test and PWM+open-loop
> works as expected, i.e. just by setting a non-zero value in set_out.
> Tell me if I missed something or if you want me to perform another
> test.

Actually, I have only problems when set_cnt worths 0xff. Please, could
you try the tests mentioned above ?

Thanks in advance,

Simon

> 
> Cheers,
> 
> a+
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Guenter Roeck]

On Tue, Jun 04, 2013 at 11:23:07PM +0200, Simon Guinot wrote:
> On Tue, Jun 04, 2013 at 08:52:12AM +0200, Arnaud Ebalard wrote:
> > Hi Simon,
> > 
> > Simon Guinot <simon.guinot@sequanux.org> writes:
> > 
> > > On Sat, Jun 01, 2013 at 07:26:54PM +0200, Arnaud Ebalard wrote:
> > >> Hi Simon and Guenter,
> > >> 
> > >> Simon Guinot <simon.guinot@sequanux.org> writes:
> > >> 
> > >> > On Tue, May 28, 2013 at 12:03:14AM +0200, Arnaud Ebalard wrote:
> > >> >> 
> > >> >> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
> > >> >> ---
> > >> >>  drivers/hwmon/Kconfig              |   10 +
> > >> >>  drivers/hwmon/Makefile             |    1 +
> > >> >>  drivers/hwmon/g762.c               | 1012 ++++++++++++++++++++++++++++++++++++
> > >> >>  include/linux/platform_data/g762.h |   54 ++
> > >> >>  4 files changed, 1077 insertions(+)
> > >> >>  create mode 100644 drivers/hwmon/g762.c
> > >> >>  create mode 100644 include/linux/platform_data/g762.h
> > >> >
> > >> > Hi Arnaud,
> > >> >
> > >> > After more tests on my 2Big Network v2 board, it appears that the fan
> > >> > doesn't rotate when PWM mode (the preferred operating mode for this
> > >> > board) is selected. Nevertheless, DC mode is usable (even if not ideal
> > >> > given the hardware). After some investigations I noticed that an extra
> > >> > initialization is needed to enable PWM mode on my board: the set_cnt
> > >> > register must be set to 0 while the default value is 0xff. Is that
> > >> > specific to my hardware ? Is PWM mode working on your ReadyNAS with
> > >> > the default set_cnt value ?
> > >> 
> > >> First, thanks for testing this!
> > >> 
> > >> Regarding your problem, I first started by booting current version of my
> > >> driver on the Duo v2 w/o touching chip registers (only clock reference
> > >> value). This way, I inherit the values installed by (NETGEAR's) u-boot:
> > >> 
> > >> $ for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
> > >> fan1_alarm:0
> > >> fan1_div:2
> > >> fan1_fault:0
> > >> fan1_input:1807
> > >> fan1_target:1807
> > >> pwm1:221
> > >> pwm1_enable:2        /* closed-loop, i.e. config is done via set_cnt */
> > >> pwm1_mode:1          /* PWM mode */ 
> > >
> > > Sorry, I realize that I have not been very accurate in my description of
> > > the problem: The fan doesn't rotate when PWM _and_ open-loop control are
> > > selected. On the 2Big2 board, 2 wires are used to drive the fan.
> > >
> > > Then with pwm1_mode=1, pwm1_enable=1 and set_cnt=0xff (default value),
> > > nothing happen whatever the value I write in pwm1.
> > 
> > After boot, with set_cnt and set_out untouched by me (set_cnt set to
> > 0x5a by u-boot, i.e. 255-0x5a read on pwm1):
> > 
> > fan1_alarm:0
> > fan1_div:1
> > fan1_fault:0
> > fan1_input:1365
> > fan1_target:1365
> > pwm1:0                  /* set_out is 0 (considering fan1_target,
> >                            set_cnt is 0x5a)*/
> > pwm1_enable:2           /* closed-loop */
> > pwm1_mode:0             /* DC mode */
> > 
> > # echo 1 > pwm1_mode    /* PWM mode */       # fan rotates 
> > # echo 1 > pwm1_enable  /* open-loop */      # fan stops rotating
> > 
> > At that point we have:
> > 
> > fan1_alarm:0
> > fan1_div:1
> > fan1_fault:0
> > fan1_input:0
> > fan1_target:1365
> > pwm1:0                  /* set_out is 0, set_cnt is still 0x5a */
> > pwm1_enable:1
> > pwm1_mode:1
> > 
> > Then:
> > 
> > # echo 100 > pwm1                            # fan rotates
> 
> OK you are lucky. Your bootloader initialize set_cnt with 0x5a. Mine
> don't and it is precisely the issue: set_cnt is still 0xff (the default
> value) when the driver controls the fan.
> 
> Please, could you try open-loop mode with set_cnt set to 0xff ?  Maybe
> you can enforce this value for the test purpose ?
> 
> If you observe the same behaviour than me, then could modify the driver
> to ensure that set_cnt is not 0xff when open-loop mode is selected ?
> Maybe by systematically setting a different value (as 0) ?
> 
I think it would make sense to set it to 0 when open-loop mode is selected, or
at least to set it to a value != 0xff. Question would be if the value has any
meaning in open loop mode, other than to enable/disable fan control entirely.

Thanks,
Guenter

> > 
> > 
> > > By testing, I have discovered that writing 0 into set_cnt allows to work
> > > around the issue. I can do this by using the closed-loop control:
> > > pwm1_mode=1, pwm1_enable=2 and pwm=255. Now, set_cnt worths 0 and if I
> > > switch back to open-loop control, all works as expected.
> > >
> > > Can you try open-loop control and PWM mode with your board ? I wonder if
> > > this issue is specific to the 2Big2 hardware.
> > 
> > AFAICT, set_cnt has never been set to 0 in my test and PWM+open-loop
> > works as expected, i.e. just by setting a non-zero value in set_out.
> > Tell me if I missed something or if you want me to perform another
> > test.
> 
> Actually, I have only problems when set_cnt worths 0xff. Please, could
> you try the tests mentioned above ?
> 
> Thanks in advance,
> 
> Simon
> 
> > 
> > Cheers,
> > 
> > a+
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 1/3] Add support for GMT G762/G763 PWM fan controller

[Arnaud Ebalard]

Hi Simon,

spoiler: end of the story below ;-)

Simon Guinot <simon.guinot@sequanux.org> writes:

> OK you are lucky. Your bootloader initialize set_cnt with 0x5a. Mine
> don't and it is precisely the issue: set_cnt is still 0xff (the default
> value) when the driver controls the fan.
>
> Please, could you try open-loop mode with set_cnt set to 0xff ?  Maybe
> you can enforce this value for the test purpose ?
>
> If you observe the same behaviour than me, then could modify the driver
> to ensure that set_cnt is not 0xff when open-loop mode is selected ?
> Maybe by systematically setting a different value (as 0) ?

Here we go:

# cd /sys/bus/i2c/drivers/g762/0-003e

# for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
fan1_alarm:0
fan1_div:1
fan1_fault:0
fan1_input:1365
fan1_pulses:2
fan1_target:1365
pwm1:0
pwm1_enable:2             /* closed-loop */
pwm1_mode:0               /* DC mode */

# echo 0 > fan1_target   /* set set_cnt to 0, fan stops rotating */

# echo 1 > pwm1_enable   /* switch to open-loop, fan does not rotate yet
                          * as default value for set_out is 0 and it has
                          * neither been touched by bootloader or driver */

# for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
fan1_alarm:0
fan1_div:1
fan1_fault:0
fan1_input:0
fan1_pulses:2
fan1_target:0
pwm1:0
pwm1_enable:1
pwm1_mode:0

# echo 100 > pwm1        /* set set_out to 100, fan rotates */


I don't get the issue in DC mode but ...


# echo 1 > pwm1_mode     /* switch to PWM mode, fan stops rotating
                          * even though set_out is still set to
                          * 100: */
# for k in fan* pwm* ; do echo -n "$k:" ; echo `cat $k `; done
fan1_alarm:0
fan1_div:1
fan1_fault:1
fan1_input:0
fan1_pulses:2
fan1_target:0
pwm1:100
pwm1_enable:1
pwm1_mode:1

# echo 1000 > fan1_target

At that point, fan starts rotating again and its speed can be controlled
using pwm1 (i.e. set_out). But, as you pointed, if set_cnt is set to 255
then, it stop rotating: with my hardware, I tried and set fan1_target to
481 (set_cnt is 255) and fan stops. Then setting fan1_target to 482
gives set_cnt a value of 254 and make the fan run again.

So, as a conclusion, I think I get the behavior you described i.e. in
PWM mode + open-loop, if set_cnt is 255, then the fan will not rotate,
no matter the value of set_out (which is supposed to control the
rotation w/ open-loop). The problem does not appear in DC mode
(i.e. set_cnt can be 255 in DC mode + open-loop and the fan will rotate
as expected based on set_out value).

In v4 version to come, I have implemented the following fix:

	switch (val) {
	case G762_FAN_MODE_OPEN_LOOP:
		data->fan_cmd1 &= ~G762_REG_FAN_CMD1_FAN_MODE;
		/*
		 * BUG FIX: if SET_CNT register value is 255 then, for some
		 * unknown reason, fan will not rotate as expected, no matter
		 * the value of SET_OUT (to be specific, this seems to happen
		 * only in PWM mode). To workaround this bug, we give SET_CNT
		 * value of 254 if it is 255 when switching to open-loop.
		 */
		if (data->set_cnt == 0xff)
			i2c_smbus_write_byte_data(client, G762_REG_SET_CNT,
						  254);
		break;
	case G762_FAN_MODE_CLOSED_LOOP:
		data->fan_cmd1 |=  G762_REG_FAN_CMD1_FAN_MODE;
		break;
	default:
		goto out;
	}

I tested it and it fixe the issue. Can you confirm it also does on your
side Simon by testing v4?

Cheers,

a+