[lm-sensors] [PATCH] Add MAX6650 support

[corentin.labbe@geomatys.fr (corentin.labbe)]

Hans-J?rgen Koch a ?crit :
> Am Sonntag, 11. Februar 2007 17:22 schrieb Hans-J?rgen Koch:
>> Am Sonntag 11 Februar 2007 16:44 schrieb corentin.labbe:
>>> 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
>>> +static void max6650_init_client(struct i2c_client *client)
>>> +{
>>> +	/* Nothing to do here - assume the BIOS has initialized the chip
>>> */ +	while(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
>> 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
> @@ -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
> @@ -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
> @@ -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
> @@ -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


hello

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:
 > +  * Maxim 6650 / 6651
 > +    Prefix: 'w83792d'

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

6550 not 6650
 > +  * Maxim 6650 / 6651
 > +    Prefix: 'w83792d'