* [PATCH] Staging: media: omap4iss: Use BIT() macro.
@ 2020-03-26 21:02 Sam Muhammed
2020-03-30 8:11 ` [Outreachy kernel] " Stefano Brivio
0 siblings, 1 reply; 8+ messages in thread
From: Sam Muhammed @ 2020-03-26 21:02 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
Cc: Sam Muhammed
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>
---
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 | 78 ++++++++++-----------
drivers/staging/media/omap4iss/iss_video.h | 16 ++---
5 files changed, 63 insertions(+), 63 deletions(-)
diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 6fb60b58447a..baa1fa5d5c00 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");
@@ -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");
diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
index b88f9529683c..0c8a790c0ac7 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)),
};
/*
diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
index 96f2ce045138..1bdd4ecd900d 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
diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
index 09a7375c89ac..7adc944e1a5d 100644
--- a/drivers/staging/media/omap4iss/iss_regs.h
+++ b/drivers/staging/media/omap4iss/iss_regs.h
@@ -38,7 +38,7 @@
#define ISS_CTRL_CLK_DIV_MASK (3 << 4)
#define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
#define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
-#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
+#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
#define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
#define ISS_CLKCTRL 0x84
@@ -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)
-#define CSI2_SYSCONFIG_SOFT_RESET (1 << 1)
-#define CSI2_SYSCONFIG_AUTO_IDLE (1 << 0)
+#define CSI2_SYSCONFIG_SOFT_RESET (BIT(1))
+#define CSI2_SYSCONFIG_AUTO_IDLE (BIT(0))
#define CSI2_SYSSTATUS 0x14
#define CSI2_SYSSTATUS_RESET_DONE BIT(0)
@@ -119,37 +119,37 @@
#define CSI2_CTRL_MFLAG_LEVH_SHIFT 20
#define CSI2_CTRL_MFLAG_LEVL_MASK (7 << 17)
#define CSI2_CTRL_MFLAG_LEVL_SHIFT 17
-#define CSI2_CTRL_BURST_SIZE_EXPAND (1 << 16)
-#define CSI2_CTRL_VP_CLK_EN (1 << 15)
-#define CSI2_CTRL_NON_POSTED_WRITE (1 << 13)
-#define CSI2_CTRL_VP_ONLY_EN (1 << 11)
+#define CSI2_CTRL_BURST_SIZE_EXPAND (BIT(16))
+#define CSI2_CTRL_VP_CLK_EN (BIT(15))
+#define CSI2_CTRL_NON_POSTED_WRITE (BIT(13))
+#define CSI2_CTRL_VP_ONLY_EN (BIT(11))
#define CSI2_CTRL_VP_OUT_CTRL_MASK (3 << 8)
#define CSI2_CTRL_VP_OUT_CTRL_SHIFT 8
-#define CSI2_CTRL_DBG_EN (1 << 7)
+#define CSI2_CTRL_DBG_EN (BIT(7))
#define CSI2_CTRL_BURST_SIZE_MASK (3 << 5)
-#define CSI2_CTRL_ENDIANNESS (1 << 4)
-#define CSI2_CTRL_FRAME (1 << 3)
-#define CSI2_CTRL_ECC_EN (1 << 2)
-#define CSI2_CTRL_IF_EN (1 << 0)
+#define CSI2_CTRL_ENDIANNESS (BIT(4))
+#define CSI2_CTRL_FRAME (BIT(3))
+#define CSI2_CTRL_ECC_EN (BIT(2))
+#define CSI2_CTRL_IF_EN (BIT(0))
#define CSI2_DBG_H 0x44
#define CSI2_COMPLEXIO_CFG 0x50
-#define CSI2_COMPLEXIO_CFG_RESET_CTRL (1 << 30)
-#define CSI2_COMPLEXIO_CFG_RESET_DONE (1 << 29)
+#define CSI2_COMPLEXIO_CFG_RESET_CTRL (BIT(30))
+#define CSI2_COMPLEXIO_CFG_RESET_DONE (BIT(29))
#define CSI2_COMPLEXIO_CFG_PWD_CMD_MASK (3 << 27)
#define CSI2_COMPLEXIO_CFG_PWD_CMD_OFF (0 << 27)
-#define CSI2_COMPLEXIO_CFG_PWD_CMD_ON (1 << 27)
+#define CSI2_COMPLEXIO_CFG_PWD_CMD_ON (BIT(27))
#define CSI2_COMPLEXIO_CFG_PWD_CMD_ULP (2 << 27)
#define CSI2_COMPLEXIO_CFG_PWD_STATUS_MASK (3 << 25)
#define CSI2_COMPLEXIO_CFG_PWD_STATUS_OFF (0 << 25)
-#define CSI2_COMPLEXIO_CFG_PWD_STATUS_ON (1 << 25)
+#define CSI2_COMPLEXIO_CFG_PWD_STATUS_ON (BIT(25))
#define CSI2_COMPLEXIO_CFG_PWD_STATUS_ULP (2 << 25)
-#define CSI2_COMPLEXIO_CFG_PWR_AUTO (1 << 24)
-#define CSI2_COMPLEXIO_CFG_DATA_POL(i) (1 << (((i) * 4) + 3))
+#define CSI2_COMPLEXIO_CFG_PWR_AUTO (BIT(24))
+#define CSI2_COMPLEXIO_CFG_DATA_POL(i) (BIT((((i) * 4) + 3)))
#define CSI2_COMPLEXIO_CFG_DATA_POSITION_MASK(i) (7 << ((i) * 4))
#define CSI2_COMPLEXIO_CFG_DATA_POSITION_SHIFT(i) ((i) * 4)
-#define CSI2_COMPLEXIO_CFG_CLOCK_POL (1 << 3)
+#define CSI2_COMPLEXIO_CFG_CLOCK_POL (BIT(3))
#define CSI2_COMPLEXIO_CFG_CLOCK_POSITION_MASK (7 << 0)
#define CSI2_COMPLEXIO_CFG_CLOCK_POSITION_SHIFT 0
@@ -218,7 +218,7 @@
(0x3 << CSI2_CTX_CTRL2_USER_DEF_MAP_SHIFT)
#define CSI2_CTX_CTRL2_VIRTUAL_ID_MASK (3 << 11)
#define CSI2_CTX_CTRL2_VIRTUAL_ID_SHIFT 11
-#define CSI2_CTX_CTRL2_DPCM_PRED (1 << 10)
+#define CSI2_CTX_CTRL2_DPCM_PRED (BIT(10))
#define CSI2_CTX_CTRL2_FORMAT_MASK (0x3ff << 0)
#define CSI2_CTX_CTRL2_FORMAT_SHIFT 0
@@ -259,9 +259,9 @@
#define ISP5_SYSCONFIG (0x0010)
#define ISP5_SYSCONFIG_STANDBYMODE_MASK (3 << 4)
#define ISP5_SYSCONFIG_STANDBYMODE_FORCE (0 << 4)
-#define ISP5_SYSCONFIG_STANDBYMODE_NO (1 << 4)
+#define ISP5_SYSCONFIG_STANDBYMODE_NO (BIT(4))
#define ISP5_SYSCONFIG_STANDBYMODE_SMART (2 << 4)
-#define ISP5_SYSCONFIG_SOFTRESET (1 << 1)
+#define ISP5_SYSCONFIG_SOFTRESET (BIT(1))
#define ISP5_IRQSTATUS(i) (0x0028 + (0x10 * (i)))
#define ISP5_IRQENABLE_SET(i) (0x002c + (0x10 * (i)))
@@ -315,14 +315,14 @@
#define ISIF_MODESET (0x0004)
#define ISIF_MODESET_INPMOD_MASK (3 << 12)
#define ISIF_MODESET_INPMOD_RAW (0 << 12)
-#define ISIF_MODESET_INPMOD_YCBCR16 (1 << 12)
+#define ISIF_MODESET_INPMOD_YCBCR16 (BIT(12))
#define ISIF_MODESET_INPMOD_YCBCR8 (2 << 12)
#define ISIF_MODESET_CCDW_MASK (7 << 8)
#define ISIF_MODESET_CCDW_2BIT (2 << 8)
-#define ISIF_MODESET_CCDMD (1 << 7)
-#define ISIF_MODESET_SWEN (1 << 5)
-#define ISIF_MODESET_HDPOL (1 << 3)
-#define ISIF_MODESET_VDPOL (1 << 2)
+#define ISIF_MODESET_CCDMD (BIT(7))
+#define ISIF_MODESET_SWEN (BIT(5))
+#define ISIF_MODESET_HDPOL (BIT(3))
+#define ISIF_MODESET_VDPOL (BIT(2))
#define ISIF_SPH (0x0018)
#define ISIF_SPH_MASK (0x7fff)
@@ -345,19 +345,19 @@
#define ISIF_CCOLP (0x004c)
#define ISIF_CCOLP_CP0_F0_R (0 << 6)
-#define ISIF_CCOLP_CP0_F0_GR (1 << 6)
+#define ISIF_CCOLP_CP0_F0_GR (BIT(6))
#define ISIF_CCOLP_CP0_F0_B (3 << 6)
#define ISIF_CCOLP_CP0_F0_GB (2 << 6)
#define ISIF_CCOLP_CP1_F0_R (0 << 4)
-#define ISIF_CCOLP_CP1_F0_GR (1 << 4)
+#define ISIF_CCOLP_CP1_F0_GR (BIT(4))
#define ISIF_CCOLP_CP1_F0_B (3 << 4)
#define ISIF_CCOLP_CP1_F0_GB (2 << 4)
#define ISIF_CCOLP_CP2_F0_R (0 << 2)
-#define ISIF_CCOLP_CP2_F0_GR (1 << 2)
+#define ISIF_CCOLP_CP2_F0_GR (BIT(2))
#define ISIF_CCOLP_CP2_F0_B (3 << 2)
#define ISIF_CCOLP_CP2_F0_GB (2 << 2)
#define ISIF_CCOLP_CP3_F0_R (0 << 0)
-#define ISIF_CCOLP_CP3_F0_GR (1 << 0)
+#define ISIF_CCOLP_CP3_F0_GR (BIT(0))
#define ISIF_CCOLP_CP3_F0_B (3 << 0)
#define ISIF_CCOLP_CP3_F0_GB (2 << 0)
@@ -377,12 +377,12 @@
#define IPIPEIF_CFG1 (0x0004)
#define IPIPEIF_CFG1_INPSRC1_MASK (3 << 14)
#define IPIPEIF_CFG1_INPSRC1_VPORT_RAW (0 << 14)
-#define IPIPEIF_CFG1_INPSRC1_SDRAM_RAW (1 << 14)
+#define IPIPEIF_CFG1_INPSRC1_SDRAM_RAW (BIT(14))
#define IPIPEIF_CFG1_INPSRC1_ISIF_DARKFM (2 << 14)
#define IPIPEIF_CFG1_INPSRC1_SDRAM_YUV (3 << 14)
#define IPIPEIF_CFG1_INPSRC2_MASK (3 << 2)
#define IPIPEIF_CFG1_INPSRC2_ISIF (0 << 2)
-#define IPIPEIF_CFG1_INPSRC2_SDRAM_RAW (1 << 2)
+#define IPIPEIF_CFG1_INPSRC2_SDRAM_RAW (BIT(2))
#define IPIPEIF_CFG1_INPSRC2_ISIF_DARKFM (2 << 2)
#define IPIPEIF_CFG1_INPSRC2_SDRAM_YUV (3 << 2)
@@ -406,25 +406,25 @@
#define IPIPE_SRC_FMT (0x0008)
#define IPIPE_SRC_FMT_RAW2YUV (0 << 0)
-#define IPIPE_SRC_FMT_RAW2RAW (1 << 0)
+#define IPIPE_SRC_FMT_RAW2RAW (BIT(0))
#define IPIPE_SRC_FMT_RAW2STATS (2 << 0)
#define IPIPE_SRC_FMT_YUV2YUV (3 << 0)
#define IPIPE_SRC_COL (0x000c)
#define IPIPE_SRC_COL_OO_R (0 << 6)
-#define IPIPE_SRC_COL_OO_GR (1 << 6)
+#define IPIPE_SRC_COL_OO_GR (BIT(6))
#define IPIPE_SRC_COL_OO_B (3 << 6)
#define IPIPE_SRC_COL_OO_GB (2 << 6)
#define IPIPE_SRC_COL_OE_R (0 << 4)
-#define IPIPE_SRC_COL_OE_GR (1 << 4)
+#define IPIPE_SRC_COL_OE_GR (BIT(4))
#define IPIPE_SRC_COL_OE_B (3 << 4)
#define IPIPE_SRC_COL_OE_GB (2 << 4)
#define IPIPE_SRC_COL_EO_R (0 << 2)
-#define IPIPE_SRC_COL_EO_GR (1 << 2)
+#define IPIPE_SRC_COL_EO_GR (BIT(2))
#define IPIPE_SRC_COL_EO_B (3 << 2)
#define IPIPE_SRC_COL_EO_GB (2 << 2)
#define IPIPE_SRC_COL_EE_R (0 << 0)
-#define IPIPE_SRC_COL_EE_GR (1 << 0)
+#define IPIPE_SRC_COL_EE_GR (BIT(0))
#define IPIPE_SRC_COL_EE_B (3 << 0)
#define IPIPE_SRC_COL_EE_GB (2 << 0)
diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
index 8b3dd92021e1..c0d021636224 100644
--- a/drivers/staging/media/omap4iss/iss_video.h
+++ b/drivers/staging/media/omap4iss/iss_video.h
@@ -56,17 +56,17 @@ enum iss_pipeline_state {
/* The stream has been started on the input video node. */
ISS_PIPELINE_STREAM_INPUT = 1,
/* The stream has been started on the output video node. */
- ISS_PIPELINE_STREAM_OUTPUT = (1 << 1),
+ ISS_PIPELINE_STREAM_OUTPUT = (BIT(1)),
/* At least one buffer is queued on the input video node. */
- ISS_PIPELINE_QUEUE_INPUT = (1 << 2),
+ ISS_PIPELINE_QUEUE_INPUT = (BIT(2)),
/* At least one buffer is queued on the output video node. */
- ISS_PIPELINE_QUEUE_OUTPUT = (1 << 3),
+ ISS_PIPELINE_QUEUE_OUTPUT = (BIT(3)),
/* The input entity is idle, ready to be started. */
- ISS_PIPELINE_IDLE_INPUT = (1 << 4),
+ ISS_PIPELINE_IDLE_INPUT = (BIT(4)),
/* The output entity is idle, ready to be started. */
- ISS_PIPELINE_IDLE_OUTPUT = (1 << 5),
+ ISS_PIPELINE_IDLE_OUTPUT = (BIT(5)),
/* The pipeline is currently streaming. */
- ISS_PIPELINE_STREAM = (1 << 6),
+ ISS_PIPELINE_STREAM = (BIT(6)),
};
/*
@@ -120,9 +120,9 @@ struct iss_buffer {
enum iss_video_dmaqueue_flags {
/* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
- ISS_VIDEO_DMAQUEUE_UNDERRUN = (1 << 0),
+ ISS_VIDEO_DMAQUEUE_UNDERRUN = (BIT(0)),
/* Set when queuing buffer to an empty DMA queue */
- ISS_VIDEO_DMAQUEUE_QUEUED = (1 << 1),
+ ISS_VIDEO_DMAQUEUE_QUEUED = (BIT(1)),
};
#define iss_video_dmaqueue_flags_clr(video) \
---
2.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: media: omap4iss: Use BIT() macro.
2020-03-26 21:02 [PATCH] Staging: media: omap4iss: Use BIT() macro Sam Muhammed
@ 2020-03-30 8:11 ` Stefano Brivio
2020-03-30 11:40 ` Sam Muhammed
0 siblings, 1 reply; 8+ messages in thread
From: Stefano Brivio @ 2020-03-30 8:11 UTC (permalink / raw)
To: Sam Muhammed
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
On Thu, 26 Mar 2020 17:02:47 -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>
> ---
> 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 | 78 ++++++++++-----------
> drivers/staging/media/omap4iss/iss_video.h | 16 ++---
> 5 files changed, 63 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index 6fb60b58447a..baa1fa5d5c00 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)
There's a reason why 1 << i is surrounded by parentheses, but it makes
no sense to do that around a macro.
> pr_cont("%s ", name[i]);
> }
> pr_cont("\n");
> @@ -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)
Same here.
> pr_cont("%s ", name[i]);
> }
> pr_cont("\n");
> diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
> index b88f9529683c..0c8a790c0ac7 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)),
Here, too.
> };
>
> 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)),
And here.
> };
>
> /*
> diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
> index 96f2ce045138..1bdd4ecd900d 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
And here.
> diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> index 09a7375c89ac..7adc944e1a5d 100644
> --- a/drivers/staging/media/omap4iss/iss_regs.h
> +++ b/drivers/staging/media/omap4iss/iss_regs.h
> @@ -38,7 +38,7 @@
> #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> -#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> +#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
> #define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
The existing code is saying: there are three bits to select the input,
starting from bit number 2. For those three bits, 0 means the CSI2A
input is selected, and 1 means CSI2B is selected.
You're changing it to: there are three bits to select the input,
starting from bit number 2. For those three bits, 0 means the CSI2A
input is selected. CSI2B is a special case, though: register bit 2 needs
to be set.
>
> #define ISS_CLKCTRL 0x84
> @@ -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)
> -#define CSI2_SYSCONFIG_SOFT_RESET (1 << 1)
> -#define CSI2_SYSCONFIG_AUTO_IDLE (1 << 0)
> +#define CSI2_SYSCONFIG_SOFT_RESET (BIT(1))
> +#define CSI2_SYSCONFIG_AUTO_IDLE (BIT(0))
Also no need for parentheses around BIT().
>
> #define CSI2_SYSSTATUS 0x14
> #define CSI2_SYSSTATUS_RESET_DONE BIT(0)
> @@ -119,37 +119,37 @@
> #define CSI2_CTRL_MFLAG_LEVH_SHIFT 20
> #define CSI2_CTRL_MFLAG_LEVL_MASK (7 << 17)
> #define CSI2_CTRL_MFLAG_LEVL_SHIFT 17
> -#define CSI2_CTRL_BURST_SIZE_EXPAND (1 << 16)
> -#define CSI2_CTRL_VP_CLK_EN (1 << 15)
> -#define CSI2_CTRL_NON_POSTED_WRITE (1 << 13)
> -#define CSI2_CTRL_VP_ONLY_EN (1 << 11)
> +#define CSI2_CTRL_BURST_SIZE_EXPAND (BIT(16))
> +#define CSI2_CTRL_VP_CLK_EN (BIT(15))
> +#define CSI2_CTRL_NON_POSTED_WRITE (BIT(13))
> +#define CSI2_CTRL_VP_ONLY_EN (BIT(11))
> #define CSI2_CTRL_VP_OUT_CTRL_MASK (3 << 8)
> #define CSI2_CTRL_VP_OUT_CTRL_SHIFT 8
> -#define CSI2_CTRL_DBG_EN (1 << 7)
> +#define CSI2_CTRL_DBG_EN (BIT(7))
> #define CSI2_CTRL_BURST_SIZE_MASK (3 << 5)
> -#define CSI2_CTRL_ENDIANNESS (1 << 4)
> -#define CSI2_CTRL_FRAME (1 << 3)
> -#define CSI2_CTRL_ECC_EN (1 << 2)
> -#define CSI2_CTRL_IF_EN (1 << 0)
> +#define CSI2_CTRL_ENDIANNESS (BIT(4))
> +#define CSI2_CTRL_FRAME (BIT(3))
> +#define CSI2_CTRL_ECC_EN (BIT(2))
> +#define CSI2_CTRL_IF_EN (BIT(0))
>
> #define CSI2_DBG_H 0x44
>
> #define CSI2_COMPLEXIO_CFG 0x50
> -#define CSI2_COMPLEXIO_CFG_RESET_CTRL (1 << 30)
> -#define CSI2_COMPLEXIO_CFG_RESET_DONE (1 << 29)
> +#define CSI2_COMPLEXIO_CFG_RESET_CTRL (BIT(30))
> +#define CSI2_COMPLEXIO_CFG_RESET_DONE (BIT(29))
> #define CSI2_COMPLEXIO_CFG_PWD_CMD_MASK (3 << 27)
> #define CSI2_COMPLEXIO_CFG_PWD_CMD_OFF (0 << 27)
> -#define CSI2_COMPLEXIO_CFG_PWD_CMD_ON (1 << 27)
> +#define CSI2_COMPLEXIO_CFG_PWD_CMD_ON (BIT(27))
> #define CSI2_COMPLEXIO_CFG_PWD_CMD_ULP (2 << 27)
> #define CSI2_COMPLEXIO_CFG_PWD_STATUS_MASK (3 << 25)
> #define CSI2_COMPLEXIO_CFG_PWD_STATUS_OFF (0 << 25)
> -#define CSI2_COMPLEXIO_CFG_PWD_STATUS_ON (1 << 25)
> +#define CSI2_COMPLEXIO_CFG_PWD_STATUS_ON (BIT(25))
> #define CSI2_COMPLEXIO_CFG_PWD_STATUS_ULP (2 << 25)
> -#define CSI2_COMPLEXIO_CFG_PWR_AUTO (1 << 24)
> -#define CSI2_COMPLEXIO_CFG_DATA_POL(i) (1 << (((i) * 4) + 3))
> +#define CSI2_COMPLEXIO_CFG_PWR_AUTO (BIT(24))
> +#define CSI2_COMPLEXIO_CFG_DATA_POL(i) (BIT((((i) * 4) + 3)))
> #define CSI2_COMPLEXIO_CFG_DATA_POSITION_MASK(i) (7 << ((i) * 4))
> #define CSI2_COMPLEXIO_CFG_DATA_POSITION_SHIFT(i) ((i) * 4)
> -#define CSI2_COMPLEXIO_CFG_CLOCK_POL (1 << 3)
> +#define CSI2_COMPLEXIO_CFG_CLOCK_POL (BIT(3))
> #define CSI2_COMPLEXIO_CFG_CLOCK_POSITION_MASK (7 << 0)
> #define CSI2_COMPLEXIO_CFG_CLOCK_POSITION_SHIFT 0
>
> @@ -218,7 +218,7 @@
> (0x3 << CSI2_CTX_CTRL2_USER_DEF_MAP_SHIFT)
> #define CSI2_CTX_CTRL2_VIRTUAL_ID_MASK (3 << 11)
> #define CSI2_CTX_CTRL2_VIRTUAL_ID_SHIFT 11
> -#define CSI2_CTX_CTRL2_DPCM_PRED (1 << 10)
> +#define CSI2_CTX_CTRL2_DPCM_PRED (BIT(10))
> #define CSI2_CTX_CTRL2_FORMAT_MASK (0x3ff << 0)
> #define CSI2_CTX_CTRL2_FORMAT_SHIFT 0
>
> @@ -259,9 +259,9 @@
> #define ISP5_SYSCONFIG (0x0010)
> #define ISP5_SYSCONFIG_STANDBYMODE_MASK (3 << 4)
> #define ISP5_SYSCONFIG_STANDBYMODE_FORCE (0 << 4)
> -#define ISP5_SYSCONFIG_STANDBYMODE_NO (1 << 4)
> +#define ISP5_SYSCONFIG_STANDBYMODE_NO (BIT(4))
"Standby mode is forced by clearing 3 bits at index 4. Smart mode is set
by setting 2 as value for those bits. No standby mode is an entirely
different story: set bit 4 of ISP5_SYSCONFIG."
> #define ISP5_SYSCONFIG_STANDBYMODE_SMART (2 << 4)
> -#define ISP5_SYSCONFIG_SOFTRESET (1 << 1)
> +#define ISP5_SYSCONFIG_SOFTRESET (BIT(1))
>
> #define ISP5_IRQSTATUS(i) (0x0028 + (0x10 * (i)))
> #define ISP5_IRQENABLE_SET(i) (0x002c + (0x10 * (i)))
> @@ -315,14 +315,14 @@
> #define ISIF_MODESET (0x0004)
> #define ISIF_MODESET_INPMOD_MASK (3 << 12)
> #define ISIF_MODESET_INPMOD_RAW (0 << 12)
> -#define ISIF_MODESET_INPMOD_YCBCR16 (1 << 12)
> +#define ISIF_MODESET_INPMOD_YCBCR16 (BIT(12))
> #define ISIF_MODESET_INPMOD_YCBCR8 (2 << 12)
> #define ISIF_MODESET_CCDW_MASK (7 << 8)
> #define ISIF_MODESET_CCDW_2BIT (2 << 8)
> -#define ISIF_MODESET_CCDMD (1 << 7)
> -#define ISIF_MODESET_SWEN (1 << 5)
> -#define ISIF_MODESET_HDPOL (1 << 3)
> -#define ISIF_MODESET_VDPOL (1 << 2)
> +#define ISIF_MODESET_CCDMD (BIT(7))
> +#define ISIF_MODESET_SWEN (BIT(5))
> +#define ISIF_MODESET_HDPOL (BIT(3))
> +#define ISIF_MODESET_VDPOL (BIT(2))
>
> #define ISIF_SPH (0x0018)
> #define ISIF_SPH_MASK (0x7fff)
> @@ -345,19 +345,19 @@
>
> #define ISIF_CCOLP (0x004c)
> #define ISIF_CCOLP_CP0_F0_R (0 << 6)
> -#define ISIF_CCOLP_CP0_F0_GR (1 << 6)
> +#define ISIF_CCOLP_CP0_F0_GR (BIT(6))
> #define ISIF_CCOLP_CP0_F0_B (3 << 6)
> #define ISIF_CCOLP_CP0_F0_GB (2 << 6)
> #define ISIF_CCOLP_CP1_F0_R (0 << 4)
> -#define ISIF_CCOLP_CP1_F0_GR (1 << 4)
> +#define ISIF_CCOLP_CP1_F0_GR (BIT(4))
> #define ISIF_CCOLP_CP1_F0_B (3 << 4)
> #define ISIF_CCOLP_CP1_F0_GB (2 << 4)
> #define ISIF_CCOLP_CP2_F0_R (0 << 2)
> -#define ISIF_CCOLP_CP2_F0_GR (1 << 2)
> +#define ISIF_CCOLP_CP2_F0_GR (BIT(2))
> #define ISIF_CCOLP_CP2_F0_B (3 << 2)
> #define ISIF_CCOLP_CP2_F0_GB (2 << 2)
> #define ISIF_CCOLP_CP3_F0_R (0 << 0)
> -#define ISIF_CCOLP_CP3_F0_GR (1 << 0)
> +#define ISIF_CCOLP_CP3_F0_GR (BIT(0))
> #define ISIF_CCOLP_CP3_F0_B (3 << 0)
> #define ISIF_CCOLP_CP3_F0_GB (2 << 0)
>
> @@ -377,12 +377,12 @@
> #define IPIPEIF_CFG1 (0x0004)
> #define IPIPEIF_CFG1_INPSRC1_MASK (3 << 14)
> #define IPIPEIF_CFG1_INPSRC1_VPORT_RAW (0 << 14)
> -#define IPIPEIF_CFG1_INPSRC1_SDRAM_RAW (1 << 14)
> +#define IPIPEIF_CFG1_INPSRC1_SDRAM_RAW (BIT(14))
> #define IPIPEIF_CFG1_INPSRC1_ISIF_DARKFM (2 << 14)
> #define IPIPEIF_CFG1_INPSRC1_SDRAM_YUV (3 << 14)
> #define IPIPEIF_CFG1_INPSRC2_MASK (3 << 2)
> #define IPIPEIF_CFG1_INPSRC2_ISIF (0 << 2)
> -#define IPIPEIF_CFG1_INPSRC2_SDRAM_RAW (1 << 2)
> +#define IPIPEIF_CFG1_INPSRC2_SDRAM_RAW (BIT(2))
> #define IPIPEIF_CFG1_INPSRC2_ISIF_DARKFM (2 << 2)
> #define IPIPEIF_CFG1_INPSRC2_SDRAM_YUV (3 << 2)
>
> @@ -406,25 +406,25 @@
>
> #define IPIPE_SRC_FMT (0x0008)
> #define IPIPE_SRC_FMT_RAW2YUV (0 << 0)
> -#define IPIPE_SRC_FMT_RAW2RAW (1 << 0)
> +#define IPIPE_SRC_FMT_RAW2RAW (BIT(0))
> #define IPIPE_SRC_FMT_RAW2STATS (2 << 0)
> #define IPIPE_SRC_FMT_YUV2YUV (3 << 0)
>
> #define IPIPE_SRC_COL (0x000c)
> #define IPIPE_SRC_COL_OO_R (0 << 6)
> -#define IPIPE_SRC_COL_OO_GR (1 << 6)
> +#define IPIPE_SRC_COL_OO_GR (BIT(6))
> #define IPIPE_SRC_COL_OO_B (3 << 6)
> #define IPIPE_SRC_COL_OO_GB (2 << 6)
> #define IPIPE_SRC_COL_OE_R (0 << 4)
> -#define IPIPE_SRC_COL_OE_GR (1 << 4)
> +#define IPIPE_SRC_COL_OE_GR (BIT(4))
> #define IPIPE_SRC_COL_OE_B (3 << 4)
> #define IPIPE_SRC_COL_OE_GB (2 << 4)
> #define IPIPE_SRC_COL_EO_R (0 << 2)
> -#define IPIPE_SRC_COL_EO_GR (1 << 2)
> +#define IPIPE_SRC_COL_EO_GR (BIT(2))
> #define IPIPE_SRC_COL_EO_B (3 << 2)
> #define IPIPE_SRC_COL_EO_GB (2 << 2)
> #define IPIPE_SRC_COL_EE_R (0 << 0)
> -#define IPIPE_SRC_COL_EE_GR (1 << 0)
> +#define IPIPE_SRC_COL_EE_GR (BIT(0))
> #define IPIPE_SRC_COL_EE_B (3 << 0)
> #define IPIPE_SRC_COL_EE_GB (2 << 0)
>
> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> index 8b3dd92021e1..c0d021636224 100644
> --- a/drivers/staging/media/omap4iss/iss_video.h
> +++ b/drivers/staging/media/omap4iss/iss_video.h
> @@ -56,17 +56,17 @@ enum iss_pipeline_state {
> /* The stream has been started on the input video node. */
> ISS_PIPELINE_STREAM_INPUT = 1,
> /* The stream has been started on the output video node. */
> - ISS_PIPELINE_STREAM_OUTPUT = (1 << 1),
> + ISS_PIPELINE_STREAM_OUTPUT = (BIT(1)),
> /* At least one buffer is queued on the input video node. */
> - ISS_PIPELINE_QUEUE_INPUT = (1 << 2),
> + ISS_PIPELINE_QUEUE_INPUT = (BIT(2)),
> /* At least one buffer is queued on the output video node. */
> - ISS_PIPELINE_QUEUE_OUTPUT = (1 << 3),
> + ISS_PIPELINE_QUEUE_OUTPUT = (BIT(3)),
> /* The input entity is idle, ready to be started. */
> - ISS_PIPELINE_IDLE_INPUT = (1 << 4),
> + ISS_PIPELINE_IDLE_INPUT = (BIT(4)),
> /* The output entity is idle, ready to be started. */
> - ISS_PIPELINE_IDLE_OUTPUT = (1 << 5),
> + ISS_PIPELINE_IDLE_OUTPUT = (BIT(5)),
> /* The pipeline is currently streaming. */
> - ISS_PIPELINE_STREAM = (1 << 6),
> + ISS_PIPELINE_STREAM = (BIT(6)),
> };
>
> /*
> @@ -120,9 +120,9 @@ struct iss_buffer {
>
> enum iss_video_dmaqueue_flags {
> /* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
> - ISS_VIDEO_DMAQUEUE_UNDERRUN = (1 << 0),
> + ISS_VIDEO_DMAQUEUE_UNDERRUN = (BIT(0)),
> /* Set when queuing buffer to an empty DMA queue */
> - ISS_VIDEO_DMAQUEUE_QUEUED = (1 << 1),
> + ISS_VIDEO_DMAQUEUE_QUEUED = (BIT(1)),
> };
No need for parentheses in all the hunks above.
--
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: media: omap4iss: Use BIT() macro.
2020-03-30 8:11 ` [Outreachy kernel] " Stefano Brivio
@ 2020-03-30 11:40 ` Sam Muhammed
2020-03-30 11:46 ` Julia Lawall
2020-03-30 14:23 ` Stefano Brivio
0 siblings, 2 replies; 8+ messages in thread
From: Sam Muhammed @ 2020-03-30 11:40 UTC (permalink / raw)
To: Stefano Brivio
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
On Mon, 2020-03-30 at 10:11 +0200, Stefano Brivio wrote:
> On Thu, 26 Mar 2020 17:02:47 -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>
> > ---
> > 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 | 78 ++++++++++-----------
> > drivers/staging/media/omap4iss/iss_video.h | 16 ++---
> > 5 files changed, 63 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> > index 6fb60b58447a..baa1fa5d5c00 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)
>
> There's a reason why 1 << i is surrounded by parentheses, but it makes
> no sense to do that around a macro.
>
> > pr_cont("%s ", name[i]);
> > }
> > pr_cont("\n");
> > @@ -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)
>
> Same here.
>
> > pr_cont("%s ", name[i]);
> > }
> > pr_cont("\n");
> > diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
> > index b88f9529683c..0c8a790c0ac7 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)),
>
> Here, too.
>
> > };
> >
> > 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)),
>
> And here.
>
> > };
> >
> > /*
> > diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
> > index 96f2ce045138..1bdd4ecd900d 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
>
> And here.
>
> > diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> > index 09a7375c89ac..7adc944e1a5d 100644
> > --- a/drivers/staging/media/omap4iss/iss_regs.h
> > +++ b/drivers/staging/media/omap4iss/iss_regs.h
> > @@ -38,7 +38,7 @@
> > #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> > #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> > #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> > -#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> > +#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
> > #define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
>
> The existing code is saying: there are three bits to select the input,
> starting from bit number 2. For those three bits, 0 means the CSI2A
> input is selected, and 1 means CSI2B is selected.
>
> You're changing it to: there are three bits to select the input,
> starting from bit number 2. For those three bits, 0 means the CSI2A
> input is selected. CSI2B is a special case, though: register bit 2 needs
> to be set.
>
Oh, So defining BIT(2) totally changed the logic?
> >
> > #define ISS_CLKCTRL 0x84
> > @@ -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)
> > -#define CSI2_SYSCONFIG_SOFT_RESET (1 << 1)
> > -#define CSI2_SYSCONFIG_AUTO_IDLE (1 << 0)
> > +#define CSI2_SYSCONFIG_SOFT_RESET (BIT(1))
> > +#define CSI2_SYSCONFIG_AUTO_IDLE (BIT(0))
>
> Also no need for parentheses around BIT().
>
> >
> > #define CSI2_SYSSTATUS 0x14
> > #define CSI2_SYSSTATUS_RESET_DONE BIT(0)
> > @@ -119,37 +119,37 @@
> > #define CSI2_CTRL_MFLAG_LEVH_SHIFT 20
> > #define CSI2_CTRL_MFLAG_LEVL_MASK (7 << 17)
> > #define CSI2_CTRL_MFLAG_LEVL_SHIFT 17
> > -#define CSI2_CTRL_BURST_SIZE_EXPAND (1 << 16)
> > -#define CSI2_CTRL_VP_CLK_EN (1 << 15)
> > -#define CSI2_CTRL_NON_POSTED_WRITE (1 << 13)
> > -#define CSI2_CTRL_VP_ONLY_EN (1 << 11)
> > +#define CSI2_CTRL_BURST_SIZE_EXPAND (BIT(16))
> > +#define CSI2_CTRL_VP_CLK_EN (BIT(15))
> > +#define CSI2_CTRL_NON_POSTED_WRITE (BIT(13))
> > +#define CSI2_CTRL_VP_ONLY_EN (BIT(11))
> > #define CSI2_CTRL_VP_OUT_CTRL_MASK (3 << 8)
> > #define CSI2_CTRL_VP_OUT_CTRL_SHIFT 8
> > -#define CSI2_CTRL_DBG_EN (1 << 7)
> > +#define CSI2_CTRL_DBG_EN (BIT(7))
> > #define CSI2_CTRL_BURST_SIZE_MASK (3 << 5)
> > -#define CSI2_CTRL_ENDIANNESS (1 << 4)
> > -#define CSI2_CTRL_FRAME (1 << 3)
> > -#define CSI2_CTRL_ECC_EN (1 << 2)
> > -#define CSI2_CTRL_IF_EN (1 << 0)
> > +#define CSI2_CTRL_ENDIANNESS (BIT(4))
> > +#define CSI2_CTRL_FRAME (BIT(3))
> > +#define CSI2_CTRL_ECC_EN (BIT(2))
> > +#define CSI2_CTRL_IF_EN (BIT(0))
> >
> > #define CSI2_DBG_H 0x44
> >
> > #define CSI2_COMPLEXIO_CFG 0x50
> > -#define CSI2_COMPLEXIO_CFG_RESET_CTRL (1 << 30)
> > -#define CSI2_COMPLEXIO_CFG_RESET_DONE (1 << 29)
> > +#define CSI2_COMPLEXIO_CFG_RESET_CTRL (BIT(30))
> > +#define CSI2_COMPLEXIO_CFG_RESET_DONE (BIT(29))
> > #define CSI2_COMPLEXIO_CFG_PWD_CMD_MASK (3 << 27)
> > #define CSI2_COMPLEXIO_CFG_PWD_CMD_OFF (0 << 27)
> > -#define CSI2_COMPLEXIO_CFG_PWD_CMD_ON (1 << 27)
> > +#define CSI2_COMPLEXIO_CFG_PWD_CMD_ON (BIT(27))
> > #define CSI2_COMPLEXIO_CFG_PWD_CMD_ULP (2 << 27)
> > #define CSI2_COMPLEXIO_CFG_PWD_STATUS_MASK (3 << 25)
> > #define CSI2_COMPLEXIO_CFG_PWD_STATUS_OFF (0 << 25)
> > -#define CSI2_COMPLEXIO_CFG_PWD_STATUS_ON (1 << 25)
> > +#define CSI2_COMPLEXIO_CFG_PWD_STATUS_ON (BIT(25))
> > #define CSI2_COMPLEXIO_CFG_PWD_STATUS_ULP (2 << 25)
> > -#define CSI2_COMPLEXIO_CFG_PWR_AUTO (1 << 24)
> > -#define CSI2_COMPLEXIO_CFG_DATA_POL(i) (1 << (((i) * 4) + 3))
> > +#define CSI2_COMPLEXIO_CFG_PWR_AUTO (BIT(24))
> > +#define CSI2_COMPLEXIO_CFG_DATA_POL(i) (BIT((((i) * 4) + 3)))
> > #define CSI2_COMPLEXIO_CFG_DATA_POSITION_MASK(i) (7 << ((i) * 4))
> > #define CSI2_COMPLEXIO_CFG_DATA_POSITION_SHIFT(i) ((i) * 4)
> > -#define CSI2_COMPLEXIO_CFG_CLOCK_POL (1 << 3)
> > +#define CSI2_COMPLEXIO_CFG_CLOCK_POL (BIT(3))
> > #define CSI2_COMPLEXIO_CFG_CLOCK_POSITION_MASK (7 << 0)
> > #define CSI2_COMPLEXIO_CFG_CLOCK_POSITION_SHIFT 0
> >
> > @@ -218,7 +218,7 @@
> > (0x3 << CSI2_CTX_CTRL2_USER_DEF_MAP_SHIFT)
> > #define CSI2_CTX_CTRL2_VIRTUAL_ID_MASK (3 << 11)
> > #define CSI2_CTX_CTRL2_VIRTUAL_ID_SHIFT 11
> > -#define CSI2_CTX_CTRL2_DPCM_PRED (1 << 10)
> > +#define CSI2_CTX_CTRL2_DPCM_PRED (BIT(10))
> > #define CSI2_CTX_CTRL2_FORMAT_MASK (0x3ff << 0)
> > #define CSI2_CTX_CTRL2_FORMAT_SHIFT 0
> >
> > @@ -259,9 +259,9 @@
> > #define ISP5_SYSCONFIG (0x0010)
> > #define ISP5_SYSCONFIG_STANDBYMODE_MASK (3 << 4)
> > #define ISP5_SYSCONFIG_STANDBYMODE_FORCE (0 << 4)
> > -#define ISP5_SYSCONFIG_STANDBYMODE_NO (1 << 4)
> > +#define ISP5_SYSCONFIG_STANDBYMODE_NO (BIT(4))
>
> "Standby mode is forced by clearing 3 bits at index 4. Smart mode is set
> by setting 2 as value for those bits. No standby mode is an entirely
> different story: set bit 4 of ISP5_SYSCONFIG."
Since iam a noob, i have this question, what's better, to leave
(1 << 4) alone since it looks familiar between the rest,
or since BIT(4) already sets HIGH bit at bit 4, we better use the macro
though it might look as an odd in the pattern?
Thanks for clarifying the code, much help to understand this a bit.
>
> > #define ISP5_SYSCONFIG_STANDBYMODE_SMART (2 << 4)
> > -#define ISP5_SYSCONFIG_SOFTRESET (1 << 1)
> > +#define ISP5_SYSCONFIG_SOFTRESET (BIT(1))
> >
> > #define ISP5_IRQSTATUS(i) (0x0028 + (0x10 * (i)))
> > #define ISP5_IRQENABLE_SET(i) (0x002c + (0x10 * (i)))
> > @@ -315,14 +315,14 @@
> > #define ISIF_MODESET (0x0004)
> > #define ISIF_MODESET_INPMOD_MASK (3 << 12)
> > #define ISIF_MODESET_INPMOD_RAW (0 << 12)
> > -#define ISIF_MODESET_INPMOD_YCBCR16 (1 << 12)
> > +#define ISIF_MODESET_INPMOD_YCBCR16 (BIT(12))
> > #define ISIF_MODESET_INPMOD_YCBCR8 (2 << 12)
> > #define ISIF_MODESET_CCDW_MASK (7 << 8)
> > #define ISIF_MODESET_CCDW_2BIT (2 << 8)
> > -#define ISIF_MODESET_CCDMD (1 << 7)
> > -#define ISIF_MODESET_SWEN (1 << 5)
> > -#define ISIF_MODESET_HDPOL (1 << 3)
> > -#define ISIF_MODESET_VDPOL (1 << 2)
> > +#define ISIF_MODESET_CCDMD (BIT(7))
> > +#define ISIF_MODESET_SWEN (BIT(5))
> > +#define ISIF_MODESET_HDPOL (BIT(3))
> > +#define ISIF_MODESET_VDPOL (BIT(2))
> >
> > #define ISIF_SPH (0x0018)
> > #define ISIF_SPH_MASK (0x7fff)
> > @@ -345,19 +345,19 @@
> >
> > #define ISIF_CCOLP (0x004c)
> > #define ISIF_CCOLP_CP0_F0_R (0 << 6)
> > -#define ISIF_CCOLP_CP0_F0_GR (1 << 6)
> > +#define ISIF_CCOLP_CP0_F0_GR (BIT(6))
> > #define ISIF_CCOLP_CP0_F0_B (3 << 6)
> > #define ISIF_CCOLP_CP0_F0_GB (2 << 6)
> > #define ISIF_CCOLP_CP1_F0_R (0 << 4)
> > -#define ISIF_CCOLP_CP1_F0_GR (1 << 4)
> > +#define ISIF_CCOLP_CP1_F0_GR (BIT(4))
> > #define ISIF_CCOLP_CP1_F0_B (3 << 4)
> > #define ISIF_CCOLP_CP1_F0_GB (2 << 4)
> > #define ISIF_CCOLP_CP2_F0_R (0 << 2)
> > -#define ISIF_CCOLP_CP2_F0_GR (1 << 2)
> > +#define ISIF_CCOLP_CP2_F0_GR (BIT(2))
> > #define ISIF_CCOLP_CP2_F0_B (3 << 2)
> > #define ISIF_CCOLP_CP2_F0_GB (2 << 2)
> > #define ISIF_CCOLP_CP3_F0_R (0 << 0)
> > -#define ISIF_CCOLP_CP3_F0_GR (1 << 0)
> > +#define ISIF_CCOLP_CP3_F0_GR (BIT(0))
> > #define ISIF_CCOLP_CP3_F0_B (3 << 0)
> > #define ISIF_CCOLP_CP3_F0_GB (2 << 0)
> >
> > @@ -377,12 +377,12 @@
> > #define IPIPEIF_CFG1 (0x0004)
> > #define IPIPEIF_CFG1_INPSRC1_MASK (3 << 14)
> > #define IPIPEIF_CFG1_INPSRC1_VPORT_RAW (0 << 14)
> > -#define IPIPEIF_CFG1_INPSRC1_SDRAM_RAW (1 << 14)
> > +#define IPIPEIF_CFG1_INPSRC1_SDRAM_RAW (BIT(14))
> > #define IPIPEIF_CFG1_INPSRC1_ISIF_DARKFM (2 << 14)
> > #define IPIPEIF_CFG1_INPSRC1_SDRAM_YUV (3 << 14)
> > #define IPIPEIF_CFG1_INPSRC2_MASK (3 << 2)
> > #define IPIPEIF_CFG1_INPSRC2_ISIF (0 << 2)
> > -#define IPIPEIF_CFG1_INPSRC2_SDRAM_RAW (1 << 2)
> > +#define IPIPEIF_CFG1_INPSRC2_SDRAM_RAW (BIT(2))
> > #define IPIPEIF_CFG1_INPSRC2_ISIF_DARKFM (2 << 2)
> > #define IPIPEIF_CFG1_INPSRC2_SDRAM_YUV (3 << 2)
> >
> > @@ -406,25 +406,25 @@
> >
> > #define IPIPE_SRC_FMT (0x0008)
> > #define IPIPE_SRC_FMT_RAW2YUV (0 << 0)
> > -#define IPIPE_SRC_FMT_RAW2RAW (1 << 0)
> > +#define IPIPE_SRC_FMT_RAW2RAW (BIT(0))
> > #define IPIPE_SRC_FMT_RAW2STATS (2 << 0)
> > #define IPIPE_SRC_FMT_YUV2YUV (3 << 0)
> >
> > #define IPIPE_SRC_COL (0x000c)
> > #define IPIPE_SRC_COL_OO_R (0 << 6)
> > -#define IPIPE_SRC_COL_OO_GR (1 << 6)
> > +#define IPIPE_SRC_COL_OO_GR (BIT(6))
> > #define IPIPE_SRC_COL_OO_B (3 << 6)
> > #define IPIPE_SRC_COL_OO_GB (2 << 6)
> > #define IPIPE_SRC_COL_OE_R (0 << 4)
> > -#define IPIPE_SRC_COL_OE_GR (1 << 4)
> > +#define IPIPE_SRC_COL_OE_GR (BIT(4))
> > #define IPIPE_SRC_COL_OE_B (3 << 4)
> > #define IPIPE_SRC_COL_OE_GB (2 << 4)
> > #define IPIPE_SRC_COL_EO_R (0 << 2)
> > -#define IPIPE_SRC_COL_EO_GR (1 << 2)
> > +#define IPIPE_SRC_COL_EO_GR (BIT(2))
> > #define IPIPE_SRC_COL_EO_B (3 << 2)
> > #define IPIPE_SRC_COL_EO_GB (2 << 2)
> > #define IPIPE_SRC_COL_EE_R (0 << 0)
> > -#define IPIPE_SRC_COL_EE_GR (1 << 0)
> > +#define IPIPE_SRC_COL_EE_GR (BIT(0))
> > #define IPIPE_SRC_COL_EE_B (3 << 0)
> > #define IPIPE_SRC_COL_EE_GB (2 << 0)
> >
> > diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> > index 8b3dd92021e1..c0d021636224 100644
> > --- a/drivers/staging/media/omap4iss/iss_video.h
> > +++ b/drivers/staging/media/omap4iss/iss_video.h
> > @@ -56,17 +56,17 @@ enum iss_pipeline_state {
> > /* The stream has been started on the input video node. */
> > ISS_PIPELINE_STREAM_INPUT = 1,
> > /* The stream has been started on the output video node. */
> > - ISS_PIPELINE_STREAM_OUTPUT = (1 << 1),
> > + ISS_PIPELINE_STREAM_OUTPUT = (BIT(1)),
> > /* At least one buffer is queued on the input video node. */
> > - ISS_PIPELINE_QUEUE_INPUT = (1 << 2),
> > + ISS_PIPELINE_QUEUE_INPUT = (BIT(2)),
> > /* At least one buffer is queued on the output video node. */
> > - ISS_PIPELINE_QUEUE_OUTPUT = (1 << 3),
> > + ISS_PIPELINE_QUEUE_OUTPUT = (BIT(3)),
> > /* The input entity is idle, ready to be started. */
> > - ISS_PIPELINE_IDLE_INPUT = (1 << 4),
> > + ISS_PIPELINE_IDLE_INPUT = (BIT(4)),
> > /* The output entity is idle, ready to be started. */
> > - ISS_PIPELINE_IDLE_OUTPUT = (1 << 5),
> > + ISS_PIPELINE_IDLE_OUTPUT = (BIT(5)),
> > /* The pipeline is currently streaming. */
> > - ISS_PIPELINE_STREAM = (1 << 6),
> > + ISS_PIPELINE_STREAM = (BIT(6)),
> > };
> >
> > /*
> > @@ -120,9 +120,9 @@ struct iss_buffer {
> >
> > enum iss_video_dmaqueue_flags {
> > /* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
> > - ISS_VIDEO_DMAQUEUE_UNDERRUN = (1 << 0),
> > + ISS_VIDEO_DMAQUEUE_UNDERRUN = (BIT(0)),
> > /* Set when queuing buffer to an empty DMA queue */
> > - ISS_VIDEO_DMAQUEUE_QUEUED = (1 << 1),
> > + ISS_VIDEO_DMAQUEUE_QUEUED = (BIT(1)),
> > };
>
> No need for parentheses in all the hunks above.
>
Absolutely right.
Thank You!
Sam
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: media: omap4iss: Use BIT() macro.
2020-03-30 11:40 ` Sam Muhammed
@ 2020-03-30 11:46 ` Julia Lawall
2020-03-30 12:05 ` Sam Muhammed
2020-03-30 14:23 ` Stefano Brivio
1 sibling, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2020-03-30 11:46 UTC (permalink / raw)
To: Sam Muhammed
Cc: Stefano Brivio, Laurent Pinchart, Mauro Carvalho Chehab,
Greg Kroah-Hartman, outreachy-kernel
> > > diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> > > index 09a7375c89ac..7adc944e1a5d 100644
> > > --- a/drivers/staging/media/omap4iss/iss_regs.h
> > > +++ b/drivers/staging/media/omap4iss/iss_regs.h
> > > @@ -38,7 +38,7 @@
> > > #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> > > #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> > > #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> > > -#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> > > +#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
> > > #define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
> >
> > The existing code is saying: there are three bits to select the input,
> > starting from bit number 2. For those three bits, 0 means the CSI2A
> > input is selected, and 1 means CSI2B is selected.
> >
> > You're changing it to: there are three bits to select the input,
> > starting from bit number 2. For those three bits, 0 means the CSI2A
> > input is selected. CSI2B is a special case, though: register bit 2 needs
> > to be set.
> >
>
> Oh, So defining BIT(2) totally changed the logic?
I'm not sure what you mean by "the logic". BIT is nice to use when there
area a lot of 1 << X, because it makes clear which different its are
relevant. When 1 << X is mixed with other things, it breaks the pattern to
suddenly use BIT for one of them.
julia
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: media: omap4iss: Use BIT() macro.
2020-03-30 11:46 ` Julia Lawall
@ 2020-03-30 12:05 ` Sam Muhammed
2020-03-30 12:52 ` Julia Lawall
0 siblings, 1 reply; 8+ messages in thread
From: Sam Muhammed @ 2020-03-30 12:05 UTC (permalink / raw)
To: Julia Lawall
Cc: Stefano Brivio, Laurent Pinchart, Mauro Carvalho Chehab,
Greg Kroah-Hartman, outreachy-kernel
On Mon, 2020-03-30 at 13:46 +0200, Julia Lawall wrote:
> > > > diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> > > > index 09a7375c89ac..7adc944e1a5d 100644
> > > > --- a/drivers/staging/media/omap4iss/iss_regs.h
> > > > +++ b/drivers/staging/media/omap4iss/iss_regs.h
> > > > @@ -38,7 +38,7 @@
> > > > #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> > > > #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> > > > #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> > > > -#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> > > > +#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
> > > > #define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
> > >
> > > The existing code is saying: there are three bits to select the input,
> > > starting from bit number 2. For those three bits, 0 means the CSI2A
> > > input is selected, and 1 means CSI2B is selected.
> > >
> > > You're changing it to: there are three bits to select the input,
> > > starting from bit number 2. For those three bits, 0 means the CSI2A
> > > input is selected. CSI2B is a special case, though: register bit 2 needs
> > > to be set.
> > >
> >
> > Oh, So defining BIT(2) totally changed the logic?
>
> I'm not sure what you mean by "the logic". BIT is nice to use when there
> area a lot of 1 << X, because it makes clear which different its are
> relevant. When 1 << X is mixed with other things, it breaks the pattern to
> suddenly use BIT for one of them.
I should have said "the implicit meaning".
Or as i questioned it's about the pattern to better leave it and choose
the pattern over the change in cases like this?
Sam
>
> julia
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: media: omap4iss: Use BIT() macro.
2020-03-30 12:05 ` Sam Muhammed
@ 2020-03-30 12:52 ` Julia Lawall
0 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2020-03-30 12:52 UTC (permalink / raw)
To: Sam Muhammed
Cc: Stefano Brivio, Laurent Pinchart, Mauro Carvalho Chehab,
Greg Kroah-Hartman, outreachy-kernel
On Mon, 30 Mar 2020, Sam Muhammed wrote:
> On Mon, 2020-03-30 at 13:46 +0200, Julia Lawall wrote:
> > > > > diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> > > > > index 09a7375c89ac..7adc944e1a5d 100644
> > > > > --- a/drivers/staging/media/omap4iss/iss_regs.h
> > > > > +++ b/drivers/staging/media/omap4iss/iss_regs.h
> > > > > @@ -38,7 +38,7 @@
> > > > > #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> > > > > #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> > > > > #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> > > > > -#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> > > > > +#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
> > > > > #define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
> > > >
> > > > The existing code is saying: there are three bits to select the input,
> > > > starting from bit number 2. For those three bits, 0 means the CSI2A
> > > > input is selected, and 1 means CSI2B is selected.
> > > >
> > > > You're changing it to: there are three bits to select the input,
> > > > starting from bit number 2. For those three bits, 0 means the CSI2A
> > > > input is selected. CSI2B is a special case, though: register bit 2 needs
> > > > to be set.
> > > >
> > >
> > > Oh, So defining BIT(2) totally changed the logic?
> >
> > I'm not sure what you mean by "the logic". BIT is nice to use when there
> > area a lot of 1 << X, because it makes clear which different its are
> > relevant. When 1 << X is mixed with other things, it breaks the pattern to
> > suddenly use BIT for one of them.
>
> I should have said "the implicit meaning".
>
> Or as i questioned it's about the pattern to better leave it and choose
> the pattern over the change in cases like this?
Yes, it's better to respect the local pattern.
julia
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: media: omap4iss: Use BIT() macro.
2020-03-30 11:40 ` Sam Muhammed
2020-03-30 11:46 ` Julia Lawall
@ 2020-03-30 14:23 ` Stefano Brivio
2020-03-30 16:32 ` Sam Muhammed
1 sibling, 1 reply; 8+ messages in thread
From: Stefano Brivio @ 2020-03-30 14:23 UTC (permalink / raw)
To: Sam Muhammed
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel, Julia Lawall
On Mon, 30 Mar 2020 07:40:22 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:
> On Mon, 2020-03-30 at 10:11 +0200, Stefano Brivio wrote:
> > On Thu, 26 Mar 2020 17:02:47 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >
> > [...]
> >
> > > +++ b/drivers/staging/media/omap4iss/iss_regs.h
> > > @@ -38,7 +38,7 @@
> > > #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> > > #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> > > #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> > > -#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> > > +#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
> > > #define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
> >
> > The existing code is saying: there are three bits to select the input,
> > starting from bit number 2. For those three bits, 0 means the CSI2A
> > input is selected, and 1 means CSI2B is selected.
> >
> > You're changing it to: there are three bits to select the input,
> > starting from bit number 2. For those three bits, 0 means the CSI2A
> > input is selected. CSI2B is a special case, though: register bit 2 needs
> > to be set.
>
> Oh, So defining BIT(2) totally changed the logic?
No, it has no functional effect. And I also don't care too much about
"the pattern" here -- I would just recommend that you practice
actually reading code stuff before you change it :) So:
1. #define ISS_CTRL 0x80
"we have a register at 0x80"
https://en.wikipedia.org/wiki/Hardware_register
2. how big is it? Datasheet tells you, but before you even look for it:
#define ISS_CTRL_CLK_DIV_MASK (3 << 4)
"at least 7 bits"
#define ISS_CLKCTRL 0x84
#define ISS_CLKCTRL_VPORT2_CLK BIT(30)
"probably they are all the same size, might be 32 bits"
3. picture it:
|_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ b2 _|_ b1 _|_ b0 _|
(might be bigger, not too important)
4. #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
"these bits define the input selection [whatever it is]":
|_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ b2 _|_ b1 _|_ b0 _|
^^ ^^ | << shift by 2
2^1 + 2^0 = 3
^----(3 << 2)
5. #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
"to select input CSI2A [whatever it is]", we need:
|_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ 0 _|_ 0 _|_ b1 _|_ b0 _|
^^ ^^ | << shift by 2
0 + 0 = 0
^----(0 << 2)
#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
"to select input CSI2B [whatever it is]", we need:
|_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ 0 _|_ 1 _|_ b1 _|_ b0 _|
^^ ^^ | << shift by 2
0 + 2^0 = 1
^----(1 << 2)
Now, that's how the current #defines work. I think they are
actually intuitive (you could use the GENMASK() macro, but I think
the difference is not that big).
With your change:
5. #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
"to select input CSI2A [whatever it is]", we need:
|_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ 0 _|_ 0 _|_ b1 _|_ b0 _|
^^ ^^ | << shift by 2
0 + 0 = 0
^----(0 << 2)
#define ISS_CTRL_INPUT_SEL_CSI2B BIT(2)
"to select input CSI2B [whatever it is]", we need:
|_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ 1 _|_ b1 _|_ b0 _|
^
' this bit set.
What, why? How is this related with the rest?
>
> [...]
> >
> > Also no need for parentheses around BIT().
> >
> [...]
> >
> > "Standby mode is forced by clearing 3 bits at index 4. Smart mode is set
> > by setting 2 as value for those bits. No standby mode is an entirely
> > different story: set bit 4 of ISP5_SYSCONFIG."
>
> Since iam a noob, i have this question, what's better, to leave
> (1 << 4) alone since it looks familiar between the rest,
>
> or since BIT(4) already sets HIGH bit at bit 4, we better use the macro
> though it might look as an odd in the pattern?
Same here, I'm not exceedingly interested in the pattern or how it looks
like -- with your change, it would still work, but it would make no
sense.
> Thanks for clarifying the code, much help to understand this a bit.
Sorry, that's why I quoted my "explanation": that's how one would read
it after your change.
That is, the "No standby mode is an entirely different story" part is
how one would read the code after your change, and... it's wrong! :)
--
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: media: omap4iss: Use BIT() macro.
2020-03-30 14:23 ` Stefano Brivio
@ 2020-03-30 16:32 ` Sam Muhammed
0 siblings, 0 replies; 8+ messages in thread
From: Sam Muhammed @ 2020-03-30 16:32 UTC (permalink / raw)
To: Stefano Brivio
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel, Julia Lawall
On Mon, 2020-03-30 at 16:23 +0200, Stefano Brivio wrote:
> On Mon, 30 Mar 2020 07:40:22 -0400
> Sam Muhammed <jane.pnx9@gmail.com> wrote:
>
> > On Mon, 2020-03-30 at 10:11 +0200, Stefano Brivio wrote:
> > > On Thu, 26 Mar 2020 17:02:47 -0400
> > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > >
> > > [...]
> > >
> > > > +++ b/drivers/staging/media/omap4iss/iss_regs.h
> > > > @@ -38,7 +38,7 @@
> > > > #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> > > > #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> > > > #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> > > > -#define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> > > > +#define ISS_CTRL_INPUT_SEL_CSI2B (BIT(2))
> > > > #define ISS_CTRL_SYNC_DETECT_VS_RAISING (3 << 0)
> > >
> > > The existing code is saying: there are three bits to select the input,
> > > starting from bit number 2. For those three bits, 0 means the CSI2A
> > > input is selected, and 1 means CSI2B is selected.
> > >
> > > You're changing it to: there are three bits to select the input,
> > > starting from bit number 2. For those three bits, 0 means the CSI2A
> > > input is selected. CSI2B is a special case, though: register bit 2 needs
> > > to be set.
> >
> > Oh, So defining BIT(2) totally changed the logic?
>
> No, it has no functional effect. And I also don't care too much about
> "the pattern" here -- I would just recommend that you practice
> actually reading code stuff before you change it :) So:
>
> 1. #define ISS_CTRL 0x80
> "we have a register at 0x80"
> https://en.wikipedia.org/wiki/Hardware_register
>
> 2. how big is it? Datasheet tells you, but before you even look for it:
> #define ISS_CTRL_CLK_DIV_MASK (3 << 4)
> "at least 7 bits"
>
> #define ISS_CLKCTRL 0x84
> #define ISS_CLKCTRL_VPORT2_CLK BIT(30)
> "probably they are all the same size, might be 32 bits"
>
> 3. picture it:
> |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ b2 _|_ b1 _|_ b0 _|
> (might be bigger, not too important)
>
> 4. #define ISS_CTRL_INPUT_SEL_MASK (3 << 2)
> "these bits define the input selection [whatever it is]":
>
> |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ b2 _|_ b1 _|_ b0 _|
> ^^ ^^ | << shift by 2
> 2^1 + 2^0 = 3
> ^----(3 << 2)
>
> 5. #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> "to select input CSI2A [whatever it is]", we need:
> |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ 0 _|_ 0 _|_ b1 _|_ b0 _|
> ^^ ^^ | << shift by 2
> 0 + 0 = 0
> ^----(0 << 2)
>
> #define ISS_CTRL_INPUT_SEL_CSI2B (1 << 2)
> "to select input CSI2B [whatever it is]", we need:
> |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ 0 _|_ 1 _|_ b1 _|_ b0 _|
> ^^ ^^ | << shift by 2
> 0 + 2^0 = 1
> ^----(1 << 2)
>
> Now, that's how the current #defines work. I think they are
> actually intuitive (you could use the GENMASK() macro, but I think
> the difference is not that big).
>
> With your change:
>
> 5. #define ISS_CTRL_INPUT_SEL_CSI2A (0 << 2)
> "to select input CSI2A [whatever it is]", we need:
> |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ 0 _|_ 0 _|_ b1 _|_ b0 _|
> ^^ ^^ | << shift by 2
> 0 + 0 = 0
> ^----(0 << 2)
>
> #define ISS_CTRL_INPUT_SEL_CSI2B BIT(2)
> "to select input CSI2B [whatever it is]", we need:
> |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ 1 _|_ b1 _|_ b0 _|
> ^
> ' this bit set.
> What, why? How is this related with the rest?
>
The difference is somehow clear now, Thanks for clarifying!
> >
> > [...]
> > >
> > > Also no need for parentheses around BIT().
> > >
> > [...]
> > >
> > > "Standby mode is forced by clearing 3 bits at index 4. Smart mode is set
> > > by setting 2 as value for those bits. No standby mode is an entirely
> > > different story: set bit 4 of ISP5_SYSCONFIG."
> >
> > Since iam a noob, i have this question, what's better, to leave
> > (1 << 4) alone since it looks familiar between the rest,
> >
> > or since BIT(4) already sets HIGH bit at bit 4, we better use the macro
> > though it might look as an odd in the pattern?
>
> Same here, I'm not exceedingly interested in the pattern or how it looks
> like -- with your change, it would still work, but it would make no
> sense.
>
> > Thanks for clarifying the code, much help to understand this a bit.
>
> Sorry, that's why I quoted my "explanation": that's how one would read
> it after your change.
>
Well, That's a different perspective that i needed to test the change,
that it's not enough to just _build it_ and i'am afraid i would never
have this perspective in mind since iam only making minor changes, and
some implications of these changes are not visible to me until it's
pointed out.
I guess that's why i send a patch and wait for review :)
> That is, the "No standby mode is an entirely different story" part is
> how one would read the code after your change, and... it's wrong! :)
>
Thank You.
Sam
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-03-30 16:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 21:02 [PATCH] Staging: media: omap4iss: Use BIT() macro Sam Muhammed
2020-03-30 8:11 ` [Outreachy kernel] " Stefano Brivio
2020-03-30 11:40 ` Sam Muhammed
2020-03-30 11:46 ` Julia Lawall
2020-03-30 12:05 ` Sam Muhammed
2020-03-30 12:52 ` Julia Lawall
2020-03-30 14:23 ` Stefano Brivio
2020-03-30 16:32 ` Sam Muhammed
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.