[PATCH v2 0/2] Use BIT() and GENMASK() macros

[PATCH v2 0/2] Use BIT() and GENMASK() macros

[sayli karnik]

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(-)

-- 
2.7.4

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

[sayli karnik]

Replace left shifting on 1 with the BIT(x) macro as suggested by
checkpatch.pl.

Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
---
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(-)

diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
index b91b50f..e8609b8 100644
--- a/drivers/staging/iio/cdc/ad7152.c
+++ b/drivers/staging/iio/cdc/ad7152.c
@@ -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

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

[sayli karnik]

Use GENMASK() macro for left shifting integers.

Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
---
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(-)

diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
index e8609b8..ab94fad 100644
--- a/drivers/staging/iio/cdc/ad7152.c
+++ b/drivers/staging/iio/cdc/ad7152.c
@@ -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

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

[Julia Lawall]

On Mon, 20 Feb 2017, sayli karnik wrote:

> Replace left shifting on 1 with the BIT(x) macro as suggested by
> checkpatch.pl.
>
> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

> ---
> 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(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> index b91b50f..e8609b8 100644
> --- a/drivers/staging/iio/cdc/ad7152.c
> +++ b/drivers/staging/iio/cdc/ad7152.c
> @@ -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 outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> 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.
>

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

[Julia Lawall]

On Mon, 20 Feb 2017, sayli karnik wrote:

> Use GENMASK() macro for left shifting integers.
>
> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>

It looks nicer this way :)

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

> ---
> 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(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> index e8609b8..ab94fad 100644
> --- a/drivers/staging/iio/cdc/ad7152.c
> +++ b/drivers/staging/iio/cdc/ad7152.c
> @@ -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 outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> 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.
>

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

[Lars-Peter Clausen]

On 02/19/2017 09:00 PM, sayli karnik wrote:
> Use GENMASK() macro for left shifting integers.
> 
> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> ---
> 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(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> index e8609b8..ab94fad 100644
> --- a/drivers/staging/iio/cdc/ad7152.c
> +++ b/drivers/staging/iio/cdc/ad7152.c
> @@ -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().

> [...]
> -#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.

>  /* Capdac Register Bit Designations (AD7152_REG_CAPDAC_XXX) */
> -#define AD7152_CAPDAC_DACEN		(1 << 7)
> [...]
> +#define AD7152_CAPDAC_DACEN		GENMASK(1, 7)

Same for DACEN.

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

[Hendrik v. Raven]

On Mon, Feb 20, 2017 at 01:30:27AM +0530, sayli karnik wrote:
> Use GENMASK() macro for left shifting integers.
> 
> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> ---
> 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(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> index e8609b8..ab94fad 100644
> --- a/drivers/staging/iio/cdc/ad7152.c
> +++ b/drivers/staging/iio/cdc/ad7152.c
> @@ -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)
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.

>  /* 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)
same here
> +#define AD7152_CAPDAC_DACP(x)		GENMASK(x, 0x1F)
this is more something like ((x) & GENMASK(5,0)).

>  /* 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
> 
> --
> 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

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

[Julia Lawall]

On Mon, 20 Feb 2017, Hendrik v. Raven wrote:

> On Mon, Feb 20, 2017 at 01:30:27AM +0530, sayli karnik wrote:
> > Use GENMASK() macro for left shifting integers.
> >
> > Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> > index e8609b8..ab94fad 100644
> > --- a/drivers/staging/iio/cdc/ad7152.c
> > +++ b/drivers/staging/iio/cdc/ad7152.c
> > @@ -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)
> 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

>
> >  /* 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)
> same here
> > +#define AD7152_CAPDAC_DACP(x)		GENMASK(x, 0x1F)
> this is more something like ((x) & GENMASK(5,0)).
>
> >  /* 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
> >
> > --
> > 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
>
> --
> 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 outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> 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.
>

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

[sayli karnik]

On Mon, Feb 20, 2017 at 4:10 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Mon, 20 Feb 2017, Hendrik v. Raven wrote:
>
>> On Mon, Feb 20, 2017 at 01:30:27AM +0530, sayli karnik wrote:
>> > Use GENMASK() macro for left shifting integers.
>> >
>> > Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
>> > ---
>> > 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(-)
>> >
>> > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
>> > index e8609b8..ab94fad 100644
>> > --- a/drivers/staging/iio/cdc/ad7152.c
>> > +++ b/drivers/staging/iio/cdc/ad7152.c
>> > @@ -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)
>> 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?

> 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
>
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
> But it doesn't seem very convenient for the code modified here.
>
> julia
>
>>
>> >  /* 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)
>> same here
>> > +#define AD7152_CAPDAC_DACP(x)              GENMASK(x, 0x1F)
>> this is more something like ((x) & GENMASK(5,0)).
>>
Sorry, my bad here!

>> >  /* 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
>> >
>> > --
>> > 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
>>
>> --
>> 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 outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> 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.
>>

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

[Julia Lawall]

On Tue, 21 Feb 2017, sayli karnik wrote:

> On Mon, Feb 20, 2017 at 4:10 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Mon, 20 Feb 2017, Hendrik v. Raven wrote:
> >
> >> On Mon, Feb 20, 2017 at 01:30:27AM +0530, sayli karnik wrote:
> >> > Use GENMASK() macro for left shifting integers.
> >> >
> >> > Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> >> > ---
> >> > 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(-)
> >> >
> >> > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> >> > index e8609b8..ab94fad 100644
> >> > --- a/drivers/staging/iio/cdc/ad7152.c
> >> > +++ b/drivers/staging/iio/cdc/ad7152.c
> >> > @@ -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)
> >> 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?

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


>
> > 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
> >
> 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
> > But it doesn't seem very convenient for the code modified here.
> >
> > julia
> >
> >>
> >> >  /* 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)
> >> same here
> >> > +#define AD7152_CAPDAC_DACP(x)              GENMASK(x, 0x1F)
> >> this is more something like ((x) & GENMASK(5,0)).
> >>
> Sorry, my bad here!
>
> >> >  /* 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
> >> >
> >> > --
> >> > 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
> >>
> >> --
> >> 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 outreachy-kernel+unsubscribe@googlegroups.com.
> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >> 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 outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> 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.
>

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

[Jonathan Cameron]

On 19/02/17 20:02, Julia Lawall wrote:
> 
> 
> On Mon, 20 Feb 2017, sayli karnik wrote:
> 
>> Replace left shifting on 1 with the BIT(x) macro as suggested by
>> checkpatch.pl.
>>
>> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> 
> Acked-by: Julia Lawall <julia.lawall@lip6.fr>
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
> 
>> ---
>> 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(-)
>>
>> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
>> index b91b50f..e8609b8 100644
>> --- a/drivers/staging/iio/cdc/ad7152.c
>> +++ b/drivers/staging/iio/cdc/ad7152.c
>> @@ -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 outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> 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.
>>