Use macros BIT() and GENMASK()
Subsystems (4)
cover Use macros BIT() and GENMASK()
Use BIT() and GENMASK() macros for left shifts. Also add a blank line after a function declaration.
sayli karnik (3):
staging: iio: ad7152: Use BIT() macro for left shifting 1
staging: iio: ad7152: Use GENMASK() macro for left shifts
staging: iio: ad7152: Add a blank line after a function
drivers/staging/iio/cdc/ad7152.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/3 staging: iio: ad7152: Use BIT() macro for left shifting 1
Replace left shifting on 1 with the BIT(x) macro as suggested by checkpatch.pl.
Done with coccinelle:
@@ int g; @@
-(1 << g)
+BIT(g)
--- drivers/staging/iio/cdc/ad7152.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/staging/iio/cdc/ad7152.c +10 -10
@@ -41,30 +41,30 @@#define AD7152_REG_CFG2 26/* Status Register Bit Designations (AD7152_REG_STATUS) */ -#define AD7152_STATUS_RDY1 (1 << 0) -#define AD7152_STATUS_RDY2 (1 << 1) -#define AD7152_STATUS_C1C2 (1 << 2) -#define AD7152_STATUS_PWDN (1 << 7) +#define AD7152_STATUS_RDY1 BIT(0) +#define AD7152_STATUS_RDY2 BIT(1) +#define AD7152_STATUS_C1C2 BIT(2) +#define AD7152_STATUS_PWDN BIT(7)/* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */ -#define AD7152_SETUP_CAPDIFF (1 << 5) +#define AD7152_SETUP_CAPDIFF BIT(5)#define AD7152_SETUP_RANGE_2pF (0 << 6) -#define AD7152_SETUP_RANGE_0_5pF (1 << 6) +#define AD7152_SETUP_RANGE_0_5pF BIT(6)#define AD7152_SETUP_RANGE_1pF (2 << 6)#define AD7152_SETUP_RANGE_4pF (3 << 6)#define AD7152_SETUP_RANGE (x) ((x) << 6)/* Config Register Bit Designations (AD7152_REG_CFG) */ -#define AD7152_CONF_CH2EN (1 << 3) -#define AD7152_CONF_CH1EN (1 << 4) +#define AD7152_CONF_CH2EN BIT(3) +#define AD7152_CONF_CH1EN BIT(4)#define AD7152_CONF_MODE_IDLE (0 << 0) -#define AD7152_CONF_MODE_CONT_CONV (1 << 0) +#define AD7152_CONF_MODE_CONT_CONV BIT(0)#define AD7152_CONF_MODE_SINGLE_CONV (2 << 0)#define AD7152_CONF_MODE_OFFS_CAL (5 << 0)#define AD7152_CONF_MODE_GAIN_CAL (6 << 0)/* Capdac Register Bit Designations (AD7152_REG_CAPDAC_XXX) */ -#define AD7152_CAPDAC_DACEN (1 << 7) +#define AD7152_CAPDAC_DACEN BIT(7)#define AD7152_CAPDAC_DACP (x) ((x) & 0x1F)/* CFG2 Register Bit Designations (AD7152_REG_CFG2) */
Signature · 1 line
2.7.4
PATCH 2/3 staging: iio: ad7152: Use GENMASK() macro for left shifts
In reply to sayli karnikUse GENMASK() macro for left shifting integers.
Done using coccinelle:
@@ int a,b; @@
-(a << b)
+GENMASK(a, b)
--- drivers/staging/iio/cdc/ad7152.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/staging/iio/cdc/ad7152.c +8 -8
@@ -48,27 +48,27 @@/* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */ #define AD7152_SETUP_CAPDIFF BIT(5) -#define AD7152_SETUP_RANGE_2pF (0 << 6) +#define AD7152_SETUP_RANGE_2pF GENMASK(0, 6)#define AD7152_SETUP_RANGE_0_5pF BIT(6) -#define AD7152_SETUP_RANGE_1pF (2 << 6) -#define AD7152_SETUP_RANGE_4pF (3 << 6) +#define AD7152_SETUP_RANGE_1pF GENMASK(2, 6) +#define AD7152_SETUP_RANGE_4pF GENMASK(3, 6)#define AD7152_SETUP_RANGE (x) ((x) << 6)/* Config Register Bit Designations (AD7152_REG_CFG) */ #define AD7152_CONF_CH2EN BIT(3)#define AD7152_CONF_CH1EN BIT(4) -#define AD7152_CONF_MODE_IDLE (0 << 0) +#define AD7152_CONF_MODE_IDLE GENMASK(0, 0)#define AD7152_CONF_MODE_CONT_CONV BIT(0) -#define AD7152_CONF_MODE_SINGLE_CONV (2 << 0) -#define AD7152_CONF_MODE_OFFS_CAL (5 << 0) -#define AD7152_CONF_MODE_GAIN_CAL (6 << 0) +#define AD7152_CONF_MODE_SINGLE_CONV GENMASK(2, 0) +#define AD7152_CONF_MODE_OFFS_CAL GENMASK(5, 0) +#define AD7152_CONF_MODE_GAIN_CAL GENMASK(6, 0)/* Capdac Register Bit Designations (AD7152_REG_CAPDAC_XXX) */ #define AD7152_CAPDAC_DACEN BIT(7)#define AD7152_CAPDAC_DACP (x) ((x) & 0x1F)/* CFG2 Register Bit Designations (AD7152_REG_CFG2) */ -#define AD7152_CFG2_OSR (x) (((x) & 0x3) << 4) +#define AD7152_CFG2_OSR (x) GENMASK(((x) & 0x3), 4)enum {AD7152_DATA ,
Signature · 1 line
2.7.4
PATCH 3/3 staging: iio: ad7152: Add a blank line after a function definition
In reply to sayli karnikThe patch resolves the checkpatch issue:
CHECK: Please use a blank line after function/struct/union/enum declarations.
--- drivers/staging/iio/cdc/ad7152.c | 1 + 1 file changed, 1 insertion(+)
drivers/staging/iio/cdc/ad7152.c +1 -0
@@ -244,6 +244,7 @@ static int ad7152_write_raw_samp_freq(struct device *dev, int val)return ret; } + static int ad7152_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int val,
Signature · 1 line
2.7.4
Re: [PATCH 1/3] staging: iio: ad7152: Use BIT() macro for left shifting 1
In reply to sayli karnikQuoted 10 lines · sayli karnik
Replace left shifting on 1 with the BIT(x) macro as suggested by checkpatch.pl.
Done with coccinelle:
@@ int g; @@(quoted diff) +1
-1-(1 << g) +BIT(g)Signed-off-by sayli karnik <[email protected]>
Hi,
Thanks for the patch. When doing these kind of conversions special care needs to be taken to make sure that they make semantical sense.
The BIT(x) transform for example should only be done for single bit bitfields.
[...]
drivers/staging/iio/cdc/ad7152.c line 47
Quoted 9 lines
/* Status Register Bit Designations (
AD7152_REG_STATUS) */drivers/staging/iio/cdc/ad7152.c +4
-4-#define AD7152_STATUS_RDY1 (1 << 0) -#define AD7152_STATUS_RDY2 (1 << 1) -#define AD7152_STATUS_C1C2 (1 << 2) -#define AD7152_STATUS_PWDN (1 << 7) +#define AD7152_STATUS_RDY1 BIT(0) +#define AD7152_STATUS_RDY2 BIT(1) +#define AD7152_STATUS_C1C2 BIT(2) +#define AD7152_STATUS_PWDN BIT(7)
This is correct, these are all single bit bitfields.
drivers/staging/iio/cdc/ad7152.c line 52
Quoted 7 lines
/* Setup Register Bit Designations (
AD7152_REG_CHx_SETUP) */drivers/staging/iio/cdc/ad7152.c +2
-2-#define AD7152_SETUP_CAPDIFF (1 << 5) +#define AD7152_SETUP_CAPDIFF BIT(5)#define AD7152_SETUP_RANGE_2pF (0 << 6) -#define AD7152_SETUP_RANGE_0_5pF (1 << 6) +#define AD7152_SETUP_RANGE_0_5pF BIT(6)
This on the other hand is a multi-bit field with 1 being one of the possible values. Hence this should be part of the second patch and use GENMASK instead.
In general when you have a whole bunch of values following the scheme of y << x. With 1 << x being one of them use GENMASK() rather than BIT().
drivers/staging/iio/cdc/ad7152.c line 61
Quoted 12 lines
#define
AD7152_SETUP_RANGE_1pF(2 << 6)
#defineAD7152_SETUP_RANGE_4pF(3 << 6)
#defineAD7152_SETUP_RANGE(x)((x) << 6)/* Config Register Bit Designations (
AD7152_REG_CFG) */drivers/staging/iio/cdc/ad7152.c +3
-3-#define AD7152_CONF_CH2EN (1 << 3) -#define AD7152_CONF_CH1EN (1 << 4) +#define AD7152_CONF_CH2EN BIT(3) +#define AD7152_CONF_CH1EN BIT(4)#define AD7152_CONF_MODE_IDLE (0 << 0) -#define AD7152_CONF_MODE_CONT_CONV (1 << 0) +#define AD7152_CONF_MODE_CONT_CONV BIT(0)
Same here.
Quoted 11 lines
#define
AD7152_CONF_MODE_SINGLE_CONV(2 << 0)
#defineAD7152_CONF_MODE_OFFS_CAL(5 << 0)
#defineAD7152_CONF_MODE_GAIN_CAL(6 << 0)/* Capdac Register Bit Designations (
AD7152_REG_CAPDAC_XXX) */drivers/staging/iio/cdc/ad7152.c +1
-1-#define AD7152_CAPDAC_DACEN (1 << 7) +#define AD7152_CAPDAC_DACEN BIT(7)#define AD7152_CAPDAC_DACP (x) ((x) & 0x1F)/* CFG2 Register Bit Designations (AD7152_REG_CFG2) */
Re: [Outreachy kernel] [PATCH 2/3] staging: iio: ad7152: Use GENMASK() macro for left shifts
In reply to sayli karnikQuoted 18 lines · sayli karnik
Use
GENMASK()macro for left shifting integers.
Done using coccinelle:
@@ int a,b; @@(quoted diff) +1
-1-(a << b) +GENMASK(a, b)Signed-off-by sayli karnik <[email protected]>--- drivers/staging/iio/cdc/ad7152.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)drivers/staging/iio/cdc/ad7152.c +0
-0@@ -48,27 +48,27 @@
drivers/staging/iio/cdc/ad7152.c line 55
Quoted 10 lines
drivers/staging/iio/cdc/ad7152.c +3
-3/* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */ #define AD7152_SETUP_CAPDIFF BIT(5) -#define AD7152_SETUP_RANGE_2pF (0 << 6) +#define AD7152_SETUP_RANGE_2pF GENMASK(0, 6)#define AD7152_SETUP_RANGE_0_5pF BIT(6) -#define AD7152_SETUP_RANGE_1pF (2 << 6) -#define AD7152_SETUP_RANGE_4pF (3 << 6) +#define AD7152_SETUP_RANGE_1pF GENMASK(2, 6) +#define AD7152_SETUP_RANGE_4pF GENMASK(3, 6)#define AD7152_SETUP_RANGE (x) ((x) << 6)
I think it would be more understandable to do all of the cases in the same way.
julia
Quoted 34 lines
drivers/staging/iio/cdc/ad7152.c +5
-6/* Config Register Bit Designations (AD7152_REG_CFG) */ #define AD7152_CONF_CH2EN BIT(3)#define AD7152_CONF_CH1EN BIT(4) -#define AD7152_CONF_MODE_IDLE (0 << 0) +#define AD7152_CONF_MODE_IDLE GENMASK(0, 0)#define AD7152_CONF_MODE_CONT_CONV BIT(0) -#define AD7152_CONF_MODE_SINGLE_CONV (2 << 0) -#define AD7152_CONF_MODE_OFFS_CAL (5 << 0) -#define AD7152_CONF_MODE_GAIN_CAL (6 << 0) +#define AD7152_CONF_MODE_SINGLE_CONV GENMASK(2, 0) +#define AD7152_CONF_MODE_OFFS_CAL GENMASK(5, 0) +#define AD7152_CONF_MODE_GAIN_CAL GENMASK(6, 0)/* Capdac Register Bit Designations (AD7152_REG_CAPDAC_XXX) */ #define AD7152_CAPDAC_DACEN BIT(7)#define AD7152_CAPDAC_DACP (x) ((x) & 0x1F)/* CFG2 Register Bit Designations (AD7152_REG_CFG2) */ -#define AD7152_CFG2_OSR (x) (((x) & 0x3) << 4) +#define AD7152_CFG2_OSR (x) GENMASK(((x) & 0x3), 4)enum {AD7152_DATA , --2.7.4
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/92240a3dca0138880fe47a7df0a5e6df869c8199.1487448326.git.karniksayli1995%40gmail.com.
For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 2/3] staging: iio: ad7152: Use GENMASK() macro for left shifts
In reply to sayli karnikQuoted 8 lines · sayli karnik
Use
GENMASK()macro for left shifting integers.
Done using coccinelle:
@@ int a,b; @@(quoted diff) +1
-1-(a << b) +GENMASK(a, b)Signed-off-by sayli karnik <[email protected]>
Hi,
Thanks for the patch. Looks mostly good, but please rework the patch for version 2 to address the comments for patch 1.
Some additional comments inline.
[...]
drivers/staging/iio/cdc/ad7152.c line 55
Quoted 10 lines
/* Setup Register Bit Designations (
AD7152_REG_CHx_SETUP) */
#defineAD7152_SETUP_CAPDIFFBIT(5)drivers/staging/iio/cdc/ad7152.c +3
-3-#define AD7152_SETUP_RANGE_2pF (0 << 6) +#define AD7152_SETUP_RANGE_2pF GENMASK(0, 6)#define AD7152_SETUP_RANGE_0_5pF BIT(6) -#define AD7152_SETUP_RANGE_1pF (2 << 6) -#define AD7152_SETUP_RANGE_4pF (3 << 6) +#define AD7152_SETUP_RANGE_1pF GENMASK(2, 6) +#define AD7152_SETUP_RANGE_4pF GENMASK(3, 6)#define AD7152_SETUP_RANGE (x) ((x) << 6)
AD7152_SETUP_RANGE() can also use the GENMASK() macro
[...]
drivers/staging/iio/cdc/ad7152.c line 71
Quoted 3 lines
/* CFG2 Register Bit Designations (
AD7152_REG_CFG2) */drivers/staging/iio/cdc/ad7152.c +1
-1-#define AD7152_CFG2_OSR (x) (((x) & 0x3) << 4) +#define AD7152_CFG2_OSR (x) GENMASK(((x) & 0x3), 4)
The extra parenthesis around (x) & 0x3 are no needed when using the macro (the macro will add them).
Re: [Outreachy kernel] Re: [PATCH 2/3] staging: iio: ad7152: Use GENMASK() macro for left shifts
Quoted 8 lines · Lars-Peter Clausen
Quoted 7 lines · sayli karnik
Use
GENMASK()macro for left shifting integers.
Done using coccinelle:
@@ int a,b; @@(quoted diff) +1
-1-(a << b) +GENMASK(a, b)
Quoted 7 lines
Quoted 1 line
Signed-off-by sayli karnik <[email protected]>Hi,
Thanks for the patch. Looks mostly good, but please rework the patch for version 2 to address the comments for patch 1.
Yes I am sending another version.
Quoted 16 lines
Some additional comments inline.
[...]
drivers/staging/iio/cdc/ad7152.c line 55
Quoted 10 lines
/* Setup Register Bit Designations (
AD7152_REG_CHx_SETUP) */
#defineAD7152_SETUP_CAPDIFFBIT(5)drivers/staging/iio/cdc/ad7152.c +3
-3-#define AD7152_SETUP_RANGE_2pF (0 << 6) +#define AD7152_SETUP_RANGE_2pF GENMASK(0, 6)#define AD7152_SETUP_RANGE_0_5pF BIT(6) -#define AD7152_SETUP_RANGE_1pF (2 << 6) -#define AD7152_SETUP_RANGE_4pF (3 << 6) +#define AD7152_SETUP_RANGE_1pF GENMASK(2, 6) +#define AD7152_SETUP_RANGE_4pF GENMASK(3, 6)#define AD7152_SETUP_RANGE (x) ((x) << 6)
AD7152_SETUP_RANGE()can also use theGENMASK()macro
Quoted 8 lines
[...]
drivers/staging/iio/cdc/ad7152.c line 71
Quoted 3 lines
/* CFG2 Register Bit Designations (
AD7152_REG_CFG2) */drivers/staging/iio/cdc/ad7152.c +1
-1-#define AD7152_CFG2_OSR (x) (((x) & 0x3) << 4) +#define AD7152_CFG2_OSR (x) GENMASK(((x) & 0x3), 4)The extra parenthesis around (x) &
0x3are no needed when using the macro (the macro will add them).
The outer parentheses are certainly not needed but for the ones around x, checkpatch.pl complains otherwise-
CHECK: Macro argument 'x' may be better as '(x)' to avoid precedence issues
Should I still remove them?
thanks,
sayli
Quoted 6 lines
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/0b5ef4d0-14b5-1196-8bc4-deda9d8f75ce%40metafoo.de.
For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] Re: [PATCH 2/3] staging: iio: ad7152: Use GENMASK() macro for left shifts
On 02/19/2017 07:01 AM, sayli karnik wrote:
[...]
Quoted 11 lines
Quoted 8 lines
[...]
drivers/staging/iio/cdc/ad7152.c line 71
Quoted 3 lines
/* CFG2 Register Bit Designations (
AD7152_REG_CFG2) */drivers/staging/iio/cdc/ad7152.c +1
-1-#define AD7152_CFG2_OSR (x) (((x) & 0x3) << 4) +#define AD7152_CFG2_OSR (x) GENMASK(((x) & 0x3), 4)The extra parenthesis around (x) &
0x3are no needed when using the macro (the macro will add them).The outer parentheses are certainly not needed but for the ones around x, checkpatch.pl complains otherwise-
CHECK: Macro argument 'x' may be better as '(x)' to avoid precedence issues
The one around x are needed and should stay.