Use BIT() and GENMASK() macros

PATCH In review sayli karnik 5 participants 11 messages Feb 19, 2017 – Feb 25, 2017

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

sayli karnik

author

2017-02-19 19:57 UTC #0 lore raw

v2 cover Use BIT() and GENMASK() macros

Use BIT() and GENMASK() macros for left shifts.

sayli karnik (2):
staging: iio: ad7152: Use BIT() macro for left shifting 1
staging: iio: ad7152: Use GENMASK() macro for left shifts

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

Signature · 1 line

2.7.4

sayli karnik

author

2017-02-19 19:58 UTC #1 lore raw

PATCH v2 1/2 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.

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

---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro

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

drivers/staging/iio/cdc/ad7152.c +4 -4

index b91b50f..e8609b8 100644
@@ -41,10 +41,10 @@
 #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)

Signature · 1 line

2.7.4

sayli karnik

author

2017-02-19 20:00 UTC #4 lore raw

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

In reply to sayli karnik

Use GENMASK() macro for left shifting integers.

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

---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro

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

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

index e8609b8..ab94fad 100644
@@ -47,28 +47,28 @@
 #define AD7152_STATUS_PWDN		BIT(7)
 
 /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
-#define AD7152_SETUP_CAPDIFF		(1 << 5)
-#define AD7152_SETUP_RANGE_2pF		(0 << 6)
-#define AD7152_SETUP_RANGE_0_5pF	(1 << 6)
-#define AD7152_SETUP_RANGE_1pF		(2 << 6)
-#define AD7152_SETUP_RANGE_4pF		(3 << 6)
-#define AD7152_SETUP_RANGE(x)		((x) << 6)
+#define AD7152_SETUP_CAPDIFF		GENMASK(1, 5)
+#define AD7152_SETUP_RANGE_2pF		GENMASK(0, 6)
+#define AD7152_SETUP_RANGE_0_5pF	GENMASK(1, 6)
+#define AD7152_SETUP_RANGE_1pF		GENMASK(2, 6)
+#define AD7152_SETUP_RANGE_4pF		GENMASK(3, 6)
+#define AD7152_SETUP_RANGE(x)		GENMASK(x, 6)
 
 /* Config Register Bit Designations (AD7152_REG_CFG) */
-#define AD7152_CONF_CH2EN		(1 << 3)
-#define AD7152_CONF_CH1EN		(1 << 4)
-#define AD7152_CONF_MODE_IDLE		(0 << 0)
-#define AD7152_CONF_MODE_CONT_CONV	(1 << 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_CH2EN		GENMASK(1, 3)
+#define AD7152_CONF_CH1EN		GENMASK(1, 4)
+#define AD7152_CONF_MODE_IDLE		GENMASK(0, 0)
+#define AD7152_CONF_MODE_CONT_CONV	GENMASK(1, 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		(1 << 7)
-#define AD7152_CAPDAC_DACP(x)		((x) & 0x1F)
+#define AD7152_CAPDAC_DACEN		GENMASK(1, 7)
+#define AD7152_CAPDAC_DACP(x)		GENMASK(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

Acked-by on v2 1/2

2017-02-19 20:02 UTC #2 lore raw

Re: [Outreachy kernel] [PATCH v2 1/2] staging: iio: ad7152: Use BIT() macro for left shifting 1

In reply to sayli karnik

Quoted 4 lines · sayli karnik

Replace left shifting on 1 with the BIT(x) macro as suggested by checkpatch.pl.
Signed-off-by sayli karnik <[email protected]>

Acked-by Julia Lawall <[email protected]>

Quoted 37 lines

---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro
 drivers/staging/iio/cdc/ad7152.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/iio/cdc/ad7152.c +4 -5
index b91b50f..e8609b8 100644
@@ -41,10 +41,10 @@
 #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)
--
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/06283c7cbccd40a7214bf4f047c3829e53b6d448.1487483384.git.karniksayli1995%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Julia Lawall

Acked-by on v2 2/2

2017-02-19 20:04 UTC #5 lore raw

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

In reply to sayli karnik

Quoted 3 lines · sayli karnik

Use GENMASK() macro for left shifting integers.
Signed-off-by sayli karnik <[email protected]>

It looks nicer this way :)

