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-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
