linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
@ 2012-08-22  6:30 AnilKumar Ch
  2012-08-22  7:48 ` Arnd Bergmann
  2012-09-23 21:38 ` Éric Piel
  0 siblings, 2 replies; 14+ messages in thread
From: AnilKumar Ch @ 2012-08-22  6:30 UTC (permalink / raw)
  To: arnd
  Cc: gregkh, eric.piel, jic23, greg, akpm, broonie, dmitry.torokhov,
	linux-kernel, AnilKumar Ch

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.

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
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@gmail.com>
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
+
 #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;
+}
+
 /**
  * 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, &reg);
 		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);
+			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},
 	{}
 };
 
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22  6:30 [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer AnilKumar Ch
@ 2012-08-22  7:48 ` Arnd Bergmann
  2012-08-22  8:44   ` Chinmay V S
  2012-08-28  5:45   ` AnilKumar, Chimata
  2012-09-23 21:38 ` Éric Piel
  1 sibling, 2 replies; 14+ messages in thread
From: Arnd Bergmann @ 2012-08-22  7:48 UTC (permalink / raw)
  To: AnilKumar Ch
  Cc: gregkh, eric.piel, jic23, greg, akpm, broonie, dmitry.torokhov,
	linux-kernel

On Wednesday 22 August 2012, 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.
> 
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22  7:48 ` Arnd Bergmann
@ 2012-08-22  8:44   ` Chinmay V S
  2012-08-22  9:00     ` AnilKumar, Chimata
  2012-08-28  5:45   ` AnilKumar, Chimata
  1 sibling, 1 reply; 14+ messages in thread
From: Chinmay V S @ 2012-08-22  8:44 UTC (permalink / raw)
  To: AnilKumar Ch
  Cc: Arnd Bergmann, gregkh, eric.piel, jic23, greg, akpm, broonie,
	dmitry.torokhov, linux-kernel

Hi All,

A few nitpicks.

> + * 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
Shouldn't it be "1LSB is 4000/1024 mG" ?
Also "outdata in 12bits" (typo. in->is?)

On a more technical note, now that LIS3331DLH has 16bit resolution,
why don't we simply return the entire 16-bit value in
lis3lv02d_read_16(). The fact that lis3lv02d_read_16() has 16-bit
resolution can be indicated by

@@ -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 = 32768; /* 16 bits for +/-2G */


-- 
regards
CVS

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22  8:44   ` Chinmay V S
@ 2012-08-22  9:00     ` AnilKumar, Chimata
  2012-08-22 10:39       ` Chinmay V S
  0 siblings, 1 reply; 14+ messages in thread
From: AnilKumar, Chimata @ 2012-08-22  9:00 UTC (permalink / raw)
  To: Chinmay V S
  Cc: Arnd Bergmann, gregkh, eric.piel, jic23, greg, akpm, broonie,
	dmitry.torokhov, linux-kernel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1793 bytes --]

Hi Chinmay,

Thanks for the comments

On Wed, Aug 22, 2012 at 14:14:55, Chinmay V S wrote:
> Hi All,
> 
> A few nitpicks.
> 
> > + * 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
> Shouldn't it be "1LSB is 4000/1024 mG" ?

Typo mistake this should be 4G/4096

> Also "outdata in 12bits" (typo. in->is?)
> 

If we look at the lis331dlh datasheet 12 bits outdata for +/- 2G resolution
range and 13 bits for +/- 4G range and 14 bits for +/- 8G range. So output
data is only 12 bits.

> On a more technical note, now that LIS3331DLH has 16bit resolution,
> why don't we simply return the entire 16-bit value in
> lis3lv02d_read_16(). The fact that lis3lv02d_read_16() has 16-bit
> resolution can be indicated by

Depending on the range 2 or 4 or 8 we have to pass the outdata that is
handled by using the shift_adj member to shift the outdata to corresponding
bits 12/13/14.

> 
> @@ -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 = 32768; /* 16 bits for +/-2G */

This driver supports only +/-2G range of values so it is limited to 2048.
With this we do not have the runtime change of G range so by default 2G
is added.

Regards
AnilKumar
 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22  9:00     ` AnilKumar, Chimata
@ 2012-08-22 10:39       ` Chinmay V S
  2012-08-22 15:40         ` AnilKumar, Chimata
  0 siblings, 1 reply; 14+ messages in thread
From: Chinmay V S @ 2012-08-22 10:39 UTC (permalink / raw)
  To: AnilKumar, Chimata
  Cc: Arnd Bergmann, gregkh, eric.piel, jic23, greg, akpm, broonie,
	dmitry.torokhov, linux-kernel

Hmmm. Interesting. As i understand LIS331DLH provides 16bit data
irrespective of the full-scale/sensitivity configuration. Hence we
could effectively map +/-2G to +/-32768(signed 16bit 2's complement).
According to the current-patch right-shifting the register values by
4(i.e. reducing 16bit --> 12bit) will mean that we lose accuracy by
~1mG.

Clearly this will NOT affect use-case like display-orientation in
smart-phones, but surely medical and industrial applications WILL
benefit from the additional accuracy by utilising the entire 16-bit
resolution provided by LIS331DLH hardware.

I went through the LIS331DLH datasheet/application-note from
http://www.st.com/internet/analog/product/218132.jsp and i'm a bit
confused from your statement about +/-2G being 12bit data. Nowhere is
it mentioned that LIS331DLH provides +/-2G|+/-4G|+/-8G as 12|13|14 bit
data respectively. Then again i might be wrong...

-- 
regards
ChinmayVS

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22 10:39       ` Chinmay V S
@ 2012-08-22 15:40         ` AnilKumar, Chimata
  2012-08-22 16:54           ` Chinmay V S
  0 siblings, 1 reply; 14+ messages in thread
From: AnilKumar, Chimata @ 2012-08-22 15:40 UTC (permalink / raw)
  To: Chinmay V S
  Cc: Arnd Bergmann, gregkh, eric.piel, jic23, greg, akpm, broonie,
	dmitry.torokhov, linux-kernel

On Wed, Aug 22, 2012 at 16:09:22, Chinmay V S wrote:
> Hmmm. Interesting. As i understand LIS331DLH provides 16bit data
> irrespective of the full-scale/sensitivity configuration. Hence we
> could effectively map +/-2G to +/-32768(signed 16bit 2's complement).
> According to the current-patch right-shifting the register values by
> 4(i.e. reducing 16bit --> 12bit) will mean that we lose accuracy by
> ~1mG.
> 
> Clearly this will NOT affect use-case like display-orientation in
> smart-phones, but surely medical and industrial applications WILL
> benefit from the additional accuracy by utilising the entire 16-bit
> resolution provided by LIS331DLH hardware.
> 
> I went through the LIS331DLH datasheet/application-note from
> http://www.st.com/internet/analog/product/218132.jsp and i'm a bit
> confused from your statement about +/-2G being 12bit data. Nowhere is
> it mentioned that LIS331DLH provides +/-2G|+/-4G|+/-8G as 12|13|14 bit
> data respectively. Then again i might be wrong...
> 

Look at this application note which talks about the outdata values
for 2G range (page 12/31) http://www.st.com/internet/com/TECHNICAL_RESOURCES/TECHNICAL_LITERATURE/APPLICATION_NOTE/CD00215823.pdf

Corresponding to the 4G and 8G I got the details form older
patches (SHIFT_ADJ_4G and SHIFT_ADJ_8G).
http://driverdev.linuxdriverproject.org/pipermail/devel/2010-November/009685.html

We can easily interpret number of bits for 4G and 8G from 2G
information.

Thanks
AnilKumar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22 15:40         ` AnilKumar, Chimata
@ 2012-08-22 16:54           ` Chinmay V S
  2012-08-23 10:00             ` AnilKumar, Chimata
  0 siblings, 1 reply; 14+ messages in thread
From: Chinmay V S @ 2012-08-22 16:54 UTC (permalink / raw)
  To: AnilKumar, Chimata, carmine.iascone
  Cc: Arnd Bergmann, gregkh, eric.piel, jic23, greg, akpm, broonie,
	dmitry.torokhov, linux-kernel

> Look at this application note which talks about the outdata values
> for 2G range (page 12/31) http://www.st.com/internet/com/TECHNICAL_RESOURCES/TECHNICAL_LITERATURE/APPLICATION_NOTE/CD00215823.pdf

Had been through the application note earlier. The table5 (on page 12)
that you refer to, does NOT contradict either 12/16bit, as in all the
examples the lower 4 bits are zero. So i don't see how one can assume
from these examples that for +/-2G they one should consider 12/16bits.
A nice side-effect of using 12|13|14bits for +/-2|4|8G is that the
values returned by the driver are in mG in all the 3 modes.

> Corresponding to the 4G and 8G I got the details form older
> patches (SHIFT_ADJ_4G and SHIFT_ADJ_8G).
> http://driverdev.linuxdriverproject.org/pipermail/devel/2010-November/009685.html
>
> We can easily interpret number of bits for 4G and 8G from 2G
> information.

Going through the code of this driver i can see what you are talking
about. Depending on the full-scale-range the device is being
configured for, the number of bits used to represent acceleration in
the driver is changed.

Again judging from the code, the driver is always returning
acceleration at a constant accuracy i.e. 1mG in all the 3 modes
(+/-2|4|8G)i.e.
+/-2G is mode means value can be anywhere from +/-2048mG,
(requiring 12bits.)
+/-4G in the range of +/-4096mG, requiring 13bits.
+/-8G i.e. +/-8192mG, requiring 14bits.

Was this done...

a. ...because LIS331DLH's theoretical MAX accuracy is ~1mG
If yes, then using 12bits is fine.

-OR-

b. ...so that the driver will report values at a constant
scale(i.e.mG) regardless of the mode?
If yes, then maybe we could consider using the additional bits to
obtain the maximum possible accuracy the LIS331DLH supports rather
than choosing to discard the LSBits.

(Can anyone please confirm the above?)

Going through the datasheet of LIS331DLH,
http://www.st.com/internet/com/TECHNICAL_RESOURCES/TECHNICAL_LITERATURE/DATASHEET/CD00213470.pdf
we see that accuracy/sensitivity of the device as per Table 2.1
(page9/38) is 1|2|4mG in +/-2|4|8Gmodes respectively. It is mentioned
in the footnotes that the readings were taken at 12bit resolution(a
point in favour of using 12bits ourselves). But its is still NOT clear
whether the accuracy/sensitivity are the LIS331DLH's physical limits
or whether they stem from the fact that ONLY 12bits were used during
the rating tests.

I hope i'm not being too much of a PIA. My sole intention is to see
that we do NOT unintentionally lose track of the device capabilities
in the midst of all these driver iterations. Thank you for your
patience.

PS: One more nitpick
> LIS3331DLH spec says 1LSBs(...)
LIS3331DLH --> LIS331DLH

-- 
regards
ChinmayVS

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22 16:54           ` Chinmay V S
@ 2012-08-23 10:00             ` AnilKumar, Chimata
  2012-08-23 10:23               ` Chinmay V S
  0 siblings, 1 reply; 14+ messages in thread
From: AnilKumar, Chimata @ 2012-08-23 10:00 UTC (permalink / raw)
  To: Chinmay V S, carmine.iascone
  Cc: Arnd Bergmann, gregkh, eric.piel, jic23, greg, akpm, broonie,
	dmitry.torokhov, linux-kernel

On Wed, Aug 22, 2012 at 22:24:10, Chinmay V S wrote:
> > Look at this application note which talks about the outdata values
> > for 2G range (page 12/31) http://www.st.com/internet/com/TECHNICAL_RESOURCES/TECHNICAL_LITERATURE/APPLICATION_NOTE/CD00215823.pdf
> 
> Had been through the application note earlier. The table5 (on page 12)
> that you refer to, does NOT contradict either 12/16bit, as in all the
> examples the lower 4 bits are zero. So i don't see how one can assume
> from these examples that for +/-2G they one should consider 12/16bits.
> A nice side-effect of using 12|13|14bits for +/-2|4|8G is that the
> values returned by the driver are in mG in all the 3 modes.
> 
> > Corresponding to the 4G and 8G I got the details form older
> > patches (SHIFT_ADJ_4G and SHIFT_ADJ_8G).
> > http://driverdev.linuxdriverproject.org/pipermail/devel/2010-November/009685.html
> >
> > We can easily interpret number of bits for 4G and 8G from 2G
> > information.
> 
> Going through the code of this driver i can see what you are talking
> about. Depending on the full-scale-range the device is being
> configured for, the number of bits used to represent acceleration in
> the driver is changed.
> 
> Again judging from the code, the driver is always returning
> acceleration at a constant accuracy i.e. 1mG in all the 3 modes
> (+/-2|4|8G)i.e.
> +/-2G is mode means value can be anywhere from +/-2048mG,
> (requiring 12bits.)
> +/-4G in the range of +/-4096mG, requiring 13bits.
> +/-8G i.e. +/-8192mG, requiring 14bits.
> 
> Was this done...
> 
> a. ...because LIS331DLH's theoretical MAX accuracy is ~1mG
> If yes, then using 12bits is fine.
> 

Note from datasheet, "1LSb=4g/4096 at 12bit representation,
±2g Full-scale"

>From this I understood that ±2G full scale value is 12 bits.
That is one more reason to take 12bit value.

Regards
AnilKumar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-23 10:00             ` AnilKumar, Chimata
@ 2012-08-23 10:23               ` Chinmay V S
  2012-08-23 11:15                 ` AnilKumar, Chimata
  0 siblings, 1 reply; 14+ messages in thread
From: Chinmay V S @ 2012-08-23 10:23 UTC (permalink / raw)
  To: AnilKumar, Chimata
  Cc: carmine.iascone, Arnd Bergmann, gregkh, eric.piel, jic23, greg,
	akpm, broonie, dmitry.torokhov, linux-kernel

> Note from datasheet, "1LSb=4g/4096 at 12bit representation,
> ±2g Full-scale"

Precisely my point. All the datasheet says is :
1. It is +/-2G mode --> so Numerator is 4G.
2. We are using 12bits --> so Denominator is 2^12 = 4096.

There is no clear reason/justification as to why 12bits was chosen in
the first place. At this point unless someone from the
design/original-driver team actually confirms why the 12bit
representation was chosen, it is all conjecture on our part.

If you have the actual LIS331DLH hardware, can you verify if the lower
4bits do actually contain any data (or are they always zero). This
will help us decide whether to use them(16bit mode) or discard
them(12bit mode)?...

regards
ChinmayVS

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-23 10:23               ` Chinmay V S
@ 2012-08-23 11:15                 ` AnilKumar, Chimata
  2012-08-23 11:33                   ` Chinmay V S
  0 siblings, 1 reply; 14+ messages in thread
From: AnilKumar, Chimata @ 2012-08-23 11:15 UTC (permalink / raw)
  To: Chinmay V S
  Cc: carmine.iascone, Arnd Bergmann, gregkh, eric.piel, jic23, greg,
	akpm, broonie, dmitry.torokhov, linux-kernel

Chinmay,

On Thu, Aug 23, 2012 at 15:53:10, Chinmay V S wrote:
> > Note from datasheet, "1LSb=4g/4096 at 12bit representation,
> > ±2g Full-scale"
> 
> Precisely my point. All the datasheet says is :
> 1. It is +/-2G mode --> so Numerator is 4G.
> 2. We are using 12bits --> so Denominator is 2^12 = 4096.
> 
> There is no clear reason/justification as to why 12bits was chosen in
> the first place. At this point unless someone from the
> design/original-driver team actually confirms why the 12bit
> representation was chosen, it is all conjecture on our part.
> 
> If you have the actual LIS331DLH hardware, can you verify if the lower
> 4bits do actually contain any data (or are they always zero). This
> will help us decide whether to use them(16bit mode) or discard
> them(12bit mode)?...
> 

This is the outdata of accelerometer

root@am335xevm:~# cat /sys/devices/platform/lis3lv02d/position
[   16.323852] lis3lv02d: hi 0x1    low 0x50   val 0x15
[   16.329742] lis3lv02d: hi 0x0    low 0x0    val 0x0
[   16.335052] lis3lv02d: hi 0x3e   low 0x50   val 0x3e5
(20,0,973)
root@am335xevm:~# cat /sys/devices/platform/lis3lv02d/position
[   25.777343] lis3lv02d: hi 0x2    low 0xa0   val 0x2a
[   25.783203] lis3lv02d: hi 0xc1   low 0xf0   val 0xfffffc1f
[   25.790130] lis3lv02d: hi 0xfd   low 0x50   val 0xffffffd5
(41,-969,-41)
root@am335xevm:~# cat /sys/devices/platform/lis3lv02d/position
[   47.607330] lis3lv02d: hi 0xc0   low 0x60   val 0xfffffc06
[   47.613464] lis3lv02d: hi 0xfd   low 0x90   val 0xffffffd9
[   47.619934] lis3lv02d: hi 0x2    low 0x40   val 0x24
(-994,-38,35)

Lower nibble always "0"

Regards
AnilKumar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-23 11:15                 ` AnilKumar, Chimata
@ 2012-08-23 11:33                   ` Chinmay V S
  0 siblings, 0 replies; 14+ messages in thread
From: Chinmay V S @ 2012-08-23 11:33 UTC (permalink / raw)
  To: AnilKumar, Chimata
  Cc: Arnd Bergmann, gregkh, eric.piel, jic23, greg, akpm, broonie,
	dmitry.torokhov, linux-kernel

12bit it IS then. :-)
LGTM. +1.

On Thu, Aug 23, 2012 at 4:45 PM, AnilKumar, Chimata <anilkumar@ti.com> wrote:
> Chinmay,
>
> On Thu, Aug 23, 2012 at 15:53:10, Chinmay V S wrote:
>> > Note from datasheet, "1LSb=4g/4096 at 12bit representation,
>> > ±2g Full-scale"
>>
>> Precisely my point. All the datasheet says is :
>> 1. It is +/-2G mode --> so Numerator is 4G.
>> 2. We are using 12bits --> so Denominator is 2^12 = 4096.
>>
>> There is no clear reason/justification as to why 12bits was chosen in
>> the first place. At this point unless someone from the
>> design/original-driver team actually confirms why the 12bit
>> representation was chosen, it is all conjecture on our part.
>>
>> If you have the actual LIS331DLH hardware, can you verify if the lower
>> 4bits do actually contain any data (or are they always zero). This
>> will help us decide whether to use them(16bit mode) or discard
>> them(12bit mode)?...
>>
>
> This is the outdata of accelerometer
>
> root@am335xevm:~# cat /sys/devices/platform/lis3lv02d/position
> [   16.323852] lis3lv02d: hi 0x1    low 0x50   val 0x15
> [   16.329742] lis3lv02d: hi 0x0    low 0x0    val 0x0
> [   16.335052] lis3lv02d: hi 0x3e   low 0x50   val 0x3e5
> (20,0,973)
> root@am335xevm:~# cat /sys/devices/platform/lis3lv02d/position
> [   25.777343] lis3lv02d: hi 0x2    low 0xa0   val 0x2a
> [   25.783203] lis3lv02d: hi 0xc1   low 0xf0   val 0xfffffc1f
> [   25.790130] lis3lv02d: hi 0xfd   low 0x50   val 0xffffffd5
> (41,-969,-41)
> root@am335xevm:~# cat /sys/devices/platform/lis3lv02d/position
> [   47.607330] lis3lv02d: hi 0xc0   low 0x60   val 0xfffffc06
> [   47.613464] lis3lv02d: hi 0xfd   low 0x90   val 0xffffffd9
> [   47.619934] lis3lv02d: hi 0x2    low 0x40   val 0x24
> (-994,-38,35)
>
> Lower nibble always "0"
>
> Regards
> AnilKumar

-- 
regards
ChinmayVS

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22  7:48 ` Arnd Bergmann
  2012-08-22  8:44   ` Chinmay V S
@ 2012-08-28  5:45   ` AnilKumar, Chimata
  1 sibling, 0 replies; 14+ messages in thread
From: AnilKumar, Chimata @ 2012-08-28  5:45 UTC (permalink / raw)
  To: eric.piel, Arnd Bergmann
  Cc: gregkh, jic23, greg, akpm, broonie, dmitry.torokhov, linux-kernel

Hi Eric,

On Wed, Aug 22, 2012 at 13:18:38, Arnd Bergmann wrote:
> On Wednesday 22 August 2012, 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.
> > 
> > Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 

If there are no further comments could you please push
this patch?

Thanks
AnilKumar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-08-22  6:30 [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer AnilKumar Ch
  2012-08-22  7:48 ` Arnd Bergmann
@ 2012-09-23 21:38 ` Éric Piel
  2012-09-24  5:46   ` AnilKumar, Chimata
  1 sibling, 1 reply; 14+ messages in thread
From: Éric Piel @ 2012-09-23 21:38 UTC (permalink / raw)
  To: AnilKumar Ch
  Cc: arnd, gregkh, jic23, greg, akpm, broonie, dmitry.torokhov, linux-kernel

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@tremplin-utc.net>


>
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
> 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@gmail.com>
> 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, &reg);
>   		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);
> +			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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer
  2012-09-23 21:38 ` Éric Piel
@ 2012-09-24  5:46   ` AnilKumar, Chimata
  0 siblings, 0 replies; 14+ messages in thread
From: AnilKumar, Chimata @ 2012-09-24  5:46 UTC (permalink / raw)
  To: Éric Piel
  Cc: arnd, gregkh, jic23, greg, akpm, broonie, dmitry.torokhov, linux-kernel

Hi Eric,

Thanks for the comments,

On Mon, Sep 24, 2012 at 03:08:19, Éric Piel wrote:
> 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?

As I can see this patch is already in linux-next, I will submit
a new patch by addressing all your comments.

> 
> Here is my:
> Reviewed-by: Éric Piel <eric.piel@tremplin-utc.net>
> 
> 
> >
> > Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> > ---
> > 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@gmail.com>
> > 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.

I will do this

> 
> 
> >   #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 :-)

Comment is available to convey the number of bits so I am changing it to
"lis331dlh_read_data".

> 
> 
> >   /**
> >    * 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, &reg);
> >   		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);
> > +			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");

I am planning to change this print message to
"LIS331DLH sensor found"

> > +		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?
> 

I will change this and update the enum lis3_type accordingly, which
has these macros

Thanks
AnilKumar


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-09-24  5:46 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-22  6:30 [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer AnilKumar Ch
2012-08-22  7:48 ` Arnd Bergmann
2012-08-22  8:44   ` Chinmay V S
2012-08-22  9:00     ` AnilKumar, Chimata
2012-08-22 10:39       ` Chinmay V S
2012-08-22 15:40         ` AnilKumar, Chimata
2012-08-22 16:54           ` Chinmay V S
2012-08-23 10:00             ` AnilKumar, Chimata
2012-08-23 10:23               ` Chinmay V S
2012-08-23 11:15                 ` AnilKumar, Chimata
2012-08-23 11:33                   ` Chinmay V S
2012-08-28  5:45   ` AnilKumar, Chimata
2012-09-23 21:38 ` Éric Piel
2012-09-24  5:46   ` AnilKumar, Chimata

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).