This is a set of follow-up patches for the mma9553 driver that fixes
issues as suggested by Hartmut and Daniel.
Changes in v2:
- split some of the the v1 patches into smaller patches
as suggested by Jonathan
- reordered the series to have bug fixes first followed by style fixes
- use Reported-by for bug fixes
- added more fixes for issues detected by checkpatch.pl --strict
Irina Tirdea (17):
iio: accel: mma9553: fix endianness issue when reading status
iio: accel: mma9553: check input value for activity period
iio: accel: mma9551_core: prevent buffer overrun
iio: accel: mma9553: add enable channel for activity
iio: accel: mma9551_core: wrong doc fixes
iio: accel: mma9551_core: typo fix in RSC APP ID
iio: accel: mma9553: check for error in reading initial activity and
stepcnt
iio: accel: mma9553: return 0 as indication of success
iio: accel: mma9553: comment and error message fixes
iio: accel: mma9553: use GENMASK
iio: accel: mma9553: prefix naming fixes
iio: accel: mma9553: fix gpio bitnum init value
iio: accel: mma9553: refactor mma9553_read_raw
iio: accel: mma9551_core: use size in words for word buffers
iio: accel: mma9553: fix alignment issues
iio: accel: mma9553: document use of mutex
iio: accel: mma9553: use unsigned counters
drivers/iio/accel/mma9551_core.c | 70 ++++++-----
drivers/iio/accel/mma9551_core.h | 8 +-
drivers/iio/accel/mma9553.c | 253 ++++++++++++++++++---------------------
3 files changed, 160 insertions(+), 171 deletions(-)
--
1.9.1
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 <[email protected]>
Reported-by: Hartmut Knaack <[email protected]>
---
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 2df1af7..607dbfc 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -316,22 +316,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
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 successfully written to device.
Signed-off-by: Irina Tirdea <[email protected]>
Reported-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9553.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 607dbfc..03120fb 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
@@ -869,6 +870,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
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.
Signed-off-by: Irina Tirdea <[email protected]>
Reported-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9551_core.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index 7f55a6d..c6d5a3a 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -389,7 +389,12 @@ int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
{
int ret, i;
int len_words = len / sizeof(u16);
- __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
+ __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
+
+ if (len_words > 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);
@@ -424,7 +429,12 @@ int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
{
int ret, i;
int len_words = len / sizeof(u16);
- __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
+ __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
+
+ if (len_words > 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);
@@ -459,7 +469,12 @@ int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
{
int i;
int len_words = len / sizeof(u16);
- __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
+ __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
+
+ if (len_words > ARRAY_SIZE(be_buf)) {
+ dev_err(&client->dev, "Invalid buffer size %d\n", len);
+ return -EINVAL;
+ }
for (i = 0; i < len_words; i++)
be_buf[i] = cpu_to_be16(buf[i]);
--
1.9.1
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 independently of events or other channels.
Signed-off-by: Irina Tirdea <[email protected]>
Reported-by: Daniel Baluta <[email protected]>
---
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 03120fb..365a109 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -972,7 +972,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
Fix docummentation for mma9553_read_* functions.
Signed-off-by: Irina Tirdea <[email protected]>
Reported-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9551_core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index c6d5a3a..efe09a3 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).
*
@@ -414,7 +414,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).
*
@@ -454,7 +454,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).
*
--
1.9.1
Fix typo in Reset/Suspend/Clear Application ID definition.
Signed-off-by: Irina Tirdea <[email protected]>
Reported-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9551_core.c | 2 +-
drivers/iio/accel/mma9551_core.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index efe09a3..2fd2a99 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -800,7 +800,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 */
--
1.9.1
When configuring gpio, we need to read initial values for activity and
step count. This function may fail due to i2c read errors.
Check the error code returned by mma9553_read_activity_stepcnt
and return the appropriate error in gpio config function.
Signed-off-by: Irina Tirdea <[email protected]>
Reported-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9553.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 365a109..6f0a14b 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -363,9 +363,12 @@ 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,
--
1.9.1
Use return 0 instead of return ret to mark
clearly the success return path.
Signed-off-by: Irina Tirdea <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9553.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 6f0a14b..0f30006 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -792,7 +792,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) {
--
1.9.1
Use "GPIO" instead of "gpio" and "ACPI" instead of "acpi".
Includes a couple of small style fixes in comments
(missing full stop, whitespace, paranthesis).
Signed-off-by: Irina Tirdea <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9553.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 0f30006..a6b74de 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -76,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_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.
*/
@@ -399,13 +399,13 @@ static int mma9553_init(struct mma9553_data *data)
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;
@@ -457,7 +457,8 @@ static int mma9553_read_raw(struct iio_dev *indio_dev,
* 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 */
+ * step counter).
+ */
powered_on =
mma9553_is_any_event_enabled(data, false, 0) ||
data->stepcnt_enabled;
@@ -900,7 +901,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;
}
@@ -1113,16 +1114,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
Use GENMASK instead of BIT or direct value to
define a mask.
Signed-off-by: Irina Tirdea <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9553.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index a6b74de..47cb14b 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -63,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
--
1.9.1
Add mma9553_ prefix to all local functions/declarations.
Signed-off-by: Irina Tirdea <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9553.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 47cb14b..9573147 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -76,8 +76,8 @@
#define MMA9553_DEFAULT_GPIO_PIN mma9551_gpio6
#define MMA9553_DEFAULT_GPIO_POLARITY 0
-#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 */
@@ -351,11 +351,11 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
* 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;
@@ -948,11 +948,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,
};
--
1.9.1
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 <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
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 9573147..5755977 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 */
@@ -404,8 +405,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
Refactor code for simplicity and clarity.
Signed-off-by: Irina Tirdea <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9553.c | 101 +++++++++++++++-----------------------------
1 file changed, 33 insertions(+), 68 deletions(-)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 5755977..8bfc618 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -440,6 +440,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)
@@ -448,70 +474,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;
@@ -536,38 +522,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
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 <[email protected]>
---
drivers/iio/accel/mma9551_core.c | 27 ++++++++++++---------------
drivers/iio/accel/mma9553.c | 9 ++++++---
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index 2fd2a99..583660b 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,20 +388,19 @@ 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);
__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
- if (len_words > ARRAY_SIZE(be_buf)) {
+ 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;
@@ -413,7 +412,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).
@@ -428,20 +427,19 @@ 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);
__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
- if (len_words > ARRAY_SIZE(be_buf)) {
+ 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;
@@ -453,7 +451,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).
@@ -468,19 +466,18 @@ 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);
__be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
- if (len_words > ARRAY_SIZE(be_buf)) {
+ 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 8bfc618..06c8707 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");
@@ -397,7 +398,8 @@ 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), (u16 *) &data->conf);
+ sizeof(data->conf) / sizeof(u16),
+ (u16 *)&data->conf);
if (ret < 0) {
dev_err(&data->client->dev,
"failed to read configuration registers\n");
@@ -430,7 +432,8 @@ static int mma9553_init(struct mma9553_data *data)
ret =
mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
MMA9553_REG_CONF_SLEEPMIN,
- sizeof(data->conf), (u16 *) &data->conf);
+ sizeof(data->conf) / sizeof(u16),
+ (u16 *)&data->conf);
if (ret < 0) {
dev_err(&data->client->dev,
"failed to write configuration registers\n");
--
1.9.1
Fix code alignment and wrap parameters.
Fix issues reported by checkpatch.pl --strict.
Signed-off-by: Irina Tirdea <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9551_core.c | 8 ++---
drivers/iio/accel/mma9551_core.h | 6 ++--
drivers/iio/accel/mma9553.c | 76 +++++++++++++++++++---------------------
3 files changed, 43 insertions(+), 47 deletions(-)
diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index 583660b..c34c5ce 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -297,7 +297,7 @@ EXPORT_SYMBOL(mma9551_read_status_byte);
* Returns: 0 on success, negative value on failure.
*/
int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
- u16 reg, u16 *val)
+ u16 reg, u16 *val)
{
int ret;
__be16 v;
@@ -328,12 +328,12 @@ EXPORT_SYMBOL(mma9551_read_config_word);
* Returns: 0 on success, negative value on failure.
*/
int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
- u16 reg, u16 val)
+ 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);
+ (u8 *)&v, 2, NULL, 0);
}
EXPORT_SYMBOL(mma9551_write_config_word);
@@ -385,7 +385,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
* 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)
+ u16 reg, u8 len, u16 *buf)
{
int ret, i;
__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
index 79939e4..5e88e64 100644
--- a/drivers/iio/accel/mma9551_core.h
+++ b/drivers/iio/accel/mma9551_core.h
@@ -53,13 +53,13 @@ int mma9551_write_config_byte(struct i2c_client *client, u8 app_id,
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);
+ u16 reg, u16 *val);
int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
- u16 reg, u16 val);
+ 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);
+ 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,
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 06c8707..08f28c3 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -343,10 +343,10 @@ 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.
@@ -372,9 +372,8 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
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;
@@ -395,18 +394,16 @@ 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) / sizeof(u16),
- (u16 *)&data->conf);
+ ret = mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
+ MMA9553_REG_CONF_SLEEPMIN,
+ sizeof(data->conf) / sizeof(u16),
+ (u16 *)&data->conf);
if (ret < 0) {
dev_err(&data->client->dev,
"failed to read configuration registers\n");
return ret;
}
-
/* Reset GPIO */
data->gpio_bitnum = MMA9553_MAX_BITNUM;
ret = mma9553_conf_gpio(data);
@@ -421,19 +418,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) / sizeof(u16),
- (u16 *)&data->conf);
+ ret = mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
+ MMA9553_REG_CONF_SLEEPMIN,
+ sizeof(data->conf) / sizeof(u16),
+ (u16 *)&data->conf);
if (ret < 0) {
dev_err(&data->client->dev,
"failed to write configuration registers\n");
@@ -570,7 +566,7 @@ static int mma9553_read_raw(struct iio_dev *indio_dev,
return IIO_VAL_INT;
case IIO_CHAN_INFO_CALIBHEIGHT:
tmp = mma9553_get_bits(data->conf.height_weight,
- MMA9553_MASK_CONF_HEIGHT);
+ MMA9553_MASK_CONF_HEIGHT);
*val = tmp / 100; /* cm to m */
*val2 = (tmp % 100) * 10000;
return IIO_VAL_INT_PLUS_MICRO;
@@ -722,7 +718,6 @@ static int mma9553_read_event_config(struct iio_dev *indio_dev,
enum iio_event_type type,
enum iio_event_direction dir)
{
-
struct mma9553_data *data = iio_priv(indio_dev);
struct mma9553_event *event;
@@ -1029,22 +1024,22 @@ 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;
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),
+ IIO_EV_DIR_NONE,
+ IIO_EV_TYPE_CHANGE, 0, 0, 0),
data->timestamp);
}
@@ -1054,17 +1049,19 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
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),
+ 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),
+ ev_activity->info->mod,
+ IIO_EV_DIR_RISING,
+ IIO_EV_TYPE_THRESH, 0, 0,
+ 0),
data->timestamp);
}
mutex_unlock(&data->mutex);
@@ -1159,7 +1156,6 @@ static int mma9553_probe(struct i2c_client *client,
client->irq);
goto out_poweroff;
}
-
}
ret = iio_device_register(indio_dev);
--
1.9.1
Fix checkpatch.pl --strict check:
CHECK: struct mutex definition without comment
+ struct mutex mutex;
Signed-off-by: Irina Tirdea <[email protected]>
---
drivers/iio/accel/mma9553.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 08f28c3..a605280 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -182,6 +182,10 @@ struct mma9553_conf_regs {
struct mma9553_data {
struct i2c_client *client;
+ /*
+ * 1. Serialize access to HW (requested by mma9551_core API).
+ * 2. Serialize sequences that power on/off the device and access HW.
+ */
struct mutex mutex;
struct mma9553_conf_regs conf;
struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];
--
1.9.1
Use unsigned counters instead of signed when all the
possible values are positive.
Signed-off-by: Irina Tirdea <[email protected]>
Suggested-by: Hartmut Knaack <[email protected]>
---
drivers/iio/accel/mma9551_core.c | 12 +++++++-----
drivers/iio/accel/mma9553.c | 8 ++++----
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index c34c5ce..44b8243 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -115,8 +115,8 @@ struct mma9551_version_info {
static int mma9551_transfer(struct i2c_client *client,
u8 app_id, u8 command, u16 offset,
- u8 *inbytes, int num_inbytes,
- u8 *outbytes, int num_outbytes)
+ u8 *inbytes, u8 num_inbytes,
+ u8 *outbytes, u8 num_outbytes)
{
struct mma9551_mbox_request req;
struct mma9551_mbox_response rsp;
@@ -387,7 +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 ret;
+ u8 i;
__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
if (len > ARRAY_SIZE(be_buf)) {
@@ -426,7 +427,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 ret;
+ u8 i;
__be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
if (len > ARRAY_SIZE(be_buf)) {
@@ -465,7 +467,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;
+ u8 i;
__be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
if (len > ARRAY_SIZE(be_buf)) {
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index a605280..ad587ec 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -189,7 +189,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:
@@ -230,7 +230,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++) {
@@ -244,7 +244,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 &&
@@ -259,7 +259,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
On 13/04/15 16:40, 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 <[email protected]>
> Reported-by: Hartmut Knaack <[email protected]>
Applied to the fixes-togreg branch of iio.git
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 2df1af7..607dbfc 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -316,22 +316,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;
> }
>
On 13/04/15 16:40, 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.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Reported-by: Hartmut Knaack <[email protected]>
Applied to the fixes-togreg branch of iio.git
Thanks,
Jonathan
> ---
> drivers/iio/accel/mma9551_core.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 7f55a6d..c6d5a3a 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -389,7 +389,12 @@ int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> {
> int ret, i;
> int len_words = len / sizeof(u16);
> - __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> +
> + if (len_words > 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);
> @@ -424,7 +429,12 @@ int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> {
> int ret, i;
> int len_words = len / sizeof(u16);
> - __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> +
> + if (len_words > 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);
> @@ -459,7 +469,12 @@ int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> {
> int i;
> int len_words = len / sizeof(u16);
> - __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> + __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
> +
> + if (len_words > ARRAY_SIZE(be_buf)) {
> + dev_err(&client->dev, "Invalid buffer size %d\n", len);
> + return -EINVAL;
> + }
>
> for (i = 0; i < len_words; i++)
> be_buf[i] = cpu_to_be16(buf[i]);
>
On 13/04/15 16:40, Irina Tirdea wrote:
Err, this first bit is not relevant to this patch so I've dropped it.
> 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 independently of events or other channels.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Reported-by: Daniel Baluta <[email protected]>
Applied to the fixes-togreg branch of iio.git
Thanks,
> ---
> 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 03120fb..365a109 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -972,7 +972,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, \
>
On 13/04/15 16:40, 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 successfully written to device.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Reported-by: Hartmut Knaack <[email protected]>
hmm. Not that critical as it just gives miss information to
userspace that is writing an incorrect value. Still it's small
so I've taken it in the fixes-togreg branch.
Thanks,
Jonathan
> ---
> drivers/iio/accel/mma9553.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 607dbfc..03120fb 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
> @@ -869,6 +870,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,
>
On 13/04/15 16:40, Irina Tirdea wrote:
> Fix docummentation for mma9553_read_* functions.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Reported-by: Hartmut Knaack <[email protected]>
Applied to the togreg branch of iio.git. Initially pushed
out as testing for the autobuilders to play with it.
Jonathan
> ---
> drivers/iio/accel/mma9551_core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index c6d5a3a..efe09a3 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).
> *
> @@ -414,7 +414,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).
> *
> @@ -454,7 +454,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).
> *
>
On 13/04/15 16:40, Irina Tirdea wrote:
> Fix typo in Reset/Suspend/Clear Application ID definition.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Reported-by: Hartmut Knaack <[email protected]>
applied to the togreg branch of iio.git
Thanks,
> ---
> drivers/iio/accel/mma9551_core.c | 2 +-
> drivers/iio/accel/mma9551_core.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index efe09a3..2fd2a99 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -800,7 +800,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 */
>
On 13/04/15 16:40, Irina Tirdea wrote:
> When configuring gpio, we need to read initial values for activity and
> step count. This function may fail due to i2c read errors.
>
> Check the error code returned by mma9553_read_activity_stepcnt
> and return the appropriate error in gpio config function.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Reported-by: Hartmut Knaack <[email protected]>
applied, togreg....
> ---
> drivers/iio/accel/mma9553.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 365a109..6f0a14b 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -363,9 +363,12 @@ 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,
>
On 13/04/15 16:40, Irina Tirdea wrote:
> Use "GPIO" instead of "gpio" and "ACPI" instead of "acpi".
>
> Includes a couple of small style fixes in comments
> (missing full stop, whitespace, paranthesis).
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Suggested-by: Hartmut Knaack <[email protected]>
Applied to the togreg branch of iio.git
Thanks.
J
> ---
> drivers/iio/accel/mma9553.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 0f30006..a6b74de 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -76,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_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.
> */
> @@ -399,13 +399,13 @@ static int mma9553_init(struct mma9553_data *data)
> 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;
> @@ -457,7 +457,8 @@ static int mma9553_read_raw(struct iio_dev *indio_dev,
> * 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 */
> + * step counter).
> + */
> powered_on =
> mma9553_is_any_event_enabled(data, false, 0) ||
> data->stepcnt_enabled;
> @@ -900,7 +901,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;
> }
> @@ -1113,16 +1114,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;
> }
>
On 13/04/15 16:40, Irina Tirdea wrote:
> Use GENMASK instead of BIT or direct value to
> define a mask.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Suggested-by: Hartmut Knaack <[email protected]>
Applied.
> ---
> drivers/iio/accel/mma9553.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index a6b74de..47cb14b 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -63,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
>
On 13/04/15 16:40, Irina Tirdea wrote:
> Add mma9553_ prefix to all local functions/declarations.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Suggested-by: Hartmut Knaack <[email protected]>
app
> ---
> drivers/iio/accel/mma9553.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 47cb14b..9573147 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -76,8 +76,8 @@
> #define MMA9553_DEFAULT_GPIO_PIN mma9551_gpio6
> #define MMA9553_DEFAULT_GPIO_POLARITY 0
>
> -#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 */
>
> @@ -351,11 +351,11 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
> * 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;
>
> @@ -948,11 +948,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,
> };
>
On 13/04/15 16:40, 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 <[email protected]>
> Suggested-by: Hartmut Knaack <[email protected]>
Applied. I thought about adding this to the fixes-togreg
branch but got lazy and added it to the togreg one instead.
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 9573147..5755977 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 */
>
> @@ -404,8 +405,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;
>
On 13/04/15 16:41, Irina Tirdea wrote:
> Refactor code for simplicity and clarity.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Suggested-by: Hartmut Knaack <[email protected]>
Applied.
> ---
> drivers/iio/accel/mma9553.c | 101 +++++++++++++++-----------------------------
> 1 file changed, 33 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 5755977..8bfc618 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -440,6 +440,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)
> @@ -448,70 +474,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;
>
> @@ -536,38 +522,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;
>
On 13/04/15 16:41, Irina Tirdea wrote:
> 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 <[email protected]>
Lots of merge issues by this point in the series so I'll hold this one at
least until the fixes filter through.
Remind me if I seem to have forgotten (it wouldn't be the first time :)
Jonathan
> ---
> drivers/iio/accel/mma9551_core.c | 27 ++++++++++++---------------
> drivers/iio/accel/mma9553.c | 9 ++++++---
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 2fd2a99..583660b 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,20 +388,19 @@ 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);
> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
>
> - if (len_words > ARRAY_SIZE(be_buf)) {
> + 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;
> @@ -413,7 +412,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).
> @@ -428,20 +427,19 @@ 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);
> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
>
> - if (len_words > ARRAY_SIZE(be_buf)) {
> + 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;
> @@ -453,7 +451,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).
> @@ -468,19 +466,18 @@ 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);
> __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
>
> - if (len_words > ARRAY_SIZE(be_buf)) {
> + 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 8bfc618..06c8707 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");
> @@ -397,7 +398,8 @@ 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), (u16 *) &data->conf);
> + sizeof(data->conf) / sizeof(u16),
> + (u16 *)&data->conf);
> if (ret < 0) {
> dev_err(&data->client->dev,
> "failed to read configuration registers\n");
> @@ -430,7 +432,8 @@ static int mma9553_init(struct mma9553_data *data)
> ret =
> mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> MMA9553_REG_CONF_SLEEPMIN,
> - sizeof(data->conf), (u16 *) &data->conf);
> + sizeof(data->conf) / sizeof(u16),
> + (u16 *)&data->conf);
> if (ret < 0) {
> dev_err(&data->client->dev,
> "failed to write configuration registers\n");
>
Irina Tirdea schrieb am 13.04.2015 um 17:40:
> This is a set of follow-up patches for the mma9553 driver that fixes
> issues as suggested by Hartmut and Daniel.
>
Finally, I got around to this series. Looks good to me.
Thanks,
Hartmut
> Changes in v2:
> - split some of the the v1 patches into smaller patches
> as suggested by Jonathan
> - reordered the series to have bug fixes first followed by style fixes
> - use Reported-by for bug fixes
> - added more fixes for issues detected by checkpatch.pl --strict
>
> Irina Tirdea (17):
> iio: accel: mma9553: fix endianness issue when reading status
> iio: accel: mma9553: check input value for activity period
> iio: accel: mma9551_core: prevent buffer overrun
> iio: accel: mma9553: add enable channel for activity
> iio: accel: mma9551_core: wrong doc fixes
> iio: accel: mma9551_core: typo fix in RSC APP ID
> iio: accel: mma9553: check for error in reading initial activity and
> stepcnt
> iio: accel: mma9553: return 0 as indication of success
> iio: accel: mma9553: comment and error message fixes
> iio: accel: mma9553: use GENMASK
> iio: accel: mma9553: prefix naming fixes
> iio: accel: mma9553: fix gpio bitnum init value
> iio: accel: mma9553: refactor mma9553_read_raw
> iio: accel: mma9551_core: use size in words for word buffers
> iio: accel: mma9553: fix alignment issues
> iio: accel: mma9553: document use of mutex
> iio: accel: mma9553: use unsigned counters
>
> drivers/iio/accel/mma9551_core.c | 70 ++++++-----
> drivers/iio/accel/mma9551_core.h | 8 +-
> drivers/iio/accel/mma9553.c | 253 ++++++++++++++++++---------------------
> 3 files changed, 160 insertions(+), 171 deletions(-)
>
> -----Original Message-----
> From: Jonathan Cameron [mailto:[email protected]]
> Sent: 26 April, 2015 22:05
> To: Tirdea, Irina; [email protected]; Hartmut Knaack
> Cc: [email protected]; Dogaru, Vlad
> Subject: Re: [PATCH v2 14/17] iio: accel: mma9551_core: use size in words for word buffers
>
> On 13/04/15 16:41, Irina Tirdea wrote:
> > 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 <[email protected]>
> Lots of merge issues by this point in the series so I'll hold this one at
> least until the fixes filter through.
>
> Remind me if I seem to have forgotten (it wouldn't be the first time :)
>
Sure, I'll keep an eye on when the fixes will be merged in togreg branch.
Thanks,
Irina
> Jonathan
> > ---
> > drivers/iio/accel/mma9551_core.c | 27 ++++++++++++---------------
> > drivers/iio/accel/mma9553.c | 9 ++++++---
> > 2 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index 2fd2a99..583660b 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,20 +388,19 @@ 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);
> > __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> >
> > - if (len_words > ARRAY_SIZE(be_buf)) {
> > + 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;
> > @@ -413,7 +412,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).
> > @@ -428,20 +427,19 @@ 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);
> > __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> >
> > - if (len_words > ARRAY_SIZE(be_buf)) {
> > + 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;
> > @@ -453,7 +451,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).
> > @@ -468,19 +466,18 @@ 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);
> > __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
> >
> > - if (len_words > ARRAY_SIZE(be_buf)) {
> > + 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 8bfc618..06c8707 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");
> > @@ -397,7 +398,8 @@ 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), (u16 *) &data->conf);
> > + sizeof(data->conf) / sizeof(u16),
> > + (u16 *)&data->conf);
> > if (ret < 0) {
> > dev_err(&data->client->dev,
> > "failed to read configuration registers\n");
> > @@ -430,7 +432,8 @@ static int mma9553_init(struct mma9553_data *data)
> > ret =
> > mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> > MMA9553_REG_CONF_SLEEPMIN,
> > - sizeof(data->conf), (u16 *) &data->conf);
> > + sizeof(data->conf) / sizeof(u16),
> > + (u16 *)&data->conf);
> > if (ret < 0) {
> > dev_err(&data->client->dev,
> > "failed to write configuration registers\n");
> >
On 29/04/15 13:20, Tirdea, Irina wrote:
>
>
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:[email protected]]
>> Sent: 26 April, 2015 22:05
>> To: Tirdea, Irina; [email protected]; Hartmut Knaack
>> Cc: [email protected]; Dogaru, Vlad
>> Subject: Re: [PATCH v2 14/17] iio: accel: mma9551_core: use size in words for word buffers
>>
>> On 13/04/15 16:41, Irina Tirdea wrote:
>>> 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 <[email protected]>
>> Lots of merge issues by this point in the series so I'll hold this one at
>> least until the fixes filter through.
>>
>> Remind me if I seem to have forgotten (it wouldn't be the first time :)
>>
> Sure, I'll keep an eye on when the fixes will be merged in togreg branch.
>
They are there now, so applied to the togreg branch of iio.git, initially pushed
out as testing for the autobuilders to play with it.
Now looking at the merge window in 3 months time though to go upstream unfortunately.
Sorry for the delay, been a busy couple of months!
Jonathan
> Thanks,
> Irina
>
>> Jonathan
>>> ---
>>> drivers/iio/accel/mma9551_core.c | 27 ++++++++++++---------------
>>> drivers/iio/accel/mma9553.c | 9 ++++++---
>>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
>>> index 2fd2a99..583660b 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,20 +388,19 @@ 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);
>>> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
>>>
>>> - if (len_words > ARRAY_SIZE(be_buf)) {
>>> + 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;
>>> @@ -413,7 +412,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).
>>> @@ -428,20 +427,19 @@ 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);
>>> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
>>>
>>> - if (len_words > ARRAY_SIZE(be_buf)) {
>>> + 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;
>>> @@ -453,7 +451,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).
>>> @@ -468,19 +466,18 @@ 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);
>>> __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
>>>
>>> - if (len_words > ARRAY_SIZE(be_buf)) {
>>> + 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 8bfc618..06c8707 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");
>>> @@ -397,7 +398,8 @@ 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), (u16 *) &data->conf);
>>> + sizeof(data->conf) / sizeof(u16),
>>> + (u16 *)&data->conf);
>>> if (ret < 0) {
>>> dev_err(&data->client->dev,
>>> "failed to read configuration registers\n");
>>> @@ -430,7 +432,8 @@ static int mma9553_init(struct mma9553_data *data)
>>> ret =
>>> mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
>>> MMA9553_REG_CONF_SLEEPMIN,
>>> - sizeof(data->conf), (u16 *) &data->conf);
>>> + sizeof(data->conf) / sizeof(u16),
>>> + (u16 *)&data->conf);
>>> if (ret < 0) {
>>> dev_err(&data->client->dev,
>>> "failed to write configuration registers\n");
>>>
>
On 13/04/15 16:41, Irina Tirdea wrote:
> Fix code alignment and wrap parameters.
> Fix issues reported by checkpatch.pl --strict.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Suggested-by: Hartmut Knaack <[email protected]>
Applied to the togreg branch of iio.git, initially pushed out
as testing etc. etc..
> ---
> drivers/iio/accel/mma9551_core.c | 8 ++---
> drivers/iio/accel/mma9551_core.h | 6 ++--
> drivers/iio/accel/mma9553.c | 76 +++++++++++++++++++---------------------
> 3 files changed, 43 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 583660b..c34c5ce 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -297,7 +297,7 @@ EXPORT_SYMBOL(mma9551_read_status_byte);
> * Returns: 0 on success, negative value on failure.
> */
> int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
> - u16 reg, u16 *val)
> + u16 reg, u16 *val)
> {
> int ret;
> __be16 v;
> @@ -328,12 +328,12 @@ EXPORT_SYMBOL(mma9551_read_config_word);
> * Returns: 0 on success, negative value on failure.
> */
> int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> - u16 reg, u16 val)
> + 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);
> + (u8 *)&v, 2, NULL, 0);
> }
> EXPORT_SYMBOL(mma9551_write_config_word);
>
> @@ -385,7 +385,7 @@ EXPORT_SYMBOL(mma9551_read_status_word);
> * 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)
> + u16 reg, u8 len, u16 *buf)
> {
> int ret, i;
> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
> index 79939e4..5e88e64 100644
> --- a/drivers/iio/accel/mma9551_core.h
> +++ b/drivers/iio/accel/mma9551_core.h
> @@ -53,13 +53,13 @@ int mma9551_write_config_byte(struct i2c_client *client, u8 app_id,
> 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);
> + u16 reg, u16 *val);
> int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> - u16 reg, u16 val);
> + 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);
> + 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,
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 06c8707..08f28c3 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -343,10 +343,10 @@ 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.
> @@ -372,9 +372,8 @@ static int mma9553_conf_gpio(struct mma9553_data *data)
> 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;
> @@ -395,18 +394,16 @@ 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) / sizeof(u16),
> - (u16 *)&data->conf);
> + ret = mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_CONF_SLEEPMIN,
> + sizeof(data->conf) / sizeof(u16),
> + (u16 *)&data->conf);
> if (ret < 0) {
> dev_err(&data->client->dev,
> "failed to read configuration registers\n");
> return ret;
> }
>
> -
> /* Reset GPIO */
> data->gpio_bitnum = MMA9553_MAX_BITNUM;
> ret = mma9553_conf_gpio(data);
> @@ -421,19 +418,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) / sizeof(u16),
> - (u16 *)&data->conf);
> + ret = mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_CONF_SLEEPMIN,
> + sizeof(data->conf) / sizeof(u16),
> + (u16 *)&data->conf);
> if (ret < 0) {
> dev_err(&data->client->dev,
> "failed to write configuration registers\n");
> @@ -570,7 +566,7 @@ static int mma9553_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_CALIBHEIGHT:
> tmp = mma9553_get_bits(data->conf.height_weight,
> - MMA9553_MASK_CONF_HEIGHT);
> + MMA9553_MASK_CONF_HEIGHT);
> *val = tmp / 100; /* cm to m */
> *val2 = (tmp % 100) * 10000;
> return IIO_VAL_INT_PLUS_MICRO;
> @@ -722,7 +718,6 @@ static int mma9553_read_event_config(struct iio_dev *indio_dev,
> enum iio_event_type type,
> enum iio_event_direction dir)
> {
> -
> struct mma9553_data *data = iio_priv(indio_dev);
> struct mma9553_event *event;
>
> @@ -1029,22 +1024,22 @@ 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;
> 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),
> + IIO_EV_DIR_NONE,
> + IIO_EV_TYPE_CHANGE, 0, 0, 0),
> data->timestamp);
> }
>
> @@ -1054,17 +1049,19 @@ static irqreturn_t mma9553_event_handler(int irq, void *private)
> 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),
> + 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),
> + ev_activity->info->mod,
> + IIO_EV_DIR_RISING,
> + IIO_EV_TYPE_THRESH, 0, 0,
> + 0),
> data->timestamp);
> }
> mutex_unlock(&data->mutex);
> @@ -1159,7 +1156,6 @@ static int mma9553_probe(struct i2c_client *client,
> client->irq);
> goto out_poweroff;
> }
> -
> }
>
> ret = iio_device_register(indio_dev);
>
On 13/04/15 16:41, Irina Tirdea wrote:
> Fix checkpatch.pl --strict check:
> CHECK: struct mutex definition without comment
> + struct mutex mutex;
>
> Signed-off-by: Irina Tirdea <[email protected]>
Applied.
Thanks,
> ---
> drivers/iio/accel/mma9553.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 08f28c3..a605280 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -182,6 +182,10 @@ struct mma9553_conf_regs {
>
> struct mma9553_data {
> struct i2c_client *client;
> + /*
> + * 1. Serialize access to HW (requested by mma9551_core API).
> + * 2. Serialize sequences that power on/off the device and access HW.
> + */
> struct mutex mutex;
> struct mma9553_conf_regs conf;
> struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];
>
On 13/04/15 16:41, Irina Tirdea wrote:
> Use unsigned counters instead of signed when all the
> possible values are positive.
>
> Signed-off-by: Irina Tirdea <[email protected]>
> Suggested-by: Hartmut Knaack <[email protected]>
Hmm. I'm actually going to drop this one. I'm not sure
there is a significant gain to be had by using minimal
sized integers as loop iterators.
int is just fine.
Jonathan
> ---
> drivers/iio/accel/mma9551_core.c | 12 +++++++-----
> drivers/iio/accel/mma9553.c | 8 ++++----
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index c34c5ce..44b8243 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -115,8 +115,8 @@ struct mma9551_version_info {
>
> static int mma9551_transfer(struct i2c_client *client,
> u8 app_id, u8 command, u16 offset,
> - u8 *inbytes, int num_inbytes,
> - u8 *outbytes, int num_outbytes)
> + u8 *inbytes, u8 num_inbytes,
> + u8 *outbytes, u8 num_outbytes)
> {
> struct mma9551_mbox_request req;
> struct mma9551_mbox_response rsp;
> @@ -387,7 +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 ret;
> + u8 i;
> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
>
> if (len > ARRAY_SIZE(be_buf)) {
> @@ -426,7 +427,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 ret;
> + u8 i;
> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
>
> if (len > ARRAY_SIZE(be_buf)) {
> @@ -465,7 +467,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;
> + u8 i;
> __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
>
> if (len > ARRAY_SIZE(be_buf)) {
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index a605280..ad587ec 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -189,7 +189,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:
> @@ -230,7 +230,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++) {
> @@ -244,7 +244,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 &&
> @@ -259,7 +259,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 &&
>
> -----Original Message-----
> From: Jonathan Cameron [mailto:[email protected]]
> Sent: 14 June, 2015 18:01
> To: Tirdea, Irina; [email protected]; Hartmut Knaack
> Cc: [email protected]; Dogaru, Vlad
> Subject: Re: [PATCH v2 14/17] iio: accel: mma9551_core: use size in words for word buffers
>
> On 29/04/15 13:20, Tirdea, Irina wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jonathan Cameron [mailto:[email protected]]
> >> Sent: 26 April, 2015 22:05
> >> To: Tirdea, Irina; [email protected]; Hartmut Knaack
> >> Cc: [email protected]; Dogaru, Vlad
> >> Subject: Re: [PATCH v2 14/17] iio: accel: mma9551_core: use size in words for word buffers
> >>
> >> On 13/04/15 16:41, Irina Tirdea wrote:
> >>> 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 <[email protected]>
> >> Lots of merge issues by this point in the series so I'll hold this one at
> >> least until the fixes filter through.
> >>
> >> Remind me if I seem to have forgotten (it wouldn't be the first time :)
> >>
> > Sure, I'll keep an eye on when the fixes will be merged in togreg branch.
> >
> They are there now, so applied to the togreg branch of iio.git, initially pushed
> out as testing for the autobuilders to play with it.
>
> Now looking at the merge window in 3 months time though to go upstream unfortunately.
> Sorry for the delay, been a busy couple of months!
>
No problem, thanks for keeping an eye on when the fixes got merged.
Thanks,
Irina
> Jonathan
> > Thanks,
> > Irina
> >
> >> Jonathan
> >>> ---
> >>> drivers/iio/accel/mma9551_core.c | 27 ++++++++++++---------------
> >>> drivers/iio/accel/mma9553.c | 9 ++++++---
> >>> 2 files changed, 18 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> >>> index 2fd2a99..583660b 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,20 +388,19 @@ 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);
> >>> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> >>>
> >>> - if (len_words > ARRAY_SIZE(be_buf)) {
> >>> + 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;
> >>> @@ -413,7 +412,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).
> >>> @@ -428,20 +427,19 @@ 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);
> >>> __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> >>>
> >>> - if (len_words > ARRAY_SIZE(be_buf)) {
> >>> + 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;
> >>> @@ -453,7 +451,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).
> >>> @@ -468,19 +466,18 @@ 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);
> >>> __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
> >>>
> >>> - if (len_words > ARRAY_SIZE(be_buf)) {
> >>> + 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 8bfc618..06c8707 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");
> >>> @@ -397,7 +398,8 @@ 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), (u16 *) &data->conf);
> >>> + sizeof(data->conf) / sizeof(u16),
> >>> + (u16 *)&data->conf);
> >>> if (ret < 0) {
> >>> dev_err(&data->client->dev,
> >>> "failed to read configuration registers\n");
> >>> @@ -430,7 +432,8 @@ static int mma9553_init(struct mma9553_data *data)
> >>> ret =
> >>> mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> >>> MMA9553_REG_CONF_SLEEPMIN,
> >>> - sizeof(data->conf), (u16 *) &data->conf);
> >>> + sizeof(data->conf) / sizeof(u16),
> >>> + (u16 *)&data->conf);
> >>> if (ret < 0) {
> >>> dev_err(&data->client->dev,
> >>> "failed to write configuration registers\n");
> >>>
> >
> -----Original Message-----
> From: Jonathan Cameron [mailto:[email protected]]
> Sent: 14 June, 2015 18:04
> To: Tirdea, Irina; [email protected]; Hartmut Knaack
> Cc: [email protected]; Dogaru, Vlad
> Subject: Re: [PATCH v2 17/17] iio: accel: mma9553: use unsigned counters
>
> On 13/04/15 16:41, Irina Tirdea wrote:
> > Use unsigned counters instead of signed when all the
> > possible values are positive.
> >
> > Signed-off-by: Irina Tirdea <[email protected]>
> > Suggested-by: Hartmut Knaack <[email protected]>
> Hmm. I'm actually going to drop this one. I'm not sure
> there is a significant gain to be had by using minimal
> sized integers as loop iterators.
>
> int is just fine.
>
Fair enough, wasn't sure about this one either.
Thanks,
Irina
> Jonathan
> > ---
> > drivers/iio/accel/mma9551_core.c | 12 +++++++-----
> > drivers/iio/accel/mma9553.c | 8 ++++----
> > 2 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index c34c5ce..44b8243 100644
> > --- a/drivers/iio/accel/mma9551_core.c
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -115,8 +115,8 @@ struct mma9551_version_info {
> >
> > static int mma9551_transfer(struct i2c_client *client,
> > u8 app_id, u8 command, u16 offset,
> > - u8 *inbytes, int num_inbytes,
> > - u8 *outbytes, int num_outbytes)
> > + u8 *inbytes, u8 num_inbytes,
> > + u8 *outbytes, u8 num_outbytes)
> > {
> > struct mma9551_mbox_request req;
> > struct mma9551_mbox_response rsp;
> > @@ -387,7 +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 ret;
> > + u8 i;
> > __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> >
> > if (len > ARRAY_SIZE(be_buf)) {
> > @@ -426,7 +427,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 ret;
> > + u8 i;
> > __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS / 2];
> >
> > if (len > ARRAY_SIZE(be_buf)) {
> > @@ -465,7 +467,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;
> > + u8 i;
> > __be16 be_buf[(MMA9551_MAX_MAILBOX_DATA_REGS - 1) / 2];
> >
> > if (len > ARRAY_SIZE(be_buf)) {
> > diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> > index a605280..ad587ec 100644
> > --- a/drivers/iio/accel/mma9553.c
> > +++ b/drivers/iio/accel/mma9553.c
> > @@ -189,7 +189,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:
> > @@ -230,7 +230,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++) {
> > @@ -244,7 +244,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 &&
> > @@ -259,7 +259,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 &&
> >