[PATCH][v2] therm_windtunnel doesn't work properly on PowerMac G4

[PATCH][v2] therm_windtunnel doesn't work properly on PowerMac G4

[Goffredo Baroncelli]

Hi All,

On a PowerMac G4 I noticed that between the kernel v3.2 and v3.14 I lost
the fan management.

I found on internet other references to this kind of problem [2]


**How reproduce:
- booting with the kernel 3.2, the fan is "quite" silent.
The module therm_windtunnel is loaded and in the log there are  
lines like:

	[ 1342.614956] CPU-temp: 58.7 C, Case: 33.7 C,  Fan: 5 (tuned +0)
	[ 1390.637793] CPU-temp: 58.6 C, Case: 33.6 C,  Fan: 5

I had also access to the temperature via the sysfs files:
/sys/devices/temperature/case_temperature  
/sys/devices/temperature/cpu_temperature


- booting with the kernel 3.14, the fan is very loud. The module 
therm_windtunnel is not loaded. In the log there aren't any message
related to the temperature. The sysfs entries don't exist.


** Analysis 
In these Apple machines the module i2c-powermac requires the
i2c drivers provided by the module therm_windtunnel.

Between the kernel v3.2 and v3.14 [1] some patches changed the 
driver name requested by the i2c-powermac module, 
so the therm_windtunnel modules is not instantiated anymore.


** Proposed solution
In the following emails I sent you 4 patches to solve this 
problem (tested on my PowerMac G4)

1) change the driver name
	therm_ds1775 -> MAC,ds1775
        therm_adm1030 -> MAC,adm1030
so the i2c driver are instantiated by i2c-powermac

2) remove the (unused) method do_attach from the i2c-driver
3) add a parameter to the therm_windtunnel module 
to control the kernel log message 
4) export the fan speed via sysfs

The patch 1) solve the problem. The patch 2) is a small cleanup.
The patch 3) allow a better control of the log in dmesg.
The patch 4) is copyied from the Bryan Christianson's patch (see
debian bug #741663)

Could you be so kindly to apply these patches ?

PS:
I am not LKML subscriber, so please put me in CC in case of reply.

BR
G.Baroncelli

Changelog:
v0: 2014/07/30
	- first issue
v1: 2014/08/01
	- protect with a mutex the check before starting the fan 
	  daemon (to protect from parallel drivers instantation)
	- reduce the number of module parameters to 1 as suggested by 
          Jean Delvare
	- export the fan speed via sysfs

[1] I think that the guilty commit is 
commit 81e5d8646ff6bf323dddcf172aa3cef84468fa12
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date:   Wed Apr 18 22:16:42 2012 +0000

    i2c/powermac: Register i2c devices from device-tree
    
    This causes i2c-powermac to register i2c devices exposed in the
    device-tree, enabling new-style probing of devices.
    
    Note that we prefix the IDs with "MAC," in order to prevent the
    generic drivers from matching. This is done on purpose as we only
    want drivers specifically tested/designed to operate on powermacs
    to match.
    
    This removes the special case we had for the AMS driver, and updates
    the driver's match table instead.
    
    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

[2] There is the debian bug #741663 which highlight the same problem. In
the bug discussion there is a patch like the my ones.

See also
https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-July/099561.html

[PATCH 1/4] Update drivers names to the ones invoked by i2c-powermac

[Goffredo Baroncelli]

Update drivers names to the ones invoked by i2c-powermac:
- therm_ds1775 -> MAC,ds1775
- therm_adm1030 -> MAC,adm1030
The background fan control loop is started from
the devices probing methods.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 drivers/macintosh/therm_windtunnel.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 3b4a157..a64a06f 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -334,6 +334,23 @@ do_attach( struct i2c_adapter *adapter )
 	return 0;
 }
 
+static void
+try_start_control_loop(void)
+{
+
+	mutex_lock(&x.lock);
+	if (!x.thermostat || !x.fan || x.running) {
+		mutex_unlock(&x.lock);
+		return;
+	}
+
+	x.running = 1;
+	mutex_unlock(&x.lock);
+
+	x.poll_task = kthread_run(control_loop, NULL, "g4fand");
+
+}
+
 static int
 do_remove(struct i2c_client *client)
 {
@@ -364,6 +381,7 @@ attach_fan( struct i2c_client *cl )
 	printk("ADM1030 fan controller [@%02x]\n", cl->addr );
 
 	x.fan = cl;
+	try_start_control_loop();
  out:
 	return 0;
 }
@@ -397,6 +415,7 @@ attach_thermostat( struct i2c_client *cl )
 	x.overheat_temp = os_temp;
 	x.overheat_hyst = hyst_temp;
 	x.thermostat = cl;
+	try_start_control_loop();
 out:
 	return 0;
 }
