Hi Jean, as always, thanks for the detailed review. On Fri, 2011-02-25 at 09:43 -0500, Jean Delvare wrote: > Hi Guenter, > > On Sun, 20 Feb 2011 17:39:12 -0800, Guenter Roeck wrote: > > EMC6D103S is similar to EMC6D103, only it does not support registers 62[5:7], > > 6D[0:7], and 6E[0:7]. Register respective sysfs attributes and update affected > > registers for all other chips only. > > > > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx> > > --- > > v2: Fix typo in subject > > Add EMC66D103 and EMC6D103S to driver documentation > > Can you please update wiki/Devices too? > Done. > Review: > > > > > Documentation/hwmon/lm85 | 13 ++++++- > > drivers/hwmon/lm85.c | 97 ++++++++++++++++++++++++++++++---------------- > > 2 files changed, 75 insertions(+), 35 deletions(-) > > > > diff --git a/Documentation/hwmon/lm85 b/Documentation/hwmon/lm85 > > index 239258a..0bc9a22 100644 > > --- a/Documentation/hwmon/lm85 > > +++ b/Documentation/hwmon/lm85 > > @@ -26,6 +26,14 @@ Supported chips: > > Prefix: 'emc6d102' > > Addresses scanned: I2C 0x2c, 0x2d, 0x2e > > Datasheet: http://www.smsc.com/main/catalog/emc6d102.html > > + * SMSC EMC6D103 > > + Prefix: 'emc6d103' > > + Addresses scanned: I2C 0x2c, 0x2d, 0x2e > > + Datasheet: http://www.smsc.com/main/catalog/emc6d103.html > > + * SMSC EMC6D103S > > + Prefix: 'emc6d103s' > > + Addresses scanned: I2C 0x2c, 0x2d, 0x2e > > + Datasheet: http://www.smsc.com/main/catalog/emc6d103s.html > > > > Authors: > > Philip Pokorny <ppokorny@xxxxxxxxxxxxxxxxxxxx>, > > @@ -122,9 +130,12 @@ to be register compatible. The EMC6D100 offers all the features of the > > EMC6D101 plus additional voltage monitoring and system control features. > > Unfortunately it is not possible to distinguish between the package > > versions on register level so these additional voltage inputs may read > > -zero. The EMC6D102 features addtional ADC bits thus extending precision > > +zero. EMC6D102 and EMC6D103 feature addtional ADC bits thus extending precision > > of voltage and temperature channels. > > > > +SMSC EMC6D103S is similar to EMC6D103, but does not support registers 62[5:7], > > +6D[0:7], and 6E[0:7]. Respective sysfs attributes (pwmX_auto_pwm_minctl and > > +tempX_auto_temp_off) are therefore not supported for this chip. > > The documentation being user-oriented, I think it is more valuable to > document the missing features than the missing registers. The register > values can be seen in the driver source code for the interested. > Ok. > If you really insist on documenting the registers, please prefix > hexadecimal values with 0x for clarity, and drop the [0:7]s which don't > add value. > I don't. > Lastly, "not created" would seem more appropriate than "not supported" > in the last sentence. > > > > > Hardware Configurations > > ----------------------- > > diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c > > index d2cc286..d4dd824 100644 > > --- a/drivers/hwmon/lm85.c > > +++ b/drivers/hwmon/lm85.c > > @@ -41,7 +41,7 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2d, 0x2e, I2C_CLIENT_END }; > > enum chips { > > any_chip, lm85b, lm85c, > > adm1027, adt7463, adt7468, > > - emc6d100, emc6d102, emc6d103 > > + emc6d100, emc6d102, emc6d103, emc6d103s > > }; > > > > /* The LM85 registers */ > > @@ -352,6 +352,7 @@ static const struct i2c_device_id lm85_id[] = { > > { "emc6d101", emc6d100 }, > > { "emc6d102", emc6d102 }, > > { "emc6d103", emc6d103 }, > > + { "emc6d103s", emc6d103s }, > > { } > > }; > > MODULE_DEVICE_TABLE(i2c, lm85_id); > > @@ -935,16 +936,18 @@ static ssize_t set_temp_auto_temp_min(struct device *dev, > > | (data->pwm_freq[nr] & 0x07)); > > > > /* Update temp_auto_hyst and temp_auto_off */ > > - data->zone[nr].hyst = HYST_TO_REG(TEMP_FROM_REG( > > - data->zone[nr].limit) - TEMP_FROM_REG( > > - data->zone[nr].off_desired)); > > - if (nr == 0 || nr == 1) { > > - lm85_write_value(client, LM85_REG_AFAN_HYST1, > > - (data->zone[0].hyst << 4) > > - | data->zone[1].hyst); > > - } else { > > - lm85_write_value(client, LM85_REG_AFAN_HYST2, > > - (data->zone[2].hyst << 4)); > > + if (data->type != emc6d103s) { > > + data->zone[nr].hyst = HYST_TO_REG(TEMP_FROM_REG( > > + data->zone[nr].limit) - TEMP_FROM_REG( > > + data->zone[nr].off_desired)); > > + if (nr == 0 || nr == 1) { > > + lm85_write_value(client, LM85_REG_AFAN_HYST1, > > + (data->zone[0].hyst << 4) > > + | data->zone[1].hyst); > > + } else { > > + lm85_write_value(client, LM85_REG_AFAN_HYST2, > > + (data->zone[2].hyst << 4)); > > + } > > } > > This piece of code could probably go away altogether. It tries to be > smart, but it's bogus. data->zone[nr].off_desired is only initialized > if set_temp_auto_temp_off() is called, so if set_temp_auto_temp_min() > is called first, the "off" value will be set improperly. > > It would seem better, and easier, to leave the delta unchanged between > "min" and "off", until the user sets the "off" value again. This is > what other drivers do (except that other drivers make it clear that > this is an hysteresis and not two separate values.) > I removed it. > > mutex_unlock(&data->update_lock); > > return count; > > @@ -1084,13 +1087,7 @@ static struct attribute *lm85_attributes[] = { > > &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, > > - &sensor_dev_attr_pwm1_auto_pwm_minctl.dev_attr.attr, > > - &sensor_dev_attr_pwm2_auto_pwm_minctl.dev_attr.attr, > > - &sensor_dev_attr_pwm3_auto_pwm_minctl.dev_attr.attr, > > > > - &sensor_dev_attr_temp1_auto_temp_off.dev_attr.attr, > > - &sensor_dev_attr_temp2_auto_temp_off.dev_attr.attr, > > - &sensor_dev_attr_temp3_auto_temp_off.dev_attr.attr, > > &sensor_dev_attr_temp1_auto_temp_min.dev_attr.attr, > > &sensor_dev_attr_temp2_auto_temp_min.dev_attr.attr, > > &sensor_dev_attr_temp3_auto_temp_min.dev_attr.attr, > > @@ -1111,6 +1108,26 @@ static const struct attribute_group lm85_group = { > > .attrs = lm85_attributes, > > }; > > > > +static struct attribute *lm85_attributes_minctl[] = { > > + &sensor_dev_attr_pwm1_auto_pwm_minctl.dev_attr.attr, > > + &sensor_dev_attr_pwm2_auto_pwm_minctl.dev_attr.attr, > > + &sensor_dev_attr_pwm3_auto_pwm_minctl.dev_attr.attr, > > +}; > > + > > +static const struct attribute_group lm85_group_minctl = { > > + .attrs = lm85_attributes_minctl, > > +}; > > + > > +static struct attribute *lm85_attributes_temp_off[] = { > > + &sensor_dev_attr_temp1_auto_temp_off.dev_attr.attr, > > + &sensor_dev_attr_temp2_auto_temp_off.dev_attr.attr, > > + &sensor_dev_attr_temp3_auto_temp_off.dev_attr.attr, > > +}; > > + > > +static const struct attribute_group lm85_group_temp_off = { > > + .attrs = lm85_attributes_temp_off, > > +}; > > + > > static struct attribute *lm85_attributes_in4[] = { > > &sensor_dev_attr_in4_input.dev_attr.attr, > > &sensor_dev_attr_in4_min.dev_attr.attr, > > @@ -1258,16 +1275,9 @@ static int lm85_detect(struct i2c_client *client, struct i2c_board_info *info) > > case LM85_VERSTEP_EMC6D103_A1: > > type_name = "emc6d103"; > > break; > > - /* > > - * Registers apparently missing in EMC6D103S/EMC6D103:A2 > > - * compared to EMC6D103:A0, EMC6D103:A1, and EMC6D102 > > - * (according to the data sheets), but used unconditionally > > - * in the driver: 62[5:7], 6D[0:7], and 6E[0:7]. > > - * So skip EMC6D103S for now. > > case LM85_VERSTEP_EMC6D103S: > > type_name = "emc6d103s"; > > break; > > - */ > > } > > } else { > > dev_dbg(&adapter->dev, > > @@ -1302,6 +1312,7 @@ static int lm85_probe(struct i2c_client *client, > > case emc6d100: > > case emc6d102: > > case emc6d103: > > + case emc6d103s: > > data->freq_map = adm1027_freq_map; > > break; > > default: > > @@ -1319,6 +1330,17 @@ static int lm85_probe(struct i2c_client *client, > > if (err) > > goto err_kfree; > > > > + /* minctl and temp_off exist on all chips except emc6d103s */ > > + if (data->type != emc6d103s) { > > + err = sysfs_create_group(&client->dev.kobj, &lm85_group_minctl); > > + if (err) > > + goto err_kfree; > > + err = sysfs_create_group(&client->dev.kobj, > > + &lm85_group_temp_off); > > + if (err) > > + goto err_kfree; > > + } > > + > > /* The ADT7463/68 have an optional VRM 10 mode where pin 21 is used > > as a sixth digital VID input rather than an analog input. */ > > data->vid = lm85_read_value(client, LM85_REG_VID); > > @@ -1345,6 +1367,8 @@ static int lm85_probe(struct i2c_client *client, > > /* Error out and cleanup code */ > > err_remove_files: > > sysfs_remove_group(&client->dev.kobj, &lm85_group); > > + sysfs_remove_group(&client->dev.kobj, &lm85_group_minctl); > > + sysfs_remove_group(&client->dev.kobj, &lm85_group_temp_off); > > sysfs_remove_group(&client->dev.kobj, &lm85_group_in4); > > if (data->type == emc6d100) > > sysfs_remove_group(&client->dev.kobj, &lm85_group_in567); > > For consistency, you should only call sysfs_remove_group for devices > which need it. > I struggled with what to do here, since in4 is also optional. Should be clean now. > > @@ -1358,6 +1382,8 @@ static int lm85_remove(struct i2c_client *client) > > struct lm85_data *data = i2c_get_clientdata(client); > > hwmon_device_unregister(data->hwmon_dev); > > sysfs_remove_group(&client->dev.kobj, &lm85_group); > > + sysfs_remove_group(&client->dev.kobj, &lm85_group_minctl); > > + sysfs_remove_group(&client->dev.kobj, &lm85_group_temp_off); > > sysfs_remove_group(&client->dev.kobj, &lm85_group_in4); > > if (data->type == emc6d100) > > sysfs_remove_group(&client->dev.kobj, &lm85_group_in567); > > This is calling for a separate function to remove the files, doesn't it? > Done. > > @@ -1487,7 +1513,8 @@ static struct lm85_data *lm85_update_device(struct device *dev) > > /* More alarm bits */ > > data->alarms |= lm85_read_value(client, > > EMC6D100_REG_ALARM3) << 16; > > - } else if (data->type == emc6d102 || data->type == emc6d103) { > > + } else if (data->type == emc6d102 || data->type == emc6d103 || > > + data->type == emc6d103s) { > > /* Have to read LSB bits after the MSB ones because > > the reading of the MSB bits has frozen the > > LSBs (backward from the ADM1027). > > @@ -1573,17 +1600,19 @@ static struct lm85_data *lm85_update_device(struct device *dev) > > } > > } > > > > - i = lm85_read_value(client, LM85_REG_AFAN_SPIKE1); > > - data->autofan[0].min_off = (i & 0x20) != 0; > > - data->autofan[1].min_off = (i & 0x40) != 0; > > - data->autofan[2].min_off = (i & 0x80) != 0; > > + if (data->type != emc6d103s) { > > + i = lm85_read_value(client, LM85_REG_AFAN_SPIKE1); > > + data->autofan[0].min_off = (i & 0x20) != 0; > > + data->autofan[1].min_off = (i & 0x40) != 0; > > + data->autofan[2].min_off = (i & 0x80) != 0; > > > > - i = lm85_read_value(client, LM85_REG_AFAN_HYST1); > > - data->zone[0].hyst = i >> 4; > > - data->zone[1].hyst = i & 0x0f; > > + i = lm85_read_value(client, LM85_REG_AFAN_HYST1); > > + data->zone[0].hyst = i >> 4; > > + data->zone[1].hyst = i & 0x0f; > > > > - i = lm85_read_value(client, LM85_REG_AFAN_HYST2); > > - data->zone[2].hyst = i >> 4; > > + i = lm85_read_value(client, LM85_REG_AFAN_HYST2); > > + data->zone[2].hyst = i >> 4; > > + } > > > > data->last_config = jiffies; > > } /* last_config */ > > Other than these minor things, the patch looks sane, and I've tested > that it causes no regression on my EMC6D102. > _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
