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

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

 



Hello Grant

 Thanks for your feedback.  I think I will remove this patch from the
patchset, I am already in the 5th iteration :S... When the cma
supports devicetree and spi I will try to re-engineer this driver.

 Thanks again

On Tue, Oct 25, 2011 at 10:28, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> 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.
>



-- 
Ricardo Ribalda
--
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