* Re: [PATCH] Staging: iio: Prefer using BIT macro
2018-01-06 12:42 ` Jonathan Cameron
@ 2018-01-06 13:33 ` Sumit Pundir
2018-01-07 23:30 ` Andy Shevchenko
1 sibling, 0 replies; 5+ messages in thread
From: Sumit Pundir @ 2018-01-06 13:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler,
Greg KH, linux-iio, devel, linux-kernel
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) */
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Staging: iio: Prefer using BIT macro
2018-01-06 12:42 ` Jonathan Cameron
2018-01-06 13:33 ` Sumit Pundir
@ 2018-01-07 23:30 ` Andy Shevchenko
2018-01-08 11:32 ` Jonathan Cameron
1 sibling, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2018-01-07 23:30 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Sumit Pundir, Lars-Peter Clausen, Michael Hennerich,
Hartmut Knaack, Peter Meerwald, Greg Kroah-Hartman, linux-iio,
devel, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread