Add MAX6650 support

This patch adds support for the MAX6650 fan controller. I'm not the author of this module, just tested it on an i386 embedded system and cleaned up a bit. The patch is against 2.6.20-rc3.

Signed-off-by Hans J. Koch <hjk at linutronix.de>

Hello

i saw some problems in your code:

On line 141 you excess the 80 characters width
+ static ssize_t get_fan(struct device *dev, struct device_attribute *devattr, char *buf, int nr)

On the comment you have written "MAX6650 also" instead of "MAX6551 also" * This module has only been tested with the MAX6650 chip. It should * work with the MAX6650 also, though with reduced functionality. It * does not distinguish max6650 and max6651 chips.

You must use dev_dbg instead of #ifdef debug

On line 535 the while(0) is useless
erase the max6650_init_client function

(quoted diff) +3 -0

+static void max6650_init_client(struct i2c_client *client)
+{
+	/* Nothing to do here - assume the BIOS has initialized the chip

*/ + while(0)

(quoted diff) +4 -0

+	{
+		;
+	}
+}

Don't use down() up()
replace with mutex

On line 553

	MAX6650_REG_TACH0, MAX6650_REG_TACH1,
	MAX6650_REG_TACH2, MAX6650_REG_TACH3
	one per line
	"," at the end
like
static const u8 tach_reg[] > {
	MAX6650_REG_TACH0,
	MAX6650_REG_TACH1,
	MAX6650_REG_TACH2,
	MAX6650_REG_TACH3,
};

Why use max6650_read_value and max6650_write_value? replace with i2c_smbus_write_byte_data and i2c_smbus_read_byte_data directly

Check error return code of i2c_detach_client(client); like
if ((err = i2c_detach_client(client)))

	return err;

Don't use many device_create_file
use sysfs_create_group

Am Sonntag 11 Februar 2007 16:44 schrieb corentin.labbe:

Quoted 57 lines

Hello

i saw some problems in your code:

On line 141 you excess the 80 characters width
+ static ssize_t get_fan(struct device *dev, struct device_attribute *devattr, char *buf, int nr)

On the comment you have written "MAX6650 also" instead of "MAX6551 also" * This module has only been tested with the MAX6650 chip. It should * work with the MAX6650 also, though with reduced functionality. It * does not distinguish max6650 and max6651 chips.

You must use dev_dbg instead of #ifdef debug

On line 535 the while(0) is useless
erase the max6650_init_client function

(quoted diff) +3 -0

+static void max6650_init_client(struct i2c_client *client)
+{
+	/* Nothing to do here - assume the BIOS has initialized the chip

*/ + while(0)

(quoted diff) +4 -0

+	{
+		;
+	}
+}

Don't use down() up()
replace with mutex

On line 553

	MAX6650_REG_TACH0, MAX6650_REG_TACH1,
	MAX6650_REG_TACH2, MAX6650_REG_TACH3
	one per line
	"," at the end
like
static const u8 tach_reg[] > > {
	MAX6650_REG_TACH0,
	MAX6650_REG_TACH1,
	MAX6650_REG_TACH2,
	MAX6650_REG_TACH3,
};

Why use max6650_read_value and max6650_write_value? replace with i2c_smbus_write_byte_data and i2c_smbus_read_byte_data directly

Check error return code of i2c_detach_client(client); like
if ((err = i2c_detach_client(client)))

	return err;

Quoted 2 lines

Don't use many device_create_file
use sysfs_create_group

Thanks for your review, I'll look at the code again and post an updated patch soon.

Hi Hans,

I have tried the driver with on kernel 2.6.20.1 for platform ARM (processor PXA255) to get fan speeds from the chip max6651.

It seems that the driver works, because it exports the values in /sys file system.

I have few small comments on the implementation:

* use mutex instead of semaphore

* define i2c driver ID in the include/i2c-id.h

However I have noticed the following issue, which might be related to the driver of libsensors. Whenever I issue the command sensors to display values of the fans, I get an error:

$ sensors
max1617a-i2c-0-18
Adapter: pxa2xx-i2c
Main PCB:    +40 C  (low  =    +0 C, high =   +50 C)

max1617a-i2c-0-29
Adapter: pxa2xx-i2c
Main PCB: +38 C (low = +0 C, high = +50 C)

max6650-i2c-0-48
Adapter: pxa2xx-i2c
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
ERROR: Can't get FAN4 data!

max6650-i2c-0-4b
Adapter: pxa2xx-i2c
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
ERROR: Can't get FAN4 data!

$ sensors --version
sensors version 2.10.2 with libsensors version 2.10.2

I tried to rename entries that the driver creates from fan0-fan3 to fan1-fan4 as used by the libsensors and sensors, but that didn't help.

Did you have to modify the (lib)sensors in any way to display the values?

Quoted 10 lines

Hi Hans,

I have tried the driver with on kernel 2.6.20.1 for platform ARM (processor PXA255) to get fan speeds from the chip max6651.

It seems that the driver works, because it exports the values in /sys file system.

I have few small comments on the implementation:

Quoted 1 line

* use mutex instead of semaphore

This was fixed in the second version of the driver (posted by Hans-J?rgen on February 12th.)

Quoted 1 line

* define i2c driver ID in the include/i2c-id.h

Even better, don't set an ID if you don't need it.

Quoted 28 lines

However I have noticed the following issue, which might be related to the driver of libsensors. Whenever I issue the command sensors to display values of the fans, I get an error:

$ sensors
max1617a-i2c-0-18
Adapter: pxa2xx-i2c
Main PCB:    +40 C  (low  =    +0 C, high =   +50 C)

max1617a-i2c-0-29
Adapter: pxa2xx-i2c
Main PCB: +38 C (low = +0 C, high = +50 C)

max6650-i2c-0-48
Adapter: pxa2xx-i2c
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
ERROR: Can't get FAN4 data!

max6650-i2c-0-4b
Adapter: pxa2xx-i2c
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
ERROR: Can't get FAN4 data!

Quoted 5 lines

$ sensors --version
sensors version 2.10.2 with libsensors version 2.10.2

I tried to rename entries that the driver creates from fan0-fan3 to fan1-fan4 as used by the libsensors and sensors, but that didn't help.

The files should actually be named fan1_input to fan4_input (for measured values.)

Quoted 2 lines

Did you have to modify the (lib)sensors in any way to display the values?

You shouldn't need to.

Quoted 1 line

* define i2c driver ID in the include/i2c-id.h

Even better, don't set an ID if you don't need it.

The files should actually be named fan1_input to fan4_input (for measured values.)

Quoted 5 lines

Quoted 2 lines

I tried to rename entries that the driver creates from fan0-fan3 to fan1-fan4 as used by the libsensors and sensors, but that didn't help.

The files should actually be named fan1_input to fan4_input (for measured values.)

I renamed the files accordingly.

I attached the current version of the patch, please tell us if this works for you.

I have just tried your patch with kernel 2.6.20.1. Now I get values with sensors:

$ sensors
max6650-i2c-0-48
Adapter: pxa2xx-i2c
fan4:       30 RPM

max6650-i2c-0-4b
Adapter: pxa2xx-i2c
fan4: 30 RPM

However, the values seem very low to me. With kernel 2.4 I got values in the range around 4000 RPM. I'll check sensors.conf as well.

2007/2/27, Jean Delvare <khali at linux-fr.org>:

Quoted 3 lines

Quoted 1 line

* define i2c driver ID in the include/i2c-id.h

Even better, don't set an ID if you don't need it.

Most of the hwmon drivers set the id. Were they set by inertia or do some of the drivers actually need it?

Quoted 2 lines

The files should actually be named fan1_input to fan4_input (for measured values.)

I've modified the names like this:

static DEVICE_ATTR(fan1_input, S_IRUGO, get_fan1, NULL); static DEVICE_ATTR(fan2_input, S_IRUGO, get_fan2, NULL); static DEVICE_ATTR(fan3_input, S_IRUGO, get_fan3, NULL); static DEVICE_ATTR(fan4_input, S_IRUGO, get_fan4, NULL); static DEVICE_ATTR(count, S_IRUGO, get_count, NULL); static DEVICE_ATTR(config, S_IRUGO, get_config, NULL); static DEVICE_ATTR(speed, S_IWUSR | S_IRUGO, get_speed, set_speed);

Other driver usually have _min and _div files.

Do the count, speed and config also have to be modified to follow a convention?

Quoted 3 lines

Do the count, speed and config also have to be modified to follow a convention?

The count and config files are non-standard files so they don't have to follow any specific convention. In fact I would even remove these files, in particular the config file infringes the fundamental sysfs rule. And the count file isn't particularly useful.

The speed file, what is it doing? If it is used to set the desired fan speed, then the right name would be fan1_target. Does it only apply to one fan or all? Is it always active?

Am Dienstag, 27. Februar 2007 19:07 schrieb Jean Delvare:

Quoted 7 lines

Quoted 2 lines

Do the count, speed and config also have to be modified to follow a convention?

The count and config files are non-standard files so they don't have to follow any specific convention. In fact I would even remove these files, in particular the config file infringes the fundamental sysfs rule. And the count file isn't particularly useful.

Well, the current implementation assumes that the MAX6650 is initialized by the BIOS. On the other hand, the 5V/12V setting is done by the driver, the value is hardcoded in the source. This doesn't look like a clean solution.

I suggest to add module parameters to allow configuration of 5V/12V, prescaler, count and mode. Then we _know_ the settings and can drop the config and the count file. We also get rid of the hardcoded 5V/12V setting and don't need to rely on a BIOS that might or might not do what we want.

I added these changes. If you modprobe max6650 without any parameters, the driver makes no changes to the chip's configuration. With one or more parameter, you can change whatever you like. I think this is a good solution because it doesn't assume there's a BIOS that does what you want.

Please test, review, comment...

2007/2/28, Hans-J?rgen Koch <hjk at linutronix.de>:

Quoted 6 lines

I added these changes. If you modprobe max6650 without any parameters, the driver makes no changes to the chip's configuration. With one or more parameter, you can change whatever you like. I think this is a good solution because it doesn't assume there's a BIOS that does what you want.

Quoted 1 line

Please test, review, comment...

