[lm-sensors] [PATCH 1/2 RESEND] hwmon: new driver for SMSC DME1737

Jean Delvare khali at linux-fr.org
Sat May 12 23:45:39 CEST 2007


Hi Juerg,

On Mon, 7 May 2007 14:24:41 -0700, Juerg Haefliger wrote:
> Incorporated changes requested by Jean (2nd review).
> 
> ---
> This patch adds support for the hardware monitoring and fan control
> capabilities of the SMSC DME1737 and Asus A8000 Super-I/O chips.
> 
> The hardware monitoring logic of this chip is similar to the LM85 but
> has some additional features that this driver supports. Even though
> it's a Super-I/O chip, the hardware monitoring logic can only be
> accessed via SMBus.
> 
> Signed-off-by: Juerg Haefliger <juergh at gmail.com>

> diff -uprN -X linux-2.6.21-mm1/Documentation/dontdiff -x MAINTAINERS -x Documentation linux-2.6.21-mm1.orig/drivers/hwmon/dme1737.c linux-2.6.21-mm1/drivers/hwmon/dme1737.c
> --- linux-2.6.21-mm1.orig/drivers/hwmon/dme1737.c	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.21-mm1/drivers/hwmon/dme1737.c	2007-05-07 13:56:14.000000000 -0700
> @@ -0,0 +1,2023 @@
> +/*
> + * dme1737.c - driver for the SMSC DME1737 and Asus A8000 Super-I/O chips
> + *             integrated hardware monitoring features.
> + * Copyright (c) 2007 Juerg Haefliger <juergh at gmail.com>
> + *
> + * This driver is based on the LM85 driver. The hardware monitoring
> + * capabilities of the DME1737 are very similar to the LM85 with some
> + * additional features. Even though the DME1737 is a Super-I/O chip, the
> + * hardware monitoring registers are only accessible via SMBus.
> + *
> + * 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/hwmon-vid.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <asm/io.h>
> +
> +/* Module load parameters */
> +static int force_start;
> +module_param(force_start, bool, 0);
> +MODULE_PARM_DESC(force_start, "Force the chip to start monitoring inputs");
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = {0x2c, 0x2d, 0x2e, I2C_CLIENT_END};
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_1(dme1737);
> +
> +/* ---------------------------------------------------------------------
> + * Registers
> + *
> + * The sensors are defined as follows:
> + *
> + * Voltages                          Temperatures
> + * --------                          ------------
> + * in0   +5VTR (+5V stdby)           temp1   Remote diode 1
> + * in1   Vccp  (proc core)           temp2   Internal temp
> + * in2   VCC   (internal +3.3V)      temp3   Remote diode 2
> + * in3   +5V
> + * in4   +12V
> + * in5   VTR   (+3.3V stby)
> + * in6   Vbat
> + *
> + * --------------------------------------------------------------------- */
> +
> +/* Voltages (in) numbered 0-6 (ix) */
> +#define	DME1737_REG_IN(ix)		((ix) < 5 ? 0x20 + (ix) \
> +						  : 0x94 + (ix))
> +#define	DME1737_REG_IN_MIN(ix)		((ix) < 5 ? 0x44 + (ix) * 2 \
> +						  : 0x91 + (ix) * 2)
> +#define	DME1737_REG_IN_MAX(ix)		((ix) < 5 ? 0x45 + (ix) * 2 \
> +						  : 0x92 + (ix) * 2)
> +
> +/* Temperatures (temp) numbered 0-2 (ix) */
> +#define DME1737_REG_TEMP(ix)		(0x25 + (ix))
> +#define DME1737_REG_TEMP_MIN(ix)	(0x4e + (ix) * 2)
> +#define DME1737_REG_TEMP_MAX(ix)	(0x4f + (ix) * 2)
> +#define DME1737_REG_TEMP_OFFSET(ix)	((ix) == 0 ? 0x1f \
> +						   : 0x1c + (ix))
> +
> +/* Voltage and temperature LSBs
> + * The LSBs (4 bits each) are stored in 5 registers with the following layouts:
> + *    IN_TEMP_LSB(0) = [in5, in6]
> + *    IN_TEMP_LSB(1) = [temp3, temp1]
> + *    IN_TEMP_LSB(2) = [in4, temp2]
> + *    IN_TEMP_LSB(3) = [in3, in0]
> + *    IN_TEMP_LSB(4) = [in2, in1] */
> +#define DME1737_REG_IN_TEMP_LSB(ix)	(0x84 + (ix))
> +static const u8 DME1737_REG_IN_LSB[] = {3, 4, 4, 3, 2, 0, 0};
> +static const u8 DME1737_REG_IN_LSB_SHL[] = {4, 4, 0, 0, 0, 0, 4};
> +static const u8 DME1737_REG_TEMP_LSB[] = {1, 2, 1};
> +static const u8 DME1737_REG_TEMP_LSB_SHL[] = {4, 4, 0};
> +
> +/* Fans numbered 0-5 (ix) */
> +#define DME1737_REG_FAN(ix)		((ix) < 4 ? 0x28 + (ix) * 2 \
> +						  : 0xa1 + (ix) * 2)
> +#define DME1737_REG_FAN_MIN(ix)		((ix) < 4 ? 0x54 + (ix) * 2 \
> +						  : 0xa5 + (ix) * 2)
> +#define DME1737_REG_FAN_OPT(ix)		((ix) < 4 ? 0x90 + (ix) \
> +						  : 0xb2 + (ix))
> +#define DME1737_REG_FAN_MAX(ix)		(0xb4 + (ix)) /* only for fan[4-5] */
> +
> +/* PWMs numbered 0-2, 4-5 (ix) */
> +#define DME1737_REG_PWM(ix)		((ix) < 3 ? 0x30 + (ix) \
> +						  : 0xa1 + (ix))
> +#define DME1737_REG_PWM_CONFIG(ix)	(0x5c + (ix)) /* only for pwm[0-2] */
> +#define DME1737_REG_PWM_MIN(ix)		(0x64 + (ix)) /* only for pwm[0-2] */
> +#define DME1737_REG_PWM_FREQ(ix)	((ix) < 3 ? 0x5f + (ix) \
> +						  : 0xa3 + (ix))
> +/* The layout of the ramp rate registers is different from the other pwm
> + * registers. The bits for the 3 PWMs are stored in 2 registers:
> + *    PWM_RR(0) = [OFF3, OFF2,  OFF1,  RES,   RR1E, RR1-2, RR1-1, RR1-0]
> + *    PWM_RR(0) = [RR2E, RR2-2, RR2-1, RR2-0, RR3E, RR3-2, RR3-1, RR3-0] */

PWM_RR(1)

> +#define DME1737_REG_PWM_RR(ix)		(0x62 + (ix)) /* only for pwm[0-2] */
> +
> +/* Thermal zones 0-2 */
> +#define DME1737_REG_ZONE_LOW(ix)	(0x67 + (ix))
> +#define DME1737_REG_ZONE_ABS(ix)	(0x6a + (ix))
> +/* The layout of the hysteresis registers is different from the other zone
> + * registers. The bits for the 3 zones are stored in 2 registers:
> + *    ZONE_HYST(0) = [H1-3,  H1-2,  H1-1, H1-0, H2-3, H2-2, H2-1, H2-0]
> + *    ZONE_HYST(1) = [H3-3,  H3-2,  H3-1, RES,  RES,  RES,  RES,  RES] */
> +#define DME1737_REG_ZONE_HYST(ix)	(0x6d + (ix))

No H3-0?

> +
> +/* Alarm registers and bit mapping
> + * The 3 8-bit alarm registers will be concatenated to a single 32-bit
> + * alarm value [0, ALARM3, ALARM2, ALARM1]. */
> +#define DME1737_REG_ALARM1		0x41
> +#define DME1737_REG_ALARM2		0x42
> +#define DME1737_REG_ALARM3		0x83
> +static const u8 DME1737_BIT_ALARM_IN[] = {0, 1, 2, 3, 8, 16, 17};
> +static const u8 DME1737_BIT_ALARM_TEMP[] = {4, 5, 6};
> +static const u8 DME1737_BIT_ALARM_FAN[] = {10, 11, 12, 13, 22, 23};
> +
> +/* Miscellaneous registers */
> +#define DME1737_REG_COMPANY		0x3e
> +#define DME1737_REG_VERSTEP		0x3f
> +#define DME1737_REG_CONFIG		0x40
> +#define DME1737_REG_CONFIG2		0x7f
> +#define DME1737_REG_VID			0x43
> +#define DME1737_REG_TACH_PWM		0x81
> +
> +/* ---------------------------------------------------------------------
> + * Misc defines
> + * --------------------------------------------------------------------- */
> +
> +/* Chip identification */
> +#define DME1737_COMPANY_SMSC	0x5c
> +#define DME1737_VERSTEP		0x88
> +#define DME1737_VERSTEP_MASK	0xf8
> +
> +/* Optional features (chip configuration dependent). These *must* match the
> + * order of the dme1737_opt_group[] and dme1737_opt_lock_group[] struct
> + * entries. */
> +#define DME1737_OPT_PWM3	0x01
> +#define DME1737_OPT_PWM5	0x02
> +#define DME1737_OPT_PWM6	0x04
> +#define DME1737_OPT_FAN3	0x08
> +#define DME1737_OPT_FAN4	0x10
> +#define DME1737_OPT_FAN5	0x20
> +#define DME1737_OPT_FAN6	0x40

May I suggest to write these (1 << 0), (1 << 1) etc...?

> +
> +/* ---------------------------------------------------------------------
> + * Data structures and manipulation thereof
> + * --------------------------------------------------------------------- */
> +
> +struct dme1737_data {
> +	struct i2c_client client;
> +	struct class_device *class_dev;
> +
> +	struct mutex update_lock;
> +	int valid;			/* !=0 if following fields are valid */
> +	unsigned long last_update;	/* in jiffies */
> +	unsigned long last_vbat;	/* in jiffies */
> +
> +	u8 opt_features;
> +	u8 vid;
> +	u8 pwm_rr_en;
> +
> +	/* Register values */
> +	u16 in[7];
> +	u8  in_min[7];
> +	u8  in_max[7];
> +	s16 temp[3];
> +	s8  temp_min[3];
> +	s8  temp_max[3];
> +	s8  temp_offset[3];
> +	u8  config;
> +	u8  config2;
> +	u8  vrm;
> +	u16 fan[6];
> +	u16 fan_min[6];
> +	u8  fan_max[2];
> +	u8  fan_opt[6];
> +	u8  pwm[6];
> +	u8  pwm_min[3];
> +	u8  pwm_config[3];
> +	u8  pwm_acz[3];
> +	u8  pwm_freq[6];
> +	u8  pwm_rr[2];
> +	u8  zone_low[3];
> +	u8  zone_abs[3];
> +	u8  zone_hyst[2];
> +	u32 alarms;
> +};
> +
> +/* Nominal voltage values */
> +static const int IN_NOMINAL[] = {5000, 2250, 3300, 5000, 12000, 3300, 3300};
> +
> +/* Voltage input
> + * Voltage inputs have 16 bits resolution, limit values have 8 bits
> + * resolution. */
> +static inline int IN_FROM_REG(int reg, int ix, int res)
> +{
> +	return (reg * IN_NOMINAL[ix] + (3 << (res - 3))) / (3 << (res - 2));
> +}
> +
> +static inline int IN_TO_REG(int val, int ix)
> +{
> +	return SENSORS_LIMIT((val * 192 + IN_NOMINAL[ix] / 2) /
> +			     IN_NOMINAL[ix], 0, 255);
> +}
> +
> +/* Temperature input 
> + * The register values represent temperatures in 2's complement notation from
> + * -127 degrees C to +127 degrees C. Temp inputs have 16 bits resolution, limit
> + * values have 8 bits resolution. */
> +static inline int TEMP_FROM_REG(int reg, int res)
> +{
> +	return (reg * 1000) >> (res - 8);
> +}
> +
> +static inline int TEMP_TO_REG(int val)
> +{
> +	return SENSORS_LIMIT((val + 500) / 1000, -128, 127);
> +}

