On Tue, Oct 25, 2011 at 09:26:10AM +0200, Ricardo Ribalda Delgado wrote: > Hello Grant > > It is similar to the crm, because they came from the same > manufacturer and they share some commands, but that is all. > > the crm is a gyroscope and the cma is an accelerometer. I tried to put > both drivers together and the result was a bit too messy, I think it > is easier to understand as two separate drivers, but if you believe > they have to live together and help me making it more understandable, > I have to problem in coding it. Alternately, are there any parts of the cma driver that can be generalized and used by both? A lot of the driver looks like boilerplate to me, which I'd rather not see duplicated. And I've filled in a few more comments below. g. > > Regards! > > On Mon, Oct 24, 2011 at 23:40, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote: > > On Mon, Oct 24, 2011 at 10:21:15PM +0200, Ricardo Ribalda Delgado wrote: > >> Add support for CMR3000 Tri-axis accelerometer. CMR3000 supports both > >> I2C/SPI bus communication, currently the driver supports SPI > >> communication, since I have no hardware to test the I2C communication. > > > > How different is this driver from the cma3000? Is it a cut and paste job? > > > > g. > > > >> > >> --- > >> > >> v4: Fixes suggested by Dmitry Torokhov > >> -Do not release the input device until the irq has been released > >> > >> v3: Fixes suggested by Jonathan Cameron > >> -Support DT > >> -Cleaner spi read/write > >> > >> v2: Fixes suggested by Jonathan Cameron > >> -Code Stype > >> -Check pdata!=NULL > >> -SPI align Cacheline > >> -More clear based on > >> -%s/set/write/ > >> -%s/accl/gyro/ > >> -remove READ/SET macros > >> > >> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx> > >> --- > >> drivers/input/misc/Kconfig | 24 ++ > >> drivers/input/misc/Makefile | 2 + > >> drivers/input/misc/cmr3000_d0x.c | 426 ++++++++++++++++++++++++++++++++++ > >> drivers/input/misc/cmr3000_d0x.h | 45 ++++ > >> drivers/input/misc/cmr3000_d0x_spi.c | 144 ++++++++++++ > >> include/linux/input/cmr3000.h | 54 +++++ > >> 6 files changed, 695 insertions(+), 0 deletions(-) > >> create mode 100644 drivers/input/misc/cmr3000_d0x.c > >> create mode 100644 drivers/input/misc/cmr3000_d0x.h > >> create mode 100644 drivers/input/misc/cmr3000_d0x_spi.c > >> create mode 100644 include/linux/input/cmr3000.h > >> > >> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > >> index b9f2e93..7c56f94 100644 > >> --- a/drivers/input/misc/Kconfig > >> +++ b/drivers/input/misc/Kconfig > >> @@ -524,6 +524,30 @@ config INPUT_CMA3000_SPI > >> To compile this driver as a module, choose M here: the > >> module will be called cma3000_d0x_spi. > >> > >> +config INPUT_CMR3000 > >> + tristate "VTI CMR3000 Tri-axis gyroscope" > >> + help > >> + Say Y here if you want to use VTI CMR3000_D0x Gyroscope > >> + driver > >> + > >> + This driver currently only supports SPI interface to the > >> + controller. Also select the SPI method. > >> + > >> + If unsure, say N > >> + > >> + To compile this driver as a module, choose M here: the > >> + module will be called cmr3000_d0x. > >> + > >> +config INPUT_CMR3000_SPI > >> + tristate "Support SPI bus connection" > >> + depends on INPUT_CMR3000 && SPI > >> + help > >> + Say Y here if you want to use VTI CMR3000_D0x Gyroscope > >> + through SPI interface. > >> + > >> + To compile this driver as a module, choose M here: the > >> + module will be called cmr3000_d0x_spi. > >> + > >> config INPUT_XEN_KBDDEV_FRONTEND > >> tristate "Xen virtual keyboard and mouse support" > >> depends on XEN_FBDEV_FRONTEND > >> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile > >> index 7305f6f..c7fe09a 100644 > >> --- a/drivers/input/misc/Makefile > >> +++ b/drivers/input/misc/Makefile > >> @@ -21,6 +21,8 @@ obj-$(CONFIG_INPUT_CM109) += cm109.o > >> obj-$(CONFIG_INPUT_CMA3000) += cma3000_d0x.o > >> obj-$(CONFIG_INPUT_CMA3000_I2C) += cma3000_d0x_i2c.o > >> obj-$(CONFIG_INPUT_CMA3000_SPI) += cma3000_d0x_spi.o > >> +obj-$(CONFIG_INPUT_CMR3000) += cmr3000_d0x.o > >> +obj-$(CONFIG_INPUT_CMR3000_SPI) += cmr3000_d0x_spi.o > >> obj-$(CONFIG_INPUT_COBALT_BTNS) += cobalt_btns.o > >> obj-$(CONFIG_INPUT_DM355EVM) += dm355evm_keys.o > >> obj-$(CONFIG_HP_SDC_RTC) += hp_sdc_rtc.o > >> diff --git a/drivers/input/misc/cmr3000_d0x.c b/drivers/input/misc/cmr3000_d0x.c > >> new file mode 100644 > >> index 0000000..d046149 > >> --- /dev/null > >> +++ b/drivers/input/misc/cmr3000_d0x.c > >> @@ -0,0 +1,426 @@ > >> +/* > >> + * VTI CMR3000_D0x Gyroscope driver > >> + * > >> + * Copyright (C) 2011 Qtechnology > >> + * Author: Ricardo Ribalda <ricardo.ribalda@xxxxxxxxx> > >> + * > >> + * Based on: > >> + * drivers/input/misc/cma3000_d0x.c by: Hemanth V > >> + * > >> + * 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/types.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/delay.h> > >> +#include <linux/slab.h> > >> +#include <linux/input.h> > >> +#include <linux/input/cmr3000.h> > >> +#include <linux/of.h> > >> + > >> +#include "cmr3000_d0x.h" > >> + > >> +#define CMR3000_REV 0x21 > >> + > >> +#define CMR3000_WHOAMI 0x00 > >> +#define CMR3000_REVID 0x01 > >> +#define CMR3000_CTRL 0x02 > >> +#define CMR3000_STATUS 0x03 > >> +#define CMR3000_X_LSB 0x0C > >> +#define CMR3000_X_MSB 0x0D > >> +#define CMR3000_Y_LSB 0x0E > >> +#define CMR3000_Y_MSB 0x0F > >> +#define CMR3000_Z_LSB 0x10 > >> +#define CMR3000_Z_MSB 0x11 > >> +#define CMR3000_I2C_ADDR 0x22 > >> +#define CMR3000_PDR 0x26 > >> + > >> +#define CMR3000_IRQDIS (1 << 0) > >> +#define CMR3000_MODEMASK (3 << 1) > >> +#define CMR3000_BUSI2C (0 << 4) > >> +#define CMR3000_BUSSPI (1 << 4) > >> +#define CMR3000_INTLOW (1 << 6) > >> +#define CMR3000_INTHIGH (0 << 6) > >> +#define CMR3000_RST (1 << 7) > >> + > >> +#define CMRMODE_SHIFT 1 > >> +#define CMRIRQLEVEL_SHIFT 6 > >> + > >> +#define CMR3000_STATUS_PERR (1 << 0) > >> +#define CMR3000_STATUS_PORST (1 << 3) > >> + > >> +/* Settling time delay in ms */ > >> +#define CMR3000_SETDELAY 30 > >> + > >> +/* > >> + * Bit weights mult/div in dps for bit 0, other bits need > >> + * multipy factor 2^n. 11th bit is the sign bit. > >> + */ > >> +#define BIT_TO_DPS_MUL 3 > >> +#define BIT_TO_DPS_DIV 32 > >> + > >> +static struct cmr3000_platform_data cmr3000_default_pdata = { > >> + .irq_level = CMR3000_INTHIGH, > >> + .mode = CMRMODE_MEAS80, > >> + .fuzz_x = 1, > >> + .fuzz_y = 1, > >> + .fuzz_z = 1, > >> + .irqflags = 0, > >> +}; > >> + > >> +struct cmr3000_gyro_data { > >> + const struct cmr3000_bus_ops *bus_ops; > >> + struct cmr3000_platform_data pdata; > >> + > >> + struct device *dev; > >> + struct input_dev *input_dev; > >> + > >> + int irq_level; > >> + u8 mode; > >> + > >> + int bit_to_mg; > >> + int irq; > >> + > >> + struct mutex mutex; > >> + bool opened; > >> + bool suspended; > >> +}; > >> + > >> +static void decode_dps(struct cmr3000_gyro_data *data, int *datax, > >> + int *datay, int *dataz) > >> +{ > >> + /* Data in 2's complement, convert to dps */ > >> + *datax = (((s16) ((*datax) << 2)) * BIT_TO_DPS_MUL) / BIT_TO_DPS_DIV; > >> + *datay = (((s16) ((*datay) << 2)) * BIT_TO_DPS_MUL) / BIT_TO_DPS_DIV; > >> + *dataz = (((s16) ((*dataz) << 2)) * BIT_TO_DPS_MUL) / BIT_TO_DPS_DIV; > >> +} > >> + > >> +static irqreturn_t cmr3000_thread_irq(int irq, void *dev_id) > >> +{ > >> + struct cmr3000_gyro_data *data = dev_id; > >> + int datax, datay, dataz; > >> + u8 mode, intr_status; > >> + > >> + intr_status = data->bus_ops->read(data->dev, CMR3000_STATUS, > >> + "irq status"); > >> + intr_status = data->bus_ops->read(data->dev, CMR3000_CTRL, > >> + "control mode"); > >> + if (intr_status < 0) > >> + return IRQ_NONE; > >> + > >> + /* Interrupt not for this device */ > >> + if (intr_status & CMR3000_IRQDIS) > >> + return IRQ_NONE; > >> + > >> + mode = (intr_status & CMR3000_MODEMASK) >> CMRMODE_SHIFT; > >> + if ((mode != CMRMODE_MEAS80) > >> + && (mode != CMRMODE_MEAS20)) > >> + return IRQ_NONE; > >> + > >> + datax = (data->bus_ops->read(data->dev, CMR3000_X_MSB, "X_MSB")) << 8; > >> + datax |= data->bus_ops->read(data->dev, CMR3000_X_LSB, "X_LSB"); > >> + datay = (data->bus_ops->read(data->dev, CMR3000_Y_MSB, "Y_MSB")) << 8; > >> + datay |= data->bus_ops->read(data->dev, CMR3000_Y_LSB, "Y_LSB"); > >> + dataz = (data->bus_ops->read(data->dev, CMR3000_Z_MSB, "Z_MSB")) << 8; > >> + dataz |= data->bus_ops->read(data->dev, CMR3000_Z_LSB, "Z_LSB"); > >> + > >> + /* Device closed */ > >> + if ((data->mode != CMRMODE_MEAS80) > >> + && (data->mode != CMRMODE_MEAS20)) > >> + return IRQ_NONE; > >> + > >> + /* Decode register values to dps */ > >> + decode_dps(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 cmr3000_poweron(struct cmr3000_gyro_data *data) > >> +{ > >> + const struct cmr3000_platform_data *pdata = &data->pdata; > >> + u8 ctrl; > >> + int ret; > >> + > >> + ctrl = pdata->irq_level << CMRIRQLEVEL_SHIFT; > >> + ctrl |= data->mode << CMRMODE_SHIFT; > >> + ctrl |= data->bus_ops->ctrl_mod; > >> + ret = data->bus_ops->write(data->dev, CMR3000_CTRL, ctrl, > >> + "Mode setting"); > >> + if (ret < 0) > >> + return -EIO; > >> + > >> + msleep(CMR3000_SETDELAY); > >> + > >> + return 0; > >> +} > >> + > >> +static int cmr3000_poweroff(struct cmr3000_gyro_data *data) > >> +{ > >> + int ret; > >> + u8 ctrl = CMRMODE_POFF; > >> + > >> + ctrl |= data->bus_ops->ctrl_mod; > >> + ctrl |= CMR3000_IRQDIS; > >> + > >> + ret = data->bus_ops->write(data->dev, CMR3000_CTRL, ctrl, > >> + "Mode setting"); > >> + msleep(CMR3000_SETDELAY); > >> + > >> + return ret; > >> +} > >> + > >> +static int cmr3000_reset(struct cmr3000_gyro_data *data) > >> +{ > >> + int val; > >> + > >> + /* Reset chip */ > >> + data->bus_ops->write(data->dev, CMR3000_CTRL, CMR3000_RST, "Reset"); > >> + mdelay(2); > >> + > >> + /* Settling time delay */ > >> + val = data->bus_ops->read(data->dev, CMR3000_STATUS, "Status"); > >> + if (val < 0) { > >> + dev_err(data->dev, "Reset failed\n"); > >> + return val; > >> + } > >> + > >> + if (val & CMR3000_STATUS_PERR) { > >> + dev_err(data->dev, "Parity Error\n"); > >> + return -EIO; > >> + } > >> + > >> + return cmr3000_poweroff(data); > >> +} > >> + > >> +static int cmr3000_open(struct input_dev *input_dev) > >> +{ > >> + struct cmr3000_gyro_data *data = input_get_drvdata(input_dev); > >> + > >> + mutex_lock(&data->mutex); > >> + > >> + if (!data->suspended) > >> + cmr3000_poweron(data); > >> + > >> + data->opened = true; > >> + > >> + mutex_unlock(&data->mutex); > >> + > >> + return 0; > >> +} > >> + > >> +static void cmr3000_close(struct input_dev *input_dev) > >> +{ > >> + struct cmr3000_gyro_data *data = input_get_drvdata(input_dev); > >> + > >> + mutex_lock(&data->mutex); > >> + > >> + if (!data->suspended) > >> + cmr3000_poweroff(data); > >> + > >> + data->opened = false; > >> + > >> + mutex_unlock(&data->mutex); > >> +} > >> + > >> +void cmr3000_suspend(struct cmr3000_gyro_data *data) > >> +{ > >> + mutex_lock(&data->mutex); > >> + > >> + if (!data->suspended && data->opened) > >> + cmr3000_poweroff(data); > >> + > >> + data->suspended = true; > >> + > >> + mutex_unlock(&data->mutex); > >> +} > >> +EXPORT_SYMBOL(cmr3000_suspend); > >> + > >> +void cmr3000_resume(struct cmr3000_gyro_data *data) > >> +{ > >> + mutex_lock(&data->mutex); > >> + > >> + if (data->suspended && data->opened) > >> + cmr3000_poweron(data); > >> + > >> + data->suspended = false; > >> + > >> + mutex_unlock(&data->mutex); > >> +} > >> +EXPORT_SYMBOL(cmr3000_resume); These four functions bother me. All they are doing is wrapping poweron/poweroff operation. It makes me wonder what is missing from core code that makes this song-and-dance necessary (or is there something available in core code that this driver should be using). > >> + > >> +#ifdef CONFIG_OF > >> +void cmr3000_get_pdata_of(struct device *dev, struct cmr3000_gyro_data *data) > >> +{ > >> + const __be32 *property; > >> + int len; > >> + > >> + property = of_get_property(dev->of_node, "vti,irq_level", &len); > >> + if (property && len == sizeof(int)) > >> + data->pdata.irq_level = be32_to_cpup(property); > >> + > >> + property = of_get_property(dev->of_node, "vti,mode", &len); > >> + if (property && len == sizeof(int)) > >> + data->pdata.mode = be32_to_cpup(property); > >> + > >> + property = of_get_property(dev->of_node, "vti,fuzz_x", &len); > >> + if (property && len == sizeof(int)) > >> + data->pdata.fuzz_x = be32_to_cpup(property); > >> + > >> + property = of_get_property(dev->of_node, "vti,fuzz_y", &len); > >> + if (property && len == sizeof(int)) > >> + data->pdata.fuzz_y = be32_to_cpup(property); > >> + > >> + property = of_get_property(dev->of_node, "vti,fuzz_z", &len); > >> + if (property && len == sizeof(int)) > >> + data->pdata.fuzz_z = be32_to_cpup(property); > >> + > >> + property = of_get_property(dev->of_node, "vti,irqflags", &len); > >> + if (property && len == sizeof(int)) > >> + data->pdata.irqflags = be32_to_cpup(property); > >> + > >> + return; > >> +} > >> +#endif > >> + > >> +struct cmr3000_gyro_data *cmr3000_init(struct device *dev, int irq, > >> + const struct cmr3000_bus_ops *bops) > >> +{ > >> + struct cmr3000_platform_data *pdata; > >> + struct cmr3000_gyro_data *data; > >> + struct input_dev *input_dev; > >> + int rev; > >> + int error; > >> + > >> + /* if no IRQ return error */ > >> + if (irq == 0) { > >> + error = -EINVAL; > >> + goto err_out; > >> + } > >> + > >> + data = kzalloc(sizeof(struct cmr3000_gyro_data), GFP_KERNEL); Consider devm_kzalloc() so the driver doesn't need to worry about rewinding it. Also, use 'sizeof(*data)' it's safer. g. -- 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
