Re: [PATCHv4 5/6] Input: add CMR3000 gyrsocope driver

From: Grant Likely
Date: Tue Oct 25 2011 - 04:28:24 EST

Next message: Grant Likely: "Re: [PATCHv4 4/6] input/cma3000_d0x: Add CMA3000 spi support"
Previous message: H. Peter Anvin: "Re: kernel.org tarball/patch signature files"
In reply to: Ricardo Ribalda Delgado: "Re: [PATCHv4 5/6] Input: add CMR3000 gyrsocope driver"
Next in thread: Ricardo Ribalda Delgado: "[PATCHv4 6/6] input/cma3000_d0x: Match comment to name of struct"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Grant Likely: "Re: [PATCHv4 4/6] input/cma3000_d0x: Add CMA3000 spi support"
Previous message: H. Peter Anvin: "Re: kernel.org tarball/patch signature files"
In reply to: Ricardo Ribalda Delgado: "Re: [PATCHv4 5/6] Input: add CMR3000 gyrsocope driver"
Next in thread: Ricardo Ribalda Delgado: "[PATCHv4 6/6] input/cma3000_d0x: Match comment to name of struct"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]