Re: [PATCH 3/8] Freescale MMA7455 accelerometer driver

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

 



Hi Gregoire,

Couple of minor style type comments you are welcome to ignore

Bigger ones are that I don't understand your signed 10 bit conversion
functions and the bits of the probe function that don't seem to have been
written (error handling etc).

Jonathan

Gregoire Gentil wrote:
> Signed-off-by: Gregoire Gentil <gregoire@xxxxxxxxxx>
> ---
>  drivers/input/misc/Kconfig    |    9 +
>  drivers/input/misc/Makefile   |    2 +-
>  drivers/input/misc/mma7455l.c |  626 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/mma7455l.h      |   11 +
>  4 files changed, 647 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/input/misc/mma7455l.c
>  create mode 100644 include/linux/mma7455l.h
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index a9bb254..454bac7 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -317,4 +317,13 @@ config INPUT_PCAP
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pcap_keys.
>  
> +config INPUT_MMA7455L
> +	tristate "Freescale MMA7455L 3-axis accelerometer"
> +	depends on SPI_MASTER
> +	help
> +	  SPI driver for the Freescale MMA7455L 3-axis accelerometer.
> +
> +	  The userspace interface is a 3-axis (X/Y/Z) relative movement
> +	  Linux input device, reporting REL_[XYZ] events.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index a8b8485..3db6347 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -30,4 +30,4 @@ obj-$(CONFIG_INPUT_WINBOND_CIR)		+= winbond-cir.o
>  obj-$(CONFIG_INPUT_WISTRON_BTNS)	+= wistron_btns.o
>  obj-$(CONFIG_INPUT_WM831X_ON)		+= wm831x-on.o
>  obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
> -
> +obj-$(CONFIG_INPUT_MMA7455L)		+= mma7455l.o
> diff --git a/drivers/input/misc/mma7455l.c b/drivers/input/misc/mma7455l.c
> new file mode 100644
> index 0000000..d415bb0
> --- /dev/null
> +++ b/drivers/input/misc/mma7455l.c
> @@ -0,0 +1,626 @@
> +/* Linux kernel driver for the Freescale MMA7455L 3-axis accelerometer
> + *
> + * Copyright (C) 2009 by Always Innovating, Inc.
> + * Author: Gregoire Gentil <gregoire@xxxxxxxxxx>
> + * Author: Tim Yamin <plasm@xxxxxxxxx>
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + *
> + */
> +
> +/*
> + * What this driver doesn't yet support:
> + *
> + * - I2C
> + * - INT2 handling
> + * - Pulse detection (and the sysctls to control it)
> + * - 10-bit measurement
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/input.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/sysfs.h>
> +#include <linux/gpio.h>
> +
> +#include <linux/mma7455l.h>
> +#include <linux/spi/spi.h>
> +
> +#define MMA7455L_WHOAMI_MAGIC		0x55
> +
> +enum mma7455l_reg {
> +	MMA7455L_REG_XOUTL		= 0x00,
> +	MMA7455L_REG_XOUTH		= 0x01,
> +	MMA7455L_REG_YOUTL		= 0x02,
> +	MMA7455L_REG_YOUTH		= 0x03,
> +	MMA7455L_REG_ZOUTL 		= 0x04,
> +	MMA7455L_REG_ZOUTH		= 0x05,
> +	MMA7455L_REG_XOUT8		= 0x06,
> +	MMA7455L_REG_YOUT8		= 0x07,
> +	MMA7455L_REG_ZOUT8		= 0x08,
> +	MMA7455L_REG_STATUS		= 0x09,
> +	MMA7455L_REG_DETSRC		= 0x0a,
> +	MMA7455L_REG_TOUT		= 0x0b,
> +	MMA7455L_REG_RESERVED1		= 0x0c,
> +	MMA7455L_REG_I2CAD		= 0x0d,
> +	MMA7455L_REG_USRINF		= 0x0e,
> +	MMA7455L_REG_WHOAMI		= 0x0f,
> +	MMA7455L_REG_XOFFL		= 0x10,
> +	MMA7455L_REG_XOFFH		= 0x11,
> +	MMA7455L_REG_YOFFL		= 0x12,
> +	MMA7455L_REG_YOFFH		= 0x13,
> +	MMA7455L_REG_ZOFFL		= 0x14,
> +	MMA7455L_REG_ZOFFH		= 0x15,
> +	MMA7455L_REG_MCTL		= 0x16,
> +	MMA7455L_REG_INTRST		= 0x17,
> +	MMA7455L_REG_CTL1		= 0x18,
> +	MMA7455L_REG_CTL2		= 0x19,
> +	MMA7455L_REG_LDTH		= 0x1a,
> +	MMA7455L_REG_PDTH		= 0x1b,
> +	MMA7455L_REG_PW			= 0x1c,
> +	MMA7455L_REG_LT			= 0x1d,
> +	MMA7455L_REG_TW			= 0x1e,
> +	MMA7455L_REG_RESERVED2		= 0x1f,
> +};
> +
> +enum mma7455l_reg_status {
> +	MMA7455L_STATUS_XDA		= 0x08,
> +	MMA7455L_STATUS_YDA		= 0x10,
> +	MMA7455L_STATUS_ZDA		= 0x20,
> +};
> +
> +enum mma7455l_mode {
> +	MMA7455L_MODE_STANDBY		= 0,
> +	MMA7455L_MODE_MEASUREMENT	= 1,
> +	MMA7455L_MODE_LEVELDETECTION	= 0x42, /* Set DRPD to on */
> +	MMA7455L_MODE_PULSEDETECTION	= 0x43, /* Set DRPD to on */
> +	MMA7455L_MODE_MASK		= 0x43,
> +};
> +
> +enum mma7455l_gselect {
> +	MMA7455L_GSELECT_8		= 0x0,
> +	MMA7455L_GSELECT_2		= 0x4,
> +	MMA7455L_GSELECT_4		= 0x8,
> +	MMA7455L_GSELECT_MASK		= 0xC,
> +};
> +
> +/* FIXME */
> +#define MMA7455L_F_FS			0x0020 	/* ADC full scale */
> +
> +
> +struct mma7455l_info {
> +	struct spi_device *spi_dev;
> +	struct input_dev *input_dev;
> +	struct mutex lock;		/* Struct mutex lock */
> +	struct delayed_work work;
> +
> +	u8 mode;
> +	u8 gSelect;
> +
> +	u8 flags;
> +	u8 working;
> +};
> +
> +/* lowlevel register access functions */
> +
> +#define WRITE_BIT	(1 << 7)
> +#define ADDR_SHIFT	1
> +
> +static inline u_int8_t __reg_read(struct mma7455l_info *mma, u_int8_t reg)
> +{
> +	int rc;
> +	u_int8_t cmd;
> +
> +	cmd = ((reg & 0x3f) << ADDR_SHIFT);
Might as well loose the rc and roll next two lines into one. Might as well roll the
cmd variable in as well seeing as it's relatively straight forward.
Mind you if you think it is clearer this way, feel free to leave it alone.
> +	rc = spi_w8r8(mma->spi_dev, cmd);
> +
> +	return rc;
> +}
> +
> +static u_int8_t reg_read(struct mma7455l_info *mma, u_int8_t reg)
> +{
> +	u_int8_t ret;
> +
> +	mutex_lock(&mma->lock);
> +	ret = __reg_read(mma, reg);
> +	mutex_unlock(&mma->lock);
> +
> +	return ret;
> +}
> +
> +static s16 __reg_read_10(struct mma7455l_info *mma, u8 reg1, u8 reg2)
> +{
> +	u8 v1, v2;
> +
> +	v1 = __reg_read(mma, reg1);
> +	v2 = __reg_read(mma, reg2);
> +
Not sure I follow this.  Are we dealing with an architecture
where we aren't using 2's complement for signed integers? 
If not you'll need
to sign extend rather than what looks to be happening here, where you are
simply moving the sign bit to the 16th lsb.  If you are doing this for
some other reason, please clarify with a comment.
> +	return (v2 & 0x4) << 13 | (v2 & 0x3) << 8 | v1;
> +}
> +
> +static inline int __reg_write(struct mma7455l_info *mma,
> +				u_int8_t reg, u_int8_t val)
> +{
> +	u_int8_t buf[2];
> +
> +	buf[0] = ((reg & 0x3f) << ADDR_SHIFT) | WRITE_BIT;
> +	buf[1] = val;
Could roll these two into an array initialization.
> +
> +	return spi_write(mma->spi_dev, buf, sizeof(buf));
> +}
> +
> +static int reg_write(struct mma7455l_info *mma, u_int8_t reg, u_int8_t val)
> +{
> +	int ret;
> +
> +	mutex_lock(&mma->lock);
> +	ret = __reg_write(mma, reg, val);
> +	mutex_unlock(&mma->lock);
> +
> +	return ret;
> +}
> +
> +static s16 __reg_write_10(struct mma7455l_info *mma,
> +				u8 reg1, u8 reg2, s16 value)
> +{
> +	int ret;
> +	u8 v1, v2;
> +
> +	v1 = value & 0xFF;
This is somewhat odd as well. 
> +	if (value < 0)
> +		v2 = ((value >> 8) & 0x3) | 0x4;
> +	else
> +		v2 = 0;
> +
> +	ret = __reg_write(mma, reg1, v1);
> +	ret = __reg_write(mma, reg2, v2);
> +	return ret;
> +}
> +
> +static void mma7455l_work(struct work_struct *work)
> +{
> +	struct mma7455l_info *mma =
> +			container_of(work, struct mma7455l_info, work.work);
unconventional spacing here.  Should be a newline after parameter declarations
rather than here.
> +
> +	s8 val;
> +	mma->working = 1;
> +
> +	/* FIXME: 10 bit accuracy? */
> +	if (!(mma->flags & MMA7455L_STATUS_XDA)) {
> +		val = reg_read(mma, MMA7455L_REG_XOUT8);
> +		input_report_abs(mma->input_dev, ABS_X, val);
> +	}
> +	if (!(mma->flags & MMA7455L_STATUS_YDA)) {
> +		val = reg_read(mma, MMA7455L_REG_YOUT8);
> +		input_report_abs(mma->input_dev, ABS_Y, val);
> +	}
> +	if (!(mma->flags & MMA7455L_STATUS_ZDA)) {
> +		val = reg_read(mma, MMA7455L_REG_ZOUT8);
> +		input_report_abs(mma->input_dev, ABS_Z, val);
> +	}
> +
> +	mma->working = 0;
> +	input_sync(mma->input_dev);
> +	put_device(&mma->spi_dev->dev);
> +
> +	/* Enable IRQ and clear out interrupt */
> +	reg_write(mma, MMA7455L_REG_INTRST, 0x3);
> +	reg_write(mma, MMA7455L_REG_INTRST, 0x0);
> +	enable_irq(mma->spi_dev->irq);
> +}

