[lm-sensors] [PATCH 2/4] hwmon: it87 only create pwm1-3 sysfs files

[lm-sensors] [PATCH 2/4] hwmon: it87 only create pwm1-3 sysfs files

[Andrew Paprocki]

This patch only creates the pwmN_enable, pwmN, and pwmN_freq
sysfs interface files if the corresponding fanN is enabled by
the chip.

Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
---
 drivers/hwmon/it87.c |   46 +++++++++++++++++++++++++++-------------------
 1 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 30cdb09..7de4af8 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -1168,25 +1168,33 @@ static int __devinit it87_probe(struct platform_device *pdev)
 	}
 
 	if (enable_pwm_interface) {
-		if ((err = device_create_file(dev,
-		     &sensor_dev_attr_pwm1_enable.dev_attr))
-		 || (err = device_create_file(dev,
-		     &sensor_dev_attr_pwm2_enable.dev_attr))
-		 || (err = device_create_file(dev,
-		     &sensor_dev_attr_pwm3_enable.dev_attr))
-		 || (err = device_create_file(dev,
-		     &sensor_dev_attr_pwm1.dev_attr))
-		 || (err = device_create_file(dev,
-		     &sensor_dev_attr_pwm2.dev_attr))
-		 || (err = device_create_file(dev,
-		     &sensor_dev_attr_pwm3.dev_attr))
-		 || (err = device_create_file(dev,
-		     &dev_attr_pwm1_freq))
-		 || (err = device_create_file(dev,
-		     &dev_attr_pwm2_freq))
-		 || (err = device_create_file(dev,
-		     &dev_attr_pwm3_freq)))
-			goto ERROR4;
+		if (data->has_fan & (1 << 0)) {
+			if ((err = device_create_file(dev,
+			     &sensor_dev_attr_pwm1_enable.dev_attr))
+			 || (err = device_create_file(dev,
+			     &sensor_dev_attr_pwm1.dev_attr))
+			 || (err = device_create_file(dev,
+			     &dev_attr_pwm1_freq)))
+				goto ERROR4;
+		}
+		if (data->has_fan & (1 << 1)) {
+			if ((err = device_create_file(dev,
+			     &sensor_dev_attr_pwm2_enable.dev_attr))
+			 || (err = device_create_file(dev,
+			     &sensor_dev_attr_pwm2.dev_attr))
+			 || (err = device_create_file(dev,
+			     &dev_attr_pwm2_freq)))
+				goto ERROR4;
+		}
+		if (data->has_fan & (1 << 2)) {
+			if ((err = device_create_file(dev,
+			     &sensor_dev_attr_pwm3_enable.dev_attr))
+			 || (err = device_create_file(dev,
+			     &sensor_dev_attr_pwm3.dev_attr))
+			 || (err = device_create_file(dev,
+			     &dev_attr_pwm3_freq)))
+				goto ERROR4;
+		}
 	}
 
 	if (data->type = it8712 || data->type = it8716
-- 
1.5.6


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

Re: [lm-sensors] [PATCH 2/4] hwmon: it87 only create pwm1-3 sysfs

[Jean Delvare]

On Mon,  7 Jul 2008 06:40:09 -0400, Andrew Paprocki wrote:
> This patch only creates the pwmN_enable, pwmN, and pwmN_freq
> sysfs interface files if the corresponding fanN is enabled by
> the chip.

Nack. PWM1 may control fan2, etc.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 2/4] hwmon: it87 only create pwm1-3 sysfs

[Andrew Paprocki]

Ah ok, for the pwm1_freq. I suppose this can be left out entirely, or
should pwm1* always exist? It just seemed wrong to make pwm4/5 show up
when needed but not the others.

-Andrew

On Mon, Jul 7, 2008 at 7:25 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Mon,  7 Jul 2008 06:40:09 -0400, Andrew Paprocki wrote:
>> This patch only creates the pwmN_enable, pwmN, and pwmN_freq
>> sysfs interface files if the corresponding fanN is enabled by
>> the chip.
>
> Nack. PWM1 may control fan2, etc.
>
> --
> Jean Delvare
>
>

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