@@ -404,8 +423,8 @@ out:
 enum chip { ds1775, adm1030 };
 
 static const struct i2c_device_id therm_windtunnel_id[] = {
-	{ "therm_ds1775", ds1775 },
-	{ "therm_adm1030", adm1030 },
+	{ "MAC,ds1775", ds1775 },
+	{ "MAC,adm1030", adm1030 },
 	{ }
 };
 
-- 
2.0.1

[PATCH 2/4] Remove attach_method because un-used

[Goffredo Baroncelli]

Remove attach_method because i2c-powermac is
in charge to instantiate the driver directly.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 drivers/macintosh/therm_windtunnel.c | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index a64a06f..1e50455 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -300,40 +300,6 @@ static int control_loop(void *dummy)
 /*	i2c probing and setup						*/
 /************************************************************************/
 
-static int
-do_attach( struct i2c_adapter *adapter )
-{
-	/* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */
-	static const unsigned short scan_ds1775[] = {
-		0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f,
-		I2C_CLIENT_END
-	};
-	static const unsigned short scan_adm1030[] = {
-		0x2c, 0x2d, 0x2e, 0x2f,
-		I2C_CLIENT_END
-	};
-
-	if( strncmp(adapter->name, "uni-n", 5) )
-		return 0;
-
-	if( !x.running ) {
-		struct i2c_board_info info;
-
-		memset(&info, 0, sizeof(struct i2c_board_info));
-		strlcpy(info.type, "therm_ds1775", I2C_NAME_SIZE);
-		i2c_new_probed_device(adapter, &info, scan_ds1775, NULL);
-
-		strlcpy(info.type, "therm_adm1030", I2C_NAME_SIZE);
-		i2c_new_probed_device(adapter, &info, scan_adm1030, NULL);
-
-		if( x.thermostat && x.fan ) {
-			x.running = 1;
-			x.poll_task = kthread_run(control_loop, NULL, "g4fand");
-		}
-	}
-	return 0;
-}
-
 static void
 try_start_control_loop(void)
 {
@@ -450,7 +416,6 @@ static struct i2c_driver g4fan_driver = {
 	.driver = {
 		.name	= "therm_windtunnel",
 	},
-	.attach_adapter = do_attach,
 	.probe		= do_probe,
 	.remove		= do_remove,
 	.id_table	= therm_windtunnel_id,
-- 
2.0.1

[PATCH 3/4] Add the "verbose" module option.

[Goffredo Baroncelli]

The "verbose" option controls the message in the kernel log
verbose = 0   no message
verbose = 1   log only the fan speed changes
verbose = 2   log the fan speed changes and the temperature changes

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 drivers/macintosh/therm_windtunnel.c | 37 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 1e50455..0c4eb85 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -44,7 +44,11 @@
 #include <asm/sections.h>
 #include <asm/macio.h>
 
-#define LOG_TEMP		0			/* continuously log temperature */
+static int verbose = 1;	  /* see description below */
+module_param(verbose, int, 0644);
+MODULE_PARM_DESC(verbose, "Vebosity level: 0=silent, "
+				"1=log the fan tuning, "
+				"2=log the temperature.");
 
 static struct {
 	volatile int		running;
@@ -157,10 +161,6 @@ tune_fan( int fan_setting )
 	/* write_reg( x.fan, 0x24, val, 1 ); */
 	write_reg( x.fan, 0x25, val, 1 );
 	write_reg( x.fan, 0x20, 0, 1 );
-	print_temp("CPU-temp: ", x.temp );
-	if( x.casetemp )
-		print_temp(", Case: ", x.casetemp );
-	printk(",  Fan: %d (tuned %+d)\n", 11-fan_setting, x.fan_level-fan_setting );
 
 	x.fan_level = fan_setting;
 }
@@ -179,14 +179,6 @@ poll_temp( void )
 	casetemp = read_reg(x.fan, 0x0b, 1) << 8;
 	casetemp |= (read_reg(x.fan, 0x06, 1) & 0x7) << 5;
 
-	if( LOG_TEMP && x.temp != temp ) {
-		print_temp("CPU-temp: ", temp );
-		print_temp(", Case: ", casetemp );
-		printk(",  Fan: %d\n", 11-x.fan_level );
-	}
-	x.temp = temp;
-	x.casetemp = casetemp;
-
 	level = -1;
 	for( i=0; (temp & 0xffff) > fan_table[i].temp ; i++ )
 		;
@@ -200,6 +192,25 @@ poll_temp( void )
 		level = fan_table[i].fan_up_setting;
 	x.upind = i;
 
+	/*
+	 * if verbose >0 log each fan tuning
+	 * if verbose >1 log each cpu temperature change
+	 */
+	if ((verbose > 1 && x.temp != temp ) ||
+	    (verbose > 0 && level >= 0)) {
+		print_temp("CPU-temp: ", temp );
+		if (casetemp)
+			print_temp(", Case: ", casetemp );
+		if (level >= 0)
+			printk(", Fan: %d (tuned %+d)\n", 11-level,
+				x.fan_level-level );
+		else
+			printk(", Fan: %d (tuned +0)\n",x.fan_level);
+	}
+
+	x.temp = temp;
+	x.casetemp = casetemp;
+
 	if( level >= 0 )
 		tune_fan( level );
 }
-- 
2.0.1

[PATCH 4/4] Return the fan speed via sysfs

[Goffredo Baroncelli]

Return the fan speed via sysfs:
/sys/devices/temperature/fan_level

This patch is copied from the Bryan Christianson's patch (see
debian bug #741663)

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 drivers/macintosh/therm_windtunnel.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 0c4eb85..a2af7db 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -111,9 +111,15 @@ show_case_temperature( struct device *dev, struct device_attribute *attr, char *
 	return sprintf(buf, "%d.%d\n", x.casetemp>>8, (x.casetemp & 255)*10/256 );
 }
 
+static ssize_t
+show_fan_level( struct device *dev, struct device_attribute *attr, char *buf )
+{
+	return sprintf(buf, "%d\n", 11 - x.fan_level );
+}
+
 static DEVICE_ATTR(cpu_temperature, S_IRUGO, show_cpu_temperature, NULL );
 static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
-
+static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL );
 
 
 /************************************************************************/
