Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/10] iio: adis: cleanups and fixes
@ 2019-11-01  9:34 Alexandru Ardelean
  2019-11-01  9:34 ` [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero Alexandru Ardelean
                   ` (9 more replies)
  0 siblings, 10 replies; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:34 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

This patch-set addresses some issues, where [when making the functions
inline], the GCC [especially for x86_64] compilers start to report
`-Wmaybe-unitialized` warnings/errors.

The warnings are valid from a general stand-point, but are also
false-positives, as the ADIS read functions return 0 or negative values. So
for most cases `ret < 0` is fine [as is done in many drivers].

To appease the compiler, this changeset converts
 `if (ret < 0)` -> `if (ret)` [which is also a valid check]

There are also 2 checkpatch complaints addressed.

And lastly, 3 minor fixes. In some cases, the value of the ADIS read
function should be checked for 0 (success) and only then assign the read
buffer/value. These also contain Fixes tags.

Alexandru Ardelean (10):
  iio: gyro: adis16136: check ret val for non-zero vs less-than-zero
  iio: imu: adis16400: check ret val for non-zero vs less-than-zero
  iio: imu: adis16460: check ret val for non-zero vs less-than-zero
  iio: imu: adis16480: check ret val for non-zero vs less-than-zero
  iio: imu: adis: check ret val for non-zero vs less-than-zero
  iio: imu: adis16480: fix indentation of return statement
  iio: imu: adis16480: prefer `unsigned int` over `unsigned`
  iio: imu: adis16480: assign bias value only if operation succeeded
  iio: imu: adis: assign read val in debugfs hook only if op successful
  iio: imu: adis: assign value only if return code zero in read funcs

 drivers/iio/gyro/adis16136.c | 24 +++++++++++------------
 drivers/iio/imu/adis.c       |  5 +++--
 drivers/iio/imu/adis16400.c  | 22 ++++++++++-----------
 drivers/iio/imu/adis16460.c  |  8 ++++----
 drivers/iio/imu/adis16480.c  | 38 +++++++++++++++++++-----------------
 include/linux/iio/imu/adis.h |  6 ++++--
 6 files changed, 54 insertions(+), 49 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