Re: [lm-sensors] [PATCH 2/4] hwmon: it87 only create pwm1-3 sysfs

[Jean Delvare]

Hi Andrew,

(No top-posting please.)

> On Mon, Jul 7, 2008 at 7:25 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Mon,  7 Jul 2008 06:40:09 -0400, Andrew Paprocki wrote:
> >> This patch only creates the pwmN_enable, pwmN, and pwmN_freq
> >> sysfs interface files if the corresponding fanN is enabled by
> >> the chip.
> > Nack. PWM1 may control fan2, etc.

On Mon, 7 Jul 2008 07:36:43 -0400, Andrew Paprocki wrote:
> Ah ok, for the pwm1_freq. I suppose this can be left out entirely, or
> should pwm1* always exist?

pwm1* should exist as long as you have no reason to believe that the
pwm1 pin is used for another function. I can't remember if the IT87xxF
chips share pwm pins have alternate functions (but some other chips do,
and our driver don't necessarily pay attention to that as they should.)
And the same holds for all pwm files.

 It just seemed wrong to make pwm4/5 show up
> when needed but not the others.

Not sure what you mean with pwm4/5 but please keep in mind that it is
perfectly possible to have pwm4 without fan4 or fan4 without pwm4, and
same for pwm5 and fan5. Inputs and outputs don't have to be related in
the way the numbers suggest. Quite often the chip internals assume that
they do, but motherboard makers can be very dump in the way they wire
the fans.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 2/4] hwmon: it87 only create pwm1-3 sysfs

[Andrew Paprocki]

