Hi Juerg, On Tue, 19 Jan 2010 21:15:43 -0800, Juerg Haefliger wrote: > This patch adds support for the hardware monitoring capabilities of > the sch5127 chip to the dme1737 driver. Thanks for doing this. Review: > Signed-off-by: Juerg Haefliger <juergh at gmail.com> > (...) > Index: linux-2.6.33-rc2/Documentation/hwmon/dme1737 > =================================================================== > --- linux-2.6.33-rc2.orig/Documentation/hwmon/dme1737 2009-12-26 12:37:20.000000000 -0800 > +++ linux-2.6.33-rc2/Documentation/hwmon/dme1737 2010-01-03 13:47:26.000000000 -0800 > @@ -9,11 +9,15 @@ > * SMSC SCH3112, SCH3114, SCH3116 > Prefix: 'sch311x' > Addresses scanned: none, address read from Super-I/O config space > - Datasheet: http://www.nuhorizons.com/FeaturedProducts/Volume1/SMSC/311x.pdf > + Datasheet: http://www.google.com/search?q=sch311x+pdf I don't think it makes sense to provide such links... there's no guarantee the search will return anything relevant in the future. And people can use whatever search engine they like... > * SMSC SCH5027 > Prefix: 'sch5027' > Addresses scanned: I2C 0x2c, 0x2d, 0x2e > Datasheet: Provided by SMSC upon request and under NDA > + * SMSC SCH5127 > + Prefix: 'sch5127' > + Addresses scanned: none, address read from Super-I/O config space > + Datasheet: Provided by SMSC upon request and under NDA > > Authors: > Juerg Haefliger <juergh@xxxxxxxxx> > @@ -36,8 +40,8 @@ > ----------- > > This driver implements support for the hardware monitoring capabilities of the > -SMSC DME1737 and Asus A8000 (which are the same), SMSC SCH5027, and SMSC > -SCH311x Super-I/O chips. These chips feature monitoring of 3 temp sensors > +SMSC DME1737 and Asus A8000 (which are the same), SMSC SCH5027, SCH311x, > +and SCH5127 Super-I/O chips. These chips feature monitoring of 3 temp sensors > temp[1-3] (2 remote diodes and 1 internal), 7 voltages in[0-6] (6 external and > 1 internal) and up to 6 fan speeds fan[1-6]. Additionally, the chips implement > up to 5 PWM outputs pwm[1-3,5-6] for controlling fan speeds both manually and > @@ -48,14 +52,14 @@ > the configuration of the chip. The driver will detect which features are > present during initialization and create the sysfs attributes accordingly. > > -For the SCH311x, fan[1-3] and pwm[1-3] are always present and fan[4-6] and > -pwm[5-6] don't exist. > +For the SCH311x and SCH5127, fan[1-3] and pwm[1-3] are always present and > +fan[4-6] and pwm[5-6] don't exist. > > The hardware monitoring features of the DME1737, A8000, and SCH5027 are only > -accessible via SMBus, while the SCH311x only provides access via the ISA bus. > -The driver will therefore register itself as an I2C client driver if it detects > -a DME1737, A8000, or SCH5027 and as a platform driver if it detects a SCH311x > -chip. > +accessible via SMBus, while the SCH311x and SCH5127 only provide access via > +the ISA bus. The driver will therefore register itself as an I2C client driver > +if it detects a DME1737, A8000, or SCH5027 and as a platform driver if it > +detects a SCH311x or SCH5127 chip. > > > Voltage Monitoring > @@ -76,7 +80,7 @@ > in6: Vbat (+3.0V) 0V - 4.38V > > SCH311x: > - in0: +2.5V 0V - 6.64V > + in0: +2.5V 0V - 3.32V > in1: Vccp (processor core) 0V - 2V > in2: VCC (internal +3.3V) 0V - 4.38V > in3: +5V 0V - 6.64V > @@ -93,6 +97,15 @@ > in5: VTR (+3.3V standby) 0V - 4.38V > in6: Vbat (+3.0V) 0V - 4.38V > > +SCH5127: > + in0: +2.5 0V - 3.32V > + in1: Vccp (processor core) 0V - 3V > + in2: VCC (internal +3.3V) 0V - 4.38V > + in3: V2_IN 0V - 1.5V > + in4: V1_IN 0V - 1.5V > + in5: VTR (+3.3V standby) 0V - 4.38V > + in6: Vbat (+3.0V) 0V - 4.38V > + > Each voltage input has associated min and max limits which trigger an alarm > when crossed. > > @@ -293,3 +306,21 @@ > pwm[1-3]_auto_point2_pwm RO Auto PWM pwm point. Auto_point2 is the > full-speed duty-cycle which is hard- > wired to 255 (100% duty-cycle). > + > +Chip Differences > +---------------- > + > +Feature dme1737 sch311x sch5027 sch5127 > +------------------------------------------------------- > +temp[1-3]_offset yes yes > +vid yes > +zone3 yes yes yes > +zone[1-3]_hyst yes yes > +pwm min/off yes yes > +fan3 opt yes opt yes > +pwm3 opt yes opt yes > +fan4 opt opt > +fan5 opt opt > +pwm5 opt opt > +fan6 opt opt > +pwm6 opt opt > Index: linux-2.6.33-rc2/drivers/hwmon/dme1737.c > =================================================================== > --- linux-2.6.33-rc2.orig/drivers/hwmon/dme1737.c 2009-12-26 12:37:21.000000000 -0800 > +++ linux-2.6.33-rc2/drivers/hwmon/dme1737.c 2010-01-03 13:37:14.000000000 -0800 > @@ -1,12 +1,14 @@ > /* > - * dme1737.c - Driver for the SMSC DME1737, Asus A8000, SMSC SCH311x and > - * SCH5027 Super-I/O chips integrated hardware monitoring features. > - * Copyright (c) 2007, 2008 Juerg Haefliger <juergh@xxxxxxxxx> > + * dme1737.c - Driver for the SMSC DME1737, Asus A8000, SMSC SCH311x, SCH5027, > + * and SCH5127 Super-I/O chips integrated hardware monitoring > + * features. > + * Copyright (c) 2007, 2008, 2009 Juerg Haefliger <juergh@xxxxxxxxx> You might want to add 2010... > * > * This driver is an I2C/ISA hybrid, meaning that it uses the I2C bus to access > * the chip registers if a DME1737, A8000, or SCH5027 is found and the ISA bus > - * if a SCH311x chip is found. Both types of chips have very similar hardware > - * monitoring capabilities but differ in the way they can be accessed. > + * if a SCH311x or SCH5127 chip is found. Both types of chips have very > + * similar hardware monitoring capabilities but differ in the way they can be > + * accessed. > * > * 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 > @@ -57,7 +59,7 @@ > /* Addresses to scan */ > static const unsigned short normal_i2c[] = {0x2c, 0x2d, 0x2e, I2C_CLIENT_END}; > > -enum chips { dme1737, sch5027, sch311x }; > +enum chips { dme1737, sch5027, sch311x, sch5127 }; > > /* --------------------------------------------------------------------- > * Registers > @@ -164,10 +166,29 @@ > #define DME1737_VERSTEP_MASK 0xf8 > #define SCH311X_DEVICE 0x8c > #define SCH5027_VERSTEP 0x69 > +#define SCH5127_DEVICE 0x8e > + > +/* Device ID values (global configuration register index 0x20) */ > +#define DME1737_ID_1 0x77 > +#define DME1737_ID_2 0x78 > +#define SCH3112_ID 0x7c > +#define SCH3114_ID 0x7d > +#define SCH3116_ID 0x7f > +#define SCH5027_ID 0x89 > +#define SCH5127_ID 0x86 > > /* Length of ISA address segment */ > #define DME1737_EXTENT 2 > > +/* chip-dependent features */ > +#define HAS_TEMP_OFFSET (1 << 0) /* bit 0 */ > +#define HAS_VID (1 << 1) /* bit 1 */ > +#define HAS_ZONE3 (1 << 2) /* bit 2 */ > +#define HAS_ZONE_HYST (1 << 3) /* bit 3 */ > +#define HAS_PWM_MIN (1 << 4) /* bit 4 */ > +#define HAS_FAN(ix) (1 << ((ix) + 5)) /* bits 5-10 */ > +#define HAS_PWM(ix) (1 << ((ix) + 11)) /* bits 11-16 */ > + > /* --------------------------------------------------------------------- > * Data structures and manipulation thereof > * --------------------------------------------------------------------- */ > @@ -187,8 +208,7 @@ > > u8 vid; > u8 pwm_rr_en; > - u8 has_pwm; > - u8 has_fan; > + u32 has_features; Hmm. While I'm generally fine with the idea of having feature flags, I don't think that including has_pwm and has_fan in has_features was really necessary. This causes code changes which would have been avoided otherwise, for no benefit that I can see. The only benefit I could see would be if you shifted all the indexes by 1 so that for example HAS_FAN(2) refers to fan2 and not fan3, but you didn't even do that... > > /* Register values */ > u16 in[7]; > @@ -224,8 +244,11 @@ > 3300}; > static const int IN_NOMINAL_SCH5027[] = {5000, 2250, 3300, 1125, 1125, 3300, > 3300}; > +static const int IN_NOMINAL_SCH5127[] = {2500, 2250, 3300, 1125, 1125, 3300, > + 3300}; > #define IN_NOMINAL(type) ((type) == sch311x ? IN_NOMINAL_SCH311x : \ > (type) == sch5027 ? IN_NOMINAL_SCH5027 : \ > + (type) == sch5127 ? IN_NOMINAL_SCH5127 : \ > IN_NOMINAL_DME1737) > > /* Voltage input > @@ -568,7 +591,7 @@ > > /* Sample register contents every 1 sec */ > if (time_after(jiffies, data->last_update + HZ) || !data->valid) { > - if (data->type == dme1737) { > + if (data->has_features & HAS_VID) { > data->vid = dme1737_read(data, DME1737_REG_VID) & > 0x3f; > } > @@ -599,7 +622,7 @@ > DME1737_REG_TEMP_MIN(ix)); > data->temp_max[ix] = dme1737_read(data, > DME1737_REG_TEMP_MAX(ix)); > - if (data->type != sch5027) { > + if (data->has_features & HAS_TEMP_OFFSET) { > data->temp_offset[ix] = dme1737_read(data, > DME1737_REG_TEMP_OFFSET(ix)); > } > @@ -626,7 +649,7 @@ > for (ix = 0; ix < ARRAY_SIZE(data->fan); ix++) { > /* Skip reading registers if optional fans are not > * present */ > - if (!(data->has_fan & (1 << ix))) { > + if (!(data->has_features & HAS_FAN(ix))) { > continue; > } > data->fan[ix] = dme1737_read(data, > @@ -650,7 +673,7 @@ > for (ix = 0; ix < ARRAY_SIZE(data->pwm); ix++) { > /* Skip reading registers if optional PWMs are not > * present */ > - if (!(data->has_pwm & (1 << ix))) { > + if (!(data->has_features & HAS_PWM(ix))) { > continue; > } > data->pwm[ix] = dme1737_read(data, > @@ -672,12 +695,24 @@ > > /* Thermal zone registers */ > for (ix = 0; ix < ARRAY_SIZE(data->zone_low); ix++) { > - data->zone_low[ix] = dme1737_read(data, > - DME1737_REG_ZONE_LOW(ix)); > - data->zone_abs[ix] = dme1737_read(data, > - DME1737_REG_ZONE_ABS(ix)); > + /* Skip reading registers if zone3 is not present */ > + if ((ix == 2) && !(data->has_features & HAS_ZONE3)) { > + continue; > + } > + /* sch5127 zone2 registers are special */ > + if ((ix == 1) && (data->type == sch5127)) { > + data->zone_low[1] = dme1737_read(data, > + DME1737_REG_ZONE_LOW(2)); > + data->zone_abs[1] = dme1737_read(data, > + DME1737_REG_ZONE_ABS(2)); > + } else { > + data->zone_low[ix] = dme1737_read(data, > + DME1737_REG_ZONE_LOW(ix)); > + data->zone_abs[ix] = dme1737_read(data, > + DME1737_REG_ZONE_ABS(ix)); > + } > } > - if (data->type != sch5027) { > + if (data->has_features & HAS_ZONE_HYST) { > for (ix = 0; ix < ARRAY_SIZE(data->zone_hyst); ix++) { > data->zone_hyst[ix] = dme1737_read(data, > DME1737_REG_ZONE_HYST(ix)); > @@ -1594,10 +1629,6 @@ > &sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr, > &sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr, > &sensor_dev_attr_zone2_auto_channels_temp.dev_attr.attr, > - &sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr, > - &sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr, > - &sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr, > - &sensor_dev_attr_zone3_auto_channels_temp.dev_attr.attr, > NULL > }; > > @@ -1605,27 +1636,23 @@ > .attrs = dme1737_attr, > }; > > -/* The following struct holds misc attributes, which are not available in all > - * chips. Their creation depends on the chip type which is determined during > - * module load. */ > -static struct attribute *dme1737_misc_attr[] = { > - /* Temperatures */ > +/* The following struct holds temp offset attributes, which are not available > + * in all chips. The following chips support them: > + * DME1737, SCH311x */ > +static struct attribute *dme1737_temp_offset_attr[] = { > &sensor_dev_attr_temp1_offset.dev_attr.attr, > &sensor_dev_attr_temp2_offset.dev_attr.attr, > &sensor_dev_attr_temp3_offset.dev_attr.attr, > - /* Zones */ > - &sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr, > - &sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr, > - &sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr, > NULL > }; > > -static const struct attribute_group dme1737_misc_group = { > - .attrs = dme1737_misc_attr, > +static const struct attribute_group dme1737_temp_offset_group = { > + .attrs = dme1737_temp_offset_attr, > }; > > -/* The following struct holds VID-related attributes. Their creation > - depends on the chip type which is determined during module load. */ > +/* The following struct holds VID related attributes, which are not available > + * in all chips. The following chips support them: > + * DME1737 */ > static struct attribute *dme1737_vid_attr[] = { > &dev_attr_vrm.attr, > &dev_attr_cpu0_vid.attr, > @@ -1636,6 +1663,36 @@ > .attrs = dme1737_vid_attr, > }; > > +/* The following struct holds temp zone 3 related attributes, which are not > + * available in all chips. The following chips support them: > + * DME1737, SCH311x, SCH5027 */ > +static struct attribute *dme1737_zone3_attr[] = { > + &sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr, > + &sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr, > + &sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr, > + &sensor_dev_attr_zone3_auto_channels_temp.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group dme1737_zone3_group = { > + .attrs = dme1737_zone3_attr, > +}; > + > + > +/* The following struct holds temp zone hysteresis related attributes, which > + * are not available in all chips. The following chips support them: > + * DME1737, SCH311x */ > +static struct attribute *dme1737_zone_hyst_attr[] = { > + &sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr, > + &sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr, > + &sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group dme1737_zone_hyst_group = { > + .attrs = dme1737_zone_hyst_attr, > +}; > + > /* The following structs hold the PWM attributes, some of which are optional. > * Their creation depends on the chip configuration which is determined during > * module load. */ > @@ -1691,10 +1748,10 @@ > { .attrs = dme1737_pwm6_attr }, > }; > > -/* The following struct holds misc PWM attributes, which are not available in > - * all chips. Their creation depends on the chip type which is determined > +/* The following struct holds auto PWM min attributes, which are not available > + * in all chips. Their creation depends on the chip type which is determined > * during module load. */ > -static struct attribute *dme1737_pwm_misc_attr[] = { > +static struct attribute *dme1737_auto_pwm_min_attr[] = { > &sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr, > &sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr, > &sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr, > @@ -1764,14 +1821,25 @@ > &sensor_dev_attr_zone2_auto_point1_temp.dev_attr.attr, > &sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr, > &sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group dme1737_zone_chmod_group = { > + .attrs = dme1737_zone_chmod_attr, > +}; > + > + > +/* The permissions of the following zone 3 attributes are changed to read- > + * writeable if the chip is *not* locked. Otherwise they stay read-only. */ > +static struct attribute *dme1737_zone3_chmod_attr[] = { > &sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr, > &sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr, > &sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr, > NULL > }; > > -static const struct attribute_group dme1737_zone_chmod_group = { > - .attrs = dme1737_zone_chmod_attr, > +static const struct attribute_group dme1737_zone3_chmod_group = { > + .attrs = dme1737_zone3_chmod_attr, > }; > > /* The permissions of the following PWM attributes are changed to read- > @@ -1887,30 +1955,35 @@ > int ix; > > for (ix = 0; ix < ARRAY_SIZE(dme1737_fan_group); ix++) { > - if (data->has_fan & (1 << ix)) { > + if (data->has_features & HAS_FAN(ix)) { > sysfs_remove_group(&dev->kobj, > &dme1737_fan_group[ix]); > } > } > > for (ix = 0; ix < ARRAY_SIZE(dme1737_pwm_group); ix++) { > - if (data->has_pwm & (1 << ix)) { > + if (data->has_features & HAS_PWM(ix)) { > sysfs_remove_group(&dev->kobj, > &dme1737_pwm_group[ix]); > - if (data->type != sch5027 && ix < 3) { > + if (data->has_features & HAS_PWM_MIN) { It doesn't seem right to me to remove the condition "ix < 3". dme1737_pwm_group has 6 elements, dme1737_auto_pwm_min_attr has only 3. > sysfs_remove_file(&dev->kobj, > - dme1737_pwm_misc_attr[ix]); > + dme1737_auto_pwm_min_attr[ix]); > } > } > } > > - if (data->type != sch5027) { > - sysfs_remove_group(&dev->kobj, &dme1737_misc_group); > + if (data->has_features & HAS_TEMP_OFFSET) { > + sysfs_remove_group(&dev->kobj, &dme1737_temp_offset_group); > } > - if (data->type == dme1737) { > + if (data->has_features & HAS_VID) { > sysfs_remove_group(&dev->kobj, &dme1737_vid_group); > } > - > + if (data->has_features & HAS_ZONE3) { > + sysfs_remove_group(&dev->kobj, &dme1737_zone3_group); > + } > + if (data->has_features & HAS_ZONE_HYST) { > + sysfs_remove_group(&dev->kobj, &dme1737_zone_hyst_group); > + } > sysfs_remove_group(&dev->kobj, &dme1737_group); > > if (!data->client) { > @@ -1934,23 +2007,31 @@ > goto exit_remove; > } > > - /* Create misc sysfs attributes */ > - if ((data->type != sch5027) && > + /* Create chip-dependent sysfs attributes */ > + if ((data->has_features & HAS_TEMP_OFFSET) && > (err = sysfs_create_group(&dev->kobj, > - &dme1737_misc_group))) { > + &dme1737_temp_offset_group))) { > goto exit_remove; > } > - > - /* Create VID-related sysfs attributes */ > - if ((data->type == dme1737) && > + if ((data->has_features & HAS_VID) && > (err = sysfs_create_group(&dev->kobj, > &dme1737_vid_group))) { > goto exit_remove; > } > + if ((data->has_features & HAS_ZONE3) && > + (err = sysfs_create_group(&dev->kobj, > + &dme1737_zone3_group))) { > + goto exit_remove; > + } > + if ((data->has_features & HAS_ZONE_HYST) && > + (err = sysfs_create_group(&dev->kobj, > + &dme1737_zone_hyst_group))) { > + goto exit_remove; > + } > > /* Create fan sysfs attributes */ > for (ix = 0; ix < ARRAY_SIZE(dme1737_fan_group); ix++) { > - if (data->has_fan & (1 << ix)) { > + if (data->has_features & HAS_FAN(ix)) { > if ((err = sysfs_create_group(&dev->kobj, > &dme1737_fan_group[ix]))) { > goto exit_remove; > @@ -1960,14 +2041,14 @@ > > /* Create PWM sysfs attributes */ > for (ix = 0; ix < ARRAY_SIZE(dme1737_pwm_group); ix++) { > - if (data->has_pwm & (1 << ix)) { > + if (data->has_features & HAS_PWM(ix)) { > if ((err = sysfs_create_group(&dev->kobj, > &dme1737_pwm_group[ix]))) { > goto exit_remove; > } > - if (data->type != sch5027 && ix < 3 && > + if ((data->has_features & HAS_PWM_MIN) && > (err = sysfs_create_file(&dev->kobj, > - dme1737_pwm_misc_attr[ix]))) { > + dme1737_auto_pwm_min_attr[ix]))) { > goto exit_remove; > } > } > @@ -1983,21 +2064,29 @@ > dme1737_chmod_group(dev, &dme1737_zone_chmod_group, > S_IRUGO | S_IWUSR); > > - /* Change permissions of misc sysfs attributes */ > - if (data->type != sch5027) { > - dme1737_chmod_group(dev, &dme1737_misc_group, > + /* Change permissions of chip-dependent sysfs attributes */ > + if (data->has_features & HAS_TEMP_OFFSET) { > + dme1737_chmod_group(dev, &dme1737_temp_offset_group, > + S_IRUGO | S_IWUSR); > + } > + if (data->has_features & HAS_ZONE3) { > + dme1737_chmod_group(dev, &dme1737_zone3_chmod_group, > + S_IRUGO | S_IWUSR); > + } > + if (data->has_features & HAS_ZONE_HYST) { > + dme1737_chmod_group(dev, &dme1737_zone_hyst_group, > S_IRUGO | S_IWUSR); > } > > /* Change permissions of PWM sysfs attributes */ > for (ix = 0; ix < ARRAY_SIZE(dme1737_pwm_chmod_group); ix++) { > - if (data->has_pwm & (1 << ix)) { > + if (data->has_features & HAS_PWM(ix)) { > dme1737_chmod_group(dev, > &dme1737_pwm_chmod_group[ix], > S_IRUGO | S_IWUSR); > - if (data->type != sch5027 && ix < 3) { > + if (data->has_features & HAS_PWM_MIN) { Same here. > dme1737_chmod_file(dev, > - dme1737_pwm_misc_attr[ix], > + dme1737_auto_pwm_min_attr[ix], > S_IRUGO | S_IWUSR); > } > } > @@ -2005,7 +2094,7 @@ > > /* Change permissions of pwm[1-3] if in manual mode */ > for (ix = 0; ix < 3; ix++) { > - if ((data->has_pwm & (1 << ix)) && > + if ((data->has_features & HAS_PWM(ix)) && > (PWM_EN_FROM_REG(data->pwm_config[ix]) == 1)) { > dme1737_chmod_file(dev, > dme1737_pwm_chmod_attr[ix], > @@ -2052,20 +2141,20 @@ > return -EFAULT; > } > > - /* Determine which optional fan and pwm features are enabled/present */ > + /* Determine which optional fan and pwm features are enabled (only > + * valid for I2C devices) */ Missing space for star alignment. > if (client) { /* I2C chip */ > data->config2 = dme1737_read(data, DME1737_REG_CONFIG2); > /* Check if optional fan3 input is enabled */ > if (data->config2 & 0x04) { > - data->has_fan |= (1 << 2); > + data->has_features |= HAS_FAN(2); > } > > /* Fan4 and pwm3 are only available if the client's I2C address > * is the default 0x2e. Otherwise the I/Os associated with > * these functions are used for addr enable/select. */ > if (client->addr == 0x2e) { > - data->has_fan |= (1 << 3); > - data->has_pwm |= (1 << 2); > + data->has_features |= HAS_FAN(3) | HAS_PWM(2); > } > > /* Determine which of the optional fan[5-6] and pwm[5-6] > @@ -2077,26 +2166,40 @@ > dev_warn(dev, "Failed to query Super-IO for optional " > "features.\n"); > } > - } else { /* ISA chip */ > - /* Fan3 and pwm3 are always available. Fan[4-5] and pwm[5-6] > - * don't exist in the ISA chip. */ > - data->has_fan |= (1 << 2); > - data->has_pwm |= (1 << 2); > } > > - /* Fan1, fan2, pwm1, and pwm2 are always present */ > - data->has_fan |= 0x03; > - data->has_pwm |= 0x03; > + /* Fan[1-2] and pwm[1-2] are present in all chips */ > + data->has_features |= HAS_FAN(0) | HAS_FAN(1) | HAS_PWM(0) | HAS_PWM(1); > + > + /* Chip-dependent features */ > + switch (data->type) { > + case dme1737: > + data->has_features |= HAS_TEMP_OFFSET | HAS_VID | HAS_ZONE3 | > + HAS_ZONE_HYST | HAS_PWM_MIN; > + break; > + case sch311x: > + data->has_features |= HAS_TEMP_OFFSET | HAS_ZONE3 | > + HAS_ZONE_HYST | HAS_PWM_MIN | HAS_FAN(2) | HAS_PWM(2); > + break; > + case sch5027: > + data->has_features |= HAS_ZONE3; > + break; > + case sch5127: > + data->has_features |= HAS_FAN(2) | HAS_PWM(2); > + break; > + default: > + break; > + } > > dev_info(dev, "Optional features: pwm3=%s, pwm5=%s, pwm6=%s, " > "fan3=%s, fan4=%s, fan5=%s, fan6=%s.\n", > - (data->has_pwm & (1 << 2)) ? "yes" : "no", > - (data->has_pwm & (1 << 4)) ? "yes" : "no", > - (data->has_pwm & (1 << 5)) ? "yes" : "no", > - (data->has_fan & (1 << 2)) ? "yes" : "no", > - (data->has_fan & (1 << 3)) ? "yes" : "no", > - (data->has_fan & (1 << 4)) ? "yes" : "no", > - (data->has_fan & (1 << 5)) ? "yes" : "no"); > + (data->has_features & HAS_PWM(2)) ? "yes" : "no", > + (data->has_features & HAS_PWM(4)) ? "yes" : "no", > + (data->has_features & HAS_PWM(5)) ? "yes" : "no", > + (data->has_features & HAS_FAN(2)) ? "yes" : "no", > + (data->has_features & HAS_FAN(3)) ? "yes" : "no", > + (data->has_features & HAS_FAN(4)) ? "yes" : "no", > + (data->has_features & HAS_FAN(5)) ? "yes" : "no"); > > reg = dme1737_read(data, DME1737_REG_TACH_PWM); > /* Inform if fan-to-pwm mapping differs from the default */ > @@ -2122,7 +2225,7 @@ > for (ix = 0; ix < 3; ix++) { > data->pwm_config[ix] = dme1737_read(data, > DME1737_REG_PWM_CONFIG(ix)); > - if ((data->has_pwm & (1 << ix)) && > + if ((data->has_features & HAS_PWM(ix)) && > (PWM_EN_FROM_REG(data->pwm_config[ix]) == -1)) { > dev_info(dev, "Switching pwm%d to " > "manual mode.\n", ix + 1); > @@ -2142,7 +2245,7 @@ > data->pwm_acz[2] = 4; /* pwm3 -> zone3 */ > > /* Set VRM */ > - if (data->type == dme1737) { > + if (data->has_features & HAS_VID) { > data->vrm = vid_which_vrm(); > } > > @@ -2163,10 +2266,10 @@ > dme1737_sio_enter(sio_cip); > > /* Check device ID > - * The DME1737 can return either 0x78 or 0x77 as its device ID. > - * The SCH5027 returns 0x89 as its device ID. */ > + * We currently know about two kinds of DME1737 and SCH5027. */ > reg = force_id ? force_id : dme1737_sio_inb(sio_cip, 0x20); > - if (!(reg == 0x77 || reg == 0x78 || reg == 0x89)) { > + if (!(reg == DME1737_ID_1 || reg == DME1737_ID_2 || > + reg == SCH5027_ID)) { > err = -ENODEV; > goto exit; > } > @@ -2185,16 +2288,16 @@ > * 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->has_fan |= (1 << 5); > + data->has_features |= HAS_FAN(5); > } > if ((inb(addr + 0x44) & 0x0c) == 0x08) { /* pwm6 */ > - data->has_pwm |= (1 << 5); > + data->has_features |= HAS_PWM(5); > } > if ((inb(addr + 0x45) & 0x0c) == 0x08) { /* fan5 */ > - data->has_fan |= (1 << 4); > + data->has_features |= HAS_FAN(4); > } > if ((inb(addr + 0x46) & 0x0c) == 0x08) { /* pwm5 */ > - data->has_pwm |= (1 << 4); > + data->has_features |= HAS_PWM(4); > } > > exit: > @@ -2222,7 +2325,6 @@ > if (company == DME1737_COMPANY_SMSC && > verstep == SCH5027_VERSTEP) { > name = "sch5027"; > - > } else if (company == DME1737_COMPANY_SMSC && > (verstep & DME1737_VERSTEP_MASK) == DME1737_VERSTEP) { > name = "dme1737"; > @@ -2329,10 +2431,10 @@ > dme1737_sio_enter(sio_cip); > > /* Check device ID > - * We currently know about SCH3112 (0x7c), SCH3114 (0x7d), and > - * SCH3116 (0x7f). */ > + * We currently know about SCH3112, SCH3114, SCH3116, and SCH5127 */ > reg = force_id ? force_id : dme1737_sio_inb(sio_cip, 0x20); > - if (!(reg == 0x7c || reg == 0x7d || reg == 0x7f)) { > + if (!(reg == SCH3112_ID || reg == SCH3114_ID || reg == SCH3116_ID || > + reg == SCH5127_ID)) { > err = -ENODEV; > goto exit; > } > @@ -2428,19 +2530,31 @@ > company = dme1737_read(data, DME1737_REG_COMPANY); > device = dme1737_read(data, DME1737_REG_DEVICE); > > - if (!((company == DME1737_COMPANY_SMSC) && > - (device == SCH311X_DEVICE))) { > + if ((company == DME1737_COMPANY_SMSC) && > + (device == SCH311X_DEVICE)) { > + force_id = SCH3112_ID; > + } else if ((company == DME1737_COMPANY_SMSC) && > + (device == SCH5127_DEVICE)) { > + force_id = SCH5127_ID; > + } else { I'm not fond of abusing force_id that way. I admit that it should work in practice, however it could break in theory, for example if more than one chip were present on a given system. I don't expect to see this any time soon, but this shows the danger of writing to a global variable that way. I'd rather do force_id ==> data->type ==> data->name, this is cleaner and shouldn't require much mode code. > err = -ENODEV; > goto exit_kfree; > } > } > - data->type = sch311x; > > - /* Fill in the remaining client fields and initialize the mutex */ > - data->name = "sch311x"; > + if (force_id == SCH5127_ID) { > + data->type = sch5127; > + data->name = "sch5127"; > + } else { > + data->type = sch311x; > + data->name = "sch311x"; > + } This assumes that force_id MUST be either SCH5127_ID or any of SCH311x_ID. This is true, but this is enforced in a different function (dme1737_isa_detect), so it's hard to guarantee that it will always be the case in the future. > + > + /* Initialize the mutex */ > mutex_init(&data->update_lock); > > - dev_info(dev, "Found a SCH311x chip at 0x%04x\n", data->addr); > + dev_info(dev, "Found a %s chip at 0x%04x\n", > + data->type == sch5127 ? "SCH5127" : "SCH311x", data->addr); > > /* Initialize the chip */ > if ((err = dme1737_init_device(dev))) { -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
