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)
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: [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/
