Re: [RFC] [PATCH 1/2] CMA3000 Accelerometer Driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05/13/10 08:53, Hemanth V wrote:
> ----- Original Message -----
> From: "Jonathan Cameron" <jic23@xxxxxxxxx>
>>
>> Hi Hemanth,
>>
>> Quick comments below.  I haven't commented much on the
>> input aspects, just stuck to the more general driver aspects.
> 
> Thanks for the comments, responses inline. I am hoping for
> some more wider review.
Good luck :)
> 
>> As ever at the moment, the should this be in input question is
>> open, but if Dmitry is happy to take it then that's fine with me.
>>
> 
> Dmitry, your thoughts on this
> 
>> Jonathan
>>
>>> >From d2842ba67e142e78b2554cebb01341c76b84b693 Mon Sep 17 00:00:00 2001
>>> From: Hemanth V <hemanthv@xxxxxx>
>>> Date: Fri, 30 Apr 2010 11:55:10 +0530
>>> Subject: [PATCH] CMA3000 Accelerometer Driver
>>>
>>> This patch adds 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 and supports
>>> sysfs for configuration changes.
>>>
>>> This chip is currently used with OMAP4 based boards
>>>
>>> Thanks to Ossi Kauppinen <Ossi.Kauppinen@xxxxxx> for the support
>>> provided during development
>>>
>>> Signed-off-by: Hemanth V <hemanthv@xxxxxx>
>>> ---
>>>  drivers/input/misc/Kconfig           |    9 +
>>>  drivers/input/misc/Makefile          |    1 +
>>>  drivers/input/misc/cma3000_d0x.c     |  627 ++++++++++++++++++++++++++++++++++
>>>  drivers/input/misc/cma3000_d0x.h     |   46 +++
>>>  drivers/input/misc/cma3000_d0x_i2c.c |  136 ++++++++
>>>  include/linux/i2c/cma3000.h          |   60 ++++
>>>  6 files changed, 879 insertions(+), 0 deletions(-)
>> General convention is pick a device and name driver after it. Avoids confusion
>> in future.
> 
> I am not sure I fully understand your comment, cma3000 being the family and cma300_d0x
> being the specific chip
Not true as far as VTI's website seems to say.  The name of the specific chip is
cma3000_d01.  From previous experience (with the sca3000 series) there is a lot of
variability in features in a given series from VTI (assuming there are later elements
in this series).  Not that I can really talk, as I called that driver after the family
rather than a specific model!
> 
>>>  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/i2c/cma3000.h
>>>
>>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>>> index 16ec523..4752a00 100644
>>> --- a/drivers/input/misc/Kconfig
>>> +++ b/drivers/input/misc/Kconfig
>>> @@ -319,4 +319,13 @@ config INPUT_PCAP
>>>  	  To compile this driver as a module, choose M here: the
>>>  	  module will be called pcap_keys.
>>>
>>> +config INPUT_CMA3000_I2C
>>> +	tristate "VTI CMA3000 Tri-axis accelerometer"
>>> +	depends on I2C
>>> +	help
>>> +	  Say Y here if you want to use VTI CMA3000 Accelerometer
>>> +	  through I2C interface.
>>> +
>>> +	  To compile this driver as a module, choose M here: the
>>> +	  module will be called cma3000_dxx
>>>  endif
>>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>>> index a8b8485..94d6eda 100644
>>> --- a/drivers/input/misc/Makefile
>>> +++ b/drivers/input/misc/Makefile
>>> @@ -30,4 +30,4 @@ obj-$(CONFIG_INPUT_WINBOND_CIR)		+= winbond-cir.o
>>>  obj-$(CONFIG_INPUT_WISTRON_BTNS)	+= wistron_btns.o
>>>  obj-$(CONFIG_INPUT_WM831X_ON)		+= wm831x-on.o
>>>  obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
>>> -
>>> +obj-$(CONFIG_INPUT_CMA3000_I2C)		+= cma3000_d0x_i2c.o cma3000_d0x.o
>>> diff --git a/drivers/input/misc/cma3000_d0x.c b/drivers/input/misc/cma3000_d0x.c
>>> new file mode 100644
>>> index 0000000..c0d0ea1
>>> --- /dev/null
>>> +++ b/drivers/input/misc/cma3000_d0x.c
>>> @@ -0,0 +1,627 @@
>>> +/*
>>> + * cma3000_d0x.c
>>> + * VTI CMA3000_D0x Accelerometer driver
>>> + * 	Supports I2C/SPI interfaces
>> The device, might but I'm not seeing SPI here.  Please clear out the define bits about
>> i2c in the code as they aren't currently relevant.
> 
> Yes, will add defines related to SPI.
> 
cool, as long as you are submitting the full support for that interface as well. The usual
rules of adding the hooks when they are relevant and not before apply here.
> 
>>> + *
>>> + * Copyright (C) 2010 Texas Instruments
>>> + * Author: Hemanth V <hemanthv@xxxxxx>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/interrupt.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/input.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/i2c/cma3000.h>
>>> +
>>> +#include "cma3000_d0x.h"
>>> +
>>> +#define CMA3000_WHOAMI      0x00
>>> +#define CMA3000_REVID       0x01
>>> +#define CMA3000_CTRL        0x02
>>> +#define CMA3000_STATUS      0x03
>>> +#define CMA3000_RSTR        0x04
>>> +#define CMA3000_INTSTATUS   0x05
>>> +#define CMA3000_DOUTX       0x06
>>> +#define CMA3000_DOUTY       0x07
>>> +#define CMA3000_DOUTZ       0x08
>>> +#define CMA3000_MDTHR       0x09
>>> +#define CMA3000_MDFFTMR     0x0A
>>> +#define CMA3000_FFTHR       0x0B
>>> +
>>> +#define CMA3000_RANGE2G    (1 << 7)
>>> +#define CMA3000_RANGE8G    (0 << 7)
>>> +#define CMA3000_MODEMASK    (7 << 1)
>>> +#define CMA3000_GRANGEMASK  (1 << 7)
>>> +
>>> +#define CMA3000_STATUS_PERR    1
>>> +#define CMA3000_INTSTATUS_FFDET (1 << 2)
>>> +
>>> +/* Settling time delay in ms */
>>> +#define CMA3000_SETDELAY    30
>>> +
>>> +/* Delay for clearing interrupt in us */
>>> +#define CMA3000_INTDELAY    44
>>> +
>>> +
>>> +/*
>>> + * Bit weights in mg for each of the seven bits,
>>> + * eight bit is the sign bit
>>> + */
>>> +static int bit_to_2g[7] = {18, 36, 71, 143, 286, 571, 1142};
>>> +static int bit_to_8g[7] = {71, 143, 286, 571, 1142, 2286, 4571};
>>> +
>>> +/*
>>> + * Conversion for each of the eight modes to g, depending
>>> + * on G range i.e 2G or 8G
>>> + */
>>> +
>>> +static int *mode_to_mg[8][2] = {
>>> +	{NULL, NULL },
>>> +	{bit_to_8g, bit_to_2g},
>>> +	{bit_to_8g, bit_to_2g},
>>> +	{bit_to_8g, bit_to_8g},
>>> +	{bit_to_8g, bit_to_8g},
>>> +	{bit_to_8g, bit_to_2g},
>>> +	{bit_to_8g, bit_to_2g},
>>> +	{NULL, NULL },
>>> +};
>>
>> This arrary looks suspicious.  Why the 8g's in the second column?
>> If they are real can we have an explanatory comment.
> 
> Yes will add explanation, basically in some modes even if the
> range is set as 2G, it really operates in 8G.
Ouch. 
> 
> 
>>> +
>>> +static ssize_t cma3000_show_attr_mode(struct device *dev,
>>> +				     struct device_attribute *attr,
>>> +				     char *buf)
>>> +{
>>> +	ssize_t ret = 0;
>>> +	uint8_t mode;
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +
>>> +	mode = cma3000_read(data, CMA3000_CTRL, "ctrl");
>>> +	ret = sprintf(buf, "%d\n", (mode & CMA3000_MODEMASK) >> 1);
>>> +	return ret;
>> Roll previous 2 lines into 1.
> 
> Agreed.
> 
>>> +}
>>> +
>>
>> Well into the realms of magic numbers here.  At the very least this needs
>> some abi documenation.
> 
> If you are referring to attribute values,
> they are explained as part of the header file cma3000.h.
> I am thinking if it would be better to add a
> documentation file
> 
> 
>>> +static ssize_t cma3000_store_attr_mode(struct device *dev,
>>> +				     struct device_attribute *attr,
>>> +				     const char *buf, size_t count)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +	unsigned long val;
>>> +	int error;
>>> +	uint8_t ctrl;
>>> +
>>> +	error = strict_strtoul(buf, 0, &val);
>>> +	if (error)
>>> +		return error;
>>> +
>>> +	if (val < CMAMODE_DEFAULT || val > CMAMODE_POFF)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +	val &= (CMA3000_MODEMASK >> 1);
>>> +	ctrl = cma3000_read(data, CMA3000_CTRL, "ctrl");
>>> +	ctrl &= ~CMA3000_MODEMASK;
>>> +	ctrl |= (val << 1);
>>> +
>>> +	data->pdata.mode = val;
>>> +
>>> +	disable_irq(data->client->irq);
>>> +	cma3000_set(data, CMA3000_CTRL, ctrl, "ctrl");
>> Personally I would have allowed the error that might result from this to be split
>> into two (i2c_smbus_write_byte_data failed: (error code) then, "Ctrl register set failed".
>> Thus no need to pass magic looking strings around.  Doesn't really matter though.
>> Also no handling of error codes that this fuction can return.  These should be passed
>> directly up to userspace.
>>
> 
> Yes will pass error codes to upper layer
> 
>>> +
>>> +	/* Settling time delay required after mode change */
>>> +	msleep(CMA3000_SETDELAY);
>>> +
>>> +	enable_irq(data->client->irq);
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	return count;
>>> +}
>>> +
>> More magic numbers.  Please document.
> 
> Yes, refer previous comments
> 
>>> +static ssize_t cma3000_show_attr_grange(struct device *dev,
>>> +				       struct device_attribute *attr,
>>> +				       char *buf)
>>> +{
>>> +	ssize_t ret = 0;
>>> +	uint8_t mode;
>>> +	int g_range;
>>> +
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +
>>> +	mode = cma3000_read(data, CMA3000_CTRL, "ctrl");
>>> +	g_range = (mode & CMA3000_GRANGEMASK) ? CMARANGE_2G : CMARANGE_8G;
>>> +	ret = sprintf(buf, "%d\n", g_range);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static ssize_t cma3000_store_attr_grange(struct device *dev,
>>> +				     struct device_attribute *attr,
>>> +				     const char *buf, size_t count)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +	unsigned long val;
>>> +	int error, g_range, fuzz_x, fuzz_y, fuzz_z;
>>> +	uint8_t ctrl;
>>> +
>>> +	error = strict_strtoul(buf, 0, &val);
>>> +	if (error)
>>> +		return error;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +	ctrl = cma3000_read(data, CMA3000_CTRL, "ctrl");
>>> +	ctrl &= ~CMA3000_GRANGEMASK;
>>> +
>>> +	if (val == CMARANGE_2G) {
>>> +		ctrl |= CMA3000_RANGE2G;
>>> +		data->pdata.g_range = CMARANGE_2G;
>>> +	} else if (val == CMARANGE_8G) {
>>> +		ctrl |= CMA3000_RANGE8G;
>>> +		data->pdata.g_range = CMARANGE_8G;
>>> +	} else {
>>> +		error = -EINVAL;
>>> +		goto err_op_failed;
>>> +	}
>>> +
>>
>> Why not just pass these directly into the following functions?
> 
> This is to make it more readable, rather than having
> large arguments.
> 
>>> +	g_range = data->pdata.g_range;
>>> +	fuzz_x = data->pdata.fuzz_x;
>>> +	fuzz_y = data->pdata.fuzz_y;
>>> +	fuzz_z = data->pdata.fuzz_z;
>>> +
>>> +	disable_irq(data->client->irq);
>>> +	cma3000_set(data, CMA3000_CTRL, ctrl, "ctrl");
>>> +
>>> +	input_set_abs_params(data->input_dev, ABS_X, -g_range,
>>> +				g_range, fuzz_x, 0);
>>> +	input_set_abs_params(data->input_dev, ABS_Y, -g_range,
>>> +				g_range, fuzz_y, 0);
>>> +	input_set_abs_params(data->input_dev, ABS_Z, -g_range,
>>> +				g_range, fuzz_z, 0);
>>> +
>>> +	enable_irq(data->client->irq);
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	return count;
>>> +
>>> +err_op_failed:
>>> +	mutex_unlock(&data->mutex);
>>> +	return error;
>>> +}
>>> +
>>> +static ssize_t cma3000_show_attr_mdthr(struct device *dev,
>>> +				      struct device_attribute *attr,
>>> +				      char *buf)
>>> +{
>>> +	ssize_t ret = 0;
>>> +	uint8_t mode;
>>> +
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +
>>> +	mode = cma3000_read(data, CMA3000_MDTHR, "mdthr");
>>> +	ret = sprintf(buf, "%d\n", mode);
>>> +
>>> +	return ret;
>> Again roll lines together. Lots more of these below.  Add code for
>> no gain in my view.
> 
> Yes, agreed
> 
>>> +}
>>> +
>>> +static ssize_t cma3000_store_attr_mdthr(struct device *dev,
>>> +				       struct device_attribute *attr,
>>> +				       const char *buf, size_t count)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +	unsigned long val;
>>> +	int error;
>>> +
>> Why not specify the base? Pretty much anything will work so why
>> make a fair bit of code run to guess what is right.
> 
> Can u clarify which base u are referring to.
If one passes 0 as the second parameter of strtoul you are
running some code that attempt to identify the base of the numeric
parameter (hex, octal, binary etc). I guess on futher inspecton this
might make sense here.   However I am less sure about some of the
values above where only a very narrow range is available. 
> 
>>> +	error = strict_strtoul(buf, 0, &val);
>>> +	if (error)
>>> +		return error;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +	data->pdata.mdthr = val;
>>> +
>>> +	disable_irq(data->client->irq);
>>> +	cma3000_set(data, CMA3000_MDTHR, val, "mdthr");
>>> +	enable_irq(data->client->irq);
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	return count;
>>> +}
>>> +
>>> +static ssize_t cma3000_show_attr_mdfftmr(struct device *dev,
>>> +					struct device_attribute *attr,
>>> +					char *buf)
>>> +{
>>> +	ssize_t ret = 0;
>>> +	uint8_t mode;
>>> +
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +
>>> +	mode = cma3000_read(data, CMA3000_MDFFTMR, "mdfftmr");
>>> +	ret = sprintf(buf, "%d\n", mode);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static ssize_t cma3000_store_attr_mdfftmr(struct device *dev,
>>> +					struct device_attribute *attr,
>>> +					const char *buf, size_t count)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +	unsigned long val;
>>> +	int error;
>>> +
>>> +	error = strict_strtoul(buf, 0, &val);
>>> +	if (error)
>>> +		return error;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +	data->pdata.mdfftmr = val;
>>> +
>>> +	disable_irq(data->client->irq);
>>> +	cma3000_set(data, CMA3000_MDFFTMR, val, "mdthr");
>> Again handle the error code returned by this.
> 
> Yes, agreed
> 
>>> +	enable_irq(data->client->irq);
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	return count;
>>> +}
>>> +
>> More documentation needed. I've no idea what this one is!
> 
> yes, agreed
> 
>>> +static ssize_t cma3000_show_attr_ffthr(struct device *dev,
>>> +					struct device_attribute *attr,
>>> +					char *buf)
>>> +{
>>> +	ssize_t ret = 0;
>>> +	uint8_t mode;
>>> +
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +
>>> +	mode = cma3000_read(data, CMA3000_FFTHR, "ffthr");
>>> +	ret = sprintf(buf, "%d\n", mode);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static ssize_t cma3000_store_attr_ffthr(struct device *dev,
>>> +				       struct device_attribute *attr,
>>> +				       const char *buf, size_t count)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
>>> +	unsigned long val;
>>> +	int error;
>>> +
>>> +	error = strict_strtoul(buf, 0, &val);
>>> +	if (error)
>>> +		return error;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +	data->pdata.ffthr = val;
>>> +
>>> +	disable_irq(data->client->irq);
>>> +	cma3000_set(data, CMA3000_FFTHR, val, "mdthr");
>> more error codes not handled.
> 
> yes, agreed
> 
>>> +	enable_irq(data->client->irq);
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	return count;
>>> +}
>>> +
>>
>> All need documentation.  What they should be called is a little
>> open to debate. I'd argue in favour of the iio naming conventions,
>> but then I wrote most of them :)
> 
> Dmity, your thoughts on this
> 
>>
>>> +static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO,
>>> +		cma3000_show_attr_mode, cma3000_store_attr_mode);
>>> +
>>> +static DEVICE_ATTR(grange, S_IWUSR | S_IRUGO,
>>> +		cma3000_show_attr_grange, cma3000_store_attr_grange);
>>> +
>>> +static DEVICE_ATTR(mdthr, S_IWUSR | S_IRUGO,
>>> +		cma3000_show_attr_mdthr, cma3000_store_attr_mdthr);
>>> +
>>> +static DEVICE_ATTR(mdfftmr, S_IWUSR | S_IRUGO,
>>> +		cma3000_show_attr_mdfftmr, cma3000_store_attr_mdfftmr);
>>> +
>>> +static DEVICE_ATTR(ffthr, S_IWUSR | S_IRUGO,
>>> +		cma3000_show_attr_ffthr, cma3000_store_attr_ffthr);
>>> +
>>> +
>>> +static struct attribute *cma_attrs[] = {
>>> +	&dev_attr_mode.attr,
>>> +	&dev_attr_grange.attr,
>>> +	&dev_attr_mdthr.attr,
>>> +	&dev_attr_mdfftmr.attr,
>>> +	&dev_attr_ffthr.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +static struct attribute_group cma3000_attr_group = {
>>> +	.attrs = cma_attrs,
>>> +};
>>> +
>>> +static void cma3000_create_sysfs(struct cma3000_accl_data *data)
>>> +{
>>> +	int ret;
>>> +	ret = sysfs_create_group(&data->client->dev.kobj, &cma3000_attr_group);
>>> +	if (ret)
>>> +		dev_err(&data->client->dev,
>>> +			"failed to create sysfs entries\n");
>> This is a void returning function?  Surely it failing should stop the driver
>> loading and exit nicely?
> 
> Yes, agreed
> 
>>> +
>>> +}
>>> +
>>> +static void cma3000_remove_sysfs(struct cma3000_accl_data *data)
>>> +{
>>> +	sysfs_remove_group(&data->client->dev.kobj, &cma3000_attr_group);
>>> +}
>>> +
>>> +static void decode_mg(struct cma3000_accl_data *data, int *datax,
>>> +				int *datay, int *dataz)
>>> +{
>>> +	int i, tmpx = 0, tmpy = 0, tmpz = 0;
>>> +
>>
>> Can this get rolled into a function per channel? Would be easier to read.
> 
> Will check if this can be further optimized.
> 
>>> +	for (i = 0; i < 7; ++i) {
>>> +		tmpx += (((*datax) & BIT(i)) >> i) * (data->bit_to_mg[i]);
>>> +		tmpy += (((*datay) & BIT(i)) >> i) * (data->bit_to_mg[i]);
>>> +		tmpz += (((*dataz) & BIT(i)) >> i) * (data->bit_to_mg[i]);
>>> +
>>> +	}
>>> +
>>> +	if ((*datax) & BIT(7))
>>> +		*datax = -tmpx;
>>> +	else
>>> +		*datax = tmpx;
>>> +
>>> +	if ((*datay) & BIT(7))
>>> +		*datay = -tmpy;
>>> +	else
>>> +		*datay = tmpy;
>>> +
>>> +	if ((*dataz) & BIT(7))
>>> +		*dataz = -tmpz;
>>> +	else
>>> +		*dataz = tmpz;
>>> +}
>>> +
>>> +static irqreturn_t cma3000_thread_irq(int irq, void *dev_id)
>>> +{
>>> +	struct cma3000_accl_data *data = dev_id;
>>> +	int  datax, datay, dataz;
>>> +	u8 ctrl, mode, range, intr_status;
>>> +
>>> +	intr_status = cma3000_read(data, CMA3000_INTSTATUS, "interrupt status");
>>> +
>>> +	/* Check if free fall is detected, report immediately */
>>> +	if (intr_status & CMA3000_INTSTATUS_FFDET) {
>>> +		input_report_abs(data->input_dev, ABS_MISC, 1);
>>> +		input_sync(data->input_dev);
>>> +	} else {
>>> +		input_report_abs(data->input_dev, ABS_MISC, 0);
>>> +	}
>>> +
>>> +
>>> +	/* Delay required between each read for interrupt clearing */
>> Weird... Not your fault obviously! Got to love vti parts sometimes.
>> Can you just confirm that the chip really sends out a data ready for each
>> channel?  That to put it lightly would be very unusual.
> 
> As per product specs there is no data ready per channel. The information I received from VTI support
> is that if there are continuous reads for all 3 channel there are issues with interrupt clearing
> hence a delay is added between each channel read.
Agreed, there is only a shared data rdy.  This is quite a common
situation.  Normally I'd interpret that as it only getting set
when there is new data in all 3 channels. You are the one
with the hardware so I'll defer to you on the fact this works!
> 
> 
>>> +	datax = cma3000_read(data, CMA3000_DOUTX, "X");
>>> +	udelay(CMA3000_INTDELAY);
>>> +	datay = cma3000_read(data, CMA3000_DOUTY, "Y");
>>> +	udelay(CMA3000_INTDELAY);
>>> +	dataz = cma3000_read(data, CMA3000_DOUTZ, "Z");
>>> +
>>> +	ctrl = cma3000_read(data, CMA3000_CTRL, "ctrl");
>>> +	mode = (ctrl & CMA3000_MODEMASK) >> 1;
>>> +	range = (ctrl & CMA3000_GRANGEMASK) >> 7;
>> Please can you cache this ctrl register.  Silly to read it every time
>> when I think you should be able to know in advance what it is set to.
> 
> Modes can change dynamically, for example from motion detect to measurement
> hence the need to get the current mode from register read.
Ah, I'd missed that.
> 
>>> +
>>> +	data->bit_to_mg = mode_to_mg[mode][range];
>>> +
>>> +	/* Interrupt not for this device */
>>> +	if (data->bit_to_mg == NULL)
>>> +		return IRQ_NONE;
>>> +
>>> +	/* Decode register values to milli g */
>>> +	decode_mg(data, &datax, &datay, &dataz);
>>> +
>>> +	input_report_abs(data->input_dev, ABS_X, datax);
>>> +	input_report_abs(data->input_dev, ABS_Y, datay);
>>> +	input_report_abs(data->input_dev, ABS_Z, dataz);
>>> +	input_sync(data->input_dev);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int cma3000_reset(struct cma3000_accl_data *data)
>>> +{
>>> +	int ret;
>>> +
>>> +	/* Reset sequence */
>>> +	cma3000_set(data, CMA3000_RSTR, 0x02, "Reset");
>>> +	cma3000_set(data, CMA3000_RSTR, 0x0A, "Reset");
>>> +	cma3000_set(data, CMA3000_RSTR, 0x04, "Reset");
>>> +
>>> +	/* Settling time delay */
>>> +	mdelay(10);
>>> +
>>> +	ret = cma3000_read(data, CMA3000_STATUS, "Status");
>>> +	if ((ret < 0) || (ret & CMA3000_STATUS_PERR)) {
>>> +		dev_err(&data->client->dev, "Reset failed\n");
>> Why kill the perfectly good error code from cma3000_read?
> 
> It could be a read error or reset failed, hence the error code
Indeed, hence you should handle these two error options separately.
I'm not denying that this is the right error if (ret & CMA3000_STATUS_PERR)
but in the other case, if (ref < 0) pass the error already stored in ret
onwards rather than squashing it.
> 
>>> +		return -EIO;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int cma3000_poweron(struct cma3000_accl_data *data)
>>> +{
>>> +	uint8_t ctrl = 0, mdthr, mdfftmr, ffthr, mode;
>>> +	int g_range, ret;
>>> +
>>
>> Any reason for local copies?
> 
> For easier reading
> 
>>> +	g_range = data->pdata.g_range;
>>> +	mode = data->pdata.mode;
>>> +	mdthr = data->pdata.mdthr;
>>> +	mdfftmr = data->pdata.mdfftmr;
>>> +	ffthr = data->pdata.ffthr;
>>> +
>>> +	if (mode < CMAMODE_DEFAULT || mode > CMAMODE_POFF) {
>>> +		data->pdata.mode = CMAMODE_MOTDET;
>>> +		mode = data->pdata.mode;
>>> +		dev_info(&data->client->dev,
>>> +			"Invalid mode specified, assuming Motion Detect\n");
>>> +	}
>>> +
>>> +	if (g_range == CMARANGE_2G) {
>>> +		ctrl = (mode << 1) | CMA3000_RANGE2G;
>>> +	} else if (g_range == CMARANGE_8G) {
>>> +		ctrl = (mode << 1) | CMA3000_RANGE8G;
>>> +	} else {
>>> +		dev_info(&data->client->dev,
>>> +			"Invalid G range specified, assuming 8G\n");
>>> +		ctrl = (mode << 1) | CMA3000_RANGE8G;
>>> +		data->pdata.g_range = CMARANGE_8G;
>>> +	}
>>> +
>>> +
>>> +	cma3000_set(data, CMA3000_MDTHR, mdthr, "Motion Detect Threshold");
>>> +	cma3000_set(data, CMA3000_MDFFTMR, mdfftmr, "Time register");
>>> +	cma3000_set(data, CMA3000_FFTHR, ffthr, "Free fall threshold");
>>> +	ret = cma3000_set(data, CMA3000_CTRL, ctrl, "Mode setting");
>>> +	if (ret < 0)
>>> +		return -EIO;
>>> +
>>> +	mdelay(CMA3000_SETDELAY);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int cma3000_poweroff(struct cma3000_accl_data *data)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = cma3000_set(data, CMA3000_CTRL, CMAMODE_POFF, "Mode setting");
>>> +	mdelay(CMA3000_SETDELAY);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +int cma3000_init(struct cma3000_accl_data *data)
>>> +{
>>> +	int ret = 0, fuzz_x, fuzz_y, fuzz_z, g_range;
>>> +	uint32_t irqflags;
>>> +
>>> +	if (data->client->dev.platform_data == NULL) {
>>> +		dev_err(&data->client->dev, "platform data not found \n");
>>> +		goto err_op2_failed;
>>> +	}
>>> +
>> Why the local copy?  Isn't data->client->dev.platform_data always available?
> 
> platform data is marked as __initdata
Fair enough.
> 
>>> +	memcpy(&(data->pdata), data->client->dev.platform_data,
>>> +		sizeof(struct cma3000_platform_data));
>>> +
>>> +	ret = cma3000_reset(data);
>>> +	if (ret)
>>> +		goto err_op2_failed;
>>> +
>>> +	ret = cma3000_read(data, CMA3000_REVID, "Revid");
>>> +	if (ret < 0)
>>> +		goto err_op2_failed;
>>> +
>>> +	pr_info("CMA3000 Acclerometer : Revision %x\n", ret);
>>> +
>>> +	/* Bring it out of default power down state */
>>> +	ret = cma3000_poweron(data);
>>> +	if (ret < 0)
>>> +		goto err_op2_failed;
>>> +
>>> +	fuzz_x = data->pdata.fuzz_x;
>>> +	fuzz_y = data->pdata.fuzz_y;
>>> +	fuzz_z = data->pdata.fuzz_z;
>>> +	g_range = data->pdata.g_range;
>>> +	irqflags = data->pdata.irqflags;
>>> +
>>> +	data->input_dev = input_allocate_device();
>>> +	if (data->input_dev == NULL) {
>>> +		ret = -ENOMEM;
>>> +		dev_err(&data->client->dev,
>>> +			"Failed to allocate input device\n");
>>> +		goto err_op2_failed;
>>> +	}
>>> +
>>> +	data->input_dev->name = "cma3000-acclerometer";
>>> +
>>> +#ifdef CONFIG_INPUT_CMA3000_I2C
>>> +	data->input_dev->id.bustype = BUS_I2C;
>>> +#endif
>>> +
>>> +	 __set_bit(EV_ABS, data->input_dev->evbit);
>>> +	 __set_bit(EV_MSC, data->input_dev->evbit);
>>> +
>>> +	input_set_abs_params(data->input_dev, ABS_X, -g_range,
>>> +				g_range, fuzz_x, 0);
>>> +	input_set_abs_params(data->input_dev, ABS_Y, -g_range,
>>> +				g_range, fuzz_y, 0);
>>> +	input_set_abs_params(data->input_dev, ABS_Z, -g_range,
>>> +				g_range, fuzz_z, 0);
>>> +	input_set_abs_params(data->input_dev, ABS_MISC, 0,
>>> +				1, 0, 0);
>>> +
>>> +	ret = input_register_device(data->input_dev);
>>> +	if (ret) {
>>> +		dev_err(&data->client->dev,
>>> +			"Unable to register input device\n");
>>> +		goto err_op2_failed;
>>> +	}
>>> +
>>> +	mutex_init(&data->mutex);
>>> +
>>> +	if (data->client->irq) {
>>> +		ret = request_threaded_irq(data->client->irq, NULL,
>>> +					cma3000_thread_irq,
>>> +					irqflags | IRQF_ONESHOT,
>>> +					data->client->name, data);
>>> +
>>> +		if (ret < 0) {
>>> +			dev_err(&data->client->dev,
>>> +				"request_threaded_irq failed\n");
>>> +			goto err_op1_failed;
>>> +		}
>>> +	}
>>> +
>>> +	cma3000_create_sysfs(data);
>>> +
>>> +	return 0;
>>> +
>>> +err_op1_failed:
>>> +	mutex_destroy(&data->mutex);
>>> +	input_unregister_device(data->input_dev);
>>> +err_op2_failed:
>>> +	if (data != NULL) {
>>> +		if (data->input_dev != NULL)
>>> +			input_free_device(data->input_dev);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +int cma3000_exit(struct cma3000_accl_data *data)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = cma3000_poweroff(data);
>>> +
>>> +	if (data->client->irq)
>>> +		free_irq(data->client->irq, data);
>>> +
>>> +	mutex_destroy(&data->mutex);
>>> +	input_unregister_device(data->input_dev);
>>> +	input_free_device(data->input_dev);
>>> +	cma3000_remove_sysfs(data);
>>> +
>>> +	return ret;
>>> +}
>>> diff --git a/drivers/input/misc/cma3000_d0x.h b/drivers/input/misc/cma3000_d0x.h
>>> new file mode 100644
>>> index 0000000..906d29e
>>> --- /dev/null
>>> +++ b/drivers/input/misc/cma3000_d0x.h
>>> @@ -0,0 +1,46 @@
>>> +/*
>>> + * cma3000_d0x.h
>>> + * VTI CMA3000_D0x Accelerometer driver
>>> + *
>>> + * Copyright (C) 2010 Texas Instruments
>>> + * Author: Hemanth V <hemanthv@xxxxxx>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef INPUT_CMA3000_H
>>> +#define INPUT_CMA3000_H
>>> +
>>> +#include <linux/i2c.h>
>>> +#include <linux/input.h>
>>> +
>>> +struct cma3000_accl_data {
>>> +#ifdef CONFIG_INPUT_CMA3000_I2C
>>> +	struct i2c_client *client;
>>> +#endif
>>> +	struct input_dev *input_dev;
>>> +	struct cma3000_platform_data pdata;
>>> +
>>> +	/* mutex for sysfs operations */
>>> +	struct mutex mutex;
>>> +	int *bit_to_mg;
>>> +};
>>> +
>>> +int cma3000_set(struct cma3000_accl_data *, u8, u8, char *);
>>> +int cma3000_read(struct cma3000_accl_data *, u8, char *);
>>> +int cma3000_init(struct cma3000_accl_data *);
>>> +int cma3000_exit(struct cma3000_accl_data *);
>>> +int cma3000_poweron(struct cma3000_accl_data *);
>>> +int cma3000_poweroff(struct cma3000_accl_data *);
>>> +
>>> +#endif
>>> diff --git a/drivers/input/misc/cma3000_d0x_i2c.c b/drivers/input/misc/cma3000_d0x_i2c.c
>>> new file mode 100644
>>> index 0000000..d2ad638
>>> --- /dev/null
>>> +++ b/drivers/input/misc/cma3000_d0x_i2c.c
>>> @@ -0,0 +1,136 @@
>>> +/*
>>> + * cma3000_d0x_i2c.c
>>> + *
>>> + * Implements I2C interface for VTI CMA300_D0x Accelerometer driver
>>> + *
>>> + * Copyright (C) 2010 Texas Instruments
>>> + * Author: Hemanth V <hemanthv@xxxxxx>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/i2c/cma3000.h>
>>> +#include "cma3000_d0x.h"
>>> +
>>> +int cma3000_set(struct cma3000_accl_data *accl, u8 reg, u8 val, char *msg)
>>> +{
>>> +	int ret = i2c_smbus_write_byte_data(accl->client, reg, val);
>>> +	if (ret < 0)
>>> +		dev_err(&accl->client->dev,
>>> +			"i2c_smbus_write_byte_data failed (%s)\n", msg);
>>> +	return ret;
>>> +}
>>> +
>>> +int cma3000_read(struct cma3000_accl_data *accl, u8 reg, char *msg)
>>> +{
>>> +	int ret = i2c_smbus_read_byte_data(accl->client, reg);
>>> +	if (ret < 0)
>>> +		dev_err(&accl->client->dev,
>>> +			"i2c_smbus_read_byte_data failed (%s)\n", msg);
>>> +	return ret;
>>> +}
>>> +
>>> +static int __devinit cma3000_accl_probe(struct i2c_client *client,
>>> +					const struct i2c_device_id *id)
>>> +{
>>> +	int ret;
>>> +	struct cma3000_accl_data *data = NULL;
>>> +
>>> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>>> +	if (data == NULL) {
>>> +		ret = -ENOMEM;
>>> +		goto err_op_failed;
>>> +	}
>>> +
>>> +	data->client = client;
>>> +	i2c_set_clientdata(client, data);
>>> +
>>> +	ret = cma3000_init(data);
>>> +	if (ret)
>>> +		goto err_op_failed;
>>> +
>>> +	return 0;
>>> +
>>> +err_op_failed:
>>> +
>>> +	if (data != NULL)
>>> +		kfree(data);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int __devexit cma3000_accl_remove(struct i2c_client *client)
>>> +{
>>> +	struct cma3000_accl_data *data = i2c_get_clientdata(client);
>>> +	int ret;
>>> +
>>> +	ret = cma3000_exit(data);
>>> +	i2c_set_clientdata(client, NULL);
>>> +	kfree(data);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM
>>> +static int cma3000_accl_suspend(struct i2c_client *client, pm_message_t mesg)
>>> +{
>>> +	struct cma3000_accl_data *data = i2c_get_clientdata(client);
>>> +
>>> +	return cma3000_poweroff(data);
>>> +}
>>> +
>>> +static int cma3000_accl_resume(struct i2c_client *client)
>>> +{
>>> +	struct cma3000_accl_data *data = i2c_get_clientdata(client);
>>> +
>>> +	return cma3000_poweron(data);
>>> +}
>>> +#endif
>>> +
>>> +static const struct i2c_device_id cma3000_id[] = {
>>> +	{ "cma3000_accl", 0 },
>>> +	{ },
>>> +};
>>> +
>>> +static struct i2c_driver cma3000_accl_driver = {
>>> +	.probe		= cma3000_accl_probe,
>>> +	.remove		= cma3000_accl_remove,
>>> +	.id_table	= cma3000_id,
>>> +#ifdef CONFIG_PM
>>> +	.suspend	= cma3000_accl_suspend,
>>> +	.resume		= cma3000_accl_resume,
>>> +#endif
>>> +	.driver = {
>>> +		.name = "cma3000_accl"
>>> +	},
>>> +};
>>> +
>>> +static int __init cma3000_accl_init(void)
>>> +{
>> Bonus white line,
> 
> Will remove
> 
>>> +	return i2c_add_driver(&cma3000_accl_driver);
>>> +}
>>> +
>>> +static void __exit cma3000_accl_exit(void)
>>> +{
>>> +	i2c_del_driver(&cma3000_accl_driver);
>>> +}
>>> +
>>> +module_init(cma3000_accl_init);
>>> +module_exit(cma3000_accl_exit);
>>> +
>>> +MODULE_DESCRIPTION("CMA3000-D0x Accelerometer Driver");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_AUTHOR("Hemanth V <hemanthv@xxxxxx>");
>>> diff --git a/include/linux/i2c/cma3000.h b/include/linux/i2c/cma3000.h
>>> new file mode 100644
>>> index 0000000..50aa3fc
>>> --- /dev/null
>>> +++ b/include/linux/i2c/cma3000.h
>>> @@ -0,0 +1,60 @@
>>> +/*
>>> + * cma3000.h
>>> + * VTI CMA300_Dxx Accelerometer driver
>>> + *
>>> + * Copyright (C) 2010 Texas Instruments
>>> + * Author: Hemanth V <hemanthv@xxxxxx>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef _LINUX_CMA3000_I2C_H
>>> +#define _LINUX_CMA3000_I2C_H
>>> +
>>> +#define CMAMODE_DEFAULT    0
>>> +#define CMAMODE_MEAS100    1
>>> +#define CMAMODE_MEAS400    2
>>> +#define CMAMODE_MEAS40     3
>>> +#define CMAMODE_MOTDET     4
>>> +#define CMAMODE_FF100      5
>>> +#define CMAMODE_FF400      6
>>> +#define CMAMODE_POFF       7
>>> +
>>> +#define CMARANGE_2G   2000
>>> +#define CMARANGE_8G   8000
>>> +
>>> +/**
>>> + * struct cma3000_i2c_platform_data - CMA3000 Platform data
>>> + * @fuzz_x: Noise on X Axis
>>> + * @fuzz_y: Noise on Y Axis
>>> + * @fuzz_z: Noise on Z Axis
>>> + * @g_range: G range in milli g i.e 2000 or 8000
>>> + * @mode: Operating mode
>>> + * @mdthr: Motion detect threshold value
>>> + * @mdfftmr: Motion detect and free fall time value
>>> + * @ffthr: Free fall threshold value
>>> + */
>>> +
>>> +struct cma3000_platform_data {
>>> +	int fuzz_x;
>>> +	int fuzz_y;
>>> +	int fuzz_z;
>>> +	int g_range;
>>> +	uint8_t  mode;
>>> +	uint8_t  mdthr;
>>> +	uint8_t  mdfftmr;
>>> +	uint8_t  ffthr;
>>> +	uint32_t  irqflags;
>>> +};
>>> +
>>> +#endif
>>
>>
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux