Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digitalaccelerometer
From: Ãric Piel
Date: Sun Sep 23 2012 - 17:38:29 EST
On 22-08-12 08:30, AnilKumar Ch wrote:
This patch adds support for lis331dlh digital accelerometer to the
lis3lv02d driver family. Adds ID field for detecting the lis331dlh
module, based on this ID field lis3lv02d driver will export the
lis331dlh module functionality.
Hello AnilKumar,
Sorry for taking so long to review your patch. Overall, it looks great :-)
There are just a few points that almost code-style/comment that needs to
be fixed. See below. Could you fix these small issues, and submit a new
patch for Andrew to pick it in his queue?
Here is my:
Reviewed-by: Ãric Piel <eric.piel@xxxxxxxxxxxxxxxx>
Signed-off-by: AnilKumar Ch <anilkumar@xxxxxx>
---
Changes from v1:
- Removed G range sysfs entry from v1
- Modified documentation to specify lis331dlh is supported
Documentation/misc-devices/lis3lv02d | 3 ++-
drivers/misc/lis3lv02d/lis3lv02d.c | 42 +++++++++++++++++++++++++++++-
drivers/misc/lis3lv02d/lis3lv02d.h | 44 +++++++++++++++++++++++++++-----
drivers/misc/lis3lv02d/lis3lv02d_i2c.c | 7 ++++-
4 files changed, 87 insertions(+), 9 deletions(-)
diff --git a/Documentation/misc-devices/lis3lv02d b/Documentation/misc-devices/lis3lv02d
index f1a4ec8..af815b9 100644
--- a/Documentation/misc-devices/lis3lv02d
+++ b/Documentation/misc-devices/lis3lv02d
@@ -4,7 +4,8 @@ Kernel driver lis3lv02d
Supported chips:
* STMicroelectronics LIS3LV02DL, LIS3LV02DQ (12 bits precision)
- * STMicroelectronics LIS302DL, LIS3L02DQ, LIS331DL (8 bits)
+ * STMicroelectronics LIS302DL, LIS3L02DQ, LIS331DL (8 bits) and
+ LIS331DLH (16 bits)
Authors:
Yan Burman <burman.yan@xxxxxxxxx>
diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
index 29d12a7..c0a9199 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d.c
@@ -80,6 +80,14 @@
#define LIS3_SENSITIVITY_12B ((LIS3_ACCURACY * 1000) / 1024)
#define LIS3_SENSITIVITY_8B (18 * LIS3_ACCURACY)
+/*
+ * LIS3331DLH spec says 1LSBs corresponds 4G/1024 -> 1LSB is 1000/1024 mG.
+ * Sensitivity values for +/-2G, outdata in 12 bits for +/-2G scale. so 4
+ * bits adjustment is required
+ */
+#define LIS3DLH_SENSITIVITY_2G ((LIS3_ACCURACY * 1000) / 1024)
+#define SHIFT_ADJ_2G 4
+
You said later:
Typo mistake this should be 4G/4096
Could you fix the comment. Even better, change LIS3331DLH to LIS331DLH
Also, if I understand correctly, there is currently no support for
changing the sensitivity. It stays at 2G all the time, right? In such
case, please add a sentence in this comment that the driver does only
support the 2G sensitivity for now.
#define LIS3_DEFAULT_FUZZ_12B 3
#define LIS3_DEFAULT_FLAT_12B 3
#define LIS3_DEFAULT_FUZZ_8B 1
@@ -133,6 +141,19 @@ static s16 lis3lv02d_read_12(struct lis3lv02d *lis3, int reg)
return (s16)((hi << 8) | lo);
}
+/* 12bits for 2G range, 13 bits for 4G range and 14 bits for 8G range */
+static s16 lis3lv02d_read_16(struct lis3lv02d *lis3, int reg)
+{
+ u8 lo, hi;
+ int v;
+
+ lis3->read(lis3, reg - 1, &lo);
+ lis3->read(lis3, reg, &hi);
+ v = (int) ((hi << 8) | lo);
+
+ return (s16) v >> lis3->shift_adj;
+}
As the method reads 12, 13, or 14 bits, it's a bit tricky to call it
"*_read_16". I'd suggest maybe "lis3lv02d_read_12_14",
"lis3lv02d_read_adj_16", or even "lis331dlh_read_data". Pick the one you
prefer :-)
/**
* lis3lv02d_get_axis - For the given axis, give the value converted
* @axis: 1,2,3 - can also be negative
@@ -193,6 +214,7 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
static int lis3_12_rates[4] = {40, 160, 640, 2560};
static int lis3_8_rates[2] = {100, 400};
static int lis3_3dc_rates[16] = {0, 1, 10, 25, 50, 100, 200, 400, 1600, 5000};
+static int lis3_3dlh_rates[4] = {50, 100, 400, 1000};
/* ODR is Output Data Rate */
static int lis3lv02d_get_odr(struct lis3lv02d *lis3)
@@ -265,7 +287,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
(LIS3_IRQ1_DATA_READY | LIS3_IRQ2_DATA_READY));
}
- if (lis3->whoami == WAI_3DC) {
+ if ((lis3->whoami == WAI_3DC) || (lis3->whoami == WAI_3DLH)) {
ctlreg = CTRL_REG4;
selftest = CTRL4_ST0;
} else {
@@ -396,9 +418,17 @@ int lis3lv02d_poweron(struct lis3lv02d *lis3)
lis3->read(lis3, CTRL_REG2, ®);
if (lis3->whoami == WAI_12B)
reg |= CTRL2_BDU | CTRL2_BOOT;
+ else if (lis3->whoami == WAI_3DLH)
+ reg |= CTRL2_BOOT_3DLH;
else
reg |= CTRL2_BOOT_8B;
lis3->write(lis3, CTRL_REG2, reg);
+
+ if (lis3->whoami == WAI_3DLH) {
+ lis3->read(lis3, CTRL_REG4, ®);
+ reg |= CTRL4_BDU;
+ lis3->write(lis3, CTRL_REG4, reg);
+ }
}
err = lis3lv02d_get_pwron_wait(lis3);
@@ -954,6 +984,16 @@ int lis3lv02d_init_device(struct lis3lv02d *lis3)
lis3->odr_mask = CTRL1_ODR0|CTRL1_ODR1|CTRL1_ODR2|CTRL1_ODR3;
lis3->scale = LIS3_SENSITIVITY_8B;
break;
+ case WAI_3DLH:
+ pr_info("16 bits 3DLH sensor found\n");
+ lis3->read_data = lis3lv02d_read_16;
+ lis3->mdps_max_val = 2048; /* 12 bits for 2G */
+ lis3->shift_adj = SHIFT_ADJ_2G;
+ lis3->pwron_delay = LIS3_PWRON_DELAY_WAI_8B;
+ lis3->odrs = lis3_3dlh_rates;
+ lis3->odr_mask = CTRL1_DR0 | CTRL1_DR1;
+ lis3->scale = LIS3DLH_SENSITIVITY_2G;
+ break;
default:
pr_err("unknown sensor type 0x%X\n", lis3->whoami);
return -EINVAL;
diff --git a/drivers/misc/lis3lv02d/lis3lv02d.h b/drivers/misc/lis3lv02d/lis3lv02d.h
index 2b1482a..c1a545e 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.h
+++ b/drivers/misc/lis3lv02d/lis3lv02d.h
@@ -26,12 +26,12 @@
/*
* This driver tries to support the "digital" accelerometer chips from
* STMicroelectronics such as LIS3LV02DL, LIS302DL, LIS3L02DQ, LIS331DL,
- * LIS35DE, or LIS202DL. They are very similar in terms of programming, with
- * almost the same registers. In addition to differing on physical properties,
- * they differ on the number of axes (2/3), precision (8/12 bits), and special
- * features (freefall detection, click...). Unfortunately, not all the
- * differences can be probed via a register.
- * They can be connected either via IÂC or SPI.
+ * LIS331DLH, LIS35DE, or LIS202DL. They are very similar in terms of
+ * programming, with almost the same registers. In addition to differing
+ * on physical properties, they differ on the number of axes (2/3),
+ * precision (8/12 bits), and special features (freefall detection,
+ * click...). Unfortunately, not all the differences can be probed via
+ * a register. They can be connected either via IÂC or SPI.
*/
#include <linux/lis3lv02d.h>
@@ -96,12 +96,22 @@ enum lis3lv02d_reg {
};
enum lis3_who_am_i {
+ WAI_3DLH = 0x32, /* 16 bits: LIS331DLH */
WAI_3DC = 0x33, /* 8 bits: LIS3DC, HP3DC */
WAI_12B = 0x3A, /* 12 bits: LIS3LV02D[LQ]... */
WAI_8B = 0x3B, /* 8 bits: LIS[23]02D[LQ]... */
WAI_6B = 0x52, /* 6 bits: LIS331DLF - not supported */
};
+enum lis3_type {
+ LIS3DC,
+ HP3DC,
+ LIS3LV02D,
+ LIS2302D,
+ LIS331DLF,
+ LIS331DLH,
+};
+
enum lis3lv02d_ctrl1_12b {
CTRL1_Xen = 0x01,
CTRL1_Yen = 0x02,
@@ -129,6 +139,27 @@ enum lis3lv02d_ctrl1_3dc {
CTRL1_ODR3 = 0x80,
};
+enum lis331dlh_ctrl1 {
+ CTRL1_DR0 = 0x08,
+ CTRL1_DR1 = 0x10,
+ CTRL1_PM0 = 0x20,
+ CTRL1_PM1 = 0x40,
+ CTRL1_PM2 = 0x80,
+};
+
+enum lis331dlh_ctrl2 {
+ CTRL2_HPEN1 = 0x04,
+ CTRL2_HPEN2 = 0x08,
+ CTRL2_FDS_3DLH = 0x10,
+ CTRL2_BOOT_3DLH = 0x80,
+};
+
+enum lis331dlh_ctrl4 {
+ CTRL4_STSIGN = 0x08,
+ CTRL4_BLE = 0x40,
+ CTRL4_BDU = 0x80,
+};
+
enum lis3lv02d_ctrl2 {
CTRL2_DAS = 0x01,
CTRL2_SIM = 0x02,
@@ -279,6 +310,7 @@ struct lis3lv02d {
int data_ready_count[2];
atomic_t wake_thread;
unsigned char irq_cfg;
+ unsigned int shift_adj;
struct lis3lv02d_platform_data *pdata; /* for passing board config */
struct mutex mutex; /* Serialize poll and selftest */
diff --git a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
index c02fea0..98cf9ba 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
@@ -90,7 +90,11 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
if (ret < 0)
return ret;
- reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen;
+ if (lis3->whoami == WAI_3DLH)
+ reg |= CTRL1_PM0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen;
+ else
+ reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen;
+
return lis3->write(lis3, CTRL_REG1, reg);
}
@@ -232,6 +236,7 @@ static int lis3_i2c_runtime_resume(struct device *dev)
static const struct i2c_device_id lis3lv02d_id[] = {
{"lis3lv02d", 0 },
+ {"lis331dlh", LIS331DLH},
I'm not fully grasping the meaning of this table. But as there seems to
be no user of the second value, I imagine this value just has to be
different for each entry? If so, I'd recommend to change the 0 to
LIS3LV02D, to make it clear that LIS331DLH != 0. Or is this value
meaningful for the user, in which case we cannot change it anymore?
Cheers,
Ãric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/