LKML Archive on
help / color / mirror / Atom feed
From: Jean Delvare <>
To: Nicolas Boichat <>
Subject: Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Date: Wed, 11 Apr 2007 14:25:56 +0200	[thread overview]
Message-ID: <20070411142556.16cd1e44@hyperion.delvare> (raw)
In-Reply-To: <>

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.

> (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.

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

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

Jean Delvare

  parent reply	other threads:[~2007-04-11 12:27 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-03-14  9:29 Nicolas Boichat
2007-03-14 11:11 ` Cong WANG
2007-03-14 14:00   ` Cong WANG
2007-03-15 11:31     ` Nicolas Boichat
2007-03-19  5:19 ` [PATCH] " Nicolas Boichat
2007-03-19  6:54   ` Andrew Morton
2007-03-19  7:35     ` Nicolas Boichat
2007-03-20  7:12     ` Nicolas Boichat
2007-03-22 15:37   ` Dmitry Torokhov
2007-04-09 13:53     ` [PATCH] Apple SMC driver - fix input device Nicolas Boichat
2007-04-09 15:17       ` Dmitry Torokhov
2007-04-09 20:04       ` Andrew Morton
2007-04-09 20:11         ` Dmitry Torokhov
2007-04-09 21:51         ` Paul Mackerras
2007-03-19 21:43 ` [RFC][PATCH] Apple SMC driver (hardware monitoring and control) Bob Copeland
2007-03-20  7:02   ` Nicolas Boichat
2007-03-20 15:14     ` Bob Copeland
2007-03-21  4:03     ` Bob Copeland
     [not found]     ` <>
2007-04-14  8:05       ` [PATCH] applesmc - fix crash when activating a led trigger on the keyboard backlight Nicolas Boichat
2007-04-14  8:45         ` Richard Purdie
2007-04-14 13:31           ` [PATCH] applesmc - fix crash when activating a led trigger on the keyboard backlight - use a workqueue Nicolas Boichat
2007-03-20 10:08   ` [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control) Jean Delvare
2007-03-22 10:36     ` Nicolas Boichat
2007-03-20 16:12 ` Gerb Stralko
2007-04-11 12:25 ` Jean Delvare [this message]
2007-04-11 12:47   ` [lm-sensors] " Nicolas Boichat
2007-04-13  5:33   ` [PATCH 1/2] Apple SMC driver - standardize and sanitize sysfs tree + minor features addition Nicolas Boichat
2007-04-13  6:38     ` [PATCH 2/2] Apple SMC driver - implement key enumeration Nicolas Boichat

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070411142556.16cd1e44@hyperion.delvare \ \ \ \ \ \ \
    --subject='Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).