[lm-sensors] Additional PWM driver support for w83792d

[lm-sensors] Additional PWM driver support for w83792d

[vt8231]

Dear LM-Sensors,

I have a Dell FS12-NV7 server and it has two W83792G chips on it (as well as
an IT87).  One of the W83792G devices is connected to three fans on PWM
outputs 3,4 and 5 respectively.

I am running with Ubuntu 15.04 (Linux 3.19.0) and the W83792D driver only
has support for PWM outputs 1,2 and 3.

The patch below adds the 4 missing PWM outputs which are supported by the
chip.  This has been tested and works as expected on my motherboard.

--- linux-3.19.0/drivers/hwmon/w83792d.c        2015-02-09
02:54:22.000000000 +0000
+++ linux-3.19.0-new/drivers/hwmon/w83792d.c    2015-05-08
21:53:06.637515282 +0100
@@ -1075,6 +1075,10 @@
 static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0);
 static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1);
 static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2);
+static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3);
+static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 4);
+static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 5);
+static SENSOR_DEVICE_ATTR(pwm7, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 6);
 static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
                        show_pwmenable, store_pwmenable, 1);
 static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
@@ -1270,6 +1274,10 @@
        &sensor_dev_attr_pwm3.dev_attr.attr,
        &sensor_dev_attr_pwm3_mode.dev_attr.attr,
        &sensor_dev_attr_pwm3_enable.dev_attr.attr,
+       &sensor_dev_attr_pwm4.dev_attr.attr,
+       &sensor_dev_attr_pwm5.dev_attr.attr,
+       &sensor_dev_attr_pwm6.dev_attr.attr,
+       &sensor_dev_attr_pwm7.dev_attr.attr,
        &dev_attr_alarms.attr,
        &dev_attr_intrusion0_alarm.attr,
        &sensor_dev_attr_tolerance1.dev_attr.attr,

I hope this patch is of use to others who are using the same server or have
a similar situation.

Best regards,

Roger Lucas


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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Jean Delvare]

Hi Roger,

Good to see you again :)

On Sat, 9 May 2015 13:57:41 +0100, vt8231@hiddenengine.co.uk wrote:
> Dear LM-Sensors,
> 
> I have a Dell FS12-NV7 server and it has two W83792G chips on it (as well as
> an IT87).  One of the W83792G devices is connected to three fans on PWM
> outputs 3,4 and 5 respectively.
> 
> I am running with Ubuntu 15.04 (Linux 3.19.0) and the W83792D driver only
> has support for PWM outputs 1,2 and 3.
> 
> The patch below adds the 4 missing PWM outputs which are supported by the
> chip.  This has been tested and works as expected on my motherboard.

It's amazing that nobody ever noticed the missing pwm files before,
thanks for reporting.

> 
> --- linux-3.19.0/drivers/hwmon/w83792d.c        2015-02-09
> 02:54:22.000000000 +0000
> +++ linux-3.19.0-new/drivers/hwmon/w83792d.c    2015-05-08
> 21:53:06.637515282 +0100
> @@ -1075,6 +1075,10 @@
>  static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0);
>  static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1);
>  static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2);
> +static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3);
> +static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 4);
> +static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 5);
> +static SENSOR_DEVICE_ATTR(pwm7, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 6);
>  static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
>                         show_pwmenable, store_pwmenable, 1);
>  static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
> @@ -1270,6 +1274,10 @@
>         &sensor_dev_attr_pwm3.dev_attr.attr,
>         &sensor_dev_attr_pwm3_mode.dev_attr.attr,
>         &sensor_dev_attr_pwm3_enable.dev_attr.attr,
> +       &sensor_dev_attr_pwm4.dev_attr.attr,
> +       &sensor_dev_attr_pwm5.dev_attr.attr,
> +       &sensor_dev_attr_pwm6.dev_attr.attr,
> +       &sensor_dev_attr_pwm7.dev_attr.attr,
>         &dev_attr_alarms.attr,
>         &dev_attr_intrusion0_alarm.attr,
>         &sensor_dev_attr_tolerance1.dev_attr.attr,
> 
> I hope this patch is of use to others who are using the same server or have
> a similar situation.

Yes we want to apply something like this. However your e-mail client
broke the formatting. Any chance you can resend using a client which
does not mangle white space nor fold long lines?

I think the new attributes should go in w83792d_attributes_fan rather
than w83792d_attributes, as they are not always enabled (mutually
exclusive with other functions.)

Your patch should also add files pwm[4-7]_mode.

Also there is this comment in the code:

	u8 pwm[7];		/*
				 * We only consider the first 3 set of pwm,
				 * although 792 chip has 7 set of pwm.
				 */

which should be removed. Documentation/hwmon/w83792d should be updated
accordingly, as it currently claims that fan outputs 4-7 aren't
supported and doesn't mention attributes pwm[4-7] and pwm[4-7]_mode.

We'll need your Signed-off-by line so that the patch can be applied.

Last note: registers 0xA3-0xA6 have extra configuration bits "Sync
T1/2/3". Maybe the driver should handle them but I am not sure how. It
could be that the extra outputs should only be exposed to user-space if
these bits are 0 (stand alone.) Guenter, any idea/opinion on this?

-- 
Jean Delvare
SUSE L3 Support

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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Guenter Roeck]

