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

[PATCH][v3] 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 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:
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
v2: 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.

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

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

[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

[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 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 7c512db..b6cba98 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);
 
 
 /************************************************************************/
@@ -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

[PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@iniwnd.it>

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 | 103 +++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index b6cba98..a6757d7 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,75 @@ 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>>8);
+}
+
+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>>8);
+}
+
+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>>8);
+}
+
+static ssize_t
+show_temp3_label(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "Case2\n");
+}
+
+#define TWT_ATTRIB(name, func) \
+	static SENSOR_DEVICE_ATTR(name##_input, S_IRUGO, func, NULL, 0); \
+	static SENSOR_DEVICE_ATTR(name##_label, S_IRUGO, func##_label, \
+					NULL, 0)
+
+TWT_ATTRIB(temp1, show_temp1);
+TWT_ATTRIB(temp2, show_temp2);
+TWT_ATTRIB(temp3, show_temp3);
+
+
+static struct attribute  *therm_windtunnel_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_label.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_label.dev_attr.attr,
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_label.dev_attr.attr,
+
+	NULL,
+};
+
+static const struct attribute_group therm_windtunnel_attr_group = {
+	.attrs  = therm_windtunnel_attributes,
+};
+
+static const struct attribute_group *therm_windtunnel_attr_groups[] = {
+	&therm_windtunnel_attr_group,
+	NULL,
+};
+
 /************************************************************************/
 /*	controller thread						*/
 /************************************************************************/
@@ -172,16 +246,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,13 +281,16 @@ poll_temp( void )
 	 * if verbose >0 log each fan tuning
 	 * if verbose >1 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)) {
 		printk(KERN_INFO);
 		print_temp("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);
@@ -216,6 +300,7 @@ poll_temp( void )
 
 	x.temp = temp;
 	x.casetemp = casetemp;
+	x.casetemp2 = casetemp2;
 
 	if( level >= 0 )
 		tune_fan( level );
@@ -275,11 +360,21 @@ 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_attr_groups);
+	if (!x.hwmon)
+		dev_warn(&x.of_dev->dev, "Failed to create the hwmon device\n");
 }
 
 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.0.1

Re: [PATCH 5/5] Export the temperatures via hwmon

[Guenter Roeck]

On Wed, Aug 06, 2014 at 09:05:03PM +0000, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@iniwnd.it>
> 
> 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
> 
Makes me wonder why you don't report the fan speed through hwmon.

> The Case2 temperature is the sensor temperature inside the adm1030.
> 
There are standard hwmon drivers for lm75, ds1775, and adm1030.
Personally I think it would make more sense to use those directly.
But I guess that would be for another day.

> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  drivers/macintosh/therm_windtunnel.c | 103 +++++++++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index b6cba98..a6757d7 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,75 @@ 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>>8);

I personally prefer if code follows coding style, with spaces around operands,
and if calculations are less cryptic.

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

... and aligned second lines.

> +}
> +
> +static ssize_t
> +show_temp3_label(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "Case2\n");
> +}
> +
> +#define TWT_ATTRIB(name, func) \
> +	static SENSOR_DEVICE_ATTR(name##_input, S_IRUGO, func, NULL, 0); \
> +	static SENSOR_DEVICE_ATTR(name##_label, S_IRUGO, func##_label, \
> +					NULL, 0)
> +
> +TWT_ATTRIB(temp1, show_temp1);
> +TWT_ATTRIB(temp2, show_temp2);
> +TWT_ATTRIB(temp3, show_temp3);
> +
> +
A single empty line should be sufficient. Personally I am also not
a fan of such macros (and neither is checkpatch), and prefer the use
of direct macros as less confusing.

Also, using three different functions for three different attributes
where the only difference is the variable name defeats the purpose
of using SENSOR_DEVICE_ATTR, which has an index variable for exactly 
that reason. I would suggest to either use an indexed array to access
the temperatures, or use DEVICE_ATTR.

> +static struct attribute  *therm_windtunnel_attributes[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_label.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_label.dev_attr.attr,
> +
> +	NULL,
> +};
> +
> +static const struct attribute_group therm_windtunnel_attr_group = {
> +	.attrs  = therm_windtunnel_attributes,
> +};
> +
> +static const struct attribute_group *therm_windtunnel_attr_groups[] = {
> +	&therm_windtunnel_attr_group,
> +	NULL,
> +};

ATTRIBUTE_GROUPS() is a nice and useful macro.

> +
>  /************************************************************************/
>  /*	controller thread						*/
>  /************************************************************************/
> @@ -172,16 +246,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
> +	 */

What if there is no adm1030 ? Or is that always there ?
Just wondering.

> +	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,13 +281,16 @@ poll_temp( void )
>  	 * if verbose >0 log each fan tuning
>  	 * if verbose >1 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)) {
>  		printk(KERN_INFO);
>  		print_temp("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);

Wow, this logging must clutter the kernel log quite substantially
if enabled.

> @@ -216,6 +300,7 @@ poll_temp( void )
>  
>  	x.temp = temp;
>  	x.casetemp = casetemp;
> +	x.casetemp2 = casetemp2;
>  
>  	if( level >= 0 )
>  		tune_fan( level );
> @@ -275,11 +360,21 @@ 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_attr_groups);
> +	if (!x.hwmon)
> +		dev_warn(&x.of_dev->dev, "Failed to create the hwmon device\n");
>  }
>  
>  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.0.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
> 

Re: [PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

On 08/07/2014 01:18 AM, Guenter Roeck wrote:
> On Wed, Aug 06, 2014 at 09:05:03PM +0000, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli <kreijack@iniwnd.it>
>>
>> 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
>>
> Makes me wonder why you don't report the fan speed through hwmon.

See the first email. Basically when I start to read the fan speed,
it became irregular, so I stopped the test. I don't know
if it is a my faulted hardware, or an hw design problem.
The fan is a 2 wire fan.

> 
>> The Case2 temperature is the sensor temperature inside the adm1030.
>>
> There are standard hwmon drivers for lm75, ds1775, and adm1030.
> Personally I think it would make more sense to use those directly.
> But I guess that would be for another day.
> 
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>>  drivers/macintosh/therm_windtunnel.c | 103 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 99 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
>> index b6cba98..a6757d7 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,75 @@ 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>>8);
> 
> I personally prefer if code follows coding style, with spaces around operands,
> and if calculations are less cryptic.
> 
>> +}
>> +
>> +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>>8);
>> +}
>> +
>> +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>>8);
> 
> ... and aligned second lines.
> 
>> +}
>> +
>> +static ssize_t
>> +show_temp3_label(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	return sprintf(buf, "Case2\n");
>> +}
>> +
>> +#define TWT_ATTRIB(name, func) \
>> +	static SENSOR_DEVICE_ATTR(name##_input, S_IRUGO, func, NULL, 0); \
>> +	static SENSOR_DEVICE_ATTR(name##_label, S_IRUGO, func##_label, \
>> +					NULL, 0)
>> +
>> +TWT_ATTRIB(temp1, show_temp1);
>> +TWT_ATTRIB(temp2, show_temp2);
>> +TWT_ATTRIB(temp3, show_temp3);
>> +
>> +
> A single empty line should be sufficient. Personally I am also not
> a fan of such macros (and neither is checkpatch), and prefer the use
> of direct macros as less confusing.

True, I put a three line macro to avoid to write three line... it
doesn't make sense.

> 
> Also, using three different functions for three different attributes
> where the only difference is the variable name defeats the purpose
> of using SENSOR_DEVICE_ATTR, which has an index variable for exactly 
> that reason. I would suggest to either use an indexed array to access
> the temperatures, or use DEVICE_ATTR.
> 
>> +static struct attribute  *therm_windtunnel_attributes[] = {
>> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
>> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
>> +	&sensor_dev_attr_temp2_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
>> +	&sensor_dev_attr_temp3_label.dev_attr.attr,
>> +
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group therm_windtunnel_attr_group = {
>> +	.attrs  = therm_windtunnel_attributes,
>> +};
>> +
>> +static const struct attribute_group *therm_windtunnel_attr_groups[] = {
>> +	&therm_windtunnel_attr_group,
>> +	NULL,
>> +};
> 
> ATTRIBUTE_GROUPS() is a nice and useful macro.
I will give a look to this macro.
> 
>> +
>>  /************************************************************************/
>>  /*	controller thread						*/
>>  /************************************************************************/
>> @@ -172,16 +246,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
>> +	 */
> 
> What if there is no adm1030 ? Or is that always there ?
> Just wondering.

poll_temp() is called by the daemon, which is started only after 
the discovering of both the adm1030 and the ds1775: (see patch #1
at the function try_start_control_loop())

> 
>> +	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,13 +281,16 @@ poll_temp( void )
>>  	 * if verbose >0 log each fan tuning
>>  	 * if verbose >1 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)) {
>>  		printk(KERN_INFO);
>>  		print_temp("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);
> 
> Wow, this logging must clutter the kernel log quite substantially
> if enabled.

It is  disable by default.

> 
>> @@ -216,6 +300,7 @@ poll_temp( void )
>>  
>>  	x.temp = temp;
>>  	x.casetemp = casetemp;
>> +	x.casetemp2 = casetemp2;
>>  
>>  	if( level >= 0 )
>>  		tune_fan( level );
>> @@ -275,11 +360,21 @@ 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_attr_groups);
>> +	if (!x.hwmon)
>> +		dev_warn(&x.of_dev->dev, "Failed to create the hwmon device\n");
>>  }
>>  
>>  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.0.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>
>>
> 


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

Re: [PATCH 5/5] Export the temperatures via hwmon

[Guenter Roeck]

On 08/06/2014 11:03 PM, Goffredo Baroncelli wrote:
> On 08/07/2014 01:18 AM, Guenter Roeck wrote:
>> On Wed, Aug 06, 2014 at 09:05:03PM +0000, Goffredo Baroncelli wrote:
>>> From: Goffredo Baroncelli <kreijack@iniwnd.it>
>>>
>>> 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
>>>
>> Makes me wonder why you don't report the fan speed through hwmon.
>
> See the first email. Basically when I start to read the fan speed,
> it became irregular, so I stopped the test. I don't know
> if it is a my faulted hardware, or an hw design problem.
> The fan is a 2 wire fan.
>

Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".

So you are saying that returning the fan speed with a non-hwmon attribute works,
but returning it with a hwmon attribute doesn't ? Not really sure if I understand
your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
or something in your line of argument is inconsistent.

Guenter

Re: [PATCH 5/5] Export the temperatures via hwmon

[Jean Delvare]

Hi Guenter,

On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
> 
> So you are saying that returning the fan speed with a non-hwmon attribute works,
> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
> or something in your line of argument is inconsistent.

fan_level is a fan speed _control_ value, like pwm1. It is not a fan
speed monitoring value.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 5/5] Export the temperatures via hwmon

[Guenter Roeck]

On 08/06/2014 11:52 PM, Jean Delvare wrote:
> Hi Guenter,
>
> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
>>
>> So you are saying that returning the fan speed with a non-hwmon attribute works,
>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
>> or something in your line of argument is inconsistent.
>
> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
> speed monitoring value.
>
Ah, ok. The patch description doesn't seem to match, though.
And why not export it as pwm1, if that is what it is ?

Guenter

Re: [PATCH 5/5] Export the temperatures via hwmon

[Jean Delvare]

Le Thursday 07 August 2014 à 00:36 -0700, Guenter Roeck a écrit :
> On 08/06/2014 11:52 PM, Jean Delvare wrote:
> > Hi Guenter,
> >
> > On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
> >> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
> >>
> >> So you are saying that returning the fan speed with a non-hwmon attribute works,
> >> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
> >> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
> >> or something in your line of argument is inconsistent.
> >
> > fan_level is a fan speed _control_ value, like pwm1. It is not a fan
> > speed monitoring value.
> >
> Ah, ok. The patch description doesn't seem to match, though.
> And why not export it as pwm1, if that is what it is ?

Well, /sys/devices/temperature is a non-standard interface anyway, so it
does not really matter. I already discussed with Goffredo the
possibility to also expose it as pwm1 on the hwmon side, this might
happen later.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/5] Remove attach_method because un-used

[Jean Delvare]

Le Wednesday 06 August 2014 à 21:05 +0000, Goffredo Baroncelli a écrit :
> 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,

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

-- 
Jean Delvare
SUSE L3 Support

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

Re: [PATCH 5/5] Export the temperatures via hwmon

[Guenter Roeck]

