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

[Goffredo Baroncelli <kreijack@inwind.it>]

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