Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 31 Mar 2020 18:58:17 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> Use BIT() across the driver, since bit masking
> is better be done using the BIT macro.
> Change is done using this coccinelle script:
> 
> @bit@
> @@
> BIT(...)
> 
> @depends on bit@
> expression E;
> constant c;
> @@
> 
> (
> -(1 << E)
> +BIT(E)
> |
> -(1 << c)
> +BIT(c)
> )
> 
> Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> ---
> Change in v2:
> - removed unneeded parentheses around BIT,
>   they are not needed.
> - reverted two hunks in iss_reg.h, using BIT
>   in both places makes the code confusing to
>   understand.
> Changes suggested by Stefano Brivio.
> 
>  drivers/staging/media/omap4iss/iss.c        |  4 +-
>  drivers/staging/media/omap4iss/iss.h        | 22 +++----
>  drivers/staging/media/omap4iss/iss_csiphy.c |  6 +-
>  drivers/staging/media/omap4iss/iss_regs.h   | 72 ++++++++++-----------
>  drivers/staging/media/omap4iss/iss_video.h  | 16 ++---
>  5 files changed, 60 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index 6fb60b58447a..2dc5c1bb34f9 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -243,7 +243,7 @@ static void iss_isr_dbg(struct iss_device *iss, u32 irqstatus)
>  	dev_dbg(iss->dev, "ISS IRQ: ");
> 
>  	for (i = 0; i < ARRAY_SIZE(name); i++) {
> -		if ((1 << i) & irqstatus)
> +		if (BIT(i) & irqstatus)
>  			pr_cont("%s ", name[i]);
>  	}
>  	pr_cont("\n");

Fine.

> @@ -290,7 +290,7 @@ static void iss_isp_isr_dbg(struct iss_device *iss, u32 irqstatus)
>  	dev_dbg(iss->dev, "ISP IRQ: ");
> 
>  	for (i = 0; i < ARRAY_SIZE(name); i++) {
> -		if ((1 << i) & irqstatus)
> +		if (BIT(i) & irqstatus)
>  			pr_cont("%s ", name[i]);
>  	}
>  	pr_cont("\n");

Fine.

> diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
> index b88f9529683c..70371461b5be 100644
> --- a/drivers/staging/media/omap4iss/iss.h
> +++ b/drivers/staging/media/omap4iss/iss.h
> @@ -50,20 +50,20 @@ enum iss_mem_resources {
>  };
> 
>  enum iss_subclk_resource {
> -	OMAP4_ISS_SUBCLK_SIMCOP		= (1 << 0),
> -	OMAP4_ISS_SUBCLK_ISP		= (1 << 1),
> -	OMAP4_ISS_SUBCLK_CSI2_A		= (1 << 2),
> -	OMAP4_ISS_SUBCLK_CSI2_B		= (1 << 3),
> -	OMAP4_ISS_SUBCLK_CCP2		= (1 << 4),
> +	OMAP4_ISS_SUBCLK_SIMCOP		= BIT(0),
> +	OMAP4_ISS_SUBCLK_ISP		= BIT(1),
> +	OMAP4_ISS_SUBCLK_CSI2_A		= BIT(2),
> +	OMAP4_ISS_SUBCLK_CSI2_B		= BIT(3),
> +	OMAP4_ISS_SUBCLK_CCP2		= BIT(4),
>  };
> 
>  enum iss_isp_subclk_resource {
> -	OMAP4_ISS_ISP_SUBCLK_BL		= (1 << 0),
> -	OMAP4_ISS_ISP_SUBCLK_ISIF	= (1 << 1),
> -	OMAP4_ISS_ISP_SUBCLK_H3A	= (1 << 2),
> -	OMAP4_ISS_ISP_SUBCLK_RSZ	= (1 << 3),
> -	OMAP4_ISS_ISP_SUBCLK_IPIPE	= (1 << 4),
> -	OMAP4_ISS_ISP_SUBCLK_IPIPEIF	= (1 << 5),
> +	OMAP4_ISS_ISP_SUBCLK_BL		= BIT(0),
> +	OMAP4_ISS_ISP_SUBCLK_ISIF	= BIT(1),
> +	OMAP4_ISS_ISP_SUBCLK_H3A	= BIT(2),
> +	OMAP4_ISS_ISP_SUBCLK_RSZ	= BIT(3),
> +	OMAP4_ISS_ISP_SUBCLK_IPIPE	= BIT(4),
> +	OMAP4_ISS_ISP_SUBCLK_IPIPEIF	= BIT(5),
>  };
> 
>  /*

Nice.

> diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
> index 96f2ce045138..e2cad0a31098 100644
> --- a/drivers/staging/media/omap4iss/iss_csiphy.c
> +++ b/drivers/staging/media/omap4iss/iss_csiphy.c
> @@ -179,10 +179,10 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>  		    lanes->data[i].pos > (csi2->phy->max_data_lanes + 1))
>  			return -EINVAL;
> 
> -		if (used_lanes & (1 << lanes->data[i].pos))
> +		if (used_lanes & BIT(lanes->data[i].pos))
>  			return -EINVAL;
> 
> -		used_lanes |= 1 << lanes->data[i].pos;
> +		used_lanes |= BIT(lanes->data[i].pos);
>  		csi2->phy->used_data_lanes++;
>  	}
> 
> @@ -190,7 +190,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>  	    lanes->clk.pos > (csi2->phy->max_data_lanes + 1))
>  		return -EINVAL;
> 
> -	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
> +	if (lanes->clk.pos == 0 || used_lanes & BIT(lanes->clk.pos))
>  		return -EINVAL;
> 
>  	csi2_ddrclk_khz = pipe->external_rate / 1000

So far so good.

> diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> index 09a7375c89ac..85c6fefeb13a 100644
> --- a/drivers/staging/media/omap4iss/iss_regs.h
> +++ b/drivers/staging/media/omap4iss/iss_regs.h
> @@ -93,10 +93,10 @@
>  #define CSI2_SYSCONFIG					0x10
>  #define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK		(3 << 12)
>  #define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE		(0 << 12)
> -#define CSI2_SYSCONFIG_MSTANDBY_MODE_NO			(1 << 12)
> +#define CSI2_SYSCONFIG_MSTANDBY_MODE_NO			BIT(12)
>  #define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART		(2 << 12)

Nope (and many other snippets like this below). This is a region
starting at bit 12. You can write 0, 1, 2, or 3. That doesn't make bit
number 12 any special.

It's a 1 written into this region starting at bit 12. It's not bit 12.
For the compiler it's the same. For humans, and for hardware design,
it's definitely not.

Now, going back to your "somehow" understanding my table... why
"somehow"? :) Please tell me what's not clear, instead.

-- 
Stefano