@@ -265,6 +271,7 @@ setup_hardware( void )
 
 	err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
 	err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
+	err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
 	if (err)
 		printk(KERN_WARNING
 			"Failed to create temperature attribute file(s).\n");
@@ -275,6 +282,7 @@ restore_regs( void )
 {
 	device_remove_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
 	device_remove_file( &x.of_dev->dev, &dev_attr_case_temperature );
+	device_remove_file( &x.of_dev->dev, &dev_attr_fan_level );
 
 	write_reg( x.fan, 0x01, x.r1, 1 );
 	write_reg( x.fan, 0x20, x.r20, 1 );
-- 
2.0.1

Re: [PATCH 3/4] Add the "verbose" module option.

[Jean Delvare]

Hi Goffredo,

You messed up your Cc's ;-)

On Fri,  1 Aug 2014 14:00:49 +0000, Goffredo Baroncelli wrote:
> The "verbose" option controls the message in the kernel log
> verbose = 0   no message
> verbose = 1   log only the fan speed changes
> verbose = 2   log the fan speed changes and the temperature changes
> 
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  drivers/macintosh/therm_windtunnel.c | 37 +++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index 1e50455..0c4eb85 100644
> --- a/drivers/macintosh/therm_windtunnel.c
> +++ b/drivers/macintosh/therm_windtunnel.c
> @@ -44,7 +44,11 @@
>  #include <asm/sections.h>
>  #include <asm/macio.h>
>  
> -#define LOG_TEMP		0			/* continuously log temperature */
> +static int verbose = 1;	  /* see description below */

This comment seems useless.

> +module_param(verbose, int, 0644);
> +MODULE_PARM_DESC(verbose, "Vebosity level: 0=silent, "

Typo: Verbosity

> +				"1=log the fan tuning, "
> +				"2=log the temperature.");

Trailing dot is not needed.

>  
>  static struct {
>  	volatile int		running;
> @@ -157,10 +161,6 @@ tune_fan( int fan_setting )
>  	/* write_reg( x.fan, 0x24, val, 1 ); */
>  	write_reg( x.fan, 0x25, val, 1 );
>  	write_reg( x.fan, 0x20, 0, 1 );
> -	print_temp("CPU-temp: ", x.temp );
> -	if( x.casetemp )
> -		print_temp(", Case: ", x.casetemp );
> -	printk(",  Fan: %d (tuned %+d)\n", 11-fan_setting, x.fan_level-fan_setting );
>  
>  	x.fan_level = fan_setting;
>  }
> @@ -179,14 +179,6 @@ poll_temp( void )
>  	casetemp = read_reg(x.fan, 0x0b, 1) << 8;
>  	casetemp |= (read_reg(x.fan, 0x06, 1) & 0x7) << 5;
>  
> -	if( LOG_TEMP && x.temp != temp ) {
> -		print_temp("CPU-temp: ", temp );
> -		print_temp(", Case: ", casetemp );
> -		printk(",  Fan: %d\n", 11-x.fan_level );
> -	}
> -	x.temp = temp;
> -	x.casetemp = casetemp;
> -
>  	level = -1;
>  	for( i=0; (temp & 0xffff) > fan_table[i].temp ; i++ )
>  		;
> @@ -200,6 +192,25 @@ poll_temp( void )
>  		level = fan_table[i].fan_up_setting;
>  	x.upind = i;
>  
> +	/*
> +	 * if verbose >0 log each fan tuning
> +	 * if verbose >1 log each cpu temperature change
> +	 */

I think it is a good idea to have all the printing in a single place.

> +	if ((verbose > 1 && x.temp != temp ) ||

No space before closing parenthesis. I know the original code did not
follow the standard coding style but you have the opportunity to make
it better. Same many times below. scripts/checkpatch.pl tells you about
that and many other style issues, most of which are easily fixable.

Also don't you want to log changes of case temperature too?

> +	    (verbose > 0 && level >= 0)) {
> +		print_temp("CPU-temp: ", temp );
> +		if (casetemp)
> +			print_temp(", Case: ", casetemp );
> +		if (level >= 0)
> +			printk(", Fan: %d (tuned %+d)\n", 11-level,
> +				x.fan_level-level );
> +		else
> +			printk(", Fan: %d (tuned +0)\n",x.fan_level);

I think you can do without the "tuned +0" which doesn't add much value.

> +	}
> +
> +	x.temp = temp;
> +	x.casetemp = casetemp;
> +
>  	if( level >= 0 )
>  		tune_fan( level );
>  }


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/4] Return the fan speed via sysfs