On 05/09/2015 05:57 AM, vt8231@hiddenengine.co.uk wrote:
> Dear LM-Sensors,
>
> I have a Dell FS12-NV7 server and it has two W83792G chips on it (as well as
> an IT87).  One of the W83792G devices is connected to three fans on PWM
> outputs 3,4 and 5 respectively.
>
> I am running with Ubuntu 15.04 (Linux 3.19.0) and the W83792D driver only
> has support for PWM outputs 1,2 and 3.
>
> The patch below adds the 4 missing PWM outputs which are supported by the
> chip.  This has been tested and works as expected on my motherboard.
>
> --- linux-3.19.0/drivers/hwmon/w83792d.c        2015-02-09
> 02:54:22.000000000 +0000
> +++ linux-3.19.0-new/drivers/hwmon/w83792d.c    2015-05-08
> 21:53:06.637515282 +0100
> @@ -1075,6 +1075,10 @@
>   static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0);
>   static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1);
>   static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2);
> +static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3);
> +static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 4);
> +static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 5);
> +static SENSOR_DEVICE_ATTR(pwm7, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 6);
>   static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
>                          show_pwmenable, store_pwmenable, 1);
>   static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
> @@ -1270,6 +1274,10 @@
>          &sensor_dev_attr_pwm3.dev_attr.attr,
>          &sensor_dev_attr_pwm3_mode.dev_attr.attr,
>          &sensor_dev_attr_pwm3_enable.dev_attr.attr,
> +       &sensor_dev_attr_pwm4.dev_attr.attr,
> +       &sensor_dev_attr_pwm5.dev_attr.attr,
> +       &sensor_dev_attr_pwm6.dev_attr.attr,
> +       &sensor_dev_attr_pwm7.dev_attr.attr,
>          &dev_attr_alarms.attr,
>          &dev_attr_intrusion0_alarm.attr,
>          &sensor_dev_attr_tolerance1.dev_attr.attr,
>

Hi Roger,

unfortunately it is not that simple.

The chip's io pins for pwm4 .. pwm7 are, like the respective fan status
pins, multi-function pins. You'll have to add the pointers to
sensor_dev_attr_pwm[4..7] to w83792d_attributes_fan[].

Also, I think you should also add pwm[4..7]_mode since it follows the same
rules as pwm[4..7] and uses the same registers.

Last but not least, can you send the patch in a form that can be applied,
as outlined in Documentation/SubmittingPatches ?

Thanks,
Guenter


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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Guenter Roeck]

On 05/09/2015 08:30 AM, Jean Delvare wrote:
> Hi Roger,
>
> Good to see you again :)
>
> On Sat, 9 May 2015 13:57:41 +0100, vt8231@hiddenengine.co.uk wrote:
>> Dear LM-Sensors,
>>
>> I have a Dell FS12-NV7 server and it has two W83792G chips on it (as well as
>> an IT87).  One of the W83792G devices is connected to three fans on PWM
>> outputs 3,4 and 5 respectively.
>>
>> I am running with Ubuntu 15.04 (Linux 3.19.0) and the W83792D driver only
>> has support for PWM outputs 1,2 and 3.
>>
>> The patch below adds the 4 missing PWM outputs which are supported by the
>> chip.  This has been tested and works as expected on my motherboard.
>
> It's amazing that nobody ever noticed the missing pwm files before,
> thanks for reporting.
>
>>
>> --- linux-3.19.0/drivers/hwmon/w83792d.c        2015-02-09
>> 02:54:22.000000000 +0000
>> +++ linux-3.19.0-new/drivers/hwmon/w83792d.c    2015-05-08
>> 21:53:06.637515282 +0100
>> @@ -1075,6 +1075,10 @@
>>   static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0);
>>   static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1);
>>   static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2);
>> +static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3);
>> +static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 4);
>> +static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 5);
>> +static SENSOR_DEVICE_ATTR(pwm7, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 6);
>>   static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
>>                          show_pwmenable, store_pwmenable, 1);
>>   static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
>> @@ -1270,6 +1274,10 @@
>>          &sensor_dev_attr_pwm3.dev_attr.attr,
>>          &sensor_dev_attr_pwm3_mode.dev_attr.attr,
>>          &sensor_dev_attr_pwm3_enable.dev_attr.attr,
>> +       &sensor_dev_attr_pwm4.dev_attr.attr,
>> +       &sensor_dev_attr_pwm5.dev_attr.attr,
>> +       &sensor_dev_attr_pwm6.dev_attr.attr,
>> +       &sensor_dev_attr_pwm7.dev_attr.attr,
>>          &dev_attr_alarms.attr,
>>          &dev_attr_intrusion0_alarm.attr,
>>          &sensor_dev_attr_tolerance1.dev_attr.attr,
>>
>> I hope this patch is of use to others who are using the same server or have
>> a similar situation.
>
> Yes we want to apply something like this. However your e-mail client
> broke the formatting. Any chance you can resend using a client which
> does not mangle white space nor fold long lines?
>
> I think the new attributes should go in w83792d_attributes_fan rather
> than w83792d_attributes, as they are not always enabled (mutually
> exclusive with other functions.)
>
> Your patch should also add files pwm[4-7]_mode.
>
> Also there is this comment in the code:
>
> 	u8 pwm[7];		/*
> 				 * We only consider the first 3 set of pwm,
> 				 * although 792 chip has 7 set of pwm.
> 				 */
>
> which should be removed. Documentation/hwmon/w83792d should be updated
> accordingly, as it currently claims that fan outputs 4-7 aren't
> supported and doesn't mention attributes pwm[4-7] and pwm[4-7]_mode.
>
> We'll need your Signed-off-by line so that the patch can be applied.
>
> Last note: registers 0xA3-0xA6 have extra configuration bits "Sync
> T1/2/3". Maybe the driver should handle them but I am not sure how. It
> could be that the extra outputs should only be exposed to user-space if
> these bits are 0 (stand alone.) Guenter, any idea/opinion on this?
>

Hi Jean,

Good point.

How about using pwm[4567]_enable ? If I understand correctly, the possible
modes would be manual or sync(x). In this case we could have 1 (manual),
2 (sync with fan1), 3 (sync with fan2), and 4 (sync with fan3), with the
caveat that the sync settings only make sense if the matching pwmX_enable
is set to thermal cruise mode.

Does this make sense ?

Btw, do you have a datasheet for the chip with 7 fan controls/status pins ?

Thanks,
Guenter


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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Guenter Roeck]

On 05/10/2015 06:42 AM, vt8231@hiddenengine.co.uk wrote:
[ ... ]
>>>
>>> Last note: registers 0xA3-0xA6 have extra configuration bits "Sync
>>> T1/2/3". Maybe the driver should handle them but I am not sure how. It
>>> could be that the extra outputs should only be exposed to user-space if
>>> these bits are 0 (stand alone.) Guenter, any idea/opinion on this?
>>>
>>
>> Hi Jean,
>>
>> Good point.
>>
>> How about using pwm[4567]_enable ? If I understand correctly, the possible
>> modes would be manual or sync(x). In this case we could have 1 (manual),
>> 2 (sync with fan1), 3 (sync with fan2), and 4 (sync with fan3), with the
>> caveat that the sync settings only make sense if the matching pwmX_enable
>> is set to thermal cruise mode.
>>
>> Does this make sense ?
>
> I don't think you have that level of control over PWM 4-7.  From what I can see
> in the datasheet (section 8.21 of the W83792G manual), you only have Thermal and
> Smart FAN modes for PWM 1 - 3.  This is why the existing driver does a limit
> check when setting the mode so that it only allows PWM[1-3] to be controlled.
>
> If I am reading the datasheet correctly, you have a simple enable/disable for PWM7
> (aka FAN7 Enable) and PWM6 (aka FAN6) in register 0x4B (Bank 0) "Pin Control
> Register", see section 8.13.  Likewise, you have a simple enable/disable for
> PWM5 (aka FAN_OUT5) and PWM4 (aka FAN_OUT4) via bits 6 and 5 of register 0x1A
> (CR1A_GPIO Enable).  FAN_OUT4 shares with GPIOA0 and FAN_OUT5 shares with GPIOA2.
>
> I would be nervous of changing these in Linux, however.  These relate to the HW
> configuration of the board so I would expect the BIOS to set these up correctly
> to ensure that the HW monitoring was correct from power-on.  My preference would
> be to *read* these values and only offer the /hwmonX/device/pwm[4-7] virtual
> files if the registers had *already* been set to enable this mode.  I don't like
> the idea that you could accidentally turn a GPIO input into a PWM output if you
> misconfigured the pwm[4-7]_enable signals (which you could do when probing, for
> example).
>
> My preference is *not* to provide pwm[4-7]_enable controls but just check the
> existing chip config and make pwm[4-7] and pwm[4-7]_mode available if (and only
> if) the chip has already been configured to enable them.
>

Roger,

Jean specifically suggested adding support for 'registers 0xA3-0xA6 have extra
configuration bits "Sync T1/2/3"'. I suggested to support those through pwmX_enable,
nothing else. You are right, you don't want to change the configuration bits
you referred to. But that is not what I suggested.

Thanks,
Guenter


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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Guenter Roeck]

On 05/10/2015 10:48 AM, vt8231@hiddenengine.co.uk wrote:
>>
>> Roger,
>>
>> Jean specifically suggested adding support for 'registers 0xA3-0xA6 have extra
>> configuration bits "Sync T1/2/3"'. I suggested to support those through pwmX_enable,
>> nothing else. You are right, you don't want to change the configuration bits
>> you referred to. But that is not what I suggested.
>>
>> Thanks,
>> Guenter
>
> I apologise - I completely misread your post and hadn't realised what you and Jean
> were referring to.
>
> Is the following list of changes that was meant:
>
> 1) Only make the pwm[4-7] and pwm[4-7]_enable files present if the corresponding
>     enable flags are set in registers 0x1A and 0x4B.  There would be individual
>     checks for pwm4 + pwm4_enable, pwm5 + pwm5_enable, pwm6 + pwm6_enable and pwm7
>     + pwm7_enable.
>
Correct, plus pwm[4-7]_mode, though the code to do this check is already there.
All you need to do is to add the new attributes to w83792d_attributes_fan[].

> 2) Add new pwm_enable modes "Sync T1" (=4), "Sync T2" (=5) and "Sync T3" (=6) for
>     PWM 4-7.  I propose the new values so that they don't collide with the existing
>     pwm_enable modes for pwm[1-3].  These would not be supported for pwm[1-3] and
>     likewise the existing pwm_enable modes would not be supported for pwm[4-7]
>     (apart from the manual mode).
>
Correct. Not sure what the new values should be (4-6 or 1-3). 4-6 may be better.
Jean, what do you think ?

> 3) Refuse to allow "Sync T1" to be set unless PWM1 is in thermal cruise mode (=3)
>     and instead set "stand alone/manual mode" (=1)
>
> 4) Refuse to allow "Sync T2" to be set unless PWM2 is in thermal cruise mode (=3)
>     and instead set "stand alone/manual mode" (=1)
>
> 5) Refuse to allow "Sync T3" to be set unless PWM3 is in thermal cruise mode (=3)
>     and instead set "stand alone/manual mode" (=1)
>

Problem is that pwm[1-3]_enable can be changed _after_ pwm[4-7]_enable was
configured. I would tend to let the user just set pwm[4-7]_enable, and add
a note to the documentation describing what happens if the mantching pwmX_enable
is not set to thermal cruse mode.

Jean, any suggestion ?

Thanks,
Guenter

> 6) Clean up comments as per Jean's mail
>
> If so, I think I know what to do and should be able to make the changes and have
> a new patch available this week.  Can you point me to the correct reference code
> for the w83792d driver so that my patch applies cleanly?  My guess would be the
> V4.1-rc2 release on kernel.org?
>
> I'll be testing on Ubuntu 15.04 (which is kernel 3.19.0) but hopefully the delta
> is minimal for this driver between 3.19.0 and 4.1?
>
> Best regards,
>
> Roger
>
>
>


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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Jean Delvare]

