diff mbox series

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

Message ID 20191101093505.9408-3-alexandru.ardelean@analog.com (mailing list archive)
State New, archived
Headers show
Series iio: adis: cleanups and fixes | expand

Commit Message

Alexandru Ardelean Nov. 1, 2019, 9:34 a.m. UTC 
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(-)

Comments

Jonathan Cameron Nov. 3, 2019, 10:21 a.m. UTC | #1 
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;
Alexandru Ardelean Nov. 4, 2019, 7:40 a.m. UTC | #2 
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;
diff mbox series

Patch

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;