* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
@ 2005-06-01 18:48 ` Rudolf Marek
2005-06-04 16:48 ` Sebastian Witt
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Rudolf Marek @ 2005-06-01 18:48 UTC (permalink / raw)
To: lm-sensors
Hi Sebastian
I will put some comments into the code. I think those I made are enough
for next iteration.
regards
Rudolf
>
> Adds options to enable the SmartGuardian automatic fan control mode in the
> IT8712 chip.
>
> This patch adds following new /sys-files:
>
> pwmX_auto_enable: Enables automatic mode (manual mode is also working)
Please could you use pwmX_enable for this?
If user will write 0 -> disable PWM
1 -> manual PWM
2 -> Automatic Mode - SmartGuardian
3 -> some other...
> pwmX_limit_off: If this temperature is reached the fan is disabled
> pwmX_limit_start: Temperatures above this will enable the fan
> pwmX_limit_full: Temperatures above this will set the max. fan speed
> pwmX_temp_input: Selects which temperature is used
> pwmX_start: PWM value to start the fan with if it was off
> pwmX_step: PWM step size / temperature C
>
> The fan is controlled linear between limit_start and limit_full by
> increasing
> the PWM value by pwmX_step every temperature increase.
This must go into the i2c/chips/it87 documentation file. Currently you can find the file here:
Of course you can inspire from 2.4 file but be careful to add only stuff that is valid.
http://ftp.sh.cvut.cz/MIRRORS/kernel/linux/kernel/people/gregkh/gregkh-2.6/gregkh-02-i2c/i2c-docs-update-2.patch
Please generate also patch for this file.
>
> Bye,
> Sebastian
>
>
> ------------------------------------------------------------------------
>
> --- it87.c-orig 2005-06-01 00:35:39.000000000 +0200
> +++ it87.c 2005-06-01 00:33:50.000000000 +0200
we need classic:
diff -Naur vanilla-kernel-mm-series/ modified/ > your_patch
Please note we accept patches right now only against -mm- tree because it has some sysfs changes.
-static ssize_t show_xxx(struct device * dev, char * buf)
+static ssize_t show_xxx(struct device * dev, struct device_attribute *attr, char * buf)
Related info: http://lists.lm-sensors.org/pipermail/lm-sensors/2005-May/012251.html
> @@ -29,6 +29,10 @@
> Modified to fix bug with not all alarms enabled.
> Added ability to read battery voltage and select temperature sensor
> type at module load time.
> +
> + se.witt@gmx.net Sebastian Witt 5/31/05
> + Added support for SmartGuardian automatic mode and added
> + module option to force PWM initializing
> */
Please do not add change log, Greg does not like it. It is not maitainable.
> #include <linux/config.h>
> @@ -104,6 +108,9 @@
> /* Update battery voltage after every reading if true */
> static int update_vbat;
>
> +/* Force enabling PWM */
> +static int force_pwm;
> +
I'm a bit confused that I cannot see static int fix_pwm_polarity;
linux-2.6.12-rc5 has something called:
/* Not all BIOSes properly configure the PWM registers */
static int fix_pwm_polarity;
http://lists.lm-sensors.org/pipermail/lm-sensors/2005-March/010860.html
Maybe it is same thing but I will check later.
> /* Chip Type */
>
> static u16 chip_type;
> @@ -151,6 +158,12 @@
>
> #define IT87_REG_CHIPID 0x58
>
> +#define IT87_REG_SG_TEMP_OFF(nr) (0x60 + (nr) * 8)
> +#define IT87_REG_SG_TEMP_START(nr) (0x61 + (nr) * 8)
> +#define IT87_REG_SG_TEMP_FULL(nr) (0x62 + (nr) * 8)
> +#define IT87_REG_SG_START_PWM(nr) (0x63 + (nr) * 8)
> +#define IT87_REG_SG_AM_CTRL(nr) (0x64 + (nr) * 8)
I guess better would be to align via tabs instead of spaces.
> #define IN_TO_REG(val) (SENSORS_LIMIT((((val) + 8)/16),0,255))
> #define IN_FROM_REG(val) ((val) * 16)
>
> @@ -197,21 +210,27 @@
> char valid; /* !=0 if following fields are valid */
> unsigned long last_updated; /* In jiffies */
>
> - u8 in[9]; /* Register value */
> + u8 in[9]; /* Register value */
> u8 in_max[9]; /* Register value */
> u8 in_min[9]; /* Register value */
> - u8 fan[3]; /* Register value */
> + u8 fan[3]; /* Register value */
> u8 fan_min[3]; /* Register value */
> - u8 temp[3]; /* Register value */
> + u8 temp[3]; /* Register value */
> u8 temp_high[3]; /* Register value */
> u8 temp_low[3]; /* Register value */
> - u8 sensor; /* Register value */
> + u8 sensor; /* Register value */
> u8 fan_div[3]; /* Register encoding, shifted right */
> - u8 vid; /* Register encoding, combined */
> + u8 vid; /* Register encoding, combined */
> int vrm;
> - u32 alarms; /* Register encoding, combined */
> + u32 alarms; /* Register encoding, combined */
> u8 fan_main_ctrl; /* Register value */
> - u8 manual_pwm_ctl[3]; /* manual PWM value set by user */
> + u8 fan_ctrl; /* Register value */
> + u8 manual_pwm_ctl[3]; /* manual PWM value set by user */
> + u8 fan_limit_off[3]; /* Register value */
> + u8 fan_limit_start[3]; /* Register value */
> + u8 fan_limit_full[3]; /* Register value */
> + u8 fan_limit_stpwm[3]; /* Register value */
> + u8 fan_am_ctrl[3]; /* Register value */
I dont know if others will like the 4 space tabs.
> };
>
>
> @@ -468,10 +487,45 @@
> struct it87_data *data = it87_update_device(dev);
> return sprintf(buf,"%d\n", (data->fan_main_ctrl & (1 << nr)) ? 1 : 0);
> }
> +static ssize_t show_pwm_auto_enable(struct device *dev, char *buf, int nr)
> +{
> + struct it87_data *data = it87_update_device(dev);
> + return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ? 1 : 0);
> +}
> static ssize_t show_pwm(struct device *dev, char *buf, int nr)
> {
> struct it87_data *data = it87_update_device(dev);
> - return sprintf(buf,"%d\n", data->manual_pwm_ctl[nr]);
> + return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ? 0 : PWM_FROM_REG(data->manual_pwm_ctl[nr]));
See above the pwmX_enable...
> +}
> +static ssize_t show_pwm_limit_off(struct device *dev, char *buf, int nr)
> +{
> + struct it87_data *data = it87_update_device(dev);
> + return sprintf(buf,"%d\n", data->fan_limit_off[nr]);
> +}
> +static ssize_t show_pwm_limit_start(struct device *dev, char *buf, int nr)
> +{
> + struct it87_data *data = it87_update_device(dev);
> + return sprintf(buf,"%d\n", data->fan_limit_start[nr]);
> +}
> +static ssize_t show_pwm_limit_full(struct device *dev, char *buf, int nr)
> +{
> + struct it87_data *data = it87_update_device(dev);
> + return sprintf(buf,"%d\n", data->fan_limit_full[nr]);
> +}
> +static ssize_t show_pwm_start(struct device *dev, char *buf, int nr)
> +{
> + struct it87_data *data = it87_update_device(dev);
> + return sprintf(buf,"%d\n", PWM_FROM_REG(data->fan_limit_stpwm[nr]));
> +}
> +static ssize_t show_pwm_temp_input(struct device *dev, char *buf, int nr)
> +{
> + struct it87_data *data = it87_update_device(dev);
> + return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ? (data->manual_pwm_ctl[nr] & 0x03) + 1 : 0);
> +}
> +static ssize_t show_pwm_step(struct device *dev, char *buf, int nr)
> +{
> + struct it87_data *data = it87_update_device(dev);
> + return sprintf(buf,"%d\n", (data->fan_am_ctrl[nr] & 0x07) ? (1 << ((data->fan_am_ctrl[nr] & 0x07)-1)) : 0);
> }
> static ssize_t set_fan_min(struct device *dev, const char *buf,
> size_t count, int nr)
> @@ -528,9 +582,9 @@
>
> if (val = 0) {
> int tmp;
> - /* make sure the fan is on when in on/off mode */
> + /* make sure the fan is on when in on/off mode and set polarity mode to active high */
Aha see the polarity fixing stuff
> tmp = it87_read_value(client, IT87_REG_FAN_CTL);
> - it87_write_value(client, IT87_REG_FAN_CTL, tmp | (1 << nr));
> + it87_write_value(client, IT87_REG_FAN_CTL, tmp | (1 << nr) | 0x80);
> /* set on/off mode */
> data->fan_main_ctrl &= ~(1 << nr);
> it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
> @@ -538,8 +592,35 @@
> /* set SmartGuardian mode */
> data->fan_main_ctrl |= (1 << nr);
> it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
> - /* set saved pwm value, clear FAN_CTLX PWM mode bit */
> - it87_write_value(client, IT87_REG_PWM(nr), PWM_TO_REG(data->manual_pwm_ctl[nr]));
> + /* set saved pwm value */
> + it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
> + } else
> + return -EINVAL;
> +
> + return count;
> +}
> +static ssize_t set_pwm_auto_enable(struct device *dev, const char *buf,
> + size_t count, int nr)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct it87_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> +
> + if (val = 0) {
> + /* Disable automatic mode, set fans to full speed */
> + data->manual_pwm_ctl[nr] = 0x7f;
> + it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
> + } else if (val = 1) {
> + /* Start PWM control if required */
> + if ((data->fan_main_ctrl & (1 << nr)) = 0) {
> + set_pwm_enable(dev, "1", 1, nr);
> + }
> + /* Enable automatic mode */
> + data->manual_pwm_ctl[nr] |= 0x80;
> + it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
> + /* Enable temp. smoothing, fan spin up time and 16 PWM / C */
more spaces up ... time
> + data->fan_am_ctrl[nr] = 0x80 | 0x18 | 0x05;
> + it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
> } else
> return -EINVAL;
>
> @@ -555,9 +636,97 @@
> if (val < 0 || val > 255)
> return -EINVAL;
>
> - data->manual_pwm_ctl[nr] = val;
> + /* Automatic mode gets disabled if writing a manual PWM value */
> + data->manual_pwm_ctl[nr] = PWM_TO_REG(val);
> if (data->fan_main_ctrl & (1 << nr))
> - it87_write_value(client, IT87_REG_PWM(nr), PWM_TO_REG(data->manual_pwm_ctl[nr]));
> + it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
> +
> + return count;
> +}
> +static ssize_t set_pwm_limit_off(struct device *dev, const char *buf,
> + size_t count, int nr)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct it87_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> +
> + data->fan_limit_off[nr] = val;
> + it87_write_value(client, IT87_REG_SG_TEMP_OFF(nr), data->fan_limit_off[nr]);
> +
> + return count;
> +}
> +static ssize_t set_pwm_limit_start(struct device *dev, const char *buf,
> + size_t count, int nr)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct it87_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> +
> + data->fan_limit_start[nr] = val;
> + it87_write_value(client, IT87_REG_SG_TEMP_START(nr), data->fan_limit_start[nr]);
> +
> + return count;
> +}
> +static ssize_t set_pwm_limit_full(struct device *dev, const char *buf,
> + size_t count, int nr)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct it87_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> +
> + data->fan_limit_full[nr] = val;
> + it87_write_value(client, IT87_REG_SG_TEMP_FULL(nr), data->fan_limit_full[nr]);
> +
> + return count;
> +}
> +static ssize_t set_pwm_start(struct device *dev, const char *buf,
> + size_t count, int nr)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct it87_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> +
> + if (val < 0 || val > 255)
> + return -EINVAL;
> +
> + data->fan_limit_stpwm[nr] = PWM_TO_REG(val);
> + it87_write_value(client, IT87_REG_SG_START_PWM(nr), data->fan_limit_stpwm[nr]);
> +
> + return count;
> +}
> +static ssize_t set_pwm_temp_input(struct device *dev, const char *buf,
> + size_t count, int nr)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct it87_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> +
> + if (val < 1 || val > 3)
> + return -EINVAL;
> +
> + /* Check if automatic control is enabled */
> + if (data->manual_pwm_ctl[nr] & 0x80) {
> + data->manual_pwm_ctl[nr] = 0x80 | (val - 1);
> + it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
> + }
> +
> + return count;
> +}
> +static ssize_t set_pwm_step(struct device *dev, const char *buf,
> + size_t count, int nr)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct it87_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> +
> + if (val < 0 || val > 7)
> + return -EINVAL;
> +
> + data->fan_am_ctrl[nr] &= ~0x07;
> + data->fan_am_ctrl[nr] |= val;
> +
> + it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
> +
>
> return count;
> }
> @@ -601,25 +770,104 @@
> { \
> return show_pwm_enable(dev, buf, offset - 1); \
> } \
> +static ssize_t show_pwm##offset##_auto_enable (struct device *dev, \
> + char *buf) \
> +{ \
> + return show_pwm_auto_enable(dev, buf, offset - 1); \
> +} \
> static ssize_t show_pwm##offset (struct device *dev, char *buf) \
> { \
> return show_pwm(dev, buf, offset - 1); \
> } \
> +static ssize_t show_pwm##offset##_limit_off (struct device *dev, char *buf) \
> +{ \
> + return show_pwm_limit_off(dev, buf, offset - 1); \
> +} \
> +static ssize_t show_pwm##offset##_limit_start (struct device *dev, char *buf) \
> +{ \
> + return show_pwm_limit_start(dev, buf, offset - 1); \
> +} \
> +static ssize_t show_pwm##offset##_limit_full (struct device *dev, char *buf) \
> +{ \
> + return show_pwm_limit_full(dev, buf, offset - 1); \
> +} \
> +static ssize_t show_pwm##offset##_start (struct device *dev, char *buf) \
> +{ \
> + return show_pwm_start(dev, buf, offset - 1); \
> +} \
> +static ssize_t show_pwm##offset##_temp_input (struct device *dev, char *buf) \
> +{ \
> + return show_pwm_temp_input(dev, buf, offset - 1); \
> +} \
> +static ssize_t show_pwm##offset##_step (struct device *dev, char *buf) \
> +{ \
> + return show_pwm_step(dev, buf, offset - 1); \
> +} \
> static ssize_t set_pwm##offset##_enable (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return set_pwm_enable(dev, buf, count, offset - 1); \
> } \
> +static ssize_t set_pwm##offset##_auto_enable (struct device *dev, \
> + const char *buf, size_t count) \
> +{ \
> + return set_pwm_auto_enable(dev, buf, count, offset - 1); \
> +} \
> static ssize_t set_pwm##offset (struct device *dev, \
> const char *buf, size_t count) \
> { \
> return set_pwm(dev, buf, count, offset - 1); \
> } \
> +static ssize_t set_pwm##offset##_limit_off (struct device *dev, \
> + const char *buf, size_t count) \
> +{ \
> + return set_pwm_limit_off(dev, buf, count, offset - 1); \
> +} \
> +static ssize_t set_pwm##offset##_limit_start (struct device *dev, \
> + const char *buf, size_t count) \
> +{ \
> + return set_pwm_limit_start(dev, buf, count, offset - 1); \
> +} \
> +static ssize_t set_pwm##offset##_limit_full (struct device *dev, \
> + const char *buf, size_t count) \
> +{ \
> + return set_pwm_limit_full(dev, buf, count, offset - 1); \
> +} \
> +static ssize_t set_pwm##offset##_start (struct device *dev, \
> + const char *buf, size_t count) \
> +{ \
> + return set_pwm_start(dev, buf, count, offset - 1); \
> +} \
> +static ssize_t set_pwm##offset##_temp_input (struct device *dev, \
> + const char *buf, size_t count) \
> +{ \
> + return set_pwm_temp_input(dev, buf, count, offset - 1); \
> +} \
> +static ssize_t set_pwm##offset##_step (struct device *dev, \
> + const char *buf, size_t count) \
> +{ \
> + return set_pwm_step(dev, buf, count, offset - 1); \
> +} \
> static DEVICE_ATTR(pwm##offset##_enable, S_IRUGO | S_IWUSR, \
> show_pwm##offset##_enable, \
> set_pwm##offset##_enable); \
> +static DEVICE_ATTR(pwm##offset##_auto_enable, S_IRUGO | S_IWUSR, \
> + show_pwm##offset##_auto_enable, \
> + set_pwm##offset##_auto_enable); \
> static DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, \
> - show_pwm##offset , set_pwm##offset );
> + show_pwm##offset , set_pwm##offset ); \
> +static DEVICE_ATTR(pwm##offset##_limit_off, S_IRUGO | S_IWUSR, \
> + show_pwm##offset##_limit_off , set_pwm##offset##_limit_off ); \
> +static DEVICE_ATTR(pwm##offset##_limit_start, S_IRUGO | S_IWUSR, \
> + show_pwm##offset##_limit_start , set_pwm##offset##_limit_start ); \
> +static DEVICE_ATTR(pwm##offset##_limit_full, S_IRUGO | S_IWUSR, \
> + show_pwm##offset##_limit_full , set_pwm##offset##_limit_full ); \
> +static DEVICE_ATTR(pwm##offset##_start, S_IRUGO | S_IWUSR, \
> + show_pwm##offset##_start , set_pwm##offset##_start ); \
> +static DEVICE_ATTR(pwm##offset##_temp_input, S_IRUGO | S_IWUSR, \
> + show_pwm##offset##_temp_input , set_pwm##offset##_temp_input ); \
> +static DEVICE_ATTR(pwm##offset##_step, S_IRUGO | S_IWUSR, \
> + show_pwm##offset##_step , set_pwm##offset##_step );
>
> show_pwm_offset(1);
> show_pwm_offset(2);
> @@ -829,11 +1077,13 @@
> * and polarity set to active low is sign that this is the case so we
> * disable pwm control to protect the user. */
> enable_pwm_interface = 1;
> - tmp = it87_read_value(new_client, IT87_REG_FAN_CTL);
> - if ((tmp & 0x87) = 0) {
> - enable_pwm_interface = 0;
> - dev_info(&new_client->dev,
> - "detected broken BIOS defaults, disabling pwm interface");
> + if (!force_pwm) {
> + tmp = it87_read_value(new_client, IT87_REG_FAN_CTL);
> + if ((tmp & 0x87) = 0) {
> + enable_pwm_interface = 0;
> + dev_info(&new_client->dev,
> + "detected broken BIOS defaults, disabling pwm interface");
> + }
> }
>
> /* Register sysfs hooks */
> @@ -888,9 +1138,30 @@
> device_create_file(&new_client->dev, &dev_attr_pwm1_enable);
> device_create_file(&new_client->dev, &dev_attr_pwm2_enable);
> device_create_file(&new_client->dev, &dev_attr_pwm3_enable);
> + device_create_file(&new_client->dev, &dev_attr_pwm1_auto_enable);
> + device_create_file(&new_client->dev, &dev_attr_pwm2_auto_enable);
> + device_create_file(&new_client->dev, &dev_attr_pwm3_auto_enable);
> device_create_file(&new_client->dev, &dev_attr_pwm1);
> device_create_file(&new_client->dev, &dev_attr_pwm2);
> device_create_file(&new_client->dev, &dev_attr_pwm3);
> + device_create_file(&new_client->dev, &dev_attr_pwm1_limit_off);
> + device_create_file(&new_client->dev, &dev_attr_pwm2_limit_off);
> + device_create_file(&new_client->dev, &dev_attr_pwm3_limit_off);
> + device_create_file(&new_client->dev, &dev_attr_pwm1_limit_start);
> + device_create_file(&new_client->dev, &dev_attr_pwm2_limit_start);
> + device_create_file(&new_client->dev, &dev_attr_pwm3_limit_start);
> + device_create_file(&new_client->dev, &dev_attr_pwm1_limit_full);
> + device_create_file(&new_client->dev, &dev_attr_pwm2_limit_full);
> + device_create_file(&new_client->dev, &dev_attr_pwm3_limit_full);
> + device_create_file(&new_client->dev, &dev_attr_pwm1_start);
> + device_create_file(&new_client->dev, &dev_attr_pwm2_start);
> + device_create_file(&new_client->dev, &dev_attr_pwm3_start);
> + device_create_file(&new_client->dev, &dev_attr_pwm1_temp_input);
> + device_create_file(&new_client->dev, &dev_attr_pwm2_temp_input);
> + device_create_file(&new_client->dev, &dev_attr_pwm3_temp_input);
> + device_create_file(&new_client->dev, &dev_attr_pwm1_step);
> + device_create_file(&new_client->dev, &dev_attr_pwm2_step);
> + device_create_file(&new_client->dev, &dev_attr_pwm3_step);
> }
>
> if (data->type = it8712) {
> @@ -971,17 +1242,6 @@
> {
> int tmp, i;
>
> - /* initialize to sane defaults:
> - * - if the chip is in manual pwm mode, this will be overwritten with
> - * the actual settings on the chip (so in this case, initialization
> - * is not needed)
> - * - if in automatic or on/off mode, we could switch to manual mode,
> - * read the registers and set manual_pwm_ctl accordingly, but currently
> - * this is not implemented, so we initialize to something sane */
> - for (i = 0; i < 3; i++) {
> - data->manual_pwm_ctl[i] = 0xff;
> - }
> -
> /* Check if temperature channnels are reset manually or by some reason */
> tmp = it87_read_value(client, IT87_REG_TEMP_ENABLE);
> if ((tmp & 0x3f) = 0) {
> @@ -1006,24 +1266,21 @@
> it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
> }
>
> - /* Set current fan mode registers and the default settings for the
> - * other mode registers */
> + /* Set current fan mode registers */
> for (i = 0; i < 3; i++) {
> - if (data->fan_main_ctrl & (1 << i)) {
> - /* pwm mode */
> - tmp = it87_read_value(client, IT87_REG_PWM(i));
> - if (tmp & 0x80) {
> - /* automatic pwm - not yet implemented, but
> - * leave the settings made by the BIOS alone
> - * until a change is requested via the sysfs
> - * interface */
> - } else {
> - /* manual pwm */
> - data->manual_pwm_ctl[i] = PWM_FROM_REG(tmp);
> - }
> - }
> + data->manual_pwm_ctl[i] = it87_read_value(client, IT87_REG_PWM(i));
> }
> -
> +
> + /* Get fan control registers for SmartGuardian automatic mode */
> + data->fan_ctrl = it87_read_value(client, IT87_REG_FAN_CTL);
> + for (i = 0; i < 3; i++) {
> + data->fan_limit_off[i] = it87_read_value(client, IT87_REG_SG_TEMP_OFF(i));
> + data->fan_limit_start[i] = it87_read_value(client, IT87_REG_SG_TEMP_START(i));
> + data->fan_limit_full[i] = it87_read_value(client, IT87_REG_SG_TEMP_FULL(i));
> + data->fan_limit_stpwm[i] = it87_read_value(client, IT87_REG_SG_START_PWM(i));
> + data->fan_am_ctrl[i] = it87_read_value(client, IT87_REG_SG_AM_CTRL(i));
> + }
> +
> /* Start monitoring */
> it87_write_value(client, IT87_REG_CONFIG,
> (it87_read_value(client, IT87_REG_CONFIG) & 0x36)
> @@ -1123,9 +1380,12 @@
>
>
> MODULE_AUTHOR("Chris Gauthron <chrisg@0-in.com>");
> +MODULE_AUTHOR("Sebastian Witt <se.witt@gmx.net>");
Are two allowed?
> MODULE_DESCRIPTION("IT8705F, IT8712F, Sis950 driver");
> module_param(update_vbat, bool, 0);
> +module_param(force_pwm, bool, 0);
This does not help if it is undocumented and without any comment.
> MODULE_PARM_DESC(update_vbat, "Update vbat if set else return powerup value");
> +MODULE_PARM_DESC(force_pwm, "Force initializing PWM");
This does not help if it is undocumented and without any comment.
> MODULE_LICENSE("GPL");
>
> module_init(sm_it87_init);
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
2005-06-01 18:48 ` Rudolf Marek
@ 2005-06-04 16:48 ` Sebastian Witt
2005-06-05 22:48 ` Jean Delvare
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Sebastian Witt @ 2005-06-04 16:48 UTC (permalink / raw)
To: lm-sensors
Rudolf Marek wrote:
> Hi Sebastian
>
> I will put some comments into the code. I think those I made are enough
> for next iteration.
>
Hi Rudolf,
thanks for the comments, here's the next one. Patch against 2.6.12-rc5-mm2.
Regards,
Sebastian
-------------- next part --------------
--- linux-2.6.12-rc5-mm2_orig/drivers/i2c/chips/it87.c 2005-06-02 00:48:17.000000000 +0200
+++ linux-2.6.12-rc5-mm2/drivers/i2c/chips/it87.c 2005-06-04 16:21:17.000000000 +0200
@@ -154,6 +154,12 @@
#define IT87_REG_CHIPID 0x58
+#define IT87_REG_SG_TEMP_OFF(nr) (0x60 + (nr) * 8)
+#define IT87_REG_SG_TEMP_START(nr) (0x61 + (nr) * 8)
+#define IT87_REG_SG_TEMP_FULL(nr) (0x62 + (nr) * 8)
+#define IT87_REG_SG_START_PWM(nr) (0x63 + (nr) * 8)
+#define IT87_REG_SG_AM_CTRL(nr) (0x64 + (nr) * 8)
+
#define IN_TO_REG(val) (SENSORS_LIMIT((((val) + 8)/16),0,255))
#define IN_FROM_REG(val) ((val) * 16)
@@ -213,6 +219,11 @@
u32 alarms; /* Register encoding, combined */
u8 fan_main_ctrl; /* Register value */
u8 manual_pwm_ctl[3]; /* manual PWM value set by user */
+ u8 fan_limit_off[3]; /* Register value */
+ u8 fan_limit_start[3]; /* Register value */
+ u8 fan_limit_full[3]; /* Register value */
+ u8 fan_limit_stpwm[3]; /* Register value */
+ u8 fan_am_ctrl[3]; /* Register value */
};
@@ -486,13 +497,52 @@
}
static ssize_t show_pwm_enable(struct device *dev, char *buf, int nr)
{
+ int val;
+
struct it87_data *data = it87_update_device(dev);
- return sprintf(buf,"%d\n", (data->fan_main_ctrl & (1 << nr)) ? 1 : 0);
+
+ /* PWM enabled or disabled */
+ val = (data->fan_main_ctrl & (1 << nr)) ? 1 : 0;
+ /* Automatic mode - SmartGuardian */
+ val += (val && (data->manual_pwm_ctl[nr] & 0x80)) ? 1 : 0;
+
+ return sprintf(buf,"%d\n", val);
}
static ssize_t show_pwm(struct device *dev, char *buf, int nr)
{
struct it87_data *data = it87_update_device(dev);
- return sprintf(buf,"%d\n", data->manual_pwm_ctl[nr]);
+ /* Return 0 if automatic mode is enabled */
+ return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ? 0 : PWM_FROM_REG(data->manual_pwm_ctl[nr]));
+}
+static ssize_t show_pwm_limit_off(struct device *dev, char *buf, int nr)
+{
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_limit_off[nr]);
+}
+static ssize_t show_pwm_limit_start(struct device *dev, char *buf, int nr)
+{
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_limit_start[nr]);
+}
+static ssize_t show_pwm_limit_full(struct device *dev, char *buf, int nr)
+{
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_limit_full[nr]);
+}
+static ssize_t show_pwm_start(struct device *dev, char *buf, int nr)
+{
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", PWM_FROM_REG(data->fan_limit_stpwm[nr]));
+}
+static ssize_t show_pwm_temp_input(struct device *dev, char *buf, int nr)
+{
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ? (data->manual_pwm_ctl[nr] & 0x03) + 1 : 0);
+}
+static ssize_t show_pwm_step(struct device *dev, char *buf, int nr)
+{
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_am_ctrl[nr] & 0x07);
}
static ssize_t set_fan_min(struct device *dev, const char *buf,
size_t count, int nr)
@@ -564,12 +614,28 @@
/* set on/off mode */
data->fan_main_ctrl &= ~(1 << nr);
it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
- } else if (val = 1) {
+ } else if ((val = 1) || (val = 2)) {
/* set SmartGuardian mode */
data->fan_main_ctrl |= (1 << nr);
it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
- /* set saved pwm value, clear FAN_CTLX PWM mode bit */
- it87_write_value(client, IT87_REG_PWM(nr), PWM_TO_REG(data->manual_pwm_ctl[nr]));
+ if (val = 2) {
+ /* Set automatic SmartGuardian mode */
+ data->manual_pwm_ctl[nr] |= 0x80;
+ it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
+ /* Enable temperature smoothing and fan spin up time */
+ data->fan_am_ctrl[nr] = 0x80 | 0x18;
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ /* If start pwm and pwm step size not set, apply safe values */
+ if (data->fan_limit_stpwm[nr] = 0) {
+ data->fan_limit_stpwm[nr] = PWM_TO_REG(128);
+ it87_write_value(client, IT87_REG_SG_START_PWM(nr), data->fan_limit_stpwm[nr]);
+ }
+ if ((data->fan_am_ctrl[nr] & 0x07) = 0) {
+ /* 16 PWM values / K */
+ data->fan_am_ctrl[nr] |= 0x05;
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ }
+ }
} else {
up(&data->update_lock);
return -EINVAL;
@@ -595,6 +661,93 @@
up(&data->update_lock);
return count;
}
+static ssize_t set_pwm_limit_off(struct device *dev, const char *buf,
+ size_t count, int nr)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ data->fan_limit_off[nr] = val;
+ it87_write_value(client, IT87_REG_SG_TEMP_OFF(nr), data->fan_limit_off[nr]);
+
+ return count;
+}
+static ssize_t set_pwm_limit_start(struct device *dev, const char *buf,
+ size_t count, int nr)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ data->fan_limit_start[nr] = val;
+ it87_write_value(client, IT87_REG_SG_TEMP_START(nr), data->fan_limit_start[nr]);
+
+ return count;
+}
+static ssize_t set_pwm_limit_full(struct device *dev, const char *buf,
+ size_t count, int nr)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ data->fan_limit_full[nr] = val;
+ it87_write_value(client, IT87_REG_SG_TEMP_FULL(nr), data->fan_limit_full[nr]);
+
+ return count;
+}
+static ssize_t set_pwm_start(struct device *dev, const char *buf,
+ size_t count, int nr)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ if (val < 0 || val > 255)
+ return -EINVAL;
+
+ data->fan_limit_stpwm[nr] = PWM_TO_REG(val);
+ it87_write_value(client, IT87_REG_SG_START_PWM(nr), data->fan_limit_stpwm[nr]);
+
+ return count;
+}
+static ssize_t set_pwm_temp_input(struct device *dev, const char *buf,
+ size_t count, int nr)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ if (val < 1 || val > 3)
+ return -EINVAL;
+
+ /* Check if automatic control is enabled */
+ if (data->manual_pwm_ctl[nr] & 0x80) {
+ data->manual_pwm_ctl[nr] = 0x80 | (val - 1);
+ it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
+ }
+
+ return count;
+}
+static ssize_t set_pwm_step(struct device *dev, const char *buf,
+ size_t count, int nr)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ if (val < 0 || val > 7)
+ return -EINVAL;
+
+ data->fan_am_ctrl[nr] &= ~0x07;
+ data->fan_am_ctrl[nr] |= val;
+
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+
+
+ return count;
+}
#define show_fan_offset(offset) \
static ssize_t show_fan_##offset (struct device *dev, struct device_attribute *attr, char *buf) \
@@ -639,6 +792,30 @@
{ \
return show_pwm(dev, buf, offset - 1); \
} \
+static ssize_t show_pwm##offset##_limit_off (struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+ return show_pwm_limit_off(dev, buf, offset - 1); \
+} \
+static ssize_t show_pwm##offset##_limit_start (struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+ return show_pwm_limit_start(dev, buf, offset - 1); \
+} \
+static ssize_t show_pwm##offset##_limit_full (struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+ return show_pwm_limit_full(dev, buf, offset - 1); \
+} \
+static ssize_t show_pwm##offset##_start (struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+ return show_pwm_start(dev, buf, offset - 1); \
+} \
+static ssize_t show_pwm##offset##_temp_input (struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+ return show_pwm_temp_input(dev, buf, offset - 1); \
+} \
+static ssize_t show_pwm##offset##_step (struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+ return show_pwm_step(dev, buf, offset - 1); \
+} \
static ssize_t set_pwm##offset##_enable (struct device *dev, struct device_attribute *attr, \
const char *buf, size_t count) \
{ \
@@ -649,11 +826,55 @@
{ \
return set_pwm(dev, buf, count, offset - 1); \
} \
+static ssize_t set_pwm##offset##_limit_off (struct device *dev, struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_pwm_limit_off(dev, buf, count, offset - 1); \
+} \
+static ssize_t set_pwm##offset##_limit_start (struct device *dev, struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_pwm_limit_start(dev, buf, count, offset - 1); \
+} \
+static ssize_t set_pwm##offset##_limit_full (struct device *dev, struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_pwm_limit_full(dev, buf, count, offset - 1); \
+} \
+static ssize_t set_pwm##offset##_start (struct device *dev, struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_pwm_start(dev, buf, count, offset - 1); \
+} \
+static ssize_t set_pwm##offset##_temp_input (struct device *dev, struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_pwm_temp_input(dev, buf, count, offset - 1); \
+} \
+static ssize_t set_pwm##offset##_step (struct device *dev, struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_pwm_step(dev, buf, count, offset - 1); \
+} \
static DEVICE_ATTR(pwm##offset##_enable, S_IRUGO | S_IWUSR, \
show_pwm##offset##_enable, \
set_pwm##offset##_enable); \
static DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, \
- show_pwm##offset , set_pwm##offset );
+ show_pwm##offset , set_pwm##offset ); \
+static DEVICE_ATTR(pwm##offset##_limit_off, S_IRUGO | S_IWUSR, \
+ show_pwm##offset##_limit_off , set_pwm##offset##_limit_off ); \
+static DEVICE_ATTR(pwm##offset##_limit_start, S_IRUGO | S_IWUSR, \
+ show_pwm##offset##_limit_start , set_pwm##offset##_limit_start ); \
+static DEVICE_ATTR(pwm##offset##_limit_full, S_IRUGO | S_IWUSR, \
+ show_pwm##offset##_limit_full , set_pwm##offset##_limit_full ); \
+static DEVICE_ATTR(pwm##offset##_start, S_IRUGO | S_IWUSR, \
+ show_pwm##offset##_start , set_pwm##offset##_start ); \
+static DEVICE_ATTR(pwm##offset##_temp_input, S_IRUGO | S_IWUSR, \
+ show_pwm##offset##_temp_input , set_pwm##offset##_temp_input ); \
+static DEVICE_ATTR(pwm##offset##_step, S_IRUGO | S_IWUSR, \
+ show_pwm##offset##_step , set_pwm##offset##_step );
+
+
show_pwm_offset(1);
show_pwm_offset(2);
@@ -915,6 +1136,24 @@
device_create_file(&new_client->dev, &dev_attr_pwm1);
device_create_file(&new_client->dev, &dev_attr_pwm2);
device_create_file(&new_client->dev, &dev_attr_pwm3);
+ device_create_file(&new_client->dev, &dev_attr_pwm1_limit_off);
+ device_create_file(&new_client->dev, &dev_attr_pwm2_limit_off);
+ device_create_file(&new_client->dev, &dev_attr_pwm3_limit_off);
+ device_create_file(&new_client->dev, &dev_attr_pwm1_limit_start);
+ device_create_file(&new_client->dev, &dev_attr_pwm2_limit_start);
+ device_create_file(&new_client->dev, &dev_attr_pwm3_limit_start);
+ device_create_file(&new_client->dev, &dev_attr_pwm1_limit_full);
+ device_create_file(&new_client->dev, &dev_attr_pwm2_limit_full);
+ device_create_file(&new_client->dev, &dev_attr_pwm3_limit_full);
+ device_create_file(&new_client->dev, &dev_attr_pwm1_start);
+ device_create_file(&new_client->dev, &dev_attr_pwm2_start);
+ device_create_file(&new_client->dev, &dev_attr_pwm3_start);
+ device_create_file(&new_client->dev, &dev_attr_pwm1_temp_input);
+ device_create_file(&new_client->dev, &dev_attr_pwm2_temp_input);
+ device_create_file(&new_client->dev, &dev_attr_pwm3_temp_input);
+ device_create_file(&new_client->dev, &dev_attr_pwm1_step);
+ device_create_file(&new_client->dev, &dev_attr_pwm2_step);
+ device_create_file(&new_client->dev, &dev_attr_pwm3_step);
}
if (data->type = it8712) {
@@ -1045,17 +1284,6 @@
{
int tmp, i;
- /* initialize to sane defaults:
- * - if the chip is in manual pwm mode, this will be overwritten with
- * the actual settings on the chip (so in this case, initialization
- * is not needed)
- * - if in automatic or on/off mode, we could switch to manual mode,
- * read the registers and set manual_pwm_ctl accordingly, but currently
- * this is not implemented, so we initialize to something sane */
- for (i = 0; i < 3; i++) {
- data->manual_pwm_ctl[i] = 0xff;
- }
-
/* Check if temperature channnels are reset manually or by some reason */
tmp = it87_read_value(client, IT87_REG_TEMP_ENABLE);
if ((tmp & 0x3f) = 0) {
@@ -1080,24 +1308,20 @@
it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
}
- /* Set current fan mode registers and the default settings for the
- * other mode registers */
+ /* Set current fan mode registers */
for (i = 0; i < 3; i++) {
- if (data->fan_main_ctrl & (1 << i)) {
- /* pwm mode */
- tmp = it87_read_value(client, IT87_REG_PWM(i));
- if (tmp & 0x80) {
- /* automatic pwm - not yet implemented, but
- * leave the settings made by the BIOS alone
- * until a change is requested via the sysfs
- * interface */
- } else {
- /* manual pwm */
- data->manual_pwm_ctl[i] = PWM_FROM_REG(tmp);
- }
- }
+ data->manual_pwm_ctl[i] = it87_read_value(client, IT87_REG_PWM(i));
}
-
+
+ /* Get fan control registers for SmartGuardian automatic mode */
+ for (i = 0; i < 3; i++) {
+ data->fan_limit_off[i] = it87_read_value(client, IT87_REG_SG_TEMP_OFF(i));
+ data->fan_limit_start[i] = it87_read_value(client, IT87_REG_SG_TEMP_START(i));
+ data->fan_limit_full[i] = it87_read_value(client, IT87_REG_SG_TEMP_FULL(i));
+ data->fan_limit_stpwm[i] = it87_read_value(client, IT87_REG_SG_START_PWM(i));
+ data->fan_am_ctrl[i] = it87_read_value(client, IT87_REG_SG_AM_CTRL(i));
+ }
+
/* Start monitoring */
it87_write_value(client, IT87_REG_CONFIG,
(it87_read_value(client, IT87_REG_CONFIG) & 0x36)
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
2005-06-01 18:48 ` Rudolf Marek
2005-06-04 16:48 ` Sebastian Witt
@ 2005-06-05 22:48 ` Jean Delvare
2005-06-10 13:11 ` Sebastian Witt
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Jean Delvare @ 2005-06-05 22:48 UTC (permalink / raw)
To: lm-sensors
Hi Sebastian,
> thanks for the comments, here's the next one. Patch against
> 2.6.12-rc5-mm2.
Thanks for taking Rudolf's comment into account, and for your work on
the it87 driver in general. Several users have been asking for the Smart
Guardian feature already.
However, there are a few problems with your patch.
First, there is a big pending patch for it87 which updates the driver to
make use of the new sysfs callbacks. I wouldn't want your patch to
collide with this one, so I'd like you to build your patch on top of
this one.
http://lists.lm-sensors.org/pipermail/lm-sensors/2005-June/012597.html
This will also have the advantage that you'll see how the new sysfs
callbacks can be written, and this should hopefully make your code much
more simple.
Second, your interface doesn't quite comply with the standard automatic
PWM interface as defined in Documentation/i2c/sysfs-interface. Please
take a look. The interface basically defines trip points (PWM,
temperature "coordinates"). You should come to something similar to what
you have already by just renaming a few files, except for the slope/step
concept which won't fit directly in, so I think you'll have to add some
arithmetics (compute the slope from the requested trip point
"coordinates", and vice-versa).
Third, the old datasheets for the IT8712F and IT8705F suggest that the
first revisions of both chips had a different SmartGuardian
implementation, trip-point based. If this is confirmed, then you will
have to check the revision of the chip before enabling your interface.
Which revision to you have yourself? I'll try to experiment on my
IT8705F rev.2.
Can you provide a dump of your chip at startup (before loading the it87
driver)?
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
` (2 preceding siblings ...)
2005-06-05 22:48 ` Jean Delvare
@ 2005-06-10 13:11 ` Sebastian Witt
2005-06-10 19:42 ` Jean Delvare
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Sebastian Witt @ 2005-06-10 13:11 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Jean Delvare wrote:
>
> First, there is a big pending patch for it87 which updates the driver to
> make use of the new sysfs callbacks. I wouldn't want your patch to
> collide with this one, so I'd like you to build your patch on top of
> this one.
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2005-June/012597.html
>
> This will also have the advantage that you'll see how the new sysfs
> callbacks can be written, and this should hopefully make your code much
> more simple.
Ok, is this patch against 2.6.12-rc5? Because it fails to apply.
> Second, your interface doesn't quite comply with the standard automatic
> PWM interface as defined in Documentation/i2c/sysfs-interface. Please
> take a look. The interface basically defines trip points (PWM,
> temperature "coordinates"). You should come to something similar to what
> you have already by just renaming a few files, except for the slope/step
> concept which won't fit directly in, so I think you'll have to add some
> arithmetics (compute the slope from the requested trip point
> "coordinates", and vice-versa).
Yes, no problem.
> Third, the old datasheets for the IT8712F and IT8705F suggest that the
> first revisions of both chips had a different SmartGuardian
> implementation, trip-point based. If this is confirmed, then you will
> have to check the revision of the chip before enabling your interface.
> Which revision to you have yourself? I'll try to experiment on my
> IT8705F rev.2.
it87: Found IT8705F chip at 0x290, revision 3
> Can you provide a dump of your chip at startup (before loading the it87
> driver)?
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: 13 00 ff 00 ff ff fd 00 00 00 3f 5b 00 48 ff ff
10: ff ff ff 30 00 00 00 00 00 ff ff ff ff ff ff ff
20: 44 a1 cb b6 b6 ba ca b3 ff 23 16 03 51 c1 c1 c1
30: ff ff ff ff ff ff ff ff ff fd ff ff ff ff ff ff
40: 3f 7f 3f 7f 3f 7f ff ff 2d ff ff ff ff ff ff ff
50: ff 2a 7f 7f 7f ff 5d 89 90 5d f9 12 80 00 00 00
60: 7f 7f 7f 00 00 ff ff ff 7f 7f 7f 00 00 ff ff ff
70: 7f 7f 7f 00 00 ff ff ff ff ff ff ff ff ff ff ff
80: 13 00 00 00 ff ff fd 00 00 00 3f 5b 00 48 ff ff
90: ff ff ff 30 00 00 00 00 00 ff ff ff ff ff ff ff
a0: 44 a1 cb b6 b6 ba ca b3 ff 23 16 03 51 c1 c1 c1
b0: ff ff ff ff ff ff ff ff ff fd ff ff ff ff ff ff
c0: 3f 7f 3f 7f 3f 7f ff ff 2d ff ff ff ff ff ff ff
d0: ff 2a 7f 7f 7f ff 5d 89 90 5d f9 12 80 00 00 00
e0: 7f 7f 7f 00 00 ff ff ff 7f 7f 7f 00 00 ff ff ff
f0: 7f 7f 7f 00 00 ff ff ff ff ff ff ff ff ff ff ff
Regards,
Sebastian
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
` (3 preceding siblings ...)
2005-06-10 13:11 ` Sebastian Witt
@ 2005-06-10 19:42 ` Jean Delvare
2005-06-14 22:33 ` Sebastian Witt
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Jean Delvare @ 2005-06-10 19:42 UTC (permalink / raw)
To: lm-sensors
Hi Sebastian,
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2005-June/012597.html
> >
> > This will also have the advantage that you'll see how the new sysfs
> > callbacks can be written, and this should hopefully make your code
> > much more simple.
>
> Ok, is this patch against 2.6.12-rc5? Because it fails to apply.
It was against Greg's current i2c tree as I wrote it. The easier for you
would be to start from 2.6.12-rc6 + this patch:
http://khali.linux-fr.org/devel/i2c/linux-2.6/linux-2.6.12-rc6-i2c.patch.gz
Or the latest -mm, at your option (but be aware that
linux-2.6.12-rc6-mm1 has a few known issues WRT hardware monitoring.)
Both include the all the latest it87 patches.
> it87: Found IT8705F chip at 0x290, revision 3
Hey, lucky you. I'd like to have this one, but unfortunately mine is
revision 2 only. So, yours implements the newer auto-pwm model, while
mine implements the old one. You might even get VID readings if your
chip was wired for this.
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
` (4 preceding siblings ...)
2005-06-10 19:42 ` Jean Delvare
@ 2005-06-14 22:33 ` Sebastian Witt
2005-06-17 15:06 ` Sebastian Witt
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Sebastian Witt @ 2005-06-14 22:33 UTC (permalink / raw)
To: lm-sensors
Hi,
Jean Delvare wrote:
> It was against Greg's current i2c tree as I wrote it. The easier for you
> would be to start from 2.6.12-rc6 + this patch:
>
> http://khali.linux-fr.org/devel/i2c/linux-2.6/linux-2.6.12-rc6-i2c.patch.gz
>
> Or the latest -mm, at your option (but be aware that
> linux-2.6.12-rc6-mm1 has a few known issues WRT hardware monitoring.)
>
> Both include the all the latest it87 patches.
>
Ok, here's a patch against 2.6.12-rc6.
>>it87: Found IT8705F chip at 0x290, revision 3
>
> Hey, lucky you. I'd like to have this one, but unfortunately mine is
> revision 2 only. So, yours implements the newer auto-pwm model, while
> mine implements the old one. You might even get VID readings if your
> chip was wired for this.
I've now added only support for the auto-pwm with slope setting and a check for
revision 3.
Bye,
Sebastian
-------------- next part --------------
--- linux-2.6.12-rc6_orig/drivers/i2c/chips/it87.c 2005-06-15 00:12:22.000000000 +0200
+++ linux-2.6.12-rc6/drivers/i2c/chips/it87.c 2005-06-15 00:20:58.000000000 +0200
@@ -111,6 +111,7 @@
/* Chip Type */
static u16 chip_type;
+static u8 chip_revision;
/* Many IT87 constants specified below */
@@ -155,6 +156,12 @@
#define IT87_REG_CHIPID 0x58
+#define IT87_REG_SG_TEMP_OFF(nr) (0x60 + (nr) * 8)
+#define IT87_REG_SG_TEMP_START(nr) (0x61 + (nr) * 8)
+#define IT87_REG_SG_TEMP_FULL(nr) (0x62 + (nr) * 8)
+#define IT87_REG_SG_START_PWM(nr) (0x63 + (nr) * 8)
+#define IT87_REG_SG_AM_CTRL(nr) (0x64 + (nr) * 8)
+
#define IN_TO_REG(val) (SENSORS_LIMIT((((val) + 8)/16),0,255))
#define IN_FROM_REG(val) ((val) * 16)
@@ -185,6 +192,7 @@
}
#define DIV_FROM_REG(val) (1 << (val))
+#define SLOPE_FROM_REG(val) ((val) ? (1 << (val-1)) : 0)
/* For each registered IT87, we need to keep some data in memory. That
data is pointed to by it87_list[NR]->data. The structure itself is
@@ -214,6 +222,12 @@
u32 alarms; /* Register encoding, combined */
u8 fan_main_ctrl; /* Register value */
u8 manual_pwm_ctl[3]; /* manual PWM value set by user */
+ u8 fan_limit_off[3]; /* Register value */
+ u8 fan_limit_start[3]; /* Register value */
+ u8 fan_limit_full[3]; /* Register value */
+ u8 fan_limit_stpwm[3]; /* Register value */
+ u8 fan_am_ctrl[3]; /* Register value */
+ u8 fan_point2_pwm[3]; /* Used for slope calculation */
};
@@ -454,6 +468,31 @@
show_sensor_offset(2);
show_sensor_offset(3);
+static void calc_slope(struct it87_data *data, int nr)
+{
+ u8 val = data->fan_point2_pwm[nr];
+
+ if (data->fan_limit_full[nr] <= data->fan_limit_start[nr])
+ val = 14;
+ else {
+ val -= data->fan_limit_stpwm[nr];
+ val /= (data->fan_limit_full[nr] - data->fan_limit_start[nr]);
+ }
+
+ data->fan_am_ctrl[nr] &= ~0x03;
+ if (val > 48)
+ data->fan_am_ctrl[nr] |= 0x07; /* 64 PWM / K */
+ else if (val > 24)
+ data->fan_am_ctrl[nr] |= 0x06; /* 32 PWM / K */
+ else if (val > 12)
+ data->fan_am_ctrl[nr] |= 0x05; /* 16 PWM / K */
+ else if (val > 6)
+ data->fan_am_ctrl[nr] |= 0x04; /* 8 PWM / K */
+ else if (val > 3)
+ data->fan_am_ctrl[nr] |= 0x03; /* 4 PWM / K */
+ else
+ data->fan_am_ctrl[nr] |= 0x02; /* 2 PWM / K */
+}
/* 3 Fans */
static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -489,9 +528,15 @@
{
struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
int nr = sensor_attr->index;
+ int val;
struct it87_data *data = it87_update_device(dev);
- return sprintf(buf,"%d\n", (data->fan_main_ctrl & (1 << nr)) ? 1 : 0);
+ /* PWM enabled or disabled */
+ val = (data->fan_main_ctrl & (1 << nr)) ? 1 : 0;
+ /* Automatic mode - SmartGuardian */
+ val += (val && (data->manual_pwm_ctl[nr] & 0x80)) ? 1 : 0;
+
+ return sprintf(buf,"%d\n", val);
}
static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -500,7 +545,64 @@
int nr = sensor_attr->index;
struct it87_data *data = it87_update_device(dev);
- return sprintf(buf,"%d\n", data->manual_pwm_ctl[nr]);
+ /* Return 0 if automatic mode is enabled */
+ return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ? 0 : PWM_FROM_REG(data->manual_pwm_ctl[nr]));
+}
+static ssize_t show_pwm_auto_channels_temp(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct it87_data *data = it87_update_device(dev);
+
+ return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ?
+ (1 << (data->manual_pwm_ctl[nr] & 0x03)): 0);
+}
+static ssize_t show_pwm_auto_point1_pwm(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", PWM_FROM_REG(data->fan_limit_stpwm[nr]));
+}
+static ssize_t show_pwm_auto_point1_temp(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_limit_start[nr]);
+}
+static ssize_t show_pwm_auto_point1_temp_hyst(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_limit_off[nr]);
+}
+static ssize_t show_pwm_auto_point2_pwm(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_point2_pwm[nr]);
+}
+static ssize_t show_pwm_auto_point2_temp(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct it87_data *data = it87_update_device(dev);
+ return sprintf(buf,"%d\n", data->fan_limit_full[nr]);
}
static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
@@ -581,12 +683,33 @@
/* set on/off mode */
data->fan_main_ctrl &= ~(1 << nr);
it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
- } else if (val = 1) {
+ } else if ((val = 1) || (val = 2)) {
/* set SmartGuardian mode */
data->fan_main_ctrl |= (1 << nr);
it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
- /* set saved pwm value, clear FAN_CTLX PWM mode bit */
- it87_write_value(client, IT87_REG_PWM(nr), PWM_TO_REG(data->manual_pwm_ctl[nr]));
+ if ((val = 2) && (chip_revision = 0x03)) {
+ /* Set automatic SmartGuardian mode */
+ data->manual_pwm_ctl[nr] |= 0x80;
+ it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
+ /* Enable temperature smoothing and fan spin up time */
+ data->fan_am_ctrl[nr] = 0x80 | 0x18;
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ /* If start pwm and pwm step size not set, apply safe values */
+ if (data->fan_limit_stpwm[nr] = 0) {
+ data->fan_limit_stpwm[nr] = PWM_TO_REG(128);
+ it87_write_value(client, IT87_REG_SG_START_PWM(nr), data->fan_limit_stpwm[nr]);
+ }
+ if ((data->fan_am_ctrl[nr] & 0x07) = 0) {
+ /* 16 PWM values / K */
+ data->fan_am_ctrl[nr] |= 0x05;
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ }
+ }
+ else {
+ /* Disable automatic SmartGuardian mode */
+ data->manual_pwm_ctl[nr] &= ~0x80;
+ it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
+ }
} else {
up(&data->update_lock);
return -EINVAL;
@@ -615,7 +738,123 @@
up(&data->update_lock);
return count;
}
+static ssize_t set_pwm_auto_point1_pwm(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ if (val < 0 || val > 255)
+ return -EINVAL;
+
+ down(&data->update_lock);
+ data->fan_limit_stpwm[nr] = PWM_TO_REG(val);
+ calc_slope(data, nr);
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ it87_write_value(client, IT87_REG_SG_START_PWM(nr), data->fan_limit_stpwm[nr]);
+ up(&data->update_lock);
+ return count;
+}
+static ssize_t set_pwm_auto_point1_temp(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ down(&data->update_lock);
+ data->fan_limit_start[nr] = val;
+ calc_slope(data, nr);
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ it87_write_value(client, IT87_REG_SG_TEMP_START(nr), data->fan_limit_start[nr]);
+ up(&data->update_lock);
+ return count;
+}
+static ssize_t set_pwm_auto_point1_temp_hyst(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ down(&data->update_lock);
+ data->fan_limit_off[nr] = val;
+ it87_write_value(client, IT87_REG_SG_TEMP_OFF(nr), data->fan_limit_off[nr]);
+ up(&data->update_lock);
+ return count;
+}
+static ssize_t set_pwm_auto_point2_pwm(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ if (val < 0 || val > 255)
+ return -EINVAL;
+
+ down(&data->update_lock);
+ data->fan_point2_pwm[nr] = val;
+ calc_slope(data, nr);
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ up(&data->update_lock);
+ return count;
+}
+static ssize_t set_pwm_auto_point2_temp(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ down(&data->update_lock);
+ data->fan_limit_full[nr] = val;
+ calc_slope(data, nr);
+ it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
+ it87_write_value(client, IT87_REG_SG_TEMP_FULL(nr), data->fan_limit_full[nr]);
+ up(&data->update_lock);
+ return count;
+}
+static ssize_t set_pwm_auto_channels_temp(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+
+ struct i2c_client *client = to_i2c_client(dev);
+ struct it87_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+
+ if (val < 0 || val > 4)
+ return -EINVAL;
+
+ /* Check if automatic control is enabled */
+ if (data->manual_pwm_ctl[nr] & 0x80) {
+ down(&data->update_lock);
+ data->manual_pwm_ctl[nr] = 0x80 | (val >> 1);
+ it87_write_value(client, IT87_REG_PWM(nr), data->manual_pwm_ctl[nr]);
+ up(&data->update_lock);
+ }
+
+ return count;
+}
+
#define show_fan_offset(offset) \
static SENSOR_DEVICE_ATTR(fan##offset##_input, S_IRUGO, \
show_fan, NULL, offset - 1); \
@@ -632,7 +871,19 @@
static SENSOR_DEVICE_ATTR(pwm##offset##_enable, S_IRUGO | S_IWUSR, \
show_pwm_enable, set_pwm_enable, offset - 1); \
static SENSOR_DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, \
- show_pwm, set_pwm, offset - 1);
+ show_pwm, set_pwm, offset - 1); \
+static SENSOR_DEVICE_ATTR(pwm##offset##_auto_channels_temp, S_IRUGO | S_IWUSR, \
+ show_pwm_auto_channels_temp, set_pwm_auto_channels_temp, offset - 1); \
+static SENSOR_DEVICE_ATTR(pwm##offset##_auto_point1_pwm, S_IRUGO | S_IWUSR, \
+ show_pwm_auto_point1_pwm, set_pwm_auto_point1_pwm, offset - 1); \
+static SENSOR_DEVICE_ATTR(pwm##offset##_auto_point1_temp, S_IRUGO | S_IWUSR, \
+ show_pwm_auto_point1_temp, set_pwm_auto_point1_temp, offset - 1); \
+static SENSOR_DEVICE_ATTR(pwm##offset##_auto_point1_temp_hyst, S_IRUGO | S_IWUSR, \
+ show_pwm_auto_point1_temp_hyst, set_pwm_auto_point1_temp_hyst, offset - 1);\
+static SENSOR_DEVICE_ATTR(pwm##offset##_auto_point2_pwm, S_IRUGO | S_IWUSR, \
+ show_pwm_auto_point2_pwm, set_pwm_auto_point2_pwm, offset - 1); \
+static SENSOR_DEVICE_ATTR(pwm##offset##_auto_point2_temp, S_IRUGO | S_IWUSR, \
+ show_pwm_auto_point2_temp, set_pwm_auto_point2_temp, offset - 1);
show_pwm_offset(1);
show_pwm_offset(2);
@@ -713,8 +964,9 @@
}
err = 0;
+ chip_revision = superio_inb(DEVREV) & 0x0f;
pr_info("it87: Found IT%04xF chip at 0x%x, revision %d\n",
- chip_type, *address, superio_inb(DEVREV) & 0x0f);
+ chip_type, *address, chip_revision);
exit:
superio_exit();
@@ -894,6 +1146,27 @@
device_create_file(&new_client->dev, &sensor_dev_attr_pwm1.dev_attr);
device_create_file(&new_client->dev, &sensor_dev_attr_pwm2.dev_attr);
device_create_file(&new_client->dev, &sensor_dev_attr_pwm3.dev_attr);
+ /* Enable automatic SmartGuardian mode */
+ if (chip_revision = 0x03) {
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_auto_channels_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_auto_channels_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_auto_channels_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_auto_point1_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_auto_point1_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_auto_point1_temp_hyst.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_auto_point1_temp_hyst.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_auto_point1_temp_hyst.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_auto_point2_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_auto_point2_temp.dev_attr);
+ }
}
if (data->type = it8712) {
@@ -1024,17 +1297,6 @@
{
int tmp, i;
- /* initialize to sane defaults:
- * - if the chip is in manual pwm mode, this will be overwritten with
- * the actual settings on the chip (so in this case, initialization
- * is not needed)
- * - if in automatic or on/off mode, we could switch to manual mode,
- * read the registers and set manual_pwm_ctl accordingly, but currently
- * this is not implemented, so we initialize to something sane */
- for (i = 0; i < 3; i++) {
- data->manual_pwm_ctl[i] = 0xff;
- }
-
/* Check if temperature channnels are reset manually or by some reason */
tmp = it87_read_value(client, IT87_REG_TEMP_ENABLE);
if ((tmp & 0x3f) = 0) {
@@ -1059,6 +1321,18 @@
it87_write_value(client, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
}
+ /* Set current fan mode registers */
+ for (i = 0; i < 3; i++) {
+ data->manual_pwm_ctl[i] = it87_read_value(client, IT87_REG_PWM(i));
+
+ /* Get fan control registers for SmartGuardian automatic mode */
+ data->fan_limit_off[i] = it87_read_value(client, IT87_REG_SG_TEMP_OFF(i));
+ data->fan_limit_start[i] = it87_read_value(client, IT87_REG_SG_TEMP_START(i));
+ data->fan_limit_full[i] = it87_read_value(client, IT87_REG_SG_TEMP_FULL(i));
+ data->fan_limit_stpwm[i] = it87_read_value(client, IT87_REG_SG_START_PWM(i));
+ data->fan_am_ctrl[i] = it87_read_value(client, IT87_REG_SG_AM_CTRL(i));
+ }
+
/* Set current fan mode registers and the default settings for the
* other mode registers */
for (i = 0; i < 3; i++) {
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
` (5 preceding siblings ...)
2005-06-14 22:33 ` Sebastian Witt
@ 2005-06-17 15:06 ` Sebastian Witt
2005-06-17 15:23 ` Jean Delvare
2005-06-20 8:02 ` Jean Delvare
8 siblings, 0 replies; 10+ messages in thread
From: Sebastian Witt @ 2005-06-17 15:06 UTC (permalink / raw)
To: lm-sensors
Hi,
I've found out that I made a mistake: I used the IT8712_V07 datasheet and
not the datasheet for the IT8705F. That's because my last board has
a IT8712F and my current board has now a IT8705F...seen it while
trying to get the VID reading.
However the automatic fan control registers/values are on both chipsets the
same, I haven't seen a difference in the datasheets. So automatic
fan control should work on both chipsets. The check for revision/version
should be fixed.
Regards,
Sebastian
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
` (6 preceding siblings ...)
2005-06-17 15:06 ` Sebastian Witt
@ 2005-06-17 15:23 ` Jean Delvare
2005-06-20 8:02 ` Jean Delvare
8 siblings, 0 replies; 10+ messages in thread
From: Jean Delvare @ 2005-06-17 15:23 UTC (permalink / raw)
To: lm-sensors
Hi Sebastian,
On 2005-06-17, Sebastian Witt wrote:
> I've found out that I made a mistake: I used the IT8712_V07 datasheet and
> not the datasheet for the IT8705F. That's because my last board has
> a IT8712F and my current board has now a IT8705F...seen it while
> trying to get the VID reading.
>
> However the automatic fan control registers/values are on both chipsets the
> same, I haven't seen a difference in the datasheets. So automatic
> fan control should work on both chipsets. The check for revision/version
> should be fixed.
Agreed, your code should work (and be enabled) on both the latest IT8505F
and the latest IT8712F chips. I'm sorry I didn't have the time to
review your patch yet, I hopefully will do so during the coming
week-end. I'll probably have some comments and suggestions on it, so
you'll have a chance to update the revision checks when submitting the
final revision of your patch.
I would also like to document the revision letter vs. revision number for
these ITE chips. This should help people (and us) understand which chip
has what kind of auto-PWM implementation. Can you please visually check
the revision letter of both chips you have (it's mentioned on the top
marking) and give the value of the ID register? I'll do the same on my
two chips, if we can gather a few more we should have an almost complete
list.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread* [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c
2005-06-01 11:43 [lm-sensors] [PATCH]: Add automatic PWM mode to it87.c Sebastian Witt
` (7 preceding siblings ...)
2005-06-17 15:23 ` Jean Delvare
@ 2005-06-20 8:02 ` Jean Delvare
8 siblings, 0 replies; 10+ messages in thread
From: Jean Delvare @ 2005-06-20 8:02 UTC (permalink / raw)
To: lm-sensors
Hi Sebastian,
> Ok, here's a patch against 2.6.12-rc6.
> (...)
> I've now added only support for the auto-pwm with slope setting and a
> check for revision 3.
Here is my review of your patch. Sorry for the delay.
> static u16 chip_type;
> +static u8 chip_revision;
No space alignment in the kernel code. Either align with tabs, or don't
align.
> +#define IT87_REG_SG_TEMP_OFF(nr) (0x60 + (nr) * 8)
> +#define IT87_REG_SG_TEMP_START(nr) (0x61 + (nr) * 8)
> +#define IT87_REG_SG_TEMP_FULL(nr) (0x62 + (nr) * 8)
> +#define IT87_REG_SG_START_PWM(nr) (0x63 + (nr) * 8)
This last name isn't logical, should be IT87_REG_SG_PWM_START (or swap
the other ones, at your option).
> +#define SLOPE_FROM_REG(val) ((val) ? (1 << (val-1)) : 0)
Parentheses are not properly placed for macro safety. It should be:
#define SLOPE_FROM_REG(val) ((val) ? 1 << ((val)-1) : 0)
> + u8 fan_limit_off[3]; /* Register value */
> + u8 fan_limit_start[3]; /* Register value */
> + u8 fan_limit_full[3]; /* Register value */
> + u8 fan_limit_stpwm[3]; /* Register value */
> + u8 fan_am_ctrl[3]; /* Register value */
> + u8 fan_point2_pwm[3]; /* Used for slope calculation */
I don't much like the names you chose. For one thing, it's not obvious
that they are for SmartGuardian/auto-PWM. You should prefix them with
sg_ or auto_pwm_ rather than fan_. For another, temperature values
aren't clearly labelled as such. What about sg_temp_off, sg_temp_start,
sg_temp_full, sg_pwm_start, sg_am_ctrl and sg_pwm_point2?
> +static void calc_slope(struct it87_data *data, int nr)
> +{
> + u8 val = data->fan_point2_pwm[nr];
> +
> + if (data->fan_limit_full[nr] <= data->fan_limit_start[nr])
> + val = 14;
> + else {
> + val -= data->fan_limit_stpwm[nr];
> + val /= (data->fan_limit_full[nr] - data->fan_limit_start[nr]);
> + }
Where does this "14" value come from? If full speed temperature happens
to be below the start temperature, we're in trouble anyway, so why
bother?
+ data->fan_am_ctrl[nr] &= ~0x03;
Shouldn't this be ~0x07?
+ if (val > 48)
+ data->fan_am_ctrl[nr] |= 0x07; /* 64 PWM / K */
What does the "K" stands for? Kelvin? I think "PWM/degree" would be
clearer.
> + /* Automatic mode - SmartGuardian */
> + val += (val && (data->manual_pwm_ctl[nr] & 0x80)) ? 1 : 0;
An "if" statement would be clearer IMHO.
> + return sprintf(buf,"%d\n", val);
Add a space after the first comma (there are many of these in the rest
of the patch).
> + /* Return 0 if automatic mode is enabled */
> + return sprintf(buf,"%d\n", (data->manual_pwm_ctl[nr] & 0x80) ? 0 : PWM_FROM_REG(data->manual_pwm_ctl[nr]));
This is unsafe. You should hold the lock (&data->update_lock), else
data->manual_pwm_ctl[nr] may have a different on second read, and you
may display a completely bogus PWM value. Same is true for all other
functions in which you access more than one field of the data structure
(or one, but more than once), you have to hold the lock.
> + /* Enable temperature smoothing and fan spin up time */
Extra space between "up" and "time". What is this spin up time BTW? The
datasheet is unclear, to say the least.
> + data->fan_am_ctrl[nr] = 0x80 | 0x18;
> + it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
> + /* If start pwm and pwm step size not set, apply safe values */
> + if (data->fan_limit_stpwm[nr] = 0) {
> + data->fan_limit_stpwm[nr] = PWM_TO_REG(128);
> + it87_write_value(client, IT87_REG_SG_START_PWM(nr), data->fan_limit_stpwm[nr]);
> + }
> + if ((data->fan_am_ctrl[nr] & 0x07) = 0) {
> + /* 16 PWM values / K */
> + data->fan_am_ctrl[nr] |= 0x05;
> + it87_write_value(client, IT87_REG_SG_AM_CTRL(nr), data->fan_am_ctrl[nr]);
> + }
This isn't correct. Tracking data->fan_am_ctrl[nr], I can see you are
setting it to 0x98, then writing it to the chip, then testing it with &
0x07 = 0 (will always be true), set it to 0x9D and write it to the chip
again. This isn't correct. You must not change the slope value if it was
set to a sane value before, and you must not write twice to the same
register (waste of time).
> + data->fan_limit_start[nr] = val;
This lacks a sanity check. Same for the two other ones.
As a side note, I think I would have forced point2's PWM value to 255,
rather than letting the user set it. Having this PWM value too low will
have bad effects due to the lack of hysteresis (the fan will go from low
speed to full speed and back again and again), forcing it to be as near
from full speed as possible would solve this problem.
> +static ssize_t set_pwm_auto_channels_temp(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> (...)
> + if (val < 0 || val > 4)
> + return -EINVAL;
3 isn't a valid value, but will pass through.
> + data->manual_pwm_ctl[nr] = 0x80 | (val >> 1);
This bit shifting is a trick which happens to work in this specific
case, but wouldn't work anymore if there were a 4th temperature channel,
for example. So please don't use it.
> + /* Set current fan mode registers */
> + for (i = 0; i < 3; i++) {
> + data->manual_pwm_ctl[i] = it87_read_value(client, IT87_REG_PWM(i));
> +
> + /* Get fan control registers for SmartGuardian automatic mode */
> + data->fan_limit_off[i] = it87_read_value(client, IT87_REG_SG_TEMP_OFF(i));
> + data->fan_limit_start[i] = it87_read_value(client, IT87_REG_SG_TEMP_START(i));
> + data->fan_limit_full[i] = it87_read_value(client, IT87_REG_SG_TEMP_FULL(i));
> + data->fan_limit_stpwm[i] = it87_read_value(client, IT87_REG_SG_START_PWM(i));
> + data->fan_am_ctrl[i] = it87_read_value(client, IT87_REG_SG_AM_CTRL(i));
> + }
This doesn't belong to the init function but to the device_update
function. Additionally, you should do this if and only if the device
actually supports the auto-pwm code (so you have to remember this in
some structure field).
Please provide an updated patch. Don't forget to also update the
revision detection code. You have to check for different revision for
the IT8705F and IT8712F chips, so you really have to test the
combination of chip and revision.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread