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

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

 



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


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux