[PATCH] lis3lv02d: support both one- and two-byte sensors

[PATCH] lis3lv02d: support both one- and two-byte sensors

[Giuseppe Bilotta]

Sensors responding with 0x3B to WHO_AM_I only have one data register per
direction, thus returning a signed byte from the position which is
occupied by the MSB in sensors responding with 0x3A.

We support both kind of sensors by checking for the sensor type on init
and defining appropriate data-access routines and sensor limits (for the
joystick) depending on what we find.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---

To make the patch smaller, I left a funky effect on the 8-bit sensor
reading routine: it actually accesses the byte AFTER the one requested,
so I didn't have to touch all the requests.

Another solution would be to let it access the requested bytes, change
all requests to the subsequent byte, and have the 16-bit routine access
the requested byte and the one BEFORE it. I can remake the patch in this
sense if reviewers judge it to be formally more correct.

 drivers/hwmon/lis3lv02d.c |   41 +++++++++++++++++++++++++++++++----------
 drivers/hwmon/lis3lv02d.h |    9 ++++++++-
 2 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 219d2d0..28a1cae 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -52,9 +52,6 @@
  * joystick.
  */
 
-/* Maximum value our axis may get for the input device (signed 12 bits) */
-#define MDPS_MAX_VAL 2048
-
 struct acpi_lis3lv02d adev;
 EXPORT_SYMBOL_GPL(adev);
 
@@ -70,6 +67,16 @@ static s16 lis3lv02d_read_16(acpi_handle handle, int reg)
 	return (s16)((hi << 8) | lo);
 }
 
+static s16 lis3lv02d_read_8(acpi_handle handle, int reg)
+{
+	u8 lo;
+	/* 8-bit sensors store their data in the byte which holds
+	   the MSB for 16-bit sensors, so we actually check the
+	   NEXT position */
+	adev.read(handle, reg + 1, &lo);
+	return *((s8*)(&lo));
+}
+
 /**
  * lis3lv02d_get_axis - For the given axis, give the value converted
  * @axis:      1,2,3 - can also be negative
@@ -98,9 +105,9 @@ static void lis3lv02d_get_xyz(acpi_handle handle, int *x, int *y, int *z)
 {
 	int position[3];
 
-	position[0] = lis3lv02d_read_16(handle, OUTX_L);
-	position[1] = lis3lv02d_read_16(handle, OUTY_L);
-	position[2] = lis3lv02d_read_16(handle, OUTZ_L);
+	position[0] = adev.lis3lv02d_read(handle, OUTX_L);
+	position[1] = adev.lis3lv02d_read(handle, OUTY_L);
+	position[2] = adev.lis3lv02d_read(handle, OUTZ_L);
 
 	*x = lis3lv02d_get_axis(adev.ac.x, position);
 	*y = lis3lv02d_get_axis(adev.ac.y, position);
@@ -121,7 +128,21 @@ void lis3lv02d_poweron(acpi_handle handle)
 
 	adev.is_on = 1;
 	adev.init(handle);
-	adev.write(handle, FF_WU_CFG, 0);
+	adev.read(handle, WHO_AM_I, &adev.whoami);
+	/* Tell apart LISxLV02Dy from LISx02Dy family by checking the LSB
+	 * (0x3A vs 0x3B). TODO More sophisticated checks on other registers
+	 * could be implemented, for example to see if we have 2 or 3 axes, and
+	 * configure the joystick accordingly.
+	 */
+	if (adev.whoami & 1) {
+		printk(KERN_INFO DRIVER_NAME ": 1-byte sensor found\n");
+		adev.lis3lv02d_read = lis3lv02d_read_8;
+		adev.mdps_max_val = 128;
+	} else {
+		printk(KERN_INFO DRIVER_NAME ": 2-byte sensor found\n");
+		adev.lis3lv02d_read = lis3lv02d_read_16;
+		adev.mdps_max_val = 2048;
+	}
 	/*
 	 * BDU: LSB and MSB values are not updated until both have been read.
 	 *      So the value read will always be correct.
@@ -231,9 +252,9 @@ int lis3lv02d_joystick_enable(void)
 	adev.idev->close      = lis3lv02d_joystick_close;
 
 	set_bit(EV_ABS, adev.idev->evbit);
-	input_set_abs_params(adev.idev, ABS_X, -MDPS_MAX_VAL, MDPS_MAX_VAL, 3, 3);
-	input_set_abs_params(adev.idev, ABS_Y, -MDPS_MAX_VAL, MDPS_MAX_VAL, 3, 3);
-	input_set_abs_params(adev.idev, ABS_Z, -MDPS_MAX_VAL, MDPS_MAX_VAL, 3, 3);
+	input_set_abs_params(adev.idev, ABS_X, -adev.mdps_max_val, adev.mdps_max_val, 3, 3);
+	input_set_abs_params(adev.idev, ABS_Y, -adev.mdps_max_val, adev.mdps_max_val, 3, 3);
+	input_set_abs_params(adev.idev, ABS_Z, -adev.mdps_max_val, adev.mdps_max_val, 3, 3);
 
 	err = input_register_device(adev.idev);
 	if (err) {
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 223f1c0..cf61f0f 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -22,11 +22,14 @@
 /*
  * The actual chip is STMicroelectronics LIS3LV02DL or LIS3LV02DQ that seems to
  * be connected via SPI. There exists also several similar chips (such as LIS302DL or
- * LIS3L02DQ) but not in the HP laptops and they have slightly different registers.
+ * LIS3L02DQ) and they have slightly different registers, but we can provide a
+ * common interface for all of them.
  * They can also be connected via I²C.
  */
 
