| Message ID | 20170919015651.GA4554@Haneen (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 --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,
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(-)