linux-kernel - Re: [PATCH] Staging: iio: Prefer using BIT macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKunu93CPAo29-giyTEOGyU7LHZOSLfXWSSo5poB_-dMfkiDaw@mail.gmail.com>
Date:   Sat, 6 Jan 2018 19:03:09 +0530
From:   Sumit Pundir <pundirsumit11@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     lars@...afoo.de, Michael.Hennerich@...log.com, knaack.h@....de,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Staging: iio: Prefer using BIT macro

On Sat, Jan 6, 2018 at 6:12 PM, Jonathan Cameron <jic23@...nel.org> wrote:
> On Thu,  4 Jan 2018 22:06:31 +0530
> Sumit Pundir <pundirsumit11@...il.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@...il.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@...nel.org> wrote:
> On Thu,  4 Jan 2018 22:06:31 +0530
> Sumit Pundir <pundirsumit11@...il.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@...il.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) */
>