[PATCH] Staging: iio: Prefer using BIT macro

[PATCH] Staging: iio: Prefer using BIT macro

[Sumit Pundir]

This patch fixes the following checkpatch.pl error at multiple lines:

CHECK: Prefer using the BIT macro

Signed-off-by: Sumit Pundir <pundirsumit11@gmail.com>
---
 drivers/staging/iio/cdc/ad7152.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
index 59d1b35..b2b15b9 100644
--- a/drivers/staging/iio/cdc/ad7152.c
+++ b/drivers/staging/iio/cdc/ad7152.c
@@ -47,24 +47,24 @@
 #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) */
-- 
2.7.4

Re: [PATCH] Staging: iio: Prefer using BIT macro

[Jonathan Cameron]

On Thu,  4 Jan 2018 22:06:31 +0530
Sumit Pundir <pundirsumit11@gmail.com> wrote:

Patch title needs to mention the specific driver being changed.

> This patch fixes the following checkpatch.pl error at multiple lines:
> 
> CHECK: Prefer using the BIT macro
> 
> Signed-off-by: Sumit Pundir <pundirsumit11@gmail.com>

The BIT Macros is just fine if they are actually a single bit field.
Some of these are not. See below.

You really have to check the datasheet to be sure on these though 
in the cases here it's obvious from the surrounding lines.

Changing those ones to BIT will actively hurt readability.

Anyhow, the other cases are good so if you can prepare a patch
changing just that and ensd it that would be great.
> ---
>  drivers/staging/iio/cdc/ad7152.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> index 59d1b35..b2b15b9 100644
> --- a/drivers/staging/iio/cdc/ad7152.c
> +++ b/drivers/staging/iio/cdc/ad7152.c
> @@ -47,24 +47,24 @@
>  #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)

This is indeed a 1 bit field so fine.

>  #define AD7152_SETUP_RANGE_2pF		(0 << 6)
> -#define AD7152_SETUP_RANGE_0_5pF	(1 << 6)
> +#define AD7152_SETUP_RANGE_0_5pF	BIT(6)
This is clearly putting the value 1 in a 2 bit field within
the register - BIT macro obscures this compeltely.
>  #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)

These two are valid I think.

>  #define AD7152_CONF_MODE_IDLE		(0 << 0)
> -#define AD7152_CONF_MODE_CONT_CONV	(1 << 0)
> +#define AD7152_CONF_MODE_CONT_CONV	BIT(0)

This one is not. Again clear from the code let alone checking the
datasheet. We write 6 to the same location.

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

This one is a 1 bit field so fine.

>  #define AD7152_CAPDAC_DACP(x)		((x) & 0x1F)
>  
>  /* CFG2 Register Bit Designations (AD7152_REG_CFG2) */

Re: [PATCH] Staging: iio: Prefer using BIT macro

[Sumit Pundir]

On Sat, Jan 6, 2018 at 6:12 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Thu,  4 Jan 2018 22:06:31 +0530
> Sumit Pundir <pundirsumit11@gmail.com> wrote:
>
> Patch title needs to mention the specific driver being changed.
>
>> This patch fixes the following checkpatch.pl error at multiple lines:
>>
>> CHECK: Prefer using the BIT macro
>>
>> Signed-off-by: Sumit Pundir <pundirsumit11@gmail.com>
>
> The BIT Macros is just fine if they are actually a single bit field.
> Some of these are not. See below.
>
> You really have to check the datasheet to be sure on these though
> in the cases here it's obvious from the surrounding lines.
>
> Changing those ones to BIT will actively hurt readability.
>
> Anyhow, the other cases are good so if you can prepare a patch
> changing just that and ensd it that would be great.
>> ---
>>  drivers/staging/iio/cdc/ad7152.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
>> index 59d1b35..b2b15b9 100644
>> --- a/drivers/staging/iio/cdc/ad7152.c
>> +++ b/drivers/staging/iio/cdc/ad7152.c
>> @@ -47,24 +47,24 @@
>>  #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)
>
> This is indeed a 1 bit field so fine.
>
>>  #define AD7152_SETUP_RANGE_2pF               (0 << 6)
>> -#define AD7152_SETUP_RANGE_0_5pF     (1 << 6)
>> +#define AD7152_SETUP_RANGE_0_5pF     BIT(6)
> This is clearly putting the value 1 in a 2 bit field within
> the register - BIT macro obscures this compeltely.
>>  #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)
>
> These two are valid I think.
>
>>  #define AD7152_CONF_MODE_IDLE                (0 << 0)
>> -#define AD7152_CONF_MODE_CONT_CONV   (1 << 0)
>> +#define AD7152_CONF_MODE_CONT_CONV   BIT(0)
>
> This one is not. Again clear from the code let alone checking the
> datasheet. We write 6 to the same location.
>
>>  #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)
>
> This one is a 1 bit field so fine.
>

