[PATCH 0/8] Fixes for the mma9553 driver

[PATCH 0/8] Fixes for the mma9553 driver

[Irina Tirdea]

This is a set of follow-up patches for the mma9553 driver that fixes
some issues as suggested by Hartmut.

I have also added the enable channel for activity as suggested by Daniel.

Irina Tirdea (8):
  iio: accel: mma9553: coding style fixes
  iio: accel: mma9553: use unsigned counters
  iio: accel: mma9553: fix gpio bitnum init value
  iio: accel: mma9553: refactor mma9553_read_activity_stepcnt
  iio: accel: mma9553: refactor mma9553_read_raw
  iio: accel: mma9553: check input value for activity period
  iio: accel: mma9551_core: prevent buffer overrun
  iio: accel: mma9553: add enable channel for activity

 drivers/iio/accel/mma9551_core.c |  58 ++++++----
 drivers/iio/accel/mma9551_core.h |   2 +-
 drivers/iio/accel/mma9553.c      | 227 +++++++++++++++++----------------------
 3 files changed, 136 insertions(+), 151 deletions(-)

-- 
1.9.1

[PATCH 1/8] iio: accel: mma9553: coding style fixes

[Irina Tirdea]

Fix a couple of coding style issues including:
  * issues reported by checkpatch.pl --strict
  * add mma9553_ prefix to all local functions/declarations
  * fix documentation
  * use GENMASK instead of direct masks
  * fix code alignment

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/mma9551_core.c |  8 ++--
 drivers/iio/accel/mma9551_core.h |  2 +-
 drivers/iio/accel/mma9553.c      | 98 ++++++++++++++++++++--------------------
 3 files changed, 55 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index 7f55a6d..54b3ae6 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -374,7 +374,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
  * @app_id:	Application ID
  * @reg:	Application register
  * @len:	Length of array to read in bytes
- * @val:	Array of words to read
+ * @buf:	Array of words to read
  *
  * Read multiple configuration registers (word-sized registers).
  *
@@ -409,7 +409,7 @@ EXPORT_SYMBOL(mma9551_read_config_words);
  * @app_id:	Application ID
  * @reg:	Application register
  * @len:	Length of array to read in bytes
- * @val:	Array of words to read
+ * @buf:	Array of words to read
  *
  * Read multiple status registers (word-sized registers).
  *
@@ -444,7 +444,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
  * @app_id:	Application ID
  * @reg:	Application register
  * @len:	Length of array to write in bytes
- * @val:	Array of words to write
+ * @buf:	Array of words to write
  *
  * Write multiple configuration registers (word-sized registers).
  *
@@ -785,7 +785,7 @@ EXPORT_SYMBOL(mma9551_read_accel_scale);
  */
 int mma9551_app_reset(struct i2c_client *client, u32 app_mask)
 {
-	return mma9551_write_config_byte(client, MMA9551_APPID_RCS,
+	return mma9551_write_config_byte(client, MMA9551_APPID_RSC,
 					 MMA9551_RSC_RESET +
 					 MMA9551_RSC_OFFSET(app_mask),
 					 MMA9551_RSC_VAL(app_mask));
diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
index edaa56b..79939e4 100644
--- a/drivers/iio/accel/mma9551_core.h
+++ b/drivers/iio/accel/mma9551_core.h
@@ -22,7 +22,7 @@
 #define MMA9551_APPID_TILT		0x0B
 #define MMA9551_APPID_SLEEP_WAKE	0x12
 #define MMA9551_APPID_PEDOMETER	        0x15
-#define MMA9551_APPID_RCS		0x17
+#define MMA9551_APPID_RSC		0x17
 #define MMA9551_APPID_NONE		0xff
 
 /* Reset/Suspend/Clear application app masks */
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 2df1af7..d095f81 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -54,6 +54,7 @@
 #define MMA9553_MASK_CONF_STEPCOALESCE		GENMASK(7, 0)
 
 #define MMA9553_REG_CONF_ACTTHD			0x0E
+#define MMA9553_MAX_ACTTHD			GENMASK(15, 0)
 
 /* Pedometer status registers (R-only) */
 #define MMA9553_REG_STATUS			0x00
@@ -62,8 +63,8 @@
 #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))
-#define MMA9553_MASK_STATUS_VERSION		0x00FF
+#define MMA9553_MASK_STATUS_ACTIVITY		GENMASK(10, 8)
+#define MMA9553_MASK_STATUS_VERSION		GENMASK(7, 0)
 
 #define MMA9553_REG_STEPCNT			0x02
 #define MMA9553_REG_DISTANCE			0x04
@@ -75,14 +76,14 @@
 #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)
+/* Bitnum used for GPIO configuration = bit number in high status byte */
+#define MMA9553_STATUS_TO_BITNUM(bit)	(ffs(bit) - 9)
 
 #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
+ * ACTIVITY is updated. The ACTIVITY variable contains the current activity
  * level and is updated every time a step is detected or once a second
  * if there are no steps.
  */
@@ -343,21 +344,21 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
 	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);
+	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);
 
 	/*
 	 * 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);
+		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_MRGFL);
 	else if (ev_step_detect->enabled)
-		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
+		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
 	else if (activity_enabled)
-		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
+		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
 	else			/* Reset */
 		appid = MMA9551_APPID_NONE;
 
@@ -365,13 +366,15 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
 		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);
+	if (activity_enabled || ev_step_detect->enabled) {
+		ret = mma9553_read_activity_stepcnt(data, &data->activity,
+						    &data->stepcnt);
+		if (ret < 0)
+			return ret;
+	}
 
-	ret = mma9551_gpio_config(data->client,
-				  MMA9553_DEFAULT_GPIO_PIN,
-				  appid, bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
+	ret = mma9551_gpio_config(data->client, MMA9553_DEFAULT_GPIO_PIN, appid,
+				  bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
 	if (ret < 0)
 		return ret;
 	data->gpio_bitnum = bitnum;
@@ -392,19 +395,19 @@ static int mma9553_init(struct mma9553_data *data)
 	 * 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);
+	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");
+			"failed to read configuration registers\n");
 		return ret;
 	}
 
 
-	/* Reset gpio */
 	data->gpio_bitnum = -1;
+	/* Reset GPIO */
 	ret = mma9553_conf_gpio(data);
 	if (ret < 0)
 		return ret;
@@ -417,18 +420,18 @@ static int mma9553_init(struct mma9553_data *data)
 	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);
+	data->conf.config = mma9553_set_bits(data->conf.config, 1,
+					     MMA9553_MASK_CONF_CONFIG);
 	/*
 	 * 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);
+	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");
@@ -791,7 +794,7 @@ static int mma9553_write_event_config(struct iio_dev *indio_dev,
 
 	mutex_unlock(&data->mutex);
 
-	return ret;
+	return 0;
 
 err_conf_gpio:
 	if (state) {
@@ -896,7 +899,7 @@ static int mma9553_get_calibgender_mode(struct iio_dev *indio_dev,
 	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
+	 * while iio index is 0 for male and 1 for female.
 	 */
 	return !gender;
 }
@@ -943,11 +946,11 @@ static const struct iio_event_spec mma9553_activity_events[] = {
 	},
 };
 
-static const char * const calibgender_modes[] = { "male", "female" };
+static const char * const mma9553_calibgender_modes[] = { "male", "female" };
 
 static const struct iio_enum mma9553_calibgender_enum = {
-	.items = calibgender_modes,
-	.num_items = ARRAY_SIZE(calibgender_modes),
+	.items = mma9553_calibgender_modes,
+	.num_items = ARRAY_SIZE(mma9553_calibgender_modes),
 	.get = mma9553_get_calibgender_mode,
 	.set = mma9553_set_calibgender_mode,
 };
@@ -1054,16 +1057,15 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
 		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);
+	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;
@@ -1108,16 +1110,16 @@ static int mma9553_gpio_probe(struct i2c_client *client)
 
 	dev = &client->dev;
 
-	/* data ready gpio interrupt pin */
+	/* data ready GPIO interrupt pin */
 	gpio = devm_gpiod_get_index(dev, MMA9553_GPIO_NAME, 0, GPIOD_IN);
 	if (IS_ERR(gpio)) {
-		dev_err(dev, "acpi gpio get index failed\n");
+		dev_err(dev, "ACPI GPIO get index failed\n");
 		return PTR_ERR(gpio);
 	}
 
 	ret = gpiod_to_irq(gpio);
 
