Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Mark Brown]

On Fri, Dec 23, 2011 at 10:54:52AM +0530, AnilKumar Ch wrote:
> This patch adds support for lis33ldlh digital accelerometer to the
> lis3lv02d driver family. Adds ID field for detecting the lis33ldlh
> module, based on this ID field lis3lv02d driver will export the
> lis33ldlh module functionality.

You shouldn't send cover letters for single patches, if there's anything
that needs saying it should be in the changelog for the patch.

RE: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[AnilKumar, Chimata]

Hi Mark,

On Fri, Dec 23, 2011 at 16:48:08, Mark Brown wrote:
> On Fri, Dec 23, 2011 at 10:54:52AM +0530, AnilKumar Ch wrote:
> > This patch adds support for lis33ldlh digital accelerometer to the
> > lis3lv02d driver family. Adds ID field for detecting the lis33ldlh
> > module, based on this ID field lis3lv02d driver will export the
> > lis33ldlh module functionality.
> 
> You shouldn't send cover letters for single patches, if there's anything
> that needs saying it should be in the changelog for the patch.

Will take care from next time onwards

Regards
AnilKumar

RE: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[AnilKumar, Chimata]

Hi All,

Recalling the patch, provide the comments if there are any if not please include
this patch to v3.3 kernel.

-----Original Message-----
From: AnilKumar, Chimata 
Sent: Friday, December 23, 2011 10:55 AM
To: arnd@arndb.de; greg@kroah.com; eric.piel@tremplin-utc.net; akpm@linux-foundation.org; broonie@opensource.wolfsonmicro.com; linux-kernel@vger.kernel.org
Cc: linux-omap@vger.kernel.org; Nori, Sekhar; AnilKumar, Chimata
Subject: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

This patch adds support for lis33ldlh digital accelerometer to the
lis3lv02d driver family. Adds ID field for detecting the lis33ldlh
module, based on this ID field lis3lv02d driver will export the
lis33ldlh module functionality.

Also exports g_range parameter to user space for run-time value
change. User must give 2/4/8 value depends on requirement.

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
 drivers/misc/lis3lv02d/lis3lv02d.c     |  111 ++++++++++++++++++++++++++++++-
 drivers/misc/lis3lv02d/lis3lv02d.h     |   59 +++++++++++++++++
 drivers/misc/lis3lv02d/lis3lv02d_i2c.c |    6 ++-
 include/linux/lis3lv02d.h              |    2 +
 4 files changed, 173 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
index 29d12a7..2381220 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d.c
@@ -80,6 +80,17 @@
 #define LIS3_SENSITIVITY_12B		((LIS3_ACCURACY * 1000) / 1024)
 #define LIS3_SENSITIVITY_8B		(18 * LIS3_ACCURACY)
 
+/* Sensitivity values for -2G, -4G, -8G and +2G, +4G, +8G scale */
+#define LIS3DLH_SENSITIVITY_2G		(LIS3_ACCURACY * 1)
+#define LIS3DLH_SENSITIVITY_4G		(LIS3_ACCURACY * 2)
+#define LIS3DLH_SENSITIVITY_8G		((LIS3_ACCURACY * 39)/10)
+
+#define SHIFT_ADJ_2G			4
+#define SHIFT_ADJ_4G			3
+#define SHIFT_ADJ_8G			2
+
+#define FS_MASK				(0x3 << 4)
+
 #define LIS3_DEFAULT_FUZZ_12B		3
 #define LIS3_DEFAULT_FLAT_12B		3
 #define LIS3_DEFAULT_FUZZ_8B		1
@@ -148,6 +159,12 @@ static inline int lis3lv02d_get_axis(s8 axis, int hw_values[3])
 		return -hw_values[-axis - 1];
 }
 
