[lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Brian Carnes]

Below is a description of two problems, some questions about how best
to resolve them, a transcript showing the behaviour, a minimal patch that
fixes just the first problem, and a transcript showing the improved
behaviour.

Problem #1:
There is a bug preventing setting the mode of the w83l786ng fan control.
sysfs will forever report mode 1.
The driver can do a one-way set of the hardware to mode 2, even though
that will never be reflected in sysfs.
It is then impossible to set hardware back to mode 1 via the driver.

Problem #2:
The "enable" and "mode" sysfs files are hooked up to the bitfields backward.
Do we want to fix this?
I'd worry about backward-compatibility, except that the current driver does
not
work with fan-control outside of mode 1, so perhaps there are no users of
the
advanced modes or PWM/DC switch, and thus no backward-compatibility
to break?  Although some people may have figured out the reversal, and
count on
setting pwmX_mode -> 0 to turn off PWM enable and drive the fan in DC mode?

While I'm submitting a patch to fix #1 and perhaps next for #2, are there
any
other updates or conventions to follow to normalize this driver against the
rest?
Perhaps:
  a) make the mode 0-based (0,1,2,3) instead of (1,2,forbidden,forbidden)
  b) make the mode named? (manual, cruise, smart, set)
  c) have the fan control naming convention not tied to 'pwm'
     (the chip supports pwm and DC modes.  I'm actually using it in DC mode)

In my own source tree, I've got a quick'n'dirty version exposing the duty
cycles and temp limits for thermal cruise and smart fan modes, permitting
modes 3 and 4, and supporting configurable voltage scaling.

Let me know your thoughts on problems #1 and #2 and naming conventions.
If the additional modes and functionality are desired, I can clean that up
and submit it, once the first round of questions and conventions are
settled.

Transcript:

# The original author is using pwmX_enable to control the FANx_MODE
#  fields, in bits 2-3, and 4-5 of register 0x80
# And is using pwmX_mode to control the EN_PWMx fields, in
#  bits 6, and 7.
# The naming is backwards, but there is also a bug after coping
#  with the reversal.
# Consider the following:

$ cat pwm1_enable pwm2_enable
1
1
$ i2cget -f -y 0 0x2e 0x80
0x00

# Everything is consistent so far.  The original author's comments in the
# code state that he allows the first two 'modes' in pwmX_enable, 1 and 2.
# His sysfs reflection of them are 1-based, so:
# bits 2-3 = 0b00 -> pwm1_enable = 0 (+1) = 1
# bits 4-5 = 0b00 -> pwm2_enable = 0 (+1) = 1
# and he is intentionally restricting us from modes 3 and 4.  Now:

$ echo 1 > pwm1_enable
$ cat pwm1_enable pwm2_enable
1
1
$ i2cget -f -y 0 0x2e 0x80
0x00

# Everything's still OK.  He's mapping his 1-based mode into the
# register correctly.  But now:

$ echo 2 > pwm1_enable
$ cat pwm1_enable pwm2_enable
1
1
$ i2cget -f -y 0 0x2e 0x80
0x04

# The bit got set - hardware is now in "mode 2" (thermal cruise)
# But the sysfs file still says we're in mode 1
# Let's try to go back:

$ echo 1 > pwm1_enable
$ cat pwm1_enable pwm2_enable
1
1
$ i2cget -f -y 0 0x2e 0x80
0x04

# Nope, we're now forever stuck in mode 2 on the hardware, but
# sysfs will forever say we're in mode 1.

# Note the power up default is actually mode 4, which is even more
# confusing (sysfs would tell you we're in mode 3, until you first
# touch the mode, then it would lock back into mode 1 behavior.
# For the above I made things less confusing for the reader by
# first forcing mode 1.)

A copy of the datasheet can be found at:

http://www.nuvoton.com/hq/enu/ProductAndSales/ProductLines/CloudAndComputingIC/HardwareMonitor/HWMonitorforGraphicCardNBAndIA/Documents/W83L786NG_W83L786NR.pdf

The pertinent section is:
   8.23 Fan Control Register - Index 80h, on page 30.

Possibly related mailing list reference to w83l786ng problems:
  http://marc.info/?l=lm-sensors&m\x136974114501850

Patch addressing just problem #1:
--- 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]);
                }
