Re: PATCH V4 1/2] input: CMA3000 Accelerometer Driver

Previous thread: [PATCH V4 2/2] omap4: platform changes for CMA3000 by Hemanth V on Monday, November 29, 2010 - 3:57 am. (2 messages)

Next thread: Atomic non-durable file write API by Olaf van der Spek on Monday, November 29, 2010 - 5:37 am. (4 messages)
From: Hemanth V
Date: Monday, November 29, 2010 - 3:57 am

From 9985dc1211ae33baa877489c26920dfd1c29bb35 Mon Sep 17 00:00:00 2001
From: Hemanth V <hemanthv@ti.com>
Date: Thu, 26 Aug 2010 17:44:48 +0530
Subject: [PATCH] input: CMA3000 Accelerometer Driver

Add support for CMA3000 Tri-axis accelerometer, which
supports Motion detect, Measurement and Free fall modes.
CMA3000 supports both I2C/SPI bus for communication, currently the
driver supports I2C based communication.

Driver reports acceleration data through input subsystem

Signed-off-by: Hemanth V <hemanthv@ti.com>
Reviewed-by: Jonathan Cameron <jic23@cam.ac.uk>
Reviewed-by: Sergio Aguirre <saaguirre@ti.com>
Reviewed-by: Shubhrajyoti <Shubhrajyoti@ti.com>
---
This is V4 of patch, which currently doesnot support a
sysfs interface. Discussions are ongoing to create a
standard sysfs interface for sensors under input subsystem.
This interface would be adopted by the driver subsequently.

 Documentation/input/cma3000_d0x.txt  |  115 ++++++++++++
 drivers/input/misc/Kconfig           |   24 +++
 drivers/input/misc/Makefile          |    2 +
 drivers/input/misc/cma3000_d0x.c     |  334 ++++++++++++++++++++++++++++++++++
 drivers/input/misc/cma3000_d0x.h     |   53 ++++++
 drivers/input/misc/cma3000_d0x_i2c.c |  142 ++++++++++++++
 include/linux/input/cma3000.h        |   60 ++++++
 7 files changed, 730 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/input/cma3000_d0x.txt
 create mode 100644 drivers/input/misc/cma3000_d0x.c
 create mode 100644 drivers/input/misc/cma3000_d0x.h
 create mode 100644 drivers/input/misc/cma3000_d0x_i2c.c
 create mode 100644 include/linux/input/cma3000.h

diff --git a/Documentation/input/cma3000_d0x.txt b/Documentation/input/cma3000_d0x.txt
new file mode 100644
index 0000000..9cc46af
--- /dev/null
+++ b/Documentation/input/cma3000_d0x.txt
@@ -0,0 +1,115 @@
+Kernel driver for CMA3000-D0x
+============================
+
+Supported chips:
+* VTI CMA3000-D0x
+Datasheet:
+  CMA3000-D0X Product Family Specification 8281000A.02.pdf
+  ...
From: Jonathan Cameron
Date: Monday, November 29, 2010 - 4:28 am

Couple of nitpicks inline.  There's one unneeded NULL assignment
that should probably be cleaned up. I'd also like to see actual
part numbers listed somewhere in the Kconfig help text.

The removal of various temporary variables is something I know
at least one other reviewer has commented on.  Basically they may
make the code look cleaner, but they add (marginally) to the
review burden and that's annoys reviewers ;)

Anyhow, all the comments are trivial. Nice driver Hemanth.

Dmitry: This is clean and for now has none of the controversial
sysfs interfaces so baring the input bits (which I'm not as
familiar with and you might want to glance over) this looks
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

(no idea if the convention is to remove the Reviewed-by when
Usual convention is to ensure all known part codes are listed in the
help text if not in the short form above.  Makes it easy to grep for
Again (not I review from the end of patches ;)
I really don't see the point in these. They just make things
more confusing for a reviewer. Just use data->pdata.whatever
Personally I'd drop out of the registration for this. If that element
of the platform data isn't there to my mind it means that it isn't being
correctly handled by the platform data.
Why eat the error by replacing it with another one and why are we
Personally I'm still against these local variables existing.
They add a layer of abstraction that doesn't do anyone any
good so I'd just used the pdata directly in the calls.
Not an important point though. Just makes anyone reviewing

--

From: Dmitry Torokhov
Date: Tuesday, November 30, 2010 - 12:59 am

Yep, almost there.

Hemanth, does the driver still work if you apply the patch below?

Thanks.

-- 
Dmitry

Input: cma3000 - misc fixes

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/misc/Kconfig           |   18 +-
 drivers/input/misc/cma3000_d0x.c     |  344 ++++++++++++++++++++--------------
 drivers/input/misc/cma3000_d0x.h     |   30 +--
 drivers/input/misc/cma3000_d0x_i2c.c |  117 ++++++------
 include/linux/input/cma3000.h        |   10 -
 5 files changed, 286 insertions(+), 233 deletions(-)


diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index d5e5fd6..f0d9017 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -449,10 +449,10 @@ config INPUT_ADXL34X_SPI
 	  module will be called adxl34x-spi.
 
 config INPUT_CMA3000
- 	tristate "VTI CMA3000 Tri-axis accelerometer"
- 	help
- 	  Say Y here if you want to use VTI CMA3000_D0x Accelerometer
- 	  driver
+	tristate "VTI CMA3000 Tri-axis accelerometer"
+	help
+	  Say Y here if you want to use VTI CMA3000_D0x Accelerometer
+	  driver
 
 	  This driver currently only supports I2C interface to the
 	  controller. Also select the I2C method.
@@ -463,11 +463,11 @@ config INPUT_CMA3000
 	  module will be called cma3000_d0x.
 
 config INPUT_CMA3000_I2C
- 	tristate "Support I2C bus connection"
- 	depends on INPUT_CMA3000 && I2C
- 	help
- 	  Say Y here if you want to use VTI CMA3000_D0x Accelerometer
- 	  through I2C interface.
+	tristate "Support I2C bus connection"
+	depends on INPUT_CMA3000 && I2C
+	help
+	  Say Y here if you want to use VTI CMA3000_D0x Accelerometer
+	  through I2C interface.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called cma3000_d0x_i2c.
diff --git a/drivers/input/misc/cma3000_d0x.c b/drivers/input/misc/cma3000_d0x.c
index 158add9..421502a 100644
--- a/drivers/input/misc/cma3000_d0x.c
+++ b/drivers/input/misc/cma3000_d0x.c
@@ ...
From: Hemanth V
Date: Tuesday, November 30, 2010 - 5:50 am

----- Original Message ----- 

Yes works pretty well, Thanks for your efforts

Some minor changes as below required.




Need to change this to "data->g_range != CMARANGE_2G   && data->g_range != 

We use this ABS_MISC event to report free fall.



--

From: Jonathan Cameron
Date: Tuesday, November 30, 2010 - 10:05 am

Free fall isn't a conventional user input...  The lis3 driver
and similar (currently hiding in hwmon)
handle this via a separately registered chrdev.  It might not
be the best plan, but it is an existing interface so we need to

--

Previous thread: [PATCH V4 2/2] omap4: platform changes for CMA3000 by Hemanth V on Monday, November 29, 2010 - 3:57 am. (2 messages)

Next thread: Atomic non-durable file write API by Olaf van der Spek on Monday, November 29, 2010 - 5:37 am. (4 messages)