* [lm-sensors] W83792 review
@ 2005-05-26 19:25 Rudolf Marek
2005-05-27 3:33 ` [lm-sensors] " Huang0
` (5 more replies)
0 siblings, 6 replies; 7+ messages in thread
From: Rudolf Marek @ 2005-05-26 19:25 UTC (permalink / raw)
To: lm-sensors
Hello,
I reviewed your driver. Please check my comments.
I'm inviting other developers to check my objections I need to develop "review skill". - Thanks
>(1) We misunderstand the meaning of temperature low limit, this bug also
>exists in the 792 driver for linux-2.4 and our 792 Windows driver.
Please can you fix this ASAP so we can send it to kernel folks?
>(2) I have not implemented the 0.5 degree to temperature2 and temperature3,
>while the driver for linux-2.4 implemented this.
That is the reason temp1 has separate handling and variable? If so, please add the code.
or better would be to fix existing code so it does not look so strange.
>(3) The 792 driver for linux-2.4 can adjust the fan divisor automatically,
>while I have not implemented it in this 2.6 one.
This would be nice but better to put there some driver instead of none...
I think this can wait but it is good to have it soon.
Also we will need similar documentation to driver similar what 2.4 driver has.
You can find some inspiration on style of document here: http://ssh.cz/~ruik/chips_doc13
Please document there the extra /sys entries for your driver.
And as last thing that we will need is KConfig & Makefile entry for the driver.
I know that you are short with time so I'm offering help with documentation and KConfig and Makefile.
On the other hand I'm expecting that you will fix other stuff in reasonable time :)
Regards
Rudolf
> /*
> w83792d.c - Part of lm_sensors, Linux kernel modules for hardware
> monitoring
> Copyright (C) 2004, 2005 Winbond Electronics Corp.
> Chunhao Huang <huang0@winbond.com.tw>
>
> 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.
>
> Note:
> 1. This driver is only for 2.6 kernel, 2.4 kernel need a different driver.
> 2. This driver is only for Winbond W83792D C version device, there
> are also some motherboards with B version W83792D device. The
> calculation method to in6-in7(measured value, limits) is a little
> different between C and B version. C or B version can be identified
> by CR[0x49h].
> */
>
> /*
> Supports following chips:
>
> Chip #vin #fanin #pwm #temp wchipid vendid i2c ISA
> w83792d 9 7 7 3 0x7a 0x5ca3 yes no
> */
>
> #include <linux/config.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/slab.h>
> #include <linux/i2c.h>
> #include <linux/i2c-sensor.h>
> #include <linux/i2c-vid.h>
> #include <asm/io.h>
Do you really need this header?
> /* #include "lm75.h" */
Please remove this.
>
>
> /* Addresses to scan */
> static unsigned short normal_i2c[] = { 0x2c, 0x2f, I2C_CLIENT_END };
> static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
>
> /* Insmod parameters */
> SENSORS_INSMOD_1(w83792d);
> I2C_CLIENT_MODULE_PARM(force_subclients, "List of subclient addresses: "
> "{bus, clientaddr, subclientaddr1, subclientaddr2}");
>
> static int init;
> module_param(init, bool, 0);
> MODULE_PARM_DESC(init, "Set to one for chip initialization");
Maybe better "Set to one to force chip initialization"
>
> /* show/display the debug message */
> #define W83792D_DEBUG 1
There is some global #define for debugging called DEBUG it seems, so maybe it would be good
to remove this and use DEBUG later.
> /* The W83792D registers */
> static const u8 W83792D_REG_IN[9] = {
> 0x20, /* Vcore A in DataSheet */
> 0x21, /* Vcore B in DataSheet */
> 0x22, /* VIN0 in DataSheet */
> 0x23, /* VIN1 in DataSheet */
> 0x24, /* VIN2 in DataSheet */
> 0x25, /* VIN3 in DataSheet */
> 0x26, /* 5VCC in DataSheet */
> 0xB0, /* 5VSB in DataSheet */
> 0xB1 /* VBAT in DataSheet */
> };
> #define W83792D_REG_LOW_BITS1 0x3E /* Low Bits I in DataSheet */
> #define W83792D_REG_LOW_BITS2 0x3F /* Low Bits II in DataSheet */
> static const u8 W83792D_REG_IN_MAX[9] = {
> 0x2B, /* Vcore A High Limit in DataSheet */
> 0x2D, /* Vcore B High Limit in DataSheet */
> 0x2F, /* VIN0 High Limit in DataSheet */
> 0x31, /* VIN1 High Limit in DataSheet */
> 0x33, /* VIN2 High Limit in DataSheet */
> 0x35, /* VIN3 High Limit in DataSheet */
> 0x37, /* 5VCC High Limit in DataSheet */
> 0xB4, /* 5VSB High Limit in DataSheet */
> 0xB6 /* VBAT High Limit in DataSheet */
> };
> static const u8 W83792D_REG_IN_MIN[9] = {
> 0x2C, /* Vcore A Low Limit in DataSheet */
> 0x2E, /* Vcore B Low Limit in DataSheet */
> 0x30, /* VIN0 Low Limit in DataSheet */
> 0x32, /* VIN1 Low Limit in DataSheet */
> 0x34, /* VIN2 Low Limit in DataSheet */
> 0x36, /* VIN3 Low Limit in DataSheet */
> 0x38, /* 5VCC Low Limit in DataSheet */
> 0xB5, /* 5VSB Low Limit in DataSheet */
> 0xB7 /* VBAT Low Limit in DataSheet */
> };
> static const u8 W83792D_REG_FAN[7] = {
> 0x28, /* FAN 1 Count in DataSheet */
> 0x29, /* FAN 2 Count in DataSheet */
> 0x2A, /* FAN 3 Count in DataSheet */
> 0xB8, /* FAN 4 Count in DataSheet */
> 0xB9, /* FAN 5 Count in DataSheet */
> 0xBA, /* FAN 6 Count in DataSheet */
> 0xBE /* FAN 7 Count in DataSheet */
> };
> static const u8 W83792D_REG_FAN_MIN[7] = {
> 0x3B, /* FAN 1 Count Low Limit in DataSheet */
> 0x3C, /* FAN 2 Count Low Limit in DataSheet */
> 0x3D, /* FAN 3 Count Low Limit in DataSheet */
> 0xBB, /* FAN 4 Count Low Limit in DataSheet */
> 0xBC, /* FAN 5 Count Low Limit in DataSheet */
> 0xBD, /* FAN 6 Count Low Limit in DataSheet */
> 0xBF /* FAN 7 Count Low Limit in DataSheet */
> };
> #define W83792D_REG_FAN_CFG 0x84 /* FAN Configuration in DataSheet */
> static const u8 W83792D_REG_FAN_DIV[4] = {
> 0x47, /* contains FAN2 and FAN1 Divisor */
> 0x5B, /* contains FAN4 and FAN3 Divisor */
> 0x5C, /* contains FAN6 and FAN5 Divisor */
> 0x9E /* contains FAN7 Divisor. */
> };
> static const u8 W83792D_REG_PWM[7] = {
> 0x81, /* FAN 1 Duty Cycle, be used to control */
> 0x83, /* FAN 2 Duty Cycle, be used to control */
> 0x94, /* FAN 3 Duty Cycle, be used to control */
> 0xA3, /* FAN 4 Duty Cycle, be used to control */
> 0xA4, /* FAN 5 Duty Cycle, be used to control */
> 0xA5, /* FAN 6 Duty Cycle, be used to control */
> 0xA6 /* FAN 7 Duty Cycle, be used to control */
> };
> #define W83792D_REG_BANK 0x4E
> #define W83792D_REG_TEMP2_CONFIG 0xC2
> #define W83792D_REG_TEMP3_CONFIG 0xCA
>
> static const u8 W83792D_REG_TEMP[3] = {
> 0x27, /* TEMP 1 in DataSheet */
> 0xC0, /* TEMP 2 in DataSheet */
> 0xC8 /* TEMP 3 in DataSheet */
> };
> static const u8 W83792D_REG_TEMP_HYST[3] = {
> 0x3A, /* TEMP 1 Hyst in DataSheet */
> 0xC3, /* TEMP 2 Hyst in DataSheet */
> 0xCB /* TEMP 3 Hyst in DataSheet */
> };
> static const u8 W83792D_REG_TEMP_OVER[3] = {
> 0x39, /* TEMP 1 Over in DataSheet */
> 0xC5, /* TEMP 2 Over in DataSheet */
> 0xCD /* TEMP 3 Over in DataSheet */
> };
>
> static const u8 W83792D_REG_THERMAL[3] = {
> 0x85, /* SmartFanI: Fan1 target value */
> 0x86, /* SmartFanI: Fan2 target value */
> 0x96 /* SmartFanI: Fan3 target value */
> };
>
> static const u8 W83792D_REG_TOLERANCE[3] = {
> 0x87, /* (bit3-0)SmartFan Fan1 tolerance */
> 0x87, /* (bit7-4)SmartFan Fan2 tolerance */
> 0x97 /* (bit3-0)SmartFan Fan3 tolerance */
> };
>
> static const u8 W83792D_REG_POINTS[3][4] = {
> { 0x85, /* SmartFanII: Fan1 temp point 1 */
> 0xE3, /* SmartFanII: Fan1 temp point 2 */
> 0xE4, /* SmartFanII: Fan1 temp point 3 */
> 0xE5 }, /* SmartFanII: Fan1 temp point 4 */
> { 0x86, /* SmartFanII: Fan2 temp point 1 */
> 0xE6, /* SmartFanII: Fan2 temp point 2 */
> 0xE7, /* SmartFanII: Fan2 temp point 3 */
> 0xE8 }, /* SmartFanII: Fan2 temp point 4 */
> { 0x96, /* SmartFanII: Fan3 temp point 1 */
> 0xE9, /* SmartFanII: Fan3 temp point 2 */
> 0xEA, /* SmartFanII: Fan3 temp point 3 */
> 0xEB } /* SmartFanII: Fan3 temp point 4 */
> };
>
> static const u8 W83792D_REG_LEVELS[3][4] = {
> { 0x88, /* (bit3-0) SmartFanII: Fan1 Non-Stop */
> 0x88, /* (bit7-4) SmartFanII: Fan1 Level 1 */
> 0xE0, /* (bit7-4) SmartFanII: Fan1 Level 2 */
> 0xE0 }, /* (bit3-0) SmartFanII: Fan1 Level 3 */
> { 0x89, /* (bit3-0) SmartFanII: Fan2 Non-Stop */
> 0x89, /* (bit7-4) SmartFanII: Fan2 Level 1 */
> 0xE1, /* (bit7-4) SmartFanII: Fan2 Level 2 */
> 0xE1 }, /* (bit3-0) SmartFanII: Fan2 Level 3 */
> { 0x98, /* (bit3-0) SmartFanII: Fan3 Non-Stop */
> 0x98, /* (bit7-4) SmartFanII: Fan3 Level 1 */
> 0xE2, /* (bit7-4) SmartFanII: Fan3 Level 2 */
> 0xE2 } /* (bit3-0) SmartFanII: Fan3 Level 3 */
> };
>
> #define W83792D_REG_CONFIG 0x40
> #define W83792D_REG_IRQ 0x4C
> #define W83792D_REG_VID_FANDIV 0x47
> #define W83792D_REG_CHIPID 0x49
> #define W83792D_REG_WCHIPID 0x58
> #define W83792D_REG_CHIPMAN 0x4F
> #define W83792D_REG_PIN 0x4B
> #define W83792D_REG_I2C_SUBADDR 0x4A
>
> #define W83792D_REG_CHASSIS 0x42 /* Bit 5: Case Open status bit */
> #define W83792D_REG_CHASSIS_CLR 0x44 /* Bit 7: Case Open CLR_CHS/Reset bit */
>
> /* ctroll in0/in1 's limit modifiability */
perhap a typo ?
> #define W83792D_REG_VID_IN_B 0x17
>
> #define W83792D_REG_VBAT 0x5D
> #define W83792D_REG_I2C_ADDR 0x48
>
> /* Conversions. Rounding and limit checking is only done on the TO_REG
> variants. Note that you should be a bit careful with which arguments
> these macros are called: arguments may be evaluated more than once.
> Fixing this is just not worth it. */
> #define IN_FROM_REG(nr,val) (((nr)<=1)?(val*2): \
> ((((nr)=6)||((nr)=7))?(val*6):(val*4)))
> #define IN_TO_REG(nr,val) (((nr)<=1)?(val/2): \
> ((((nr)=6)||((nr)=7))?(val/6):(val/4)))
>
> static inline u8
> FAN_TO_REG(long rpm, int div)
> {
I'm not sure, but I think it should be like
static inline u8 FAN_TO_REG(long rpm, int div)
{
> if (rpm = 0)
> return 255;
> rpm = SENSORS_LIMIT(rpm, 1, 1000000);
> return SENSORS_LIMIT((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
> }
>
> #define FAN_FROM_REG(val,div) ((val) = 0 ? -1 : \
> ((val) = 255 ? 0 : \
> 1350000 / ((val) * (div))))
>
> #define TEMP_TO_REG(val) (SENSORS_LIMIT(((val) < 0 ? (val)+0x100*1000 \
> : (val)) / 1000, 0, 0xff))
> #define TEMP_FROM_REG(val) (((val) & 0x80 ? (val)-0x100 : (val)) * 1000)
>
> #define PWM_FROM_REG(val) (val)
> #define PWM_TO_REG(val) (SENSORS_LIMIT((val),0,255))
> #define DIV_FROM_REG(val) (1 << (val))
>
> static inline u8
> DIV_TO_REG(long val)
> {
Same here?
> int i;
> val = SENSORS_LIMIT(val, 1, 128) >> 1;
> for (i = 0; i < 6; i++) {
> if (val = 0)
> break;
> val >>= 1;
> }
> return ((u8) i);
> }
>
> struct w83792d_data {
> struct i2c_client client;
> struct semaphore lock;
> enum chips type;
>
> struct semaphore update_lock;
> char valid; /* !=0 if following fields are valid */
> unsigned long last_updated; /* In jiffies */
>
> /* array of 2 pointers to subclients */
> struct i2c_client *lm75[2];
The subclients are lm75 compatible? (I think so), but I'm not sure about the name...
> u8 in[9]; /* Register value */
> u8 in_max[9]; /* Register value */
> u8 in_min[9]; /* Register value */
> u8 low_bits[2]; /* Register value */
Please comment that lowbits belong to IN, or choose better name for the variable
> u8 fan[7]; /* Register value */
> u8 fan_min[7]; /* Register value */
> u8 temp;
> u8 temp_max; /* Register value */
> u8 temp_max_hyst; /* Register value */
> u8 temp_add[2]; /* Register value */
Please align comment with tab to same col as above.
Also why is temp1 separated? why there is not temps[3] instead?
> u8 temp_max_add[2]; /* Register value */
> u8 temp_max_hyst_add[2]; /* Register value */
Big I and align too please
> u8 fan_div[7]; /* Register encoding, shifted right */
> u8 pwm[7]; /* We only consider the first 3 set of pwm,
> although 792 chip has 7 set of pwm. */
> u8 pwmenable[3];
> u8 pwm_mode[7]; /* indicates PWM or DC mode: 1->PWM; 0->DC */
> u8 chassis; /* Chassis status */
> u8 chassis_clear; /* CLR_CHS, clear chassis intrusion detection */
> u8 thermal_cruise[3]; /* Smart FanI: Fan1,2,3 target value */
> u8 tolerance[3]; /* Fan1,2,3 tolerance(Smart Fan I/II) */
> u8 sf2_points[3][4]; /* Smart FanII: Fan1,2,3 temperature points */
> u8 sf2_levels[3][4]; /* Smart FanII: Fan1,2,3 duty cycle levels */
> };
>
> static int w83792d_attach_adapter(struct i2c_adapter *adapter);
> static int w83792d_detect(struct i2c_adapter *adapter, int address, int kind);
> static int w83792d_detach_client(struct i2c_client *client);
>
> static int w83792d_read_value(struct i2c_client *client, u8 register);
> static int w83792d_write_value(struct i2c_client *client, u8 register,
> u8 value);
> static struct w83792d_data *w83792d_update_device(struct device *dev);
>
> #ifdef W83792D_DEBUG
> static void w83792d_print_debug(struct w83792d_data *data);
> #endif
I think you can change it to: #ifdef DEBUG
> static void w83792d_init_client(struct i2c_client *client);
>
> static struct i2c_driver w83792d_driver = {
> .owner = THIS_MODULE,
> .name = "w83792d",
> .flags = I2C_DF_NOTIFY,
> .attach_adapter = w83792d_attach_adapter,
> .detach_client = w83792d_detach_client,
> };
>
> static long in_count_from_reg(int nr, struct w83792d_data *data)
> {
> u16 vol_count = data->in[nr];
> u16 low_bits = 0;
> vol_count = (vol_count << 2);
> switch (nr)
> {
> case 0: /* vin0 */
> low_bits = (data->low_bits[0]) & 0x03;
> break;
> case 1: /* vin1 */
> low_bits = ((data->low_bits[0]) & 0x0c) >> 2;
> break;
> case 2: /* vin2 */
> low_bits = ((data->low_bits[0]) & 0x30) >> 4;
> break;
> case 3: /* vin3 */
> low_bits = ((data->low_bits[0]) & 0xc0) >> 6;
> break;
> case 4: /* vin4 */
> low_bits = (data->low_bits[1]) & 0x03;
> break;
> case 5: /* vin5 */
> low_bits = ((data->low_bits[1]) & 0x0c) >> 2;
> break;
> case 6: /* vin6 */
> low_bits = ((data->low_bits[1]) & 0x30) >> 4;
> default:
> break;
> }
> vol_count = vol_count | low_bits;
> return vol_count;
> }
>
> /* following are the sysfs callback functions */
> static ssize_t show_in(struct device *dev, char *buf, int nr)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf,"%ld\n", IN_FROM_REG(nr,(in_count_from_reg(nr, data))));
> }
>
> #define show_in_reg(reg) \
> static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> { \
> struct w83792d_data *data = w83792d_update_device(dev); \
> return sprintf(buf,"%ld\n", (long)(IN_FROM_REG(nr, (data->reg[nr])*4))); \
> }
>
> show_in_reg(in_min);
> show_in_reg(in_max);
>
> #define store_in_reg(REG, reg) \
> static ssize_t store_in_##reg (struct device *dev, const char *buf, size_t count, int nr) \
> { \
> struct i2c_client *client = to_i2c_client(dev); \
> struct w83792d_data *data = i2c_get_clientdata(client); \
> u32 val; \
> \
> val = simple_strtoul(buf, NULL, 10); \
> data->in_##reg[nr] = SENSORS_LIMIT(IN_TO_REG(nr, val)/4, 0, 255); \
> w83792d_write_value(client, W83792D_REG_IN_##REG[nr], data->in_##reg[nr]); \
> \
> return count; \
> }
> store_in_reg(MIN, min);
> store_in_reg(MAX, max);
>
> #define sysfs_in_offset(offset) \
> static ssize_t \
> show_regs_in_##offset (struct device *dev, char *buf) \
> { \
> return show_in(dev, buf, offset); \
> } \
> static DEVICE_ATTR(in##offset##_input, S_IRUGO, show_regs_in_##offset, NULL);
>
> #define sysfs_in_reg_offset(reg, offset) \
> static ssize_t show_regs_in_##reg##offset (struct device *dev, char *buf) \
> { \
> return show_in_##reg (dev, buf, offset); \
> } \
> static ssize_t store_regs_in_##reg##offset (struct device *dev, const char *buf, size_t count) \
> { \
> return store_in_##reg (dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(in##offset##_##reg, S_IRUGO| S_IWUSR, show_regs_in_##reg##offset, store_regs_in_##reg##offset);
>
> #define sysfs_in_offsets(offset) \
> sysfs_in_offset(offset); \
> sysfs_in_reg_offset(min, offset); \
> sysfs_in_reg_offset(max, offset);
>
> sysfs_in_offsets(0);
> sysfs_in_offsets(1);
> sysfs_in_offsets(2);
> sysfs_in_offsets(3);
> sysfs_in_offsets(4);
> sysfs_in_offsets(5);
> sysfs_in_offsets(6);
> sysfs_in_offsets(7);
> sysfs_in_offsets(8);
>
> #define device_create_file_in(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_in##offset##_input); \
> device_create_file(&client->dev, &dev_attr_in##offset##_min); \
> device_create_file(&client->dev, &dev_attr_in##offset##_max); \
> } while (0)
>
> #define show_fan_reg(reg) \
> static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> { \
> struct w83792d_data *data = w83792d_update_device(dev); \
> return sprintf(buf,"%ld\n", \
> FAN_FROM_REG(data->reg[nr-1], (long)DIV_FROM_REG(data->fan_div[nr-1]))); \
> }
> show_fan_reg(fan);
> show_fan_reg(fan_min);
>
> static ssize_t
> store_fan_min(struct device *dev, const char *buf, size_t count, int nr)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
>
> val = simple_strtoul(buf, NULL, 10);
> data->fan_min[nr - 1] > FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr - 1]));
Please use second tab instead of 4 spaces
> w83792d_write_value(client, W83792D_REG_FAN_MIN[nr-1],
> data->fan_min[nr - 1]);
>
> return count;
> }
>
> #define sysfs_fan_offset(offset) \
> static ssize_t show_regs_fan_##offset (struct device *dev, char *buf) \
> { \
> return show_fan(dev, buf, offset); \
> } \
> static DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_regs_fan_##offset, NULL);
>
> #define sysfs_fan_min_offset(offset) \
> static ssize_t show_regs_fan_min##offset (struct device *dev, char *buf) \
> { \
> return show_fan_min(dev, buf, offset); \
> } \
> static ssize_t store_regs_fan_min##offset (struct device *dev, const char *buf, size_t count) \
> { \
> return store_fan_min(dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, show_regs_fan_min##offset, store_regs_fan_min##offset);
>
> sysfs_fan_offset(1);
> sysfs_fan_min_offset(1);
> sysfs_fan_offset(2);
> sysfs_fan_min_offset(2);
> sysfs_fan_offset(3);
> sysfs_fan_min_offset(3);
> sysfs_fan_offset(4);
> sysfs_fan_min_offset(4);
> sysfs_fan_offset(5);
> sysfs_fan_min_offset(5);
> sysfs_fan_offset(6);
> sysfs_fan_min_offset(6);
> sysfs_fan_offset(7);
> sysfs_fan_min_offset(7);
>
> #define device_create_file_fan(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_fan##offset##_input); \
> device_create_file(&client->dev, &dev_attr_fan##offset##_min); \
> } while (0)
>
> #define show_temp_reg(reg) \
> static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> { \
> struct w83792d_data *data = w83792d_update_device(dev); \
> if (nr >= 2) { /* TEMP2 and TEMP3 */ \
> return sprintf(buf,"%ld\n", \
> (long)TEMP_FROM_REG(data->reg##_add[nr-2])); \
> } else { /* TEMP1 */ \
> return sprintf(buf,"%ld\n", (long)TEMP_FROM_REG(data->reg)); \
> } \
> }
Same question related here? Why temp1 separate?
> show_temp_reg(temp);
> show_temp_reg(temp_max);
> show_temp_reg(temp_max_hyst);
>
> #define store_temp_reg(REG, reg) \
> static ssize_t store_temp_##reg (struct device *dev, const char *buf, size_t count, int nr) \
> { \
> struct i2c_client *client = to_i2c_client(dev); \
> struct w83792d_data *data = i2c_get_clientdata(client); \
> s32 val; \
> \
> val = simple_strtol(buf, NULL, 10); \
> \
> if (nr >= 2) { \
> data->temp_##reg##_add[nr-2] = TEMP_TO_REG(val); \
> w83792d_write_value(client, W83792D_REG_TEMP_##REG[nr-1], \
> data->temp_##reg##_add[nr-2]); \
> } else { \
> data->temp_##reg = TEMP_TO_REG(val); \
> w83792d_write_value(client, W83792D_REG_TEMP_##REG[nr-1], \
> data->temp_##reg); \
> } \
> \
> return count; \
> }
Same question related here? Why temp1 separate?
> store_temp_reg(OVER, max);
> store_temp_reg(HYST, max_hyst);
>
> #define sysfs_temp_offset(offset) \
> static ssize_t \
> show_regs_temp_##offset (struct device *dev, char *buf) \
> { \
> return show_temp(dev, buf, offset); \
> } \
> static DEVICE_ATTR(temp##offset##_input, S_IRUGO, show_regs_temp_##offset, NULL);
>
> #define sysfs_temp_reg_offset(reg, offset) \
> static ssize_t show_regs_temp_##reg##offset (struct device *dev, char *buf) \
> { \
> return show_temp_##reg (dev, buf, offset); \
> } \
> static ssize_t store_regs_temp_##reg##offset (struct device *dev, const char *buf, size_t count) \
> { \
> return store_temp_##reg (dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(temp##offset##_##reg, S_IRUGO| S_IWUSR, show_regs_temp_##reg##offset, store_regs_temp_##reg##offset);
>
> #define sysfs_temp_offsets(offset) \
> sysfs_temp_offset(offset); \
> sysfs_temp_reg_offset(max, offset); \
> sysfs_temp_reg_offset(max_hyst, offset);
>
> sysfs_temp_offsets(1);
> sysfs_temp_offsets(2);
> sysfs_temp_offsets(3);
>
> #define device_create_file_temp(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_temp##offset##_input); \
> device_create_file(&client->dev, &dev_attr_temp##offset##_max); \
> device_create_file(&client->dev, &dev_attr_temp##offset##_max_hyst); \
> } while (0)
>
> static ssize_t
> show_fan_div_reg(struct device *dev, char *buf, int nr)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n",
> (long) DIV_FROM_REG(data->fan_div[nr - 1]));
> }
>
> /* Note: we save and restore the fan minimum here, because its value is
> determined in part by the fan divisor. This follows the principle of
> least suprise; the user doesn't expect the fan minimum to change just
> because the divisor changed. */
> static ssize_t
> store_fan_div_reg(struct device *dev, const char *buf, size_t count, int nr)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> unsigned long min;
> /*u8 reg;*/
Please remove
> u8 fan_div_reg=0; u8 tmp_fan_div;
Please separate them on two lines.
> /* Save fan_min */
> min = FAN_FROM_REG(data->fan_min[nr],
> DIV_FROM_REG(data->fan_div[nr]));
>
> data->fan_div[nr] = DIV_TO_REG(simple_strtoul(buf, NULL, 10));
>
> fan_div_reg = w83792d_read_value(client, W83792D_REG_FAN_DIV[nr/2]);
nr/2 could be [nr >> 1]
> fan_div_reg &= (nr%2 = 0) ? 0xf8 : 0x8f;
and here
fan_div_reg &= (nr & 1) ? 0x8f : 0xf8;
> tmp_fan_div = (nr%2 = 0) ? ((data->fan_div[nr])&0x07)
> : (((data->fan_div[nr])<<4)&0x70);
This needs definetly spaces between operators but it can be also done this way:
tmp_fan_div = (nr & 1) ? ((data->fan_div[nr] << 4) & 0x70)
: data->fan_div[nr] & 0x07;
> w83792d_write_value(client, W83792D_REG_FAN_DIV[nr/2],
nr >> 1
> fan_div_reg|tmp_fan_div);
>
> /* Restore fan_min */
> data->fan_min[nr] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
> w83792d_write_value(client, W83792D_REG_FAN_MIN[nr], data->fan_min[nr]);
>
> return count;
> }
>
> #define sysfs_fan_div(offset) \
> static ssize_t show_regs_fan_div_##offset (struct device *dev, char *buf) \
> { \
> return show_fan_div_reg(dev, buf, offset); \
> } \
> static ssize_t store_regs_fan_div_##offset (struct device *dev, const char *buf, size_t count) \
> { \
> return store_fan_div_reg(dev, buf, count, offset - 1); \
> } \
> static DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, show_regs_fan_div_##offset, store_regs_fan_div_##offset);
>
> sysfs_fan_div(1);
> sysfs_fan_div(2);
> sysfs_fan_div(3);
> sysfs_fan_div(4);
> sysfs_fan_div(5);
> sysfs_fan_div(6);
> sysfs_fan_div(7);
>
> #define device_create_file_fan_div(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_fan##offset##_div); \
> } while (0)
>
> static ssize_t
> show_pwm_reg(struct device *dev, char *buf, int nr)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long) PWM_FROM_REG(data->pwm[nr-1]));
> }
>
> static ssize_t
> show_pwmenable_reg(struct device *dev, char *buf, int nr)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> long pwm_enable_tmp = 1;
Should be u32 instead?
> if (data->pwmenable[nr-1] = 0) {
> pwm_enable_tmp = 1; /* manual mode */
> } else if (data->pwmenable[nr-1] = 1) {
> pwm_enable_tmp = 3; /* thermal cruise/Smart Fan I */
> } else if (data->pwmenable[nr-1] = 2) {
> pwm_enable_tmp = 2; /* Smart Fan II */
> }
> return sprintf(buf, "%ld\n", pwm_enable_tmp);
> }
>
> static ssize_t
> store_pwm_reg(struct device *dev, const char *buf, size_t count, int nr)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
>
> val = simple_strtoul(buf, NULL, 10);
>
> data->pwm[nr-1] = PWM_TO_REG(val);
> w83792d_write_value(client, W83792D_REG_PWM[nr-1], data->pwm[nr-1]);
>
> return count;
> }
>
> static ssize_t
> store_pwmenable_reg(struct device *dev, const char *buf, size_t count, int nr)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
> u8 fan_cfg_tmp, cfg1_tmp, cfg2_tmp, cfg3_tmp, cfg4_tmp;
>
> val = simple_strtoul(buf, NULL, 10);
>
> switch (val) {
> case 1:
> data->pwmenable[nr-1] = 0; /* manual mode */
> break;
> case 2:
> data->pwmenable[nr-1] = 2; /* Smart Fan II */
> break;
> case 3:
> data->pwmenable[nr-1] = 1; /* thermal cruise/Smart Fan I */
> break;
> default:
> return -EINVAL;
> }
> cfg1_tmp = data->pwmenable[0];
> cfg2_tmp = (data->pwmenable[1]) << 2;
> cfg3_tmp = (data->pwmenable[2]) << 4;
> cfg4_tmp = w83792d_read_value(client,W83792D_REG_FAN_CFG) & 0xc0;
> fan_cfg_tmp = ((cfg4_tmp|cfg3_tmp)|cfg2_tmp)|cfg1_tmp;
Missing spaces
fan_cfg_tmp = ((cfg4_tmp | cfg3_tmp) | cfg2_tmp) | cfg1_tmp;
> w83792d_write_value(client,W83792D_REG_FAN_CFG,fan_cfg_tmp);
>
> return count;
> }
>
> #define sysfs_pwm(offset) \
> static ssize_t show_regs_pwm_##offset (struct device *dev, char *buf) \
> { \
> return show_pwm_reg(dev, buf, offset); \
> } \
> static ssize_t store_regs_pwm_##offset (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return store_pwm_reg(dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, \
> show_regs_pwm_##offset, store_regs_pwm_##offset);
>
> #define sysfs_pwmenable(offset) \
> static ssize_t show_regs_pwmenable_##offset (struct device *dev, char *buf) \
> { \
> return show_pwmenable_reg(dev, buf, offset); \
> } \
> static ssize_t store_regs_pwmenable_##offset (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return store_pwmenable_reg(dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(pwm##offset##_enable, S_IRUGO | S_IWUSR, \
> show_regs_pwmenable_##offset, store_regs_pwmenable_##offset);
>
> sysfs_pwm(1);
> sysfs_pwm(2);
> sysfs_pwm(3);
> sysfs_pwmenable(1);
> sysfs_pwmenable(2);
> sysfs_pwmenable(3);
>
> #define device_create_file_pwm(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_pwm##offset); \
> } while (0)
>
> #define device_create_file_pwmenable(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_pwm##offset##_enable); \
> } while (0)
>
>
> static ssize_t
> show_pwm_mode_reg(struct device *dev, char *buf, int nr)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long)data->pwm_mode[nr-1]);
> }
>
> static ssize_t
> store_pwm_mode_reg(struct device *dev, const char *buf, size_t count, int nr)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
> u8 pwm_mode_mask = 0;
>
> val = simple_strtoul(buf, NULL, 10);
> data->pwm_mode[nr-1] = SENSORS_LIMIT(val, 0, 1);
> pwm_mode_mask = w83792d_read_value(client,
> W83792D_REG_PWM[nr-1]) & 0x7f;
> w83792d_write_value(client, W83792D_REG_PWM[nr-1],
> ((data->pwm_mode[nr-1])<<7)|pwm_mode_mask);
>
> return count;
> }
>
> #define sysfs_pwm_mode(offset) \
> static ssize_t show_regs_pwm_mode_##offset (struct device *dev, char *buf) \
> { \
> return show_pwm_mode_reg(dev, buf, offset); \
> } \
> static ssize_t store_regs_pwm_mode_##offset (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return store_pwm_mode_reg(dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(pwm##offset##_mode, S_IRUGO | S_IWUSR, \
> show_regs_pwm_mode_##offset, store_regs_pwm_mode_##offset);
>
> sysfs_pwm_mode(1);
> sysfs_pwm_mode(2);
> sysfs_pwm_mode(3);
>
> #define device_create_file_pwm_mode(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_pwm##offset##_mode); \
> } while (0)
>
>
> static ssize_t
> show_chassis_reg(struct device *dev, char *buf)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long)data->chassis);
> }
>
> static ssize_t show_regs_chassis (struct device *dev, char *buf)
> {
> return show_chassis_reg(dev, buf);
> }
>
> static DEVICE_ATTR(chassis, S_IRUGO, show_regs_chassis, NULL);
>
> #define device_create_file_chassis(client) \
> do { \
> device_create_file(&client->dev, &dev_attr_chassis); \
> } while (0)
>
>
> static ssize_t
> show_chassis_clear_reg(struct device *dev, char *buf)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long)data->chassis_clear);
> }
>
> static ssize_t
> store_chassis_clear_reg(struct device *dev, const char *buf, size_t count)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
> u8 temp1 = 0, temp2 = 0;
>
> val = simple_strtoul(buf, NULL, 10);
>
> data->chassis_clear = SENSORS_LIMIT(val, 0 ,1);
> temp1 = ((data->chassis_clear) << 7) & 0x80;
> temp2 = w83792d_read_value(client,
> W83792D_REG_CHASSIS_CLR) & 0x7f;
> w83792d_write_value(client, W83792D_REG_CHASSIS_CLR, temp1|temp2);
Missing spaces between |
>
> return count;
> }
>
> static ssize_t show_regs_chassis_clear (struct device *dev, char *buf)
> {
> return show_chassis_clear_reg(dev, buf);
> }
>
> static ssize_t store_regs_chassis_clear (struct device *dev,
> const char *buf, size_t count)
> {
> return store_chassis_clear_reg(dev, buf, count);
> }
>
> static DEVICE_ATTR(chassis_clear, S_IRUGO | S_IWUSR,
> show_regs_chassis_clear, store_regs_chassis_clear);
>
> #define device_create_file_chassis_clear(client) \
> do { \
> device_create_file(&client->dev, &dev_attr_chassis_clear); \
> } while (0)
>
>
>
> /* For Smart Fan I / Thermal Cruise */
> static ssize_t
> show_thermal_cruise_reg(struct device *dev, char *buf, int nr)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long)data->thermal_cruise[nr-1]);
> }
>
> static ssize_t
> store_thermal_cruise_reg(struct device *dev, const char *buf, size_t count, int nr)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
> u8 target_tmp=0, target_mask=0;
>
> val = simple_strtoul(buf, NULL, 10);
>
> target_tmp = val;
> target_tmp = target_tmp & 0x7f;
> target_mask = w83792d_read_value(client, W83792D_REG_THERMAL[nr-1]) & 0x80;
> data->thermal_cruise[nr-1] = SENSORS_LIMIT(target_tmp, 0, 255);
> w83792d_write_value(client, W83792D_REG_THERMAL[nr-1],
> (data->thermal_cruise[nr-1])|target_mask);
Missing spaces between |
>
> return count;
> }
>
> #define sysfs_thermal_cruise(offset) \
> static ssize_t show_regs_thermal_cruise##offset (struct device *dev, char *buf) \
> { \
> return show_thermal_cruise_reg(dev, buf, offset); \
> } \
> static ssize_t store_regs_thermal_cruise##offset (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return store_thermal_cruise_reg(dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(thermal_cruise##offset, S_IRUGO | S_IWUSR, \
> show_regs_thermal_cruise##offset, store_regs_thermal_cruise##offset);
>
> sysfs_thermal_cruise(1);
> sysfs_thermal_cruise(2);
> sysfs_thermal_cruise(3);
>
> #define device_create_file_thermal_cruise(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_thermal_cruise##offset); \
> } while (0)
>
>
> /* For Smart Fan I/Thermal Cruise and Smart Fan II */
> static ssize_t
> show_tolerance_reg(struct device *dev, char *buf, int nr)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long)data->tolerance[nr-1]);
> }
>
> static ssize_t
> store_tolerance_reg(struct device *dev, const char *buf, size_t count, int nr)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
> u8 tol_tmp, tol_mask;
>
> val = simple_strtoul(buf, NULL, 10);
>
> tol_mask = w83792d_read_value(client,
> W83792D_REG_TOLERANCE[nr-1]) & ((nr=2)?0x0f:0xf0);
Missing spaces: ((nr = 2) ? 0x0f : 0xf0);
> tol_tmp = SENSORS_LIMIT(val, 0, 15);
> tol_tmp &= 0x0f;
> data->tolerance[nr-1] = tol_tmp;
> if (nr=2) {
> tol_tmp <<= 4;
> }
> w83792d_write_value(client, W83792D_REG_TOLERANCE[nr-1],
> tol_mask|tol_tmp);
Missing spaces between |
>
> return count;
> }
>
> #define sysfs_tolerance(offset) \
> static ssize_t show_regs_tolerance##offset (struct device *dev, char *buf) \
> { \
> return show_tolerance_reg(dev, buf, offset); \
> } \
> static ssize_t store_regs_tolerance##offset (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return store_tolerance_reg(dev, buf, count, offset); \
> } \
> static DEVICE_ATTR(tolerance##offset, S_IRUGO | S_IWUSR, \
> show_regs_tolerance##offset, store_regs_tolerance##offset);
>
> sysfs_tolerance(1);
> sysfs_tolerance(2);
> sysfs_tolerance(3);
>
> #define device_create_file_tolerance(client, offset) \
> do { \
> device_create_file(&client->dev, &dev_attr_tolerance##offset); \
> } while (0)
>
>
> /* For Smart Fan II */
> static ssize_t
> show_sf2_point_reg(struct device *dev, char *buf, int nr, int index)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long)data->sf2_points[index-1][nr-1]);
> }
>
> static ssize_t
> store_sf2_point_reg(struct device *dev, const char *buf, size_t count, int nr, int index)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
> u8 mask_tmp = 0;
>
> val = simple_strtoul(buf, NULL, 10);
>
> data->sf2_points[index-1][nr-1] = SENSORS_LIMIT(val, 0, 127);
> mask_tmp = w83792d_read_value(client,
> W83792D_REG_POINTS[index-1][nr-1]) & 0x80;
> w83792d_write_value(client, W83792D_REG_POINTS[index-1][nr-1],
> mask_tmp|data->sf2_points[index-1][nr-1]);
>
> return count;
> }
>
> #define sysfs_sf2_point(offset, index) \
> static ssize_t show_regs_sf2_point##offset##_fan##index (struct device *dev, char *buf) \
> { \
> return show_sf2_point_reg(dev, buf, offset, index); \
> } \
> static ssize_t store_regs_sf2_point##offset##_fan##index (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return store_sf2_point_reg(dev, buf, count, offset, index); \
> } \
> static DEVICE_ATTR(sf2_point##offset##_fan##index, S_IRUGO | S_IWUSR, \
> show_regs_sf2_point##offset##_fan##index, \
> store_regs_sf2_point##offset##_fan##index);
>
> sysfs_sf2_point(1, 1); /* Fan1 */
> sysfs_sf2_point(2, 1); /* Fan1 */
> sysfs_sf2_point(3, 1); /* Fan1 */
> sysfs_sf2_point(4, 1); /* Fan1 */
> sysfs_sf2_point(1, 2); /* Fan2 */
> sysfs_sf2_point(2, 2); /* Fan2 */
> sysfs_sf2_point(3, 2); /* Fan2 */
> sysfs_sf2_point(4, 2); /* Fan2 */
> sysfs_sf2_point(1, 3); /* Fan3 */
> sysfs_sf2_point(2, 3); /* Fan3 */
> sysfs_sf2_point(3, 3); /* Fan3 */
> sysfs_sf2_point(4, 3); /* Fan3 */
>
> #define device_create_file_sf2_point(client, offset, index) \
> do { \
> device_create_file(&client->dev, &dev_attr_sf2_point##offset##_fan##index); \
> } while (0)
>
>
> static ssize_t
> show_sf2_level_reg(struct device *dev, char *buf, int nr, int index)
> {
> struct w83792d_data *data = w83792d_update_device(dev);
> return sprintf(buf, "%ld\n", (long)(((data->sf2_levels[index-1][nr])*100)/15));
Missing spaces
> }
>
> static ssize_t
> store_sf2_level_reg(struct device *dev, const char *buf, size_t count, int nr, int index)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> u32 val;
> u8 mask_tmp=0, level_tmp=0;
>
> val = simple_strtoul(buf, NULL, 10);
>
> data->sf2_levels[index-1][nr] > SENSORS_LIMIT((val*15)/100, 0, 15);
And here too it seems
> mask_tmp = w83792d_read_value(client, W83792D_REG_LEVELS[index-1][nr])
> & ((nr=3) ? 0xf0 : 0x0f);
> if (nr=3) {
> level_tmp = data->sf2_levels[index-1][nr];
> } else {
> level_tmp = data->sf2_levels[index-1][nr] << 4;
> }
> w83792d_write_value(client, W83792D_REG_LEVELS[index-1][nr], level_tmp | mask_tmp);
>
> return count;
> }
>
> #define sysfs_sf2_level(offset, index) \
> static ssize_t show_regs_sf2_level##offset##_fan##index (struct device *dev, char *buf) \
> { \
> return show_sf2_level_reg(dev, buf, offset, index); \
> } \
> static ssize_t store_regs_sf2_level##offset##_fan##index (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return store_sf2_level_reg(dev, buf, count, offset, index); \
> } \
> static DEVICE_ATTR(sf2_level##offset##_fan##index, S_IRUGO | S_IWUSR, \
> show_regs_sf2_level##offset##_fan##index, \
> store_regs_sf2_level##offset##_fan##index);
>
>
> sysfs_sf2_level(1, 1); /* Fan1 */
> sysfs_sf2_level(2, 1); /* Fan1 */
> sysfs_sf2_level(3, 1); /* Fan1 */
> sysfs_sf2_level(1, 2); /* Fan2 */
> sysfs_sf2_level(2, 2); /* Fan2 */
> sysfs_sf2_level(3, 2); /* Fan2 */
> sysfs_sf2_level(1, 3); /* Fan3 */
> sysfs_sf2_level(2, 3); /* Fan3 */
> sysfs_sf2_level(3, 3); /* Fan3 */
>
> #define device_create_file_sf2_level(client, offset, index) \
> do { \
> device_create_file(&client->dev, &dev_attr_sf2_level##offset##_fan##index); \
> } while (0)
>
>
> /* This function is called when:
> * w83792d_driver is inserted (when this module is loaded), for each
> available adapter
> * when a new adapter is inserted (and w83792d_driver is still present) */
> static int
> w83792d_attach_adapter(struct i2c_adapter *adapter)
I think static int and rest belong to one line
> {
> if (!(adapter->class & I2C_CLASS_HWMON))
> return 0;
> return i2c_detect(adapter, &addr_data, w83792d_detect);
> }
>
>
> static int
> w83792d_detect_subclients(struct i2c_adapter *adapter, int address, int kind,
> struct i2c_client *new_client)
and here too
> {
> int i, id, err;
> struct w83792d_data *data = i2c_get_clientdata(new_client);
>
> data->lm75[0] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
> if (!(data->lm75[0])) {
> err = -ENOMEM;
> goto ERROR_SC_0;
> }
> memset(data->lm75[0], 0x00, sizeof(struct i2c_client));
>
> data->lm75[1] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
> if (!(data->lm75[1])) {
> err = -ENOMEM;
> goto ERROR_SC_1;
> }
> memset(data->lm75[1], 0x00, sizeof(struct i2c_client));
>
> id = i2c_adapter_id(adapter);
>
> if (force_subclients[0] = id && force_subclients[1] = address) {
> for (i = 2; i <= 3; i++) {
> if (force_subclients[i] < 0x48 ||
> force_subclients[i] > 0x4f) {
> dev_err(&new_client->dev, "invalid subclient "
> "address %d; must be 0x48-0x4f\n",
> force_subclients[i]);
> err = -ENODEV;
> goto ERROR_SC_2;
> }
> }
> w83792d_write_value(new_client, W83792D_REG_I2C_SUBADDR,
> (force_subclients[2] & 0x03) |
> ((force_subclients[3] & 0x03) <<4));
> data->lm75[0]->addr = force_subclients[2];
> data->lm75[1]->addr = force_subclients[3];
> } else {
> int val = w83792d_read_value(new_client, W83792D_REG_I2C_SUBADDR);
This should be u8 val
> data->lm75[0]->addr = 0x48 + (val & 0x07);
> data->lm75[1]->addr = 0x48 + ((val >> 4) & 0x07);
> }
>
> if (data->lm75[0]->addr = data->lm75[1]->addr) {
> dev_err(&new_client->dev, "duplicate addresses 0x%x "
> "for subclients\n", data->lm75[0]->addr);
> err = -ENODEV;
> goto ERROR_SC_2;
> }
>
> for (i = 0; i <= 1; i++) {
> i2c_set_clientdata(data->lm75[i], NULL);
> data->lm75[i]->adapter = adapter;
> data->lm75[i]->driver = &w83792d_driver;
> data->lm75[i]->flags = 0;
> strlcpy(data->lm75[i]->name, "w83792d subclient", I2C_NAME_SIZE);
> }
>
> if ((err = i2c_attach_client(data->lm75[0]))) {
> dev_err(&new_client->dev, "subclient %d registration "
> "at address 0x%x failed.\n", i, data->lm75[0]->addr);
> goto ERROR_SC_2;
> }
>
> if ((err = i2c_attach_client(data->lm75[1]))) {
> dev_err(&new_client->dev, "subclient %d registration "
> "at address 0x%x failed.\n", i, data->lm75[1]->addr);
> goto ERROR_SC_3;
> }
>
> return 0;
>
> /* Undo inits in case of errors */
> ERROR_SC_3:
> i2c_detach_client(data->lm75[0]);
> ERROR_SC_2:
> kfree(data->lm75[1]);
> ERROR_SC_1:
> kfree(data->lm75[0]);
> ERROR_SC_0:
> return err;
> }
>
>
> static int
> w83792d_detect(struct i2c_adapter *adapter, int address, int kind)
> {
Same line?
> int i = 0, val1 = 0, val2;
> struct i2c_client *new_client;
> struct w83792d_data *data;
> int err = 0;
> const char *client_name = "";
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> goto ERROR0;
> }
>
> /* OK. For now, we presume we have a valid client. We now create the
> client structure, even though we cannot fill it completely yet.
> But it allows us to access w83792d_{read,write}_value. */
>
> if (!(data = kmalloc(sizeof(struct w83792d_data), GFP_KERNEL))) {
> err = -ENOMEM;
> goto ERROR0;
> }
> memset(data, 0, sizeof(struct w83792d_data));
>
> new_client = &data->client;
> i2c_set_clientdata(new_client, data);
> new_client->addr = address;
> init_MUTEX(&data->lock);
> new_client->adapter = adapter;
> new_client->driver = &w83792d_driver;
> new_client->flags = 0;
>
> /* Now, we do the remaining detection. */
>
> /* The w8378?d may be stuck in some other bank than bank 0. This may
Bad chip name in comment
> make reading other information impossible. Specify a force=... or
> force_*=... parameter, and the Winbond will be reset to the right
> bank. */
> if (kind < 0) {
> if (w83792d_read_value(new_client, W83792D_REG_CONFIG) & 0x80) {
> dev_warn(&new_client->dev, "Detection failed at step "
> "3\n");
> goto ERROR1;
> }
> val1 = w83792d_read_value(new_client, W83792D_REG_BANK);
> val2 = w83792d_read_value(new_client, W83792D_REG_CHIPMAN);
> /* Check for Winbond ID if in bank 0 */
> if (!(val1 & 0x07)) { /* is Bank0 */
> if (((!(val1 & 0x80)) && (val2 != 0xa3)) ||
> ((val1 & 0x80) && (val2 != 0x5c))) {
> goto ERROR1;
> }
> }
> /* If Winbond SMBus, check address at 0x48 */
better comment would be:
If Winbond chip, address of chip and W83792D_REG_I2C_ADDR should match.
> if (w83792d_read_value(new_client, W83792D_REG_I2C_ADDR) != address) {
> dev_warn(&new_client->dev, "Detection failed "
> "at step 5\n");
> goto ERROR1;
> }
> }
>
> /* We have either had a force parameter, or we have already detected the
> Winbond. Put it now into bank 0 and Vendor ID High Byte */
> w83792d_write_value(new_client, W83792D_REG_BANK,
> (w83792d_read_value(new_client,
> W83792D_REG_BANK) & 0x78) |
> 0x80);
>
Maybe better indent?
> /* Determine the chip type. */
> if (kind <= 0) {
> /* get vendor ID */
> val2 = w83792d_read_value(new_client, W83792D_REG_CHIPMAN);
> if (val2 != 0x5c) { /* the vendor is NOT Winbond */
> goto ERROR1;
> }
> val1 = w83792d_read_value(new_client, W83792D_REG_WCHIPID);
> if (val1 = 0x7a && address >= 0x2c) {
> kind = w83792d;
> } else {
> if (kind = 0)
> printk
> (KERN_WARNING "w83792d: Ignoring 'force' parameter for unknown chip at"
> "adapter %d, address 0x%02x\n",
> i2c_adapter_id(adapter), address);
> goto ERROR1;
> }
> }
>
> if (kind = w83792d) {
> client_name = "w83792d";
> } else {
> dev_warn(&new_client->dev, "w83792d: Internal error: unknown kind (%d)?!?",
> kind);
> goto ERROR1;
> }
>
> /* Fill in the remaining client fields and put into the global list */
> strlcpy(new_client->name, client_name, I2C_NAME_SIZE);
> data->type = kind;
>
> data->valid = 0;
> init_MUTEX(&data->update_lock);
>
> /* Tell the I2C layer a new client has arrived */
> if ((err = i2c_attach_client(new_client)))
> goto ERROR1;
>
> if ((err = w83792d_detect_subclients(adapter, address,
> kind, new_client)))
> goto ERROR2;
>
> /* Initialize the chip */
> w83792d_init_client(new_client);
>
> /* A few vars need to be filled upon startup */
> for (i = 1; i <= 7; i++) {
> data->fan_min[i - 1] = w83792d_read_value(new_client,
> W83792D_REG_FAN_MIN[i]);
> }
hmm really? why not it init_client?
>
> /* Register sysfs hooks */
> device_create_file_in(new_client, 0);
> device_create_file_in(new_client, 1);
> device_create_file_in(new_client, 2);
> device_create_file_in(new_client, 3);
> device_create_file_in(new_client, 4);
> device_create_file_in(new_client, 5);
> device_create_file_in(new_client, 6);
> device_create_file_in(new_client, 7);
> device_create_file_in(new_client, 8);
>
> device_create_file_fan(new_client, 1);
> device_create_file_fan(new_client, 2);
> device_create_file_fan(new_client, 3);
> device_create_file_fan(new_client, 4);
> device_create_file_fan(new_client, 5);
> device_create_file_fan(new_client, 6);
> device_create_file_fan(new_client, 7);
>
> device_create_file_temp(new_client, 1);
> device_create_file_temp(new_client, 2);
> device_create_file_temp(new_client, 3);
>
> device_create_file_fan_div(new_client, 1);
> device_create_file_fan_div(new_client, 2);
> device_create_file_fan_div(new_client, 3);
> device_create_file_fan_div(new_client, 4);
> device_create_file_fan_div(new_client, 5);
> device_create_file_fan_div(new_client, 6);
> device_create_file_fan_div(new_client, 7);
>
> device_create_file_pwm(new_client, 1);
> device_create_file_pwm(new_client, 2);
> device_create_file_pwm(new_client, 3);
>
> device_create_file_pwmenable(new_client, 1);
> device_create_file_pwmenable(new_client, 2);
> device_create_file_pwmenable(new_client, 3);
>
> device_create_file_pwm_mode(new_client, 1);
> device_create_file_pwm_mode(new_client, 2);
> device_create_file_pwm_mode(new_client, 3);
>
> device_create_file_chassis(new_client);
> device_create_file_chassis_clear(new_client);
>
> device_create_file_thermal_cruise(new_client, 1);
> device_create_file_thermal_cruise(new_client, 2);
> device_create_file_thermal_cruise(new_client, 3);
>
> device_create_file_tolerance(new_client, 1);
> device_create_file_tolerance(new_client, 2);
> device_create_file_tolerance(new_client, 3);
>
> device_create_file_sf2_point(new_client, 1, 1); /* Fan1 */
> device_create_file_sf2_point(new_client, 2, 1); /* Fan1 */
> device_create_file_sf2_point(new_client, 3, 1); /* Fan1 */
> device_create_file_sf2_point(new_client, 4, 1); /* Fan1 */
> device_create_file_sf2_point(new_client, 1, 2); /* Fan2 */
> device_create_file_sf2_point(new_client, 2, 2); /* Fan2 */
> device_create_file_sf2_point(new_client, 3, 2); /* Fan2 */
> device_create_file_sf2_point(new_client, 4, 2); /* Fan2 */
> device_create_file_sf2_point(new_client, 1, 3); /* Fan3 */
> device_create_file_sf2_point(new_client, 2, 3); /* Fan3 */
> device_create_file_sf2_point(new_client, 3, 3); /* Fan3 */
> device_create_file_sf2_point(new_client, 4, 3); /* Fan3 */
>
> device_create_file_sf2_level(new_client, 1, 1); /* Fan1 */
> device_create_file_sf2_level(new_client, 2, 1); /* Fan1 */
> device_create_file_sf2_level(new_client, 3, 1); /* Fan1 */
> device_create_file_sf2_level(new_client, 1, 2); /* Fan2 */
> device_create_file_sf2_level(new_client, 2, 2); /* Fan2 */
> device_create_file_sf2_level(new_client, 3, 2); /* Fan2 */
> device_create_file_sf2_level(new_client, 1, 3); /* Fan3 */
> device_create_file_sf2_level(new_client, 2, 3); /* Fan3 */
> device_create_file_sf2_level(new_client, 3, 3); /* Fan3 */
>
> return 0;
>
> ERROR2:
> i2c_detach_client(new_client);
> ERROR1:
> kfree(data);
> ERROR0:
> return err;
> }
>
> static int
> w83792d_detach_client(struct i2c_client *client)
I think one line too.
> {
> int err;
>
> if ((err = i2c_detach_client(client))) {
> dev_err(&client->dev,
> "Client deregistration failed, client not detached.\n");
> return err;
> }
>
> if (i2c_get_clientdata(client)=NULL) {
> /* subclients */
> kfree(client);
> } else {
> /* main client */
> kfree(i2c_get_clientdata(client));
> }
>
> return 0;
> }
>
> /* The SMBus locks itself, usually, but nothing may access the Winbond between
> bank switches. ISA access must always be locked explicitly!
> We ignore the W83792D BUSY flag at this moment - it could lead to deadlocks,
> would slow down the W83792D access and should not be necessary.
> There are some ugly typecasts here, but the good news is - they should
> nowhere else be necessary! */
> static int
> w83792d_read_value(struct i2c_client *client, u8 reg)
I think one line too.
> {
> int res=0;
> res = i2c_smbus_read_byte_data(client, reg);
>
> return res;
> }
>
> static int
> w83792d_write_value(struct i2c_client *client, u8 reg, u8 value)
I think one line too.
> {
> i2c_smbus_write_byte_data(client, reg, value);
> return 0;
> }
>
> /* Called when we have found a new W83792D. It should set limits, etc. */
> static void
> w83792d_init_client(struct i2c_client *client)
> {
> int temp2_cfg, temp3_cfg, i=0;
should not be u8 ? (except of i)
> u8 vid_in_b;
>
> if (init) {
> w83792d_write_value(client, W83792D_REG_CONFIG, 0x80);
> }
> /* Clear the bit6 of W83792D_REG_VID_IN_B(set it into 0):
> W83792D_REG_VID_IN_B bit6 = 0: the high/low limit of
> vin0/vin1 can be modified by user;
> W83792D_REG_VID_IN_B bit6 = 1: the high/low limit of
> vin0/vin1 auto-updated, can NOT be modified by user. */
> vid_in_b = w83792d_read_value(client, W83792D_REG_VID_IN_B);
> w83792d_write_value(client, W83792D_REG_VID_IN_B,
> vid_in_b & 0xbf);
>
> temp2_cfg = w83792d_read_value(client, W83792D_REG_TEMP2_CONFIG);
> temp3_cfg = w83792d_read_value(client, W83792D_REG_TEMP3_CONFIG);
> w83792d_write_value(client, W83792D_REG_TEMP2_CONFIG,
> temp2_cfg & 0xe6);
> w83792d_write_value(client, W83792D_REG_TEMP3_CONFIG,
> temp3_cfg & 0xe6);
>
> /* enable comparator mode for temp2 and temp3 so
> alarm indication will work correctly */
> i = w83792d_read_value(client, W83792D_REG_IRQ);
> if (!(i & 0x40)) {
> w83792d_write_value(client, W83792D_REG_IRQ,
> i | 0x40);
> }
>
> /* Start monitoring */
> w83792d_write_value(client, W83792D_REG_CONFIG,
> (w83792d_read_value(client,
> W83792D_REG_CONFIG) & 0xf7)
> | 0x01);
> }
>
> static struct w83792d_data *w83792d_update_device(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct w83792d_data *data = i2c_get_clientdata(client);
> int i, j;
> u8 reg_array_tmp[4], pwm_array_tmp[7], reg_tmp;
>
> down(&data->update_lock);
>
> if (time_after
> (jiffies - data->last_updated, (unsigned long) (HZ * 3))
> || time_before(jiffies, data->last_updated) || !data->valid) {
> dev_warn(dev, "Starting device update\n");
>
> /* Update the voltages measured value and limits */
> for (i = 0; i < 9; i++) {
> data->in[i] = w83792d_read_value(client,
> W83792D_REG_IN[i]);
> data->in_max[i] = w83792d_read_value(client,
> W83792D_REG_IN_MAX[i]);
> data->in_min[i] = w83792d_read_value(client,
> W83792D_REG_IN_MIN[i]);
> }
> data->low_bits[0] = w83792d_read_value(client,
> W83792D_REG_LOW_BITS1);
> data->low_bits[1] = w83792d_read_value(client,
> W83792D_REG_LOW_BITS2);
> for (i = 0; i < 7; i++) {
> /* Update the Fan measured value and limits */
> data->fan[i] = w83792d_read_value(client,
> W83792D_REG_FAN[i]);
> data->fan_min[i] = w83792d_read_value(client,
> W83792D_REG_FAN_MIN[i]);
> /* Update the PWM/DC Value and PWM/DC flag */
> pwm_array_tmp[i] = w83792d_read_value(client,
> W83792D_REG_PWM[i]);
> data->pwm[i] = pwm_array_tmp[i] & 0x0f;
> data->pwm_mode[i] = (pwm_array_tmp[i] >> 7) & 0x01;
> }
>
> reg_tmp = w83792d_read_value(client, W83792D_REG_FAN_CFG);
> data->pwmenable[0] = reg_tmp & 0x03;
> data->pwmenable[1] = (reg_tmp>>2) & 0x03;
> data->pwmenable[2] = (reg_tmp>>4) & 0x03;
>
> data->temp = w83792d_read_value(client, W83792D_REG_TEMP[0]);
> data->temp_max > w83792d_read_value(client, W83792D_REG_TEMP_OVER[0]);
> data->temp_max_hyst > w83792d_read_value(client, W83792D_REG_TEMP_HYST[0]);
> data->temp_add[0] > w83792d_read_value(client, W83792D_REG_TEMP[1]);
> data->temp_max_add[0] > w83792d_read_value(client, W83792D_REG_TEMP_OVER[1]);
> data->temp_max_hyst_add[0] > w83792d_read_value(client, W83792D_REG_TEMP_HYST[1]);
> data->temp_add[1] > w83792d_read_value(client, W83792D_REG_TEMP[2]);
> data->temp_max_add[1] > w83792d_read_value(client,
> W83792D_REG_TEMP_OVER[2]);
> data->temp_max_hyst_add[1] > w83792d_read_value(client,
> W83792D_REG_TEMP_HYST[2]);
>
> /* Update the Fan Divisor */
> for (i = 0; i < 4; i++) {
> reg_array_tmp[i] = w83792d_read_value(client, W83792D_REG_FAN_DIV[i]);
> }
> data->fan_div[0] = reg_array_tmp[0] & 0x07;
> data->fan_div[1] = (reg_array_tmp[0] >> 4) & 0x07;
> data->fan_div[2] = reg_array_tmp[1] & 0x07;
> data->fan_div[3] = (reg_array_tmp[1] >> 4) & 0x07;
> data->fan_div[4] = reg_array_tmp[2] & 0x07;
> data->fan_div[5] = (reg_array_tmp[2] >> 4) & 0x07;
> data->fan_div[6] = reg_array_tmp[3] & 0x07;
>
> /* Update CaseOpen status and it's CLR_CHS. */
> data->chassis = (w83792d_read_value(client,
> W83792D_REG_CHASSIS) >> 5) & 0x01;
> data->chassis_clear = (w83792d_read_value(client,
> W83792D_REG_CHASSIS_CLR) >> 7) & 0x01;
>
> /* Update Thermal Cruise/Smart Fan I target value */
> for (i = 0; i < 3; i++) {
> data->thermal_cruise[i] > w83792d_read_value(client,
> W83792D_REG_THERMAL[i]) & 0x7f;
> }
>
> /* Update Smart Fan I/II tolerance */
> reg_tmp = w83792d_read_value(client, W83792D_REG_TOLERANCE[0]);
> data->tolerance[0] = reg_tmp & 0x0f;
> data->tolerance[1] = (reg_tmp >> 4) & 0x0f;
> data->tolerance[2] > w83792d_read_value(client, W83792D_REG_TOLERANCE[2]) & 0x0f;
>
> /* Update Smart Fan II temperature points */
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 4; j++) {
> data->sf2_points[i][j] = w83792d_read_value(
> client,W83792D_REG_POINTS[i][j]) & 0x7f;
> }
> }
>
> /* Update Smart Fan II duty cycle levels */
> for (i = 0; i < 3; i++) {
> reg_tmp = w83792d_read_value(client,
> W83792D_REG_LEVELS[i][0]);
> data->sf2_levels[i][0] = reg_tmp & 0x0f;
> data->sf2_levels[i][1] = (reg_tmp >> 4) & 0x0f;
> reg_tmp = w83792d_read_value(client,
> W83792D_REG_LEVELS[i][2]);
> data->sf2_levels[i][2] = (reg_tmp >> 4) & 0x0f;
> data->sf2_levels[i][3] = reg_tmp & 0x0f;
> }
>
> data->last_updated = jiffies;
> data->valid = 1;
> }
>
> up(&data->update_lock);
>
> #ifdef W83792D_DEBUG
> w83792d_print_debug(data);
> #endif
Do not forget to change this too.
>
> return data;
> }
>
> #ifdef W83792D_DEBUG
and here :)
> static void w83792d_print_debug(struct w83792d_data *data)
> {
> int i=0;
> printk(KERN_DEBUG "=====The following is the debug message...====\n");
> printk(KERN_DEBUG "9 set of Voltages: ===>\n");
> for (i=0; i<=8; i++) {
> printk(KERN_DEBUG "vin[%d] is: 0x%x\n", i, data->in[i]);
> printk(KERN_DEBUG "vin[%d] max is: 0x%x\n", i, data->in_max[i]);
> printk(KERN_DEBUG "vin[%d] min is: 0x%x\n", i, data->in_min[i]);
> }
> printk(KERN_DEBUG "Low Bit1 is: 0x%x\n", data->low_bits[0]);
> printk(KERN_DEBUG "Low Bit2 is: 0x%x\n", data->low_bits[1]);
> printk(KERN_DEBUG "7 set of Fan Counts and Duty Cycles: ===>\n");
> /* printk(KERN_DEBUG "fan_cfg is: 0x%x\n", data->fan_cfg); */
> for (i=0; i<=6; i++) {
> printk(KERN_DEBUG "fan[%d] is: 0x%x\n", i, data->fan[i]);
> printk(KERN_DEBUG "fan[%d] min is: 0x%x\n", i, data->fan_min[i]);
> printk(KERN_DEBUG "pwm[%d] is: 0x%x\n", i, data->pwm[i]);
> printk(KERN_DEBUG "pwm_mode[%d] is: 0x%x\n", i, data->pwm_mode[i]);
> }
> printk(KERN_DEBUG "3 set of Temperatures: ===>\n");
> printk(KERN_DEBUG "temp1 is: 0x%x\n", data->temp);
> printk(KERN_DEBUG "temp1 high limit is: 0x%x\n", data->temp_max);
> printk(KERN_DEBUG "temp1 low limit is: 0x%x\n", data->temp_max_hyst);
>
> printk(KERN_DEBUG "temp2 is: 0x%x\n", data->temp_add[0]);
> printk(KERN_DEBUG "temp2 high limit is: 0x%x\n", data->temp_max_add[0]);
> printk(KERN_DEBUG "temp2 low limit is: 0x%x\n", data->temp_max_hyst_add[0]);
>
> printk(KERN_DEBUG "temp3 is: 0x%x\n", data->temp_add[1]);
> printk(KERN_DEBUG "temp3 high limit is: 0x%x\n", data->temp_max_add[1]);
> printk(KERN_DEBUG "temp3 low limit is: 0x%x\n", data->temp_max_hyst_add[1]);
>
> for (i=0; i<=6; i++) {
> printk(KERN_DEBUG "fan_div[%d] is: 0x%x\n", i, data->fan_div[i]);
> }
> printk(KERN_DEBUG "=====End of the debug message...=========\n");
> printk(KERN_DEBUG "\n");
> }
> #endif
>
> static int __init
> sensors_w83792d_init(void)
> {
> return i2c_add_driver(&w83792d_driver);
> }
>
> static void __exit
> sensors_w83792d_exit(void)
> {
> i2c_del_driver(&w83792d_driver);
> }
>
> MODULE_AUTHOR("Chunhao Huang @ Winbond");
No email?
> MODULE_DESCRIPTION("W83792AD/D driver for linux-2.6");
> MODULE_LICENSE("GPL");
>
> module_init(sensors_w83792d_init);
> module_exit(sensors_w83792d_exit);
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lm-sensors] RE: W83792 review
2005-05-26 19:25 [lm-sensors] W83792 review Rudolf Marek
@ 2005-05-27 3:33 ` Huang0
2005-05-28 15:31 ` [lm-sensors] " Jean Delvare
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Huang0 @ 2005-05-27 3:33 UTC (permalink / raw)
To: lm-sensors
Hi Rudolf, Jean
Thank you very much for your review to my codes, I read it briefly just now, you
gave me the help in detail.:-)
I will fix some of them ASAP when I'm a little free, maybe this weekend such as
tomorrow or the day after tomorrow. I will read your review again carefully
before I'll start to modify my codes. If your comments are really right, I
will fix them and give you the response, If I disagree some of your comments,
I will also send you the response to discuss with you again.
I wish some other guys can give me more suggestion to my codes, as well as
to your comments. Where is Jean? It seems that he is busy these days? He gave
me many good suggestions to the 792 driver for linux-2.4
Thanks
Best Regards
Chunhao
> -----Original Message-----
> From: Rudolf Marek [mailto:r.marek@sh.cvut.cz]
> Sent: 2005爛5堎27゜ 1:22
> To: PI14 HUANG0; lm-sensors@lm-sensors.org
> Subject: W83792 review
>
> Hello,
>
> I reviewed your driver. Please check my comments.
> I'm inviting other developers to check my objections I need to develop "review
> skill". - Thanks
>
> >(1) We misunderstand the meaning of temperature low limit, this bug also
> >exists in the 792 driver for linux-2.4 and our 792 Windows driver.
>
> Please can you fix this ASAP so we can send it to kernel folks?
>
> >(2) I have not implemented the 0.5 degree to temperature2 and temperature3,
> >while the driver for linux-2.4 implemented this.
>
> That is the reason temp1 has separate handling and variable? If so, please add
> the code.
> or better would be to fix existing code so it does not look so strange.
>
> >(3) The 792 driver for linux-2.4 can adjust the fan divisor automatically,
> >while I have not implemented it in this 2.6 one.
>
> This would be nice but better to put there some driver instead of none...
> I think this can wait but it is good to have it soon.
>
> Also we will need similar documentation to driver similar what 2.4 driver has.
>
> You can find some inspiration on style of document here:
> http://ssh.cz/~ruik/chips_doc13
> Please document there the extra /sys entries for your driver.
> And as last thing that we will need is KConfig & Makefile entry for the driver.
>
> I know that you are short with time so I'm offering help with documentation
> and KConfig and Makefile.
> On the other hand I'm expecting that you will fix other stuff in reasonable
> time :)
>
> Regards
>
> Rudolf
>
>
> > /*
> > w83792d.c - Part of lm_sensors, Linux kernel modules for hardware
> > monitoring
> > Copyright (C) 2004, 2005 Winbond Electronics Corp.
> > Chunhao Huang <huang0@winbond.com.tw>
> >
> > 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.
> >
> > Note:
> > 1. This driver is only for 2.6 kernel, 2.4 kernel need a different driver.
> > 2. This driver is only for Winbond W83792D C version device, there
> > are also some motherboards with B version W83792D device. The
> > calculation method to in6-in7(measured value, limits) is a little
> > different between C and B version. C or B version can be identified
> > by CR[0x49h].
> > */
> >
> > /*
> > Supports following chips:
> >
> > Chip #vin #fanin #pwm #temp wchipid vendid i2c ISA
> > w83792d 9 7 7 3 0x7a 0x5ca3 yes no
> > */
> >
> > #include <linux/config.h>
> > #include <linux/module.h>
> > #include <linux/init.h>
> > #include <linux/slab.h>
> > #include <linux/i2c.h>
> > #include <linux/i2c-sensor.h>
> > #include <linux/i2c-vid.h>
> > #include <asm/io.h>
> Do you really need this header?
>
> > /* #include "lm75.h" */
> Please remove this.
>
> >
> >
> > /* Addresses to scan */
> > static unsigned short normal_i2c[] = { 0x2c, 0x2f, I2C_CLIENT_END };
> > static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
> >
> > /* Insmod parameters */
> > SENSORS_INSMOD_1(w83792d);
> > I2C_CLIENT_MODULE_PARM(force_subclients, "List of subclient addresses: "
> > "{bus, clientaddr, subclientaddr1, subclientaddr2}");
> >
> > static int init;
> > module_param(init, bool, 0);
> > MODULE_PARM_DESC(init, "Set to one for chip initialization");
>
> Maybe better "Set to one to force chip initialization"
>
> >
> > /* show/display the debug message */
> > #define W83792D_DEBUG 1
>
> There is some global #define for debugging called DEBUG it seems, so maybe it
> would be good
> to remove this and use DEBUG later.
>
> > /* The W83792D registers */
> > static const u8 W83792D_REG_IN[9] = {
> > 0x20, /* Vcore A in DataSheet */
> > 0x21, /* Vcore B in DataSheet */
> > 0x22, /* VIN0 in DataSheet */
> > 0x23, /* VIN1 in DataSheet */
> > 0x24, /* VIN2 in DataSheet */
> > 0x25, /* VIN3 in DataSheet */
> > 0x26, /* 5VCC in DataSheet */
> > 0xB0, /* 5VSB in DataSheet */
> > 0xB1 /* VBAT in DataSheet */
> > };
> > #define W83792D_REG_LOW_BITS1 0x3E /* Low Bits I in DataSheet */
> > #define W83792D_REG_LOW_BITS2 0x3F /* Low Bits II in DataSheet */
> > static const u8 W83792D_REG_IN_MAX[9] = {
> > 0x2B, /* Vcore A High Limit in DataSheet */
> > 0x2D, /* Vcore B High Limit in DataSheet */
> > 0x2F, /* VIN0 High Limit in DataSheet */
> > 0x31, /* VIN1 High Limit in DataSheet */
> > 0x33, /* VIN2 High Limit in DataSheet */
> > 0x35, /* VIN3 High Limit in DataSheet */
> > 0x37, /* 5VCC High Limit in DataSheet */
> > 0xB4, /* 5VSB High Limit in DataSheet */
> > 0xB6 /* VBAT High Limit in DataSheet */
> > };
> > static const u8 W83792D_REG_IN_MIN[9] = {
> > 0x2C, /* Vcore A Low Limit in DataSheet */
> > 0x2E, /* Vcore B Low Limit in DataSheet */
> > 0x30, /* VIN0 Low Limit in DataSheet */
> > 0x32, /* VIN1 Low Limit in DataSheet */
> > 0x34, /* VIN2 Low Limit in DataSheet */
> > 0x36, /* VIN3 Low Limit in DataSheet */
> > 0x38, /* 5VCC Low Limit in DataSheet */
> > 0xB5, /* 5VSB Low Limit in DataSheet */
> > 0xB7 /* VBAT Low Limit in DataSheet */
> > };
> > static const u8 W83792D_REG_FAN[7] = {
> > 0x28, /* FAN 1 Count in DataSheet */
> > 0x29, /* FAN 2 Count in DataSheet */
> > 0x2A, /* FAN 3 Count in DataSheet */
> > 0xB8, /* FAN 4 Count in DataSheet */
> > 0xB9, /* FAN 5 Count in DataSheet */
> > 0xBA, /* FAN 6 Count in DataSheet */
> > 0xBE /* FAN 7 Count in DataSheet */
> > };
> > static const u8 W83792D_REG_FAN_MIN[7] = {
> > 0x3B, /* FAN 1 Count Low Limit in DataSheet */
> > 0x3C, /* FAN 2 Count Low Limit in DataSheet */
> > 0x3D, /* FAN 3 Count Low Limit in DataSheet */
> > 0xBB, /* FAN 4 Count Low Limit in DataSheet */
> > 0xBC, /* FAN 5 Count Low Limit in DataSheet */
> > 0xBD, /* FAN 6 Count Low Limit in DataSheet */
> > 0xBF /* FAN 7 Count Low Limit in DataSheet */
> > };
> > #define W83792D_REG_FAN_CFG 0x84 /* FAN Configuration in DataSheet */
> > static const u8 W83792D_REG_FAN_DIV[4] = {
> > 0x47, /* contains FAN2 and FAN1 Divisor */
> > 0x5B, /* contains FAN4 and FAN3 Divisor */
> > 0x5C, /* contains FAN6 and FAN5 Divisor */
> > 0x9E /* contains FAN7 Divisor. */
> > };
> > static const u8 W83792D_REG_PWM[7] = {
> > 0x81, /* FAN 1 Duty Cycle, be used to control */
> > 0x83, /* FAN 2 Duty Cycle, be used to control */
> > 0x94, /* FAN 3 Duty Cycle, be used to control */
> > 0xA3, /* FAN 4 Duty Cycle, be used to control */
> > 0xA4, /* FAN 5 Duty Cycle, be used to control */
> > 0xA5, /* FAN 6 Duty Cycle, be used to control */
> > 0xA6 /* FAN 7 Duty Cycle, be used to control */
> > };
> > #define W83792D_REG_BANK 0x4E
> > #define W83792D_REG_TEMP2_CONFIG 0xC2
> > #define W83792D_REG_TEMP3_CONFIG 0xCA
> >
> > static const u8 W83792D_REG_TEMP[3] = {
> > 0x27, /* TEMP 1 in DataSheet */
> > 0xC0, /* TEMP 2 in DataSheet */
> > 0xC8 /* TEMP 3 in DataSheet */
> > };
> > static const u8 W83792D_REG_TEMP_HYST[3] = {
> > 0x3A, /* TEMP 1 Hyst in DataSheet */
> > 0xC3, /* TEMP 2 Hyst in DataSheet */
> > 0xCB /* TEMP 3 Hyst in DataSheet */
> > };
> > static const u8 W83792D_REG_TEMP_OVER[3] = {
> > 0x39, /* TEMP 1 Over in DataSheet */
> > 0xC5, /* TEMP 2 Over in DataSheet */
> > 0xCD /* TEMP 3 Over in DataSheet */
> > };
> >
> > static const u8 W83792D_REG_THERMAL[3] = {
> > 0x85, /* SmartFanI: Fan1 target value */
> > 0x86, /* SmartFanI: Fan2 target value */
> > 0x96 /* SmartFanI: Fan3 target value */
> > };
> >
> > static const u8 W83792D_REG_TOLERANCE[3] = {
> > 0x87, /* (bit3-0)SmartFan Fan1 tolerance */
> > 0x87, /* (bit7-4)SmartFan Fan2 tolerance */
> > 0x97 /* (bit3-0)SmartFan Fan3 tolerance */
> > };
> >
> > static const u8 W83792D_REG_POINTS[3][4] = {
> > { 0x85, /* SmartFanII: Fan1 temp point 1 */
> > 0xE3, /* SmartFanII: Fan1 temp point 2 */
> > 0xE4, /* SmartFanII: Fan1 temp point 3 */
> > 0xE5 }, /* SmartFanII: Fan1 temp point 4 */
> > { 0x86, /* SmartFanII: Fan2 temp point 1 */
> > 0xE6, /* SmartFanII: Fan2 temp point 2 */
> > 0xE7, /* SmartFanII: Fan2 temp point 3 */
> > 0xE8 }, /* SmartFanII: Fan2 temp point 4 */
> > { 0x96, /* SmartFanII: Fan3 temp point 1 */
> > 0xE9, /* SmartFanII: Fan3 temp point 2 */
> > 0xEA, /* SmartFanII: Fan3 temp point 3 */
> > 0xEB } /* SmartFanII: Fan3 temp point 4 */
> > };
> >
> > static const u8 W83792D_REG_LEVELS[3][4] = {
> > { 0x88, /* (bit3-0) SmartFanII: Fan1 Non-Stop */
> > 0x88, /* (bit7-4) SmartFanII: Fan1 Level 1 */
> > 0xE0, /* (bit7-4) SmartFanII: Fan1 Level 2 */
> > 0xE0 }, /* (bit3-0) SmartFanII: Fan1 Level 3 */
> > { 0x89, /* (bit3-0) SmartFanII: Fan2 Non-Stop */
> > 0x89, /* (bit7-4) SmartFanII: Fan2 Level 1 */
> > 0xE1, /* (bit7-4) SmartFanII: Fan2 Level 2 */
> > 0xE1 }, /* (bit3-0) SmartFanII: Fan2 Level 3 */
> > { 0x98, /* (bit3-0) SmartFanII: Fan3 Non-Stop */
> > 0x98, /* (bit7-4) SmartFanII: Fan3 Level 1 */
> > 0xE2, /* (bit7-4) SmartFanII: Fan3 Level 2 */
> > 0xE2 } /* (bit3-0) SmartFanII: Fan3 Level 3 */
> > };
> >
> > #define W83792D_REG_CONFIG 0x40
> > #define W83792D_REG_IRQ 0x4C
> > #define W83792D_REG_VID_FANDIV 0x47
> > #define W83792D_REG_CHIPID 0x49
> > #define W83792D_REG_WCHIPID 0x58
> > #define W83792D_REG_CHIPMAN 0x4F
> > #define W83792D_REG_PIN 0x4B
> > #define W83792D_REG_I2C_SUBADDR 0x4A
> >
> > #define W83792D_REG_CHASSIS 0x42 /* Bit 5: Case Open status bit */
> > #define W83792D_REG_CHASSIS_CLR 0x44 /* Bit 7: Case Open CLR_CHS/Reset
> bit */
> >
> > /* ctroll in0/in1 's limit modifiability */
> perhap a typo ?
>
> > #define W83792D_REG_VID_IN_B 0x17
> >
> > #define W83792D_REG_VBAT 0x5D
> > #define W83792D_REG_I2C_ADDR 0x48
> >
> > /* Conversions. Rounding and limit checking is only done on the TO_REG
> > variants. Note that you should be a bit careful with which arguments
> > these macros are called: arguments may be evaluated more than once.
> > Fixing this is just not worth it. */
> > #define IN_FROM_REG(nr,val) (((nr)<=1)?(val*2): \
> > ((((nr)==6)||((nr)==7))?(val*6):(val*4)))
> > #define IN_TO_REG(nr,val) (((nr)<=1)?(val/2): \
> > ((((nr)==6)||((nr)==7))?(val/6):(val/4)))
> >
> > static inline u8
> > FAN_TO_REG(long rpm, int div)
> > {
>
> I'm not sure, but I think it should be like
>
> static inline u8 FAN_TO_REG(long rpm, int div)
> {
>
>
> > if (rpm == 0)
> > return 255;
> > rpm = SENSORS_LIMIT(rpm, 1, 1000000);
> > return SENSORS_LIMIT((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
> > }
> >
> > #define FAN_FROM_REG(val,div) ((val) == 0 ? -1 : \
> > ((val) == 255 ? 0 : \
> > 1350000 / ((val) * (div))))
> >
> > #define TEMP_TO_REG(val) (SENSORS_LIMIT(((val) < 0 ?
> (val)+0x100*1000 \
> > : (val)) / 1000, 0, 0xff))
> > #define TEMP_FROM_REG(val) (((val) & 0x80 ? (val)-0x100 : (val))
> * 1000)
> >
> > #define PWM_FROM_REG(val) (val)
> > #define PWM_TO_REG(val) (SENSORS_LIMIT((val),0,255))
> > #define DIV_FROM_REG(val) (1 << (val))
> >
> > static inline u8
> > DIV_TO_REG(long val)
> > {
>
> Same here?
>
> > int i;
> > val = SENSORS_LIMIT(val, 1, 128) >> 1;
> > for (i = 0; i < 6; i++) {
> > if (val == 0)
> > break;
> > val >>= 1;
> > }
> > return ((u8) i);
> > }
> >
> > struct w83792d_data {
> > struct i2c_client client;
> > struct semaphore lock;
> > enum chips type;
> >
> > struct semaphore update_lock;
> > char valid; /* !=0 if following fields are valid */
> > unsigned long last_updated; /* In jiffies */
> >
> > /* array of 2 pointers to subclients */
> > struct i2c_client *lm75[2];
>
> The subclients are lm75 compatible? (I think so), but I'm not sure about the
> name...
>
> > u8 in[9]; /* Register value */
> > u8 in_max[9]; /* Register value */
> > u8 in_min[9]; /* Register value */
> > u8 low_bits[2]; /* Register value */
>
> Please comment that lowbits belong to IN, or choose better name for the variable
>
> > u8 fan[7]; /* Register value */
> > u8 fan_min[7]; /* Register value */
> > u8 temp;
> > u8 temp_max; /* Register value */
> > u8 temp_max_hyst; /* Register value */
> > u8 temp_add[2]; /* Register value */
>
> Please align comment with tab to same col as above.
>
> Also why is temp1 separated? why there is not temps[3] instead?
>
>
> > u8 temp_max_add[2]; /* Register value */
> > u8 temp_max_hyst_add[2]; /* Register value */
>
> Big I and align too please
>
> > u8 fan_div[7]; /* Register encoding, shifted right */
> > u8 pwm[7]; /* We only consider the first 3 set of pwm,
> > although 792 chip has 7 set of pwm. */
> > u8 pwmenable[3];
> > u8 pwm_mode[7]; /* indicates PWM or DC mode: 1->PWM; 0->DC */
> > u8 chassis; /* Chassis status */
> > u8 chassis_clear; /* CLR_CHS, clear chassis intrusion detection */
> > u8 thermal_cruise[3]; /* Smart FanI: Fan1,2,3 target value */
> > u8 tolerance[3]; /* Fan1,2,3 tolerance(Smart Fan I/II) */
> > u8 sf2_points[3][4]; /* Smart FanII: Fan1,2,3 temperature points */
> > u8 sf2_levels[3][4]; /* Smart FanII: Fan1,2,3 duty cycle levels */
> > };
> >
> > static int w83792d_attach_adapter(struct i2c_adapter *adapter);
> > static int w83792d_detect(struct i2c_adapter *adapter, int address, int
> kind);
> > static int w83792d_detach_client(struct i2c_client *client);
> >
> > static int w83792d_read_value(struct i2c_client *client, u8 register);
> > static int w83792d_write_value(struct i2c_client *client, u8 register,
> > u8 value);
> > static struct w83792d_data *w83792d_update_device(struct device *dev);
> >
> > #ifdef W83792D_DEBUG
> > static void w83792d_print_debug(struct w83792d_data *data);
> > #endif
>
> I think you can change it to: #ifdef DEBUG
>
> > static void w83792d_init_client(struct i2c_client *client);
> >
> > static struct i2c_driver w83792d_driver = {
> > .owner = THIS_MODULE,
> > .name = "w83792d",
> > .flags = I2C_DF_NOTIFY,
> > .attach_adapter = w83792d_attach_adapter,
> > .detach_client = w83792d_detach_client,
> > };
> >
> > static long in_count_from_reg(int nr, struct w83792d_data *data)
> > {
> > u16 vol_count = data->in[nr];
> > u16 low_bits = 0;
> > vol_count = (vol_count << 2);
> > switch (nr)
> > {
> > case 0: /* vin0 */
> > low_bits = (data->low_bits[0]) & 0x03;
> > break;
> > case 1: /* vin1 */
> > low_bits = ((data->low_bits[0]) & 0x0c) >> 2;
> > break;
> > case 2: /* vin2 */
> > low_bits = ((data->low_bits[0]) & 0x30) >> 4;
> > break;
> > case 3: /* vin3 */
> > low_bits = ((data->low_bits[0]) & 0xc0) >> 6;
> > break;
> > case 4: /* vin4 */
> > low_bits = (data->low_bits[1]) & 0x03;
> > break;
> > case 5: /* vin5 */
> > low_bits = ((data->low_bits[1]) & 0x0c) >> 2;
> > break;
> > case 6: /* vin6 */
> > low_bits = ((data->low_bits[1]) & 0x30) >> 4;
> > default:
> > break;
> > }
> > vol_count = vol_count | low_bits;
> > return vol_count;
> > }
> >
> > /* following are the sysfs callback functions */
> > static ssize_t show_in(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf,"%ld\n", IN_FROM_REG(nr,(in_count_from_reg(nr,
> data))));
> > }
> >
> > #define show_in_reg(reg) \
> > static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> > { \
> > struct w83792d_data *data = w83792d_update_device(dev); \
> > return sprintf(buf,"%ld\n", (long)(IN_FROM_REG(nr,
> (data->reg[nr])*4))); \
> > }
> >
> > show_in_reg(in_min);
> > show_in_reg(in_max);
> >
> > #define store_in_reg(REG, reg) \
> > static ssize_t store_in_##reg (struct device *dev, const char *buf, size_t
> count, int nr) \
> > { \
> > struct i2c_client *client = to_i2c_client(dev); \
> > struct w83792d_data *data = i2c_get_clientdata(client); \
> > u32 val; \
> > \
> > val = simple_strtoul(buf, NULL, 10); \
> > data->in_##reg[nr] = SENSORS_LIMIT(IN_TO_REG(nr, val)/4, 0, 255); \
> > w83792d_write_value(client, W83792D_REG_IN_##REG[nr],
> data->in_##reg[nr]); \
> > \
> > return count; \
> > }
> > store_in_reg(MIN, min);
> > store_in_reg(MAX, max);
> >
> > #define sysfs_in_offset(offset) \
> > static ssize_t \
> > show_regs_in_##offset (struct device *dev, char *buf) \
> > { \
> > return show_in(dev, buf, offset); \
> > } \
> > static DEVICE_ATTR(in##offset##_input, S_IRUGO, show_regs_in_##offset,
> NULL);
> >
> > #define sysfs_in_reg_offset(reg, offset) \
> > static ssize_t show_regs_in_##reg##offset (struct device *dev, char *buf)
> \
> > { \
> > return show_in_##reg (dev, buf, offset); \
> > } \
> > static ssize_t store_regs_in_##reg##offset (struct device *dev, const char
> *buf, size_t count) \
> > { \
> > return store_in_##reg (dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(in##offset##_##reg, S_IRUGO| S_IWUSR,
> show_regs_in_##reg##offset, store_regs_in_##reg##offset);
> >
> > #define sysfs_in_offsets(offset) \
> > sysfs_in_offset(offset); \
> > sysfs_in_reg_offset(min, offset); \
> > sysfs_in_reg_offset(max, offset);
> >
> > sysfs_in_offsets(0);
> > sysfs_in_offsets(1);
> > sysfs_in_offsets(2);
> > sysfs_in_offsets(3);
> > sysfs_in_offsets(4);
> > sysfs_in_offsets(5);
> > sysfs_in_offsets(6);
> > sysfs_in_offsets(7);
> > sysfs_in_offsets(8);
> >
> > #define device_create_file_in(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_in##offset##_input); \
> > device_create_file(&client->dev, &dev_attr_in##offset##_min); \
> > device_create_file(&client->dev, &dev_attr_in##offset##_max); \
> > } while (0)
> >
> > #define show_fan_reg(reg) \
> > static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> > { \
> > struct w83792d_data *data = w83792d_update_device(dev); \
> > return sprintf(buf,"%ld\n", \
> > FAN_FROM_REG(data->reg[nr-1],
> (long)DIV_FROM_REG(data->fan_div[nr-1]))); \
> > }
> > show_fan_reg(fan);
> > show_fan_reg(fan_min);
> >
> > static ssize_t
> > store_fan_min(struct device *dev, const char *buf, size_t count, int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> >
> > val = simple_strtoul(buf, NULL, 10);
> > data->fan_min[nr - 1] =
> > FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr - 1]));
>
> Please use second tab instead of 4 spaces
>
> > w83792d_write_value(client, W83792D_REG_FAN_MIN[nr-1],
> > data->fan_min[nr - 1]);
> >
> > return count;
> > }
> >
> > #define sysfs_fan_offset(offset) \
> > static ssize_t show_regs_fan_##offset (struct device *dev, char *buf) \
> > { \
> > return show_fan(dev, buf, offset); \
> > } \
> > static DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_regs_fan_##offset,
> NULL);
> >
> > #define sysfs_fan_min_offset(offset) \
> > static ssize_t show_regs_fan_min##offset (struct device *dev, char *buf) \
> > { \
> > return show_fan_min(dev, buf, offset); \
> > } \
> > static ssize_t store_regs_fan_min##offset (struct device *dev, const char
> *buf, size_t count) \
> > { \
> > return store_fan_min(dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR,
> show_regs_fan_min##offset, store_regs_fan_min##offset);
> >
> > sysfs_fan_offset(1);
> > sysfs_fan_min_offset(1);
> > sysfs_fan_offset(2);
> > sysfs_fan_min_offset(2);
> > sysfs_fan_offset(3);
> > sysfs_fan_min_offset(3);
> > sysfs_fan_offset(4);
> > sysfs_fan_min_offset(4);
> > sysfs_fan_offset(5);
> > sysfs_fan_min_offset(5);
> > sysfs_fan_offset(6);
> > sysfs_fan_min_offset(6);
> > sysfs_fan_offset(7);
> > sysfs_fan_min_offset(7);
> >
> > #define device_create_file_fan(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_fan##offset##_input); \
> > device_create_file(&client->dev, &dev_attr_fan##offset##_min); \
> > } while (0)
> >
> > #define show_temp_reg(reg) \
> > static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> > { \
> > struct w83792d_data *data = w83792d_update_device(dev); \
> > if (nr >= 2) { /* TEMP2 and TEMP3 */ \
> > return sprintf(buf,"%ld\n", \
> > (long)TEMP_FROM_REG(data->reg##_add[nr-2])); \
> > } else { /* TEMP1 */ \
> > return sprintf(buf,"%ld\n", (long)TEMP_FROM_REG(data->reg)); \
> > } \
> > }
>
> Same question related here? Why temp1 separate?
>
>
> > show_temp_reg(temp);
> > show_temp_reg(temp_max);
> > show_temp_reg(temp_max_hyst);
> >
> > #define store_temp_reg(REG, reg) \
> > static ssize_t store_temp_##reg (struct device *dev, const char *buf, size_t
> count, int nr) \
> > { \
> > struct i2c_client *client = to_i2c_client(dev); \
> > struct w83792d_data *data = i2c_get_clientdata(client); \
> > s32 val; \
> > \
> > val = simple_strtol(buf, NULL, 10); \
> > \
> > if (nr >= 2) { \
> > data->temp_##reg##_add[nr-2] = TEMP_TO_REG(val); \
> > w83792d_write_value(client, W83792D_REG_TEMP_##REG[nr-1], \
> > data->temp_##reg##_add[nr-2]); \
> > } else { \
> > data->temp_##reg = TEMP_TO_REG(val); \
> > w83792d_write_value(client, W83792D_REG_TEMP_##REG[nr-1], \
> > data->temp_##reg); \
> > } \
> > \
> > return count; \
> > }
>
> Same question related here? Why temp1 separate?
>
> > store_temp_reg(OVER, max);
> > store_temp_reg(HYST, max_hyst);
> >
> > #define sysfs_temp_offset(offset) \
> > static ssize_t \
> > show_regs_temp_##offset (struct device *dev, char *buf) \
> > { \
> > return show_temp(dev, buf, offset); \
> > } \
> > static DEVICE_ATTR(temp##offset##_input, S_IRUGO, show_regs_temp_##offset,
> NULL);
> >
> > #define sysfs_temp_reg_offset(reg, offset) \
> > static ssize_t show_regs_temp_##reg##offset (struct device *dev, char *buf)
> \
> > { \
> > return show_temp_##reg (dev, buf, offset); \
> > } \
> > static ssize_t store_regs_temp_##reg##offset (struct device *dev, const char
> *buf, size_t count) \
> > { \
> > return store_temp_##reg (dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(temp##offset##_##reg, S_IRUGO| S_IWUSR,
> show_regs_temp_##reg##offset, store_regs_temp_##reg##offset);
> >
> > #define sysfs_temp_offsets(offset) \
> > sysfs_temp_offset(offset); \
> > sysfs_temp_reg_offset(max, offset); \
> > sysfs_temp_reg_offset(max_hyst, offset);
> >
> > sysfs_temp_offsets(1);
> > sysfs_temp_offsets(2);
> > sysfs_temp_offsets(3);
> >
> > #define device_create_file_temp(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_temp##offset##_input); \
> > device_create_file(&client->dev, &dev_attr_temp##offset##_max); \
> > device_create_file(&client->dev, &dev_attr_temp##offset##_max_hyst); \
> > } while (0)
> >
> > static ssize_t
> > show_fan_div_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n",
> > (long) DIV_FROM_REG(data->fan_div[nr - 1]));
> > }
> >
> > /* Note: we save and restore the fan minimum here, because its value is
> > determined in part by the fan divisor. This follows the principle of
> > least suprise; the user doesn't expect the fan minimum to change just
> > because the divisor changed. */
> > static ssize_t
> > store_fan_div_reg(struct device *dev, const char *buf, size_t count, int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > unsigned long min;
> > /*u8 reg;*/
>
> Please remove
>
> > u8 fan_div_reg=0; u8 tmp_fan_div;
>
> Please separate them on two lines.
>
> > /* Save fan_min */
> > min = FAN_FROM_REG(data->fan_min[nr],
> > DIV_FROM_REG(data->fan_div[nr]));
> >
> > data->fan_div[nr] = DIV_TO_REG(simple_strtoul(buf, NULL, 10));
> >
> > fan_div_reg = w83792d_read_value(client, W83792D_REG_FAN_DIV[nr/2]);
>
> nr/2 could be [nr >> 1]
>
> > fan_div_reg &= (nr%2 == 0) ? 0xf8 : 0x8f;
>
> and here
>
> fan_div_reg &= (nr & 1) ? 0x8f : 0xf8;
>
> > tmp_fan_div = (nr%2 == 0) ? ((data->fan_div[nr])&0x07)
> > : (((data->fan_div[nr])<<4)&0x70);
>
> This needs definetly spaces between operators but it can be also done this way:
>
> tmp_fan_div = (nr & 1) ? ((data->fan_div[nr] << 4) & 0x70)
> : data->fan_div[nr] & 0x07;
>
>
>
> > w83792d_write_value(client, W83792D_REG_FAN_DIV[nr/2],
>
> nr >> 1
> > fan_div_reg|tmp_fan_div);
> >
> > /* Restore fan_min */
> > data->fan_min[nr] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
> > w83792d_write_value(client, W83792D_REG_FAN_MIN[nr],
> data->fan_min[nr]);
> >
> > return count;
> > }
> >
> > #define sysfs_fan_div(offset) \
> > static ssize_t show_regs_fan_div_##offset (struct device *dev, char *buf)
> \
> > { \
> > return show_fan_div_reg(dev, buf, offset); \
> > } \
> > static ssize_t store_regs_fan_div_##offset (struct device *dev, const char
> *buf, size_t count) \
> > { \
> > return store_fan_div_reg(dev, buf, count, offset - 1); \
> > } \
> > static DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR,
> show_regs_fan_div_##offset, store_regs_fan_div_##offset);
> >
> > sysfs_fan_div(1);
> > sysfs_fan_div(2);
> > sysfs_fan_div(3);
> > sysfs_fan_div(4);
> > sysfs_fan_div(5);
> > sysfs_fan_div(6);
> > sysfs_fan_div(7);
> >
> > #define device_create_file_fan_div(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_fan##offset##_div); \
> > } while (0)
> >
> > static ssize_t
> > show_pwm_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n", (long) PWM_FROM_REG(data->pwm[nr-1]));
> > }
> >
> > static ssize_t
> > show_pwmenable_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > long pwm_enable_tmp = 1;
>
> Should be u32 instead?
>
> > if (data->pwmenable[nr-1] == 0) {
> > pwm_enable_tmp = 1; /* manual mode */
> > } else if (data->pwmenable[nr-1] == 1) {
> > pwm_enable_tmp = 3; /* thermal cruise/Smart Fan I */
> > } else if (data->pwmenable[nr-1] == 2) {
> > pwm_enable_tmp = 2; /* Smart Fan II */
> > }
> > return sprintf(buf, "%ld\n", pwm_enable_tmp);
> > }
> >
> > static ssize_t
> > store_pwm_reg(struct device *dev, const char *buf, size_t count, int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->pwm[nr-1] = PWM_TO_REG(val);
> > w83792d_write_value(client, W83792D_REG_PWM[nr-1], data->pwm[nr-1]);
> >
> > return count;
> > }
> >
> > static ssize_t
> > store_pwmenable_reg(struct device *dev, const char *buf, size_t count, int
> nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 fan_cfg_tmp, cfg1_tmp, cfg2_tmp, cfg3_tmp, cfg4_tmp;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > switch (val) {
> > case 1:
> > data->pwmenable[nr-1] = 0; /* manual mode */
> > break;
> > case 2:
> > data->pwmenable[nr-1] = 2; /* Smart Fan II */
> > break;
> > case 3:
> > data->pwmenable[nr-1] = 1; /* thermal cruise/Smart Fan I */
> > break;
> > default:
> > return -EINVAL;
> > }
> > cfg1_tmp = data->pwmenable[0];
> > cfg2_tmp = (data->pwmenable[1]) << 2;
> > cfg3_tmp = (data->pwmenable[2]) << 4;
> > cfg4_tmp = w83792d_read_value(client,W83792D_REG_FAN_CFG) & 0xc0;
> > fan_cfg_tmp = ((cfg4_tmp|cfg3_tmp)|cfg2_tmp)|cfg1_tmp;
>
> Missing spaces
> fan_cfg_tmp = ((cfg4_tmp | cfg3_tmp) | cfg2_tmp) | cfg1_tmp;
>
> > w83792d_write_value(client,W83792D_REG_FAN_CFG,fan_cfg_tmp);
> >
> > return count;
> > }
> >
> > #define sysfs_pwm(offset) \
> > static ssize_t show_regs_pwm_##offset (struct device *dev, char *buf) \
> > { \
> > return show_pwm_reg(dev, buf, offset); \
> > } \
> > static ssize_t store_regs_pwm_##offset (struct device *dev, \
> > const char *buf, size_t count) \
> > { \
> > return store_pwm_reg(dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, \
> > show_regs_pwm_##offset, store_regs_pwm_##offset);
> >
> > #define sysfs_pwmenable(offset) \
> > static ssize_t show_regs_pwmenable_##offset (struct device *dev, char *buf)
> \
> > { \
> > return show_pwmenable_reg(dev, buf, offset); \
> > } \
> > static ssize_t store_regs_pwmenable_##offset (struct device *dev, \
> > const char *buf, size_t count) \
> > { \
> > return store_pwmenable_reg(dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(pwm##offset##_enable, S_IRUGO | S_IWUSR, \
> > show_regs_pwmenable_##offset, store_regs_pwmenable_##offset);
> >
> > sysfs_pwm(1);
> > sysfs_pwm(2);
> > sysfs_pwm(3);
> > sysfs_pwmenable(1);
> > sysfs_pwmenable(2);
> > sysfs_pwmenable(3);
> >
> > #define device_create_file_pwm(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_pwm##offset); \
> > } while (0)
> >
> > #define device_create_file_pwmenable(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_pwm##offset##_enable); \
> > } while (0)
> >
> >
> > static ssize_t
> > show_pwm_mode_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n", (long)data->pwm_mode[nr-1]);
> > }
> >
> > static ssize_t
> > store_pwm_mode_reg(struct device *dev, const char *buf, size_t count, int
> nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 pwm_mode_mask = 0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> > data->pwm_mode[nr-1] = SENSORS_LIMIT(val, 0, 1);
> > pwm_mode_mask = w83792d_read_value(client,
> > W83792D_REG_PWM[nr-1]) & 0x7f;
> > w83792d_write_value(client, W83792D_REG_PWM[nr-1],
> > ((data->pwm_mode[nr-1])<<7)|pwm_mode_mask);
> >
> > return count;
> > }
> >
> > #define sysfs_pwm_mode(offset) \
> > static ssize_t show_regs_pwm_mode_##offset (struct device *dev, char *buf)
> \
> > { \
> > return show_pwm_mode_reg(dev, buf, offset); \
> > } \
> > static ssize_t store_regs_pwm_mode_##offset (struct device *dev, \
> > const char *buf, size_t count) \
> > { \
> > return store_pwm_mode_reg(dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(pwm##offset##_mode, S_IRUGO | S_IWUSR, \
> > show_regs_pwm_mode_##offset, store_regs_pwm_mode_##offset);
> >
> > sysfs_pwm_mode(1);
> > sysfs_pwm_mode(2);
> > sysfs_pwm_mode(3);
> >
> > #define device_create_file_pwm_mode(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_pwm##offset##_mode); \
> > } while (0)
> >
> >
> > static ssize_t
> > show_chassis_reg(struct device *dev, char *buf)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n", (long)data->chassis);
> > }
> >
> > static ssize_t show_regs_chassis (struct device *dev, char *buf)
> > {
> > return show_chassis_reg(dev, buf);
> > }
> >
> > static DEVICE_ATTR(chassis, S_IRUGO, show_regs_chassis, NULL);
> >
> > #define device_create_file_chassis(client) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_chassis); \
> > } while (0)
> >
> >
> > static ssize_t
> > show_chassis_clear_reg(struct device *dev, char *buf)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n", (long)data->chassis_clear);
> > }
> >
> > static ssize_t
> > store_chassis_clear_reg(struct device *dev, const char *buf, size_t count)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 temp1 = 0, temp2 = 0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->chassis_clear = SENSORS_LIMIT(val, 0 ,1);
> > temp1 = ((data->chassis_clear) << 7) & 0x80;
> > temp2 = w83792d_read_value(client,
> > W83792D_REG_CHASSIS_CLR) & 0x7f;
> > w83792d_write_value(client, W83792D_REG_CHASSIS_CLR, temp1|temp2);
>
> Missing spaces between |
>
> >
> > return count;
> > }
> >
> > static ssize_t show_regs_chassis_clear (struct device *dev, char *buf)
> > {
> > return show_chassis_clear_reg(dev, buf);
> > }
> >
> > static ssize_t store_regs_chassis_clear (struct device *dev,
> > const char *buf, size_t count)
> > {
> > return store_chassis_clear_reg(dev, buf, count);
> > }
> >
> > static DEVICE_ATTR(chassis_clear, S_IRUGO | S_IWUSR,
> > show_regs_chassis_clear, store_regs_chassis_clear);
> >
> > #define device_create_file_chassis_clear(client) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_chassis_clear); \
> > } while (0)
> >
> >
> >
> > /* For Smart Fan I / Thermal Cruise */
> > static ssize_t
> > show_thermal_cruise_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n", (long)data->thermal_cruise[nr-1]);
> > }
> >
> > static ssize_t
> > store_thermal_cruise_reg(struct device *dev, const char *buf, size_t count,
> int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 target_tmp=0, target_mask=0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > target_tmp = val;
> > target_tmp = target_tmp & 0x7f;
> > target_mask = w83792d_read_value(client, W83792D_REG_THERMAL[nr-1]) &
> 0x80;
> > data->thermal_cruise[nr-1] = SENSORS_LIMIT(target_tmp, 0, 255);
> > w83792d_write_value(client, W83792D_REG_THERMAL[nr-1],
> > (data->thermal_cruise[nr-1])|target_mask);
>
> Missing spaces between |
>
> >
> > return count;
> > }
> >
> > #define sysfs_thermal_cruise(offset) \
> > static ssize_t show_regs_thermal_cruise##offset (struct device *dev, char
> *buf) \
> > { \
> > return show_thermal_cruise_reg(dev, buf, offset); \
> > } \
> > static ssize_t store_regs_thermal_cruise##offset (struct device *dev, \
> > const char *buf, size_t count) \
> > { \
> > return store_thermal_cruise_reg(dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(thermal_cruise##offset, S_IRUGO | S_IWUSR, \
> > show_regs_thermal_cruise##offset,
> store_regs_thermal_cruise##offset);
> >
> > sysfs_thermal_cruise(1);
> > sysfs_thermal_cruise(2);
> > sysfs_thermal_cruise(3);
> >
> > #define device_create_file_thermal_cruise(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_thermal_cruise##offset); \
> > } while (0)
> >
> >
> > /* For Smart Fan I/Thermal Cruise and Smart Fan II */
> > static ssize_t
> > show_tolerance_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n", (long)data->tolerance[nr-1]);
> > }
> >
> > static ssize_t
> > store_tolerance_reg(struct device *dev, const char *buf, size_t count, int
> nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 tol_tmp, tol_mask;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > tol_mask = w83792d_read_value(client,
> > W83792D_REG_TOLERANCE[nr-1]) & ((nr==2)?0x0f:0xf0);
>
> Missing spaces: ((nr == 2) ? 0x0f : 0xf0);
>
> > tol_tmp = SENSORS_LIMIT(val, 0, 15);
> > tol_tmp &= 0x0f;
> > data->tolerance[nr-1] = tol_tmp;
> > if (nr==2) {
> > tol_tmp <<= 4;
> > }
> > w83792d_write_value(client, W83792D_REG_TOLERANCE[nr-1],
> > tol_mask|tol_tmp);
>
> Missing spaces between |
>
> >
> > return count;
> > }
> >
> > #define sysfs_tolerance(offset) \
> > static ssize_t show_regs_tolerance##offset (struct device *dev, char *buf)
> \
> > { \
> > return show_tolerance_reg(dev, buf, offset); \
> > } \
> > static ssize_t store_regs_tolerance##offset (struct device *dev, \
> > const char *buf, size_t count) \
> > { \
> > return store_tolerance_reg(dev, buf, count, offset); \
> > } \
> > static DEVICE_ATTR(tolerance##offset, S_IRUGO | S_IWUSR, \
> > show_regs_tolerance##offset, store_regs_tolerance##offset);
> >
> > sysfs_tolerance(1);
> > sysfs_tolerance(2);
> > sysfs_tolerance(3);
> >
> > #define device_create_file_tolerance(client, offset) \
> > do { \
> > device_create_file(&client->dev, &dev_attr_tolerance##offset); \
> > } while (0)
> >
> >
> > /* For Smart Fan II */
> > static ssize_t
> > show_sf2_point_reg(struct device *dev, char *buf, int nr, int index)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n", (long)data->sf2_points[index-1][nr-1]);
> > }
> >
> > static ssize_t
> > store_sf2_point_reg(struct device *dev, const char *buf, size_t count, int
> nr, int index)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 mask_tmp = 0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->sf2_points[index-1][nr-1] = SENSORS_LIMIT(val, 0, 127);
> > mask_tmp = w83792d_read_value(client,
> > W83792D_REG_POINTS[index-1][nr-1]) & 0x80;
> > w83792d_write_value(client, W83792D_REG_POINTS[index-1][nr-1],
> > mask_tmp|data->sf2_points[index-1][nr-1]);
> >
> > return count;
> > }
> >
> > #define sysfs_sf2_point(offset, index) \
> > static ssize_t show_regs_sf2_point##offset##_fan##index (struct device *dev,
> char *buf) \
> > { \
> > return show_sf2_point_reg(dev, buf, offset, index); \
> > } \
> > static ssize_t store_regs_sf2_point##offset##_fan##index (struct device
> *dev, \
> > const char *buf, size_t count) \
> > { \
> > return store_sf2_point_reg(dev, buf, count, offset, index); \
> > } \
> > static DEVICE_ATTR(sf2_point##offset##_fan##index, S_IRUGO | S_IWUSR, \
> > show_regs_sf2_point##offset##_fan##index, \
> > store_regs_sf2_point##offset##_fan##index);
> >
> > sysfs_sf2_point(1, 1); /* Fan1 */
> > sysfs_sf2_point(2, 1); /* Fan1 */
> > sysfs_sf2_point(3, 1); /* Fan1 */
> > sysfs_sf2_point(4, 1); /* Fan1 */
> > sysfs_sf2_point(1, 2); /* Fan2 */
> > sysfs_sf2_point(2, 2); /* Fan2 */
> > sysfs_sf2_point(3, 2); /* Fan2 */
> > sysfs_sf2_point(4, 2); /* Fan2 */
> > sysfs_sf2_point(1, 3); /* Fan3 */
> > sysfs_sf2_point(2, 3); /* Fan3 */
> > sysfs_sf2_point(3, 3); /* Fan3 */
> > sysfs_sf2_point(4, 3); /* Fan3 */
> >
> > #define device_create_file_sf2_point(client, offset, index) \
> > do { \
> > device_create_file(&client->dev,
> &dev_attr_sf2_point##offset##_fan##index); \
> > } while (0)
> >
> >
> > static ssize_t
> > show_sf2_level_reg(struct device *dev, char *buf, int nr, int index)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n",
> (long)(((data->sf2_levels[index-1][nr])*100)/15));
>
> Missing spaces
>
> > }
> >
> > static ssize_t
> > store_sf2_level_reg(struct device *dev, const char *buf, size_t count, int
> nr, int index)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 mask_tmp=0, level_tmp=0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->sf2_levels[index-1][nr] =
> > SENSORS_LIMIT((val*15)/100, 0, 15);
>
> And here too it seems
>
> > mask_tmp = w83792d_read_value(client, W83792D_REG_LEVELS[index-1][nr])
> > & ((nr==3) ? 0xf0 : 0x0f);
> > if (nr==3) {
> > level_tmp = data->sf2_levels[index-1][nr];
> > } else {
> > level_tmp = data->sf2_levels[index-1][nr] << 4;
> > }
> > w83792d_write_value(client, W83792D_REG_LEVELS[index-1][nr], level_tmp
> | mask_tmp);
> >
> > return count;
> > }
> >
> > #define sysfs_sf2_level(offset, index) \
> > static ssize_t show_regs_sf2_level##offset##_fan##index (struct device *dev,
> char *buf) \
> > { \
> > return show_sf2_level_reg(dev, buf, offset, index); \
> > } \
> > static ssize_t store_regs_sf2_level##offset##_fan##index (struct device
> *dev, \
> > const char *buf, size_t count) \
> > { \
> > return store_sf2_level_reg(dev, buf, count, offset, index); \
> > } \
> > static DEVICE_ATTR(sf2_level##offset##_fan##index, S_IRUGO | S_IWUSR, \
> > show_regs_sf2_level##offset##_fan##index, \
> > store_regs_sf2_level##offset##_fan##index);
> >
> >
> > sysfs_sf2_level(1, 1); /* Fan1 */
> > sysfs_sf2_level(2, 1); /* Fan1 */
> > sysfs_sf2_level(3, 1); /* Fan1 */
> > sysfs_sf2_level(1, 2); /* Fan2 */
> > sysfs_sf2_level(2, 2); /* Fan2 */
> > sysfs_sf2_level(3, 2); /* Fan2 */
> > sysfs_sf2_level(1, 3); /* Fan3 */
> > sysfs_sf2_level(2, 3); /* Fan3 */
> > sysfs_sf2_level(3, 3); /* Fan3 */
> >
> > #define device_create_file_sf2_level(client, offset, index) \
> > do { \
> > device_create_file(&client->dev,
> &dev_attr_sf2_level##offset##_fan##index); \
> > } while (0)
> >
> >
> > /* This function is called when:
> > * w83792d_driver is inserted (when this module is loaded), for each
> > available adapter
> > * when a new adapter is inserted (and w83792d_driver is still present)
> */
> > static int
> > w83792d_attach_adapter(struct i2c_adapter *adapter)
>
> I think static int and rest belong to one line
>
> > {
> > if (!(adapter->class & I2C_CLASS_HWMON))
> > return 0;
> > return i2c_detect(adapter, &addr_data, w83792d_detect);
> > }
> >
> >
> > static int
> > w83792d_detect_subclients(struct i2c_adapter *adapter, int address, int
> kind,
> > struct i2c_client *new_client)
>
> and here too
>
> > {
> > int i, id, err;
> > struct w83792d_data *data = i2c_get_clientdata(new_client);
> >
> > data->lm75[0] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
> > if (!(data->lm75[0])) {
> > err = -ENOMEM;
> > goto ERROR_SC_0;
> > }
> > memset(data->lm75[0], 0x00, sizeof(struct i2c_client));
> >
> > data->lm75[1] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
> > if (!(data->lm75[1])) {
> > err = -ENOMEM;
> > goto ERROR_SC_1;
> > }
> > memset(data->lm75[1], 0x00, sizeof(struct i2c_client));
> >
> > id = i2c_adapter_id(adapter);
> >
> > if (force_subclients[0] == id && force_subclients[1] == address) {
> > for (i = 2; i <= 3; i++) {
> > if (force_subclients[i] < 0x48 ||
> > force_subclients[i] > 0x4f) {
> > dev_err(&new_client->dev, "invalid subclient "
> > "address %d; must be 0x48-0x4f\n",
> > force_subclients[i]);
> > err = -ENODEV;
> > goto ERROR_SC_2;
> > }
> > }
> > w83792d_write_value(new_client, W83792D_REG_I2C_SUBADDR,
> > (force_subclients[2] & 0x03) |
> > ((force_subclients[3] & 0x03) <<4));
> > data->lm75[0]->addr = force_subclients[2];
> > data->lm75[1]->addr = force_subclients[3];
> > } else {
> > int val = w83792d_read_value(new_client, W83792D_REG_I2C_SUBADDR);
>
> This should be u8 val =
>
> > data->lm75[0]->addr = 0x48 + (val & 0x07);
> > data->lm75[1]->addr = 0x48 + ((val >> 4) & 0x07);
> > }
> >
> > if (data->lm75[0]->addr == data->lm75[1]->addr) {
> > dev_err(&new_client->dev, "duplicate addresses 0x%x "
> > "for subclients\n", data->lm75[0]->addr);
> > err = -ENODEV;
> > goto ERROR_SC_2;
> > }
> >
> > for (i = 0; i <= 1; i++) {
> > i2c_set_clientdata(data->lm75[i], NULL);
> > data->lm75[i]->adapter = adapter;
> > data->lm75[i]->driver = &w83792d_driver;
> > data->lm75[i]->flags = 0;
> > strlcpy(data->lm75[i]->name, "w83792d subclient", I2C_NAME_SIZE);
> > }
> >
> > if ((err = i2c_attach_client(data->lm75[0]))) {
> > dev_err(&new_client->dev, "subclient %d registration "
> > "at address 0x%x failed.\n", i, data->lm75[0]->addr);
> > goto ERROR_SC_2;
> > }
> >
> > if ((err = i2c_attach_client(data->lm75[1]))) {
> > dev_err(&new_client->dev, "subclient %d registration "
> > "at address 0x%x failed.\n", i, data->lm75[1]->addr);
> > goto ERROR_SC_3;
> > }
> >
> > return 0;
> >
> > /* Undo inits in case of errors */
> > ERROR_SC_3:
> > i2c_detach_client(data->lm75[0]);
> > ERROR_SC_2:
> > kfree(data->lm75[1]);
> > ERROR_SC_1:
> > kfree(data->lm75[0]);
> > ERROR_SC_0:
> > return err;
> > }
> >
> >
> > static int
> > w83792d_detect(struct i2c_adapter *adapter, int address, int kind)
> > {
>
> Same line?
>
> > int i = 0, val1 = 0, val2;
> > struct i2c_client *new_client;
> > struct w83792d_data *data;
> > int err = 0;
> > const char *client_name = "";
> >
> > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > goto ERROR0;
> > }
> >
> > /* OK. For now, we presume we have a valid client. We now create the
> > client structure, even though we cannot fill it completely yet.
> > But it allows us to access w83792d_{read,write}_value. */
> >
> > if (!(data = kmalloc(sizeof(struct w83792d_data), GFP_KERNEL))) {
> > err = -ENOMEM;
> > goto ERROR0;
> > }
> > memset(data, 0, sizeof(struct w83792d_data));
> >
> > new_client = &data->client;
> > i2c_set_clientdata(new_client, data);
> > new_client->addr = address;
> > init_MUTEX(&data->lock);
> > new_client->adapter = adapter;
> > new_client->driver = &w83792d_driver;
> > new_client->flags = 0;
> >
> > /* Now, we do the remaining detection. */
> >
> > /* The w8378?d may be stuck in some other bank than bank 0. This may
>
> Bad chip name in comment
>
> > make reading other information impossible. Specify a force=... or
> > force_*=... parameter, and the Winbond will be reset to the right
> > bank. */
> > if (kind < 0) {
> > if (w83792d_read_value(new_client, W83792D_REG_CONFIG) & 0x80) {
> > dev_warn(&new_client->dev, "Detection failed at step "
> > "3\n");
> > goto ERROR1;
> > }
> > val1 = w83792d_read_value(new_client, W83792D_REG_BANK);
> > val2 = w83792d_read_value(new_client, W83792D_REG_CHIPMAN);
> > /* Check for Winbond ID if in bank 0 */
> > if (!(val1 & 0x07)) { /* is Bank0 */
> > if (((!(val1 & 0x80)) && (val2 != 0xa3)) ||
> > ((val1 & 0x80) && (val2 != 0x5c))) {
> > goto ERROR1;
> > }
> > }
> > /* If Winbond SMBus, check address at 0x48 */
>
> better comment would be:
>
> If Winbond chip, address of chip and W83792D_REG_I2C_ADDR should match.
>
> > if (w83792d_read_value(new_client, W83792D_REG_I2C_ADDR) !=
> address) {
> > dev_warn(&new_client->dev, "Detection failed "
> > "at step 5\n");
> > goto ERROR1;
> > }
> > }
> >
> > /* We have either had a force parameter, or we have already detected the
> > Winbond. Put it now into bank 0 and Vendor ID High Byte */
>
> > w83792d_write_value(new_client, W83792D_REG_BANK,
> > (w83792d_read_value(new_client,
> > W83792D_REG_BANK) & 0x78) |
> > 0x80);
> >
>
> Maybe better indent?
>
>
> > /* Determine the chip type. */
> > if (kind <= 0) {
> > /* get vendor ID */
> > val2 = w83792d_read_value(new_client, W83792D_REG_CHIPMAN);
> > if (val2 != 0x5c) { /* the vendor is NOT Winbond */
> > goto ERROR1;
> > }
> > val1 = w83792d_read_value(new_client, W83792D_REG_WCHIPID);
> > if (val1 == 0x7a && address >= 0x2c) {
> > kind = w83792d;
> > } else {
> > if (kind == 0)
> > printk
> > (KERN_WARNING "w83792d: Ignoring 'force' parameter
> for unknown chip at"
> > "adapter %d, address 0x%02x\n",
> > i2c_adapter_id(adapter), address);
> > goto ERROR1;
> > }
> > }
> >
> > if (kind == w83792d) {
> > client_name = "w83792d";
> > } else {
> > dev_warn(&new_client->dev, "w83792d: Internal error: unknown kind
> (%d)?!?",
> > kind);
> > goto ERROR1;
> > }
> >
> > /* Fill in the remaining client fields and put into the global list */
> > strlcpy(new_client->name, client_name, I2C_NAME_SIZE);
> > data->type = kind;
> >
> > data->valid = 0;
> > init_MUTEX(&data->update_lock);
> >
> > /* Tell the I2C layer a new client has arrived */
> > if ((err = i2c_attach_client(new_client)))
> > goto ERROR1;
> >
> > if ((err = w83792d_detect_subclients(adapter, address,
> > kind, new_client)))
> > goto ERROR2;
> >
> > /* Initialize the chip */
> > w83792d_init_client(new_client);
> >
> > /* A few vars need to be filled upon startup */
> > for (i = 1; i <= 7; i++) {
> > data->fan_min[i - 1] = w83792d_read_value(new_client,
> > W83792D_REG_FAN_MIN[i]);
> > }
>
> hmm really? why not it init_client?
>
> >
> > /* Register sysfs hooks */
> > device_create_file_in(new_client, 0);
> > device_create_file_in(new_client, 1);
> > device_create_file_in(new_client, 2);
> > device_create_file_in(new_client, 3);
> > device_create_file_in(new_client, 4);
> > device_create_file_in(new_client, 5);
> > device_create_file_in(new_client, 6);
> > device_create_file_in(new_client, 7);
> > device_create_file_in(new_client, 8);
> >
> > device_create_file_fan(new_client, 1);
> > device_create_file_fan(new_client, 2);
> > device_create_file_fan(new_client, 3);
> > device_create_file_fan(new_client, 4);
> > device_create_file_fan(new_client, 5);
> > device_create_file_fan(new_client, 6);
> > device_create_file_fan(new_client, 7);
> >
> > device_create_file_temp(new_client, 1);
> > device_create_file_temp(new_client, 2);
> > device_create_file_temp(new_client, 3);
> >
> > device_create_file_fan_div(new_client, 1);
> > device_create_file_fan_div(new_client, 2);
> > device_create_file_fan_div(new_client, 3);
> > device_create_file_fan_div(new_client, 4);
> > device_create_file_fan_div(new_client, 5);
> > device_create_file_fan_div(new_client, 6);
> > device_create_file_fan_div(new_client, 7);
> >
> > device_create_file_pwm(new_client, 1);
> > device_create_file_pwm(new_client, 2);
> > device_create_file_pwm(new_client, 3);
> >
> > device_create_file_pwmenable(new_client, 1);
> > device_create_file_pwmenable(new_client, 2);
> > device_create_file_pwmenable(new_client, 3);
> >
> > device_create_file_pwm_mode(new_client, 1);
> > device_create_file_pwm_mode(new_client, 2);
> > device_create_file_pwm_mode(new_client, 3);
> >
> > device_create_file_chassis(new_client);
> > device_create_file_chassis_clear(new_client);
> >
> > device_create_file_thermal_cruise(new_client, 1);
> > device_create_file_thermal_cruise(new_client, 2);
> > device_create_file_thermal_cruise(new_client, 3);
> >
> > device_create_file_tolerance(new_client, 1);
> > device_create_file_tolerance(new_client, 2);
> > device_create_file_tolerance(new_client, 3);
> >
> > device_create_file_sf2_point(new_client, 1, 1); /* Fan1 */
> > device_create_file_sf2_point(new_client, 2, 1); /* Fan1 */
> > device_create_file_sf2_point(new_client, 3, 1); /* Fan1 */
> > device_create_file_sf2_point(new_client, 4, 1); /* Fan1 */
> > device_create_file_sf2_point(new_client, 1, 2); /* Fan2 */
> > device_create_file_sf2_point(new_client, 2, 2); /* Fan2 */
> > device_create_file_sf2_point(new_client, 3, 2); /* Fan2 */
> > device_create_file_sf2_point(new_client, 4, 2); /* Fan2 */
> > device_create_file_sf2_point(new_client, 1, 3); /* Fan3 */
> > device_create_file_sf2_point(new_client, 2, 3); /* Fan3 */
> > device_create_file_sf2_point(new_client, 3, 3); /* Fan3 */
> > device_create_file_sf2_point(new_client, 4, 3); /* Fan3 */
> >
> > device_create_file_sf2_level(new_client, 1, 1); /* Fan1 */
> > device_create_file_sf2_level(new_client, 2, 1); /* Fan1 */
> > device_create_file_sf2_level(new_client, 3, 1); /* Fan1 */
> > device_create_file_sf2_level(new_client, 1, 2); /* Fan2 */
> > device_create_file_sf2_level(new_client, 2, 2); /* Fan2 */
> > device_create_file_sf2_level(new_client, 3, 2); /* Fan2 */
> > device_create_file_sf2_level(new_client, 1, 3); /* Fan3 */
> > device_create_file_sf2_level(new_client, 2, 3); /* Fan3 */
> > device_create_file_sf2_level(new_client, 3, 3); /* Fan3 */
> >
> > return 0;
> >
> > ERROR2:
> > i2c_detach_client(new_client);
> > ERROR1:
> > kfree(data);
> > ERROR0:
> > return err;
> > }
> >
> > static int
> > w83792d_detach_client(struct i2c_client *client)
>
> I think one line too.
>
> > {
> > int err;
> >
> > if ((err = i2c_detach_client(client))) {
> > dev_err(&client->dev,
> > "Client deregistration failed, client not detached.\n");
> > return err;
> > }
> >
> > if (i2c_get_clientdata(client)==NULL) {
> > /* subclients */
> > kfree(client);
> > } else {
> > /* main client */
> > kfree(i2c_get_clientdata(client));
> > }
> >
> > return 0;
> > }
> >
> > /* The SMBus locks itself, usually, but nothing may access the Winbond between
> > bank switches. ISA access must always be locked explicitly!
> > We ignore the W83792D BUSY flag at this moment - it could lead to deadlocks,
> > would slow down the W83792D access and should not be necessary.
> > There are some ugly typecasts here, but the good news is - they should
> > nowhere else be necessary! */
> > static int
> > w83792d_read_value(struct i2c_client *client, u8 reg)
>
> I think one line too.
> > {
> > int res=0;
> > res = i2c_smbus_read_byte_data(client, reg);
> >
> > return res;
> > }
> >
> > static int
> > w83792d_write_value(struct i2c_client *client, u8 reg, u8 value)
>
> I think one line too.
>
> > {
> > i2c_smbus_write_byte_data(client, reg, value);
> > return 0;
> > }
> >
> > /* Called when we have found a new W83792D. It should set limits, etc. */
> > static void
> > w83792d_init_client(struct i2c_client *client)
> > {
> > int temp2_cfg, temp3_cfg, i=0;
>
> should not be u8 ? (except of i)
>
> > u8 vid_in_b;
> >
> > if (init) {
> > w83792d_write_value(client, W83792D_REG_CONFIG, 0x80);
> > }
> > /* Clear the bit6 of W83792D_REG_VID_IN_B(set it into 0):
> > W83792D_REG_VID_IN_B bit6 = 0: the high/low limit of
> > vin0/vin1 can be modified by user;
> > W83792D_REG_VID_IN_B bit6 = 1: the high/low limit of
> > vin0/vin1 auto-updated, can NOT be modified by user. */
> > vid_in_b = w83792d_read_value(client, W83792D_REG_VID_IN_B);
> > w83792d_write_value(client, W83792D_REG_VID_IN_B,
> > vid_in_b & 0xbf);
> >
> > temp2_cfg = w83792d_read_value(client, W83792D_REG_TEMP2_CONFIG);
> > temp3_cfg = w83792d_read_value(client, W83792D_REG_TEMP3_CONFIG);
> > w83792d_write_value(client, W83792D_REG_TEMP2_CONFIG,
> > temp2_cfg & 0xe6);
> > w83792d_write_value(client, W83792D_REG_TEMP3_CONFIG,
> > temp3_cfg & 0xe6);
> >
> > /* enable comparator mode for temp2 and temp3 so
> > alarm indication will work correctly */
> > i = w83792d_read_value(client, W83792D_REG_IRQ);
> > if (!(i & 0x40)) {
> > w83792d_write_value(client, W83792D_REG_IRQ,
> > i | 0x40);
> > }
> >
> > /* Start monitoring */
> > w83792d_write_value(client, W83792D_REG_CONFIG,
> > (w83792d_read_value(client,
> > W83792D_REG_CONFIG) & 0xf7)
> > | 0x01);
> > }
> >
> > static struct w83792d_data *w83792d_update_device(struct device *dev)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > int i, j;
> > u8 reg_array_tmp[4], pwm_array_tmp[7], reg_tmp;
> >
> > down(&data->update_lock);
> >
> > if (time_after
> > (jiffies - data->last_updated, (unsigned long) (HZ * 3))
> > || time_before(jiffies, data->last_updated) || !data->valid) {
> > dev_warn(dev, "Starting device update\n");
> >
> > /* Update the voltages measured value and limits */
> > for (i = 0; i < 9; i++) {
> > data->in[i] = w83792d_read_value(client,
> > W83792D_REG_IN[i]);
> > data->in_max[i] = w83792d_read_value(client,
> > W83792D_REG_IN_MAX[i]);
> > data->in_min[i] = w83792d_read_value(client,
> > W83792D_REG_IN_MIN[i]);
> > }
> > data->low_bits[0] = w83792d_read_value(client,
> > W83792D_REG_LOW_BITS1);
> > data->low_bits[1] = w83792d_read_value(client,
> > W83792D_REG_LOW_BITS2);
> > for (i = 0; i < 7; i++) {
> > /* Update the Fan measured value and limits */
> > data->fan[i] = w83792d_read_value(client,
> > W83792D_REG_FAN[i]);
> > data->fan_min[i] = w83792d_read_value(client,
> > W83792D_REG_FAN_MIN[i]);
> > /* Update the PWM/DC Value and PWM/DC flag */
> > pwm_array_tmp[i] = w83792d_read_value(client,
> > W83792D_REG_PWM[i]);
> > data->pwm[i] = pwm_array_tmp[i] & 0x0f;
> > data->pwm_mode[i] = (pwm_array_tmp[i] >> 7) & 0x01;
> > }
> >
> > reg_tmp = w83792d_read_value(client, W83792D_REG_FAN_CFG);
> > data->pwmenable[0] = reg_tmp & 0x03;
> > data->pwmenable[1] = (reg_tmp>>2) & 0x03;
> > data->pwmenable[2] = (reg_tmp>>4) & 0x03;
> >
> > data->temp = w83792d_read_value(client, W83792D_REG_TEMP[0]);
> > data->temp_max =
> > w83792d_read_value(client, W83792D_REG_TEMP_OVER[0]);
> > data->temp_max_hyst =
> > w83792d_read_value(client, W83792D_REG_TEMP_HYST[0]);
> > data->temp_add[0] =
> > w83792d_read_value(client, W83792D_REG_TEMP[1]);
> > data->temp_max_add[0] =
> > w83792d_read_value(client, W83792D_REG_TEMP_OVER[1]);
> > data->temp_max_hyst_add[0] =
> > w83792d_read_value(client, W83792D_REG_TEMP_HYST[1]);
> > data->temp_add[1] =
> > w83792d_read_value(client, W83792D_REG_TEMP[2]);
> > data->temp_max_add[1] =
> > w83792d_read_value(client,
> > W83792D_REG_TEMP_OVER[2]);
> > data->temp_max_hyst_add[1] =
> > w83792d_read_value(client,
> > W83792D_REG_TEMP_HYST[2]);
> >
> > /* Update the Fan Divisor */
> > for (i = 0; i < 4; i++) {
> > reg_array_tmp[i] = w83792d_read_value(client,
> W83792D_REG_FAN_DIV[i]);
> > }
> > data->fan_div[0] = reg_array_tmp[0] & 0x07;
> > data->fan_div[1] = (reg_array_tmp[0] >> 4) & 0x07;
> > data->fan_div[2] = reg_array_tmp[1] & 0x07;
> > data->fan_div[3] = (reg_array_tmp[1] >> 4) & 0x07;
> > data->fan_div[4] = reg_array_tmp[2] & 0x07;
> > data->fan_div[5] = (reg_array_tmp[2] >> 4) & 0x07;
> > data->fan_div[6] = reg_array_tmp[3] & 0x07;
> >
> > /* Update CaseOpen status and it's CLR_CHS. */
> > data->chassis = (w83792d_read_value(client,
> > W83792D_REG_CHASSIS) >> 5) & 0x01;
> > data->chassis_clear = (w83792d_read_value(client,
> > W83792D_REG_CHASSIS_CLR) >> 7) & 0x01;
> >
> > /* Update Thermal Cruise/Smart Fan I target value */
> > for (i = 0; i < 3; i++) {
> > data->thermal_cruise[i] =
> > w83792d_read_value(client,
> > W83792D_REG_THERMAL[i]) & 0x7f;
> > }
> >
> > /* Update Smart Fan I/II tolerance */
> > reg_tmp = w83792d_read_value(client, W83792D_REG_TOLERANCE[0]);
> > data->tolerance[0] = reg_tmp & 0x0f;
> > data->tolerance[1] = (reg_tmp >> 4) & 0x0f;
> > data->tolerance[2] =
> > w83792d_read_value(client, W83792D_REG_TOLERANCE[2]) & 0x0f;
> >
> > /* Update Smart Fan II temperature points */
> > for (i = 0; i < 3; i++) {
> > for (j = 0; j < 4; j++) {
> > data->sf2_points[i][j] = w83792d_read_value(
> > client,W83792D_REG_POINTS[i][j]) & 0x7f;
> > }
> > }
> >
> > /* Update Smart Fan II duty cycle levels */
> > for (i = 0; i < 3; i++) {
> > reg_tmp = w83792d_read_value(client,
> > W83792D_REG_LEVELS[i][0]);
> > data->sf2_levels[i][0] = reg_tmp & 0x0f;
> > data->sf2_levels[i][1] = (reg_tmp >> 4) & 0x0f;
> > reg_tmp = w83792d_read_value(client,
> > W83792D_REG_LEVELS[i][2]);
> > data->sf2_levels[i][2] = (reg_tmp >> 4) & 0x0f;
> > data->sf2_levels[i][3] = reg_tmp & 0x0f;
> > }
> >
> > data->last_updated = jiffies;
> > data->valid = 1;
> > }
> >
> > up(&data->update_lock);
> >
> > #ifdef W83792D_DEBUG
> > w83792d_print_debug(data);
> > #endif
>
> Do not forget to change this too.
>
> >
> > return data;
> > }
> >
> > #ifdef W83792D_DEBUG
>
> and here :)
>
> > static void w83792d_print_debug(struct w83792d_data *data)
> > {
> > int i=0;
> > printk(KERN_DEBUG "==========The following is the debug
> message...========\n");
> > printk(KERN_DEBUG "9 set of Voltages: =====>\n");
> > for (i=0; i<=8; i++) {
> > printk(KERN_DEBUG "vin[%d] is: 0x%x\n", i, data->in[i]);
> > printk(KERN_DEBUG "vin[%d] max is: 0x%x\n", i, data->in_max[i]);
> > printk(KERN_DEBUG "vin[%d] min is: 0x%x\n", i, data->in_min[i]);
> > }
> > printk(KERN_DEBUG "Low Bit1 is: 0x%x\n", data->low_bits[0]);
> > printk(KERN_DEBUG "Low Bit2 is: 0x%x\n", data->low_bits[1]);
> > printk(KERN_DEBUG "7 set of Fan Counts and Duty Cycles: =====>\n");
> > /* printk(KERN_DEBUG "fan_cfg is: 0x%x\n", data->fan_cfg); */
> > for (i=0; i<=6; i++) {
> > printk(KERN_DEBUG "fan[%d] is: 0x%x\n", i, data->fan[i]);
> > printk(KERN_DEBUG "fan[%d] min is: 0x%x\n", i, data->fan_min[i]);
> > printk(KERN_DEBUG "pwm[%d] is: 0x%x\n", i, data->pwm[i]);
> > printk(KERN_DEBUG "pwm_mode[%d] is: 0x%x\n", i, data->pwm_mode[i]);
> > }
> > printk(KERN_DEBUG "3 set of Temperatures: =====>\n");
> > printk(KERN_DEBUG "temp1 is: 0x%x\n", data->temp);
> > printk(KERN_DEBUG "temp1 high limit is: 0x%x\n", data->temp_max);
> > printk(KERN_DEBUG "temp1 low limit is: 0x%x\n", data->temp_max_hyst);
> >
> > printk(KERN_DEBUG "temp2 is: 0x%x\n", data->temp_add[0]);
> > printk(KERN_DEBUG "temp2 high limit is: 0x%x\n", data->temp_max_add[0]);
> > printk(KERN_DEBUG "temp2 low limit is: 0x%x\n",
> data->temp_max_hyst_add[0]);
> >
> > printk(KERN_DEBUG "temp3 is: 0x%x\n", data->temp_add[1]);
> > printk(KERN_DEBUG "temp3 high limit is: 0x%x\n", data->temp_max_add[1]);
> > printk(KERN_DEBUG "temp3 low limit is: 0x%x\n",
> data->temp_max_hyst_add[1]);
> >
> > for (i=0; i<=6; i++) {
> > printk(KERN_DEBUG "fan_div[%d] is: 0x%x\n", i, data->fan_div[i]);
> > }
> > printk(KERN_DEBUG "==========End of the debug
> message...==================\n");
> > printk(KERN_DEBUG "\n");
> > }
> > #endif
> >
> > static int __init
> > sensors_w83792d_init(void)
> > {
> > return i2c_add_driver(&w83792d_driver);
> > }
> >
> > static void __exit
> > sensors_w83792d_exit(void)
> > {
> > i2c_del_driver(&w83792d_driver);
> > }
> >
> > MODULE_AUTHOR("Chunhao Huang @ Winbond");
>
> No email?
>
> > MODULE_DESCRIPTION("W83792AD/D driver for linux-2.6");
> > MODULE_LICENSE("GPL");
> >
> > module_init(sensors_w83792d_init);
> > module_exit(sensors_w83792d_exit);
>
>
===========================================================================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.
===========================================================================================If your computer is unable to decode Chinese font, please ignore the following message.It essentially repeats the statement in English given above.本信件內所含華邦電子的財產性機密性資訊, 僅授權原發信人指定之收信人取閱\\\之用. 假使您並非被指定之收信人或因任何原因在未經授權的情形之下收到本信件, 請您告知原發信人並立即將信件從電腦與網路伺服器中予以消除. 對於您的合作, 我們先此致謝. 特此提醒, 任何未經授權擅自使用華邦電子的機密資訊的行為是被嚴格禁止的. 信件與華邦電子營業無關之內容,不得視為華邦電子之立場或意見.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lm-sensors] W83792 review
2005-05-26 19:25 [lm-sensors] W83792 review Rudolf Marek
2005-05-27 3:33 ` [lm-sensors] " Huang0
@ 2005-05-28 15:31 ` Jean Delvare
2005-05-28 15:55 ` [lm-sensors] " Jean Delvare
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2005-05-28 15:31 UTC (permalink / raw)
To: lm-sensors
Hi Rudolf,
> I'm inviting other developers to check my objections I need to develop
> "review skill". - Thanks
OK, let's go. Everything I don't comment on, I agree with.
> > (2) I have not implemented the 0.5 degree to temperature2 and
> > temperature3, while the driver for linux-2.4 implemented this.
>
> That is the reason temp1 has separate handling and variable? If so,
> please add the code. or better would be to fix existing code so it
> does not look so strange.
No, better would be to actually add the handling of the additional
resolution bit.
> > static inline u8
> > FAN_TO_REG(long rpm, int div)
> > {
>
> I'm not sure, but I think it should be like
>
> static inline u8 FAN_TO_REG(long rpm, int div)
> {
Both styles exist and seem to be accepted in the kernel tree.
CodingStyle doesn't mention that point, so I'd say it's fine and there
is no need for Chunhao to to change his code.
> > /* array of 2 pointers to subclients */
> > struct i2c_client *lm75[2];
>
> The subclients are lm75 compatible? (I think so), but I'm not sure
> about the name...
Same we did in all other Winbond drivers. That's alright.
> > u8 temp_max_add[2]; /* Register value */
> > u8 temp_max_hyst_add[2]; /* Register value */
>
> Big I and align too please
"Big I" ? :)
> > static ssize_t
> > show_pwmenable_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > long pwm_enable_tmp = 1;
>
> Should be u32 instead?
>
> > if (data->pwmenable[nr-1] = 0) {
> > pwm_enable_tmp = 1; /* manual mode */
> > } else if (data->pwmenable[nr-1] = 1) {
> > pwm_enable_tmp = 3; /* thermal cruise/Smart Fan I */
> > } else if (data->pwmenable[nr-1] = 2) {
> > pwm_enable_tmp = 2; /* Smart Fan II */
> > }
> > return sprintf(buf, "%ld\n", pwm_enable_tmp);
> > }
u32 or long doesn't make much difference when you only need to store
values from 0 to 2. Even an u8 would do.
Note that this function could be made more readable/compact/efficient by
using either a switch/case construct, or a PWM_ENABLE_FROM_REG macro or
inline function, or even a lookup table.
> > 0xc0; fan_cfg_tmp = ((cfg4_tmp|cfg3_tmp)|cfg2_tmp)|cfg1_tmp;
>
> Missing spaces
> fan_cfg_tmp = ((cfg4_tmp | cfg3_tmp) | cfg2_tmp) | cfg1_tmp;
And useless parentheses ;)
> > static ssize_t
> > show_sf2_level_reg(struct device *dev, char *buf, int nr, int index)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n",
> > (long)(((data->sf2_levels[index-1][nr])*100)/15));
>
> Missing spaces
Also note that the (long) cast could (and should) be avoided.
> > /* A few vars need to be filled upon startup */
> > for (i = 1; i <= 7; i++) {
> > data->fan_min[i - 1] = w83792d_read_value(new_client,
> > W83792D_REG_FAN_MIN[i]);
> > }
>
> hmm really? why not it init_client?
The purpose of init_client is to initialize the chip. These lines
initalize the cache on driver side, so it's different. I think all
drivers currently do exactly what Chunhao does and it looks OK to me.
Good work Rudolf, most of your points were valid. Good review :)
--
Jean Delvare
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lm-sensors] RE: W83792 review
2005-05-26 19:25 [lm-sensors] W83792 review Rudolf Marek
2005-05-27 3:33 ` [lm-sensors] " Huang0
2005-05-28 15:31 ` [lm-sensors] " Jean Delvare
@ 2005-05-28 15:55 ` Jean Delvare
2005-05-29 5:22 ` Huang0
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2005-05-28 15:55 UTC (permalink / raw)
To: lm-sensors
Hi Chunhao,
> I wish some other guys can give me more suggestion to my codes, as
> well as to your comments. Where is Jean? It seems that he is busy
> these days? He gave me many good suggestions to the 792 driver for
> linux-2.4
I'm actually busy, working on various other stuff, plus day job and real
life. I'm doing as much as I can. Anyway, it is great that others start
reviewing the patches that are submitted to the sensors list. I have
been doing the job almost all by myself for too long. Having different
points of view is always better.
I did review Rudolf's review, hopefully this will be of some help to
both of you.
BTW: In the interest of all the readers of this list, I would appreciate
if everyone could avoid including all of the post they are replying to.
Please only include the relevant part. For example, your answer here was
66.7K in size, while the useful part of it was really only a dozen
lines.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lm-sensors] RE: W83792 review
2005-05-26 19:25 [lm-sensors] W83792 review Rudolf Marek
` (2 preceding siblings ...)
2005-05-28 15:55 ` [lm-sensors] " Jean Delvare
@ 2005-05-29 5:22 ` Huang0
2005-05-30 11:02 ` [lm-sensors] " Huang0
2005-05-31 13:40 ` Huang0
5 siblings, 0 replies; 7+ messages in thread
From: Huang0 @ 2005-05-29 5:22 UTC (permalink / raw)
To: lm-sensors
Hi Jean, Rudolf
> I'm actually busy, working on various other stuff, plus day job and
real
> life. I'm doing as much as I can. Anyway, it is great that others
start
> reviewing the patches that are submitted to the sensors list. I have
> been doing the job almost all by myself for too long. Having different
> points of view is always better.
>
> I did review Rudolf's review, hopefully this will be of some help to
> both of you.
Yes, I see. I did get your comments and Rudolf's, I will start the
modification from now on, I will check your comment one by one carefully
after a while, If I have some questions or disagreement, I will send
you.
> BTW: In the interest of all the readers of this list, I would
appreciate
> if everyone could avoid including all of the post they are replying
to.
> Please only include the relevant part. For example, your answer here
was
> 66.7K in size, while the useful part of it was really only a dozen
> lines.
Ok, I see, sorry for that, I will delete the useless lines next time.
Thanks
Best Regards
Chunhao
==============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.
==============================================If your computer is unable to decode Chinese font, please ignore the following message.It essentially repeats the statement in English given above.本信件內所含華邦電子的財產性機密性資訊, 僅授權原發信人指定之收信人取閱\\\之用. 假使您並非被指定之收信人或因任何原因在未經授權的情形之下收到本信件, 請您告知原發信人並立即將信件從電腦與網路伺服器中予以消除. 對於您的合作, 我們先此致謝. 特此提醒, 任何未經授權擅自使用華邦電子的機密資訊的行為是被嚴格禁止的. 信件與華邦電子營業無關之內容,不得視為華邦電子之立場或意見.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lm-sensors] W83792 review
2005-05-26 19:25 [lm-sensors] W83792 review Rudolf Marek
` (3 preceding siblings ...)
2005-05-29 5:22 ` Huang0
@ 2005-05-30 11:02 ` Huang0
2005-05-31 13:40 ` Huang0
5 siblings, 0 replies; 7+ messages in thread
From: Huang0 @ 2005-05-30 11:02 UTC (permalink / raw)
To: lm-sensors
Hi Jean, Rudolf, MDS
Quoting MDS
> I reviewed the datasheet about comparator/interrupt modes.
> Unfortunately, only the temps support comparator mode; voltages and
fans
> support only interrupt mode (datasheet sections 7.7.5-7.7.7)
Quoting Rudolf
> >(1) We misunderstand the meaning of temperature low limit, this bug
also
> >exists in the 792 driver for linux-2.4 and our 792 Windows driver.
>
> Please can you fix this ASAP so we can send it to kernel folks?
I'm fixing this bug in lm_sensors-2.9.1, by using the "comparator mode"
to
temp1-3 instead of the original method, I find this method works well in
792 driver for linux-2.4.
Next step I will modify the 792 driver for linux-2.6.
My question here is:
When I send the patch to 792 driver for linux-2.6, which lm_sensors
version
Should I use to generate the patch? lm_sensors-2.9.1 or
lm_sensors-daily?
Thanks
Best Regards
Chunhao
==============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.
==============================================If your computer is unable to decode Chinese font, please ignore the following message.It essentially repeats the statement in English given above.本信件內所含華邦電子的財產性機密性資訊, 僅授權原發信人指定之收信人取閱\\\之用. 假使您並非被指定之收信人或因任何原因在未經授權的情形之下收到本信件, 請您告知原發信人並立即將信件從電腦與網路伺服器中予以消除. 對於您的合作, 我們先此致謝. 特此提醒, 任何未經授權擅自使用華邦電子的機密資訊的行為是被嚴格禁止的. 信件與華邦電子營業無關之內容,不得視為華邦電子之立場或意見.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lm-sensors] W83792 review
2005-05-26 19:25 [lm-sensors] W83792 review Rudolf Marek
` (4 preceding siblings ...)
2005-05-30 11:02 ` [lm-sensors] " Huang0
@ 2005-05-31 13:40 ` Huang0
5 siblings, 0 replies; 7+ messages in thread
From: Huang0 @ 2005-05-31 13:40 UTC (permalink / raw)
To: lm-sensors
Hi Rudolf, Jean
I finished the modification the 792 driver for linux-2.6 with your
suggestions
after your review, please refer to the contents below for more details.
The attachment is the 792 driver for linux-2.6, if you have time, would
you
please check my codes again before other guys start the test? Especially
the modified codes. After your confirmation and test, please send it to
the
kernel group for me, then give me a response, thanks.
On your request, the "sign-off" is:
Signed-off-by: Chunhao Huang <huang0@winbond.com.tw>
> >(1) We misunderstand the meaning of temperature low limit, this bug
also
> >exists in the 792 driver for linux-2.4 and our 792 Windows driver.
>
> Please can you fix this ASAP so we can send it to kernel folks?
Fixed, need the up to date lm_sensors in the CVS, because the
lm_sensors-2.9.1
does NOT work.
> >(2) I have not implemented the 0.5 degree to temperature2 and
temperature3,
> >while the driver for linux-2.4 implemented this.
>
> That is the reason temp1 has separate handling and variable? If so,
please add
> the code.
> or better would be to fix existing code so it does not look so
strange.
>
> From Jean:
> No, better would be to actually add the handling of the additional
> resolution bit.
Fixed, it took me much time to fix this bug :-(
I don't think the chip drivers for linux-2.6 are of good
maintainability,
and readability because of those many macros in the driver.
> >(3) The 792 driver for linux-2.4 can adjust the fan divisor
automatically,
> >while I have not implemented it in this 2.6 one.
>
> This would be nice but better to put there some driver instead of
none...
> I think this can wait but it is good to have it soon.
NOT fixed, because I do NOT think the method in 792 driver for linux-2.4
is good enough. I will keep it until finding a good method to realize
the fan divisor's auto modification
> > #include <linux/config.h>
> > #include <linux/module.h>
> > #include <linux/init.h>
> > #include <linux/slab.h>
> > #include <linux/i2c.h>
> > #include <linux/i2c-sensor.h>
> > #include <linux/i2c-vid.h>
> > #include <asm/io.h>
> Do you really need this header?
Fixed
> > /* #include "lm75.h" */
> Please remove this.
Fixed
> > /* Addresses to scan */
> > static unsigned short normal_i2c[] = { 0x2c, 0x2f, I2C_CLIENT_END };
> > static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
> >
> > /* Insmod parameters */
> > SENSORS_INSMOD_1(w83792d);
> > I2C_CLIENT_MODULE_PARM(force_subclients, "List of subclient
addresses: "
> > "{bus, clientaddr, subclientaddr1,
subclientaddr2}");
> >
> > static int init;
> > module_param(init, bool, 0);
> > MODULE_PARM_DESC(init, "Set to one for chip initialization");
>
> Maybe better "Set to one to force chip initialization"
Fixed
> > /* show/display the debug message */
> > #define W83792D_DEBUG 1
>
> There is some global #define for debugging called DEBUG it seems, so
maybe it
> would be good
> to remove this and use DEBUG later.
Fiexed
> > #define W83792D_REG_CHASSIS 0x42 /* Bit 5: Case Open status bit
*/
> > #define W83792D_REG_CHASSIS_CLR 0x44 /* Bit 7: Case Open
CLR_CHS/Reset
> bit */
> >
> > /* ctroll in0/in1 's limit modifiability */
> perhap a typo ?
Fixed
> > u8 in[9]; /* Register value */
> > u8 in_max[9]; /* Register value */
> > u8 in_min[9]; /* Register value */
> > u8 low_bits[2]; /* Register value */
>
> Please comment that lowbits belong to IN, or choose better name for
the variable
Fixed
> > u8 fan[7]; /* Register value */
> > u8 fan_min[7]; /* Register value */
> > u8 temp;
> > u8 temp_max; /* Register value */
> > u8 temp_max_hyst; /* Register value */
> > u8 temp_add[2]; /* Register value */
>
> Please align comment with tab to same col as above.
Fixed
> Also why is temp1 separated? why there is not temps[3] instead?
temp1 is separated because temp2 and temp3 need addtional resolution bit
to get/set the 0.5 degree, while temp1 need NOT.
> > u8 temp_max_add[2]; /* Register value */
> > u8 temp_max_hyst_add[2]; /* Register value */
>
> Big I and align too please, with tab to same col as above.
Fixed
> > #ifdef W83792D_DEBUG
> > static void w83792d_print_debug(struct w83792d_data *data);
> > #endif
>
> I think you can change it to: #ifdef DEBUG
Fixed
> > static ssize_t
> > store_fan_min(struct device *dev, const char *buf, size_t count, int
nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> >
> > val = simple_strtoul(buf, NULL, 10);
> > data->fan_min[nr - 1] > > FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr - 1]));
>
> Please use second tab instead of 4 spaces
Fixed
> > #define show_temp_reg(reg) \
> > static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> > { \
> > struct w83792d_data *data = w83792d_update_device(dev); \
> > if (nr >= 2) { /* TEMP2 and TEMP3 */ \
> > return sprintf(buf,"%ld\n", \
> > (long)TEMP_FROM_REG(data->reg##_add[nr-2])); \
> > } else { /* TEMP1 */ \
> > return sprintf(buf,"%ld\n",
(long)TEMP_FROM_REG(data->reg)); \
> > } \
> > }
>
> Same question related here? Why temp1 separate?
Fixed, please refer to one reason above:
temp2 and temp3 need addtional resolution
> > show_temp_reg(temp);
> > show_temp_reg(temp_max);
> > show_temp_reg(temp_max_hyst);
> >
> > #define store_temp_reg(REG, reg) \
> > static ssize_t store_temp_##reg (struct device *dev, const char
*buf, size_t
> count, int nr) \
> > { \
> > struct i2c_client *client = to_i2c_client(dev); \
> > struct w83792d_data *data = i2c_get_clientdata(client); \
> > s32 val; \
> > \
> > val = simple_strtol(buf, NULL, 10); \
> > \
> > if (nr >= 2) { \
> > data->temp_##reg##_add[nr-2] = TEMP_TO_REG(val); \
> > w83792d_write_value(client,
W83792D_REG_TEMP_##REG[nr-1], \
> > data->temp_##reg##_add[nr-2]); \
> > } else { \
> > data->temp_##reg = TEMP_TO_REG(val); \
> > w83792d_write_value(client,
W83792D_REG_TEMP_##REG[nr-1], \
> > data->temp_##reg); \
> > } \
> > \
> > return count; \
> > }
>
> Same question related here? Why temp1 separate?
Same as above
> > store_fan_div_reg(struct device *dev, const char *buf, size_t count,
int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > unsigned long min;
> > /*u8 reg;*/
>
> Please remove
>
> > u8 fan_div_reg=0; u8 tmp_fan_div;
>
> Please separate them on two lines.
Fixed
> > /* Save fan_min */
> > min = FAN_FROM_REG(data->fan_min[nr],
> > DIV_FROM_REG(data->fan_div[nr]));
> >
> > data->fan_div[nr] = DIV_TO_REG(simple_strtoul(buf, NULL, 10));
> >
> > fan_div_reg = w83792d_read_value(client,
W83792D_REG_FAN_DIV[nr/2]);
>
> nr/2 could be [nr >> 1]
>
> > fan_div_reg &= (nr%2 = 0) ? 0xf8 : 0x8f;
>
> and here
>
> fan_div_reg &= (nr & 1) ? 0x8f : 0xf8;
Fixed
> > tmp_fan_div = (nr%2 = 0) ? ((data->fan_div[nr])&0x07)
> > :
(((data->fan_div[nr])<<4)&0x70);
>
> This needs definetly spaces between operators but it can be also done
this way:
>
> tmp_fan_div = (nr & 1) ? ((data->fan_div[nr] << 4) & 0x70)
> : data->fan_div[nr] & 0x07;
Fixed
> > w83792d_write_value(client, W83792D_REG_FAN_DIV[nr/2],
>
> nr >> 1
Fixed
> > static ssize_t
> > show_pwmenable_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > long pwm_enable_tmp = 1;
>
> Should be u32 instead?
>
> From Jean:
> u32 or long doesn't make much difference when you only need to store
> values from 0 to 2. Even an u8 would do.
> Note that this function could be made more readable/compact/efficient
by
> using either a switch/case construct, or a PWM_ENABLE_FROM_REG macro
or
> inline function, or even a lookup table.
Not fixed, original codes is kept.
> > cfg1_tmp = data->pwmenable[0];
> > cfg2_tmp = (data->pwmenable[1]) << 2;
> > cfg3_tmp = (data->pwmenable[2]) << 4;
> > cfg4_tmp = w83792d_read_value(client,W83792D_REG_FAN_CFG) &
0xc0;
> > fan_cfg_tmp = ((cfg4_tmp|cfg3_tmp)|cfg2_tmp)|cfg1_tmp;
>
> Missing spaces
> fan_cfg_tmp = ((cfg4_tmp | cfg3_tmp) | cfg2_tmp) | cfg1_tmp;
>
> From Jean:
> And useless parentheses ;)
Fixed, spaces have been added, but parentheses is kept because of my
own coding style :-)
> > static ssize_t
> > store_chassis_clear_reg(struct device *dev, const char *buf, size_t
count)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 temp1 = 0, temp2 = 0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->chassis_clear = SENSORS_LIMIT(val, 0 ,1);
> > temp1 = ((data->chassis_clear) << 7) & 0x80;
> > temp2 = w83792d_read_value(client,
> > W83792D_REG_CHASSIS_CLR) & 0x7f;
> > w83792d_write_value(client, W83792D_REG_CHASSIS_CLR,
temp1|temp2);
>
> Missing spaces between |
Fixed
> > static ssize_t
> > store_thermal_cruise_reg(struct device *dev, const char *buf, size_t
count,
> int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 target_tmp=0, target_mask=0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > target_tmp = val;
> > target_tmp = target_tmp & 0x7f;
> > target_mask = w83792d_read_value(client,
W83792D_REG_THERMAL[nr-1]) &
> > 0x80;
> > data->thermal_cruise[nr-1] = SENSORS_LIMIT(target_tmp, 0, 255);
> > w83792d_write_value(client, W83792D_REG_THERMAL[nr-1],
> > (data->thermal_cruise[nr-1])|target_mask);
>
> Missing spaces between |
Fixed
> > static ssize_t
> > store_tolerance_reg(struct device *dev, const char *buf, size_t
count, int
> nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 tol_tmp, tol_mask;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > tol_mask = w83792d_read_value(client,
> > W83792D_REG_TOLERANCE[nr-1]) & ((nr=2)?0x0f:0xf0);
>
> Missing spaces: ((nr = 2) ? 0x0f : 0xf0);
Fixed
> > tol_tmp = SENSORS_LIMIT(val, 0, 15);
> > tol_tmp &= 0x0f;
> > data->tolerance[nr-1] = tol_tmp;
> > if (nr=2) {
> > tol_tmp <<= 4;
> > }
> > w83792d_write_value(client, W83792D_REG_TOLERANCE[nr-1],
> > tol_mask|tol_tmp);
>
> Missing spaces between |
Fixed
> > static ssize_t
> > show_sf2_level_reg(struct device *dev, char *buf, int nr, int index)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n",
> (long)(((data->sf2_levels[index-1][nr])*100)/15));
>
> Missing spaces
>
> From Jean:
> Also note that the (long) cast could (and should) be avoided.
>
Fixed
> > static ssize_t
> > store_sf2_level_reg(struct device *dev, const char *buf, size_t
count, int
> nr, int index)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 mask_tmp=0, level_tmp=0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->sf2_levels[index-1][nr] > > SENSORS_LIMIT((val*15)/100, 0, 15);
>
> And here too it seems
Fixed
> > /* This function is called when:
> > * w83792d_driver is inserted (when this module is loaded), for
each
> > available adapter
> > * when a new adapter is inserted (and w83792d_driver is still
present)
> */
> > static int
> > w83792d_attach_adapter(struct i2c_adapter *adapter)
>
> I think static int and rest belong to one line
NOT Fixed, code is kept accroding Jean's comments.
> > {
> > if (!(adapter->class & I2C_CLASS_HWMON))
> > return 0;
> > return i2c_detect(adapter, &addr_data, w83792d_detect);
> > }
> >
> >
> > static int
> > w83792d_detect_subclients(struct i2c_adapter *adapter, int address,
int
> kind,
> > struct i2c_client *new_client)
>
> and here too
Not Fixed, same as above
> > if (force_subclients[0] = id && force_subclients[1] = address)
{
> > for (i = 2; i <= 3; i++) {
> > if (force_subclients[i] < 0x48 ||
> > force_subclients[i] > 0x4f) {
> > dev_err(&new_client->dev, "invalid
subclient "
> > "address %d; must be
0x48-0x4f\n",
> > force_subclients[i]);
> > err = -ENODEV;
> > goto ERROR_SC_2;
> > }
> > }
> > w83792d_write_value(new_client, W83792D_REG_I2C_SUBADDR,
> > (force_subclients[2] & 0x03) |
> > ((force_subclients[3] & 0x03)
<<4));
> > data->lm75[0]->addr = force_subclients[2];
> > data->lm75[1]->addr = force_subclients[3];
> > } else {
> > int val = w83792d_read_value(new_client,
W83792D_REG_I2C_SUBADDR);
>
> This should be u8 val
Fixed
> > static int
> > w83792d_detect(struct i2c_adapter *adapter, int address, int kind)
> > {
>
> Same line?
NOT Fixed, same as above
> > /* Now, we do the remaining detection. */
> >
> > /* The w8378?d may be stuck in some other bank than bank 0. This
may
>
> Bad chip name in comment
Fixed
> > if (kind < 0) {
> > if (w83792d_read_value(new_client, W83792D_REG_CONFIG) &
0x80) {
> > dev_warn(&new_client->dev, "Detection failed at
step "
> > "3\n");
> > goto ERROR1;
> > }
> > val1 = w83792d_read_value(new_client, W83792D_REG_BANK);
> > val2 = w83792d_read_value(new_client,
W83792D_REG_CHIPMAN);
> > /* Check for Winbond ID if in bank 0 */
> > if (!(val1 & 0x07)) { /* is Bank0 */
> > if (((!(val1 & 0x80)) && (val2 != 0xa3)) ||
> > ((val1 & 0x80) && (val2 != 0x5c))) {
> > goto ERROR1;
> > }
> > }
> > /* If Winbond SMBus, check address at 0x48 */
>
> better comment would be:
> If Winbond chip, address of chip and W83792D_REG_I2C_ADDR should
match.
Fixed
> > /* We have either had a force parameter, or we have already
detected the
> > Winbond. Put it now into bank 0 and Vendor ID High Byte */
>
> > w83792d_write_value(new_client, W83792D_REG_BANK,
> > (w83792d_read_value(new_client,
> > W83792D_REG_BANK) &
0x78) |
> > 0x80);
> >
>
> Maybe better indent?
Fixed
> > static int
> > w83792d_detach_client(struct i2c_client *client)
>
> I think one line too.
Not Fixed, same as above
> > static int
> > w83792d_read_value(struct i2c_client *client, u8 reg)
>
> I think one line too.
Not Fixed, same as above
> > static int
> > w83792d_write_value(struct i2c_client *client, u8 reg, u8 value)
>
> I think one line too.
Not Fixed, same as above
> > /* Called when we have found a new W83792D. It should set limits,
etc. */
> > static void
> > w83792d_init_client(struct i2c_client *client)
> > {
> > int temp2_cfg, temp3_cfg, i=0;
>
> should not be u8 ? (except of i)
Fixed
> > #ifdef W83792D_DEBUG
> > w83792d_print_debug(data);
> > #endif
>
> Do not forget to change this too.
Fixed
> > return data;
> > }
> >
> > #ifdef W83792D_DEBUG
>
> and here :)
Fixed
> > MODULE_AUTHOR("Chunhao Huang @ Winbond");
>
> No email?
Fixed
Thanks
Best Regards
Chunhao
==============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.
==============================================If your computer is unable to decode Chinese font, please ignore the following message.It essentially repeats the statement in English given above.本信件內所含華邦電子的財產性機密性資訊, 僅授權原發信人指定之收信人取閱\\\之用. 假使您並非被指定之收信人或因任何原因在未經授權的情形之下收到本信件, 請您告知原發信人並立即將信件從電腦與網路伺服器中予以消除. 對於您的合作, 我們先此致謝. 特此提醒, 任何未經授權擅自使用華邦電子的機密資訊的行為是被嚴格禁止的. 信件與華邦電子營業無關之內容,不得視為華邦電子之立場或意見.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83792d.c
Type: application/octet-stream
Size: 54327 bytes
Desc: w83792d.c
Url : http://lists.atrpms.net/pipermail/lm-sensors/attachments/20050531/c8f57679/w83792d-0001.obj
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2005-05-31 13:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-05-26 19:25 [lm-sensors] W83792 review Rudolf Marek
2005-05-27 3:33 ` [lm-sensors] " Huang0
2005-05-28 15:31 ` [lm-sensors] " Jean Delvare
2005-05-28 15:55 ` [lm-sensors] " Jean Delvare
2005-05-29 5:22 ` Huang0
2005-05-30 11:02 ` [lm-sensors] " Huang0
2005-05-31 13:40 ` Huang0
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.