-	dev_dbg(dev, "gpio resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
+	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH 1/8] iio: accel: mma9553: coding style fixes

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> Fix a couple of coding style issues including:
>   * issues reported by checkpatch.pl --strict
>   * add mma9553_ prefix to all local functions/declarations
>   * fix documentation
>   * use GENMASK instead of direct masks
>   * fix code alignment
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
Please break this up into smaller focused patches as it's a mixture of
various different things.

First high urgency patches:

Typo fixes
Wrong docs fixes

Then lower urgency (nice to have ones)

Prefixing naming
GENMASK
Aligment

So I'd say at least 5 short patches.
> ---
>  drivers/iio/accel/mma9551_core.c |  8 ++--
>  drivers/iio/accel/mma9551_core.h |  2 +-
>  drivers/iio/accel/mma9553.c      | 98 ++++++++++++++++++++--------------------
>  3 files changed, 55 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 7f55a6d..54b3ae6 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -374,7 +374,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
>   * @app_id:	Application ID
>   * @reg:	Application register
>   * @len:	Length of array to read in bytes
> - * @val:	Array of words to read
> + * @buf:	Array of words to read
wrong docs aren't a style fix and probably want to go in faster so
please break these out into a separate patch.
>   *
>   * Read multiple configuration registers (word-sized registers).
>   *
> @@ -409,7 +409,7 @@ EXPORT_SYMBOL(mma9551_read_config_words);
>   * @app_id:	Application ID
>   * @reg:	Application register
>   * @len:	Length of array to read in bytes
> - * @val:	Array of words to read
> + * @buf:	Array of words to read
>   *
>   * Read multiple status registers (word-sized registers).
>   *
> @@ -444,7 +444,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
>   * @app_id:	Application ID
>   * @reg:	Application register
>   * @len:	Length of array to write in bytes
> - * @val:	Array of words to write
> + * @buf:	Array of words to write
>   *
>   * Write multiple configuration registers (word-sized registers).
>   *
> @@ -785,7 +785,7 @@ EXPORT_SYMBOL(mma9551_read_accel_scale);
>   */
>  int mma9551_app_reset(struct i2c_client *client, u32 app_mask)
>  {
> -	return mma9551_write_config_byte(client, MMA9551_APPID_RCS,
This isn't a style fix.  I'm guessing it is a straight fix for a typo?
Please break this out to it's own patch. More important than style fixes.

> +	return mma9551_write_config_byte(client, MMA9551_APPID_RSC,
>  					 MMA9551_RSC_RESET +
>  					 MMA9551_RSC_OFFSET(app_mask),
>  					 MMA9551_RSC_VAL(app_mask));
> diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
> index edaa56b..79939e4 100644
> --- a/drivers/iio/accel/mma9551_core.h
> +++ b/drivers/iio/accel/mma9551_core.h
> @@ -22,7 +22,7 @@
>  #define MMA9551_APPID_TILT		0x0B
>  #define MMA9551_APPID_SLEEP_WAKE	0x12
>  #define MMA9551_APPID_PEDOMETER	        0x15
> -#define MMA9551_APPID_RCS		0x17
> +#define MMA9551_APPID_RSC		0x17
>  #define MMA9551_APPID_NONE		0xff
>  
>  /* Reset/Suspend/Clear application app masks */
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 2df1af7..d095f81 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -54,6 +54,7 @@
>  #define MMA9553_MASK_CONF_STEPCOALESCE		GENMASK(7, 0)
>  
>  #define MMA9553_REG_CONF_ACTTHD			0x0E
> +#define MMA9553_MAX_ACTTHD			GENMASK(15, 0)
>  
>  /* Pedometer status registers (R-only) */
>  #define MMA9553_REG_STATUS			0x00
> @@ -62,8 +63,8 @@
>  #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))
> -#define MMA9553_MASK_STATUS_VERSION		0x00FF
> +#define MMA9553_MASK_STATUS_ACTIVITY		GENMASK(10, 8)
> +#define MMA9553_MASK_STATUS_VERSION		GENMASK(7, 0)
>  
>  #define MMA9553_REG_STEPCNT			0x02
>  #define MMA9553_REG_DISTANCE			0x04
> @@ -75,14 +76,14 @@
>  #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)
> +/* Bitnum used for GPIO configuration = bit number in high status byte */
> +#define MMA9553_STATUS_TO_BITNUM(bit)	(ffs(bit) - 9)
>  
>  #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
> + * ACTIVITY is updated. The ACTIVITY variable contains the current activity
>   * level and is updated every time a step is detected or once a second
>   * if there are no steps.
>   */
> @@ -343,21 +344,21 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
>  	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);
> +	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);
This is defintlely a matter of taste rather than required, but fair enough
in a style cleanup patch.
>  
>  	/*
>  	 * 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);
> +		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_MRGFL);
>  	else if (ev_step_detect->enabled)
> -		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
> +		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
>  	else if (activity_enabled)
> -		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
> +		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
>  	else			/* Reset */
>  		appid = MMA9551_APPID_NONE;
>  
> @@ -365,13 +366,15 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
>  		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);
> +	if (activity_enabled || ev_step_detect->enabled) {
> +		ret = mma9553_read_activity_stepcnt(data, &data->activity,
> +						    &data->stepcnt);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
> -	ret = mma9551_gpio_config(data->client,
> -				  MMA9553_DEFAULT_GPIO_PIN,
> -				  appid, bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
> +	ret = mma9551_gpio_config(data->client, MMA9553_DEFAULT_GPIO_PIN, appid,
> +				  bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
>  	if (ret < 0)
>  		return ret;
>  	data->gpio_bitnum = bitnum;
> @@ -392,19 +395,19 @@ static int mma9553_init(struct mma9553_data *data)
>  	 * 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);
> +	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");
> +			"failed to read configuration registers\n");
>  		return ret;
>  	}
>  
>  
> -	/* Reset gpio */
>  	data->gpio_bitnum = -1;
> +	/* Reset GPIO */
>  	ret = mma9553_conf_gpio(data);
>  	if (ret < 0)
>  		return ret;
> @@ -417,18 +420,18 @@ static int mma9553_init(struct mma9553_data *data)
>  	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);
> +	data->conf.config = mma9553_set_bits(data->conf.config, 1,
> +					     MMA9553_MASK_CONF_CONFIG);
>  	/*
>  	 * 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);
> +	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");
> @@ -791,7 +794,7 @@ static int mma9553_write_event_config(struct iio_dev *indio_dev,
>  
>  	mutex_unlock(&data->mutex);
>  
> -	return ret;
> +	return 0;
>  
>  err_conf_gpio:
>  	if (state) {
> @@ -896,7 +899,7 @@ static int mma9553_get_calibgender_mode(struct iio_dev *indio_dev,
>  	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
> +	 * while iio index is 0 for male and 1 for female.
>  	 */
>  	return !gender;
>  }
> @@ -943,11 +946,11 @@ static const struct iio_event_spec mma9553_activity_events[] = {
>  	},
>  };
>  
> -static const char * const calibgender_modes[] = { "male", "female" };
> +static const char * const mma9553_calibgender_modes[] = { "male", "female" };
>  
>  static const struct iio_enum mma9553_calibgender_enum = {
> -	.items = calibgender_modes,
> -	.num_items = ARRAY_SIZE(calibgender_modes),
> +	.items = mma9553_calibgender_modes,
> +	.num_items = ARRAY_SIZE(mma9553_calibgender_modes),
>  	.get = mma9553_get_calibgender_mode,
>  	.set = mma9553_set_calibgender_mode,
>  };
> @@ -1054,16 +1057,15 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
>  		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);
> +	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;
> @@ -1108,16 +1110,16 @@ static int mma9553_gpio_probe(struct i2c_client *client)
>  
>  	dev = &client->dev;
>  
> -	/* data ready gpio interrupt pin */
> +	/* data ready GPIO interrupt pin */
>  	gpio = devm_gpiod_get_index(dev, MMA9553_GPIO_NAME, 0, GPIOD_IN);
>  	if (IS_ERR(gpio)) {
> -		dev_err(dev, "acpi gpio get index failed\n");
> +		dev_err(dev, "ACPI GPIO get index failed\n");
>  		return PTR_ERR(gpio);
>  	}
>  
>  	ret = gpiod_to_irq(gpio);
>  
> -	dev_dbg(dev, "gpio resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> +	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
>  
>  	return ret;
>  }
> 

RE: [PATCH 1/8] iio: accel: mma9553: coding style fixes

