Mailing List Archive: [PATCH v2] staging: iio: Replace a bit shift by a use of BIT.

[PATCH v2] staging: iio: Replace a bit shift by a use of BIT.

arushisinghal19971997 at gmail

Mar 22, 2017, 2:20 AM

Post #1 of 6 (25 views)

This patch replaces bit shifting on 1 with the BIT(x) macro.
This was done with coccinelle:
@@
constant c;
@@

-1 << c
+BIT(c)

Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
---
changes in v2
-change the cc list

drivers/staging/iio/adc/ad7816.c | 2 +-
drivers/staging/iio/cdc/ad7150.c | 2 +-
drivers/staging/iio/cdc/ad7746.c | 22 +++++++++++-----------
3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 17d280581e24..42f637ca0251 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -222,7 +222,7 @@ static ssize_t ad7816_show_value(struct device *dev,
value = (s8)((data >> AD7816_TEMP_FLOAT_OFFSET) - 103);
data &= AD7816_TEMP_FLOAT_MASK;
if (value < 0)
- data = (1 << AD7816_TEMP_FLOAT_OFFSET) - data;
+ data = (BIT(AD7816_TEMP_FLOAT_OFFSET)) - data;
return sprintf(buf, "%d.%.2d\n", value, data * 25);
}
return sprintf(buf, "%u\n", data);
diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index ca72af3e9d4b..c5574b3ee939 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -232,7 +232,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
if (ret < 0)
goto error_ret;

- cfg = ret & ~((0x03 << 5) | (0x1 << 7));
+ cfg = ret & ~((0x03 << 5) | (BIT(7)));

switch (type) {
case IIO_EV_TYPE_MAG_ADAPTIVE:
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 81f8b9ee1120..8c573817826f 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -45,20 +45,20 @@
#define AD7746_STATUS_RDYCAP BIT(0)

/* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) */
-#define AD7746_CAPSETUP_CAPEN (1 << 7)
-#define AD7746_CAPSETUP_CIN2 (1 << 6) /* AD7746 only */
-#define AD7746_CAPSETUP_CAPDIFF (1 << 5)
-#define AD7746_CAPSETUP_CACHOP (1 << 0)
+#define AD7746_CAPSETUP_CAPEN (BIT(7))
+#define AD7746_CAPSETUP_CIN2 (BIT(6)) /* AD7746 only */
+#define AD7746_CAPSETUP_CAPDIFF (BIT(5))
+#define AD7746_CAPSETUP_CACHOP (BIT(0))

/* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SETUP) */
-#define AD7746_VTSETUP_VTEN (1 << 7)
+#define AD7746_VTSETUP_VTEN (BIT(7))
#define AD7746_VTSETUP_VTMD_INT_TEMP (0 << 5)
-#define AD7746_VTSETUP_VTMD_EXT_TEMP (1 << 5)
+#define AD7746_VTSETUP_VTMD_EXT_TEMP (BIT(5))
#define AD7746_VTSETUP_VTMD_VDD_MON (2 << 5)
#define AD7746_VTSETUP_VTMD_EXT_VIN (3 << 5)
-#define AD7746_VTSETUP_EXTREF (1 << 4)
-#define AD7746_VTSETUP_VTSHORT (1 << 1)
-#define AD7746_VTSETUP_VTCHOP (1 << 0)
+#define AD7746_VTSETUP_EXTREF (BIT(4))
+#define AD7746_VTSETUP_VTSHORT (BIT(1))
+#define AD7746_VTSETUP_VTCHOP (BIT(0))

/* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
#define AD7746_EXCSETUP_CLKCTRL BIT(7)
@@ -75,14 +75,14 @@
#define AD7746_CONF_VTFS_MASK GENMASK(7, 6)
#define AD7746_CONF_CAPFS_MASK GENMASK(5, 3)
#define AD7746_CONF_MODE_IDLE (0 << 0)
-#define AD7746_CONF_MODE_CONT_CONV (1 << 0)
+#define AD7746_CONF_MODE_CONT_CONV (BIT(0))
#define AD7746_CONF_MODE_SINGLE_CONV (2 << 0)
#define AD7746_CONF_MODE_PWRDN (3 << 0)
#define AD7746_CONF_MODE_OFFS_CAL (5 << 0)
#define AD7746_CONF_MODE_GAIN_CAL (6 << 0)

/* CAPDAC Register Bit Designations (AD7746_REG_CAPDACx) */
-#define AD7746_CAPDAC_DACEN (1 << 7)
+#define AD7746_CAPDAC_DACEN (BIT(7))
#define AD7746_CAPDAC_DACP(x) ((x) & 0x7F)

/*
--
2.11.0

Re: [PATCH v2] staging: iio: Replace a bit shift by a use of BIT. [ In reply to ]

lars at metafoo

Mar 22, 2017, 6:20 AM

Post #2 of 6 (23 views)

Permalink

On 03/22/2017 09:38 AM, Arushi Singhal wrote:
> This patch replaces bit shifting on 1 with the BIT(x) macro.
> This was done with coccinelle:
> @@
> constant c;
> @@
>
> -1 << c
> +BIT(c)

When it comes to doing this type of conversion semantics, i.e. the meaning
of the value, are important. The BIT() macro should only be used when the
value is really a 1 bit value. Otherwise it will be confusing to the reader
of the code.

Also one patch per driver please (Unless it is an absolutely identical
change to multiple driver, not just the same class of change).

>
> Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> ---
> changes in v2
> -change the cc list
>
> drivers/staging/iio/adc/ad7816.c | 2 +-
> drivers/staging/iio/cdc/ad7150.c | 2 +-
> drivers/staging/iio/cdc/ad7746.c | 22 +++++++++++-----------
> 3 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 17d280581e24..42f637ca0251 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -222,7 +222,7 @@ static ssize_t ad7816_show_value(struct device *dev,
> value = (s8)((data >> AD7816_TEMP_FLOAT_OFFSET) - 103);
> data &= AD7816_TEMP_FLOAT_MASK;
> if (value < 0)
> - data = (1 << AD7816_TEMP_FLOAT_OFFSET) - data;
> + data = (BIT(AD7816_TEMP_FLOAT_OFFSET)) - data;

This for example is just a really poor implementation of unsigned binary to
signed conversion (The formula I think is straight from the datasheet).
Semantically talking about a 1-bit value here makes no sense. What the code
does is to construct a value that is equal to 2**AD7816_TEMP_FLOAT_OFFSET.

> return sprintf(buf, "%d.%.2d\n", value, data * 25);
> }
> return sprintf(buf, "%u\n", data);
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index ca72af3e9d4b..c5574b3ee939 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -232,7 +232,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> if (ret < 0)
> goto error_ret;
>
> - cfg = ret & ~((0x03 << 5) | (0x1 << 7));
> + cfg = ret & ~((0x03 << 5) | (BIT(7)));

This is OK. It's a one bit field that is set here.

>
> switch (type) {
> case IIO_EV_TYPE_MAG_ADAPTIVE:
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 81f8b9ee1120..8c573817826f 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -45,20 +45,20 @@
> #define AD7746_STATUS_RDYCAP BIT(0)
>
> /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) */
> -#define AD7746_CAPSETUP_CAPEN (1 << 7)
> -#define AD7746_CAPSETUP_CIN2 (1 << 6) /* AD7746 only */
> -#define AD7746_CAPSETUP_CAPDIFF (1 << 5)
> -#define AD7746_CAPSETUP_CACHOP (1 << 0)
> +#define AD7746_CAPSETUP_CAPEN (BIT(7))
> +#define AD7746_CAPSETUP_CIN2 (BIT(6)) /* AD7746 only */
> +#define AD7746_CAPSETUP_CAPDIFF (BIT(5))
> +#define AD7746_CAPSETUP_CACHOP (BIT(0))

These are all OK, they are single bit values or flags.

>
> /* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SETUP) */
> -#define AD7746_VTSETUP_VTEN (1 << 7)
> +#define AD7746_VTSETUP_VTEN (BIT(7))
> #define AD7746_VTSETUP_VTMD_INT_TEMP (0 << 5)
> -#define AD7746_VTSETUP_VTMD_EXT_TEMP (1 << 5)
> +#define AD7746_VTSETUP_VTMD_EXT_TEMP (BIT(5))

This is not OK. This is a multi-bit field. b01 just happens to be one of the
valid values.

> #define AD7746_VTSETUP_VTMD_VDD_MON (2 << 5)
> #define AD7746_VTSETUP_VTMD_EXT_VIN (3 << 5)
> -#define AD7746_VTSETUP_EXTREF (1 << 4)
> -#define AD7746_VTSETUP_VTSHORT (1 << 1)
> -#define AD7746_VTSETUP_VTCHOP (1 << 0)
> +#define AD7746_VTSETUP_EXTREF (BIT(4))
> +#define AD7746_VTSETUP_VTSHORT (BIT(1))
> +#define AD7746_VTSETUP_VTCHOP (BIT(0))

These are OK. All single bit values.

>
> /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
> #define AD7746_EXCSETUP_CLKCTRL BIT(7)
> @@ -75,14 +75,14 @@
> #define AD7746_CONF_VTFS_MASK GENMASK(7, 6)
> #define AD7746_CONF_CAPFS_MASK GENMASK(5, 3)
> #define AD7746_CONF_MODE_IDLE (0 << 0)
> -#define AD7746_CONF_MODE_CONT_CONV (1 << 0)
> +#define AD7746_CONF_MODE_CONT_CONV (BIT(0))

This again is a multi-bit field where 1 is one of the possible values. So
not OK.

> #define AD7746_CONF_MODE_SINGLE_CONV (2 << 0)
> #define AD7746_CONF_MODE_PWRDN (3 << 0)
> #define AD7746_CONF_MODE_OFFS_CAL (5 << 0)
> #define AD7746_CONF_MODE_GAIN_CAL (6 << 0)
>
> /* CAPDAC Register Bit Designations (AD7746_REG_CAPDACx) */
> -#define AD7746_CAPDAC_DACEN (1 << 7)
> +#define AD7746_CAPDAC_DACEN (BIT(7))

OK.

> #define AD7746_CAPDAC_DACP(x) ((x) & 0x7F)
>
> /*
>

[PATCH v2] staging: iio: Replace a bit shift by a use of BIT. [ In reply to ]

arushisinghal19971997 at gmail

Mar 22, 2017, 10:20 AM

Post #3 of 6 (23 views)

Permalink

Re: [PATCH v2] staging: iio: Replace a bit shift by a use of BIT. [ In reply to ]

jic23 at kernel

Mar 22, 2017, 2:20 PM

Post #4 of 6 (23 views)

Permalink

On 22/03/17 16:42, Arushi Singhal wrote:
> This patch replaces bit shifting on 1 with the BIT(x) macro.
> This was done with coccinelle:
> @@
> constant c;
> @@
>
> -1 << c
> +BIT(c)
>
> Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan
> ---
> changes in v2
> -remove/correct the wrong code.
>
> drivers/staging/iio/cdc/ad7150.c | 2 +-
> drivers/staging/iio/cdc/ad7746.c | 16 ++++++++--------
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index ca72af3e9d4b..a6f249e9c1e1 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -232,7 +232,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> if (ret < 0)
> goto error_ret;
>
> - cfg = ret & ~((0x03 << 5) | (0x1 << 7));
> + cfg = ret & ~((0x03 << 5) | BIT(7));
>
> switch (type) {
> case IIO_EV_TYPE_MAG_ADAPTIVE:
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 81f8b9ee1120..453d3978879d 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -45,10 +45,10 @@
> #define AD7746_STATUS_RDYCAP BIT(0)
>
> /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) */
> -#define AD7746_CAPSETUP_CAPEN (1 << 7)
> -#define AD7746_CAPSETUP_CIN2 (1 << 6) /* AD7746 only */
> -#define AD7746_CAPSETUP_CAPDIFF (1 << 5)
> -#define AD7746_CAPSETUP_CACHOP (1 << 0)
> +#define AD7746_CAPSETUP_CAPEN (BIT(7))
> +#define AD7746_CAPSETUP_CIN2 (BIT(6)) /* AD7746 only */
> +#define AD7746_CAPSETUP_CAPDIFF (BIT(5))
> +#define AD7746_CAPSETUP_CACHOP (BIT(0))
>
> /* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SETUP) */
> #define AD7746_VTSETUP_VTEN (1 << 7)
> @@ -56,9 +56,9 @@
> #define AD7746_VTSETUP_VTMD_EXT_TEMP (1 << 5)
> #define AD7746_VTSETUP_VTMD_VDD_MON (2 << 5)
> #define AD7746_VTSETUP_VTMD_EXT_VIN (3 << 5)
> -#define AD7746_VTSETUP_EXTREF (1 << 4)
> -#define AD7746_VTSETUP_VTSHORT (1 << 1)
> -#define AD7746_VTSETUP_VTCHOP (1 << 0)
> +#define AD7746_VTSETUP_EXTREF (BIT(4))
> +#define AD7746_VTSETUP_VTSHORT (BIT(1))
> +#define AD7746_VTSETUP_VTCHOP (BIT(0))
>
> /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
> #define AD7746_EXCSETUP_CLKCTRL BIT(7)
> @@ -82,7 +82,7 @@
> #define AD7746_CONF_MODE_GAIN_CAL (6 << 0)
>
> /* CAPDAC Register Bit Designations (AD7746_REG_CAPDACx) */
> -#define AD7746_CAPDAC_DACEN (1 << 7)
> +#define AD7746_CAPDAC_DACEN (BIT(7))
> #define AD7746_CAPDAC_DACP(x) ((x) & 0x7F)
>
> /*
>

Re: [Outreachy kernel] [PATCH v2] staging: iio: Replace a bit shift by a use of BIT. [ In reply to ]

julia.lawall at lip6

Mar 22, 2017, 4:20 PM

Post #5 of 6 (23 views)

Permalink

On Wed, 22 Mar 2017, Arushi Singhal wrote:

> This patch replaces bit shifting on 1 with the BIT(x) macro.
> This was done with coccinelle:
> @@
> constant c;
> @@
>
> -1 << c
> +BIT(c)
>
> Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> ---
> changes in v2
> -remove/correct the wrong code.
>
> drivers/staging/iio/cdc/ad7150.c | 2 +-
> drivers/staging/iio/cdc/ad7746.c | 16 ++++++++--------
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index ca72af3e9d4b..a6f249e9c1e1 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -232,7 +232,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> if (ret < 0)
> goto error_ret;
>
> - cfg = ret & ~((0x03 << 5) | (0x1 << 7));
> + cfg = ret & ~((0x03 << 5) | BIT(7));
>
> switch (type) {
> case IIO_EV_TYPE_MAG_ADAPTIVE:
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 81f8b9ee1120..453d3978879d 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -45,10 +45,10 @@
> #define AD7746_STATUS_RDYCAP BIT(0)
>
> /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) */
> -#define AD7746_CAPSETUP_CAPEN (1 << 7)
> -#define AD7746_CAPSETUP_CIN2 (1 << 6) /* AD7746 only */
> -#define AD7746_CAPSETUP_CAPDIFF (1 << 5)
> -#define AD7746_CAPSETUP_CACHOP (1 << 0)
> +#define AD7746_CAPSETUP_CAPEN (BIT(7))
> +#define AD7746_CAPSETUP_CIN2 (BIT(6)) /* AD7746 only */
> +#define AD7746_CAPSETUP_CAPDIFF (BIT(5))
> +#define AD7746_CAPSETUP_CACHOP (BIT(0))

The parentheses here are not needed.

>
> /* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SETUP) */
> #define AD7746_VTSETUP_VTEN (1 << 7)
> @@ -56,9 +56,9 @@
> #define AD7746_VTSETUP_VTMD_EXT_TEMP (1 << 5)
> #define AD7746_VTSETUP_VTMD_VDD_MON (2 << 5)
> #define AD7746_VTSETUP_VTMD_EXT_VIN (3 << 5)
> -#define AD7746_VTSETUP_EXTREF (1 << 4)
> -#define AD7746_VTSETUP_VTSHORT (1 << 1)
> -#define AD7746_VTSETUP_VTCHOP (1 << 0)
> +#define AD7746_VTSETUP_EXTREF (BIT(4))
> +#define AD7746_VTSETUP_VTSHORT (BIT(1))
> +#define AD7746_VTSETUP_VTCHOP (BIT(0))
>

Same here.

> /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
> #define AD7746_EXCSETUP_CLKCTRL BIT(7)
> @@ -82,7 +82,7 @@
> #define AD7746_CONF_MODE_GAIN_CAL (6 << 0)
>
> /* CAPDAC Register Bit Designations (AD7746_REG_CAPDACx) */
> -#define AD7746_CAPDAC_DACEN (1 << 7)
> +#define AD7746_CAPDAC_DACEN (BIT(7))

Here too.

julia

> #define AD7746_CAPDAC_DACP(x) ((x) & 0x7F)
>
> /*
> --
> 2.11.0
>
> --
> 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/20170322164234.GA18805%40arushi-HP-Pavilion-Notebook.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH v2] staging: iio: Replace a bit shift by a use of BIT. [ In reply to ]

dan.carpenter at oracle

Mar 23, 2017, 4:20 AM

Post #6 of 6 (23 views)

Permalink

On Wed, Mar 22, 2017 at 10:12:34PM +0530, Arushi Singhal wrote:
> /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) */
> -#define AD7746_CAPSETUP_CAPEN (1 << 7)
> -#define AD7746_CAPSETUP_CIN2 (1 << 6) /* AD7746 only */
> -#define AD7746_CAPSETUP_CAPDIFF (1 << 5)
> -#define AD7746_CAPSETUP_CACHOP (1 << 0)
> +#define AD7746_CAPSETUP_CAPEN (BIT(7))
> +#define AD7746_CAPSETUP_CIN2 (BIT(6)) /* AD7746 only */
> +#define AD7746_CAPSETUP_CAPDIFF (BIT(5))
> +#define AD7746_CAPSETUP_CACHOP (BIT(0))

Extra parens.

regards,
dan carpenter