On Sat, Jan 6, 2018 at 6:12 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Thu, 4 Jan 2018 22:06:31 +0530 > Sumit Pundir <pundirsumit11@xxxxxxxxx> 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@xxxxxxxxx> > > 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@xxxxxxxxxx> wrote: > On Thu, 4 Jan 2018 22:06:31 +0530 > Sumit Pundir <pundirsumit11@xxxxxxxxx> 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@xxxxxxxxx> > > 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) */ > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