+static int lis3lv02d_decode(u8 pl, u8 ph, int adj)
+{
+	s16 v = pl | ph << 8;
+	return (int) v >> adj;
+}
+
 /**
  * lis3lv02d_get_xyz - Get X, Y and Z axis values from the accelerometer
  * @lis3: pointer to the device struct
@@ -176,9 +193,24 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
 				position[i] = (s8)data[i * 2];
 		}
 	} else {
-		position[0] = lis3->read_data(lis3, OUTX);
-		position[1] = lis3->read_data(lis3, OUTY);
-		position[2] = lis3->read_data(lis3, OUTZ);
+		if (lis3_dev.whoami == WAI_3DLH) {
+			position[0] =
+				lis3lv02d_decode(lis3->read_data(lis3, OUTX_L),
+				lis3->read_data(lis3, OUTX_H),
+				lis3_dev.shift_adj);
+			position[1] =
+				lis3lv02d_decode(lis3->read_data(lis3, OUTY_L),
+				lis3->read_data(lis3, OUTY_H),
+				lis3_dev.shift_adj);
+			position[2] =
+				lis3lv02d_decode(lis3->read_data(lis3, OUTZ_L),
+				lis3->read_data(lis3, OUTZ_H),
+				lis3_dev.shift_adj);
+		} else {
+			position[0] = lis3->read_data(lis3, OUTX);
+			position[1] = lis3->read_data(lis3, OUTY);
+			position[2] = lis3->read_data(lis3, OUTZ);
+		}
 	}
 
 	for (i = 0; i < 3; i++)
@@ -193,6 +225,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 +298,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_dev.whoami == WAI_3DC) || (lis3_dev.whoami == WAI_3DLH)) {
 		ctlreg = CTRL_REG4;
 		selftest = CTRL4_ST0;
 	} else {
@@ -396,6 +429,8 @@ 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);
@@ -724,6 +759,36 @@ void lis3lv02d_joystick_disable(struct lis3lv02d *lis3)
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
 
+static void lis3lv02d_update_g_range(struct lis3lv02d *lis3)
+{
+	u8 reg;
+	u8 val;
+	u8 shift;
+
+	switch (lis3->g_range) {
+	case 8:
+		val = FS_8G_REGVAL;
+		shift = SHIFT_ADJ_8G;
+		lis3->scale = LIS3DLH_SENSITIVITY_8G;
+		break;
+	case 4:
+		val = FS_4G_REGVAL;
+		shift = SHIFT_ADJ_4G;
+		lis3->scale = LIS3DLH_SENSITIVITY_4G;
+		break;
+	case 2:
+	default:
+		val = FS_2G_REGVAL;
+		shift = SHIFT_ADJ_2G;
+		lis3->scale = LIS3DLH_SENSITIVITY_2G;
+		break;
+	}
+
+	lis3->shift_adj = shift;
+	lis3->read(lis3, CTRL_REG4, &reg);
+	lis3->write(lis3, CTRL_REG4, ((reg & ~FS_MASK) | val));
+}
+
 /* Sysfs stuff */
 static void lis3lv02d_sysfs_poweron(struct lis3lv02d *lis3)
 {
@@ -792,6 +857,13 @@ static ssize_t lis3lv02d_rate_show(struct device *dev,
 	return sprintf(buf, "%d\n", lis3lv02d_get_odr(lis3));
 }
 
+static ssize_t lis3lv02d_range_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	lis3lv02d_sysfs_poweron(&lis3_dev);
+	return sprintf(buf, "%d\n", lis3_dev.g_range);
+}
+
 static ssize_t lis3lv02d_rate_set(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -809,15 +881,33 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
 	return count;
 }
 
+static ssize_t lis3lv02d_range_set(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	unsigned long range;
+
+	if (strict_strtoul(buf, 0, &range))
+		return -EINVAL;
+
+	lis3_dev.g_range = range;
+	lis3lv02d_update_g_range(&lis3_dev);
+
+	return count;
+}
+
 static DEVICE_ATTR(selftest, S_IRUSR, lis3lv02d_selftest_show, NULL);
 static DEVICE_ATTR(position, S_IRUGO, lis3lv02d_position_show, NULL);
 static DEVICE_ATTR(rate, S_IRUGO | S_IWUSR, lis3lv02d_rate_show,
 					    lis3lv02d_rate_set);
+static DEVICE_ATTR(range, S_IRUGO | S_IWUSR, lis3lv02d_range_show,
+					    lis3lv02d_range_set);
 
 static struct attribute *lis3lv02d_attributes[] = {
 	&dev_attr_selftest.attr,
 	&dev_attr_position.attr,
 	&dev_attr_rate.attr,
+	&dev_attr_range.attr,
 	NULL
 };
 
@@ -954,6 +1044,19 @@ 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("8 bits 3DLH sensor found\n");
+		lis3->read_data = lis3lv02d_read_8;
+		lis3->mdps_max_val = 128;
+		lis3->pwron_delay = LIS3_PWRON_DELAY_WAI_8B;
+		lis3->odrs = lis3_3dlh_rates;
+		lis3->odr_mask = CTRL1_DR0 | CTRL1_DR1;
+		if (lis3->pdata) {
+			lis3->g_range = lis3->pdata->g_range;
+			lis3lv02d_update_g_range(lis3);
+		} else
+			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..0e6fe06 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.h
+++ b/drivers/misc/lis3lv02d/lis3lv02d.h
@@ -95,13 +95,29 @@ enum lis3lv02d_reg {
 	DD_THSE_H	= 0x3F,
 };
 