Hi Jonathan,

I will send a v2 of this patch with all the prescribed changes.

Thanks,
Sumit

On Sat, Jan 6, 2018 at 6:12 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Thu,  4 Jan 2018 22:06:31 +0530
> Sumit Pundir <pundirsumit11@gmail.com> wrote:
>
> Patch title needs to mention the specific driver being changed.
>
>> This patch fixes the following checkpatch.pl error at multiple lines:
>>
>> CHECK: Prefer using the BIT macro
>>
>> Signed-off-by: Sumit Pundir <pundirsumit11@gmail.com>
>
> The BIT Macros is just fine if they are actually a single bit field.
> Some of these are not. See below.
>
> You really have to check the datasheet to be sure on these though
> in the cases here it's obvious from the surrounding lines.
>
> Changing those ones to BIT will actively hurt readability.
>
> Anyhow, the other cases are good so if you can prepare a patch
> changing just that and ensd it that would be great.
>> ---
>>  drivers/staging/iio/cdc/ad7152.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
>> index 59d1b35..b2b15b9 100644
>> --- a/drivers/staging/iio/cdc/ad7152.c
>> +++ b/drivers/staging/iio/cdc/ad7152.c
>> @@ -47,24 +47,24 @@
>>  #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)
>
> This is indeed a 1 bit field so fine.
>
>>  #define AD7152_SETUP_RANGE_2pF               (0 << 6)
>> -#define AD7152_SETUP_RANGE_0_5pF     (1 << 6)
>> +#define AD7152_SETUP_RANGE_0_5pF     BIT(6)
> This is clearly putting the value 1 in a 2 bit field within
> the register - BIT macro obscures this compeltely.
>>  #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)
>
> These two are valid I think.
>
>>  #define AD7152_CONF_MODE_IDLE                (0 << 0)
>> -#define AD7152_CONF_MODE_CONT_CONV   (1 << 0)
>> +#define AD7152_CONF_MODE_CONT_CONV   BIT(0)
>
> This one is not. Again clear from the code let alone checking the
> datasheet. We write 6 to the same location.
>
>>  #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)
>
> This one is a 1 bit field so fine.
>
>>  #define AD7152_CAPDAC_DACP(x)                ((x) & 0x1F)
>>
>>  /* CFG2 Register Bit Designations (AD7152_REG_CFG2) */
>

Re: [PATCH] Staging: iio: Prefer using BIT macro

[Andy Shevchenko]

On Sat, Jan 6, 2018 at 2:42 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Thu,  4 Jan 2018 22:06:31 +0530

>>  /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
>> -#define AD7152_SETUP_CAPDIFF         (1 << 5)
>> +#define AD7152_SETUP_CAPDIFF         BIT(5)
>
> This is indeed a 1 bit field so fine.

But shouldn't we prevent style over the module? Otherwise it might be
hard to decode one field from the other because of style differences.

>>  #define AD7152_SETUP_RANGE_2pF               (0 << 6)
>> -#define AD7152_SETUP_RANGE_0_5pF     (1 << 6)
>> +#define AD7152_SETUP_RANGE_0_5pF     BIT(6)
> This is clearly putting the value 1 in a 2 bit field within
> the register - BIT macro obscures this compeltely.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Staging: iio: Prefer using BIT macro

[Jonathan Cameron]

On Mon, 8 Jan 2018 01:30:29 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Jan 6, 2018 at 2:42 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Thu,  4 Jan 2018 22:06:31 +0530  
> 
> >>  /* Setup Register Bit Designations (AD7152_REG_CHx_SETUP) */
> >> -#define AD7152_SETUP_CAPDIFF         (1 << 5)
> >> +#define AD7152_SETUP_CAPDIFF         BIT(5)  
> >
> > This is indeed a 1 bit field so fine.  
> 
> But shouldn't we prevent style over the module? Otherwise it might be
> hard to decode one field from the other because of style differences.

I'm not sure using BIT for single bit fields really makes it much harder
to read, but perhaps you are right.  Anyhow, can't say I feel strongly
about this one either way!

Jonathan

> 
> >>  #define AD7152_SETUP_RANGE_2pF               (0 << 6)
> >> -#define AD7152_SETUP_RANGE_0_5pF     (1 << 6)
> >> +#define AD7152_SETUP_RANGE_0_5pF     BIT(6)  
> > This is clearly putting the value 1 in a 2 bit field within
> > the register - BIT macro obscures this compeltely.  
>