On Sun, 10 May 2015 09:01:25 -0700, Guenter Roeck wrote:
> On 05/10/2015 06:42 AM, vt8231@hiddenengine.co.uk wrote:
> [ ... ]
> >>>
> >>> Last note: registers 0xA3-0xA6 have extra configuration bits "Sync
> >>> T1/2/3". Maybe the driver should handle them but I am not sure how. It
> >>> could be that the extra outputs should only be exposed to user-space if
> >>> these bits are 0 (stand alone.) Guenter, any idea/opinion on this?
> >>
> >> How about using pwm[4567]_enable ? If I understand correctly, the possible
> >> modes would be manual or sync(x). In this case we could have 1 (manual),
> >> 2 (sync with fan1), 3 (sync with fan2), and 4 (sync with fan3), with the
> >> caveat that the sync settings only make sense if the matching pwmX_enable
> >> is set to thermal cruise mode.
> >>
> >> Does this make sense ?

Not really. The problem is that for now I'm not sure what "sync" really
refers to. The datasheet mentions temperature channels, and as far as I
can see each temperature channel is hard-bound to a specific fan
output. So I suspect that what these configuration bit really mean is
that the pwm4 output (for example) mirrors the pwm1 output, i.e. pwm4
has no independent existence. In which case it's better to not expose
it at all.

If the BIOS specifically configured these bits, there must be a reason
and that reason would be the way the fans are connected to the chipset.
Better not change it.

If we really want to expose these bits then abusing pwmX_enable the way
you suggested is still not correct, pwmX_auto_channels_temp would be
better suited. But then again I don't think it adds any value if the
extra PWM outputs only mirror already existing PWM outputs.

> > I don't think you have that level of control over PWM 4-7.  From what I can see
> > in the datasheet (section 8.21 of the W83792G manual), you only have Thermal and
> > Smart FAN modes for PWM 1 - 3.  This is why the existing driver does a limit
> > check when setting the mode so that it only allows PWM[1-3] to be controlled.
> >
> > If I am reading the datasheet correctly, you have a simple enable/disable for PWM7
> > (aka FAN7 Enable) and PWM6 (aka FAN6) in register 0x4B (Bank 0) "Pin Control
> > Register", see section 8.13.  Likewise, you have a simple enable/disable for
> > PWM5 (aka FAN_OUT5) and PWM4 (aka FAN_OUT4) via bits 6 and 5 of register 0x1A
> > (CR1A_GPIO Enable).  FAN_OUT4 shares with GPIOA0 and FAN_OUT5 shares with GPIOA2.
> >
> > I would be nervous of changing these in Linux, however.

We definitely don't want to change these.

> > These relate to the HW
> > configuration of the board so I would expect the BIOS to set these up correctly
> > to ensure that the HW monitoring was correct from power-on.  My preference would
> > be to *read* these values and only offer the /hwmonX/device/pwm[4-7] virtual
> > files if the registers had *already* been set to enable this mode.  I don't like
> > the idea that you could accidentally turn a GPIO input into a PWM output if you
> > misconfigured the pwm[4-7]_enable signals (which you could do when probing, for
> > example).
> >
> > My preference is *not* to provide pwm[4-7]_enable controls but just check the
> > existing chip config and make pwm[4-7] and pwm[4-7]_mode available if (and only
> > if) the chip has already been configured to enable them.

+1

> Jean specifically suggested adding support for 'registers 0xA3-0xA6 have extra
> configuration bits "Sync T1/2/3"'.

Err, not really. What I meant is that these configuration bits must
have some effect that we can't ignore. But like Roger I would go for
the minimum checks and either expose pwm[4-7] in manual mode (including
pwm[4_7]_mode) if the chip was configured that way, or not expose them
at all. I don't think it makes sense to let the user change these sync
T1/2/3 bits.

This feeling is reinforced by the fact that the W83792D/G chip is
getting old so improving it is unlikely to benefit a large number of
users. So let's do whatever Roger needs for himself, and not much more.
We simply need to ensure we don't break anything for other users.

> I suggested to support those through pwmX_enable,
> nothing else. You are right, you don't want to change the configuration bits
> you referred to. But that is not what I suggested.

-- 
Jean Delvare
SUSE L3 Support

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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Jean Delvare]

On Sun, 10 May 2015 14:42:18 +0100, vt8231@hiddenengine.co.uk wrote:
> With regards to generating the patch, I've checked the LM-Sensors Wiki for
> submitting patches (http://lm-sensors.org/wiki/FAQ/Chapter5) and it refers to
> CVS as the reference... but I can't see the chip drivers there.  As far as I
> can tell, they are in the Linux kernel itself.  Where is the reference source 
> code for the chip drivers?  I patch that I offered above was against the 
> Ubuntu source for kernel 3.19.0 but this isn't the latest kernel...

The FAQ is old, and only refers to the user-space side of things now.
So you can ignore it. Better read Documentation/SubmittingPatches and
Documentation/hwmon/submitting-patches in the kernel tree.

A patch against kernel 3.19.0 will be just fine, the w83792d driver
didn't see any change for over a year.

-- 
Jean Delvare
SUSE L3 Support

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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Guenter Roeck]

On 05/11/2015 03:45 AM, Jean Delvare wrote:
> On Sun, 10 May 2015 09:01:25 -0700, Guenter Roeck wrote:
>> On 05/10/2015 06:42 AM, vt8231@hiddenengine.co.uk wrote:
>> [ ... ]
>>>>>
>>>>> Last note: registers 0xA3-0xA6 have extra configuration bits "Sync
>>>>> T1/2/3". Maybe the driver should handle them but I am not sure how. It
>>>>> could be that the extra outputs should only be exposed to user-space if
>>>>> these bits are 0 (stand alone.) Guenter, any idea/opinion on this?
>>>>
>>>> How about using pwm[4567]_enable ? If I understand correctly, the possible
>>>> modes would be manual or sync(x). In this case we could have 1 (manual),
>>>> 2 (sync with fan1), 3 (sync with fan2), and 4 (sync with fan3), with the
>>>> caveat that the sync settings only make sense if the matching pwmX_enable
>>>> is set to thermal cruise mode.
>>>>
>>>> Does this make sense ?
>
> Not really. The problem is that for now I'm not sure what "sync" really
> refers to. The datasheet mentions temperature channels, and as far as I
> can see each temperature channel is hard-bound to a specific fan
> output. So I suspect that what these configuration bit really mean is
> that the pwm4 output (for example) mirrors the pwm1 output, i.e. pwm4
> has no independent existence. In which case it's better to not expose
> it at all.
>
> If the BIOS specifically configured these bits, there must be a reason
> and that reason would be the way the fans are connected to the chipset.
> Better not change it.
>
> If we really want to expose these bits then abusing pwmX_enable the way
> you suggested is still not correct, pwmX_auto_channels_temp would be
> better suited. But then again I don't think it adds any value if the
> extra PWM outputs only mirror already existing PWM outputs.
>
Ok, fine with me.

