Hi Jean,
On Thu, 2012-01-12 at 16:51 -0500, Jean Delvare wrote:
> Hi Guenter,
>
> On Tue, 10 Jan 2012 09:14:19 -0800, Guenter Roeck wrote:
> > On LM96163, the external temperature sensor type is configurable to
> > either a thermal diode or a 3904 transistor. The chip reports a wrong
> > temperature if misconfigured. Add writable attribute to support sensor type
> > configuration.
> >
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > drivers/hwmon/lm63.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 47 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > index 368db01..f8f4739 100644
> > --- a/drivers/hwmon/lm63.c
> > +++ b/drivers/hwmon/lm63.c
> > @@ -93,6 +93,7 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
> > #define LM63_REG_MAN_ID 0xFE
> > #define LM63_REG_CHIP_ID 0xFF
> >
> > +#define LM96163_REG_TRUTHERM 0x30
> > #define LM96163_REG_REMOTE_TEMP_U_MSB 0x31
> > #define LM96163_REG_REMOTE_TEMP_U_LSB 0x32
> > #define LM96163_REG_CONFIG_ENHANCED 0x45
> > @@ -214,6 +215,7 @@ struct lm63_data {
> > u8 alarms;
> > bool pwm_highres;
> > bool remote_unsigned; /* true if unsigned remote upper limits */
> > + bool trutherm;
> > };
> >
> > static inline int temp8_from_reg(struct lm63_data *data, int nr)
> > @@ -529,6 +531,38 @@ static ssize_t set_update_interval(struct device *dev,
> > return count;
> > }
> >
> > +static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct lm63_data *data = i2c_get_clientdata(client);
> > +
> > + return snprintf(buf, PAGE_SIZE - 1,
>
> This is a little inconsistent, we're using sprintf() everywhere else in
> this driver.
>
ok
> > + data->trutherm ? "3\n" : "2\n");
>
> It's questionable whether you want 3 or rather 1 for TruTherm beta
> compensation mode. While Documentation/hwmon/sysfs-interface says 1 is
> PII/Celeron Diode, I think this is an historical glitch and what it
> really means is "CPU embedded diode". It might be the right time to fix
> that. BTW "sensors" says just "diode" for type 1, with no reference to
> PII/Celeron.
>
Ok with me. Should we change the documentation as well (separate
patch) ?
> > +}
> > +
> > +static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct lm63_data *data = i2c_get_clientdata(client);
> > + unsigned long val;
> > +
> > + if (kstrtoul(buf, 10, &val) < 0)
> > + return -EINVAL;
>
> Here again this is inconsistent with the rest of the driver where the
> error value returned by kstrto*l is passed down to the caller.
>
ok. Just being lazy.
> > + if (val != 2 && val != 3)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->update_lock);
> > + data->trutherm = val == 3;
> > + i2c_smbus_write_byte_data(client, LM96163_REG_TRUTHERM,
> > + data->trutherm ? 0x02 : 0x00);
>
> I'd rather do a read-modify-write cycle even though the datasheet says
> other bits are unused. Datasheets aren't always accurate and future
> chips might add something in this register.
>
ok
> It might make sense to return the error value if the write fails?
>
We could do that, but it should be a separate patch to add error
handling to the entire driver.
> > + data->valid = 0;
> > + mutex_unlock(&data->update_lock);
> > +
> > + return count;
> > +}
> > +
> > static ssize_t show_alarms(struct device *dev, struct device_attribute *dummy,
> > char *buf)
> > {
> > @@ -569,6 +603,8 @@ static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8,
> > static DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_temp2_crit_hyst,
> > set_temp2_crit_hyst);
> >
> > +static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
> > +
> > /* Individual alarm files */
> > static SENSOR_DEVICE_ATTR(fan1_min_alarm, S_IRUGO, show_alarm, NULL, 0);
> > static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL, 1);
> > @@ -728,6 +764,12 @@ static int lm63_probe(struct i2c_client *new_client,
> > if (err)
> > goto exit_remove_files;
> > }
> > + if (data->kind == lm96163) {
> > + err = device_create_file(&new_client->dev,
> > + &dev_attr_temp2_type);
> > + if (err)
> > + goto exit_remove_files;
> > + }
> >
> > data->hwmon_dev = hwmon_device_register(&new_client->dev);
> > if (IS_ERR(data->hwmon_dev)) {
> > @@ -738,6 +780,7 @@ static int lm63_probe(struct i2c_client *new_client,
> > return 0;
> >
> > exit_remove_files:
> > + device_remove_file(&new_client->dev, &dev_attr_temp2_type);
> > sysfs_remove_group(&new_client->dev.kobj, &lm63_group);
> > sysfs_remove_group(&new_client->dev.kobj, &lm63_group_fan1);
> > exit_free:
> > @@ -779,6 +822,9 @@ static void lm63_init_client(struct i2c_client *client)
> > break;
> > case lm96163:
> > data->max_convrate_hz = LM96163_MAX_CONVRATE_HZ;
> > + data->trutherm
> > + = i2c_smbus_read_byte_data(client,
> > + LM96163_REG_TRUTHERM) == 0x02;
>
> Here too I'd play it safe and check the single bit, not the whole
> register value.
>
ok
> > break;
> > default:
> > BUG();
> > @@ -822,6 +868,7 @@ static int lm63_remove(struct i2c_client *client)
> > struct lm63_data *data = i2c_get_clientdata(client);
> >
> > hwmon_device_unregister(data->hwmon_dev);
> > + device_remove_file(&client->dev, &dev_attr_temp2_type);
> > sysfs_remove_group(&client->dev.kobj, &lm63_group);
> > sysfs_remove_group(&client->dev.kobj, &lm63_group_fan1);
> >
>
> Maybe you could also mention this specific feature at the end of
> Documentation/hwmon/lm63 (where the LM96163 differences are listed.)
>
Ok, will do.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