[Tirdea, Irina]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 09 April, 2015 14:31
> To: Tirdea, Irina; linux-iio@vger.kernel.org; Hartmut Knaack
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/8] iio: accel: mma9553: coding style fixes
> 
> On 08/04/15 15:37, Irina Tirdea wrote:
> > Fix a couple of coding style issues including:
> >   * issues reported by checkpatch.pl --strict
> >   * add mma9553_ prefix to all local functions/declarations
> >   * fix documentation
> >   * use GENMASK instead of direct masks
> >   * fix code alignment
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
> Please break this up into smaller focused patches as it's a mixture of
> various different things.
> 
> First high urgency patches:
> 
> Typo fixes
> Wrong docs fixes
> 
> Then lower urgency (nice to have ones)
> 
> Prefixing naming
> GENMASK
> Aligment
> 
> So I'd say at least 5 short patches.

Sure, I'll break them into smaller patches.
Thanks for pointing out which are high urgency patches and which nice to have - I wasn't sure which is which in this case.

> > ---
> >  drivers/iio/accel/mma9551_core.c |  8 ++--
> >  drivers/iio/accel/mma9551_core.h |  2 +-
> >  drivers/iio/accel/mma9553.c      | 98 ++++++++++++++++++++--------------------
> >  3 files changed, 55 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index 7f55a6d..54b3ae6 100644
> > --- a/drivers/iio/accel/mma9551_core.c
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -374,7 +374,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
> >   * @app_id:	Application ID
> >   * @reg:	Application register
> >   * @len:	Length of array to read in bytes
> > - * @val:	Array of words to read
> > + * @buf:	Array of words to read
> wrong docs aren't a style fix and probably want to go in faster so
> please break these out into a separate patch.
> >   *
> >   * Read multiple configuration registers (word-sized registers).
> >   *
> > @@ -409,7 +409,7 @@ EXPORT_SYMBOL(mma9551_read_config_words);
> >   * @app_id:	Application ID
> >   * @reg:	Application register
> >   * @len:	Length of array to read in bytes
> > - * @val:	Array of words to read
> > + * @buf:	Array of words to read
> >   *
> >   * Read multiple status registers (word-sized registers).
> >   *
> > @@ -444,7 +444,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
> >   * @app_id:	Application ID
> >   * @reg:	Application register
> >   * @len:	Length of array to write in bytes
> > - * @val:	Array of words to write
> > + * @buf:	Array of words to write
> >   *
> >   * Write multiple configuration registers (word-sized registers).
> >   *
> > @@ -785,7 +785,7 @@ EXPORT_SYMBOL(mma9551_read_accel_scale);
> >   */
> >  int mma9551_app_reset(struct i2c_client *client, u32 app_mask)
> >  {
> > -	return mma9551_write_config_byte(client, MMA9551_APPID_RCS,
> This isn't a style fix.  I'm guessing it is a straight fix for a typo?
> Please break this out to it's own patch. More important than style fixes.
> 
> > +	return mma9551_write_config_byte(client, MMA9551_APPID_RSC,
> >  					 MMA9551_RSC_RESET +
> >  					 MMA9551_RSC_OFFSET(app_mask),
> >  					 MMA9551_RSC_VAL(app_mask));
> > diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
> > index edaa56b..79939e4 100644
> > --- a/drivers/iio/accel/mma9551_core.h
> > +++ b/drivers/iio/accel/mma9551_core.h
> > @@ -22,7 +22,7 @@
> >  #define MMA9551_APPID_TILT		0x0B
> >  #define MMA9551_APPID_SLEEP_WAKE	0x12
> >  #define MMA9551_APPID_PEDOMETER	        0x15
> > -#define MMA9551_APPID_RCS		0x17
> > +#define MMA9551_APPID_RSC		0x17
> >  #define MMA9551_APPID_NONE		0xff
> >
> >  /* Reset/Suspend/Clear application app masks */
> > diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> > index 2df1af7..d095f81 100644
> > --- a/drivers/iio/accel/mma9553.c
> > +++ b/drivers/iio/accel/mma9553.c
> > @@ -54,6 +54,7 @@
> >  #define MMA9553_MASK_CONF_STEPCOALESCE		GENMASK(7, 0)
> >
> >  #define MMA9553_REG_CONF_ACTTHD			0x0E
> > +#define MMA9553_MAX_ACTTHD			GENMASK(15, 0)
> >
> >  /* Pedometer status registers (R-only) */
> >  #define MMA9553_REG_STATUS			0x00
> > @@ -62,8 +63,8 @@
> >  #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))
> > -#define MMA9553_MASK_STATUS_VERSION		0x00FF
> > +#define MMA9553_MASK_STATUS_ACTIVITY		GENMASK(10, 8)
> > +#define MMA9553_MASK_STATUS_VERSION		GENMASK(7, 0)
> >
> >  #define MMA9553_REG_STEPCNT			0x02
> >  #define MMA9553_REG_DISTANCE			0x04
> > @@ -75,14 +76,14 @@
> >  #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)
> > +/* Bitnum used for GPIO configuration = bit number in high status byte */
> > +#define MMA9553_STATUS_TO_BITNUM(bit)	(ffs(bit) - 9)
> >
> >  #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
> > + * ACTIVITY is updated. The ACTIVITY variable contains the current activity
> >   * level and is updated every time a step is detected or once a second
> >   * if there are no steps.
> >   */
> > @@ -343,21 +344,21 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
> >  	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);
> > +	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);
> This is defintlely a matter of taste rather than required, but fair enough
> in a style cleanup patch.
> >
> >  	/*
> >  	 * 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);
> > +		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_MRGFL);
> >  	else if (ev_step_detect->enabled)
> > -		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
> > +		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
> >  	else if (activity_enabled)
> > -		bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
> > +		bitnum = MMA9553_STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
> >  	else			/* Reset */
> >  		appid = MMA9551_APPID_NONE;
> >
> > @@ -365,13 +366,15 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
> >  		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);
> > +	if (activity_enabled || ev_step_detect->enabled) {
> > +		ret = mma9553_read_activity_stepcnt(data, &data->activity,
> > +						    &data->stepcnt);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >
> > -	ret = mma9551_gpio_config(data->client,
> > -				  MMA9553_DEFAULT_GPIO_PIN,
> > -				  appid, bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
> > +	ret = mma9551_gpio_config(data->client, MMA9553_DEFAULT_GPIO_PIN, appid,
> > +				  bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
> >  	if (ret < 0)
> >  		return ret;
> >  	data->gpio_bitnum = bitnum;
> > @@ -392,19 +395,19 @@ static int mma9553_init(struct mma9553_data *data)
> >  	 * 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);
> > +	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");
> > +			"failed to read configuration registers\n");
> >  		return ret;
> >  	}
> >
> >
> > -	/* Reset gpio */
> >  	data->gpio_bitnum = -1;
> > +	/* Reset GPIO */
> >  	ret = mma9553_conf_gpio(data);
> >  	if (ret < 0)
> >  		return ret;
> > @@ -417,18 +420,18 @@ static int mma9553_init(struct mma9553_data *data)
> >  	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);
> > +	data->conf.config = mma9553_set_bits(data->conf.config, 1,
> > +					     MMA9553_MASK_CONF_CONFIG);
> >  	/*
> >  	 * 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);
> > +	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");
> > @@ -791,7 +794,7 @@ static int mma9553_write_event_config(struct iio_dev *indio_dev,
> >
> >  	mutex_unlock(&data->mutex);
> >
> > -	return ret;
> > +	return 0;
> >
> >  err_conf_gpio:
> >  	if (state) {
> > @@ -896,7 +899,7 @@ static int mma9553_get_calibgender_mode(struct iio_dev *indio_dev,
> >  	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
> > +	 * while iio index is 0 for male and 1 for female.
> >  	 */
> >  	return !gender;
> >  }
> > @@ -943,11 +946,11 @@ static const struct iio_event_spec mma9553_activity_events[] = {
> >  	},
> >  };
> >
> > -static const char * const calibgender_modes[] = { "male", "female" };
> > +static const char * const mma9553_calibgender_modes[] = { "male", "female" };
> >
> >  static const struct iio_enum mma9553_calibgender_enum = {
> > -	.items = calibgender_modes,
> > -	.num_items = ARRAY_SIZE(calibgender_modes),
> > +	.items = mma9553_calibgender_modes,
> > +	.num_items = ARRAY_SIZE(mma9553_calibgender_modes),
> >  	.get = mma9553_get_calibgender_mode,
> >  	.set = mma9553_set_calibgender_mode,
> >  };
> > @@ -1054,16 +1057,15 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
> >  		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);
> > +	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;
> > @@ -1108,16 +1110,16 @@ static int mma9553_gpio_probe(struct i2c_client *client)
> >
> >  	dev = &client->dev;
> >
> > -	/* data ready gpio interrupt pin */
> > +	/* data ready GPIO interrupt pin */
> >  	gpio = devm_gpiod_get_index(dev, MMA9553_GPIO_NAME, 0, GPIOD_IN);
> >  	if (IS_ERR(gpio)) {
> > -		dev_err(dev, "acpi gpio get index failed\n");
> > +		dev_err(dev, "ACPI GPIO get index failed\n");
> >  		return PTR_ERR(gpio);
> >  	}
> >
> >  	ret = gpiod_to_irq(gpio);
> >
> > -	dev_dbg(dev, "gpio resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> > +	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> >
> >  	return ret;
> >  }
> >

