On Sat, 31 May 2008 15:05:33 +0300 Yan Burman <burman.yan at gmail.com> 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);
> +}
> +
>
> ...
>
