[PATCH 00/10] iio: adis: cleanups and fixes

[PATCH 00/10] iio: adis: cleanups and fixes

[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

[PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero

[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

[PATCH 02/10] iio: imu: adis16400: check ret val for non-zero vs less-than-zero

[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

[PATCH 03/10] iio: imu: adis16460: check ret val for non-zero vs less-than-zero

[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

[PATCH 04/10] iio: imu: adis16480: check ret val for non-zero vs less-than-zero

[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

[PATCH 05/10] iio: imu: adis: check ret val for non-zero vs less-than-zero

[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

[PATCH 06/10] iio: imu: adis16480: fix indentation of return statement

[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

[PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned`

[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

[PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded

[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

[PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful

[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

[PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs

[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

Re: [PATCH 01/10] iio: gyro: adis16136: check ret val for non-zero vs less-than-zero

[Jonathan Cameron]

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

Re: [PATCH 02/10] iio: imu: adis16400: check ret val for non-zero vs less-than-zero

[Jonathan Cameron]

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;

Re: [PATCH 03/10] iio: imu: adis16460: check ret val for non-zero vs less-than-zero

[Jonathan Cameron]

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

Re: [PATCH 04/10] iio: imu: adis16480: check ret val for non-zero vs less-than-zero

[Jonathan Cameron]

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

Re: [PATCH 05/10] iio: imu: adis: check ret val for non-zero vs less-than-zero

[Jonathan Cameron]

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;

Re: [PATCH 06/10] iio: imu: adis16480: fix indentation of return statement

[Jonathan Cameron]

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)

Re: [PATCH 07/10] iio: imu: adis16480: prefer `unsigned int` over `unsigned`

[Jonathan Cameron]

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)

Re: [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded

[Jonathan Cameron]

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;

Re: [PATCH 09/10] iio: imu: adis: assign read val in debugfs hook only if op successful

[Jonathan Cameron]

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 {

Re: [PATCH 10/10] iio: imu: adis: assign value only if return code zero in read funcs

[Jonathan Cameron]

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

Re: [PATCH 02/10] iio: imu: adis16400: check ret val for non-zero vs less-than-zero

[Ardelean, Alexandru]

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;

Re: [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded

[Ardelean, Alexandru]

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;