-----PATCH END-----

In short:
chunk 1 used to clear just bit 2 (so no modes 3 or 4), then always OR in
bit 1.
  Once set, no going back!  Now we clear both bits, and then OR in the mode.
chunk 2 used to always request bit 2 (which this driver always set to zero),
  and add one.  Thus you always appeared to be in "mode 1", unless the
hardware
  was in mode 3 or 4 by means other than the driver, then you would
  appear to be in mode 3...
  Now the driver will correctly report the mode (1, 2, 3, or 4).

Transcript after loading patched module:

$ sudo rmmod w83l786ng
$ sudo insmod ./w83l786ng.ko
$ cat pwm1_enable pwm2_enable
1
1
$ i2cget -f -y 0 0x2e 0x80
0x00
$ echo 1 > pwm1_enable
$ cat pwm1_enable pwm2_enable
1
1
$ i2cget -f -y 0 0x2e 0x80
0x00
$ echo 2 > pwm1_enable
$ cat pwm1_enable pwm2_enable
2
1
$ i2cget -f -y 0 0x2e 0x80
0x04
$ echo 1 > pwm1_enable
$ cat pwm1_enable pwm2_enable
1
1
$ i2cget -f -y 0 0x2e 0x80
0x00

Which is the expected behaviour.

Regards,

  Brian
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Jean Delvare]

Hi Brian,

On Fri, 29 Nov 2013 23:57:49 -0800, Brian Carnes wrote:
> Below is a description of two problems, some questions about how best
> to resolve them, a transcript showing the behaviour, a minimal patch that
> fixes just the first problem, and a transcript showing the improved
> behaviour.
> 
> Problem #1:
> There is a bug preventing setting the mode of the w83l786ng fan control.
> sysfs will forever report mode 1.
> The driver can do a one-way set of the hardware to mode 2, even though
> that will never be reflected in sysfs.
> It is then impossible to set hardware back to mode 1 via the driver.
> 
> Problem #2:
> The "enable" and "mode" sysfs files are hooked up to the bitfields backward.
> Do we want to fix this?

No. There is a standard for sysfs attributes of hwmon drivers. In this
standard, pwmN_mode is used to report PWM vs. DC output mode, and
pwmN_enable is used to report (and change) full-on vs. manual vs.
automatic fan speed control. See Documentation/hwmon/sysfs-interface
for the details.

It happens that the author of the W83L786NG datasheet used the term
"mode" for the latter and the term "en" for the former, which can lead
to some confusion. However the sysfs interface is and must stay
independent from datasheet wordings, else each driver would have its own
interface and libsensors and monitoring applications (and subsequently
users) would be lost.

> I'd worry about backward-compatibility, except that the current driver does
> not work with fan-control outside of mode 1, so perhaps there are no users of
> the advanced modes or PWM/DC switch, and thus no backward-compatibility
> to break?  Although some people may have figured out the reversal, and
> count on setting pwmX_mode -> 0 to turn off PWM enable and drive the fan in DC mode?

I certainly hope users are counting on it, as this is properly
implementing the standard sysfs interface. That being said, 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.

> While I'm submitting a patch to fix #1 and perhaps next for #2, are there
> any other updates or conventions to follow to normalize this driver against the
> rest?
> Perhaps:
>   a) make the mode 0-based (0,1,2,3) instead of (1,2,forbidden,forbidden)
>   b) make the mode named? (manual, cruise, smart, set)

Anything which doesn't follow the standard interface should be changed.
Anything which does follow the standard interface should be left
unchanged.

