[lkml]   [2007]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)

    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

    >> 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,

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2007-04-11 14:53    [W:0.032 / U:40.028 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site