+enum lis331dlh_reg {
+	CTRL_REG5	= 0x24,
+	HP_FILTER_RESET_3DLH	= 0x25,
+	REFERENCE	= 0x26,
+};
+
 enum lis3_who_am_i {
+	WAI_3DLH	= 0x32,	/* 8 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 +145,32 @@ 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 lis331dlh_ctrl5 {
+	CTRL5_TURNON0	= 0x01,
+	CTRL5_TURNON1	= 0x20,
+};
+
 enum lis3lv02d_ctrl2 {
 	CTRL2_DAS	= 0x01,
 	CTRL2_SIM	= 0x02,
@@ -148,6 +190,13 @@ enum lis3lv02d_ctrl4_3dc {
 	CTRL4_FS1	= 0x20,
 };
 
+/* Measurement Range */
+enum lis3lv02d_fs {
+	FS_2G_REGVAL = 0x00,
+	FS_4G_REGVAL = 0x10,
+	FS_8G_REGVAL = 0x30,
+};
+
 enum lis302d_ctrl2 {
 	HP_FF_WU2	= 0x08,
 	HP_FF_WU1	= 0x04,
@@ -185,6 +234,10 @@ enum lis3lv02d_ff_wu_cfg {
 	FF_WU_CFG_AOI	= 0x80,
 };
 
+enum lis331dlh_ff_wu_cfg {
+	FF_WU_CFG_6D	= 0x40,
+};
+
 enum lis3lv02d_ff_wu_src {
 	FF_WU_SRC_XL	= 0x01,
 	FF_WU_SRC_XH	= 0x02,
@@ -206,6 +259,10 @@ enum lis3lv02d_dd_cfg {
 	DD_CFG_IEND	= 0x80,
 };
 
+enum lis331dlh_dd_cfg {
+	DD_CFG_6D	= 0x40,
+};
+
 enum lis3lv02d_dd_src {
 	DD_SRC_XL	= 0x01,
 	DD_SRC_XH	= 0x02,
@@ -282,6 +339,8 @@ struct lis3lv02d {
 
 	struct lis3lv02d_platform_data *pdata;	/* for passing board config */
 	struct mutex		mutex;     /* Serialize poll and selftest */
+	u8			g_range; /* Hold the g range */
+	u8			shift_adj;
 };
 
 int lis3lv02d_init_device(struct lis3lv02d *lis3);
diff --git a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
index c02fea0..32b322f 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c
@@ -90,7 +90,10 @@ 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 +235,7 @@ static int lis3_i2c_runtime_resume(struct device *dev)
 
 static const struct i2c_device_id lis3lv02d_id[] = {
 	{"lis3lv02d", 0 },
+	{"lis331dlh", LIS331DLH},
 	{}
 };
 
diff --git a/include/linux/lis3lv02d.h b/include/linux/lis3lv02d.h
index f1664c6..32d4912 100644
--- a/include/linux/lis3lv02d.h
+++ b/include/linux/lis3lv02d.h
@@ -25,6 +25,7 @@
  * @axis_x:		Sensor orientation remapping for x-axis
  * @axis_y:		Sensor orientation remapping for y-axis
  * @axis_z:		Sensor orientation remapping for z-axis
+ * @g_range:		Value contains the acceleration range, +/-2, +/-4 and +/-8
  * @driver_features:	Enable bits for different features. Disabled by default
  * @default_rate:	Default sampling rate. 0 means reset default
  * @setup_resources:	Interrupt line setup call back function
@@ -113,6 +114,7 @@ struct lis3lv02d_platform_data {
 	s8 axis_x;
 	s8 axis_y;
 	s8 axis_z;
+	u8 g_range;
 #define LIS3_USE_BLOCK_READ	0x02
 	u16 driver_features;
 	int default_rate;
-- 
1.7.0.4

Thanks
AnilKumar

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Mark Brown]

On Tue, Jan 17, 2012 at 07:32:47AM +0000, AnilKumar, Chimata wrote:

> Recalling the patch, provide the comments if there are any if not please include
> this patch to v3.3 kernel.

There's no way this is going to make it into the 3.3 kernel at this
point, we're almost at the end of the merge window.

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[greg]

On Tue, Jan 17, 2012 at 10:54:42AM +0000, Mark Brown wrote:
> On Tue, Jan 17, 2012 at 07:32:47AM +0000, AnilKumar, Chimata wrote:
> 
> > Recalling the patch, provide the comments if there are any if not please include
> > this patch to v3.3 kernel.
> 
> There's no way this is going to make it into the 3.3 kernel at this
> point, we're almost at the end of the merge window.

Agreed, the merge window is for subsystem maintainers, not everyone
else.  I'll queue this up for review for after 3.3-rc1 is out.

thanks,

greg k-h

RE: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[AnilKumar, Chimata]

Thanks Greg

On Tue, Jan 17, 2012 at 21:33:35, greg@kroah.com wrote:
> On Tue, Jan 17, 2012 at 10:54:42AM +0000, Mark Brown wrote:
> > On Tue, Jan 17, 2012 at 07:32:47AM +0000, AnilKumar, Chimata wrote:
> > 
> > > Recalling the patch, provide the comments if there are any if not please include
> > > this patch to v3.3 kernel.
> > 
> > There's no way this is going to make it into the 3.3 kernel at this
> > point, we're almost at the end of the merge window.
> 
> Agreed, the merge window is for subsystem maintainers, not everyone
> else.  I'll queue this up for review for after 3.3-rc1 is out.
> 
> thanks,
> 
> greg k-h
> 

Regards
AnilKumar

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Andrew Morton]

On Tue, 17 Jan 2012 07:32:47 +0000
"AnilKumar, Chimata" <anilkumar@ti.com> wrote:

> Hi All,
> 
> Recalling the patch, provide the comments if there are any if not please include
> this patch to v3.3 kernel.

The patch is all mangled by someone's email client.  Wordwrapping, mime
crap which my MUA can't decrypt, etc.

> -----Original Message-----
> From: AnilKumar, Chimata 
> Sent: Friday, December 23, 2011 10:55 AM
> To: arnd@arndb.de; greg@kroah.com; eric.piel@tremplin-utc.net; akpm@linux-foundation.org; broonie@opensource.wolfsonmicro.com; linux-kernel@vger.kernel.org
> Cc: linux-omap@vger.kernel.org; Nori, Sekhar; AnilKumar, Chimata

A pet peeve which I haven't told anyone about.  If you've cc'ed someone
on a patch then I want to cc them on the patch too.  That means I have
to add their Cc: lines to the changelog.  But such Cc: lines include
their real names.  By omitting their real names in the above fashion,
you cause extra hassle for me.  Ideally, you should add their Cc: to
the changelog as well as to the mail headers, to give thsoe people the
best chance of seeing what is happening with the patch.

>
> ..
>
> +static ssize_t lis3lv02d_range_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	lis3lv02d_sysfs_poweron(&lis3_dev);
> +	return sprintf(buf, "%d\n", lis3_dev.g_range);
> +}

Are these interfaces documented anywhere?  If so, please update that
documentation.  If not, why not ;)

> @@ -809,15 +881,33 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t lis3lv02d_range_set(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t count)
> +{
> +	unsigned long range;
> +
> +	if (strict_strtoul(buf, 0, &range))

checkpatch would have told you that strict_strtoul() is deprecated. 
Please always use checkpatch.

RE: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[AnilKumar, Chimata]

Hi Andrew,

Thanks for reviewing the patch

On Wed, Jan 18, 2012 at 05:17:02, Andrew Morton wrote:
> On Tue, 17 Jan 2012 07:32:47 +0000
> "AnilKumar, Chimata" <anilkumar@ti.com> wrote:
> 
> > Hi All,
> > 
> > Recalling the patch, provide the comments if there are any if not please include
> > this patch to v3.3 kernel.
> 
> The patch is all mangled by someone's email client.  Wordwrapping, mime
> crap which my MUA can't decrypt, etc.

My bad, in a hurry I have sent a mail by doing "reply to all" that makes
all this non-sense will not repeated again.
Thanks for your patience here.

...snip...

> 
> A pet peeve which I haven't told anyone about.  If you've cc'ed someone
> on a patch then I want to cc them on the patch too.  That means I have
> to add their Cc: lines to the changelog.  But such Cc: lines include
> their real names.  By omitting their real names in the above fashion,
> you cause extra hassle for me.  Ideally, you should add their Cc: to
> the changelog as well as to the mail headers, to give thsoe people the
> best chance of seeing what is happening with the patch.
> 

Above comment applies here as well.

> >
> > ..
> >
> > +static ssize_t lis3lv02d_range_show(struct device *dev,
> > +			       struct device_attribute *attr, char *buf)
> > +{
> > +	lis3lv02d_sysfs_poweron(&lis3_dev);
> > +	return sprintf(buf, "%d\n", lis3_dev.g_range);
> > +}
> 
> Are these interfaces documented anywhere?  If so, please update that
> documentation.  If not, why not ;)

Yes, these interfaces are documented at
"Documentation/misc-devices/lis3lv02d" but range is not included
there, which is added in this patch. Will be updated and send it
as a separate patch.

> 
> > @@ -809,15 +881,33 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
> >  	return count;
> >  }
> >  
> > +static ssize_t lis3lv02d_range_set(struct device *dev,
> > +				struct device_attribute *attr, const char *buf,
> > +				size_t count)
> > +{
> > +	unsigned long range;
> > +
> > +	if (strict_strtoul(buf, 0, &range))
> 
> checkpatch would have told you that strict_strtoul() is deprecated. 
> Please always use checkpatch.
> 

Somehow I missed this, I will update with "kstrtoul". I have to update
the same API in other set function.

> 

Thanks
AnilKumar

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Arnd Bergmann]

On Tuesday 17 January 2012, AnilKumar, Chimata wrote:
> Hi All,
> 
> Recalling the patch, provide the comments if there are any if not please include
> this patch to v3.3 kernel.

As Mark and Greg said, 3.4 would be appropriate.

> +static ssize_t lis3lv02d_range_set(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t count)
> +{
> +	unsigned long range;
> +
> +	if (strict_strtoul(buf, 0, &range))
> +		return -EINVAL;
> +
> +	lis3_dev.g_range = range;
> +	lis3lv02d_update_g_range(&lis3_dev);
> +
> +	return count;
> +}
> +
>  static DEVICE_ATTR(selftest, S_IRUSR, lis3lv02d_selftest_show, NULL);
>  static DEVICE_ATTR(position, S_IRUGO, lis3lv02d_position_show, NULL);
>  static DEVICE_ATTR(rate, S_IRUGO | S_IWUSR, lis3lv02d_rate_show,
>  					    lis3lv02d_rate_set);
> +static DEVICE_ATTR(range, S_IRUGO | S_IWUSR, lis3lv02d_range_show,
> +					    lis3lv02d_range_set);

I think you need to document this new attribute in the Documentation
directory, unless I missed the other patch doing this.

On a more general topic, do we have a kernel-wide policy for accelerometer
drivers? AFAICT, we currently have three subsystems that contain 
accelerometer drivers, plus a few ad-hoc ones like this, which is a
rather unpleasant situation. What I found are these:

$ git grep -l accelerometer drivers/ | manual_grep
drivers/hwmon/applesmc.c
   (one hwmon sysfs attribute for x/y/z)
drivers/input/misc/adxl34x.c
   (lots of sysfs attributes, input_report_key)
drivers/input/misc/cma3000_d0x.c
   (input_report_abs)
drivers/input/misc/kxtj9.c
   (input_report_abs, plus one aux sysfs attribute)
drivers/macintosh/ams/ams-core.c
   (one sysfs attribute for x/y/z)
drivers/misc/lis3lv02d/
   (multiple sysfs attributes)
drivers/platform/x86/hdaps.c
   (multiple sysfs attributes, only x/y)
drivers/platform/x86/hp_accel.c
   (hooks into drivers/misc/lis3lv02d/)
drivers/staging/iio/accel/kxsd9.c
   (iio)
drivers/staging/iio/accel/sca3000_core.c
   (iio plus extra attributes)

While I'm not blaming you for the current situation, but I think the
situation is no longer sustainable and we need to decide on one place
and interface for these to go in the long run so we don't grow even
more nonstandard interfaces.

Any opinions where they should live? input, iio or a new subsystem?

	Arnd

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Jean Delvare]

Hi Arnd and all,