Why status given you do nothing with it? 
> +static void mma7455l_schedule_work(struct mma7455l_info *mma)
> +{
> +	int status;
> +
> +	get_device(&mma->spi_dev->dev);
> +	status = schedule_delayed_work(&mma->work, HZ / 10);
> +}
> +
> +static irqreturn_t mma7455l_interrupt(int irq, void *_mma)
> +{
> +	struct mma7455l_info *mma = _mma;
> +	mma7455l_schedule_work(mma);
> +
> +	/* Disable any further interrupts until we have processed
> +	 * the current one */
> +	disable_irq(mma->spi_dev->irq);
> +	return IRQ_HANDLED;
> +}
> +
> +/* sysfs */
> +
> +static void get_mode(struct mma7455l_info *mma, u8 *mode, u8 *gSelect)
> +{
> +	u8 tmp = reg_read(mma, MMA7455L_REG_MCTL);
> +
> +	*mode = tmp & MMA7455L_MODE_MASK;
> +	*gSelect = tmp & MMA7455L_GSELECT_MASK;
> +}
> +
> +static void set_mode(struct mma7455l_info *mma, u8 mode, u8 gSelect)
> +{
> +	reg_write(mma, MMA7455L_REG_MCTL, mode | gSelect);
> +}
> +
> +static void update_mode(struct mma7455l_info *mma, u8 mode, u8 gSelect)
> +{
> +	mma->mode = mode;
> +	mma->gSelect = gSelect;
> +
> +	reg_write(mma, MMA7455L_REG_MCTL, mma->mode | mma->gSelect);
> +}
> +
> +static ssize_t show_measure(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +	s8 x, y, z;
> +	u8 old_Mode, old_gSelect;
> +
> +	get_mode(mma, &old_Mode, &old_gSelect);
> +	set_mode(mma, MMA7455L_MODE_MEASUREMENT, MMA7455L_GSELECT_2);
> +
This would probably benefit from an explanatory comment.
> +	while (reg_read(mma, MMA7455L_REG_STATUS) == 0)
> +		msleep(10);
> +
> +	x = reg_read(mma, MMA7455L_REG_XOUT8);
> +	y = reg_read(mma, MMA7455L_REG_YOUT8);
> +	z = reg_read(mma, MMA7455L_REG_ZOUT8);
> +
> +	set_mode(mma, old_Mode, old_gSelect);
> +	return sprintf(buf, "%d %d %d\n", x, y, z);
> +}
> +
> +static ssize_t show_mode(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +
> +	switch (mma->mode) {
> +	case MMA7455L_MODE_STANDBY:
> +		return sprintf(buf, "Standby\n");
> +		break;
> +	case MMA7455L_MODE_MEASUREMENT:
> +		return sprintf(buf, "Measurement\n");
> +		break;
> +	case MMA7455L_MODE_LEVELDETECTION:
> +		return sprintf(buf, "Level Detection\n");
> +		break;
> +	case MMA7455L_MODE_PULSEDETECTION:
> +		return sprintf(buf, "Pulse Detection\n");
> +		break;
> +	}
> +
Is this possible?  If not I'd go for returning suitable error code rather
than printing.
> +	return sprintf(buf, "Unknown mode!\n");
> +}
> +
> +static ssize_t show_gSelect(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +
> +	switch (mma->gSelect) {
> +	case MMA7455L_GSELECT_8:
> +		return sprintf(buf, "8\n");
> +		break;
> +	case MMA7455L_GSELECT_4:
> +		return sprintf(buf, "4\n");
> +		break;
> +	case MMA7455L_GSELECT_2:
> +		return sprintf(buf, "2\n");
> +		break;
> +	}
> +
Likewise here.  Not useful in a sense to say we don't know what it is.
> +	return sprintf(buf, "Unknown gSelect!\n");
> +}
> +
> +static ssize_t show_level_threshold(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +	return sprintf(buf, "%u\n", reg_read(mma, MMA7455L_REG_LDTH));
> +}
> +
> +static ssize_t show_calibration(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	s16 x, y, z;
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +
> +	mutex_lock(&mma->lock);
> +	x = __reg_read_10(mma, MMA7455L_REG_XOFFL, MMA7455L_REG_XOFFH);
> +	y = __reg_read_10(mma, MMA7455L_REG_YOFFL, MMA7455L_REG_YOFFH);
> +	z = __reg_read_10(mma, MMA7455L_REG_ZOFFL, MMA7455L_REG_ZOFFH);
> +	mutex_unlock(&mma->lock);
> +
> +	return sprintf(buf, "%d %d %d\n", x, y, z);
> +}

Are there any more standardized terms for these?  Not sure having sysfs interface
where you have to write such long names is ideal...
> +static ssize_t write_mode(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +
> +	if (!strncmp(buf, "Standby", count))
> +		update_mode(mma, MMA7455L_MODE_STANDBY, mma->gSelect);
> +	else if (!strncmp(buf, "Measurement", count))
> +		update_mode(mma, MMA7455L_MODE_MEASUREMENT, mma->gSelect);
> +	else if (!strncmp(buf, "Level Detection", count))
> +		update_mode(mma, MMA7455L_MODE_LEVELDETECTION, mma->gSelect);
> +	else if (!strncmp(buf, "Pulse Detection", count))
> +		update_mode(mma, MMA7455L_MODE_PULSEDETECTION, mma->gSelect);
> +
> +	return count;
> +}
> +
> +static ssize_t write_gSelect(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	unsigned long v;
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +
Worth rounding to nearest rather than only accepting a subset that the userspace
program might not know about? Either that or publishing via sysfs what values
are valid?  Or at absolute minimum documenting this in the api docs.
> +	if (strict_strtoul(buf, 10, &v) == 0) {
> +		switch (v) {
> +		case 8:
> +			update_mode(mma, mma->mode, MMA7455L_GSELECT_8);
> +			break;
> +		case 4:
> +			update_mode(mma, mma->mode, MMA7455L_GSELECT_4);
> +			break;
> +		case 2:
> +			update_mode(mma, mma->mode, MMA7455L_GSELECT_2);
> +			break;
> +		default:
> +			return -EINVAL;
> +			break;
> +		}
> +		return count;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t write_level_threshold(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	unsigned long v;
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +
> +	if (strict_strtoul(buf, 10, &v) == 0) {
> +		if (v <= 0xFF) {
> +			reg_write(mma, MMA7455L_REG_LDTH, v);
> +			return count;

Else is pointless as dropping through will return same error. Hence
you might as well make this function

unsigned long v;
struct mma7455l_info *mma = dev_get_drvdata(dev);
if ((struct_strtoul(buf, 10, &v) == 0) && (v <= 0xFF))
	return count;
return -EINVAL; 
> +		} else
> +			return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t write_calibration(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	int x, y, z;
> +	struct mma7455l_info *mma = dev_get_drvdata(dev);
> +
> +	if (sscanf(buf, "%d %d %d", &x, &y, &z) == 3) {
> +		mutex_lock(&mma->lock);
> +		__reg_write_10(mma, MMA7455L_REG_XOFFL, MMA7455L_REG_XOFFH, x);
> +		__reg_write_10(mma, MMA7455L_REG_YOFFL, MMA7455L_REG_YOFFH, y);
> +		__reg_write_10(mma, MMA7455L_REG_ZOFFL, MMA7455L_REG_ZOFFH, z);
> +		mutex_unlock(&mma->lock);
> +
> +		return count;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static DEVICE_ATTR(measure, S_IRUGO, show_measure, NULL);
> +static DEVICE_ATTR(mode, S_IRUGO | S_IWUGO, show_mode, write_mode);
> +static DEVICE_ATTR(gSelect, S_IRUGO | S_IWUGO, show_gSelect, write_gSelect);
> +static DEVICE_ATTR(level_threshold, S_IRUGO | S_IWUGO, show_level_threshold,
> +			write_level_threshold);
> +static DEVICE_ATTR(calibration, S_IRUGO | S_IWUGO, show_calibration,
> +			write_calibration);
> +
> +static struct attribute *mma7455l_sysfs_entries[] = {
> +       &dev_attr_measure.attr,
> +	&dev_attr_mode.attr,
> +	&dev_attr_gSelect.attr,
> +	&dev_attr_level_threshold.attr,
> +	&dev_attr_calibration.attr,
> +	NULL
> +};
> +
> +static struct attribute_group mma7455l_attr_group = {
> +       .attrs  = mma7455l_sysfs_entries,
> +};
> +
> +/* input device handling and driver core interaction */
> +static int mma7455l_input_open(struct input_dev *inp)
> +{
> +	struct mma7455l_info *mma = input_get_drvdata(inp);
> +	if (mma->mode == MMA7455L_MODE_STANDBY)
> +		update_mode(mma, MMA7455L_MODE_MEASUREMENT, mma->gSelect);
> +
> +	return 0;
> +}
> +
> +static void mma7455l_input_close(struct input_dev *inp)
> +{
> +	struct mma7455l_info *mma = input_get_drvdata(inp);
> +	update_mode(mma, MMA7455L_MODE_STANDBY, MMA7455L_GSELECT_2);
> +}
> +
> +static int __devinit mma7455l_probe(struct spi_device *spi)
> +{
> +	int rc;
> +	struct mma7455l_info *mma;
> +	struct mma7455l_platform_data *pdata = spi->dev.platform_data;
> +	u_int8_t wai;
> +
> +	mma = kzalloc(sizeof(*mma), GFP_KERNEL);
> +	if (!mma)
> +		return -ENOMEM;
> +
> +	mutex_init(&mma->lock);
> +	INIT_DELAYED_WORK(&mma->work, mma7455l_work);
> +	mma->spi_dev = spi;
> +	mma->flags = 0;
> +	mma->working = 0;
> +
> +	spi_set_drvdata(spi, mma);
> +

There is a lot of repeated clean up code in the error conditions.  Use
goto and do it all at the end of the function.
> +	rc = spi_setup(spi);
> +	if (rc < 0) {
> +		printk(KERN_ERR
> +			"mma7455l error durign spi_setup of mma7455l driver\n");
> +		dev_set_drvdata(&spi->dev, NULL);
> +		kfree(mma);
> +		return rc;
> +	}
> +
> +	wai = reg_read(mma, MMA7455L_REG_WHOAMI);
> +	if (wai != MMA7455L_WHOAMI_MAGIC) {
> +		printk(KERN_ERR
> +			"mma7455l unknown whoami signature 0x%02x\n", wai);
> +		dev_set_drvdata(&spi->dev, NULL);
> +		kfree(mma);
> +		return -ENODEV;
> +	}
> +
> +	rc = request_irq(mma->spi_dev->irq, mma7455l_interrupt,
> +			IRQF_TRIGGER_HIGH, "mma7455l", mma);
> +	if (rc < 0) {
> +		dev_err(&spi->dev, "mma7455l error requesting IRQ %d\n",
> +			mma->spi_dev->irq);
> +		/* FIXME */
Fix what? I'm guessing the fact that there is no cleanup going on.
> +		return rc;
> +	}
> +
> +	rc = sysfs_create_group(&spi->dev.kobj, &mma7455l_attr_group);
> +	if (rc) {
> +		dev_err(&spi->dev, "error creating sysfs group\n");
> +		return rc;
> +	}
> +
> +	/* initialize input layer details */
> +	mma->input_dev = input_allocate_device();
> +	if (!mma->input_dev) {
> +		dev_err(&spi->dev,
> +			"mma7455l Unable to allocate input device\n");
> +		/* FIXME */
All sorts of fun could occur here ;)
> +	}
> +
> +	set_bit(EV_ABS, mma->input_dev->evbit);
> +	set_bit(ABS_X, mma->input_dev->absbit);
> +	set_bit(ABS_Y, mma->input_dev->absbit);
> +	set_bit(ABS_Z, mma->input_dev->absbit);
> +
> +	input_set_drvdata(mma->input_dev, mma);
> +	mma->input_dev->name = "MMA7455L";
> +	mma->input_dev->open = mma7455l_input_open;
> +	mma->input_dev->close = mma7455l_input_close;
> +
> +	rc = input_register_device(mma->input_dev);
> +	if (!rc) {
> +		update_mode(mma, MMA7455L_MODE_STANDBY, MMA7455L_GSELECT_2);
> +
> +		mutex_lock(&mma->lock);
> +		__reg_write_10(mma, MMA7455L_REG_XOFFL,
> +				MMA7455L_REG_XOFFH, pdata->calibration_x);
> +		__reg_write_10(mma, MMA7455L_REG_YOFFL,
> +				MMA7455L_REG_YOFFH, pdata->calibration_y);
> +		__reg_write_10(mma, MMA7455L_REG_ZOFFL,
> +				MMA7455L_REG_ZOFFH, pdata->calibration_z);
> +		mutex_unlock(&mma->lock);
> +
> +		return 0;
> +	}
> +
> +	input_free_device(mma->input_dev);
> +	return rc;
> +}
> +
> +static int __devexit mma7455l_remove(struct spi_device *spi)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(&spi->dev);
> +
> +	sysfs_remove_group(&spi->dev.kobj, &mma7455l_attr_group);
> +	input_unregister_device(mma->input_dev);
> +	dev_set_drvdata(&spi->dev, NULL);
> +	kfree(mma);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int mma7455l_suspend(struct spi_device *spi, pm_message_t message)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(&spi->dev);
> +	get_mode(mma, &mma->mode, &mma->gSelect);
> +	set_mode(mma, MMA7455L_MODE_STANDBY, MMA7455L_GSELECT_2);
> +
> +	return 0;
> +}
> +
> +static int mma7455l_resume(struct spi_device *spi)
> +{
> +	struct mma7455l_info *mma = dev_get_drvdata(&spi->dev);
> +	update_mode(mma, mma->mode, mma->gSelect);
> +
> +	return 0;
> +}
> +#else
> +#define mma7455l_suspend NULL
> +#define mma7455l_resume  NULL
> +#endif
> +
> +static struct spi_driver mma7455l_driver = {
> +	.driver = {
> +		.name	= "mma7455l",
> +		.owner	= THIS_MODULE,
> +	},
> +
> +	.probe	 = mma7455l_probe,
> +	.remove	 = __devexit_p(mma7455l_remove),
> +	.suspend = mma7455l_suspend,
> +	.resume	 = mma7455l_resume,
> +};
> +
> +static int __init mma7455l_init(void)
> +{
> +	return spi_register_driver(&mma7455l_driver);
> +}
> +
> +static void __exit mma7455l_exit(void)
> +{
> +	spi_unregister_driver(&mma7455l_driver);
> +}
> +
> +MODULE_AUTHOR("Gregoire Gentil <gregoire@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +
> +module_init(mma7455l_init);
> diff --git a/include/linux/mma7455l.h b/include/linux/mma7455l.h
> new file mode 100644
> index 0000000..12ab50a
> --- /dev/null
> +++ b/include/linux/mma7455l.h
> @@ -0,0 +1,11 @@
> +#ifndef _LINUX_MMA7455L_H
> +#define _LINUX_MMA7455L_H
> +
> +struct mma7455l_platform_data {
> +	/* Calibration offsets */
> +	s16 calibration_x;
> +	s16 calibration_y;
> +	s16 calibration_z;
> +};
> +
> +#endif /* _LINUX_MMA7455L_H */

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