[PATCH][v5] therm_windtunnel does not work properly on PowerMac G4

[PATCH][v5] therm_windtunnel does not 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 5 patches to solve this 
problem (tested on my PowerMac G4). Only the first two are strictly
related to the problem, the others three may be skipped.

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

5) export the temperature via the hwmon subsistem

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)
The patch 5) export the temperatures via hwmon, a more standard 
interface. I also added the internal sensor of the adm1030, which 
I called "Case2", because it measure the same temperature /+/- 1C) 
of the "Case" sensor.

I have to highlight that I tried to export also the fan speed,
but I was unable to get it, without affecting the fan speed:
when I set the bit 2 (TACH 1 En) of the register 0x1 
(Configuration 2), it seemed that the speed every 4/5s dropped,
then it raised quickly....
I didn't perform other test to avoid damages.

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:
v1: 2014/07/30
	- first issue
v2: 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
v3: 2014/08/06
	- export the temperatures via hwmon
	- export the internal temperature sensor of the adm1030
	- little cleanup due to the suggestion of checkpatch.pl (
	  and Jean Delvare)
	- removed the "(tune +0)" in the log.
v4: 2014/08/07
	- accepted some small cleanup suggestions from Jean Delvare
	- replaced SENSOR_DEVICE_ATTR with DEVICE_ATTR, and
	  added ATTRIBUTE_GROUPS() use
v5: 2014/08/09
	- hanlde the return error of 
	  hwmon_device_register_with_groups() with IS_ERR() "macro"
	- better explanation about the source of patch #4