The lis3lv02d driver was moved outside of drivers/hwmon to make it
clear that we (hwmon/lm-sensors people) are not involved in maintaining
this driver in any way. So please remove the lm-sensors list (and
myself) from this discussion.

Thanks,
-- 
Jean Delvare

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Arnd Bergmann]

On Wednesday 18 January 2012, Jean Delvare wrote:
> Hi Arnd and all,
> 
> The lis3lv02d driver was moved outside of drivers/hwmon to make it
> clear that we (hwmon/lm-sensors people) are not involved in maintaining
> this driver in any way. So please remove the lm-sensors list (and
> myself) from this discussion.

Ok, I see. What about the applesmc driver then? If we come up with
a solution that works for the other drivers, should the accelerometer
part of that be converted to use that interface?

	Arnd

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Jean Delvare]

On Wed, 18 Jan 2012 14:49:35 +0000, Arnd Bergmann wrote:
> On Wednesday 18 January 2012, Jean Delvare wrote:
> > Hi Arnd and all,
> > 
> > The lis3lv02d driver was moved outside of drivers/hwmon to make it
> > clear that we (hwmon/lm-sensors people) are not involved in maintaining
> > this driver in any way. So please remove the lm-sensors list (and
> > myself) from this discussion.
> 
> Ok, I see. What about the applesmc driver then? If we come up with
> a solution that works for the other drivers, should the accelerometer
> part of that be converted to use that interface?

Yes of course.

-- 
Jean Delvare

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Jonathan Cameron]

Arnd Bergmann <arnd@arndb.de> wrote:

>On Tuesday 17 January 2012, AnilKumar, Chimata wrote:
>> Hi All,
>> 
>> Recalling the patch, provide the comments if there are any if not
>please include
>> this patch to v3.3 kernel.
>
>As Mark and Greg said, 3.4 would be appropriate.
>
>> +static ssize_t lis3lv02d_range_set(struct device *dev,
>> +				struct device_attribute *attr, const char *buf,
>> +				size_t count)
>> +{
>> +	unsigned long range;
>> +
>> +	if (strict_strtoul(buf, 0, &range))
>> +		return -EINVAL;
>> +
>> +	lis3_dev.g_range = range;
>> +	lis3lv02d_update_g_range(&lis3_dev);
>> +
>> +	return count;
>> +}
>> +
>>  static DEVICE_ATTR(selftest, S_IRUSR, lis3lv02d_selftest_show,
>NULL);
>>  static DEVICE_ATTR(position, S_IRUGO, lis3lv02d_position_show,
>NULL);
>>  static DEVICE_ATTR(rate, S_IRUGO | S_IWUSR, lis3lv02d_rate_show,
>>  					    lis3lv02d_rate_set);
>> +static DEVICE_ATTR(range, S_IRUGO | S_IWUSR, lis3lv02d_range_show,
>> +					    lis3lv02d_range_set);
>
>I think you need to document this new attribute in the Documentation
>directory, unless I missed the other patch doing this.
>
>On a more general topic, do we have a kernel-wide policy for
>accelerometer
>drivers? AFAICT, we currently have three subsystems that contain 
>accelerometer drivers, plus a few ad-hoc ones like this, which is a
>rather unpleasant situation. What I found are these:
>
>$ git grep -l accelerometer drivers/ | manual_grep
>drivers/hwmon/applesmc.c
>   (one hwmon sysfs attribute for x/y/z)
>drivers/input/misc/adxl34x.c
>   (lots of sysfs attributes, input_report_key)
>drivers/input/misc/cma3000_d0x.c
>   (input_report_abs)
>drivers/input/misc/kxtj9.c
>   (input_report_abs, plus one aux sysfs attribute)
>drivers/macintosh/ams/ams-core.c
>   (one sysfs attribute for x/y/z)
>drivers/misc/lis3lv02d/
>   (multiple sysfs attributes)
>drivers/platform/x86/hdaps.c
>   (multiple sysfs attributes, only x/y)
>drivers/platform/x86/hp_accel.c
>   (hooks into drivers/misc/lis3lv02d/)
>drivers/staging/iio/accel/kxsd9.c
>   (iio)
>drivers/staging/iio/accel/sca3000_core.c
>   (iio plus extra attributes)
>
For what it is worth there are a total of nine iio acceleration drivers. Some are special purpose though.
>While I'm not blaming you for the current situation, but I think the
>situation is no longer sustainable and we need to decide on one place
>and interface for these to go in the long run so we don't grow even
>more nonstandard interfaces.
>
>Any opinions where they should live? input, iio or a new subsystem?
>
Personal thought is that straight 3d devices very directed at input should go there. Iio has a few wrinkles left for the in kernel interfaces needed for a bridge to input. For starters the pull and push data interfaces have not merged yet (in staging) and we have not started on in kernel acess to non data events yet(thresholds).  Still happy to take them when we can support usecases fully. 
Of course any help getting there would be most welcome!

Jonathan

>	Arnd

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Arnd Bergmann]

On Wednesday 18 January 2012, Jonathan Cameron wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> >Any opinions where they should live? input, iio or a new subsystem?
> >
> Personal thought is that straight 3d devices very directed at input
> should go there. Iio has a few wrinkles left for the in kernel
> interfaces needed for a bridge to input. For starters the pull 
> and push data interfaces have not merged yet (in staging) and we
> have not started on in kernel acess to non data events yet(thresholds).
> Still happy to take them when we can support usecases fully. 

Ok, thanks for the information, sounds all reasonable. I think we
can definitely say no to new accelerometer drivers in drivers/misc
then and refer them to iio and input.