[Jean Delvare]

On Fri,  1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
> Return the fan speed via sysfs:
> /sys/devices/temperature/fan_level

Good idea. Even better would be if the driver would expose a standard
hwmon interface for the temperature values. Fan level could go in
attribute "pwm1" after proper scaling.

Please follow scripts/checkpatch.pl's advice to fix the coding style.

> This patch is copied from the Bryan Christianson's patch (see
> debian bug #741663)
> 
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  drivers/macintosh/therm_windtunnel.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index 0c4eb85..a2af7db 100644
> --- a/drivers/macintosh/therm_windtunnel.c
> +++ b/drivers/macintosh/therm_windtunnel.c
> @@ -111,9 +111,15 @@ show_case_temperature( struct device *dev, struct device_attribute *attr, char *
>  	return sprintf(buf, "%d.%d\n", x.casetemp>>8, (x.casetemp & 255)*10/256 );
>  }
>  
> +static ssize_t
> +show_fan_level( struct device *dev, struct device_attribute *attr, char *buf )
> +{
> +	return sprintf(buf, "%d\n", 11 - x.fan_level );
> +}
> +
>  static DEVICE_ATTR(cpu_temperature, S_IRUGO, show_cpu_temperature, NULL );
>  static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
> -
> +static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL );
>  
>  
>  /************************************************************************/
> @@ -265,6 +271,7 @@ setup_hardware( void )
>  
>  	err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>  	err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
> +	err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
>  	if (err)
>  		printk(KERN_WARNING
>  			"Failed to create temperature attribute file(s).\n");

That's not your fault but please note that this construct is broken.
You can't "or" error codes together, the result if two or more calls
fail with different error codes is pretty random. Instead, each error
must be tested individually. I know checkpatch.pl will complain if you
do that but you can ignore it as is it the right thing to do in that
case.

> @@ -275,6 +282,7 @@ restore_regs( void )
>  {
>  	device_remove_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>  	device_remove_file( &x.of_dev->dev, &dev_attr_case_temperature );
> +	device_remove_file( &x.of_dev->dev, &dev_attr_fan_level );
>  
>  	write_reg( x.fan, 0x01, x.r1, 1 );
>  	write_reg( x.fan, 0x20, x.r20, 1 );


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] Add the "verbose" module option.

[Goffredo Baroncelli]

On 08/03/2014 04:12 PM, Jean Delvare wrote:
> Hi Goffredo,
> 
> You messed up your Cc's ;-)

I fight hard with git-send-email.... In the next trip I will check two times !


> 
> On Fri,  1 Aug 2014 14:00:49 +0000, Goffredo Baroncelli wrote:
>> The "verbose" option controls the message in the kernel log
>> verbose = 0   no message
>> verbose = 1   log only the fan speed changes
>> verbose = 2   log the fan speed changes and the temperature changes
>>
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>>  drivers/macintosh/therm_windtunnel.c | 37 +++++++++++++++++++++++-------------
>>  1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
>> index 1e50455..0c4eb85 100644
>> --- a/drivers/macintosh/therm_windtunnel.c
>> +++ b/drivers/macintosh/therm_windtunnel.c
>> @@ -44,7 +44,11 @@
>>  #include <asm/sections.h>
>>  #include <asm/macio.h>
>>  
>> -#define LOG_TEMP		0			/* continuously log temperature */
>> +static int verbose = 1;	  /* see description below */
> 
> This comment seems useless.
ok

> 
>> +module_param(verbose, int, 0644);
>> +MODULE_PARM_DESC(verbose, "Vebosity level: 0=silent, "
> 
> Typo: Verbosity
Ok
> 
>> +				"1=log the fan tuning, "
>> +				"2=log the temperature.");
> 
> Trailing dot is not needed.
OK


