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

[Jean Delvare <jdelvare@suse.de>]

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