* Re: [PATCH] staging: media: omap4iss: code style - avoid macro argument precedence issues
@ 2021-02-21 21:21 Nikolay K.
2021-02-21 21:33 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay K. @ 2021-02-21 21:21 UTC (permalink / raw)
To: Laurent Pinchart
Cc: devel, linux-kernel, linux-media, Greg Kroah-Hartman,
Mauro Carvalho Chehab
Hi Laurent,
Thank you for the review.
I think that if we drop the unneeded parentheses here, we need to drop
them everywhere in the file for consistency, even in places checkpatch.pl
doesn't warn about. It'll increase patch size without actual usefullness
gain, as for me. I am very (very) novice to the kernel,
but who wants slightly more readable one-line simple macros?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: media: omap4iss: code style - avoid macro argument precedence issues
2021-02-21 21:21 [PATCH] staging: media: omap4iss: code style - avoid macro argument precedence issues Nikolay K.
@ 2021-02-21 21:33 ` Laurent Pinchart
0 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2021-02-21 21:33 UTC (permalink / raw)
To: Nikolay K.
Cc: devel, linux-kernel, linux-media, Greg Kroah-Hartman,
Mauro Carvalho Chehab
Hi Nikolay,
On Mon, Feb 22, 2021 at 12:21:36AM +0300, Nikolay K. wrote:
> Hi Laurent,
>
> Thank you for the review.
> I think that if we drop the unneeded parentheses here, we need to drop
> them everywhere in the file for consistency, even in places checkpatch.pl
That's a good point.
> doesn't warn about. It'll increase patch size without actual usefullness
> gain, as for me. I am very (very) novice to the kernel,
> but who wants slightly more readable one-line simple macros?
Let's keep your patch as-is, we can drop the unneeded parentheses in a
subsequent patch if desired.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] staging: media: omap4iss: code style - avoid macro argument precedence issues
@ 2021-02-21 19:53 Nikolay Kyx
2021-02-21 20:02 ` Laurent Pinchart
2021-02-21 21:33 ` Laurent Pinchart
0 siblings, 2 replies; 5+ messages in thread
From: Nikolay Kyx @ 2021-02-21 19:53 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab, Greg Kroah-Hartman
Cc: Nikolay Kyx, linux-media, devel, linux-kernel
This patch fixes the following checkpatch.pl check:
CHECK: Macro argument 'i' may be better as '(i)' to avoid precedence issues
in file iss_regs.h
Signed-off-by: Nikolay Kyx <knv418@gmail.com>
---
Additionally some style warnings remain valid here and could be fixed by
another patch.
drivers/staging/media/omap4iss/iss_regs.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
index 09a7375c89ac..cfe0bb075072 100644
--- a/drivers/staging/media/omap4iss/iss_regs.h
+++ b/drivers/staging/media/omap4iss/iss_regs.h
@@ -197,7 +197,7 @@
#define CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK (0x1fff << 0)
#define CSI2_TIMING_STOP_STATE_COUNTER_IO1_SHIFT 0
-#define CSI2_CTX_CTRL1(i) (0x70 + (0x20 * i))
+#define CSI2_CTX_CTRL1(i) (0x70 + (0x20 * (i)))
#define CSI2_CTX_CTRL1_GENERIC BIT(30)
#define CSI2_CTX_CTRL1_TRANSCODE (0xf << 24)
#define CSI2_CTX_CTRL1_FEC_NUMBER_MASK (0xff << 16)
@@ -210,7 +210,7 @@
#define CSI2_CTX_CTRL1_PING_PONG BIT(3)
#define CSI2_CTX_CTRL1_CTX_EN BIT(0)
-#define CSI2_CTX_CTRL2(i) (0x74 + (0x20 * i))
+#define CSI2_CTX_CTRL2(i) (0x74 + (0x20 * (i)))
#define CSI2_CTX_CTRL2_FRAME_MASK (0xffff << 16)
#define CSI2_CTX_CTRL2_FRAME_SHIFT 16
#define CSI2_CTX_CTRL2_USER_DEF_MAP_SHIFT 13
@@ -222,19 +222,19 @@
#define CSI2_CTX_CTRL2_FORMAT_MASK (0x3ff << 0)
#define CSI2_CTX_CTRL2_FORMAT_SHIFT 0
-#define CSI2_CTX_DAT_OFST(i) (0x78 + (0x20 * i))
+#define CSI2_CTX_DAT_OFST(i) (0x78 + (0x20 * (i)))
#define CSI2_CTX_DAT_OFST_MASK (0xfff << 5)
-#define CSI2_CTX_PING_ADDR(i) (0x7c + (0x20 * i))
+#define CSI2_CTX_PING_ADDR(i) (0x7c + (0x20 * (i)))
#define CSI2_CTX_PING_ADDR_MASK 0xffffffe0
-#define CSI2_CTX_PONG_ADDR(i) (0x80 + (0x20 * i))
+#define CSI2_CTX_PONG_ADDR(i) (0x80 + (0x20 * (i)))
#define CSI2_CTX_PONG_ADDR_MASK CSI2_CTX_PING_ADDR_MASK
-#define CSI2_CTX_IRQENABLE(i) (0x84 + (0x20 * i))
-#define CSI2_CTX_IRQSTATUS(i) (0x88 + (0x20 * i))
+#define CSI2_CTX_IRQENABLE(i) (0x84 + (0x20 * (i)))
+#define CSI2_CTX_IRQSTATUS(i) (0x88 + (0x20 * (i)))
-#define CSI2_CTX_CTRL3(i) (0x8c + (0x20 * i))
+#define CSI2_CTX_CTRL3(i) (0x8c + (0x20 * (i)))
#define CSI2_CTX_CTRL3_ALPHA_SHIFT 5
#define CSI2_CTX_CTRL3_ALPHA_MASK \
(0x3fff << CSI2_CTX_CTRL3_ALPHA_SHIFT)
--
2.30.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: media: omap4iss: code style - avoid macro argument precedence issues
2021-02-21 19:53 Nikolay Kyx
@ 2021-02-21 20:02 ` Laurent Pinchart
2021-02-21 21:33 ` Laurent Pinchart
1 sibling, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2021-02-21 20:02 UTC (permalink / raw)
To: Nikolay Kyx
Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media, devel,
linux-kernel
Hi Nikolay,
Thank you for the patch.
On Sun, Feb 21, 2021 at 10:53:08PM +0300, Nikolay Kyx wrote:
> This patch fixes the following checkpatch.pl check:
>
> CHECK: Macro argument 'i' may be better as '(i)' to avoid precedence issues
>
> in file iss_regs.h
>
> Signed-off-by: Nikolay Kyx <knv418@gmail.com>
> ---
>
> Additionally some style warnings remain valid here and could be fixed by
> another patch.
>
> drivers/staging/media/omap4iss/iss_regs.h | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> index 09a7375c89ac..cfe0bb075072 100644
> --- a/drivers/staging/media/omap4iss/iss_regs.h
> +++ b/drivers/staging/media/omap4iss/iss_regs.h
> @@ -197,7 +197,7 @@
> #define CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK (0x1fff << 0)
> #define CSI2_TIMING_STOP_STATE_COUNTER_IO1_SHIFT 0
>
> -#define CSI2_CTX_CTRL1(i) (0x70 + (0x20 * i))
> +#define CSI2_CTX_CTRL1(i) (0x70 + (0x20 * (i)))
This is a good change, as it fixes potential issues, but maybe we could
go one step forward and drop the unneeded parentheses ?
-#define CSI2_CTX_CTRL1(i) (0x70 + (0x20 * i))
+#define CSI2_CTX_CTRL1(i) (0x70 + 0x20 * (i))
What do you think ?
> #define CSI2_CTX_CTRL1_GENERIC BIT(30)
> #define CSI2_CTX_CTRL1_TRANSCODE (0xf << 24)
> #define CSI2_CTX_CTRL1_FEC_NUMBER_MASK (0xff << 16)
> @@ -210,7 +210,7 @@
> #define CSI2_CTX_CTRL1_PING_PONG BIT(3)
> #define CSI2_CTX_CTRL1_CTX_EN BIT(0)
>
> -#define CSI2_CTX_CTRL2(i) (0x74 + (0x20 * i))
> +#define CSI2_CTX_CTRL2(i) (0x74 + (0x20 * (i)))
> #define CSI2_CTX_CTRL2_FRAME_MASK (0xffff << 16)
> #define CSI2_CTX_CTRL2_FRAME_SHIFT 16
> #define CSI2_CTX_CTRL2_USER_DEF_MAP_SHIFT 13
> @@ -222,19 +222,19 @@
> #define CSI2_CTX_CTRL2_FORMAT_MASK (0x3ff << 0)
> #define CSI2_CTX_CTRL2_FORMAT_SHIFT 0
>
> -#define CSI2_CTX_DAT_OFST(i) (0x78 + (0x20 * i))
> +#define CSI2_CTX_DAT_OFST(i) (0x78 + (0x20 * (i)))
> #define CSI2_CTX_DAT_OFST_MASK (0xfff << 5)
>
> -#define CSI2_CTX_PING_ADDR(i) (0x7c + (0x20 * i))
> +#define CSI2_CTX_PING_ADDR(i) (0x7c + (0x20 * (i)))
> #define CSI2_CTX_PING_ADDR_MASK 0xffffffe0
>
> -#define CSI2_CTX_PONG_ADDR(i) (0x80 + (0x20 * i))
> +#define CSI2_CTX_PONG_ADDR(i) (0x80 + (0x20 * (i)))
> #define CSI2_CTX_PONG_ADDR_MASK CSI2_CTX_PING_ADDR_MASK
>
> -#define CSI2_CTX_IRQENABLE(i) (0x84 + (0x20 * i))
> -#define CSI2_CTX_IRQSTATUS(i) (0x88 + (0x20 * i))
> +#define CSI2_CTX_IRQENABLE(i) (0x84 + (0x20 * (i)))
> +#define CSI2_CTX_IRQSTATUS(i) (0x88 + (0x20 * (i)))
>
> -#define CSI2_CTX_CTRL3(i) (0x8c + (0x20 * i))
> +#define CSI2_CTX_CTRL3(i) (0x8c + (0x20 * (i)))
> #define CSI2_CTX_CTRL3_ALPHA_SHIFT 5
> #define CSI2_CTX_CTRL3_ALPHA_MASK \
> (0x3fff << CSI2_CTX_CTRL3_ALPHA_SHIFT)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: media: omap4iss: code style - avoid macro argument precedence issues
2021-02-21 19:53 Nikolay Kyx
2021-02-21 20:02 ` Laurent Pinchart
@ 2021-02-21 21:33 ` Laurent Pinchart
1 sibling, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2021-02-21 21:33 UTC (permalink / raw)
To: Nikolay Kyx
Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media, devel,
linux-kernel
Hi Nikolay,
Thank you for the patch.
On Sun, Feb 21, 2021 at 10:53:08PM +0300, Nikolay Kyx wrote:
> This patch fixes the following checkpatch.pl check:
>
> CHECK: Macro argument 'i' may be better as '(i)' to avoid precedence issues
>
> in file iss_regs.h
>
> Signed-off-by: Nikolay Kyx <knv418@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>
> Additionally some style warnings remain valid here and could be fixed by
> another patch.
>
> drivers/staging/media/omap4iss/iss_regs.h | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> index 09a7375c89ac..cfe0bb075072 100644
> --- a/drivers/staging/media/omap4iss/iss_regs.h
> +++ b/drivers/staging/media/omap4iss/iss_regs.h
> @@ -197,7 +197,7 @@
> #define CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK (0x1fff << 0)
> #define CSI2_TIMING_STOP_STATE_COUNTER_IO1_SHIFT 0
>
> -#define CSI2_CTX_CTRL1(i) (0x70 + (0x20 * i))
> +#define CSI2_CTX_CTRL1(i) (0x70 + (0x20 * (i)))
> #define CSI2_CTX_CTRL1_GENERIC BIT(30)
> #define CSI2_CTX_CTRL1_TRANSCODE (0xf << 24)
> #define CSI2_CTX_CTRL1_FEC_NUMBER_MASK (0xff << 16)
> @@ -210,7 +210,7 @@
> #define CSI2_CTX_CTRL1_PING_PONG BIT(3)
> #define CSI2_CTX_CTRL1_CTX_EN BIT(0)
>
> -#define CSI2_CTX_CTRL2(i) (0x74 + (0x20 * i))
> +#define CSI2_CTX_CTRL2(i) (0x74 + (0x20 * (i)))
> #define CSI2_CTX_CTRL2_FRAME_MASK (0xffff << 16)
> #define CSI2_CTX_CTRL2_FRAME_SHIFT 16
> #define CSI2_CTX_CTRL2_USER_DEF_MAP_SHIFT 13
> @@ -222,19 +222,19 @@
> #define CSI2_CTX_CTRL2_FORMAT_MASK (0x3ff << 0)
> #define CSI2_CTX_CTRL2_FORMAT_SHIFT 0
>
> -#define CSI2_CTX_DAT_OFST(i) (0x78 + (0x20 * i))
> +#define CSI2_CTX_DAT_OFST(i) (0x78 + (0x20 * (i)))
> #define CSI2_CTX_DAT_OFST_MASK (0xfff << 5)
>
> -#define CSI2_CTX_PING_ADDR(i) (0x7c + (0x20 * i))
> +#define CSI2_CTX_PING_ADDR(i) (0x7c + (0x20 * (i)))
> #define CSI2_CTX_PING_ADDR_MASK 0xffffffe0
>
> -#define CSI2_CTX_PONG_ADDR(i) (0x80 + (0x20 * i))
> +#define CSI2_CTX_PONG_ADDR(i) (0x80 + (0x20 * (i)))
> #define CSI2_CTX_PONG_ADDR_MASK CSI2_CTX_PING_ADDR_MASK
>
> -#define CSI2_CTX_IRQENABLE(i) (0x84 + (0x20 * i))
> -#define CSI2_CTX_IRQSTATUS(i) (0x88 + (0x20 * i))
> +#define CSI2_CTX_IRQENABLE(i) (0x84 + (0x20 * (i)))
> +#define CSI2_CTX_IRQSTATUS(i) (0x88 + (0x20 * (i)))
>
> -#define CSI2_CTX_CTRL3(i) (0x8c + (0x20 * i))
> +#define CSI2_CTX_CTRL3(i) (0x8c + (0x20 * (i)))
> #define CSI2_CTX_CTRL3_ALPHA_SHIFT 5
> #define CSI2_CTX_CTRL3_ALPHA_MASK \
> (0x3fff << CSI2_CTX_CTRL3_ALPHA_SHIFT)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-21 21:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-21 21:21 [PATCH] staging: media: omap4iss: code style - avoid macro argument precedence issues Nikolay K.
2021-02-21 21:33 ` Laurent Pinchart
-- strict thread matches above, loose matches on Subject: below --
2021-02-21 19:53 Nikolay Kyx
2021-02-21 20:02 ` Laurent Pinchart
2021-02-21 21:33 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®