Re: [lm-sensors] [PATCH v2 4/9] hwmon: (it87) Introduce support for tempX_offset sysfs attribute

[Jean Delvare <khali@linux-fr.org>]

Hi Guenter,

On Sun, 28 Oct 2012 11:19:56 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors