Hello George, I tested on Intel WX58BP with Intel Core i7, it works great, thanks for the patch. Here are few knits : > From: George Joseph <george.joseph@xxxxxxxxxxxxx> > > Hwmon driver for Andigilog aSC7621 family monitoring chips. > > Signed-off-by: George Joseph <george.joseph@xxxxxxxxxxxxx> > --- > > Patch against 2.6.33-rc6. > > Documentation/hwmon/asc7621 | 295 ++++++++++ > MAINTAINERS | 7 > drivers/hwmon/Kconfig | 13 > drivers/hwmon/Makefile | 1 > drivers/hwmon/asc7621.c | 1266 ++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 1582 insertions(+) > > diff -uprN a/Documentation/hwmon/asc7621 b/Documentation/hwmon/asc7621 > --- a/Documentation/hwmon/asc7621 1969-12-31 17:00:00.000000000 -0700 > +++ b/Documentation/hwmon/asc7621 2010-01-30 12:43:13.000000000 -0700 > @@ -0,0 +1,295 @@ > +Kernel driver asc7621 > +================== > + > +Supported chips: > + Andigilog aSC7621 and aSC7621a > + Prefix: 'asc7621' > + Addresses scanned: I2C 0x2c, 0x2d, 0x2e > + Datasheet: http://www.andigilog.com/downloads/aSC7621_70A06010.pdf > + This link does not exist, if possible provide this datasheet to Jean so that he can host it on lm-sensors website to use as reference. > +Author: > + George Joseph > + > +Description provided by Dave Pivin @ Andigilog: > + > +Andigilog has both the PECI and pre-PECI versions of the Heceta-6, as > +Intel calls them. Heceta-6e has high frequency PWM and Heceta-6p has > +added PECI and a 4th thermal zone. The Andigilog aSC7611 is the > +Heceta-6e part and aSC7621 is the Heceta-6p part. They are both in > +volume production, shipping to Intel and their subs. > + > +We have enhanced both parts relative to the governing Intel > +specification. First enhancement is temperature reading resolution. We > +have used registers below 20h for vendor-specific functions in addition > +to those in the Intel-specified vendor range. > + > +Our conversion process produces a result that is reported as two bytes. > +The fan speed control uses this finer value to produce a "step-less" fan > +PWM output. These two bytes are "read-locked" to guarantee that once a > +high or low byte is read, the other byte is locked-in until after the > +next read of any register. So to get an atomic reading, read high or low > +byte, then the very next read should be the opposite byte. Our data > +sheet says 10-bits of resolution, although you may find the lower bits > +are active, they are not necessarily reliable or useful externally. We > +chose not to mask them. > + > +We employ significant filtering that is user tunable as described in the > +data sheet. Our temperature reports and fan PWM outputs are very smooth > +when compared to the competition, in addition to the higher resolution > +temperature reports. The smoother PWM output does not require user > +intervention. > + > +We offer GPIO features on the former VID pins. These are open-drain > +outputs or inputs and may be used as general purpose I/O or as alarm > +outputs that are based on temperature limits. These are in 19h and 1Ah. > + > +We offer flexible mapping of temperature readings to thermal zones. Any > +temperature may be mapped to any zone, which has a default assignment > +that follows Intel's specs. > + > +Since there is a fan to zone assignment that allows for the "hotter" of > +a set of zones to control the PWM of an individual fan, but there is no > +indication to the user, we have added an indicator that shows which zone > +is currently controlling the PWM for a given fan. This is in register > +00h. > + > +Both remote diode temperature readings may be given an offset value such > +that the reported reading as well as the temperature used to determine > +PWM may be offset for system calibration purposes. > + > +PECI Extended configuration allows for having more than two domains per > +PECI address and also provides an enabling function for each PECI > +address. One could use our flexible zone assignment to have a zone > +assigned to up to 4 PECI addresses. This is not possible in the default > +Intel configuration. This would be useful in multi-CPU systems with > +individual fans on each that would benefit from individual fan control. > +This is in register 0Eh. > + > +The tachometer measurement system is flexible and able to adapt to many > +fan types. We can also support pulse-stretched PWM so that 3-wire fans > +may be used. These characteristics are in registers 04h to 07h. > + > +Finally, we have added a tach disable function that turns off the tach > +measurement system for individual tachs in order to save power. That is > +in register 75h. > + > +-- > +aSC7621 Product Description > + > +The aSC7621 has a two wire digital interface compatible with SMBus 2.0. > +Using a 10-bit ADC, the aSC7621 measures the temperature of two remote diode > +connected transistors as well as its own die. Support for Platform > +Environmental Control Interface (PECI) is included. > + > +Using temperature information from these four zones, an automatic fan speed > +control algorithm is employed to minimize acoustic impact while achieving > +recommended CPU temperature under varying operational loads. > + > +To set fan speed, the aSC7621 has three independent pulse width modulation > +(PWM) outputs that are controlled by one, or a combination of three, > +temperature zones. Both high- and low-frequency PWM ranges are supported. > + > +The aSC7621 also includes a digital filter that can be invoked to smooth > +temperature readings for better control of fan speed and minimum acoustic > +impact. > + > +The aSC7621 has tachometer inputs to measure fan speed on up to four fans. > +Limit and status registers for all measured values are included to alert > +the system host that any measurements are outside of programmed limits > +via status registers. > + > +System voltages of VCCP, 2.5V, 3.3V, 5.0V, and 12V motherboard power are > +monitored efficiently with internal scaling resistors. > + > +Features > +- Supports PECI interface and monitors internal and remote thermal diodes > +- 2-wire, SMBus 2.0 compliant, serial interface > +- 10-bit ADC > +- Monitors VCCP, 2.5V, 3.3V, 5.0V, and 12V motherboard/processor supplies > +- Programmable autonomous fan control based on temperature readings > +- Noise filtering of temperature reading for fan speed control > +- 0.25C digital temperature sensor resolution > +- 3 PWM fan speed control outputs for 2-, 3- or 4-wire fans and up to 4 fan > + tachometer inputs > +- Enhanced measured temperature to Temperature Zone assignment. > +- Provides high and low PWM frequency ranges > +- 3 GPIO pins for custom use > +- 24-Lead QSOP package > + > +Configuration Notes > +=================== > + > +Except where noted below, the sysfs entries created by this driver follow > +the standards defined in "sysfs-interface". > + > +temp1_source > + 0 (default) peci_legacy = 0, Remote 1 Temperature > + peci_legacy = 1, PECI Processor Temperature 0 > + 1 Remote 1 Temperature > + 2 Remote 2 Temperature > + 3 Internal Temperature > + 4 PECI Processor Temperature 0 > + 5 PECI Processor Temperature 1 > + 6 PECI Processor Temperature 2 > + 7 PECI Processor Temperature 3 > + > +temp2_source > + 0 (default) Internal Temperature > + 1 Remote 1 Temperature > + 2 Remote 2 Temperature > + 3 Internal Temperature > + 4 PECI Processor Temperature 0 > + 5 PECI Processor Temperature 1 > + 6 PECI Processor Temperature 2 > + 7 PECI Processor Temperature 3 > + > +temp3_source > + 0 (default) Remote 2 Temperature > + 1 Remote 1 Temperature > + 2 Remote 2 Temperature > + 3 Internal Temperature > + 4 PECI Processor Temperature 0 > + 5 PECI Processor Temperature 1 > + 6 PECI Processor Temperature 2 > + 7 PECI Processor Temperature 3 > + > +temp4_source > + 0 (default) peci_legacy = 0, PECI Processor Temperature 0 > + peci_legacy = 1, Remote 1 Temperature > + 1 Remote 1 Temperature > + 2 Remote 2 Temperature > + 3 Internal Temperature > + 4 PECI Processor Temperature 0 > + 5 PECI Processor Temperature 1 > + 6 PECI Processor Temperature 2 > + 7 PECI Processor Temperature 3 > + > +temp[1-4]_smoothing_enable > +temp[1-4]_smoothing_time > + Smooths spikes in temp readings caused by noise. > + 0 35 sec > + 1 17.6 sec > + 2 11.8 sec > + 3 7.0 sec > + 4 4.4 sec > + 5 3.0 sec > + 6 1.6 sec > + 7 0.8 sec > + > +temp[1-4]_crit > + When the corresponding zone temperature reaches this value, > + ALL pwm outputs will got to 100%. > + > +temp[5-8]_input > +temp[5-8]_enable > + The aSC7621 can also read temperatures provided by the processor > + via the PECI bus. Usually these are "core" temps and are relative > + to the point where the automatical thermal control circuit starts Fix the typo : + to the point where the automatically thermal control circuit starts > + throttling. This means that these are usually negative numbers. > + > +pwm[1-3]_enable > + 0 Fan off. > + 1 Fan on manual control. > + 2 Fan on automatic control and will run at the minimum pwm > + if the temperature for the zone is below the minimum. > + 3 Fan on automatic control but will be off if the temperature > + for the zone is below the minimum. > + 4-254 Ignored. > + 255 Fan on full. > + > +pwm[1-3]_auto_channels > + Bitmap as described in sysctl-interface with the following > + exceptions... > + Only the following combinations of zones (and their corresponding masks) + Only the following combination of zones (and their corresponding masks) > + are valid: > + 1 > + 2 > + 3 > + 2,3 > + 1,2,3 > + 4 > + 1,2,3,4 > + > + Special values: > + 0 Disabled. > + 16 Fan on manual control. > + 31 Fan on full. > + > + > +pwm[1-3]_invert > + When set, inverts the meaning of pwm[1-3]. > + I.E. when pwm = 0, the fan will be on full and I.E. is used for Internet Explorer, I think you mean : + i.e. when pwm = 0, the fan will be on full and > + when pwm = 255 the fan will be off. > + > +pwm[1-3]_freq > + PWM frequency in Hz > + Valid values in Hz are: > + > + 10 > + 15 > + 23 > + 30 (default) > + 38 > + 47 > + 62 > + 94 > + 23000 > + 24000 > + 25000 > + 26000 > + 27000 > + 28000 > + 29000 > + 30000 > + > + Setting any other value will be ignored. > + > +peci_enable > + Enables or disables PECI > + > +peci_avg > + Input filter averate time. Fix this typo : + Input filter average time. > + > + 0 0 Sec. (no Smoothing) (default) > + 1 0.25 Sec. > + 2 0.5 Sec. > + 3 1.0 Sec. > + 4 2.0 Sec. > + 5 4.0 Sec. > + 6 8.0 Sec. > + 7 0.0 Sec. > + > +peci_legacy > + > + 0 Standard Mode (default) > + Remote Diode 1 reading is associated with > + Temperature Zone 1, PECI is associated with > + Zone 4 > + > + 1 Legacy Mode > + PECI is associated with Temperature Zone 1, > + Remote Diode 1 is associated with Zone 4 > + > +peci_diode > + Diode filter > + > + 0 0.25 Sec. > + 1 1.1 Sec. > + 2 2.4 Sec. (default) > + 3 3.4 Sec. > + 4 5.0 Sec. > + 5 6.8 Sec. > + 6 10.2 Sec. > + 7 16.4 Sec. > + > +peci_4domain > + Four domain enable > + > + 0 1 or 2 Domains for enabled processors (default) > + 1 3 or 4 Domains for enabled processors > + > +peci_domain > + Domain > + > + 0 Processor contains a single domain (0) (default) > + 1 Processor contains two domains (0,1) > diff -uprN a/drivers/hwmon/asc7621.c b/drivers/hwmon/asc7621.c > --- a/drivers/hwmon/asc7621.c 1969-12-31 17:00:00.000000000 -0700 > +++ b/drivers/hwmon/asc7621.c 2010-01-30 14:53:56.000000000 -0700 > @@ -0,0 +1,1266 @@ > +/* > + asc7621.c - Part of lm_sensors, Linux kernel modules for hardware monitoring > + Copyright (c) 2007, 2010 George Joseph <george.joseph@xxxxxxxxxxxxx> > + > + Chip details at <http://www.andigilog.com> > + Please keep it consistent and this web site is showing wrong information replace above 5 lines with : + * asc7621.c - Part of lm_sensors, Linux kernel modules for hardware monitoring + * Copyright (C) 2007, 2010 George Joseph <george.joseph@xxxxxxxxxxxxx> + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/jiffies.h> > +#include <linux/i2c.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/err.h> > +#include <linux/mutex.h> > + > +/* Addresses to scan */ > +static unsigned short normal_i2c[] = { > + 0x2c, 0x2d, 0x2e, I2C_CLIENT_END > +}; > + > +enum asc7621_type { > + asc7621, > + asc7621a > +}; > + > +#define INTERVAL_HIGH (HZ + HZ / 2) > +#define INTERVAL_LOW (1 * 60 * HZ) > +#define PRI_NONE 0 > +#define PRI_LOW 1 > +#define PRI_HIGH 2 > +#define FIRST_CHIP asc7621 > +#define LAST_CHIP asc7621a Please aligned it to make it more readable : +#define INTERVAL_HIGH (HZ + HZ / 2) +#define INTERVAL_LOW (1 * 60 * HZ) +#define PRI_NONE 0 +#define PRI_LOW 1 +#define PRI_HIGH 2 +#define FIRST_CHIP asc7621 +#define LAST_CHIP asc7621a > + > +struct asc7621_chip { > + char *name; > + enum asc7621_type chip_type; > + u8 company_reg; > + u8 company_id; > + u8 verstep_reg; > + u8 verstep_id; > + unsigned short *addresses; > +}; > + > +static struct asc7621_chip asc7621_chips[] = { > + { > + .name = "asc7621", > + .chip_type = asc7621, > + .company_reg = 0x3e, > + .company_id = 0x61, > + .verstep_reg = 0x3f, > + .verstep_id = 0x6c, > + .addresses = normal_i2c, Please also align this : + .name = "asc7621", + .chip_type = asc7621, + .company_reg = 0x3e, + .company_id = 0x61, + .verstep_reg = 0x3f, + .verstep_id = 0x6c, + .addresses = normal_i2c, > + }, > + { > + .name = "asc7621a", > + .chip_type = asc7621a, > + .company_reg = 0x3e, > + .company_id = 0x61, > + .verstep_reg = 0x3f, > + .verstep_id = 0x6d, > + .addresses = normal_i2c, ditto : + .name = "asc7621a", + .chip_type = asc7621a, + .company_reg = 0x3e, + .company_id = 0x61, + .verstep_reg = 0x3f, + .verstep_id = 0x6d, + .addresses = normal_i2c, > + }, > +}; > + > +/* > + * Defines the highest register to be used, not the count. > + * The actual count will probably be smaller because of gaps > + * in the implementation (unused register locations). > + * This define will safely set the array size of both the parameter > + * and data arrays. > + * This comes from the data sheet register description table. > + */ > +#define LAST_REGISTER 0xff > + > +struct asc7621_data { > + struct i2c_client client; > + struct device *class_dev; > + struct mutex update_lock; > + int valid; /* !=0 if following fields are valid */ > + unsigned long last_high_reading; /* In jiffies */ > + unsigned long last_low_reading; /* In jiffies */ ditto : + unsigned long last_low_reading; /* In jiffies */ > + /* > + * Registers we care about occupy the corresponding index > + * in the array. Registers we don't care about are left > + * at 0. > + */ > + u8 reg[LAST_REGISTER + 1]; > +}; > + > +/* > + * Macro to get the parent asc7621_param structure > + * from a sensor_device_attribute passed into the > + * show/store functions. > + */ > +#define to_asc7621_param(_sda) \ > + container_of(_sda, struct asc7621_param, sda) > + > +/* > + * Each parameter to be retrieved needs an asc7621_param structure > + * allocated. It contains the sensor_device_attribute structure > + * and the control info needed to retrieve the value from the register map. > + */ > +struct asc7621_param { > + struct sensor_device_attribute sda; > + u8 priority; > + u8 msb[3]; > + u8 lsb[3]; > + u8 mask[3]; > + u8 shift[3]; > +}; > + > +/* > + * This is the map that ultimately indicates whether we'll be > + * retrieving a register value or not, and at what frequency. > + */ > +static u8 asc7621_register_priorities[255]; > + > +static struct asc7621_data *asc7621_update_device(struct device *dev); > + > +#define read_byte(reg) (i2c_smbus_read_byte_data(client, reg) & 0xff) > +#define write_byte(reg, data) i2c_smbus_write_byte_data(client, reg, data) Please also align this : +#define read_byte(reg) (i2c_smbus_read_byte_data(client, reg) & 0xff) +#define write_byte(reg, data) i2c_smbus_write_byte_data(client, reg, data) > + > +/* > + * Data Handlers > + * Each function handles the formatting, storage > + * and retrieval of like parameters. > + */ > + > +#define SETUP_SHOW_data_param(d, a) \ > + struct sensor_device_attribute *sda = to_sensor_dev_attr(a); \ > + struct asc7621_data *data = asc7621_update_device(d); \ > + struct asc7621_param *param = to_asc7621_param(sda) > + > +#define SETUP_STORE_data_param(d, a) \ > + struct sensor_device_attribute *sda = to_sensor_dev_attr(a); \ > + struct i2c_client *client = to_i2c_client(d); \ > + struct asc7621_data *data = i2c_get_clientdata(client); \ > + struct asc7621_param *param = to_asc7621_param(sda) > + > +/* > + * u8 is just what it sounds like...an unsigned byte with no > + * special formatting. > + */ > +static ssize_t show_u8(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + > + return sprintf(buf, "%u\n", data->reg[param->msb[0]]); > +} > + > +static ssize_t store_u8(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + reqval = SENSORS_LIMIT(reqval, 0, 255); > + > + mutex_lock(&data->update_lock); > + data->reg[param->msb[0]] = reqval; > + write_byte(param->msb[0], reqval); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +/* > + * Many of the config values occupy only a few bits of a register. > + */ > +static ssize_t show_bitmask(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + > + return sprintf(buf, "%u\n", > + (data->reg[param->msb[0]] >> param-> > + shift[0]) & param->mask[0]); > +} > + > +static ssize_t store_bitmask(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + remove above line. > + long reqval; > + u8 currval; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + reqval = SENSORS_LIMIT(reqval, 0, param->mask[0]); > + > + reqval = (reqval & param->mask[0]) << param->shift[0]; > + > + mutex_lock(&data->update_lock); > + currval = read_byte(param->msb[0]); > + reqval |= (currval & ~(param->mask[0] << param->shift[0])); > + data->reg[param->msb[0]] = reqval; > + write_byte(param->msb[0], reqval); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +/* > + * 16 bit fan rpm values > + * reported by the device as the number of 11.111us periods (90khz) > + * between full fan rotations. Therefore... > + * RPM = (90000 * 60) / register value > + */ > +static ssize_t show_fan16(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + u16 regval; > + > + mutex_lock(&data->update_lock); > + regval = (data->reg[param->msb[0]] << 8) | data->reg[param->lsb[0]]; > + mutex_unlock(&data->update_lock); > + > + return sprintf(buf, "%u\n", > + (regval == 0 ? -1 : (regval) == > + 0xffff ? 0 : 5400000 / regval)); > +} > + > +static ssize_t store_fan16(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + reqval = > + (SENSORS_LIMIT((reqval) <= 0 ? 0 : 5400000 / (reqval), 0, 65534)); > + > + mutex_lock(&data->update_lock); > + data->reg[param->msb[0]] = (reqval >> 8) & 0xff; > + data->reg[param->lsb[0]] = reqval & 0xff; > + write_byte(param->msb[0], data->reg[param->msb[0]]); > + write_byte(param->lsb[0], data->reg[param->lsb[0]]); > + mutex_unlock(&data->update_lock); > + > + return count; > +} > + > +/* > + * Voltages are scaled in the device so that the nominal voltage > + * is 3/4ths of the 0-255 range (I.E 192). you mean : + * is 3/4ths of the 0-255 range (i.e. 192). > + * If all voltages are 'normal' then all voltage registers will > + * read 0xC0. This doesn't help us if we don't have a point of refernce. > + * The data sheet however provides us with the full scale value for each > + * which is stored in in_scaling. The sda->index parameter value provides > + * the index into in_scaling. > + * > + * NOTE: The chip expects the first 2 inputs be 2.5 and 2.25 volts > + * respectively. That doesn't mean that's what the motherboard provides. :) > + */ > + > +static int asc7621_in_scaling[] = { > + 3320, 3000, 4380, 6640, 16000 > +}; > + > +static ssize_t show_in10(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + u16 regval; > + u8 nr = sda->index; > + > + mutex_lock(&data->update_lock); > + regval = (data->reg[param->msb[0]] * asc7621_in_scaling[nr]) / 256; > + > + /* The LSB value is a 2-bit scaling of the MSB's LSbit value. > + * I.E. If the maximim voltage for this input is 6640 millivolts then > + * a MSB register value of 0 = 0mv and 255 = 6640mv. > + * A 1 step change therefore represents 25.9mv (6640 / 256). > + * The extra 2-bits therefore represent increments of 6.48mv. > + */ > + regval += ((asc7621_in_scaling[nr] / 256) / 4) * > + (data->reg[param->lsb[0]] >> 6); > + > + mutex_unlock(&data->update_lock); > + > + return sprintf(buf, "%u\n", regval); > +} > + > +/* 8 bit voltage values (the mins and maxs) */ > +static ssize_t show_in8(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + <--- remove this line. > + u8 nr = sda->index; > + > + return sprintf(buf, "%u\n", > + ((data->reg[param->msb[0]] * > + asc7621_in_scaling[nr]) / 256)); > +} > + > +static ssize_t store_in8(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + u8 nr = sda->index; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + reqval = SENSORS_LIMIT(reqval, 0, asc7621_in_scaling[nr]); > + > + reqval = (reqval * 256) / asc7621_in_scaling[nr]; > + > + mutex_lock(&data->update_lock); > + data->reg[param->msb[0]] = reqval; > + write_byte(param->msb[0], reqval); > + mutex_unlock(&data->update_lock); > + > + return count; > +} > + > +static ssize_t show_temp8(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + > + return sprintf(buf, "%d\n", ((s8) data->reg[param->msb[0]]) * 1000); > +} > + > +static ssize_t store_temp8(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + s8 temp; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + reqval = SENSORS_LIMIT(reqval, -127000, 127000); > + > + temp = reqval / 1000; > + > + mutex_lock(&data->update_lock); > + data->reg[param->msb[0]] = temp; > + write_byte(param->msb[0], temp); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +/* > + * Temperatures that occupy 2 bytes always have the whole > + * number of degrees in the MSB with some part of the LSB > + * indicating fractional degrees. > + */ > + > +/* mmmmmmmm.llxxxxxx */ > +static ssize_t show_temp10(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + u8 msb; > + u8 lsb; save one line by : + u8 msb, lsb; > + int temp; > + > + mutex_lock(&data->update_lock); > + msb = data->reg[param->msb[0]]; > + lsb = (data->reg[param->lsb[0]] >> 6) & 0x03; > + temp = (((s8) msb) * 1000) + (lsb * 250); > + mutex_unlock(&data->update_lock); > + > + return sprintf(buf, "%d\n", temp); > +} > + > +/* mmmmmm.ll */ > +static ssize_t show_temp62(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + <-- save one more line by remove this line. > + u8 regval = data->reg[param->msb[0]]; > + int temp = ((s8) (regval & 0xfc) * 1000) + ((regval & 0x03) * 250); > + > + return sprintf(buf, "%d\n", temp); > +} > + > +static ssize_t store_temp62(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + long i, f; save one more line : + long reqval, i, f; > + s8 temp; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + reqval = SENSORS_LIMIT(reqval, -32000, 31750); > + i = reqval / 1000; > + f = reqval - (i * 1000); > + temp = i << 2; > + temp |= f / 250; > + > + mutex_lock(&data->update_lock); > + data->reg[param->msb[0]] = temp; > + write_byte(param->msb[0], temp); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +/* > + * The aSC7621 doesn't provide an "auto_point2". Instead, you > + * specify the auto_point1 and a range. To keep with the sysfs > + * hwmon specs, we synthesize the auto_point_2 from them. > + */ > + > +static u32 asc7621_range_map[] = { > + 2000, 2500, 3330, 4000, 5000, 6670, 8000, 10000, > + 13330, 16000, 20000, 26670, 32000, 40000, 53330, 80000, > +}; > + > +static ssize_t show_ap2_temp(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + long auto_point1; > + u8 regval; > + int temp; > + > + mutex_lock(&data->update_lock); > + auto_point1 = ((s8) data->reg[param->msb[1]]) * 1000; > + regval = > + ((data->reg[param->msb[0]] >> param->shift[0]) & param->mask[0]); > + temp = auto_point1 + asc7621_range_map[SENSORS_LIMIT(regval, 0, 15)]; > + mutex_unlock(&data->update_lock); > + > + return sprintf(buf, "%d\n", temp); > + > +} > + > +static ssize_t store_ap2_temp(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + long auto_point1; > + int i = 0; > + u8 currval = 0; > + u8 newval = 255; Save two lines and no need to initialize i and currval : + long reqval, auto_point1; + int i; + u8 currval, newval = 255; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + mutex_lock(&data->update_lock); > + auto_point1 = data->reg[param->msb[1]] * 1000; > + for (i = ARRAY_SIZE(asc7621_range_map) - 1; i >= 0; i--) { > + if (reqval >= auto_point1 + asc7621_range_map[i]) { > + newval = i; > + break; > + } > + } > + if (newval == 255) { > + mutex_unlock(&data->update_lock); > + return -EINVAL; > + } > + > + newval = (newval & param->mask[0]) << param->shift[0]; > + currval = read_byte(param->msb[0]); > + newval |= (currval & ~(param->mask[0] << param->shift[0])); > + data->reg[param->msb[0]] = newval; > + write_byte(param->msb[0], newval); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t show_pwm_ac(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + u8 config; > + u8 altbit; > + u8 regval; You can also save 2 lines here : + u8 config, altbit, regval; > + u8 map[] = { > + 0x01, 0x02, 0x04, 0x1f, 0x00, 0x06, 0x07, 0x10, > + 0x08, 0x0f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f > + }; > + > + mutex_lock(&data->update_lock); > + config = (data->reg[param->msb[0]] >> param->shift[0]) & param->mask[0]; > + altbit = (data->reg[param->msb[1]] >> param->shift[1]) & param->mask[1]; > + regval = config | (altbit << 3); > + mutex_unlock(&data->update_lock); > + > + return sprintf(buf, "%u\n", map[SENSORS_LIMIT(regval, 0, 15)]); > +} > + > +static ssize_t store_pwm_ac(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + unsigned long reqval; > + > + u8 currval = 0; > + u8 config = 0; > + u8 altbit = 0; > + u8 newval = 0; You do not need to initialize there variables : + u8 currval, config, altbit, newval; > + u16 map[] = { > + 0x04, 0x00, 0x01, 0xff, 0x02, 0xff, 0x05, 0x06, > + 0x08, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x0f, > + 0x07, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x03, > + }; > + > + if (strict_strtoul(buf, 10, &reqval)) > + return -EINVAL; > + > + if (reqval > 31) > + return -EINVAL; > + > + reqval = map[reqval]; > + > + config = reqval & 0x07; > + altbit = (reqval >> 3) & 0x01; > + > + config = (config & param->mask[0]) << param->shift[0]; > + altbit = (altbit & param->mask[1]) << param->shift[1]; > + > + mutex_lock(&data->update_lock); > + currval = read_byte(param->msb[0]); > + newval = config | (currval & ~(param->mask[0] << param->shift[0])); > + newval = altbit | (newval & ~(param->mask[1] << param->shift[1])); > + data->reg[param->msb[0]] = newval; > + write_byte(param->msb[0], newval); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t show_pwm_enable(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + u8 config; > + u8 altbit; > + u8 minoff; > + u8 val; > + u8 newval; Save 4 lines by : + u8 config, altbit, minoff, val, newval; > + > + mutex_lock(&data->update_lock); > + config = (data->reg[param->msb[0]] >> param->shift[0]) & param->mask[0]; > + altbit = (data->reg[param->msb[1]] >> param->shift[1]) & param->mask[1]; > + minoff = (data->reg[param->msb[2]] >> param->shift[2]) & param->mask[2]; > + mutex_unlock(&data->update_lock); > + > + val = config | (altbit << 3); > + newval = 0; > + > + if (val == 3 || val >= 10) > + newval = 255; > + else if (val == 4) > + newval = 0; > + else if (val == 7) > + newval = 1; > + else if (minoff == 1) > + newval = 2; > + else > + newval = 3; > + > + return sprintf(buf, "%u\n", newval); > +} > + > +static ssize_t store_pwm_enable(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + u8 currval = 0; > + u8 config = 0; > + u8 altbit = 0; > + u8 newval = 0; > + u8 minoff = 255; Only minoff is need to initialize and save 4 lines by : + u8 currval, config, altbit, newval, minoff = 255; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + switch (reqval) { > + case (0): Please use this : + case 0: > + newval = 0x04; > + break; > + case (1): + case 1: > + newval = 0x07; > + break; > + case (2): + case 2: > + newval = 0x00; > + minoff = 1; > + break; > + case (3): + case 3: > + newval = 0x00; > + minoff = 0; > + break; > + case (255): + case 255: > + newval = 0x03; > + break; > + default: > + return -EINVAL; > + } > + > + config = newval & 0x07; > + altbit = (newval >> 3) & 0x01; > + > + mutex_lock(&data->update_lock); > + config = (config & param->mask[0]) << param->shift[0]; > + altbit = (altbit & param->mask[1]) << param->shift[1]; > + currval = read_byte(param->msb[0]); > + newval = config | (currval & ~(param->mask[0] << param->shift[0])); > + newval = altbit | (newval & ~(param->mask[1] << param->shift[1])); > + data->reg[param->msb[0]] = newval; > + write_byte(param->msb[0], newval); > + if (minoff < 255) { > + minoff = (minoff & param->mask[2]) << param->shift[2]; > + currval = read_byte(param->msb[2]); > + newval = > + minoff | (currval & ~(param->mask[2] << param->shift[2])); > + data->reg[param->msb[2]] = newval; > + write_byte(param->msb[2], newval); > + } > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static u32 asc7621_pwm_freq_map[] = { > + 10, 15, 23, 30, 38, 47, 62, 94, > + 23000, 24000, 25000, 26000, 27000, 28000, 29000, 30000 > +}; > + > +static ssize_t show_pwm_freq(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + <--- Remove above line for consistency. > + u8 regval = > + (data->reg[param->msb[0]] >> param->shift[0]) & param->mask[0]; And give one extra line here : + > + regval = SENSORS_LIMIT(regval, 0, 15); > + > + return sprintf(buf, "%u\n", asc7621_pwm_freq_map[regval]); > +} > + > +static ssize_t store_pwm_freq(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + Remove above line. > + unsigned long reqval; > + u8 currval = 0; > + u8 newval = 255; + u8 currval, newval = 255; > + int i; > + > + if (strict_strtoul(buf, 10, &reqval)) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(asc7621_pwm_freq_map); i++) { > + if (reqval == asc7621_pwm_freq_map[i]) { > + newval = i; > + break; > + } > + } > + if (newval == 255) > + return -EINVAL; > + > + newval = (newval & param->mask[0]) << param->shift[0]; > + > + mutex_lock(&data->update_lock); > + currval = read_byte(param->msb[0]); > + newval |= (currval & ~(param->mask[0] << param->shift[0])); > + data->reg[param->msb[0]] = newval; > + write_byte(param->msb[0], newval); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static u32 asc7621_pwm_auto_spinup_map[] = { > + 0, 100, 250, 400, 700, 1000, 2000, 4000 }; This gives error by checkpatch : trailing whitespace +static u32 asc7621_pwm_auto_spinup_map[] = { + 0, 100, 250, 400, 700, 1000, 2000, 4000 +}; > + > +static ssize_t show_pwm_ast(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + Remove above line. > + u8 regval = > + (data->reg[param->msb[0]] >> param->shift[0]) & param->mask[0]; > + > + regval = SENSORS_LIMIT(regval, 0, 7); > + > + return sprintf(buf, "%u\n", asc7621_pwm_auto_spinup_map[regval]); > + > +} > + > +static ssize_t store_pwm_ast(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + u8 currval = 0; > + u8 newval = 255; + u8 currval, newval = 255; > + u32 i; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(asc7621_pwm_auto_spinup_map); i++) { > + if (reqval == asc7621_pwm_auto_spinup_map[i]) { > + newval = i; > + break; > + } > + } > + if (newval == 255) > + return -EINVAL; > + > + newval = (newval & param->mask[0]) << param->shift[0]; > + > + mutex_lock(&data->update_lock); > + currval = read_byte(param->msb[0]); > + newval |= (currval & ~(param->mask[0] << param->shift[0])); > + data->reg[param->msb[0]] = newval; > + write_byte(param->msb[0], newval); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static u32 asc7621_temp_smoothing_time_map[] = { > + 35000, 17600, 11800, 7000, 4400, 3000, 1600, 800 }; This gives error by checkpatch : trailing whitespace +static u32 asc7621_temp_smoothing_time_map[] = { + 35000, 17600, 11800, 7000, 4400, 3000, 1600, 800 +}; > + > +static ssize_t show_temp_st(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + SETUP_SHOW_data_param(dev, attr); > + Remove above line. > + u8 regval = > + (data->reg[param->msb[0]] >> param->shift[0]) & param->mask[0]; Give extra line here after variables declaration. > + regval = SENSORS_LIMIT(regval, 0, 7); > + > + return sprintf(buf, "%u\n", asc7621_temp_smoothing_time_map[regval]); > +} > + > +static ssize_t store_temp_st(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + SETUP_STORE_data_param(dev, attr); > + long reqval; > + u8 currval; > + u8 newval = 255; Save one line : + u8 currval, newval = 255; > + u32 i; > + > + if (strict_strtol(buf, 10, &reqval)) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(asc7621_pwm_auto_spinup_map); i++) { > + if (reqval == asc7621_temp_smoothing_time_map[i]) { > + newval = i; > + break; > + } > + } > + > + if (newval == 255) > + return -EINVAL; > + > + newval = (newval & param->mask[0]) << param->shift[0]; > + > + mutex_lock(&data->update_lock); > + currval = read_byte(param->msb[0]); > + newval |= (currval & ~(param->mask[0] << param->shift[0])); > + data->reg[param->msb[0]] = newval; > + write_byte(param->msb[0], newval); > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +/* End of data handlers */ > + > +/* These defines do nothing more than make the table easier Replace above lines by : +/* + * End of data handlers + * + * These defines do nothing more than make the table easier > + * to read when wrapped at column 80. > + */ > + > +/* > + * Creates a variable length array inititalizer. > + * VAA(1,3,5,7) would produce {1,3,5,7} > + */ > +#define VAA(args...) {args} > + > +#define PREAD(name, n, pri, rm, rl, m, s, r) \ > + {.sda = SENSOR_ATTR(name, S_IRUGO, show_##r, NULL, n), \ > + .priority = pri, .msb[0] = rm, .lsb[0] = rl, .mask[0] = m, \ > + .shift[0] = s,} > + > +#define PWRITE(name, n, pri, rm, rl, m, s, r) \ > + {.sda = SENSOR_ATTR(name, S_IRUGO | S_IWUSR, show_##r, store_##r, n), \ > + .priority = pri, .msb[0] = rm, .lsb[0] = rl, .mask[0] = m, \ > + .shift[0] = s,} > + > +/* > + * PWRITEM assumes that the initializers for the .msb, .lsb, .mask and .shift > + * were created using the VAA macro. > + */ > +#define PWRITEM(name, n, pri, rm, rl, m, s, r) \ > + {.sda = SENSOR_ATTR(name, S_IRUGO | S_IWUSR, show_##r, store_##r, n), \ > + .priority = pri, .msb = rm, .lsb = rl, .mask = m, .shift = s,} > + > +static struct asc7621_param asc7621_params[] = { > + PREAD(in0_input, 0, PRI_HIGH, 0x20, 0x13, 0, 0, in10), > + PREAD(in1_input, 1, PRI_HIGH, 0x21, 0x18, 0, 0, in10), > + PREAD(in2_input, 2, PRI_HIGH, 0x22, 0x11, 0, 0, in10), > + PREAD(in3_input, 3, PRI_HIGH, 0x23, 0x12, 0, 0, in10), > + PREAD(in4_input, 4, PRI_HIGH, 0x24, 0x14, 0, 0, in10), > + > + PWRITE(in0_min, 0, PRI_LOW, 0x44, 0, 0, 0, in8), > + PWRITE(in1_min, 1, PRI_LOW, 0x46, 0, 0, 0, in8), > + PWRITE(in2_min, 2, PRI_LOW, 0x48, 0, 0, 0, in8), > + PWRITE(in3_min, 3, PRI_LOW, 0x4a, 0, 0, 0, in8), > + PWRITE(in4_min, 4, PRI_LOW, 0x4c, 0, 0, 0, in8), > + > + PWRITE(in0_max, 0, PRI_LOW, 0x45, 0, 0, 0, in8), > + PWRITE(in1_max, 1, PRI_LOW, 0x47, 0, 0, 0, in8), > + PWRITE(in2_max, 2, PRI_LOW, 0x49, 0, 0, 0, in8), > + PWRITE(in3_max, 3, PRI_LOW, 0x4b, 0, 0, 0, in8), > + PWRITE(in4_max, 4, PRI_LOW, 0x4d, 0, 0, 0, in8), > + > + PREAD(in0_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 0, bitmask), > + PREAD(in1_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 1, bitmask), > + PREAD(in2_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 2, bitmask), > + PREAD(in3_alarm, 3, PRI_LOW, 0x41, 0, 0x01, 3, bitmask), > + PREAD(in4_alarm, 4, PRI_LOW, 0x42, 0, 0x01, 0, bitmask), > + > + PREAD(fan1_input, 0, PRI_HIGH, 0x29, 0x28, 0, 0, fan16), > + PREAD(fan2_input, 1, PRI_HIGH, 0x2b, 0x2a, 0, 0, fan16), > + PREAD(fan3_input, 2, PRI_HIGH, 0x2d, 0x2c, 0, 0, fan16), > + PREAD(fan4_input, 3, PRI_HIGH, 0x2f, 0x2e, 0, 0, fan16), > + > + PWRITE(fan1_min, 0, PRI_LOW, 0x55, 0x54, 0, 0, fan16), > + PWRITE(fan2_min, 1, PRI_LOW, 0x57, 0x56, 0, 0, fan16), > + PWRITE(fan3_min, 2, PRI_LOW, 0x59, 0x58, 0, 0, fan16), > + PWRITE(fan4_min, 3, PRI_LOW, 0x5b, 0x5a, 0, 0, fan16), > + > + PREAD(fan1_alarm, 0, PRI_LOW, 0x42, 0, 0x01, 0, bitmask), > + PREAD(fan2_alarm, 1, PRI_LOW, 0x42, 0, 0x01, 1, bitmask), > + PREAD(fan3_alarm, 2, PRI_LOW, 0x42, 0, 0x01, 2, bitmask), > + PREAD(fan4_alarm, 3, PRI_LOW, 0x42, 0, 0x01, 3, bitmask), > + > + PREAD(temp1_input, 0, PRI_HIGH, 0x25, 0x10, 0, 0, temp10), > + PREAD(temp2_input, 1, PRI_HIGH, 0x26, 0x15, 0, 0, temp10), > + PREAD(temp3_input, 2, PRI_HIGH, 0x27, 0x16, 0, 0, temp10), > + PREAD(temp4_input, 3, PRI_HIGH, 0x33, 0x17, 0, 0, temp10), > + PREAD(temp5_input, 4, PRI_HIGH, 0xf7, 0xf6, 0, 0, temp10), > + PREAD(temp6_input, 5, PRI_HIGH, 0xf9, 0xf8, 0, 0, temp10), > + PREAD(temp7_input, 6, PRI_HIGH, 0xfb, 0xfa, 0, 0, temp10), > + PREAD(temp8_input, 7, PRI_HIGH, 0xfd, 0xfc, 0, 0, temp10), > + > + PWRITE(temp1_min, 0, PRI_LOW, 0x4e, 0, 0, 0, temp8), > + PWRITE(temp2_min, 1, PRI_LOW, 0x50, 0, 0, 0, temp8), > + PWRITE(temp3_min, 2, PRI_LOW, 0x52, 0, 0, 0, temp8), > + PWRITE(temp4_min, 3, PRI_LOW, 0x34, 0, 0, 0, temp8), > + > + PWRITE(temp1_max, 0, PRI_LOW, 0x4f, 0, 0, 0, temp8), > + PWRITE(temp2_max, 1, PRI_LOW, 0x51, 0, 0, 0, temp8), > + PWRITE(temp3_max, 2, PRI_LOW, 0x53, 0, 0, 0, temp8), > + PWRITE(temp4_max, 3, PRI_LOW, 0x35, 0, 0, 0, temp8), > + > + PREAD(temp1_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 4, bitmask), > + PREAD(temp2_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 5, bitmask), > + PREAD(temp3_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 6, bitmask), > + PREAD(temp4_alarm, 3, PRI_LOW, 0x43, 0, 0x01, 0, bitmask), > + > + PWRITE(temp1_source, 0, PRI_LOW, 0x02, 0, 0x07, 4, bitmask), > + PWRITE(temp2_source, 1, PRI_LOW, 0x02, 0, 0x07, 0, bitmask), > + PWRITE(temp3_source, 2, PRI_LOW, 0x03, 0, 0x07, 4, bitmask), > + PWRITE(temp4_source, 3, PRI_LOW, 0x03, 0, 0x07, 0, bitmask), > + > + PWRITE(temp1_smoothing_enable, 0, PRI_LOW, 0x62, 0, 0x01, 3, bitmask), > + PWRITE(temp2_smoothing_enable, 1, PRI_LOW, 0x63, 0, 0x01, 7, bitmask), > + PWRITE(temp3_smoothing_enable, 2, PRI_LOW, 0x64, 0, 0x01, 3, bitmask), > + PWRITE(temp4_smoothing_enable, 3, PRI_LOW, 0x3c, 0, 0x01, 3, bitmask), > + > + PWRITE(temp1_smoothing_time, 0, PRI_LOW, 0x62, 0, 0x07, 0, temp_st), > + PWRITE(temp2_smoothing_time, 1, PRI_LOW, 0x63, 0, 0x07, 4, temp_st), > + PWRITE(temp3_smoothing_time, 2, PRI_LOW, 0x63, 0, 0x07, 0, temp_st), > + PWRITE(temp4_smoothing_time, 3, PRI_LOW, 0x3c, 0, 0x07, 0, temp_st), > + > + PWRITE(temp1_auto_point1_temp_hyst, 0, PRI_LOW, 0x6d, 0, 0x0f, 4, > + bitmask), > + PWRITE(temp2_auto_point1_temp_hyst, 1, PRI_LOW, 0x6d, 0, 0x0f, 0, > + bitmask), > + PWRITE(temp3_auto_point1_temp_hyst, 2, PRI_LOW, 0x6e, 0, 0x0f, 4, > + bitmask), > + PWRITE(temp4_auto_point1_temp_hyst, 3, PRI_LOW, 0x6e, 0, 0x0f, 0, > + bitmask), > + > + PREAD(temp1_auto_point2_temp_hyst, 0, PRI_LOW, 0x6d, 0, 0x0f, 4, > + bitmask), > + PREAD(temp2_auto_point2_temp_hyst, 1, PRI_LOW, 0x6d, 0, 0x0f, 0, > + bitmask), > + PREAD(temp3_auto_point2_temp_hyst, 2, PRI_LOW, 0x6e, 0, 0x0f, 4, > + bitmask), > + PREAD(temp4_auto_point2_temp_hyst, 3, PRI_LOW, 0x6e, 0, 0x0f, 0, > + bitmask), > + > + PWRITE(temp1_auto_point1_temp, 0, PRI_LOW, 0x67, 0, 0, 0, temp8), > + PWRITE(temp2_auto_point1_temp, 1, PRI_LOW, 0x68, 0, 0, 0, temp8), > + PWRITE(temp3_auto_point1_temp, 2, PRI_LOW, 0x69, 0, 0, 0, temp8), > + PWRITE(temp4_auto_point1_temp, 3, PRI_LOW, 0x3b, 0, 0, 0, temp8), > + > + PWRITEM(temp1_auto_point2_temp, 0, PRI_LOW, VAA(0x5f, 0x67), VAA(0), > + VAA(0x0f), VAA(4), ap2_temp), > + PWRITEM(temp2_auto_point2_temp, 1, PRI_LOW, VAA(0x60, 0x68), VAA(0), > + VAA(0x0f), VAA(4), ap2_temp), > + PWRITEM(temp3_auto_point2_temp, 2, PRI_LOW, VAA(0x61, 0x69), VAA(0), > + VAA(0x0f), VAA(4), ap2_temp), > + PWRITEM(temp4_auto_point2_temp, 3, PRI_LOW, VAA(0x3c, 0x3b), VAA(0), > + VAA(0x0f), VAA(4), ap2_temp), > + > + PWRITE(temp1_crit, 0, PRI_LOW, 0x6a, 0, 0, 0, temp8), > + PWRITE(temp2_crit, 1, PRI_LOW, 0x6b, 0, 0, 0, temp8), > + PWRITE(temp3_crit, 2, PRI_LOW, 0x6c, 0, 0, 0, temp8), > + PWRITE(temp4_crit, 3, PRI_LOW, 0x3d, 0, 0, 0, temp8), > + > + PWRITE(temp5_enable, 4, PRI_LOW, 0x0e, 0, 0x01, 0, bitmask), > + PWRITE(temp6_enable, 5, PRI_LOW, 0x0e, 0, 0x01, 1, bitmask), > + PWRITE(temp7_enable, 6, PRI_LOW, 0x0e, 0, 0x01, 2, bitmask), > + PWRITE(temp8_enable, 7, PRI_LOW, 0x0e, 0, 0x01, 3, bitmask), > + > + PWRITE(remote1_offset, 0, PRI_LOW, 0x1c, 0, 0, 0, temp62), > + PWRITE(remote2_offset, 1, PRI_LOW, 0x1d, 0, 0, 0, temp62), > + > + PWRITE(pwm1, 0, PRI_HIGH, 0x30, 0, 0, 0, u8), > + PWRITE(pwm2, 1, PRI_HIGH, 0x31, 0, 0, 0, u8), > + PWRITE(pwm3, 2, PRI_HIGH, 0x32, 0, 0, 0, u8), > + > + PWRITE(pwm1_invert, 0, PRI_LOW, 0x5c, 0, 0x01, 4, bitmask), > + PWRITE(pwm2_invert, 1, PRI_LOW, 0x5d, 0, 0x01, 4, bitmask), > + PWRITE(pwm3_invert, 2, PRI_LOW, 0x5e, 0, 0x01, 4, bitmask), > + > + PWRITEM(pwm1_enable, 0, PRI_LOW, VAA(0x5c, 0x5c, 0x62), VAA(0, 0, 0), > + VAA(0x07, 0x01, 0x01), VAA(5, 3, 5), pwm_enable), > + PWRITEM(pwm2_enable, 1, PRI_LOW, VAA(0x5d, 0x5d, 0x62), VAA(0, 0, 0), > + VAA(0x07, 0x01, 0x01), VAA(5, 3, 6), pwm_enable), > + PWRITEM(pwm3_enable, 2, PRI_LOW, VAA(0x5e, 0x5e, 0x62), VAA(0, 0, 0), > + VAA(0x07, 0x01, 0x01), VAA(5, 3, 7), pwm_enable), > + > + PWRITEM(pwm1_auto_channels, 0, PRI_LOW, VAA(0x5c, 0x5c), VAA(0, 0), > + VAA(0x07, 0x01), VAA(5, 3), pwm_ac), > + PWRITEM(pwm2_auto_channels, 1, PRI_LOW, VAA(0x5d, 0x5d), VAA(0, 0), > + VAA(0x07, 0x01), VAA(5, 3), pwm_ac), > + PWRITEM(pwm3_auto_channels, 2, PRI_LOW, VAA(0x5e, 0x5e), VAA(0, 0), > + VAA(0x07, 0x01), VAA(5, 3), pwm_ac), > + > + PWRITE(pwm1_auto_point1_pwm, 0, PRI_LOW, 0x64, 0, 0, 0, u8), > + PWRITE(pwm2_auto_point1_pwm, 1, PRI_LOW, 0x65, 0, 0, 0, u8), > + PWRITE(pwm3_auto_point1_pwm, 2, PRI_LOW, 0x66, 0, 0, 0, u8), > + > + PWRITE(pwm1_auto_point2_pwm, 0, PRI_LOW, 0x38, 0, 0, 0, u8), > + PWRITE(pwm2_auto_point2_pwm, 1, PRI_LOW, 0x39, 0, 0, 0, u8), > + PWRITE(pwm3_auto_point2_pwm, 2, PRI_LOW, 0x3a, 0, 0, 0, u8), > + > + PWRITE(pwm1_freq, 0, PRI_LOW, 0x5f, 0, 0x0f, 0, pwm_freq), > + PWRITE(pwm2_freq, 1, PRI_LOW, 0x60, 0, 0x0f, 0, pwm_freq), > + PWRITE(pwm3_freq, 2, PRI_LOW, 0x61, 0, 0x0f, 0, pwm_freq), > + > + PREAD(pwm1_auto_zone_assigned, 0, PRI_LOW, 0, 0, 0x03, 2, bitmask), > + PREAD(pwm2_auto_zone_assigned, 1, PRI_LOW, 0, 0, 0x03, 4, bitmask), > + PREAD(pwm3_auto_zone_assigned, 2, PRI_LOW, 0, 0, 0x03, 6, bitmask), > + > + PWRITE(pwm1_auto_spinup_time, 0, PRI_LOW, 0x5c, 0, 0x07, 0, pwm_ast), > + PWRITE(pwm2_auto_spinup_time, 1, PRI_LOW, 0x5d, 0, 0x07, 0, pwm_ast), > + PWRITE(pwm3_auto_spinup_time, 2, PRI_LOW, 0x5e, 0, 0x07, 0, pwm_ast), > + > + PWRITE(peci_enable, 0, PRI_LOW, 0x40, 0, 0x01, 4, bitmask), > + PWRITE(peci_avg, 0, PRI_LOW, 0x36, 0, 0x07, 0, bitmask), > + PWRITE(peci_domain, 0, PRI_LOW, 0x36, 0, 0x01, 3, bitmask), > + PWRITE(peci_legacy, 0, PRI_LOW, 0x36, 0, 0x01, 4, bitmask), > + PWRITE(peci_diode, 0, PRI_LOW, 0x0e, 0, 0x07, 4, bitmask), > + PWRITE(peci_4domain, 0, PRI_LOW, 0x0e, 0, 0x01, 4, bitmask), > + > +}; > + > +static struct asc7621_data *asc7621_update_device(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct asc7621_data *data = i2c_get_clientdata(client); > + int i; > + > +/* > + * The asc7621 chips guarantee consistent reads of multi-byte values > + * regardless of the order of the reads. No special logic is needed > + * so we can just read the registers in whatever order they appear > + * in the asc7621_params array. > + */ > + > + mutex_lock(&data->update_lock); > + > + /* Read all the high priority registers */ > + > + if (!data->valid || > + time_after(jiffies, data->last_high_reading + INTERVAL_HIGH)) { > + > + for (i = 0; i < ARRAY_SIZE(asc7621_register_priorities); i++) { > + if (asc7621_register_priorities[i] == PRI_HIGH) { > + data->reg[i] = > + i2c_smbus_read_byte_data(client, i) & 0xff; > + } > + } > + data->last_high_reading = jiffies; > + }; /* last_reading */ > + > + /* Read all the low priority registers. */ > + > + if (!data->valid || > + time_after(jiffies, data->last_high_reading + INTERVAL_LOW)) { > + > + for (i = 0; i < ARRAY_SIZE(asc7621_params); i++) { > + if (asc7621_register_priorities[i] == PRI_LOW) { > + data->reg[i] = > + i2c_smbus_read_byte_data(client, i) & 0xff; > + } > + } > + data->last_low_reading = jiffies; > + }; /* last_reading */ > + > + data->valid = 1; > + > + mutex_unlock(&data->update_lock); > + > + return data; > +} > + > +/* Standard detection and initialization below */ > + > +/* Helper function that checks if an address is valid > + * for a particular chip. Replace above lines with : +/* + * Standard detection and initialization below + * + * Helper function that checks if an address is valid for a particular chip. > + */ > + > +static inline int valid_address_for_chip(int chip_type, int address) > +{ > + int i = 0; + int i; + > + for (i = 0; asc7621_chips[chip_type].addresses[i] != I2C_CLIENT_END; > + i++) { > + if (asc7621_chips[chip_type].addresses[i] == address) > + return 1; > + } > + return 0; > +} > + > +static void asc7621_init_client(struct i2c_client *client) > +{ > + int value, i, j; > + > + /* Warn if part was not "READY" */ > + > + value = i2c_smbus_read_byte_data(client, 0x40) & 0xff; > + Replace above 3 lines with : + value = read_byte(0x40); > + if (value & 0x02) { > + dev_err(&client->dev, > + "Client (%d,0x%02x) config is locked.\n", > + i2c_adapter_id(client->adapter), client->addr); > + }; > + if (!(value & 0x04)) { > + dev_err(&client->dev, "Client (%d,0x%02x) is not ready.\n", > + i2c_adapter_id(client->adapter), client->addr); > + }; > + > + /* Start monitoring */ > + > + value = i2c_smbus_read_byte_data(client, 0x40) & 0xff; Do we really need to read value here again, we already read value above and it is unchanged. > + /* Try to clear LOCK, Set START, save everything else */ > + value = (value & ~0x02) | 0x01; > + write_byte(0x40, value & 0xff); > + > + /* > + * Collect all the registers needed into a single array. > + * This way, if a register isn't actually used for anything, > + * we don't retrieve it. > + */ > + > + for (i = 0; i < ARRAY_SIZE(asc7621_params); i++) { > + for (j = 0; j < ARRAY_SIZE(asc7621_params[i].msb); j++) > + asc7621_register_priorities[asc7621_params[i].msb[j]] = > + asc7621_params[i].priority; > + for (j = 0; j < ARRAY_SIZE(asc7621_params[i].lsb); j++) > + asc7621_register_priorities[asc7621_params[i].lsb[j]] = > + asc7621_params[i].priority; > + } > +} > + > +static struct i2c_driver asc7621_driver; Remove this declaration, we do not need it. > + > +static int > +asc7621_probe(struct i2c_client *client, const struct i2c_device_id *id) > +{ > + struct asc7621_data *data; > + int i; > + int err; Save one line : + int i, err; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA)) > + return -EIO; > + > + data = kzalloc(sizeof(struct asc7621_data), GFP_KERNEL); > + if (data == 0) Sparse gives the warning : Using plain integer as NULL pointer + if (data == NULL) > + return -ENOMEM; > + > + i2c_set_clientdata(client, data); > + data->valid = 0; > + mutex_init(&data->update_lock); > + > + /* Initialize the asc7621 chip */ > + asc7621_init_client(client); > + > + /* Create the sysfs entries */ > + for (i = 0; i < ARRAY_SIZE(asc7621_params); i++) { > + err = > + device_create_file(&client->dev, > + &(asc7621_params[i].sda.dev_attr)); > + if (err) > + goto exit_remove; > + } > + > + data->class_dev = hwmon_device_register(&client->dev); > + if (IS_ERR(data->class_dev)) { > + err = PTR_ERR(data->class_dev); > + goto exit_remove; > + } > + > + return 0; > + > +exit_remove: > + for (i = 0; i < ARRAY_SIZE(asc7621_params); i++) { > + device_remove_file(&client->dev, > + &(asc7621_params[i].sda.dev_attr)); > + } > + > + i2c_set_clientdata(client, NULL); > + kfree(data); > + return err; > +} > + > +static int asc7621_detect(struct i2c_client *client, > + struct i2c_board_info *info) > +{ > + struct i2c_adapter *adapter = client->adapter; > + int company, verstep; > + struct device *dev; > + int chip_index = 0; Keep struct together and int together like this : + int company, verstep, chip_index; > + > + dev = &client->dev; > + > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) > + return -ENODEV; > + > + for (chip_index = FIRST_CHIP; chip_index <= LAST_CHIP; chip_index++) { > + > + if (!valid_address_for_chip(chip_index, client->addr)) > + continue; > + > + company = i2c_smbus_read_byte_data(client, > + asc7621_chips > + [chip_index].company_reg) & > + 0xff; > + verstep = > + i2c_smbus_read_byte_data(client, > + asc7621_chips > + [chip_index].verstep_reg) & 0xff; Replace above 8 lines with : + company = read_byte(asc7621_chips[chip_index].company_reg); + verstep = read_byte(asc7621_chips[chip_index].verstep_reg); > + > + if (company == asc7621_chips[chip_index].company_id && > + verstep == asc7621_chips[chip_index].verstep_id) { > + strlcpy(client->name, asc7621_chips[chip_index].name, > + I2C_NAME_SIZE); > + strlcpy(info->type, asc7621_chips[chip_index].name, > + I2C_NAME_SIZE); > + > + dev_info(&adapter->dev, " Matched %s\n", Please fix this as this seems funny as output : + dev_info(&adapter->dev, "Matched %s\n", By replacing it, this will looks like : [ 2005.917005] i2c i2c-0: Matched asc7621a [ 2218.577843] i2c i2c-0: Matched asc7621a > + asc7621_chips[chip_index].name); > + return 0; > + } > + } > + > + return -ENODEV; > +} > + > +static int asc7621_remove(struct i2c_client *client) > +{ > + struct asc7621_data *data = i2c_get_clientdata(client); > + int i; > + > + hwmon_device_unregister(data->class_dev); > + > + for (i = 0; i < ARRAY_SIZE(asc7621_params); i++) { > + device_remove_file(&client->dev, > + &(asc7621_params[i].sda.dev_attr)); > + } > + > + i2c_set_clientdata(client, NULL); > + kfree(data); > + return 0; > +} > + > +static const struct i2c_device_id asc7621_id[] = { > + {"asc7621", asc7621}, > + {"asc7621a", asc7621a}, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(i2c, asc7621_id); > + > +static struct i2c_driver asc7621_driver = { > + .class = I2C_CLASS_HWMON, > + .driver = { > + .name = "asc7621", > + }, Please align this : + .name = "asc7621", + }, > + .probe = asc7621_probe, > + .remove = asc7621_remove, > + .id_table = asc7621_id, > + .detect = asc7621_detect, > + .address_list = normal_i2c, > +}; > + > +static int __init sm_asc7621_init(void) > +{ > + return i2c_add_driver(&asc7621_driver); > +} > + > +static void __exit sm_asc7621_exit(void) > +{ > + i2c_del_driver(&asc7621_driver); > +} > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("George Joseph"); > +MODULE_DESCRIPTION("Andigilog aSC7621 and aSC7621a driver"); > + > +module_init(sm_asc7621_init); > +module_exit(sm_asc7621_exit); Thanks, -- Jaswinder Singh. _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
