* [PATCH v2] Staging: media: omap4iss: Use BIT() macro
@ 2020-03-31 22:58 Sam Muhammed
2020-04-01 2:55 ` [Outreachy kernel] " Stefano Brivio
0 siblings, 1 reply; 9+ messages in thread
From: Sam Muhammed @ 2020-03-31 22:58 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>
---
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");
@@ -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..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),
};
/*
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
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)
-#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
@@ -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..230a73d2cbdc 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] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-03-31 22:58 [PATCH v2] Staging: media: omap4iss: Use BIT() macro Sam Muhammed
@ 2020-04-01 2:55 ` Stefano Brivio
2020-04-04 5:33 ` Sam Muhammed
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Brivio @ 2020-04-01 2:55 UTC (permalink / raw)
To: Sam Muhammed
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-04-01 2:55 ` [Outreachy kernel] " Stefano Brivio
@ 2020-04-04 5:33 ` Sam Muhammed
2020-04-04 12:11 ` Stefano Brivio
2020-04-04 12:55 ` Julia Lawall
0 siblings, 2 replies; 9+ messages in thread
From: Sam Muhammed @ 2020-04-04 5:33 UTC (permalink / raw)
To: Stefano Brivio
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
On Wed, 2020-04-01 at 04:55 +0200, Stefano Brivio wrote:
> 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)
>
Hi Stefano,
honestly i'am confused about how is there a difference between BIT(x)
and (1 << x)
So i'am just going to interpret this hunk as i understand it and
please correct me.
#define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK (3 << 12)
shift binary 3 by 12: "write 3 starting at bit 12"
0011 ---- ---- ----
#define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE (0 << 12)
"write 0 at bit 12"
0000 ---- ---- ----
#define CSI2_SYSCONFIG_MSTANDBY_MODE_NO (1 << 12)
"shift binary 1 by 12": "write 1 at bit 12"
0001 ---- ---- ----
#define CSI2_SYSCONFIG_MSTANDBY_MODE_NO BIT(12)
"shift binary 1 by 12": _isn't this what BIT() does?_
0001 ---- ---- ----
now i dont understand what you meant by "That doesn't make bit 12 any
special", does that assume that iam wrong with what BIT() is defined
for?
#define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART (2 << 12)
"shift binary 2 by 12": write 2 at bit 12
0010 ---- ---- ----
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
for your previous explanation: _quoting_ :
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?
++++
now right at this point i got an overlapped definition of BIT()
this expansion/explanation of BIT(2) in my mind got translated to:
SetBit(register,bit) => (register|=(1<<bit))
NOT
shift binary 1 by 2
++++
Basically i forgot about this patch and i'am more into clearing this
confusion because i'am definitely missing something thats very basic
and probably have a wrong understanding of something.
Thank You,
Sam
> 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.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-04-04 5:33 ` Sam Muhammed
@ 2020-04-04 12:11 ` Stefano Brivio
2020-04-04 19:32 ` Sam Muhammed
2020-04-04 12:55 ` Julia Lawall
1 sibling, 1 reply; 9+ messages in thread
From: Stefano Brivio @ 2020-04-04 12:11 UTC (permalink / raw)
To: Sam Muhammed
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
Hi Sam,
On Sat, 04 Apr 2020 01:33:09 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:
> On Wed, 2020-04-01 at 04:55 +0200, Stefano Brivio wrote:
> > On Tue, 31 Mar 2020 18:58:17 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
>
> [...]
>
> > > 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)
> >
>
> Hi Stefano,
> honestly i'am confused about how is there a difference between BIT(x)
> and (1 << x)
>
> So i'am just going to interpret this hunk as i understand it and
> please correct me.
>
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK (3 << 12)
> shift binary 3 by 12: "write 3 starting at bit 12"
> 0011 ---- ---- ----
>
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE (0 << 12)
> "write 0 at bit 12"
> 0000 ---- ---- ----
>
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO (1 << 12)
> "shift binary 1 by 12": "write 1 at bit 12"
> 0001 ---- ---- ----
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO BIT(12)
> "shift binary 1 by 12": _isn't this what BIT() does?_
> 0001 ---- ---- ----
This is the key perhaps: it is what BIT() does. And it's not what you
should be... thinking of doing. Even though the result is clearly the
same.
BIT() is used to set a single bit. You need to write *some* bits, here.
That "some" is 2: you can write 0, 1, 2, 3, to this region. It's not 1.
Now, if you _write_ 1, you need to _set_ one bit, so coincidentally
BIT() works too.
Suppose you have a tray to make ice cubes. You use one, and put the
whole thing back in the freezer. Before you do that, it is your duty
towards the society to fill it completely with water.
After you use one ice cube, it might look like this:
1 2 3 4
.----.----.----.----.
|xxxx|xxxx|xxxx|xxxx|
|----|----|----|----|
|xxxx|xxxx| |xxxx|
'----'----'----'----'
5 6 7 8
so we have two ways to express what you have to do:
a. fill hole number 7
b. refill the whole thing
a. will do the job just like b., but b. is how you would most naturally
describe the operation. Especially because sometimes you take two ice
cubes, sometimes three.
> now i dont understand what you meant by "That doesn't make bit 12 any
> special", does that assume that iam wrong with what BIT() is defined
> for?
Yes, in some sense. BIT() is used to operate on a single bit: the
difference is between:
a. set bit 12
b. write a number to bit region from 12 to 13
if the number from b. is 1, then a. is equivalent.
But right above and below this #define you have defines for 0 << 12,
2 << 12, 3 << 12. And 1 << 12 is not a special case.
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART (2 << 12)
> "shift binary 2 by 12": write 2 at bit 12
> 0010 ---- ---- ----
>
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>
> for your previous explanation: _quoting_ :
>
> 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?
> ++++
> now right at this point i got an overlapped definition of BIT()
> this expansion/explanation of BIT(2) in my mind got translated to:
>
> SetBit(register,bit) => (register|=(1<<bit))
> NOT
> shift binary 1 by 2
> ++++
Yes, it's also about this. But from your example above:
SetBit(register,bit) => (register|=(1<<bit))
you need to distinguish when you're operating on a bit *array* or a
generic group of bit regions. That is, it's:
SetBit(register,bit) => (register|=(n<<bit))
where 'n' happens to be 1, sometimes. It's not always '1' by design.
Let's take a driver operating a lamp that has three LEDs inside: it
might work like this:
#define CTRL_REGISTER 0x5 /* a generic 8-bit register */
#define RED_ON BIT(0)
#define GREEN_ON BIT(1)
#define BLUE_ON BIT(2)
those are single bits, and consistently so, so using BIT() is fine.
Now, the lamp can be on, off, blink slow or fast. In the same register,
you have 2 bits (starting from bit 4) controlling that: 0 means "off", 1
means "blink slow", 2 means "blink fast", 3 means "on":
#define MODE_OFF (0 << 4)
#define MODE_SLOW (1 << 4)
#define MODE_FAST (2 << 4)
#define MODE_ON (3 << 4)
this is a natural way of describing things. Suppose you do, instead:
#define MODE_OFF (0 << 4)
#define MODE_SLOW BIT(4)
#define MODE_FAST (2 << 4)
#define MODE_ON (3 << 4)
the compiler won't judge you, but I will. Why is the "slow" mode
special? This makes the "slow" mode look like it's a binary option, but
it's not.
> Basically i forgot about this patch and i'am more into clearing this
> confusion because i'am definitely missing something thats very basic
> and probably have a wrong understanding of something.
Thanks for sticking with this, I appreciate that you're trying to
figure out what I'm saying. Is it a bit clearer now?
--
Stefano
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-04-04 5:33 ` Sam Muhammed
2020-04-04 12:11 ` Stefano Brivio
@ 2020-04-04 12:55 ` Julia Lawall
2020-04-04 19:50 ` Sam Muhammed
1 sibling, 1 reply; 9+ messages in thread
From: Julia Lawall @ 2020-04-04 12:55 UTC (permalink / raw)
To: Sam Muhammed
Cc: Stefano Brivio, Laurent Pinchart, Mauro Carvalho Chehab,
Greg Kroah-Hartman, outreachy-kernel
> > > #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)
> >
>
> Hi Stefano,
> honestly i'am confused about how is there a difference between BIT(x)
> and (1 << x)
In the original code you have a bunch of things shifted to the left by 12
bits. 3 0 1 and 2. They all look the same, and one can think about a
little region of bits some of which can be on and some of which can be off.
If you put BIT(12) in one case you don't see that.
For example, if you had
1 + 100
2 + 100
3 + 100
104
5 + 100
How easy is it to check that you have a sequence of 5 numbers? It's really
easy to add a single digit integer to 100, but probably you had to think
just a little extra bit when you came to 104, because it's not following
the same pattern.
julia
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-04-04 12:11 ` Stefano Brivio
@ 2020-04-04 19:32 ` Sam Muhammed
2020-04-04 23:52 ` Stefano Brivio
0 siblings, 1 reply; 9+ messages in thread
From: Sam Muhammed @ 2020-04-04 19:32 UTC (permalink / raw)
To: Stefano Brivio
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
On Sat, 2020-04-04 at 14:11 +0200, Stefano Brivio wrote:
> Hi Sam,
>
> On Sat, 04 Apr 2020 01:33:09 -0400
> Sam Muhammed <jane.pnx9@gmail.com> wrote:
>
> > On Wed, 2020-04-01 at 04:55 +0200, Stefano Brivio wrote:
> > > On Tue, 31 Mar 2020 18:58:17 -0400
> > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >
> > [...]
> >
> > > > 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)
> > >
> >
> > Hi Stefano,
> > honestly i'am confused about how is there a difference between BIT(x)
> > and (1 << x)
> >
> > So i'am just going to interpret this hunk as i understand it and
> > please correct me.
> >
> > #define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK (3 << 12)
> > shift binary 3 by 12: "write 3 starting at bit 12"
> > 0011 ---- ---- ----
> >
> > #define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE (0 << 12)
> > "write 0 at bit 12"
> > 0000 ---- ---- ----
> >
> > #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO (1 << 12)
> > "shift binary 1 by 12": "write 1 at bit 12"
> > 0001 ---- ---- ----
> > #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO BIT(12)
> > "shift binary 1 by 12": _isn't this what BIT() does?_
> > 0001 ---- ---- ----
>
> This is the key perhaps: it is what BIT() does. And it's not what you
> should be... thinking of doing. Even though the result is clearly the
> same.
>
> BIT() is used to set a single bit. You need to write *some* bits, here.
> That "some" is 2: you can write 0, 1, 2, 3, to this region. It's not 1.
>
> Now, if you _write_ 1, you need to _set_ one bit, so coincidentally
> BIT() works too.
>
> Suppose you have a tray to make ice cubes. You use one, and put the
> whole thing back in the freezer. Before you do that, it is your duty
> towards the society to fill it completely with water.
>
> After you use one ice cube, it might look like this:
>
> 1 2 3 4
> .----.----.----.----.
> |xxxx|xxxx|xxxx|xxxx|
> |----|----|----|----|
> |xxxx|xxxx| |xxxx|
> '----'----'----'----'
> 5 6 7 8
>
> so we have two ways to express what you have to do:
>
> a. fill hole number 7
> b. refill the whole thing
>
> a. will do the job just like b., but b. is how you would most naturally
> describe the operation. Especially because sometimes you take two ice
> cubes, sometimes three.
>
> > now i dont understand what you meant by "That doesn't make bit 12 any
> > special", does that assume that iam wrong with what BIT() is defined
> > for?
>
> Yes, in some sense. BIT() is used to operate on a single bit: the
> difference is between:
>
> a. set bit 12
> b. write a number to bit region from 12 to 13
>
> if the number from b. is 1, then a. is equivalent.
>
> But right above and below this #define you have defines for 0 << 12,
> 2 << 12, 3 << 12. And 1 << 12 is not a special case.
>
> > #define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART (2 << 12)
> > "shift binary 2 by 12": write 2 at bit 12
> > 0010 ---- ---- ----
> >
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> > for your previous explanation: _quoting_ :
> >
> > 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?
> > ++++
> > now right at this point i got an overlapped definition of BIT()
> > this expansion/explanation of BIT(2) in my mind got translated to:
> >
> > SetBit(register,bit) => (register|=(1<<bit))
> > NOT
> > shift binary 1 by 2
> > ++++
>
> Yes, it's also about this. But from your example above:
> SetBit(register,bit) => (register|=(1<<bit))
>
> you need to distinguish when you're operating on a bit *array* or a
> generic group of bit regions. That is, it's:
> SetBit(register,bit) => (register|=(n<<bit))
>
> where 'n' happens to be 1, sometimes. It's not always '1' by design.
>
> Let's take a driver operating a lamp that has three LEDs inside: it
> might work like this:
>
> #define CTRL_REGISTER 0x5 /* a generic 8-bit register */
> #define RED_ON BIT(0)
> #define GREEN_ON BIT(1)
> #define BLUE_ON BIT(2)
>
> those are single bits, and consistently so, so using BIT() is fine.
>
> Now, the lamp can be on, off, blink slow or fast. In the same register,
> you have 2 bits (starting from bit 4) controlling that: 0 means "off", 1
> means "blink slow", 2 means "blink fast", 3 means "on":
>
> #define MODE_OFF (0 << 4)
> #define MODE_SLOW (1 << 4)
> #define MODE_FAST (2 << 4)
> #define MODE_ON (3 << 4)
>
> this is a natural way of describing things. Suppose you do, instead:
>
> #define MODE_OFF (0 << 4)
> #define MODE_SLOW BIT(4)
> #define MODE_FAST (2 << 4)
> #define MODE_ON (3 << 4)
>
I guess that is a great example:
So if the operations are on a single bit being toggled on, BIT() is
welcome to be used.
But when the operations involve a region of bits being toggled on/off i
should stick to the typical way of bit manipulation even if there
happens to be an operation that requires only one bit to be set.
++++
My thoughts were: _quoting your example_
#define RED_ON BIT(0)
#define GREEN_ON BIT(1) => 0001
#define BLUE_ON BIT(2) => 0010
This does set bit 2, but affected the rest
"so it was a _lame_ way of saying that there is a region of bits being
manipulated though only one of them is set" :D
++++
so when it came to:
#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)
*my brain* --> yes, makes sense
#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?
"my brain" --> why b3 is not 0 now?
shouldn't BIT(2) == 1*2^2 == 4 == 0100 ??
So i saw it like:
lets mask b2 to be set regardless of the rest
so if b3 was set, it will still be set next to setting b2.
so the meaning of BIT(2) got twisted up.
That why i got lost between these two:
SetBit(register,bit) => (register|=(1<<bit))
AND
shift binary 1 by 2
I guess that was a twisted thought?? but now i can say it like _after
going through this conversation_, that:
we are operating on a region of n bits starting from bit x, don't act
like we're operating on a single bit by choosing BIT(y) over (1 << y).
++++
> the compiler won't judge you, but I will. Why is the "slow" mode
> special? This makes the "slow" mode look like it's a binary option, but
> it's not.
>
> > Basically i forgot about this patch and i'am more into clearing this
> > confusion because i'am definitely missing something thats very basic
> > and probably have a wrong understanding of something.
>
> Thanks for sticking with this, I appreciate that you're trying to
> figure out what I'm saying. Is it a bit clearer now?
>
Thank You,
I guess i got it right this time? have i??
Sam
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-04-04 12:55 ` Julia Lawall
@ 2020-04-04 19:50 ` Sam Muhammed
0 siblings, 0 replies; 9+ messages in thread
From: Sam Muhammed @ 2020-04-04 19:50 UTC (permalink / raw)
To: Julia Lawall
Cc: Stefano Brivio, Laurent Pinchart, Mauro Carvalho Chehab,
Greg Kroah-Hartman, outreachy-kernel
On Sat, 2020-04-04 at 14:55 +0200, Julia Lawall wrote:
> > > > #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)
> > >
> >
> > Hi Stefano,
> > honestly i'am confused about how is there a difference between BIT(x)
> > and (1 << x)
>
> In the original code you have a bunch of things shifted to the left by 12
> bits. 3 0 1 and 2. They all look the same, and one can think about a
> little region of bits some of which can be on and some of which can be off.
> If you put BIT(12) in one case you don't see that.
>
> For example, if you had
>
> 1 + 100
> 2 + 100
> 3 + 100
> 104
> 5 + 100
>
Thank you Julia!
this is what i thought of with using the word _pattern_
now its easier to say in other words:
"we are manipulating a number of bits, if there happened to be an
operation where a single bit is set that doesn't kill the fact that _a
number of bits_ are being manipulated and that you could use BIT for
this instance" -> i mean i shouldn't use BIT here
on the other hand:
when there is a behaviour that involves manipulating a single bit only
for all operations, BIT() in functional and meaningful.
I think it's clear for me now ヽ(´▽`)/
> How easy is it to check that you have a sequence of 5 numbers? It's really
> easy to add a single digit integer to 100, but probably you had to think
> just a little extra bit when you came to 104, because it's not following
> the same pattern.
>
> julia
Thank You
Sam
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-04-04 19:32 ` Sam Muhammed
@ 2020-04-04 23:52 ` Stefano Brivio
2020-04-05 0:41 ` Sam Muhammed
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Brivio @ 2020-04-04 23:52 UTC (permalink / raw)
To: Sam Muhammed
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
On Sat, 04 Apr 2020 15:32:23 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:
> On Sat, 2020-04-04 at 14:11 +0200, Stefano Brivio wrote:
> > Hi Sam,
> >
> > On Sat, 04 Apr 2020 01:33:09 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >
> > > On Wed, 2020-04-01 at 04:55 +0200, Stefano Brivio wrote:
> > > > On Tue, 31 Mar 2020 18:58:17 -0400
> > > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > >
> > > [...]
> > >
> > > > > 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)
> > > >
> > >
> > > Hi Stefano,
> > > honestly i'am confused about how is there a difference between BIT(x)
> > > and (1 << x)
> > >
> > > So i'am just going to interpret this hunk as i understand it and
> > > please correct me.
> > >
> > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK (3 << 12)
> > > shift binary 3 by 12: "write 3 starting at bit 12"
> > > 0011 ---- ---- ----
> > >
> > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE (0 << 12)
> > > "write 0 at bit 12"
> > > 0000 ---- ---- ----
> > >
> > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO (1 << 12)
> > > "shift binary 1 by 12": "write 1 at bit 12"
> > > 0001 ---- ---- ----
> > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO BIT(12)
> > > "shift binary 1 by 12": _isn't this what BIT() does?_
> > > 0001 ---- ---- ----
> >
> > This is the key perhaps: it is what BIT() does. And it's not what you
> > should be... thinking of doing. Even though the result is clearly the
> > same.
> >
> > BIT() is used to set a single bit. You need to write *some* bits, here.
> > That "some" is 2: you can write 0, 1, 2, 3, to this region. It's not 1.
> >
> > Now, if you _write_ 1, you need to _set_ one bit, so coincidentally
> > BIT() works too.
> >
> > Suppose you have a tray to make ice cubes. You use one, and put the
> > whole thing back in the freezer. Before you do that, it is your duty
> > towards the society to fill it completely with water.
> >
> > After you use one ice cube, it might look like this:
> >
> > 1 2 3 4
> > .----.----.----.----.
> > |xxxx|xxxx|xxxx|xxxx|
> > |----|----|----|----|
> > |xxxx|xxxx| |xxxx|
> > '----'----'----'----'
> > 5 6 7 8
> >
> > so we have two ways to express what you have to do:
> >
> > a. fill hole number 7
> > b. refill the whole thing
> >
> > a. will do the job just like b., but b. is how you would most naturally
> > describe the operation. Especially because sometimes you take two ice
> > cubes, sometimes three.
> >
> > > now i dont understand what you meant by "That doesn't make bit 12 any
> > > special", does that assume that iam wrong with what BIT() is defined
> > > for?
> >
> > Yes, in some sense. BIT() is used to operate on a single bit: the
> > difference is between:
> >
> > a. set bit 12
> > b. write a number to bit region from 12 to 13
> >
> > if the number from b. is 1, then a. is equivalent.
> >
> > But right above and below this #define you have defines for 0 << 12,
> > 2 << 12, 3 << 12. And 1 << 12 is not a special case.
> >
> > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART (2 << 12)
> > > "shift binary 2 by 12": write 2 at bit 12
> > > 0010 ---- ---- ----
> > >
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >
> > > for your previous explanation: _quoting_ :
> > >
> > > 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?
> > > ++++
> > > now right at this point i got an overlapped definition of BIT()
> > > this expansion/explanation of BIT(2) in my mind got translated to:
> > >
> > > SetBit(register,bit) => (register|=(1<<bit))
> > > NOT
> > > shift binary 1 by 2
> > > ++++
> >
> > Yes, it's also about this. But from your example above:
> > SetBit(register,bit) => (register|=(1<<bit))
> >
> > you need to distinguish when you're operating on a bit *array* or a
> > generic group of bit regions. That is, it's:
> > SetBit(register,bit) => (register|=(n<<bit))
> >
> > where 'n' happens to be 1, sometimes. It's not always '1' by design.
> >
> > Let's take a driver operating a lamp that has three LEDs inside: it
> > might work like this:
> >
> > #define CTRL_REGISTER 0x5 /* a generic 8-bit register */
> > #define RED_ON BIT(0)
> > #define GREEN_ON BIT(1)
> > #define BLUE_ON BIT(2)
> >
> > those are single bits, and consistently so, so using BIT() is fine.
> >
> > Now, the lamp can be on, off, blink slow or fast. In the same register,
> > you have 2 bits (starting from bit 4) controlling that: 0 means "off", 1
> > means "blink slow", 2 means "blink fast", 3 means "on":
> >
> > #define MODE_OFF (0 << 4)
> > #define MODE_SLOW (1 << 4)
> > #define MODE_FAST (2 << 4)
> > #define MODE_ON (3 << 4)
> >
> > this is a natural way of describing things. Suppose you do, instead:
> >
> > #define MODE_OFF (0 << 4)
> > #define MODE_SLOW BIT(4)
> > #define MODE_FAST (2 << 4)
> > #define MODE_ON (3 << 4)
> >
>
> I guess that is a great example:
> So if the operations are on a single bit being toggled on, BIT() is
> welcome to be used.
>
> But when the operations involve a region of bits being toggled on/off i
> should stick to the typical way of bit manipulation even if there
> happens to be an operation that requires only one bit to be set.
>
> ++++
> My thoughts were: _quoting your example_
> #define RED_ON BIT(0)
> #define GREEN_ON BIT(1) => 0001
>
> #define BLUE_ON BIT(2) => 0010
> This does set bit 2, but affected the rest
> "so it was a _lame_ way of saying that there is a region of bits being
> manipulated though only one of them is set" :D
Well, yes, I see what you mean. Two things here:
1. you don't know if my lamp can have more than one colour switched on
at the same time. But it's irrelevant for the purposes of the data
representation: it's still one bit per colour. Other bits might need
to be cleared, and in that case it might be convenient to have:
#define COLOUR_MASK (RED_ON | GREEN_ON | BLUE_ON)
or, somewhat less appropriately:
#define COLOUR_MASK GENMASK(0, 2)
but that's a "meta" operation you do on a set of homogeneous bits,
so it's a completely separated fact.
2. most likely, you would need to write the register as a whole, so you
read, then set or clear bits, then write -- this means essentially
that you write all the bits at a time, yes. But this is not
conceptually relevant, it's an implementation detail.
> ++++
> so when it came to:
> #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)
> *my brain* --> yes, makes sense
>
> #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?
>
> "my brain" --> why b3 is not 0 now?
That's what I would ask, too. Yeah, sure, it will be, but BIT() is
deceiving in this sense.
> shouldn't BIT(2) == 1*2^2 == 4 == 0100 ??
> So i saw it like:
> lets mask b2 to be set regardless of the rest
> so if b3 was set, it will still be set next to setting b2.
> so the meaning of BIT(2) got twisted up.
>
> That why i got lost between these two:
> SetBit(register,bit) => (register|=(1<<bit))
> AND
> shift binary 1 by 2
>
> I guess that was a twisted thought?? but now i can say it like _after
> going through this conversation_, that:
>
> we are operating on a region of n bits starting from bit x, don't act
> like we're operating on a single bit by choosing BIT(y) over (1 << y).
Yes, makes sense to me.
> ++++
>
> > the compiler won't judge you, but I will. Why is the "slow" mode
> > special? This makes the "slow" mode look like it's a binary option, but
> > it's not.
> >
> > > Basically i forgot about this patch and i'am more into clearing this
> > > confusion because i'am definitely missing something thats very basic
> > > and probably have a wrong understanding of something.
> >
> > Thanks for sticking with this, I appreciate that you're trying to
> > figure out what I'm saying. Is it a bit clearer now?
> >
>
> Thank You,
> I guess i got it right this time? have i??
I think so! :) Or use Julia's example, I think it's equivalent to mine
and it can't fail.
--
Stefano
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro
2020-04-04 23:52 ` Stefano Brivio
@ 2020-04-05 0:41 ` Sam Muhammed
0 siblings, 0 replies; 9+ messages in thread
From: Sam Muhammed @ 2020-04-05 0:41 UTC (permalink / raw)
To: Stefano Brivio
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman,
outreachy-kernel
On Sun, 2020-04-05 at 01:52 +0200, Stefano Brivio wrote:
> On Sat, 04 Apr 2020 15:32:23 -0400
> Sam Muhammed <jane.pnx9@gmail.com> wrote:
>
> > On Sat, 2020-04-04 at 14:11 +0200, Stefano Brivio wrote:
> > > Hi Sam,
> > >
> > > On Sat, 04 Apr 2020 01:33:09 -0400
> > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > >
> > > > On Wed, 2020-04-01 at 04:55 +0200, Stefano Brivio wrote:
> > > > > On Tue, 31 Mar 2020 18:58:17 -0400
> > > > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > > >
> > > > [...]
> > > >
> > > > > > 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)
> > > > >
> > > >
> > > > Hi Stefano,
> > > > honestly i'am confused about how is there a difference between BIT(x)
> > > > and (1 << x)
> > > >
> > > > So i'am just going to interpret this hunk as i understand it and
> > > > please correct me.
> > > >
> > > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK (3 << 12)
> > > > shift binary 3 by 12: "write 3 starting at bit 12"
> > > > 0011 ---- ---- ----
> > > >
> > > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE (0 << 12)
> > > > "write 0 at bit 12"
> > > > 0000 ---- ---- ----
> > > >
> > > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO (1 << 12)
> > > > "shift binary 1 by 12": "write 1 at bit 12"
> > > > 0001 ---- ---- ----
> > > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO BIT(12)
> > > > "shift binary 1 by 12": _isn't this what BIT() does?_
> > > > 0001 ---- ---- ----
> > >
> > > This is the key perhaps: it is what BIT() does. And it's not what you
> > > should be... thinking of doing. Even though the result is clearly the
> > > same.
> > >
> > > BIT() is used to set a single bit. You need to write *some* bits, here.
> > > That "some" is 2: you can write 0, 1, 2, 3, to this region. It's not 1.
> > >
> > > Now, if you _write_ 1, you need to _set_ one bit, so coincidentally
> > > BIT() works too.
> > >
> > > Suppose you have a tray to make ice cubes. You use one, and put the
> > > whole thing back in the freezer. Before you do that, it is your duty
> > > towards the society to fill it completely with water.
> > >
> > > After you use one ice cube, it might look like this:
> > >
> > > 1 2 3 4
> > > .----.----.----.----.
> > > |xxxx|xxxx|xxxx|xxxx|
> > > |----|----|----|----|
> > > |xxxx|xxxx| |xxxx|
> > > '----'----'----'----'
> > > 5 6 7 8
> > >
> > > so we have two ways to express what you have to do:
> > >
> > > a. fill hole number 7
> > > b. refill the whole thing
> > >
> > > a. will do the job just like b., but b. is how you would most naturally
> > > describe the operation. Especially because sometimes you take two ice
> > > cubes, sometimes three.
> > >
> > > > now i dont understand what you meant by "That doesn't make bit 12 any
> > > > special", does that assume that iam wrong with what BIT() is defined
> > > > for?
> > >
> > > Yes, in some sense. BIT() is used to operate on a single bit: the
> > > difference is between:
> > >
> > > a. set bit 12
> > > b. write a number to bit region from 12 to 13
> > >
> > > if the number from b. is 1, then a. is equivalent.
> > >
> > > But right above and below this #define you have defines for 0 << 12,
> > > 2 << 12, 3 << 12. And 1 << 12 is not a special case.
> > >
> > > > #define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART (2 << 12)
> > > > "shift binary 2 by 12": write 2 at bit 12
> > > > 0010 ---- ---- ----
> > > >
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >
> > > > for your previous explanation: _quoting_ :
> > > >
> > > > 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?
> > > > ++++
> > > > now right at this point i got an overlapped definition of BIT()
> > > > this expansion/explanation of BIT(2) in my mind got translated to:
> > > >
> > > > SetBit(register,bit) => (register|=(1<<bit))
> > > > NOT
> > > > shift binary 1 by 2
> > > > ++++
> > >
> > > Yes, it's also about this. But from your example above:
> > > SetBit(register,bit) => (register|=(1<<bit))
> > >
> > > you need to distinguish when you're operating on a bit *array* or a
> > > generic group of bit regions. That is, it's:
> > > SetBit(register,bit) => (register|=(n<<bit))
> > >
> > > where 'n' happens to be 1, sometimes. It's not always '1' by design.
> > >
> > > Let's take a driver operating a lamp that has three LEDs inside: it
> > > might work like this:
> > >
> > > #define CTRL_REGISTER 0x5 /* a generic 8-bit register */
> > > #define RED_ON BIT(0)
> > > #define GREEN_ON BIT(1)
> > > #define BLUE_ON BIT(2)
> > >
> > > those are single bits, and consistently so, so using BIT() is fine.
> > >
> > > Now, the lamp can be on, off, blink slow or fast. In the same register,
> > > you have 2 bits (starting from bit 4) controlling that: 0 means "off", 1
> > > means "blink slow", 2 means "blink fast", 3 means "on":
> > >
> > > #define MODE_OFF (0 << 4)
> > > #define MODE_SLOW (1 << 4)
> > > #define MODE_FAST (2 << 4)
> > > #define MODE_ON (3 << 4)
> > >
> > > this is a natural way of describing things. Suppose you do, instead:
> > >
> > > #define MODE_OFF (0 << 4)
> > > #define MODE_SLOW BIT(4)
> > > #define MODE_FAST (2 << 4)
> > > #define MODE_ON (3 << 4)
> > >
> >
> > I guess that is a great example:
> > So if the operations are on a single bit being toggled on, BIT() is
> > welcome to be used.
> >
> > But when the operations involve a region of bits being toggled on/off i
> > should stick to the typical way of bit manipulation even if there
> > happens to be an operation that requires only one bit to be set.
> >
> > ++++
> > My thoughts were: _quoting your example_
> > #define RED_ON BIT(0)
> > #define GREEN_ON BIT(1) => 0001
> >
> > #define BLUE_ON BIT(2) => 0010
> > This does set bit 2, but affected the rest
> > "so it was a _lame_ way of saying that there is a region of bits being
> > manipulated though only one of them is set" :D
>
> Well, yes, I see what you mean. Two things here:
>
> 1. you don't know if my lamp can have more than one colour switched on
> at the same time. But it's irrelevant for the purposes of the data
> representation: it's still one bit per colour. Other bits might need
> to be cleared, and in that case it might be convenient to have:
> #define COLOUR_MASK (RED_ON | GREEN_ON | BLUE_ON)
>
> or, somewhat less appropriately:
> #define COLOUR_MASK GENMASK(0, 2)
>
> but that's a "meta" operation you do on a set of homogeneous bits,
> so it's a completely separated fact.
>
> 2. most likely, you would need to write the register as a whole, so you
> read, then set or clear bits, then write -- this means essentially
> that you write all the bits at a time, yes. But this is not
> conceptually relevant, it's an implementation detail.
>
> > ++++
> > so when it came to:
> > #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)
> > *my brain* --> yes, makes sense
> >
> > #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?
> >
> > "my brain" --> why b3 is not 0 now?
>
> That's what I would ask, too. Yeah, sure, it will be, but BIT() is
> deceiving in this sense.
>
> > shouldn't BIT(2) == 1*2^2 == 4 == 0100 ??
> > So i saw it like:
> > lets mask b2 to be set regardless of the rest
> > so if b3 was set, it will still be set next to setting b2.
> > so the meaning of BIT(2) got twisted up.
> >
> > That why i got lost between these two:
> > SetBit(register,bit) => (register|=(1<<bit))
> > AND
> > shift binary 1 by 2
> >
> > I guess that was a twisted thought?? but now i can say it like _after
> > going through this conversation_, that:
> >
> > we are operating on a region of n bits starting from bit x, don't act
> > like we're operating on a single bit by choosing BIT(y) over (1 << y).
>
> Yes, makes sense to me.
>
> > ++++
> >
> > > the compiler won't judge you, but I will. Why is the "slow" mode
> > > special? This makes the "slow" mode look like it's a binary option, but
> > > it's not.
> > >
> > > > Basically i forgot about this patch and i'am more into clearing this
> > > > confusion because i'am definitely missing something thats very basic
> > > > and probably have a wrong understanding of something.
> > >
> > > Thanks for sticking with this, I appreciate that you're trying to
> > > figure out what I'm saying. Is it a bit clearer now?
> > >
> >
> > Thank You,
> > I guess i got it right this time? have i??
>
> I think so! :) Or use Julia's example, I think it's equivalent to mine
> and it can't fail.
>
Alright, Great, I'll check for a 3rd revision then.
Thank You.
Sam
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-05 0:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-31 22:58 [PATCH v2] Staging: media: omap4iss: Use BIT() macro Sam Muhammed
2020-04-01 2:55 ` [Outreachy kernel] " Stefano Brivio
2020-04-04 5:33 ` Sam Muhammed
2020-04-04 12:11 ` Stefano Brivio
2020-04-04 19:32 ` Sam Muhammed
2020-04-04 23:52 ` Stefano Brivio
2020-04-05 0:41 ` Sam Muhammed
2020-04-04 12:55 ` Julia Lawall
2020-04-04 19:50 ` 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.