Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Jean Delvare wrote:
> On Mon, 19 Mar 2007 17:43:38 -0400, Bob Copeland wrote:
>
>> I tried out an earlier version of this patch several months ago just to play
>> around with the joystick part of the accelerometer driver on my MacBook, and
>> found that it was backwards in the y-direction compared to what Neverball
>> seemed to want (of course, NB has no way to invert the joystick). I think
>> I just did something like this in my own copy:
>>
>> + y = -y;
>> input_report_abs(applesmc_idev, ABS_X, x - rest_x);
>> input_report_abs(applesmc_idev, ABS_Y, y - rest_y);
>>
>> I don't claim you necessarily want to change it, but thought I'd pass it
>> along.
>>
>
> This appears to be a common problem with these devices, the hdaps driver
> (IBM) needs to invert the axis on some models too, and I seem to
> remember something similar for the (not yet merged) HP laptops
> accelerometer driver.
>
Ok, so let's invert the axis on the input device.
I think the raw x value in sysfs behaves the same way as hdaps (because
tools like hdaps-gl don't need any inversion), so I don't invert it.
Anyone with an IBM laptop could confirm this please?
- x value gets more positive when you lift the right side on the laptop
(= tilted to the left)
- y value gets more negative when you tilt the laptop backwards
(simply cat /sys/devices/platform/hdaps/position)
Thanks,
Best regards,
Nicolas
Invert x axis on applesmc input device to make it usable as a joystick.
Signed-off-by: Nicolas Boichat <[EMAIL PROTECTED]>
---
drivers/hwmon/applesmc.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 4060667..581ed3e 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -349,6 +349,7 @@ static void applesmc_calibrate(void)
{
applesmc_read_motion_sensor(SENSOR_X, _x);
applesmc_read_motion_sensor(SENSOR_Y, _y);
+ rest_x = -rest_x;
}
static void applesmc_mousedev_poll(unsigned long unused)
@@ -366,6 +367,7 @@ static void applesmc_mousedev_poll(unsigned long unused)
if (applesmc_read_motion_sensor(SENSOR_Y, ))
goto out;
+ x = -x;
input_report_abs(applesmc_idev, ABS_X, x - rest_x);
input_report_abs(applesmc_idev, ABS_Y, y - rest_y);
input_sync(applesmc_idev);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Jean Delvare wrote:
On Mon, 19 Mar 2007 17:43:38 -0400, Bob Copeland wrote:
I tried out an earlier version of this patch several months ago just to play
around with the joystick part of the accelerometer driver on my MacBook, and
found that it was backwards in the y-direction compared to what Neverball
seemed to want (of course, NB has no way to invert the joystick). I think
I just did something like this in my own copy:
+ y = -y;
input_report_abs(applesmc_idev, ABS_X, x - rest_x);
input_report_abs(applesmc_idev, ABS_Y, y - rest_y);
I don't claim you necessarily want to change it, but thought I'd pass it
along.
This appears to be a common problem with these devices, the hdaps driver
(IBM) needs to invert the axis on some models too, and I seem to
remember something similar for the (not yet merged) HP laptops
accelerometer driver.
Ok, so let's invert the axis on the input device.
I think the raw x value in sysfs behaves the same way as hdaps (because
tools like hdaps-gl don't need any inversion), so I don't invert it.
Anyone with an IBM laptop could confirm this please?
- x value gets more positive when you lift the right side on the laptop
(= tilted to the left)
- y value gets more negative when you tilt the laptop backwards
(simply cat /sys/devices/platform/hdaps/position)
Thanks,
Best regards,
Nicolas
Invert x axis on applesmc input device to make it usable as a joystick.
Signed-off-by: Nicolas Boichat [EMAIL PROTECTED]
---
drivers/hwmon/applesmc.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 4060667..581ed3e 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -349,6 +349,7 @@ static void applesmc_calibrate(void)
{
applesmc_read_motion_sensor(SENSOR_X, rest_x);
applesmc_read_motion_sensor(SENSOR_Y, rest_y);
+ rest_x = -rest_x;
}
static void applesmc_mousedev_poll(unsigned long unused)
@@ -366,6 +367,7 @@ static void applesmc_mousedev_poll(unsigned long unused)
if (applesmc_read_motion_sensor(SENSOR_Y, y))
goto out;
+ x = -x;
input_report_abs(applesmc_idev, ABS_X, x - rest_x);
input_report_abs(applesmc_idev, ABS_Y, y - rest_y);
input_sync(applesmc_idev);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
On Tue, Mar 20, 2007 at 03:02:14PM +0800, Nicolas Boichat wrote: > I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 > Duo), and the x axis in inverted, not the y axis. > > Could you confirm which axis is inverted on your Macbook? > > Also, have you tried the modified hdaps-gl, available here: > http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ > ? Is it working correctly? Ok I tried it out again and it is in fact the x-axis that is inverted within neverball. The hdaps-gl works fine (Macbook Core Duo here). Thanks for the driver! -- Bob Copeland %% www.bobcopeland.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
+/* data port used by apple SMC */
+#define APPLESMC_DATA_PORT 0x300
+/* command/status port used by apple SMC */
+#define APPLESMC_CMD_PORT 0x304
+
+#define APPLESMC_NR_PORTS 5 /* 0x300-0x304 */
+
+#define APPLESMC_STATUS_MASK 0x0f
+#define APPLESMC_READ_CMD 0x10
+#define APPLESMC_WRITE_CMD 0x11
+
+#define LIGHT_SENSOR_LEFT_KEY "ALV0" //r-o length 6
+#define LIGHT_SENSOR_RIGHT_KEY "ALV1" //r-o length 6
+#define BACKLIGHT_KEY "LKSB" //w-o
+
+#define CLAMSHELL_KEY "MSLD" //r-o length 1 (unused)
+
+#define MOTION_SENSOR_X_KEY"MO_X" //r-o length 2
+#define MOTION_SENSOR_Y_KEY"MO_Y" //r-o length 2
+#define MOTION_SENSOR_Z_KEY"MO_Z" //r-o length 2
+#define MOTION_SENSOR_KEY "MOCN" //r/w length 2
+
+#define FANS_COUNT "FNum" //r-o length 1
+#define FANS_MANUAL"FS! " //r-w length 2
+#define FAN_ACTUAL_SPEED "F0Ac" //r-o length 2
+#define FAN_MIN_SPEED "F0Mn" //r-o length 2
+#define FAN_MAX_SPEED "F0Mx" //r-o length 2
+#define FAN_SAFE_SPEED "F0Sf" //r-o length 2
+#define FAN_TARGET_SPEED "F0Tg" //r-w length 2
+
+/* Temperature sensors keys. First set for Macbook(Pro), second for Macmini */
+static const char* temperature_sensors_sets[][8] = {
+ { "TB0T", "TC0D", "TC0P", "Th0H", "Ts0P", "Th1H", "Ts1P", NULL },
+ { "TC0D", "TC0P", NULL }
+};
+
+#define INIT_TIMEOUT_MSECS 5000/* wait up to 5s for device init ... */
+#define INIT_WAIT_MSECS50 /* ... in 50ms increments */
+
+#define APPLESMC_POLL_PERIOD (HZ/20) /* poll for input every 1/20s */
+#define APPLESMC_INPUT_FUZZ4 /* input event threshold */
+#define APPLESMC_INPUT_FLAT4
+
+#define SENSOR_X 0
+#define SENSOR_Y 1
+#define SENSOR_Z 2
+
+/* Structure to be passed to DMI_MATCH function */
+struct dmi_match_data {
+/* Indicates whether this computer has an accelerometer. */
+ int accelerometer;
+/* Indicates whether this computer has light sensors and keyboard backlight. */
+ int light;
+/* Indicates which temperature sensors set to use. */
+ int temperature_set;
+};
+
+static int debug = 0;
+static struct platform_device *pdev;
+static s16 rest_x;
+static s16 rest_y;
+static struct timer_list applesmc_timer;
+static struct input_dev *applesmc_idev;
+
+/* Indicates whether this computer has an accelerometer. */
+static unsigned int applesmc_accelerometer = 0;
+
+/* Indicates whether this computer has light sensors and keyboard backlight. */
+static unsigned int applesmc_light = 0;
+
+/* Indicates which temperature sensors set to use. */
+static unsigned int applesmc_temperature_set = 0;
+
Is it possible to put some of this in a header file? I think it may
make the driver look alittle nicer and IMO easier to read. If this is
a problem or not just plain _stupid_ then just ignore me. I'm not
trying to be nit picky, i just think this is a *great* driver and I'm
excited to use it, but the cleaner it is the more manageable it will
be in the future.
-Jerry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 Duo), and the x axis in inverted, not the y axis. Could you confirm which axis is inverted on your Macbook? Indeed, my memory is hazy and it may well have been the x-axis. I can't find my modified copy. I'll check it out again tonight along with your hdaps-gl and let you know how it goes... -Bob - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
On Mon, 19 Mar 2007 17:43:38 -0400, Bob Copeland wrote: > I tried out an earlier version of this patch several months ago just to play > around with the joystick part of the accelerometer driver on my MacBook, and > found that it was backwards in the y-direction compared to what Neverball > seemed to want (of course, NB has no way to invert the joystick). I think > I just did something like this in my own copy: > > + y = -y; > input_report_abs(applesmc_idev, ABS_X, x - rest_x); > input_report_abs(applesmc_idev, ABS_Y, y - rest_y); > > I don't claim you necessarily want to change it, but thought I'd pass it > along. This appears to be a common problem with these devices, the hdaps driver (IBM) needs to invert the axis on some models too, and I seem to remember something similar for the (not yet merged) HP laptops accelerometer driver. -- Jean Delvare - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Hello, Bob Copeland wrote: > On 3/14/07, Nicolas Boichat <[EMAIL PROTECTED]> wrote: >> Hello, >> >> 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). > > Hi Nicolas, > > I tried out an earlier version of this patch several months ago just to > play > around with the joystick part of the accelerometer driver on my MacBook, > and > found that it was backwards in the y-direction compared to what Neverball > seemed to want (of course, NB has no way to invert the joystick). I think > I just did something like this in my own copy: > > + y = -y; >input_report_abs(applesmc_idev, ABS_X, x - rest_x); >input_report_abs(applesmc_idev, ABS_Y, y - rest_y); > > I don't claim you necessarily want to change it, but thought I'd pass it > along. I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 Duo), and the x axis in inverted, not the y axis. Could you confirm which axis is inverted on your Macbook? Also, have you tried the modified hdaps-gl, available here: http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ ? Is it working correctly? Thanks, Best regards, Nicolas - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
On Mon, 19 Mar 2007 17:43:38 -0400, Bob Copeland wrote: I tried out an earlier version of this patch several months ago just to play around with the joystick part of the accelerometer driver on my MacBook, and found that it was backwards in the y-direction compared to what Neverball seemed to want (of course, NB has no way to invert the joystick). I think I just did something like this in my own copy: + y = -y; input_report_abs(applesmc_idev, ABS_X, x - rest_x); input_report_abs(applesmc_idev, ABS_Y, y - rest_y); I don't claim you necessarily want to change it, but thought I'd pass it along. This appears to be a common problem with these devices, the hdaps driver (IBM) needs to invert the axis on some models too, and I seem to remember something similar for the (not yet merged) HP laptops accelerometer driver. -- Jean Delvare - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Hello, Bob Copeland wrote: On 3/14/07, Nicolas Boichat [EMAIL PROTECTED] wrote: Hello, 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). Hi Nicolas, I tried out an earlier version of this patch several months ago just to play around with the joystick part of the accelerometer driver on my MacBook, and found that it was backwards in the y-direction compared to what Neverball seemed to want (of course, NB has no way to invert the joystick). I think I just did something like this in my own copy: + y = -y; input_report_abs(applesmc_idev, ABS_X, x - rest_x); input_report_abs(applesmc_idev, ABS_Y, y - rest_y); I don't claim you necessarily want to change it, but thought I'd pass it along. I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 Duo), and the x axis in inverted, not the y axis. Could you confirm which axis is inverted on your Macbook? Also, have you tried the modified hdaps-gl, available here: http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ ? Is it working correctly? Thanks, Best regards, Nicolas - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
+/* data port used by apple SMC */
+#define APPLESMC_DATA_PORT 0x300
+/* command/status port used by apple SMC */
+#define APPLESMC_CMD_PORT 0x304
+
+#define APPLESMC_NR_PORTS 5 /* 0x300-0x304 */
+
+#define APPLESMC_STATUS_MASK 0x0f
+#define APPLESMC_READ_CMD 0x10
+#define APPLESMC_WRITE_CMD 0x11
+
+#define LIGHT_SENSOR_LEFT_KEY ALV0 //r-o length 6
+#define LIGHT_SENSOR_RIGHT_KEY ALV1 //r-o length 6
+#define BACKLIGHT_KEY LKSB //w-o
+
+#define CLAMSHELL_KEY MSLD //r-o length 1 (unused)
+
+#define MOTION_SENSOR_X_KEYMO_X //r-o length 2
+#define MOTION_SENSOR_Y_KEYMO_Y //r-o length 2
+#define MOTION_SENSOR_Z_KEYMO_Z //r-o length 2
+#define MOTION_SENSOR_KEY MOCN //r/w length 2
+
+#define FANS_COUNT FNum //r-o length 1
+#define FANS_MANUALFS! //r-w length 2
+#define FAN_ACTUAL_SPEED F0Ac //r-o length 2
+#define FAN_MIN_SPEED F0Mn //r-o length 2
+#define FAN_MAX_SPEED F0Mx //r-o length 2
+#define FAN_SAFE_SPEED F0Sf //r-o length 2
+#define FAN_TARGET_SPEED F0Tg //r-w length 2
+
+/* Temperature sensors keys. First set for Macbook(Pro), second for Macmini */
+static const char* temperature_sensors_sets[][8] = {
+ { TB0T, TC0D, TC0P, Th0H, Ts0P, Th1H, Ts1P, NULL },
+ { TC0D, TC0P, NULL }
+};
+
+#define INIT_TIMEOUT_MSECS 5000/* wait up to 5s for device init ... */
+#define INIT_WAIT_MSECS50 /* ... in 50ms increments */
+
+#define APPLESMC_POLL_PERIOD (HZ/20) /* poll for input every 1/20s */
+#define APPLESMC_INPUT_FUZZ4 /* input event threshold */
+#define APPLESMC_INPUT_FLAT4
+
+#define SENSOR_X 0
+#define SENSOR_Y 1
+#define SENSOR_Z 2
+
+/* Structure to be passed to DMI_MATCH function */
+struct dmi_match_data {
+/* Indicates whether this computer has an accelerometer. */
+ int accelerometer;
+/* Indicates whether this computer has light sensors and keyboard backlight. */
+ int light;
+/* Indicates which temperature sensors set to use. */
+ int temperature_set;
+};
+
+static int debug = 0;
+static struct platform_device *pdev;
+static s16 rest_x;
+static s16 rest_y;
+static struct timer_list applesmc_timer;
+static struct input_dev *applesmc_idev;
+
+/* Indicates whether this computer has an accelerometer. */
+static unsigned int applesmc_accelerometer = 0;
+
+/* Indicates whether this computer has light sensors and keyboard backlight. */
+static unsigned int applesmc_light = 0;
+
+/* Indicates which temperature sensors set to use. */
+static unsigned int applesmc_temperature_set = 0;
+
Is it possible to put some of this in a header file? I think it may
make the driver look alittle nicer and IMO easier to read. If this is
a problem or not just plain _stupid_ then just ignore me. I'm not
trying to be nit picky, i just think this is a *great* driver and I'm
excited to use it, but the cleaner it is the more manageable it will
be in the future.
-Jerry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 Duo), and the x axis in inverted, not the y axis. Could you confirm which axis is inverted on your Macbook? Indeed, my memory is hazy and it may well have been the x-axis. I can't find my modified copy. I'll check it out again tonight along with your hdaps-gl and let you know how it goes... -Bob - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
On Tue, Mar 20, 2007 at 03:02:14PM +0800, Nicolas Boichat wrote: I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 Duo), and the x axis in inverted, not the y axis. Could you confirm which axis is inverted on your Macbook? Also, have you tried the modified hdaps-gl, available here: http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ ? Is it working correctly? Ok I tried it out again and it is in fact the x-axis that is inverted within neverball. The hdaps-gl works fine (Macbook Core Duo here). Thanks for the driver! -- Bob Copeland %% www.bobcopeland.com - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
On 3/14/07, Nicolas Boichat <[EMAIL PROTECTED]> wrote: Hello, 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). Hi Nicolas, I tried out an earlier version of this patch several months ago just to play around with the joystick part of the accelerometer driver on my MacBook, and found that it was backwards in the y-direction compared to what Neverball seemed to want (of course, NB has no way to invert the joystick). I think I just did something like this in my own copy: + y = -y; input_report_abs(applesmc_idev, ABS_X, x - rest_x); input_report_abs(applesmc_idev, ABS_Y, y - rest_y); I don't claim you necessarily want to change it, but thought I'd pass it along. Bob - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
On 3/14/07, Nicolas Boichat [EMAIL PROTECTED] wrote: Hello, 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). Hi Nicolas, I tried out an earlier version of this patch several months ago just to play around with the joystick part of the accelerometer driver on my MacBook, and found that it was backwards in the y-direction compared to what Neverball seemed to want (of course, NB has no way to invert the joystick). I think I just did something like this in my own copy: + y = -y; input_report_abs(applesmc_idev, ABS_X, x - rest_x); input_report_abs(applesmc_idev, ABS_Y, y - rest_y); I don't claim you necessarily want to change it, but thought I'd pass it along. Bob - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Hello,
Cong WANG wrote:
> 2007/3/14, Cong WANG wrote:
>> I am sorry. I forgot to CC to the list.
>>
>> 2007/3/14, Nicolas Boichat wrote:
>> > Hello,
>> >
>>
>>
>>
>> > +static ssize_t applesmc_show_fan_manual(struct device *dev, char *buf,
>> > + int
>> offset)
>> > +{
>> > + int ret;
>> > + u16 manual = 0;
>> > + u8 buffer[2];
>> > +
>> > + down(_sem);
>> > +
>> > + ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
>> > + manual = ((buffer[0] << 8 | buffer[1]) >> offset) & 0x01;
>> > +
>> > + up(_sem);
>> > + if (ret)
>> > + return ret;
>> > + else
>> > + return sprintf(buf, "%d\n", manual);
>> > +}
>> > +
>>
>> I doubt about your last 'sprintf'. Your 'buf' just has only two 'u8's,
>> which maybe only has two bytes, and '\n' already consumes one. So only
>> one byte is left for the decimal vaule of 'manual'. Even it is just
>> less than 10, just as what you want, the final '\0' is omitted!
>>
>> What's more, you can't get such information from the return value of
>> 'sprintf'. So I suggest you to choose 'snprintf' instead.
>>
>
> Sorry. I thought his 'buffer' as 'buf'. But my suggestion is still
> worthy your thinking.
I'm quite sure using sprintf is ok. At least it's the way sysfs helper
functions are coded in other parts of the kernel too.
I agree that the variable names (buf and buffer) used are quite
confusing though... I'll fix that.
Best regards,
Nicolas
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Hello,
Cong WANG wrote:
2007/3/14, Cong WANG wrote:
I am sorry. I forgot to CC to the list.
2007/3/14, Nicolas Boichat wrote:
Hello,
snip
+static ssize_t applesmc_show_fan_manual(struct device *dev, char *buf,
+ int
offset)
+{
+ int ret;
+ u16 manual = 0;
+ u8 buffer[2];
+
+ down(applesmc_sem);
+
+ ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
+ manual = ((buffer[0] 8 | buffer[1]) offset) 0x01;
+
+ up(applesmc_sem);
+ if (ret)
+ return ret;
+ else
+ return sprintf(buf, %d\n, manual);
+}
+
I doubt about your last 'sprintf'. Your 'buf' just has only two 'u8's,
which maybe only has two bytes, and '\n' already consumes one. So only
one byte is left for the decimal vaule of 'manual'. Even it is just
less than 10, just as what you want, the final '\0' is omitted!
What's more, you can't get such information from the return value of
'sprintf'. So I suggest you to choose 'snprintf' instead.
Sorry. I thought his 'buffer' as 'buf'. But my suggestion is still
worthy your thinking.
I'm quite sure using sprintf is ok. At least it's the way sysfs helper
functions are coded in other parts of the kernel too.
I agree that the variable names (buf and buffer) used are quite
confusing though... I'll fix that.
Best regards,
Nicolas
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
2007/3/14, Cong WANG wrote:
I am sorry. I forgot to CC to the list.
2007/3/14, Nicolas Boichat wrote:
> Hello,
>
> +static ssize_t applesmc_show_fan_manual(struct device *dev, char *buf,
> + int offset)
> +{
> + int ret;
> + u16 manual = 0;
> + u8 buffer[2];
> +
> + down(_sem);
> +
> + ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> + manual = ((buffer[0] << 8 | buffer[1]) >> offset) & 0x01;
> +
> + up(_sem);
> + if (ret)
> + return ret;
> + else
> + return sprintf(buf, "%d\n", manual);
> +}
> +
I doubt about your last 'sprintf'. Your 'buf' just has only two 'u8's,
which maybe only has two bytes, and '\n' already consumes one. So only
one byte is left for the decimal vaule of 'manual'. Even it is just
less than 10, just as what you want, the final '\0' is omitted!
What's more, you can't get such information from the return value of
'sprintf'. So I suggest you to choose 'snprintf' instead.
Sorry. I thought his 'buffer' as 'buf'. But my suggestion is still
worthy your thinking.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
I am sorry. I forgot to CC to the list.
2007/3/14, Nicolas Boichat wrote:
Hello,
+static ssize_t applesmc_show_fan_manual(struct device *dev, char *buf,
+ int offset)
+{
+ int ret;
+ u16 manual = 0;
+ u8 buffer[2];
+
+ down(_sem);
+
+ ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
+ manual = ((buffer[0] << 8 | buffer[1]) >> offset) & 0x01;
+
+ up(_sem);
+ if (ret)
+ return ret;
+ else
+ return sprintf(buf, "%d\n", manual);
+}
+
I doubt about your last 'sprintf'. Your 'buf' just has only two 'u8's,
which maybe only has two bytes, and '\n' already consumes one. So only
one byte is left for the decimal vaule of 'manual'. Even it is just
less than 10, just as what you want, the final '\0' is omitted!
What's more, you can't get such information from the return value of
'sprintf'. So I suggest you to choose 'snprintf' instead.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Hello, 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 (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? 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 my main concerns, however, I would be happy to have comments on the other parts of the code. (Please cc me I'm not subscribed to lkml) Best regards, Nicolas Boichat From: Nicolas Boichat <[EMAIL PROTECTED]> --- drivers/hwmon/Kconfig| 24 + drivers/hwmon/Makefile |1 drivers/hwmon/applesmc.c | 964 ++ 3 files changed, 989 insertions(+), 0 deletions(-) diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index c3d4856..798b91d 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -593,6 +593,30 @@ config SENSORS_HDAPS Say Y here if you have an applicable laptop and want to experience the awesome power of hdaps. +config SENSORS_APPLESMC + tristate "Apple SMC (Motion sensor, light sensor, keyboard backlight)" + depends on HWMON && INPUT && X86 + select NEW_LEDS + select LEDS_CLASS + default n + help + This driver provides support for the Apple System Management + Controller, which provides an accelerometer (Apple Sudden Motion + Sensor), light sensors, temperature sensors, keyboard backlight + control and fan control. + + Only Intel-based Apple's computers are supported (MacBook Pro, + MacBook, MacMini). + + Data from the different sensors, keyboard backlight control and fan + control are accessible via sysfs. + + This driver also provides an absolute input class device, allowing + the laptop to act as a pinball machine-esque joystick. + + Say Y here if you have an applicable laptop and want to experience + the awesome power of applesmc. + config HWMON_DEBUG_CHIP bool "Hardware Monitoring Chip debugging messages" depends on HWMON diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 4165c27..544f8d8 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_ADM1026) += adm1026.o obj-$(CONFIG_SENSORS_ADM1029) += adm1029.o obj-$(CONFIG_SENSORS_ADM1031) += adm1031.o obj-$(CONFIG_SENSORS_ADM9240) += adm9240.o +obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o obj-$(CONFIG_SENSORS_AMS) += ams/ obj-$(CONFIG_SENSORS_ATXP1)+= atxp1.o obj-$(CONFIG_SENSORS_DS1621) += ds1621.o diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c new file mode 100644 index 000..3bdd1a8 --- /dev/null +++ b/drivers/hwmon/applesmc.c @@ -0,0 +1,964 @@ +/* + * drivers/hwmon/applesmc.c - driver for Apple's SMC (accelerometer, temperature + * sensors, fan control, keyboard backlight control) used in Intel-based Apple + * computers. + * + * Copyright (C) 2007 Nicolas Boichat <[EMAIL PROTECTED]> + * + * Based on hdaps.c driver: + * Copyright (C) 2005 Robert Love <[EMAIL PROTECTED]> + * Copyright (C) 2005 Jesper Juhl <[EMAIL PROTECTED]> + * + * Fan control based on smcFanControl: + * Copyright (C) 2006 Hendrik Holtmann <[EMAIL PROTECTED]> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License v2 as published by the + * Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ +
[RFC][PATCH] Apple SMC driver (hardware monitoring and control)
Hello, 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 (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? 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 my main concerns, however, I would be happy to have comments on the other parts of the code. (Please cc me I'm not subscribed to lkml) Best regards, Nicolas Boichat From: Nicolas Boichat [EMAIL PROTECTED] --- drivers/hwmon/Kconfig| 24 + drivers/hwmon/Makefile |1 drivers/hwmon/applesmc.c | 964 ++ 3 files changed, 989 insertions(+), 0 deletions(-) diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index c3d4856..798b91d 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -593,6 +593,30 @@ config SENSORS_HDAPS Say Y here if you have an applicable laptop and want to experience the awesome power of hdaps. +config SENSORS_APPLESMC + tristate Apple SMC (Motion sensor, light sensor, keyboard backlight) + depends on HWMON INPUT X86 + select NEW_LEDS + select LEDS_CLASS + default n + help + This driver provides support for the Apple System Management + Controller, which provides an accelerometer (Apple Sudden Motion + Sensor), light sensors, temperature sensors, keyboard backlight + control and fan control. + + Only Intel-based Apple's computers are supported (MacBook Pro, + MacBook, MacMini). + + Data from the different sensors, keyboard backlight control and fan + control are accessible via sysfs. + + This driver also provides an absolute input class device, allowing + the laptop to act as a pinball machine-esque joystick. + + Say Y here if you have an applicable laptop and want to experience + the awesome power of applesmc. + config HWMON_DEBUG_CHIP bool Hardware Monitoring Chip debugging messages depends on HWMON diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 4165c27..544f8d8 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_ADM1026) += adm1026.o obj-$(CONFIG_SENSORS_ADM1029) += adm1029.o obj-$(CONFIG_SENSORS_ADM1031) += adm1031.o obj-$(CONFIG_SENSORS_ADM9240) += adm9240.o +obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o obj-$(CONFIG_SENSORS_AMS) += ams/ obj-$(CONFIG_SENSORS_ATXP1)+= atxp1.o obj-$(CONFIG_SENSORS_DS1621) += ds1621.o diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c new file mode 100644 index 000..3bdd1a8 --- /dev/null +++ b/drivers/hwmon/applesmc.c @@ -0,0 +1,964 @@ +/* + * drivers/hwmon/applesmc.c - driver for Apple's SMC (accelerometer, temperature + * sensors, fan control, keyboard backlight control) used in Intel-based Apple + * computers. + * + * Copyright (C) 2007 Nicolas Boichat [EMAIL PROTECTED] + * + * Based on hdaps.c driver: + * Copyright (C) 2005 Robert Love [EMAIL PROTECTED] + * Copyright (C) 2005 Jesper Juhl [EMAIL PROTECTED] + * + * Fan control based on smcFanControl: + * Copyright (C) 2006 Hendrik Holtmann [EMAIL PROTECTED] + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License v2 as published by the + * Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include linux/delay.h
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
I am sorry. I forgot to CC to the list.
2007/3/14, Nicolas Boichat wrote:
Hello,
snip
+static ssize_t applesmc_show_fan_manual(struct device *dev, char *buf,
+ int offset)
+{
+ int ret;
+ u16 manual = 0;
+ u8 buffer[2];
+
+ down(applesmc_sem);
+
+ ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
+ manual = ((buffer[0] 8 | buffer[1]) offset) 0x01;
+
+ up(applesmc_sem);
+ if (ret)
+ return ret;
+ else
+ return sprintf(buf, %d\n, manual);
+}
+
I doubt about your last 'sprintf'. Your 'buf' just has only two 'u8's,
which maybe only has two bytes, and '\n' already consumes one. So only
one byte is left for the decimal vaule of 'manual'. Even it is just
less than 10, just as what you want, the final '\0' is omitted!
What's more, you can't get such information from the return value of
'sprintf'. So I suggest you to choose 'snprintf' instead.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] Apple SMC driver (hardware monitoring and control)
2007/3/14, Cong WANG wrote:
I am sorry. I forgot to CC to the list.
2007/3/14, Nicolas Boichat wrote:
Hello,
snip
+static ssize_t applesmc_show_fan_manual(struct device *dev, char *buf,
+ int offset)
+{
+ int ret;
+ u16 manual = 0;
+ u8 buffer[2];
+
+ down(applesmc_sem);
+
+ ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
+ manual = ((buffer[0] 8 | buffer[1]) offset) 0x01;
+
+ up(applesmc_sem);
+ if (ret)
+ return ret;
+ else
+ return sprintf(buf, %d\n, manual);
+}
+
I doubt about your last 'sprintf'. Your 'buf' just has only two 'u8's,
which maybe only has two bytes, and '\n' already consumes one. So only
one byte is left for the decimal vaule of 'manual'. Even it is just
less than 10, just as what you want, the final '\0' is omitted!
What's more, you can't get such information from the return value of
'sprintf'. So I suggest you to choose 'snprintf' instead.
Sorry. I thought his 'buffer' as 'buf'. But my suggestion is still
worthy your thinking.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