@ 2019-11-01  9:34 ` Alexandru Ardelean
  2019-11-03 10:15   ` Jonathan Cameron
  2019-11-01  9:34 ` [PATCH 02/10] iio: imu: adis16400: " Alexandru Ardelean
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:34 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

The ADIS library functions return zero on success, and negative values for
error. Positive values aren't returned, but we only care about the success
value (which is zero).

This change is mostly needed so that the compiler won't make any inferences
about some about values being potentially un-initialized. This only
triggers after making some functions inline, because the compiler can
better follow return paths.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/gyro/adis16136.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index 5bec7ad53d8b..d637d52d051a 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -80,19 +80,19 @@ static ssize_t adis16136_show_serial(struct file *file,
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SERIAL_NUM,
 		&serial);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_LOT1, &lot1);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_LOT2, &lot2);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_LOT3, &lot3);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
@@ -116,7 +116,7 @@ static int adis16136_show_product_id(void *arg, u64 *val)
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_PROD_ID,
 		&prod_id);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = prod_id;
@@ -134,7 +134,7 @@ static int adis16136_show_flash_count(void *arg, u64 *val)
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_FLASH_CNT,
 		&flash_count);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = flash_count;
@@ -191,7 +191,7 @@ static int adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
 	int ret;
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*freq = 32768 / (t + 1);
@@ -228,7 +228,7 @@ static ssize_t adis16136_read_frequency(struct device *dev,
 	int ret;
 
 	ret = adis16136_get_freq(adis16136, &freq);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	return sprintf(buf, "%d\n", freq);
@@ -256,7 +256,7 @@ static int adis16136_set_filter(struct iio_dev *indio_dev, int val)
 	int i, ret;
 
 	ret = adis16136_get_freq(adis16136, &freq);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	for (i = ARRAY_SIZE(adis16136_3db_divisors) - 1; i >= 1; i--) {
@@ -277,11 +277,11 @@ static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
 	mutex_lock(&indio_dev->mlock);
 
 	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, &val16);
-	if (ret < 0)
+	if (ret)
 		goto err_unlock;
 
 	ret = adis16136_get_freq(adis16136, &freq);
-	if (ret < 0)
+	if (ret)
 		goto err_unlock;
 
 	*val = freq / adis16136_3db_divisors[val16 & 0x07];
@@ -318,7 +318,7 @@ static int adis16136_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_CALIBBIAS:
 		ret = adis_read_reg_32(&adis16136->adis,
 			ADIS16136_REG_GYRO_OFF2, &val32);
-		if (ret < 0)
+		if (ret)
 			return ret;
 
 		*val = sign_extend32(val32, 31);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 02/10] iio: imu: adis16400: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
  2019-11-01  9:34 ` [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero Alexandru Ardelean
@ 2019-11-01  9:34 ` " Alexandru Ardelean
  2019-11-03 10:21   ` Jonathan Cameron
  2019-11-01  9:34 ` [PATCH 03/10] iio: imu: adis16460: " Alexandru Ardelean
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:34 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

The ADIS library functions return zero on success, and negative values for
error. Positive values aren't returned, but we only care about the success
value (which is zero).

This change is mostly needed so that the compiler won't make any inferences
about some about values being potentially un-initialized. This only
triggers after making some functions inline, because the compiler can
better follow return paths.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16400.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 0575ff706bd4..44e46dc96e00 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -217,16 +217,16 @@ static ssize_t adis16400_show_serial_number(struct file *file,
 	int ret;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16334_LOT_ID1, &lot1);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16334_LOT_ID2, &lot2);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16334_SERIAL_NUMBER,
 			&serial_number);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	len = snprintf(buf, sizeof(buf), "%.4x-%.4x-%.4x\n", lot1, lot2,
@@ -249,7 +249,7 @@ static int adis16400_show_product_id(void *arg, u64 *val)
 	int ret;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16400_PRODUCT_ID, &prod_id);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = prod_id;
@@ -266,7 +266,7 @@ static int adis16400_show_flash_count(void *arg, u64 *val)
 	int ret;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16400_FLASH_CNT, &flash_count);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = flash_count;
@@ -327,7 +327,7 @@ static int adis16334_get_freq(struct adis16400_state *st)
 	uint16_t t;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	t >>= ADIS16334_RATE_DIV_SHIFT;
@@ -359,7 +359,7 @@ static int adis16400_get_freq(struct adis16400_state *st)
 	uint16_t t;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	sps = (t & ADIS16400_SMPL_PRD_TIME_BASE) ? 52851 : 1638404;
@@ -416,7 +416,7 @@ static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
 	}
 
 	ret = adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
@@ -615,7 +615,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 		ret = adis_read_reg_16(&st->adis,
 						ADIS16400_SENS_AVG,
 						&val16);
-		if (ret < 0) {
+		if (ret) {
 			mutex_unlock(&indio_dev->mlock);
 			return ret;
 		}
@@ -626,12 +626,12 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 			*val2 = (ret % 1000) * 1000;
 		}
 		mutex_unlock(&indio_dev->mlock);
-		if (ret < 0)
+		if (ret)
 			return ret;
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		ret = st->variant->get_freq(st);
-		if (ret < 0)
+		if (ret)
 			return ret;
 		*val = ret / 1000;
 		*val2 = (ret % 1000) * 1000;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 03/10] iio: imu: adis16460: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
  2019-11-01  9:34 ` [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero Alexandru Ardelean
  2019-11-01  9:34 ` [PATCH 02/10] iio: imu: adis16400: " Alexandru Ardelean
@ 2019-11-01  9:34 ` " Alexandru Ardelean
  2019-11-03 10:25   ` Jonathan Cameron
  2019-11-01  9:34 ` [PATCH 04/10] iio: imu: adis16480: " Alexandru Ardelean
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:34 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

The ADIS library functions return zero on success, and negative values for
error. Positive values aren't returned, but we only care about the success
value (which is zero).

This change is mostly needed so that the compiler won't make any inferences
about some about values being potentially un-initialized. This only
triggers after making some functions inline, because the compiler can
better follow return paths.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16460.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index 6aed9e84abbf..b55812521537 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -80,7 +80,7 @@ static int adis16460_show_serial_number(void *arg, u64 *val)
 
 	ret = adis_read_reg_16(&adis16460->adis, ADIS16460_REG_SERIAL_NUM,
 		&serial);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = serial;
@@ -98,7 +98,7 @@ static int adis16460_show_product_id(void *arg, u64 *val)
 
 	ret = adis_read_reg_16(&adis16460->adis, ADIS16460_REG_PROD_ID,
 		&prod_id);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = prod_id;
@@ -116,7 +116,7 @@ static int adis16460_show_flash_count(void *arg, u64 *val)
 
 	ret = adis_read_reg_32(&adis16460->adis, ADIS16460_REG_FLASH_CNT,
 		&flash_count);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = flash_count;
@@ -176,7 +176,7 @@ static int adis16460_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
 	unsigned int freq;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16460_REG_DEC_RATE, &t);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	freq = 2048000 / (t + 1);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 04/10] iio: imu: adis16480: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
                   ` (2 preceding siblings ...)
  2019-11-01  9:34 ` [PATCH 03/10] iio: imu: adis16460: " Alexandru Ardelean