>>  
>>  static struct {
[... cut cut cut ...]
>> +	/*
>> +	 * if verbose >0 log each fan tuning
>> +	 * if verbose >1 log each cpu temperature change
>> +	 */
> 
> I think it is a good idea to have all the printing in a single place.
> 
>> +	if ((verbose > 1 && x.temp != temp ) ||
> 
> No space before closing parenthesis. I know the original code did not
> follow the standard coding style but you have the opportunity to make
> it better. Same many times below. scripts/checkpatch.pl tells you about
> that and many other style issues, most of which are easily fixable.
> 
> Also don't you want to log changes of case temperature too?

yes it make sense.

> 
>> +	    (verbose > 0 && level >= 0)) {
>> +		print_temp("CPU-temp: ", temp );
>> +		if (casetemp)
>> +			print_temp(", Case: ", casetemp );
>> +		if (level >= 0)
>> +			printk(", Fan: %d (tuned %+d)\n", 11-level,
>> +				x.fan_level-level );
>> +		else
>> +			printk(", Fan: %d (tuned +0)\n",x.fan_level);
> 
> I think you can do without the "tuned +0" which doesn't add much value.

Me too. But the old driver does the same, so I preferred to 
leave it as is.

> 
>> +	}
>> +
>> +	x.temp = temp;
>> +	x.casetemp = casetemp;
>> +
>>  	if( level >= 0 )
>>  		tune_fan( level );
>>  }
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 4/4] Return the fan speed via sysfs

[Goffredo Baroncelli]

On 08/03/2014 04:17 PM, Jean Delvare wrote:
> On Fri,  1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
>> Return the fan speed via sysfs:
>> /sys/devices/temperature/fan_level
> 
> Good idea. Even better would be if the driver would expose a standard
> hwmon interface for the temperature values. Fan level could go in
> attribute "pwm1" after proper scaling.

I tought the same. But until now the value logged is an integer value
between 1 and 11. So I preferred to leave it as is.

The thing that I can do is to *add* a further attribute called pwm1.
( I have to check how adm1031.c computes its pwm), because is a 
more standard interface.

I even thought to allow to change the fan speed from user space....



> 
> Please follow scripts/checkpatch.pl's advice to fix the coding style.
> 
>> This patch is copied from the Bryan Christianson's patch (see
>> debian bug #741663)
>>
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>>  drivers/macintosh/therm_windtunnel.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
>> index 0c4eb85..a2af7db 100644
>> --- a/drivers/macintosh/therm_windtunnel.c
>> +++ b/drivers/macintosh/therm_windtunnel.c
>> @@ -111,9 +111,15 @@ show_case_temperature( struct device *dev, struct device_attribute *attr, char *
>>  	return sprintf(buf, "%d.%d\n", x.casetemp>>8, (x.casetemp & 255)*10/256 );
>>  }
>>  
>> +static ssize_t
>> +show_fan_level( struct device *dev, struct device_attribute *attr, char *buf )
>> +{
>> +	return sprintf(buf, "%d\n", 11 - x.fan_level );
>> +}
>> +
>>  static DEVICE_ATTR(cpu_temperature, S_IRUGO, show_cpu_temperature, NULL );
>>  static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
>> -
>> +static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL );
>>  
>>  
>>  /************************************************************************/
>> @@ -265,6 +271,7 @@ setup_hardware( void )
>>  
>>  	err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>>  	err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
>> +	err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
>>  	if (err)
>>  		printk(KERN_WARNING
>>  			"Failed to create temperature attribute file(s).\n");
> 
> That's not your fault but please note that this construct is broken.
> You can't "or" error codes together, the result if two or more calls
> fail with different error codes is pretty random. Instead, each error
> must be tested individually. I know checkpatch.pl will complain if you
> do that but you can ignore it as is it the right thing to do in that
> case.

The really question is what we should do if the 2nd device_create_file()
would fail: we should fails the driver initialization ? or we should
continue, because even if there aren't some sysfs attributes the driver
work enough ?

> 
>> @@ -275,6 +282,7 @@ restore_regs( void )
>>  {
>>  	device_remove_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>>  	device_remove_file( &x.of_dev->dev, &dev_attr_case_temperature );
>> +	device_remove_file( &x.of_dev->dev, &dev_attr_fan_level );
>>  
>>  	write_reg( x.fan, 0x01, x.r1, 1 );
>>  	write_reg( x.fan, 0x20, x.r20, 1 );
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 3/4] Add the "verbose" module option.

[Jean Delvare]