>   c) have the fan control naming convention not tied to 'pwm'
>      (the chip supports pwm and DC modes.  I'm actually using it in DC mode)

All fan speed control file names start with "pwm" for historical
reasons (first chips supported PWM only.) The names are arbitrary but
now they are part of the standard so they can no longer be changed.

> In my own source tree, I've got a quick'n'dirty version exposing the duty
> cycles and temp limits for thermal cruise and smart fan modes, permitting
> modes 3 and 4, and supporting configurable voltage scaling.
> 
> Let me know your thoughts on problems #1 and #2 and naming conventions.
> If the additional modes and functionality are desired, I can clean that up
> and submit it, once the first round of questions and conventions are
> settled.
> 
> Transcript:
> 
> # The original author is using pwmX_enable to control the FANx_MODE
> #  fields, in bits 2-3, and 4-5 of register 0x80
> # And is using pwmX_mode to control the EN_PWMx fields, in
> #  bits 6, and 7.
> # The naming is backwards,

As mentioned before, this is all OK.

> but there is also a bug after coping
> #  with the reversal.
> # Consider the following:
> 
> $ cat pwm1_enable pwm2_enable
> 1
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x00
> 
> # Everything is consistent so far.  The original author's comments in the
> # code state that he allows the first two 'modes' in pwmX_enable, 1 and 2.
> # His sysfs reflection of them are 1-based, so:
> # bits 2-3 = 0b00 -> pwm1_enable = 0 (+1) = 1
> # bits 4-5 = 0b00 -> pwm2_enable = 0 (+1) = 1
> # and he is intentionally restricting us from modes 3 and 4.  Now:
> 
> $ echo 1 > pwm1_enable
> $ cat pwm1_enable pwm2_enable
> 1
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x00
> 
> # Everything's still OK.  He's mapping his 1-based mode into the
> # register correctly.  But now:
> 
> $ echo 2 > pwm1_enable
> $ cat pwm1_enable pwm2_enable
> 1
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x04
> 
> # The bit got set - hardware is now in "mode 2" (thermal cruise)
> # But the sysfs file still says we're in mode 1
> # Let's try to go back:
> 
> $ echo 1 > pwm1_enable
> $ cat pwm1_enable pwm2_enable
> 1
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x04
> 
> # Nope, we're now forever stuck in mode 2 on the hardware, but
> # sysfs will forever say we're in mode 1.

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...

> # Note the power up default is actually mode 4, which is even more
> # confusing (sysfs would tell you we're in mode 3, until you first
> # touch the mode, then it would lock back into mode 1 behavior.
> # For the above I made things less confusing for the reader by
> # first forcing mode 1.)

Mode "FAN_SET" is problematic on its own, independent of the bug you
found and fixed. There is no room for it in our standard sysfs
interface. 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". 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.

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.

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?

> A copy of the datasheet can be found at:
> 
> http://www.nuvoton.com/hq/enu/ProductAndSales/ProductLines/CloudAndComputingIC/HardwareMonitor/HWMonitorforGraphicCardNBAndIA/Documents/W83L786NG_W83L786NR.pdf
> 
> The pertinent section is:
>    8.23 Fan Control Register - Index 80h, on page 30.
> 
> Possibly related mailing list reference to w83l786ng problems:
>   http://marc.info/?l=lm-sensors&m\x136974114501850

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.

> Patch addressing just problem #1:
> --- 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]);
>                 }
> -----PATCH END-----
> 
> In short:
> chunk 1 used to clear just bit 2 (so no modes 3 or 4), then always OR in
> bit 1.
>   Once set, no going back!  Now we clear both bits, and then OR in the mode.
> chunk 2 used to always request bit 2 (which this driver always set to zero),
>   and add one.  Thus you always appeared to be in "mode 1", unless the
> hardware
>   was in mode 3 or 4 by means other than the driver, then you would
>   appear to be in mode 3...
>   Now the driver will correctly report the mode (1, 2, 3, or 4).

Yes, I came up to the same conclusion, your patch is correct. It should
be backported to the stable kernel series as well. However formatting
was destroyed by your e-mail client. Please resend with long line
wrapping disabled (or as an attachment if you can't manage to do that.)
You also must add your Signed-off-by line to it as explained in
Documentation/SubmittingPatches section 12.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n307

> Transcript after loading patched module:
> 
> $ sudo rmmod w83l786ng
> $ sudo insmod ./w83l786ng.ko
> $ cat pwm1_enable pwm2_enable
> 1
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x00
> $ echo 1 > pwm1_enable
> $ cat pwm1_enable pwm2_enable
> 1
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x00
> $ echo 2 > pwm1_enable
> $ cat pwm1_enable pwm2_enable
> 2
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x04
> $ echo 1 > pwm1_enable
> $ cat pwm1_enable pwm2_enable
> 1
> 1
> $ i2cget -f -y 0 0x2e 0x80
> 0x00
> 
> Which is the expected behaviour.

Indeed. 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.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Kevin Lo]

