Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)

[Nicolas Boichat <nicolas@boichat.ch>]

Hi,

Thanks for your comments.

Jean Delvare wrote:
> Hi Nicolas,
> 
> Sorry for the delay.
> 
> On Wed, 14 Mar 2007 17:29:39 +0800, Nicolas Boichat wrote:
>> I developed, a while ago, a driver the Apple System Management
>> Controller, which provides an accelerometer (Apple Sudden Motion
>> Sensor), light sensors, temperature sensors, keyboard backlight control
>> and fan control on Intel-based Apple's computers (MacBook Pro, MacBook,
>> MacMini).
>>
>> This patch has been tested successfully since kernel 2.6.18 (i.e. 3-4
>> months ago) by various users on different systems on the mactel-linux lists.
>>
>> However, I'm not really satisfied with the way sysfs files are created:
>> I use a lot of preprocessor macros to avoid repetition of code.
>> The files created with these macros in /sys/devices/platform/applesmc are
>> the following (on a Macbook Pro):
>> fan0_actual_speed
>> fan0_manual
>> fan0_maximum_speed
>> fan0_minimum_speed
>> fan0_safe_speed
>> fan0_target_speed
>> fan1_actual_speed
>> fan1_manual
>> fan1_maximum_speed
>> fan1_minimum_speed
>> fan1_safe_speed
>> fan1_target_speed
>> temperature_0
>> temperature_1
>> temperature_2
>> temperature_3
>> temperature_4
>> temperature_5
>> temperature_6
> 
> First of all, please read Documentation/hwmon/sysfs-documentation, and
> rename the entries to match the standard names whenever possible. Also
> make sure that you use the standard units. If you use the standard
> names and units and if you register your device with the hwmon class,
> standard monitoring application will be able to support your driver.

Ok I'll have a look at this.

>> (i.e. temperature_* is created by one macro, fan*_actual_speed by
>> another, ...)
>> Is it acceptable programming practice? Is there a way to create these
>> files in a more elegant manner?
> 
> Some old hardware monitoring drivers are still doing this, but this is
> strongly discouraged for new drivers. It is possible (and easy) to
> avoid using such macros, by sharing callback functions between various
> sysfs files.
> 
> This is done by using SENSOR_DEVICE_ATTR instead of DEVICE_ATTR to
> declare the sysfs attributes. It takes an extra parameter, which is the
> entry number/index. You retrieve that index in the callback function:
> 
> static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
> 			 char *buf)
> {
> 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> 	int nr = attr->index;
> 	(...)
> }
> 
> Take a look at the hwmon/f71805f.c driver for examples.

Yes I'm using something like this in the version that is in the -mm tree
now.

>> Also, I never call any sysfs_remove_* function, as the files are
>> deleted when the module is unloaded. Is it safe to do so? Doesn't it
>> cause any memory leak?
> 
> This is considered a bad practice, as in theory you driver shouldn't
> create the device by itself, and the files are associated to the device,
> not the driver. All hardware monitoring drivers have been fixed now, so
> please add the file removal calls in your driver too. You might find it
> easier to use file groups rather than individual files. Again, see for
> example the f71805f driver, and in particular the f71805f_attributes
> array and f71805f_group structure, and the sysfs_create_group() and
> sysfs_remove_group() calls.

Ok I'll fix this.

> I'm sorry but I really don't have the time for a complete review of
> your driver.

Your comments are already very valuable, thanks.

Best regards,

Nicolas