Well done, Hans!

This version of the driver works well on my ARM-based system.

Well, the current implementation assumes that the MAX6650 is initialized by the BIOS. On the other hand, the 5V/12V setting is done by the driver, the value is hardcoded in the source. This doesn't look like a clean solution.

I suggest to add module parameters to allow configuration of 5V/12V, prescaler, count and mode. Then we _know_ the settings and can drop the config and the count file. We also get rid of the hardcoded 5V/12V setting and don't need to rely on a BIOS that might or might not do what we want.

Any objections?

Quoted 3 lines

The speed file, what is it doing? If it is used to set the desired fan speed, then the right name would be fan1_target. Does it only apply to one fan or all? Is it always active?

It sets the speed of the single fan for a MAX6650. For a MAX6651, it sets a speed that is valid for all connected fans, if I understand the data sheet correctly. In any case, this works only if mode is set to "closed loop".

Thanks a lot for testing, Matej! Jean, what do you say to that? If you think you can apply this I'll update the documentation file.

Hi Hans-J?rgen,

Quoted 3 lines · Hans-J?rgen Koch

Thanks a lot for testing, Matej! Jean, what do you say to that? If you think you can apply this I'll update the documentation file.

Thanks for all the work you've been doing on the driver lately. I am waiting for Corentin Labbe to comment before I do the final review and merge the driver - I know he found some more things that could be fixed or improved since his first review, some of which I think you addressed yesterday, some not.

I look briefly at the new options you implemented. They are definitely better as module options than hardcoded in the driver code, however I'm not sure we really want all of them to be module options.

General remark: the module parameters are driver-wide and not device-specific, so they must me checked at module load time (in sensors_max6650_init).

Also, for all settings that are not forced
by the user, it might be convenient to print in the logs the value that was found for each chip.

voltage_12V: Must indeed be an option, although the convention you chose is a bit strange IMHO. What about a parameter named "fan_voltage" with possible values 0 (default, no change), 5 and 12?

(BTW, is this an output voltage? input voltage? both?)

prescaler: What does it do? It smells like a fan clock divider to me. In that case it would be better implemented as a fan1_div sysfs file, which the user can adjust at runtime.

clock: OK.

mode: This smells like fan speed control parameters, which we typically implement as the pwm1_enable sysfs file. Please take a look at the description in Documentation/hwmon/sysfs-interface and tell me what you think.

counttime: This one too seems to be related to a fan clock divider? How does it functionally differ from prescaler?

Am Sonntag, 11. Februar 2007 17:22 schrieb Hans-J?rgen Koch:

Quoted 58 lines

Am Sonntag 11 Februar 2007 16:44 schrieb corentin.labbe:

Quoted 57 lines

Hello

i saw some problems in your code:

On line 141 you excess the 80 characters width
+ static ssize_t get_fan(struct device *dev, struct device_attribute *devattr, char *buf, int nr)

On the comment you have written "MAX6650 also" instead of "MAX6551 also" * This module has only been tested with the MAX6650 chip. It should * work with the MAX6650 also, though with reduced functionality. It * does not distinguish max6650 and max6651 chips.

You must use dev_dbg instead of #ifdef debug

On line 535 the while(0) is useless
erase the max6650_init_client function

(quoted diff) +3 -0

+static void max6650_init_client(struct i2c_client *client)
+{
+	/* Nothing to do here - assume the BIOS has initialized the chip

*/ + while(0)

(quoted diff) +4 -0

+	{
+		;
+	}
+}

Don't use down() up()
replace with mutex

On line 553

	MAX6650_REG_TACH0, MAX6650_REG_TACH1,
	MAX6650_REG_TACH2, MAX6650_REG_TACH3
	one per line
	"," at the end
like
static const u8 tach_reg[] >>> {
	MAX6650_REG_TACH0,
	MAX6650_REG_TACH1,
	MAX6650_REG_TACH2,
	MAX6650_REG_TACH3,
};

Why use max6650_read_value and max6650_write_value? replace with i2c_smbus_write_byte_data and i2c_smbus_read_byte_data directly

Check error return code of i2c_detach_client(client); like
if ((err = i2c_detach_client(client)))

	return err;

Quoted 5 lines

Quoted 2 lines

Don't use many device_create_file
use sysfs_create_group

Thanks for your review, I'll look at the code again and post an updated patch soon.

Hi,
here's the promised patch with these issues hopefully solved. I tested the module on a Kontron CPX board, with a vanilla 2.6.20 kernel, it seems to work fine.

Thanks again for the review,

Hans

------------------------------------------------------------------------

Index: linux-2.6.20/Documentation/hwmon/max6650
=================================> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20/Documentation/hwmon/max6650 2007-02-12 14:40:26.000000000 +0100

(quoted diff) +32 -0

@@ -0,0 +1,33 @@
+Kernel driver max6650
+==========> +
+Supported chips:
+  * Maxim 6650 / 6651
+    Prefix: 'w83792d'
+    Addresses scanned: I2C 0x1b, 0x1f, 0x48, 0x4b
+    Datasheet: http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
+
+Authors:
+    John Morris <john.morris at spirentcom.com>
+    Claus Gindhart <claus.gindhart at kontron.com>
+    Hans J. Koch <hjk at linutronix.de>
+
+Description
+-----------
+
+This driver implements support for the Maxim 6650/6651
+
+The 2 devices are very similar, bit the Maxim 6550 has a reduced feature
+set, e.g. only one fan-input, instead of 4 for the 6651.
+
+The driver is not able to distinguish between the 2 devices.
+
+The driver provides the following sensor accesses
+
+fan0   ro     fan tachometer speed in RPM
+fan1   ro     "
+fan2   ro     "
+fan3   ro     "
+config ro     contents of the config register (for debugging)
+count  ro     tachometer count time in milliseconds
+speed  rw     fan speed adjustment value

Index: linux-2.6.20/drivers/hwmon/Kconfig
=================================> --- linux-2.6.20.orig/drivers/hwmon/Kconfig 2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/hwmon/Kconfig 2007-02-12 09:01:25.000000000 +0100

(quoted diff) +10 -0

@@ -364,6 +364,16 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called max1619.
 
+config SENSORS_MAX6650
+	tristate "Maxim MAX6650 sensor chip"
+	depends on HWMON && I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the MAX6650 / MAX6651
+	  sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max6650.
+
 config SENSORS_PC87360
 	tristate "National Semiconductor PC87360 family"
 	depends on HWMON && I2C && EXPERIMENTAL

Index: linux-2.6.20/drivers/hwmon/Makefile
=================================> --- linux-2.6.20.orig/drivers/hwmon/Makefile 2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/hwmon/Makefile 2007-02-12 09:01:25.000000000 +0100

(quoted diff) +1 -0

@@ -42,6 +42,7 @@
 obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_MAX1619)	+= max1619.o
+obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o

Index: linux-2.6.20/drivers/hwmon/max6650.c
=================================> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20/drivers/hwmon/max6650.c 2007-02-12 14:34:50.000000000 +0100

(quoted diff) +557 -1