In case of lis33ldlh, I would like to see it move out at some point
once the necessary parts of iio have graduated out of staging.
Hopefully it won't be a large amount of work to convert it to
a different internal API.

AnilKumar, what are the existing users of the driver? Is it still
possible to convert them to use iio without causing problems for
people that rely on this driver, or do we have to keep supporting the
current interface?

	Arnd

RE: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[AnilKumar, Chimata]

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

Hi Arnd,

On Wed, Jan 18, 2012 at 22:41:15, Arnd Bergmann wrote:
> On Wednesday 18 January 2012, Jonathan Cameron wrote:
> > Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > >Any opinions where they should live? input, iio or a new subsystem?
> > >
> > Personal thought is that straight 3d devices very directed at input
> > should go there. Iio has a few wrinkles left for the in kernel
> > interfaces needed for a bridge to input. For starters the pull 
> > and push data interfaces have not merged yet (in staging) and we
> > have not started on in kernel acess to non data events yet(thresholds).
> > Still happy to take them when we can support usecases fully. 
> 
> Ok, thanks for the information, sounds all reasonable. I think we
> can definitely say no to new accelerometer drivers in drivers/misc
> then and refer them to iio and input.
> 
> In case of lis33ldlh, I would like to see it move out at some point
> once the necessary parts of iio have graduated out of staging.
> Hopefully it won't be a large amount of work to convert it to
> a different internal API.
> 
> AnilKumar, what are the existing users of the driver? Is it still
> possible to convert them to use iio without causing problems for
> people that rely on this driver, or do we have to keep supporting the
> current interface?

Android userspace running on TI AM335x EVM is using the interface
provided by lis3lv02d. They were asking some more interfaces from
lis3lvo2d driver.

There are multiple ways we can interface accelerometer to Android layers,
which is implemented on hardware abstraction layer (HAL) in Andriod.

1) Interrupt mode
2) Polling mode
   2a) Kernel polling
   2b) Timer polling

Based on the interfaces provided by the lis3lv02d as well as
lis331dlh (H/W not supporting the interrupts) they were implementing
the kernel polling mechanism.

So implementation on HAL is like this if accelerometer interface is
opened then kernel will start polling this driver periodically and
pass events to input subsystem. (It's a little bit over head)

Generally the device should be open but kernel should only poll
when an app that uses accelerometer is started.

The biggest requirement for them (Andriod people) is to allow user to
enable / disable accelerometer from user space and to configure
the accelerometer polling frequency.

Today there is no option in lis3lvo2d driver to provide this kind
of functionalities

> 
> 	Arnd
> 

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¥

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Arnd Bergmann]

On Thursday 19 January 2012, AnilKumar, Chimata wrote:
> Android userspace running on TI AM335x EVM is using the interface
> provided by lis3lv02d. They were asking some more interfaces from
> lis3lvo2d driver.
> 
> There are multiple ways we can interface accelerometer to Android layers,
> which is implemented on hardware abstraction layer (HAL) in Andriod.
> 
> 1) Interrupt mode
> 2) Polling mode
>    2a) Kernel polling
>    2b) Timer polling
> 
> Based on the interfaces provided by the lis3lv02d as well as
> lis331dlh (H/W not supporting the interrupts) they were implementing
> the kernel polling mechanism.
> 
> So implementation on HAL is like this if accelerometer interface is
> opened then kernel will start polling this driver periodically and
> pass events to input subsystem. (It's a little bit over head)
> 
> Generally the device should be open but kernel should only poll
> when an app that uses accelerometer is started.
> 
> The biggest requirement for them (Andriod people) is to allow user to
> enable / disable accelerometer from user space and to configure
> the accelerometer polling frequency.
> 
> Today there is no option in lis3lvo2d driver to provide this kind
> of functionalities

Hi AnilKumar,

This all sounds like the interface is not completely thought through.

I did not realize that the driver actually uses the input subsystem
in addition to its own interfaces. This is definitely good, and it
means that we can move the files from drivers/misc to drivers/input/misc
or drivers/input/accelerometer, making it Dmitry's problem instead of
mine ;-)

Having custom user interfaces inside an input driver however is very
bad. I'm sure that other accelerometers will have the same requirements
regarding polling frequency and enable/disable in android as well as
anytwhere else and it should absolutely not be handled by a user space
HAL but instead inside of the kernel, using a common method for all
available drivers.

Based on that, I also doubt we should apply your patch to add the
"range" attribute (adding support for lis33ldlh is fine though).
Instead I would ask you to first fix what's there by moving the
user space interfaces into the input core from the driver
and documenting them.

I don't know what the preferred way for doing things is there
(joystick ioctl, sysfs attribute, ...) but Dmitry should
be able to provide advice there. Then add interfaces for the
additional stuff you need (range, disable, ...) in the same
place and implement them as callbacks in the driver itself.

	Arnd

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Mark Brown]

On Thu, Jan 19, 2012 at 05:10:45PM +0000, Arnd Bergmann wrote:

> Having custom user interfaces inside an input driver however is very
> bad. I'm sure that other accelerometers will have the same requirements
> regarding polling frequency and enable/disable in android as well as
> anytwhere else and it should absolutely not be handled by a user space
> HAL but instead inside of the kernel, using a common method for all
> available drivers.

The polling frequency thing isn't even accelerometer specific, there's a
general need for this on a wide range of other things like touchscreens
(where controlling the sample rate is interesting for power, there are
widely varying requirements depending on what the screen is doing).