[PATCH 2/8] iio: accel: mma9553: use unsigned counters

[Irina Tirdea]

Use unsigned counters instead of signed when all the
possible values are positive.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/mma9551_core.c | 11 +++++------
 drivers/iio/accel/mma9553.c      |  8 ++++----
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index 54b3ae6..438cfed 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -387,8 +387,8 @@ EXPORT_SYMBOL(mma9551_read_status_word);
 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);
+	int ret;
+	u8 i, len_words = len / sizeof(u16);
 	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
 
 	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
@@ -422,8 +422,8 @@ EXPORT_SYMBOL(mma9551_read_config_words);
 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);
+	int ret;
+	u8 i, len_words = len / sizeof(u16);
 	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
 
 	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
@@ -457,8 +457,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
 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);
+	u8 i, len_words = len / sizeof(u16);
 	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
 
 	for (i = 0; i < len_words; i++)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index d095f81..9cfedb5 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -184,7 +184,7 @@ struct mma9553_data {
 	struct mutex mutex;
 	struct mma9553_conf_regs conf;
 	struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];
-	int num_events;
+	u8 num_events;
 	u8 gpio_bitnum;
 	/*
 	 * This is used for all features that depend on step count:
@@ -225,7 +225,7 @@ static enum iio_modifier mma9553_activity_to_mod(enum activity_level activity)
 
 static void mma9553_init_events(struct mma9553_data *data)
 {
-	int i;
+	u8 i;
 
 	data->num_events = MMA9553_EVENTS_INFO_SIZE;
 	for (i = 0; i < data->num_events; i++) {
@@ -239,7 +239,7 @@ static struct mma9553_event *mma9553_get_event(struct mma9553_data *data,
 					       enum iio_modifier mod,
 					       enum iio_event_direction dir)
 {
-	int i;
+	u8 i;
 
 	for (i = 0; i < data->num_events; i++)
 		if (data->events[i].info->type == type &&
@@ -254,7 +254,7 @@ static bool mma9553_is_any_event_enabled(struct mma9553_data *data,
 					 bool check_type,
 					 enum iio_chan_type type)
 {
-	int i;
+	u8 i;
 
 	for (i = 0; i < data->num_events; i++)
 		if ((check_type && data->events[i].info->type == type &&
-- 
1.9.1

Re: [PATCH 2/8] iio: accel: mma9553: use unsigned counters

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> Use unsigned counters instead of signed when all the
> possible values are positive.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
Does it make sense to carry this through to mma9551_transfer as well?
Can't say I really care about this one. It's nice, but the compiler may
well mess with the types used anyway given it can trivially tell their limits...

Still it does no harm so what the heck.  Certainly not a high
priority change though.
> ---
>  drivers/iio/accel/mma9551_core.c | 11 +++++------
>  drivers/iio/accel/mma9553.c      |  8 ++++----
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 54b3ae6..438cfed 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -387,8 +387,8 @@ EXPORT_SYMBOL(mma9551_read_status_word);
>  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);
> +	int ret;
> +	u8 i, len_words = len / sizeof(u16);
>  	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
>  
>  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> @@ -422,8 +422,8 @@ EXPORT_SYMBOL(mma9551_read_config_words);
>  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);
> +	int ret;
> +	u8 i, len_words = len / sizeof(u16);
>  	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
>  
>  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> @@ -457,8 +457,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
>  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);
> +	u8 i, len_words = len / sizeof(u16);
>  	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
>  
>  	for (i = 0; i < len_words; i++)
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index d095f81..9cfedb5 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -184,7 +184,7 @@ struct mma9553_data {
>  	struct mutex mutex;
>  	struct mma9553_conf_regs conf;
>  	struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];#define MMA9551_APPID_RCS		0x17
+#define MMA9551_APPID_RSC		0x17
> -	int num_events;
> +	u8 num_events;
>  	u8 gpio_bitnum;
>  	/*
>  	 * This is used for all features that depend on step count:
> @@ -225,7 +225,7 @@ static enum iio_modifier mma9553_activity_to_mod(enum activity_level activity)
>  
>  static void mma9553_init_events(struct mma9553_data *data)
>  {
> -	int i;
> +	u8 i;
>  
>  	data->num_events = MMA9553_EVENTS_INFO_SIZE;
>  	for (i = 0; i < data->num_events; i++) {
> @@ -239,7 +239,7 @@ static struct mma9553_event *mma9553_get_event(struct mma9553_data *data,
>  					       enum iio_modifier mod,
>  					       enum iio_event_direction dir)
>  {
> -	int i;
> +	u8 i;
>  
>  	for (i = 0; i < data->num_events; i++)
>  		if (data->events[i].info->type == type &&
> @@ -254,7 +254,7 @@ static bool mma9553_is_any_event_enabled(struct mma9553_data *data,
>  					 bool check_type,
>  					 enum iio_chan_type type)
>  {
> -	int i;
> +	u8 i;
>  
>  	for (i = 0; i < data->num_events; i++)
>  		if ((check_type && data->events[i].info->type == type &&
> 

RE: [PATCH 2/8] iio: accel: mma9553: use unsigned counters

[Tirdea, Irina]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 09 April, 2015 14:37
> To: Tirdea, Irina; linux-iio@vger.kernel.org; Hartmut Knaack
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/8] iio: accel: mma9553: use unsigned counters
> 
> On 08/04/15 15:37, Irina Tirdea wrote:
> > Use unsigned counters instead of signed when all the
> > possible values are positive.
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
> Does it make sense to carry this through to mma9551_transfer as well?
> Can't say I really care about this one. It's nice, but the compiler may
> well mess with the types used anyway given it can trivially tell their limits...
> 
> Still it does no harm so what the heck.  Certainly not a high
> priority change though.
Indeed, I should also change mma9551_transfer signature to reflect the changes for length.
Since this is just a "nice to have" change, will add it at the end of the series.

Thanks,
Irina
> > ---
> >  drivers/iio/accel/mma9551_core.c | 11 +++++------
> >  drivers/iio/accel/mma9553.c      |  8 ++++----
> >  2 files changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index 54b3ae6..438cfed 100644
> > --- a/drivers/iio/accel/mma9551_core.c
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -387,8 +387,8 @@ EXPORT_SYMBOL(mma9551_read_status_word);
> >  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);
> > +	int ret;
> > +	u8 i, len_words = len / sizeof(u16);
> >  	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> >
> >  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> > @@ -422,8 +422,8 @@ EXPORT_SYMBOL(mma9551_read_config_words);
> >  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);
> > +	int ret;
> > +	u8 i, len_words = len / sizeof(u16);
> >  	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> >
> >  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> > @@ -457,8 +457,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
> >  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);
> > +	u8 i, len_words = len / sizeof(u16);
> >  	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> >
> >  	for (i = 0; i < len_words; i++)
> > diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> > index d095f81..9cfedb5 100644
> > --- a/drivers/iio/accel/mma9553.c
> > +++ b/drivers/iio/accel/mma9553.c
> > @@ -184,7 +184,7 @@ struct mma9553_data {
> >  	struct mutex mutex;
> >  	struct mma9553_conf_regs conf;
> >  	struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];#define MMA9551_APPID_RCS		0x17
> +#define MMA9551_APPID_RSC		0x17
> > -	int num_events;
> > +	u8 num_events;
> >  	u8 gpio_bitnum;
> >  	/*
> >  	 * This is used for all features that depend on step count:
> > @@ -225,7 +225,7 @@ static enum iio_modifier mma9553_activity_to_mod(enum activity_level activity)
> >
> >  static void mma9553_init_events(struct mma9553_data *data)
> >  {
> > -	int i;
> > +	u8 i;
> >
> >  	data->num_events = MMA9553_EVENTS_INFO_SIZE;
> >  	for (i = 0; i < data->num_events; i++) {
> > @@ -239,7 +239,7 @@ static struct mma9553_event *mma9553_get_event(struct mma9553_data *data,
> >  					       enum iio_modifier mod,
> >  					       enum iio_event_direction dir)
> >  {
> > -	int i;
> > +	u8 i;
> >
> >  	for (i = 0; i < data->num_events; i++)
> >  		if (data->events[i].info->type == type &&
> > @@ -254,7 +254,7 @@ static bool mma9553_is_any_event_enabled(struct mma9553_data *data,
> >  					 bool check_type,
> >  					 enum iio_chan_type type)
> >  {
> > -	int i;
> > +	u8 i;
> >
> >  	for (i = 0; i < data->num_events; i++)
> >  		if ((check_type && data->events[i].info->type == type &&
> >

[PATCH 3/8] iio: accel: mma9553: fix gpio bitnum init value

[Irina Tirdea]

Initial value of gpio bitnum is set to -1, but
the variable is declared as unsigned.

Use a positive invalid value for initial gpio
bitnum.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/mma9553.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 9cfedb5..d781999 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -78,6 +78,7 @@
 
 /* Bitnum used for GPIO configuration = bit number in high status byte */
 #define MMA9553_STATUS_TO_BITNUM(bit)	(ffs(bit) - 9)