On Mon, Jul 7, 2008 at 8:19 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Mon, 7 Jul 2008 07:36:43 -0400, Andrew Paprocki wrote:
>> Ah ok, for the pwm1_freq. I suppose this can be left out entirely, or
>> should pwm1* always exist?
>
> pwm1* should exist as long as you have no reason to believe that the
> pwm1 pin is used for another function. I can't remember if the IT87xxF
> chips share pwm pins have alternate functions (but some other chips do,
> and our driver don't necessarily pay attention to that as they should.)
> And the same holds for all pwm files.

At least for the 8712, FAN_CTL1 has a dedicated pin, but all the other
FAN_CTL* pins are shared with other functions (such as joystick port,
or GPIO usage). Are the pwm* files actually used by anyone in
userspace for something other than controlling the fans?

>  It just seemed wrong to make pwm4/5 show up
>> when needed but not the others.
>
> Not sure what you mean with pwm4/5 but please keep in mind that it is
> perfectly possible to have pwm4 without fan4 or fan4 without pwm4, and
> same for pwm5 and fan5. Inputs and outputs don't have to be related in
> the way the numbers suggest. Quite often the chip internals assume that
> they do, but motherboard makers can be very dump in the way they wire
> the fans.

So in this particular case, FAN_CTL4 shares a pin with JSBB2 (Joystick
button 2) and GPIO27. Similarly, FAN_CTL5 shares a pin with JSBB1 and
GPIO26. Should pwm4* and pwm5* files always exist for the IT871[268]
just because the pins exist, even if the chip configuration reports
there is no fan4/fan5 connected?

In a related question, I saw this comment in the driver:
                                /* automatic pwm - not yet implemented, but
                                 * leave the settings made by the BIOS alone
                                 * until a change is requested via the sysfs
                                 * interface */

What needs to be implemented for automatic mode? As far as I can tell
in the datasheet, automatic mode changes the meaning of bits0-6 in the
CTL register to be a selection of temp1/temp2/temp3 instead of a
128-step PWM control value. I could take a stab at implementing
something for this if you know what you want it to look like. I assume
the sysfs pwm inputs would change meaning in automatic mode to only be
a 2 bit value indicating which temp to select? Is there anything more
complicated needed?

-Andrew

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

Re: [lm-sensors] [PATCH 2/4] hwmon: it87 only create pwm1-3 sysfs

[Jean Delvare]

Hi Andrew,

On Mon, 7 Jul 2008 09:56:02 -0400, Andrew Paprocki wrote:
> On Mon, Jul 7, 2008 at 8:19 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > pwm1* should exist as long as you have no reason to believe that the
> > pwm1 pin is used for another function. I can't remember if the IT87xxF
> > chips share pwm pins have alternate functions (but some other chips do,
> > and our driver don't necessarily pay attention to that as they should.)
> > And the same holds for all pwm files.
> 
> At least for the 8712, FAN_CTL1 has a dedicated pin, but all the other
> FAN_CTL* pins are shared with other functions (such as joystick port,
> or GPIO usage). Are the pwm* files actually used by anyone in
> userspace for something other than controlling the fans?

In theory the PWM signals could be used for anything, but in practice,
the motherboard manufacturer decides, and thankfully I am not aware of
any other use than fan control. The it87 driver assumes that PWM pins
are used for fan speed control.

If pins have shared functions, then the it87 driver should check that
each pin is indeed used for PWM before in exports it to user-space.
I've just taken a quick look at the IT8705F datasheet and all 3 PWM
pins can be used for other functions. The default is PWM so the driver
probably gets it right in most cases, but we should still add a check.
And apparently, same is true of the fan tachometer inputs.

Are you up to write a patch implementing this? I can help with review
and testing.

> > (...)
> > Not sure what you mean with pwm4/5 but please keep in mind that it is
> > perfectly possible to have pwm4 without fan4 or fan4 without pwm4, and
> > same for pwm5 and fan5. Inputs and outputs don't have to be related in
> > the way the numbers suggest. Quite often the chip internals assume that
> > they do, but motherboard makers can be very dump in the way they wire
> > the fans.
> 
> So in this particular case, FAN_CTL4 shares a pin with JSBB2 (Joystick
> button 2) and GPIO27. Similarly, FAN_CTL5 shares a pin with JSBB1 and
> GPIO26. Should pwm4* and pwm5* files always exist for the IT871[268]
> just because the pins exist, even if the chip configuration reports
> there is no fan4/fan5 connected?

Again, FAN_CTL4 is NOT related to "fan 4 being connected". Not that you
can know for sure whether a given fan input has a fan connected anyway,
but even if you could, you should never assume a correlation between
fanN and pwmN, for motherboard makers can wire the chip in completely
unexpected ways. I've made that mistake before...

As written above, what needs to be checked is whether a given pin is
configured for fan speed monitoring or control, or for an alternative
function. If the pin is not configured for a hardware monitoring
function then the it87 driver shouldn't expose it (it won't work
anyway.)

> 
> In a related question, I saw this comment in the driver:
>                                 /* automatic pwm - not yet implemented, but
>                                  * leave the settings made by the BIOS alone
>                                  * until a change is requested via the sysfs
>                                  * interface */
> 
> What needs to be implemented for automatic mode? As far as I can tell
> in the datasheet, automatic mode changes the meaning of bits0-6 in the
> CTL register to be a selection of temp1/temp2/temp3 instead of a
> 128-step PWM control value. I could take a stab at implementing
> something for this if you know what you want it to look like. I assume
> the sysfs pwm inputs would change meaning in automatic mode to only be
> a 2 bit value indicating which temp to select? Is there anything more
> complicated needed?

Please take a look at Documentation/hwmon/sysfs-interface. There's more
than temperature channel selection. The more difficult part is the
interface to let the user select the different trip points that define
the speed/temperature response curve.

The fact that some registers have a different meaning depending on the
mode is indeed a problem. I didn't give it much thought, and won't have
the time to. If you give it a try, just do things the way you think is
best, and whoever disagrees will speak up.

Thanks,
-- 
Jean Delvare

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