Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver
- From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
- Date: Wed, 4 Jun 2008 22:17:24 -0700
On Sat, 31 May 2008 15:05:33 +0300 Yan Burman <burman.yan@xxxxxxxxx> wrote:
HP Mobile Data Protection System 4D ACPI driver. Similar to hdaps and applesmc in functionality.
This driver provides 3 kinds of functionality:
1) Creates a misc device /dev/accel that acts similar to /dev/rtc and unblocks
the process reading from it when the device detects free-fall interrupt
2) Functions as an input class device to provide similar functionality to
hdaps, in order to be able to use the laptop as a joystick
3) Makes it possible to power the device off.
Changes from previous post:
Clean up all checkpatch.pl warnings
Remove hdaps "compatibility" that made the drive disguise itself as hdaps
Always provide 3D sensor readings instead of selecting 2D or 3D operation
Make the driver load automatically for supported hardware
Identify laptop models based on DMI and adjust axis information accordingly
The previous version of the driver seems to be used by quite a few people it seems
based on the feedback I'm getting by mail, so perhaps it can be considered for
inclusion in the kernel.
Patch looks reasonable, although my head's spinning a bit over the
what-subsystem-is-this issue.
+ }
+
+ close(fd);
+ return EXIT_SUCCESS;
+}
...
+#include <asm/atomic.h>
+
+#define VERSION "0.92"
Beware that the version becomes essentially meaningless once the code
is merged into mainline. It cannot be used to identify the exact
source code which a user is running - this is done via the main kernel
version instead.
I'd suggest removal.
+#define DRIVER_NAME "mdps"
+#define ACPI_MDPS_CLASS "accelerometer"
+#define ACPI_MDPS_ID "HPQ0004"
+
...
+/** Create a single value from 2 bytes received from the accelerometer
+ * @param hi the high byte
+ * @param lo the low byte
+ * @return the resulting value
+ */
All these comments you have sort-of look like kerneldoc only they aren't.
I suggest that either they be converted to kerneldoc or the /** and
@param annotations be removed.
+static inline s16 mdps_glue_bytes(unsigned long hi, unsigned long lo)
+{
+ /* In "12 bit right justified" mode, bit 6, bit 7, bit 8 = bit 5 */
+ return (s16)((hi << 8) | lo);
+}
+
...
+static int mdps_input_kthread(void *data)
+{
+ int x, y, z;
+
+ while (!kthread_should_stop()) {
+ mdps_get_xyz(mdps.device->handle, &x, &y, &z);
+ input_report_abs(mdps.idev, ABS_X, x - mdps.xcalib);
+ input_report_abs(mdps.idev, ABS_Y, y - mdps.ycalib);
+ input_report_abs(mdps.idev, ABS_Z, z - mdps.zcalib);
+
+ input_sync(mdps.idev);
+
+ try_to_freeze();
+ msleep_interruptible(MDPS_POLL_INTERVAL);
+ }
+
+ return 0;
+}
This wakes up every 30 milliseconds which will make powertop users unhappy.
Is this unavoidable?
...
+static int mdps_misc_open(struct inode *inode, struct file *file)
+{
+ int ret;
+
+ if (!atomic_dec_and_test(&mdps.available)) {
+ atomic_inc(&mdps.available);
+ return -EBUSY; /* already open */
+ }
These games with mdps.available are awkward and the above looks racy.
It will work out better if it is converted to test_and_set_bit() and
friends.
+ atomic_set(&mdps.count, 0);
+
+ /* Can't have shared interrupts here, since we have no way
+ * to determine in interrupt context
+ * if it was our device that caused the interrupt */
+ ret = request_irq(mdps.irq, mdps_irq, 0, "mdps", mdps_irq);
+ if (ret) {
+ atomic_inc(&mdps.available);
+ printk(KERN_ERR "mdps: IRQ%d allocation failed\n", mdps.irq);
+ return -ENODEV;
+ }
+
+ return 0;
+}
...
+static ssize_t mdps_misc_read(struct file *file, char __user *buf,
+ size_t count, loff_t *pos)
+{
+ DECLARE_WAITQUEUE(wait, current);
+ u32 data;
+ ssize_t retval = count;
+
+ if (count != sizeof(u32))
+ return -EINVAL;
+
+ add_wait_queue(&mdps.misc_wait, &wait);
+ for (; ; ) {
for ( ; ; ) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ data = atomic_xchg(&mdps.count, 0);
+ if (data)
+ break;
+
+ if (file->f_flags & O_NONBLOCK) {
+ retval = -EAGAIN;
+ goto out;
+ }
+
+ if (signal_pending(current)) {
+ retval = -ERESTARTSYS;
+ goto out;
+ }
+
+ schedule();
+ }
+
+ /* make sure we are not going into copy_to_user() with
+ * TASK_INTERRUPTIBLE state */
+ set_current_state(TASK_RUNNING);
+ if (copy_to_user(buf, &data, sizeof(data)))
+ retval = -EFAULT;
+
+out:
+ __set_current_state(TASK_RUNNING);
+ remove_wait_queue(&mdps.misc_wait, &wait);
+
+ return retval;
+}
+
...
+static int mdps_dmi_matched(const struct dmi_system_id *dmi)
+{
+ mdps.ac = *(struct axis_conversion *)dmi->driver_data;
Please remove the nasty-looking cast. dmi_system_id.driver_data is
already void*, and the cast will suppress compiler warnings which might
be useful.
+ printk(KERN_INFO "mdps: hardware type %s found.\n", dmi->ident);
+
+ return 1;
+}
+
...
+static int mdps_add(struct acpi_device *device)
+{
+ 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);
So.. we avoid overflow via dead reckoning. hm.
+ acpi_driver_data(device) = &mdps;
+
+ mdps_ALRD(device->handle, MDPS_WHO_AM_I, &val);
+ if (val != MDPS_ID) {
+ printk(KERN_ERR
+ "mdps: Accelerometer chip not LIS3LV02D{L,Q}\n");
+ return -ENODEV;
+ }
+
+ /* This is just to make sure that the same physical move
+ * is reported identically */
+ if (dmi_check_system(mdps_dmi_ids) == 0) {
+ printk(KERN_INFO "mdps: laptop model unknown, "
+ "using default axes configuration\n");
+ mdps.ac = mdps_axis_normal;
+ }
+
+ mdps_add_fs(device);
+ mdps_resume(device);
+
+ mdps_joystick_enable();
+
+ /* obtain IRQ number of our device from ACPI */
+ mdps_enum_resources(device);
+
+ if (power_off) /* see if user wanted to power off the device on load */
+ mdps_poweroff(mdps.device->handle);
+
+ /* if we did not get an IRQ from ACPI - we have nothing more to do */
+ if (!mdps.irq) {
+ printk(KERN_INFO
+ "mdps: No IRQ in ACPI. Disabling /dev/accel\n");
+ return 0;
+ }
+
+ atomic_set(&mdps.available, 1); /* init the misc device open count */
+ init_waitqueue_head(&mdps.misc_wait);
It is preferable to do this at compile-time, which can be done with
__WAIT_QUEUE_HEAD_INITIALIZER(), and a bit of pain.
+ ret = misc_register(&mdps_misc_device);
+ if (ret)
+ printk(KERN_ERR "mdps: misc_register failed\n");
+
+ return 0;
+}
...
+static void mdps_joystick_enable(void)
+{
+ if (mdps.idev)
+ return;
+
+ mdps.idev = input_allocate_device();
+ if (!mdps.idev)
+ return;
No error propagation or reporting here?
+ mdps_calibrate_joystick();
+
+ mdps.idev->name = "HP Mobile Data Protection System";
+ mdps.idev->phys = "mdps/input0";
+ mdps.idev->id.bustype = BUS_HOST;
+ mdps.idev->id.vendor = 0;
+ mdps.idev->dev.parent = &mdps.pdev->dev;
+
+ set_bit(EV_ABS, mdps.idev->evbit);
+
+ input_set_abs_params(mdps.idev, ABS_X, -MDPS_MAX_VAL, MDPS_MAX_VAL,
+ 3, 0);
+ input_set_abs_params(mdps.idev, ABS_Y, -MDPS_MAX_VAL, MDPS_MAX_VAL,
+ 3, 0);
+ input_set_abs_params(mdps.idev, ABS_Z, -MDPS_MAX_VAL, MDPS_MAX_VAL,
+ 3, 0);
+
+ mdps.idev->open = mdps_joystick_open;
+ mdps.idev->close = mdps_joystick_close;
+
+ if (input_register_device(mdps.idev)) {
+ input_free_device(mdps.idev);
+ mdps.idev = NULL;
+ }
+}
...
+/* Sysfs stuff */
Was it tested with CONFIG_SYSFS=n?
+static ssize_t mdps_position_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int x, y, z;
+ mdps_get_xyz(mdps.device->handle, &x, &y, &z);
We typically put a blank line between end-of-locals and start-of-code.
+ return sprintf(buf, "(%d,%d,%d)\n", x, y, z);
+}
+
+static ssize_t mdps_state_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%s\n", (mdps.is_on ? "on" : "off"));
strcpy(buf, mdps.is_on ? "on\n" : "off\n");
:)
+}
...
+static ssize_t mdps_rate_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ unsigned long ctrl;
+ int rate = 0;
This is an unneeded initialisation to shut the compiler up. Problems
are a) it takes a lot of staring for the reader to work out why this
initialisation is here and b) it generates extra code.
unintialized_var() sovles both those issues, although some don't like it.
+ mdps_ALRD(mdps.device->handle, MDPS_CTRL_REG1, &ctrl);
+
+ /* get the sampling rate of the accelerometer in HZ */
+ switch ((ctrl & 0x30) >> 4) {
+ case 00:
+ rate = 40;
+ break;
+
+ case 01:
+ rate = 160;
+ break;
+
+ case 02:
+ rate = 640;
+ break;
+
+ case 03:
+ rate = 2560;
+ break;
+ }
+
+ return sprintf(buf, "%d\n", rate);
+}
+
+static ssize_t mdps_state_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int state;
+ if (sscanf(buf, "%d", &state) != 1 || (state != 1 && state != 0))
+ return -EINVAL;
space-after-locals, please.
+ mdps.is_on = state;
+
+ if (mdps.is_on)
+ mdps_poweron(mdps.device->handle);
+ else
+ mdps_poweroff(mdps.device->handle);
+
+ return count;
+}
...
+static int mdps_add_fs(struct acpi_device *device)
+{
+ mdps.pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+ if (IS_ERR(mdps.pdev))
+ return PTR_ERR(mdps.pdev);
in lots of places.
+ return sysfs_create_group(&mdps.pdev->dev.kobj, &mdps_attribute_group);--
+}
+
...
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Prev by Date: Re: [RFC][PATCH] introduce task cgroup (#task restrictioon for prevent fork bomb by cgroup)
- Next by Date: Re: [ANNOUNCE] sched: schedtop utility
- Previous by thread: [RFC][PATCH] introduce task cgroup (#task restrictioon for prevent fork bomb by cgroup)
- Next by thread: [PATCH] cgroup: anotate two variables with __read_mostly
- Index(es):
Relevant Pages
|