On 08/07/2014 01:35 AM, Jean Delvare wrote:
> Le Thursday 07 August 2014 à 00:36 -0700, Guenter Roeck a écrit :
>> On 08/06/2014 11:52 PM, Jean Delvare wrote:
>>> Hi Guenter,
>>>
>>> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
>>>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
>>>>
>>>> So you are saying that returning the fan speed with a non-hwmon attribute works,
>>>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
>>>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
>>>> or something in your line of argument is inconsistent.
>>>
>>> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
>>> speed monitoring value.
>>>
>> Ah, ok. The patch description doesn't seem to match, though.
>> And why not export it as pwm1, if that is what it is ?
>
> Well, /sys/devices/temperature is a non-standard interface anyway, so it
> does not really matter. I already discussed with Goffredo the

That is true.

> possibility to also expose it as pwm1 on the hwmon side, this might
> happen later.
>
Ok.

Thanks,
Guenter

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]

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 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 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

On 08/07/2014 09:36 AM, Guenter Roeck wrote:
> On 08/06/2014 11:52 PM, Jean Delvare wrote:
>> Hi Guenter,
>>
>> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
>>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
>>>
>>> So you are saying that returning the fan speed with a non-hwmon attribute works,
>>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
>>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
>>> or something in your line of argument is inconsistent.
>>
>> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
>> speed monitoring value.
>>
> Ah, ok. The patch description doesn't seem to match, though.
> And why not export it as pwm1, if that is what it is ?
> 
> Guenter
> 
> 

the exported fan_level value is a coefficient near proportional to the speed [*];
so it is not the speed nor the pwm.
I tried to read the pwm/speed value, but when I did it, every 5/6 seconds the
fan seemed to stop for 1s, then the speed raised.... So I stopped the test.

These patches (the first two) solved a real issue: with the last kernels this
driver doesn't work at all, and the fan go to maximum speed (very loud !)
The other three are an improvement.

When (if) these patches will be accepted I want to write another solution,
but definitely not now. And even if it would work for me, it is very likely 
that will  be accepted because nobody is able to test it on all hardware.

BR
G.Baroncelli

[*] It is a bit more complicated: basically the adm1030 controls the fan speed
on the basis of an external temperature sensor (the "case" sensor). 
Higher is this temperature higher is the fan speed.
The pwm of the fan is computed on the basis of the following value:
	- min_temp
	- range_temp -> max_temp=min_temp+range_temp
	- min_pwm_value
and of course
	- "case" sensor


The fan_level is an index computed on the basis of the "cpu" temperature (
which is different from the "case" temperature referred above). This
temperature is used to update the min_temp above.

So the fan speed id computed on the basis of two external sensor:
- cpu sensor
- case sensor.

To complicate further the things, the range_temp is set to an undocumented 
value: this parameter is set via a register (0x25, bit 2:0), which accepted 
values between 0x00 and 0x04. However it is set to 0x07

See http://lxr.free-electrons.com/source/drivers/macintosh/therm_windtunnel.c#L153
and the adm1030 datasheet.

I am reluctant to change this thing because this driver is for an obsolete 
platform (apple stopped the powermac in 2004/2005), and bad or good I have to
suppose that it works. Moreover I am not in the position to test the changes 
outside my hardware (1 powermac DP@1Ghz mdd)


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

Re: [PATCH 5/5] Export the temperatures via hwmon

[Guenter Roeck]

On Thu, Aug 07, 2014 at 07:50:00PM +0200, Goffredo Baroncelli wrote:
> On 08/07/2014 09:36 AM, Guenter Roeck wrote:
> > On 08/06/2014 11:52 PM, Jean Delvare wrote:
> >> Hi Guenter,
> >>
> >> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
> >>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
> >>>
> >>> So you are saying that returning the fan speed with a non-hwmon attribute works,
> >>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
> >>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
> >>> or something in your line of argument is inconsistent.
> >>
> >> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
> >> speed monitoring value.
> >>
> > Ah, ok. The patch description doesn't seem to match, though.
> > And why not export it as pwm1, if that is what it is ?
> > 
> > Guenter
> > 
> > 
> 
> the exported fan_level value is a coefficient near proportional to the speed [*];
> so it is not the speed nor the pwm.
> I tried to read the pwm/speed value, but when I did it, every 5/6 seconds the
> fan seemed to stop for 1s, then the speed raised.... So I stopped the test.
> 
> These patches (the first two) solved a real issue: with the last kernels this
> driver doesn't work at all, and the fan go to maximum speed (very loud !)
> The other three are an improvement.
> 
> When (if) these patches will be accepted I want to write another solution,
> but definitely not now. And even if it would work for me, it is very likely 
> that will  be accepted because nobody is able to test it on all hardware.
> 
Might have been easier to just drop all this non-standard code and instantiate
the adm1030 using the adm1031 driver instead.