This gives invalid rounding for negative temperatures.

> +
> +/* Temperature range */
> +static const int TEMP_RANGE[] = {2000, 2500, 3333, 4000, 5000, 6666, 8000,
> +				 10000, 13333, 16000, 20000, 26666, 32000,
> +				 40000, 53333, 80000};
> +
> +static inline int TEMP_RANGE_FROM_REG(int reg)
> +{
> +	return TEMP_RANGE[(reg >> 4) & 0x0f];
> +}
> +
> +static int TEMP_RANGE_TO_REG(int val, int reg)
> +{
> +	int i;
> +
> +	for (i = 15; i > 0; i--) {
> +		if (val > (TEMP_RANGE[i] + TEMP_RANGE[i - 1] + 1) / 2) {
> +			break;
> +		}
> +	}
> +
> +	return (reg & 0x0f) | (i << 4);
> +}
> +
> +/* Temperature hysteresis
> + * Register layout:
> + *    reg[0] = [H1-3, H1-2, H1-1, H1-0, H2-3, H2-2, H2-1, H2-0]
> + *    reg[1] = [H3-3, H3-2, H3-1, H3-0, xxxx, xxxx, xxxx, xxxx] */
> +static inline int TEMP_HYST_FROM_REG(int reg, int ix)
> +{
> +	return (((ix == 1) ? reg : reg >> 4) & 0x0f) * 1000;
> +}
> +
> +static inline int TEMP_HYST_TO_REG(int val, int ix, int reg)
> +{
> +	int hyst = SENSORS_LIMIT((val + 500) / 1000, 0, 15);
> +
> +	return (ix == 1) ? (reg & 0xf0) | hyst : (reg & 0x0f) | (hyst << 4); 
> +}
> +
> +/* Fan input RPM */
> +static inline int FAN_FROM_REG(int reg, int tpc)
> +{
> +	return (reg == 0 || reg == 0xffff) ? 0 :
> +		(tpc == 0) ? (90000 * 60 + reg / 2) / reg : tpc * reg;
> +}
> +
> +static inline int FAN_TO_REG(int val, int tpc)
> +{
> +	return SENSORS_LIMIT((tpc == 0) ? (90000 * 60 + val / 2) / val :
> +			     (val + tpc / 2) / tpc, 0, 0xffff);
> +}

I do not recommend rounding fan speed values. I don't think any driver
does it. Due to the way the measurements are done, it doesn't make much
sense.