On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
> On 08/03/2014 04:12 PM, Jean Delvare wrote:
> >> +	    (verbose > 0 && level >= 0)) {
> >> +		print_temp("CPU-temp: ", temp );
> >> +		if (casetemp)
> >> +			print_temp(", Case: ", casetemp );
> >> +		if (level >= 0)
> >> +			printk(", Fan: %d (tuned %+d)\n", 11-level,
> >> +				x.fan_level-level );
> >> +		else
> >> +			printk(", Fan: %d (tuned +0)\n",x.fan_level);
> > 
> > I think you can do without the "tuned +0" which doesn't add much value.
> 
> Me too. But the old driver does the same, so I preferred to 
> leave it as is.

I looked at the code again and no, I can't see the old code doing that.
It has "tuned %+d" only in tune_fan() which is only called if
level >= 0. The other printk (when tune_fan isn't called) doesn't have
a "tuned" part.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/4] Return the fan speed via sysfs

[Jean Delvare]

On Sun, 03 Aug 2014 17:27:17 +0200, Goffredo Baroncelli wrote:
> On 08/03/2014 04:17 PM, Jean Delvare wrote:
> > On Fri,  1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
> >> Return the fan speed via sysfs:
> >> /sys/devices/temperature/fan_level
> > 
> > Good idea. Even better would be if the driver would expose a standard
> > hwmon interface for the temperature values. Fan level could go in
> > attribute "pwm1" after proper scaling.
> 
> I tought the same. But until now the value logged is an integer value
> between 1 and 11. So I preferred to leave it as is.
> 
> The thing that I can do is to *add* a further attribute called pwm1.
> ( I have to check how adm1031.c computes its pwm), because is a 
> more standard interface.

The temperature attributes in hwmon would need different names and
units too (temp1_input and temp2_input, in millidegree C.) The
advantage is that all monitoring applications out there would pick up
these values automatically.

> I even thought to allow to change the fan speed from user space....

Ben will never let you do that ;-)

> >> @@ -265,6 +271,7 @@ setup_hardware( void )
> >>  
> >>  	err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
> >>  	err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
> >> +	err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
> >>  	if (err)
> >>  		printk(KERN_WARNING
> >>  			"Failed to create temperature attribute file(s).\n");
> > 
> > That's not your fault but please note that this construct is broken.
> > You can't "or" error codes together, the result if two or more calls
> > fail with different error codes is pretty random. Instead, each error
> > must be tested individually. I know checkpatch.pl will complain if you
> > do that but you can ignore it as is it the right thing to do in that
> > case.
> 
> The really question is what we should do if the 2nd device_create_file()
> would fail: we should fails the driver initialization ? or we should
> continue, because even if there aren't some sysfs attributes the driver
> work enough ?

I would log a warning and continue because it's a secondary feature of
the driver. Exactly as the driver already does - no change here.

In practice it's never going to happen so it doesn't really matter. I
just wanted to point out that the construct used in the driver was
dangerous. In this specific case it's harmless because the value of
"err" is never used (other than the fact that it's non-zero.) But if
the error code was included in the log message for example (which is
recommended), it would possibly make no sense.

Feel free to ignore this problem in this patch, it's not so important.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] Add the "verbose" module option.

[Goffredo Baroncelli]

On 08/03/2014 05:52 PM, Jean Delvare wrote:
> On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
>> On 08/03/2014 04:12 PM, Jean Delvare wrote:
>>>> +	    (verbose > 0 && level >= 0)) {
>>>> +		print_temp("CPU-temp: ", temp );
>>>> +		if (casetemp)
>>>> +			print_temp(", Case: ", casetemp );
>>>> +		if (level >= 0)
>>>> +			printk(", Fan: %d (tuned %+d)\n", 11-level,
>>>> +				x.fan_level-level );
>>>> +		else
>>>> +			printk(", Fan: %d (tuned +0)\n",x.fan_level);
>>>
>>> I think you can do without the "tuned +0" which doesn't add much value.
>>
>> Me too. But the old driver does the same, so I preferred to 
>> leave it as is.
> 
> I looked at the code again and no, I can't see the old code doing that.
> It has "tuned %+d" only in tune_fan() which is only called if
> level >= 0. The other printk (when tune_fan isn't called) doesn't have
> a "tuned" part.
> 

This is taken from an old log of a v3.2 kernel (no changes here):

[  886.510879] CPU-temp: 55.4 C, Case: 33.1 C,  Fan: 0 (tuned -11)
[  910.522869] CPU-temp: 56.0 C, Case: 33.5 C,  Fan: 0 (tuned +0)
[  958.546880] CPU-temp: 57.0 C, Case: 34.1 C,  Fan: 3 (tuned +3)

in the code if level <0, then there is no update in the log. But if
level >0 and level is equal to the previous one, this leads to
have "tuned +0"...

But I have to be honest: I have not fully understand how 
"level" is computed.

The printk without "(tuned %+d)" is never called because 
LOG_TEMP was #define(d) equal to 0.




-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 4/4] Return the fan speed via sysfs

[Goffredo Baroncelli]

On 08/03/2014 05:59 PM, Jean Delvare wrote:
> On Sun, 03 Aug 2014 17:27:17 +0200, Goffredo Baroncelli wrote:
>> On 08/03/2014 04:17 PM, Jean Delvare wrote:
>>> On Fri,  1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
>>>> Return the fan speed via sysfs:
>>>> /sys/devices/temperature/fan_level
>>>
>>> Good idea. Even better would be if the driver would expose a standard
>>> hwmon interface for the temperature values. Fan level could go in
>>> attribute "pwm1" after proper scaling.
>>
>> I tought the same. But until now the value logged is an integer value
>> between 1 and 11. So I preferred to leave it as is.
>>
>> The thing that I can do is to *add* a further attribute called pwm1.
>> ( I have to check how adm1031.c computes its pwm), because is a 
>> more standard interface.
> 
> The temperature attributes in hwmon would need different names and
> units too (temp1_input and temp2_input, in millidegree C.) The
> advantage is that all monitoring applications out there would pick up
> these values automatically.

Are you suggesting to add also a "temp1_input" attribute ?

> 
>> I even thought to allow to change the fan speed from user space....
> 
> Ben will never let you do that ;-)