@@ -0,0 +1,558 @@
+/*
+ * max6650.c - Part of lm_sensors, Linux kernel modules for hardware
+ *             monitoring.
+ *
+ * Author: John Morris <john.morris at spirentcom.com>
+ * Copyright (c) 2003 Spirent Communications
+ *
+ * This module has only been tested with the MAX6650 chip. It should
+ * also work with the MAX6651, though with reduced functionality. It
+ * does not distinguish max6650 and max6651 chips.
+ *
+ * Tha datasheet was last seen at:
+ *
+ *        http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
+ *
+ * Ported to Linux 2.6 by Claus Gindhart <claus.gindhart at kontron.com>
+ * Some cleanup done by Hans J. Koch <hjk at linutronix.de>
+ *
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+
+#ifndef I2C_DRIVERID_MAX6650
+#define I2C_DRIVERID_MAX6650	1044
+#endif
+
+/* HW-related: Set this to 1 for 12V and 0 for 5V configuration */
+static const int voltage_12V=0;
+
+/*
+ * Addresses to scan. There are four disjoint possibilities, by pin config.
+ */
+
+static unsigned short normal_i2c[] = {0x1b, 0x1f, 0x48, 0x4b, I2C_CLIENT_END};
+
+/*
+ * Insmod parameters
+ */
+
+I2C_CLIENT_INSMOD_1(max6650);
+
+/*
+ * MAX 6650/6651 registers
+ */
+
+#define MAX6650_REG_SPEED       0x00
+#define MAX6650_REG_CONFIG      0x02
+#define MAX6650_REG_GPIO_DEF    0x04
+#define MAX6650_REG_DAC         0x06
+#define MAX6650_REG_ALARM_EN    0x08
+#define MAX6650_REG_ALARM       0x0A
+#define MAX6650_REG_TACH0       0x0C
+#define MAX6650_REG_TACH1       0x0E
+#define MAX6650_REG_TACH2       0x10
+#define MAX6650_REG_TACH3       0x12
+#define MAX6650_REG_GPIO_STAT   0x14
+#define MAX6650_REG_COUNT       0x16
+
+
+/*
+ * Config register bits
+ */
+
+#define MAX6650_CFG_V12           	0x08
+#define MAX6650_CFG_MODE_MASK           0x30
+#define MAX6650_CFG_MODE_ON             0x00
+#define MAX6650_CFG_MODE_OFF            0x10
+#define MAX6650_CFG_MODE_CLOSED_LOOP    0x20
+#define MAX6650_CFG_MODE_OPEN_LOOP      0x30
+
+#define MAX6650_INT_CLK 254000  /* Default clock speed - 254 kHz */
+
+/* Minimum and maximum values of the FAN-RPM */
+#define FAN_RPM_MIN 240
+#define FAN_RPM_MAX 30000
+
+#define DIV_FROM_REG(reg) (1 << (reg & 7))
+
+static int max6650_attach_adapter(struct i2c_adapter *adapter);
+static int max6650_detect(struct i2c_adapter *adapter, int address, int kind);
+static void max6650_init_client(struct i2c_client *client);
+static int max6650_detach_client(struct i2c_client *client);
+static struct max6650_data *max6650_update_device(struct device *dev);
+
+/*
+ * Driver data (common to all clients)
+ */
+
+static struct i2c_driver max6650_driver = {
+	.driver = {
+		.name	= "max6650",
+	},
+	.id             = I2C_DRIVERID_MAX6650,
+	.attach_adapter = max6650_attach_adapter,
+	.detach_client  = max6650_detach_client
+};
+
+/*
+ * Client data (each client gets its own)
+ */
+
+struct max6650_data
+{
+	struct i2c_client client;
+	struct class_device *class_dev;
+	struct mutex update_lock;
+	char valid; /* zero until following fields are valid */
+	unsigned long last_updated; /* in jiffies */
+
+	/* register values */
+	u8 speed;
+	u8 config;
+	u8 tach[4];
+	u8 count;
+};
+
+static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
+		       char *buf, int nr)
+{
+	struct max6650_data *data = max6650_update_device(dev);
+
+	/*
+	* Calculation details:
+	*
+	* Each tachometer counts over an interval given by the "count"
+	* register (0.25, 0.5, 1 or 2 seconds). This module assumes
+	* that the fans produce two pulses per revolution (this seems
+	* to be the most common).
+	*/
+
+	int rpm = ((data->tach[nr] * 120) / DIV_FROM_REG(data->count));
+	return sprintf(buf, "%d\n", rpm);
+}
+
+static ssize_t get_fan0(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,0);
+}
+
+static ssize_t get_fan1(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,1);
+}
+
+static ssize_t get_fan2(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,2);
+}
+
+static ssize_t get_fan3(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,3);
+}
+
+/* Show values of the config register for debugging purposes
+ */
+
+static ssize_t get_config(struct device *dev, struct device_attribute *devattr,
+			  char *buf)
+{
+	struct max6650_data *data = max6650_update_device(dev);
+
+	return sprintf(buf, "mode: %s%s%s%s, voltage:%s prescale: %d\n",
+		((data->config & MAX6650_CFG_MODE_MASK) =
+		  MAX6650_CFG_MODE_ON) ? "On" : "",
+		((data->config & MAX6650_CFG_MODE_MASK) =
+		  MAX6650_CFG_MODE_OFF) ? "Off" : "",
+		((data->config & MAX6650_CFG_MODE_MASK) =
+		  MAX6650_CFG_MODE_CLOSED_LOOP) ? "closed loop" : "",
+		((data->config & MAX6650_CFG_MODE_MASK) =
+		  MAX6650_CFG_MODE_OPEN_LOOP) ? "open Loop" : "",
+		(data->config & MAX6650_CFG_V12) ? "12V" : "5V",
+		DIV_FROM_REG(data->config)
+		);
+}
+
+/* Returns count time in ms */
+static ssize_t get_count(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	struct max6650_data *data = max6650_update_device(dev);
+
+	return sprintf(buf, "%d\n", DIV_FROM_REG(data->count) * 250 );
+}
+
+/*
+ * Set the fan speed to the specified RPM (or read back the RPM setting).
+ *
+ * The MAX6650/1 will automatically control fan speed when in closed loop
+ * mode.
+ *
+ * Assumptions:
+ *
+ * 1) The MAX6650/1 is running from its internal 254kHz clock (perhaps
+ *    this should be made a module parameter).
+ *
+ * 2) The prescaler (low three bits of the config register) has already
+ *    been set to an appropriate value.
+ *
+ * The relevant equations are given on pages 21 and 22 of the datasheet.
+ *
+ * From the datasheet, the relevant equation when in regulation is:
+ *
+ *    [fCLK / (128 x (KTACH + 1))] = 2 x FanSpeed / KSCALE
+ *
+ * where:
+ *
+ *    fCLK is the oscillator frequency (either the 254kHz internal
+ *         oscillator or the externally applied clock)
+ *
+ *    KTACH is the value in the speed register
+ *
+ *    FanSpeed is the speed of the fan in rps
+ *
+ *    KSCALE is the prescaler value (1, 2, 4, 8, or 16)
+ *
+ * When reading, we need to solve for FanSpeed. When writing, we need to
+ * solve for KTACH.
+ *
+ * Note: this tachometer is completely separate from the tachometers
+ * used to measure the fan speeds. Only one fan's speed (fan1) is
+ * controlled.
+ */
+
+static ssize_t get_speed(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	struct max6650_data *data = max6650_update_device(dev);
+	int kscale, ktach, fclk, rpm;
+
+	/*
+	* Use the datasheet equation:
+	*
+	*    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
+	*
+	* then multiply by 60 to give rpm.
+	*/
+
+	kscale = DIV_FROM_REG(data->config);
+	ktach  = data->speed;
+	fclk   = MAX6650_INT_CLK;
+	rpm    = 60 * kscale * fclk / (256 * (ktach + 1));
+	return sprintf(buf, "%d\n", rpm);
+}
+
+static ssize_t set_speed(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);
+	int rpm = simple_strtoul(buf, NULL, 10);
+	int kscale, ktach, fclk;
+
+	dev_dbg(dev, "%s called, rpm=%d\n", __FUNCTION__, rpm);
+
+	mutex_lock(&data->update_lock);
+	if (rpm = 0)
+	{
+		/* Switch totally off */
+		data->speed  = 0;
+		data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
+				| MAX6650_CFG_MODE_OFF;
+	}
+	else
+	{
+		rpm = SENSORS_LIMIT(rpm, FAN_RPM_MIN, FAN_RPM_MAX);
+
+		/*
+		* Divide the required speed by 60 to get from rpm to rps, then
+		* use the datasheet equation:
+		*
+		*     KTACH = [(fCLK x KSCALE) / (256 x FanSpeed)] - 1
+		*/
+
+		kscale = DIV_FROM_REG(data->config);
+		fclk   = MAX6650_INT_CLK;
+		ktach  = ((fclk * kscale) / (256 * rpm / 60)) - 1;
+
+		data->speed  = ktach;
+		data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
+				| MAX6650_CFG_MODE_CLOSED_LOOP;
+	}
+	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
+	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED,  data->speed);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static DEVICE_ATTR(fan0, S_IRUGO, get_fan0, NULL);
+static DEVICE_ATTR(fan1, S_IRUGO, get_fan1, NULL);
+static DEVICE_ATTR(fan2, S_IRUGO, get_fan2, NULL);
+static DEVICE_ATTR(fan3, S_IRUGO, get_fan3, NULL);
+static DEVICE_ATTR(count, S_IRUGO, get_count, NULL);
+static DEVICE_ATTR(config, S_IRUGO, get_config, NULL);
+static DEVICE_ATTR(speed, S_IWUSR | S_IRUGO, get_speed, set_speed);
+
+static struct attribute *max6650_attrs[] = {
+	&dev_attr_fan0.attr,
+	&dev_attr_fan1.attr,
+	&dev_attr_fan2.attr,
+	&dev_attr_fan3.attr,
+	&dev_attr_count.attr,
+	&dev_attr_config.attr,
+	&dev_attr_speed.attr,
+	NULL,
+};
+
+static struct attribute_group max6650_attr_grp = {
+	.attrs = max6650_attrs,
+};
+
+/*
+ * Real code
+ */
+
+static int max6650_attach_adapter(struct i2c_adapter *adapter)
+{
+	dev_dbg(adapter->dev, "max6650_attach_adapter called for %s\n",
+		(char*)&adapter->name);
+
+	if (!(adapter->class & I2C_CLASS_HWMON)) {
+		dev_dbg(adapter->dev,
+			"FATAL: max6650_attach_adapter class HWMON not set\n");
+		return 0;
+	}
+
+	return i2c_probe(adapter, &addr_data, max6650_detect);
+}
+
+/*
+ * The following function does more than just detection. If detection
+ * succeeds, it also registers the new chip.
+ */
+
+static int max6650_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client *new_client;
+	struct max6650_data *data;
+	int err = -ENODEV;
+	const char *name = "";
+
+	dev_dbg(adapter->dev, "max6650_detect called, kind = %d\n",kind);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_dbg(adapter->dev, "MAX6650: I2C bus doesn't support byte"
+			"read mode, skipping.\n");
+		return 0;
+	}
+
+	if (!(data = kzalloc(sizeof(struct max6650_data), GFP_KERNEL))) {
+		printk("MAX6650: Out of memory in max6650_detect "
+			"(new_client).\n");
+		return -ENOMEM;
+	}
+
+	/*
+	 * The common I2C client data is placed right before the
+	 * max6650-specific data. The max6650-specific data is pointed to by
+	 * the data field from the I2C client data.
+	 */
+
+	new_client = &data->client;
+	i2c_set_clientdata(new_client, data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &max6650_driver;
+	new_client->flags = 0;
+
+	/*
+	 * Now we do the remaining detection. A negative kind means that
+	 * the driver was loaded with no force parameter (default), so we
+	 * must both detect and identify the chip (actually there is only
+	 * one possible kind of chip for now, max6650). A zero kind means that
+	 * the driver was loaded with the force parameter, the detection
+	 * step shall be skipped. A positive kind means that the driver
+	 * was loaded with the force parameter and a given kind of chip is
+	 * requested, so both the detection and the identification steps
+	 * are skipped.
+	 *
+	 * Currently I can find no way to distinguish between a MAX6650 and
+	 * a MAX6651. This driver has only been tried on the latter.
+	 */
+
+	if ((kind < 0)&&
+	   (  (i2c_smbus_read_byte_data(new_client,MAX6650_REG_CONFIG) & 0xC0)
+	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_GPIO_STAT)& 0xE0)
+	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM_EN) & 0xE0)
+	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM) & 0xE0)
+	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_COUNT) & 0xFC)))
+	{
+		dev_dbg(adapter->dev, "max6650.o: max6650 detection failed"
+			"at 0x%02x.\n", address);
+
+		goto err_free;
+        }
+
+	/* Configure HW-related voltage setting */
+	if (voltage_12V) {
+		i2c_smbus_write_byte_data(new_client, MAX6650_REG_CONFIG,
+			i2c_smbus_read_byte_data(new_client, MAX6650_REG_CONFIG)
+			| MAX6650_CFG_V12);
+	}
+	else {
+		i2c_smbus_write_byte_data(new_client, MAX6650_REG_CONFIG,
+			i2c_smbus_read_byte_data(new_client, MAX6650_REG_CONFIG)
+			& ~MAX6650_CFG_V12);
+	}
+
+	if (kind <= 0) {
+		kind = max6650;
+	}
+
+	if (kind = max6650) {
+		name = "max6650";
+		printk("MAX6650: Chip found at 0x%02x.\n", address);
+	}
+	else {
+		printk("MAX6650: Unsupported chip.\n");
+		goto err_free;
+	}
+
+	/*
+	 * OK, we got a valid chip so we can fill in the remaining client
+	 * fields.
+	 */
+
+	strlcpy(new_client->name, name, I2C_NAME_SIZE);
+	data->valid = 0;
+	mutex_init(&data->update_lock);
+
+	/*
+	 * Tell the I2C layer a new client has arrived.
+	 */
+
+	if ((err = i2c_attach_client(new_client))) {
+		dev_dbg(adapter->dev, "max6650.o: Failed attaching client.\n");
+		goto err_free;
+	}
+
+	/*
+	 * Initialize the max6650 chip
+	 */
+	max6650_init_client(new_client);
+
+	/* Register sysfs hooks */
+	data->class_dev = hwmon_device_register(&new_client->dev);
+	if (IS_ERR(data->class_dev)) {
+		err = PTR_ERR(data->class_dev);
+		goto err_detach;
+	}
+
+	err = sysfs_create_group(&new_client->dev.kobj, &max6650_attr_grp);
+	if (!err)
+		return 0;
+
+	dev_err(&new_client->dev, "error creating sysfs files (%d)\n", err);
+	hwmon_device_unregister(data->class_dev);
+err_detach:
+	i2c_detach_client(new_client);
+err_free:
+	kfree(data);
+	return err;
+}
+
+static int max6650_detach_client(struct i2c_client *client)
+{
+	struct max6650_data *data = i2c_get_clientdata(client);
+	int err;
+	hwmon_device_unregister(data->class_dev);
+	err = i2c_detach_client(client);
+	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
+	kfree(data);
+	return err;
+}
+
+static void max6650_init_client(struct i2c_client *client)
+{
+	/* Nothing to do here - assume the BIOS has initialized the chip */
+}
+
+static const u8 tach_reg[] > +{
+	MAX6650_REG_TACH0,
+	MAX6650_REG_TACH1,
+	MAX6650_REG_TACH2,
+	MAX6650_REG_TACH3,
+};
+
+static struct max6650_data *max6650_update_device(struct device *dev)
+{
+	int i;
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
+		data->speed = i2c_smbus_read_byte_data(client,
+			 			       MAX6650_REG_SPEED);
+		data->config = i2c_smbus_read_byte_data(client,
+							MAX6650_REG_CONFIG);
+		for (i = 0; i < 4; i++) {
+			data->tach[i] = i2c_smbus_read_byte_data(client,
+								 tach_reg[i]);
+		}
+		data->count = i2c_smbus_read_byte_data (client,
+							MAX6650_REG_COUNT);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static int __init sensors_max6650_init(void)
+{
+	return i2c_add_driver(&max6650_driver);
+}
+
+static void __exit sensors_max6650_exit(void)
+{
+	i2c_del_driver(&max6650_driver);
+}
+
+MODULE_AUTHOR("john.morris at spirentcom.com");
+MODULE_DESCRIPTION("max6650 sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_max6650_init);
+module_exit(sensors_max6650_exit);
 
 
------------------------------------------------------------------------

_______________________________________________
lm-sensors mailing list
lm-sensors at lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

(quoted diff) +2 -0

+  * Maxim 6650 / 6651
+    Prefix: 'w83792d'

Instead of declaring four static DEVICE_ATTR(fan1, S_IRUGO, get_fan1, NULL); I'm suggest you to use
static SENSOR_DEVICE_ATTR(fan1, S_IRUGO, get_fan, NULL,1); static SENSOR_DEVICE_ATTR(fan2, S_IRUGO, get_fan, NULL,2); etc....

secondly in /Documentation/hwmon/max6650 you have a wrong prefix +Supported chips:

Review

Quoted 2 lines

(quoted diff) +2 -0

+  * Maxim 6650 / 6651
+    Prefix: 'w83792d'

and 2 typos errors
(but and not bit)
-> bit the Maxim 6550 has a reduced feature

6550 not 6650

Quoted 2 lines

(quoted diff) +2 -0

+  * Maxim 6650 / 6651
+    Prefix: 'w83792d'

Hi Jean,
I updated the driver once again. Basically, I made these changes:

* "mode" is now not only a module parameter, but also a sysfs file. This is needed to simply switch the fan fully on/off. Depending on the other settings, this might not be possible with the "speed" attribute alone.

* Setting "speed" really only sets the speed value. The original implementation changed "mode" between "off" and "closed loop" when speed was 0 or >0, respectively. I consider this bad style, a driver should do what it's told to do and not make guesses about what the user might wish.

* Updated documentation.

I left the other values as module parameters. I think it's unlikely that somebody wants to set these to different values if he has more than one chip. Of course we could make all of them sysfs files, too, but I think this is not necessary. Let's wait and see if somebody actually wants that.

Am Donnerstag 01 M?rz 2007 08:34 schrieb Jean Delvare:

Quoted 3 lines

prescaler: What does it do? It smells like a fan clock divider to me. In that case it would be better implemented as a fan1_div sysfs file, which the user can adjust at runtime.

The chip generates a reference frequency which is determined by the speed register, the prescaler, and the internal clock (nominal value 254kHz). The regulator then tries to make the tacho frequency equal to this reference frequency.
As clock is fixed and speed has a well defined meaning, the prescaler is the only way to adapt to different tacho generators. The data sheet gives hints about how to select a prescaler value to achieve optimal range and resolution. If you get that wrong, the fan might not reach it's full speed, switch between two speed values only or similar effects.

I read Documentation/hwmon/sysfs-interface, but I have to admit that I don't really understand if fan[1-*]_div has this meaning. If it has, we could possibly make prescaler such a sysfs file.

Quoted 1 line

clock: OK.

This is the only one where I think it could be omitted. The internal 254kHz oscillator has a tolerance of +/-10%, so fan speed can also be off +/-10%. This parameter is just there to compensate for that. I doubt it will ever be used in practical applications.

Quoted 4 lines

mode: This smells like fan speed control parameters, which we typically implement as the pwm1_enable sysfs file. Please take a look at the description in Documentation/hwmon/sysfs-interface and tell me what you think.

That looks good. We can implement pwm1_enable that way. Problem is that we can have up to four fan speeds (fan[1-4]_input), but the control values are there only once (pwm1, pwm1_enable). Is that possible?

Quoted 2 lines

counttime: This one too seems to be related to a fan clock divider? How does it functionally differ from prescaler?

counttime is the time interval during which tacho pulses are counted. It determines the resolution of the measured speed values. It should be set to the longest time that still ensures the 8-bit counter doesn't overflow under worst case conditions.

Hi Hans,

Sorry for the delay, I've been busy with large i2c-core changes.

Quoted 4 lines

I read Documentation/hwmon/sysfs-interface, but I have to admit that I don't really understand if fan[1-*]_div has this meaning. If it has, we could possibly make prescaler such a sysfs file.

I'm not certain myself. fan1_div is usually related to the monitoring only and not to the output.

Typical chips monitor fan speeds by gating a well defined frequency with the tachometer pulses. The result is stored in an 8-bit register, so basically you can only measure fan speeds down to F/255, speeds below this limit overflow the register. So the chips usually let the user divide F by a power of two to be able to monitor slower fans - at the price of resolution.

So the concept is similar to your "prescaler" in that it's a tradeoff between resolution and range. But the fan speed control method of the MAX6650 is completely different from what the other chips do, so other chips don't need any kind of divider for fan speed control.

Yes, this isn't a problem. I am not a specialist but I don't think it's possible to have a closed loop fan control working with 4 inputs and 1 output, so I believe the output is really related to fan1 only. A comment in the original driver seems to confirm that ("Only one fan's speed (fan1) is controlled.")

Quoted 5 lines

counttime is the time interval during which tacho pulses are counted. It determines the resolution of the measured speed values. It should be set to the longest time that still ensures the 8-bit counter doesn't overflow under worst case conditions.

_This_ really sounds like fan1_div. It changes the period of time rather than the clock frequency, but from a functional point of view the result is the same: preventing 8-bit register overflow for certain speed fans. The only difference is that typical chips overflow for slow fans, while the MAX6650 is more likely to overflow when the fan is too fast, if I understood correctly.

So please implement it that way. fan1_div = 1 should correspond to the smaller period of time (faster fan.)

Aren't the count time and precaler values somewhat related? Won't the user have to change both the same way if he/she gets a new fan running at a different speed? Maybe it would make sense to link fan1_div to both the prescaler and the count time after all.

Obviously, we've reached some limits of the current concept. IMHO, the 'pwm' and 'divider' sysfs files are much too hardware dependent. We should have more abstraction, e.g. like this:

fan1_input: (ro) Current speed in RPM
fan1_speed: (rw) Desired speed in RPM

fan1_min_speed,
fan1_max_speed: (rw) Planned/defined operating range in RPM

fan1_tacho: (rw) Pulses per round of the tacho generator

fan1_mode: (rw) on, off, closed_loop, open_loop, ...

From this, _every_ driver could calculate the necessary register values it needs, without bothering the user with chip internal details. Why should a user care if the fan regulator chip uses PWM or not? The values I mentioned above are the ones he/she knows and can easily set without having the data sheet in one hand and the pocket calculator in the other.

Of course, implementation is slightly more complicated, as the values depend on each other, so setting one of them might invalidate the setup until an other is set. The driver needs to deal with that in a reasonable way. But I think I could easily do it for the max6650. It shouldn't be difficult for other drivers, too.

Quoted 5 lines

Yes, this isn't a problem. I am not a specialist but I don't think it's possible to have a closed loop fan control working with 4 inputs and 1 output, so I believe the output is really related to fan1 only. A comment in the original driver seems to confirm that ("Only one fan's speed (fan1) is controlled.")

That comment, as well as the data sheet, are a bit misleading. In fact, the MAX6651 is NOT able to drive four independent fans. It just has four tacho inputs. The first one can be used to close the loop if you want closed loop operation. To the other three, you can connect whatever you like. The intended use is to connect four identical fans in parallel to the output. The tacho signal of one of them is used for closed loop regulation. If the fans are _really_ identical, all of them should have the same speed. You can check their actual speeds by looking at the other tacho inputs.

Quoted 12 lines

Quoted 4 lines

counttime is the time interval during which tacho pulses are counted. It determines the resolution of the measured speed values. It should be set to the longest time that still ensures the 8-bit counter doesn't overflow under worst case conditions.

_This_ really sounds like fan1_div. It changes the period of time rather than the clock frequency, but from a functional point of view the result is the same: preventing 8-bit register overflow for certain speed fans. The only difference is that typical chips overflow for slow fans, while the MAX6650 is more likely to overflow when the fan is too fast, if I understood correctly.

Quoted 2 lines

So please implement it that way. fan1_div = 1 should correspond to the smaller period of time (faster fan.)

As I said above, I really believe we shouldn't do that but be less hardware dependent.

Quoted 4 lines

Aren't the count time and precaler values somewhat related? Won't the user have to change both the same way if he/she gets a new fan running at a different speed? Maybe it would make sense to link fan1_div to both the prescaler and the count time after all.

If I did that, confusion would be complete. A user would not only need data sheet in one hand, pocket calculator in other hand, but also driver source code in the third hand to understand what's going on :-)

Initially, I didn't want to make a lot of changes to that driver, just have it in the mainline kernel. But meanwhile, I feel if we do it, we should do it properly. That means, either we accept the driver as it is now (with special sysfs files no other driver has), or we change the specification in Documentation/hwmon/sysfs-interface to be more general so that all fan drivers fit in.

What's your opinion?

Hi Hans-J?rgen,

Quoted 3 lines · Hans-J?rgen Koch

Obviously, we've reached some limits of the current concept. IMHO, the 'pwm' and 'divider' sysfs files are much too hardware dependent. We should have more abstraction, e.g. like this:

Forget about it right away, this interface is here to stay and we're not changing it.

Quoted 3 lines

fan1_input: (ro) Current speed in RPM
fan1_speed: (rw) Desired speed in RPM

We have fan1_input and fan1_target for this.

Quoted 3 lines

fan1_min_speed,
fan1_max_speed: (rw) Planned/defined operating range in RPM

We have fan1_min for the former. We could have fan1_max for the latter, but no known chip actually supports upper limit for fan speeds.

Quoted 2 lines

fan1_tacho: (rw) Pulses per round of the tacho generator

99% of the fans out there emit two pulses per revolution, so we didn't bother, although a couple driver do expose a non-standard sysfs file for that. The fact is that this can be addressed easily in user-space, so the interest in implementing it in every driver is questionable.

Quoted 2 lines

fan1_mode: (rw) on, off, closed_loop, open_loop, ...

This is exactly what pwm1_enable does. The name could have been chosen better, granted.

Quoted 13 lines

Quoted 13 lines

Quoted 5 lines

counttime is the time interval during which tacho pulses are counted. It determines the resolution of the measured speed values. It should be set to the longest time that still ensures the 8-bit counter doesn't overflow under worst case conditions.

_This_ really sounds like fan1_div. It changes the period of time rather than the clock frequency, but from a functional point of view the result is the same: preventing 8-bit register overflow for certain speed fans. The only difference is that typical chips overflow for slow fans, while the MAX6650 is more likely to overflow when the fan is too fast, if I understood correctly.

Quoted 2 lines

Quoted 2 lines

So please implement it that way. fan1_div = 1 should correspond to the smaller period of time (faster fan.)

Quoted 3 lines

As I said above, I really believe we shouldn't do that but be less hardware dependent.

It's just giving a different name to something to make it fit in the standard. It's rather _less_ hardware dependent than what you propose.

Quoted 5 lines

Quoted 4 lines

Aren't the count time and precaler values somewhat related? Won't the user have to change both the same way if he/she gets a new fan running at a different speed? Maybe it would make sense to link fan1_div to both the prescaler and the count time after all.

Quoted 3 lines

If I did that, confusion would be complete. A user would not only need data sheet in one hand, pocket calculator in other hand, but also driver source code in the third hand to understand what's going on :-)

I'm not insisting on this particular point then. This whole prescaler thing is above my head anyway.

Quoted 8 lines

Initially, I didn't want to make a lot of changes to that driver, just have it in the mainline kernel. But meanwhile, I feel if we do it, we should do it properly. That means, either we accept the driver as it is now (with special sysfs files no other driver has), or we change the specification in Documentation/hwmon/sysfs-interface to be more general so that all fan drivers fit in.

Quoted 1 line

What's your opinion?

That either the max6650 driver complies with the standard interface as much as possible and I'll be pleased to merge it in mainline, or it uses it's own sysfs files no other driver uses and no user-space application supports, that makes it essentially useless and I can't take it in mainline.

I'm not asking that you remove features from the driver because they just don't fit in the interface (as seems to be the case of the prescaler thing), just that you use the standard names where they exist, even if the name itself doesn't match the physical reality.

Am Montag, 12. M?rz 2007 14:50 schrieb Jean Delvare:

Quoted 1 line

We have fan1_input and fan1_target for this.

Documentation/hwmon/sysfs-interface doesn't mention fan1_target. I tried to follow that specification. I'll rename "speed" accordingly.

Quoted 5 lines

Quoted 2 lines

fan1_min_speed,
fan1_max_speed: (rw) Planned/defined operating range in RPM

We have fan1_min for the former. We could have fan1_max for the latter, but no known chip actually supports upper limit for fan speeds.

My intention was not to set upper/lower limits but to allow the driver to calculate things like "divider" or "counttime". If I make "divider" or "counttime" (or whatever algorithm a chip uses) sysfs attributes, then a user needs data sheet and calculator to find appropriate values for them. Min/max values are either known or can simply be found by experimenting and the rest is calculated by the driver, not by the user.

Quoted 4 lines

Quoted 1 line

fan1_mode: (rw) on, off, closed_loop, open_loop, ...

This is exactly what pwm1_enable does. The name could have been chosen better, granted.

I don't understand Documentation/hwmon/sysfs-interface in this point. Which values for pwm1_enable correspond to on, off, closed_loop, and open_loop? Can I define new ones? The text mentions "2+"...

I'll do that if possible. After all, I have two registers (prescaler and countime) that are affected. As I'm limited to powers of two by the specification, it might not fit. If that is the case, I'll leave that attribute away and we have to live with a fixed value given by a module parameter.

All right. New version will follow soon.

Notice that the counttime is also only expressed as powers of 2.

Quoted 1 line

All right. New version will follow soon.

Great, thanks.

Here it is: The driver creates only standard sysfs files now. I left the module insmod parameters in place, though. This allows users to initialize ALL registers if their BIOS doesn't do so. I hope you do not object.

sysfs files follow the specification as close as possible. I used pwm1_enable=3 for "fan off". There is no other possibility to switch the fan off, setting pwm1_enable to 1 and fan1_target to 0 doesn't turn it off completely.

You mentioned problems compiling my code. Sorry, I can't reproduce this here. It compiles without any warnings. If you still notice problems, please tell me.

Quoted 1 line

I guess you didn't try with debug enabled...

Index: linux-2.6.20-cpx/Documentation/hwmon/max6650
=================================> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20-cpx/Documentation/hwmon/max6650 2007-03-13 12:46:07.000000000 +0100
...)
+voltage_12V: 0=5V fan, 1V fan, -1=don't change

Index: linux-2.6.20-cpx/drivers/hwmon/max6650.c
=================================> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20-cpx/drivers/hwmon/max6650.c 2007-03-13 12:36:44.000000000 +0100

(quoted diff) +121 -0

@@ -0,0 +1,746 @@
+/*
+ * max6650.c - Part of lm_sensors, Linux kernel modules for hardware
+ *             monitoring.
+ *
+ * Author: John Morris <john.morris at spirentcom.com>
+ * Copyright (c) 2003 Spirent Communications
+ *
+ * This module has only been tested with the MAX6650 chip. It should
+ * also work with the MAX6651, though with reduced functionality. It
+ * does not distinguish max6650 and max6651 chips.
+ *
+ * Tha datasheet was last seen at:
+ *
+ *        http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
+ *
+ * Ported to Linux 2.6 by Claus Gindhart <claus.gindhart at kontron.com>
+ * Modifications (C)2007 by Hans J. Koch <hjk at linutronix.de>:
+ *   - added insmod parameters
+ *   - made sysfs interface compatible with hwmon specifications
+ *   - general cleanup
+ *   - updated documentation
+ *
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+
+/*
+ * Addresses to scan. There are four disjoint possibilities, by pin config.
+ */
+
+static unsigned short normal_i2c[] = {0x1b, 0x1f, 0x48, 0x4b, I2C_CLIENT_END};
+
+/*
+ * Insmod parameters
+ */
+
+/* fan_voltage: 5=5V fan, 12V fan, -1=don't change */
+static int fan_voltage = -1;
+/* prescaler: Possible values are 1,2,4,8,16, or -1 for don't change */
+static int prescaler = -1;
+/* clock: The clock frequency of the chip the driver should assume */
+static int clock = 254000;
+/* mode: 0=on, 1=off, 2=closed loop, 3=open loop, -1=don't change */
+static int mode = -1;
+/* counttime: 0=0.25s, 1=0.5s, 2=1.0s, 3=2.0s, -1=don't change */
+static int counttime = -1;
+
+module_param(fan_voltage, int, S_IRUGO);
+module_param(prescaler, int, S_IRUGO);
+module_param(clock, int, S_IRUGO);
+module_param(mode, int, S_IRUGO);
+module_param(counttime, int, S_IRUGO);
+
+I2C_CLIENT_INSMOD_1(max6650);
+
+/*
+ * MAX 6650/6651 registers
+ */
+
+#define MAX6650_REG_SPEED       0x00
+#define MAX6650_REG_CONFIG      0x02
+#define MAX6650_REG_GPIO_DEF    0x04
+#define MAX6650_REG_DAC         0x06
+#define MAX6650_REG_ALARM_EN    0x08
+#define MAX6650_REG_ALARM       0x0A
+#define MAX6650_REG_TACH0       0x0C
+#define MAX6650_REG_TACH1       0x0E
+#define MAX6650_REG_TACH2       0x10
+#define MAX6650_REG_TACH3       0x12
+#define MAX6650_REG_GPIO_STAT   0x14
+#define MAX6650_REG_COUNT       0x16
+
+/*
+ * Config register bits
+ */
+
+#define MAX6650_CFG_V12           	0x08
+#define MAX6650_CFG_PRESCALER_MASK	0x07
+#define MAX6650_CFG_PRESCALER_2		0x01
+#define MAX6650_CFG_PRESCALER_4		0x02
+#define MAX6650_CFG_PRESCALER_8		0x03
+#define MAX6650_CFG_PRESCALER_16	0x04
+#define MAX6650_CFG_MODE_MASK           0x30
+#define MAX6650_CFG_MODE_ON             0x00
+#define MAX6650_CFG_MODE_OFF            0x10
+#define MAX6650_CFG_MODE_CLOSED_LOOP    0x20
+#define MAX6650_CFG_MODE_OPEN_LOOP      0x30
+#define MAX6650_COUNT_MASK		0x03
+
+/* Minimum and maximum values of the FAN-RPM */
+#define FAN_RPM_MIN 240
+#define FAN_RPM_MAX 30000
+
+#define DIV_FROM_REG(reg) (1 << (reg & 7))
+
+static int max6650_attach_adapter(struct i2c_adapter *adapter);
+static int max6650_detect(struct i2c_adapter *adapter, int address, int kind);
+static int max6650_init_client(struct i2c_client *client);
+static int max6650_detach_client(struct i2c_client *client);
+static struct max6650_data *max6650_update_device(struct device *dev);
+

(quoted diff) +63 -0

+};
+
+/*
+ * Client data (each client gets its own)
+ */
+
+struct max6650_data
+{
+	struct i2c_client client;
+	struct class_device *class_dev;
+	struct mutex update_lock;
+	char valid; /* zero until following fields are valid */
+	unsigned long last_updated; /* in jiffies */
+
+	/* register values */
+	u8 speed;
+	u8 config;
+	u8 tach[4];
+	u8 count;
+};
+
+static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
+		       char *buf, int nr)
+{
+	struct max6650_data *data = max6650_update_device(dev);
+
+	/*
+	* Calculation details:
+	*
+	* Each tachometer counts over an interval given by the "count"
+	* register (0.25, 0.5, 1 or 2 seconds). This module assumes
+	* that the fans produce two pulses per revolution (this seems
+	* to be the most common).
+	*/
+
+	int rpm = ((data->tach[nr] * 120) / DIV_FROM_REG(data->count));
+	return sprintf(buf, "%d\n", rpm);
+}
+
+static ssize_t get_fan1(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,0);
+}
+
+static ssize_t get_fan2(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,1);
+}
+
+static ssize_t get_fan3(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,2);
+}
+
+static ssize_t get_fan4(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	return get_fan(dev,devattr,buf,3);
+}
+

