Re: [PATCH] hwmon: Consider LM64 temperature offset
From: Guenter Roeck
Date: Tue Feb 08 2011 - 10:29:25 EST
On Tue, Feb 08, 2011 at 08:16:06AM -0500, Dirk Eibach wrote:
> LM64 has 16 degrees Celsius temperature offset on all
> remote sensor registers.
> This was not considered When LM64 support was added to lm63.c.
>
> Signed-off-by: Dirk Eibach <eibach@xxxxxxxx>
Comments inline.
> ---
> drivers/hwmon/lm63.c | 34 +++++++++++++++++++++++++++++-----
> 1 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 776aeb3..87bfefb 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -98,6 +98,9 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
> * value, it uses signed 8-bit values with LSB = 1 degree Celsius.
> * For remote temperature, low and high limits, it uses signed 11-bit values
> * with LSB = 0.125 degree Celsius, left-justified in 16-bit registers.
> + * For LM64 the actual remote diode temperature is 16 degree Celsius higher
> + * than the register reading. Remote temperature setpoints have to be
> + * adapted accordingly.
> */
>
> #define FAN_FROM_REG(reg) ((reg) == 0xFFFC || (reg) == 0 ? 0 : \
> @@ -180,6 +183,7 @@ struct lm63_data {
> 2: remote high limit */
> u8 temp2_crit_hyst;
> u8 alarms;
> + bool is_lm64;
It would be easier to add an offset variable, since all you use it for is
to calculate the offset. Then you can just use data->offset instead of
calculating the offset repeatedly.
> };
>
> /*
> @@ -255,6 +259,17 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->temp8[attr->index]));
> }
>
> +static ssize_t show_remote_temp8(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + struct lm63_data *data = lm63_update_device(dev);
> + int offset = data->is_lm64 ? 16000 : 0;
> + return sprintf(buf, "%d\n",
> + TEMP8_FROM_REG(data->temp8[attr->index]) + offset);
> +}
For consistency, if you should name this function to match
show_temp2_crit_hyst(), ie show_temp2_crit().
> +
> static ssize_t set_temp8(struct device *dev, struct device_attribute *dummy,
> const char *buf, size_t count)
> {
> @@ -274,7 +289,9 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> {
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct lm63_data *data = lm63_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP11_FROM_REG(data->temp11[attr->index]));
> + int offset = data->is_lm64 ? 16000 : 0;
> + return sprintf(buf, "%d\n",
> + TEMP11_FROM_REG(data->temp11[attr->index]) + offset);
> }
>
> static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> @@ -292,9 +309,10 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> struct lm63_data *data = i2c_get_clientdata(client);
> long val = simple_strtol(buf, NULL, 10);
> int nr = attr->index;
> + int offset = data->is_lm64 ? 16000 : 0;
>
> mutex_lock(&data->update_lock);
> - data->temp11[nr] = TEMP11_TO_REG(val);
> + data->temp11[nr] = TEMP11_TO_REG(val - offset);
> i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
> data->temp11[nr] >> 8);
> i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
> @@ -309,7 +327,8 @@ static ssize_t show_temp2_crit_hyst(struct device *dev, struct device_attribute
> char *buf)
> {
> struct lm63_data *data = lm63_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->temp8[2])
> + int offset = data->is_lm64 ? 16000 : 0;
> + return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->temp8[2]) + offset
> - TEMP8_FROM_REG(data->temp2_crit_hyst));
> }
>
> @@ -320,11 +339,12 @@ static ssize_t set_temp2_crit_hyst(struct device *dev, struct device_attribute *
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct lm63_data *data = i2c_get_clientdata(client);
> + int offset = data->is_lm64 ? 16000 : 0;
> long val = simple_strtol(buf, NULL, 10);
> long hyst;
>
> mutex_lock(&data->update_lock);
> - hyst = TEMP8_FROM_REG(data->temp8[2]) - val;
> + hyst = TEMP8_FROM_REG(data->temp8[2]) + offset - val;
> i2c_smbus_write_byte_data(client, LM63_REG_REMOTE_TCRIT_HYST,
> HYST_TO_REG(hyst));
> mutex_unlock(&data->update_lock);
> @@ -364,7 +384,7 @@ static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
> set_temp11, 1);
> static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
> set_temp11, 2);
> -static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_temp8, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8, NULL, 2);
Any idea why the remote critical limit can not be set ?
> static DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_temp2_crit_hyst,
> set_temp2_crit_hyst);
>
> @@ -466,6 +486,7 @@ static int lm63_detect(struct i2c_client *new_client,
> static int lm63_probe(struct i2c_client *new_client,
> const struct i2c_device_id *id)
> {
> + u8 chip_id;
> struct lm63_data *data;
> int err;
>
> @@ -479,6 +500,9 @@ static int lm63_probe(struct i2c_client *new_client,
> data->valid = 0;
> mutex_init(&data->update_lock);
>
> + chip_id = i2c_smbus_read_byte_data(new_client, LM63_REG_CHIP_ID);
> + data->is_lm64 = (chip_id == 0x51);
> +
Chip id is already detected in lm63_detect. You don't need to detect it again.
The more common approach would be something along the line of
data->kind = id->driver_data;
You would then use
if (data->kind == lm64)
throughout the code. In addition to that, you could define
data->kind = id->driver_data;
if (data->kind == lm64)
data->offset = 16000;
which would save you the repeated recalculation of offset as mentioned before.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/