Message-ID: <20150327080327.GA10964@mwanda>
Date:	Fri, 27 Mar 2015 11:03:27 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Martin Kepplinger <martink@...teo.de>
Cc:	gregkh@...uxfoundation.org, mark.rutland@....com,
	devel@...verdev.osuosl.org, dmitry.torokhov@...il.com,
	Martin Kepplinger <martin.kepplinger@...obroma-systems.com>,
	Pawel.Moll@....com, ijc+devicetree@...lion.org.uk,
	varkabhadram@...il.com, linux-kernel@...r.kernel.org,
	robh+dt@...nel.org, hadess@...ess.net, galak@...eaurora.org,
	Christoph Muellner <christoph.muellner@...obroma-systems.com>,
	benjamin.tissoires@...il.com, alexander.stein@...tec-electronic.com
Subject: Re: [PATCH v6] add support for Freescale's MMA8653FC 10 bit
 accelerometer

On Fri, Mar 27, 2015 at 08:38:55AM +0100, Martin Kepplinger wrote:
> From: Martin Kepplinger <martin.kepplinger@...obroma-systems.com>
> 
> The MMA8653FC is a low-power, three-axis, capacitive micromachined
> accelerometer with 10 bits of resolution with flexible user-programmable
> options.
> 
> Embedded interrupt functions enable overall power savings, by relieving the
> host processor from continuously polling data, for example using the poll()
> system call.
> 
> The device can be configured to generate wake-up interrupt signals from any
> combination of the configurable embedded functions, enabling the MMA8653FC
> to monitor events while remaining in a low-power mode during periods of
> inactivity.
> 
> This driver provides devicetree properties to program the device's behaviour
> and a simple, tested and documented sysfs interface. The data sheet and more
> information is available on Freescale's website.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@...obroma-systems.com>
> Signed-off-by: Christoph Muellner <christoph.muellner@...obroma-systems.com>
> ---
> 
> patch revision history
> ......................
> v6 fix staging integration, base it on next-20150326 and change recipients
> v5 clean up (suggested by Varka Bhadram) and move the driver to staging
> v4 changes DT propery names, adds a missing interrupt source and removes
>    the DT option to set interrupt line active high due to unsuccesful testing
> v3 moves the driver from drivers/input/misc to drivers/misc
> v2 corrects licensing and commit messages and adds appropriate recipients
> 
>  drivers/staging/Kconfig               |   2 +
>  drivers/staging/Makefile              |   1 +
>  drivers/staging/mma8653fc/Kconfig     |  10 +
>  drivers/staging/mma8653fc/Makefile    |   1 +
>  drivers/staging/mma8653fc/TODO        | 146 ++++++
>  drivers/staging/mma8653fc/mma8653fc.c | 864 ++++++++++++++++++++++++++++++++++
>  6 files changed, 1024 insertions(+)
>  create mode 100644 drivers/staging/mma8653fc/Kconfig
>  create mode 100644 drivers/staging/mma8653fc/Makefile
>  create mode 100644 drivers/staging/mma8653fc/TODO
>  create mode 100644 drivers/staging/mma8653fc/mma8653fc.c
> 
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index bfacf69..834d949 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -112,4 +112,6 @@ source "drivers/staging/i2o/Kconfig"
>  
>  source "drivers/staging/fsl-mc/Kconfig"
>  
> +source "drivers/staging/mma8653fc/Kconfig"
> +
>  endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 2bbd1bf..cfea86a 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -48,3 +48,4 @@ obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD)	+= clocking-wizard/
>  obj-$(CONFIG_FB_TFT)		+= fbtft/
>  obj-$(CONFIG_I2O)		+= i2o/
>  obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
> +obj-$(CONFIG_MMA8653FC)		+= mma8653fc/
> diff --git a/drivers/staging/mma8653fc/Kconfig b/drivers/staging/mma8653fc/Kconfig
> new file mode 100644
> index 0000000..988451b
> --- /dev/null
> +++ b/drivers/staging/mma8653fc/Kconfig
> @@ -0,0 +1,10 @@
> +config MMA8653FC
> +        tristate "MMA8653FC - Freescale's 3-Axis, 10-bit Digital Accelerometer"
> +        depends on I2C
> +        default n
> +        help
> +          Say Y here if you want to support Freescale's MMA8653FC Accelerometer
> +          through I2C interface.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called mma8653fc.
> diff --git a/drivers/staging/mma8653fc/Makefile b/drivers/staging/mma8653fc/Makefile
> new file mode 100644
> index 0000000..9a245a3
> --- /dev/null
> +++ b/drivers/staging/mma8653fc/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_MMA8653FC)         += mma8653fc.o
> diff --git a/drivers/staging/mma8653fc/TODO b/drivers/staging/mma8653fc/TODO
> new file mode 100644
> index 0000000..0a31225
> --- /dev/null
> +++ b/drivers/staging/mma8653fc/TODO
> @@ -0,0 +1,146 @@
> +- move to IIO device API. The current DT/sysfs interface is documented below
> +
> +Documentation/ABI/testing/...
> +-----------------------------
> +What:		/sys/bus/i2c/drivers/mma8653fc/*/standby
> +Date:		March 2015
> +Contact:	Martin Kepplinger <martin.kepplinger@...obroma-systems.com>
> +Description:
> +		Write 0 to this in order to turn on the device, and 1 to turn
> +		it off. Read to see if it is turned on or off.
> +
> +
> +What:		/sys/bus/i2c/drivers/mma8653fc/*/currentmode
> +Date:		March 2015
> +Contact:	Martin Kepplinger <martin.kepplinger@...obroma-systems.com>
> +Description:
> +		Reading this provides the current state of the device, read
> +		directly from a register. This can be "standby", "wake" or
> +		"sleep".
> +
> +
> +What:		/sys/bus/i2c/drivers/mma8653fc/*/position
> +Date:		March 2015
> +Contact:	Martin Kepplinger <martin.kepplinger@...obroma-systems.com>
> +Description:
> +		Read only. Without interrupts enabled gets current position
> +		values by reading. Poll "position" with interrupt conditions
> +		set, to get notified; see Documentation/.../fsl,mma8653fc.txt
> +
> +		position file format:
> +		"x y z [landscape/portrait status] [front/back status]"
> +
> +		x y z values:
> +			in mg
> +		landscape/portrait status char:
> +			r	landscape right
> +			d	portrait down
> +			u	portrait up
> +			l	landscape left
> +		front/back status char:
> +			f	front facing
> +			b	back facing
> +
> +
> +Documentation/devicetree/bindings/...
> +-------------------------------------
> +Required properties:
> +- compatible
> +	"fsl,mma8653fc"
> +- reg
> +	I2C address
> +
> +Optional properties:
> +
> +- interrupt-parent
> +	a phandle for the interrupt controller (see
> +	Documentation/devicetree/bindings/interrupt-controller/interrupts.txt)
> +- interrupts
> +	interrupt line to which the chip is connected (active low)
> +- int1
> +	set to use interrupt line 1, default is line 2
> +	the interrupt sources can be routed to one of the two lines
> +- ir-freefall-motion-x
> +	activate freefall/motion interrupts on x axis
> +- ir-freefall-motion-y
> +	activate freefall/motion interrupts on y axis
> +- ir-freefall-motion-z
> +	activate freefall/motion interrupts on z axis
> +- irq-threshold
> +	0 < value < 8000: threshold for motion interrupts in mg
> +- ir-landscape-portrait
> +	activate landscape/portrait interrupts
> +- ir-auto-wake
> +	activate wake/sleep change interrupts
> +- ir-data-ready:
> +	activate data-ready interrupts
> +	Interrupt events can be activated in any combination.
> +- dynamic-range
> +	2, 4, or 8: dynamic measurement range in g, default: 2
> +	In ±2 g mode, sensitivity = 256 counts/g.
> +	In ±4 g mode, sensitivity = 128 counts/g.
> +	In ±8 g mode, sensitivity = 64 counts/g.
> +- auto-wake-sleep
> +	auto sleep mode (lower frequency)
> +- motion-mode
> +	use motion mode instead of freefall mode (trigger if >threshold).
> +	per default an interrupt occurs if motion values fall below the
> +	value set in "threshold" and therefore can detect free fall on the
> +	vertical axis (depending on the position of the device).
> +	Setting this values inverts the behaviour and an interrupt occurs
> +	above the threshold value, so usually activate horizontal axis in
> +	this case.
> +
> +- x-offset
> +	0 < value < 500: calibration offset in mg
> +	The offset correction values are used to realign the Zero-g position
> +	of the X, Y, and Z-axis after the device is mounted on a board.
> +	this value has an offset of 250 itself:
> +	0 is -250mg, 250 is 0 mg, 500 is 250mg
> +- y-offset
> +	see x-offset
> +- z-offset
> +	see x-offset
> +
> +Example 1:
> +for a device laying on flat ground to recognize acceleration over 100mg.
> +x-axis is calibrated to +10mg. Adapt interrupt line to your device.
> +
> +mma8653fc@1d {
> +		compatible = "fsl,mma8653fc";
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <5 0>;
> +		reg = <0x1d>;
> +
> +		dynamic-range = <2>;
> +		motion-mode;
> +		ir-freefall-motion-x;
> +		ir-freefall-motion-y;
> +		irq-threshold = <100>;
> +		x-offset = <160>;

Just one tab.

> +};
> +
> +Example 2:
> +for a device mounted on a wall with y being the vertical axis. This recognizes
> +y-acceleration below 800mg, so free fall or changing the orientation of the
> +device (y not being the vertical axis and having ~1000mg anymore).
> +
> +mma8653fc@1d {
> +		compatible = "fsl,mma8653fc";
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <5 0>;
> +		reg = <0x1d>;
> +
> +		dynamic-range = <2>;
> +		ir-freefall-motion-y;
> +		irq-threshold = <800>;
> +};
> +
> +Example 3:
> +minimal example. lets users read current acceleration values. No polling
> +is available.
> +
> +mma8653fc@1d {
> +		compatible = "fsl,mma8653fc";
> +		reg = <0x1d>;
> +};
> diff --git a/drivers/staging/mma8653fc/mma8653fc.c b/drivers/staging/mma8653fc/mma8653fc.c
> new file mode 100644
> index 0000000..4bd7f99
> --- /dev/null
> +++ b/drivers/staging/mma8653fc/mma8653fc.c
> @@ -0,0 +1,864 @@
> +/*
> + * mma8653fc.c - Support for Freescale MMA8653FC 3-axis 10-bit accelerometer
> + *
> + * Copyright (c) 2014 Theobroma Systems Design and Consulting GmbH
> + *
> + * 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.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/types.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +
> +#define DRV_NAME	"mma8653fc"
> +#define MMA8653FC_DEVICE_ID	0x5a
> +
> +#define MMA8653FC_STATUS	0x00
> +
> +#define ZYXOW_MASK	0x80
> +#define ZYXDR		0x08
> +
> +#define MMA8653FC_WHO_AM_I	0x0d
> +
> +#define MMA8653FC_SYSMOD	0x0b
> +#define STATE_STANDBY	0x00
> +#define STATE_WAKE	0x01
> +#define STATE_SLEEP	0x02
> +
> +#define MMA8450_STATUS_ZXYDR	0x08
> +
> +/*
> + * 10 bit output data registers
> + * MSB: 7:0 bits 9:2 of data word
> + * LSB: 7:6 bits 1:0 of data word
> + */
> +#define MMA8653FC_OUT_X_MSB	0x01
> +#define MMA8653FC_OUT_X_LSB	0x02
> +#define MMA8653FC_OUT_Y_MSB	0x03
> +#define MMA8653FC_OUT_Y_LSB	0x04
> +#define MMA8653FC_OUT_Z_MSB	0x05
> +#define MMA8653FC_OUT_Z_LSB	0x06
> +
> +/*
> + * Portrait/Landscape Status
> + */
> +#define MMA8653FC_PL_STATUS	0x10
> +
> +#define NEWLP		0x80
> +#define LAPO_HIGH	0x04
> +#define LAPO_LOW	0x02
> +#define BAFRO		0x01
> +
> +/*
> + * Portrait/Landscape Configuration
> + */
> +#define MMA8653FC_PL_CFG	0x11
> +
> +#define PL_EN		(1 << 6)
> +
> +/*
> + * Data calibration registers
> + */
> +#define MMA8653FC_OFF_X		0x2f
> +#define MMA8653FC_OFF_Y		0x30
> +#define MMA8653FC_OFF_Z		0x31
> +
> +/* 0 to 500 for dts, but -250 to 250 in mg */
> +#define DEFAULT_OFF	250
> +
> +/*
> + * bits 1:0 dynamic range
> + * 00 +/- 2g
> + * 01 +/- 4g
> + * 10 +/- 8g
> + *
> + * HPF_Out bit 4 - data high pass or low pass filtered
> + */
> +#define MMA8653FC_XYZ_DATA_CFG	0x0e
> +
> +#define RANGE_MASK	0x03
> +#define RANGE2G		0x00
> +#define RANGE4G		0x01
> +#define RANGE8G		0x02
> +/* values for calculation */
> +#define SHIFT_2G	8
> +#define INCR_2G		128
> +#define SHIFT_4G	7
> +#define INCR_4G		64
> +#define SHIFT_8G	6
> +#define INCR_8G		32
> +#define DYN_RANGE_2G	2
> +#define DYN_RANGE_4G	4
> +#define DYN_RANGE_8G	8
> +
> +/*
> + * System Control Reg 1
> + */
> +#define MMA8653FC_CTRL_REG1	0x2a
> +
> +#define ACTIVE_BIT	(1 << 0)
> +#define ODR_MASK	0x38
> +#define ODR_DEFAULT	0x20 /* 50 Hz */
> +#define ASLP_RATE_MASK	0xc0
> +#define ASLP_RATE_DEFAULT 0x80 /* 6.25 Hz */
> +
> +/*
> + * Sys Control Reg 2
> + *
> + * auto-sleep enable, software reset
> + */
> +#define MMA8653FC_CTRL_REG2	0x2b
> +
> +#define SLPE		(1 << 2)
> +#define SELFTEST	(1 << 7)
> +#define SOFT_RESET	(1 << 6)
> +
> +/*
> + * Interrupt Source
> + */
> +#define MMA8653FC_INT_SOURCE	0x0c
> +
> +#define SRC_ASLP	(1 << 7)
> +#define SRC_LNDPRT	(1 << 4)
> +#define SRC_FF_MT	(1 << 2)
> +#define SRC_DRDY	(1 << 0)
> +
> +/*
> + * Interrupt Control Register
> + *
> + * default: active low
> + * default push-pull, not open-drain
> + */
> +#define MMA8653FC_CTRL_REG3	0x2c
> +
> +#define WAKE_LNDPRT	(1 << 5)
> +#define WAKE_FF_MT	(1 << 3)
> +#define IPOL		(1 << 1)
> +#define PP_OD		(1 << 0)
> +
> +/*
> + * Interrupt Enable Register
> + */
> +#define MMA8653FC_CTRL_REG4	0x2d
> +
> +#define INT_EN_ASLP	(1 << 7)
> +#define INT_EN_LNDPRT	(1 << 4)
> +#define INT_EN_FF_MT	(1 << 2)
> +#define INT_EN_DRDY	(1 << 0)
> +
> +/*
> + * Interrupt Configuration Register
> + * bit value 0 ... INT2 (default)
> + * bit value 1 ... INT1
> + */
> +#define MMA8653FC_CTRL_REG5	0x2e
> +
> +#define INT_CFG_ASLP	(1 << 7)
> +#define INT_CFG_LNDPRT	(1 << 4)
> +#define INT_CFG_FF_MT	(1 << 2)
> +#define INT_CFG_DRDY	(1 << 0)
> +
> +/*
> + * Freefall / Motion Configuration Register
> + *
> + * Event Latch enable/disable, motion or freefall mode
> + * and event flag enable per axis
> + */
> +#define MMA8653FC_FF_MT_CFG	0x15
> +
> +#define FF_MT_CFG_ELE	(1 << 7)
> +#define FF_MT_CFG_OAE	(1 << 6)
> +#define FF_MT_CFG_ZEFE	(1 << 5)
> +#define FF_MT_CFG_YEFE	(1 << 4)
> +#define FF_MT_CFG_XEFE	(1 << 3)
> +
> +/*
> + * Freefall / Motion Source Register
> + */
> +#define MMA8653FC_FF_MT_SRC	0x16
> +
> +/*
> + * Freefall / Motion Threshold Register
> + *
> + * define motion threshold
> + * 0.063 g/LSB, 127 counts(0x7f) (7 bit from LSB)
> + * range: 0.063g - 8g
> + */
> +#define MMA8653FC_FF_MT_THS	0x17
> +
> +struct axis_triple {
> +	s16 x;
> +	s16 y;
> +	s16 z;
> +};
> +
> +struct mma8653fc_pdata {
> +	s8 x_axis_offset;
> +	s8 y_axis_offset;
> +	s8 z_axis_offset;
> +	bool auto_wake_sleep;
> +	u32 range;
> +	bool int1;
> +	bool motion_mode;
> +	u8 freefall_motion_thr;
> +	bool int_src_data_ready;
> +	bool int_src_ff_mt_x;
> +	bool int_src_ff_mt_y;
> +	bool int_src_ff_mt_z;
> +	bool int_src_lndprt;
> +	bool int_src_aslp;
> +};
> +
> +struct mma8653fc {
> +	struct i2c_client *client;
> +	struct mutex mutex;
> +	struct mma8653fc_pdata pdata;
> +	struct axis_triple saved;
> +	char orientation;
> +	char bafro;
> +	bool standby;
> +	int irq;
> +	unsigned int_mask;
> +	u8 (*read)(struct mma8653fc *, unsigned char);
> +	void (*write)(struct mma8653fc *, unsigned char, unsigned char);
> +};
> +
> +/* defaults */
> +static const struct mma8653fc_pdata mma8653fc_default_init = {
> +	.range = 2,
> +	.x_axis_offset = DEFAULT_OFF,
> +	.y_axis_offset = DEFAULT_OFF,
> +	.z_axis_offset = DEFAULT_OFF,
> +	.auto_wake_sleep = false,
> +	.int1 = false,
> +	.motion_mode = false,
> +	.freefall_motion_thr = 5,
> +	.int_src_data_ready = false,
> +	.int_src_ff_mt_x = false,
> +	.int_src_ff_mt_y = false,
> +	.int_src_ff_mt_z = false,
> +	.int_src_lndprt = false,
> +	.int_src_aslp = false,
> +};
> +
> +static void mma8653fc_get_triple(struct mma8653fc *mma)
> +{
> +	u8 buf[6];
> +	u8 status;
> +
> +	buf[0] = 0;

We don't use this.

> +
> +	status = mma->read(mma, MMA8653FC_STATUS);
> +	if (status & ZYXOW_MASK)
> +		dev_dbg(&mma->client->dev, "previous read not completed\n");
> +
> +	buf[0] = mma->read(mma, MMA8653FC_OUT_X_MSB);
> +	buf[1] = mma->read(mma, MMA8653FC_OUT_X_LSB);
> +	buf[2] = mma->read(mma, MMA8653FC_OUT_Y_MSB);
> +	buf[3] = mma->read(mma, MMA8653FC_OUT_Y_LSB);
> +	buf[4] = mma->read(mma, MMA8653FC_OUT_Z_MSB);
> +	buf[5] = mma->read(mma, MMA8653FC_OUT_Z_LSB);
> +
> +	mutex_lock(&mma->mutex);
> +	/* move from registers to s16 */
> +	mma->saved.x = (buf[1] | (buf[0] << 8)) >> 6;
> +	mma->saved.y = (buf[3] | (buf[2] << 8)) >> 6;
> +	mma->saved.z = (buf[5] | (buf[4] << 8)) >> 6;

Why is buf[] and array instead of individual variables?

u8 x_msb, x_lsb, etc..

> +	mma->saved.x = sign_extend32(mma->saved.x, 9);
> +	mma->saved.y = sign_extend32(mma->saved.y, 9);
> +	mma->saved.z = sign_extend32(mma->saved.z, 9);
> +
> +	/* calc g, see data sheet and application note */
> +	switch (mma->pdata.range) {
> +	case DYN_RANGE_2G:
> +		mma->saved.x = le16_to_cpu((1000 * mma->saved.x +
> +					    INCR_2G) >> SHIFT_2G);

It might be nicer to break these up like this:

		mma->saved.x = le16_to_cpu((1000 * mma->saved.x + INCR_2G) >>
					   SHIFT_2G);

Little endian math always confuses me.  mma->saved.x starts as little
endian and then we multiply it and add and shift and change it to cpu
endian?  Does that work?

> +		mma->saved.y = le16_to_cpu((1000 * mma->saved.y +
> +					   INCR_2G) >> SHIFT_2G);
> +		mma->saved.z = le16_to_cpu((1000 * mma->saved.z +
> +					   INCR_2G) >> SHIFT_2G);
> +		break;
> +	case DYN_RANGE_4G:
> +		mma->saved.x = le16_to_cpu((1000 * mma->saved.x +
> +					   INCR_4G) >> SHIFT_4G);
> +		mma->saved.y = le16_to_cpu((1000 * mma->saved.y +
> +					   INCR_4G) >> SHIFT_4G);
> +		mma->saved.z = le16_to_cpu((1000 * mma->saved.z +
> +					   INCR_4G) >> SHIFT_4G);
> +		break;
> +	case DYN_RANGE_8G:
> +		mma->saved.x = le16_to_cpu((1000 * mma->saved.x +
> +					   INCR_8G) >> SHIFT_8G);
> +		mma->saved.y = le16_to_cpu((1000 * mma->saved.y +
> +					   INCR_8G) >> SHIFT_8G);
> +		mma->saved.z = le16_to_cpu((1000 * mma->saved.z +
> +					   INCR_8G) >> SHIFT_8G);
> +		break;
> +	default:
> +		dev_err(&mma->client->dev, "internal data corrupt\n");
> +	}
> +	mutex_unlock(&mma->mutex);
> +}
> +
> +static void mma8653fc_get_orientation(struct mma8653fc *mma, u8 byte)
> +{
> +	if ((byte & LAPO_HIGH) && !(LAPO_LOW))
> +		mma->orientation = 'r'; /* landscape right */
> +	if (!(byte & LAPO_HIGH) && (byte & LAPO_LOW))
> +		mma->orientation = 'd'; /* portrait down */
> +	if (!(byte & LAPO_HIGH) && !(byte & LAPO_LOW))
> +		mma->orientation = 'u'; /* portrait up */
> +	if ((byte & LAPO_HIGH) && (byte & LAPO_LOW))
> +		mma->orientation = 'l'; /* landscape left */
> +
> +	if (byte & BAFRO)
> +		mma->bafro = 'b'; /* back facing */
> +	else
> +		mma->bafro = 'f'; /* front facing */

We could use defines here maybe:

#define LANDSCAPE_RIGHT 'r'

> +}
> +
> +static irqreturn_t mma8653fc_irq(int irq, void *handle)
> +{
> +	struct mma8653fc *mma = handle;
> +	u8 int_src;
> +	u8 byte;
> +
> +	int_src = mma->read(mma, MMA8653FC_INT_SOURCE);
> +	if (int_src & SRC_DRDY)
> +		/* data ready handle */

Where is the body of this if statement?

> +	if (int_src & SRC_FF_MT) {
> +		/* freefall/motion change handle */
> +		dev_dbg(&mma->client->dev,
> +			"freefall or motion change\n");
> +		byte = mma->read(mma, MMA8653FC_FF_MT_SRC);
> +	}
> +	if (int_src & SRC_LNDPRT) {
> +		/* landscape/portrait change handle */
> +		dev_dbg(&mma->client->dev,
> +			"landscape / portrait change\n");
> +		byte = mma->read(mma, MMA8653FC_PL_STATUS);
> +		mma8653fc_get_orientation(mma, byte);
> +	}
> +	if (int_src & SRC_ASLP)
> +		/* autosleep change handle */

Same.

> +	mma8653fc_get_triple(mma);
> +
> +	sysfs_notify(&mma->client->dev.kobj, NULL, "position");
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void __mma8653fc_enable(struct mma8653fc *mma)
> +{
> +	u8 byte;
> +
> +	byte = mma->read(mma, MMA8653FC_CTRL_REG1);
> +	mma->write(mma, MMA8653FC_CTRL_REG1, byte | ACTIVE_BIT);
> +}
> +
> +static void __mma8653fc_disable(struct mma8653fc *mma)
> +{
> +	u8 byte;
> +
> +	byte = mma->read(mma, MMA8653FC_CTRL_REG1);
> +	mma->write(mma, MMA8653FC_CTRL_REG1, byte & ~ACTIVE_BIT);
> +}
> +
> +static ssize_t mma8653fc_standby_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct mma8653fc *mma = i2c_get_clientdata(client);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%u\n", mma->standby);
> +}
> +
> +static ssize_t mma8653fc_standby_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct mma8653fc *mma = i2c_get_clientdata(client);
> +	unsigned int val;
> +	int error;
> +
> +	error = kstrtouint(buf, 10, &val);
> +	if (error)
> +		return error;
> +
> +	mutex_lock(&mma->mutex);
> +
> +	if (val) {
> +		if (!mma->standby)
> +			__mma8653fc_disable(mma);
> +	} else {
> +		if (mma->standby)
> +			__mma8653fc_enable(mma);
> +	}
> +
> +	mma->standby = !!val;
> +
> +	mutex_unlock(&mma->mutex);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(standby, 0664, mma8653fc_standby_show,
> +				  mma8653fc_standby_store);
> +
> +static ssize_t mma8653fc_currentmode_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct mma8653fc *mma = i2c_get_clientdata(client);
> +	ssize_t count;
> +	u8 byte;
> +
> +	byte = mma->read(mma, MMA8653FC_SYSMOD);
> +	if (byte < 0)
> +		return byte;
> +
> +	switch (byte) {
> +	case STATE_STANDBY:
> +		count = scnprintf(buf, PAGE_SIZE, "standby\n");
> +		break;
> +	case STATE_WAKE:
> +		count = scnprintf(buf, PAGE_SIZE, "wake\n");
> +		break;
> +	case STATE_SLEEP:
> +		count = scnprintf(buf, PAGE_SIZE, "sleep\n");
> +		break;
> +	default:
> +		count = scnprintf(buf, PAGE_SIZE, "unknown\n");
> +	}
> +	return count;
> +}
> +static DEVICE_ATTR(currentmode, S_IRUGO, mma8653fc_currentmode_show, NULL);
> +
> +static ssize_t mma8653fc_position_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct mma8653fc *mma = i2c_get_clientdata(client);
> +	ssize_t count;
> +	u8 byte;
> +
> +	if (!mma->irq) {
> +		byte = mma->read(mma, MMA8653FC_PL_STATUS);
> +		if (byte & NEWLP)
> +			mma8653fc_get_orientation(mma, byte);
> +
> +		byte = mma->read(mma, MMA8653FC_STATUS);
> +		if (byte & ZYXDR)
> +			mma8653fc_get_triple(mma);
> +
> +		msleep(20);
> +		dev_dbg(&client->dev, "data polled\n");
> +	}
> +	mutex_lock(&mma->mutex);
> +	count = scnprintf(buf, PAGE_SIZE, "%d %d %d %c %c\n",
> +			mma->saved.x, mma->saved.y, mma->saved.z,
> +			mma->orientation, mma->bafro);
> +	mutex_unlock(&mma->mutex);
> +
> +	return count;
> +}
> +static DEVICE_ATTR(position, S_IRUGO, mma8653fc_position_show, NULL);
> +
> +static struct attribute *mma8653fc_attributes[] = {
> +	&dev_attr_position.attr,
> +	&dev_attr_standby.attr,
> +	&dev_attr_currentmode.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group mma8653fc_attr_group = {
> +	.attrs = mma8653fc_attributes,
> +};
> +
> +static u8 mma8653fc_read(struct mma8653fc *mma, unsigned char reg)
> +{
> +	u8 val;
> +
> +	val = i2c_smbus_read_byte_data(mma->client, reg);
> +	if (val < 0) {

u8 is never less than zero.

> +		dev_err(&mma->client->dev,
> +			"failed to read %x register\n", reg);
> +	}
> +	return val;
> +}
> +
> +static void mma8653fc_write(struct mma8653fc *mma, unsigned char reg,
> +			   unsigned char val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(mma->client, reg, val);
> +	if (ret) {
> +		dev_err(&mma->client->dev,
> +			"failed to write %x register\n", reg);
> +	}
> +}
> +
> +static const struct of_device_id mma8653fc_dt_ids[] = {
> +	{ .compatible = "fsl,mma8653fc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mma8653fc_dt_ids);
> +
> +static const struct mma8653fc_pdata *mma8653fc_probe_dt(struct device *dev)
> +{
> +	struct mma8653fc_pdata *pdata;
> +	struct device_node *node = dev->of_node;
> +	const struct of_device_id *match;
> +	u32 testu32;
> +	s32 tests32;
> +
> +	if (!node) {
> +		dev_err(dev, "no associated DT data\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	match = of_match_device(mma8653fc_dt_ids, dev);
> +	if (!match) {
> +		dev_err(dev, "unknown device model\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	*pdata = mma8653fc_default_init;
> +
> +	/* overwrite from dts */
> +	testu32 = pdata->x_axis_offset;
> +	tests32 = 0;

Not used.

> +	of_property_read_u32(node, "x-offset", &testu32);
> +	tests32 = testu32 - DEFAULT_OFF;
> +	if ((tests32) && (tests32 <= DEFAULT_OFF) &&
> +	    (tests32 >= -DEFAULT_OFF)) {

Emacs adds parenthesis all over the place because it is trying to
convert the world to LISP.

> +		dev_info(dev, "use %dmg offset on X axis\n", tests32);
> +		/* calc register value, resolution: 1.96mg */
> +		pdata->x_axis_offset = (s8) (tests32 / 2);
> +	}
> +	testu32 = pdata->y_axis_offset;
> +	tests32 = 0;

Not used.

The signedness bug and the missing if statement bodies could have been
caught by Smatch I think.  It might be worth running.  It gets run
automatically after merging also.  http://smatch.sf.net

regards,
dan carpenter

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