* [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control on
@ 2010-11-29 13:00 Jean Delvare
2010-11-29 16:58 ` [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control Guenter Roeck
2010-11-29 20:26 ` Jean Delvare
0 siblings, 2 replies; 3+ messages in thread
From: Jean Delvare @ 2010-11-29 13:00 UTC (permalink / raw)
To: lm-sensors
The manual fan speed control logic of the IT8721F is much different
from what older devices had. Update the code to properly support that.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
drivers/hwmon/it87.c | 61 ++++++++++++++++++++++++++++++++++++--------------
1 file changed, 45 insertions(+), 16 deletions(-)
--- linux-2.6.37-rc3.orig/drivers/hwmon/it87.c 2010-11-24 10:06:22.000000000 +0100
+++ linux-2.6.37-rc3/drivers/hwmon/it87.c 2010-11-29 13:56:10.000000000 +0100
@@ -187,6 +187,7 @@ static const u8 IT87_REG_FANX_MIN[] = {
#define IT87_REG_FAN_MAIN_CTRL 0x13
#define IT87_REG_FAN_CTL 0x14
#define IT87_REG_PWM(nr) (0x15 + (nr))
+#define IT87_REG_PWM_DUTY(nr) (0x63 + (nr) * 8)
#define IT87_REG_VIN(nr) (0x20 + (nr))
#define IT87_REG_TEMP(nr) (0x29 + (nr))
@@ -251,12 +252,16 @@ struct it87_data {
u8 fan_main_ctrl; /* Register value */
u8 fan_ctl; /* Register value */
- /* The following 3 arrays correspond to the same registers. The
- * meaning of bits 6-0 depends on the value of bit 7, and we want
- * to preserve settings on mode changes, so we have to track all
- * values separately. */
+ /* The following 3 arrays correspond to the same registers up to
+ * the IT8720F. The meaning of bits 6-0 depends on the value of bit
+ * 7, and we want to preserve settings on mode changes, so we have
+ * to track all values separately.
+ * Starting with the IT8721F, the manual PWM duty cycles are stored
+ * in separate registers (8-bit values), so the separate tracking
+ * is no longer needed, but it is still done to keep the driver
+ * simple. */
u8 pwm_ctrl[3]; /* Register value */
- u8 pwm_duty[3]; /* Manual PWM value set by user (bit 6-0) */
+ u8 pwm_duty[3]; /* Manual PWM value set by user */
u8 pwm_temp_map[3]; /* PWM to temp. chan. mapping (bits 1-0) */
/* Automatic fan speed control registers */
@@ -832,7 +837,9 @@ static ssize_t set_pwm_enable(struct dev
data->fan_main_ctrl);
} else {
if (val = 1) /* Manual mode */
- data->pwm_ctrl[nr] = data->pwm_duty[nr];
+ data->pwm_ctrl[nr] = data->type = it8721 ?
+ data->pwm_temp_map[nr] :
+ data->pwm_duty[nr];
else /* Automatic mode */
data->pwm_ctrl[nr] = 0x80 | data->pwm_temp_map[nr];
it87_write_value(data, IT87_REG_PWM(nr), data->pwm_ctrl[nr]);
@@ -858,12 +865,25 @@ static ssize_t set_pwm(struct device *de
return -EINVAL;
mutex_lock(&data->update_lock);
- data->pwm_duty[nr] = pwm_to_reg(data, val);
- /* If we are in manual mode, write the duty cycle immediately;
- * otherwise, just store it for later use. */
- if (!(data->pwm_ctrl[nr] & 0x80)) {
- data->pwm_ctrl[nr] = data->pwm_duty[nr];
- it87_write_value(data, IT87_REG_PWM(nr), data->pwm_ctrl[nr]);
+ if (data->type = it8721) {
+ /* If we are in automatic mode, the PWM duty cycle register
+ * is read-only so we can't write the value */
+ if (data->pwm_ctrl[nr] & 0x80) {
+ mutex_unlock(&data->update_lock);
+ return -EBUSY;
+ }
+ data->pwm_duty[nr] = pwm_to_reg(data, val);
+ it87_write_value(data, IT87_REG_PWM_DUTY(nr),
+ data->pwm_duty[nr]);
+ } else {
+ data->pwm_duty[nr] = pwm_to_reg(data, val);
+ /* If we are in manual mode, write the duty cycle immediately;
+ * otherwise, just store it for later use. */
+ if (!(data->pwm_ctrl[nr] & 0x80)) {
+ data->pwm_ctrl[nr] = data->pwm_duty[nr];
+ it87_write_value(data, IT87_REG_PWM(nr),
+ data->pwm_ctrl[nr]);
+ }
}
mutex_unlock(&data->update_lock);
return count;
@@ -1958,7 +1978,10 @@ static void __devinit it87_init_device(s
* channels to use when later setting to automatic mode later.
* Use a 1:1 mapping by default (we are clueless.)
* In both cases, the value can (and should) be changed by the user
- * prior to switching to a different mode. */
+ * prior to switching to a different mode.
+ * Note that this is no longer needed for the IT8721F and later, as
+ * these have separate registers for the temperature mapping and the
+ * manual duty cycle. */
for (i = 0; i < 3; i++) {
data->pwm_temp_map[i] = i;
data->pwm_duty[i] = 0x7f; /* Full speed */
@@ -2034,10 +2057,16 @@ static void __devinit it87_init_device(s
static void it87_update_pwm_ctrl(struct it87_data *data, int nr)
{
data->pwm_ctrl[nr] = it87_read_value(data, IT87_REG_PWM(nr));
- if (data->pwm_ctrl[nr] & 0x80) /* Automatic mode */
+ if (data->type = it8721) {
data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
- else /* Manual mode */
- data->pwm_duty[nr] = data->pwm_ctrl[nr] & 0x7f;
+ data->pwm_duty[nr] = it87_read_value(data,
+ IT87_REG_PWM_DUTY(nr));
+ } else {
+ if (data->pwm_ctrl[nr] & 0x80) /* Automatic mode */
+ data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
+ else /* Manual mode */
+ data->pwm_duty[nr] = (data->pwm_ctrl[nr] & 0x7f) << 1;
+ }
if (has_old_autopwm(data)) {
int i;
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control
2010-11-29 13:00 [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control on Jean Delvare
@ 2010-11-29 16:58 ` Guenter Roeck
2010-11-29 20:26 ` Jean Delvare
1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2010-11-29 16:58 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On Mon, Nov 29, 2010 at 08:00:10AM -0500, Jean Delvare wrote:
> The manual fan speed control logic of the IT8721F is much different
> from what older devices had. Update the code to properly support that.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
Minor comment below, otherwise
Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
> drivers/hwmon/it87.c | 61 ++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 45 insertions(+), 16 deletions(-)
>
> --- linux-2.6.37-rc3.orig/drivers/hwmon/it87.c 2010-11-24 10:06:22.000000000 +0100
> +++ linux-2.6.37-rc3/drivers/hwmon/it87.c 2010-11-29 13:56:10.000000000 +0100
> @@ -187,6 +187,7 @@ static const u8 IT87_REG_FANX_MIN[] = {
> #define IT87_REG_FAN_MAIN_CTRL 0x13
> #define IT87_REG_FAN_CTL 0x14
> #define IT87_REG_PWM(nr) (0x15 + (nr))
> +#define IT87_REG_PWM_DUTY(nr) (0x63 + (nr) * 8)
>
> #define IT87_REG_VIN(nr) (0x20 + (nr))
> #define IT87_REG_TEMP(nr) (0x29 + (nr))
> @@ -251,12 +252,16 @@ struct it87_data {
> u8 fan_main_ctrl; /* Register value */
> u8 fan_ctl; /* Register value */
>
> - /* The following 3 arrays correspond to the same registers. The
> - * meaning of bits 6-0 depends on the value of bit 7, and we want
> - * to preserve settings on mode changes, so we have to track all
> - * values separately. */
> + /* The following 3 arrays correspond to the same registers up to
> + * the IT8720F. The meaning of bits 6-0 depends on the value of bit
> + * 7, and we want to preserve settings on mode changes, so we have
> + * to track all values separately.
> + * Starting with the IT8721F, the manual PWM duty cycles are stored
> + * in separate registers (8-bit values), so the separate tracking
> + * is no longer needed, but it is still done to keep the driver
> + * simple. */
> u8 pwm_ctrl[3]; /* Register value */
> - u8 pwm_duty[3]; /* Manual PWM value set by user (bit 6-0) */
> + u8 pwm_duty[3]; /* Manual PWM value set by user */
> u8 pwm_temp_map[3]; /* PWM to temp. chan. mapping (bits 1-0) */
>
> /* Automatic fan speed control registers */
> @@ -832,7 +837,9 @@ static ssize_t set_pwm_enable(struct dev
> data->fan_main_ctrl);
> } else {
> if (val = 1) /* Manual mode */
> - data->pwm_ctrl[nr] = data->pwm_duty[nr];
> + data->pwm_ctrl[nr] = data->type = it8721 ?
> + data->pwm_temp_map[nr] :
> + data->pwm_duty[nr];
> else /* Automatic mode */
> data->pwm_ctrl[nr] = 0x80 | data->pwm_temp_map[nr];
> it87_write_value(data, IT87_REG_PWM(nr), data->pwm_ctrl[nr]);
> @@ -858,12 +865,25 @@ static ssize_t set_pwm(struct device *de
> return -EINVAL;
>
> mutex_lock(&data->update_lock);
> - data->pwm_duty[nr] = pwm_to_reg(data, val);
> - /* If we are in manual mode, write the duty cycle immediately;
> - * otherwise, just store it for later use. */
> - if (!(data->pwm_ctrl[nr] & 0x80)) {
> - data->pwm_ctrl[nr] = data->pwm_duty[nr];
> - it87_write_value(data, IT87_REG_PWM(nr), data->pwm_ctrl[nr]);
> + if (data->type = it8721) {
> + /* If we are in automatic mode, the PWM duty cycle register
> + * is read-only so we can't write the value */
> + if (data->pwm_ctrl[nr] & 0x80) {
> + mutex_unlock(&data->update_lock);
> + return -EBUSY;
> + }
> + data->pwm_duty[nr] = pwm_to_reg(data, val);
> + it87_write_value(data, IT87_REG_PWM_DUTY(nr),
> + data->pwm_duty[nr]);
> + } else {
> + data->pwm_duty[nr] = pwm_to_reg(data, val);
> + /* If we are in manual mode, write the duty cycle immediately;
> + * otherwise, just store it for later use. */
> + if (!(data->pwm_ctrl[nr] & 0x80)) {
> + data->pwm_ctrl[nr] = data->pwm_duty[nr];
> + it87_write_value(data, IT87_REG_PWM(nr),
> + data->pwm_ctrl[nr]);
> + }
> }
> mutex_unlock(&data->update_lock);
> return count;
> @@ -1958,7 +1978,10 @@ static void __devinit it87_init_device(s
> * channels to use when later setting to automatic mode later.
> * Use a 1:1 mapping by default (we are clueless.)
> * In both cases, the value can (and should) be changed by the user
> - * prior to switching to a different mode. */
> + * prior to switching to a different mode.
> + * Note that this is no longer needed for the IT8721F and later, as
> + * these have separate registers for the temperature mapping and the
> + * manual duty cycle. */
> for (i = 0; i < 3; i++) {
> data->pwm_temp_map[i] = i;
> data->pwm_duty[i] = 0x7f; /* Full speed */
> @@ -2034,10 +2057,16 @@ static void __devinit it87_init_device(s
> static void it87_update_pwm_ctrl(struct it87_data *data, int nr)
> {
> data->pwm_ctrl[nr] = it87_read_value(data, IT87_REG_PWM(nr));
> - if (data->pwm_ctrl[nr] & 0x80) /* Automatic mode */
> + if (data->type = it8721) {
> data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
> - else /* Manual mode */
> - data->pwm_duty[nr] = data->pwm_ctrl[nr] & 0x7f;
> + data->pwm_duty[nr] = it87_read_value(data,
> + IT87_REG_PWM_DUTY(nr));
> + } else {
> + if (data->pwm_ctrl[nr] & 0x80) /* Automatic mode */
> + data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
> + else /* Manual mode */
> + data->pwm_duty[nr] = (data->pwm_ctrl[nr] & 0x7f) << 1;
This is a bug fix, isn't it ? You might want to mention it in the commit log.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control
2010-11-29 13:00 [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control on Jean Delvare
2010-11-29 16:58 ` [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control Guenter Roeck
@ 2010-11-29 20:26 ` Jean Delvare
1 sibling, 0 replies; 3+ messages in thread
From: Jean Delvare @ 2010-11-29 20:26 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
On Mon, 29 Nov 2010 08:58:49 -0800, Guenter Roeck wrote:
> Hi Jean,
>
> On Mon, Nov 29, 2010 at 08:00:10AM -0500, Jean Delvare wrote:
> > The manual fan speed control logic of the IT8721F is much different
> > from what older devices had. Update the code to properly support that.
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
>
> Minor comment below, otherwise
>
> Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
>
> > ---
> > drivers/hwmon/it87.c | 61 ++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 45 insertions(+), 16 deletions(-)
> >
> > --- linux-2.6.37-rc3.orig/drivers/hwmon/it87.c 2010-11-24 10:06:22.000000000 +0100
> > +++ linux-2.6.37-rc3/drivers/hwmon/it87.c 2010-11-29 13:56:10.000000000 +0100
> > (...)
> > @@ -2034,10 +2057,16 @@ static void __devinit it87_init_device(s
> > static void it87_update_pwm_ctrl(struct it87_data *data, int nr)
> > {
> > data->pwm_ctrl[nr] = it87_read_value(data, IT87_REG_PWM(nr));
> > - if (data->pwm_ctrl[nr] & 0x80) /* Automatic mode */
> > + if (data->type = it8721) {
> > data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
> > - else /* Manual mode */
> > - data->pwm_duty[nr] = data->pwm_ctrl[nr] & 0x7f;
> > + data->pwm_duty[nr] = it87_read_value(data,
> > + IT87_REG_PWM_DUTY(nr));
> > + } else {
> > + if (data->pwm_ctrl[nr] & 0x80) /* Automatic mode */
> > + data->pwm_temp_map[nr] = data->pwm_ctrl[nr] & 0x03;
> > + else /* Manual mode */
> > + data->pwm_duty[nr] = (data->pwm_ctrl[nr] & 0x7f) << 1;
>
> This is a bug fix, isn't it ? You might want to mention it in the commit log.
Huh. No, it isn't. It's me adding a bug actually... Glad you spotted
it, thanks for the careful review. I seem to recall that at some point
when writing this patch, I considered left-aligning the PWM duty cycle
value in the hope to simplify the code, but apparently I changed my
mind half way through.
I'll send a fixed patch...
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-11-29 20:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-29 13:00 [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control on Jean Delvare
2010-11-29 16:58 ` [lm-sensors] [PATCH] hwmon: (it87) Fix manual fan speed control Guenter Roeck
2010-11-29 20:26 ` Jean Delvare
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.