Acked-by Julia Lawall <[email protected]>

Quoted 67 lines

---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro

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

drivers/staging/iio/cdc/ad7152.c +16 ~~-17~~

index e8609b8..ab94fad 100644
@@ -47,28 +47,28 @@
 #define AD7152_STATUS_PWDN		BIT(7)
 
 /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
-#define AD7152_SETUP_CAPDIFF		(1 << 5)
-#define AD7152_SETUP_RANGE_2pF		(0 << 6)
-#define AD7152_SETUP_RANGE_0_5pF	(1 << 6)
-#define AD7152_SETUP_RANGE_1pF		(2 << 6)
-#define AD7152_SETUP_RANGE_4pF		(3 << 6)
-#define AD7152_SETUP_RANGE(x)		((x) << 6)
+#define AD7152_SETUP_CAPDIFF		GENMASK(1, 5)
+#define AD7152_SETUP_RANGE_2pF		GENMASK(0, 6)
+#define AD7152_SETUP_RANGE_0_5pF	GENMASK(1, 6)
+#define AD7152_SETUP_RANGE_1pF		GENMASK(2, 6)
+#define AD7152_SETUP_RANGE_4pF		GENMASK(3, 6)
+#define AD7152_SETUP_RANGE(x)		GENMASK(x, 6)
 
 /* Config Register Bit Designations (AD7152_REG_CFG) */
-#define AD7152_CONF_CH2EN		(1 << 3)
-#define AD7152_CONF_CH1EN		(1 << 4)
-#define AD7152_CONF_MODE_IDLE		(0 << 0)
-#define AD7152_CONF_MODE_CONT_CONV	(1 << 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_CH2EN		GENMASK(1, 3)
+#define AD7152_CONF_CH1EN		GENMASK(1, 4)
+#define AD7152_CONF_MODE_IDLE		GENMASK(0, 0)
+#define AD7152_CONF_MODE_CONT_CONV	GENMASK(1, 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		(1 << 7)
-#define AD7152_CAPDAC_DACP(x)		((x) & 0x1F)
+#define AD7152_CAPDAC_DACEN		GENMASK(1, 7)
+#define AD7152_CAPDAC_DACP(x)		GENMASK(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/b74094e5c94e11cad734b1bde11db0356e6d3aab.1487483384.git.karniksayli1995%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Lars-Peter Clausen

2017-02-20 08:29 UTC #6 lore raw

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

In reply to sayli karnik

Quoted 16 lines · sayli karnik

Use GENMASK() macro for left shifting integers.
Signed-off-by sayli karnik <[email protected]>
---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro
 drivers/staging/iio/cdc/ad7152.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/staging/iio/cdc/ad7152.c +0 -0
index e8609b8..ab94fad 100644

drivers/staging/iio/cdc/ad7152.c line 50

Quoted 6 lines

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

@@ -47,28 +47,28 @@
 #define AD7152_STATUS_PWDN		BIT(7)
 
 /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
-#define AD7152_SETUP_CAPDIFF		(1 << 5)
[...]
+#define AD7152_SETUP_CAPDIFF		GENMASK(1, 5)

This was actually right, the first time, CAPDIFF is a single bit bitfield, so it should use BIT().

drivers/staging/iio/cdc/ad7152.c line 59

Quoted 6 lines

[...]

drivers/staging/iio/cdc/ad7152.c +2 -2

-#define AD7152_CONF_CH2EN		(1 << 3)
-#define AD7152_CONF_CH1EN		(1 << 4)
[...]
+#define AD7152_CONF_CH2EN		GENMASK(1, 3)
+#define AD7152_CONF_CH1EN		GENMASK(1, 4)

Same here CH2EN and CH1EN are single bit bitfields.

drivers/staging/iio/cdc/ad7152.c line 67

Quoted 4 lines

/* 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		GENMASK(1, 7)

Same for DACEN.

Hendrik v. Raven

2017-02-20 09:48 UTC #7 lore raw

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

In reply to sayli karnik

Quoted 21 lines · sayli karnik

Use GENMASK() macro for left shifting integers.
Signed-off-by sayli karnik <[email protected]>
---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro
 drivers/staging/iio/cdc/ad7152.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/staging/iio/cdc/ad7152.c +0 -2
index e8609b8..ab94fad 100644
@@ -47,28 +47,28 @@
 #define AD7152_STATUS_PWDN		BIT(7)
 
 /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
-#define AD7152_SETUP_CAPDIFF		(1 << 5)
-#define AD7152_SETUP_RANGE_2pF		(0 << 6)

drivers/staging/iio/cdc/ad7152.c line 55

Quoted 10 lines

drivers/staging/iio/cdc/ad7152.c +6 -4

-#define AD7152_SETUP_RANGE_0_5pF	(1 << 6)
-#define AD7152_SETUP_RANGE_1pF		(2 << 6)
-#define AD7152_SETUP_RANGE_4pF		(3 << 6)
-#define AD7152_SETUP_RANGE(x)		((x) << 6)
+#define AD7152_SETUP_CAPDIFF		GENMASK(1, 5)
+#define AD7152_SETUP_RANGE_2pF		GENMASK(0, 6)
+#define AD7152_SETUP_RANGE_0_5pF	GENMASK(1, 6)
+#define AD7152_SETUP_RANGE_1pF		GENMASK(2, 6)
+#define AD7152_SETUP_RANGE_4pF		GENMASK(3, 6)
+#define AD7152_SETUP_RANGE(x)		GENMASK(x, 6)

I am a bit confused about the usage of GENMASK here. Unless I completely misunderstand the macro these are not at all equivalent to the previous bitshifts as most of them just expand to 0.

Quoted 10 lines

drivers/staging/iio/cdc/ad7152.c +2 -7

 /* Config Register Bit Designations (AD7152_REG_CFG) */
-#define AD7152_CONF_CH2EN		(1 << 3)
-#define AD7152_CONF_CH1EN		(1 << 4)
-#define AD7152_CONF_MODE_IDLE		(0 << 0)
-#define AD7152_CONF_MODE_CONT_CONV	(1 << 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_CH2EN		GENMASK(1, 3)
+#define AD7152_CONF_CH1EN		GENMASK(1, 4)

drivers/staging/iio/cdc/ad7152.c line 67

Quoted 10 lines

drivers/staging/iio/cdc/ad7152.c +6 -2

+#define AD7152_CONF_MODE_IDLE		GENMASK(0, 0)
+#define AD7152_CONF_MODE_CONT_CONV	GENMASK(1, 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		(1 << 7)
-#define AD7152_CAPDAC_DACP(x)		((x) & 0x1F)
+#define AD7152_CAPDAC_DACEN		GENMASK(1, 7)

same here

drivers/staging/iio/cdc/ad7152.c line 68

Quoted 1 line

drivers/staging/iio/cdc/ad7152.c +1 -0
+#define AD7152_CAPDAC_DACP(x)		GENMASK(x, 0x1F)

this is more something like ((x) & GENMASK(5,0)).

Quoted 13 lines

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

 /* 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 · 6 lines

2.7.4

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

Julia Lawall

2017-02-20 10:40 UTC #8 lore raw

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

Quoted 19 lines · Hendrik v. Raven

Quoted 18 lines · sayli karnik
Use GENMASK() macro for left shifting integers.
Signed-off-by sayli karnik <[email protected]>
---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro
 drivers/staging/iio/cdc/ad7152.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/staging/iio/cdc/ad7152.c +0 -0
index e8609b8..ab94fad 100644
@@ -47,28 +47,28 @@
 #define AD7152_STATUS_PWDN		BIT(7)
 

Quoted 16 lines

drivers/staging/iio/cdc/ad7152.c line 55

Quoted 13 lines

drivers/staging/iio/cdc/ad7152.c +6 -6

 /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
-#define AD7152_SETUP_CAPDIFF		(1 << 5)
-#define AD7152_SETUP_RANGE_2pF		(0 << 6)
-#define AD7152_SETUP_RANGE_0_5pF	(1 << 6)
-#define AD7152_SETUP_RANGE_1pF		(2 << 6)
-#define AD7152_SETUP_RANGE_4pF		(3 << 6)
-#define AD7152_SETUP_RANGE(x)		((x) << 6)
+#define AD7152_SETUP_CAPDIFF		GENMASK(1, 5)
+#define AD7152_SETUP_RANGE_2pF		GENMASK(0, 6)
+#define AD7152_SETUP_RANGE_0_5pF	GENMASK(1, 6)
+#define AD7152_SETUP_RANGE_1pF		GENMASK(2, 6)
+#define AD7152_SETUP_RANGE_4pF		GENMASK(3, 6)
+#define AD7152_SETUP_RANGE(x)		GENMASK(x, 6)

I am a bit confused about the usage of GENMASK here. Unless I completely misunderstand the macro these are not at all equivalent to the previous bitshifts as most of them just expand to 0.

Indeed. Sorry not to have been more attentive. The comment before the definition of GENMASK says:

/*
* Create a contiguous bitmask starting at bit position @l and ending at
* position @h. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000. */

Here is an example of a commit that converts BIT calls to use GENMASK:

408fec3ff8

But it doesn't seem very convenient for the code modified here.

julia

Quoted 46 lines

Quoted 11 lines
drivers/staging/iio/cdc/ad7152.c +2 -7
 /* Config Register Bit Designations (AD7152_REG_CFG) */
-#define AD7152_CONF_CH2EN		(1 << 3)
-#define AD7152_CONF_CH1EN		(1 << 4)
-#define AD7152_CONF_MODE_IDLE		(0 << 0)
-#define AD7152_CONF_MODE_CONT_CONV	(1 << 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_CH2EN		GENMASK(1, 3)
+#define AD7152_CONF_CH1EN		GENMASK(1, 4)
drivers/staging/iio/cdc/ad7152.c line 67
Quoted 10 lines
drivers/staging/iio/cdc/ad7152.c +6 -2
+#define AD7152_CONF_MODE_IDLE		GENMASK(0, 0)
+#define AD7152_CONF_MODE_CONT_CONV	GENMASK(1, 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		(1 << 7)
-#define AD7152_CAPDAC_DACP(x)		((x) & 0x1F)
+#define AD7152_CAPDAC_DACEN		GENMASK(1, 7)
same here
drivers/staging/iio/cdc/ad7152.c line 68
Quoted 1 line
drivers/staging/iio/cdc/ad7152.c +1 -0
+#define AD7152_CAPDAC_DACP(x)		GENMASK(x, 0x1F)
this is more something like ((x) & GENMASK(5,0)).
Quoted 9 lines
drivers/staging/iio/cdc/ad7152.c +1 -2
 /* 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
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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/20170220094845.GA17071%40psyche.fritz.box.
For more options, visit https://groups.google.com/d/optout.

sayli karnik

author

2017-02-21 14:01 UTC #9 lore raw

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

Quoted 23 lines · Julia Lawall

Quoted 19 lines · Hendrik v. Raven
Quoted 18 lines · sayli karnik
Use GENMASK() macro for left shifting integers.
Signed-off-by sayli karnik <[email protected]>
---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro
 drivers/staging/iio/cdc/ad7152.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/staging/iio/cdc/ad7152.c +0 -0
index e8609b8..ab94fad 100644
@@ -47,28 +47,28 @@
 #define AD7152_STATUS_PWDN         BIT(7)
 

Quoted 17 lines

Quoted 8 lines

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

 /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
-#define AD7152_SETUP_CAPDIFF               (1 << 5)
-#define AD7152_SETUP_RANGE_2pF             (0 << 6)
-#define AD7152_SETUP_RANGE_0_5pF   (1 << 6)
-#define AD7152_SETUP_RANGE_1pF             (2 << 6)
-#define AD7152_SETUP_RANGE_4pF             (3 << 6)
-#define AD7152_SETUP_RANGE(x)              ((x) << 6)
+#define AD7152_SETUP_CAPDIFF               GENMASK(1, 5)

Quoted 8 lines

drivers/staging/iio/cdc/ad7152.c line 55
Quoted 5 lines
drivers/staging/iio/cdc/ad7152.c +5 -0
+#define AD7152_SETUP_RANGE_2pF             GENMASK(0, 6)
+#define AD7152_SETUP_RANGE_0_5pF   GENMASK(1, 6)
+#define AD7152_SETUP_RANGE_1pF             GENMASK(2, 6)
+#define AD7152_SETUP_RANGE_4pF             GENMASK(3, 6)
+#define AD7152_SETUP_RANGE(x)              GENMASK(x, 6)
I am a bit confused about the usage of GENMASK here. Unless I completely misunderstand the macro these are not at all equivalent to the previous bitshifts as most of them just expand to 0.

Hello,
Could you explain how these aren't equivalent? And should we drop the plan to use genmask here altogether?

Quoted 9 lines

Indeed. Sorry not to have been more attentive. The comment before the definition of GENMASK says:
/*
* Create a contiguous bitmask starting at bit position @l and ending at
* position @h. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000. */

Quoted 4 lines

Here is an example of a commit that converts BIT calls to use GENMASK:
408fec3ff8

Okay so the above commit concludes that BIT(x) | BIT(y) equals GENMASK(x, y) But how do we use this for the code here?

thanks,
sayli

Quoted 29 lines

But it doesn't seem very convenient for the code modified here.

julia

Quoted 25 lines

Quoted 11 lines

drivers/staging/iio/cdc/ad7152.c +2 -7

 /* Config Register Bit Designations (AD7152_REG_CFG) */
-#define AD7152_CONF_CH2EN          (1 << 3)
-#define AD7152_CONF_CH1EN          (1 << 4)
-#define AD7152_CONF_MODE_IDLE              (0 << 0)
-#define AD7152_CONF_MODE_CONT_CONV (1 << 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_CH2EN          GENMASK(1, 3)
+#define AD7152_CONF_CH1EN          GENMASK(1, 4)

drivers/staging/iio/cdc/ad7152.c line 67

Quoted 10 lines

drivers/staging/iio/cdc/ad7152.c +6 -2

+#define AD7152_CONF_MODE_IDLE              GENMASK(0, 0)
+#define AD7152_CONF_MODE_CONT_CONV GENMASK(1, 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                (1 << 7)
-#define AD7152_CAPDAC_DACP(x)              ((x) & 0x1F)
+#define AD7152_CAPDAC_DACEN                GENMASK(1, 7)

same here

drivers/staging/iio/cdc/ad7152.c line 68

Quoted 1 line

drivers/staging/iio/cdc/ad7152.c +1 -0
+#define AD7152_CAPDAC_DACP(x)              GENMASK(x, 0x1F)

this is more something like ((x) & GENMASK(5,0)).

Sorry, my bad here!

Quoted 21 lines

Quoted 21 lines
Quoted 9 lines
drivers/staging/iio/cdc/ad7152.c +1 -2
 /* 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
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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/20170220094845.GA17071%40psyche.fritz.box.
For more options, visit https://groups.google.com/d/optout.

Julia Lawall

2017-02-21 14:09 UTC #10 lore raw

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

Quoted 41 lines · sayli karnik

Quoted 23 lines · Julia Lawall
Quoted 19 lines · Hendrik v. Raven
Quoted 18 lines · sayli karnik
Use GENMASK() macro for left shifting integers.
Signed-off-by sayli karnik <[email protected]>
---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro
 drivers/staging/iio/cdc/ad7152.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/staging/iio/cdc/ad7152.c +0 -0
index e8609b8..ab94fad 100644
@@ -47,28 +47,28 @@
 #define AD7152_STATUS_PWDN         BIT(7)
 
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/staging/iio/cdc/ad7152.c +1 -6
 /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
-#define AD7152_SETUP_CAPDIFF               (1 << 5)
-#define AD7152_SETUP_RANGE_2pF             (0 << 6)
-#define AD7152_SETUP_RANGE_0_5pF   (1 << 6)
-#define AD7152_SETUP_RANGE_1pF             (2 << 6)
-#define AD7152_SETUP_RANGE_4pF             (3 << 6)
-#define AD7152_SETUP_RANGE(x)              ((x) << 6)
+#define AD7152_SETUP_CAPDIFF               GENMASK(1, 5)
Quoted 8 lines
drivers/staging/iio/cdc/ad7152.c line 55
Quoted 5 lines
drivers/staging/iio/cdc/ad7152.c +5 -0
+#define AD7152_SETUP_RANGE_2pF             GENMASK(0, 6)
+#define AD7152_SETUP_RANGE_0_5pF   GENMASK(1, 6)
+#define AD7152_SETUP_RANGE_1pF             GENMASK(2, 6)
+#define AD7152_SETUP_RANGE_4pF             GENMASK(3, 6)
+#define AD7152_SETUP_RANGE(x)              GENMASK(x, 6)
I am a bit confused about the usage of GENMASK here. Unless I completely misunderstand the macro these are not at all equivalent to the previous bitshifts as most of them just expand to 0.

Quoted 3 lines

Hello,
Could you explain how these aren't equivalent? And should we drop the plan to use genmask here altogether?

Consider something like 5 << 6. 5 has the bit pattern 0101. There are not contiguous 1s in this bit pattern. So all you could do with GENMASK is make two one-bit masks and or them together. Or you could do the same with BIT. But probably neither is a big conceptual improvement over 5 <<
6. So it seems that GENMASK is not so useful in this context, as far as I can see.

julia

Quoted 79 lines

Quoted 10 lines
Indeed. Sorry not to have been more attentive. The comment before the definition of GENMASK says:
/*
* Create a contiguous bitmask starting at bit position @l and ending at
* position @h. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000. */
Quoted 4 lines
Here is an example of a commit that converts BIT calls to use GENMASK:
408fec3ff8
Okay so the above commit concludes that BIT(x) | BIT(y) equals GENMASK(x, y) But how do we use this for the code here?
thanks,
sayli
Quoted 29 lines
But it doesn't seem very convenient for the code modified here.
julia
Quoted 25 lines
Quoted 11 lines
drivers/staging/iio/cdc/ad7152.c +2 -7
 /* Config Register Bit Designations (AD7152_REG_CFG) */
-#define AD7152_CONF_CH2EN          (1 << 3)
-#define AD7152_CONF_CH1EN          (1 << 4)
-#define AD7152_CONF_MODE_IDLE              (0 << 0)
-#define AD7152_CONF_MODE_CONT_CONV (1 << 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_CH2EN          GENMASK(1, 3)
+#define AD7152_CONF_CH1EN          GENMASK(1, 4)
drivers/staging/iio/cdc/ad7152.c line 67
Quoted 10 lines
drivers/staging/iio/cdc/ad7152.c +6 -2
+#define AD7152_CONF_MODE_IDLE              GENMASK(0, 0)
+#define AD7152_CONF_MODE_CONT_CONV GENMASK(1, 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                (1 << 7)
-#define AD7152_CAPDAC_DACP(x)              ((x) & 0x1F)
+#define AD7152_CAPDAC_DACEN                GENMASK(1, 7)
same here
drivers/staging/iio/cdc/ad7152.c line 68
Quoted 1 line
drivers/staging/iio/cdc/ad7152.c +1 -0
+#define AD7152_CAPDAC_DACP(x)              GENMASK(x, 0x1F)
this is more something like ((x) & GENMASK(5,0)).
Sorry, my bad here!
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/staging/iio/cdc/ad7152.c +1 -2
 /* 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
Quoted 12 lines
Quoted 5 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 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/20170220094845.GA17071%40psyche.fritz.box.
For more options, visit https://groups.google.com/d/optout.
--
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/CAKG5xWiqR95XiVz8zu_fR5kFdvjqYSDf1fEg_uDNDRax0Nqj%2Bw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Jonathan Cameron

maintainer

2017-02-25 16:48 UTC #3 lore raw

Re: [Outreachy kernel] [PATCH v2 1/2] staging: iio: ad7152: Use BIT() macro for left shifting 1

In reply to Julia Lawall

Quoted 10 lines · Julia Lawall

Quoted 4 lines · sayli karnik
Replace left shifting on 1 with the BIT(x) macro as suggested by checkpatch.pl.
Signed-off-by sayli karnik <[email protected]>
Acked-by Julia Lawall <[email protected]>

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

For obvious reasons I'm going to leave patch 2 alone!

Thanks,

Jonathan

Quoted 38 lines

Quoted 38 lines
---
v2:
Used GENMASK() macro instead of BIT() macro for multi-bit bitfields. Removed extra parentheses around argument to macro
 drivers/staging/iio/cdc/ad7152.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/iio/cdc/ad7152.c +4 -5
index b91b50f..e8609b8 100644
@@ -41,10 +41,10 @@
 #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)
--
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/06283c7cbccd40a7214bf4f047c3829e53b6d448.1487483384.git.karniksayli1995%40gmail.com.
For more options, visit https://groups.google.com/d/optout.