* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-05-31 12:05 [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver Yan Burman
@ 2008-06-04 19:24 ` Éric Piel
2008-06-04 20:58 ` Jean Delvare
` (2 more replies)
2008-06-05 5:17 ` [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver Andrew Morton
2008-06-05 7:43 ` [lm-sensors] " Riku Voipio
2 siblings, 3 replies; 27+ messages in thread
From: Éric Piel @ 2008-06-04 19:24 UTC (permalink / raw)
To: Andrew Morton, pavel, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman
Cc: Yan Burman, LKML, HWMON
Hello,
We haven't received any review for this driver since last week's post by
Yan :-( That's why I'm adding explicitly as receivers of this email all
the people who have reviewed the first two previous takes. Apologies for
the possible multiple receptions.
It would be _really really kind_ if some of you could take some time and
comment (or simply ACK ;-) ) this code. It adds support for hardware
which can be found in mostly every HP Compaq laptop less than three
years old. It's quite worthy.
Mark, as maintainer of the hwmon tree, would accept this driver? Let us
know if there are things we need to change.
Thanks,
Eric
31-05-08 14:05, Yan Burman wrote/a écrit:
> 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.
>
> Signed-off-by: Yan Burman <burman.yan@gmail.com>
> Signed-off-by: Eric Piel <eric.piel@tremplin-utc.net>
>
>
> diff -Nrubp linux-2.6.25.4_orig/Documentation/hwmon/mdps linux-2.6.25.4/Documentation/hwmon/mdps
> --- linux-2.6.25.4_orig/Documentation/hwmon/mdps 1970-01-01 02:00:00.000000000 +0200
> +++ linux-2.6.25.4/Documentation/hwmon/mdps 2008-05-28 11:15:33.000000000 +0300
> @@ -0,0 +1,96 @@
> +Kernel driver mdps
> +==================
> +
> +Supported chips:
> +
> + * STMicroelectronics LIS3LV02DL and LIS3LV02DQ
> +
> +Author:
> + Yan Burman <burman.yan@gmail.com>
> + Eric Piel <eric.piel@tremplin-utc.net>
> +
> +
> +Description
> +-----------
> +
> +This driver provides support for the accelerometer found in various HP laptops
> +sporting the feature officially called "HP Mobile Data Protection System 3D" or
> +"HP 3D DriveGuard". HP nc6420, nc2510, nw9440 and nx9420 are supported right now, but
> +it should work on other models as well. The accelerometer data is readable via
> +/sys/devices/platform/mdps.
> +
> +Sysfs attributes under /sys/devices/platform/mdps/:
> +position - 3D position that the accelerometer reports. Format: "(x,y,z)"
> +calibrate - read: values (x, y, z) that are used as the base for input class device operation.
> + write: forces the base to be recalibrated.
> +rate - reports the sampling rate of the accelerometer device in HZ
> +state - read: the current power state of the accelerometer device
> + write: "0" or "1" to power on/off the device
> +
> +Load time parameters:
> +bool power_off - whether to power off the device on module load (default 0)
> +
> +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 accel that acts
> +similar to /dev/rtc and reacts on free-fall interrupts received from
> +the device. It supports blocking operations, poll/select and fasync
> +operation modes. You must read 4 bytes from the device.
> +The result is number of free-fall interrupts since the last successful read.
> +
> +
> +Axes orientation
> +----------------
> +
> +For better compatibility between the various laptops. The values reported by
> +the accelerometer are converted into a "standard" organisation of the axes
> +(aka "can play neverball out of the box"):
> + * When the laptop is horizontal the position reported is about 0 for X and Y
> +and a positive value for Z
> + * If the left side is elevated, X increases (becomes positive)
> + * If the front side (where the touchpad is) is elevated, Y decreases (becomes negative)
> + * If the laptop is put upside-down, Z becomes negative
> +
> +If your laptop model is not recognized (cf "dmesg"), you can send an email to the
> +authors to add it to the database. When reporting a new laptop, please include
> +the output of "dmidecode" plus the value of /sys/devices/platform/mdps/position
> +in these four cases.
> +
> +
> +Example userspace code for reading the accel device
> +---------------------------------------------------
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +int main(int argc, char* argv[])
> +{
> + int fd, ret;
> + uint32_t count;
> +
> + fd = open("/dev/accel", O_RDONLY);
> + if (fd < 0) {
> + perror("open");
> + return EXIT_FAILURE;
> + }
> +
> + for (;;) {
> + ret = read(fd, &count, sizeof(count));
> + if (ret != sizeof(count)) {
> + perror("read");
> + break;
> + }
> +
> + printf("Got %d free-fall interrupts\n", count);
> + }
> +
> + close(fd);
> + return EXIT_SUCCESS;
> +}
> diff -Nrubp linux-2.6.25.4_orig/drivers/hwmon/Kconfig linux-2.6.25.4/drivers/hwmon/Kconfig
> --- linux-2.6.25.4_orig/drivers/hwmon/Kconfig 2008-05-28 11:09:04.000000000 +0300
> +++ linux-2.6.25.4/drivers/hwmon/Kconfig 2008-05-28 11:19:01.000000000 +0300
> @@ -760,6 +760,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 && INPUT && X86
> + default n
> + help
> + This driver provides support for the HP Mobile Data Protection
> + System 3D (mdps), which is an accelerometer. HP nc6420, nw9440, nx9420 and
> + nc2510 are 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 accel 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.
> +
> config SENSORS_APPLESMC
> tristate "Apple SMC (Motion sensor, light sensor, keyboard backlight)"
> depends on INPUT && X86
> diff -Nrubp linux-2.6.25.4_orig/drivers/hwmon/Makefile linux-2.6.25.4/drivers/hwmon/Makefile
> --- linux-2.6.25.4_orig/drivers/hwmon/Makefile 2008-05-28 11:09:04.000000000 +0300
> +++ linux-2.6.25.4/drivers/hwmon/Makefile 2008-05-28 11:13:48.000000000 +0300
> @@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_FSCPOS) += fscpos.o
> obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
> +obj-$(CONFIG_SENSORS_MDPS) += mdps.o
> obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o
> obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o
> obj-$(CONFIG_SENSORS_IT87) += it87.o
> diff -Nrubp linux-2.6.25.4_orig/drivers/hwmon/mdps.c linux-2.6.25.4/drivers/hwmon/mdps.c
> --- linux-2.6.25.4_orig/drivers/hwmon/mdps.c 1970-01-01 02:00:00.000000000 +0200
> +++ linux-2.6.25.4/drivers/hwmon/mdps.c 2008-05-28 11:15:38.000000000 +0300
> @@ -0,0 +1,783 @@
> +/*
> + * mdps.c - HP Mobile Data Protection System 3D ACPI driver
> + *
> + * Copyright (C) 2007-2008 Yan Burman
> + * Copyright (C) 2008 Eric Piel
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +/*
> + * 30/12/2006
> + * Added support for NX9420 and hdaps compatibility mode.
> + * Thanks to Jonas Majauskas for testing this on NX9420
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/dmi.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/miscdevice.h>
> +#include <linux/wait.h>
> +#include <linux/poll.h>
> +#include <linux/freezer.h>
> +#include <linux/version.h>
> +#include <linux/uaccess.h>
> +
> +#include <acpi/acpi_drivers.h>
> +
> +#include <asm/atomic.h>
> +
> +#define VERSION "0.92"
> +
> +#define DRIVER_NAME "mdps"
> +#define ACPI_MDPS_CLASS "accelerometer"
> +#define ACPI_MDPS_ID "HPQ0004"
> +
> +/* The actual chip is STMicroelectronics LIS3LV02DL or LIS3LV02DQ
> + * that seems to be connected via SPI */
> +
> +#define MDPS_WHO_AM_I 0x0F /*r 00111010 */
> +#define MDPS_OFFSET_X 0x16 /*rw */
> +#define MDPS_OFFSET_Y 0x17 /*rw */
> +#define MDPS_OFFSET_Z 0x18 /*rw */
> +#define MDPS_GAIN_X 0x19 /*rw */
> +#define MDPS_GAIN_Y 0x1A /*rw */
> +#define MDPS_GAIN_Z 0x1B /*rw */
> +#define MDPS_CTRL_REG1 0x20 /*rw 00000111 */
> +#define MDPS_CTRL_REG2 0x21 /*rw 00000000 */
> +#define MDPS_CTRL_REG3 0x22 /*rw 00001000 */
> +#define MDPS_HP_FILTER RESET 0x23 /*r */
> +#define MDPS_STATUS_REG 0x27 /*rw 00000000 */
> +#define MDPS_OUTX_L 0x28 /*r */
> +#define MDPS_OUTX_H 0x29 /*r */
> +#define MDPS_OUTY_L 0x2A /*r */
> +#define MDPS_OUTY_H 0x2B /*r */
> +#define MDPS_OUTZ_L 0x2C /*r */
> +#define MDPS_OUTZ_H 0x2D /*r */
> +#define MDPS_FF_WU_CFG 0x30 /*rw 00000000 */
> +#define MDPS_FF_WU_SRC 0x31 /*rw 00000000 */
> +#define MDPS_FF_WU_ACK 0x32 /*r */
> +#define MDPS_FF_WU_THS_L 0x34 /*rw 00000000 */
> +#define MDPS_FF_WU_THS_H 0x35 /*rw 00000000 */
> +#define MDPS_FF_WU_DURATION 0x36 /*rw 00000000 */
> +#define MDPS_DD_CFG 0x38 /*rw 00000000 */
> +#define MDPS_DD_SRC 0x39 /*rw 00000000 */
> +#define MDPS_DD_ACK 0x3A /*r */
> +#define MDPS_DD_THSI_L 0x3C /*rw 00000000 */
> +#define MDPS_DD_THSI_H 0x3D /*rw 00000000 */
> +#define MDPS_DD_THSE_L 0x3E /*rw 00000000 */
> +#define MDPS_DD_THSE_H 0x3F /*rw 00000000 */
> +
> +#define MDPS_ID 0x3A /* MDPS_WHO_AM_I */
> +#define MDPS_FS (1<<7) /* MDPS_CTRL_REG2 : Full Scale selection */
> +#define MDPS_BDU (1<<6) /* MDPS_CTRL_REG2 : Block Data Update */
> +
> +/* joystick device poll interval in milliseconds */
> +#define MDPS_POLL_INTERVAL 30
> +
> +/* Maximum value our axis may get for the input device */
> +#define MDPS_MAX_VAL 2048
> +
> +static unsigned int power_off;
> +module_param(power_off, bool, S_IRUGO);
> +MODULE_PARM_DESC(power_off, "Turn off device on module load");
> +
> +struct axis_conversion {
> + s8 x;
> + s8 y;
> + s8 z;
> +};
> +
> +struct acpi_mdps {
> + struct acpi_device *device; /* The ACPI device */
> + 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 zcalib; /* calibrated null value for z */
> + int is_on; /* whether the device is on or off */
> + struct platform_device *pdev; /* platform device */
> + atomic_t count; /* interrupt count after last read */
> + struct fasync_struct *async_queue;
> + atomic_t available; /* whether the device is open */
> + wait_queue_head_t misc_wait; /* Wait queue for the misc device */
> + /* conversion between hw axis and logical ones */
> + struct axis_conversion ac;
> +};
> +
> +static struct acpi_mdps mdps;
> +
> +static int mdps_remove_fs(void);
> +static int mdps_add_fs(struct acpi_device *device);
> +static void mdps_joystick_enable(void);
> +static void mdps_joystick_disable(void);
> +
> +static struct acpi_device_id mdps_device_ids[] = {
> + {ACPI_MDPS_ID, 0},
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, mdps_device_ids);
> +
> +/** 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
> + */
> +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);
> +}
> +
> +/** ACPI ALRD method: read a register
> + * @param handle the handle of the device
> + * @param reg the register to read
> + * @param[out] ret result of the operation
> + * @return AE_OK on success
> + */
> +static acpi_status mdps_ALRD(acpi_handle handle, int reg,
> + unsigned long *ret)
> +{
> + union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> + struct acpi_object_list args = { 1, &arg0 };
> +
> + arg0.integer.value = reg;
> +
> + return acpi_evaluate_integer(handle, "ALRD", &args, ret);
> +}
> +
> +/** ACPI _INI method: initialize the device.
> + * @param handle the handle of the device
> + * @return 0 on success
> + */
> +static inline acpi_status mdps__INI(acpi_handle handle)
> +{
> + return acpi_evaluate_object(handle, METHOD_NAME__INI, NULL, NULL);
> +}
> +
> +/** ACPI ALWR method: write to a register
> + * @param handle the handle of the device
> + * @param reg the register to write to
> + * @param val the value to write
> + * @param[out] ret result of the operation
> + * @return AE_OK on success
> + */
> +static acpi_status mdps_ALWR(acpi_handle handle, int reg, int val,
> + unsigned long *ret)
> +{
> + union acpi_object in_obj[2];
> + struct acpi_object_list args = { 2, in_obj };
> +
> + in_obj[0].type = ACPI_TYPE_INTEGER;
> + in_obj[0].integer.value = reg;
> + in_obj[1].type = ACPI_TYPE_INTEGER;
> + in_obj[1].integer.value = val;
> +
> + return acpi_evaluate_integer(handle, "ALWR", &args, ret);
> +}
> +
> +static int mdps_read_axis(acpi_handle handle, int lo_const, int hi_const)
> +{
> + unsigned long lo_val, hi_val;
> + mdps_ALRD(handle, lo_const, &lo_val);
> + mdps_ALRD(handle, hi_const, &hi_val);
> + return mdps_glue_bytes(hi_val, lo_val);
> +}
> +
> +static inline int mdps_get_axis(s8 axis, int hw_values[3])
> +{
> + if (axis > 0)
> + return hw_values[axis - 1];
> + else
> + return -hw_values[-axis - 1];
> +}
> +
> +/** Get X, Y and Z axis values from the accelerometer
> + * @param handle the handle to the device
> + * @param[out] x where to store the X axis value
> + * @param[out] y where to store the Y axis value
> + * @param[out] z where to store the Z axis value
> + * @note 40Hz input device can eat up about 10% CPU at 800MHZ
> + */
> +static void mdps_get_xyz(acpi_handle handle, int *x, int *y, int *z)
> +{
> + int position[3];
> + position[0] = mdps_read_axis(handle, MDPS_OUTX_L, MDPS_OUTX_H);
> + position[1] = mdps_read_axis(handle, MDPS_OUTY_L, MDPS_OUTY_H);
> + position[2] = mdps_read_axis(handle, MDPS_OUTZ_L, MDPS_OUTZ_H);
> +
> + *x = mdps_get_axis(mdps.ac.x, position);
> + *y = mdps_get_axis(mdps.ac.y, position);
> + *z = mdps_get_axis(mdps.ac.z, position);
> +}
> +
> +/** Kthread polling function
> + * @param data unused - here to conform to threadfn prototype
> + */
> +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;
> +}
> +
> +static inline void mdps_poweroff(acpi_handle handle)
> +{
> + unsigned long ret;
> + mdps.is_on = 0;
> + /* disable X,Y,Z axis and power down */
> + mdps_ALWR(handle, MDPS_CTRL_REG1, 0x00, &ret);
> +}
> +
> +static inline void mdps_poweron(acpi_handle handle)
> +{
> + unsigned long val, retw;
> +
> + mdps.is_on = 1;
> + mdps__INI(handle);
> + /*
> + * Change to Block Data Update mode: LSB and MSB values are not updated
> + * until both have been read. So the value read will always be correct.
> + */
> + mdps_ALRD(handle, MDPS_CTRL_REG2, &val);
> + val |= MDPS_BDU;
> + mdps_ALWR(handle, MDPS_CTRL_REG2, val, &retw);
> +}
> +
> +#ifdef CONFIG_PM
> +static int mdps_suspend(struct acpi_device *device, pm_message_t state)
> +{
> + /* make sure the device is off when we suspend */
> + mdps_poweroff(mdps.device->handle);
> + return 0;
> +}
> +#endif
> +
> +static int mdps_resume(struct acpi_device *device)
> +{
> + /* make sure the device went online */
> + mdps_poweron(mdps.device->handle);
> + return 0;
> +}
> +
> +static irqreturn_t mdps_irq(int irq, void *dev_id)
> +{
> + atomic_inc(&mdps.count);
> +
> + wake_up_interruptible(&mdps.misc_wait);
> + kill_fasync(&mdps.async_queue, SIGIO, POLL_IN);
> +
> + return IRQ_HANDLED;
> +}
> +
> +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 */
> + }
> +
> + 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 int mdps_misc_release(struct inode *inode, struct file *file)
> +{
> + fasync_helper(-1, file, 0, &mdps.async_queue);
> + free_irq(mdps.irq, mdps_irq);
> + atomic_inc(&mdps.available); /* release the device */
> + 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 (; ; ) {
> + 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 unsigned int mdps_misc_poll(struct file *file, poll_table *wait)
> +{
> + poll_wait(file, &mdps.misc_wait, wait);
> + if (atomic_read(&mdps.count))
> + return POLLIN | POLLRDNORM;
> + return 0;
> +}
> +
> +static int mdps_misc_fasync(int fd, struct file *file, int on)
> +{
> + return fasync_helper(fd, file, on, &mdps.async_queue);
> +}
> +
> +static const struct file_operations mdps_misc_fops = {
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .read = mdps_misc_read,
> + .open = mdps_misc_open,
> + .release = mdps_misc_release,
> + .poll = mdps_misc_poll,
> + .fasync = mdps_misc_fasync,
> +};
> +
> +static struct miscdevice mdps_misc_device = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "accel",
> + .fops = &mdps_misc_fops,
> +};
> +
> +static acpi_status
> +mdps_get_resource(struct acpi_resource *resource, void *context)
> +{
> + if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ) {
> + struct acpi_resource_extended_irq *irq;
> + u32 *device_irq = context;
> +
> + irq = &resource->data.extended_irq;
> + *device_irq = irq->interrupts[0];
> + }
> +
> + return AE_OK;
> +}
> +
> +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");
> +}
> +
> +static int mdps_dmi_matched(const struct dmi_system_id *dmi)
> +{
> + mdps.ac = *(struct axis_conversion *)dmi->driver_data;
> + printk(KERN_INFO "mdps: hardware type %s found.\n", dmi->ident);
> +
> + return 1;
> +}
> +
> +
> +/* Represents, for each axis seen by userspace, the corresponding hw axis (+1).
> + * If the value is negative, the opposite of the hw value is used. */
> +static struct axis_conversion mdps_axis_normal = {1, 2, 3};
> +static struct axis_conversion mdps_axis_y_inverted = {1, -2, 3};
> +static struct axis_conversion mdps_axis_x_inverted = {-1, 2, 3};
> +
> +static struct dmi_system_id mdps_dmi_ids[] = {
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NC64x0",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nc64"),
> + },
> + .driver_data = &mdps_axis_x_inverted
> + },
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NX9420",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx9420"),
> + },
> + .driver_data = &mdps_axis_x_inverted
> + },
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NW9440",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nw9440"),
> + },
> + .driver_data = &mdps_axis_x_inverted
> + },
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NC2510",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq 2510"),
> + },
> + .driver_data = &mdps_axis_y_inverted
> + },
> + { NULL, }
> +/* Laptop models without axis info (yet):
> + * "NC84x0" "HP Compaq nc84"
> + * "NC651xx" "HP Compaq 651"
> + * "NC671xx" "HP Compaq 671"
> + * "NC6910" "HP Compaq 6910"
> + * HP Compaq 8510x Notebook PC / Mobile Workstation
> + * HP Compaq 8710x Notebook PC / Mobile Workstation
> + * "NC2400" "HP Compaq nc2400"
> + * "NX74x0" "HP Compaq nx74"
> + * "NX6325" "HP Compaq nx6325"
> + * "NC4400" "HP Compaq nc4400"
> + */
> +};
> +
> +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);
> + 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);
> +
> + ret = misc_register(&mdps_misc_device);
> + if (ret)
> + printk(KERN_ERR "mdps: misc_register failed\n");
> +
> + return 0;
> +}
> +
> +static int mdps_remove(struct acpi_device *device, int type)
> +{
> + if (!device)
> + return -EINVAL;
> +
> + if (mdps.irq)
> + misc_deregister(&mdps_misc_device);
> +
> + mdps_joystick_disable();
> +
> + return mdps_remove_fs();
> +}
> +
> +static inline void mdps_calibrate_joystick(void)
> +{
> + mdps_get_xyz(mdps.device->handle, &mdps.xcalib, &mdps.ycalib,
> + &mdps.zcalib);
> +}
> +
> +static int mdps_joystick_open(struct input_dev *dev)
> +{
> + mdps.kthread = kthread_run(mdps_input_kthread, NULL, "kmdps");
> + if (IS_ERR(mdps.kthread))
> + return PTR_ERR(mdps.kthread);
> +
> + return 0;
> +}
> +
> +static void mdps_joystick_close(struct input_dev *dev)
> +{
> + kthread_stop(mdps.kthread);
> +}
> +
> +static void mdps_joystick_enable(void)
> +{
> + if (mdps.idev)
> + return;
> +
> + mdps.idev = input_allocate_device();
> + if (!mdps.idev)
> + return;
> +
> + 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;
> + }
> +}
> +
> +static void mdps_joystick_disable(void)
> +{
> + if (!mdps.idev)
> + return;
> +
> + input_unregister_device(mdps.idev);
> + mdps.idev = NULL;
> +}
> +
> +/* Sysfs stuff */
> +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);
> +
> + 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"));
> +}
> +
> +static ssize_t mdps_calibrate_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "(%d,%d,%d)\n", mdps.xcalib, mdps.ycalib,
> + mdps.zcalib);
> +}
> +
> +static ssize_t mdps_calibrate_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + mdps_calibrate_joystick();
> + return count;
> +}
> +
> +static ssize_t mdps_rate_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + unsigned long ctrl;
> + int rate = 0;
> +
> + 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;
> +
> + mdps.is_on = state;
> +
> + if (mdps.is_on)
> + mdps_poweron(mdps.device->handle);
> + else
> + mdps_poweroff(mdps.device->handle);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(position, S_IRUGO, mdps_position_show, NULL);
> +static DEVICE_ATTR(calibrate, S_IRUGO|S_IWUSR, mdps_calibrate_show,
> + mdps_calibrate_store);
> +static DEVICE_ATTR(rate, S_IRUGO, mdps_rate_show, NULL);
> +static DEVICE_ATTR(state, S_IRUGO|S_IWUSR, mdps_state_show, mdps_state_store);
> +
> +static struct attribute *mdps_attributes[] = {
> + &dev_attr_position.attr,
> + &dev_attr_calibrate.attr,
> + &dev_attr_rate.attr,
> + &dev_attr_state.attr,
> + NULL
> +};
> +
> +static struct attribute_group mdps_attribute_group = {
> + .attrs = mdps_attributes
> +};
> +
> +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);
> +
> + return sysfs_create_group(&mdps.pdev->dev.kobj, &mdps_attribute_group);
> +}
> +
> +static int mdps_remove_fs(void)
> +{
> + sysfs_remove_group(&mdps.pdev->dev.kobj, &mdps_attribute_group);
> + platform_device_unregister(mdps.pdev);
> + return 0;
> +}
> +
> +static struct acpi_driver mdps_driver = {
> + .name = DRIVER_NAME,
> + .class = ACPI_MDPS_CLASS,
> + .ids = mdps_device_ids,
> + .ops = {
> + .add = mdps_add,
> + .remove = mdps_remove,
> +#ifdef CONFIG_PM
> + .suspend = mdps_suspend,
> + .resume = mdps_resume
> +#endif
> + }
> +};
> +
> +static int __init mdps_init_module(void)
> +{
> + int ret;
> +
> + if (acpi_disabled)
> + return -ENODEV;
> +
> + ret = acpi_bus_register_driver(&mdps_driver);
> + if (ret < 0)
> + return ret;
> +
> + printk(KERN_INFO "mdps version " VERSION " loaded.\n");
> +
> + return 0;
> +}
> +
> +static void __exit mdps_exit_module(void)
> +{
> + acpi_bus_unregister_driver(&mdps_driver);
> +}
> +
> +MODULE_DESCRIPTION("HP three-axis digital accelerometer ACPI driver");
> +MODULE_AUTHOR("Yan Burman (burman.yan@gmail.com)");
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
> +
> +module_init(mdps_init_module);
> +module_exit(mdps_exit_module);
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 19:24 ` [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review! Éric Piel
@ 2008-06-04 20:58 ` Jean Delvare
2008-06-04 22:57 ` Éric Piel
2008-06-04 23:05 ` Pavel Machek
2008-06-04 23:03 ` Pavel Machek
2008-06-06 14:27 ` Dmitry Torokhov
2 siblings, 2 replies; 27+ messages in thread
From: Jean Delvare @ 2008-06-04 20:58 UTC (permalink / raw)
To: Éric Piel
Cc: Andrew Morton, pavel, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
Hi Eric,
On Wed, 04 Jun 2008 21:24:37 +0200, Éric Piel wrote:
> We haven't received any review for this driver since last week's post by
> Yan :-( That's why I'm adding explicitly as receivers of this email all
> the people who have reviewed the first two previous takes. Apologies for
> the possible multiple receptions.
>
> It would be _really really kind_ if some of you could take some time and
> comment (or simply ACK ;-) ) this code. It adds support for hardware
> which can be found in mostly every HP Compaq laptop less than three
> years old. It's quite worthy.
>
> Mark, as maintainer of the hwmon tree, would accept this driver? Let us
> know if there are things we need to change.
If you didn't receive any answer, my guess is that it's because you're
trying to add this driver to the wrong tree. Why would this driver go
in drivers/hwmon and be reviewed by hwmon folks, when it doesn't expose
a single hwmon attribute to user-space?
I guess you thought putting the driver there would be fine because
that's where the hdaps and ams drivers are, but IMHO the hdaps and ams
drivers should never have been placed in drivers/hwmon. These devices
really aren't hardware monitoring chips is the traditional sense of the
term. They are different chips, serving different purposes and
deserving a totally different interface. They'd better live in a
different subsystem and be maintainer by a different crew with interest
in these devices and hardware to test the drivers.
This discussion thread might sched some light on a possible approach:
http://marc.info/?l=lm-sensors&m=121127824726050&w=2
So my advice is that you don't wait for a review from the hwmon people,
because apparently we don't have much interest in this type of device,
so you'll be waiting forever. You're much better adding the mdps driver
to drivers/misc through Andrew Morton, at least until someone takes
care of creating a subsystem for this type of devices and move all
existing drivers there.
Would someone object to moving the hdaps and ams (and possibly
applesmc) drivers to drivers/misc?
--
Jean Delvare
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 20:58 ` Jean Delvare
@ 2008-06-04 22:57 ` Éric Piel
2008-06-05 6:27 ` Jean Delvare
2008-06-04 23:05 ` Pavel Machek
1 sibling, 1 reply; 27+ messages in thread
From: Éric Piel @ 2008-06-04 22:57 UTC (permalink / raw)
To: Jean Delvare
Cc: Andrew Morton, pavel, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
04-06-08 22:58, Jean Delvare wrote/a écrit:
> Hi Eric,
>
> On Wed, 04 Jun 2008 21:24:37 +0200, Éric Piel wrote:
>> We haven't received any review for this driver since last week's post by
>> Yan :-( That's why I'm adding explicitly as receivers of this email all
>> the people who have reviewed the first two previous takes. Apologies for
>> the possible multiple receptions.
>>
>> It would be _really really kind_ if some of you could take some time and
>> comment (or simply ACK ;-) ) this code. It adds support for hardware
>> which can be found in mostly every HP Compaq laptop less than three
>> years old. It's quite worthy.
>>
>> Mark, as maintainer of the hwmon tree, would accept this driver? Let us
>> know if there are things we need to change.
>
> If you didn't receive any answer, my guess is that it's because you're
> trying to add this driver to the wrong tree. Why would this driver go
> in drivers/hwmon and be reviewed by hwmon folks, when it doesn't expose
> a single hwmon attribute to user-space?
>
> I guess you thought putting the driver there would be fine because
> that's where the hdaps and ams drivers are, but IMHO the hdaps and ams
> drivers should never have been placed in drivers/hwmon. These devices
> really aren't hardware monitoring chips is the traditional sense of the
> term. They are different chips, serving different purposes and
> deserving a totally different interface. They'd better live in a
> different subsystem and be maintainer by a different crew with interest
> in these devices and hardware to test the drivers.
>
> This discussion thread might sched some light on a possible approach:
> http://marc.info/?l=lm-sensors&m=121127824726050&w=2
>
> So my advice is that you don't wait for a review from the hwmon people,
> because apparently we don't have much interest in this type of device,
> so you'll be waiting forever. You're much better adding the mdps driver
> to drivers/misc through Andrew Morton, at least until someone takes
> care of creating a subsystem for this type of devices and move all
> existing drivers there.
Hi Jean,
I understand your argumentation. Indeed, this driver has nothing
specially related to hwmon, excepted that all the other accelerometers
so far have been put in hwmon. I've got nothing against redoing the
patch to move the code to drivers/misc. Andrew, would you accept it?
> Would someone object to moving the hdaps and ams (and possibly
> applesmc) drivers to drivers/misc?
Yeah, applesmc seems a bit less obvious because it also exposes really
some hardware sensors. Any idea how to handle this? Just leave it in
hwmon for now?
So, the rational about moving all these drivers to drivers/misc/ would
be that it shows they have no relation with hwmon, and they are
maintained separately. This would be a temporary place until the
"accelerometer" subsystem exists by it own, right?
If everyone is happy with this idea, I can submit a separate patch to
move them.
See you,
Eric
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 22:57 ` Éric Piel
@ 2008-06-05 6:27 ` Jean Delvare
0 siblings, 0 replies; 27+ messages in thread
From: Jean Delvare @ 2008-06-05 6:27 UTC (permalink / raw)
To: Éric Piel
Cc: Andrew Morton, pavel, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
Hi Eric,
On Thu, 05 Jun 2008 00:57:53 +0200, Éric Piel wrote:
> 04-06-08 22:58, Jean Delvare wrote/a écrit:
> > If you didn't receive any answer, my guess is that it's because you're
> > trying to add this driver to the wrong tree. Why would this driver go
> > in drivers/hwmon and be reviewed by hwmon folks, when it doesn't expose
> > a single hwmon attribute to user-space?
> >
> > I guess you thought putting the driver there would be fine because
> > that's where the hdaps and ams drivers are, but IMHO the hdaps and ams
> > drivers should never have been placed in drivers/hwmon. These devices
> > really aren't hardware monitoring chips is the traditional sense of the
> > term. They are different chips, serving different purposes and
> > deserving a totally different interface. They'd better live in a
> > different subsystem and be maintainer by a different crew with interest
> > in these devices and hardware to test the drivers.
> >
> > This discussion thread might sched some light on a possible approach:
> > http://marc.info/?l=lm-sensors&m=121127824726050&w=2
> >
> > So my advice is that you don't wait for a review from the hwmon people,
> > because apparently we don't have much interest in this type of device,
> > so you'll be waiting forever. You're much better adding the mdps driver
> > to drivers/misc through Andrew Morton, at least until someone takes
> > care of creating a subsystem for this type of devices and move all
> > existing drivers there.
>
> I understand your argumentation. Indeed, this driver has nothing
> specially related to hwmon, excepted that all the other accelerometers
> so far have been put in hwmon. I've got nothing against redoing the
> patch to move the code to drivers/misc. Andrew, would you accept it?
>
> > Would someone object to moving the hdaps and ams (and possibly
> > applesmc) drivers to drivers/misc?
>
> Yeah, applesmc seems a bit less obvious because it also exposes really
> some hardware sensors. Any idea how to handle this? Just leave it in
> hwmon for now?
There are already other drivers in this case:
drivers/thermal/thermal_sys.c, drivers/misc/eeepc-laptop.c,
drivers/misc/thinkpad_acpi.c and drivers/input/touchscreen/ads7846.c.
All these drivers have multiple functions, one of them is hardware
monitoring but that's not the main one, so they didn't go under
drivers/hwmon. The applesmc driver would simply be one more driver in
this case.
Note that drivers/mfd is a possible alternative to drivers/misc for
these multifunction chips.
> So, the rational about moving all these drivers to drivers/misc/ would
> be that it shows they have no relation with hwmon, and they are
> maintained separately. This would be a temporary place until the
> "accelerometer" subsystem exists by it own, right?
> If everyone is happy with this idea, I can submit a separate patch to
> move them.
I'm fine with moving the drivers right now, yes.
--
Jean Delvare
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 20:58 ` Jean Delvare
2008-06-04 22:57 ` Éric Piel
@ 2008-06-04 23:05 ` Pavel Machek
2008-06-04 23:43 ` Éric Piel
1 sibling, 1 reply; 27+ messages in thread
From: Pavel Machek @ 2008-06-04 23:05 UTC (permalink / raw)
To: Jean Delvare
Cc: Éric Piel, Andrew Morton, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
Hi!
> So my advice is that you don't wait for a review from the hwmon people,
> because apparently we don't have much interest in this type of device,
> so you'll be waiting forever. You're much better adding the mdps driver
> to drivers/misc through Andrew Morton, at least until someone takes
> care of creating a subsystem for this type of devices and move all
> existing drivers there.
Just create drivers/accel ... accelerometers are quite common these
days. Going to drivers/misc is okay, too, I guess -- moves are easy
with git ;-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 23:05 ` Pavel Machek
@ 2008-06-04 23:43 ` Éric Piel
2008-06-05 6:29 ` Jean Delvare
0 siblings, 1 reply; 27+ messages in thread
From: Éric Piel @ 2008-06-04 23:43 UTC (permalink / raw)
To: Pavel Machek
Cc: Jean Delvare, Andrew Morton, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
05-06-08 01:05, Pavel Machek wrote/a écrit:
> Hi!
>
>> So my advice is that you don't wait for a review from the hwmon people,
>> because apparently we don't have much interest in this type of device,
>> so you'll be waiting forever. You're much better adding the mdps driver
>> to drivers/misc through Andrew Morton, at least until someone takes
>> care of creating a subsystem for this type of devices and move all
>> existing drivers there.
>
> Just create drivers/accel ... accelerometers are quite common these
> days. Going to drivers/misc is okay, too, I guess -- moves are easy
> with git ;-).
Yes, why not do one little step to the correct direction now and create
drivers/accel, it will likely be the chosen name anyway! That will not
yet be a "real" subsystem, just a place to put all the related drivers
already present together.
Anyone against this idea?
Eric
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 23:43 ` Éric Piel
@ 2008-06-05 6:29 ` Jean Delvare
2008-06-05 9:38 ` [lm-sensors] " Jonathan Cameron
0 siblings, 1 reply; 27+ messages in thread
From: Jean Delvare @ 2008-06-05 6:29 UTC (permalink / raw)
To: Éric Piel
Cc: Pavel Machek, Andrew Morton, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
On Thu, 05 Jun 2008 01:43:46 +0200, Éric Piel wrote:
> 05-06-08 01:05, Pavel Machek wrote/a écrit:
> >> So my advice is that you don't wait for a review from the hwmon people,
> >> because apparently we don't have much interest in this type of device,
> >> so you'll be waiting forever. You're much better adding the mdps driver
> >> to drivers/misc through Andrew Morton, at least until someone takes
> >> care of creating a subsystem for this type of devices and move all
> >> existing drivers there.
> >
> > Just create drivers/accel ... accelerometers are quite common these
> > days. Going to drivers/misc is okay, too, I guess -- moves are easy
> > with git ;-).
>
> Yes, why not do one little step to the correct direction now and create
> drivers/accel, it will likely be the chosen name anyway! That will not
> yet be a "real" subsystem, just a place to put all the related drivers
> already present together.
Well, I would expect all these accelerometer drivers to have a common,
standardized interface, so this should most probably become a real
subsystem in the long run.
> Anyone against this idea?
I am in favor of it.
--
Jean Delvare
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-05 6:29 ` Jean Delvare
@ 2008-06-05 9:38 ` Jonathan Cameron
2008-06-05 20:34 ` Éric Piel
0 siblings, 1 reply; 27+ messages in thread
From: Jonathan Cameron @ 2008-06-05 9:38 UTC (permalink / raw)
To: Jean Delvare
Cc: Éric Piel, Andrew Morton, Torokhov, Burman, LKML, HWMON,
Mark M. Hoffman, Pavel Machek, Yan, Dmitry, Arjan van de Ven
Jean Delvare wrote:
> On Thu, 05 Jun 2008 01:43:46 +0200, Éric Piel wrote:
>> 05-06-08 01:05, Pavel Machek wrote/a écrit:
>>>> So my advice is that you don't wait for a review from the hwmon people,
>>>> because apparently we don't have much interest in this type of device,
>>>> so you'll be waiting forever. You're much better adding the mdps driver
>>>> to drivers/misc through Andrew Morton, at least until someone takes
>>>> care of creating a subsystem for this type of devices and move all
>>>> existing drivers there.
>>> Just create drivers/accel ... accelerometers are quite common these
>>> days. Going to drivers/misc is okay, too, I guess -- moves are easy
>>> with git ;-).
>> Yes, why not do one little step to the correct direction now and create
>> drivers/accel, it will likely be the chosen name anyway! That will not
>> yet be a "real" subsystem, just a place to put all the related drivers
>> already present together.
>
> Well, I would expect all these accelerometer drivers to have a common,
> standardized interface, so this should most probably become a real
> subsystem in the long run.
>
>> Anyone against this idea?
>
> I am in favor of it.
The upshot of the discussion Jean referenced was that the there were a
number of other devices / sensors that will form part of the same system.
For an initial bash I'm throwing in a couple of accelerometers, ADC's and
probably one or two other sensors as well. Haven't come up with a good
name as yet, but the point is that accelerometers would be just part of
the relevant subsystem.
Based on one suggestion (and current layout in my source tree),
drivers/industrialio/accelerometer
drivers/industrialio/adc
and crucially for all those weird and wonderful sensors,
drivers/industrialio/misc
The reason for making it an io system is that many chips combine ADCs and
DACs. Also suggestions for alternative names would be most welcome.
Still, it may take some time to iron out the various kinks in this system
and get it fully tested, so may be worth putting stuff elsewhere for now
with the intention to move it as and when the more unified stuff has been
written.
Also, the LI3L02DQ (very similar to the chip you are using) is one of my
test chips, so combining the two drivers should be straight forward.
Also the driver I have is SPI only, so this may give us an easy way to
test a combined driver.
Jonathan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-05 9:38 ` [lm-sensors] " Jonathan Cameron
@ 2008-06-05 20:34 ` Éric Piel
2008-06-05 22:27 ` Pau Oliva Fora
2008-06-06 8:51 ` Jonathan Cameron
0 siblings, 2 replies; 27+ messages in thread
From: Éric Piel @ 2008-06-05 20:34 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jean Delvare, Andrew Morton, Torokhov, Burman, LKML, HWMON,
Mark M. Hoffman, Pavel Machek, Yan, Dmitry, Arjan van de Ven
05-06-08 11:38, Jonathan Cameron wrote/a écrit:
:
> For an initial bash I'm throwing in a couple of accelerometers, ADC's and
> probably one or two other sensors as well. Haven't come up with a good
> name as yet, but the point is that accelerometers would be just part of
> the relevant subsystem.
>
> Based on one suggestion (and current layout in my source tree),
> drivers/industrialio/accelerometer
> drivers/industrialio/adc
> and crucially for all those weird and wonderful sensors,
> drivers/industrialio/misc
>
> The reason for making it an io system is that many chips combine ADCs and
> DACs. Also suggestions for alternative names would be most welcome.
Great to hear that you have started the new subsystem!
It's also nice that you are still hesitant about the name because
"industrialio" sounds for me too... let's say... industrial ;-) For
accelerometers found in laptops and mobile phones, it will sound strange
(although a name is just a name). Maybe "physicsio" (physics as "the
observation of the matter and its motion")?
> Still, it may take some time to iron out the various kinks in this system
> and get it fully tested, so may be worth putting stuff elsewhere for now
> with the intention to move it as and when the more unified stuff has been
> written.
Yes, so I'll keep my plan of first submitting the driver with the
current interface, and latter updating the interface to the new
subsystem when you have stabilised its interface.
BTW, just by curiosity, is your tree available publicly somewhere?
> Also, the LI3L02DQ (very similar to the chip you are using) is one of my
> test chips, so combining the two drivers should be straight forward.
> Also the driver I have is SPI only, so this may give us an easy way to
> test a combined driver.
For now I'll go back to the drawing board (with Yan) in order to address
the comments received, all very insightful. I'll also go around the
other versions of the driver to pick up the nice parts. Looking at the
openmoko driver, I finally understood how to avoid polling. It will
likely be worthy :-)
In addition, I'll try to make the main functions as independent as
possible of the platform, hoping that we can then easily merge the SPI
and the ACPI versions together.
See you,
Eric
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-05 20:34 ` Éric Piel
@ 2008-06-05 22:27 ` Pau Oliva Fora
2008-06-06 14:24 ` Dmitry Torokhov
2008-06-06 14:56 ` Eric Piel
2008-06-06 8:51 ` Jonathan Cameron
1 sibling, 2 replies; 27+ messages in thread
From: Pau Oliva Fora @ 2008-06-05 22:27 UTC (permalink / raw)
To: Éric Piel
Cc: Jonathan Cameron, Jean Delvare, Andrew Morton, Torokhov, Burman,
LKML, HWMON, Mark M. Hoffman, Pavel Machek, Yan, Dmitry,
Arjan van de Ven
From: Pau Oliva Fora <pau@eslack.org>
Add support for HTC Shift UMPC G-Sensor LIS3LV02DL accelerometer.
Signed-off-by: Pau Oliva Fora <pau@eslack.org>
---
Éric Piel wrote:
> 05-06-08 11:38, Jonathan Cameron wrote/a écrit:
>> Also, the LI3L02DQ (very similar to the chip you are using) is one of my
>> test chips, so combining the two drivers should be straight forward.
>> Also the driver I have is SPI only, so this may give us an easy way to
>> test a combined driver.
> For now I'll go back to the drawing board (with Yan) in order to address
> the comments received, all very insightful. I'll also go around the
> other versions of the driver to pick up the nice parts. Looking at the
> openmoko driver, I finally understood how to avoid polling. It will
> likely be worthy :-)
> In addition, I'll try to make the main functions as independent as
> possible of the platform, hoping that we can then easily merge the SPI
> and the ACPI versions together.
Here's an i2c version of the driver for the LIS3LV02DL accelerometer,
please try to combine it too, so we'll end up having ACPI, SPI and I2C.
Patch against linux-2.6.26-rc5, should apply clean on other versions too.
diff -uprN linux-2.6.26-rc5/drivers/input/joystick/htcgsensor.c linux/drivers/input/joystick/htcgsensor.c
--- linux-2.6.26-rc5/drivers/input/joystick/htcgsensor.c 1970-01-01 01:00:00.000000000 +0100
+++ linux/drivers/input/joystick/htcgsensor.c 2008-06-05 23:56:58.000000000 +0200
@@ -0,0 +1,303 @@
+/*
+ * HTC Shift G-Sensor Joystick driver
+ * The G-Sensor chip is a STMicroelectronics LIS3LV02DL connected via i2c
+ *
+ * Copyright (C) 2008 Pau Oliva Fora <pof@eslack.org>
+ *
+ * Based on:
+ * mdps.c - HP Mobile Data Protection System 3D ACPI driver
+ * ams.c - Apple Motion Sensor driver
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/input-polldev.h>
+#include <linux/dmi.h>
+
+#define GSENSOR_WHO_AM_I 0x0F /*r 00111010 */
+#define GSENSOR_CTRL_REG1 0x20 /*rw 00000111 */
+#define GSENSOR_CTRL_REG2 0x21 /*rw 00000000 */
+#define GSENSOR_CTRL_REG3 0x22 /*rw 00001000 */
+#define GSENSOR_OUTX_L 0x28 /*r */
+#define GSENSOR_OUTX_H 0x29 /*r */
+#define GSENSOR_OUTY_L 0x2A /*r */
+#define GSENSOR_OUTY_H 0x2B /*r */
+#define GSENSOR_OUTZ_L 0x2C /*r */
+#define GSENSOR_OUTZ_H 0x2D /*r */
+
+#define GSENSOR_ID 0x3A /* GSENSOR_WHO_AM_I */
+
+#define AXIS_MAX 16384
+
+MODULE_DESCRIPTION("HTC Shift G-Sensor driver");
+MODULE_AUTHOR("Pau Oliva Fora <pof@eslack.org>");
+MODULE_LICENSE("GPL");
+
+static unsigned int invert_x;
+module_param(invert_x, bool, 0644);
+MODULE_PARM_DESC(invert_x, "Invert input data on X axis");
+
+static unsigned int invert_y;
+module_param(invert_y, bool, 0644);
+MODULE_PARM_DESC(invert_y, "Invert input data on Y axis");
+
+static unsigned int swap_xy;
+module_param(swap_xy, bool, 0644);
+MODULE_PARM_DESC(swap_xy, "Swap X and Y axis");
+
+static struct dmi_system_id __initdata htcshift_dmi_table[] = {
+ {
+ .ident = "Shift",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "High Tech Computer Corp"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Shift"),
+ },
+ },
+ { }
+};
+
+struct gsensor {
+ char init; /* has it been initialized ? */
+ int bus; /* i2c bus */
+ int address; /* i2c address */
+ struct i2c_client client; /* i2c client */
+ struct input_polled_dev *idev; /* input device */
+ int xcalib; /* x calibrated null value */
+ int ycalib; /* y calibrated null value */
+ int zcalib; /* z calibrated null value */
+};
+
+static struct gsensor gsensor;
+
+static int gsensor_attach(struct i2c_adapter *adapter);
+static int gsensor_detach(struct i2c_adapter *adapter);
+#ifdef CONFIG_PM
+static int gsensor_suspend(struct i2c_client *adapter, pm_message_t state);
+static int gsensor_resume(struct i2c_client *adapter);
+#endif
+
+static struct i2c_driver gsensor_driver = {
+ .driver = {
+ .name = "gsensor",
+ .owner = THIS_MODULE,
+ },
+ .attach_adapter = gsensor_attach,
+ .detach_adapter = gsensor_detach,
+#ifdef CONFIG_PM
+ .suspend = gsensor_suspend,
+ .resume = gsensor_resume,
+#endif
+};
+
+static inline int gsensor_read(int reg)
+{
+ return i2c_smbus_read_byte_data(&gsensor.client, reg);
+}
+
+static inline int gsensor_write(int reg, int value)
+{
+ return i2c_smbus_write_byte_data(&gsensor.client, reg, value);
+}
+
+static void gsensor_sensors(short *x, short *y, short *z)
+{
+ int xh, xl, yh, yl, zh, zl;
+ short tmp;
+
+ xh = gsensor_read(GSENSOR_OUTX_L);
+ xl = gsensor_read(GSENSOR_OUTX_H);
+ *x = (xh << 8) | (xl);
+
+ yh = gsensor_read(GSENSOR_OUTY_L);
+ yl = gsensor_read(GSENSOR_OUTY_H);
+ *y = (yh << 8) | (yl);
+
+ zh = gsensor_read(GSENSOR_OUTZ_L);
+ zl = gsensor_read(GSENSOR_OUTZ_H);
+ *z = (zh << 8) | (zl);
+
+ if (swap_xy) {
+ tmp = *x;
+ *x = *y;
+ *y = tmp;
+ }
+
+ /* printk(KERN_DEBUG "gsensor: Sensors (%d, %d, %d)\n", x, y, z); */
+}
+
+static void gsensor_idev_poll(struct input_polled_dev *dev)
+{
+ struct input_dev *idev = dev->input;
+ short x, y, z;
+
+ gsensor_sensors(&x, &y, &z);
+
+ x -= gsensor.xcalib;
+ y -= gsensor.ycalib;
+ z -= gsensor.zcalib;
+
+ input_report_abs(idev, ABS_X, invert_x ? -x : x);
+ input_report_abs(idev, ABS_Y, invert_y ? -y : y);
+ input_report_abs(idev, ABS_Z, z);
+ input_sync(idev);
+}
+
+static void gsensor_joystick_enable(void)
+{
+ short x, y, z;
+ struct input_dev *input;
+
+ if (gsensor.idev) {
+ printk(KERN_INFO "gsensor: joystick already enabled\n");
+ return;
+ }
+
+ gsensor_sensors(&x, &y, &z);
+ gsensor.xcalib = x;
+ gsensor.ycalib = y;
+ gsensor.zcalib = z;
+
+ gsensor.idev = input_allocate_polled_device();
+ if (!gsensor.idev) {
+ printk(KERN_INFO "gsensor: can't allocate input dev\n");
+ return;
+ }
+
+ gsensor.idev->poll = gsensor_idev_poll;
+ gsensor.idev->poll_interval = 30;
+
+ input = gsensor.idev->input;
+ input->name = "HTC G-Sensor";
+ input->id.bustype = BUS_I2C;
+
+ input_set_abs_params(input, ABS_X, -AXIS_MAX, AXIS_MAX, 9, 0);
+ input_set_abs_params(input, ABS_Y, -AXIS_MAX, AXIS_MAX, 9, 0);
+ input_set_abs_params(input, ABS_Z, -AXIS_MAX, AXIS_MAX, 9, 0);
+ set_bit(EV_ABS, input->evbit);
+
+ if (input_register_polled_device(gsensor.idev)) {
+ printk(KERN_INFO "gsensor: can't register input dev\n");
+ input_free_polled_device(gsensor.idev);
+ gsensor.idev = NULL;
+ return;
+ }
+}
+
+static void gsensor_joystick_disable(void)
+{
+ if (gsensor.idev) {
+ input_unregister_polled_device(gsensor.idev);
+ input_free_polled_device(gsensor.idev);
+ gsensor.idev = NULL;
+ }
+}
+
+static inline void gsensor_poweron(void)
+{
+ /* device on, decimate by 512, X, Y and Z axis enabled */
+ gsensor_write(GSENSOR_CTRL_REG1, 0x47);
+
+ /* scale +-2g, BDU non continuous, big endian, 3-wire,
+ * alignment 16-bit left */
+ gsensor_write(GSENSOR_CTRL_REG2, 0x63);
+}
+
+
+static inline void gsensor_poweroff(void)
+{
+ /* disable X,Y,Z axis and power off */
+ gsensor_write(GSENSOR_CTRL_REG1, 0x00);
+}
+
+#ifdef CONFIG_PM
+static int gsensor_suspend(struct i2c_client *adapter, pm_message_t state)
+{
+ gsensor_poweroff();
+ return 0;
+}
+
+static int gsensor_resume(struct i2c_client *adapter)
+{
+ gsensor_poweron();
+ return 0;
+}
+#endif
+
+static int gsensor_attach(struct i2c_adapter *adapter)
+{
+ if (gsensor.init)
+ return 0;
+
+ gsensor.client.addr = gsensor.address;
+ gsensor.client.adapter = adapter;
+ gsensor.client.driver = &gsensor_driver;
+ strcpy(gsensor.client.name, "HTC G-Sensor");
+
+ if (gsensor_read(GSENSOR_WHO_AM_I) != GSENSOR_ID) {
+ printk(KERN_INFO "gsensor: invalid device\n");
+ return -ENODEV;
+ }
+
+ gsensor_poweron();
+
+ if (i2c_attach_client(&gsensor.client)) {
+ printk(KERN_INFO "gsensor: i2c attach client failed\n");
+ return -ENODEV;
+ }
+
+ gsensor_joystick_enable();
+ gsensor.init = 1;
+ printk(KERN_INFO "gsensor: HTC G-Sensor enabled\n");
+
+ return 0;
+}
+
+static int gsensor_detach(struct i2c_adapter *adapter)
+{
+ if (!gsensor.init) {
+ printk(KERN_INFO "gsensor: already disabled\n");
+ return 0;
+ }
+
+ gsensor_poweroff();
+ gsensor_joystick_disable();
+ i2c_detach_client(&gsensor.client);
+ gsensor.init = 0;
+
+ printk(KERN_INFO "gsensor: HTC G-Sensor disabled\n");
+
+ return 0;
+}
+
+static int __init gsensor_init(void)
+{
+ gsensor.bus = 0;
+ gsensor.address = 0x1d;
+
+ if (dmi_check_system(htcshift_dmi_table)) {
+ invert_x = 1;
+ invert_y = 1;
+ swap_xy = 1;
+ }
+
+ if (i2c_add_driver(&gsensor_driver) < 0)
+ return -ENODEV;
+
+ return 0;
+}
+
+static void __exit gsensor_exit(void)
+{
+ i2c_del_driver(&gsensor_driver);
+}
+
+module_init(gsensor_init);
+module_exit(gsensor_exit);
diff -uprN linux-2.6.26-rc5/drivers/input/joystick/Kconfig linux/drivers/input/joystick/Kconfig
--- linux-2.6.26-rc5/drivers/input/joystick/Kconfig 2008-06-06 00:04:36.000000000 +0200
+++ linux/drivers/input/joystick/Kconfig 2008-06-06 00:02:46.000000000 +0200
@@ -100,6 +100,16 @@ config JOYSTICK_GUILLEMOT
To compile this driver as a module, choose M here: the
module will be called guillemot.
+config JOYSTICK_HTCGSENSOR
+ tristate "HTC Shift G-Sensor accelerometer"
+ depends on I2C
+ help
+ Say Y here if you have an HTC Shift UMPC and want to use
+ the integrated LIS3LV02DL accelerometer as a joystick.
+
+ To compile this driver as a module, choose M here: the
+ module will be called htcgsensor.
+
config JOYSTICK_INTERACT
tristate "InterAct digital joysticks and gamepads"
select GAMEPORT
diff -uprN linux-2.6.26-rc5/drivers/input/joystick/Makefile linux/drivers/input/joystick/Makefile
--- linux-2.6.26-rc5/drivers/input/joystick/Makefile 2008-06-06 00:04:36.000000000 +0200
+++ linux/drivers/input/joystick/Makefile 2008-06-05 23:56:31.000000000 +0200
@@ -15,6 +15,7 @@ obj-$(CONFIG_JOYSTICK_GF2K) += gf2k.o
obj-$(CONFIG_JOYSTICK_GRIP) += grip.o
obj-$(CONFIG_JOYSTICK_GRIP_MP) += grip_mp.o
obj-$(CONFIG_JOYSTICK_GUILLEMOT) += guillemot.o
+obj-$(CONFIG_JOYSTICK_HTCGSENSOR) += htcgsensor.o
obj-$(CONFIG_JOYSTICK_IFORCE) += iforce/
obj-$(CONFIG_JOYSTICK_INTERACT) += interact.o
obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-05 22:27 ` Pau Oliva Fora
@ 2008-06-06 14:24 ` Dmitry Torokhov
2008-06-06 15:29 ` Pau Oliva Fora
2008-06-06 14:56 ` Eric Piel
1 sibling, 1 reply; 27+ messages in thread
From: Dmitry Torokhov @ 2008-06-06 14:24 UTC (permalink / raw)
To: Pau Oliva Fora
Cc: ?ric Piel, Jonathan Cameron, Jean Delvare, Andrew Morton, Burman,
LKML, HWMON, Mark M. Hoffman, Pavel Machek, Yan, Dmitry,
Arjan van de Ven
Hi,
On Fri, Jun 06, 2008 at 12:27:10AM +0200, Pau Oliva Fora wrote:
> From: Pau Oliva Fora <pau@eslack.org>
>
> Add support for HTC Shift UMPC G-Sensor LIS3LV02DL accelerometer.
>
> Signed-off-by: Pau Oliva Fora <pau@eslack.org>
>
The input piece looks good, however it seens that the input is the
only functionality that this driver exports so I don't see any reason
to make input device registration optional. Or are you planning on
adding more functions to it?
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-06 14:24 ` Dmitry Torokhov
@ 2008-06-06 15:29 ` Pau Oliva Fora
0 siblings, 0 replies; 27+ messages in thread
From: Pau Oliva Fora @ 2008-06-06 15:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: ?ric Piel, Jonathan Cameron, Jean Delvare, Andrew Morton, Burman,
LKML, HWMON, Mark M. Hoffman, Pavel Machek, Yan, Dmitry,
Arjan van de Ven
Dmitry Torokhov wrote:
> Hi,
>
> On Fri, Jun 06, 2008 at 12:27:10AM +0200, Pau Oliva Fora wrote:
>> From: Pau Oliva Fora <pau@eslack.org>
>>
>> Add support for HTC Shift UMPC G-Sensor LIS3LV02DL accelerometer.
>>
>> Signed-off-by: Pau Oliva Fora <pau@eslack.org>
>>
>
> The input piece looks good, however it seens that the input is the
> only functionality that this driver exports so I don't see any reason
> to make input device registration optional. Or are you planning on
> adding more functions to it?
>
I wasn't planning on adding more functions, I posted it so that Éric
can have a look at the i2c part and integrate it in his driver.
Pau Oliva
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-05 22:27 ` Pau Oliva Fora
2008-06-06 14:24 ` Dmitry Torokhov
@ 2008-06-06 14:56 ` Eric Piel
2008-06-06 15:30 ` Pau Oliva Fora
1 sibling, 1 reply; 27+ messages in thread
From: Eric Piel @ 2008-06-06 14:56 UTC (permalink / raw)
To: Pau Oliva Fora
Cc: Éric Piel, Jonathan Cameron, Jean Delvare, Andrew Morton,
Torokhov, Burman, LKML, HWMON, Mark M. Hoffman, Pavel Machek, Yan,
Dmitry, Arjan van de Ven
[-- Attachment #1: Type: text/plain, Size: 494 bytes --]
Pau Oliva Fora wrote:
> Here's an i2c version of the driver for the LIS3LV02DL accelerometer,
> please try to combine it too, so we'll end up having ACPI, SPI and I2C.
Whaoo, you went fast!
As there is a dmi_match, that shouldn't be too hard to incorporate it
nicely :-)
BTW, do you really need the invert_x and invert_y and swap_xy
parameters? Don't all the HTC shift laptops have the sensors soldered
the same way? We've worked hard to remove those parameters in mdps ;-)
See you,
Eric
[-- Attachment #2: E_A_B_Piel.vcf --]
[-- Type: text/x-vcard, Size: 367 bytes --]
begin:vcard
fn;quoted-printable:=C3=89ric Piel
n;quoted-printable:Piel;=C3=89ric
org:Technical University of Delft;Software Engineering Research Group
adr:HB 08.080;;Mekelweg 4;Delft;;2628 CD;The Netherlands
email;internet:E.A.B.Piel@tudelft.nl
tel;work:+31 15 278 6338
tel;cell:+31 6 2437 9135
x-mozilla-html:FALSE
url:http://pieleric.free.fr
version:2.1
end:vcard
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-06 14:56 ` Eric Piel
@ 2008-06-06 15:30 ` Pau Oliva Fora
0 siblings, 0 replies; 27+ messages in thread
From: Pau Oliva Fora @ 2008-06-06 15:30 UTC (permalink / raw)
To: Eric Piel
Cc: Éric Piel, Jonathan Cameron, Jean Delvare, Andrew Morton,
Torokhov, Burman, LKML, HWMON, Mark M. Hoffman, Pavel Machek, Yan,
Dmitry, Arjan van de Ven
Eric Piel wrote:
> Pau Oliva Fora wrote:
>> Here's an i2c version of the driver for the LIS3LV02DL accelerometer,
>> please try to combine it too, so we'll end up having ACPI, SPI and I2C.
> Whaoo, you went fast!
> As there is a dmi_match, that shouldn't be too hard to incorporate it
> nicely :-)
>
> BTW, do you really need the invert_x and invert_y and swap_xy
> parameters? Don't all the HTC shift laptops have the sensors soldered
> the same way? We've worked hard to remove those parameters in mdps ;-)
>
Yes, you can remove the module params, and use the same approach you
have based in DMI table.
Cheers,
Pau Oliva
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-05 20:34 ` Éric Piel
2008-06-05 22:27 ` Pau Oliva Fora
@ 2008-06-06 8:51 ` Jonathan Cameron
1 sibling, 0 replies; 27+ messages in thread
From: Jonathan Cameron @ 2008-06-06 8:51 UTC (permalink / raw)
To: Éric Piel
Cc: Jean Delvare, Andrew Morton, Torokhov, Burman, LKML, HWMON,
Mark M. Hoffman, Pavel Machek, Yan, Dmitry, Arjan van de Ven
>> Still, it may take some time to iron out the various kinks in this system
>> and get it fully tested, so may be worth putting stuff elsewhere for now
>> with the intention to move it as and when the more unified stuff has been
>> written.
> Yes, so I'll keep my plan of first submitting the driver with the
> current interface, and latter updating the interface to the new
> subsystem when you have stabilised its interface.
Probably best plan!
> BTW, just by curiosity, is your tree available publicly somewhere?
Not just yet. (doesn't all work as of yet and I need to get at least a couple
of devices working with it be sure I haven't written something too device
specific)
I'll post to the relevant mailing lists when it reaches a more reasonable
state and the 'arguments' can begin ;)
Jonathan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 19:24 ` [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review! Éric Piel
2008-06-04 20:58 ` Jean Delvare
@ 2008-06-04 23:03 ` Pavel Machek
2008-06-04 23:29 ` Éric Piel
2008-06-05 1:06 ` Henrique de Moraes Holschuh
2008-06-06 14:27 ` Dmitry Torokhov
2 siblings, 2 replies; 27+ messages in thread
From: Pavel Machek @ 2008-06-04 23:03 UTC (permalink / raw)
To: Éric Piel
Cc: Andrew Morton, Dmitry Torokhov, Arjan van de Ven, Mark M. Hoffman,
Yan Burman, LKML, HWMON
Hi!
> We haven't received any review for this driver since last week's post by
> Yan :-( That's why I'm adding explicitly as receivers of this email all
> the people who have reviewed the first two previous takes. Apologies for
> the possible multiple receptions.
>
> It would be _really really kind_ if some of you could take some time and
> comment (or simply ACK ;-) ) this code. It adds support for hardware
> which can be found in mostly every HP Compaq laptop less than three
> years old. It's quite worthy.
It looked pretty much okay on _very_ quick look.
> > 3) Makes it possible to power the device off.
But this part is what I don't understand. powering off does not seem
to belong there. Why can't you just use acpi to power down?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 23:03 ` Pavel Machek
@ 2008-06-04 23:29 ` Éric Piel
2008-06-05 1:06 ` Henrique de Moraes Holschuh
1 sibling, 0 replies; 27+ messages in thread
From: Éric Piel @ 2008-06-04 23:29 UTC (permalink / raw)
To: Pavel Machek
Cc: Andrew Morton, Dmitry Torokhov, Arjan van de Ven, Mark M. Hoffman,
Yan Burman, LKML, HWMON
05-06-08 01:03, Pavel Machek wrote/a écrit:
> Hi!
>>> 3) Makes it possible to power the device off.
>
> But this part is what I don't understand. powering off does not seem
> to belong there. Why can't you just use acpi to power down?
Not sure what you mean. Are you saying acpi has already a user interface
to power up/down a device? If so, I'm not aware of it, is it in /sys ?
Or did you mean that there is already a kernel api to turn on/off a
device via acpi, no need to access the device directly?
That said, this special /sys interface to power the device off is
probably going to be used rarely. It was put there because it is costly
to wake the device up each time we want to read it. What I could do is
put a timer to turn the device off after 10s of non-usage, and power it
on automatically again as soon as the user wants to read some info. That
would permit to remove this interface.
Eric
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 23:03 ` Pavel Machek
2008-06-04 23:29 ` Éric Piel
@ 2008-06-05 1:06 ` Henrique de Moraes Holschuh
2008-06-05 8:19 ` Pavel Machek
1 sibling, 1 reply; 27+ messages in thread
From: Henrique de Moraes Holschuh @ 2008-06-05 1:06 UTC (permalink / raw)
To: Pavel Machek
Cc: Éric Piel, Andrew Morton, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
On Thu, 05 Jun 2008, Pavel Machek wrote:
> > > 3) Makes it possible to power the device off.
>
> But this part is what I don't understand. powering off does not seem
> to belong there. Why can't you just use acpi to power down?
I think he means power down the g-sensors and related filters and A/D
circuitry. The out-of-tree version of the HDAPS driver can do it as
well.
For what is it worth, I think drivers/accel is a good idea, and I think
the other accel drivers should be moved there.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-05 1:06 ` Henrique de Moraes Holschuh
@ 2008-06-05 8:19 ` Pavel Machek
0 siblings, 0 replies; 27+ messages in thread
From: Pavel Machek @ 2008-06-05 8:19 UTC (permalink / raw)
To: Henrique de Moraes Holschuh
Cc: Éric Piel, Andrew Morton, Dmitry Torokhov, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
On Wed 2008-06-04 22:06:47, Henrique de Moraes Holschuh wrote:
> On Thu, 05 Jun 2008, Pavel Machek wrote:
> > > > 3) Makes it possible to power the device off.
> >
> > But this part is what I don't understand. powering off does not seem
> > to belong there. Why can't you just use acpi to power down?
>
> I think he means power down the g-sensors and related filters and A/D
> circuitry. The out-of-tree version of the HDAPS driver can do it as
> well.
Yes, I misunderstood.
> For what is it worth, I think drivers/accel is a good idea, and I think
> the other accel drivers should be moved there.
Agreed.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-04 19:24 ` [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review! Éric Piel
2008-06-04 20:58 ` Jean Delvare
2008-06-04 23:03 ` Pavel Machek
@ 2008-06-06 14:27 ` Dmitry Torokhov
2008-06-06 14:59 ` Eric Piel
2 siblings, 1 reply; 27+ messages in thread
From: Dmitry Torokhov @ 2008-06-06 14:27 UTC (permalink / raw)
To: ??ric Piel
Cc: Andrew Morton, pavel, Arjan van de Ven, Mark M. Hoffman,
Yan Burman, LKML, HWMON
Hi,
On Wed, Jun 04, 2008 at 09:24:37PM +0200, ??ric Piel wrote:
> Hello,
> We haven't received any review for this driver since last week's post by
> Yan :-( That's why I'm adding explicitly as receivers of this email all
> the people who have reviewed the first two previous takes. Apologies for
> the possible multiple receptions.
>
> It would be _really really kind_ if some of you could take some time and
> comment (or simply ACK ;-) ) this code. It adds support for hardware
> which can be found in mostly every HP Compaq laptop less than three
> years old. It's quite worthy.
>
I would recommend converting the input piece to input_polled_device
instead of managing the polling thread by yourselves.
--
Dmitry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-06 14:27 ` Dmitry Torokhov
@ 2008-06-06 14:59 ` Eric Piel
2008-06-06 15:11 ` Dmitry Torokhov
2008-06-06 15:37 ` Pau Oliva Fora
0 siblings, 2 replies; 27+ messages in thread
From: Eric Piel @ 2008-06-06 14:59 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Morton, pavel, Arjan van de Ven, Mark M. Hoffman,
Yan Burman, LKML, HWMON
Dmitry Torokhov wrote:
> Hi,
Hello,
>
>
> I would recommend converting the input piece to input_polled_device
> instead of managing the polling thread by yourselves.
Yes, you are right, I had forgotten about this one! That said, for now
I'm working on removing the polling and relying only the interrupts. So
if it works that shouldn't be needed :-)
See you,
Eric
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-06 14:59 ` Eric Piel
@ 2008-06-06 15:11 ` Dmitry Torokhov
2008-06-06 15:37 ` Pau Oliva Fora
1 sibling, 0 replies; 27+ messages in thread
From: Dmitry Torokhov @ 2008-06-06 15:11 UTC (permalink / raw)
To: Eric Piel
Cc: Andrew Morton, pavel, Arjan van de Ven, Mark M. Hoffman,
Yan Burman, LKML, HWMON
On Fri, Jun 06, 2008 at 04:59:30PM +0200, Eric Piel wrote:
> Dmitry Torokhov wrote:
>> Hi,
> Hello,
>>
>>
>> I would recommend converting the input piece to input_polled_device
>> instead of managing the polling thread by yourselves.
> Yes, you are right, I had forgotten about this one! That said, for now I'm
> working on removing the polling and relying only the interrupts. So if it
> works that shouldn't be needed :-)
>
Oh, great, this is even better.
--
Dmitry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review!
2008-06-06 14:59 ` Eric Piel
2008-06-06 15:11 ` Dmitry Torokhov
@ 2008-06-06 15:37 ` Pau Oliva Fora
1 sibling, 0 replies; 27+ messages in thread
From: Pau Oliva Fora @ 2008-06-06 15:37 UTC (permalink / raw)
To: Eric Piel
Cc: Dmitry Torokhov, Andrew Morton, pavel, Arjan van de Ven,
Mark M. Hoffman, Yan Burman, LKML, HWMON
Eric Piel wrote:
>> I would recommend converting the input piece to input_polled_device
>> instead of managing the polling thread by yourselves.
> Yes, you are right, I had forgotten about this one! That said, for now
> I'm working on removing the polling and relying only the interrupts. So
> if it works that shouldn't be needed :-)
>
I think going to interrupt-only approach will cause a problem in the HTC
Shift:
In CTRL_REG2 (21h) if you set to 1 the Interrupt ENable (IEN) field the
device shuts down.
I guess HTC took the "polling" approach in the windows driver, and they
enable the interrupt when the sensor reports a "free fall" event, to
have the disk parked and protect the hardware from being damaged.
Maybe you can keep the input_polled_device together with interrupts and
select one or the other depending on DMI information (same as for
swaping the axis, etc.)
Cheers,
Pau Oliva
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver
2008-05-31 12:05 [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver Yan Burman
2008-06-04 19:24 ` [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review! Éric Piel
@ 2008-06-05 5:17 ` Andrew Morton
2008-06-05 7:43 ` [lm-sensors] " Riku Voipio
2 siblings, 0 replies; 27+ messages in thread
From: Andrew Morton @ 2008-06-05 5:17 UTC (permalink / raw)
To: Yan Burman; +Cc: LKML, Eric Piel, HWMON
On Sat, 31 May 2008 15:05:33 +0300 Yan Burman <burman.yan@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);
> +}
> +
>
> ...
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver
2008-05-31 12:05 [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver Yan Burman
2008-06-04 19:24 ` [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver -- please review! Éric Piel
2008-06-05 5:17 ` [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver Andrew Morton
@ 2008-06-05 7:43 ` Riku Voipio
2008-06-05 8:28 ` Eric Piel
2 siblings, 1 reply; 27+ messages in thread
From: Riku Voipio @ 2008-06-05 7:43 UTC (permalink / raw)
To: Yan Burman; +Cc: LKML, Eric Piel, HWMON, spi-devel-general, jic23
Yan Burman wrote:
> +==================
> +
> +Supported chips:
> +
> + * STMicroelectronics LIS3LV02DL and LIS3LV02DQ
> +
These chips are connected to either I2C or SPI - This is the 4th driver for
(apparently) these same chips:
http://docwiki.gumstix.org/Lis3lv02dq_spi.c
http://svn.openmoko.org/branches/src/target/kernel/2.6.24.x/patches/lis302dl.patch
http://article.gmane.org/gmane.linux.kernel.spi.devel/1010
> + depends on ACPI && INPUT && X86
>
> +/* The actual chip is STMicroelectronics LIS3LV02DL or LIS3LV02DQ
> + * that seems to be connected via SPI */
>
Perhaps it would make more sense implement support for SPI
bus on the laptop and use the SPI interface directly instead or
routing via the ACPI hiding layer?
> +
> +#define MDPS_WHO_AM_I 0x0F /*r 00111010 */
> +#define MDPS_OFFSET_X 0x16 /*rw */
> +#define MDPS_OFFSET_Y 0x17 /*rw */
> +#define MDPS_OFFSET_Z 0x18 /*rw */
> +#define MDPS_GAIN_X 0x19 /*rw */
> +#define MDPS_GAIN_Y 0x1A /*rw */
> +#define MDPS_GAIN_Z 0x1B /*rw */
> +#define MDPS_CTRL_REG1 0x20 /*rw 00000111 */
> +#define MDPS_CTRL_REG2 0x21 /*rw 00000000 */
> +#define MDPS_CTRL_REG3 0x22 /*rw 00001000 */
> +#define MDPS_HP_FILTER RESET 0x23 /*r */
> +#define MDPS_STATUS_REG 0x27 /*rw 00000000 */
> +#define MDPS_OUTX_L 0x28 /*r */
> +#define MDPS_OUTX_H 0x29 /*r */
> +#define MDPS_OUTY_L 0x2A /*r */
> +#define MDPS_OUTY_H 0x2B /*r */
> +#define MDPS_OUTZ_L 0x2C /*r */
> +#define MDPS_OUTZ_H 0x2D /*r */
> +#define MDPS_FF_WU_CFG 0x30 /*rw 00000000 */
> +#define MDPS_FF_WU_SRC 0x31 /*rw 00000000 */
> +#define MDPS_FF_WU_ACK 0x32 /*r */
> +#define MDPS_FF_WU_THS_L 0x34 /*rw 00000000 */
> +#define MDPS_FF_WU_THS_H 0x35 /*rw 00000000 */
> +#define MDPS_FF_WU_DURATION 0x36 /*rw 00000000 */
> +#define MDPS_DD_CFG 0x38 /*rw 00000000 */
> +#define MDPS_DD_SRC 0x39 /*rw 00000000 */
> +#define MDPS_DD_ACK 0x3A /*r */
> +#define MDPS_DD_THSI_L 0x3C /*rw 00000000 */
> +#define MDPS_DD_THSI_H 0x3D /*rw 00000000 */
> +#define MDPS_DD_THSE_L 0x3E /*rw 00000000 */
> +#define MDPS_DD_THSE_H 0x3F /*rw 00000000 */
> +
> +#define MDPS_ID 0x3A /* MDPS_WHO_AM_I */
> +#define MDPS_FS (1<<7) /* MDPS_CTRL_REG2 : Full Scale selection */
> +#define MDPS_BDU (1<<6) /* MDPS_CTRL_REG2 : Block Data Update */
> +
> +/* joystick device poll interval in milliseconds */
> +#define MDPS_POLL_INTERVAL 30
> +
> +/* Maximum value our axis may get for the input device */
> +#define MDPS_MAX_VAL 2048
> +
> +static unsigned int power_off;
> +module_param(power_off, bool, S_IRUGO);
> +MODULE_PARM_DESC(power_off, "Turn off device on module load");
> +
> +struct axis_conversion {
> + s8 x;
> + s8 y;
> + s8 z;
> +};
> +
> +struct acpi_mdps {
> + struct acpi_device *device; /* The ACPI device */
> + 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 zcalib; /* calibrated null value for z */
> + int is_on; /* whether the device is on or off */
> + struct platform_device *pdev; /* platform device */
> + atomic_t count; /* interrupt count after last read */
> + struct fasync_struct *async_queue;
> + atomic_t available; /* whether the device is open */
> + wait_queue_head_t misc_wait; /* Wait queue for the misc device */
> + /* conversion between hw axis and logical ones */
> + struct axis_conversion ac;
> +};
> +
> +static struct acpi_mdps mdps;
> +
> +static int mdps_remove_fs(void);
> +static int mdps_add_fs(struct acpi_device *device);
> +static void mdps_joystick_enable(void);
> +static void mdps_joystick_disable(void);
> +
> +static struct acpi_device_id mdps_device_ids[] = {
> + {ACPI_MDPS_ID, 0},
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, mdps_device_ids);
> +
> +/** 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
> + */
> +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);
> +}
> +
> +/** ACPI ALRD method: read a register
> + * @param handle the handle of the device
> + * @param reg the register to read
> + * @param[out] ret result of the operation
> + * @return AE_OK on success
> + */
> +static acpi_status mdps_ALRD(acpi_handle handle, int reg,
> + unsigned long *ret)
> +{
> + union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> + struct acpi_object_list args = { 1, &arg0 };
> +
> + arg0.integer.value = reg;
> +
> + return acpi_evaluate_integer(handle, "ALRD", &args, ret);
> +}
> +
> +/** ACPI _INI method: initialize the device.
> + * @param handle the handle of the device
> + * @return 0 on success
> + */
> +static inline acpi_status mdps__INI(acpi_handle handle)
> +{
> + return acpi_evaluate_object(handle, METHOD_NAME__INI, NULL, NULL);
> +}
> +
> +/** ACPI ALWR method: write to a register
> + * @param handle the handle of the device
> + * @param reg the register to write to
> + * @param val the value to write
> + * @param[out] ret result of the operation
> + * @return AE_OK on success
> + */
> +static acpi_status mdps_ALWR(acpi_handle handle, int reg, int val,
> + unsigned long *ret)
> +{
> + union acpi_object in_obj[2];
> + struct acpi_object_list args = { 2, in_obj };
> +
> + in_obj[0].type = ACPI_TYPE_INTEGER;
> + in_obj[0].integer.value = reg;
> + in_obj[1].type = ACPI_TYPE_INTEGER;
> + in_obj[1].integer.value = val;
> +
> + return acpi_evaluate_integer(handle, "ALWR", &args, ret);
> +}
> +
> +static int mdps_read_axis(acpi_handle handle, int lo_const, int hi_const)
> +{
> + unsigned long lo_val, hi_val;
> + mdps_ALRD(handle, lo_const, &lo_val);
> + mdps_ALRD(handle, hi_const, &hi_val);
> + return mdps_glue_bytes(hi_val, lo_val);
> +}
> +
> +static inline int mdps_get_axis(s8 axis, int hw_values[3])
> +{
> + if (axis > 0)
> + return hw_values[axis - 1];
> + else
> + return -hw_values[-axis - 1];
> +}
> +
> +/** Get X, Y and Z axis values from the accelerometer
> + * @param handle the handle to the device
> + * @param[out] x where to store the X axis value
> + * @param[out] y where to store the Y axis value
> + * @param[out] z where to store the Z axis value
> + * @note 40Hz input device can eat up about 10% CPU at 800MHZ
> + */
> +static void mdps_get_xyz(acpi_handle handle, int *x, int *y, int *z)
> +{
> + int position[3];
> + position[0] = mdps_read_axis(handle, MDPS_OUTX_L, MDPS_OUTX_H);
> + position[1] = mdps_read_axis(handle, MDPS_OUTY_L, MDPS_OUTY_H);
> + position[2] = mdps_read_axis(handle, MDPS_OUTZ_L, MDPS_OUTZ_H);
> +
> + *x = mdps_get_axis(mdps.ac.x, position);
> + *y = mdps_get_axis(mdps.ac.y, position);
> + *z = mdps_get_axis(mdps.ac.z, position);
> +}
> +
> +/** Kthread polling function
> + * @param data unused - here to conform to threadfn prototype
> + */
> +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;
> +}
> +
> +static inline void mdps_poweroff(acpi_handle handle)
> +{
> + unsigned long ret;
> + mdps.is_on = 0;
> + /* disable X,Y,Z axis and power down */
> + mdps_ALWR(handle, MDPS_CTRL_REG1, 0x00, &ret);
> +}
> +
> +static inline void mdps_poweron(acpi_handle handle)
> +{
> + unsigned long val, retw;
> +
> + mdps.is_on = 1;
> + mdps__INI(handle);
> + /*
> + * Change to Block Data Update mode: LSB and MSB values are not updated
> + * until both have been read. So the value read will always be correct.
> + */
> + mdps_ALRD(handle, MDPS_CTRL_REG2, &val);
> + val |= MDPS_BDU;
> + mdps_ALWR(handle, MDPS_CTRL_REG2, val, &retw);
> +}
> +
> +#ifdef CONFIG_PM
> +static int mdps_suspend(struct acpi_device *device, pm_message_t state)
> +{
> + /* make sure the device is off when we suspend */
> + mdps_poweroff(mdps.device->handle);
> + return 0;
> +}
> +#endif
> +
> +static int mdps_resume(struct acpi_device *device)
> +{
> + /* make sure the device went online */
> + mdps_poweron(mdps.device->handle);
> + return 0;
> +}
> +
> +static irqreturn_t mdps_irq(int irq, void *dev_id)
> +{
> + atomic_inc(&mdps.count);
> +
> + wake_up_interruptible(&mdps.misc_wait);
> + kill_fasync(&mdps.async_queue, SIGIO, POLL_IN);
> +
> + return IRQ_HANDLED;
> +}
> +
> +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 */
> + }
> +
> + 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 int mdps_misc_release(struct inode *inode, struct file *file)
> +{
> + fasync_helper(-1, file, 0, &mdps.async_queue);
> + free_irq(mdps.irq, mdps_irq);
> + atomic_inc(&mdps.available); /* release the device */
> + 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 (; ; ) {
> + 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 unsigned int mdps_misc_poll(struct file *file, poll_table *wait)
> +{
> + poll_wait(file, &mdps.misc_wait, wait);
> + if (atomic_read(&mdps.count))
> + return POLLIN | POLLRDNORM;
> + return 0;
> +}
> +
> +static int mdps_misc_fasync(int fd, struct file *file, int on)
> +{
> + return fasync_helper(fd, file, on, &mdps.async_queue);
> +}
> +
> +static const struct file_operations mdps_misc_fops = {
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .read = mdps_misc_read,
> + .open = mdps_misc_open,
> + .release = mdps_misc_release,
> + .poll = mdps_misc_poll,
> + .fasync = mdps_misc_fasync,
> +};
> +
> +static struct miscdevice mdps_misc_device = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "accel",
> + .fops = &mdps_misc_fops,
> +};
> +
> +static acpi_status
> +mdps_get_resource(struct acpi_resource *resource, void *context)
> +{
> + if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ) {
> + struct acpi_resource_extended_irq *irq;
> + u32 *device_irq = context;
> +
> + irq = &resource->data.extended_irq;
> + *device_irq = irq->interrupts[0];
> + }
> +
> + return AE_OK;
> +}
> +
> +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");
> +}
> +
> +static int mdps_dmi_matched(const struct dmi_system_id *dmi)
> +{
> + mdps.ac = *(struct axis_conversion *)dmi->driver_data;
> + printk(KERN_INFO "mdps: hardware type %s found.\n", dmi->ident);
> +
> + return 1;
> +}
> +
> +
> +/* Represents, for each axis seen by userspace, the corresponding hw axis (+1).
> + * If the value is negative, the opposite of the hw value is used. */
> +static struct axis_conversion mdps_axis_normal = {1, 2, 3};
> +static struct axis_conversion mdps_axis_y_inverted = {1, -2, 3};
> +static struct axis_conversion mdps_axis_x_inverted = {-1, 2, 3};
> +
> +static struct dmi_system_id mdps_dmi_ids[] = {
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NC64x0",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nc64"),
> + },
> + .driver_data = &mdps_axis_x_inverted
> + },
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NX9420",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx9420"),
> + },
> + .driver_data = &mdps_axis_x_inverted
> + },
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NW9440",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nw9440"),
> + },
> + .driver_data = &mdps_axis_x_inverted
> + },
> + {
> + .callback = mdps_dmi_matched,
> + .ident = "NC2510",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq 2510"),
> + },
> + .driver_data = &mdps_axis_y_inverted
> + },
> + { NULL, }
> +/* Laptop models without axis info (yet):
> + * "NC84x0" "HP Compaq nc84"
> + * "NC651xx" "HP Compaq 651"
> + * "NC671xx" "HP Compaq 671"
> + * "NC6910" "HP Compaq 6910"
> + * HP Compaq 8510x Notebook PC / Mobile Workstation
> + * HP Compaq 8710x Notebook PC / Mobile Workstation
> + * "NC2400" "HP Compaq nc2400"
> + * "NX74x0" "HP Compaq nx74"
> + * "NX6325" "HP Compaq nx6325"
> + * "NC4400" "HP Compaq nc4400"
> + */
> +};
> +
> +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);
> + 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);
> +
> + ret = misc_register(&mdps_misc_device);
> + if (ret)
> + printk(KERN_ERR "mdps: misc_register failed\n");
> +
> + return 0;
> +}
> +
> +static int mdps_remove(struct acpi_device *device, int type)
> +{
> + if (!device)
> + return -EINVAL;
> +
> + if (mdps.irq)
> + misc_deregister(&mdps_misc_device);
> +
> + mdps_joystick_disable();
> +
> + return mdps_remove_fs();
> +}
> +
> +static inline void mdps_calibrate_joystick(void)
> +{
> + mdps_get_xyz(mdps.device->handle, &mdps.xcalib, &mdps.ycalib,
> + &mdps.zcalib);
> +}
> +
> +static int mdps_joystick_open(struct input_dev *dev)
> +{
> + mdps.kthread = kthread_run(mdps_input_kthread, NULL, "kmdps");
> + if (IS_ERR(mdps.kthread))
> + return PTR_ERR(mdps.kthread);
> +
> + return 0;
> +}
> +
> +static void mdps_joystick_close(struct input_dev *dev)
> +{
> + kthread_stop(mdps.kthread);
> +}
> +
> +static void mdps_joystick_enable(void)
> +{
> + if (mdps.idev)
> + return;
> +
> + mdps.idev = input_allocate_device();
> + if (!mdps.idev)
> + return;
> +
> + 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;
> + }
> +}
> +
> +static void mdps_joystick_disable(void)
> +{
> + if (!mdps.idev)
> + return;
> +
> + input_unregister_device(mdps.idev);
> + mdps.idev = NULL;
> +}
> +
> +/* Sysfs stuff */
> +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);
> +
> + 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"));
> +}
> +
> +static ssize_t mdps_calibrate_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "(%d,%d,%d)\n", mdps.xcalib, mdps.ycalib,
> + mdps.zcalib);
> +}
> +
> +static ssize_t mdps_calibrate_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + mdps_calibrate_joystick();
> + return count;
> +}
> +
> +static ssize_t mdps_rate_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + unsigned long ctrl;
> + int rate = 0;
> +
> + 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;
> +
> + mdps.is_on = state;
> +
> + if (mdps.is_on)
> + mdps_poweron(mdps.device->handle);
> + else
> + mdps_poweroff(mdps.device->handle);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(position, S_IRUGO, mdps_position_show, NULL);
> +static DEVICE_ATTR(calibrate, S_IRUGO|S_IWUSR, mdps_calibrate_show,
> + mdps_calibrate_store);
> +static DEVICE_ATTR(rate, S_IRUGO, mdps_rate_show, NULL);
> +static DEVICE_ATTR(state, S_IRUGO|S_IWUSR, mdps_state_show, mdps_state_store);
> +
> +static struct attribute *mdps_attributes[] = {
> + &dev_attr_position.attr,
> + &dev_attr_calibrate.attr,
> + &dev_attr_rate.attr,
> + &dev_attr_state.attr,
> + NULL
> +};
> +
> +static struct attribute_group mdps_attribute_group = {
> + .attrs = mdps_attributes
> +};
> +
> +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);
> +
> + return sysfs_create_group(&mdps.pdev->dev.kobj, &mdps_attribute_group);
> +}
> +
> +static int mdps_remove_fs(void)
> +{
> + sysfs_remove_group(&mdps.pdev->dev.kobj, &mdps_attribute_group);
> + platform_device_unregister(mdps.pdev);
> + return 0;
> +}
> +
> +static struct acpi_driver mdps_driver = {
> + .name = DRIVER_NAME,
> + .class = ACPI_MDPS_CLASS,
> + .ids = mdps_device_ids,
> + .ops = {
> + .add = mdps_add,
> + .remove = mdps_remove,
> +#ifdef CONFIG_PM
> + .suspend = mdps_suspend,
> + .resume = mdps_resume
> +#endif
> + }
> +};
> +
> +static int __init mdps_init_module(void)
> +{
> + int ret;
> +
> + if (acpi_disabled)
> + return -ENODEV;
> +
> + ret = acpi_bus_register_driver(&mdps_driver);
> + if (ret < 0)
> + return ret;
> +
> + printk(KERN_INFO "mdps version " VERSION " loaded.\n");
> +
> + return 0;
> +}
> +
> +static void __exit mdps_exit_module(void)
> +{
> + acpi_bus_unregister_driver(&mdps_driver);
> +}
> +
> +MODULE_DESCRIPTION("HP three-axis digital accelerometer ACPI driver");
> +MODULE_AUTHOR("Yan Burman (burman.yan@gmail.com)");
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
> +
> +module_init(mdps_init_module);
> +module_exit(mdps_exit_module);
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [lm-sensors] [PATCH 2.6.25.4] hwmon: HP Mobile Data Protection System 3D ACPI driver
2008-06-05 7:43 ` [lm-sensors] " Riku Voipio
@ 2008-06-05 8:28 ` Eric Piel
0 siblings, 0 replies; 27+ messages in thread
From: Eric Piel @ 2008-06-05 8:28 UTC (permalink / raw)
To: Riku Voipio; +Cc: Yan Burman, LKML, HWMON, spi-devel-general, jic23, pau
Riku Voipio wrote:
> Yan Burman wrote:
>> +==================
>> +
>> +Supported chips:
>> +
>> + * STMicroelectronics LIS3LV02DL and LIS3LV02DQ
>> +
> These chips are connected to either I2C or SPI - This is the 4th driver for
> (apparently) these same chips:
>
> http://docwiki.gumstix.org/Lis3lv02dq_spi.c
> http://svn.openmoko.org/branches/src/target/kernel/2.6.24.x/patches/lis302dl.patch
>
> http://article.gmane.org/gmane.linux.kernel.spi.devel/1010
Hi!
Thanks a lot for these links (which I was not aware of). They indeed
seem to handle the same hardware (all via spi). To be even more
complete, here is a link we received off-list for accessing the same
hardware via I²C:
http://pof.eslack.org/HTC/shift/i2c-gsensor.tar.gz
There are for sure some nice things we could borrow ;-)
>
>> + depends on ACPI && INPUT && X86
>>
>
>
>> +/* The actual chip is STMicroelectronics LIS3LV02DL or LIS3LV02DQ
>> + * that seems to be connected via SPI */
>>
> Perhaps it would make more sense implement support for SPI
> bus on the laptop and use the SPI interface directly instead or
> routing via the ACPI hiding layer?
Getting rid of ACPI could be nice, as it tends to be rather slow.
However, so far we've stick to it because it ensures that for the ~15
different models of HP laptop, we can access the hardware exactly the
same way. I have the gut feeling that if HP spent some time to add an
interface in ACPI, there was some kind of reason.
However, I know nothing about SPI. Maybe you'll tell that if this chip
is on the SPI bus, it will always be accessed the same way, located at
the same address... or whatever that can ensure us that from the moment
we know this device is in the laptop (and that's easy via HPQ0004) we
cannot mess up. In that case, going to SPI would be definitely worthy.
Eric
^ permalink raw reply [flat|nested] 27+ messages in thread