+#define MMA9553_MAX_BITNUM		MMA9553_STATUS_TO_BITNUM(BIT(16))
 
 #define MMA9553_DEFAULT_SAMPLE_RATE	30	/* Hz */
 
@@ -406,8 +407,8 @@ static int mma9553_init(struct mma9553_data *data)
 	}
 
 
-	data->gpio_bitnum = -1;
 	/* Reset GPIO */
+	data->gpio_bitnum = MMA9553_MAX_BITNUM;
 	ret = mma9553_conf_gpio(data);
 	if (ret < 0)
 		return ret;
-- 
1.9.1

Re: [PATCH 3/8] iio: accel: mma9553: fix gpio bitnum init value

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> Initial value of gpio bitnum is set to -1, but
> the variable is declared as unsigned.
> 
> Use a positive invalid value for initial gpio
> bitnum.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
This is fine, but I'm feeling lazy as this isn't going
anywhere until after the merge window now. Will pick it
up once you've done the reordering to pull the fixes to the
top of the series.

J
> ---
>  drivers/iio/accel/mma9553.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 9cfedb5..d781999 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -78,6 +78,7 @@
>  
>  /* Bitnum used for GPIO configuration = bit number in high status byte */
>  #define MMA9553_STATUS_TO_BITNUM(bit)	(ffs(bit) - 9)
> +#define MMA9553_MAX_BITNUM		MMA9553_STATUS_TO_BITNUM(BIT(16))
>  
>  #define MMA9553_DEFAULT_SAMPLE_RATE	30	/* Hz */
>  
> @@ -406,8 +407,8 @@ static int mma9553_init(struct mma9553_data *data)
>  	}
>  
>  
> -	data->gpio_bitnum = -1;
>  	/* Reset GPIO */
> +	data->gpio_bitnum = MMA9553_MAX_BITNUM;
>  	ret = mma9553_conf_gpio(data);
>  	if (ret < 0)
>  		return ret;
> 

[PATCH 4/8] iio: accel: mma9553: refactor mma9553_read_activity_stepcnt

[Irina Tirdea]

Refactor code for simplicity and clarity.

This also fixes an endianness issue with the original code.
When reading multiple registers, the received buffer of
16-bytes words is little endian (status, step count). On
big endian machines, casting them to u32 would result in
reversed order in the buffer (step count, status) leading
to incorrect values for step count and activity.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/mma9553.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index d781999..ee2be3f 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -318,22 +318,19 @@ static int mma9553_set_config(struct mma9553_data *data, u16 reg,
 static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
 					 u8 *activity, u16 *stepcnt)
 {
-	u32 status_stepcnt;
-	u16 status;
+	u16 buf[2];
 	int ret;
 
 	ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
-					MMA9553_REG_STATUS, sizeof(u32),
-					(u16 *) &status_stepcnt);
+					MMA9553_REG_STATUS, sizeof(u32), buf);
 	if (ret < 0) {
 		dev_err(&data->client->dev,
 			"error reading status and stepcnt\n");
 		return ret;
 	}
 
-	status = status_stepcnt & MMA9553_MASK_CONF_WORD;
-	*activity = mma9553_get_bits(status, MMA9553_MASK_STATUS_ACTIVITY);
-	*stepcnt = status_stepcnt >> 16;
+	*activity = mma9553_get_bits(buf[0], MMA9553_MASK_STATUS_ACTIVITY);
+	*stepcnt = buf[1];
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 4/8] iio: accel: mma9553: refactor mma9553_read_activity_stepcnt

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> Refactor code for simplicity and clarity.
> 
> This also fixes an endianness issue with the original code.
> When reading multiple registers, the received buffer of
> 16-bytes words is little endian (status, step count). On
> big endian machines, casting them to u32 would result in
> reversed order in the buffer (step count, status) leading
> to incorrect values for step count and activity.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
Sensible fix, again fill pickup from reordered series.
Thanks,
> ---
>  drivers/iio/accel/mma9553.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index d781999..ee2be3f 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -318,22 +318,19 @@ static int mma9553_set_config(struct mma9553_data *data, u16 reg,
>  static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
>  					 u8 *activity, u16 *stepcnt)
>  {
> -	u32 status_stepcnt;
> -	u16 status;
> +	u16 buf[2];
>  	int ret;
>  
>  	ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
> -					MMA9553_REG_STATUS, sizeof(u32),
> -					(u16 *) &status_stepcnt);
> +					MMA9553_REG_STATUS, sizeof(u32), buf);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev,
>  			"error reading status and stepcnt\n");
>  		return ret;
>  	}
>  
> -	status = status_stepcnt & MMA9553_MASK_CONF_WORD;
> -	*activity = mma9553_get_bits(status, MMA9553_MASK_STATUS_ACTIVITY);
> -	*stepcnt = status_stepcnt >> 16;
> +	*activity = mma9553_get_bits(buf[0], MMA9553_MASK_STATUS_ACTIVITY);
> +	*stepcnt = buf[1];
>  
>  	return 0;
>  }
> 

[PATCH 5/8] iio: accel: mma9553: refactor mma9553_read_raw

[Irina Tirdea]

Refactor code for simplicity and clarity.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/mma9553.c | 100 +++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index ee2be3f..57868fb 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -439,6 +439,32 @@ static int mma9553_init(struct mma9553_data *data)
 	return mma9551_set_device_state(data->client, true);
 }
 