Jean Delvare wrote:
> Hi Brian,
>
> On Fri, 29 Nov 2013 23:57:49 -0800, Brian Carnes wrote:
>

[snip]

> 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.
>
>> Patch addressing just problem #1:
>> --- 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]);
>>                  }
>> -----PATCH END-----
>>
>> In short:
>> chunk 1 used to clear just bit 2 (so no modes 3 or 4), then always OR in
>> bit 1.
>>    Once set, no going back!  Now we clear both bits, and then OR in the mode.
>> chunk 2 used to always request bit 2 (which this driver always set to zero),
>>    and add one.  Thus you always appeared to be in "mode 1", unless the
>> hardware
>>    was in mode 3 or 4 by means other than the driver, then you would
>>    appear to be in mode 3...
>>    Now the driver will correctly report the mode (1, 2, 3, or 4).
> Yes, I came up to the same conclusion, your patch is correct. It should
> be backported to the stable kernel series as well. However formatting
> was destroyed by your e-mail client. Please resend with long line
> wrapping disabled (or as an attachment if you can't manage to do that.)
> You also must add your Signed-off-by line to it as explained in
> Documentation/SubmittingPatches section 12.
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n307

First off, I'm sorry that I don't have any hardware with a w83l786ng chip.
Second, your patch looks correct to me after reading the datasheet and
the code.  Thanks for fixing it.

>
>> Transcript after loading patched module:
>>
>> $ sudo rmmod w83l786ng
>> $ sudo insmod ./w83l786ng.ko
>> $ cat pwm1_enable pwm2_enable
>> 1
>> 1
>> $ i2cget -f -y 0 0x2e 0x80
>> 0x00
>> $ echo 1 > pwm1_enable
>> $ cat pwm1_enable pwm2_enable
>> 1
>> 1
>> $ i2cget -f -y 0 0x2e 0x80
>> 0x00
>> $ echo 2 > pwm1_enable
>> $ cat pwm1_enable pwm2_enable
>> 2
>> 1
>> $ i2cget -f -y 0 0x2e 0x80
>> 0x04
>> $ echo 1 > pwm1_enable
>> $ cat pwm1_enable pwm2_enable
>> 1
>> 1
>> $ i2cget -f -y 0 0x2e 0x80
>> 0x00
>>
>> Which is the expected behaviour.
> Indeed. 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.

     Kevin

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Guenter Roeck]

On 12/01/2013 01:38 AM, Jean Delvare wrote:
[ ... ]
>
>> # Note the power up default is actually mode 4, which is even more
>> # confusing (sysfs would tell you we're in mode 3, until you first
>> # touch the mode, then it would lock back into mode 1 behavior.
>> # For the above I made things less confusing for the reader by
>> # first forcing mode 1.)
>
> Mode "FAN_SET" is problematic on its own, independent of the bug you
> found and fixed. There is no room for it in our standard sysfs
> interface. 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". 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.
>
> 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.
>
> 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?
>
Hi Jean,

The asc7621 driver uses pwmX_enable=0 to turn the fan off, and %5 to set
it to full speed. I would not necessarily take that as good example, though.

Your proposal to hide FAN_SET underneath mode 1 would be one possibility.
I would also not mind using pwmX_enable=0 ('disabled') for that purpose,
ie let the fan speed revert to the pre-programmed speed instead of 100%
if it is set. While not following the ABI to the letter, one could argue
that it follows the idea.

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Brian Carnes]

[-- Attachment #1: Type: text/plain, Size: 5473 bytes --]

> 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@gmail.com>

[-- Attachment #2: 01_pwmX_enable_bugfix.diff --]
[-- Type: text/plain, Size: 970 bytes --]

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]);
 		}

[-- Attachment #3: 02_pwmX_bugfix.diff --]
[-- Type: text/plain, Size: 4708 bytes --]

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;
+			}
 		}
 
 

[-- Attachment #4: 03_pwmX_enable_synthetic_mode_0.diff --]
[-- Type: text/plain, Size: 2667 bytes --]

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;
+			}
 		}
 
 

[-- Attachment #5: 04_pwmX_enable_collapse_and_hide_fan_set_mode_4.diff --]
[-- Type: text/plain, Size: 5658 bytes --]

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++) {

[-- Attachment #6: signed-off-by.txt --]
[-- Type: text/plain, Size: 186 bytes --]

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@gmail.com>

[-- Attachment #7: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Jean Delvare]

Hi Brian,

On Sun, 1 Dec 2013 21:21:29 -0800, Brian Carnes wrote:
> > 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 understand :)

> 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...

Sysfs is not the proper interface to work around the lack of proper
BIOS-style initialization. Embedded systems should provide device
initialization data and the driver should initialize the chip based on
that. Leaving the responsibility to the user is insane. I would even
argue that, in the case of fans, leaving the responsibility to the OS
is quite dangerous already.

> > 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.

Got it, thanks. For next time:
* Each patch should come with a subject and a description.
* Each patch should come with its own Signed-off-by line.
* It's much easier for me if the patches can be applied with patch -p1,
  i.e. the file paths in the patch header includes the directory where
  the source files live.
* No compressed tarballs please. Patches are preferred inline or as
  plain-text attachments.

I did it this time, the result is:
http://khali.linux-fr.org/devel/linux-3/jdelvare-hwmon/hwmon-w83l786ng-01-fix-pwmX_enable.patch

> > 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.

It isn't necessary, we have a lot of drivers already which do not
implement mode 0, exactly because the hardware itself does not.
User-space already knows how to deal with this. In other words, I am
not interested in 03_pwmX_enable_synthetic_mode_0.diff.

> > 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.

This is generally desirable indeed. However there are a few cases where
transparently reconfiguring the chip at driver load time allows for
run-time code simplification. This is allowed as long as there is no
side effect.

> > 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.

Yes, you are correct.

> > 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.

I would do the transition from FAN_SET to manual transparently at
driver initialization time, to make the run-time code as fast as
possible. But I understand and respect your views on this.

> > 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.

As a side note, since Lavabit is down *sigh*, I'm not sure if Richard
can actually read us.

> 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

Doh, this is very bad :(

> 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

Much better... Unfortunately your patch is too large to qualify for
stable kernel series. We'll have to find a more simple fix which really
only addresses the immediate problem. The rest of the code can go in a
separate patch, to go in kernel 3.13+ but not older stable kernel
series.

If I'm not mistaken, the following minimal fix should work:

From: Jean Delvare <khali@linux-fr.org>
Subject: hwmon: (w83l768ng) Fix fan speed control range

The W83L786NG stores the fan speed on 4 bits while the sysfs interface
uses a 0-255 range. Thus the driver should scale the user input down
to map it to the device range, and scale up the value read from the
device before presenting it to the user.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: stable@vger.kernel.org
---
 drivers/hwmon/w83l786ng.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- linux-3.13-rc2.orig/drivers/hwmon/w83l786ng.c	2013-12-02 09:14:56.247009803 +0100
+++ linux-3.13-rc2/drivers/hwmon/w83l786ng.c	2013-12-02 10:07:49.718037923 +0100
@@ -481,6 +481,7 @@ store_pwm(struct device *dev, struct dev
 	if (err)
 		return err;
 	val = clamp_val(val, 0, 255);
+	val = DIV_ROUND_CLOSEST(val, 0x11);
 
 	mutex_lock(&data->update_lock);
 	data->pwm[nr] = val;
@@ -777,8 +778,9 @@ static struct w83l786ng_data *w83l786ng_
 			    ? 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]);
+			data->pwm[i] +			    (w83l786ng_read_value(client, W83L786NG_REG_PWM[i])
+			     & 0x0f) * 0x11;
 		}
 
 
Please test and confirm.

> >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.

It's all OK, no worry. Be persistent and I'll be patient :)

> Between gmail and the mailing list software, don't know who's mangling.

That would be gmail, the mailing list software does not. It might be
configurable, I don't know, I don't use gmail.

> I'll just attach the patches as files to be safe.

This is OK with me.

> 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.

Yes.

> 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.