RE: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[AnilKumar, Chimata]

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

Hi Arnd,

On Thu, Jan 19, 2012 at 22:40:45, Arnd Bergmann wrote:
> On Thursday 19 January 2012, AnilKumar, Chimata wrote:
> > Android userspace running on TI AM335x EVM is using the interface
> > provided by lis3lv02d. They were asking some more interfaces from
> > lis3lvo2d driver.
> > 
> > There are multiple ways we can interface accelerometer to Android layers,
> > which is implemented on hardware abstraction layer (HAL) in Andriod.
> > 
> > 1) Interrupt mode
> > 2) Polling mode
> >    2a) Kernel polling
> >    2b) Timer polling
> > 
> > Based on the interfaces provided by the lis3lv02d as well as
> > lis331dlh (H/W not supporting the interrupts) they were implementing
> > the kernel polling mechanism.
> > 
> > So implementation on HAL is like this if accelerometer interface is
> > opened then kernel will start polling this driver periodically and
> > pass events to input subsystem. (It's a little bit over head)
> > 
> > Generally the device should be open but kernel should only poll
> > when an app that uses accelerometer is started.
> > 
> > The biggest requirement for them (Andriod people) is to allow user to
> > enable / disable accelerometer from user space and to configure
> > the accelerometer polling frequency.
> > 
> > Today there is no option in lis3lvo2d driver to provide this kind
> > of functionalities
> 
> Hi AnilKumar,
> 
> This all sounds like the interface is not completely thought through.
> 
> I did not realize that the driver actually uses the input subsystem
> in addition to its own interfaces. This is definitely good, and it
> means that we can move the files from drivers/misc to drivers/input/misc
> or drivers/input/accelerometer, making it Dmitry's problem instead of
> mine ;-)
> 
> Having custom user interfaces inside an input driver however is very
> bad. I'm sure that other accelerometers will have the same requirements
> regarding polling frequency and enable/disable in android as well as
> anytwhere else and it should absolutely not be handled by a user space
> HAL but instead inside of the kernel, using a common method for all
> available drivers.
> 
> Based on that, I also doubt we should apply your patch to add the
> "range" attribute (adding support for lis33ldlh is fine though).
>
> Instead I would ask you to first fix what's there by moving the
> user space interfaces into the input core from the driver
> and documenting them.
>
> I don't know what the preferred way for doing things is there
> (joystick ioctl, sysfs attribute, ...) but Dmitry should
> be able to provide advice there. Then add interfaces for the
> additional stuff you need (range, disable, ...) in the same
> place and implement them as callbacks in the driver itself.

I will remove the range attribute and I will submit v2. The rest of
the patch deals with adding support for lis33ldlh and should be fairly
non-controversial. We can revisit the range attribute addition at a
later time.

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¥

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Andi]

Hi Arnd,

 
What do you mean with "kernel-wide policy for accelerometer drivers"?
As far as I know, accelerometer drivers are written between the i2c driver and the
input driver.
The input driver provides already some accelerometer specific event types, ABS_X, ABS_Y, ABS_Z,
in your opinion isn't it enough?
If you mean something like collecting common properties like g range or frequency or whatever in a
standard interface, then I think that accelerometers are quite different as devices and sometimes
it could be difficult to arrange a common interface.

Andi


On Wed, Jan 18, 2012 at 02:03:47PM +0000, Arnd Bergmann wrote:
> On Tuesday 17 January 2012, AnilKumar, Chimata wrote:
> > Hi All,
> > 
> > Recalling the patch, provide the comments if there are any if not please include
> > this patch to v3.3 kernel.
> 
> As Mark and Greg said, 3.4 would be appropriate.
> 
> > +static ssize_t lis3lv02d_range_set(struct device *dev,
> > +				struct device_attribute *attr, const char *buf,
> > +				size_t count)
> > +{
> > +	unsigned long range;
> > +
> > +	if (strict_strtoul(buf, 0, &range))
> > +		return -EINVAL;
> > +
> > +	lis3_dev.g_range = range;
> > +	lis3lv02d_update_g_range(&lis3_dev);
> > +
> > +	return count;
> > +}
> > +
> >  static DEVICE_ATTR(selftest, S_IRUSR, lis3lv02d_selftest_show, NULL);
> >  static DEVICE_ATTR(position, S_IRUGO, lis3lv02d_position_show, NULL);
> >  static DEVICE_ATTR(rate, S_IRUGO | S_IWUSR, lis3lv02d_rate_show,
> >  					    lis3lv02d_rate_set);
> > +static DEVICE_ATTR(range, S_IRUGO | S_IWUSR, lis3lv02d_range_show,
> > +					    lis3lv02d_range_set);
> 
> I think you need to document this new attribute in the Documentation
> directory, unless I missed the other patch doing this.
> 
> On a more general topic, do we have a kernel-wide policy for accelerometer
> drivers? AFAICT, we currently have three subsystems that contain 
> accelerometer drivers, plus a few ad-hoc ones like this, which is a
> rather unpleasant situation. What I found are these:
> 
> $ git grep -l accelerometer drivers/ | manual_grep
> drivers/hwmon/applesmc.c
>    (one hwmon sysfs attribute for x/y/z)
> drivers/input/misc/adxl34x.c
>    (lots of sysfs attributes, input_report_key)
> drivers/input/misc/cma3000_d0x.c
>    (input_report_abs)
> drivers/input/misc/kxtj9.c
>    (input_report_abs, plus one aux sysfs attribute)
> drivers/macintosh/ams/ams-core.c
>    (one sysfs attribute for x/y/z)
> drivers/misc/lis3lv02d/
>    (multiple sysfs attributes)
> drivers/platform/x86/hdaps.c
>    (multiple sysfs attributes, only x/y)
> drivers/platform/x86/hp_accel.c
>    (hooks into drivers/misc/lis3lv02d/)
> drivers/staging/iio/accel/kxsd9.c
>    (iio)
> drivers/staging/iio/accel/sca3000_core.c
>    (iio plus extra attributes)
> 
> While I'm not blaming you for the current situation, but I think the
> situation is no longer sustainable and we need to decide on one place
> and interface for these to go in the long run so we don't grow even
> more nonstandard interfaces.
> 
> Any opinions where they should live? input, iio or a new subsystem?
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Arnd Bergmann]