+/* 2-byte registers */
 #define LIS3LV02DL_ID	0x3A /* Also the LIS3LV02DQ */
+/* 1-byte registers */
 #define LIS302DL_ID	0x3B /* Also the LIS202DL! */
 
 enum lis3lv02d_reg {
@@ -159,6 +162,10 @@ struct acpi_lis3lv02d {
 	acpi_status (*write) (acpi_handle handle, int reg, u8 val);
 	acpi_status (*read) (acpi_handle handle, int reg, u8 *ret);
 
+	u8			whoami;    /* 3Ah: 2-byte registries, 3Bh: 1-byte registries */
+	s16 (*lis3lv02d_read) (acpi_handle handle, int reg);
+	int			mdps_max_val;
+
 	struct input_dev	*idev;     /* input device */
 	struct task_struct	*kthread;  /* kthread for input */
 	struct mutex            lock;
-- 
1.6.2.rc0.173.g5e148

[PATCH] lis3lv02d: support both one- and two-byte sensors

[Giuseppe Bilotta]

Sensors responding with 0x3B to WHO_AM_I only have one data register per
direction, thus returning a signed byte from the position which is
occupied by the MSB in sensors responding with 0x3A.

We support both kind of sensors by checking for the sensor type on init
and defining appropriate data-access routines and sensor limits (for the
joystick) depending on what we find.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
As pointed out by Eric, there's no need to jump through hoops to obtain
a signed 16-bit number from a signed 8-bit number.

This patch is good for -mm

Note also that this patch is only about 8 vs 16 bit data, so the axis
orientation for HP notebooks still has to be fixed in a separate patch,
but at least now I should be able to check it out properly.

 drivers/hwmon/lis3lv02d.c |   41 ++++++++++++++++++++++++++++++-----------
 drivers/hwmon/lis3lv02d.h |   12 +++++++++++-
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 3afa3af..0f6528f 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -53,9 +53,6 @@
  * joystick.
  */
 
-/* Maximum value our axis may get for the input device (signed 12 bits) */
-#define MDPS_MAX_VAL 2048
-
 struct acpi_lis3lv02d adev = {
 	.misc_wait   = __WAIT_QUEUE_HEAD_INITIALIZER(adev.misc_wait),
 };
@@ -68,12 +65,19 @@ static s16 lis3lv02d_read_16(acpi_handle handle, int reg)
 {
 	u8 lo, hi;
 
-	adev.read(handle, reg, &lo);
-	adev.read(handle, reg + 1, &hi);
+	adev.read(handle, reg - 1, &lo);
+	adev.read(handle, reg, &hi);
 	/* In "12 bit right justified" mode, bit 6, bit 7, bit 8 = bit 5 */
 	return (s16)((hi << 8) | lo);
 }
 
+static s16 lis3lv02d_read_8(acpi_handle handle, int reg)
+{
+	s8 lo;
+	adev.read(handle, reg, &lo);
+	return lo;
+}
+
 /**
  * lis3lv02d_get_axis - For the given axis, give the value converted
  * @axis:      1,2,3 - can also be negative
@@ -102,9 +106,9 @@ static void lis3lv02d_get_xyz(acpi_handle handle, int *x, int *y, int *z)
 {
 	int position[3];
 
-	position[0] = lis3lv02d_read_16(handle, OUTX_L);
-	position[1] = lis3lv02d_read_16(handle, OUTY_L);
-	position[2] = lis3lv02d_read_16(handle, OUTZ_L);
+	position[0] = adev.lis3lv02d_read(handle, OUTX);
+	position[1] = adev.lis3lv02d_read(handle, OUTY);
+	position[2] = adev.lis3lv02d_read(handle, OUTZ);
 
 	*x = lis3lv02d_get_axis(adev.ac.x, position);
 	*y = lis3lv02d_get_axis(adev.ac.y, position);
@@ -121,6 +125,21 @@ void lis3lv02d_poweron(acpi_handle handle)
 {
 	adev.is_on = 1;
 	adev.init(handle);
+	adev.read(handle, WHO_AM_I, &adev.whoami);
+	/* Tell apart LISxLV02Dy from LISx02Dy family by checking the LSB
+	 * (0x3A vs 0x3B). TODO More sophisticated checks on other registers
+	 * could be implemented, for example to see if we have 2 or 3 axes, and
+	 * configure the joystick accordingly.
+	 */
+	if (adev.whoami & 1) {
+		printk(KERN_INFO DRIVER_NAME ": 1-byte sensor found\n");
+		adev.lis3lv02d_read = lis3lv02d_read_8;
+		adev.mdps_max_val = 128;
+	} else {
+		printk(KERN_INFO DRIVER_NAME ": 2-byte sensor found\n");
+		adev.lis3lv02d_read = lis3lv02d_read_16;
+		adev.mdps_max_val = 2048;
+	}
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
 
@@ -355,9 +374,9 @@ int lis3lv02d_joystick_enable(void)
 	adev.idev->close      = lis3lv02d_joystick_close;
 
 	set_bit(EV_ABS, adev.idev->evbit);
-	input_set_abs_params(adev.idev, ABS_X, -MDPS_MAX_VAL, MDPS_MAX_VAL, 3, 3);
-	input_set_abs_params(adev.idev, ABS_Y, -MDPS_MAX_VAL, MDPS_MAX_VAL, 3, 3);
-	input_set_abs_params(adev.idev, ABS_Z, -MDPS_MAX_VAL, MDPS_MAX_VAL, 3, 3);
+	input_set_abs_params(adev.idev, ABS_X, -adev.mdps_max_val, adev.mdps_max_val, 3, 3);
+	input_set_abs_params(adev.idev, ABS_Y, -adev.mdps_max_val, adev.mdps_max_val, 3, 3);
+	input_set_abs_params(adev.idev, ABS_Z, -adev.mdps_max_val, adev.mdps_max_val, 3, 3);
 
 	err = input_register_device(adev.idev);
 	if (err) {
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 2e7597c..cd0e838 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -22,11 +22,14 @@
 /*
  * The actual chip is STMicroelectronics LIS3LV02DL or LIS3LV02DQ that seems to
  * be connected via SPI. There exists also several similar chips (such as LIS302DL or
- * LIS3L02DQ) but not in the HP laptops and they have slightly different registers.
+ * LIS3L02DQ) and they have slightly different registers, but we can provide a
+ * common interface for all of them.
  * They can also be connected via I²C.
  */
 
+/* 2-byte registers */
 #define LIS3LV02DL_ID	0x3A /* Also the LIS3LV02DQ */
+/* 1-byte registers */
 #define LIS302DL_ID	0x3B /* Also the LIS202DL! */
 
 enum lis3lv02d_reg {
@@ -44,10 +47,13 @@ enum lis3lv02d_reg {
 	STATUS_REG	= 0x27,
 	OUTX_L		= 0x28,
 	OUTX_H		= 0x29,
+	OUTX		= 0x29,
 	OUTY_L		= 0x2A,
 	OUTY_H		= 0x2B,
+	OUTY		= 0x2B,
 	OUTZ_L		= 0x2C,
 	OUTZ_H		= 0x2D,
+	OUTZ		= 0x2D,
 	FF_WU_CFG	= 0x30,
 	FF_WU_SRC	= 0x31,
 	FF_WU_ACK	= 0x32,
@@ -159,6 +165,10 @@ struct acpi_lis3lv02d {
 	acpi_status (*write) (acpi_handle handle, int reg, u8 val);
 	acpi_status (*read) (acpi_handle handle, int reg, u8 *ret);
 
+	u8			whoami;    /* 3Ah: 2-byte registries, 3Bh: 1-byte registries */
+	s16 (*lis3lv02d_read) (acpi_handle handle, int reg);
+	int			mdps_max_val;
+
 	struct input_dev	*idev;     /* input device */
 	struct task_struct	*kthread;  /* kthread for input */
 	struct mutex            lock;
-- 
1.6.2.rc0.173.g5e148

Re: [PATCH] lis3lv02d: support both one- and two-byte sensors

[Michael J Gruber]

Giuseppe,

you may prefer sending this to the kernel mailing list rather than the
git list. But we can tell you the patch is formatted as a proper git
patch ;)

Cheers,
Michael

Re: [PATCH] lis3lv02d: support both one- and two-byte sensors

[Giuseppe Bilotta]

On Wednesday 11 February 2009 12:16, Michael J Gruber wrote:

> Giuseppe,
> 
> you may prefer sending this to the kernel mailing list rather than the
> git list. But we can tell you the patch is formatted as a proper git
> patch ;)

I know, I'm terribly sorry for the spamming I did. Fingers automatically went
for 'git@vger' instead of 'linux-kernel@vger' ... I'm so much more used to
sending patches to this list that it goes on automatically 8-/

I'll try not to repeat the mistake in the future 8-)

-- 
Giuseppe "Oblomov" Bilotta