Use macros BIT() and GENMASK()

PATCH Superseded sayli karnik 3 participants 9 messages Feb 18, 2017 – Feb 19, 2017

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

sayli karnik

author

2017-02-18 20:19 UTC #0 lore raw

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

sayli karnik

author

2017-02-18 20:20 UTC #1 lore raw

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)

Signed-off-by sayli karnik <[email protected]>

---
 drivers/staging/iio/cdc/ad7152.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/staging/iio/cdc/ad7152.c +10 ~~-10~~

index b91b50f..0c97d5a 100644
@@ -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

Lars-Peter Clausen

2017-02-18 20:35 UTC #2 lore raw

Re: [PATCH 1/3] staging: iio: ad7152: Use BIT() macro for left shifting 1

Quoted 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)
#define AD7152_SETUP_RANGE_4pF (3 << 6)
#define AD7152_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)
#define AD7152_CONF_MODE_OFFS_CAL (5 << 0)
#define AD7152_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) */

sayli karnik

author

2017-02-18 20:21 UTC #3 lore raw

PATCH 2/3 staging: iio: ad7152: Use GENMASK() macro for left shifts

In reply to sayli karnik

Use GENMASK() macro for left shifting integers.
Done using coccinelle:
@@ int a,b; @@

-(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 +8 -8

index 0c97d5a..988b89b 100644
@@ -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

Julia Lawall

2017-02-18 20:35 UTC #4 lore raw

Re: [Outreachy kernel] [PATCH 2/3] staging: iio: ad7152: Use GENMASK() macro for left shifts

Quoted 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
index 0c97d5a..988b89b 100644
@@ -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.

Lars-Peter Clausen

2017-02-18 20:40 UTC #5 lore raw

Re: [PATCH 2/3] staging: iio: ad7152: Use GENMASK() macro for left shifts

In reply to sayli karnik

Quoted 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) */
#define AD7152_SETUP_CAPDIFF BIT(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).

sayli karnik

author

2017-02-19 06:01 UTC #6 lore raw

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) */
#define AD7152_SETUP_CAPDIFF BIT(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

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) & 0x3 are 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.

Lars-Peter Clausen

2017-02-19 09:29 UTC #7 lore raw

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) & 0x3 are 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.

sayli karnik

author

2017-02-18 20:23 UTC #8 lore raw

PATCH 3/3 staging: iio: ad7152: Add a blank line after a function definition

In reply to sayli karnik

The patch resolves the checkpatch issue:
CHECK: Please use a blank line after function/struct/union/enum declarations.

Signed-off-by sayli karnik <[email protected]>

---
 drivers/staging/iio/cdc/ad7152.c | 1 +
 1 file changed, 1 insertion(+)

drivers/staging/iio/cdc/ad7152.c +1 -0

index 988b89b..2ed6a90 100644
@@ -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