> +
> +/* Fan TPC (tach pulse count)
> + * Converts a register value to a TPC multiplier or returns 0 if the tachometer
> + * is configured in legacy (non-tpc) mode */
> +static inline int FAN_TPC_FROM_REG(int reg)
> +{
> +	return (reg & 0x20) ? 0 : 60 >> (reg & 0x03);
> +}
> +
> +/* Fan type
> + * The type of a fan is expressed in number of pulses-per-revolution that it 
> + * emits */
> +static inline int FAN_TYPE_FROM_REG(int reg)
> +{
> +	int edge = (reg >> 1) & 0x03;
> +
> +	return (edge > 0) ? 1 << (edge - 1) : 0;
> +}
> +
> +static inline int FAN_TYPE_TO_REG(int val, int reg)
> +{
> +	int edge = (val == 4) ? 3 : val;
> +
> +	return (reg & 0xf9) | (edge << 1);
> +}
> +
> +/* Fan max RPM */
> +static const int FAN_MAX[11][2] = {{0x54, 1000}, {0x38, 1500}, {0x2a, 2000},
> +				   {0x21, 2500}, {0x1c, 3000}, {0x18, 3500},
> +				   {0x15, 4000}, {0x12, 4500}, {0x11, 5000},
> +				   {0x0f, 5500}, {0x0e, 6000}};
> +
> +static int FAN_MAX_FROM_REG(int reg)
> +{
> +	int i;
> +
> +	for (i = 10; i > 0; i--) {
> +		if (reg == FAN_MAX[i][1]) {
> +			break;
> +		}
> +	}
> +
> +	return FAN_MAX[i][2];

Array overrun!

Also, you don't really need a 2D array here. For every value of i,
FAN_MAX[i][1] == (1000 + i * 500).

> +}
> +
> +static int FAN_MAX_TO_REG(int val)
> +{
> +	int i;
> +
> +	for (i = 10; i > 0; i--) {
> +		if (val > FAN_MAX[i - 1][2]) {

Array overrun!

> +			break;
> +		}
> +	}
> +
> +	return FAN_MAX[i][1];
> +}
> +
> +/* PWM enable
> + * Register to enable mapping:
> + * 000:  2  fan on zone 1 auto
> + * 001:  2  fan on zone 2 auto
> + * 010:  2  fan on zone 3 auto
> + * 011:  0  fan full on
> + * 100: -1  fan disabled
> + * 101:  2  fan on hottest of zones 2,3 auto
> + * 110:  2  fan on hottest of zones 1,2,3 auto
> + * 111:  1  fan in manual mode */
> +static inline int PWM_EN_FROM_REG(int reg)
> +{
> +	static const int en[] = {2, 2, 2, 0, -1, 2, 2, 1};
> +
> +	return en[(reg >> 5) & 0x07];
> +}
> +
> +static inline int PWM_EN_TO_REG(int val, int reg)
> +{
> +	int en = (val == 1) ? 7 : 3;
> +
> +	return (reg & 0x1f) | ((en & 0x07) << 5);
> +}
> +
> +/* PWM auto channels zone
> + * Register to auto channels zone mapping (ACZ is a bitfield with bit x
> + * corresponding to zone x+1):
> + * 000: 001  fan on zone 1 auto
> + * 001: 010  fan on zone 2 auto
> + * 010: 100  fan on zone 3 auto
> + * 011: 000  fan full on
> + * 100: 000  fan disabled
> + * 101: 110  fan on hottest of zones 2,3 auto
> + * 110: 111  fan on hottest of zones 1,2,3 auto
> + * 111: 000  fan in manual mode */
> +static inline int PWM_ACZ_FROM_REG(int reg)
> +{
> +	static const int acz[] = {1, 2, 4, 0, 0, 6, 7, 0};
> +
> +	return acz[(reg >> 5) & 0x07];
> +}
> +
> +static inline int PWM_ACZ_TO_REG(int val, int reg)
> +{
> +	int acz = (val == 1) ? 0 : (val == 2) ? 1 : (val == 4) ? 2 : val - 1;

Can be simplified to:

	int acz = (val == 4) ? 2 : val - 1;

> +
> +	return (reg & 0x1f) | ((acz & 0x07) << 5);
> +}
> +
> +/* PWM frequency */
> +static const int PWM_FREQ[] = {11, 15, 22, 29, 35, 44, 59, 88,
> +			       15000, 20000, 30000, 25000, 0, 0, 0, 0};
> +
> +static inline int PWM_FREQ_FROM_REG(int reg)
> +{
> +	return PWM_FREQ[reg & 0x0f];
> +}
> +
> +static int PWM_FREQ_TO_REG(int val, int reg)
> +{
> +	int i;
> +
> +	/* the first two cases are special - stupid chip design! */
> +	if (val > 27500) {
> +		i = 10;
> +	} else if (val > 22500) {
> +		i = 11;
> +	} else {
> +		for (i = 9; i > 0; i--) {
> +			if (val > (PWM_FREQ[i] + PWM_FREQ[i - 1] + 1) / 2) {
> +				break;
> +			}
> +		}
> +	}
> +
> +	return (reg & 0xf0) | i;
> +}
> +
> +/* PWM ramp rate
> + * Register layout:
> + *    reg[0] = [OFF3,  OFF2,  OFF1,  RES,   RR1-E, RR1-2, RR1-1, RR1-0] 
> + *    reg[1] = [RR2-E, RR2-2, RR2-1, RR2-0, RR3-E, RR3-2, RR3-1, RR3-0] */
> +static const u8 PWM_RR[] = {206, 104, 69, 41, 26, 18, 10, 5};
> +
> +static inline int PWM_RR_FROM_REG(int reg, int ix)
> +{
> +	int rr = (ix == 1) ? reg >> 4 : reg;
> +
> +	return (rr & 0x08) ? PWM_RR[rr & 0x07] : 0;
> +}
> +
> +static int PWM_RR_TO_REG(int val, int ix, int reg)
> +{
> +	int i;
> +
> +	for (i = 0; i < 7; i++) {
> +		if (val > (PWM_RR[i] + PWM_RR[i + 1] + 1) / 2) {
> +			break;
> +		}
> +	}
> +
> +	return (ix == 1) ? (reg & 0x8f) | (i << 4) : (reg & 0xf8) | i;
> +}
> +
> +/* PWM ramp rate enable */
> +static inline int PWM_RR_EN_FROM_REG(int reg, int ix)
> +{
> +	return PWM_RR_FROM_REG(reg, ix) ? 1 : 0;
> +}
> +
> +static inline int PWM_RR_EN_TO_REG(int val, int ix, int reg)
> +{
> +	int en = (ix == 1) ? 0x80 : 0x08;
> +
> +	return val ? reg | en : reg & ~en;
> +}
> +
> +/* PWM min/off
> + * The PWM min/off bits are part of the PMW ramp rate register 0 (see above for
> + * the register layout). */
> +static inline int PWM_OFF_FROM_REG(int reg, int ix)
> +{
> +	return (reg >> (ix + 5)) & 0x01;
> +}
> +
> +static inline int PWM_OFF_TO_REG(int val, int ix, int reg)
> +{
> +	return (reg & ~(1 << (ix + 5))) | ((val & 0x01) << (ix + 5));
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Device I/O access
> + * --------------------------------------------------------------------- */
> +
> +static u8 dme1737_read(struct i2c_client *client, u8 reg)
> +{
> +	s32 val = i2c_smbus_read_byte_data(client, reg);
> +
> +	if (val < 0) {
> +		dev_warn(&client->dev, "Read from register 0x%02x failed! "
> +			 "Please report to the driver maintainer.\n", reg);
> +	}
> +
> +	return val;
> +}
> +
> +static s32 dme1737_write(struct i2c_client *client, u8 reg, u8 value)
> +{
> +	s32 res = i2c_smbus_write_byte_data(client, reg, value);
> +
> +	if (res < 0) {
> +		dev_warn(&client->dev, "Write to register 0x%02x failed! "
> +			 "Please report to the driver maintainer.\n", reg);
> +	}
> +
> +	return res;
> +}
> +
> +static struct dme1737_data *dme1737_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	int ix;
> +	u8 lsb[5];
> +
> +	mutex_lock(&data->update_lock);
> +
> +	/* Enable a Vbat monitoring cycle every 10 mins */
> +	if (time_after(jiffies, data->last_vbat + 600 * HZ) || !data->valid) {
> +		dme1737_write(client, DME1737_REG_CONFIG, dme1737_read(client,
> +						DME1737_REG_CONFIG) | 0x10);
> +		data->last_vbat = jiffies;
> +	}
> +
> +	/* Sample register contents every 1 sec */
> +	if (time_after(jiffies, data->last_update + HZ) || !data->valid) {
> +		data->vid = dme1737_read(client, DME1737_REG_VID) & 0x3f;
> +
> +		/* In (voltage) registers */
> +		for (ix = 0; ix < ARRAY_SIZE(data->in); ix++) {
> +			/* Voltage inputs are stored as 16 bit values even
> +			 * though they have only 12 bits resolution. This is
> +			 * to make it consistent with the temp inputs. */
> +			data->in[ix] = dme1737_read(client,
> +					DME1737_REG_IN(ix)) << 8;
> +			data->in_min[ix] = dme1737_read(client,
> +					DME1737_REG_IN_MIN(ix));
> +			data->in_max[ix] = dme1737_read(client,
> +					DME1737_REG_IN_MAX(ix));
> +		}
> +
> +		/* Temp registers */
> +		for (ix = 0; ix < ARRAY_SIZE(data->temp); ix++) {
> +			/* Temp inputs are stored as 16 bit values even
> +			 * though they have only 12 bits resolution. This is
> +			 * to take advantage of implicit conversions between
> +			 * register values (2's complement) and temp values
> +			 * (signed decimal). */
> +			data->temp[ix] = dme1737_read(client,
> +					DME1737_REG_TEMP(ix)) << 8;
> +			data->temp_min[ix] = dme1737_read(client,
> +					DME1737_REG_TEMP_MIN(ix));
> +			data->temp_max[ix] = dme1737_read(client,
> +					DME1737_REG_TEMP_MAX(ix));
> +			data->temp_offset[ix] = dme1737_read(client,
> +					DME1737_REG_TEMP_OFFSET(ix));
> +		}
> +
> +		/* In and temp LSB registers
> +		 * The LSBs are latched when the MSBs are read, so the order in
> +		 * which the registers are read (MSB first, then LSB) is
> +		 * important! */
> +		for (ix = 0; ix < ARRAY_SIZE(lsb); ix++) {
> +			lsb[ix] = dme1737_read(client,
> +					DME1737_REG_IN_TEMP_LSB(ix));
> +		}
> +		for (ix = 0; ix < ARRAY_SIZE(data->in); ix++) {
> +			data->in[ix] |= (lsb[DME1737_REG_IN_LSB[ix]] << 
> +					DME1737_REG_IN_LSB_SHL[ix]) & 0xf0;
> +		}
> +		for (ix = 0; ix < ARRAY_SIZE(data->temp); ix++) {
> +			data->temp[ix] |= (lsb[DME1737_REG_TEMP_LSB[ix]] << 
> +					DME1737_REG_TEMP_LSB_SHL[ix]) & 0xf0;
> +		}
> +		
> +		/* Fan registers */
> +		for (ix = 0; ix < ARRAY_SIZE(data->fan); ix++) {
> +			/* Skip reading registers if optional fans are not
> +			 * present */
> +			if ((ix == 2 && !(data->opt_features &
> +					  DME1737_OPT_FAN3)) ||
> +			    (ix == 3 && !(data->opt_features &
> +					  DME1737_OPT_FAN4)) ||
> +			    (ix == 4 && !(data->opt_features &
> +					  DME1737_OPT_FAN5)) ||
> +			    (ix == 5 && !(data->opt_features &
> +					  DME1737_OPT_FAN6))) {
> +				continue;
> +			}

This works, but isn't very elegant. Other drivers do it differently:

* For each input type where inputs can be optionally disabled, have a
struct member: has_fan, has_temp... Size it as needed.
* Each bit in these members correspond to an input, inputs which are
always enabled are hard-coded to 1, others are computed based on the
chip configuration.
* In the update loop, you simply have to check for e.g.
  (has_fan & (1 << nr)) being set or not.

The f71805f driver does this for voltage inputs for example. You may
consider switching to this model.

> +			data->fan[ix] = dme1737_read(client,
> +					DME1737_REG_FAN(ix));
> +			data->fan[ix] |= dme1737_read(client,
> +					DME1737_REG_FAN(ix) + 1) << 8;
> +			data->fan_min[ix] = dme1737_read(client,
> +					DME1737_REG_FAN_MIN(ix));
> +			data->fan_min[ix] |= dme1737_read(client,
> +					DME1737_REG_FAN_MIN(ix) + 1) << 8;
> +			data->fan_opt[ix] = dme1737_read(client,
> +					DME1737_REG_FAN_OPT(ix));
> +			/* fan_max exists only for fan[5-6] */
> +			if (ix > 3) {
> +				data->fan_max[ix - 4] = dme1737_read(client,
> +					DME1737_REG_FAN_MAX(ix));
> +			}
> +		}
> +
> +		/* PWM registers */
> +		for (ix = 0; ix < ARRAY_SIZE(data->pwm); ix++) {
> +			/* Skip reading registers if optional PWMs are not
> +			 * present */
> +			if ((ix == 2 && !(data->opt_features &
> +					  DME1737_OPT_PWM3)) ||
> +			    (ix == 3) || /* pwm4 doesn't exist at all */
> +			    (ix == 4 && !(data->opt_features &
> +					  DME1737_OPT_PWM5)) ||
> +			    (ix == 5 && !(data->opt_features &
> +					  DME1737_OPT_PWM6))) {
> +				continue;
> +			}
> +			data->pwm[ix] = dme1737_read(client,
> +					DME1737_REG_PWM(ix));
> +			data->pwm_freq[ix] = dme1737_read(client,
> +					DME1737_REG_PWM_FREQ(ix));
> +			/* pwm_config and pwm_min exist only for pwm[1-3] */
> +			if (ix < 3) {
> +				data->pwm_config[ix] = dme1737_read(client,
> +						DME1737_REG_PWM_CONFIG(ix));
> +				data->pwm_min[ix] = dme1737_read(client,
> +						DME1737_REG_PWM_MIN(ix));
> +			}
> +		}
> +		for (ix = 0; ix < ARRAY_SIZE(data->pwm_rr); ix++) {
> +			data->pwm_rr[ix] = dme1737_read(client,
> +						DME1737_REG_PWM_RR(ix));
> +		}
> +
> +		/* Thermal zone registers */
> +		for (ix = 0; ix < ARRAY_SIZE(data->zone_low); ix++) {
> +			data->zone_low[ix] = dme1737_read(client,
> +					DME1737_REG_ZONE_LOW(ix));
> +			data->zone_abs[ix] = dme1737_read(client,
> +					DME1737_REG_ZONE_ABS(ix));
> +		}
> +		for (ix = 0; ix < ARRAY_SIZE(data->zone_hyst); ix++) {
> +			data->zone_hyst[ix] = dme1737_read(client,
> +						DME1737_REG_ZONE_HYST(ix));
> +		}
> +
> +		/* Alarm registers */
> +		data->alarms = dme1737_read(client,
> +						DME1737_REG_ALARM1);
> +		/* Bit 7 tells us if the other alarm registers are non-zero and
> +		 * therefore also need to be read */
> +		if (data->alarms & 0x80) {
> +			data->alarms |= dme1737_read(client,
> +						DME1737_REG_ALARM2) << 8;
> +			data->alarms |= dme1737_read(client,
> +						DME1737_REG_ALARM3) << 16;
> +		}
> +
> +		data->last_update = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Voltage sysfs attributes
> + * ix = [0-5]
> + * --------------------------------------------------------------------- */
> +
> +#define SYS_IN_INPUT	0
> +#define SYS_IN_MIN	1
> +#define SYS_IN_MAX	2
> +#define SYS_IN_ALARM	3
> +
> +static ssize_t show_in(struct device *dev, struct device_attribute *attr,
> +		       char *buf)
> +{
> +	struct dme1737_data *data = dme1737_update_device(dev);
> +	struct sensor_device_attribute_2
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	int res;
> +
> +	switch (fn) {
> +	case SYS_IN_INPUT:
> +		res = IN_FROM_REG(data->in[ix], ix, 16);
> +		break;
> +	case SYS_IN_MIN:
> +		res = IN_FROM_REG(data->in_min[ix], ix, 8);
> +		break;
> +	case SYS_IN_MAX:
> +		res = IN_FROM_REG(data->in_max[ix], ix, 8);
> +		break;
> +	case SYS_IN_ALARM:
> +		res = (data->alarms >>DME1737_BIT_ALARM_IN[ix]) & 0x01;

Missing space.

> +		break;
> +	default:
> +		res = 0;
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +
> +	return sprintf(buf, "%d\n", res);
> +}
> +
> +static ssize_t set_in(struct device *dev, struct device_attribute *attr,
> +		      const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute_2 
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	long val = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->update_lock);
> +	switch (fn) {
> +	case SYS_IN_MIN:
> +		data->in_min[ix] = IN_TO_REG(val, ix);
> +		dme1737_write(client, DME1737_REG_IN_MIN(ix),
> +			      data->in_min[ix]);
> +		break;
> +	case SYS_IN_MAX:
> +		data->in_max[ix] = IN_TO_REG(val, ix);
> +		dme1737_write(client, DME1737_REG_IN_MAX(ix),
> +			      data->in_max[ix]);
> +		break;
> +	default:
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Temperature sysfs attributes
> + * ix = [0-2]
> + * --------------------------------------------------------------------- */
> +
> +#define SYS_TEMP_INPUT			0
> +#define SYS_TEMP_MIN			1
> +#define SYS_TEMP_MAX			2
> +#define SYS_TEMP_OFFSET			3
> +#define SYS_TEMP_ALARM			4
> +#define SYS_TEMP_FAULT			5
> +
> +static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct dme1737_data *data = dme1737_update_device(dev);
> +	struct sensor_device_attribute_2
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	int res;
> +
> +	switch (fn) {
> +	case SYS_TEMP_INPUT:
> +		res = TEMP_FROM_REG(data->temp[ix], 16);
> +		break;
> +	case SYS_TEMP_MIN:
> +		res = TEMP_FROM_REG(data->temp_min[ix], 8);
> +		break;
> +	case SYS_TEMP_MAX:
> +		res = TEMP_FROM_REG(data->temp_max[ix], 8);
> +		break;
> +	case SYS_TEMP_OFFSET:
> +		res = TEMP_FROM_REG(data->temp_offset[ix], 8);
> +		break;
> +	case SYS_TEMP_ALARM:
> +		res = (data->alarms >> DME1737_BIT_ALARM_TEMP[ix]) & 0x01;
> +		break;
> +	case SYS_TEMP_FAULT:
> +		res = (data->temp[ix] == 0x0800);
> +		break;
> +	default:
> +		res = 0;
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +
> +	return sprintf(buf, "%d\n", res);
> +}
> +
> +static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute_2 
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	long val = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->update_lock);
> +	switch (fn) {
> +	case SYS_TEMP_MIN:
> +		data->temp_min[ix] = TEMP_TO_REG(val);
> +		dme1737_write(client, DME1737_REG_TEMP_MIN(ix),
> +			      data->temp_min[ix]);
> +		break;
> +	case SYS_TEMP_MAX:
> +		data->temp_max[ix] = TEMP_TO_REG(val);
> +		dme1737_write(client, DME1737_REG_TEMP_MAX(ix),
> +			      data->temp_max[ix]);
> +		break;
> +	case SYS_TEMP_OFFSET:
> +		data->temp_offset[ix] = TEMP_TO_REG(val);
> +		dme1737_write(client, DME1737_REG_TEMP_OFFSET(ix),
> +			      data->temp_offset[ix]);
> +		break;
> +	default:
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Zone sysfs attributes
> + * ix = [0-2]
> + * --------------------------------------------------------------------- */
> +
> +#define SYS_ZONE_AUTO_CHANNELS_TEMP	0
> +#define SYS_ZONE_AUTO_POINT1_TEMP	1
> +#define SYS_ZONE_AUTO_POINT2_TEMP	2
> +#define SYS_ZONE_AUTO_POINT3_TEMP	3
> +#define SYS_ZONE_AUTO_POINT4_TEMP	4

You can't handle hysteresis as just one more trip point. Hysteresis is
special in that it defines a different behavior when the temperature is
increasing and when the temperature is decreasing. Distinct trip points
aren't supposed to exhibit this behavior.

> +
> +static ssize_t show_zone(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct dme1737_data *data = dme1737_update_device(dev);
> +	struct sensor_device_attribute_2
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	int res;
> +
> +	switch (fn) {
> +	case SYS_ZONE_AUTO_CHANNELS_TEMP:
> +		/* check config2 for non-standard temp-to-zone mapping */
> +		if ((ix == 1) && (data->config2 & 0x02)) {
> +			res = 4;
> +		} else {
> +			res = 1 << ix;
> +		}
> +		break;
> +	case SYS_ZONE_AUTO_POINT1_TEMP:
> +		res = TEMP_FROM_REG(data->zone_low[ix], 8) -
> +		      TEMP_HYST_FROM_REG(data->zone_hyst[ix == 2], ix);
> +		break;
> +	case SYS_ZONE_AUTO_POINT2_TEMP:
> +		res = TEMP_FROM_REG(data->zone_low[ix], 8);
> +		break;
> +	case SYS_ZONE_AUTO_POINT3_TEMP:
> +		/* pwm_freq holds the temp range bits in the upper nibble */
> +		res = TEMP_FROM_REG(data->zone_low[ix], 8) +
> +		      TEMP_RANGE_FROM_REG(data->pwm_freq[ix]);
> +		break;
> +	case SYS_ZONE_AUTO_POINT4_TEMP:
> +		res = TEMP_FROM_REG(data->zone_abs[ix], 8);
> +		break;
> +	default:
> +		res = 0;
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +
> +	return sprintf(buf, "%d\n", res);
> +}
> +
> +static ssize_t set_zone(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute_2 
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	long val = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->update_lock);
> +	switch (fn) {
> +	case SYS_ZONE_AUTO_POINT1_TEMP:
> +		/* Refresh the cache */
> +		data->zone_low[ix] = dme1737_read(client,
> +						  DME1737_REG_ZONE_LOW(ix));
> +		/* Modify the temp hyst value */
> +		data->zone_hyst[ix == 2] = TEMP_HYST_TO_REG(
> +					TEMP_FROM_REG(data->zone_low[ix], 8) -
> +					val, ix, dme1737_read(client,
> +					DME1737_REG_ZONE_HYST(ix == 2)));
> +		dme1737_write(client, DME1737_REG_ZONE_HYST(ix == 2),
> +			      data->zone_hyst[ix == 2]);
> +		break;
> +	case SYS_ZONE_AUTO_POINT2_TEMP:
> +		data->zone_low[ix] = TEMP_TO_REG(val);
> +		dme1737_write(client, DME1737_REG_ZONE_LOW(ix),
> +			      data->zone_low[ix]);
> +		break;
> +	case SYS_ZONE_AUTO_POINT3_TEMP:
> +		/* Refresh the cache */
> +		data->zone_low[ix] = dme1737_read(client,
> +						  DME1737_REG_ZONE_LOW(ix));
> +		/* Modify the temp range value (which is stored in the upper
> +		 * nibble of the pwm_freq register) */
> +		data->pwm_freq[ix] = TEMP_RANGE_TO_REG(val - 
> +					TEMP_FROM_REG(data->zone_low[ix], 8),
> +					dme1737_read(client,
> +					DME1737_REG_PWM_FREQ(ix)));
> +		dme1737_write(client, DME1737_REG_PWM_FREQ(ix),
> +			      data->pwm_freq[ix]);
> +		break;
> +	case SYS_ZONE_AUTO_POINT4_TEMP:
> +		data->zone_abs[ix] = TEMP_TO_REG(val);
> +		dme1737_write(client, DME1737_REG_ZONE_ABS(ix),
> +			      data->zone_abs[ix]);
> +		break;
> +	default:
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Fan sysfs attributes
> + * ix = [0-5]
> + * --------------------------------------------------------------------- */
> +
> +#define SYS_FAN_INPUT	0
> +#define SYS_FAN_MIN	1
> +#define SYS_FAN_MAX	2
> +#define SYS_FAN_ALARM	3
> +#define SYS_FAN_TYPE	4
> +
> +static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct dme1737_data *data = dme1737_update_device(dev);
> +	struct sensor_device_attribute_2
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	int res;
> +
> +	switch (fn) {
> +	case SYS_FAN_INPUT:
> +		res = FAN_FROM_REG(data->fan[ix],
> +				   ix < 4 ? 0 :
> +				   FAN_TPC_FROM_REG(data->fan_opt[ix]));
> +		break;
> +	case SYS_FAN_MIN:
> +		res = FAN_FROM_REG(data->fan_min[ix],
> +				   ix < 4 ? 0 :
> +				   FAN_TPC_FROM_REG(data->fan_opt[ix]));
> +		break;
> +	case SYS_FAN_MAX:
> +		/* only valid for fan[5-6] */
> +		res = FAN_MAX_FROM_REG(data->fan_max[ix - 4]);
> +		break;
> +	case SYS_FAN_ALARM:
> +		res = (data->alarms >> DME1737_BIT_ALARM_FAN[ix]) & 0x01;
> +		break;
> +	case SYS_FAN_TYPE:
> +		/* only falid for fan[1-4] */

Typo: valid.

> +		res = FAN_TYPE_FROM_REG(data->fan_opt[ix]);
> +		break;
> +	default:
> +		res = 0;
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +
> +	return sprintf(buf, "%d\n", res);
> +}
> +
> +static ssize_t set_fan(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute_2 
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	long val = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->update_lock);
> +	switch (fn) {
> +	case SYS_FAN_MIN:
> +		if (ix < 4) {
> +			data->fan_min[ix] = FAN_TO_REG(val, 0);
> +		} else {
> +			/* Refresh the cache */
> +			data->fan_opt[ix] = dme1737_read(client,
> +						DME1737_REG_FAN_OPT(ix));
> +			/* Modify the fan min value */
> +			data->fan_min[ix] = FAN_TO_REG(val,
> +					FAN_TPC_FROM_REG(data->fan_opt[ix]));
> +		}
> +		dme1737_write(client, DME1737_REG_FAN_MIN(ix),
> +			      data->fan_min[ix] & 0xff);
> +		dme1737_write(client, DME1737_REG_FAN_MIN(ix) + 1,
> +			      data->fan_min[ix] >> 8);
> +		break;
> +	case SYS_FAN_MAX:
> +		/* Only valid for fan[5-6] */
> +		data->fan_max[ix - 4] = FAN_MAX_TO_REG(val);
> +		dme1737_write(client, DME1737_REG_FAN_MAX(ix),
> +			      data->fan_max[ix - 4]);
> +		break;
> +	case SYS_FAN_TYPE:
> +		/* Only valid for fan[1-4] */
> +		if (!(val == 1 || val == 2 || val == 4)) {
> +			count = -EINVAL;
> +			dev_warn(&client->dev, "Fan type value %ld not "
> +				 "supported. Choose one of 1, 2, or 4.\n",
> +				 val);
> +			goto exit;
> +		}
> +		data->fan_opt[ix] = FAN_TYPE_TO_REG(val, dme1737_read(client,
> +					DME1737_REG_FAN_OPT(ix)));
> +		dme1737_write(client, DME1737_REG_FAN_OPT(ix),
> +			      data->fan_opt[ix]);
> +		break;
> +	default:
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +exit:
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +/* ---------------------------------------------------------------------
> + * PWM sysfs attributes
> + * ix = [0-4]
> + * --------------------------------------------------------------------- */
> +
> +#define SYS_PWM				0
> +#define SYS_PWM_FREQ			1
> +#define SYS_PWM_ENABLE			2
> +#define SYS_PWM_RAMP_RATE		3
> +#define SYS_PWM_AUTO_CHANNELS_ZONE	4
> +#define SYS_PWM_AUTO_POINT1_PWM		5
> +#define SYS_PWM_AUTO_POINT2_PWM		6
> +#define SYS_PWM_AUTO_POINT3_PWM		7
> +
> +static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct dme1737_data *data = dme1737_update_device(dev);
> +	struct sensor_device_attribute_2
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	int res;
> +
> +	switch (fn) {
> +	case SYS_PWM:
> +		res = data->pwm[ix];
> +		break;
> +	case SYS_PWM_FREQ:
> +		res = PWM_FREQ_FROM_REG(data->pwm_freq[ix]);
> +		break;
> +	case SYS_PWM_ENABLE:
> +		res = (ix > 3 ? 1 : /* pwm[5-6] hard-wired to manual mode */
> +		       PWM_EN_FROM_REG(data->pwm_config[ix]));
> +		break;
> +	case SYS_PWM_RAMP_RATE:
> +		/* Only valid for pwm[1-3] */
> +		res = PWM_RR_FROM_REG(data->pwm_rr[ix > 0], ix);
> +		break;
> +	case SYS_PWM_AUTO_CHANNELS_ZONE:
> +		/* Only valid for pwm[1-3] */
> +		if (PWM_EN_FROM_REG(data->pwm_config[ix]) == 2) {
> +			res = PWM_ACZ_FROM_REG(data->pwm_config[ix]);
> +		} else {
> +			res = data->pwm_acz[ix];
> +		}
> +		break;
> +	case SYS_PWM_AUTO_POINT1_PWM:
> +		/* Only valid for pwm[1-3] */
> +		if (PWM_OFF_FROM_REG(data->pwm_rr[0], ix)) {
> +			res = data->pwm_min[ix];
> +		} else {
> +			res = 0;
> +		}
> +		break;
> +	case SYS_PWM_AUTO_POINT2_PWM:
> +		/* Only valid for pwm[1-3] */
> +		res = data->pwm_min[ix];
> +		break;
> +	case SYS_PWM_AUTO_POINT3_PWM:
> +		/* Only valid for pwm[1-3] */
> +		res = 255; /* hard-wired */
> +		break;
> +	default:
> +		res = 0;
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +
> +	return sprintf(buf, "%d\n", res);
> +}
> +
> +static struct attribute *dme1737_attr_pwm[];
> +static void dme1737_chmod_file(struct i2c_client*, struct attribute*, mode_t);
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute_2 
> +		*sensor_attr_2 = to_sensor_dev_attr_2(attr);
> +	int ix = sensor_attr_2->index;
> +	int fn = sensor_attr_2->nr;
> +	long val = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->update_lock);
> +	switch (fn) {
> +	case SYS_PWM:
> +		data->pwm[ix] = SENSORS_LIMIT(val, 0, 255);
> +		dme1737_write(client, DME1737_REG_PWM(ix), data->pwm[ix]);
> +		break;
> +	case SYS_PWM_FREQ:
> +		data->pwm_freq[ix] = PWM_FREQ_TO_REG(val, dme1737_read(client,
> +						DME1737_REG_PWM_FREQ(ix)));
> +		dme1737_write(client, DME1737_REG_PWM_FREQ(ix),
> +			      data->pwm_freq[ix]);
> +		break;
> +	case SYS_PWM_ENABLE:
> +		/* Only valid for pwm[1-3] */
> +		if (val < 0 || val > 2) {
> +			count = -EINVAL;
> +			dev_warn(&client->dev, "PWM enable %ld not "
> +				 "supported. Choose one of 0, 1, or 2.\n",
> +				 val);
> +			goto exit;
> +		}
> +		/* Refresh the cache */
> +		data->pwm_config[ix] = dme1737_read(client,
> +						DME1737_REG_PWM_CONFIG(ix));
> +		if (val == PWM_EN_FROM_REG(data->pwm_config[ix])) {
> +			/* Bail out if no change */
> +			goto exit;
> +		} else if (val == 0) {
> +			/* Turn fan fully on, i.e., set duty-cycle to 100% */
> +			data->pwm[ix] = 255;
> +			dme1737_write(client, DME1737_REG_PWM(ix),
> +				      data->pwm[ix]);

I'm not sure I understand why this is necessary. You're going to switch
pwm_config[ix] to full speed mode a few lines below anyway, so why
bother? Also, it seems to me that the register could be in read-only
mode (if the PWM was in automatic mode before) so the write would have
no effect.

> +		} else if (val == 2) {
> +			/* Turn on auto mode using the saved zone channel
> +			 * assignment */
> +			data->pwm_config[ix] = PWM_ACZ_TO_REG(
> +							data->pwm_acz[ix],
> +							data->pwm_config[ix]);
> +			dme1737_write(client, DME1737_REG_PWM_CONFIG(ix),
> +				      data->pwm_config[ix]);
> +			/* Enable PWM ramp rate if previously enabled */
> +			if (data->pwm_rr_en & (1 << ix)) {
> +				data->pwm_rr[ix > 0] = PWM_RR_EN_TO_REG(1, ix,
> +						dme1737_read(client,
> +						DME1737_REG_PWM_RR(ix > 0)));
> +				dme1737_write(client,
> +					      DME1737_REG_PWM_RR(ix > 0),
> +					      data->pwm_rr[ix > 0]);
> +			}
> +			/* Change permissions of pwm[ix] to read-only */
> +			dme1737_chmod_file(client, dme1737_attr_pwm[ix],
> +					   S_IRUGO);

This is racy. You should switch the file to read-only before writing to
the configuration register.

> +		}
> +		if (PWM_EN_FROM_REG(data->pwm_config[ix]) == 2) {

I seem to understand that this test is equivalent to "val < 2"? This
would IMHO be clearer.

> +			/* Turn on manual mode and save the current zone
> +			 * channel assignment */
> +			data->pwm_acz[ix] = PWM_ACZ_FROM_REG(
> +							data->pwm_config[ix]);
> +			data->pwm_config[ix] = PWM_EN_TO_REG(val,
> +							data->pwm_config[ix]);
> +			dme1737_write(client, DME1737_REG_PWM_CONFIG(ix),
> +				      data->pwm_config[ix]);
> +			/* Save current ramp rate state and disable it */
> +			data->pwm_rr[ix > 0] = dme1737_read(client,
> +						DME1737_REG_PWM_RR(ix > 0));
> +			data->pwm_rr_en &= ~(1 << ix);
> +			if (PWM_RR_EN_FROM_REG(data->pwm_rr[ix > 0], ix)) {
> +				data->pwm_rr_en |= (1 << ix);
> +				data->pwm_rr[ix > 0] = PWM_RR_EN_TO_REG(0, ix,
> +							data->pwm_rr[ix > 0]);
> +				dme1737_write(client,
> +					      DME1737_REG_PWM_RR(ix > 0),
> +					      data->pwm_rr[ix > 0]);
> +			}
> +			/* Change permissions of pwm[ix] to read-writeable */
> +			dme1737_chmod_file(client, dme1737_attr_pwm[ix],
> +					   S_IRUGO | S_IWUSR);
> +		}
> +		break;
> +	case SYS_PWM_RAMP_RATE:
> +		/* Only valid for pwm[1-3] */
> +		/* Refresh the cache */
> +		data->pwm_config[ix] = dme1737_read(client,
> +						DME1737_REG_PWM_CONFIG(ix));
> +		data->pwm_rr[ix > 0] = dme1737_read(client,
> +						DME1737_REG_PWM_RR(ix > 0));
> +		/* Set the ramp rate value */
> +		if (val > 0) {
> +			data->pwm_rr[ix > 0] = PWM_RR_TO_REG(val, ix,
> +							data->pwm_rr[ix > 0]);
> +		}
> +		/* Enable/disable the feature only if the associated PWM
> +		 * output is in automatic mode. */
> +		if (PWM_EN_FROM_REG(data->pwm_config[ix]) == 2) {
> +			data->pwm_rr[ix > 0] = PWM_RR_EN_TO_REG(val > 0, ix,
> +							data->pwm_rr[ix > 0]);
> +		}
> +		dme1737_write(client, DME1737_REG_PWM_RR(ix > 0),
> +			      data->pwm_rr[ix > 0]);
> +		break;
> +	case SYS_PWM_AUTO_CHANNELS_ZONE:
> +		/* Only valid for pwm[1-3] */
> +		if (!(val == 1 || val == 2 || val == 4 ||
> +		      val == 6 || val == 7)) {
> +			count = -EINVAL;
> +			dev_warn(&client->dev, "PWM auto channels zone %ld "
> +				 "not supported. Choose one of 1, 2, 4, 6, "
> +				 "or 7.\n", val);
> +			goto exit;
> +		}
> +		/* Refresh the cache */
> +		data->pwm_config[ix] = dme1737_read(client,
> +						DME1737_REG_PWM_CONFIG(ix));
> +		if (PWM_EN_FROM_REG(data->pwm_config[ix]) == 2) {
> +			/* PWM is already in auto mode so update the temp
> +			 * channel assignment */
> +			data->pwm_config[ix] = PWM_ACZ_TO_REG(val,
> +						data->pwm_config[ix]);
> +			dme1737_write(client, DME1737_REG_PWM_CONFIG(ix),
> +				      data->pwm_config[ix]);
> +		} else {
> +			/* PWM is not in auto mode so we save the temp
> +			 * channel assignment for later use */
> +			data->pwm_acz[ix] = val;
> +		}
> +		break;
> +	case SYS_PWM_AUTO_POINT1_PWM:
> +		/* Only valid for pwm[1-3] */
> +		/* Refresh the cache */
> +		data->pwm_min[ix] = dme1737_read(client,
> +						DME1737_REG_PWM_MIN(ix));
> +		/* There are only 2 values supported for the auto_point1_pwm
> +		 * value: 0 or auto_point2_pwm. So if the temperature drops
> +		 * below the auto_point1_temp value, the fan either turns off
> +		 * or runs at auto_point2_pwm duty-cycle. */
> +		if (val > ((data->pwm_min[ix] + 1) / 2)) {
> +			data->pwm_rr[0] = PWM_OFF_TO_REG(1, ix,
> +						dme1737_read(client,
> +						DME1737_REG_PWM_RR(0)));
> +			
> +		} else {
> +			data->pwm_rr[0] = PWM_OFF_TO_REG(0, ix,
> +						dme1737_read(client,
> +						DME1737_REG_PWM_RR(0)));
> +			
> +		}
> +		dme1737_write(client, DME1737_REG_PWM_RR(0),
> +			      data->pwm_rr[0]);
> +		break;
> +	case SYS_PWM_AUTO_POINT2_PWM:
> +		/* Only valid for pwm[1-3] */
> +		data->pwm_min[ix] = SENSORS_LIMIT(val, 0, 255);
> +		dme1737_write(client, DME1737_REG_PWM_MIN(ix),
> +			      data->pwm_min[ix]);
> +		break;
> +	default:
> +		dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> +	}
> +exit:
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Miscellaneous sysfs attributes
> + * --------------------------------------------------------------------- */
> +
> +static ssize_t show_vrm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +
> +	return sprintf(buf, "%d\n", data->vrm);
> +}
> +
> +static ssize_t set_vrm(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	long val = simple_strtol(buf, NULL, 10);
> +
> +	data->vrm = val;
> +	return count;
> +}
> +
> +static ssize_t show_vid(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct dme1737_data *data = dme1737_update_device(dev);
> +
> +	return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Sysfs device attribute defines and structs
> + * --------------------------------------------------------------------- */
> +
> +/* Voltages 0-6 */
> +
> +#define SENSOR_DEVICE_ATTR_IN(ix) \
> +static SENSOR_DEVICE_ATTR_2(in##ix##_input, S_IRUGO, \
> +        show_in, NULL, SYS_IN_INPUT, ix); \
> +static SENSOR_DEVICE_ATTR_2(in##ix##_min, S_IRUGO | S_IWUSR, \
> +        show_in, set_in, SYS_IN_MIN, ix); \
> +static SENSOR_DEVICE_ATTR_2(in##ix##_max, S_IRUGO | S_IWUSR, \
> +        show_in, set_in, SYS_IN_MAX, ix); \
> +static SENSOR_DEVICE_ATTR_2(in##ix##_alarm, S_IRUGO, \
> +        show_in, NULL, SYS_IN_ALARM, ix)
> +
> +SENSOR_DEVICE_ATTR_IN(0);
> +SENSOR_DEVICE_ATTR_IN(1);
> +SENSOR_DEVICE_ATTR_IN(2);
> +SENSOR_DEVICE_ATTR_IN(3);
> +SENSOR_DEVICE_ATTR_IN(4);
> +SENSOR_DEVICE_ATTR_IN(5);
> +SENSOR_DEVICE_ATTR_IN(6);
> +
> +/* Temperatures 1-3 */
> +
> +#define SENSOR_DEVICE_ATTR_TEMP(ix) \
> +static SENSOR_DEVICE_ATTR_2(temp##ix##_input, S_IRUGO, \
> +        show_temp, NULL, SYS_TEMP_INPUT, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(temp##ix##_min, S_IRUGO | S_IWUSR, \
> +        show_temp, set_temp, SYS_TEMP_MIN, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(temp##ix##_max, S_IRUGO | S_IWUSR, \
> +        show_temp, set_temp, SYS_TEMP_MAX, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(temp##ix##_offset, S_IRUGO, \
> +        show_temp, set_temp, SYS_TEMP_OFFSET, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(temp##ix##_alarm, S_IRUGO, \
> +        show_temp, NULL, SYS_TEMP_ALARM, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(temp##ix##_fault, S_IRUGO, \
> +        show_temp, NULL, SYS_TEMP_FAULT, ix-1)
> +
> +SENSOR_DEVICE_ATTR_TEMP(1);
> +SENSOR_DEVICE_ATTR_TEMP(2);
> +SENSOR_DEVICE_ATTR_TEMP(3);
> +
> +/* Zones 1-3 */
> +
> +#define SENSOR_DEVICE_ATTR_ZONE(ix) \
> +static SENSOR_DEVICE_ATTR_2(zone##ix##_auto_channels_temp, S_IRUGO, \
> +        show_zone, NULL, SYS_ZONE_AUTO_CHANNELS_TEMP, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(zone##ix##_auto_point1_temp, S_IRUGO, \
> +        show_zone, set_zone, SYS_ZONE_AUTO_POINT1_TEMP, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(zone##ix##_auto_point2_temp, S_IRUGO, \
> +        show_zone, set_zone, SYS_ZONE_AUTO_POINT2_TEMP, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(zone##ix##_auto_point3_temp, S_IRUGO, \
> +        show_zone, set_zone, SYS_ZONE_AUTO_POINT3_TEMP, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(zone##ix##_auto_point4_temp, S_IRUGO, \
> +        show_zone, set_zone, SYS_ZONE_AUTO_POINT4_TEMP, ix-1)
> +
> +SENSOR_DEVICE_ATTR_ZONE(1);
> +SENSOR_DEVICE_ATTR_ZONE(2);
> +SENSOR_DEVICE_ATTR_ZONE(3);
> +
> +/* Fans 1-4 */
> +
> +#define SENSOR_DEVICE_ATTR_FAN_1TO4(ix) \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_input, S_IRUGO, \
> +        show_fan, NULL, SYS_FAN_INPUT, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_min, S_IRUGO | S_IWUSR, \
> +        show_fan, set_fan, SYS_FAN_MIN, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_alarm, S_IRUGO, \
> +        show_fan, NULL, SYS_FAN_ALARM, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_type, S_IRUGO | S_IWUSR, \
> +        show_fan, set_fan, SYS_FAN_TYPE, ix-1)
> +
> +SENSOR_DEVICE_ATTR_FAN_1TO4(1);
> +SENSOR_DEVICE_ATTR_FAN_1TO4(2);
> +SENSOR_DEVICE_ATTR_FAN_1TO4(3);
> +SENSOR_DEVICE_ATTR_FAN_1TO4(4);
> +
> +/* Fans 5-6 */
> +
> +#define SENSOR_DEVICE_ATTR_FAN_5TO6(ix) \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_input, S_IRUGO, \
> +        show_fan, NULL, SYS_FAN_INPUT, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_min, S_IRUGO | S_IWUSR, \
> +        show_fan, set_fan, SYS_FAN_MIN, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_alarm, S_IRUGO, \
> +        show_fan, NULL, SYS_FAN_ALARM, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(fan##ix##_max, S_IRUGO | S_IWUSR, \
> +        show_fan, set_fan, SYS_FAN_MAX, ix-1)
> +
> +SENSOR_DEVICE_ATTR_FAN_5TO6(5);
> +SENSOR_DEVICE_ATTR_FAN_5TO6(6);
> +
> +/* PWMs 1-3 */
> +
> +#define SENSOR_DEVICE_ATTR_PWM_1TO3(ix) \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix, S_IRUGO, \
> +        show_pwm, set_pwm, SYS_PWM, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_freq, S_IRUGO, \
> +        show_pwm, set_pwm, SYS_PWM_FREQ, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_enable, S_IRUGO, \
> +        show_pwm, set_pwm, SYS_PWM_ENABLE, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_ramp_rate, S_IRUGO, \
> +        show_pwm, set_pwm, SYS_PWM_RAMP_RATE, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_auto_channels_zone, S_IRUGO, \
> +        show_pwm, set_pwm, SYS_PWM_AUTO_CHANNELS_ZONE, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_auto_point1_pwm, S_IRUGO, \
> +        show_pwm, set_pwm, SYS_PWM_AUTO_POINT1_PWM, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_auto_point2_pwm, S_IRUGO, \
> +        show_pwm, set_pwm, SYS_PWM_AUTO_POINT2_PWM, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_auto_point3_pwm, S_IRUGO, \
> +        show_pwm, NULL, SYS_PWM_AUTO_POINT3_PWM, ix-1)
> +
> +SENSOR_DEVICE_ATTR_PWM_1TO3(1);
> +SENSOR_DEVICE_ATTR_PWM_1TO3(2);
> +SENSOR_DEVICE_ATTR_PWM_1TO3(3);
> +
> +/* PWMs 5-6 */
> +
> +#define SENSOR_DEVICE_ATTR_PWM_5TO6(ix) \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix, S_IRUGO | S_IWUSR, \
> +        show_pwm, set_pwm, SYS_PWM, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_freq, S_IRUGO | S_IWUSR, \
> +        show_pwm, set_pwm, SYS_PWM_FREQ, ix-1); \
> +static SENSOR_DEVICE_ATTR_2(pwm##ix##_enable, S_IRUGO, \
> +        show_pwm, NULL, SYS_PWM_ENABLE, ix-1)
> +
> +SENSOR_DEVICE_ATTR_PWM_5TO6(5);
> +SENSOR_DEVICE_ATTR_PWM_5TO6(6);
> +
> +/* Misc */
> +
> +static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm, set_vrm);
> +static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
> +
> +#define SENSOR_DEV_ATTR_IN(ix) \
> +&sensor_dev_attr_in##ix##_input.dev_attr.attr, \
> +&sensor_dev_attr_in##ix##_min.dev_attr.attr, \
> +&sensor_dev_attr_in##ix##_max.dev_attr.attr, \
> +&sensor_dev_attr_in##ix##_alarm.dev_attr.attr
> +
> +/* These attributes are read-writeable only if the chip is *not* locked */
> +#define SENSOR_DEV_ATTR_TEMP_LOCK(ix) \
> +&sensor_dev_attr_temp##ix##_offset.dev_attr.attr
> +
> +#define SENSOR_DEV_ATTR_TEMP(ix) \
> +SENSOR_DEV_ATTR_TEMP_LOCK(ix), \
> +&sensor_dev_attr_temp##ix##_input.dev_attr.attr, \
> +&sensor_dev_attr_temp##ix##_min.dev_attr.attr, \
> +&sensor_dev_attr_temp##ix##_max.dev_attr.attr, \
> +&sensor_dev_attr_temp##ix##_alarm.dev_attr.attr, \
> +&sensor_dev_attr_temp##ix##_fault.dev_attr.attr
> +
> +/* These attributes are read-writeable only if the chip is *not* locked */
> +#define SENSOR_DEV_ATTR_ZONE_LOCK(ix) \
> +&sensor_dev_attr_zone##ix##_auto_point1_temp.dev_attr.attr, \
> +&sensor_dev_attr_zone##ix##_auto_point2_temp.dev_attr.attr, \
> +&sensor_dev_attr_zone##ix##_auto_point3_temp.dev_attr.attr, \
> +&sensor_dev_attr_zone##ix##_auto_point4_temp.dev_attr.attr
> +
> +#define SENSOR_DEV_ATTR_ZONE(ix) \
> +SENSOR_DEV_ATTR_ZONE_LOCK(ix), \
> +&sensor_dev_attr_zone##ix##_auto_channels_temp.dev_attr.attr
> +
> +#define SENSOR_DEV_ATTR_FAN_1TO4(ix) \
> +&sensor_dev_attr_fan##ix##_input.dev_attr.attr, \
> +&sensor_dev_attr_fan##ix##_min.dev_attr.attr, \
> +&sensor_dev_attr_fan##ix##_alarm.dev_attr.attr, \
> +&sensor_dev_attr_fan##ix##_type.dev_attr.attr
> +
> +#define SENSOR_DEV_ATTR_FAN_5TO6(ix) \
> +&sensor_dev_attr_fan##ix##_input.dev_attr.attr, \
> +&sensor_dev_attr_fan##ix##_min.dev_attr.attr, \
> +&sensor_dev_attr_fan##ix##_alarm.dev_attr.attr, \
> +&sensor_dev_attr_fan##ix##_max.dev_attr.attr
> +
> +/* These attributes are read-writeable only if the chip is *not* locked */
> +#define SENSOR_DEV_ATTR_PWM_1TO3_LOCK(ix) \
> +&sensor_dev_attr_pwm##ix##_freq.dev_attr.attr, \
> +&sensor_dev_attr_pwm##ix##_enable.dev_attr.attr, \
> +&sensor_dev_attr_pwm##ix##_ramp_rate.dev_attr.attr, \
> +&sensor_dev_attr_pwm##ix##_auto_channels_zone.dev_attr.attr, \
> +&sensor_dev_attr_pwm##ix##_auto_point1_pwm.dev_attr.attr, \
> +&sensor_dev_attr_pwm##ix##_auto_point2_pwm.dev_attr.attr
> +
> +#define SENSOR_DEV_ATTR_PWM_1TO3(ix) \
> +SENSOR_DEV_ATTR_PWM_1TO3_LOCK(ix), \
> +&sensor_dev_attr_pwm##ix.dev_attr.attr, \
> +&sensor_dev_attr_pwm##ix##_auto_point3_pwm.dev_attr.attr
> +
> +/* These attributes are read-writeable only if the chip is *not* locked */
> +#define SENSOR_DEV_ATTR_PWM_5TO6_LOCK(ix) \
> +&sensor_dev_attr_pwm##ix.dev_attr.attr, \
> +&sensor_dev_attr_pwm##ix##_freq.dev_attr.attr
> +
> +#define SENSOR_DEV_ATTR_PWM_5TO6(ix) \
> +SENSOR_DEV_ATTR_PWM_5TO6_LOCK(ix), \
> +&sensor_dev_attr_pwm##ix##_enable.dev_attr.attr
> +
> +/* This struct holds all the attributes that are always present and need to be
> + * created unconditionally. The attributes that need modification of their 
> + * permissions are created read-only and write permissions are added or removed
> + * on the fly when required */
> +static struct attribute *dme1737_attr[] ={
> +        /* Voltages */
> +        SENSOR_DEV_ATTR_IN(0),
> +        SENSOR_DEV_ATTR_IN(1),
> +        SENSOR_DEV_ATTR_IN(2),
> +        SENSOR_DEV_ATTR_IN(3),
> +        SENSOR_DEV_ATTR_IN(4),
> +        SENSOR_DEV_ATTR_IN(5),
> +        SENSOR_DEV_ATTR_IN(6),
> +        /* Temperatures */
> +        SENSOR_DEV_ATTR_TEMP(1),
> +        SENSOR_DEV_ATTR_TEMP(2),
> +        SENSOR_DEV_ATTR_TEMP(3),
> +        /* Zones */
> +        SENSOR_DEV_ATTR_ZONE(1),
> +        SENSOR_DEV_ATTR_ZONE(2),
> +        SENSOR_DEV_ATTR_ZONE(3),
> +        /* Fans */
> +        SENSOR_DEV_ATTR_FAN_1TO4(1),
> +        SENSOR_DEV_ATTR_FAN_1TO4(2),
> +        /* PWM */
> +        SENSOR_DEV_ATTR_PWM_1TO3(1),
> +        SENSOR_DEV_ATTR_PWM_1TO3(2),
> +        /* Misc */
> +        &dev_attr_vrm.attr,
> +        &dev_attr_cpu0_vid.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dme1737_group = {
> +        .attrs = dme1737_attr,
> +};
> +
> +/* The following structs hold the optional attributes. Their creation depends
> + * on the chip configuration which is determined during module load. */
> +static struct attribute *dme1737_attr_pwm3[] = {
> +        SENSOR_DEV_ATTR_PWM_1TO3(3),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_pwm5[] = {
> +        SENSOR_DEV_ATTR_PWM_5TO6(5),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_pwm6[] = {
> +        SENSOR_DEV_ATTR_PWM_5TO6(6),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_fan3[] = {
> +        SENSOR_DEV_ATTR_FAN_1TO4(3),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_fan4[] = {
> +        SENSOR_DEV_ATTR_FAN_1TO4(4),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_fan5[] = {
> +        SENSOR_DEV_ATTR_FAN_5TO6(5),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_fan6[] = {
> +        SENSOR_DEV_ATTR_FAN_5TO6(6),
> +	NULL
> +};
> +
> +static const struct attribute_group dme1737_opt_group[] = {
> +	{ .attrs = dme1737_attr_pwm3 },
> +	{ .attrs = dme1737_attr_pwm5 },
> +	{ .attrs = dme1737_attr_pwm6 },
> +	{ .attrs = dme1737_attr_fan3 },
> +	{ .attrs = dme1737_attr_fan4 },
> +	{ .attrs = dme1737_attr_fan5 },
> +	{ .attrs = dme1737_attr_fan6 },
> +};
> +
> +/* The permissions of all of the following attributes are changed to read-
> + * writeable if the chip is *not* locked. Otherwise they stay read-only. */
> +static struct attribute *dme1737_attr_lock[] = {
> +	/* Temperatures */
> +	SENSOR_DEV_ATTR_TEMP_LOCK(1),
> +	SENSOR_DEV_ATTR_TEMP_LOCK(2),
> +	SENSOR_DEV_ATTR_TEMP_LOCK(3),
> +	/* Zones */
> +	SENSOR_DEV_ATTR_ZONE_LOCK(1),
> +	SENSOR_DEV_ATTR_ZONE_LOCK(2),
> +	SENSOR_DEV_ATTR_ZONE_LOCK(3),
> +	/* PWMs */
> +	SENSOR_DEV_ATTR_PWM_1TO3_LOCK(1),
> +	SENSOR_DEV_ATTR_PWM_1TO3_LOCK(2),
> +	NULL
> +};
> +
> +static const struct attribute_group dme1737_lock_group = {
> +	.attrs = dme1737_attr_lock,
> +};
> +
> +/* The permissions of all of the following attributes are changed to read-
> + * writeable if the chip is *not* locked and the respective feature is
> + * available. Otherwise they stay read-only. */
> +static struct attribute *dme1737_attr_pwm3_lock[] = {
> +        SENSOR_DEV_ATTR_PWM_1TO3_LOCK(3),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_pwm5_lock[] = {
> +        SENSOR_DEV_ATTR_PWM_5TO6_LOCK(5),
> +	NULL
> +};
> +static struct attribute *dme1737_attr_pwm6_lock[] = {
> +        SENSOR_DEV_ATTR_PWM_5TO6_LOCK(6),
> +	NULL
> +};
> +
> +static const struct attribute_group dme1737_opt_lock_group[] = {
> +	{ .attrs = dme1737_attr_pwm3_lock },
> +	{ .attrs = dme1737_attr_pwm5_lock },
> +	{ .attrs = dme1737_attr_pwm6_lock },
> +};
> +
> +/* Pwm[1-3] are read-writeable if the associated pwm is in manual mode and the
> + * chip is not locked. Otherwise they are read-only. */
> +static struct attribute *dme1737_attr_pwm[] = {
> +	&sensor_dev_attr_pwm1.dev_attr.attr,
> +	&sensor_dev_attr_pwm2.dev_attr.attr,
> +	&sensor_dev_attr_pwm3.dev_attr.attr,
> +};
> +
> +/* ---------------------------------------------------------------------
> + * Super-IO functions
> + * --------------------------------------------------------------------- */
> +
> +static inline int dme1737_sio_inb(int sio_cip, int reg)
> +{
> +	outb(reg, sio_cip);
> +	return inb(sio_cip + 1);
> +}
> +
> +static inline void dme1737_sio_outb(int sio_cip, int reg, int val)
> +{
> +	outb(reg, sio_cip);
> +	outb(val, sio_cip + 1);
> +}
> +
> +static int dme1737_sio_get_features(int sio_cip, struct i2c_client *client)
> +{
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	int err = 0, reg;
> +	u16 addr;
> +
> +	/* Enter configuration mode */
> +	outb(0x55, sio_cip);
> +
> +	/* Check device ID 
> +	 * The DME1737 can return either 0x78 or 0x77 as its device ID. */
> +	reg = dme1737_sio_inb(sio_cip, 0x20);
> +	if (!(reg == 0x77 || reg == 0x78)) {
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +
> +	/* Select logical device A (runtime registers) */
> +	dme1737_sio_outb(sio_cip, 0x07, 0x0a);
> +
> +	/* Get the base address of the runtime registers */
> +	if (!(addr = (dme1737_sio_inb(sio_cip, 0x60) << 8) | 
> +		      dme1737_sio_inb(sio_cip, 0x61))) {
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +
> +	/* Read the runtime registers to determine which optional features
> +	 * are enabled and available. Bits [3:2] of registers 0x43-0x46 are set
> +	 * to '10' if the respective feature is enabled. */
> +	if ((inb(addr + 0x43) & 0x0c) == 0x08) { /* fan6 */
> +		data->opt_features |= DME1737_OPT_FAN6;
> +	}
> +	if ((inb(addr + 0x44) & 0x0c) == 0x08) { /* pwm6 */
> +		data->opt_features |= DME1737_OPT_PWM6;
> +	}
> +	if ((inb(addr + 0x45) & 0x0c) == 0x08) { /* fan5 */
> +		data->opt_features |= DME1737_OPT_FAN5;
> +	}
> +	if ((inb(addr + 0x46) & 0x0c) == 0x08) { /* pwm5 */
> +		data->opt_features |= DME1737_OPT_PWM5;
> +	}
> +
> +exit:
> +	/* Exit configuration mode */
> +	outb(0xaa, sio_cip);
> +
> +	return err;
> +}
> +
> +/* ---------------------------------------------------------------------
> + * Device detection, registration and initialization
> + * --------------------------------------------------------------------- */
> +
> +static struct i2c_driver dme1737_driver;
> +
> +static void dme1737_chmod_file(struct i2c_client *client,
> +			       struct attribute *attr, mode_t mode)
> +{
> +	if (sysfs_chmod_file(&client->dev.kobj, attr, mode)) {
> +		dev_warn(&client->dev, "Failed to change permissions of %s.\n",
> +			 attr->name);
> +	}
> +}
> +
> +static void dme1737_chmod_group(struct i2c_client *client,
> +				const struct attribute_group *group,
> +				mode_t mode)
> +{
> +	struct attribute **attr;
> +	
> +	for (attr = group->attrs; *attr; attr++) {
> +		dme1737_chmod_file(client, *attr, mode);
> +	}
> +}
> +
> +static int dme1737_init_client(struct i2c_client *client)
> +{
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	int ix;
> +	u8 reg;
> +
> +        data->config = dme1737_read(client, DME1737_REG_CONFIG);
> +        /* Inform if part is not monitoring/started */
> +        if (!(data->config & 0x01)) {
> +                if (!force_start) {
> +                        dev_err(&client->dev, "Device is not monitoring. "
> +                                "Use the force_start load parameter to "
> +                                "override.\n");
> +                        return -EFAULT;
> +                }
> +
> +                /* Force monitoring */
> +                data->config |= 0x01;
> +                dme1737_write(client, DME1737_REG_CONFIG, data->config);
> +        }
> +	/* Inform if part is not ready */
> +	if (!(data->config & 0x04)) {
> +		dev_err(&client->dev, "Device is not ready.\n");
> +		return -EFAULT;
> +	}
> +
> +	data->config2 = dme1737_read(client, DME1737_REG_CONFIG2);
> +	/* Check if optional fan3 input is enabled */
> +	if (data->config2 & 0x04) {
> +		data->opt_features |= DME1737_OPT_FAN3;
> +	}
> +
> +	/* Fan[4] and pwm[3] are only available if the client's I2C address

This notation is error-prone, "Fan4 and pwm3" would be better.

> +	 * is the default 0x2e. Otherwise the I/Os associated with these
> +	 * functions are used for addr enable/select. */
> +	if (client->addr == 0x2e) {
> +		data->opt_features |= DME1737_OPT_PWM3 | DME1737_OPT_FAN4;
> +	}
> +
> +	/* Determine if the optional fan[5-6] and/or pwm[5-6] are enabled.
> +	 * For this, we need to query the runtime registers through the
> +	 * Super-IO LPC interface. Try both config ports 0x2e and 0x4e. */
> +	if (dme1737_sio_get_features(0x2e, client) &&
> +	    dme1737_sio_get_features(0x4e, client)) {
> +		dev_warn(&client->dev, "Failed to query Super-IO for optional "
> +			 "features.\n");
> +	}
> +
> +	dev_info(&client->dev, "Optional features: pwm3 (%s), pwm5 (%s), "

pwm3=%s etc... would look better?

> +		 "pwm6 (%s), fan3 (%s), fan4 (%s), fan5 (%s), fan6 (%s).\n",
> +		 (data->opt_features & DME1737_OPT_PWM3) ? "yes" : "no",
> +		 (data->opt_features & DME1737_OPT_PWM5) ? "yes" : "no",
> +		 (data->opt_features & DME1737_OPT_PWM6) ? "yes" : "no",
> +		 (data->opt_features & DME1737_OPT_FAN3) ? "yes" : "no",
> +		 (data->opt_features & DME1737_OPT_FAN4) ? "yes" : "no",
> +		 (data->opt_features & DME1737_OPT_FAN5) ? "yes" : "no",
> +		 (data->opt_features & DME1737_OPT_FAN6) ? "yes" : "no");
> +
> +	reg = dme1737_read(client, DME1737_REG_TACH_PWM);
> +	/* Inform if fan-to-pwm mapping differs from the default */
> +	if (reg != 0xa4) {
> +		dev_warn(&client->dev, "Non-standard fan to pwm mapping: "
> +			 "fan1->pwm%d, fan2->pwm%d, fan3->pwm%d, "
> +			 "fan4->pwm%d. Please report to the driver "
> +			 "maintainer.\n",
> +			 (reg & 0x03) + 1, ((reg >> 2) & 0x03) + 1,
> +			 ((reg >> 4) & 0x03) + 1, ((reg >> 6) & 0x03) + 1);
> +	}
> +	
> +	/* Switch pwm[1-3] to manual mode if they are currently disabled and
> +	 * set the duty-cycles to 0% (which is identical to the PWMs being
> +	 * disabled). */
> +	if (!(data->config & 0x02)) {
> +		for (ix = 0; 
> +		     ix < ((data->opt_features & DME1737_OPT_PWM3) ? 3 : 2);
> +		     ix++) {
> +			data->pwm_config[ix] = dme1737_read(client, 
> +						DME1737_REG_PWM_CONFIG(ix));
> +			if (PWM_EN_FROM_REG(data->pwm_config[ix]) == -1) {
> +				dev_info(&client->dev, "Switching pwm%d to "
> +					 "manual mode.\n", ix + 1);
> +				data->pwm_config[ix] = PWM_EN_TO_REG(1, 
> +							data->pwm_config[ix]);
> +				dme1737_write(client, DME1737_REG_PWM(ix), 0);
> +				dme1737_write(client,
> +					      DME1737_REG_PWM_CONFIG(ix),
> +					      data->pwm_config[ix]);
> +			}
> +		}
> +	}
> +
> +	/* Initialize the default PWM auto channels zone (acz) assignments */
> +	data->pwm_acz[0] = 1;	/* pwm1 -> zone1 */
> +	data->pwm_acz[1] = 2;	/* pwm2 -> zone2 */
> +	data->pwm_acz[2] = 4;	/* pwm3 -> zone3 */
> +
> +	/* Set VRM */
> +	data->vrm = vid_which_vrm();
> +
> +	return 0;
> +}
> +
> +static int dme1737_detect(struct i2c_adapter *adapter, int address,
> +			  int kind)
> +{
> +	u8 company, verstep = 0;
> +	struct i2c_client *client;
> +	struct dme1737_data *data;
> +	int ix, err = 0;
> +	const char *name;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		goto exit;
> +	}
> +
> +	if (!(data = kzalloc(sizeof(struct dme1737_data), GFP_KERNEL))) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	client = &data->client;
> +	i2c_set_clientdata(client, data);
> +	client->addr = address;
> +	client->adapter = adapter;
> +	client->driver = &dme1737_driver;
> +
> +        /* A negative kind means that the driver was loaded with no force

Broken indentation.

> +	 * parameter (default), so we must identify the chip. */
> +       	if (kind < 0) {

Broken indentation.

> +		company = dme1737_read(client, DME1737_REG_COMPANY);
> +		verstep = dme1737_read(client, DME1737_REG_VERSTEP);
> +
> +		if (!((company == DME1737_COMPANY_SMSC) &&
> +		      ((verstep & DME1737_VERSTEP_MASK) == DME1737_VERSTEP))) {
> +			err = -ENODEV;
> +			goto exit_kfree;
> +		}
> +	}
> +
> +	kind = dme1737;
> +	name = "dme1737";
> +
> +	/* Fill in the remaining client fields and put it into the global 
> +	 * list */
> +	strlcpy(client->name, name, I2C_NAME_SIZE);
> +	mutex_init(&data->update_lock);
> +
> +	/* Tell the I2C layer a new client has arrived */
> +	if ((err = i2c_attach_client(client))) {
> +		goto exit_kfree;
> +	}
> +
> +	/* Initialize the DME1737 chip */
> +	if ((err = dme1737_init_client(client))) {
> +		goto exit_detach;
> +	}
> +
> +	/* Create standard sysfs attributes */
> +	if ((err = sysfs_create_group(&client->dev.kobj, &dme1737_group))) {
> +                goto exit_detach;
> +	}
> +
> +	/* Create optional sysfs attributes */
> +	for (ix = 0; ix < ARRAY_SIZE(dme1737_opt_group); ix++) {
> +		if (data->opt_features & (1 << ix)) {
> +			if ((err = sysfs_create_group(&client->dev.kobj,
> +						&dme1737_opt_group[ix]))) {
> +				goto exit_remove;
> +			}
> +		}
> +	}
> +
> +	/* Inform if the device is locked. Otherwise change the permissions of
> +	 * selected attributes from read-only to read-writeable. */
> +	if (data->config & 0x02) {
> +		dev_info(&client->dev, "Device is locked. Some attributes "
> +			 "will be read-only.\n");
> +	} else {
> +		/* Change permissions of standard attributes */
> +		dme1737_chmod_group(client, &dme1737_lock_group,
> +				    S_IRUGO | S_IWUSR);
> +
> +		/* Change permissions of optional pwm[3,5-6] attributes */
> +		for (ix = 0; ix < 2; ix++) {
> +			if (data->opt_features & (1 << ix)) {
> +				dme1737_chmod_group(client,
> +						&dme1737_opt_lock_group[ix],
> +						S_IRUGO | S_IWUSR);
> +			}
> +		}
> +
> +		/* Change permissions of pwm[1-3] if in manual mode */
> +		for (ix = 0; 
> +		     ix < ((data->opt_features & DME1737_OPT_PWM3) ? 3 : 2);
> +		     ix++) {
> +			if (PWM_EN_FROM_REG(data->pwm_config[ix]) == 1) {
> +				dme1737_chmod_file(client,
> +						   dme1737_attr_pwm[ix],
> +						   S_IRUGO | S_IWUSR);
> +			}
> +		}
> +	}
> +
> +	/* Register device */
> +	data->class_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto exit_remove;
> +	}
> +
> +	dev_info(&adapter->dev, "Found a DME1737 chip at 0x%02x "
> +		 "(rev 0x%02x)\n", client->addr, verstep);
> +
> +	return 0;
> +
> +exit_remove:
> +	for (ix = 0; ix < ARRAY_SIZE(dme1737_opt_group); ix++) {
> +		sysfs_remove_group(&client->dev.kobj, &dme1737_opt_group[ix]);
> +	}
> +	sysfs_remove_group(&client->dev.kobj, &dme1737_group);
> +exit_detach:
> +	i2c_detach_client(client);
> +exit_kfree:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int dme1737_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	if (!(adapter->class & I2C_CLASS_HWMON)) {
> +		return 0;
> +	}
> +
> +	return i2c_probe(adapter, &addr_data, dme1737_detect);
> +}
> +
> +static int dme1737_detach_client(struct i2c_client *client)
> +{
> +	struct dme1737_data *data = i2c_get_clientdata(client);
> +	int i, err;
> +
> +	hwmon_device_unregister(data->class_dev);
> +
> +	for (i = 0; i < ARRAY_SIZE(dme1737_opt_group); i++) {
> +		sysfs_remove_group(&client->dev.kobj, &dme1737_opt_group[i]);
> +	}
> +	sysfs_remove_group(&client->dev.kobj, &dme1737_group);
> +
> +	if ((err = i2c_detach_client(client))) {
> +		return err;
> +	}
> +
> +	kfree(data);
> +	return 0;
> +}
> +
> +static struct i2c_driver dme1737_driver = {
> +	.driver = {
> +		.name = "dme1737",
> +	},
> +	.attach_adapter	= dme1737_attach_adapter,
> +	.detach_client = dme1737_detach_client,
> +};
> +
> +static int __init dme1737_init(void)
> +{
> +	return i2c_add_driver(&dme1737_driver);
> +}
> +
> +static void __exit dme1737_exit(void)
> +{
> +	i2c_del_driver(&dme1737_driver);
> +}
> +
> +MODULE_AUTHOR("Juerg Haefliger <juergh at gmail.com>");
> +MODULE_DESCRIPTION("DME1737 sensors");
> +MODULE_LICENSE("GPL");
> +
> +module_init(dme1737_init);
> +module_exit(dme1737_exit);
> diff -uprN -X linux-2.6.21-mm1/Documentation/dontdiff -x MAINTAINERS -x Documentation linux-2.6.21-mm1.orig/drivers/hwmon/Kconfig linux-2.6.21-mm1/drivers/hwmon/Kconfig
> --- linux-2.6.21-mm1.orig/drivers/hwmon/Kconfig	2007-05-07 12:16:28.000000000 -0700
> +++ linux-2.6.21-mm1/drivers/hwmon/Kconfig	2007-05-07 12:28:57.000000000 -0700
> @@ -441,6 +441,18 @@ config SENSORS_SIS5595
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called sis5595.
>  
> +config SENSORS_DME1737
> +	tristate "SMSC DME1737 and compatibles"
> +	depends on HWMON && I2C && EXPERIMENTAL

Dependency on HWMON is now handled at menu level, don't include it here.

> +	select HWMON_VID
> +	help
> +	  If you say yes here you get support for the hardware monitoring
> +	  and fan control features of the SMSC DME1737 (and compatibles)
> +	  and Asus A8000 Super-I/O chips.

Strange wording. The Asus A8000 _is_ a DME1737-compatible chip. If I
remember properly, it's even impossible to distinguish it from a true
DME1737.

> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called dme1737.
> +
>  config SENSORS_SMSC47M1
>  	tristate "SMSC LPC47M10x and compatibles"
>  	depends on HWMON
> diff -uprN -X linux-2.6.21-mm1/Documentation/dontdiff -x MAINTAINERS -x Documentation linux-2.6.21-mm1.orig/drivers/hwmon/Makefile linux-2.6.21-mm1/drivers/hwmon/Makefile
> --- linux-2.6.21-mm1.orig/drivers/hwmon/Makefile	2007-05-07 12:16:28.000000000 -0700
> +++ linux-2.6.21-mm1/drivers/hwmon/Makefile	2007-05-07 12:29:32.000000000 -0700
> @@ -25,6 +25,7 @@ obj-$(CONFIG_SENSORS_APPLESMC)	+= apples
>  obj-$(CONFIG_SENSORS_AMS)	+= ams/
>  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
> +obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
>  obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
>  obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
>  obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o

Can you please fix these last few problems and resubmit? Either a
complete patch or an incremental patch.

Thanks,
-- 
Jean Delvare




More information about the lm-sensors mailing list