Hi Riku: Thanks for your patience & continuing to resend this driver until I got to it. Keep in mind, one way to get your work reviewed faster is to arrange to do some reviews for others. That convention is starting to work nicely around here. Overall, this is pretty clean for a first attempt; nice work. Comments are inline... * Riku Voipio <riku.voipio at movial.fi> [2007-07-19 17:58:32 +0300]: > Following Krzysztof Helt's advices (thanks!), converted RPM_TO_REG > conversion to static inline function, use SENSORS_LIMIT() where applicable, > really read limits registers only every 60sec and coding some style fixes. > > >From 3855e184a0bc4095b0191795c1618464a31200b2 Mon Sep 17 00:00:00 2001 > From: Riku Voipio <riku.voipio at movial.fi> > Date: Fri, 29 Jun 2007 01:21:03 +0300 > Subject: [PATCH] Add f75375s driver > > Signed-off-by: Riku Voipio <riku.voipio at movial.fi> > --- > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/f75375s.c | 686 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 697 insertions(+), 0 deletions(-) > create mode 100644 drivers/hwmon/f75375s.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 13eea47..9138604 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -199,6 +199,16 @@ config SENSORS_F71805F > This driver can also be built as a module. If so, the module > will be called f71805f. > > +config SENSORS_F75375S > + tristate "Fintek F75375S/SP and F75373"; > + depends on I2C && EXPERIMENTAL > + help > + If you say yes here you get support for hardware monitoring > + features of the Fintek F75375S/SP and F75373 > + > + This driver can also be built as a module. If so, the module > + will be called f75375s. > + > config SENSORS_FSCHER > tristate "FSC Hermes" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index cfaf338..689131a 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -27,6 +27,7 @@ obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o > obj-$(CONFIG_SENSORS_DS1621) += ds1621.o > obj-$(CONFIG_SENSORS_F71805F) += f71805f.o > +obj-$(CONFIG_SENSORS_F75375S) += f75375s.o > obj-$(CONFIG_SENSORS_FSCHER) += fscher.o > obj-$(CONFIG_SENSORS_FSCPOS) += fscpos.o > obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o > diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c > new file mode 100644 > index 0000000..20a5860 > --- /dev/null > +++ b/drivers/hwmon/f75375s.c > @@ -0,0 +1,686 @@ > +/* > + * f75375s.c - driver for the Fintek F75375/SP and F75373 > + * hardware monitoring features > + * Copyright (C) 2006-2007 Riku Voipio <riku.voipio at movial.fi> > + * > + * Datasheets available at: > + * > + * f75375: > + * http://www.fintek.com.tw/files/productfiles/2005111152950.pdf > + * > + * f75373: > + * http://www.fintek.com.tw/files/productfiles/2005111153128.pdf > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + * > + */ > + > +#include <linux/module.h> > +#include <linux/jiffies.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/i2c.h> > +#include <linux/err.h> > +#include <linux/mutex.h> > + > +/* Addresses to scan */ > +static unsigned short normal_i2c[] = { 0x2d, 0x2e, I2C_CLIENT_END }; > + > +/* Insmod parameters */ > +I2C_CLIENT_INSMOD_1(f75375); I think you want _INSMOD_2 with f75373. > + > +/* Fintek F75375 registers */ > +#define F75375_REG_CONFIG0 0x0 > +#define F75375_REG_CONFIG1 0x1 > +#define F75375_REG_CONFIG2 0x2 > +#define F75375_REG_CONFIG3 0x3 > +#define F75375_REG_ADDR 0x4 > +#define F75375_REG_INTR 0x31 > +#define F75375_CHIP_ID 0x5A > +#define F75375_REG_VERSION 0x5C > +#define F75375_REG_VENDOR 0x5D > +#define F75375_REG_FAN_TIMER 0x60 > + > +#define F75375_REG_VOLT(nr) (0x10 + (nr)) > +#define F75375_REG_VOLT_HIGH(nr) (0x20 + (nr) * 2) > +#define F75375_REG_VOLT_LOW(nr) (0x21 + (nr) * 2) > + > +#define F75375_REG_TEMP(nr) (0x14 + (nr)) > +#define F75375_REG_TEMP_HIGH(nr) (0x28 + (nr) * 2) > +#define F75375_REG_TEMP_HYST(nr) (0x29 + (nr) * 2) > + > +#define F75375_REG_FAN(nr) (0x16 + (nr) * 2) > +#define F75375_REG_FAN_MIN(nr) (0x2C + (nr) * 2) > +#define F75375_REG_FAN_FULL(nr) (0x70 + (nr) * 0x10) > +#define F75375_REG_FAN_PWM_DUTY(nr) (0x76 + (nr) * 0x10) > +#define F75375_REG_FAN_PWM_CLOCK(nr) (0x7D + (nr) * 0x10) > + > +#define F75375_REG_FAN_EXP(nr) (0x74 + (nr) * 0x10) > +#define F75375_REG_FAN_B_TEMP(nr, step) ((0xA0 + (nr) * 0x10) + (step)) > +#define F75375_REG_FAN_B_SPEED(nr, step) \ > + ((0xA5 + (nr) * 0x10) + (step) * 2) > + > +#define F75375_REG_PWM1_RAISE_DUTY 0x69 > +#define F75375_REG_PWM2_RAISE_DUTY 0x6A > +#define F75375_REG_PWM1_DROP_DUTY 0x6B > +#define F75375_REG_PWM2_DROP_DUTY 0x6C > + > +#define FAN_CTRL_LINEAR(nr) (4 + nr) > +#define FAN_CTRL_MODE(nr) (5 + (nr * 2)) > + Needs parens around nr. > +enum kinds {F75375, F75373}; This is not necessary... the _INSMOD_2 macro already does this for you (albeit with lowercase f, as that is what you put there - so you'll need to change those elsewhere as well). > + > +/* > + * Data structures and manipulation thereof > + */ > + > +struct f75375_data { > + unsigned short addr; > + struct i2c_client client; > + struct class_device *class_dev; > + > + const char *name; > + enum kinds kind; Again, just "int kind" is enough here. > + struct mutex update_lock; /* protect register access */ > + char valid; > + unsigned long last_updated; /* In jiffies */ > + unsigned long last_limits; /* In jiffies */ > + > + /* Register values */ > + u8 in[4]; > + u8 in_max[4]; > + u8 in_min[4]; > + u16 fan[2]; > + u16 fan_min[2]; > + u16 fan_full[2]; > + u16 fan_exp[2]; > + u8 fan_timer; > + u8 pwm[2]; > + u8 pwm_mode[2]; > + u8 pwm_enable[2]; > + u8 temp[2]; > + s8 temp_high[2]; > + s8 temp_max_hyst[2]; > +}; > + > +static int f75375_attach_adapter(struct i2c_adapter *adapter); > +static int f75375_detect(struct i2c_adapter *adapter, int address, int kind); > +static int f75375_detach_client(struct i2c_client *client); > + > +static struct i2c_driver f75375_driver = { > + .driver = { > + .name = "f75375", > + }, > + .attach_adapter = f75375_attach_adapter, > + .detach_client = f75375_detach_client, > +}; > + > +static inline int f75375_read8(struct i2c_client *client, u8 reg) > +{ > + return i2c_smbus_read_byte_data(client, reg); > +} > + > +static inline u16 f75375_read16(struct i2c_client *client, u8 reg) > +{ > + return ((i2c_smbus_read_byte_data(client, reg) << 8) > + | i2c_smbus_read_byte_data(client, reg + 1)); > +} > + This is horrible hardware design. A single _read_word_data() would be much better here, but the datasheet apparently doesn't allow for it. Neither does the datasheet give advice for how to read the word-sized values coherently. E.g. it's common in such situations for the hardware to lock the second byte from updating for some time after the first byte is read. That's what I was looking for in the datasheet to make sure you did the accesses in the right order. AFAICT, nothing - so it's possible this thing is inherently racy. > +static inline void f75375_write8(struct i2c_client *client, u8 reg, > + u8 value) > +{ > + i2c_smbus_write_byte_data(client, reg, value); > +} > + > +static inline void f75375_write16(struct i2c_client *client, u8 reg, > + u16 value) > +{ > + int err = i2c_smbus_write_byte_data(client, reg, (value << 8)); > + if (err) > + return; > + i2c_smbus_write_byte_data(client, reg + 1, (value & 0xFF)); > +} > + > +static struct f75375_data *f75375_update_device(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int nr; > + > + mutex_lock(&data->update_lock); > + > + /* Limit registers cache is refreshed after 60 seconds */ > + if (time_after(jiffies, data->last_limits + 60 * HZ) > + || !data->valid) { > + for (nr = 0; nr < 2; nr++) { > + data->temp_high[nr] = > + f75375_read8(client, F75375_REG_TEMP_HIGH(nr)); > + data->temp_max_hyst[nr] = > + f75375_read8(client, F75375_REG_TEMP_HYST(nr)); > + data->fan_full[nr] = > + f75375_read16(client, F75375_REG_FAN_FULL(nr)); > + data->fan_min[nr] = > + f75375_read16(client, F75375_REG_FAN_MIN(nr)); > + data->fan_exp[nr] = > + f75375_read16(client, F75375_REG_FAN_EXP(nr)); > + data->pwm[nr] = f75375_read8(client, > + F75375_REG_FAN_PWM_DUTY(nr)); > + > + } > + for (nr = 0; nr < 4; nr++) { > + data->in_max[nr] = > + f75375_read8(client, F75375_REG_VOLT_HIGH(nr)); > + data->in_min[nr] = > + f75375_read8(client, F75375_REG_VOLT_LOW(nr)); > + } > + data->fan_timer = f75375_read8(client, F75375_REG_FAN_TIMER); > + data->last_limits = jiffies; > + } > + > + /* Measurement registers cache is refreshed after 2 second */ > + if (time_after(jiffies, data->last_updated + 2 * HZ) > + || !data->valid) { > + for (nr = 0; nr < 2; nr++) { > + data->temp[nr] = > + f75375_read8(client, F75375_REG_TEMP(nr)); > + data->fan[nr] = > + f75375_read16(client, F75375_REG_FAN(nr)); > + } > + for (nr = 0; nr < 4; nr++) { > + data->in[nr] = > + f75375_read8(client, F75375_REG_VOLT(nr)); > + } > + data->last_updated = jiffies; > + data->valid = 1; > + } > + > + mutex_unlock(&data->update_lock); > + return data; > +} > + > +static inline u16 rpm_from_reg(u16 reg) > +{ > + if (reg == 0 || reg == 0xffff) > + return 0; > + return (1500000 / reg); > +} > + > +static inline u16 rpm_to_reg(int rpm) > +{ > + if (rpm < 367 || rpm > 0xffff) > + return 0xffff; > + return (1500000 / rpm); > +} > + > +static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + > + mutex_lock(&data->update_lock); > + data->fan_min[nr] = rpm_to_reg(val); > + f75375_write16(client, F75375_REG_FAN_MIN(nr), data->fan_min[nr]); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t set_fan_exp(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + > + mutex_lock(&data->update_lock); > + data->fan_exp[nr] = rpm_to_reg(val); > + f75375_write16(client, F75375_REG_FAN_MIN(nr), data->fan_exp[nr]); F75375_REG_FAN_EXP(nr) ? > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + > + mutex_lock(&data->update_lock); > + data->pwm[nr] = SENSORS_LIMIT(val,0,255); > + f75375_write8(client, F75375_REG_FAN_PWM_DUTY(nr), data->pwm[nr]); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t show_pwm_enable(struct device *dev, struct device_attribute > + *attr, char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", data->pwm_enable[nr]); > +} > + > +static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + u8 fanmode; > + > + if (val < 0 || val > 4) > + return -EINVAL; > + > + mutex_lock(&data->update_lock); > + fanmode = f75375_read8(client, F75375_REG_FAN_TIMER); > + fanmode = ~(3 << FAN_CTRL_MODE(nr) ); > + > + switch (val) { > + case 0: /* Full speed */ > + fanmode |= (3 << FAN_CTRL_MODE(nr) ); > + data->pwm[nr] = 255; > + f75375_write8( client, F75375_REG_FAN_PWM_DUTY(nr), > + data->pwm[nr]); > + break; > + case 1: /* PWM */ > + fanmode |= (3 << FAN_CTRL_MODE(nr) ); > + break; > + case 2: /* AUTOMATIC*/ > + fanmode |= (2 << FAN_CTRL_MODE(nr) ); > + break; > + case 3: /* fan speed */ > + break; > + } > + f75375_write8(client, F75375_REG_FAN_TIMER, fanmode); > + data->pwm_enable[nr] = val; > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int kind = data->kind; > + int val = simple_strtoul(buf, NULL, 10); > + u8 conf = 0; > + > + if (val != 0 || val != 1 || kind == F75373 ) Suggestion: use data->kind directly and kill the local var. > + return -EINVAL; > + > + mutex_lock(&data->update_lock); > + conf = f75375_read8(client, F75375_REG_CONFIG1); > + conf = ~(1 << FAN_CTRL_LINEAR(nr) ); > + > + if (val == 0) > + conf |= (1 << FAN_CTRL_LINEAR(nr) ) ; > + > + f75375_write8(client, F75375_REG_CONFIG1, conf); > + data->pwm_mode[nr] = val; > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t show_pwm(struct device *dev, struct device_attribute > + *attr, char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", data->pwm[nr]); > +} > + > +static ssize_t show_pwm_mode(struct device *dev, struct device_attribute > + *attr, char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", data->pwm_mode[nr]); > +} > + > +#define VOLT_FROM_REG(val) ((val) * 8) > +#define VOLT_TO_REG(val) ((val) / 8) > + > +static ssize_t show_in(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", VOLT_FROM_REG(data->in[nr])); > +} > + > +static ssize_t show_in_max(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", VOLT_FROM_REG(data->in_max[nr])); > +} > + > +static ssize_t show_in_min(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", VOLT_FROM_REG(data->in_min[nr])); > +} > + > +static ssize_t set_in_max(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + val=SENSORS_LIMIT(VOLT_TO_REG(val),0,0xff); > + mutex_lock(&data->update_lock); > + data->in_max[nr] = val; > + f75375_write8(client, F75375_REG_VOLT_HIGH(nr), data->in_max[nr]); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t set_in_min(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + val=SENSORS_LIMIT(VOLT_TO_REG(val),0,0xff); > + mutex_lock(&data->update_lock); > + data->in_min[nr] = val; > + f75375_write8(client, F75375_REG_VOLT_LOW(nr), data->in_min[nr]); > + mutex_unlock(&data->update_lock); > + return count; > +} > +#define TEMP_FROM_REG(val) ((val) * 1000) > +#define TEMP_TO_REG(val) ((val) / 1000) > + > +static ssize_t show_temp(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp[nr])); > +} > + > +static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_high[nr])); > +} > + > +static ssize_t show_temp_max_hyst(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct f75375_data *data = f75375_update_device(dev); > + return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_max_hyst[nr])); > +} > + > +static ssize_t set_temp_max(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + val=SENSORS_LIMIT(TEMP_TO_REG(val),0,0xff); > + mutex_lock(&data->update_lock); > + data->temp_high[nr] = val; > + f75375_write8(client, F75375_REG_TEMP_HIGH(nr), data->temp_high[nr]); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t set_temp_max_hyst(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t count) > +{ > + int nr = to_sensor_dev_attr(attr)->index; > + struct i2c_client *client = to_i2c_client(dev); > + struct f75375_data *data = i2c_get_clientdata(client); > + int val = simple_strtoul(buf, NULL, 10); > + val=SENSORS_LIMIT(TEMP_TO_REG(val),0,0xff); > + mutex_lock(&data->update_lock); > + data->temp_max_hyst[nr] = val; > + f75375_write8(client, F75375_REG_TEMP_HYST(nr), > + data->temp_max_hyst[nr]); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +#define show_fan(thing) \ > +static ssize_t show_##thing(struct device *dev, struct device_attribute *attr, \ > + char *buf)\ > +{\ > + int nr = to_sensor_dev_attr(attr)->index;\ > + struct f75375_data *data = f75375_update_device(dev); \ > + return sprintf(buf, "%d\n", rpm_from_reg(data->thing[nr])); \ > +} > + > +show_fan(fan); > +show_fan(fan_min); > +show_fan(fan_full); > +show_fan(fan_exp); > + > +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, show_in, NULL, 0); > +static SENSOR_DEVICE_ATTR(in0_max, S_IRUGO|S_IWUSR, > + show_in_max, set_in_max, 0); > +static SENSOR_DEVICE_ATTR(in0_min, S_IRUGO|S_IWUSR, > + show_in_min, set_in_min, 0); > +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_in, NULL, 1); > +static SENSOR_DEVICE_ATTR(in1_max, S_IRUGO|S_IWUSR, > + show_in_max, set_in_max, 1); > +static SENSOR_DEVICE_ATTR(in1_min, S_IRUGO|S_IWUSR, > + show_in_min, set_in_min, 1); > +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_in, NULL, 2); > +static SENSOR_DEVICE_ATTR(in2_max, S_IRUGO|S_IWUSR, > + show_in_max, set_in_max, 2); > +static SENSOR_DEVICE_ATTR(in2_min, S_IRUGO|S_IWUSR, > + show_in_min, set_in_min, 2); > +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_in, NULL, 3); > +static SENSOR_DEVICE_ATTR(in3_max, S_IRUGO|S_IWUSR, > + show_in_max, set_in_max, 3); > +static SENSOR_DEVICE_ATTR(in3_min, S_IRUGO|S_IWUSR, > + show_in_min, set_in_min, 3); > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0); > +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IRUGO|S_IWUSR, > + show_temp_max_hyst, set_temp_max_hyst, 0); > +static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO|S_IWUSR, > + show_temp_max, set_temp_max, 0); > +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 1); > +static SENSOR_DEVICE_ATTR(temp2_max_hyst, S_IRUGO|S_IWUSR, > + show_temp_max_hyst, set_temp_max_hyst, 1); > +static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO|S_IWUSR, > + show_temp_max, set_temp_max, 1); > +static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL, 0); > +static SENSOR_DEVICE_ATTR(fan1_full, S_IRUGO, show_fan_full, NULL, 0); > +static SENSOR_DEVICE_ATTR(fan1_min, S_IRUGO|S_IWUSR, > + show_fan_min, set_fan_min, 0); > +static SENSOR_DEVICE_ATTR(fan1_exp, S_IRUGO|S_IWUSR, > + show_fan_exp, set_fan_exp, 0); > +static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, show_fan, NULL, 1); > +static SENSOR_DEVICE_ATTR(fan2_full, S_IRUGO, show_fan_full, NULL, 1); > +static SENSOR_DEVICE_ATTR(fan2_min, S_IRUGO|S_IWUSR, > + show_fan_min, set_fan_min, 1); > +static SENSOR_DEVICE_ATTR(fan2_exp, S_IRUGO|S_IWUSR, > + show_fan_exp, set_fan_exp, 1); > +static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO|S_IWUSR, > + show_pwm, set_pwm, 0); > +static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO|S_IWUSR, > + show_pwm_enable, set_pwm_enable, 0); > +static SENSOR_DEVICE_ATTR(pwm1_mode, S_IRUGO|S_IWUSR, > + show_pwm_mode, set_pwm_mode, 0); > +static SENSOR_DEVICE_ATTR(pwm2, S_IRUGO | S_IWUSR, > + show_pwm, set_pwm, 1); > +static SENSOR_DEVICE_ATTR(pwm2_enable, S_IRUGO|S_IWUSR, > + show_pwm_enable, set_pwm_enable, 1); > +static SENSOR_DEVICE_ATTR(pwm2_mode, S_IRUGO|S_IWUSR, > + show_pwm_mode, set_pwm_mode, 1); > + > +static struct attribute *f75375_attributes[] = { > + &sensor_dev_attr_temp1_input.dev_attr.attr, > + &sensor_dev_attr_temp1_max.dev_attr.attr, > + &sensor_dev_attr_temp1_max_hyst.dev_attr.attr, > + &sensor_dev_attr_temp2_input.dev_attr.attr, > + &sensor_dev_attr_temp2_max.dev_attr.attr, > + &sensor_dev_attr_temp2_max_hyst.dev_attr.attr, > + &sensor_dev_attr_fan1_input.dev_attr.attr, > + &sensor_dev_attr_fan1_full.dev_attr.attr, > + &sensor_dev_attr_fan1_min.dev_attr.attr, > + &sensor_dev_attr_fan1_exp.dev_attr.attr, > + &sensor_dev_attr_fan2_input.dev_attr.attr, > + &sensor_dev_attr_fan2_full.dev_attr.attr, > + &sensor_dev_attr_fan2_min.dev_attr.attr, > + &sensor_dev_attr_fan2_exp.dev_attr.attr, > + &sensor_dev_attr_pwm1.dev_attr.attr, > + &sensor_dev_attr_pwm1_enable.dev_attr.attr, > + &sensor_dev_attr_pwm1_mode.dev_attr.attr, > + &sensor_dev_attr_pwm2.dev_attr.attr, > + &sensor_dev_attr_pwm2_enable.dev_attr.attr, > + &sensor_dev_attr_pwm2_mode.dev_attr.attr, > + &sensor_dev_attr_in0_input.dev_attr.attr, > + &sensor_dev_attr_in0_max.dev_attr.attr, > + &sensor_dev_attr_in0_min.dev_attr.attr, > + &sensor_dev_attr_in1_input.dev_attr.attr, > + &sensor_dev_attr_in1_max.dev_attr.attr, > + &sensor_dev_attr_in1_min.dev_attr.attr, > + &sensor_dev_attr_in2_input.dev_attr.attr, > + &sensor_dev_attr_in2_max.dev_attr.attr, > + &sensor_dev_attr_in2_min.dev_attr.attr, > + &sensor_dev_attr_in3_input.dev_attr.attr, > + &sensor_dev_attr_in3_max.dev_attr.attr, > + &sensor_dev_attr_in3_min.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group f75375_group = > +{ > + .attrs = f75375_attributes, > +}; > + > +static int f75375_detach_client(struct i2c_client *client) > +{ > + struct f75375_data *data = i2c_get_clientdata(client); > + int err; > + > + sysfs_remove_group(&client->dev.kobj, &f75375_group); > + hwmon_device_unregister(data->class_dev); > + Wrong order there: unregister the class device first. > + err = i2c_detach_client(client); > + if (err) { > + dev_err(&client->dev, > + "Client deregistration failed, " > + "client not detached.\n"); > + return err; > + } > + kfree(data); > + return 0; > +} > + > +static int f75375_attach_adapter(struct i2c_adapter *adapter) > +{ You're missing a test here... if (!(adapter->class & I2C_CLASS_HWMON)) return 0; > + return i2c_probe(adapter, &addr_data, f75375_detect); > +} > + > +/* This function is called by i2c_probe */ > +static int f75375_detect(struct i2c_adapter *adapter, int address, int kind) > +{ > + struct i2c_client *client; > + struct f75375_data *data; > + u8 version = 0; > + int err = 0; > + char *name = ""; That's not good... > + > + if (!(data = kzalloc(sizeof(struct f75375_data), GFP_KERNEL))) { > + err = -ENOMEM; > + goto exit; > + } > + client = &data->client; > + i2c_set_clientdata(client, data); > + client->addr = address; > + client->adapter = adapter; > + client->driver = &f75375_driver; > + ... if there was a "force" module parameter, this will get skipped... > + if (kind < 0) { > + u16 vendid = f75375_read16(client, F75375_REG_VENDOR); > + u16 chipid = f75375_read16(client, F75375_CHIP_ID); > + version = f75375_read8(client, F75375_REG_VERSION); > + if (chipid == 0x0306 && vendid == 0x1934) { > + name = "f75375"; > + data->kind = F75375; > + } else if (chipid == 0x0204 && vendid == 0x1934) { > + name = "f75373"; > + data->kind = F75373; > + } else { > + dev_err(&adapter->dev, > + "failed,%02X,%02X,%02X\n", > + chipid, version, vendid); > + goto exit_free; > + } > + } > + dev_info(&adapter->dev, "found %s version: %02X\n", name, version); > + strlcpy(client->name, name, I2C_NAME_SIZE); ... and you'll end up with name = "". Take a look at w83781d.c for an example of how to do this properly. > + mutex_init(&data->update_lock); > + if ((err = i2c_attach_client(client))) > + goto exit_free; > + > + if ((err = sysfs_create_group(&client->dev.kobj, &f75375_group))) > + goto exit_detach; > + > + data->class_dev = hwmon_device_register(&client->dev); > + if (IS_ERR(data->class_dev)) { > + err = PTR_ERR(data->class_dev); > + goto exit_remove; > + } > + > + return 0; > + > +exit_remove: > + sysfs_remove_group(&client->dev.kobj, &f75375_group); > +exit_detach: > + i2c_detach_client(client); > +exit_free: > + kfree(data); > +exit: > + return err; > +} > + > +static int __init sensors_f75375_init(void) > +{ > + return i2c_add_driver(&f75375_driver); > +} > + > +static void __exit sensors_f75375_exit(void) > +{ > + i2c_del_driver(&f75375_driver); > +} > + > +MODULE_AUTHOR("Riku Voipio <riku.voipio at movial.fi> "); Extra space in the string. > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("F75373/F75375 hardware monitoring driver"); > + > +module_init(sensors_f75375_init); > +module_exit(sensors_f75375_exit); > -- > 1.5.2.3 > -- Mark M. Hoffman mhoffman at lightlink.com