[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

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

[PATCH 1/5] 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.1.0.rc1

[PATCH 2/5] 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.1.0.rc1

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

[Goffredo Baroncelli]

Add a "verbose" option to control 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 | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 1e50455..68f1067 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -44,7 +44,9 @@
 #include <asm/sections.h>
 #include <asm/macio.h>
 
-#define LOG_TEMP		0			/* continuously log temperature */
+static int verbose = 1;
+module_param(verbose, int, 0644);
+MODULE_PARM_DESC(verbose, "Verbosity level: 0=silent, 1=log the fan tuning, 2=log the temperature");
 
 static struct {
 	volatile int		running;
@@ -157,10 +159,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;
 }
@@ -168,7 +166,7 @@ tune_fan( int fan_setting )
 static void
 poll_temp( void )
 {
-	int temp, i, level, casetemp;
+	int temp, i, level, casetemp, tempchanged;
 
 	temp = read_reg( x.thermostat, 0, 2 );
 
@@ -179,14 +177,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 +190,26 @@ poll_temp( void )
 		level = fan_table[i].fan_up_setting;
 	x.upind = i;
 
+	/*
+	 * if verbose >=1 log each fan tuning
+	 * if verbose >=2 log each cpu temperature change
+	 */
+	tempchanged = x.temp != temp || x.casetemp != casetemp;
+	if ((verbose > 1 && tempchanged) ||
+	    (verbose > 0 && level >= 0)) {
+		print_temp(KERN_INFO "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\n", 11-x.fan_level);
+	}
+
+	x.temp = temp;
+	x.casetemp = casetemp;
+
 	if( level >= 0 )
 		tune_fan( level );
 }
-- 
2.1.0.rc1

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

[Goffredo Baroncelli]

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

This patch is extracted from a Bryan Christianson's bigger one (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 68f1067..30a6588 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -109,9 +109,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);
 
 
 /************************************************************************/
@@ -264,6 +270,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");
@@ -274,6 +281,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.1.0.rc1

[PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

Export the temperature via the hwmon subsystem.
See the list below for the sensors exported:

$ cd /sys/devices/temperature/hwmon/hwmon0
$ echo "name: $(cat name)"; for i in temp*; do echo "$i: $(cat $i)"; done
name: therm_windtunnel
temp1_input: 59312
temp1_label: CPU
temp2_input: 36750
temp2_label: Case
temp3_input: 37750
temp3_label: Case2

The Case2 temperature is the sensor temperature inside the adm1030.

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

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 30a6588..fd9061d 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -37,6 +37,8 @@
 #include <linux/init.h>
 #include <linux/kthread.h>
 #include <linux/of_platform.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 
 #include <asm/prom.h>
 #include <asm/machdep.h>
@@ -58,9 +60,12 @@ static struct {
 	struct i2c_client	*thermostat;
 	struct i2c_client	*fan;
 
+	struct device		*hwmon;
+
 	int			overheat_temp;		/* 100% fan at this temp */
 	int			overheat_hyst;
 	int			temp;
+	int			casetemp2;
 	int			casetemp;
 	int			fan_level;		/* active fan_table setting */
 
@@ -120,6 +125,66 @@ static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
 static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL);
 
 
+static ssize_t
+show_temp1(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d%03d\n", x.temp>>8,
+					(x.temp & 0xff) * 1000 / 256);
+}
+
+static ssize_t
+show_temp1_label(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "CPU\n");
+}
+
+static ssize_t
+show_temp2(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d%03d\n", x.casetemp>>8,
+					(x.casetemp & 0xff) * 1000 / 256);
+}
+
+static ssize_t
+show_temp2_label(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "Case\n");
+}
+
+
+static ssize_t
+show_temp3(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d%03d\n", x.casetemp2>>8,
+					(x.casetemp2 & 0xff) * 1000 / 256);
+}
+
+static ssize_t
+show_temp3_label(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "Case2\n");
+}
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp1, NULL);
+static DEVICE_ATTR(temp1_label, S_IRUGO, show_temp1_label, NULL);
+static DEVICE_ATTR(temp2_input, S_IRUGO, show_temp2, NULL);
+static DEVICE_ATTR(temp2_label, S_IRUGO, show_temp2_label, NULL);
+static DEVICE_ATTR(temp3_input, S_IRUGO, show_temp3, NULL);
+static DEVICE_ATTR(temp3_label, S_IRUGO, show_temp3_label, NULL);
+
+static struct attribute  *therm_windtunnel_attrs[] = {
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_label.attr,
+	&dev_attr_temp2_input.attr,
+	&dev_attr_temp2_label.attr,
+	&dev_attr_temp3_input.attr,
+	&dev_attr_temp3_label.attr,
+
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(therm_windtunnel);
+
 /************************************************************************/
 /*	controller thread						*/
 /************************************************************************/
@@ -172,16 +237,23 @@ tune_fan( int fan_setting )
 static void
 poll_temp( void )
 {
-	int temp, i, level, casetemp, tempchanged;
+	int temp, i, level, casetemp, tempchanged, casetemp2, reg06;
 
+	/* temperature read from ds1775 */
 	temp = read_reg( x.thermostat, 0, 2 );
 
 	/* this actually occurs when the computer is loaded */
 	if( temp < 0 )
 		return;
 
-	casetemp = read_reg(x.fan, 0x0b, 1) << 8;
-	casetemp |= (read_reg(x.fan, 0x06, 1) & 0x7) << 5;
+	/*
+	 * temperatures read from the adm1030
+	 * casetemp is the external temperature sensor
+	 * casetemp2 is the internal temperature sensor
+	 */
+	reg06 = read_reg(x.fan, 0x06, 1);
+	casetemp = (read_reg(x.fan, 0x0b, 1) << 8) | (reg06 & 0x07 << 5);
+	casetemp2 = (read_reg(x.fan, 0x0a, 1) << 8) | (reg06 & 0xc0);
 
 	level = -1;
 	for( i=0; (temp & 0xffff) > fan_table[i].temp ; i++ )
@@ -200,12 +272,15 @@ poll_temp( void )
 	 * if verbose >=1 log each fan tuning
 	 * if verbose >=2 log each cpu temperature change
 	 */
-	tempchanged = x.temp != temp || x.casetemp != casetemp;
+	tempchanged = x.temp != temp || x.casetemp != casetemp ||
+		x.casetemp2 != casetemp2;
 	if ((verbose > 1 && tempchanged) ||
 	    (verbose > 0 && level >= 0)) {
 		print_temp(KERN_INFO "CPU-temp: ", temp);
 		if (casetemp)
 			print_temp(", Case: ", casetemp);
+		if (casetemp2)
+			print_temp(", Case2: ", casetemp2);
 		if (level >= 0)
 			printk(", Fan: %d (tuned %+d)\n", 11-level,
 				x.fan_level-level);
@@ -215,6 +290,7 @@ poll_temp( void )
 
 	x.temp = temp;
 	x.casetemp = casetemp;
+	x.casetemp2 = casetemp2;
 
 	if( level >= 0 )
 		tune_fan( level );
@@ -274,11 +350,23 @@ setup_hardware( void )
 	if (err)
 		printk(KERN_WARNING
 			"Failed to create temperature attribute file(s).\n");
+
+	x.hwmon = hwmon_device_register_with_groups(&x.of_dev->dev,
+			"therm_windtunnel", NULL,
+			therm_windtunnel_groups);
+	if (IS_ERR(x.hwmon)) {
+		dev_warn(&x.of_dev->dev, "Failed to create the hwmon device\n");
+		x.hwmon = NULL;
+	}
 }
 
 static void
 restore_regs( void )
 {
+	if (x.hwmon)
+		hwmon_device_unregister(x.hwmon);
+	x.hwmon = NULL;
+
 	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);
-- 
2.1.0.rc1

Re: [PATCH][v5] therm_windtunnel does not work properly on PowerMac G4

[Goffredo Baroncelli]

Hi Benjamin,

do you have any feedback about this ? Do you think that it would be possible 
to include these patches in a next pull-for-linus ?

Let me know, if you want other changes.

BR
G.Baroncelli

On 08/09/2014 08:49 AM, Goffredo Baroncelli wrote:
> 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 5 patches to solve this 
> problem (tested on my PowerMac G4). Only the first two are strictly
> related to the problem, the others three may be skipped.
> 
> 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
> 
> 5) export the temperature via the hwmon subsistem
> 
> 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)
> The patch 5) export the temperatures via hwmon, a more standard 
> interface. I also added the internal sensor of the adm1030, which 
> I called "Case2", because it measure the same temperature /+/- 1C) 
> of the "Case" sensor.
> 
> I have to highlight that I tried to export also the fan speed,
> but I was unable to get it, without affecting the fan speed:
> when I set the bit 2 (TACH 1 En) of the register 0x1 
> (Configuration 2), it seemed that the speed every 4/5s dropped,
> then it raised quickly....
> I didn't perform other test to avoid damages.
> 
> 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:
> v1: 2014/07/30
> 	- first issue
> v2: 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
> v3: 2014/08/06
> 	- export the temperatures via hwmon
> 	- export the internal temperature sensor of the adm1030
> 	- little cleanup due to the suggestion of checkpatch.pl (
> 	  and Jean Delvare)
> 	- removed the "(tune +0)" in the log.
> v4: 2014/08/07
> 	- accepted some small cleanup suggestions from Jean Delvare
> 	- replaced SENSOR_DEVICE_ATTR with DEVICE_ATTR, and
> 	  added ATTRIBUTE_GROUPS() use
> v5: 2014/08/09
> 	- hanlde the return error of 
> 	  hwmon_device_register_with_groups() with IS_ERR() "macro"
> 	- better explanation about the source of patch #4
> 
> [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
> 


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

[RESEND][PATCH 1/5] 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.1.0.rc1

[RESEND][PATCH 2/5] 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.1.0.rc1

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

[Goffredo Baroncelli]

Add a "verbose" option to control 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 | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 1e50455..68f1067 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -44,7 +44,9 @@
 #include <asm/sections.h>
 #include <asm/macio.h>
 
-#define LOG_TEMP		0			/* continuously log temperature */
+static int verbose = 1;
+module_param(verbose, int, 0644);
+MODULE_PARM_DESC(verbose, "Verbosity level: 0=silent, 1=log the fan tuning, 2=log the temperature");
 
 static struct {
 	volatile int		running;
@@ -157,10 +159,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;
 }
@@ -168,7 +166,7 @@ tune_fan( int fan_setting )
 static void
 poll_temp( void )
 {
-	int temp, i, level, casetemp;
+	int temp, i, level, casetemp, tempchanged;
 
 	temp = read_reg( x.thermostat, 0, 2 );
 
@@ -179,14 +177,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 +190,26 @@ poll_temp( void )
 		level = fan_table[i].fan_up_setting;
 	x.upind = i;
 
+	/*
+	 * if verbose >=1 log each fan tuning
+	 * if verbose >=2 log each cpu temperature change
+	 */
+	tempchanged = x.temp != temp || x.casetemp != casetemp;
+	if ((verbose > 1 && tempchanged) ||
+	    (verbose > 0 && level >= 0)) {
+		print_temp(KERN_INFO "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\n", 11-x.fan_level);
+	}
+
+	x.temp = temp;
+	x.casetemp = casetemp;
+
 	if( level >= 0 )
 		tune_fan( level );
 }
-- 
2.1.0.rc1

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

[Goffredo Baroncelli]

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

This patch is extracted from a Bryan Christianson's bigger one (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 68f1067..30a6588 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -109,9 +109,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);
 
 
 /************************************************************************/
@@ -264,6 +270,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");
@@ -274,6 +281,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.1.0.rc1

[RESEND][PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

Export the temperature via the hwmon subsystem.
See the list below for the sensors exported:

$ cd /sys/devices/temperature/hwmon/hwmon0
$ echo "name: $(cat name)"; for i in temp*; do echo "$i: $(cat $i)"; done
name: therm_windtunnel
temp1_input: 59312
temp1_label: CPU
temp2_input: 36750
temp2_label: Case
temp3_input: 37750
temp3_label: Case2

The Case2 temperature is the sensor temperature inside the adm1030.

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

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 30a6588..fd9061d 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -37,6 +37,8 @@
 #include <linux/init.h>
 #include <linux/kthread.h>
 #include <linux/of_platform.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 
 #include <asm/prom.h>
 #include <asm/machdep.h>
@@ -58,9 +60,12 @@ static struct {
 	struct i2c_client	*thermostat;
 	struct i2c_client	*fan;
 
+	struct device		*hwmon;
+
 	int			overheat_temp;		/* 100% fan at this temp */
 	int			overheat_hyst;
 	int			temp;
+	int			casetemp2;
 	int			casetemp;
 	int			fan_level;		/* active fan_table setting */
 
@@ -120,6 +125,66 @@ static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
 static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL);
 
 
+static ssize_t
+show_temp1(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d%03d\n", x.temp>>8,
+					(x.temp & 0xff) * 1000 / 256);
+}
+
+static ssize_t
+show_temp1_label(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "CPU\n");
+}
+
+static ssize_t
+show_temp2(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d%03d\n", x.casetemp>>8,
+					(x.casetemp & 0xff) * 1000 / 256);
+}
+
+static ssize_t
+show_temp2_label(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "Case\n");
+}
+
+
+static ssize_t
+show_temp3(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d%03d\n", x.casetemp2>>8,
+					(x.casetemp2 & 0xff) * 1000 / 256);
+}
+
+static ssize_t
+show_temp3_label(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "Case2\n");
+}
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp1, NULL);
+static DEVICE_ATTR(temp1_label, S_IRUGO, show_temp1_label, NULL);
+static DEVICE_ATTR(temp2_input, S_IRUGO, show_temp2, NULL);
+static DEVICE_ATTR(temp2_label, S_IRUGO, show_temp2_label, NULL);
+static DEVICE_ATTR(temp3_input, S_IRUGO, show_temp3, NULL);
+static DEVICE_ATTR(temp3_label, S_IRUGO, show_temp3_label, NULL);
+
+static struct attribute  *therm_windtunnel_attrs[] = {
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_label.attr,
+	&dev_attr_temp2_input.attr,
+	&dev_attr_temp2_label.attr,
+	&dev_attr_temp3_input.attr,
+	&dev_attr_temp3_label.attr,
+
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(therm_windtunnel);
+
 /************************************************************************/
 /*	controller thread						*/
 /************************************************************************/
@@ -172,16 +237,23 @@ tune_fan( int fan_setting )
 static void
 poll_temp( void )
 {
-	int temp, i, level, casetemp, tempchanged;
+	int temp, i, level, casetemp, tempchanged, casetemp2, reg06;
 
+	/* temperature read from ds1775 */
 	temp = read_reg( x.thermostat, 0, 2 );
 
 	/* this actually occurs when the computer is loaded */
 	if( temp < 0 )
 		return;
 
-	casetemp = read_reg(x.fan, 0x0b, 1) << 8;
-	casetemp |= (read_reg(x.fan, 0x06, 1) & 0x7) << 5;
+	/*
+	 * temperatures read from the adm1030
+	 * casetemp is the external temperature sensor
+	 * casetemp2 is the internal temperature sensor
+	 */
+	reg06 = read_reg(x.fan, 0x06, 1);
+	casetemp = (read_reg(x.fan, 0x0b, 1) << 8) | (reg06 & 0x07 << 5);
+	casetemp2 = (read_reg(x.fan, 0x0a, 1) << 8) | (reg06 & 0xc0);
 
 	level = -1;
 	for( i=0; (temp & 0xffff) > fan_table[i].temp ; i++ )
@@ -200,12 +272,15 @@ poll_temp( void )
 	 * if verbose >=1 log each fan tuning
 	 * if verbose >=2 log each cpu temperature change
 	 */
-	tempchanged = x.temp != temp || x.casetemp != casetemp;
+	tempchanged = x.temp != temp || x.casetemp != casetemp ||
+		x.casetemp2 != casetemp2;
 	if ((verbose > 1 && tempchanged) ||
 	    (verbose > 0 && level >= 0)) {
 		print_temp(KERN_INFO "CPU-temp: ", temp);
 		if (casetemp)
 			print_temp(", Case: ", casetemp);
+		if (casetemp2)
+			print_temp(", Case2: ", casetemp2);
 		if (level >= 0)
 			printk(", Fan: %d (tuned %+d)\n", 11-level,
 				x.fan_level-level);
@@ -215,6 +290,7 @@ poll_temp( void )
 
 	x.temp = temp;
 	x.casetemp = casetemp;
+	x.casetemp2 = casetemp2;
 
 	if( level >= 0 )
 		tune_fan( level );
@@ -274,11 +350,23 @@ setup_hardware( void )
 	if (err)
 		printk(KERN_WARNING
 			"Failed to create temperature attribute file(s).\n");
+
+	x.hwmon = hwmon_device_register_with_groups(&x.of_dev->dev,
+			"therm_windtunnel", NULL,
+			therm_windtunnel_groups);
+	if (IS_ERR(x.hwmon)) {
+		dev_warn(&x.of_dev->dev, "Failed to create the hwmon device\n");
+		x.hwmon = NULL;
+	}
 }
 
 static void
 restore_regs( void )
 {
+	if (x.hwmon)
+		hwmon_device_unregister(x.hwmon);
+	x.hwmon = NULL;
+
 	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);
-- 
2.1.0.rc1

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

[Goffredo Baroncelli]

Add a "verbose" option to control 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 | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 1e50455..68f1067 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -44,7 +44,9 @@
 #include <asm/sections.h>
 #include <asm/macio.h>
 
-#define LOG_TEMP		0			/* continuously log temperature */
+static int verbose = 1;
+module_param(verbose, int, 0644);
+MODULE_PARM_DESC(verbose, "Verbosity level: 0=silent, 1=log the fan tuning, 2=log the temperature");
 
 static struct {
 	volatile int		running;
@@ -157,10 +159,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;
 }
@@ -168,7 +166,7 @@ tune_fan( int fan_setting )
 static void
 poll_temp( void )
 {
-	int temp, i, level, casetemp;
+	int temp, i, level, casetemp, tempchanged;
 
 	temp = read_reg( x.thermostat, 0, 2 );
 
@@ -179,14 +177,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 +190,26 @@ poll_temp( void )
 		level = fan_table[i].fan_up_setting;
 	x.upind = i;
 
+	/*
+	 * if verbose >=1 log each fan tuning
+	 * if verbose >=2 log each cpu temperature change
+	 */
+	tempchanged = x.temp != temp || x.casetemp != casetemp;
+	if ((verbose > 1 && tempchanged) ||
+	    (verbose > 0 && level >= 0)) {
+		print_temp(KERN_INFO "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\n", 11-x.fan_level);
+	}
+
+	x.temp = temp;
+	x.casetemp = casetemp;
+
 	if( level >= 0 )
 		tune_fan( level );
 }
-- 
2.0.1

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

[Goffredo Baroncelli]

On 08/07/2014 06:43 PM, Jean Delvare wrote:
> On Thu, 07 Aug 2014 18:29:23 +0200, Goffredo Baroncelli wrote:
>> On 08/07/2014 10:52 AM, Jean Delvare wrote:
>>> Le Wednesday 06 August 2014 à 21:05 +0000, Goffredo Baroncelli a écrit :
>>>> +	 */
>>>> +	tempchanged = x.temp != temp || x.casetemp != casetemp;
>>>> +	if ((verbose > 1 && tempchanged) ||
>>>> +	    (verbose > 0 && level >= 0)) {
>>>> +		printk(KERN_INFO);
>>>> +		print_temp("CPU-temp: ", temp);
>>>
>>> This can be written more efficiently as a single statement:
>>>
>>> 		print_temp(KERN_INFO "CPU-temp: ", temp);
>>
>> I suppose that KERN_* has to be in the beginning of the line. 
> 
> Correct.
> 
>> Because a single line is composed by several prink,
> 
> In this case, it is, but FYI, this is generally discouraged. The reason
> is that another piece of the kernel may be calling printk at the same
> time, and then that other message may split your own message into
> pieces. If you run checkpatch.pl on this file, you'll see it complains
> about this.
> 
>> KERN_INFO has 
>> to be only in the first printk. To me it seems more polite to have
>> one printk for the level, and the others (there are more than one) 
>> for the message parts.
> 
> The fewer printks is better. Ideally there would be only one to avoid
> the risk of line splitting altogether. I understand this isn't easy to
> achieve in this case, but I still believe that you shouldn't have more
> calls to printk than necessary, to reduce the risk.
> 
Ok, now I understand the reason. I will remove the first printk.

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

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

[Jean Delvare]

On Thu, 07 Aug 2014 18:29:23 +0200, Goffredo Baroncelli wrote:
> On 08/07/2014 10:52 AM, Jean Delvare wrote:
> > Le Wednesday 06 August 2014 à 21:05 +0000, Goffredo Baroncelli a écrit :
> >> +	 */
> >> +	tempchanged = x.temp != temp || x.casetemp != casetemp;
> >> +	if ((verbose > 1 && tempchanged) ||
> >> +	    (verbose > 0 && level >= 0)) {
> >> +		printk(KERN_INFO);
> >> +		print_temp("CPU-temp: ", temp);
> > 
> > This can be written more efficiently as a single statement:
> > 
> > 		print_temp(KERN_INFO "CPU-temp: ", temp);
> 
> I suppose that KERN_* has to be in the beginning of the line. 

Correct.

> Because a single line is composed by several prink,

In this case, it is, but FYI, this is generally discouraged. The reason
is that another piece of the kernel may be calling printk at the same
time, and then that other message may split your own message into
pieces. If you run checkpatch.pl on this file, you'll see it complains
about this.

> KERN_INFO has 
> to be only in the first printk. To me it seems more polite to have
> one printk for the level, and the others (there are more than one) 
> for the message parts.

The fewer printks is better. Ideally there would be only one to avoid
the risk of line splitting altogether. I understand this isn't easy to
achieve in this case, but I still believe that you shouldn't have more
calls to printk than necessary, to reduce the risk.

-- 
Jean Delvare
SUSE L3 Support

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

[Goffredo Baroncelli]

On 08/07/2014 10:52 AM, Jean Delvare wrote:
> Le Wednesday 06 August 2014 à 21:05 +0000, Goffredo Baroncelli a écrit :
>> Add a "verbose" option to control 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 | 39 +++++++++++++++++++++++-------------
>>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> Looks overall good, minor comments inline below.
> 
>> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
>> index 1e50455..7c512db 100644
>> --- a/drivers/macintosh/therm_windtunnel.c
>> +++ b/drivers/macintosh/therm_windtunnel.c
>> @@ -44,7 +44,9 @@
>>  #include <asm/sections.h>
>>  #include <asm/macio.h>
>>  
>> -#define LOG_TEMP		0			/* continuously log temperature */
>> +static int verbose = 1;
>> +module_param(verbose, int, 0644);
>> +MODULE_PARM_DESC(verbose, "Verbosity level: 0=silent, 1=log the fan tuning, 2=log the temperature");
>>  
>>  static struct {
>>  	volatile int		running;
>> @@ -157,10 +159,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;
>>  }
>> @@ -168,7 +166,7 @@ tune_fan( int fan_setting )
>>  static void
>>  poll_temp( void )
>>  {
>> -	int temp, i, level, casetemp;
>> +	int temp, i, level, casetemp, tempchanged;
>>  
>>  	temp = read_reg( x.thermostat, 0, 2 );
>>  
>> @@ -179,14 +177,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 +190,27 @@ 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
> 
> Maybe this is just me but I think "verbose >= 1" and "verbose >= 2"
> would be easier to understand. Same in the code below.

correct, I am planning another revision due to Guenter concern. So 
I will correct it.
> 
>> +	 */
>> +	tempchanged = x.temp != temp || x.casetemp != casetemp;
>> +	if ((verbose > 1 && tempchanged) ||
>> +	    (verbose > 0 && level >= 0)) {
>> +		printk(KERN_INFO);
>> +		print_temp("CPU-temp: ", temp);
> 
> This can be written more efficiently as a single statement:
> 
> 		print_temp(KERN_INFO "CPU-temp: ", temp);

I suppose that KERN_* has to be in the beginning of the line. 

Because a single line is composed by several prink, KERN_INFO has 
to be only in the first printk. To me it seems more polite to have
one printk for the level, and the others (there are more than one) 
for the message parts.

> 
>> +		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\n", 11-x.fan_level);
>> +	}
>> +
>> +	x.temp = temp;
>> +	x.casetemp = casetemp;
>> +
>>  	if( level >= 0 )
>>  		tune_fan( level );
>>  }
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 


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

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

[Jean Delvare]

Le Wednesday 06 August 2014 à 21:05 +0000, Goffredo Baroncelli a écrit :
> Add a "verbose" option to control 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 | 39 +++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 14 deletions(-)

Looks overall good, minor comments inline below.

> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index 1e50455..7c512db 100644
> --- a/drivers/macintosh/therm_windtunnel.c
> +++ b/drivers/macintosh/therm_windtunnel.c
> @@ -44,7 +44,9 @@
>  #include <asm/sections.h>
>  #include <asm/macio.h>
>  
> -#define LOG_TEMP		0			/* continuously log temperature */
> +static int verbose = 1;
> +module_param(verbose, int, 0644);
> +MODULE_PARM_DESC(verbose, "Verbosity level: 0=silent, 1=log the fan tuning, 2=log the temperature");
>  
>  static struct {
>  	volatile int		running;
> @@ -157,10 +159,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;
>  }
> @@ -168,7 +166,7 @@ tune_fan( int fan_setting )
>  static void
>  poll_temp( void )
>  {
> -	int temp, i, level, casetemp;
> +	int temp, i, level, casetemp, tempchanged;
>  
>  	temp = read_reg( x.thermostat, 0, 2 );
>  
> @@ -179,14 +177,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 +190,27 @@ 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

Maybe this is just me but I think "verbose >= 1" and "verbose >= 2"
would be easier to understand. Same in the code below.

> +	 */
> +	tempchanged = x.temp != temp || x.casetemp != casetemp;
> +	if ((verbose > 1 && tempchanged) ||
> +	    (verbose > 0 && level >= 0)) {
> +		printk(KERN_INFO);
> +		print_temp("CPU-temp: ", temp);

This can be written more efficiently as a single statement:

		print_temp(KERN_INFO "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\n", 11-x.fan_level);
> +	}
> +
> +	x.temp = temp;
> +	x.casetemp = casetemp;
> +
>  	if( level >= 0 )
>  		tune_fan( level );
>  }

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Goffredo Baroncelli]

Add a "verbose" option to control 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 | 39 +++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 1e50455..7c512db 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -44,7 +44,9 @@
 #include <asm/sections.h>
 #include <asm/macio.h>
 
-#define LOG_TEMP		0			/* continuously log temperature */
+static int verbose = 1;
+module_param(verbose, int, 0644);
+MODULE_PARM_DESC(verbose, "Verbosity level: 0=silent, 1=log the fan tuning, 2=log the temperature");
 
 static struct {
 	volatile int		running;
@@ -157,10 +159,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;
 }
@@ -168,7 +166,7 @@ tune_fan( int fan_setting )
 static void
 poll_temp( void )
 {
-	int temp, i, level, casetemp;
+	int temp, i, level, casetemp, tempchanged;
 
 	temp = read_reg( x.thermostat, 0, 2 );
 
@@ -179,14 +177,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 +190,27 @@ 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
+	 */
+	tempchanged = x.temp != temp || x.casetemp != casetemp;
+	if ((verbose > 1 && tempchanged) ||
+	    (verbose > 0 && level >= 0)) {
+		printk(KERN_INFO);
+		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\n", 11-x.fan_level);
+	}
+
+	x.temp = temp;
+	x.casetemp = casetemp;
+
 	if( level >= 0 )
 		tune_fan( level );
 }
-- 
2.0.1