* [PATCH 1/4] Update drivers names to the ones invoked by i2c-powermac
2014-08-01 14:00 [PATCH][v2] therm_windtunnel doesn't work properly on PowerMac G4 Goffredo Baroncelli
@ 2014-08-01 14:00 ` Goffredo Baroncelli
2014-08-01 14:00 ` [PATCH 2/4] Remove attach_method because un-used Goffredo Baroncelli
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-01 14:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: ", ", ", 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/4] Remove attach_method because un-used
2014-08-01 14:00 [PATCH][v2] therm_windtunnel doesn't work properly on PowerMac G4 Goffredo Baroncelli
2014-08-01 14:00 ` [PATCH 1/4] Update drivers names to the ones invoked by i2c-powermac Goffredo Baroncelli
@ 2014-08-01 14:00 ` Goffredo Baroncelli
2014-08-01 14:00 ` [PATCH 3/4] Add the "verbose" module option Goffredo Baroncelli
2014-08-01 14:00 ` [PATCH 4/4] Return the fan speed via sysfs Goffredo Baroncelli
3 siblings, 0 replies; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-01 14:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: ", ", ", 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/4] Add the "verbose" module option.
2014-08-01 14:00 [PATCH][v2] therm_windtunnel doesn't work properly on PowerMac G4 Goffredo Baroncelli
2014-08-01 14:00 ` [PATCH 1/4] Update drivers names to the ones invoked by i2c-powermac Goffredo Baroncelli
2014-08-01 14:00 ` [PATCH 2/4] Remove attach_method because un-used Goffredo Baroncelli
@ 2014-08-01 14:00 ` Goffredo Baroncelli
2014-08-03 14:12 ` Jean Delvare
2014-08-01 14:00 ` [PATCH 4/4] Return the fan speed via sysfs Goffredo Baroncelli
3 siblings, 1 reply; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-01 14:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: ", ", ", Goffredo Baroncelli
The "verbose" option controls the message in the kernel log
verbose = 0 no message
verbose = 1 log only the fan speed changes
verbose = 2 log the fan speed changes and the temperature changes
Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
drivers/macintosh/therm_windtunnel.c | 37 +++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 1e50455..0c4eb85 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -44,7 +44,11 @@
#include <asm/sections.h>
#include <asm/macio.h>
-#define LOG_TEMP 0 /* continuously log temperature */
+static int verbose = 1; /* see description below */
+module_param(verbose, int, 0644);
+MODULE_PARM_DESC(verbose, "Vebosity level: 0=silent, "
+ "1=log the fan tuning, "
+ "2=log the temperature.");
static struct {
volatile int running;
@@ -157,10 +161,6 @@ tune_fan( int fan_setting )
/* write_reg( x.fan, 0x24, val, 1 ); */
write_reg( x.fan, 0x25, val, 1 );
write_reg( x.fan, 0x20, 0, 1 );
- print_temp("CPU-temp: ", x.temp );
- if( x.casetemp )
- print_temp(", Case: ", x.casetemp );
- printk(", Fan: %d (tuned %+d)\n", 11-fan_setting, x.fan_level-fan_setting );
x.fan_level = fan_setting;
}
@@ -179,14 +179,6 @@ poll_temp( void )
casetemp = read_reg(x.fan, 0x0b, 1) << 8;
casetemp |= (read_reg(x.fan, 0x06, 1) & 0x7) << 5;
- if( LOG_TEMP && x.temp != temp ) {
- print_temp("CPU-temp: ", temp );
- print_temp(", Case: ", casetemp );
- printk(", Fan: %d\n", 11-x.fan_level );
- }
- x.temp = temp;
- x.casetemp = casetemp;
-
level = -1;
for( i=0; (temp & 0xffff) > fan_table[i].temp ; i++ )
;
@@ -200,6 +192,25 @@ poll_temp( void )
level = fan_table[i].fan_up_setting;
x.upind = i;
+ /*
+ * if verbose >0 log each fan tuning
+ * if verbose >1 log each cpu temperature change
+ */
+ if ((verbose > 1 && x.temp != temp ) ||
+ (verbose > 0 && level >= 0)) {
+ print_temp("CPU-temp: ", temp );
+ if (casetemp)
+ print_temp(", Case: ", casetemp );
+ if (level >= 0)
+ printk(", Fan: %d (tuned %+d)\n", 11-level,
+ x.fan_level-level );
+ else
+ printk(", Fan: %d (tuned +0)\n",x.fan_level);
+ }
+
+ x.temp = temp;
+ x.casetemp = casetemp;
+
if( level >= 0 )
tune_fan( level );
}
--
2.0.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] Add the "verbose" module option.
2014-08-01 14:00 ` [PATCH 3/4] Add the "verbose" module option Goffredo Baroncelli
@ 2014-08-03 14:12 ` Jean Delvare
2014-08-03 15:12 ` Goffredo Baroncelli
0 siblings, 1 reply; 17+ messages in thread
From: Jean Delvare @ 2014-08-03 14:12 UTC (permalink / raw)
To: Goffredo Baroncelli
Cc: Benjamin Herrenschmidt, jdelvare, linux-kernel, bryan,
Goffredo Baroncelli
Hi Goffredo,
You messed up your Cc's ;-)
On Fri, 1 Aug 2014 14:00:49 +0000, Goffredo Baroncelli wrote:
> The "verbose" option controls the message in the kernel log
> verbose = 0 no message
> verbose = 1 log only the fan speed changes
> verbose = 2 log the fan speed changes and the temperature changes
>
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
> drivers/macintosh/therm_windtunnel.c | 37 +++++++++++++++++++++++-------------
> 1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index 1e50455..0c4eb85 100644
> --- a/drivers/macintosh/therm_windtunnel.c
> +++ b/drivers/macintosh/therm_windtunnel.c
> @@ -44,7 +44,11 @@
> #include <asm/sections.h>
> #include <asm/macio.h>
>
> -#define LOG_TEMP 0 /* continuously log temperature */
> +static int verbose = 1; /* see description below */
This comment seems useless.
> +module_param(verbose, int, 0644);
> +MODULE_PARM_DESC(verbose, "Vebosity level: 0=silent, "
Typo: Verbosity
> + "1=log the fan tuning, "
> + "2=log the temperature.");
Trailing dot is not needed.
>
> static struct {
> volatile int running;
> @@ -157,10 +161,6 @@ tune_fan( int fan_setting )
> /* write_reg( x.fan, 0x24, val, 1 ); */
> write_reg( x.fan, 0x25, val, 1 );
> write_reg( x.fan, 0x20, 0, 1 );
> - print_temp("CPU-temp: ", x.temp );
> - if( x.casetemp )
> - print_temp(", Case: ", x.casetemp );
> - printk(", Fan: %d (tuned %+d)\n", 11-fan_setting, x.fan_level-fan_setting );
>
> x.fan_level = fan_setting;
> }
> @@ -179,14 +179,6 @@ poll_temp( void )
> casetemp = read_reg(x.fan, 0x0b, 1) << 8;
> casetemp |= (read_reg(x.fan, 0x06, 1) & 0x7) << 5;
>
> - if( LOG_TEMP && x.temp != temp ) {
> - print_temp("CPU-temp: ", temp );
> - print_temp(", Case: ", casetemp );
> - printk(", Fan: %d\n", 11-x.fan_level );
> - }
> - x.temp = temp;
> - x.casetemp = casetemp;
> -
> level = -1;
> for( i=0; (temp & 0xffff) > fan_table[i].temp ; i++ )
> ;
> @@ -200,6 +192,25 @@ poll_temp( void )
> level = fan_table[i].fan_up_setting;
> x.upind = i;
>
> + /*
> + * if verbose >0 log each fan tuning
> + * if verbose >1 log each cpu temperature change
> + */
I think it is a good idea to have all the printing in a single place.
> + if ((verbose > 1 && x.temp != temp ) ||
No space before closing parenthesis. I know the original code did not
follow the standard coding style but you have the opportunity to make
it better. Same many times below. scripts/checkpatch.pl tells you about
that and many other style issues, most of which are easily fixable.
Also don't you want to log changes of case temperature too?
> + (verbose > 0 && level >= 0)) {
> + print_temp("CPU-temp: ", temp );
> + if (casetemp)
> + print_temp(", Case: ", casetemp );
> + if (level >= 0)
> + printk(", Fan: %d (tuned %+d)\n", 11-level,
> + x.fan_level-level );
> + else
> + printk(", Fan: %d (tuned +0)\n",x.fan_level);
I think you can do without the "tuned +0" which doesn't add much value.
> + }
> +
> + x.temp = temp;
> + x.casetemp = casetemp;
> +
> if( level >= 0 )
> tune_fan( level );
> }
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] Add the "verbose" module option.
2014-08-03 14:12 ` Jean Delvare
@ 2014-08-03 15:12 ` Goffredo Baroncelli
2014-08-03 15:52 ` Jean Delvare
0 siblings, 1 reply; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-03 15:12 UTC (permalink / raw)
To: Jean Delvare
Cc: Benjamin Herrenschmidt, linux-kernel, bryan, Goffredo Baroncelli
On 08/03/2014 04:12 PM, Jean Delvare wrote:
> Hi Goffredo,
>
> You messed up your Cc's ;-)
I fight hard with git-send-email.... In the next trip I will check two times !
>
> On Fri, 1 Aug 2014 14:00:49 +0000, Goffredo Baroncelli wrote:
>> The "verbose" option controls the message in the kernel log
>> verbose = 0 no message
>> verbose = 1 log only the fan speed changes
>> verbose = 2 log the fan speed changes and the temperature changes
>>
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>> drivers/macintosh/therm_windtunnel.c | 37 +++++++++++++++++++++++-------------
>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
>> index 1e50455..0c4eb85 100644
>> --- a/drivers/macintosh/therm_windtunnel.c
>> +++ b/drivers/macintosh/therm_windtunnel.c
>> @@ -44,7 +44,11 @@
>> #include <asm/sections.h>
>> #include <asm/macio.h>
>>
>> -#define LOG_TEMP 0 /* continuously log temperature */
>> +static int verbose = 1; /* see description below */
>
> This comment seems useless.
ok
>
>> +module_param(verbose, int, 0644);
>> +MODULE_PARM_DESC(verbose, "Vebosity level: 0=silent, "
>
> Typo: Verbosity
Ok
>
>> + "1=log the fan tuning, "
>> + "2=log the temperature.");
>
> Trailing dot is not needed.
OK
>>
>> static struct {
[... cut cut cut ...]
>> + /*
>> + * if verbose >0 log each fan tuning
>> + * if verbose >1 log each cpu temperature change
>> + */
>
> I think it is a good idea to have all the printing in a single place.
>
>> + if ((verbose > 1 && x.temp != temp ) ||
>
> No space before closing parenthesis. I know the original code did not
> follow the standard coding style but you have the opportunity to make
> it better. Same many times below. scripts/checkpatch.pl tells you about
> that and many other style issues, most of which are easily fixable.
>
> Also don't you want to log changes of case temperature too?
yes it make sense.
>
>> + (verbose > 0 && level >= 0)) {
>> + print_temp("CPU-temp: ", temp );
>> + if (casetemp)
>> + print_temp(", Case: ", casetemp );
>> + if (level >= 0)
>> + printk(", Fan: %d (tuned %+d)\n", 11-level,
>> + x.fan_level-level );
>> + else
>> + printk(", Fan: %d (tuned +0)\n",x.fan_level);
>
> I think you can do without the "tuned +0" which doesn't add much value.
Me too. But the old driver does the same, so I preferred to
leave it as is.
>
>> + }
>> +
>> + x.temp = temp;
>> + x.casetemp = casetemp;
>> +
>> if( level >= 0 )
>> tune_fan( level );
>> }
>
>
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] Add the "verbose" module option.
2014-08-03 15:12 ` Goffredo Baroncelli
@ 2014-08-03 15:52 ` Jean Delvare
2014-08-03 16:36 ` Goffredo Baroncelli
0 siblings, 1 reply; 17+ messages in thread
From: Jean Delvare @ 2014-08-03 15:52 UTC (permalink / raw)
To: kreijack; +Cc: kreijack, Benjamin Herrenschmidt, linux-kernel, bryan
On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
> On 08/03/2014 04:12 PM, Jean Delvare wrote:
> >> + (verbose > 0 && level >= 0)) {
> >> + print_temp("CPU-temp: ", temp );
> >> + if (casetemp)
> >> + print_temp(", Case: ", casetemp );
> >> + if (level >= 0)
> >> + printk(", Fan: %d (tuned %+d)\n", 11-level,
> >> + x.fan_level-level );
> >> + else
> >> + printk(", Fan: %d (tuned +0)\n",x.fan_level);
> >
> > I think you can do without the "tuned +0" which doesn't add much value.
>
> Me too. But the old driver does the same, so I preferred to
> leave it as is.
I looked at the code again and no, I can't see the old code doing that.
It has "tuned %+d" only in tune_fan() which is only called if
level >= 0. The other printk (when tune_fan isn't called) doesn't have
a "tuned" part.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] Add the "verbose" module option.
2014-08-03 15:52 ` Jean Delvare
@ 2014-08-03 16:36 ` Goffredo Baroncelli
2014-08-04 8:46 ` Jean Delvare
0 siblings, 1 reply; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-03 16:36 UTC (permalink / raw)
To: Jean Delvare, kreijack; +Cc: Benjamin Herrenschmidt, linux-kernel, bryan
On 08/03/2014 05:52 PM, Jean Delvare wrote:
> On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
>> On 08/03/2014 04:12 PM, Jean Delvare wrote:
>>>> + (verbose > 0 && level >= 0)) {
>>>> + print_temp("CPU-temp: ", temp );
>>>> + if (casetemp)
>>>> + print_temp(", Case: ", casetemp );
>>>> + if (level >= 0)
>>>> + printk(", Fan: %d (tuned %+d)\n", 11-level,
>>>> + x.fan_level-level );
>>>> + else
>>>> + printk(", Fan: %d (tuned +0)\n",x.fan_level);
>>>
>>> I think you can do without the "tuned +0" which doesn't add much value.
>>
>> Me too. But the old driver does the same, so I preferred to
>> leave it as is.
>
> I looked at the code again and no, I can't see the old code doing that.
> It has "tuned %+d" only in tune_fan() which is only called if
> level >= 0. The other printk (when tune_fan isn't called) doesn't have
> a "tuned" part.
>
This is taken from an old log of a v3.2 kernel (no changes here):
[ 886.510879] CPU-temp: 55.4 C, Case: 33.1 C, Fan: 0 (tuned -11)
[ 910.522869] CPU-temp: 56.0 C, Case: 33.5 C, Fan: 0 (tuned +0)
[ 958.546880] CPU-temp: 57.0 C, Case: 34.1 C, Fan: 3 (tuned +3)
in the code if level <0, then there is no update in the log. But if
level >0 and level is equal to the previous one, this leads to
have "tuned +0"...
But I have to be honest: I have not fully understand how
"level" is computed.
The printk without "(tuned %+d)" is never called because
LOG_TEMP was #define(d) equal to 0.
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] Add the "verbose" module option.
2014-08-03 16:36 ` Goffredo Baroncelli
@ 2014-08-04 8:46 ` Jean Delvare
2014-08-04 17:10 ` Goffredo Baroncelli
0 siblings, 1 reply; 17+ messages in thread
From: Jean Delvare @ 2014-08-04 8:46 UTC (permalink / raw)
To: kreijack; +Cc: Benjamin Herrenschmidt, linux-kernel, bryan
Le Sunday 03 August 2014 à 18:36 +0200, Goffredo Baroncelli a écrit :
> On 08/03/2014 05:52 PM, Jean Delvare wrote:
> > On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
> >> On 08/03/2014 04:12 PM, Jean Delvare wrote:
> >>>> + (verbose > 0 && level >= 0)) {
> >>>> + print_temp("CPU-temp: ", temp );
> >>>> + if (casetemp)
> >>>> + print_temp(", Case: ", casetemp );
> >>>> + if (level >= 0)
> >>>> + printk(", Fan: %d (tuned %+d)\n", 11-level,
> >>>> + x.fan_level-level );
> >>>> + else
> >>>> + printk(", Fan: %d (tuned +0)\n",x.fan_level);
> >>>
> >>> I think you can do without the "tuned +0" which doesn't add much value.
> >>
> >> Me too. But the old driver does the same, so I preferred to
> >> leave it as is.
> >
> > I looked at the code again and no, I can't see the old code doing that.
> > It has "tuned %+d" only in tune_fan() which is only called if
> > level >= 0. The other printk (when tune_fan isn't called) doesn't have
> > a "tuned" part.
>
> This is taken from an old log of a v3.2 kernel (no changes here):
>
> [ 886.510879] CPU-temp: 55.4 C, Case: 33.1 C, Fan: 0 (tuned -11)
> [ 910.522869] CPU-temp: 56.0 C, Case: 33.5 C, Fan: 0 (tuned +0)
> [ 958.546880] CPU-temp: 57.0 C, Case: 34.1 C, Fan: 3 (tuned +3)
>
> in the code if level <0, then there is no update in the log. But if
> level >0 and level is equal to the previous one, this leads to
> have "tuned +0"...
I agree with that.
> But I have to be honest: I have not fully understand how
> "level" is computed.
I agree with that too :/
> The printk without "(tuned %+d)" is never called because
> LOG_TEMP was #define(d) equal to 0.
And this is what your second printk is replacing. So it should not have
the "(tuned *)" either.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] Add the "verbose" module option.
2014-08-04 8:46 ` Jean Delvare
@ 2014-08-04 17:10 ` Goffredo Baroncelli
2014-08-05 8:59 ` Jean Delvare
0 siblings, 1 reply; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-04 17:10 UTC (permalink / raw)
To: Jean Delvare, Benjamin Herrenschmidt; +Cc: linux-kernel, bryan
On 08/04/2014 10:46 AM, Jean Delvare wrote:
> Le Sunday 03 August 2014 à 18:36 +0200, Goffredo Baroncelli a écrit :
>> On 08/03/2014 05:52 PM, Jean Delvare wrote:
>>> On Sun, 03 Aug 2014 17:12:57 +0200, Goffredo Baroncelli wrote:
>>>> On 08/03/2014 04:12 PM, Jean Delvare wrote:
>>>>>> + (verbose > 0 && level >= 0)) {
>>>>>> + print_temp("CPU-temp: ", temp );
>>>>>> + if (casetemp)
>>>>>> + print_temp(", Case: ", casetemp );
>>>>>> + if (level >= 0)
>>>>>> + printk(", Fan: %d (tuned %+d)\n", 11-level,
>>>>>> + x.fan_level-level );
>>>>>> + else
>>>>>> + printk(", Fan: %d (tuned +0)\n",x.fan_level);
>>>>>
>>>>> I think you can do without the "tuned +0" which doesn't add much value.
>>>>
>>>> Me too. But the old driver does the same, so I preferred to
>>>> leave it as is.
>>>
>>> I looked at the code again and no, I can't see the old code doing that.
>>> It has "tuned %+d" only in tune_fan() which is only called if
>>> level >= 0. The other printk (when tune_fan isn't called) doesn't have
>>> a "tuned" part.
>>
>> This is taken from an old log of a v3.2 kernel (no changes here):
>>
>> [ 886.510879] CPU-temp: 55.4 C, Case: 33.1 C, Fan: 0 (tuned -11)
>> [ 910.522869] CPU-temp: 56.0 C, Case: 33.5 C, Fan: 0 (tuned +0)
>> [ 958.546880] CPU-temp: 57.0 C, Case: 34.1 C, Fan: 3 (tuned +3)
>>
>> in the code if level <0, then there is no update in the log. But if
>> level >0 and level is equal to the previous one, this leads to
>> have "tuned +0"...
>
> I agree with that.
>
>> But I have to be honest: I have not fully understand how
>> "level" is computed.
>
> I agree with that too :/
>
>> The printk without "(tuned %+d)" is never called because
>> LOG_TEMP was #define(d) equal to 0.
>
> And this is what your second printk is replacing. So it should not have
> the "(tuned *)" either.
>
I removed the printk(s) from tune_fan(); the ones leaved replaced
both the ones inside tune_fan() and the ones outside.
Anyway, Benjamin which is your opinion ?
For me is equal to remove or to leave "(tune +0)" (when the tuning is equal to 0).
Jean think it is better to remove "(tune +0)" (when the tuning is equal to 0).
So if you haven't any objection I will remove it.
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] Add the "verbose" module option.
2014-08-04 17:10 ` Goffredo Baroncelli
@ 2014-08-05 8:59 ` Jean Delvare
0 siblings, 0 replies; 17+ messages in thread
From: Jean Delvare @ 2014-08-05 8:59 UTC (permalink / raw)
To: kreijack; +Cc: Benjamin Herrenschmidt, linux-kernel, bryan
Le Monday 04 August 2014 à 19:10 +0200, Goffredo Baroncelli a écrit :
> On 08/04/2014 10:46 AM, Jean Delvare wrote:
> > Le Sunday 03 August 2014 à 18:36 +0200, Goffredo Baroncelli a écrit :
> >> The printk without "(tuned %+d)" is never called because
> >> LOG_TEMP was #define(d) equal to 0.
> >
> > And this is what your second printk is replacing. So it should not have
> > the "(tuned *)" either.
> >
> I removed the printk(s) from tune_fan(); the ones leaved replaced
> both the ones inside tune_fan() and the ones outside.
I understand that. But you still had two final printks, one with "(tuned
%+d)" when level >= 0, which corresponds to what was printed in tune_fan
before, and another one when level < 0, which corresponds to what was
printed in poll_temp before, and that one did not have a "tuned +0" part
so I simply fail to see why its replacement should have it.
I admit I'm surprised we're arguing on that as it seems really obvious
to me, so I can only hope I'm not missing something even more obvious.
> Anyway, Benjamin which is your opinion ?
> For me is equal to remove or to leave "(tune +0)" (when the tuning is equal to 0).
> Jean think it is better to remove "(tune +0)" (when the tuning is equal to 0).
> So if you haven't any objection I will remove it.
s/remove/not introduce/ is my actual point.
But I'm not going to argue more, I'm not even using that driver and it's
a debug message only anyway, so do as you wish.
Thanks,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/4] Return the fan speed via sysfs
2014-08-01 14:00 [PATCH][v2] therm_windtunnel doesn't work properly on PowerMac G4 Goffredo Baroncelli
` (2 preceding siblings ...)
2014-08-01 14:00 ` [PATCH 3/4] Add the "verbose" module option Goffredo Baroncelli
@ 2014-08-01 14:00 ` Goffredo Baroncelli
2014-08-03 14:17 ` Jean Delvare
3 siblings, 1 reply; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-01 14:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: ", ", ", Goffredo Baroncelli
Return the fan speed via sysfs:
/sys/devices/temperature/fan_level
This patch is copied from the Bryan Christianson's patch (see
debian bug #741663)
Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
drivers/macintosh/therm_windtunnel.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index 0c4eb85..a2af7db 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -111,9 +111,15 @@ show_case_temperature( struct device *dev, struct device_attribute *attr, char *
return sprintf(buf, "%d.%d\n", x.casetemp>>8, (x.casetemp & 255)*10/256 );
}
+static ssize_t
+show_fan_level( struct device *dev, struct device_attribute *attr, char *buf )
+{
+ return sprintf(buf, "%d\n", 11 - x.fan_level );
+}
+
static DEVICE_ATTR(cpu_temperature, S_IRUGO, show_cpu_temperature, NULL );
static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
-
+static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL );
/************************************************************************/
@@ -265,6 +271,7 @@ setup_hardware( void )
err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
+ err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
if (err)
printk(KERN_WARNING
"Failed to create temperature attribute file(s).\n");
@@ -275,6 +282,7 @@ restore_regs( void )
{
device_remove_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
device_remove_file( &x.of_dev->dev, &dev_attr_case_temperature );
+ device_remove_file( &x.of_dev->dev, &dev_attr_fan_level );
write_reg( x.fan, 0x01, x.r1, 1 );
write_reg( x.fan, 0x20, x.r20, 1 );
--
2.0.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/4] Return the fan speed via sysfs
2014-08-01 14:00 ` [PATCH 4/4] Return the fan speed via sysfs Goffredo Baroncelli
@ 2014-08-03 14:17 ` Jean Delvare
2014-08-03 15:27 ` Goffredo Baroncelli
0 siblings, 1 reply; 17+ messages in thread
From: Jean Delvare @ 2014-08-03 14:17 UTC (permalink / raw)
To: Goffredo Baroncelli
Cc: Benjamin Herrenschmidt, linux-kernel, bryan, Goffredo Baroncelli
On Fri, 1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
> Return the fan speed via sysfs:
> /sys/devices/temperature/fan_level
Good idea. Even better would be if the driver would expose a standard
hwmon interface for the temperature values. Fan level could go in
attribute "pwm1" after proper scaling.
Please follow scripts/checkpatch.pl's advice to fix the coding style.
> This patch is copied from the Bryan Christianson's patch (see
> debian bug #741663)
>
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
> drivers/macintosh/therm_windtunnel.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index 0c4eb85..a2af7db 100644
> --- a/drivers/macintosh/therm_windtunnel.c
> +++ b/drivers/macintosh/therm_windtunnel.c
> @@ -111,9 +111,15 @@ show_case_temperature( struct device *dev, struct device_attribute *attr, char *
> return sprintf(buf, "%d.%d\n", x.casetemp>>8, (x.casetemp & 255)*10/256 );
> }
>
> +static ssize_t
> +show_fan_level( struct device *dev, struct device_attribute *attr, char *buf )
> +{
> + return sprintf(buf, "%d\n", 11 - x.fan_level );
> +}
> +
> static DEVICE_ATTR(cpu_temperature, S_IRUGO, show_cpu_temperature, NULL );
> static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
> -
> +static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL );
>
>
> /************************************************************************/
> @@ -265,6 +271,7 @@ setup_hardware( void )
>
> err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
> err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
> + err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
> if (err)
> printk(KERN_WARNING
> "Failed to create temperature attribute file(s).\n");
That's not your fault but please note that this construct is broken.
You can't "or" error codes together, the result if two or more calls
fail with different error codes is pretty random. Instead, each error
must be tested individually. I know checkpatch.pl will complain if you
do that but you can ignore it as is it the right thing to do in that
case.
> @@ -275,6 +282,7 @@ restore_regs( void )
> {
> device_remove_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
> device_remove_file( &x.of_dev->dev, &dev_attr_case_temperature );
> + device_remove_file( &x.of_dev->dev, &dev_attr_fan_level );
>
> write_reg( x.fan, 0x01, x.r1, 1 );
> write_reg( x.fan, 0x20, x.r20, 1 );
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/4] Return the fan speed via sysfs
2014-08-03 14:17 ` Jean Delvare
@ 2014-08-03 15:27 ` Goffredo Baroncelli
2014-08-03 15:59 ` Jean Delvare
0 siblings, 1 reply; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-03 15:27 UTC (permalink / raw)
To: Jean Delvare, Goffredo Baroncelli
Cc: Benjamin Herrenschmidt, linux-kernel, bryan
On 08/03/2014 04:17 PM, Jean Delvare wrote:
> On Fri, 1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
>> Return the fan speed via sysfs:
>> /sys/devices/temperature/fan_level
>
> Good idea. Even better would be if the driver would expose a standard
> hwmon interface for the temperature values. Fan level could go in
> attribute "pwm1" after proper scaling.
I tought the same. But until now the value logged is an integer value
between 1 and 11. So I preferred to leave it as is.
The thing that I can do is to *add* a further attribute called pwm1.
( I have to check how adm1031.c computes its pwm), because is a
more standard interface.
I even thought to allow to change the fan speed from user space....
>
> Please follow scripts/checkpatch.pl's advice to fix the coding style.
>
>> This patch is copied from the Bryan Christianson's patch (see
>> debian bug #741663)
>>
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>> drivers/macintosh/therm_windtunnel.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
>> index 0c4eb85..a2af7db 100644
>> --- a/drivers/macintosh/therm_windtunnel.c
>> +++ b/drivers/macintosh/therm_windtunnel.c
>> @@ -111,9 +111,15 @@ show_case_temperature( struct device *dev, struct device_attribute *attr, char *
>> return sprintf(buf, "%d.%d\n", x.casetemp>>8, (x.casetemp & 255)*10/256 );
>> }
>>
>> +static ssize_t
>> +show_fan_level( struct device *dev, struct device_attribute *attr, char *buf )
>> +{
>> + return sprintf(buf, "%d\n", 11 - x.fan_level );
>> +}
>> +
>> static DEVICE_ATTR(cpu_temperature, S_IRUGO, show_cpu_temperature, NULL );
>> static DEVICE_ATTR(case_temperature, S_IRUGO, show_case_temperature, NULL );
>> -
>> +static DEVICE_ATTR(fan_level, S_IRUGO, show_fan_level, NULL );
>>
>>
>> /************************************************************************/
>> @@ -265,6 +271,7 @@ setup_hardware( void )
>>
>> err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>> err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
>> + err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
>> if (err)
>> printk(KERN_WARNING
>> "Failed to create temperature attribute file(s).\n");
>
> That's not your fault but please note that this construct is broken.
> You can't "or" error codes together, the result if two or more calls
> fail with different error codes is pretty random. Instead, each error
> must be tested individually. I know checkpatch.pl will complain if you
> do that but you can ignore it as is it the right thing to do in that
> case.
The really question is what we should do if the 2nd device_create_file()
would fail: we should fails the driver initialization ? or we should
continue, because even if there aren't some sysfs attributes the driver
work enough ?
>
>> @@ -275,6 +282,7 @@ restore_regs( void )
>> {
>> device_remove_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>> device_remove_file( &x.of_dev->dev, &dev_attr_case_temperature );
>> + device_remove_file( &x.of_dev->dev, &dev_attr_fan_level );
>>
>> write_reg( x.fan, 0x01, x.r1, 1 );
>> write_reg( x.fan, 0x20, x.r20, 1 );
>
>
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/4] Return the fan speed via sysfs
2014-08-03 15:27 ` Goffredo Baroncelli
@ 2014-08-03 15:59 ` Jean Delvare
2014-08-03 16:42 ` Goffredo Baroncelli
0 siblings, 1 reply; 17+ messages in thread
From: Jean Delvare @ 2014-08-03 15:59 UTC (permalink / raw)
To: kreijack; +Cc: Goffredo Baroncelli, Benjamin Herrenschmidt, linux-kernel, bryan
On Sun, 03 Aug 2014 17:27:17 +0200, Goffredo Baroncelli wrote:
> On 08/03/2014 04:17 PM, Jean Delvare wrote:
> > On Fri, 1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
> >> Return the fan speed via sysfs:
> >> /sys/devices/temperature/fan_level
> >
> > Good idea. Even better would be if the driver would expose a standard
> > hwmon interface for the temperature values. Fan level could go in
> > attribute "pwm1" after proper scaling.
>
> I tought the same. But until now the value logged is an integer value
> between 1 and 11. So I preferred to leave it as is.
>
> The thing that I can do is to *add* a further attribute called pwm1.
> ( I have to check how adm1031.c computes its pwm), because is a
> more standard interface.
The temperature attributes in hwmon would need different names and
units too (temp1_input and temp2_input, in millidegree C.) The
advantage is that all monitoring applications out there would pick up
these values automatically.
> I even thought to allow to change the fan speed from user space....
Ben will never let you do that ;-)
> >> @@ -265,6 +271,7 @@ setup_hardware( void )
> >>
> >> err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
> >> err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
> >> + err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
> >> if (err)
> >> printk(KERN_WARNING
> >> "Failed to create temperature attribute file(s).\n");
> >
> > That's not your fault but please note that this construct is broken.
> > You can't "or" error codes together, the result if two or more calls
> > fail with different error codes is pretty random. Instead, each error
> > must be tested individually. I know checkpatch.pl will complain if you
> > do that but you can ignore it as is it the right thing to do in that
> > case.
>
> The really question is what we should do if the 2nd device_create_file()
> would fail: we should fails the driver initialization ? or we should
> continue, because even if there aren't some sysfs attributes the driver
> work enough ?
I would log a warning and continue because it's a secondary feature of
the driver. Exactly as the driver already does - no change here.
In practice it's never going to happen so it doesn't really matter. I
just wanted to point out that the construct used in the driver was
dangerous. In this specific case it's harmless because the value of
"err" is never used (other than the fact that it's non-zero.) But if
the error code was included in the log message for example (which is
recommended), it would possibly make no sense.
Feel free to ignore this problem in this patch, it's not so important.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] Return the fan speed via sysfs
2014-08-03 15:59 ` Jean Delvare
@ 2014-08-03 16:42 ` Goffredo Baroncelli
2014-08-04 8:44 ` Jean Delvare
0 siblings, 1 reply; 17+ messages in thread
From: Goffredo Baroncelli @ 2014-08-03 16:42 UTC (permalink / raw)
To: Jean Delvare, kreijack; +Cc: Benjamin Herrenschmidt, linux-kernel, bryan
On 08/03/2014 05:59 PM, Jean Delvare wrote:
> On Sun, 03 Aug 2014 17:27:17 +0200, Goffredo Baroncelli wrote:
>> On 08/03/2014 04:17 PM, Jean Delvare wrote:
>>> On Fri, 1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
>>>> Return the fan speed via sysfs:
>>>> /sys/devices/temperature/fan_level
>>>
>>> Good idea. Even better would be if the driver would expose a standard
>>> hwmon interface for the temperature values. Fan level could go in
>>> attribute "pwm1" after proper scaling.
>>
>> I tought the same. But until now the value logged is an integer value
>> between 1 and 11. So I preferred to leave it as is.
>>
>> The thing that I can do is to *add* a further attribute called pwm1.
>> ( I have to check how adm1031.c computes its pwm), because is a
>> more standard interface.
>
> The temperature attributes in hwmon would need different names and
> units too (temp1_input and temp2_input, in millidegree C.) The
> advantage is that all monitoring applications out there would pick up
> these values automatically.
Are you suggesting to add also a "temp1_input" attribute ?
>
>> I even thought to allow to change the fan speed from user space....
>
> Ben will never let you do that ;-)
What would be the risk ?. When the CPU temperature goes behind the limit,
then the computer is switched off by an hardware protection (I am sure because
I had to changed a cpu board because a drift of the temperature sensor).
I am not suggesting to allow to change the fan speed, I am only asking which would
be the risk.
>
>>>> @@ -265,6 +271,7 @@ setup_hardware( void )
>>>>
>>>> err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>>>> err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
>>>> + err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
>>>> if (err)
>>>> printk(KERN_WARNING
>>>> "Failed to create temperature attribute file(s).\n");
>>>
>>> That's not your fault but please note that this construct is broken.
>>> You can't "or" error codes together, the result if two or more calls
>>> fail with different error codes is pretty random. Instead, each error
>>> must be tested individually. I know checkpatch.pl will complain if you
>>> do that but you can ignore it as is it the right thing to do in that
>>> case.
>>
>> The really question is what we should do if the 2nd device_create_file()
>> would fail: we should fails the driver initialization ? or we should
>> continue, because even if there aren't some sysfs attributes the driver
>> work enough ?
>
> I would log a warning and continue because it's a secondary feature of
> the driver. Exactly as the driver already does - no change here.
>
> In practice it's never going to happen so it doesn't really matter. I
> just wanted to point out that the construct used in the driver was
> dangerous. In this specific case it's harmless because the value of
> "err" is never used (other than the fact that it's non-zero.) But if
> the error code was included in the log message for example (which is
> recommended), it would possibly make no sense.
>
> Feel free to ignore this problem in this patch, it's not so important.
>
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] Return the fan speed via sysfs
2014-08-03 16:42 ` Goffredo Baroncelli
@ 2014-08-04 8:44 ` Jean Delvare
0 siblings, 0 replies; 17+ messages in thread
From: Jean Delvare @ 2014-08-04 8:44 UTC (permalink / raw)
To: kreijack; +Cc: Benjamin Herrenschmidt, linux-kernel, bryan
Le Sunday 03 August 2014 à 18:42 +0200, Goffredo Baroncelli a écrit :
> On 08/03/2014 05:59 PM, Jean Delvare wrote:
> > The temperature attributes in hwmon would need different names and
> > units too (temp1_input and temp2_input, in millidegree C.) The
> > advantage is that all monitoring applications out there would pick up
> > these values automatically.
>
> Are you suggesting to add also a "temp1_input" attribute ?
Yes, but as a hwmon class device attribute, not as an attribute
in /sys/devices/temperature.
> >> I even thought to allow to change the fan speed from user space....
> >
> > Ben will never let you do that ;-)
>
> What would be the risk ?. When the CPU temperature goes behind the limit,
> then the computer is switched off by an hardware protection (I am sure because
> I had to changed a cpu board because a drift of the temperature sensor).
And you are still asking what the risk is? ;-)
> I am not suggesting to allow to change the fan speed, I am only asking which would
> be the risk.
If the user forces the fan too low, the system can overheat, and that
can result in user injury or hardware damage (or reduced lifetime of
hardware, at least.)
Some people think that the user should always be given full power over
his/her hardware. Others think that hardware should just work and users
should never have to care about such low-level details.
In this specific case I'd say don't change it unless you have a
compelling reason to.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 17+ messages in thread