(quoted diff) +48 -0

+ *
+ * 2) The prescaler (low three bits of the config register) has already
+ *    been set to an appropriate value.
+ *
+ * The relevant equations are given on pages 21 and 22 of the datasheet.
+ *
+ * From the datasheet, the relevant equation when in regulation is:
+ *
+ *    [fCLK / (128 x (KTACH + 1))] = 2 x FanSpeed / KSCALE
+ *
+ * where:
+ *
+ *    fCLK is the oscillator frequency (either the 254kHz internal
+ *         oscillator or the externally applied clock)
+ *
+ *    KTACH is the value in the speed register
+ *
+ *    FanSpeed is the speed of the fan in rps
+ *
+ *    KSCALE is the prescaler value (1, 2, 4, 8, or 16)
+ *
+ * When reading, we need to solve for FanSpeed. When writing, we need to
+ * solve for KTACH.
+ *
+ * Note: this tachometer is completely separate from the tachometers
+ * used to measure the fan speeds. Only one fan's speed (fan1) is
+ * controlled.
+ */
+
+static ssize_t get_target(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	struct max6650_data *data = max6650_update_device(dev);
+	int kscale, ktach, rpm;
+
+	/*
+	* Use the datasheet equation:
+	*
+	*    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
+	*
+	* then multiply by 60 to give rpm.
+	*/
+
+	kscale = DIV_FROM_REG(data->config);
+	ktach  = data->speed;
+	rpm    = 60 * kscale * clock / (256 * (ktach + 1));
+	return sprintf(buf, "%d\n", rpm);
+}