Fine with me.

> 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.

Indeed... Same as for every other device BTW.

> I'm also open to exposing FAN_INI value directly in sysfs, if you had a
> preferred naming convention for that.

No, we don't have a standard file name for that and I don't think it
makes sense to craft one for just this rare device.

So, please confirm that my patch above is enough to fix the second bug.
Then rebase your second patch on top of it, drop your third patch,
rebase the 4th patch on top of the result, and resubmit the two rebased
patches for me to review (could be in a separate discussion thread.)

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Jean Delvare]

Hi Brian,

On Mon, 2 Dec 2013 10:17:56 +0100, Jean Delvare wrote:
> If I'm not mistaken, the following minimal fix should work:
> 
> From: Jean Delvare <khali@linux-fr.org>
> Subject: hwmon: (w83l768ng) Fix fan speed control range
> 
> The W83L786NG stores the fan speed on 4 bits while the sysfs interface
> uses a 0-255 range. Thus the driver should scale the user input down
> to map it to the device range, and scale up the value read from the
> device before presenting it to the user.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/hwmon/w83l786ng.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> --- linux-3.13-rc2.orig/drivers/hwmon/w83l786ng.c	2013-12-02 09:14:56.247009803 +0100
> +++ linux-3.13-rc2/drivers/hwmon/w83l786ng.c	2013-12-02 10:07:49.718037923 +0100
> @@ -481,6 +481,7 @@ store_pwm(struct device *dev, struct dev
>  	if (err)
>  		return err;
>  	val = clamp_val(val, 0, 255);
> +	val = DIV_ROUND_CLOSEST(val, 0x11);
>  
>  	mutex_lock(&data->update_lock);
>  	data->pwm[nr] = val;
> @@ -777,8 +778,9 @@ static struct w83l786ng_data *w83l786ng_
>  			    ? 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]);
> +			data->pwm[i] > +			    (w83l786ng_read_value(client, W83L786NG_REG_PWM[i])
> +			     & 0x0f) * 0x11;
>  		}
>  
>  
> Please test and confirm.

I was actually mistaken, my patch had a minor flaw that caused the
driver to temporarily return the wrong pwmN value right after setting
it. Additionally it was overwriting reserved bits in registers
W83L786NG_REG_PWM, which is bad. Patch V2 below fixes both issues:

From: Jean Delvare <khali@linux-fr.org>
Subject: hwmon: (w83l768ng) Fix fan speed control range

The W83L786NG stores the fan speed on 4 bits while the sysfs interface
uses a 0-255 range. Thus the driver should scale the user input down
to map it to the device range, and scale up the value read from the
device before presenting it to the user. The reserved register nibble
should be left unchanged.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: stable@vger.kernel.org
---
Changes in v2:
 * Scale data->pwm[nr] up when writing a new value to the
   W83L786NG_REG_PWM[nr] register.
 * Don't overwrite the 4 MSB of W83L786NG_REG_PWM.

 drivers/hwmon/w83l786ng.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--- linux-3.13-rc3.orig/drivers/hwmon/w83l786ng.c	2013-12-11 15:01:33.162221180 +0100
+++ linux-3.13-rc3/drivers/hwmon/w83l786ng.c	2013-12-11 15:22:36.004771774 +0100
@@ -481,9 +481,11 @@ store_pwm(struct device *dev, struct dev
 	if (err)
 		return err;
 	val = clamp_val(val, 0, 255);
+	val = DIV_ROUND_CLOSEST(val, 0x11);
 
 	mutex_lock(&data->update_lock);
-	data->pwm[nr] = val;
+	data->pwm[nr] = val * 0x11;
+	val |= w83l786ng_read_value(client, W83L786NG_REG_PWM[nr]) & 0xf0;
 	w83l786ng_write_value(client, W83L786NG_REG_PWM[nr], val);
 	mutex_unlock(&data->update_lock);
 	return count;
@@ -777,8 +779,9 @@ static struct w83l786ng_data *w83l786ng_
 			    ? 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]);
+			data->pwm[i] +			    (w83l786ng_read_value(client, W83L786NG_REG_PWM[i])
+			     & 0x0f) * 0x11;
 		}
 
Again, testing would be appreciated.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] w83l786ng driver bug, questions, and 1st round patch

[Guenter Roeck]

On 12/11/2013 06:33 AM, Jean Delvare wrote:
> Hi Brian,
>
> On Mon, 2 Dec 2013 10:17:56 +0100, Jean Delvare wrote:
>> If I'm not mistaken, the following minimal fix should work:
>>
>> From: Jean Delvare <khali@linux-fr.org>
>> Subject: hwmon: (w83l768ng) Fix fan speed control range
>>
>> The W83L786NG stores the fan speed on 4 bits while the sysfs interface
>> uses a 0-255 range. Thus the driver should scale the user input down
>> to map it to the device range, and scale up the value read from the
>> device before presenting it to the user.
>>
>> Signed-off-by: Jean Delvare <khali@linux-fr.org>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/hwmon/w83l786ng.c |    6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> --- linux-3.13-rc2.orig/drivers/hwmon/w83l786ng.c	2013-12-02 09:14:56.247009803 +0100
>> +++ linux-3.13-rc2/drivers/hwmon/w83l786ng.c	2013-12-02 10:07:49.718037923 +0100
>> @@ -481,6 +481,7 @@ store_pwm(struct device *dev, struct dev
>>   	if (err)
>>   		return err;
>>   	val = clamp_val(val, 0, 255);
>> +	val = DIV_ROUND_CLOSEST(val, 0x11);
>>
>>   	mutex_lock(&data->update_lock);
>>   	data->pwm[nr] = val;
>> @@ -777,8 +778,9 @@ static struct w83l786ng_data *w83l786ng_
>>   			    ? 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]);
>> +			data->pwm[i] >> +			    (w83l786ng_read_value(client, W83L786NG_REG_PWM[i])
>> +			     & 0x0f) * 0x11;
>>   		}
>>
>>
>> Please test and confirm.
>
> I was actually mistaken, my patch had a minor flaw that caused the
> driver to temporarily return the wrong pwmN value right after setting
> it. Additionally it was overwriting reserved bits in registers
> W83L786NG_REG_PWM, which is bad. Patch V2 below fixes both issues:
>
> From: Jean Delvare <khali@linux-fr.org>
> Subject: hwmon: (w83l768ng) Fix fan speed control range
>
> The W83L786NG stores the fan speed on 4 bits while the sysfs interface
> uses a 0-255 range. Thus the driver should scale the user input down
> to map it to the device range, and scale up the value read from the
> device before presenting it to the user. The reserved register nibble
> should be left unchanged.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: stable@vger.kernel.org
> ---
> Changes in v2:
>   * Scale data->pwm[nr] up when writing a new value to the
>     W83L786NG_REG_PWM[nr] register.
>   * Don't overwrite the 4 MSB of W83L786NG_REG_PWM.
>

Looks reasonable.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

>   drivers/hwmon/w83l786ng.c |    9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> --- linux-3.13-rc3.orig/drivers/hwmon/w83l786ng.c	2013-12-11 15:01:33.162221180 +0100
> +++ linux-3.13-rc3/drivers/hwmon/w83l786ng.c	2013-12-11 15:22:36.004771774 +0100
> @@ -481,9 +481,11 @@ store_pwm(struct device *dev, struct dev
>   	if (err)
>   		return err;
>   	val = clamp_val(val, 0, 255);
> +	val = DIV_ROUND_CLOSEST(val, 0x11);
>
>   	mutex_lock(&data->update_lock);
> -	data->pwm[nr] = val;
> +	data->pwm[nr] = val * 0x11;
> +	val |= w83l786ng_read_value(client, W83L786NG_REG_PWM[nr]) & 0xf0;
>   	w83l786ng_write_value(client, W83L786NG_REG_PWM[nr], val);
>   	mutex_unlock(&data->update_lock);
>   	return count;
> @@ -777,8 +779,9 @@ static struct w83l786ng_data *w83l786ng_
>   			    ? 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]);
> +			data->pwm[i] > +			    (w83l786ng_read_value(client, W83L786NG_REG_PWM[i])
> +			     & 0x0f) * 0x11;
>   		}
>
> Again, testing would be appreciated.
>
> Thanks,
>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors