Re: [PATCH 07/10] Staging: comedi: fix BIT macro issue in das6402.c

[Ian Abbott]

On 01/06/16 11:26, Ravishankar Karkala Mallikarjunayya wrote:
> This patch Replace all occurences of (1<<x) by BIT(x) in the file das6402.c
> to get rid of checkpatch.pl "CHECK" output "Prefer using the BIT macro"
>
> Signed-off-by: Ravishankar Karkala Mallikarjunayya <ravishankarkm32@xxxxxxxxx>
> ---
>   drivers/staging/comedi/drivers/das6402.c | 56 ++++++++++++++++----------------
>   1 file changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/das6402.c b/drivers/staging/comedi/drivers/das6402.c
> index 1701294..f6319b3 100644
> --- a/drivers/staging/comedi/drivers/das6402.c
> +++ b/drivers/staging/comedi/drivers/das6402.c
[snip]
>   #define DAS6402_CTRL_REG		0x09
>   #define DAS6402_CTRL_SOFT_TRIG		(0 << 0)
> -#define DAS6402_CTRL_EXT_FALL_TRIG	(1 << 0)
> +#define DAS6402_CTRL_EXT_FALL_TRIG	BIT(0)
>   #define DAS6402_CTRL_EXT_RISE_TRIG	(2 << 0)
>   #define DAS6402_CTRL_PACER_TRIG		(3 << 0)

In cases like this, where there are several values defined for a 
multi-bit field, all shifted by the same amount, the 'BIT(x)' usage 
makes the code more unreadable.  The DAS6402_CTRL_SOFT_TRIG, 
DAS6402_CTRL_EXT_FALL_TRIG, DAS6402_CTRL_EXT_RISE_TRIG, and 
DAS6402_CTRL_PACER_TRIG values should all share the same style, perhaps 
something like this:

#define DAS6402_CTRL_TRIG(x)		((x) << 0)
#define DAS6402_CTRL_SOFT_TRIG		DAS6402_CTRL_TRIG(0)
#define DAS6402_CTRL_EXT_FALL_TRIG	DAS6402_CTRL_TRIG(1)
#define DAS6402_CTRL_EXT_RISE_TRIG	DAS6402_CTRL_TRIG(2)
#define DAS6402_CTRL_PACER_TRIG		DAS6402_CTRL_TRIG(3)

[snip]
>   #define DAS6402_MODE_REG		0x0b
>   #define DAS6402_MODE_RANGE(x)		((x) << 0)
>   #define DAS6402_MODE_POLLED		(0 << 2)
> -#define DAS6402_MODE_FIFONEPTY		(1 << 2)
> +#define DAS6402_MODE_FIFONEPTY		BIT(2)
>   #define DAS6402_MODE_FIFOHFULL		(2 << 2)
>   #define DAS6402_MODE_EOB		(3 << 2)

Similarly, DAS6402_MODE_POLLED, DAS6402_MODE_FIFONEPTY, 
DAS6402_MODE_FIFOHFULL, and DAS6402_MODE_EOB are all values for the same 
multi-bit field, and should be defined in the same style, perhaps 
something like this:

#define DAS6402_MODE_BUF(x)		((x) << 2)
#define DAS6402_MODE_BUF_POLLED		DAS6402_MODE_BUF(0)
#define DAS6402_MODE_FIFONEPTY		DAS6402_MODE_BUF(1)
#define DAS6402_MODE_FIFOHFULL		DAS6402_MODE_BUF(2)
#define DAS6402_MODE_EOB		DAS6402_MODE_BUF(3)

> -#define DAS6402_MODE_ENHANCED		(1 << 4)
> -#define DAS6402_MODE_SE			(1 << 5)
> -#define DAS6402_MODE_UNI		(1 << 6)
> +#define DAS6402_MODE_ENHANCED		BIT(4)
> +#define DAS6402_MODE_SE			BIT(5)
> +#define DAS6402_MODE_UNI		BIT(6)
>   #define DAS6402_MODE_DMA1		(0 << 7)
> -#define DAS6402_MODE_DMA3		(1 << 7)
> +#define DAS6402_MODE_DMA3		BIT(7)

Similarly, DAS6402_MODE_DMA1 and DAS6402_MODE_DMA3 should be defined in 
the same style, perhaps something like this:

#define DAS6402_MODE_DMA(x)		((x) << 7)
#define DAS6402_MODE_DMA1		DAS6402_MODE_DMA(0)
#define DAS6402_MODE_DMA3		DAS6402_MODE_DMA(1)


--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@xxxxxxxxx> )=-
-=(                          Web: http://www.mev.co.uk/  )=-