+static int mma9553_read_status_word(struct mma9553_data *data, u16 reg,
+				    u16 *tmp)
+{
+	bool powered_on;
+	int ret;
+
+	/*
+	 * 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).
+	 */
+	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,
+				       reg, tmp);
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
 static int mma9553_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -447,69 +473,30 @@ static int mma9553_read_raw(struct iio_dev *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 */
-			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,
+			ret = mma9553_read_status_word(data,
 						       MMA9553_REG_STEPCNT,
 						       &tmp);
-			mutex_unlock(&data->mutex);
 			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,
+			ret = mma9553_read_status_word(data,
 						       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,
+			ret = mma9553_read_status_word(data,
 						       MMA9553_REG_STATUS,
 						       &tmp);
-			mutex_unlock(&data->mutex);
 			if (ret < 0)
 				return ret;
 
@@ -534,38 +521,17 @@ static int mma9553_read_raw(struct iio_dev *indio_dev,
 		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);
+			ret = mma9553_read_status_word(data,
+						       MMA9553_REG_SPEED,
+						       &tmp);
 			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,
+			ret = mma9553_read_status_word(data,
 						       MMA9553_REG_CALORIES,
 						       &tmp);
-			mutex_unlock(&data->mutex);
 			if (ret < 0)
 				return ret;
 			*val = tmp;
-- 
1.9.1

Re: [PATCH 5/8] iio: accel: mma9553: refactor mma9553_read_raw

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> Refactor code for simplicity and clarity.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
A good refactor indeed.
Will pick up after series reorder and possibly waiting for the
fixes to propagate through to my upstream.

J
> ---
>  drivers/iio/accel/mma9553.c | 100 +++++++++++++++-----------------------------
>  1 file changed, 33 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index ee2be3f..57868fb 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -439,6 +439,32 @@ static int mma9553_init(struct mma9553_data *data)
>  	return mma9551_set_device_state(data->client, true);
>  }
>  
> +static int mma9553_read_status_word(struct mma9553_data *data, u16 reg,
> +				    u16 *tmp)
> +{
> +	bool powered_on;
> +	int ret;
> +
> +	/*
> +	 * 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).
> +	 */
> +	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,
> +				       reg, tmp);
> +	mutex_unlock(&data->mutex);
> +	return ret;
> +}
> +
>  static int mma9553_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -447,69 +473,30 @@ static int mma9553_read_raw(struct iio_dev *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 */
> -			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,
> +			ret = mma9553_read_status_word(data,
>  						       MMA9553_REG_STEPCNT,
>  						       &tmp);
> -			mutex_unlock(&data->mutex);
>  			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,
> +			ret = mma9553_read_status_word(data,
>  						       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,
> +			ret = mma9553_read_status_word(data,
>  						       MMA9553_REG_STATUS,
>  						       &tmp);
> -			mutex_unlock(&data->mutex);
>  			if (ret < 0)
>  				return ret;
>  
> @@ -534,38 +521,17 @@ static int mma9553_read_raw(struct iio_dev *indio_dev,
>  		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);
> +			ret = mma9553_read_status_word(data,
> +						       MMA9553_REG_SPEED,
> +						       &tmp);
>  			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,
> +			ret = mma9553_read_status_word(data,
>  						       MMA9553_REG_CALORIES,
>  						       &tmp);
> -			mutex_unlock(&data->mutex);
>  			if (ret < 0)
>  				return ret;
>  			*val = tmp;
> 

[PATCH 6/8] iio: accel: mma9553: check input value for activity period

[Irina Tirdea]

When setting the activity period, the value introduced by
the user in sysfs is not checked for validity.

Add a boundary check so that only allowed values are
reported as suuccesfully written to device.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/mma9553.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 57868fb..96e4708 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -839,6 +839,9 @@ static int mma9553_write_event_value(struct iio_dev *indio_dev,
 	case IIO_EV_INFO_PERIOD:
 		switch (chan->type) {
 		case IIO_ACTIVITY:
+			if (val < 0 || val > MMA9553_ACTIVITY_THD_TO_SEC(
+			    MMA9553_MAX_ACTTHD))
+				return -EINVAL;
 			mutex_lock(&data->mutex);
 			ret = mma9553_set_config(data, MMA9553_REG_CONF_ACTTHD,
 						 &data->conf.actthd,
-- 
1.9.1

Re: [PATCH 6/8] iio: accel: mma9553: check input value for activity period

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> When setting the activity period, the value introduced by
> the user in sysfs is not checked for validity.
> 
> Add a boundary check so that only allowed values are
> reported as suuccesfully written to device.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
Good point.  This one counts as a fix so put it early in the
reordered series.
> ---
>  drivers/iio/accel/mma9553.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 57868fb..96e4708 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -839,6 +839,9 @@ static int mma9553_write_event_value(struct iio_dev *indio_dev,
>  	case IIO_EV_INFO_PERIOD:
>  		switch (chan->type) {
>  		case IIO_ACTIVITY:
> +			if (val < 0 || val > MMA9553_ACTIVITY_THD_TO_SEC(
> +			    MMA9553_MAX_ACTTHD))
> +				return -EINVAL;
>  			mutex_lock(&data->mutex);
>  			ret = mma9553_set_config(data, MMA9553_REG_CONF_ACTTHD,
>  						 &data->conf.actthd,
> 

[PATCH 7/8] iio: accel: mma9551_core: prevent buffer overrun

[Irina Tirdea]

The mma9551 functions that read/write word arrays from the
device have a limit for the buffer size given by the device
specifications.

Check that the requested buffer length is within required limits
when transferring word arrays. This will prevent buffer overrun
in the mma9551_read/write_*_words functions and also in the
mma9551_transfer call when writing into the MBOX response/request
structure.

Change the prototype for the mma9551_read/write_*_words functions
to receive the length of the buffer in words (instead of bytes) since
we are using a word buffer. This will prevent users from sending an
odd number of bytes for a word array.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/mma9551_core.c | 45 ++++++++++++++++++++++++++--------------
 drivers/iio/accel/mma9553.c      |  7 ++++---
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index 438cfed..5c6f69d 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -373,7 +373,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
  * @client:	I2C client
  * @app_id:	Application ID
  * @reg:	Application register
- * @len:	Length of array to read in bytes
+ * @len:	Length of array to read (in words)
  * @buf:	Array of words to read
  *
  * Read multiple configuration registers (word-sized registers).
@@ -388,15 +388,20 @@ int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
 			     u16 reg, u8 len, u16 *buf)
 {
 	int ret;
-	u8 i, len_words = len / sizeof(u16);
-	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
+	u8 i;
+	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
+
+	if (len > ARRAY_SIZE(be_buf)) {
+		dev_err(&client->dev, "Invalid buffer size %d\n", len);
+		return -EINVAL;
+	}
 
 	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
-			       reg, NULL, 0, (u8 *) be_buf, len);
+			       reg, NULL, 0, (u8 *)be_buf, len * sizeof(u16));
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < len_words; i++)
+	for (i = 0; i < len; i++)
 		buf[i] = be16_to_cpu(be_buf[i]);
 
 	return 0;
@@ -408,7 +413,7 @@ EXPORT_SYMBOL(mma9551_read_config_words);
  * @client:	I2C client
  * @app_id:	Application ID
  * @reg:	Application register
- * @len:	Length of array to read in bytes
+ * @len:	Length of array to read (in words)
  * @buf:	Array of words to read
  *
  * Read multiple status registers (word-sized registers).
@@ -423,15 +428,20 @@ int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
 			      u16 reg, u8 len, u16 *buf)
 {
 	int ret;
-	u8 i, len_words = len / sizeof(u16);
-	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
+	u8 i;
+	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
+
+	if (len > ARRAY_SIZE(be_buf)) {
+		dev_err(&client->dev, "Invalid buffer size %d\n", len);
+		return -EINVAL;
+	}
 
 	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
-			       reg, NULL, 0, (u8 *) be_buf, len);
+			       reg, NULL, 0, (u8 *)be_buf, len * sizeof(u16));
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < len_words; i++)
+	for (i = 0; i < len; i++)
 		buf[i] = be16_to_cpu(be_buf[i]);
 
 	return 0;
@@ -443,7 +453,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
  * @client:	I2C client
  * @app_id:	Application ID
  * @reg:	Application register
- * @len:	Length of array to write in bytes
+ * @len:	Length of array to write (in words)
  * @buf:	Array of words to write
  *
  * Write multiple configuration registers (word-sized registers).
@@ -457,14 +467,19 @@ EXPORT_SYMBOL(mma9551_read_status_words);
 int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
 			       u16 reg, u8 len, u16 *buf)
 {
-	u8 i, len_words = len / sizeof(u16);
-	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
+	u8 i;
+	__be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
+
+	if (len > ARRAY_SIZE(be_buf)) {
+		dev_err(&client->dev, "Invalid buffer size %d\n", len);
+		return -EINVAL;
+	}
 
-	for (i = 0; i < len_words; i++)
+	for (i = 0; i < len; 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);
+				reg, (u8 *)be_buf, len * sizeof(u16), NULL, 0);
 }
 EXPORT_SYMBOL(mma9551_write_config_words);
 
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 96e4708..6991b01 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -322,7 +322,8 @@ static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
 	int ret;
 
 	ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
-					MMA9553_REG_STATUS, sizeof(u32), buf);
+					MMA9553_REG_STATUS, ARRAY_SIZE(buf),
+					buf);
 	if (ret < 0) {
 		dev_err(&data->client->dev,
 			"error reading status and stepcnt\n");
@@ -395,7 +396,7 @@ static int mma9553_init(struct mma9553_data *data)
 	 */
 	ret = mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
 					MMA9553_REG_CONF_SLEEPMIN,
-					sizeof(data->conf),
+					sizeof(data->conf) / sizeof(u16),
 					(u16 *)&data->conf);
 	if (ret < 0) {
 		dev_err(&data->client->dev,
@@ -428,7 +429,7 @@ static int mma9553_init(struct mma9553_data *data)
 					     MMA9553_MASK_CONF_ACT_DBCNTM);
 	ret = mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
 					 MMA9553_REG_CONF_SLEEPMIN,
-					 sizeof(data->conf),
+					 sizeof(data->conf) / sizeof(u16),
 					 (u16 *)&data->conf);
 	if (ret < 0) {
 		dev_err(&data->client->dev,
-- 
1.9.1

Re: [PATCH 7/8] iio: accel: mma9551_core: prevent buffer overrun

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> The mma9551 functions that read/write word arrays from the
> device have a limit for the buffer size given by the device
> specifications.
> 
> Check that the requested buffer length is within required limits
> when transferring word arrays. This will prevent buffer overrun
> in the mma9551_read/write_*_words functions and also in the
> mma9551_transfer call when writing into the MBOX response/request
> structure.
> 
> Change the prototype for the mma9551_read/write_*_words functions
> to receive the length of the buffer in words (instead of bytes) since
> we are using a word buffer. This will prevent users from sending an
> odd number of bytes for a word array.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
This is a bit of a mixture of a fix and a refactor.  That leaves
us with a rather longer fix patch than would be ideal. 

could you break this into the minimal fix (just check limits and perhaps
fix the buffer length to be half as long) then a follow up rework patch
that gets you to the state at the end of this current patch?

Thanks,

J
> ---
>  drivers/iio/accel/mma9551_core.c | 45 ++++++++++++++++++++++++++--------------
>  drivers/iio/accel/mma9553.c      |  7 ++++---
>  2 files changed, 34 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 438cfed..5c6f69d 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -373,7 +373,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
>   * @client:	I2C client
>   * @app_id:	Application ID
>   * @reg:	Application register
> - * @len:	Length of array to read in bytes
> + * @len:	Length of array to read (in words)
>   * @buf:	Array of words to read
>   *
>   * Read multiple configuration registers (word-sized registers).
> @@ -388,15 +388,20 @@ int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
>  			     u16 reg, u8 len, u16 *buf)
>  {
>  	int ret;
> -	u8 i, len_words = len / sizeof(u16);
> -	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> +	u8 i;
> +	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> +
> +	if (len > ARRAY_SIZE(be_buf)) {
> +		dev_err(&client->dev, "Invalid buffer size %d\n", len);
> +		return -EINVAL;
> +	}
>  
>  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> -			       reg, NULL, 0, (u8 *) be_buf, len);
> +			       reg, NULL, 0, (u8 *)be_buf, len * sizeof(u16));
>  	if (ret < 0)
>  		return ret;
>  
> -	for (i = 0; i < len_words; i++)
> +	for (i = 0; i < len; i++)
>  		buf[i] = be16_to_cpu(be_buf[i]);
>  
>  	return 0;
> @@ -408,7 +413,7 @@ EXPORT_SYMBOL(mma9551_read_config_words);
>   * @client:	I2C client
>   * @app_id:	Application ID
>   * @reg:	Application register
> - * @len:	Length of array to read in bytes
> + * @len:	Length of array to read (in words)
>   * @buf:	Array of words to read
>   *
>   * Read multiple status registers (word-sized registers).
> @@ -423,15 +428,20 @@ int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
>  			      u16 reg, u8 len, u16 *buf)
>  {
>  	int ret;
> -	u8 i, len_words = len / sizeof(u16);
> -	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> +	u8 i;
> +	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> +
> +	if (len > ARRAY_SIZE(be_buf)) {
> +		dev_err(&client->dev, "Invalid buffer size %d\n", len);
> +		return -EINVAL;
> +	}
>  
>  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> -			       reg, NULL, 0, (u8 *) be_buf, len);
> +			       reg, NULL, 0, (u8 *)be_buf, len * sizeof(u16));
>  	if (ret < 0)
>  		return ret;
>  
> -	for (i = 0; i < len_words; i++)
> +	for (i = 0; i < len; i++)
>  		buf[i] = be16_to_cpu(be_buf[i]);
>  
>  	return 0;
> @@ -443,7 +453,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
>   * @client:	I2C client
>   * @app_id:	Application ID
>   * @reg:	Application register
> - * @len:	Length of array to write in bytes
> + * @len:	Length of array to write (in words)
>   * @buf:	Array of words to write
>   *
>   * Write multiple configuration registers (word-sized registers).
> @@ -457,14 +467,19 @@ EXPORT_SYMBOL(mma9551_read_status_words);
>  int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
>  			       u16 reg, u8 len, u16 *buf)
>  {
> -	u8 i, len_words = len / sizeof(u16);
> -	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> +	u8 i;
> +	__be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
> +
> +	if (len > ARRAY_SIZE(be_buf)) {
> +		dev_err(&client->dev, "Invalid buffer size %d\n", len);
> +		return -EINVAL;
> +	}
>  
> -	for (i = 0; i < len_words; i++)
> +	for (i = 0; i < len; 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);
> +				reg, (u8 *)be_buf, len * sizeof(u16), NULL, 0);
>  }
>  EXPORT_SYMBOL(mma9551_write_config_words);
>  
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 96e4708..6991b01 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -322,7 +322,8 @@ static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
>  	int ret;
>  
>  	ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
> -					MMA9553_REG_STATUS, sizeof(u32), buf);
> +					MMA9553_REG_STATUS, ARRAY_SIZE(buf),
> +					buf);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev,
>  			"error reading status and stepcnt\n");
> @@ -395,7 +396,7 @@ static int mma9553_init(struct mma9553_data *data)
>  	 */
>  	ret = mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
>  					MMA9553_REG_CONF_SLEEPMIN,
> -					sizeof(data->conf),
> +					sizeof(data->conf) / sizeof(u16),
>  					(u16 *)&data->conf);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev,
> @@ -428,7 +429,7 @@ static int mma9553_init(struct mma9553_data *data)
>  					     MMA9553_MASK_CONF_ACT_DBCNTM);
>  	ret = mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
>  					 MMA9553_REG_CONF_SLEEPMIN,
> -					 sizeof(data->conf),
> +					 sizeof(data->conf) / sizeof(u16),
>  					 (u16 *)&data->conf);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev,
> 

RE: [PATCH 7/8] iio: accel: mma9551_core: prevent buffer overrun

[Tirdea, Irina]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 09 April, 2015 14:51
> To: Tirdea, Irina; linux-iio@vger.kernel.org; Hartmut Knaack
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 7/8] iio: accel: mma9551_core: prevent buffer overrun
> 
> On 08/04/15 15:37, Irina Tirdea wrote:
> > The mma9551 functions that read/write word arrays from the
> > device have a limit for the buffer size given by the device
> > specifications.
> >
> > Check that the requested buffer length is within required limits
> > when transferring word arrays. This will prevent buffer overrun
> > in the mma9551_read/write_*_words functions and also in the
> > mma9551_transfer call when writing into the MBOX response/request
> > structure.
> >
> > Change the prototype for the mma9551_read/write_*_words functions
> > to receive the length of the buffer in words (instead of bytes) since
> > we are using a word buffer. This will prevent users from sending an
> > odd number of bytes for a word array.
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Suggested-by: Hartmut Knaack <knaack.h@gmx.de>
> This is a bit of a mixture of a fix and a refactor.  That leaves
> us with a rather longer fix patch than would be ideal.
> 
> could you break this into the minimal fix (just check limits and perhaps
> fix the buffer length to be half as long) then a follow up rework patch
> that gets you to the state at the end of this current patch?
> 
Sure, I'll break this in two patches: the actual fix and the change in function
signature.
> Thanks,
> 
> J
> > ---
> >  drivers/iio/accel/mma9551_core.c | 45 ++++++++++++++++++++++++++--------------
> >  drivers/iio/accel/mma9553.c      |  7 ++++---
> >  2 files changed, 34 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index 438cfed..5c6f69d 100644
> > --- a/drivers/iio/accel/mma9551_core.c
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -373,7 +373,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
> >   * @client:	I2C client
> >   * @app_id:	Application ID
> >   * @reg:	Application register
> > - * @len:	Length of array to read in bytes
> > + * @len:	Length of array to read (in words)
> >   * @buf:	Array of words to read
> >   *
> >   * Read multiple configuration registers (word-sized registers).
> > @@ -388,15 +388,20 @@ int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> >  			     u16 reg, u8 len, u16 *buf)
> >  {
> >  	int ret;
> > -	u8 i, len_words = len / sizeof(u16);
> > -	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +	u8 i;
> > +	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> > +
> > +	if (len > ARRAY_SIZE(be_buf)) {
> > +		dev_err(&client->dev, "Invalid buffer size %d\n", len);
> > +		return -EINVAL;
> > +	}
> >
> >  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> > -			       reg, NULL, 0, (u8 *) be_buf, len);
> > +			       reg, NULL, 0, (u8 *)be_buf, len * sizeof(u16));
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	for (i = 0; i < len_words; i++)
> > +	for (i = 0; i < len; i++)
> >  		buf[i] = be16_to_cpu(be_buf[i]);
> >
> >  	return 0;
> > @@ -408,7 +413,7 @@ EXPORT_SYMBOL(mma9551_read_config_words);
> >   * @client:	I2C client
> >   * @app_id:	Application ID
> >   * @reg:	Application register
> > - * @len:	Length of array to read in bytes
> > + * @len:	Length of array to read (in words)
> >   * @buf:	Array of words to read
> >   *
> >   * Read multiple status registers (word-sized registers).
> > @@ -423,15 +428,20 @@ int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> >  			      u16 reg, u8 len, u16 *buf)
> >  {
> >  	int ret;
> > -	u8 i, len_words = len / sizeof(u16);
> > -	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +	u8 i;
> > +	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> > +
> > +	if (len > ARRAY_SIZE(be_buf)) {
> > +		dev_err(&client->dev, "Invalid buffer size %d\n", len);
> > +		return -EINVAL;
> > +	}
> >
> >  	ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> > -			       reg, NULL, 0, (u8 *) be_buf, len);
> > +			       reg, NULL, 0, (u8 *)be_buf, len * sizeof(u16));
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	for (i = 0; i < len_words; i++)
> > +	for (i = 0; i < len; i++)
> >  		buf[i] = be16_to_cpu(be_buf[i]);
> >
> >  	return 0;
> > @@ -443,7 +453,7 @@ EXPORT_SYMBOL(mma9551_read_status_words);
> >   * @client:	I2C client
> >   * @app_id:	Application ID
> >   * @reg:	Application register
> > - * @len:	Length of array to write in bytes
> > + * @len:	Length of array to write (in words)
> >   * @buf:	Array of words to write
> >   *
> >   * Write multiple configuration registers (word-sized registers).
> > @@ -457,14 +467,19 @@ EXPORT_SYMBOL(mma9551_read_status_words);
> >  int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> >  			       u16 reg, u8 len, u16 *buf)
> >  {
> > -	u8 i, len_words = len / sizeof(u16);
> > -	__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +	u8 i;
> > +	__be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
> > +
> > +	if (len > ARRAY_SIZE(be_buf)) {
> > +		dev_err(&client->dev, "Invalid buffer size %d\n", len);
> > +		return -EINVAL;
> > +	}
> >
> > -	for (i = 0; i < len_words; i++)
> > +	for (i = 0; i < len; 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);
> > +				reg, (u8 *)be_buf, len * sizeof(u16), NULL, 0);
> >  }
> >  EXPORT_SYMBOL(mma9551_write_config_words);
> >
> > diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> > index 96e4708..6991b01 100644
> > --- a/drivers/iio/accel/mma9553.c
> > +++ b/drivers/iio/accel/mma9553.c
> > @@ -322,7 +322,8 @@ static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
> >  	int ret;
> >
> >  	ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
> > -					MMA9553_REG_STATUS, sizeof(u32), buf);
> > +					MMA9553_REG_STATUS, ARRAY_SIZE(buf),
> > +					buf);
> >  	if (ret < 0) {
> >  		dev_err(&data->client->dev,
> >  			"error reading status and stepcnt\n");
> > @@ -395,7 +396,7 @@ static int mma9553_init(struct mma9553_data *data)
> >  	 */
> >  	ret = mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
> >  					MMA9553_REG_CONF_SLEEPMIN,
> > -					sizeof(data->conf),
> > +					sizeof(data->conf) / sizeof(u16),
> >  					(u16 *)&data->conf);
> >  	if (ret < 0) {
> >  		dev_err(&data->client->dev,
> > @@ -428,7 +429,7 @@ static int mma9553_init(struct mma9553_data *data)
> >  					     MMA9553_MASK_CONF_ACT_DBCNTM);
> >  	ret = mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> >  					 MMA9553_REG_CONF_SLEEPMIN,
> > -					 sizeof(data->conf),
> > +					 sizeof(data->conf) / sizeof(u16),
> >  					 (u16 *)&data->conf);
> >  	if (ret < 0) {
> >  		dev_err(&data->client->dev,
> >

[PATCH 8/8] iio: accel: mma9553: add enable channel for activity

[Irina Tirdea]

Since we need to specifically power on the device if used
in polling mode, we have an enable channel for each type
(step count, distance, speed, calories, etc.).

Add also an enable channel for activity, so it can also
be polled independenly of events or other channels.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Suggested-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/accel/mma9553.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 6991b01..1359985 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -942,7 +942,8 @@ static const struct iio_chan_spec_ext_info mma9553_ext_info[] = {
 	.modified = 1,							\
 	.channel2 = _chan2,						\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),		\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT) |	\
+				    BIT(IIO_CHAN_INFO_ENABLE),		\
 	.event_spec = mma9553_activity_events,				\
 	.num_event_specs = ARRAY_SIZE(mma9553_activity_events),		\
 	.ext_info = mma9553_ext_info,					\
-- 
1.9.1

Re: [PATCH 8/8] iio: accel: mma9553: add enable channel for activity

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> Since we need to specifically power on the device if used
> in polling mode, we have an enable channel for each type
> (step count, distance, speed, calories, etc.).
> 
> Add also an enable channel for activity, so it can also
> be polled independenly of events or other channels.
> 
typo - independently

> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Suggested-by: Daniel Baluta <daniel.baluta@intel.com>
For the ones that are bug fixes wants to be reported-by
to get correctly picked up in the gitdm stats etc.

Again, this looks to be a fix to me so up the top of the series
please.

J
> ---
>  drivers/iio/accel/mma9553.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 6991b01..1359985 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -942,7 +942,8 @@ static const struct iio_chan_spec_ext_info mma9553_ext_info[] = {
>  	.modified = 1,							\
>  	.channel2 = _chan2,						\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),		\
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT) |	\
> +				    BIT(IIO_CHAN_INFO_ENABLE),		\
>  	.event_spec = mma9553_activity_events,				\
>  	.num_event_specs = ARRAY_SIZE(mma9553_activity_events),		\
>  	.ext_info = mma9553_ext_info,					\
> 

Re: [PATCH 0/8] Fixes for the mma9553 driver

[Jonathan Cameron]

On 08/04/15 15:37, Irina Tirdea wrote:
> This is a set of follow-up patches for the mma9553 driver that fixes
> some issues as suggested by Hartmut.
> 
> I have also added the enable channel for activity as suggested by Daniel.
> 
> Irina Tirdea (8):
>   iio: accel: mma9553: coding style fixes
>   iio: accel: mma9553: use unsigned counters
>   iio: accel: mma9553: fix gpio bitnum init value
>   iio: accel: mma9553: refactor mma9553_read_activity_stepcnt
>   iio: accel: mma9553: refactor mma9553_read_raw
>   iio: accel: mma9553: check input value for activity period
>   iio: accel: mma9551_core: prevent buffer overrun
>   iio: accel: mma9553: add enable channel for activity
> 
>  drivers/iio/accel/mma9551_core.c |  58 ++++++----
>  drivers/iio/accel/mma9551_core.h |   2 +-
>  drivers/iio/accel/mma9553.c      | 227 +++++++++++++++++----------------------
>  3 files changed, 136 insertions(+), 151 deletions(-)
> 
Hi Irina,

We've missed the merge window, so these are going to mostly hit one cycle
after the original driver posting.

As such can you reorder the patches to pull all the bug fixes to the beginning
as they can go into 4.1 whereas the style fixes etc will go into 4.2

RE: [PATCH 0/8] Fixes for the mma9553 driver

[Tirdea, Irina]

> -----Original Message-----
> From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-owner@vger.kernel.org] On Behalf Of Jonathan Cameron
> Sent: 09 April, 2015 14:13
> To: Tirdea, Irina; linux-iio@vger.kernel.org; Hartmut Knaack
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/8] Fixes for the mma9553 driver
> 
> On 08/04/15 15:37, Irina Tirdea wrote:
> > This is a set of follow-up patches for the mma9553 driver that fixes
> > some issues as suggested by Hartmut.
> >
> > I have also added the enable channel for activity as suggested by Daniel.
> >
> > Irina Tirdea (8):
> >   iio: accel: mma9553: coding style fixes
> >   iio: accel: mma9553: use unsigned counters
> >   iio: accel: mma9553: fix gpio bitnum init value
> >   iio: accel: mma9553: refactor mma9553_read_activity_stepcnt
> >   iio: accel: mma9553: refactor mma9553_read_raw
> >   iio: accel: mma9553: check input value for activity period
> >   iio: accel: mma9551_core: prevent buffer overrun
> >   iio: accel: mma9553: add enable channel for activity
> >
> >  drivers/iio/accel/mma9551_core.c |  58 ++++++----
> >  drivers/iio/accel/mma9551_core.h |   2 +-
> >  drivers/iio/accel/mma9553.c      | 227 +++++++++++++++++----------------------
> >  3 files changed, 136 insertions(+), 151 deletions(-)
> >
> Hi Irina,
Hi Jonathan,
> 
> We've missed the merge window, so these are going to mostly hit one cycle
> after the original driver posting.
> 
> As such can you reorder the patches to pull all the bug fixes to the beginning
> as they can go into 4.1 whereas the style fixes etc will go into 4.2
> --

Sure, I'll reorder the series and send v2.

Thanks,
Irina

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