Guenter

Re: [PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

On 08/07/2014 08:16 PM, Guenter Roeck wrote:
> On Thu, Aug 07, 2014 at 07:50:00PM +0200, Goffredo Baroncelli wrote:

>> When (if) these patches will be accepted I want to write another solution,
>> but definitely not now. And even if it would work for me, it is very likely 
>> that will  be accepted because nobody is able to test it on all hardware.
>>
> Might have been easier to just drop all this non-standard code and instantiate
> the adm1030 using the adm1031 driver instead.

I really like the idea, and this is a possible future work; but now we can't. 

We have to suppose that the current driver work (they stopped to work because
it was changed the i2c api) and because I can't test a change on
all the apple hardware (powermac, apple laptop...), I am allowed to make only 
very small changes.

My (our) first goal is to bring to life *this* driver.

I repeat I am fully convinced that re-using the "official" i2c drive/client
is the "right thing to do" (tm). But due to the lacking of the hardware where it
is possible to test this module, we need to be very conservative.

> 
> Guenter
>
Goffredo


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

Re: [PATCH 5/5] Export the temperatures via hwmon

[Matt Helsley]

On Thu, Aug 07, 2014 at 07:50:00PM +0200, Goffredo Baroncelli wrote:
> On 08/07/2014 09:36 AM, Guenter Roeck wrote:
> > On 08/06/2014 11:52 PM, Jean Delvare wrote:
> >> Hi Guenter,
> >>
> >> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
> >>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
> >>>
> >>> So you are saying that returning the fan speed with a non-hwmon attribute works,
> >>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
> >>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
> >>> or something in your line of argument is inconsistent.
> >>
> >> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
> >> speed monitoring value.
> >>
> > Ah, ok. The patch description doesn't seem to match, though.
> > And why not export it as pwm1, if that is what it is ?
> > 
> > Guenter
> > 
> > 
> 
> the exported fan_level value is a coefficient near proportional to the speed [*];
> so it is not the speed nor the pwm.

> I tried to read the pwm/speed value, but when I did it, every 5/6 seconds the
> fan seemed to stop for 1s, then the speed raised.... So I stopped the test.
> 
> These patches (the first two) solved a real issue: with the last kernels this
> driver doesn't work at all, and the fan go to maximum speed (very loud !)
> The other three are an improvement.
> 
> When (if) these patches will be accepted I want to write another solution,
> but definitely not now. And even if it would work for me, it is very likely 
> that will  be accepted because nobody is able to test it on all hardware.
> 
> BR
> G.Baroncelli
> 
> [*] It is a bit more complicated: basically the adm1030 controls the fan speed
> on the basis of an external temperature sensor (the "case" sensor). 
> Higher is this temperature higher is the fan speed.
> The pwm of the fan is computed on the basis of the following value:
> 	- min_temp
> 	- range_temp -> max_temp=min_temp+range_temp
> 	- min_pwm_value
> and of course
> 	- "case" sensor
> 
> 
> The fan_level is an index computed on the basis of the "cpu" temperature (
> which is different from the "case" temperature referred above). This
> temperature is used to update the min_temp above.
> 
> So the fan speed id computed on the basis of two external sensor:
> - cpu sensor
> - case sensor.
> 
> To complicate further the things, the range_temp is set to an undocumented 
> value: this parameter is set via a register (0x25, bit 2:0), which accepted 
> values between 0x00 and 0x04. However it is set to 0x07
> 
> See http://lxr.free-electrons.com/source/drivers/macintosh/therm_windtunnel.c#L153
> and the adm1030 datasheet.
> 
> I am reluctant to change this thing because this driver is for an obsolete 
> platform (apple stopped the powermac in 2004/2005), and bad or good I have to
> suppose that it works. Moreover I am not in the position to test the changes 
> outside my hardware (1 powermac DP@1Ghz mdd)

This seems like a valuable explanation of the meaning of the fan_level
values. When I read the code I was wondering what the magic number "11" was
doing for instance and this helps clear it up a little. It's a nit perhaps
but some of this information might be useful in the commit message for that
patch, the code, or both.

Also, I think the patches from "Bryan" should have a "From:" attribution line
at the start of the commit message (See Documentation/SubmittingPatches).

Cheers,
	-Matt Helsley

Re: [PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

On 08/07/2014 11:19 PM, Matt Helsley wrote:
> On Thu, Aug 07, 2014 at 07:50:00PM +0200, Goffredo Baroncelli wrote:
>> On 08/07/2014 09:36 AM, Guenter Roeck wrote:
>>> On 08/06/2014 11:52 PM, Jean Delvare wrote:
>>>> Hi Guenter,
>>>>
>>>> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
>>>>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
>>>>>
>>>>> So you are saying that returning the fan speed with a non-hwmon attribute works,
>>>>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
>>>>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
>>>>> or something in your line of argument is inconsistent.
>>>>
>>>> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
>>>> speed monitoring value.
>>>>
>>> Ah, ok. The patch description doesn't seem to match, though.
>>> And why not export it as pwm1, if that is what it is ?
>>>
>>> Guenter
>>>
>>>
>>
>> the exported fan_level value is a coefficient near proportional to the speed [*];
>> so it is not the speed nor the pwm.
> 
>> I tried to read the pwm/speed value, but when I did it, every 5/6 seconds the
>> fan seemed to stop for 1s, then the speed raised.... So I stopped the test.
>>
>> These patches (the first two) solved a real issue: with the last kernels this
>> driver doesn't work at all, and the fan go to maximum speed (very loud !)
>> The other three are an improvement.
>>
>> When (if) these patches will be accepted I want to write another solution,
>> but definitely not now. And even if it would work for me, it is very likely 
>> that will  be accepted because nobody is able to test it on all hardware.
>>
>> BR
>> G.Baroncelli
>>
>> [*] It is a bit more complicated: basically the adm1030 controls the fan speed
>> on the basis of an external temperature sensor (the "case" sensor). 
>> Higher is this temperature higher is the fan speed.
>> The pwm of the fan is computed on the basis of the following value:
>> 	- min_temp
>> 	- range_temp -> max_temp=min_temp+range_temp
>> 	- min_pwm_value
>> and of course
>> 	- "case" sensor
>>
>>
>> The fan_level is an index computed on the basis of the "cpu" temperature (
>> which is different from the "case" temperature referred above). This
>> temperature is used to update the min_temp above.
>>
>> So the fan speed id computed on the basis of two external sensor:
>> - cpu sensor
>> - case sensor.
>>
>> To complicate further the things, the range_temp is set to an undocumented 
>> value: this parameter is set via a register (0x25, bit 2:0), which accepted 
>> values between 0x00 and 0x04. However it is set to 0x07
>>
>> See http://lxr.free-electrons.com/source/drivers/macintosh/therm_windtunnel.c#L153
>> and the adm1030 datasheet.
>>
>> I am reluctant to change this thing because this driver is for an obsolete 
>> platform (apple stopped the powermac in 2004/2005), and bad or good I have to
>> suppose that it works. Moreover I am not in the position to test the changes 
>> outside my hardware (1 powermac DP@1Ghz mdd)
> 
> This seems like a valuable explanation of the meaning of the fan_level
> values. When I read the code I was wondering what the magic number "11" was
> doing for instance and this helps clear it up a little. It's a nit perhaps
> but some of this information might be useful in the commit message for that
> patch, the code, or both.

I will take in account your suggestion. In case of another revision, I will improve the comment.



> Also, I think the patches from "Bryan" should have a "From:" attribution line
> at the start of the commit message (See Documentation/SubmittingPatches).

Thanks for the suggestion. In case of another revision I will put the "From:" tag.

> Cheers,
> 	-Matt Helsley
> 


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

Re: [PATCH 5/5] Export the temperatures via hwmon

[Guenter Roeck]

On 08/08/2014 07:54 AM, Goffredo Baroncelli wrote:
> On 08/07/2014 11:19 PM, Matt Helsley wrote:
>> On Thu, Aug 07, 2014 at 07:50:00PM +0200, Goffredo Baroncelli wrote:
>>> On 08/07/2014 09:36 AM, Guenter Roeck wrote:
>>>> On 08/06/2014 11:52 PM, Jean Delvare wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
>>>>>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
>>>>>>
>>>>>> So you are saying that returning the fan speed with a non-hwmon attribute works,
>>>>>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
>>>>>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
>>>>>> or something in your line of argument is inconsistent.
>>>>>
>>>>> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
>>>>> speed monitoring value.
>>>>>
>>>> Ah, ok. The patch description doesn't seem to match, though.
>>>> And why not export it as pwm1, if that is what it is ?
>>>>
>>>> Guenter
>>>>
>>>>
>>>
>>> the exported fan_level value is a coefficient near proportional to the speed [*];
>>> so it is not the speed nor the pwm.
>>
>>> I tried to read the pwm/speed value, but when I did it, every 5/6 seconds the
>>> fan seemed to stop for 1s, then the speed raised.... So I stopped the test.
>>>
>>> These patches (the first two) solved a real issue: with the last kernels this
>>> driver doesn't work at all, and the fan go to maximum speed (very loud !)
>>> The other three are an improvement.
>>>
>>> When (if) these patches will be accepted I want to write another solution,
>>> but definitely not now. And even if it would work for me, it is very likely
>>> that will  be accepted because nobody is able to test it on all hardware.
>>>
>>> BR
>>> G.Baroncelli
>>>
>>> [*] It is a bit more complicated: basically the adm1030 controls the fan speed
>>> on the basis of an external temperature sensor (the "case" sensor).
>>> Higher is this temperature higher is the fan speed.
>>> The pwm of the fan is computed on the basis of the following value:
>>> 	- min_temp
>>> 	- range_temp -> max_temp=min_temp+range_temp
>>> 	- min_pwm_value
>>> and of course
>>> 	- "case" sensor
>>>
>>>
>>> The fan_level is an index computed on the basis of the "cpu" temperature (
>>> which is different from the "case" temperature referred above). This
>>> temperature is used to update the min_temp above.
>>>
>>> So the fan speed id computed on the basis of two external sensor:
>>> - cpu sensor
>>> - case sensor.
>>>
>>> To complicate further the things, the range_temp is set to an undocumented
>>> value: this parameter is set via a register (0x25, bit 2:0), which accepted
>>> values between 0x00 and 0x04. However it is set to 0x07
>>>
>>> See http://lxr.free-electrons.com/source/drivers/macintosh/therm_windtunnel.c#L153
>>> and the adm1030 datasheet.
>>>
>>> I am reluctant to change this thing because this driver is for an obsolete
>>> platform (apple stopped the powermac in 2004/2005), and bad or good I have to
>>> suppose that it works. Moreover I am not in the position to test the changes
>>> outside my hardware (1 powermac DP@1Ghz mdd)
>>
>> This seems like a valuable explanation of the meaning of the fan_level
>> values. When I read the code I was wondering what the magic number "11" was
>> doing for instance and this helps clear it up a little. It's a nit perhaps
>> but some of this information might be useful in the commit message for that
>> patch, the code, or both.
>
> I will take in account your suggestion. In case of another revision, I will improve the comment.
>
I just noticed - hwmon_device_register_with_groups() returns ERR_PTR
on errors, not NULL, so checking for a NULL return value won't help
much if there is an error.

Guenter

Re: [PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli]

On 08/08/2014 06:30 PM, Guenter Roeck wrote:
> On 08/08/2014 07:54 AM, Goffredo Baroncelli wrote:
[...]
> I just noticed - hwmon_device_register_with_groups() returns ERR_PTR
> on errors, not NULL, so checking for a NULL return value won't help
> much if there is an error.

Thanks, due to this, in the next day I will issue a new revision.


> Guenter
> 
> 


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

[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 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 | 94 ++++++++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 30a6588..f41b0d3 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,21 @@ 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 (!x.hwmon)
+		dev_warn(&x.of_dev->dev, "Failed to create the hwmon device\n");
 }
 
 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.0.1