@ 2019-11-01  9:34 ` " Alexandru Ardelean
  2019-11-03 10:27   ` Jonathan Cameron
  2019-11-01  9:35 ` [PATCH 05/10] iio: imu: adis: " Alexandru Ardelean
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:34 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

The ADIS library functions return zero on success, and negative values for
error. Positive values aren't returned, but we only care about the success
value (which is zero).

This change is mostly needed so that the compiler won't make any inferences
about some about values being potentially un-initialized. This only
triggers after making some functions inline, because the compiler can
better follow return paths.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index b99d73887c9f..86801f3c5f0d 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -181,7 +181,7 @@ static ssize_t adis16480_show_firmware_revision(struct file *file,
 	int ret;
 
 	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_FIRM_REV, &rev);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	len = scnprintf(buf, sizeof(buf), "%x.%x\n", rev >> 8, rev & 0xff);
@@ -206,11 +206,11 @@ static ssize_t adis16480_show_firmware_date(struct file *file,
 	int ret;
 
 	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_FIRM_Y, &year);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_FIRM_DM, &md);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	len = snprintf(buf, sizeof(buf), "%.2x-%.2x-%.4x\n",
@@ -234,7 +234,7 @@ static int adis16480_show_serial_number(void *arg, u64 *val)
 
 	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_SERIAL_NUM,
 		&serial);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = serial;
@@ -252,7 +252,7 @@ static int adis16480_show_product_id(void *arg, u64 *val)
 
 	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_PROD_ID,
 		&prod_id);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = prod_id;
@@ -270,7 +270,7 @@ static int adis16480_show_flash_count(void *arg, u64 *val)
 
 	ret = adis_read_reg_32(&adis16480->adis, ADIS16480_REG_FLASH_CNT,
 		&flash_count);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*val = flash_count;
@@ -359,7 +359,7 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
 		reg = ADIS16480_REG_DEC_RATE;
 
 	ret = adis_read_reg_16(&st->adis, reg, &t);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	/*
@@ -462,7 +462,7 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev,
 			ret = -EINVAL;
 	}
 
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	return IIO_VAL_INT;
@@ -489,7 +489,7 @@ static int adis16480_get_calibscale(struct iio_dev *indio_dev,
 	int ret;
 
 	ret = adis_read_reg_16(&st->adis, reg, &val16);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*scale = sign_extend32(val16, 15);
@@ -535,7 +535,7 @@ static int adis16480_get_filter_freq(struct iio_dev *indio_dev,
 	enable_mask = BIT(offset + 2);
 
 	ret = adis_read_reg_16(&st->adis, reg, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	if (!(val & enable_mask))
@@ -561,7 +561,7 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
 	enable_mask = BIT(offset + 2);
 
 	ret = adis_read_reg_16(&st->adis, reg, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	if (freq == 0) {
@@ -937,7 +937,7 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
 	int ret;
 
 	ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	val &= ~ADIS16480_DRDY_EN_MSK;
@@ -1115,7 +1115,7 @@ static int adis16480_ext_clk_config(struct adis16480 *st,
 	int ret;
 
 	ret = adis_read_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	pin = adis16480_of_get_ext_clk_pin(st, of_node);
@@ -1141,7 +1141,7 @@ static int adis16480_ext_clk_config(struct adis16480 *st,
 	val |= mode;
 
 	ret = adis_write_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL, val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	return clk_prepare_enable(st->ext_clk);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 05/10] iio: imu: adis: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
                   ` (3 preceding siblings ...)
  2019-11-01  9:34 ` [PATCH 04/10] iio: imu: adis16480: " Alexandru Ardelean
@ 2019-11-01  9:35 ` " Alexandru Ardelean
  2019-11-03 10:31   ` Jonathan Cameron
  2019-11-01  9:35 ` [PATCH 06/10] iio: imu: adis16480: fix indentation of return statement Alexandru Ardelean
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:35 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

The ADIS library functions return zero on success, and negative values for
error. Positive values aren't returned, but we only care about the success
value (which is zero).

This change is mostly needed so that the compiler won't make any inferences
about some about values being potentially un-initialized. This only
triggers after making some functions inline, because the compiler can
better follow return paths.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 1631c255deab..dc2f9e061d98 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -286,7 +286,7 @@ int adis_check_status(struct adis *adis)
 	int i;
 
 	ret = adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	status &= adis->data->status_error_mask;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 06/10] iio: imu: adis16480: fix indentation of return statement
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
                   ` (4 preceding siblings ...)
  2019-11-01  9:35 ` [PATCH 05/10] iio: imu: adis: " Alexandru Ardelean
@ 2019-11-01  9:35 ` Alexandru Ardelean
  2019-11-03 10:35   ` Jonathan Cameron
  2019-11-01  9:35 ` [PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned` Alexandru Ardelean
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:35 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

This is just a minor indentation/alignment fix for the default case of a
switch statement.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 86801f3c5f0d..d199d3d3c4bd 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -459,7 +459,7 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev,
 		*bias = sign_extend32(val32, 31);
 		break;
 	default:
-			ret = -EINVAL;
+		ret = -EINVAL;
 	}
 
 	if (ret)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned`
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
                   ` (5 preceding siblings ...)
  2019-11-01  9:35 ` [PATCH 06/10] iio: imu: adis16480: fix indentation of return statement Alexandru Ardelean
@ 2019-11-01  9:35 ` Alexandru Ardelean
  2019-11-03 10:36   ` Jonathan Cameron
  2019-11-01  9:35 ` [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded Alexandru Ardelean
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:35 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

This is a typical checkpatch warning. The change just renames the type of
the `freq` var to `unsigned int`.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index d199d3d3c4bd..b9e2695bcfad 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -350,7 +350,7 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
 	struct adis16480 *st = iio_priv(indio_dev);
 	uint16_t t;
 	int ret;
-	unsigned freq;
+	unsigned int freq;
 	unsigned int reg;
 
 	if (st->clk_mode == ADIS16480_CLK_PPS)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
                   ` (6 preceding siblings ...)
  2019-11-01  9:35 ` [PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned` Alexandru Ardelean
@ 2019-11-01  9:35 ` Alexandru Ardelean
  2019-11-03 10:41   ` Jonathan Cameron
  2019-11-01  9:35 ` [PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful Alexandru Ardelean
  2019-11-01  9:35 ` [PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs Alexandru Ardelean
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:35 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

This was found only after the whole thing with the inline functions, but
the compiler actually found something. The value of the `bias` (in
adis16480_get_calibbias()) should only be set if the read operation was
successful.

Fixes: 2f3abe6cbb6c9 ("iio:imu: Add support for the ADIS16480 and similar IMUs")
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index b9e2695bcfad..c0e7e768be41 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -451,12 +451,14 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev,
 	case IIO_MAGN:
 	case IIO_PRESSURE:
 		ret = adis_read_reg_16(&st->adis, reg, &val16);
-		*bias = sign_extend32(val16, 15);
+		if (ret == 0)
+			*bias = sign_extend32(val16, 15);
 		break;
 	case IIO_ANGL_VEL:
 	case IIO_ACCEL:
 		ret = adis_read_reg_32(&st->adis, reg, &val32);
-		*bias = sign_extend32(val32, 31);
+		if (ret == 0)
+			*bias = sign_extend32(val32, 31);
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
                   ` (7 preceding siblings ...)
  2019-11-01  9:35 ` [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded Alexandru Ardelean
@ 2019-11-01  9:35 ` Alexandru Ardelean
  2019-11-03 10:46   ` Jonathan Cameron
  2019-11-01  9:35 ` [PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs Alexandru Ardelean
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:35 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

This was also caught by the `-Wmaybe-uninitialized` warning, which
(ironically as-is) it makes quite a lot of sense to do for this.

Fixes: 78026a6fde8f7 ("iio:imu:adis: Add debugfs register access support")
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index dc2f9e061d98..85de565a4e80 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -229,7 +229,8 @@ int adis_debugfs_reg_access(struct iio_dev *indio_dev,
 		int ret;
 
 		ret = adis_read_reg_16(adis, reg, &val16);
-		*readval = val16;
+		if (ret == 0)
+			*readval = val16;
 
 		return ret;
 	} else {
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs
  2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
                   ` (8 preceding siblings ...)
  2019-11-01  9:35 ` [PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful Alexandru Ardelean
@ 2019-11-01  9:35 ` Alexandru Ardelean
  2019-11-03 10:49   ` Jonathan Cameron
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Ardelean @ 2019-11-01  9:35 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, dragos.bogdan, Alexandru Ardelean

The inline read functions in the ADIS library don't check the return value
of the `adis_read_reg()` function and assign the value of `tmp` regardless.

Fix this by checking if return value is zero and only then assigning the
value of `tmp`.

Fixes: 57a1228a06b7a ("iio:imu:adis: Add support for 32bit registers")
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/iio/imu/adis.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 4c53815bb729..92aae14593bf 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -129,7 +129,8 @@ static inline int adis_read_reg_16(struct adis *adis, unsigned int reg,
 	int ret;
 
 	ret = adis_read_reg(adis, reg, &tmp, 2);
-	*val = tmp;
+	if (ret == 0)
+		*val = tmp;
 
 	return ret;
 }
@@ -147,7 +148,8 @@ static inline int adis_read_reg_32(struct adis *adis, unsigned int reg,
 	int ret;
 
 	ret = adis_read_reg(adis, reg, &tmp, 4);
-	*val = tmp;
+	if (ret == 0)
+		*val = tmp;
 
 	return ret;
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 ` [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero Alexandru Ardelean
@ 2019-11-03 10:15   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:15 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:34:56 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The ADIS library functions return zero on success, and negative values for
> error. Positive values aren't returned, but we only care about the success
> value (which is zero).
> 
> This change is mostly needed so that the compiler won't make any inferences
> about some about values being potentially un-initialized. This only
> triggers after making some functions inline, because the compiler can
> better follow return paths.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to work their magic.

Thanks,

Jonathan

> ---
>  drivers/iio/gyro/adis16136.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
> index 5bec7ad53d8b..d637d52d051a 100644
> --- a/drivers/iio/gyro/adis16136.c
> +++ b/drivers/iio/gyro/adis16136.c
> @@ -80,19 +80,19 @@ static ssize_t adis16136_show_serial(struct file *file,
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SERIAL_NUM,
>  		&serial);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_LOT1, &lot1);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_LOT2, &lot2);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_LOT3, &lot3);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> @@ -116,7 +116,7 @@ static int adis16136_show_product_id(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_PROD_ID,
>  		&prod_id);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = prod_id;
> @@ -134,7 +134,7 @@ static int adis16136_show_flash_count(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_FLASH_CNT,
>  		&flash_count);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = flash_count;
> @@ -191,7 +191,7 @@ static int adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
>  	int ret;
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*freq = 32768 / (t + 1);
> @@ -228,7 +228,7 @@ static ssize_t adis16136_read_frequency(struct device *dev,
>  	int ret;
>  
>  	ret = adis16136_get_freq(adis16136, &freq);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	return sprintf(buf, "%d\n", freq);
> @@ -256,7 +256,7 @@ static int adis16136_set_filter(struct iio_dev *indio_dev, int val)
>  	int i, ret;
>  
>  	ret = adis16136_get_freq(adis16136, &freq);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	for (i = ARRAY_SIZE(adis16136_3db_divisors) - 1; i >= 1; i--) {
> @@ -277,11 +277,11 @@ static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
>  	mutex_lock(&indio_dev->mlock);
>  
>  	ret = adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, &val16);
> -	if (ret < 0)
> +	if (ret)
>  		goto err_unlock;
>  
>  	ret = adis16136_get_freq(adis16136, &freq);
> -	if (ret < 0)
> +	if (ret)
>  		goto err_unlock;
>  
>  	*val = freq / adis16136_3db_divisors[val16 & 0x07];
> @@ -318,7 +318,7 @@ static int adis16136_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		ret = adis_read_reg_32(&adis16136->adis,
>  			ADIS16136_REG_GYRO_OFF2, &val32);
> -		if (ret < 0)
> +		if (ret)
>  			return ret;
>  
>  		*val = sign_extend32(val32, 31);


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 02/10] iio: imu: adis16400: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 ` [PATCH 02/10] iio: imu: adis16400: " Alexandru Ardelean
@ 2019-11-03 10:21   ` Jonathan Cameron
  2019-11-04  7:40     ` Ardelean, Alexandru
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:21 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:34:57 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The ADIS library functions return zero on success, and negative values for
> error. Positive values aren't returned, but we only care about the success
> value (which is zero).
> 
> This change is mostly needed so that the compiler won't make any inferences
> about some about values being potentially un-initialized. This only
> triggers after making some functions inline, because the compiler can
> better follow return paths.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied,

An observation whilst I was looking at the driver though...

It has some cases of goto label; where the label doesn't then do anything
in *_initial_setup.  Direct returns would be a bit neater.

Really minor point but if you happen to be touching that driver again
soon nice to tidy up ;)

Thanks,

Jonathan


> ---
>  drivers/iio/imu/adis16400.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index 0575ff706bd4..44e46dc96e00 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -217,16 +217,16 @@ static ssize_t adis16400_show_serial_number(struct file *file,
>  	int ret;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16334_LOT_ID1, &lot1);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16334_LOT_ID2, &lot2);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16334_SERIAL_NUMBER,
>  			&serial_number);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	len = snprintf(buf, sizeof(buf), "%.4x-%.4x-%.4x\n", lot1, lot2,
> @@ -249,7 +249,7 @@ static int adis16400_show_product_id(void *arg, u64 *val)
>  	int ret;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16400_PRODUCT_ID, &prod_id);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = prod_id;
> @@ -266,7 +266,7 @@ static int adis16400_show_flash_count(void *arg, u64 *val)
>  	int ret;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16400_FLASH_CNT, &flash_count);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = flash_count;
> @@ -327,7 +327,7 @@ static int adis16334_get_freq(struct adis16400_state *st)
>  	uint16_t t;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	t >>= ADIS16334_RATE_DIV_SHIFT;
> @@ -359,7 +359,7 @@ static int adis16400_get_freq(struct adis16400_state *st)
>  	uint16_t t;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	sps = (t & ADIS16400_SMPL_PRD_TIME_BASE) ? 52851 : 1638404;
> @@ -416,7 +416,7 @@ static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
>  	}
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
> @@ -615,7 +615,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
>  		ret = adis_read_reg_16(&st->adis,
>  						ADIS16400_SENS_AVG,
>  						&val16);
> -		if (ret < 0) {
> +		if (ret) {
>  			mutex_unlock(&indio_dev->mlock);
>  			return ret;
>  		}
> @@ -626,12 +626,12 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
>  			*val2 = (ret % 1000) * 1000;
>  		}
>  		mutex_unlock(&indio_dev->mlock);
> -		if (ret < 0)
> +		if (ret)
>  			return ret;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = st->variant->get_freq(st);
> -		if (ret < 0)
> +		if (ret)
>  			return ret;
>  		*val = ret / 1000;
>  		*val2 = (ret % 1000) * 1000;


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 03/10] iio: imu: adis16460: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 ` [PATCH 03/10] iio: imu: adis16460: " Alexandru Ardelean
@ 2019-11-03 10:25   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:25 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:34:58 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The ADIS library functions return zero on success, and negative values for
> error. Positive values aren't returned, but we only care about the success
> value (which is zero).
> 
> This change is mostly needed so that the compiler won't make any inferences
> about some about values being potentially un-initialized. This only
> triggers after making some functions inline, because the compiler can
> better follow return paths.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied thanks.

Jonathan

> ---
>  drivers/iio/imu/adis16460.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
> index 6aed9e84abbf..b55812521537 100644
> --- a/drivers/iio/imu/adis16460.c
> +++ b/drivers/iio/imu/adis16460.c
> @@ -80,7 +80,7 @@ static int adis16460_show_serial_number(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_16(&adis16460->adis, ADIS16460_REG_SERIAL_NUM,
>  		&serial);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = serial;
> @@ -98,7 +98,7 @@ static int adis16460_show_product_id(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_16(&adis16460->adis, ADIS16460_REG_PROD_ID,
>  		&prod_id);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = prod_id;
> @@ -116,7 +116,7 @@ static int adis16460_show_flash_count(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_32(&adis16460->adis, ADIS16460_REG_FLASH_CNT,
>  		&flash_count);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = flash_count;
> @@ -176,7 +176,7 @@ static int adis16460_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
>  	unsigned int freq;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16460_REG_DEC_RATE, &t);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	freq = 2048000 / (t + 1);


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 04/10] iio: imu: adis16480: check ret val for non-zero vs less-than-zero
  2019-11-01  9:34 ` [PATCH 04/10] iio: imu: adis16480: " Alexandru Ardelean
@ 2019-11-03 10:27   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:27 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:34:59 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The ADIS library functions return zero on success, and negative values for
> error. Positive values aren't returned, but we only care about the success
> value (which is zero).
> 
> This change is mostly needed so that the compiler won't make any inferences
> about some about values being potentially un-initialized. This only
> triggers after making some functions inline, because the compiler can
> better follow return paths.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index b99d73887c9f..86801f3c5f0d 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -181,7 +181,7 @@ static ssize_t adis16480_show_firmware_revision(struct file *file,
>  	int ret;
>  
>  	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_FIRM_REV, &rev);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	len = scnprintf(buf, sizeof(buf), "%x.%x\n", rev >> 8, rev & 0xff);
> @@ -206,11 +206,11 @@ static ssize_t adis16480_show_firmware_date(struct file *file,
>  	int ret;
>  
>  	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_FIRM_Y, &year);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_FIRM_DM, &md);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	len = snprintf(buf, sizeof(buf), "%.2x-%.2x-%.4x\n",
> @@ -234,7 +234,7 @@ static int adis16480_show_serial_number(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_SERIAL_NUM,
>  		&serial);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = serial;
> @@ -252,7 +252,7 @@ static int adis16480_show_product_id(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_16(&adis16480->adis, ADIS16480_REG_PROD_ID,
>  		&prod_id);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = prod_id;
> @@ -270,7 +270,7 @@ static int adis16480_show_flash_count(void *arg, u64 *val)
>  
>  	ret = adis_read_reg_32(&adis16480->adis, ADIS16480_REG_FLASH_CNT,
>  		&flash_count);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*val = flash_count;
> @@ -359,7 +359,7 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
>  		reg = ADIS16480_REG_DEC_RATE;
>  
>  	ret = adis_read_reg_16(&st->adis, reg, &t);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	/*
> @@ -462,7 +462,7 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev,
>  			ret = -EINVAL;
>  	}
>  
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	return IIO_VAL_INT;
> @@ -489,7 +489,7 @@ static int adis16480_get_calibscale(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	ret = adis_read_reg_16(&st->adis, reg, &val16);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	*scale = sign_extend32(val16, 15);
> @@ -535,7 +535,7 @@ static int adis16480_get_filter_freq(struct iio_dev *indio_dev,
>  	enable_mask = BIT(offset + 2);
>  
>  	ret = adis_read_reg_16(&st->adis, reg, &val);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	if (!(val & enable_mask))
> @@ -561,7 +561,7 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
>  	enable_mask = BIT(offset + 2);
>  
>  	ret = adis_read_reg_16(&st->adis, reg, &val);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	if (freq == 0) {
> @@ -937,7 +937,7 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
>  	int ret;
>  
>  	ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	val &= ~ADIS16480_DRDY_EN_MSK;
> @@ -1115,7 +1115,7 @@ static int adis16480_ext_clk_config(struct adis16480 *st,
>  	int ret;
>  
>  	ret = adis_read_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL, &val);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	pin = adis16480_of_get_ext_clk_pin(st, of_node);
> @@ -1141,7 +1141,7 @@ static int adis16480_ext_clk_config(struct adis16480 *st,
>  	val |= mode;
>  
>  	ret = adis_write_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL, val);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	return clk_prepare_enable(st->ext_clk);


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 05/10] iio: imu: adis: check ret val for non-zero vs less-than-zero
  2019-11-01  9:35 ` [PATCH 05/10] iio: imu: adis: " Alexandru Ardelean
@ 2019-11-03 10:31   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:31 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:35:00 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The ADIS library functions return zero on success, and negative values for
> error. Positive values aren't returned, but we only care about the success
> value (which is zero).
> 
> This change is mostly needed so that the compiler won't make any inferences
> about some about values being potentially un-initialized. This only
> triggers after making some functions inline, because the compiler can
> better follow return paths.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

thanks,

Jonathan

> ---
>  drivers/iio/imu/adis.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 1631c255deab..dc2f9e061d98 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -286,7 +286,7 @@ int adis_check_status(struct adis *adis)
>  	int i;
>  
>  	ret = adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	status &= adis->data->status_error_mask;


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 06/10] iio: imu: adis16480: fix indentation of return statement
  2019-11-01  9:35 ` [PATCH 06/10] iio: imu: adis16480: fix indentation of return statement Alexandru Ardelean
@ 2019-11-03 10:35   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:35 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:35:01 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This is just a minor indentation/alignment fix for the default case of a
> switch statement.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied,

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index 86801f3c5f0d..d199d3d3c4bd 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -459,7 +459,7 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev,
>  		*bias = sign_extend32(val32, 31);
>  		break;
>  	default:
> -			ret = -EINVAL;
> +		ret = -EINVAL;
>  	}
>  
>  	if (ret)


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned`
  2019-11-01  9:35 ` [PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned` Alexandru Ardelean
@ 2019-11-03 10:36   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:36 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:35:02 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This is a typical checkpatch warning. The change just renames the type of
> the `freq` var to `unsigned int`.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.  Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index d199d3d3c4bd..b9e2695bcfad 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -350,7 +350,7 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
>  	struct adis16480 *st = iio_priv(indio_dev);
>  	uint16_t t;
>  	int ret;
> -	unsigned freq;
> +	unsigned int freq;
>  	unsigned int reg;
>  
>  	if (st->clk_mode == ADIS16480_CLK_PPS)


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded
  2019-11-01  9:35 ` [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded Alexandru Ardelean
@ 2019-11-03 10:41   ` Jonathan Cameron
  2019-11-04  8:50     ` Ardelean, Alexandru
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:41 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:35:03 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This was found only after the whole thing with the inline functions, but
> the compiler actually found something. The value of the `bias` (in
> adis16480_get_calibbias()) should only be set if the read operation was
> successful.
> 
> Fixes: 2f3abe6cbb6c9 ("iio:imu: Add support for the ADIS16480 and similar IMUs")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Hmm. It's not really a fix as such unless there is an in kernel consumer
that is using this and not checking the return value.   I thought about dropping
the fixes tag, but it is avoiding confusing the compiler so I suppose we might
as well leave it marked as a fix.  I will add a note that this doesn't cause
any known real problems and so probably isn't stable material.

Applied.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index b9e2695bcfad..c0e7e768be41 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -451,12 +451,14 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev,
>  	case IIO_MAGN:
>  	case IIO_PRESSURE:
>  		ret = adis_read_reg_16(&st->adis, reg, &val16);
> -		*bias = sign_extend32(val16, 15);
> +		if (ret == 0)
> +			*bias = sign_extend32(val16, 15);
>  		break;
>  	case IIO_ANGL_VEL:
>  	case IIO_ACCEL:
>  		ret = adis_read_reg_32(&st->adis, reg, &val32);
> -		*bias = sign_extend32(val32, 31);
> +		if (ret == 0)
> +			*bias = sign_extend32(val32, 31);
>  		break;
>  	default:
>  		ret = -EINVAL;


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful
  2019-11-01  9:35 ` [PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful Alexandru Ardelean
@ 2019-11-03 10:46   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:46 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:35:04 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This was also caught by the `-Wmaybe-uninitialized` warning, which
> (ironically as-is) it makes quite a lot of sense to do for this.
> 
> Fixes: 78026a6fde8f7 ("iio:imu:adis: Add debugfs register access support")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
This one is protected against any actual results in the caller of the
function, so I've added a not again to say this is not stable material.

Need to do this explicitly to avoid it getting picked up in efforts
to catch fixes that should have been tagged.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index dc2f9e061d98..85de565a4e80 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -229,7 +229,8 @@ int adis_debugfs_reg_access(struct iio_dev *indio_dev,
>  		int ret;
>  
>  		ret = adis_read_reg_16(adis, reg, &val16);
> -		*readval = val16;
> +		if (ret == 0)
> +			*readval = val16;
>  
>  		return ret;
>  	} else {


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs
  2019-11-01  9:35 ` [PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs Alexandru Ardelean
@ 2019-11-03 10:49   ` Jonathan Cameron
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-11-03 10:49 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, lars, Michael.Hennerich, dragos.bogdan

On Fri, 1 Nov 2019 11:35:05 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The inline read functions in the ADIS library don't check the return value
> of the `adis_read_reg()` function and assign the value of `tmp` regardless.
> 
> Fix this by checking if return value is zero and only then assigning the
> value of `tmp`.
> 
> Fixes: 57a1228a06b7a ("iio:imu:adis: Add support for 32bit registers")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Again, I added a note to say I don't think this is stable material.
As far as I know we don't have any actual bugs caused by this as we always
check the return value further up the stack.

Let me know if I've missed something.

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> ---
>  include/linux/iio/imu/adis.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 4c53815bb729..92aae14593bf 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -129,7 +129,8 @@ static inline int adis_read_reg_16(struct adis *adis, unsigned int reg,
>  	int ret;
>  
>  	ret = adis_read_reg(adis, reg, &tmp, 2);
> -	*val = tmp;
> +	if (ret == 0)
> +		*val = tmp;
>  
>  	return ret;
>  }
> @@ -147,7 +148,8 @@ static inline int adis_read_reg_32(struct adis *adis, unsigned int reg,
>  	int ret;
>  
>  	ret = adis_read_reg(adis, reg, &tmp, 4);
> -	*val = tmp;
> +	if (ret == 0)
> +		*val = tmp;
>  
>  	return ret;
>  }


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 02/10] iio: imu: adis16400: check ret val for non-zero vs less-than-zero
  2019-11-03 10:21   ` Jonathan Cameron
@ 2019-11-04  7:40     ` Ardelean, Alexandru
  0 siblings, 0 replies; 23+ messages in thread
From: Ardelean, Alexandru @ 2019-11-04  7:40 UTC (permalink / raw)
  To: jic23; +Cc: Hennerich, Michael, linux-kernel, linux-iio, Bogdan, Dragos, lars

On Sun, 2019-11-03 at 10:21 +0000, Jonathan Cameron wrote:
> On Fri, 1 Nov 2019 11:34:57 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > The ADIS library functions return zero on success, and negative values
> > for
> > error. Positive values aren't returned, but we only care about the
> > success
> > value (which is zero).
> > 
> > This change is mostly needed so that the compiler won't make any
> > inferences
> > about some about values being potentially un-initialized. This only
> > triggers after making some functions inline, because the compiler can
> > better follow return paths.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Applied,
> 
> An observation whilst I was looking at the driver though...
> 
> It has some cases of goto label; where the label doesn't then do anything
> in *_initial_setup.  Direct returns would be a bit neater.
> 
> Really minor point but if you happen to be touching that driver again
> soon nice to tidy up ;)

As you can probably guess, a lot of cleanups will be coming to the ADIS
family & library.
So, I'll try to include this into the next cleanup set.

I'll try to re-spin the ADIS lock cleanup.
We still need to do the devm_ variants + related cleanup.

Thanks
Alex

> 
> Thanks,
> 
> Jonathan
> 
> 
> > ---
> >  drivers/iio/imu/adis16400.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> > index 0575ff706bd4..44e46dc96e00 100644
> > --- a/drivers/iio/imu/adis16400.c
> > +++ b/drivers/iio/imu/adis16400.c
> > @@ -217,16 +217,16 @@ static ssize_t
> > adis16400_show_serial_number(struct file *file,
> >  	int ret;
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16334_LOT_ID1, &lot1);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16334_LOT_ID2, &lot2);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16334_SERIAL_NUMBER,
> >  			&serial_number);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	len = snprintf(buf, sizeof(buf), "%.4x-%.4x-%.4x\n", lot1, lot2,
> > @@ -249,7 +249,7 @@ static int adis16400_show_product_id(void *arg, u64
> > *val)
> >  	int ret;
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16400_PRODUCT_ID, &prod_id);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	*val = prod_id;
> > @@ -266,7 +266,7 @@ static int adis16400_show_flash_count(void *arg,
> > u64 *val)
> >  	int ret;
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16400_FLASH_CNT,
> > &flash_count);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	*val = flash_count;
> > @@ -327,7 +327,7 @@ static int adis16334_get_freq(struct
> > adis16400_state *st)
> >  	uint16_t t;
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	t >>= ADIS16334_RATE_DIV_SHIFT;
> > @@ -359,7 +359,7 @@ static int adis16400_get_freq(struct
> > adis16400_state *st)
> >  	uint16_t t;
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	sps = (t & ADIS16400_SMPL_PRD_TIME_BASE) ? 52851 : 1638404;
> > @@ -416,7 +416,7 @@ static int adis16400_set_filter(struct iio_dev
> > *indio_dev, int sps, int val)
> >  	}
> >  
> >  	ret = adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> >  
> >  	ret = adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
> > @@ -615,7 +615,7 @@ static int adis16400_read_raw(struct iio_dev
> > *indio_dev,
> >  		ret = adis_read_reg_16(&st->adis,
> >  						ADIS16400_SENS_AVG,
> >  						&val16);
> > -		if (ret < 0) {
> > +		if (ret) {
> >  			mutex_unlock(&indio_dev->mlock);
> >  			return ret;
> >  		}
> > @@ -626,12 +626,12 @@ static int adis16400_read_raw(struct iio_dev
> > *indio_dev,
> >  			*val2 = (ret % 1000) * 1000;
> >  		}
> >  		mutex_unlock(&indio_dev->mlock);
> > -		if (ret < 0)
> > +		if (ret)
> >  			return ret;
> >  		return IIO_VAL_INT_PLUS_MICRO;
> >  	case IIO_CHAN_INFO_SAMP_FREQ:
> >  		ret = st->variant->get_freq(st);
> > -		if (ret < 0)
> > +		if (ret)
> >  			return ret;
> >  		*val = ret / 1000;
> >  		*val2 = (ret % 1000) * 1000;

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded
  2019-11-03 10:41   ` Jonathan Cameron
@ 2019-11-04  8:50     ` Ardelean, Alexandru
  0 siblings, 0 replies; 23+ messages in thread
From: Ardelean, Alexandru @ 2019-11-04  8:50 UTC (permalink / raw)
  To: jic23; +Cc: Hennerich, Michael, linux-kernel, linux-iio, Bogdan, Dragos, lars

On Sun, 2019-11-03 at 10:41 +0000, Jonathan Cameron wrote:
> On Fri, 1 Nov 2019 11:35:03 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This was found only after the whole thing with the inline functions,
> > but
> > the compiler actually found something. The value of the `bias` (in
> > adis16480_get_calibbias()) should only be set if the read operation was
> > successful.
> > 
> > Fixes: 2f3abe6cbb6c9 ("iio:imu: Add support for the ADIS16480 and
> > similar IMUs")
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Hmm. It's not really a fix as such unless there is an in kernel consumer
> that is using this and not checking the return value.   I thought about
> dropping
> the fixes tag, but it is avoiding confusing the compiler so I suppose we
> might
> as well leave it marked as a fix.  I will add a note that this doesn't
> cause
> any known real problems and so probably isn't stable material.

Thanks.
No idea if it's worth having a Fixes tag or not.
I thought about adding it, since it seemed like a-bit-of-a-fix, and wasn't
sure, so I added. I guess after upstreaming things a bit, I started
wondering about these tags a bit more.

Alex

> 
> Applied.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/imu/adis16480.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index b9e2695bcfad..c0e7e768be41 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -451,12 +451,14 @@ static int adis16480_get_calibbias(struct iio_dev
> > *indio_dev,
> >  	case IIO_MAGN:
> >  	case IIO_PRESSURE:
> >  		ret = adis_read_reg_16(&st->adis, reg, &val16);
> > -		*bias = sign_extend32(val16, 15);
> > +		if (ret == 0)
> > +			*bias = sign_extend32(val16, 15);
> >  		break;
> >  	case IIO_ANGL_VEL:
> >  	case IIO_ACCEL:
> >  		ret = adis_read_reg_32(&st->adis, reg, &val32);
> > -		*bias = sign_extend32(val32, 31);
> > +		if (ret == 0)
> > +			*bias = sign_extend32(val32, 31);
> >  		break;
> >  	default:
> >  		ret = -EINVAL;

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-01  9:34 [PATCH 00/10] iio: adis: cleanups and fixes Alexandru Ardelean
2019-11-01  9:34 ` [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero Alexandru Ardelean
2019-11-03 10:15   ` Jonathan Cameron
2019-11-01  9:34 ` [PATCH 02/10] iio: imu: adis16400: " Alexandru Ardelean
2019-11-03 10:21   ` Jonathan Cameron
2019-11-04  7:40     ` Ardelean, Alexandru
2019-11-01  9:34 ` [PATCH 03/10] iio: imu: adis16460: " Alexandru Ardelean
2019-11-03 10:25   ` Jonathan Cameron
2019-11-01  9:34 ` [PATCH 04/10] iio: imu: adis16480: " Alexandru Ardelean
2019-11-03 10:27   ` Jonathan Cameron
2019-11-01  9:35 ` [PATCH 05/10] iio: imu: adis: " Alexandru Ardelean
2019-11-03 10:31   ` Jonathan Cameron
2019-11-01  9:35 ` [PATCH 06/10] iio: imu: adis16480: fix indentation of return statement Alexandru Ardelean
2019-11-03 10:35   ` Jonathan Cameron
2019-11-01  9:35 ` [PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned` Alexandru Ardelean
2019-11-03 10:36   ` Jonathan Cameron
2019-11-01  9:35 ` [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded Alexandru Ardelean
2019-11-03 10:41   ` Jonathan Cameron
2019-11-04  8:50     ` Ardelean, Alexandru
2019-11-01  9:35 ` [PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful Alexandru Ardelean
2019-11-03 10:46   ` Jonathan Cameron
2019-11-01  9:35 ` [PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs Alexandru Ardelean
2019-11-03 10:49   ` Jonathan Cameron

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git