On Wednesday 18 January 2012, Andi wrote:
> What do you mean with "kernel-wide policy for accelerometer drivers"?
> As far as I know, accelerometer drivers are written between the i2c driver and the
> input driver.
> The input driver provides already some accelerometer specific event types, ABS_X, ABS_Y, ABS_Z,
> in your opinion isn't it enough?
> If you mean something like collecting common properties like g range or frequency or whatever in a
> standard interface, then I think that accelerometers are quite different as devices and sometimes
> it could be difficult to arrange a common interface.

The problem is that we have some drivers using the input subsystem, some
drivers using the iio subsystem and some drivers using none of these but
interfacing with user space using some homegrown method, see my list:

> > $ git grep -l accelerometer drivers/ | manual_grep
> > drivers/hwmon/applesmc.c
> >    (one hwmon sysfs attribute for x/y/z)
> > drivers/input/misc/adxl34x.c
> >    (lots of sysfs attributes, input_report_key)
> > drivers/input/misc/cma3000_d0x.c
> >    (input_report_abs)
> > drivers/input/misc/kxtj9.c
> >    (input_report_abs, plus one aux sysfs attribute)
> > drivers/macintosh/ams/ams-core.c
> >    (one sysfs attribute for x/y/z)
> > drivers/misc/lis3lv02d/
> >    (multiple sysfs attributes)
> > drivers/platform/x86/hdaps.c
> >    (multiple sysfs attributes, only x/y)
> > drivers/platform/x86/hp_accel.c
> >    (hooks into drivers/misc/lis3lv02d/)
> > drivers/staging/iio/accel/kxsd9.c
> >    (iio)
> > drivers/staging/iio/accel/sca3000_core.c
> >    (iio plus extra attributes)

If all drivers were using the input subsystem in the way you describe,
that would be a good solution, but right now the majority of the
drivers don't do this.

	Arnd

Re: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[Jonathan Cameron]

Arnd Bergmann <arnd@arndb.de> wrote:

>On Wednesday 18 January 2012, Andi wrote:
>> What do you mean with "kernel-wide policy for accelerometer drivers"?
>> As far as I know, accelerometer drivers are written between the i2c
>driver and the
>> input driver.
>> The input driver provides already some accelerometer specific event
>types, ABS_X, ABS_Y, ABS_Z,
>> in your opinion isn't it enough?
>> If you mean something like collecting common properties like g range
>or frequency or whatever in a
>> standard interface, then I think that accelerometers are quite
>different as devices and sometimes
>> it could be difficult to arrange a common interface.
>
>The problem is that we have some drivers using the input subsystem,
>some
>drivers using the iio subsystem and some drivers using none of these
>but
>interfacing with user space using some homegrown method, see my list:
>
>> > $ git grep -l accelerometer drivers/ | manual_grep
>> > drivers/hwmon/applesmc.c
>> >    (one hwmon sysfs attribute for x/y/z)
>> > drivers/input/misc/adxl34x.c
>> >    (lots of sysfs attributes, input_report_key)
>> > drivers/input/misc/cma3000_d0x.c
>> >    (input_report_abs)
>> > drivers/input/misc/kxtj9.c
>> >    (input_report_abs, plus one aux sysfs attribute)
>> > drivers/macintosh/ams/ams-core.c
>> >    (one sysfs attribute for x/y/z)
>> > drivers/misc/lis3lv02d/
>> >    (multiple sysfs attributes)
>> > drivers/platform/x86/hdaps.c
>> >    (multiple sysfs attributes, only x/y)
>> > drivers/platform/x86/hp_accel.c
>> >    (hooks into drivers/misc/lis3lv02d/)
>> > drivers/staging/iio/accel/kxsd9.c
>> >    (iio)
>> > drivers/staging/iio/accel/sca3000_core.c
>> >    (iio plus extra attributes)
>
>If all drivers were using the input subsystem in the way you describe,
>that would be a good solution, but right now the majority of the
>drivers don't do this.
Also worth noting that there are numerous devices that are completely inappropriate for human input. Take specialised impact and vibration accelerometers. Very high speed sampling for short time periods. I agree that your bog standard 3 axis sub 10 g accelerometer belongs in input. The kxsd9 and lis3lo2dq in iio are there for historical reasons and because they are very handy for testing our bridging to input. That said it should ultimately make very little difference to user space if they are accessing a true input device or a iio device via an input bridge.  I also think that actually there is very little in the way of unusual controls for accelerometers particularly those designed for human input. Not standardizing gain or sampling frequency control is lazy. Even filtering and fft controls can and should be standardised. Doing that is a large part of what iio is about.
>
>	Arnd

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.