RE: [PATCH v3 3/3] iio: add driver for Freescale MMA9553

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

 




> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@xxxxxx]
> Sent: 05 April, 2015 2:18
> To: Tirdea, Irina; Jonathan Cameron; linux-iio@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; Baluta, Daniel; Lars-Peter Clausen; Peter Meerwald
> Subject: Re: [PATCH v3 3/3] iio: add driver for Freescale MMA9553
> 
> Irina Tirdea schrieb am 27.01.2015 um 19:41:
> > Add support for Freescale MMA9553L Intelligent Pedometer Platform.
> >
> > The following functionalities are supported:
> >  - step counter (counts the number of steps using a HW register)
> >  - step detector (generates an iio event at every step the user takes)
> >  - activity recognition (rest, walking, jogging, running)
> >  - speed
> >  - calories
> >  - distance
> >
> > To get accurate pedometer results, the user's height, weight and gender
> > need to be configured.
> >
> > The specifications can be downloaded from:
> > http://www.freescale.com/files/sensors/doc/ref_manual/MMA955xLSWRM.pdf
> > http://www.freescale.com/files/sensors/doc/ref_manual/MMA9553LSWRM.pdf
> >
> 
> Hi Irina,
> I still spotted a few issues and have some recommendations which you
> could address in a follow-up patch. Also, there are some minor style
> issues (spaces after cast, alignment issues, unnecessary blank lines),
> which checkpatch.pl --strict reveals.
> 

Hi Hartmut,

Thanks for the review!
I will run checkpatch.pl --strict and fix any issues as well as all the problems you mentioned below.

> One other issue, where I would like to get the opinion of the other
> maintainers as well, concerns some of the functions you added to
> mma9551_core.c. The way they are used so far doesn't pose a threat, yet.
> But I am worried about the __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS],
> where the elements are accessed in relation to the len parameter. So,
> should there be a check in those functions to prevent buffer overrun
> problems, or just be very careful when using those functions?
> 
I think a check wouldn't hurt and it will prevent any future issues with using this API.
I will add this check in a separate patch from the other fixes so we can see if anybody else has a different opinion.

> > Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio |   49 +-
> >  drivers/iio/accel/Kconfig               |   10 +
> >  drivers/iio/accel/Makefile              |    1 +
> >  drivers/iio/accel/mma9551_core.c        |  183 +++++
> >  drivers/iio/accel/mma9551_core.h        |   17 +-
> >  drivers/iio/accel/mma9553.c             | 1334 +++++++++++++++++++++++++++++++
> >  6 files changed, 1589 insertions(+), 5 deletions(-)
> >  create mode 100644 drivers/iio/accel/mma9553.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index 588620e..9a70c31 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -343,7 +343,30 @@ Description:
> >  		production inaccuracies).  If shared across all channels,
> >  		<type>_calibscale is used.
> >
> > -What:		/sys/bus/iio/devices/iio:deviceX/in_steps_calibheight
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_activity_calibgender
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_energy_calibgender
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_distance_calibgender
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_velocity_calibgender
> > +KernelVersion:	3.20
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		Gender of the user (e.g.: male, female) used by some pedometers
> > +		to compute the stride length, distance, speed and activity
> > +		type.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_activity_calibgender_available
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_energy_calibgender_available
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_distance_calibgender_available
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_velocity_calibgender_available
> > +KernelVersion:	3.20
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		Lists all available gender values (e.g.: male, female).
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_activity_calibheight
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_energy_calibheight
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_distance_calibheight
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_velocity_calibheight
> >  KernelVersion:	3.19
> >  Contact:	linux-iio@xxxxxxxxxxxxxxx
> >  Description:
> > @@ -818,6 +841,14 @@ What:		/sys/.../events/in_tempY_roc_falling_period
> >  What:		/sys/.../events/in_accel_x&y&z_mag_falling_period
> >  What:		/sys/.../events/in_intensity0_thresh_period
> >  What:		/sys/.../events/in_proximity0_thresh_period
> > +What:		/sys/.../events/in_activity_still_thresh_rising_period
> > +What:		/sys/.../events/in_activity_still_thresh_falling_period
> > +What:		/sys/.../events/in_activity_walking_thresh_rising_period
> > +What:		/sys/.../events/in_activity_walking_thresh_falling_period
> > +What:		/sys/.../events/in_activity_jogging_thresh_rising_period
> > +What:		/sys/.../events/in_activity_jogging_thresh_falling_period
> > +What:		/sys/.../events/in_activity_running_thresh_rising_period
> > +What:		/sys/.../events/in_activity_running_thresh_falling_period
> >  KernelVersion:	2.6.37
> >  Contact:	linux-iio@xxxxxxxxxxxxxxx
> >  Description:
> > @@ -1142,6 +1173,12 @@ Description:
> >  		This attribute is used to get/set the integration time in
> >  		seconds.
> >
> > +What:		/sys/.../iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_integration_time
> > +KernelVersion:	3.20
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		Number of seconds in which to compute speed.
> > +
> >  What:		/sys/bus/iio/devices/iio:deviceX/in_rot_quaternion_raw
> >  KernelVersion:	3.15
> >  Contact:	linux-iio@xxxxxxxxxxxxxxx
> > @@ -1170,13 +1207,17 @@ Description:
> >  		present, output should be considered as processed with the
> >  		unit in milliamps.
> >
> > +What:		/sys/.../iio:deviceX/in_energy_en
> > +What:		/sys/.../iio:deviceX/in_distance_en
> > +What:		/sys/.../iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_en
> >  What:		/sys/.../iio:deviceX/in_steps_en
> >  KernelVersion:	3.19
> >  Contact:	linux-iio@xxxxxxxxxxxxxxx
> >  Description:
> > -		Activates the step counter. After activation, the number of steps
> > -		taken by the user will be counted in hardware and exported through
> > -		in_steps_input.
> > +		Activates a device feature that runs in firmware/hardware.
> > +		E.g. for steps: the pedometer saves power while not used;
> > +		when activated, it will count the steps taken by the user in
> > +		firmware and export them through in_steps_input.
> >
> >  What:		/sys/.../iio:deviceX/in_steps_input
> >  KernelVersion:	3.19
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index c53047d..7c9a9a9 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -126,4 +126,14 @@ config MMA9551
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called mma9551.
> >
> > +config MMA9553
> > +	tristate "Freescale MMA9553L Intelligent Pedometer Platform Driver"
> > +	depends on I2C
> > +	select MMA9551_CORE
> > +	help
> > +	  Say yes here to build support for the Freescale MMA9553L
> > +	  Intelligent Pedometer Platform Driver.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called mma9553.
> >  endmenu
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index 8105316..f815695 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_MMA8452)	+= mma8452.o
> >
> >  obj-$(CONFIG_MMA9551_CORE)	+= mma9551_core.o
> >  obj-$(CONFIG_MMA9551)		+= mma9551.o
> > +obj-$(CONFIG_MMA9553)		+= mma9553.o
> >
> >  obj-$(CONFIG_IIO_ST_ACCEL_3AXIS) += st_accel.o
> >  st_accel-y := st_accel_core.o
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index 7f1a73e..7f55a6d 100644
> > --- a/drivers/iio/accel/mma9551_core.c
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -53,6 +53,11 @@
> >  #define MMA9551_AFE_Y_ACCEL_REG		0x02
> >  #define MMA9551_AFE_Z_ACCEL_REG		0x04
> >
> > +/* Reset/Suspend/Clear application */
> > +#define MMA9551_RSC_RESET		0x00
> > +#define MMA9551_RSC_OFFSET(mask)	(3 - (ffs(mask) - 1) / 8)
> > +#define MMA9551_RSC_VAL(mask)		(mask >> (((ffs(mask) - 1) / 8) * 8))
> > +
> >  /*
> >   * A response is composed of:
> >   * - control registers: MB0-3
> > @@ -275,6 +280,64 @@ int mma9551_read_status_byte(struct i2c_client *client, u8 app_id,
> >  EXPORT_SYMBOL(mma9551_read_status_byte);
> >
> >  /**
> > + * mma9551_read_config_word() - read 1 config word
> > + * @client:	I2C client
> > + * @app_id:	Application ID
> > + * @reg:	Application register
> > + * @val:	Pointer to store value read
> > + *
> > + * Read one configuration word from the device using MMA955xL command format.
> > + * Commands to the MMA955xL platform consist of a write followed by one or
> > + * more reads.
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
> > +			    u16 reg, u16 *val)
> > +{
> > +	int ret;
> > +	__be16 v;
> > +
> > +	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> > +			       reg, NULL, 0, (u8 *)&v, 2);
> > +	*val = be16_to_cpu(v);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(mma9551_read_config_word);
> > +
> > +/**
> > + * mma9551_write_config_word() - write 1 config word
> > + * @client:	I2C client
> > + * @app_id:	Application ID
> > + * @reg:	Application register
> > + * @val:	Value to write
> > + *
> > + * Write one configuration word from the device using MMA955xL command format.
> > + * Commands to the MMA955xL platform consist of a write followed by one or
> > + * more reads.
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> > +			     u16 reg, u16 val)
> > +{
> > +	__be16 v = cpu_to_be16(val);
> > +
> > +	return mma9551_transfer(client, app_id, MMA9551_CMD_WRITE_CONFIG, reg,
> > +				(u8 *) &v, 2, NULL, 0);
> > +}
> > +EXPORT_SYMBOL(mma9551_write_config_word);
> > +
> > +/**
> >   * mma9551_read_status_word() - read 1 status word
> >   * @client:	I2C client
> >   * @app_id:	Application ID
> > @@ -306,6 +369,107 @@ int mma9551_read_status_word(struct i2c_client *client, u8 app_id,
> >  EXPORT_SYMBOL(mma9551_read_status_word);
> >
> >  /**
> > + * mma9551_read_config_words() - read multiple config words
> > + * @client:	I2C client
> > + * @app_id:	Application ID
> > + * @reg:	Application register
> > + * @len:	Length of array to read in bytes
> > + * @val:	Array of words to read
> 
> val is called buf in the function declaration.
Will fix all instances.
> 
> > + *
> > + * Read multiple configuration registers (word-sized registers).
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> > +			     u16 reg, u8 len, u16 *buf)
> > +{
> > +	int ret, i;
> > +	int len_words = len / sizeof(u16);
> len_words could be unsigned.
Agree. Will fix all instances of unsigned counters.
> > +	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +
> > +	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> > +			       reg, NULL, 0, (u8 *) be_buf, len);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for (i = 0; i < len_words; i++)
> > +		buf[i] = be16_to_cpu(be_buf[i]);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mma9551_read_config_words);
> > +
> > +/**
> > + * mma9551_read_status_words() - read multiple status words
> > + * @client:	I2C client
> > + * @app_id:	Application ID
> > + * @reg:	Application register
> > + * @len:	Length of array to read in bytes
> > + * @val:	Array of words to read
> 
> Also here, val is called buf in the function declaration.
> 
> > + *
> > + * Read multiple status registers (word-sized registers).
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> > +			      u16 reg, u8 len, u16 *buf)
> > +{
> > +	int ret, i;
> > +	int len_words = len / sizeof(u16);
> 
> Same here.
> 
> > +	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +
> > +	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> > +			       reg, NULL, 0, (u8 *) be_buf, len);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for (i = 0; i < len_words; i++)
> > +		buf[i] = be16_to_cpu(be_buf[i]);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mma9551_read_status_words);
> > +
> > +/**
> > + * mma9551_write_config_words() - write multiple config words
> > + * @client:	I2C client
> > + * @app_id:	Application ID
> > + * @reg:	Application register
> > + * @len:	Length of array to write in bytes
> > + * @val:	Array of words to write
> 
> Same with val/buf here.
> 
> > + *
> > + * Write multiple configuration registers (word-sized registers).
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> > +			       u16 reg, u8 len, u16 *buf)
> > +{
> > +	int i;
> > +	int len_words = len / sizeof(u16);
> 
> Same here.
> 
> > +	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +
> > +	for (i = 0; i < len_words; i++)
> > +		be_buf[i] = cpu_to_be16(buf[i]);
> > +
> > +	return mma9551_transfer(client, app_id, MMA9551_CMD_WRITE_CONFIG,
> > +				reg, (u8 *) be_buf, len, NULL, 0);
> > +}
> > +EXPORT_SYMBOL(mma9551_write_config_words);
> > +
> > +/**
> >   * mma9551_update_config_bits() - update bits in register
> >   * @client:	I2C client
> >   * @app_id:	Application ID
> > @@ -609,6 +773,25 @@ int mma9551_read_accel_scale(int *val, int *val2)
> >  }
> >  EXPORT_SYMBOL(mma9551_read_accel_scale);
> >
> > +/**
> > + * mma9551_app_reset() - reset application
> > + * @client:	I2C client
> > + * @app_mask:	Application to reset
> > + *
> > + * Reset the given application (using the Reset/Suspend/Clear
> > + * Control Application)
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_app_reset(struct i2c_client *client, u32 app_mask)
> > +{
> > +	return mma9551_write_config_byte(client, MMA9551_APPID_RCS,
> 
> Shouldn't this be MMA9551_APPID_RSC?
Yes, it should. Will fix.
> 
> > +					 MMA9551_RSC_RESET +
> > +					 MMA9551_RSC_OFFSET(app_mask),
> > +					 MMA9551_RSC_VAL(app_mask));
> > +}
> > +EXPORT_SYMBOL(mma9551_app_reset);
> > +
> >  MODULE_AUTHOR("Irina Tirdea <irina.tirdea@xxxxxxxxx>");
> >  MODULE_AUTHOR("Vlad Dogaru <vlad.dogaru@xxxxxxxxx>");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
> > index e6efd02..edaa56b 100644
> > --- a/drivers/iio/accel/mma9551_core.h
> > +++ b/drivers/iio/accel/mma9551_core.h
> > @@ -21,9 +21,13 @@
> >  #define MMA9551_APPID_AFE		0x06
> >  #define MMA9551_APPID_TILT		0x0B
> >  #define MMA9551_APPID_SLEEP_WAKE	0x12
> > -#define MMA9551_APPID_RESET		0x17
> > +#define MMA9551_APPID_PEDOMETER	        0x15
> > +#define MMA9551_APPID_RCS		0x17
> 
> And here as well _RSC?
> 
> >  #define MMA9551_APPID_NONE		0xff
> >
> > +/* Reset/Suspend/Clear application app masks */
> > +#define MMA9551_RSC_PED			BIT(21)
> > +
> >  #define MMA9551_AUTO_SUSPEND_DELAY_MS	2000
> >
> >  enum mma9551_gpio_pin {
> > @@ -48,8 +52,18 @@ int mma9551_write_config_byte(struct i2c_client *client, u8 app_id,
> >  			      u16 reg, u8 val);
> >  int mma9551_read_status_byte(struct i2c_client *client, u8 app_id,
> >  			     u16 reg, u8 *val);
> > +int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
> > +			    u16 reg, u16 *val);
> > +int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> > +			     u16 reg, u16 val);
> >  int mma9551_read_status_word(struct i2c_client *client, u8 app_id,
> >  			     u16 reg, u16 *val);
> > +int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> > +			     u16 reg, u8 len, u16 *buf);
> > +int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> > +			      u16 reg, u8 len, u16 *buf);
> > +int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> > +			       u16 reg, u8 len, u16 *buf);
> >  int mma9551_update_config_bits(struct i2c_client *client, u8 app_id,
> >  			       u16 reg, u8 mask, u8 val);
> >  int mma9551_gpio_config(struct i2c_client *client, enum mma9551_gpio_pin pin,
> > @@ -62,5 +76,6 @@ int mma9551_read_accel_chan(struct i2c_client *client,
> >  			    const struct iio_chan_spec *chan,
> >  			    int *val, int *val2);
> >  int mma9551_read_accel_scale(int *val, int *val2);
> > +int mma9551_app_reset(struct i2c_client *client, u32 app_mask);
> >
> >  #endif /* _MMA9551_CORE_H_ */
> > diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> > new file mode 100644
> > index 0000000..d23ebf1
> > --- /dev/null
> > +++ b/drivers/iio/accel/mma9553.c
> > @@ -0,0 +1,1334 @@
> > +/*
> > + * Freescale MMA9553L Intelligent Pedometer driver
> > + * Copyright (c) 2014, Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope 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/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/slab.h>
> > +#include <linux/acpi.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/pm_runtime.h>
> > +#include "mma9551_core.h"
> > +
> > +#define MMA9553_DRV_NAME			"mma9553"
> > +#define MMA9553_IRQ_NAME			"mma9553_event"
> > +#define MMA9553_GPIO_NAME			"mma9553_int"
> > +
> > +/* Pedometer configuration registers (R/W) */
> > +#define MMA9553_REG_CONF_SLEEPMIN		0x00
> > +#define MMA9553_REG_CONF_SLEEPMAX		0x02
> > +#define MMA9553_REG_CONF_SLEEPTHD		0x04
> > +#define MMA9553_MASK_CONF_WORD			GENMASK(15, 0)
> > +
> > +#define MMA9553_REG_CONF_CONF_STEPLEN		0x06
> > +#define MMA9553_MASK_CONF_CONFIG		BIT(15)
> > +#define MMA9553_MASK_CONF_ACT_DBCNTM		BIT(14)
> > +#define MMA9553_MASK_CONF_SLP_DBCNTM		BIT(13)
> > +#define MMA9553_MASK_CONF_STEPLEN		GENMASK(7, 0)
> > +
> > +#define MMA9553_REG_CONF_HEIGHT_WEIGHT		0x08
> > +#define MMA9553_MASK_CONF_HEIGHT		GENMASK(15, 8)
> > +#define MMA9553_MASK_CONF_WEIGHT		GENMASK(7, 0)
> > +
> > +#define MMA9553_REG_CONF_FILTER			0x0A
> > +#define MMA9553_MASK_CONF_FILTSTEP		GENMASK(15, 8)
> > +#define MMA9553_MASK_CONF_MALE			BIT(7)
> > +#define MMA9553_MASK_CONF_FILTTIME		GENMASK(6, 0)
> > +
> > +#define MMA9553_REG_CONF_SPEED_STEP		0x0C
> > +#define MMA9553_MASK_CONF_SPDPRD		GENMASK(15, 8)
> > +#define MMA9553_MASK_CONF_STEPCOALESCE		GENMASK(7, 0)
> > +
> > +#define MMA9553_REG_CONF_ACTTHD			0x0E
> > +
> > +/* Pedometer status registers (R-only) */
> > +#define MMA9553_REG_STATUS			0x00
> > +#define MMA9553_MASK_STATUS_MRGFL		BIT(15)
> > +#define MMA9553_MASK_STATUS_SUSPCHG		BIT(14)
> > +#define MMA9553_MASK_STATUS_STEPCHG		BIT(13)
> > +#define MMA9553_MASK_STATUS_ACTCHG		BIT(12)
> > +#define MMA9553_MASK_STATUS_SUSP		BIT(11)
> > +#define MMA9553_MASK_STATUS_ACTIVITY		(BIT(10) | BIT(9) | BIT(8))
> 
> This could use a GENMASK.
Cannot figure out why I did not use GENMASK here as well. Will fix it.
> 
> > +#define MMA9553_MASK_STATUS_VERSION		0x00FF
> 
> This is also a typical use-case for GENMASK.
> 
> > +
> > +#define MMA9553_REG_STEPCNT			0x02
> > +#define MMA9553_REG_DISTANCE			0x04
> > +#define MMA9553_REG_SPEED			0x06
> > +#define MMA9553_REG_CALORIES			0x08
> > +#define MMA9553_REG_SLEEPCNT			0x0A
> > +
> > +/* Pedometer events are always mapped to this pin. */
> > +#define MMA9553_DEFAULT_GPIO_PIN	mma9551_gpio6
> > +#define MMA9553_DEFAULT_GPIO_POLARITY	0
> > +
> > +/* Bitnum used for gpio configuration = bit number in high status byte */
> > +#define STATUS_TO_BITNUM(bit)		(ffs(bit) - 9)
> 
> This macro is missing its prefix.
Ok.
> 
> > +
> > +#define MMA9553_DEFAULT_SAMPLE_RATE	30	/* Hz */
> > +
> > +/*
> > + * The internal activity level must be stable for ACTTHD samples before
> > + * ACTIVITY is updated.The ACTIVITY variable contains the current activity
> 
> A whitespace is missing at the end of the first sentence.
> 
> > + * level and is updated every time a step is detected or once a second
> > + * if there are no steps.
> > + */
> > +#define MMA9553_ACTIVITY_THD_TO_SEC(thd) ((thd) / MMA9553_DEFAULT_SAMPLE_RATE)
> > +#define MMA9553_ACTIVITY_SEC_TO_THD(sec) ((sec) * MMA9553_DEFAULT_SAMPLE_RATE)
> > +
> > +/*
> > + * Autonomously suspend pedometer if acceleration vector magnitude
> > + * is near 1g (4096 at 0.244 mg/LSB resolution) for 30 seconds.
> > + */
> > +#define MMA9553_DEFAULT_SLEEPMIN	3688	/* 0,9 g */
> > +#define MMA9553_DEFAULT_SLEEPMAX	4508	/* 1,1 g */
> > +#define MMA9553_DEFAULT_SLEEPTHD	(MMA9553_DEFAULT_SAMPLE_RATE * 30)
> > +
> > +#define MMA9553_CONFIG_RETRIES		2
> > +
> > +/* Status register - activity field  */
> > +enum activity_level {
> > +	ACTIVITY_UNKNOWN,
> > +	ACTIVITY_REST,
> > +	ACTIVITY_WALKING,
> > +	ACTIVITY_JOGGING,
> > +	ACTIVITY_RUNNING,
> > +};
> > +
> > +static struct mma9553_event_info {
> > +	enum iio_chan_type type;
> > +	enum iio_modifier mod;
> > +	enum iio_event_direction dir;
> > +} mma9553_events_info[] = {
> > +	{
> > +		.type = IIO_STEPS,
> > +		.mod = IIO_NO_MOD,
> > +		.dir = IIO_EV_DIR_NONE,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_STILL,
> > +		.dir = IIO_EV_DIR_RISING,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_STILL,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_WALKING,
> > +		.dir = IIO_EV_DIR_RISING,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_WALKING,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_JOGGING,
> > +		.dir = IIO_EV_DIR_RISING,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_JOGGING,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_RUNNING,
> > +		.dir = IIO_EV_DIR_RISING,
> > +	},
> > +	{
> > +		.type = IIO_ACTIVITY,
> > +		.mod = IIO_MOD_RUNNING,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +	},
> > +};
> > +
> > +#define MMA9553_EVENTS_INFO_SIZE ARRAY_SIZE(mma9553_events_info)
> > +
> > +struct mma9553_event {
> > +	struct mma9553_event_info *info;
> > +	bool enabled;
> > +};
> > +
> > +struct mma9553_conf_regs {
> > +	u16 sleepmin;
> > +	u16 sleepmax;
> > +	u16 sleepthd;
> > +	u16 config;
> > +	u16 height_weight;
> > +	u16 filter;
> > +	u16 speed_step;
> > +	u16 actthd;
> > +} __packed;
> > +
> > +struct mma9553_data {
> > +	struct i2c_client *client;
> > +	struct mutex mutex;
> > +	struct mma9553_conf_regs conf;
> > +	struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];
> > +	int num_events;
> 
> num_events could be unsigned.
> 
> > +	u8 gpio_bitnum;
> > +	/*
> > +	 * This is used for all features that depend on step count:
> > +	 * step count, distance, speed, calories.
> > +	 */
> > +	bool stepcnt_enabled;
> > +	u16 stepcnt;
> > +	u8 activity;
> > +	s64 timestamp;
> > +};
> > +
> > +static u8 mma9553_get_bits(u16 val, u16 mask)
> > +{
> > +	return (val & mask) >> (ffs(mask) - 1);
> > +}
> > +
> > +static u16 mma9553_set_bits(u16 current_val, u16 val, u16 mask)
> > +{
> > +	return (current_val & ~mask) | (val << (ffs(mask) - 1));
> > +}
> > +
> > +static enum iio_modifier mma9553_activity_to_mod(enum activity_level activity)
> > +{
> > +	switch (activity) {
> > +	case ACTIVITY_RUNNING:
> > +		return IIO_MOD_RUNNING;
> > +	case ACTIVITY_JOGGING:
> > +		return IIO_MOD_JOGGING;
> > +	case ACTIVITY_WALKING:
> > +		return IIO_MOD_WALKING;
> > +	case ACTIVITY_REST:
> > +		return IIO_MOD_STILL;
> > +	case ACTIVITY_UNKNOWN:
> > +	default:
> > +		return IIO_NO_MOD;
> > +	}
> > +}
> > +
> > +static void mma9553_init_events(struct mma9553_data *data)
> > +{
> > +	int i;
> > +
> > +	data->num_events = MMA9553_EVENTS_INFO_SIZE;
> > +	for (i = 0; i < data->num_events; i++) {
> > +		data->events[i].info = &mma9553_events_info[i];
> > +		data->events[i].enabled = false;
> > +	}
> > +}
> > +
> > +static struct mma9553_event *mma9553_get_event(struct mma9553_data *data,
> > +					       enum iio_chan_type type,
> > +					       enum iio_modifier mod,
> > +					       enum iio_event_direction dir)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < data->num_events; i++)
> > +		if (data->events[i].info->type == type &&
> > +		    data->events[i].info->mod == mod &&
> > +		    data->events[i].info->dir == dir)
> > +			return &data->events[i];
> > +
> > +	return NULL;
> > +}
> > +
> > +static bool mma9553_is_any_event_enabled(struct mma9553_data *data,
> > +					 bool check_type,
> > +					 enum iio_chan_type type)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < data->num_events; i++)
> > +		if ((check_type && data->events[i].info->type == type &&
> > +		     data->events[i].enabled) ||
> > +		     (!check_type && data->events[i].enabled))
> > +			return true;
> > +
> > +	return false;
> > +}
> > +
> > +static int mma9553_set_config(struct mma9553_data *data, u16 reg,
> > +			      u16 *p_reg_val, u16 val, u16 mask)
> > +{
> > +	int ret, retries;
> > +	u16 reg_val, config;
> > +
> > +	reg_val = *p_reg_val;
> > +	if (val == mma9553_get_bits(reg_val, mask))
> > +		return 0;
> > +
> > +	reg_val = mma9553_set_bits(reg_val, val, mask);
> > +	ret = mma9551_write_config_word(data->client, MMA9551_APPID_PEDOMETER,
> > +					reg, reg_val);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev,
> > +			"error writing config register 0x%x\n", reg);
> > +		return ret;
> > +	}
> > +
> > +	*p_reg_val = reg_val;
> > +
> > +	/* Reinitializes the pedometer with current configuration values */
> > +	config = mma9553_set_bits(data->conf.config, 1,
> > +				  MMA9553_MASK_CONF_CONFIG);
> > +
> > +	ret = mma9551_write_config_word(data->client, MMA9551_APPID_PEDOMETER,
> > +					MMA9553_REG_CONF_CONF_STEPLEN, config);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev,
> > +			"error writing config register 0x%x\n",
> > +			MMA9553_REG_CONF_CONF_STEPLEN);
> > +		return ret;
> > +	}
> > +
> > +	retries = MMA9553_CONFIG_RETRIES;
> > +	do {
> > +		mma9551_sleep(MMA9553_DEFAULT_SAMPLE_RATE);
> > +		ret = mma9551_read_config_word(data->client,
> > +					       MMA9551_APPID_PEDOMETER,
> > +					       MMA9553_REG_CONF_CONF_STEPLEN,
> > +					       &config);
> > +		if (ret < 0)
> > +			return ret;
> > +	} while (mma9553_get_bits(config, MMA9553_MASK_CONF_CONFIG) &&
> > +		 --retries > 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
> > +					 u8 *activity, u16 *stepcnt)
> > +{
> > +	u32 status_stepcnt;
> > +	u16 status;
> > +	int ret;
> > +
> > +	ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
> > +					MMA9553_REG_STATUS, sizeof(u32),
> > +					(u16 *) &status_stepcnt);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev,
> > +			"error reading status and stepcnt\n");
> > +		return ret;
> > +	}
> > +
> 
> I think this could be done a bit simpler by using u16 buf[2] instead of
> status_stepcnt (making status obsolete). That's what it would boil down
> to:
> 	*activity = mma9553_get_bits(buf[0], MMA9553_MASK_STATUS_ACTIVITY);
> 	*stepcnt = buf[1];
> 
That does look more simple. Will change it accordingly.
> > +	status = status_stepcnt & MMA9553_MASK_CONF_WORD;
> > +	*activity = mma9553_get_bits(status, MMA9553_MASK_STATUS_ACTIVITY);
> > +	*stepcnt = status_stepcnt >> 16;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mma9553_conf_gpio(struct mma9553_data *data)
> > +{
> > +	u8 bitnum = 0, appid = MMA9551_APPID_PEDOMETER;
> > +	int ret;
> > +	struct mma9553_event *ev_step_detect;
> > +	bool activity_enabled;
> > +
> > +	activity_enabled =
> > +	    mma9553_is_any_event_enabled(data, true, IIO_ACTIVITY);
> > +	ev_step_detect =
> > +	    mma9553_get_event(data, IIO_STEPS, IIO_NO_MOD, IIO_EV_DIR_NONE);
> 
> Usually the parameters are wrapped, if possible. A matter of taste.
> 
Yes, somehow I mixed the two approaches. I will fix all instances.
> > +
> > +	/*
> > +	 * If both step detector and activity are enabled, use the MRGFL bit.
> > +	 * This bit is the logical OR of the SUSPCHG, STEPCHG, and ACTCHG flags.
> > +	 */
> > +	if (activity_enabled && ev_step_detect->enabled)
> > +		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_MRGFL);
> > +	else if (ev_step_detect->enabled)
> > +		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
> > +	else if (activity_enabled)
> > +		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
> > +	else			/* Reset */
> > +		appid = MMA9551_APPID_NONE;
> > +
> > +	if (data->gpio_bitnum == bitnum)
> > +		return 0;
> > +
> > +	/* Save initial values for activity and stepcnt */
> > +	if (activity_enabled || ev_step_detect->enabled)
> > +		mma9553_read_activity_stepcnt(data, &data->activity,
> > +					      &data->stepcnt);
> 
> Check for errors here?
Ok.
> 
> > +
> > +	ret = mma9551_gpio_config(data->client,
> > +				  MMA9553_DEFAULT_GPIO_PIN,
> > +				  appid, bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
> 
> These parameters could be consolidated to fit into two lines.
> 
> > +	if (ret < 0)
> > +		return ret;
> > +	data->gpio_bitnum = bitnum;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mma9553_init(struct mma9553_data *data)
> > +{
> > +	int ret;
> > +
> > +	ret = mma9551_read_version(data->client);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Read all the pedometer configuration registers. This is used as
> > +	 * a device identification command to differentiate the MMA9553L
> > +	 * from the MMA9550L.
> > +	 */
> > +	ret =
> > +	    mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
> > +				      MMA9553_REG_CONF_SLEEPMIN,
> > +				      sizeof(data->conf), (u16 *) &data->conf);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev,
> > +			"device is not MMA9553L: failed to read cfg regs\n");
> 
> I would reduce the error message to the second part.
> 
Will fix.
> > +		return ret;
> > +	}
> > +
> > +
> > +	/* Reset gpio */
> > +	data->gpio_bitnum = -1;
> 
> gpio_bitnum is u8, so you need to find a different solution here.
> 
Good point! I will use a positive invalid value to force reset.
> > +	ret = mma9553_conf_gpio(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = mma9551_app_reset(data->client, MMA9551_RSC_PED);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Init config registers */
> > +	data->conf.sleepmin = MMA9553_DEFAULT_SLEEPMIN;
> > +	data->conf.sleepmax = MMA9553_DEFAULT_SLEEPMAX;
> > +	data->conf.sleepthd = MMA9553_DEFAULT_SLEEPTHD;
> > +	data->conf.config =
> > +	    mma9553_set_bits(data->conf.config, 1, MMA9553_MASK_CONF_CONFIG);
> 
> Again, I would prefer to wrap parameters.
> 
> > +	/*
> > +	 * Clear the activity debounce counter when the activity level changes,
> > +	 * so that the confidence level applies for any activity level.
> > +	 */
> > +	data->conf.config = mma9553_set_bits(data->conf.config, 1,
> > +					     MMA9553_MASK_CONF_ACT_DBCNTM);
> > +	ret =
> > +	    mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> > +				       MMA9553_REG_CONF_SLEEPMIN,
> > +				       sizeof(data->conf), (u16 *) &data->conf);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev,
> > +			"failed to write configuration registers\n");
> > +		return ret;
> > +	}
> > +
> > +	return mma9551_set_device_state(data->client, true);
> > +}
> > +
> > +static int mma9553_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 tmp;
> > +	u8 activity;
> > +	bool powered_on;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		switch (chan->type) {
> > +		case IIO_STEPS:
> > +			/*
> > +			 * The HW only counts steps and other dependent
> > +			 * parameters (speed, distance, calories, activity)
> > +			 * if power is on (from enabling an event or the
> > +			 * step counter */
> 
> The multiline comment should end on a separate line. Also, the parenthesis
> should be closed and the sentence ended with a full stop.
> 
> > +			powered_on =
> > +			    mma9553_is_any_event_enabled(data, false, 0) ||
> > +			    data->stepcnt_enabled;
> > +			if (!powered_on) {
> > +				dev_err(&data->client->dev,
> > +					"No channels enabled\n");
> > +				return -EINVAL;
> > +			}
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9551_read_status_word(data->client,
> > +						       MMA9551_APPID_PEDOMETER,
> > +						       MMA9553_REG_STEPCNT,
> > +						       &tmp);
> > +			mutex_unlock(&data->mutex);
> 
> This block is repeatedly used with small changes only. So, better put it into
> a separate function like this:
> static int mma9553_fancy_name(mma9553_data *data, u16 reg, u16 *tmp)
> 
That would look much better! Will refactor the code. Thanks for the suggestion!
> > +			if (ret < 0)
> > +				return ret;
> > +			*val = tmp;
> > +			return IIO_VAL_INT;
> > +		case IIO_DISTANCE:
> > +			powered_on =
> > +			    mma9553_is_any_event_enabled(data, false, 0) ||
> > +			    data->stepcnt_enabled;
> > +			if (!powered_on) {
> > +				dev_err(&data->client->dev,
> > +					"No channels enabled\n");
> > +				return -EINVAL;
> > +			}
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9551_read_status_word(data->client,
> > +						       MMA9551_APPID_PEDOMETER,
> > +						       MMA9553_REG_DISTANCE,
> > +						       &tmp);
> > +			mutex_unlock(&data->mutex);
> > +			if (ret < 0)
> > +				return ret;
> > +			*val = tmp;
> > +			return IIO_VAL_INT;
> > +		case IIO_ACTIVITY:
> > +			powered_on =
> > +			    mma9553_is_any_event_enabled(data, false, 0) ||
> > +			    data->stepcnt_enabled;
> > +			if (!powered_on) {
> > +				dev_err(&data->client->dev,
> > +					"No channels enabled\n");
> > +				return -EINVAL;
> > +			}
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9551_read_status_word(data->client,
> > +						       MMA9551_APPID_PEDOMETER,
> > +						       MMA9553_REG_STATUS,
> > +						       &tmp);
> > +			mutex_unlock(&data->mutex);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			activity =
> > +			    mma9553_get_bits(tmp, MMA9553_MASK_STATUS_ACTIVITY);
> > +
> > +			/*
> > +			 * The device does not support confidence value levels,
> > +			 * so we will always have 100% for current activity and
> > +			 * 0% for the others.
> > +			 */
> > +			if (chan->channel2 == mma9553_activity_to_mod(activity))
> > +				*val = 100;
> > +			else
> > +				*val = 0;
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_VELOCITY:	/* m/h */
> > +			if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > +				return -EINVAL;
> > +			powered_on =
> > +			    mma9553_is_any_event_enabled(data, false, 0) ||
> > +			    data->stepcnt_enabled;
> > +			if (!powered_on) {
> > +				dev_err(&data->client->dev,
> > +					"No channels enabled\n");
> > +				return -EINVAL;
> > +			}
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9551_read_status_word(data->client,
> > +						       MMA9551_APPID_PEDOMETER,
> > +						       MMA9553_REG_SPEED, &tmp);
> > +			mutex_unlock(&data->mutex);
> > +			if (ret < 0)
> > +				return ret;
> > +			*val = tmp;
> > +			return IIO_VAL_INT;
> > +		case IIO_ENERGY:	/* Cal or kcal */
> > +			powered_on =
> > +			    mma9553_is_any_event_enabled(data, false, 0) ||
> > +			    data->stepcnt_enabled;
> > +			if (!powered_on) {
> > +				dev_err(&data->client->dev,
> > +					"No channels enabled\n");
> > +				return -EINVAL;
> > +			}
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9551_read_status_word(data->client,
> > +						       MMA9551_APPID_PEDOMETER,
> > +						       MMA9553_REG_CALORIES,
> > +						       &tmp);
> > +			mutex_unlock(&data->mutex);
> > +			if (ret < 0)
> > +				return ret;
> > +			*val = tmp;
> > +			return IIO_VAL_INT;
> > +		case IIO_ACCEL:
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9551_read_accel_chan(data->client,
> > +						      chan, val, val2);
> > +			mutex_unlock(&data->mutex);
> > +			return ret;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_VELOCITY:	/* m/h to m/s */
> > +			if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > +				return -EINVAL;
> > +			*val = 0;
> > +			*val2 = 277;	/* 0.000277 */
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		case IIO_ENERGY:	/* Cal or kcal to J */
> > +			*val = 4184;
> > +			return IIO_VAL_INT;
> > +		case IIO_ACCEL:
> > +			return mma9551_read_accel_scale(val, val2);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_ENABLE:
> > +		*val = data->stepcnt_enabled;
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_CALIBHEIGHT:
> > +		tmp = mma9553_get_bits(data->conf.height_weight,
> > +					MMA9553_MASK_CONF_HEIGHT);
> > +		*val = tmp / 100;	/* cm to m */
> > +		*val2 = (tmp % 100) * 10000;
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_CHAN_INFO_CALIBWEIGHT:
> > +		*val = mma9553_get_bits(data->conf.height_weight,
> > +					MMA9553_MASK_CONF_WEIGHT);
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_DEBOUNCE_COUNT:
> > +		switch (chan->type) {
> > +		case IIO_STEPS:
> > +			*val = mma9553_get_bits(data->conf.filter,
> > +						MMA9553_MASK_CONF_FILTSTEP);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_DEBOUNCE_TIME:
> > +		switch (chan->type) {
> > +		case IIO_STEPS:
> > +			*val = mma9553_get_bits(data->conf.filter,
> > +						MMA9553_MASK_CONF_FILTTIME);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		switch (chan->type) {
> > +		case IIO_VELOCITY:
> > +			if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > +				return -EINVAL;
> > +			*val = mma9553_get_bits(data->conf.speed_step,
> > +						MMA9553_MASK_CONF_SPDPRD);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mma9553_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	int ret, tmp;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_ENABLE:
> > +		if (data->stepcnt_enabled == !!val)
> > +			return 0;
> > +		mutex_lock(&data->mutex);
> > +		ret = mma9551_set_power_state(data->client, val);
> > +		if (ret < 0) {
> > +			mutex_unlock(&data->mutex);
> > +			return ret;
> > +		}
> > +		data->stepcnt_enabled = val;
> > +		mutex_unlock(&data->mutex);
> > +		return 0;
> > +	case IIO_CHAN_INFO_CALIBHEIGHT:
> > +		/* m to cm */
> > +		tmp = val * 100 + val2 / 10000;
> > +		if (tmp < 0 || tmp > 255)
> > +			return -EINVAL;
> > +		mutex_lock(&data->mutex);
> > +		ret = mma9553_set_config(data,
> > +					 MMA9553_REG_CONF_HEIGHT_WEIGHT,
> > +					 &data->conf.height_weight,
> > +					 tmp, MMA9553_MASK_CONF_HEIGHT);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	case IIO_CHAN_INFO_CALIBWEIGHT:
> > +		if (val < 0 || val > 255)
> > +			return -EINVAL;
> > +		mutex_lock(&data->mutex);
> > +		ret = mma9553_set_config(data,
> > +					 MMA9553_REG_CONF_HEIGHT_WEIGHT,
> > +					 &data->conf.height_weight,
> > +					 val, MMA9553_MASK_CONF_WEIGHT);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	case IIO_CHAN_INFO_DEBOUNCE_COUNT:
> > +		switch (chan->type) {
> > +		case IIO_STEPS:
> > +			/*
> > +			 * Set to 0 to disable step filtering. If the value
> > +			 * specified is greater than 6, then 6 will be used.
> > +			 */
> > +			if (val < 0)
> > +				return -EINVAL;
> > +			if (val > 6)
> > +				val = 6;
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> > +						 &data->conf.filter, val,
> > +						 MMA9553_MASK_CONF_FILTSTEP);
> > +			mutex_unlock(&data->mutex);
> > +			return ret;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_DEBOUNCE_TIME:
> > +		switch (chan->type) {
> > +		case IIO_STEPS:
> > +			if (val < 0 || val > 127)
> > +				return -EINVAL;
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> > +						 &data->conf.filter, val,
> > +						 MMA9553_MASK_CONF_FILTTIME);
> > +			mutex_unlock(&data->mutex);
> > +			return ret;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		switch (chan->type) {
> > +		case IIO_VELOCITY:
> > +			if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > +				return -EINVAL;
> > +			/*
> > +			 * If set to a value greater than 5, then 5 will be
> > +			 * used. Warning: Do not set SPDPRD to 0 or 1 as
> > +			 * this may cause undesirable behavior.
> > +			 */
> > +			if (val < 2)
> > +				return -EINVAL;
> > +			if (val > 5)
> > +				val = 5;
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9553_set_config(data,
> > +						 MMA9553_REG_CONF_SPEED_STEP,
> > +						 &data->conf.speed_step, val,
> > +						 MMA9553_MASK_CONF_SPDPRD);
> > +			mutex_unlock(&data->mutex);
> > +			return ret;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mma9553_read_event_config(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir)
> > +{
> > +
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	struct mma9553_event *event;
> > +
> > +	event = mma9553_get_event(data, chan->type, chan->channel2, dir);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	return event->enabled;
> > +}
> > +
> > +static int mma9553_write_event_config(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      enum iio_event_type type,
> > +				      enum iio_event_direction dir, int state)
> > +{
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	struct mma9553_event *event;
> > +	int ret;
> > +
> > +	event = mma9553_get_event(data, chan->type, chan->channel2, dir);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	if (event->enabled == state)
> > +		return 0;
> > +
> > +	mutex_lock(&data->mutex);
> > +
> > +	ret = mma9551_set_power_state(data->client, state);
> > +	if (ret < 0)
> > +		goto err_out;
> > +	event->enabled = state;
> > +
> > +	ret = mma9553_conf_gpio(data);
> > +	if (ret < 0)
> > +		goto err_conf_gpio;
> > +
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> 
> Return 0 as indication of success?
> 
Yes.
> > +
> > +err_conf_gpio:
> > +	if (state) {
> > +		event->enabled = false;
> > +		mma9551_set_power_state(data->client, false);
> > +	}
> > +err_out:
> > +	mutex_unlock(&data->mutex);
> > +	return ret;
> > +}
> > +
> > +static int mma9553_read_event_value(struct iio_dev *indio_dev,
> > +				    const struct iio_chan_spec *chan,
> > +				    enum iio_event_type type,
> > +				    enum iio_event_direction dir,
> > +				    enum iio_event_info info,
> > +				    int *val, int *val2)
> > +{
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +
> > +	*val2 = 0;
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		switch (chan->type) {
> > +		case IIO_STEPS:
> > +			*val = mma9553_get_bits(data->conf.speed_step,
> > +						MMA9553_MASK_CONF_STEPCOALESCE);
> > +			return IIO_VAL_INT;
> > +		case IIO_ACTIVITY:
> > +			/*
> > +			 * The device does not support confidence value levels.
> > +			 * We set an average of 50%.
> > +			 */
> > +			*val = 50;
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_EV_INFO_PERIOD:
> > +		switch (chan->type) {
> > +		case IIO_ACTIVITY:
> > +			*val = MMA9553_ACTIVITY_THD_TO_SEC(data->conf.actthd);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mma9553_write_event_value(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir,
> > +				     enum iio_event_info info,
> > +				     int val, int val2)
> > +{
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		switch (chan->type) {
> > +		case IIO_STEPS:
> > +			if (val < 0 || val > 255)
> > +				return -EINVAL;
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9553_set_config(data,
> > +						MMA9553_REG_CONF_SPEED_STEP,
> > +						&data->conf.speed_step, val,
> > +						MMA9553_MASK_CONF_STEPCOALESCE);
> > +			mutex_unlock(&data->mutex);
> > +			return ret;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_EV_INFO_PERIOD:
> > +		switch (chan->type) {
> > +		case IIO_ACTIVITY:
> 
> Have a boundary check of val here, as well?
> 
Seems this is the only value missing boundary check. Will fix this.
> > +			mutex_lock(&data->mutex);
> > +			ret = mma9553_set_config(data, MMA9553_REG_CONF_ACTTHD,
> > +						 &data->conf.actthd,
> > +						 MMA9553_ACTIVITY_SEC_TO_THD
> > +						 (val), MMA9553_MASK_CONF_WORD);
> > +			mutex_unlock(&data->mutex);
> > +			return ret;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mma9553_get_calibgender_mode(struct iio_dev *indio_dev,
> > +					const struct iio_chan_spec *chan)
> > +{
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	u8 gender;
> > +
> > +	gender = mma9553_get_bits(data->conf.filter, MMA9553_MASK_CONF_MALE);
> > +	/*
> > +	 * HW expects 0 for female and 1 for male,
> > +	 * while iio index is 0 for male and 1 for female
> 
> End this sentence with a full stop?
> 
> > +	 */
> > +	return !gender;
> > +}
> > +
> > +static int mma9553_set_calibgender_mode(struct iio_dev *indio_dev,
> > +					const struct iio_chan_spec *chan,
> > +					unsigned int mode)
> > +{
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	u8 gender = !mode;
> > +	int ret;
> > +
> > +	if ((mode != 0) && (mode != 1))
> > +		return -EINVAL;
> > +	mutex_lock(&data->mutex);
> > +	ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> > +				 &data->conf.filter, gender,
> > +				 MMA9553_MASK_CONF_MALE);
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_event_spec mma9553_step_event = {
> > +	.type = IIO_EV_TYPE_CHANGE,
> > +	.dir = IIO_EV_DIR_NONE,
> > +	.mask_separate = BIT(IIO_EV_INFO_ENABLE) | BIT(IIO_EV_INFO_VALUE),
> > +};
> > +
> > +static const struct iio_event_spec mma9553_activity_events[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> > +				 BIT(IIO_EV_INFO_VALUE) |
> > +				 BIT(IIO_EV_INFO_PERIOD),
> > +	 },
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> > +				 BIT(IIO_EV_INFO_VALUE) |
> > +				 BIT(IIO_EV_INFO_PERIOD),
> > +	},
> > +};
> > +
> > +static const char * const calibgender_modes[] = { "male", "female" };
> 
> Shouldn't this be prefixed?
> 
It should. Will fix.
> > +
> > +static const struct iio_enum mma9553_calibgender_enum = {
> > +	.items = calibgender_modes,
> > +	.num_items = ARRAY_SIZE(calibgender_modes),
> > +	.get = mma9553_get_calibgender_mode,
> > +	.set = mma9553_set_calibgender_mode,
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info mma9553_ext_info[] = {
> > +	IIO_ENUM("calibgender", IIO_SHARED_BY_TYPE, &mma9553_calibgender_enum),
> > +	IIO_ENUM_AVAILABLE("calibgender", &mma9553_calibgender_enum),
> > +	{},
> > +};
> > +
> > +#define MMA9553_PEDOMETER_CHANNEL(_type, _mask) {		\
> > +	.type = _type,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE)      |	\
> > +			      BIT(IIO_CHAN_INFO_CALIBHEIGHT) |	\
> > +			      _mask,				\
> > +	.ext_info = mma9553_ext_info,				\
> > +}
> > +
> > +#define MMA9553_ACTIVITY_CHANNEL(_chan2) {				\
> > +	.type = IIO_ACTIVITY,						\
> > +	.modified = 1,							\
> > +	.channel2 = _chan2,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),		\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT),	\
> > +	.event_spec = mma9553_activity_events,				\
> > +	.num_event_specs = ARRAY_SIZE(mma9553_activity_events),		\
> > +	.ext_info = mma9553_ext_info,					\
> > +}
> > +
> > +static const struct iio_chan_spec mma9553_channels[] = {
> > +	MMA9551_ACCEL_CHANNEL(IIO_MOD_X),
> > +	MMA9551_ACCEL_CHANNEL(IIO_MOD_Y),
> > +	MMA9551_ACCEL_CHANNEL(IIO_MOD_Z),
> > +
> > +	{
> > +		.type = IIO_STEPS,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > +				     BIT(IIO_CHAN_INFO_ENABLE) |
> > +				     BIT(IIO_CHAN_INFO_DEBOUNCE_COUNT) |
> > +				     BIT(IIO_CHAN_INFO_DEBOUNCE_TIME),
> > +		.event_spec = &mma9553_step_event,
> > +		.num_event_specs = 1,
> > +	},
> > +
> > +	MMA9553_PEDOMETER_CHANNEL(IIO_DISTANCE, BIT(IIO_CHAN_INFO_PROCESSED)),
> > +	{
> > +		.type = IIO_VELOCITY,
> > +		.modified = 1,
> > +		.channel2 = IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE) |
> > +				      BIT(IIO_CHAN_INFO_INT_TIME) |
> > +				      BIT(IIO_CHAN_INFO_ENABLE),
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT),
> > +		.ext_info = mma9553_ext_info,
> > +	},
> > +	MMA9553_PEDOMETER_CHANNEL(IIO_ENERGY, BIT(IIO_CHAN_INFO_RAW) |
> > +				  BIT(IIO_CHAN_INFO_SCALE) |
> > +				  BIT(IIO_CHAN_INFO_CALIBWEIGHT)),
> > +
> > +	MMA9553_ACTIVITY_CHANNEL(IIO_MOD_RUNNING),
> > +	MMA9553_ACTIVITY_CHANNEL(IIO_MOD_JOGGING),
> > +	MMA9553_ACTIVITY_CHANNEL(IIO_MOD_WALKING),
> > +	MMA9553_ACTIVITY_CHANNEL(IIO_MOD_STILL),
> > +};
> > +
> > +static const struct iio_info mma9553_info = {
> > +	.driver_module = THIS_MODULE,
> > +	.read_raw = mma9553_read_raw,
> > +	.write_raw = mma9553_write_raw,
> > +	.read_event_config = mma9553_read_event_config,
> > +	.write_event_config = mma9553_write_event_config,
> > +	.read_event_value = mma9553_read_event_value,
> > +	.write_event_value = mma9553_write_event_value,
> > +};
> > +
> > +static irqreturn_t mma9553_irq_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +
> > +	data->timestamp = iio_get_time_ns();
> > +	/*
> > +	 * Since we only configure the interrupt pin when an
> > +	 * event is enabled, we are sure we have at least
> > +	 * one event enabled at this point.
> > +	 */
> > +	return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t mma9553_event_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	u16 stepcnt;
> > +	u8 activity;
> > +	struct mma9553_event *ev_activity, *ev_prev_activity, *ev_step_detect;
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +	ret = mma9553_read_activity_stepcnt(data, &activity, &stepcnt);
> > +	if (ret < 0) {
> > +		mutex_unlock(&data->mutex);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	ev_prev_activity =
> > +	    mma9553_get_event(data, IIO_ACTIVITY,
> > +			      mma9553_activity_to_mod(data->activity),
> > +			      IIO_EV_DIR_FALLING);
> > +	ev_activity =
> > +	    mma9553_get_event(data, IIO_ACTIVITY,
> > +			      mma9553_activity_to_mod(activity),
> > +			      IIO_EV_DIR_RISING);
> > +	ev_step_detect =
> > +	    mma9553_get_event(data, IIO_STEPS, IIO_NO_MOD, IIO_EV_DIR_NONE);
> > +
> > +	if (ev_step_detect->enabled && (stepcnt != data->stepcnt)) {
> > +		data->stepcnt = stepcnt;
> > +		iio_push_event(indio_dev,
> > +			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > +			       IIO_EV_DIR_NONE, IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > +			       data->timestamp);
> > +	}
> > +
> > +	if (activity != data->activity) {
> > +		data->activity = activity;
> > +		/* ev_activity can be NULL if activity == ACTIVITY_UNKNOWN */
> > +		if (ev_prev_activity && ev_prev_activity->enabled)
> > +			iio_push_event(indio_dev,
> > +				       IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> > +				       ev_prev_activity->info->mod,
> > +				       IIO_EV_DIR_FALLING,
> > +				       IIO_EV_TYPE_THRESH, 0, 0, 0),
> > +				       data->timestamp);
> > +
> > +		if (ev_activity && ev_activity->enabled)
> > +			iio_push_event(indio_dev,
> > +				       IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> > +				       ev_activity->info->mod,
> > +				       IIO_EV_DIR_RISING,
> > +				       IIO_EV_TYPE_THRESH, 0, 0, 0),
> > +				       data->timestamp);
> > +	}
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mma9553_gpio_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev;
> > +	struct gpio_desc *gpio;
> > +	int ret;
> > +
> > +	if (!client)
> > +		return -EINVAL;
> > +
> > +	dev = &client->dev;
> > +
> > +	/* data ready gpio interrupt pin */
> 
> Better use upper case for abreviations like GPIO and ACPI here and in
> the messages below.
> 
Will fix all instances.
> > +	gpio = devm_gpiod_get_index(dev, MMA9553_GPIO_NAME, 0);
> > +	if (IS_ERR(gpio)) {
> > +		dev_err(dev, "acpi gpio get index failed\n");
> > +		return PTR_ERR(gpio);
> > +	}
> > +
> > +	ret = gpiod_direction_input(gpio);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = gpiod_to_irq(gpio);
> > +
> > +	dev_dbg(dev, "gpio resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static const char *mma9553_match_acpi_device(struct device *dev)
> > +{
> > +	const struct acpi_device_id *id;
> > +
> > +	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> > +	if (!id)
> > +		return NULL;
> > +
> > +	return dev_name(dev);
> > +}
> > +
> > +static int mma9553_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct mma9553_data *data;
> > +	struct iio_dev *indio_dev;
> > +	const char *name = NULL;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	data->client = client;
> > +
> > +	if (id)
> > +		name = id->name;
> > +	else if (ACPI_HANDLE(&client->dev))
> > +		name = mma9553_match_acpi_device(&client->dev);
> > +	else
> > +		return -ENOSYS;
> > +
> > +	mutex_init(&data->mutex);
> > +	mma9553_init_events(data);
> > +
> > +	ret = mma9553_init(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->channels = mma9553_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(mma9553_channels);
> > +	indio_dev->name = name;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &mma9553_info;
> > +
> > +	if (client->irq < 0)
> > +		client->irq = mma9553_gpio_probe(client);
> > +
> > +	if (client->irq >= 0) {
> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +						mma9553_irq_handler,
> > +						mma9553_event_handler,
> > +						IRQF_TRIGGER_RISING,
> > +						MMA9553_IRQ_NAME, indio_dev);
> > +		if (ret < 0) {
> > +			dev_err(&client->dev, "request irq %d failed\n",
> > +				client->irq);
> > +			goto out_poweroff;
> > +		}
> > +
> > +	}
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "unable to register iio device\n");
> > +		goto out_poweroff;
> > +	}
> > +
> > +	ret = pm_runtime_set_active(&client->dev);
> > +	if (ret < 0)
> > +		goto out_iio_unregister;
> > +
> > +	pm_runtime_enable(&client->dev);
> > +	pm_runtime_set_autosuspend_delay(&client->dev,
> > +					 MMA9551_AUTO_SUSPEND_DELAY_MS);
> > +	pm_runtime_use_autosuspend(&client->dev);
> > +
> > +	dev_dbg(&indio_dev->dev, "Registered device %s\n", name);
> > +
> > +	return 0;
> > +
> > +out_iio_unregister:
> > +	iio_device_unregister(indio_dev);
> > +out_poweroff:
> > +	mma9551_set_device_state(client, false);
> > +	return ret;
> > +}
> > +
> > +static int mma9553_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +
> > +	pm_runtime_disable(&client->dev);
> > +	pm_runtime_set_suspended(&client->dev);
> > +	pm_runtime_put_noidle(&client->dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	mutex_lock(&data->mutex);
> > +	mma9551_set_device_state(data->client, false);
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int mma9553_runtime_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +	ret = mma9551_set_device_state(data->client, false);
> > +	mutex_unlock(&data->mutex);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "powering off device failed\n");
> > +		return -EAGAIN;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mma9553_runtime_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = mma9551_set_device_state(data->client, true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mma9551_sleep(MMA9553_DEFAULT_SAMPLE_RATE);
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mma9553_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +	ret = mma9551_set_device_state(data->client, false);
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mma9553_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct mma9553_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +	ret = mma9551_set_device_state(data->client, true);
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops mma9553_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume)
> > +	SET_RUNTIME_PM_OPS(mma9553_runtime_suspend,
> > +			   mma9553_runtime_resume, NULL)
> > +};
> > +
> > +static const struct acpi_device_id mma9553_acpi_match[] = {
> > +	{"MMA9553", 0},
> > +	{},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, mma9553_acpi_match);
> > +
> > +static const struct i2c_device_id mma9553_id[] = {
> > +	{"mma9553", 0},
> > +	{},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, mma9553_id);
> > +
> > +static struct i2c_driver mma9553_driver = {
> > +	.driver = {
> > +		   .name = MMA9553_DRV_NAME,
> > +		   .acpi_match_table = ACPI_PTR(mma9553_acpi_match),
> > +		   .pm = &mma9553_pm_ops,
> > +		   },
> > +	.probe = mma9553_probe,
> > +	.remove = mma9553_remove,
> > +	.id_table = mma9553_id,
> > +};
> > +
> > +module_i2c_driver(mma9553_driver);
> > +
> > +MODULE_AUTHOR("Irina Tirdea <irina.tirdea@xxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MMA9553L pedometer platform driver");
> > -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to
> majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux