| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPybu_1yX85JkH-wVk3c76_-Q7miF_A01ZUEs_CMuta1OjdRsA@mail.gmail.com>
Date: Wed, 2 Nov 2011 15:28:36 +0100
From: Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To: Grant Likely <grant.likely@...retlab.ca>
Cc: dmitry.torokhov@...il.com, sameo@...ux.intel.com,
peter.ujfalusi@...com, jic23@....ac.uk, aghayal@...eaurora.org,
david@...deman.nu, Shubhrajyoti@...com, saaguirre@...com,
hemanthv@...com, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 5/6] Input: add CMR3000 gyrsocope driver
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@...retlab.ca> 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@...retlab.ca> 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@...il.com>
>> >> ---
>> >> 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@...il.com>
>> >> + *
>> >> + * 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists