Skip to contentSkip to conversation

staging:iio:accel:adis16240: sign extend function avoiding code duplication

PATCH Superseded Karim Eshapa 2 participants 3 messages Jun 12, 2018 – Jun 17, 2018

Subsystems (4)
Staging IIO Analog Devices IIO drivers Staging - industrial io

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Karim Eshapa

author
#0 lore raw

PATCH v2 1/1 staging:iio:accel:adis16240: sign extend function avoiding code duplication

Use sign_extend32 kernel function instead of code duplication, Safe also for 16 bit. and remove declaration of bits variable not needed.

Signed-off-by Karim Eshapa <[email protected]>
Quoted 23 lines · Karim Eshapa
Quoted 2 lines

Use sign_extend32 kernel function instead of code duplication. Safe also for 16 bit.

Perhaps remove the bits declaration and assignments and just use 9 directly.

Quoted 16 lines
drivers/staging/iio/accel/adis16240.c +1 -3
@@ -292,9 +292,7 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, bits - 1);
              return IIO_VAL_INT;
      case IIO_CHAN_INFO_PEAK:
              bits = 10;

@@ -302,9 +300,7 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
Quoted 8 lines

drivers/staging/iio/accel/adis16240.c static int adis16240_read_raw(struct iio_dev *indio_dev, · line 306

Quoted 8 lines
drivers/staging/iio/accel/adis16240.c +1 -3
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, bits - 1);
              return IIO_VAL_INT;
      }
      return -EINVAL;

---
 drivers/staging/iio/accel/adis16240.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/staging/iio/accel/adis16240.c +2 -9
index fff6d99089cc..24e525f1ef25 100644
@@ -250,7 +250,6 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
 {
 	struct adis *st = iio_priv(indio_dev);
 	int ret;
-	int bits;
 	u8 addr;
 	s16 val16;
 

@@ -287,24 +286,18 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
 		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_CALIBBIAS:
-		bits = 10;
 		addr = adis16240_addresses[chan->scan_index][0];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret)
 			return ret;
-		val16 &= (1 << bits) - 1;
-		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-		*val = val16;
+		*val = sign_extend32(val16, 9);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_PEAK:
-		bits = 10;
 		addr = adis16240_addresses[chan->scan_index][1];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret)
 			return ret;
-		val16 &= (1 << bits) - 1;
-		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-		*val = val16;
+		*val = sign_extend32(val16, 9);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
Signature · 1 line
2.17.1
maintainer
#1 lore raw
Quoted 4 lines · On Tue, 12 Jun 2018 18:21:53 +0200 Karim Eshapa <[email protected]> wrote:

Use sign_extend32 kernel function instead of code duplication, Safe also for 16 bit. and remove declaration of bits variable not needed.

Signed-off-by Karim Eshapa <[email protected]>

Please consider how a patch is applied and resend as a v3 which can actually be applied. Test it (by applying it yourself) to make sure you have everything correct...

Jonathan

Quoted 75 lines
Quoted 24 lines · Karim Eshapa
Quoted 2 lines

Use sign_extend32 kernel function instead of code duplication. Safe also for 16 bit.

Perhaps remove the bits declaration and assignments and just use 9 directly.

Quoted 16 lines
drivers/staging/iio/accel/adis16240.c +1 -3
@@ -292,9 +292,7 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, bits - 1);
              return IIO_VAL_INT;
      case IIO_CHAN_INFO_PEAK:
              bits = 10;

@@ -302,9 +300,7 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
Quoted 8 lines

drivers/staging/iio/accel/adis16240.c static int adis16240_read_raw(struct iio_dev *indio_dev, · line 303

Quoted 8 lines
drivers/staging/iio/accel/adis16240.c +1 -3
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, bits - 1);
              return IIO_VAL_INT;
      }
      return -EINVAL;  

---
 drivers/staging/iio/accel/adis16240.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/staging/iio/accel/adis16240.c +2 -9
index fff6d99089cc..24e525f1ef25 100644
@@ -250,7 +250,6 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
 {
 	struct adis *st = iio_priv(indio_dev);
 	int ret;
-	int bits;
 	u8 addr;
 	s16 val16;
 

@@ -287,24 +286,18 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
 		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_CALIBBIAS:
-		bits = 10;
 		addr = adis16240_addresses[chan->scan_index][0];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret)
 			return ret;
-		val16 &= (1 << bits) - 1;
-		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-		*val = val16;
+		*val = sign_extend32(val16, 9);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_PEAK:
-		bits = 10;
 		addr = adis16240_addresses[chan->scan_index][1];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret)
 			return ret;
-		val16 &= (1 << bits) - 1;
-		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-		*val = val16;
+		*val = sign_extend32(val16, 9);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
author
#2 lore raw
Quoted 6 lines
Quoted 4 lines

Use sign_extend32 kernel function instead of code duplication, Safe also for 16 bit. and remove declaration of bits variable not needed.

Signed-off-by Karim Eshapa <[email protected]>

Please consider how a patch is applied and resend as a v3 which can

actually

Quoted 1 line

be applied. Test it (by applying it yourself) to make sure you have

everything correct...

OK I did with the previous patches, but i didn't know what you meant in the previous
mail with resend patch V2 included the change log so, I did what you have seen :)
sorry for that.

Thanks,

Quoted 98 lines · Jonathan Cameron
Quoted 4 lines · On Tue, 12 Jun 2018 18:21:53 +0200 Karim Eshapa <[email protected]> wrote:

Use sign_extend32 kernel function instead of code duplication, Safe also for 16 bit. and remove declaration of bits variable not needed.

Signed-off-by Karim Eshapa <[email protected]>

Please consider how a patch is applied and resend as a v3 which can actually
be applied. Test it (by applying it yourself) to make sure you have everything correct...

Jonathan

Quoted 9 lines
Quoted 9 lines · Karim Eshapa
Quoted 2 lines

Use sign_extend32 kernel function instead of code duplication. Safe also for 16 bit.

Perhaps remove the bits declaration and assignments and just use 9 directly.

Review

Quoted 1 line
(quoted diff) +0 -0

b/drivers/staging/iio/accel/adis16240.c

Quoted 2 lines
Quoted 2 lines

Review static int adis16240_read_raw(struct iio_dev

Quoted 2 lines
(quoted diff) +0 -0
@@ -292,9 +292,7 @@ static int adis16240_read_raw(struct iio_dev

*indio_dev,

Quoted 11 lines
Quoted 11 lines

drivers/staging/iio/accel/adis16240.c static int adis16240_read_raw(struct iio_dev

Quoted 11 lines
drivers/staging/iio/accel/adis16240.c +1 -3
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, bits - 1);
              return IIO_VAL_INT;
      case IIO_CHAN_INFO_PEAK:
              bits = 10;

@@ -302,9 +300,7 @@ static int adis16240_read_raw(struct iio_dev

*indio_dev,

Quoted 10 lines
Quoted 10 lines

drivers/staging/iio/accel/adis16240.c static int adis16240_read_raw(struct iio_dev *indio_dev, · line 303

Quoted 10 lines
drivers/staging/iio/accel/adis16240.c +1 -3
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, bits - 1);
              return IIO_VAL_INT;
      }
      return -EINVAL;

Review

Quoted 5 lines
---
 drivers/staging/iio/accel/adis16240.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)
(quoted diff) +0 -0

b/drivers/staging/iio/accel/adis16240.c

drivers/staging/iio/accel/adis16240.c static int adis16240_read_raw(struct iio_dev

Quoted 4 lines

index fff6d99089cc..24e525f1ef25 100644

drivers/staging/iio/accel/adis16240.c +0 -0
@@ -250,7 +250,6 @@ static int adis16240_read_raw(struct iio_dev

*indio_dev,

drivers/staging/iio/accel/adis16240.c static int adis16240_read_raw(struct iio_dev

Quoted 8 lines
drivers/staging/iio/accel/adis16240.c +0 -1
 {
      struct adis *st = iio_priv(indio_dev);
      int ret;
-     int bits;
      u8 addr;
      s16 val16;
 

@@ -287,24 +286,18 @@ static int adis16240_read_raw(struct iio_dev

*indio_dev,

Quoted 26 lines
drivers/staging/iio/accel/adis16240.c +2 -8
              *val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
              return IIO_VAL_INT;
      case IIO_CHAN_INFO_CALIBBIAS:
-             bits = 10;
              addr = adis16240_addresses[chan->scan_index][0];
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, 9);
              return IIO_VAL_INT;
      case IIO_CHAN_INFO_PEAK:
-             bits = 10;
              addr = adis16240_addresses[chan->scan_index][1];
              ret = adis_read_reg_16(st, addr, &val16);
              if (ret)
                      return ret;
-             val16 &= (1 << bits) - 1;
-             val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-             *val = val16;
+             *val = sign_extend32(val16, 9);
              return IIO_VAL_INT;
      }
      return -EINVAL;