What would be the risk ?. When the CPU temperature goes behind the limit, 
then the computer is switched off by an hardware protection (I am sure because
I had to changed a cpu board because a drift of the temperature sensor).

I am not suggesting to allow to change the fan speed, I am only asking which would
be the risk.

> 
>>>> @@ -265,6 +271,7 @@ setup_hardware( void )
>>>>  
>>>>  	err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>>>>  	err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
>>>> +	err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
>>>>  	if (err)
>>>>  		printk(KERN_WARNING
>>>>  			"Failed to create temperature attribute file(s).\n");
>>>
>>> That's not your fault but please note that this construct is broken.
>>> You can't "or" error codes together, the result if two or more calls
>>> fail with different error codes is pretty random. Instead, each error
>>> must be tested individually. I know checkpatch.pl will complain if you
>>> do that but you can ignore it as is it the right thing to do in that
>>> case.
>>
>> The really question is what we should do if the 2nd device_create_file()
>> would fail: we should fails the driver initialization ? or we should
>> continue, because even if there aren't some sysfs attributes the driver
>> work enough ?
> 
> I would log a warning and continue because it's a secondary feature of
> the driver. Exactly as the driver already does - no change here.
> 
> In practice it's never going to happen so it doesn't really matter. I
> just wanted to point out that the construct used in the driver was
> dangerous. In this specific case it's harmless because the value of
> "err" is never used (other than the fact that it's non-zero.) But if
> the error code was included in the log message for example (which is
> recommended), it would possibly make no sense.
> 
> Feel free to ignore this problem in this patch, it's not so important.
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 4/4] Return the fan speed via sysfs

[Jean Delvare]

Le Sunday 03 August 2014 à 18:42 +0200, Goffredo Baroncelli a écrit :
> On 08/03/2014 05:59 PM, Jean Delvare wrote:
> > The temperature attributes in hwmon would need different names and
> > units too (temp1_input and temp2_input, in millidegree C.) The
> > advantage is that all monitoring applications out there would pick up
> > these values automatically.
> 
> Are you suggesting to add also a "temp1_input" attribute ?

Yes, but as a hwmon class device attribute, not as an attribute
in /sys/devices/temperature.

> >> I even thought to allow to change the fan speed from user space....
> > 
> > Ben will never let you do that ;-)
> 
> What would be the risk ?. When the CPU temperature goes behind the limit, 
> then the computer is switched off by an hardware protection (I am sure because
> I had to changed a cpu board because a drift of the temperature sensor).

And you are still asking what the risk is? ;-)

> I am not suggesting to allow to change the fan speed, I am only asking which would
> be the risk.

If the user forces the fan too low, the system can overheat, and that
can result in user injury or hardware damage (or reduced lifetime of
hardware, at least.)

Some people think that the user should always be given full power over
his/her hardware. Others think that hardware should just work and users
should never have to care about such low-level details.

In this specific case I'd say don't change it unless you have a
compelling reason to.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] Add the "verbose" module option.

[Jean Delvare]

Le Sunday 03 August 2014 à 18:36 +0200, Goffredo Baroncelli a écrit :
> On 08/03/2014 05:52 PM, Jean Delvare wrote:
> > On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
> >> On 08/03/2014 04:12 PM, Jean Delvare wrote:
> >>>> +	    (verbose > 0 && level >= 0)) {
> >>>> +		print_temp("CPU-temp: ", temp );
> >>>> +		if (casetemp)
> >>>> +			print_temp(", Case: ", casetemp );
> >>>> +		if (level >= 0)
> >>>> +			printk(", Fan: %d (tuned %+d)\n", 11-level,
> >>>> +				x.fan_level-level );
> >>>> +		else
> >>>> +			printk(", Fan: %d (tuned +0)\n",x.fan_level);
> >>>
> >>> I think you can do without the "tuned +0" which doesn't add much value.
> >>
> >> Me too. But the old driver does the same, so I preferred to 
> >> leave it as is.
> > 
> > I looked at the code again and no, I can't see the old code doing that.
> > It has "tuned %+d" only in tune_fan() which is only called if
> > level >= 0. The other printk (when tune_fan isn't called) doesn't have
> > a "tuned" part.
>
> This is taken from an old log of a v3.2 kernel (no changes here):
> 
> [  886.510879] CPU-temp: 55.4 C, Case: 33.1 C,  Fan: 0 (tuned -11)
> [  910.522869] CPU-temp: 56.0 C, Case: 33.5 C,  Fan: 0 (tuned +0)
> [  958.546880] CPU-temp: 57.0 C, Case: 34.1 C,  Fan: 3 (tuned +3)
> 
> in the code if level <0, then there is no update in the log. But if
> level >0 and level is equal to the previous one, this leads to
> have "tuned +0"...

I agree with that.

> But I have to be honest: I have not fully understand how 
> "level" is computed.

I agree with that too :/

> The printk without "(tuned %+d)" is never called because 
> LOG_TEMP was #define(d) equal to 0.

And this is what your second printk is replacing. So it should not have
the "(tuned *)" either.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] Add the "verbose" module option.

[Goffredo Baroncelli]

On 08/04/2014 10:46 AM, Jean Delvare wrote:
> Le Sunday 03 August 2014 à 18:36 +0200, Goffredo Baroncelli a écrit :
>> On 08/03/2014 05:52 PM, Jean Delvare wrote:
>>> On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
>>>> On 08/03/2014 04:12 PM, Jean Delvare wrote:
>>>>>> +	    (verbose > 0 && level >= 0)) {
>>>>>> +		print_temp("CPU-temp: ", temp );
>>>>>> +		if (casetemp)
>>>>>> +			print_temp(", Case: ", casetemp );
>>>>>> +		if (level >= 0)
>>>>>> +			printk(", Fan: %d (tuned %+d)\n", 11-level,
>>>>>> +				x.fan_level-level );
>>>>>> +		else
>>>>>> +			printk(", Fan: %d (tuned +0)\n",x.fan_level);
>>>>>
>>>>> I think you can do without the "tuned +0" which doesn't add much value.
>>>>
>>>> Me too. But the old driver does the same, so I preferred to 
>>>> leave it as is.
>>>
>>> I looked at the code again and no, I can't see the old code doing that.
>>> It has "tuned %+d" only in tune_fan() which is only called if
>>> level >= 0. The other printk (when tune_fan isn't called) doesn't have
>>> a "tuned" part.
>>
>> This is taken from an old log of a v3.2 kernel (no changes here):
>>
>> [  886.510879] CPU-temp: 55.4 C, Case: 33.1 C,  Fan: 0 (tuned -11)
>> [  910.522869] CPU-temp: 56.0 C, Case: 33.5 C,  Fan: 0 (tuned +0)
>> [  958.546880] CPU-temp: 57.0 C, Case: 34.1 C,  Fan: 3 (tuned +3)
>>
>> in the code if level <0, then there is no update in the log. But if
>> level >0 and level is equal to the previous one, this leads to
>> have "tuned +0"...
> 
> I agree with that.
> 
>> But I have to be honest: I have not fully understand how 
>> "level" is computed.
> 
> I agree with that too :/
> 
>> The printk without "(tuned %+d)" is never called because 
>> LOG_TEMP was #define(d) equal to 0.
> 
> And this is what your second printk is replacing. So it should not have
> the "(tuned *)" either.
> 
I removed the printk(s) from tune_fan(); the ones leaved replaced 
both the ones inside tune_fan() and the ones outside.

Anyway, Benjamin which is your opinion ? 
For me is equal to remove or to leave "(tune +0)" (when the tuning is equal to 0).
Jean think it is better to remove "(tune +0)" (when the tuning is equal to 0).
So if you haven't any objection I will remove it.

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 3/4] Add the "verbose" module option.

[Jean Delvare]

Le Monday 04 August 2014 à 19:10 +0200, Goffredo Baroncelli a écrit :
> On 08/04/2014 10:46 AM, Jean Delvare wrote:
> > Le Sunday 03 August 2014 à 18:36 +0200, Goffredo Baroncelli a écrit :
> >> The printk without "(tuned %+d)" is never called because 
> >> LOG_TEMP was #define(d) equal to 0.
> > 
> > And this is what your second printk is replacing. So it should not have
> > the "(tuned *)" either.
> > 
> I removed the printk(s) from tune_fan(); the ones leaved replaced 
> both the ones inside tune_fan() and the ones outside.

I understand that. But you still had two final printks, one with "(tuned
%+d)" when level >= 0, which corresponds to what was printed in tune_fan
before, and another one when level < 0, which corresponds to what was
printed in poll_temp before, and that one did not have a "tuned +0" part
so I simply fail to see why its replacement should have it.

I admit I'm surprised we're arguing on that as it seems really obvious
to me, so I can only hope I'm not missing something even more obvious.

> Anyway, Benjamin which is your opinion ? 
> For me is equal to remove or to leave "(tune +0)" (when the tuning is equal to 0).
> Jean think it is better to remove "(tune +0)" (when the tuning is equal to 0).
> So if you haven't any objection I will remove it.

s/remove/not introduce/ is my actual point.

But I'm not going to argue more, I'm not even using that driver and it's
a debug message only anyway, so do as you wish.

Thanks,
-- 
Jean Delvare
SUSE L3 Support