Guenter


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

Re: [lm-sensors] Additional PWM driver support for w83792d

[vt8231]

> On 05/11/2015 03:45 AM, Jean Delvare wrote:
> > On Sun, 10 May 2015 09:01:25 -0700, Guenter Roeck wrote:
> >> On 05/10/2015 06:42 AM, vt8231@hiddenengine.co.uk wrote:
> >> [ ... ]
> >>>>>
> >>>>> Last note: registers 0xA3-0xA6 have extra configuration bits "Sync
> >>>>> T1/2/3". Maybe the driver should handle them but I am not sure how. It
> >>>>> could be that the extra outputs should only be exposed to user-space if
> >>>>> these bits are 0 (stand alone.) Guenter, any idea/opinion on this?
> >>>>
> >>>> How about using pwm[4567]_enable ? If I understand correctly, the possible
> >>>> modes would be manual or sync(x). In this case we could have 1 (manual),
> >>>> 2 (sync with fan1), 3 (sync with fan2), and 4 (sync with fan3), with the
> >>>> caveat that the sync settings only make sense if the matching pwmX_enable
> >>>> is set to thermal cruise mode.
> >>>>
> >>>> Does this make sense ?
> >
> > Not really. The problem is that for now I'm not sure what "sync" really
> > refers to. The datasheet mentions temperature channels, and as far as I
> > can see each temperature channel is hard-bound to a specific fan
> > output. So I suspect that what these configuration bit really mean is
> > that the pwm4 output (for example) mirrors the pwm1 output, i.e. pwm4
> > has no independent existence. In which case it's better to not expose
> > it at all.
> >
> > If the BIOS specifically configured these bits, there must be a reason
> > and that reason would be the way the fans are connected to the chipset.
> > Better not change it.
> >
> > If we really want to expose these bits then abusing pwmX_enable the way
> > you suggested is still not correct, pwmX_auto_channels_temp would be
> > better suited. But then again I don't think it adds any value if the
> > extra PWM outputs only mirror already existing PWM outputs.
> >
> Ok, fine with me.
> 
> Guenter

Thanks Jean and Guenter,

If I understand this correctly, I shouldn't be implementing pwm[4-7]_enable.

I can see the same argument for *not* implementing pwm[4-7]_mode either. 
The pwmX_mode changes the chip from a PWM to analogue output.  Again, this
would depend on the attached HW so the BIOS should have sorted this out based
on the PCB design rather than allowing Linux to change it.  The HW 
requirements indicated in sections 7.7.2.1 and 7.7.2.2 of the datasheet are
very different so I wouldn't want the user to change this and damage the chip
as a result.

If you want me to add pwm[4-7]_mode then I'll do it... but I can only see
reasons not to and if it was me, I'd either remove the existing pwm[1-3]_mode
or at least make it a read-only value.

Anyway, the patch is below.  I've tested this on my Dell FS12-NV7 running Ubuntu
15.04 and kernel 3.19.0.  It works as expected with pwm4, pwm5 and pwm6 present.
I don't see pwm7 but then again, I don't see fan7 either so that suggests 
everything is as it should be for my HW.

Best regards,

Roger



Signed-off-by:	Roger Lucas <vt8231@hiddenengine.co.uk>

diff -uprN -X a/Documentation/dontdiff a/Documentation/hwmon/w83792d b/Documentation/hwmon/w83792d
--- linux-3.19.0/Documentation/hwmon/w83792d	2015-02-09 02:54:22.000000000 +0000
+++ linux-3.19.0-new/Documentation/hwmon/w83792d	2015-05-11 21:01:48.264679792 +0100
@@ -8,7 +8,7 @@ Supported chips:
     Datasheet: http://www.winbond.com.tw
 
 Author: Shane Huang (Winbond)
-
+Updated: Roger Lucas
 
 Module Parameters
 -----------------
@@ -38,9 +38,17 @@ parameter; this will put it into a more
 The driver implements three temperature sensors, seven fan rotation speed
 sensors, nine voltage sensors, and two automatic fan regulation
 strategies called: Smart Fan I (Thermal Cruise mode) and Smart Fan II.
-Automatic fan control mode is possible only for fan1-fan3. Fan4-fan7 can run
-synchronized with selected fan (fan1-fan3). This functionality and manual PWM
-control for fan4-fan7 is not yet implemented.
+Automatic fan control mode is possible only for fan1-fan3
+
+The driver also implements up to seven fan control outputs: pwm1-7.  Pwm1, 
+pwm2 and pwm3 can be configured to PWM output or Analogue DC output via their
+associated pwmX_mode.  Outputs pwm4 through pwm7 may or may not be present
+depending on how the W83792AD/D was configured by the BIOS.  Additionally, 
+the PWM/Analogue output can not be changed for pwm4-7 and respects the 
+configuration written by the BIOS.
+
+For all pwmX outputs, a value of 0 means minimum fan speed and a value of
+255 means maximum fan speed.
 
 Temperatures are measured in degrees Celsius and measurement resolution is 1
 degC for temp1 and 0.5 degC for temp2 and temp3. An alarm is triggered when
@@ -157,16 +165,19 @@ for each fan.
 /sys files
 ----------
 
-pwm[1-3] - this file stores PWM duty cycle or DC value (fan speed) in range:
+pwm[1-7] - this file stores PWM duty cycle or DC value (fan speed) in range:
 	0 (stop) to 255 (full)
 pwm[1-3]_enable - this file controls mode of fan/temperature control:
             * 0 Disabled
             * 1 Manual mode
             * 2 Smart Fan II
             * 3 Thermal Cruise
+	Note that pwm4-7 support manual mode only.
 pwm[1-3]_mode - Select PWM of DC mode
             * 0 DC
             * 1 PWM
+	Note that pwm4-7 respect the configuration written by the BIOS and
+	it cannot be changed by the user.
 thermal_cruise[1-3] - Selects the desired temperature for cruise (degC)
 tolerance[1-3] - Value in degrees of Celsius (degC) for +- T
 sf2_point[1-4]_fan[1-3] - four temperature points for each fan for Smart Fan II
diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/w83792d.c b/drivers/hwmon/w83792d.c
--- linux-3.19.0/drivers/hwmon/w83792d.c	2015-02-09 02:54:22.000000000 +0000
+++ linux-3.19.0-new/drivers/hwmon/w83792d.c	2015-05-11 20:38:34.996792747 +0100
@@ -219,6 +219,8 @@ static const u8 W83792D_REG_LEVELS[3][4]
 #define W83792D_REG_VBAT		0x5D
 #define W83792D_REG_I2C_ADDR		0x48
 
+#define W83792D_REG_WDOG_ENABLE		0x02
+
 /*
  * Conversions. Rounding and limit checking is only done on the TO_REG
  * variants. Note that you should be a bit careful with which arguments
@@ -289,11 +291,8 @@ struct w83792d_data {
 	u8 temp1[3];		/* current, over, thyst */
 	u8 temp_add[2][6];	/* Register value */
 	u8 fan_div[7];		/* Register encoding, shifted right */
-	u8 pwm[7];		/*
-				 * We only consider the first 3 set of pwm,
-				 * although 792 chip has 7 set of pwm.
-				 */
-	u8 pwmenable[3];
+	u8 pwm[7];		/* The 7 PWM outputs */
+	u8 pwmenable[3];	/* Only for the first 3 PWM outputs */
 	u32 alarms;		/* realtime status register encoding,combined */
 	u8 chassis;		/* Chassis status */
 	u8 thermal_cruise[3];	/* Smart FanI: Fan1,2,3 target value */
@@ -1075,6 +1074,10 @@ static DEVICE_ATTR(intrusion0_alarm, S_I
 static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0);
 static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1);
 static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2);
+static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3);
+static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 4);
+static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 5);
+static SENSOR_DEVICE_ATTR(pwm7, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 6);
 static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
 			show_pwmenable, store_pwmenable, 1);
 static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
@@ -1212,6 +1215,32 @@ static const struct attribute_group w837
 	{ .attrs = w83792d_attributes_fan[3] },
 };
 
+static struct attribute *w83792d_attributes_pwm[4][2] = {
+	{
+		&sensor_dev_attr_pwm4.dev_attr.attr,
+		NULL
+	},
+	{
+		&sensor_dev_attr_pwm5.dev_attr.attr,
+		NULL
+	},
+	{
+		&sensor_dev_attr_pwm6.dev_attr.attr,
+		NULL
+	},
+	{
+		&sensor_dev_attr_pwm7.dev_attr.attr,
+		NULL
+	}
+};
+
+static const struct attribute_group w83792d_group_pwm[4] = {
+	{ .attrs = w83792d_attributes_pwm[0] },
+	{ .attrs = w83792d_attributes_pwm[1] },
+	{ .attrs = w83792d_attributes_pwm[2] },
+	{ .attrs = w83792d_attributes_pwm[3] },
+};
+
 static struct attribute *w83792d_attributes[] = {
 	&sensor_dev_attr_in0_input.dev_attr.attr,
 	&sensor_dev_attr_in0_max.dev_attr.attr,
@@ -1406,12 +1435,20 @@ w83792d_probe(struct i2c_client *client,
 		err = sysfs_create_group(&dev->kobj, &w83792d_group_fan[0]);
 		if (err)
 			goto exit_remove_files;
+
+		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[0]);
+		if (err)
+			goto exit_remove_files;
 	}
 
 	if (!(val1 & 0x20)) {
 		err = sysfs_create_group(&dev->kobj, &w83792d_group_fan[1]);
 		if (err)
 			goto exit_remove_files;
+
+		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[1]);
+		if (err)
+			goto exit_remove_files;
 	}
 
 	val1 = w83792d_read_value(client, W83792D_REG_PIN);
@@ -1425,6 +1462,17 @@ w83792d_probe(struct i2c_client *client,
 		err = sysfs_create_group(&dev->kobj, &w83792d_group_fan[3]);
 		if (err)
 			goto exit_remove_files;
+
+		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[3]);
+		if (err)
+			goto exit_remove_files;
+	}
+
+	val1 = w83792d_read_value(client, W83792D_REG_WDOG_ENABLE);
+	if (!(val1 & 0x02)) {
+		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[2]);
+		if (err)
+			goto exit_remove_files;
 	}
 
 	data->hwmon_dev = hwmon_device_register(dev);




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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Jean Delvare]

Hi Roger,

On Mon, 11 May 2015 21:15:39 +0100, vt8231@hiddenengine.co.uk wrote:
> > On 05/11/2015 03:45 AM, Jean Delvare wrote:
> > > Not really. The problem is that for now I'm not sure what "sync" really
> > > refers to. The datasheet mentions temperature channels, and as far as I
> > > can see each temperature channel is hard-bound to a specific fan
> > > output. So I suspect that what these configuration bit really mean is
> > > that the pwm4 output (for example) mirrors the pwm1 output, i.e. pwm4
> > > has no independent existence. In which case it's better to not expose
> > > it at all.
> > >
> > > If the BIOS specifically configured these bits, there must be a reason
> > > and that reason would be the way the fans are connected to the chipset.
> > > Better not change it.
> > >
> > > If we really want to expose these bits then abusing pwmX_enable the way
> > > you suggested is still not correct, pwmX_auto_channels_temp would be
> > > better suited. But then again I don't think it adds any value if the
> > > extra PWM outputs only mirror already existing PWM outputs.
> >
> > Ok, fine with me.
> (...)
> 
> If I understand this correctly, I shouldn't be implementing pwm[4-7]_enable.

Correct.

> I can see the same argument for *not* implementing pwm[4-7]_mode either. 
> The pwmX_mode changes the chip from a PWM to analogue output.  Again, this
> would depend on the attached HW so the BIOS should have sorted this out based
> on the PCB design rather than allowing Linux to change it.  The HW 
> requirements indicated in sections 7.7.2.1 and 7.7.2.2 of the datasheet are
> very different so I wouldn't want the user to change this and damage the chip
> as a result.

In principle you are correct. I can't really remember the history,
maybe someone needed to reconfigure this on some board with broken
BIOS, or maybe this was "just in case". We have a total of 6 drivers
which allow changing the PWM/DC flags and w83792d is one of them. 4
drivers export the PWM/DC flags but in read-only mode.

Can you explain how setting the bit wrong could cause damage? I'm not
very skilled in electronics but my understanding so far was that in the
worse case the fan could just not be controlled. If there really is a
risk of damaging the hardware then we should update the 6 affected
drivers quickly.

> If you want me to add pwm[4-7]_mode then I'll do it... but I can only see
> reasons not to and if it was me, I'd either remove the existing pwm[1-3]_mode
> or at least make it a read-only value.

What I would like is consistency. There is no reason to treat pwm[1-3]
and pwm[4-7] differently. Your patch adds inconsistency and I don't
like that. So please add the pwm[4-7]_attributes and we can discuss
separately whether they should be made read-only in all 6 affected
drivers. I would be in favor of this change, Guenter, what do you think?

> Anyway, the patch is below.  I've tested this on my Dell FS12-NV7 running Ubuntu
> 15.04 and kernel 3.19.0.  It works as expected with pwm4, pwm5 and pwm6 present.
> I don't see pwm7 but then again, I don't see fan7 either so that suggests 
> everything is as it should be for my HW.
> (...)
> Signed-off-by:	Roger Lucas <vt8231@hiddenengine.co.uk>

It should be a space after the colon.

Also this needs a patch description before the Signed-off-by line.

> 
> diff -uprN -X a/Documentation/dontdiff a/Documentation/hwmon/w83792d b/Documentation/hwmon/w83792d
> --- linux-3.19.0/Documentation/hwmon/w83792d	2015-02-09 02:54:22.000000000 +0000
> +++ linux-3.19.0-new/Documentation/hwmon/w83792d	2015-05-11 21:01:48.264679792 +0100
> @@ -8,7 +8,7 @@ Supported chips:
>      Datasheet: http://www.winbond.com.tw
>  
>  Author: Shane Huang (Winbond)
> -
> +Updated: Roger Lucas

Please leave the extra blank line in place for consistency.

>  
>  Module Parameters
>  -----------------
> @@ -38,9 +38,17 @@ parameter; this will put it into a more
>  The driver implements three temperature sensors, seven fan rotation speed
>  sensors, nine voltage sensors, and two automatic fan regulation
>  strategies called: Smart Fan I (Thermal Cruise mode) and Smart Fan II.
> -Automatic fan control mode is possible only for fan1-fan3. Fan4-fan7 can run
> -synchronized with selected fan (fan1-fan3). This functionality and manual PWM
> -control for fan4-fan7 is not yet implemented.
> +Automatic fan control mode is possible only for fan1-fan3

The last part (related to fan control) should be moved to the next
paragraph, after you mention fan control outputs, otherwise it's
confusing.

