> The "enable" and "mode" sysfs files are hooked up to the bitfields
backward.
>> No. There is a standard for sysfs attributes of hwmon drivers.
Gotcha. Having now read and digested hwmon's sysfs doc, I agree completely.
My path to this was:
a) What chip do I have here?
b) Where's the datasheet?
c) Ooh, there's a kernel module for this already!
d) The names match the datasheet, but backwards... :(
I'll view the world through hwmon-ABI-colored glasses for the rest of this
discussion...
> switching from DC to PWM or the other way around is normally not needed
as both
> modes need different electronics on the board and the BIOS should set
> the right mode.
Embedded. pre-boot environment is just leaving things at power-on
defaults...
> I see. This is caused by the mask error you have already found out.
> Looks like this feature of the driver didn't receive much testing...
Kevin concurred on the fix. The patch as an attachment is re-included w/
this msg as patch #1.
> Mode "FAN_SET" is problematic on its own, independent of the bug...
> It's not really an automatic fan speed mode so it shouldn't have value >=
2.
> The closest standard mode would be 0, except that 0 is supposed to mean
"100%", not "some arbitrary initial value".
I like keeping mode 0 as universal fail-safe "100% fan". We can synthesize
a mode 0 for this hardware even though it is lacking one.
See patch #3, attached.
> Well, we could write 0xf to register 0x90 bits 3..0 (assuming they are
writable
> as the datasheet says) to force the 100% but then this would prevent
> the user from returning to the initial state, plus I'm not sure what
> would happen at suspend/resume or reboot.
>
They are writable. But part of the current behavior is nice in that the
driver doesn't change the state of the hardware at all upon loading. It'd
be nice to preserve the passive nature of the driver until someone actually
writes to a sysfs file.
> OTOH, if we stick to value 4 for this mode, then there's no reason to
> not let the user switch back to it. Supporting it would be truly
> trivial.
>
Yes, but your ABI seems to want to have enable modes >= 2 be
smart/non-manual. This is like another flavor of manual, with a separate
pwm register for FAN_SET.
> Another possibility would be to hide this mode, by transparently
> switching to the equivalent speed in manual control mode when the
> driver is loaded. Then only modes 1, 2 and 3 would be visible to the
> user. To be honest I'm not sure why the chip maker didn't implement it
> this way in the first place. I think this option has my preference.
> Guenter, what do you think? Do we have any precedent?
>
This is my preference also. Although I'd do the extra work to make this
passive (FAN_SET appears as manual w/ pwmX sys returning the special
FAN_INI duty cycle, until such time as someone writes to sysfs in a way to
have us commit to switching from hardware mode 4 (fan_set) to hardware mode
1 (manual).
See patch #4, attached.
> Indeed, Richard (Cc'd) noticed some problems with fan speed control
> back then, but he did not provide enough details for investigation. The
> bug you found could well explain these problems, and Richard would
> probably be interested in testing your fix.
>
There's a 2nd bug, that almost certainly affected Richard also.
The current code sets "pwmX" as an 8-bit value. This hardware has a 4-bit
pwm field. The driver is setting both pwmX (aka pwmX_auto_point1) and
pwmX_auto_point2 when the user attempts to assign to pwmX.
It's especially noticeable when running pwmconfig, as the 2nd level
attempted is "240", which has the low 4-bits cleared, so the fan speed
drops to zero on step 2. Example:
Would you like to generate a detailed correlation (y)?
PWM 255 FAN 3026
PWM 240 FAN 0
Fan Stopped at PWM = 240
See attached patch #2. After applying it, we get instead:
Would you like to generate a detailed correlation (y)?
PWM 255 FAN 3096
PWM 240 FAN 2909
PWM 225 FAN 2721
PWM 210 FAN 2500
PWM 195 FAN 2343
PWM 180 FAN 2327
PWM 165 FAN 2096
PWM 150 FAN 1885
PWM 135 FAN 1614
PWM 120 FAN 1424
PWM 105 FAN 0
Fan Stopped at PWM = 105
>Thanks for finding and fixing this bug. Please resend your
>patch as explained above and I'll be happy to apply it, forward it to
>the stable kernel maintainers, and make the updated driver available in
>a stand-alone flavor.
First time submitter. Sorry for the newbie submission mistake.
Between gmail and the mailing list software, don't know who's mangling.
I'll just attach the patches as files to be safe.
As for Guenter's comments on using pwmX_enable=0, it sounds like everyone
prefers enable=0 to be 100% fan if we can present it that way.
After the patches below, we get the illusion of FAN_SET as manual mode,
which then collapses into actual manual mode upon touching pwmX or
pwmX_enable in a way that matters.
If anyone wanted to return to FAN_SET mode, they could save off the initial
value returned by pwmX (which is the masquerading FAN_INI value) and use
that in manual mode to reach a state indistinguishable from FAN_SET mode.
I'm also open to exposing FAN_INI value directly in sysfs, if you had a
preferred naming convention for that.
Regards,
Brian
01_pwmX_enable_bugfix.diff
02_pwmX_bugfix.diff
03_pwmX_enable_synthetic_mode_0.diff
04_pwmX_enable_collapse_and_hide_fan_set_mode_4.diff
Signed-off-by: Brian Carnes <bmcarnes@xxxxxxxxx>
diff --git a/w83l786ng.c b/w83l786ng.c
index e343099..6670f38 100644
--- a/w83l786ng.c
+++ b/w83l786ng.c
@@ -523,7 +523,7 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
mutex_lock(&data->update_lock);
reg = w83l786ng_read_value(client, W83L786NG_REG_FAN_CFG);
data->pwm_enable[nr] = val;
- reg &= ~(0x02 << W83L786NG_PWM_ENABLE_SHIFT[nr]);
+ reg &= ~(0x03 << W83L786NG_PWM_ENABLE_SHIFT[nr]);
reg |= (val - 1) << W83L786NG_PWM_ENABLE_SHIFT[nr];
w83l786ng_write_value(client, W83L786NG_REG_FAN_CFG, reg);
mutex_unlock(&data->update_lock);
@@ -790,7 +790,7 @@ static struct w83l786ng_data *w83l786ng_update_device(struct device *dev)
((pwmcfg >> W83L786NG_PWM_MODE_SHIFT[i]) & 1)
? 0 : 1;
data->pwm_enable[i] =
- ((pwmcfg >> W83L786NG_PWM_ENABLE_SHIFT[i]) & 2) + 1;
+ ((pwmcfg >> W83L786NG_PWM_ENABLE_SHIFT[i]) & 3) + 1;
data->pwm[i] = w83l786ng_read_value(client,
W83L786NG_REG_PWM[i]);
}
diff --git a/w83l786ng.c b/w83l786ng.c
index 6670f38..a67deb9 100644
--- a/w83l786ng.c
+++ b/w83l786ng.c
@@ -140,7 +140,7 @@ struct w83l786ng_data {
u8 fan_min[2];
u8 temp_type[2];
u8 temp[2][3];
- u8 pwm[2];
+ u8 pwm[2][4];
u8 pwm_mode[2]; /* 0->DC variable voltage
* 1->PWM variable duty cycle */
@@ -450,7 +450,6 @@ static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
show_pwm_reg(pwm_mode)
show_pwm_reg(pwm_enable)
-show_pwm_reg(pwm)
static ssize_t
store_pwm_mode(struct device *dev, struct device_attribute *attr,
@@ -481,23 +480,54 @@ store_pwm_mode(struct device *dev, struct device_attribute *attr,
}
static ssize_t
+show_pwm(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute_2 *sensor_attr =
+ to_sensor_dev_attr_2(attr);
+ struct w83l786ng_data *data = w83l786ng_update_device(dev);
+ int nr = sensor_attr->nr;
+ int point = sensor_attr->index;
+ u8 reg = data->pwm[nr][point];
+ int val;
+
+ /* map 0..15 -> 0..255 in a nice way, s.t. 0->0 and 15->255 */
+ val = reg * 0x11;
+ return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t
store_pwm(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- int nr = to_sensor_dev_attr(attr)->index;
+ struct sensor_device_attribute_2 *sensor_attr =
+ to_sensor_dev_attr_2(attr);
+ int nr = sensor_attr->nr;
+ int point = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct w83l786ng_data *data = i2c_get_clientdata(client);
unsigned long val;
int err;
+ int regofs = point / 2;
+ int regshift = (point % 2) * 4;
+ u8 reg;
err = kstrtoul(buf, 10, &val);
if (err)
return err;
- val = clamp_val(val, 0, 255);
+
+ /* duty cycles are 0..15 */
+ /* behave according to the mapping in show_pwm, s.t. 0->0, 15->255 */
+ /* so round according to set points: 0x00, 0x11, ..., 0xee, 0xff */
+ val = (val + 8) / 17;
+ val = clamp_val(val, 0, 15);
mutex_lock(&data->update_lock);
- data->pwm[nr] = val;
- w83l786ng_write_value(client, W83L786NG_REG_PWM[nr], val);
+ data->pwm[nr][point] = val;
+ reg = w83l786ng_read_value(client, W83L786NG_REG_PWM[nr] + regofs);
+ reg &= ~(0xf << regshift);
+ reg |= val << regshift;
+ w83l786ng_write_value(client, W83L786NG_REG_PWM[nr] + regofs, reg);
mutex_unlock(&data->update_lock);
return count;
}
@@ -530,9 +560,28 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
return count;
}
-static struct sensor_device_attribute sda_pwm[] = {
- SENSOR_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0),
- SENSOR_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1),
+static struct sensor_device_attribute_2 sda_pwm[] = {
+ SENSOR_ATTR_2(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0, 0),
+ SENSOR_ATTR_2(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1, 0),
+};
+
+static struct sensor_device_attribute_2 sda_pwm_points[] = {
+ SENSOR_ATTR_2(pwm1_auto_point_1, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 0, 0),
+ SENSOR_ATTR_2(pwm1_auto_point_2, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 0, 1),
+ SENSOR_ATTR_2(pwm1_auto_point_3, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 0, 2),
+ SENSOR_ATTR_2(pwm1_auto_point_4, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 0, 3),
+ SENSOR_ATTR_2(pwm2_auto_point_1, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 1, 0),
+ SENSOR_ATTR_2(pwm2_auto_point_2, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 1, 1),
+ SENSOR_ATTR_2(pwm2_auto_point_3, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 1, 2),
+ SENSOR_ATTR_2(pwm2_auto_point_4, S_IWUSR | S_IRUGO,
+ show_pwm, store_pwm, 1, 3),
};
static struct sensor_device_attribute sda_pwm_mode[] = {
@@ -614,7 +663,11 @@ static struct sensor_device_attribute sda_tolerance[] = {
#define PWM_UNIT_ATTRS(X) \
&sda_pwm[X].dev_attr.attr, \
&sda_pwm_mode[X].dev_attr.attr, \
- &sda_pwm_enable[X].dev_attr.attr
+ &sda_pwm_enable[X].dev_attr.attr, \
+ &sda_pwm_points[X*4+0].dev_attr.attr, \
+ &sda_pwm_points[X*4+1].dev_attr.attr, \
+ &sda_pwm_points[X*4+2].dev_attr.attr, \
+ &sda_pwm_points[X*4+3].dev_attr.attr
#define TOLERANCE_UNIT_ATTRS(X) \
&sda_tolerance[X].dev_attr.attr
@@ -791,8 +844,12 @@ static struct w83l786ng_data *w83l786ng_update_device(struct device *dev)
? 0 : 1;
data->pwm_enable[i] =
((pwmcfg >> W83L786NG_PWM_ENABLE_SHIFT[i]) & 3) + 1;
- data->pwm[i] = w83l786ng_read_value(client,
- W83L786NG_REG_PWM[i]);
+ for (j = 0; j < 2; j++) {
+ u8 pwms_packed = w83l786ng_read_value(client,
+ W83L786NG_REG_PWM[i] + j);
+ data->pwm[i][j*2] = pwms_packed & 0xf;
+ data->pwm[i][j*2 + 1] = pwms_packed >> 4;
+ }
}
diff --git a/w83l786ng.c b/w83l786ng.c
index a67deb9..1b6d040 100644
--- a/w83l786ng.c
+++ b/w83l786ng.c
@@ -144,7 +144,8 @@ struct w83l786ng_data {
u8 pwm_mode[2]; /* 0->DC variable voltage
* 1->PWM variable duty cycle */
- u8 pwm_enable[2]; /* 1->manual
+ u8 pwm_enable[2]; /* 0->100% fan mode (synthetic)
+ * 1->manual
* 2->thermal cruise (also called SmartFan I) */
u8 tolerance[2];
};
@@ -547,12 +548,21 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
if (err)
return err;
- if (!val || val > 2) /* only modes 1 and 2 are supported */
+ if (val > 2) /* only modes 0, 1 and 2 are supported */
return -EINVAL;
mutex_lock(&data->update_lock);
- reg = w83l786ng_read_value(client, W83L786NG_REG_FAN_CFG);
data->pwm_enable[nr] = val;
+ if (val == 0) { /* synthesize a 100% fan mode */
+ /* turn pwmX all the way up */
+ /* (but don't touch pwmX_auto_point_2 in high bits) */
+ reg = w83l786ng_read_value(client, W83L786NG_REG_PWM[nr]);
+ reg |= 0xf;
+ w83l786ng_write_value(client, W83L786NG_REG_PWM[nr], reg);
+ /* now tell hardware we're in manual mode */
+ val = 1;
+ }
+ reg = w83l786ng_read_value(client, W83L786NG_REG_FAN_CFG);
reg &= ~(0x03 << W83L786NG_PWM_ENABLE_SHIFT[nr]);
reg |= (val - 1) << W83L786NG_PWM_ENABLE_SHIFT[nr];
w83l786ng_write_value(client, W83L786NG_REG_FAN_CFG, reg);
@@ -757,6 +767,14 @@ w83l786ng_probe(struct i2c_client *client, const struct i2c_device_id *id)
data->fan_div[0] = reg_tmp & 0x07;
data->fan_div[1] = (reg_tmp >> 4) & 0x07;
+ /* Ensure the pwm_enable saved regs start off non-zero,
+ * to help with remembering if the user has chosen
+ * our synthetic pwm_enable = 0 mode.
+ * (update_device now reads them before overwriting them).
+ */
+ data->pwm_enable[0] = 0xff;
+ data->pwm_enable[1] = 0xff;
+
/* Register sysfs hooks */
err = sysfs_create_group(&client->dev.kobj, &w83l786ng_group);
if (err)
@@ -839,6 +857,8 @@ static struct w83l786ng_data *w83l786ng_update_device(struct device *dev)
pwmcfg = w83l786ng_read_value(client, W83L786NG_REG_FAN_CFG);
for (i = 0; i < 2; i++) {
+ u8 prev_enable = data->pwm_enable[i];
+
data->pwm_mode[i] =
((pwmcfg >> W83L786NG_PWM_MODE_SHIFT[i]) & 1)
? 0 : 1;
@@ -850,6 +870,12 @@ static struct w83l786ng_data *w83l786ng_update_device(struct device *dev)
data->pwm[i][j*2] = pwms_packed & 0xf;
data->pwm[i][j*2 + 1] = pwms_packed >> 4;
}
+ /* if we were in synthetic mode 0, preserve that illusion. */
+ if (prev_enable == 0 &&
+ data->pwm_enable[i] == 1 &&
+ data->pwm[i][0] == 0xf) {
+ data->pwm_enable[i] = 0;
+ }
}
diff --git a/w83l786ng.c b/w83l786ng.c
index 1b6d040..ed09a27 100644
--- a/w83l786ng.c
+++ b/w83l786ng.c
@@ -81,6 +81,9 @@ static const u8 W83L786NG_PWM_ENABLE_SHIFT[] = {2, 4};
/* FAN Duty Cycle, be used to control */
static const u8 W83L786NG_REG_PWM[] = {0x81, 0x87};
+/* Initial fan speed at power up */
+static const u8 W83L786NG_REG_FANINI = 0x90;
+
static inline u8
FAN_TO_REG(long rpm, int div)
@@ -144,9 +147,12 @@ struct w83l786ng_data {
u8 pwm_mode[2]; /* 0->DC variable voltage
* 1->PWM variable duty cycle */
- u8 pwm_enable[2]; /* 0->100% fan mode (synthetic)
- * 1->manual
- * 2->thermal cruise (also called SmartFan I) */
+ u8 pwm_enable[2]; /* 0->100% fan mode (synthetic)
+ * 1->manual
+ * 2->thermal cruise (also called SmartFan I)
+ * 3->SmartFan II
+ * 4->(internal) FAN_SET (sysfs sees manual) */
+ u8 pwm_ini; /* power on setting */
u8 tolerance[2];
};
@@ -450,7 +456,50 @@ static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
}
show_pwm_reg(pwm_mode)
-show_pwm_reg(pwm_enable)
+
+/* Internal helper function to get the current HARDWARE enable mode.
+ * This will be in the range of 1..4 - it is blind to fake enable mode 0.
+ * Factored out, since the hardware mode is needed in multiple store* functions,
+ * to assist with collapsing the FAN_SET enable mode into manual.
+ */
+static u8 get_hw_pwm_enable(struct i2c_client *client, int nr) {
+ u8 pwm_cfg = w83l786ng_read_value(client, W83L786NG_REG_FAN_CFG);
+ return ((pwm_cfg >> W83L786NG_PWM_ENABLE_SHIFT[nr]) & 3) + 1;
+}
+
+static void set_hw_pwm_enable(struct i2c_client *client, int nr, u8 val) {
+ u8 reg = w83l786ng_read_value(client, W83L786NG_REG_FAN_CFG);
+ reg &= ~(0x03 << W83L786NG_PWM_ENABLE_SHIFT[nr]);
+ reg |= (val - 1) << W83L786NG_PWM_ENABLE_SHIFT[nr];
+ w83l786ng_write_value(client, W83L786NG_REG_FAN_CFG, reg);
+}
+
+static void
+set_hw_pwm_point(struct i2c_client *client, int nr, u8 point, u8 val) {
+ int regofs = point / 2;
+ int regshift = (point % 2) * 4;
+ u8 reg = w83l786ng_read_value(client, W83L786NG_REG_PWM[nr] + regofs);
+ reg &= ~(0xf << regshift);
+ reg |= val << regshift;
+ w83l786ng_write_value(client, W83L786NG_REG_PWM[nr] + regofs, reg);
+}
+
+static u8 get_hw_pwm_ini(struct i2c_client *client) {
+ return w83l786ng_read_value(client, W83L786NG_REG_FANINI) & 0xf;
+}
+
+static ssize_t show_pwm_enable(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct w83l786ng_data *data = w83l786ng_update_device(dev);
+ int nr = to_sensor_dev_attr(attr)->index;
+ u8 enable_mode = data->pwm_enable[nr];
+ if (enable_mode == 4) {
+ /* present FAN_SET enable mode as manual */
+ enable_mode = 1;
+ }
+ return sprintf(buf, "%d\n", enable_mode);
+}
static ssize_t
store_pwm_mode(struct device *dev, struct device_attribute *attr,
@@ -492,8 +541,12 @@ show_pwm(struct device *dev, struct device_attribute *attr,
u8 reg = data->pwm[nr][point];
int val;
+ if (data->pwm_enable[nr] == 4 && point == 0) {
+ /* present FAN_SET enable mode as manual */
+ reg = data->pwm_ini;
+ }
/* map 0..15 -> 0..255 in a nice way, s.t. 0->0 and 15->255 */
- val = reg * 0x11;
+ val = reg * 0x11;
return sprintf(buf, "%d\n", val);
}
@@ -509,9 +562,6 @@ store_pwm(struct device *dev, struct device_attribute *attr,
struct w83l786ng_data *data = i2c_get_clientdata(client);
unsigned long val;
int err;
- int regofs = point / 2;
- int regshift = (point % 2) * 4;
- u8 reg;
err = kstrtoul(buf, 10, &val);
if (err)
@@ -525,10 +575,14 @@ store_pwm(struct device *dev, struct device_attribute *attr,
mutex_lock(&data->update_lock);
data->pwm[nr][point] = val;
- reg = w83l786ng_read_value(client, W83L786NG_REG_PWM[nr] + regofs);
- reg &= ~(0xf << regshift);
- reg |= val << regshift;
- w83l786ng_write_value(client, W83L786NG_REG_PWM[nr] + regofs, reg);
+ set_hw_pwm_point(client, nr, point, val);
+ /* Maybe we're a FAN_SET masquerading as manual, and someone
+ * just called our bluff by setting pwmX (aka point 0)
+ */
+ if (point == 0 && get_hw_pwm_enable(client, nr) == 4) {
+ /* Time to commit to being in manual mode. */
+ set_hw_pwm_enable(client, nr, 1);
+ }
mutex_unlock(&data->update_lock);
return count;
}
@@ -553,6 +607,16 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
mutex_lock(&data->update_lock);
data->pwm_enable[nr] = val;
+ /* Maybe we're a FAN_SET masquerading as manual, and someone
+ * just called our bluff by setting pwm_enable to 1.
+ */
+ if (val == 1 && get_hw_pwm_enable(client, nr) == 4) {
+ /* We're about to be in manual mode for real.
+ * So better set pwmX, aka point 0, to pwm_ini.
+ */
+ set_hw_pwm_point(client, nr, 0, get_hw_pwm_ini(client));
+ }
+
if (val == 0) { /* synthesize a 100% fan mode */
/* turn pwmX all the way up */
/* (but don't touch pwmX_auto_point_2 in high bits) */
@@ -562,10 +626,7 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
/* now tell hardware we're in manual mode */
val = 1;
}
- reg = w83l786ng_read_value(client, W83L786NG_REG_FAN_CFG);
- reg &= ~(0x03 << W83L786NG_PWM_ENABLE_SHIFT[nr]);
- reg |= (val - 1) << W83L786NG_PWM_ENABLE_SHIFT[nr];
- w83l786ng_write_value(client, W83L786NG_REG_FAN_CFG, reg);
+ set_hw_pwm_enable(client, nr, val);
mutex_unlock(&data->update_lock);
return count;
}
@@ -878,6 +939,8 @@ static struct w83l786ng_data *w83l786ng_update_device(struct device *dev)
}
}
+ /* Get the power on default duty cycle, for FAN_SET */
+ data->pwm_ini = get_hw_pwm_ini(client);
/* Update the temperature sensors */
for (i = 0; i < 2; i++) {
01_pwmX_enable_bugfix.diff
02_pwmX_bugfix.diff
03_pwmX_enable_synthetic_mode_0.diff
04_pwmX_enable_collapse_and_hide_fan_set_mode_4.diff
Signed-off-by: Brian Carnes <bmcarnes@xxxxxxxxx>
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
