diff mbox

[v2] staging: iio: Replace bit shifting with BIT macro

Message ID 20170919015651.GA4554@Haneen
State New
Headers show

Commit Message

Haneen Mohammed Sept. 19, 2017, 1:56 a.m. UTC
This patch replace bit shifting on 1, and 3 with BIT(x) macro.
Issue resolved with the following Coccinelle script:

@r1@
identifier x;
constant int g;
@@
(
0<<\(x\|g\)
|
1<<\(x\|g\)
|
2<<\(x\|g\)
|
3<<\(x\|g\)
)

@script:python b@
g2 <<r1.g;
y;
@@

coccinelle.y = int(g2) + 1

@c@
constant int r1.g;
identifier b.y;
@@
(
-(1 << g)
+BIT(g)
|
-(0 << g)
+ 0
|
-(2 << g)
+BIT(y)
|
-(3 << g)
+(BIT(y) | BIT(g))
)

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
Changes in v2: 
- undo changes where it results in different styles mixed up
- remove parenthesis aroun BIT(y) | BIT(g)

 drivers/staging/iio/impedance-analyzer/ad5933.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Lars-Peter Clausen Sept. 19, 2017, 11:48 a.m. UTC | #1
On 09/19/2017 03:56 AM, Haneen Mohammed wrote:
> This patch replace bit shifting on 1, and 3 with BIT(x) macro.
> Issue resolved with the following Coccinelle script:

Can you explain why this is an issue and why using the BIT() macro is better
in this case?

These kind of changes need to make semantic sense, the goal should generally
be to improve the legibility of the code, so somebody looking at the code
easier understands what the intention of the code is.

E.g. we can easily write "x *= BIT(2) | BIT(3)" instead of "x *= 12", both
have the same effect. But the later is much easier to understand for the
casual reader.

The intended semantics for BIT() are that we are talking about a single bit
field. There were a few of those in version 1 of your patch, were using the
BIT() macro makes sense. But in my opinion none of the changes in this patch
fall in this category. I mean, what is the meaning of multiplying by
BIT(27)? Maybe we need a POW2() macro for places where we are semantically
speaking are talking about a number that is a power of two.

> @@ -229,7 +229,7 @@ static int ad5933_set_freq(struct ad5933_state *st,
>  		u8 d8[4];
>  	} dat;
>  
> -	freqreg = (u64) freq * (u64) (1 << 27);
> +	freqreg = (u64)freq * (u64)BIT(27);
>  	do_div(freqreg, st->mclk_hz / 4);
>  
>  	switch (reg) {
> @@ -318,7 +318,7 @@ static ssize_t ad5933_show_frequency(struct device *dev,
>  	freqreg = be32_to_cpu(dat.d32) & 0xFFFFFF;
>  
>  	freqreg = (u64)freqreg * (u64)(st->mclk_hz / 4);
> -	do_div(freqreg, 1 << 27);
> +	do_div(freqreg, BIT(27));
>  
>  	return sprintf(buf, "%d\n", (int)freqreg);
>  }
> @@ -452,9 +452,9 @@ static ssize_t ad5933_store(struct device *dev,
>  
>  		/* 2x, 4x handling, see datasheet */
>  		if (val > 1022)
> -			val = (val >> 2) | (3 << 9);
> +			val = (val >> 2) | BIT(10) | BIT(9);
>  		else if (val > 511)
> -			val = (val >> 1) | (1 << 9);
> +			val = (val >> 1) | BIT(9);
>  
>  		dat = cpu_to_be16(val);
>  		ret = ad5933_i2c_write(st->client,
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 3d539ee..4cb418e 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -229,7 +229,7 @@  static int ad5933_set_freq(struct ad5933_state *st,
 		u8 d8[4];
 	} dat;
 
-	freqreg = (u64) freq * (u64) (1 << 27);
+	freqreg = (u64)freq * (u64)BIT(27);
 	do_div(freqreg, st->mclk_hz / 4);
 
 	switch (reg) {
@@ -318,7 +318,7 @@  static ssize_t ad5933_show_frequency(struct device *dev,
 	freqreg = be32_to_cpu(dat.d32) & 0xFFFFFF;
 
 	freqreg = (u64)freqreg * (u64)(st->mclk_hz / 4);
-	do_div(freqreg, 1 << 27);
+	do_div(freqreg, BIT(27));
 
 	return sprintf(buf, "%d\n", (int)freqreg);
 }
@@ -452,9 +452,9 @@  static ssize_t ad5933_store(struct device *dev,
 
 		/* 2x, 4x handling, see datasheet */
 		if (val > 1022)
-			val = (val >> 2) | (3 << 9);
+			val = (val >> 2) | BIT(10) | BIT(9);
 		else if (val > 511)
-			val = (val >> 1) | (1 << 9);
+			val = (val >> 1) | BIT(9);
 
 		dat = cpu_to_be16(val);
 		ret = ad5933_i2c_write(st->client,