> +
> +The driver also implements up to seven fan control outputs: pwm1-7.  Pwm1, 
> +pwm2 and pwm3 can be configured to PWM output or Analogue DC output via their
> +associated pwmX_mode.  Outputs pwm4 through pwm7 may or may not be present
> +depending on how the W83792AD/D was configured by the BIOS.  Additionally, 
> +the PWM/Analogue output can not be changed for pwm4-7 and respects the 
> +configuration written by the BIOS.
> +
> +For all pwmX outputs, a value of 0 means minimum fan speed and a value of
> +255 means maximum fan speed.
>  
>  Temperatures are measured in degrees Celsius and measurement resolution is 1
>  degC for temp1 and 0.5 degC for temp2 and temp3. An alarm is triggered when
> @@ -157,16 +165,19 @@ for each fan.
>  /sys files
>  ----------
>  
> -pwm[1-3] - this file stores PWM duty cycle or DC value (fan speed) in range:
> +pwm[1-7] - this file stores PWM duty cycle or DC value (fan speed) in range:
>  	0 (stop) to 255 (full)
>  pwm[1-3]_enable - this file controls mode of fan/temperature control:
>              * 0 Disabled
>              * 1 Manual mode
>              * 2 Smart Fan II
>              * 3 Thermal Cruise
> +	Note that pwm4-7 support manual mode only.
>  pwm[1-3]_mode - Select PWM of DC mode
>              * 0 DC
>              * 1 PWM
> +	Note that pwm4-7 respect the configuration written by the BIOS and
> +	it cannot be changed by the user.
>  thermal_cruise[1-3] - Selects the desired temperature for cruise (degC)
>  tolerance[1-3] - Value in degrees of Celsius (degC) for +- T
>  sf2_point[1-4]_fan[1-3] - four temperature points for each fan for Smart Fan II
> diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/w83792d.c b/drivers/hwmon/w83792d.c
> --- linux-3.19.0/drivers/hwmon/w83792d.c	2015-02-09 02:54:22.000000000 +0000
> +++ linux-3.19.0-new/drivers/hwmon/w83792d.c	2015-05-11 20:38:34.996792747 +0100
> @@ -219,6 +219,8 @@ static const u8 W83792D_REG_LEVELS[3][4]
>  #define W83792D_REG_VBAT		0x5D
>  #define W83792D_REG_I2C_ADDR		0x48
>  
> +#define W83792D_REG_WDOG_ENABLE		0x02
> +
>  /*
>   * Conversions. Rounding and limit checking is only done on the TO_REG
>   * variants. Note that you should be a bit careful with which arguments
> @@ -289,11 +291,8 @@ struct w83792d_data {
>  	u8 temp1[3];		/* current, over, thyst */
>  	u8 temp_add[2][6];	/* Register value */
>  	u8 fan_div[7];		/* Register encoding, shifted right */
> -	u8 pwm[7];		/*
> -				 * We only consider the first 3 set of pwm,
> -				 * although 792 chip has 7 set of pwm.
> -				 */
> -	u8 pwmenable[3];
> +	u8 pwm[7];		/* The 7 PWM outputs */
> +	u8 pwmenable[3];	/* Only for the first 3 PWM outputs */
>  	u32 alarms;		/* realtime status register encoding,combined */
>  	u8 chassis;		/* Chassis status */
>  	u8 thermal_cruise[3];	/* Smart FanI: Fan1,2,3 target value */
> @@ -1075,6 +1074,10 @@ static DEVICE_ATTR(intrusion0_alarm, S_I
>  static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0);
>  static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1);
>  static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2);
> +static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3);
> +static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 4);
> +static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 5);
> +static SENSOR_DEVICE_ATTR(pwm7, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 6);
>  static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
>  			show_pwmenable, store_pwmenable, 1);
>  static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
> @@ -1212,6 +1215,32 @@ static const struct attribute_group w837
>  	{ .attrs = w83792d_attributes_fan[3] },
>  };
>  
> +static struct attribute *w83792d_attributes_pwm[4][2] = {
> +	{
> +		&sensor_dev_attr_pwm4.dev_attr.attr,
> +		NULL
> +	},
> +	{
> +		&sensor_dev_attr_pwm5.dev_attr.attr,
> +		NULL
> +	},
> +	{
> +		&sensor_dev_attr_pwm6.dev_attr.attr,
> +		NULL
> +	},
> +	{
> +		&sensor_dev_attr_pwm7.dev_attr.attr,
> +		NULL
> +	}
> +};
> +
> +static const struct attribute_group w83792d_group_pwm[4] = {
> +	{ .attrs = w83792d_attributes_pwm[0] },
> +	{ .attrs = w83792d_attributes_pwm[1] },
> +	{ .attrs = w83792d_attributes_pwm[2] },
> +	{ .attrs = w83792d_attributes_pwm[3] },
> +};
> +
>  static struct attribute *w83792d_attributes[] = {
>  	&sensor_dev_attr_in0_input.dev_attr.attr,
>  	&sensor_dev_attr_in0_max.dev_attr.attr,
> @@ -1406,12 +1435,20 @@ w83792d_probe(struct i2c_client *client,
>  		err = sysfs_create_group(&dev->kobj, &w83792d_group_fan[0]);
>  		if (err)
>  			goto exit_remove_files;
> +
> +		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[0]);
> +		if (err)
> +			goto exit_remove_files;
>  	}
>  
>  	if (!(val1 & 0x20)) {
>  		err = sysfs_create_group(&dev->kobj, &w83792d_group_fan[1]);
>  		if (err)
>  			goto exit_remove_files;
> +
> +		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[1]);
> +		if (err)
> +			goto exit_remove_files;
>  	}
>  
>  	val1 = w83792d_read_value(client, W83792D_REG_PIN);
> @@ -1425,6 +1462,17 @@ w83792d_probe(struct i2c_client *client,
>  		err = sysfs_create_group(&dev->kobj, &w83792d_group_fan[3]);
>  		if (err)
>  			goto exit_remove_files;
> +
> +		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[3]);
> +		if (err)
> +			goto exit_remove_files;
> +	}
> +
> +	val1 = w83792d_read_value(client, W83792D_REG_WDOG_ENABLE);
> +	if (!(val1 & 0x02)) {
> +		err = sysfs_create_group(&dev->kobj, &w83792d_group_pwm[2]);
> +		if (err)
> +			goto exit_remove_files;
>  	}

I don't understand this test. How does FAN_OUT6 have anything to do
with the hard watchdog enabled bit? The way I read the datasheet, bit 6
in register 0x4B (Pin Control Register) controls the usage of both pins
48 (SYSRSTIN#/FAN_IN6) and 47 (WDTRST#/FAN_OUT6.)

So you should be able to add the new attributes to existing arrays
rather than introducing new arrays.

>  
>  	data->hwmon_dev = hwmon_device_register(dev);
> 
> 
> 


-- 
Jean Delvare
SUSE L3 Support

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

Re: [lm-sensors] Additional PWM driver support for w83792d

[Guenter Roeck]

Hi Jean,

On Tue, May 12, 2015 at 04:19:47PM +0200, Jean Delvare wrote:
> Hi Roger,
> 
[ ... ]

> > If you want me to add pwm[4-7]_mode then I'll do it... but I can only see
> > reasons not to and if it was me, I'd either remove the existing pwm[1-3]_mode
> > or at least make it a read-only value.
> 
> What I would like is consistency. There is no reason to treat pwm[1-3]
> and pwm[4-7] differently. Your patch adds inconsistency and I don't
> like that. So please add the pwm[4-7]_attributes and we can discuss
> separately whether they should be made read-only in all 6 affected
> drivers. I would be in favor of this change, Guenter, what do you think?
> 

Ok with me. We'll need to do some reading to get an understanding how
the wrong mode would or could cause hardware defects.

Guenter

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

Re: [lm-sensors] Additional PWM driver support for w83792d - PATCH [1/1]

[Jean Delvare]

On Tue, 12 May 2015 22:01:37 +0100, Roger Lucas wrote:
> This adds pwm[4-7] and the associated pwm[4-7]_mode signals.
> 
> Signed-off-by: Roger Lucas <vt8231@hiddenengine.co.uk>
> (...)

Applied, thanks!

-- 
Jean Delvare
SUSE L3 Support

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