(quoted diff) +5 -0

+
+	/*
+	* Divide the required speed by 60 to get from rpm to rps, then
+	* use the datasheet equation:
+	*

(quoted diff) +10 -0

+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+/*
+ * Get/Set controller mode:
+ * Possible values:
+ * 0 = Fan always full on

(quoted diff) +25 -0

+	int mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
+
+	switch (mode) {
+	case 1:
+		mode = 3;
+		break;
+	case 3:
+		mode = 1;
+	}
+
+	return sprintf(buf, "%d\n", mode);
+}
+
+static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);
+	int mode = simple_strtoul(buf, NULL, 10);
+
+	if ((mode < 0)||(mode > 3))
+		return count;
+
+	switch (mode) {
+	case 1:

(quoted diff) +12 -0

+
+	return count;
+}
+
+/*
+ * Read/write functions for fan1_div sysfs file. The MAX6650 has no such
+ * divider. We handle this by converting between divider and counttime:
+ *
+ * (counttime = k) <=> (divider = 2^k), k = 0, 1, 2, or 3
+ *
+ * Lower values of k allow to connect a faster fan without the risk of
+ * counter overflow. The price is lower resolution. You can also set counttime

(quoted diff) +14 -0

+}
+
+static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);
+	int div = simple_strtoul(buf, NULL, 10);
+
+	switch (div) {
+	case 1:
+		data->count = 0;
+		break;
+	case 2:

(quoted diff) +10 -0

+	return count;
+}
+
+static DEVICE_ATTR(fan1_input, S_IRUGO, get_fan1, NULL);
+static DEVICE_ATTR(fan2_input, S_IRUGO, get_fan2, NULL);
+static DEVICE_ATTR(fan3_input, S_IRUGO, get_fan3, NULL);
+static DEVICE_ATTR(fan4_input, S_IRUGO, get_fan4, NULL);
+static DEVICE_ATTR(fan1_target, S_IWUSR | S_IRUGO, get_target, set_target);
+static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO, get_enable, set_enable);
+static DEVICE_ATTR(fan1_div, S_IWUSR | S_IRUGO, get_div, set_div);

(quoted diff) +4 -0

+};
+
+static struct attribute_group max6650_attr_grp = {
+	.attrs = max6650_attrs,

(quoted diff) +8 -0

+
+	if (!(adapter->class & I2C_CLASS_HWMON)) {
+		dev_dbg(adapter->dev,
+			"FATAL: max6650_attach_adapter class HWMON not set\n");
+		return 0;
+	}
+
+	return i2c_probe(adapter, &addr_data, max6650_detect);

(quoted diff) +5 -0

+
+	/*
+	 * Now we do the remaining detection. A negative kind means that
+	 * the driver was loaded with no force parameter (default), so we
+	 * must both detect and identify the chip (actually there is only

(quoted diff) +13 -0

+	mutex_init(&data->update_lock);
+
+	/*
+	 * Tell the I2C layer a new client has arrived.
+	 */
+
+	if ((err = i2c_attach_client(new_client))) {
+		dev_dbg(adapter->dev, "max6650.o: Failed attaching client.\n");
+		goto err_free;
+	}
+
+	/*
+	 * Initialize the max6650 chip

(quoted diff) +12 -0

+
+	err = sysfs_create_group(&new_client->dev.kobj, &max6650_attr_grp);
+	if (!err)
+		return 0;
+
+	dev_err(&new_client->dev, "error creating sysfs files (%d)\n", err);
+	hwmon_device_unregister(data->class_dev);
+err_detach:
+	i2c_detach_client(new_client);
+err_free:
+	kfree(data);
+	return err;

(quoted diff) +45 -0

+	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+
+	if (config < 0) {
+		dev_err(&client->dev, "Error reading config, aborting.\n");
+		goto out;
+	}
+
+	switch (fan_voltage) {
+		case -1:
+			break;
+		case  5:
+			config &= ~MAX6650_CFG_V12;
+			break;
+		case 12:
+			config |= MAX6650_CFG_V12;
+			break;
+		default:
+			dev_err(&client->dev,
+				"illegal value for fan_voltage (%d)\n",
+				fan_voltage);
+	}
+
+	dev_info(&client->dev, "Fan voltage is set to %dV.\n",
+		 (config & MAX6650_CFG_V12) ? 12 : 5);
+
+	switch (prescaler) {
+		case -1:
+			break;
+		case  1:
+			config &= ~MAX6650_CFG_PRESCALER_MASK;
+			break;
+		case  2:
+			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)
+				  | MAX6650_CFG_PRESCALER_2 );
+			break;
+		case  4:
+			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)
+				  | MAX6650_CFG_PRESCALER_4 );
+			break;
+		case  8:
+			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)
+				  | MAX6650_CFG_PRESCALER_8 );
+			break;
+		case 16:
+			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)

(quoted diff) +18 -0

+
+	switch (mode) {
+		case -1:
+			break;
+		case  0:
+		case  1:
+		case  2:
+		case  3:
+			config = (  (config & ~MAX6650_CFG_MODE_MASK)
+				  | (mode << 4) );
+			break;
+		default:
+			dev_err(&client->dev,
+				 "illegal value for mode (%d)\n", mode);
+	}
+
+	dev_info(&client->dev, "Mode is set to %s.\n",
+		mode_strings[(config & MAX6650_CFG_MODE_MASK) >> 4]);

(quoted diff) +50 -0

+	if (countreg < 0) {
+		dev_err(&client->dev, "Error reading count time, aborting.\n");
+		goto out;
+	}
+
+	countreg &= MAX6650_COUNT_MASK;
+	data->count = countreg;
+
+	if ((counttime >= 0) && (counttime <= 3)) {
+		if (i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT,
+					      counttime)) {
+			dev_err(&client->dev,
+				"Error writing counttime, aborting.\n");
+			goto out;
+		}
+		data->count = counttime;
+	}
+	else {
+		if (counttime != -1) {
+			dev_err(&client->dev,
+				"illegal value for counttime (%d)\n",
+				counttime);
+			goto out;
+		}
+	}
+
+	err = 0;
+	dev_info(&client->dev, "Count time is set to %d (%d msec).\n",
+		 data->count, (1 << data->count) * 250);
+out:
+	mutex_unlock(&data->update_lock);
+	return err;
+}
+
+static const u8 tach_reg[] > +{
+	MAX6650_REG_TACH0,
+	MAX6650_REG_TACH1,
+	MAX6650_REG_TACH2,
+	MAX6650_REG_TACH3,
+};
+
+static struct max6650_data *max6650_update_device(struct device *dev)
+{
+	int i;
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {

(quoted diff) +10 -0

+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static int __init sensors_max6650_init(void)

So please get rid of mode and counttime.

About module parameters: please don't initialize them to -1 if you don't need to. The compiler can optimize static globals which default to zero nicely.

Quoted 5 lines

sysfs files follow the specification as close as possible. I used pwm1_enable=3 for "fan off". There is no other possibility to switch the fan off, setting pwm1_enable to 1 and fan1_target to 0 doesn't turn it off completely.

I don't understand. For one thing, if you know how to turn the fan off, then you could do it when fan1_target is set to 0, it's just a matter of adding a few more lines of code. For another, fan1_target is related to the closed loop mode (pwm1_enable=2) while the fan off mode would generally be implemented as part of the open loop mode: pwm1_enable=1 and pwm1=0. But I see that you don't have a pwm1 file, so... How does the open loop mode work at all?

Quoted 4 lines

You mentioned problems compiling my code. Sorry, I can't reproduce this here. It compiles without any warnings. If you still notice problems, please tell me.

The problem is still there, and I explained why. Quoting myself:

Quoted 1 line

Quoted 1 line

I guess you didn't try with debug enabled...

The driver compiles fine with CONFIG_HWMON_DEBUG_CHIP=n, but fails to compile with CONFIG_HWMON_DEBUG_CHIP=y. Try it yourself. It's probably trivial to fix, too.

I see that you still did not fix the problems Corentin Labbe reported after his second review [1]. Please do. When someone takes the time to review your code - and it's not exactly a fun job - the least you can do is take his/her findings into account for the next iteration of the driver.

Quoted 1 line

+voltage_12V: 0=5V fan, 1V fan, -1=don't change

This is no longer true as you changed the convention.

Review

Quoted 6 lines

(quoted diff) +6 -0

+static struct i2c_driver max6650_driver = {
+	.driver = {
+		.name	= "max6650",
+	},
+	.attach_adapter = max6650_attach_adapter,
+	.detach_client  = max6650_detach_client

Please add a comma at the end of this line.

Quoted 5 lines

+ *
+ * Assumptions:
+ *
+ * 1) The MAX6650/1 is running from its internal 254kHz clock (perhaps
+ * this should be made a module parameter).

Well, it is now.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	dev_dbg(dev, "%s called, rpm=%d\n", __FUNCTION__, rpm);

I read lately that the standard notation is __func__ not __FUNCTION. Not that this debug line looks particularly interesting anyway.

Review

Quoted 5 lines

(quoted diff) +4 -0

+
+	mutex_lock(&data->update_lock);
+
+	rpm = SENSORS_LIMIT(rpm, FAN_RPM_MIN, FAN_RPM_MAX);

You don't need to hold the lock for this instruction.

Review

Quoted 1 line

(quoted diff) +1 -0

+	data->speed  = ktach;

Doubled space.

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED,  data->speed);

Doubled space.

Review

Quoted 5 lines

+static ssize_t get_enable(struct device *dev, struct device_attribute
*devattr, + char *buf)

(quoted diff) +3 -0

+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);

You are supposed to call the update function here, otherwise data->config may not be valid.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);

You shouldn't touch data->config without holding data->update_lock. Also, you should read the value of MAX6650_REG_CONFIG from the device before changing it, as you are not writing all the bits.

Review

Quoted 5 lines

+static ssize_t get_div(struct device *dev, struct device_attribute
*devattr, + char *buf)

(quoted diff) +3 -0

+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);

Here again you forgot to call the update function.

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+	return sprintf(buf, "%d\n", 1 << data->count);

Why don't you use DIV_FROM_REG?

Quoted 21 lines

(quoted diff) +3 -0

+}
+
+static ssize_t set_div(struct device *dev, struct device_attribute

*devattr, + const char *buf, size_t count)

(quoted diff) +16 -0

+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);
+	int div = simple_strtoul(buf, NULL, 10);
+
+	switch (div) {
+	case 1:
+		data->count = 0;
+		break;
+	case 2:
+		data->count = 1;
+		break;
+	case 4:
+		data->count = 2;
+		break;
+	case 8:

Review

Quoted 4 lines

(quoted diff) +4 -0

+		data->count = 3;
+		break;
+	default:
+		return count;

This returns with success, while you were not able to set the divider the user requested! Better return an error (-EINVAL) possibly with a message in the logs.

Review

Quoted 4 lines

(quoted diff) +3 -0

+	}
+
+	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);

Again the while section should be protected by taking the data->update_lock mutex.

Review

Quoted 9 lines

(quoted diff) +9 -0

+static struct attribute *max6650_attrs[] = {
+	&dev_attr_fan1_input.attr,
+	&dev_attr_fan2_input.attr,
+	&dev_attr_fan3_input.attr,
+	&dev_attr_fan4_input.attr,
+	&dev_attr_fan1_target.attr,
+	&dev_attr_pwm1_enable.attr,
+	&dev_attr_fan1_div.attr,
+	NULL,

No comma at the end of this one - it cannot be extended by definition.

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+static int max6650_attach_adapter(struct i2c_adapter *adapter)
+{
+	dev_dbg(adapter->dev, "max6650_attach_adapter called for %s\n",
+		(char*)&adapter->name);

This is bogus.

Review

Quoted 3 lines

+static int max6650_detect(struct i2c_adapter *adapter, int address, int kind) +{

(quoted diff) +1 -0

+	struct i2c_client *new_client;

Please rename this to just "client". I know many other drivers do that, but it sucks. It's pretty obvious that the client is new.

Review

Quoted 6 lines

(quoted diff) +5 -0

+	struct max6650_data *data;
+	int err = -ENODEV;
+	const char *name = "";
+
+	dev_dbg(adapter->dev, "max6650_detect called, kind = %d\n",kind);

Missing space after comma.

Review

Quoted 4 lines

(quoted diff) +3 -0

+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_dbg(adapter->dev, "MAX6650: I2C bus doesn't support byte"

Missing space at the end of the first part of the string.

Review

Quoted 7 lines

(quoted diff) +6 -0

+			"read mode, skipping.\n");
+		return 0;
+	}
+
+	if (!(data = kzalloc(sizeof(struct max6650_data), GFP_KERNEL))) {
+		printk("MAX6650: Out of memory in max6650_detect "

Please either switch to dev_err(&adapter->dev, ...) or at least add KERN_ERR before the string.

Review

Quoted 2 lines

(quoted diff) +1 -0

+			"(new_client).\n");

You are actually trying to allocate "data", not "new_client". But given that there's a single allocation in the whole driver anyway...

Review

Quoted 9 lines

(quoted diff) +8 -0

+		return -ENOMEM;
+	}
+
+	/*
+	 * The common I2C client data is placed right before the
+	 * max6650-specific data. The max6650-specific data is pointed to by
+	 * the data field from the I2C client data.
+	 */

Please rip off this useless comment.

Review

Quoted 8 lines

(quoted diff) +7 -0

+
+	new_client = &data->client;
+	i2c_set_clientdata(new_client, data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &max6650_driver;
+	new_client->flags = 0;

You can omit this one, the kzalloc above already set the flags to 0.

Quoted 10 lines

(quoted diff) +9 -0

+
+	/*
+	 * Now we do the remaining detection. A negative kind means that
+	 * the driver was loaded with no force parameter (default), so we
+	 * must both detect and identify the chip (actually there is only
+	 * one possible kind of chip for now, max6650). A zero kind means that
+	 * the driver was loaded with the force parameter, the detection
+	 * step shall be skipped. A positive kind means that the driver
+	 * was loaded with the force parameter and a given kind of chip is

Review

Quoted 6 lines

(quoted diff) +6 -0

+	 * requested, so both the detection and the identification steps
+	 * are skipped.
+	 *
+	 * Currently I can find no way to distinguish between a MAX6650 and
+	 * a MAX6651. This driver has only been tried on the latter.
+	 */

I thought it was on the former?

Review

Quoted 10 lines

(quoted diff) +4 -0

+
+	if ((kind < 0)&&
+	   (  (i2c_smbus_read_byte_data(new_client,MAX6650_REG_CONFIG) & 0xC0)
+	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_GPIO_STAT)&

0xE0) + ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM_EN)
& 0xE0) + ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM) &
0xE0) + ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_COUNT) &
0xFC))) + {

(quoted diff) +1 -0

+		dev_dbg(adapter->dev, "max6650.o: max6650 detection failed"

Missing space at the end of the first half, and this is not max6650.o. Please format all your debug messages the same way, it's confusing otherwise.

Review

Quoted 5 lines

(quoted diff) +4 -0

+			"at 0x%02x.\n", address);
+
+		goto err_free;
+        }

Indentation problem, the line above uses spaces instead of a tabulation.

Review

Quoted 8 lines

(quoted diff) +7 -0

+
+	if (kind <= 0)
+		kind = max6650;
+
+	if (kind = max6650) {
+		name = "max6650";
+		printk("MAX6650: Chip found at 0x%02x.\n", address);

Missing log level.

Review

Quoted 4 lines

(quoted diff) +3 -0

+	}
+	else {
+		printk("MAX6650: Unsupported chip.\n");

Missing log level.

Review

Quoted 3 lines

(quoted diff) +2 -0

+		goto err_free;
+	}

Given that this just cannot happen, and your driver supports only one kind anyway, you could simply hardcode the kind and name.

Review

Quoted 9 lines

(quoted diff) +8 -0

+
+	/*
+	 * OK, we got a valid chip so we can fill in the remaining client
+	 * fields.
+	 */
+
+	strlcpy(new_client->name, name, I2C_NAME_SIZE);
+	data->valid = 0;

This one too can be omitted.

Quoted 18 lines

(quoted diff) +17 -0

+	mutex_init(&data->update_lock);
+
+	/*
+	 * Tell the I2C layer a new client has arrived.
+	 */
+
+	if ((err = i2c_attach_client(new_client))) {
+		dev_dbg(adapter->dev, "max6650.o: Failed attaching client.\n");
+		goto err_free;
+	}
+
+	/*
+	 * Initialize the max6650 chip
+	 */
+	if (max6650_init_client(new_client))
+		goto err_detach;
+

