Hi.
I posted a previous version of my driver a few weeks ago, so here is a new
version that
handles interrupts from the device when the accelerometer detects that the
laptop is falling.
The interrupt part only works with 2.6.19-rc* since previous kernels (at
least 2.6.17 and 2.6.18 could not allocate the device's IRQ - perhaps due to
some ACPI bios bug that now is handled better)
The driver supports:
1) interface similar to hdaps that allows running hdapsd with trivial
modifiations
2) input class device that allows playing games such as neverball by using
the laptop as a joystick
3) Ability to power off the acceleromter (it may prolong just a litlte
battery life)
4) A misc device /dev/acel similar in interface to /dev/rtc that reacts on
interrupts from the accelerometer allowing userspace to catch such events
and react accordingly - park the HD heads, or perhaps print "Your laptop is
falling. Are you sure you want to catch it?" The daemon for that
i trivial.
Should I also add a documentation file to document all the interfaces - it
should be quite short though.
I sent this patch as a reply to previous thread about 24 hours ago, but for
some reason didn't see it on the list, so here it is again. Didn't inline
the patch since hotmail does line wrapping on it making it impossible to
apply.
P.S.
Should I send this to hwmon maintainer perhaps, so that other people could
benefit from this?
Regards
Yan Burman
_________________________________________________________________
FREE pop-up blocking with the new MSN Toolbar - get it now!
http://toolbar.msn.click-url.com/go/onm00200415ave/direct/01/
On Fri, 03 Nov 2006 18:33:31 +0200
"Burman Yan" <[email protected]> wrote:
> Hi.
>
> I posted a previous version of my driver a few weeks ago, so here is a new
> version that
> handles interrupts from the device when the accelerometer detects that the
> laptop is falling.
> The interrupt part only works with 2.6.19-rc* since previous kernels (at
> least 2.6.17 and 2.6.18 could not allocate the device's IRQ - perhaps due to
> some ACPI bios bug that now is handled better)
> The driver supports:
> 1) interface similar to hdaps that allows running hdapsd with trivial
> modifiations
It would (I assume) be nice to make the interface 100% compatible with
existing hdaps drivers.
What are the interface differences and why are they necessary?
> 2) input class device that allows playing games such as neverball by using
> the laptop as a joystick
heh.
> 3) Ability to power off the acceleromter (it may prolong just a litlte
> battery life)
OK.
> 4) A misc device /dev/acel similar in interface to /dev/rtc that reacts on
> interrupts from the accelerometer allowing userspace to catch such events
> and react accordingly - park the HD heads, or perhaps print "Your laptop is
> falling. Are you sure you want to catch it?" The daemon for that
> i trivial.
Should be spelled "accel".
Is this capability something which other hdaps drivers could provide? If
so, all drivers should provide the same interface as much as poss, so we'd
need to look at this one from that point of view.
> Should I also add a documentation file to document all the interfaces - it
> should be quite short though.
That would help a lot. In fact it'll be hard to address the above
questions without that document, because they're all about the interface.
Some minor points:
> diff -puN drivers/hwmon/Kconfig~hp-mobile-data-protection-system-driver-with-interrupt-handling drivers/hwmon/Kconfig
> --- a/drivers/hwmon/Kconfig~hp-mobile-data-protection-system-driver-with-interrupt-handling
> +++ a/drivers/hwmon/Kconfig
> @@ -547,6 +547,26 @@ config SENSORS_HDAPS
> Say Y here if you have an applicable laptop and want to experience
> the awesome power of hdaps.
>
> +config SENSORS_MDPS
> + tristate "HP Mobile Data Protection System 3D (mdps)"
> + depends on ACPI && HWMON && INPUT && X86
OK, so it's x86-only. It needs to be...
> + default n
> + help
> + This driver provides support for the HP Mobile Data Protection
> + System 3D (mdps), which is an accelerometer. Only HP nc6400 and nc6420
> + is supported right now, but it may work on other models as well. The
> + accelerometer data is readable via /sys/devices/platform/mdps.
> +
> + This driver also provides an absolute input class device, allowing
> + the laptop to act as a pinball machine-esque joystick.
> +
> + Another feature of the driver is misc device called mdps that acts
> + similar to /dev/rtc and reacts on free-fall interrupts received from
> + the device.
> +
> + This driver can also be built as a module. If so, the module
> + will be called mdps.
> +
>
> ...
>
> +
> +#define MDPS_ID 0x3A
> +
> +/* mouse device poll interval in milliseconds */
> +#define MDPS_POLL_INTERVAL 30
> +
> +static unsigned int mouse = 0;
The `= 0' is unneeded.
> +module_param(mouse, bool, S_IRUGO);
> +MODULE_PARM_DESC(mouse, "Enable the input class device on module load");
> +
> +static unsigned int power_off = 0;
Here also.
> +module_param(power_off, bool, S_IRUGO);
> +MODULE_PARM_DESC(power_off, "Turn off device on module load");
> +
> +struct acpi_mdps
> +{
Should be formatted as
struct acpi_mdps {
> + struct acpi_device* device; /* The ACPI device */
Should be formatted as
struct acpi_device *device; /* The ACPI device */
(entire patch)
> + u32 irq; /* IRQ number */
> + struct input_dev* idev; /* input device */
> + struct task_struct* kthread; /* kthread for input */
> + int xcalib; /* calibrated null value for x */
> + int ycalib; /* calibrated null value for y */
> + int is_on; /* whether the device is on or off */
> + struct platform_device* pdev; /* platform device */
> + atomic_t count; /* how many times we got interrupts after the last read */
> + struct completion complete; /* we wait on this in read */
> + int reader_waiting;
> +};
> +
>
> ...
>
> +
> +/** Kthread polling function
> + * @param data unused - here to conform to threadfn prototype
> + * @return 0 unused - here to conform to threadfn prototype
> + */
> +static int mdps_mouse_kthread(void *data)
> +{
> + int x, y;
> +
> + while (!kthread_should_stop()) {
> + mdps_get_xy(mdps.device->handle, &x, &y);
> +
> + /* need to invert the X axis for this to look natural */
> + input_report_abs(mdps.idev, ABS_X, -(x - mdps.xcalib));
> + input_report_abs(mdps.idev, ABS_Y, y - mdps.ycalib);
> +
> + input_sync(mdps.idev);
> +
> + msleep(MDPS_POLL_INTERVAL);
> + }
> +
> + return 0;
> +}
This will probably break software suspend. You'll need a try_to_freeze()
in that loop.
>
> ...
>
> +
> +static int mdps_misc_open(struct inode *inode, struct file *file)
> +{
> + if (!atomic_dec_and_test(&mdps_available)) {
> + atomic_inc(&mdps_available);
> + return -EBUSY; /* already open */
> + }
> +
> + atomic_set(&mdps.count, 0);
> + return 0;
> +}
That's a bit awkward-looking. It would be simpler to use mdps_lock
to force the single-opener behaviour.
> +static ssize_t mdps_misc_read(struct file *file, char __user *buf,
> + size_t count, loff_t *pos)
> +{
> + u32 tmp;
> + int ret;
> + unsigned long flags;
> + u32* user_buf = (u32*)buf;
> +
> + if (count != sizeof(u32))
> + return -EINVAL;
> +
> + init_completion(&mdps.complete);
> +
> + spin_lock_irqsave(&mdps_lock, flags);
> + mdps.reader_waiting = 1;
> + spin_unlock_irqrestore(&mdps_lock, flags);
> +
> + ret = wait_for_completion_interruptible(&mdps.complete);
> + if (ret)
> + return ret;
> + tmp = atomic_read(&mdps.count);
> + atomic_set(&mdps.count, 0);
> +
> + if (put_user(tmp, user_buf))
> + return -EFAULT;
> +
> + return count;
> +}
It'd be nice to have a few more code comments in there..
This function is the one which provides the use-your-laptop-as-a-joystick
interface, yes?
It's peculiar that the read() function will lose events if there's no
process waiting in the read() function. A more typical implementation
would queue the events up and the read() implementation would dequeue them.
I'm sure the input layer provides planty of things to support that.
The forcing of a char __user * into a u32* is a bit ugly. Fortunately x86
will permit random alignments, but perhaps a copy_to_user() would be
cleaner. Or perhaps just return the number as an ascii decimal string. At
a minimum the type should be u32 __user * to keep sparse happy.
>
> ..
>
> +
> +static void mdps_enum_resources(struct acpi_device * device)
> +{
> + acpi_status status;
> +
> + status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> + mdps_get_resource, &mdps.irq);
> + if (ACPI_FAILURE(status))
> + printk(KERN_DEBUG "mdps: Error getting resources\n");
> +}
> +
> +int mdps_add(struct acpi_device *device)
mdps_add() was declared as static, but it wasn't defined as static.
> +{
> + unsigned long val;
> + int ret;
> +
> + if (!device)
> + return -EINVAL;
> +
> + mdps.device = device;
> + strcpy(acpi_device_name(device), DRIVER_NAME);
> + strcpy(acpi_device_class(device), ACPI_MDPS_CLASS);
> + acpi_driver_data(device) = &mdps;
> +
> + mdps_ALRD(device->handle, MDPS_WHO_AM_I, &val);
> + if (val != MDPS_ID) {
> + printk(KERN_INFO "mdps: Accelerometer chip not LIS3LV02D{L,Q}\n");
> + return -ENODEV;
> + }
> +
> + mdps_enum_resources(device);
> + mdps_add_fs(device);
> + mdps_resume(device, 3);
> +
> + if (power_off)
> + mdps_poweroff(mdps.device->handle);
> +
> + if (mdps.irq) {
> + ret = request_irq(mdps.irq, mdps_irq, 0, "mdps", mdps_irq);
> + if (ret) {
> + printk(KERN_INFO "mdps: (IRQ%d) allocation failed\n", mdps.irq);
> + mdps.irq = 0;
> + } else {
> + ret = misc_register(&mdps_misc_device);
> + if (ret) {
> + free_irq(mdps.irq, mdps_irq);
> + mdps.irq = 0;
> + printk(KERN_ERR "mdps: misc_register failed\n");
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +int mdps_remove(struct acpi_device *device, int type)
static scope.
> +{
> + if (!device)
> + return -EINVAL;
> +
> + if (mdps.irq) {
> + misc_deregister(&mdps_misc_device);
> + free_irq(mdps.irq, mdps_irq);
> + mdps.irq = 0;
> + }
> +
> + if (mouse)
> + mdps_mouse_disable();
> +
> + return mdps_remove_fs();
> +}
> +
> +static void mdps_calibrate_mouse(void)
> +{
> + int x, y;
> + mdps_get_xy(mdps.device->handle, &x, &y);
> +
> + mdps.xcalib = x;
> + mdps.ycalib = y;
> +}
> +
> +void mdps_mouse_enable(void)
static scope.
> +{
> + if (mdps.idev)
> + return;
> +
> + mdps.idev = input_allocate_device();
> + if (!mdps.idev)
> + return;
> +
> + mdps_calibrate_mouse();
> +
> + mdps.idev->name = "HP Mobile Data Protection System";
> + mdps.idev->id.bustype = BUS_HOST;
> + mdps.idev->id.vendor = 0;
> +
> + set_bit(EV_ABS, mdps.idev->evbit);
> + set_bit(EV_KEY, mdps.idev->evbit);
> +
> + input_set_abs_params(mdps.idev, ABS_X, -2048, 2048, 3, 0);
> + input_set_abs_params(mdps.idev, ABS_Y, -2048, 2048, 3, 0);
> +
> + if (input_register_device(mdps.idev)) {
> + input_free_device(mdps.idev);
> + mdps.idev = NULL;
> + return;
> + }
> +
> + mdps.kthread = kthread_run(mdps_mouse_kthread, NULL, "kmdps");
> + if (IS_ERR(mdps.kthread)) {
> + input_unregister_device(mdps.idev);
> + mdps.idev = NULL;
> + return;
> + }
> +
> + mouse = 1;
> +}
> +
> +void mdps_mouse_disable(void)
static scope.
> +{
> + if (!mdps.idev)
> + return;
> +
> + kthread_stop(mdps.kthread);
> +
> + input_unregister_device(mdps.idev);
> + mdps.idev = NULL;
> +}
> +
>
> ..
>
> +static struct attribute_group mdps_attribute_group = {
> + .attrs = mdps_attributes
> +};
> +
> +int mdps_add_fs(struct acpi_device *device)
static
> +{
> + mdps.pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> + if (IS_ERR(mdps.pdev))
> + return PTR_ERR(mdps.pdev);
> +
> + return sysfs_create_group(&mdps.pdev->dev.kobj, &mdps_attribute_group);
> +}
> +
> +int mdps_remove_fs(void)
static
On 11/6/06, Andrew Morton <[email protected]> wrote:
> On Fri, 03 Nov 2006 18:33:31 +0200
> "Burman Yan" <[email protected]> wrote:
> > +
> > +static unsigned int mouse = 0;
>
> The `= 0' is unneeded.
>
> > +module_param(mouse, bool, S_IRUGO);
> > +MODULE_PARM_DESC(mouse, "Enable the input class device on module load");
Does the parameter have to be called "mouse"? I'd rename it to "input"
and drop the work "class" from parameter description.
> > +
> > + if (input_register_device(mdps.idev)) {
> > + input_free_device(mdps.idev);
> > + mdps.idev = NULL;
> > + return;
> > + }
> > +
> > + mdps.kthread = kthread_run(mdps_mouse_kthread, NULL, "kmdps");
> > + if (IS_ERR(mdps.kthread)) {
> > + input_unregister_device(mdps.idev);
> > + mdps.idev = NULL;
> > + return;
> > + }
> > +
Please consider implementing mdps_input_open() and mdps_input_close()
and create and run the polling thread from there - there is no point
in polling the device if noone listens to its events.
--
Dmitry
Hi!
> The driver supports:
> 1) interface similar to hdaps that allows running hdapsd with trivial
> modifiations
> 2) input class device that allows playing games such as neverball by using
> the laptop as a joystick
...
> 4) A misc device /dev/acel similar in interface to /dev/rtc that reacts on
> interrupts from the accelerometer allowing userspace to catch such events
> and react accordingly - park the HD heads, or perhaps print "Your laptop is
> falling. Are you sure you want to catch it?" The daemon for that
> i trivial.
Ahh, so 2 different interfaces (/sys + input) for hdaps was not
enough, now we have 3rd one :-(. Can't we somehow improve one of them
(input?) so that we can remove the remaining two?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>From: Andrew Morton <[email protected]>
>To: "Burman Yan" <[email protected]>
>CC: [email protected], Jean Delvare <[email protected]>,
>Dmitry Torokhov <[email protected]>
>Subject: Re: [PATCH] HP Mobile data protection system driver with interrupt
>handling
>Date: Mon, 6 Nov 2006 13:15:35 -0800
>
>On Fri, 03 Nov 2006 18:33:31 +0200
>"Burman Yan" <[email protected]> wrote:
>
> > Hi.
> >
> > I posted a previous version of my driver a few weeks ago, so here is a
>new
> > version that
> > handles interrupts from the device when the accelerometer detects that
>the
> > laptop is falling.
> > The interrupt part only works with 2.6.19-rc* since previous kernels (at
> > least 2.6.17 and 2.6.18 could not allocate the device's IRQ - perhaps
>due to
> > some ACPI bios bug that now is handled better)
> > The driver supports:
> > 1) interface similar to hdaps that allows running hdapsd with trivial
> > modifiations
>
>It would (I assume) be nice to make the interface 100% compatible with
>existing hdaps drivers.
>
>What are the interface differences and why are they necessary?
The diference is that hdaps is something specific to IBM/Lenovo laptops.
hdaps provides /sys/devices/platform/hdaps/*
I provide /sys/devices/platform/mdps/*, but the interface is kept similar
to hdaps just to make it possible to use hdapsd. This interface is far from
the best way to use mdps, since it does polling on accelerometer values.
HP's accelerometer can provide an interrupt when it detects that the laptop
is falling. I implemented that by the means of /dev/accel. This makes the
user space
daemon extremely dumb, since it has no need to do heuristics in order to
detect
if the laptop is falling or just somebody wanted to scratch his knee and the
laptop shifted.
>
> > 2) input class device that allows playing games such as neverball by
>using
> > the laptop as a joystick
>
>heh.
>
> > 3) Ability to power off the acceleromter (it may prolong just a litlte
> > battery life)
>
>OK.
>
> > 4) A misc device /dev/acel similar in interface to /dev/rtc that reacts
>on
> > interrupts from the accelerometer allowing userspace to catch such
>events
> > and react accordingly - park the HD heads, or perhaps print "Your laptop
>is
> > falling. Are you sure you want to catch it?" The daemon for that
> > i trivial.
>
>Should be spelled "accel".
Point taken and fixed.
>
>Is this capability something which other hdaps drivers could provide? If
>so, all drivers should provide the same interface as much as poss, so we'd
>need to look at this one from that point of view.
>From my googling, I found that there are 3 drivers that provide similar
functionality
right now:
1) hdaps. I don't know if the device is capable of generating interrupts on
free falls.
The driver does not do that right now from what I saw. It is also the only
2D acceleromter of the 3.
2) ams - something that is present on apple laptops - these can generate
interrupts.
3) mdps - this can generate interrupts on free fall - that's what I created
/dev/acel for.
>From what I saw these are the only 3 laptop classes that have accelerometers
in them right now.
>
> > Should I also add a documentation file to document all the interfaces -
>it
> > should be quite short though.
>
>That would help a lot. In fact it'll be hard to address the above
>questions without that document, because they're all about the interface.
No problem - working on it.
>
>
>Some minor points:
>
>
> > diff -puN
>drivers/hwmon/Kconfig~hp-mobile-data-protection-system-driver-with-interrupt-handling
>drivers/hwmon/Kconfig
> > ---
>a/drivers/hwmon/Kconfig~hp-mobile-data-protection-system-driver-with-interrupt-handling
> > +++ a/drivers/hwmon/Kconfig
> > @@ -547,6 +547,26 @@ config SENSORS_HDAPS
> > Say Y here if you have an applicable laptop and want to experience
> > the awesome power of hdaps.
> >
> > +config SENSORS_MDPS
> > + tristate "HP Mobile Data Protection System 3D (mdps)"
> > + depends on ACPI && HWMON && INPUT && X86
>
>OK, so it's x86-only. It needs to be...
>
> > + default n
> > + help
> > + This driver provides support for the HP Mobile Data
>Protection
> > + System 3D (mdps), which is an accelerometer. Only HP nc6400
>and nc6420
> > + is supported right now, but it may work on other models as
>well. The
> > + accelerometer data is readable via
>/sys/devices/platform/mdps.
> > +
> > + This driver also provides an absolute input class device,
>allowing
> > + the laptop to act as a pinball machine-esque joystick.
> > +
> > + Another feature of the driver is misc device called mdps that
>acts
> > + similar to /dev/rtc and reacts on free-fall interrupts
>received from
> > + the device.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called mdps.
> > +
> >
> > ...
> >
> > +
> > +#define MDPS_ID 0x3A
> > +
> > +/* mouse device poll interval in milliseconds */
> > +#define MDPS_POLL_INTERVAL 30
> > +
> > +static unsigned int mouse = 0;
>
>The `= 0' is unneeded.
>
> > +module_param(mouse, bool, S_IRUGO);
> > +MODULE_PARM_DESC(mouse, "Enable the input class device on module
>load");
> > +
> > +static unsigned int power_off = 0;
>
>Here also.
I though it would be more visual this way, but you're probably right.
Changed that.
>
> > +module_param(power_off, bool, S_IRUGO);
> > +MODULE_PARM_DESC(power_off, "Turn off device on module load");
> > +
> > +struct acpi_mdps
> > +{
>
>Should be formatted as
>
>struct acpi_mdps {
>
> > + struct acpi_device* device; /* The ACPI device */
>
>Should be formatted as
>
> struct acpi_device *device; /* The ACPI device */
>
>(entire patch)
I'm more used to C++ semantics, but changed that for the next iteration
patch.
>
> > + u32 irq; /* IRQ number */
> > + struct input_dev* idev; /* input device */
> > + struct task_struct* kthread; /* kthread for input */
> > + int xcalib; /* calibrated null value for x */
> > + int ycalib; /* calibrated null value for y */
> > + int is_on; /* whether the device is on or off
>*/
> > + struct platform_device* pdev; /* platform device */
> > + atomic_t count; /* how many times we got interrupts
>after the last read */
> > + struct completion complete; /* we wait on this in read */
> > + int reader_waiting;
> > +};
> > +
> >
> > ...
> >
> > +
> > +/** Kthread polling function
> > + * @param data unused - here to conform to threadfn prototype
> > + * @return 0 unused - here to conform to threadfn prototype
> > + */
> > +static int mdps_mouse_kthread(void *data)
> > +{
> > + int x, y;
> > +
> > + while (!kthread_should_stop()) {
> > + mdps_get_xy(mdps.device->handle, &x, &y);
> > +
> > + /* need to invert the X axis for this to look natural */
> > + input_report_abs(mdps.idev, ABS_X, -(x - mdps.xcalib));
> > + input_report_abs(mdps.idev, ABS_Y, y - mdps.ycalib);
> > +
> > + input_sync(mdps.idev);
> > +
> > + msleep(MDPS_POLL_INTERVAL);
> > + }
> > +
> > + return 0;
> > +}
>
>This will probably break software suspend. You'll need a try_to_freeze()
>in that loop.
Well, I stop the kthread all together upon suspend, but I added this just in
case, since
to be honest I don't know all the details of software suspend handling.
>
> >
> > ...
> >
> > +
> > +static int mdps_misc_open(struct inode *inode, struct file *file)
> > +{
> > + if (!atomic_dec_and_test(&mdps_available)) {
> > + atomic_inc(&mdps_available);
> > + return -EBUSY; /* already open */
> > + }
> > +
> > + atomic_set(&mdps.count, 0);
> > + return 0;
> > +}
>
>That's a bit awkward-looking. It would be simpler to use mdps_lock
>to force the single-opener behaviour.
I took this code almost verbatim from LDD3 and I saw it in a few places
before.
mdps_lock is used for interrupt counter access.
>
> > +static ssize_t mdps_misc_read(struct file *file, char __user *buf,
> > + size_t count, loff_t *pos)
> > +{
> > + u32 tmp;
> > + int ret;
> > + unsigned long flags;
> > + u32* user_buf = (u32*)buf;
> > +
> > + if (count != sizeof(u32))
> > + return -EINVAL;
> > +
> > + init_completion(&mdps.complete);
> > +
> > + spin_lock_irqsave(&mdps_lock, flags);
> > + mdps.reader_waiting = 1;
> > + spin_unlock_irqrestore(&mdps_lock, flags);
> > +
> > + ret = wait_for_completion_interruptible(&mdps.complete);
> > + if (ret)
> > + return ret;
> > + tmp = atomic_read(&mdps.count);
> > + atomic_set(&mdps.count, 0);
> > +
> > + if (put_user(tmp, user_buf))
> > + return -EFAULT;
> > +
> > + return count;
> > +}
>
>It'd be nice to have a few more code comments in there..
>
>This function is the one which provides the use-your-laptop-as-a-joystick
>interface, yes?
No - this stuff gives you /dev/rtc like interface (/dev/accel) to whenever
your laptop is thrown in
the air or dropped from the 5th floor.
>
>It's peculiar that the read() function will lose events if there's no
>process waiting in the read() function. A more typical implementation
>would queue the events up and the read() implementation would dequeue them.
>I'm sure the input layer provides planty of things to support that.
I added poll and fasync to this interface, so that should be cleaner in my
next post,
since the completion API had to go anyway and I replaced that with
wait_queue stuff.
>
>The forcing of a char __user * into a u32* is a bit ugly. Fortunately x86
>will permit random alignments, but perhaps a copy_to_user() would be
>cleaner. Or perhaps just return the number as an ascii decimal string. At
>a minimum the type should be u32 __user * to keep sparse happy.
Point taken - converted to copy_to_user.
>
> >
> > ..
> >
> > +
> > +static void mdps_enum_resources(struct acpi_device * device)
> > +{
> > + acpi_status status;
> > +
> > + status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> > + mdps_get_resource, &mdps.irq);
> > + if (ACPI_FAILURE(status))
> > + printk(KERN_DEBUG "mdps: Error getting resources\n");
> > +}
> > +
> > +int mdps_add(struct acpi_device *device)
>
>mdps_add() was declared as static, but it wasn't defined as static.
Been programming too much C++ lately - this stuff generates an error in C++.
Fixed.
P.S.
Off topic.
Would that be of any value going through kernel and replacing
kmalloc+memset with kzalloc - this should lower memory footprint, no?
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
>From: "Dmitry Torokhov" <[email protected]>
>To: "Andrew Morton" <[email protected]>
>CC: "Burman Yan" <[email protected]>, [email protected], "Jean
>Delvare" <[email protected]>
>Subject: Re: [PATCH] HP Mobile data protection system driver with interrupt
>handling
>Date: Mon, 6 Nov 2006 17:18:53 -0500
>
>On 11/6/06, Andrew Morton <[email protected]> wrote:
>>On Fri, 03 Nov 2006 18:33:31 +0200
>>"Burman Yan" <[email protected]> wrote:
>> > +
>> > +static unsigned int mouse = 0;
>>
>>The `= 0' is unneeded.
>>
>> > +module_param(mouse, bool, S_IRUGO);
>> > +MODULE_PARM_DESC(mouse, "Enable the input class device on module
>>load");
>
>Does the parameter have to be called "mouse"? I'd rename it to "input"
>and drop the work "class" from parameter description.
Dropping the "class" seems logical, but calling the parameter input
seems confusing to me - to a user that doesn't want to read too much
manual/code and just wants to play around with the device (I do that
sometimes)
mouse sounds more reasonable to me.
>
>> > +
>> > + if (input_register_device(mdps.idev)) {
>> > + input_free_device(mdps.idev);
>> > + mdps.idev = NULL;
>> > + return;
>> > + }
>> > +
>> > + mdps.kthread = kthread_run(mdps_mouse_kthread, NULL, "kmdps");
>> > + if (IS_ERR(mdps.kthread)) {
>> > + input_unregister_device(mdps.idev);
>> > + mdps.idev = NULL;
>> > + return;
>> > + }
>> > +
>
>Please consider implementing mdps_input_open() and mdps_input_close()
>and create and run the polling thread from there - there is no point
>in polling the device if noone listens to its events.
You have a point - I'll see into that.
>
>--
>Dmitry
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
On 11/7/06, Burman Yan <[email protected]> wrote:
>
> >From: "Dmitry Torokhov" <[email protected]>
> >To: "Andrew Morton" <[email protected]>
> >CC: "Burman Yan" <[email protected]>, [email protected], "Jean
> >Delvare" <[email protected]>
> >Subject: Re: [PATCH] HP Mobile data protection system driver with interrupt
> >handling
> >Date: Mon, 6 Nov 2006 17:18:53 -0500
> >
> >On 11/6/06, Andrew Morton <[email protected]> wrote:
> >>On Fri, 03 Nov 2006 18:33:31 +0200
> >>"Burman Yan" <[email protected]> wrote:
> >> > +
> >> > +static unsigned int mouse = 0;
> >>
> >>The `= 0' is unneeded.
> >>
> >> > +module_param(mouse, bool, S_IRUGO);
> >> > +MODULE_PARM_DESC(mouse, "Enable the input class device on module
> >>load");
> >
> >Does the parameter have to be called "mouse"? I'd rename it to "input"
> >and drop the work "class" from parameter description.
>
> Dropping the "class" seems logical, but calling the parameter input
> seems confusing to me - to a user that doesn't want to read too much
> manual/code and just wants to play around with the device (I do that
> sometimes)
> mouse sounds more reasonable to me.
>
Except that the device is more similar to a joystick than a mouse...
--
Dmitry
>From: "Dmitry Torokhov" <[email protected]>
>To: "Burman Yan" <[email protected]>
>CC: [email protected], [email protected], [email protected]
>Subject: Re: [PATCH] HP Mobile data protection system driver with interrupt
>handling
>Date: Tue, 7 Nov 2006 15:37:51 -0500
>
>On 11/7/06, Burman Yan <[email protected]> wrote:
>>
>> >From: "Dmitry Torokhov" <[email protected]>
>> >To: "Andrew Morton" <[email protected]>
>> >CC: "Burman Yan" <[email protected]>, [email protected],
>>"Jean
>> >Delvare" <[email protected]>
>> >Subject: Re: [PATCH] HP Mobile data protection system driver with
>>interrupt
>> >handling
>> >Date: Mon, 6 Nov 2006 17:18:53 -0500
>> >
>> >On 11/6/06, Andrew Morton <[email protected]> wrote:
>> >>On Fri, 03 Nov 2006 18:33:31 +0200
>> >>"Burman Yan" <[email protected]> wrote:
>> >> > +
>> >> > +static unsigned int mouse = 0;
>> >>
>> >>The `= 0' is unneeded.
>> >>
>> >> > +module_param(mouse, bool, S_IRUGO);
>> >> > +MODULE_PARM_DESC(mouse, "Enable the input class device on module
>> >>load");
>> >
>> >Does the parameter have to be called "mouse"? I'd rename it to "input"
>> >and drop the work "class" from parameter description.
>>
>>Dropping the "class" seems logical, but calling the parameter input
>>seems confusing to me - to a user that doesn't want to read too much
>>manual/code and just wants to play around with the device (I do that
>>sometimes)
>>mouse sounds more reasonable to me.
>>
>
>Except that the device is more similar to a joystick than a mouse...
Agreed - joystick it is.
>
>--
>Dmitry
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/