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 <[email protected]>
---
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;
};
/*
@@ -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);
+}
+
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);
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);
+
/* Initialize the LM63 chip */
lm63_init_client(new_client);
--
1.5.6.5
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 <[email protected]>
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
Dear Guenter,
> 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.
I don't understand, what structures "data" and "id" you are referring to
here and where the fields driver_data and kind come from. I remember to
have seen such in older kernels, but wasn't that replaced sometime ago?
Cheers
Dirk
On Tue, Feb 08, 2011 at 10:54:52AM -0500, Eibach, Dirk wrote:
>
> Dear Guenter,
>
> > 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.
>
> I don't understand, what structures "data" and "id" you are referring to
> here and where the fields driver_data and kind come from. I remember to
> have seen such in older kernels, but wasn't that replaced sometime ago?
>
static int lm63_probe(struct i2c_client *new_client,
const struct i2c_device_id *id)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
{
struct lm63_data *data;
^^^^^^^^^^^^^^^^^^^^^^
...
/* Set the device type */
data->kind = id->driver_data;
with
struct lm63_data {
...
int kind; // instead of is_lm64
...
};
id is from
static const struct i2c_device_id lm63_id[] = {
{ "lm63", lm63 },
{ "lm64", lm64 },
{ }
};
id->driver_data is thus either lm63 or lm64, depending on the chip type
detected in lm63_detect().
Thanks,
Guenter
Hi Dirk,
On Tue, 8 Feb 2011 16:54:52 +0100, Eibach, Dirk wrote:
>
> Dear Guenter,
>
> > 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.
>
> I don't understand, what structures "data" and "id" you are referring to
data is struct lm63_data and id is const struct i2c_device_id which is
passed as a second parameter to the probe function.
> here and where the fields driver_data and kind come from. I remember to
> have seen such in older kernels, but wasn't that replaced sometime ago?
There were indeed some changes in this area in the past few years, but
what Guenter says still applies to current kernels. Look at lm90_probe()
in drivers/hwmon/lm90.c for an example (amongst may others.)
--
Jean Delvare
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 <[email protected]>
---
Changes since v1:
- avoid calculating offset repeatedly
- gave reasonable names to 8bit temperature accessors
- removed redundant chip id detection
- added comment why temp2_crit cannot be set
drivers/hwmon/lm63.c | 56 +++++++++++++++++++++++++++++++++++++++----------
1 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 776aeb3..2d0cf6d 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 : \
@@ -165,6 +168,8 @@ struct lm63_data {
struct mutex update_lock;
char valid; /* zero until following fields are valid */
unsigned long last_updated; /* in jiffies */
+ int kind;
+ int temp2_offset;
/* registers values */
u8 config, config_fan;
@@ -247,16 +252,34 @@ static ssize_t show_pwm1_enable(struct device *dev, struct device_attribute *dum
return sprintf(buf, "%d\n", data->config_fan & 0x20 ? 1 : 2);
}
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
- char *buf)
+/*
+ * There are 8bit registers for both local(temp1) and remote(temp2) sensor.
+ * For remote sensor registers temp2_offset has to be considered,
+ * for local sensor it must not.
+ * So we need separate 8bit accessors for local and remote sensor.
+ */
+static ssize_t show_local_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);
return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->temp8[attr->index]));
}
-static ssize_t set_temp8(struct device *dev, struct device_attribute *dummy,
- const char *buf, size_t count)
+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);
+ return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->temp8[attr->index])
+ + data->temp2_offset);
+}
+
+static ssize_t set_local_temp8(struct device *dev,
+ struct device_attribute *dummy,
+ const char *buf, size_t count)
{
struct i2c_client *client = to_i2c_client(dev);
struct lm63_data *data = i2c_get_clientdata(client);
@@ -274,7 +297,8 @@ 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]));
+ return sprintf(buf, "%d\n", TEMP11_FROM_REG(data->temp11[attr->index])
+ + data->temp2_offset);
}
static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
@@ -294,7 +318,7 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
int nr = attr->index;
mutex_lock(&data->update_lock);
- data->temp11[nr] = TEMP11_TO_REG(val);
+ data->temp11[nr] = TEMP11_TO_REG(val - data->temp2_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],
@@ -310,6 +334,7 @@ static ssize_t show_temp2_crit_hyst(struct device *dev, struct device_attribute
{
struct lm63_data *data = lm63_update_device(dev);
return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->temp8[2])
+ + data->temp2_offset
- TEMP8_FROM_REG(data->temp2_crit_hyst));
}
@@ -324,7 +349,7 @@ static ssize_t set_temp2_crit_hyst(struct device *dev, struct device_attribute *
long hyst;
mutex_lock(&data->update_lock);
- hyst = TEMP8_FROM_REG(data->temp8[2]) - val;
+ hyst = TEMP8_FROM_REG(data->temp8[2]) + data->temp2_offset - val;
i2c_smbus_write_byte_data(client, LM63_REG_REMOTE_TCRIT_HYST,
HYST_TO_REG(hyst));
mutex_unlock(&data->update_lock);
@@ -355,16 +380,18 @@ static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan,
static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm1, set_pwm1);
static DEVICE_ATTR(pwm1_enable, S_IRUGO, show_pwm1_enable, NULL);
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp8, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 1);
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_local_temp8, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_local_temp8,
+ set_local_temp8, 1);
static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp11, NULL, 0);
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);
+/* temp2_crit can only be set once which should be job of the bootloader */
+static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8,
+ NULL, 2);
static DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_temp2_crit_hyst,
set_temp2_crit_hyst);
@@ -479,7 +506,12 @@ static int lm63_probe(struct i2c_client *new_client,
data->valid = 0;
mutex_init(&data->update_lock);
- /* Initialize the LM63 chip */
+ /* Set the device type */
+ data->kind = id->driver_data;
+ if (data->kind == lm64)
+ data->temp2_offset = 16000;
+
+ /* Initialize chip */
lm63_init_client(new_client);
/* Register sysfs hooks */
--
1.5.6.5
Hi Dirk,
On Wed, Feb 09, 2011 at 04:51:34AM -0500, Dirk Eibach wrote:
[ ... ]
> +/* temp2_crit can only be set once which should be job of the bootloader */
> +static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8,
> + NULL, 2);
That only applies to lm63, though. It can be rewritten anytime on LM64.
That would be a separate patch, though, to not mix improvements with bug fixes.
Otherwise, looks good, so I'll apply it with an added remark that the restriction
only applies to lm63.
Thanks,
Guenter