Review

Quoted 6 lines

(quoted diff) +6 -0

+	/* Register sysfs hooks */
+	data->class_dev = hwmon_device_register(&new_client->dev);
+	if (IS_ERR(data->class_dev)) {
+		err = PTR_ERR(data->class_dev);
+		goto err_detach;
+	}

Please create the files first, and register the class only after that.

Review

Quoted 4 lines

(quoted diff) +4 -0

+
+	hwmon_device_unregister(data->class_dev);
+	err = i2c_detach_client(client);
+	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);

You should remove the files before detaching the client.

Review

Quoted 2 lines

(quoted diff) +1 -0

+	kfree(data);

Do not free the memory if you failed to detach the client!

Review

Quoted 11 lines

(quoted diff) +10 -0

+	return err;
+}
+
+static int max6650_init_client(struct i2c_client *client)
+{
+	struct max6650_data *data = i2c_get_clientdata(client);
+	const char* mode_strings[4] = {"full-on", "always-off",
+					"closed-loop", "open-loop"};
+	int config, countreg;
+	int err = -ENODEV;

The only errors that can happen here are I/O errors, so that would be -EIO.

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+	mutex_lock(&data->update_lock);

You don't really need to take the mutex here - there's nobody else who can access your data at this point.

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+	dev_info(&client->dev, "Prescaler is set to %d.\n",
+		 DIV_FROM_REG(config) );

This is confusing, you're using DIV_FROM_REG for both the prescaler and the counttime. It works more or less by accident...

Quoted 13 lines

(quoted diff) +13 -0

+static struct max6650_data *max6650_update_device(struct device *dev)
+{
+	int i;
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
+		data->speed = i2c_smbus_read_byte_data(client,
+			 			       MAX6650_REG_SPEED);
+		data->config = i2c_smbus_read_byte_data(client,
+							MAX6650_REG_CONFIG);

Review

Quoted 6 lines

(quoted diff) +6 -0

+		for (i = 0; i < 4; i++) {
+			data->tach[i] = i2c_smbus_read_byte_data(client,
+								 tach_reg[i]);
+		}
+		data->count = i2c_smbus_read_byte_data (client,
+							MAX6650_REG_COUNT);

Extra space before opening parenthesis.

Review

Quoted 3 lines

(quoted diff) +3 -0

+}
+
+MODULE_AUTHOR("john.morris at spirentcom.com");

This is more or less your driver now...

BTW (I can't remember if I told
you already) feel free to add yourself to MAINTAINERS as the maintainer for this new driver.

Quoted 7 lines

I don't understand. For one thing, if you know how to turn the fan off, then you could do it when fan1_target is set to 0, it's just a matter of adding a few more lines of code. For another, fan1_target is related to the closed loop mode (pwm1_enable=2) while the fan off mode would generally be implemented as part of the open loop mode: pwm1_enable=1 and pwm1=0. But I see that you don't have a pwm1 file, so... How does the open loop mode work at all?

Open loop mode just sets the ouput voltage according to a DAC value (0..255). If someone needs a defined speed, he's supposed to do the regulator in software. It is not influenced by the speed register used in closed loop mode. I added a pwm1 file to read/write that DAC value. After doing that, I noticed the fan can be brought to full/zero speed by setting pwm1 to 0 or 255, respectively. That's OK for me.
For pwm1_enable, I only use the well defined values now: 0=off, 1=open loop, 2=closed loop. If I find the chip in "full on" mode at load time, I change mode to open loop and set it to full speed. I hope this always works and won't set somebodies board on fire :-)

BTW, meanwhile I'm a bit confused about what pwm1_enable=0 means. Is it "fan off" or "PWM off, fan full on"? At the moment, I implemented "fan off", but I can easily change that. The other way round would be slightly simpler.

/* fan_voltage: 5=5V fan, 12V fan, 0=don't change */ static int fan_voltage = 0;
/* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */ static int prescaler = 0;

static int max6650_attach_adapter(struct i2c_adapter *adapter)

{
	if (!(adapter->class & I2C_CLASS_HWMON)) {
		dev_err(&adapter->dev,
			"FATAL: max6650_attach_adapter class HWMON not set\n");
		return 0;
	}

	return i2c_probe(adapter, &addr_data, max6650_detect);
}

	if ((kind < 0)&&
	   (  (i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG) & 0xC0)
	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_GPIO_STAT) & 0xE0)
	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN) & 0xE0)
	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM) & 0xE0)
	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT) & 0xFC)))
	{
		dev_dbg(&adapter->dev,
			"max6650: detection failed at 0x%02x.\n", address);
		goto err_free;
	}

static const u8 tach_reg[] > {
	MAX6650_REG_TACH0,
	MAX6650_REG_TACH1,
	MAX6650_REG_TACH2,
	MAX6650_REG_TACH3,
};

pwm1_enable=0 means fan full on, so you'll have to update your code and documentation a bit, sorry.

You should not need to change anything at module load time.

Quoted 5 lines

/* fan_voltage: 5=5V fan, 12V fan, 0=don't change */ static int fan_voltage = 0;
/* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */ static int prescaler = 0;

The trick is to _not_ initialize these variables at all. The compiler will put them in a special section and will zero them all at once automatically.

Quoted 9 lines

static int max6650_attach_adapter(struct i2c_adapter *adapter)

{
	if (!(adapter->class & I2C_CLASS_HWMON)) {
		dev_err(&adapter->dev,
			"FATAL: max6650_attach_adapter class HWMON not set\n");
		return 0;
	}

Quoted 2 lines

	return i2c_probe(adapter, &addr_data, max6650_detect);
}

Please make it a dev_dbg(). There are more than one I2C bus on most systems, and usually only one with I2C_CLASS_HWMON set, on purpose. You don't want to generate an error message in the logs for every other I2C bus.

Quoted 9 lines

	if ((kind < 0)&&
	   (  (i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG) & 0xC0)

	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_GPIO_STAT) & 0xE0)
	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN) & 0xE0)
	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM) & 0xE0)
	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT) & 0xFC)))

Quoted 5 lines

	{
		dev_dbg(&adapter->dev,
			"max6650: detection failed at 0x%02x.\n", address);
		goto err_free;
	}

Opening curly brace is misplaced.

Quoted 7 lines

static const u8 tach_reg[] > > {
	MAX6650_REG_TACH0,
	MAX6650_REG_TACH1,
	MAX6650_REG_TACH2,
	MAX6650_REG_TACH3,
};

And same here.

Other than that I am very happy with this latest version. Great job! Thanks for your patience, I know it's always a bit frustrating when your code works well enough for yourself and you are still told to make many changes before it is acceptable upstream.

Can you please test the user-space support now? I suspect that we need to update libsensors at least to match the name change of the "speed" file. We're about to release lm_sensors 2.10.3 (in a few days now) and I'd like it to support your new driver properly.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

/* fan_voltage: 5=5V fan, 12V fan, 0=don't change */ static int fan_voltage = 0;
/* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */ static int prescaler = 0;

The trick is to _not_ initialize these variables at all. The compiler will put them in a special section and will zero them all at once automatically.

Quoted 1 line

Sounds dangerous. But I trust you :-)

It's one of the things which actually work reliable in gcc :)

tglx

Quoted 8 lines

Quoted 4 lines

/* fan_voltage: 5=5V fan, 12V fan, 0=don't change */ static int fan_voltage = 0;
/* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */ static int prescaler = 0;

The trick is to _not_ initialize these variables at all. The compiler will put them in a special section and will zero them all at once automatically.

Sounds dangerous. But I trust you :-)

Am Freitag, 16. M?rz 2007 22:19 schrieb Thomas Gleixner:

Quoted 10 lines · Hans-J?rgen Koch

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

/* fan_voltage: 5=5V fan, 12V fan, 0=don't change */ static int fan_voltage = 0;
/* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */ static int prescaler = 0;

The trick is to _not_ initialize these variables at all. The compiler will put them in a special section and will zero them all at once automatically.

Quoted 5 lines

Quoted 1 line

Sounds dangerous. But I trust you :-)

It's one of the things which actually work reliable in gcc :)

tglx

Fine, I'll remember that. But I think readability is better if you explicitly initialize to zero. Is that optimization really worth it?

Well, I really appreciate good code quality. If this is the price, I'm willing to pay it. Actually, I thank you for helping me so much.

Quoted 4 lines

Can you please test the user-space support now? I suspect that we need to update libsensors at least to match the name change of the "speed" file. We're about to release lm_sensors 2.10.3 (in a few days now) and I'd like it to support your new driver properly.

Yes, but not so soon, sorry. I'm not at home the whole next week and will not have any sensors hardware available. After that, I might find some time to have a look. I have an LM93 driver almost ready, so that'll be the next thing I'll keep you busy with :-) In the course of that development I can have a short look at user space stuff, too.

Patch accepted, will be in next -mm, and from there will go to 2.6.22. Thanks :)

If you start working on the lm93 driver, make sure to search the mailing list archives first. I remember that the driver was already ported to Linux 2.6, but never reviewed. This was some times ago though, many things changed in the kernel meanwhile, so it's up to you whether you want to start from an old port or from scratch.

Beware, the
lm93 driver is a fat beast.