Hi Guenter,
On Sun, 28 Oct 2012 11:19:56 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> v2: The macro to calculate IT87_REG_TEMP_OFFSET was broken. Use array instead.
> When writing the temperature offset attribute, set the flag to enable it.
> Add documentation describing the new attributes.
>
> Documentation/hwmon/it87 | 9 +++++++++
> drivers/hwmon/it87.c | 25 ++++++++++++++++++++++---
> 2 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
> index 87850d8..92ce617 100644
> --- a/Documentation/hwmon/it87
> +++ b/Documentation/hwmon/it87
> @@ -209,3 +209,12 @@ doesn't use CPU cycles.
> Trip points must be set properly before switching to automatic fan speed
> control mode. The driver will perform basic integrity checks before
> actually switching to automatic control mode.
> +
> +
> +Temperature offset attributes
> +-----------------------------
> +
> +The driver supports temp[1-3]_offset sysfs attributes to adjust the reported
> +temperature for thermal diodes or diode connected thermal transistors.
diode-connected
> +If a temperature sensor is configured for thermistors, the attribute values
> +are ignored.
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 82f7924..fe2cdd4 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -203,6 +203,8 @@ static const u8 IT87_REG_FAN[] = { 0x0d, 0x0e, 0x0f, 0x80, 0x82 };
> static const u8 IT87_REG_FAN_MIN[] = { 0x10, 0x11, 0x12, 0x84, 0x86 };
> static const u8 IT87_REG_FANX[] = { 0x18, 0x19, 0x1a, 0x81, 0x83 };
> static const u8 IT87_REG_FANX_MIN[] = { 0x1b, 0x1c, 0x1d, 0x85, 0x87 };
> +static const u8 IT87_REG_TEMP_OFFSET[] = { 0x56, 0x57, 0x59 };
Not all supported chips have these registers. For example, the IT8712F
rev. D doesn't have registers at 0x56 and 0x57, and register 0x59
apparently adjusts the offset for all 3 temperature channels. Also the
value is expressed as a reference voltage, not an offset in degrees C:
"Thermal Diode Zero Degree Voltage value (default: 0.664V 156h)."
(I don't quite understand how they can make 156h fit in an 8-bit
register, but that a different problem.)
So you will have to check all chips and revisions for support and only
create the sysfs attributes if the chip supports per-channel, degree C
offsets. A quick grep suggests that even the latest IT8705F and IT8712F
chip revisons had a voltage value in these registers, so support would
start with the IT8716F.
> +
> #define IT87_REG_FAN_MAIN_CTRL 0x13
> #define IT87_REG_FAN_CTL 0x14
> #define IT87_REG_PWM(nr) (0x15 + (nr))
> @@ -263,7 +265,7 @@ struct it87_data {
> u16 fan[5]; /* Register values, possibly combined */
> u16 fan_min[5]; /* Register values, possibly combined */
> u8 has_temp; /* Bitfield, temp sensors enabled */
> - s8 temp[3][3]; /* [nr][0]=temp, [1]=min, [2]=max */
> + s8 temp[3][4]; /* [nr][0]=temp, [1]=min, [2]=max, [3]=offset */
> u8 sensor; /* Register value */
> u8 fan_div[3]; /* Register encoding, shifted right */
> u8 vid; /* Register encoding, combined */
> @@ -538,10 +540,19 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
>
> mutex_lock(&data->update_lock);
> data->temp[nr][index] = TEMP_TO_REG(val);
> + if (index == 3) {
> + u8 reg = it87_read_value(data, IT87_REG_BEEP_ENABLE);
> + if (!(reg & 0x80)) {
> + reg |= 0x80;
> + it87_write_value(data, IT87_REG_BEEP_ENABLE, reg);
> + }
> + }
> it87_write_value(data,
> index == 1 ? IT87_REG_TEMP_LOW(nr)
> - : IT87_REG_TEMP_HIGH(nr),
> + : index == 2 ? IT87_REG_TEMP_HIGH(nr)
> + : IT87_REG_TEMP_OFFSET[nr],
This starts being a little difficult to read. Plus you have more tests
on the index value than you could have. What about introducing a local
variable in which you would store the register number, and use that
later in the code? You can use a switch/case for that, this will be a
lot more indentation-friendly.
> data->temp[nr][index]);
> + data->valid = 0;
I see no reason for doing that unconditionally. This is only needed
when changing an offset, not a limit, right?
> mutex_unlock(&data->update_lock);
> return count;
> }
> @@ -549,12 +560,15 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
> static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp, NULL, 0, 0);
> static SENSOR_DEVICE_ATTR_2(temp1_min, S_IRUGOWU, show_temp, set_temp, 0, 1);
> static SENSOR_DEVICE_ATTR_2(temp1_max, S_IRUGOWU, show_temp, set_temp, 0, 2);
> +static SENSOR_DEVICE_ATTR_2(temp1_offset, S_IRUGOWU, show_temp, set_temp, 0, 3);
> static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, 1, 0);
> static SENSOR_DEVICE_ATTR_2(temp2_min, S_IRUGOWU, show_temp, set_temp, 1, 1);
> static SENSOR_DEVICE_ATTR_2(temp2_max, S_IRUGOWU, show_temp, set_temp, 1, 2);
> +static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IRUGOWU, show_temp, set_temp, 1, 3);
> static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, 2, 0);
> static SENSOR_DEVICE_ATTR_2(temp3_min, S_IRUGOWU, show_temp, set_temp, 2, 1);
> static SENSOR_DEVICE_ATTR_2(temp3_max, S_IRUGOWU, show_temp, set_temp, 2, 2);
> +static SENSOR_DEVICE_ATTR_2(temp3_offset, S_IRUGOWU, show_temp, set_temp, 2, 3);
>
> static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> char *buf)
> @@ -1376,11 +1390,12 @@ static const struct attribute_group it87_group_in[9] = {
> { .attrs = it87_attributes_in[8] },
> };
>
> -static struct attribute *it87_attributes_temp[3][6] = {
> +static struct attribute *it87_attributes_temp[3][7] = {
> {
> &sensor_dev_attr_temp1_input.dev_attr.attr,
> &sensor_dev_attr_temp1_max.dev_attr.attr,
> &sensor_dev_attr_temp1_min.dev_attr.attr,
> + &sensor_dev_attr_temp1_offset.dev_attr.attr,
> &sensor_dev_attr_temp1_type.dev_attr.attr,
> &sensor_dev_attr_temp1_alarm.dev_attr.attr,
> NULL
> @@ -1388,6 +1403,7 @@ static struct attribute *it87_attributes_temp[3][6] = {
> &sensor_dev_attr_temp2_input.dev_attr.attr,
> &sensor_dev_attr_temp2_max.dev_attr.attr,
> &sensor_dev_attr_temp2_min.dev_attr.attr,
> + &sensor_dev_attr_temp2_offset.dev_attr.attr,
> &sensor_dev_attr_temp2_type.dev_attr.attr,
> &sensor_dev_attr_temp2_alarm.dev_attr.attr,
> NULL
> @@ -1395,6 +1411,7 @@ static struct attribute *it87_attributes_temp[3][6] = {
> &sensor_dev_attr_temp3_input.dev_attr.attr,
> &sensor_dev_attr_temp3_max.dev_attr.attr,
> &sensor_dev_attr_temp3_min.dev_attr.attr,
> + &sensor_dev_attr_temp3_offset.dev_attr.attr,
> &sensor_dev_attr_temp3_type.dev_attr.attr,
> &sensor_dev_attr_temp3_alarm.dev_attr.attr,
> NULL
As creation of these attributes will be conditional, you'll have to
move them to their own array I'm afraid.
> @@ -2360,6 +2377,8 @@ static struct it87_data *it87_update_device(struct device *dev)
> it87_read_value(data, IT87_REG_TEMP_LOW(i));
> data->temp[i][2] =
> it87_read_value(data, IT87_REG_TEMP_HIGH(i));
> + data->temp[i][3] =
> + it87_read_value(data, IT87_REG_TEMP_OFFSET[i]);
Ideally this would be conditional too.
> }
>
> /* Newer chips don't have clock dividers */
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
