[PATCH] video: Add parentheses around VNBYTES() macro
The VNBYTES() macro needs to have parentheses to prevent some (harmless) macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
The * operation is done before the / operation. It still ends up with the same results, but it's not ideal.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org --- include/video.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/video.h b/include/video.h index 9729fa348aa5..b364bf91825f 100644 --- a/include/video.h +++ b/include/video.h @@ -58,7 +58,7 @@ enum video_log2_bpp { * Convert enum video_log2_bpp to bytes and bits. Note we omit the outer * brackets to allow multiplication by fractional pixels. */ -#define VNBYTES(bpix) (1 << (bpix)) / 8 +#define VNBYTES(bpix) ((1 << (bpix)) / 8)
#define VNBITS(bpix) (1 << (bpix))
On Wed, 26 Jul 2023 at 00:54, Dan Carpenter dan.carpenter@linaro.org wrote:
The VNBYTES() macro needs to have parentheses to prevent some (harmless) macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)The * operation is done before the / operation. It still ends up with the same results, but it's not ideal.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org
include/video.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
I'd argue that doing * before / is better, since it might be more accurate. But I don't know if it actually matters.
Reviewed-by: Simon Glass sjg@chromium.org
On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
The VNBYTES() macro needs to have parentheses to prevent some (harmless) macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
The * operation is done before the / operation. It still ends up with the same results, but it's not ideal.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
In that this seems correct:
Applied to u-boot/next, thanks!
But I want to note that with gcc-13.1 (and binutils 2.40) or more specifically the kernel.org arm/aarch64/etc toolchains, this causes the generated code to change: aarch64: (for 1/1 boards) all +8.0 text +8.0 bananapi-m5 : all +8 text +8 u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8) function old new delta video_post_probe 248 256 +8
So I don't know that this was a harmless bug.
Hi Tom, Dan,
On Tue, 8 Aug 2023 at 19:39, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
The VNBYTES() macro needs to have parentheses to prevent some (harmless) macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)The * operation is done before the / operation. It still ends up with the same results, but it's not ideal.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
In that this seems correct:
Applied to u-boot/next, thanks!
But I want to note that with gcc-13.1 (and binutils 2.40) or more specifically the kernel.org arm/aarch64/etc toolchains, this causes the generated code to change: aarch64: (for 1/1 boards) all +8.0 text +8.0 bananapi-m5 : all +8 text +8 u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8) function old new delta video_post_probe 248 256 +8
So I don't know that this was a harmless bug.
Hmm I should have read the comment directly above...
"Note we omit the outer brackets to allow multiplication by fractional pixels."
Having said that, it doesn't seem to matter getting (if indeed we do) a less accurate result.
Regards, Simon
On Mon, Sep 18, 2023 at 07:04:41PM -0600, Simon Glass wrote:
Hi Tom, Dan,
On Tue, 8 Aug 2023 at 19:39, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
The VNBYTES() macro needs to have parentheses to prevent some (harmless) macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)The * operation is done before the / operation. It still ends up with the same results, but it's not ideal.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
In that this seems correct:
Applied to u-boot/next, thanks!
But I want to note that with gcc-13.1 (and binutils 2.40) or more specifically the kernel.org arm/aarch64/etc toolchains, this causes the generated code to change: aarch64: (for 1/1 boards) all +8.0 text +8.0 bananapi-m5 : all +8 text +8 u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8) function old new delta video_post_probe 248 256 +8
So I don't know that this was a harmless bug.
Hmm I should have read the comment directly above...
"Note we omit the outer brackets to allow multiplication by fractional pixels."
Having said that, it doesn't seem to matter getting (if indeed we do) a less accurate result.
But we should fix the comment then, or decide no, this is intentional.
Hello,
trini@konsulko.com wrote on Tue, 19 Sep 2023 11:14:56 -0400:
On Mon, Sep 18, 2023 at 07:04:41PM -0600, Simon Glass wrote:
Hi Tom, Dan,
On Tue, 8 Aug 2023 at 19:39, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
The VNBYTES() macro needs to have parentheses to prevent some (harmless) macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)The * operation is done before the / operation. It still ends up with the same results, but it's not ideal.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
In that this seems correct:
Applied to u-boot/next, thanks!
But I want to note that with gcc-13.1 (and binutils 2.40) or more specifically the kernel.org arm/aarch64/etc toolchains, this causes the generated code to change: aarch64: (for 1/1 boards) all +8.0 text +8.0 bananapi-m5 : all +8 text +8 u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8) function old new delta video_post_probe 248 256 +8
So I don't know that this was a harmless bug.
Hmm I should have read the comment directly above...
"Note we omit the outer brackets to allow multiplication by fractional pixels."
Having said that, it doesn't seem to matter getting (if indeed we do) a less accurate result.
But we should fix the comment then, or decide no, this is intentional.
One year has passed and I just stumbled upon this incoherency. It would be nice to clarify the comment or otherwise to revert Dan's patch if judged irrelevant in the end.
Thanks! Miquèl
On Mon, Sep 02, 2024 at 10:17:59AM +0200, Miquel Raynal wrote:
Hello,
trini@konsulko.com wrote on Tue, 19 Sep 2023 11:14:56 -0400:
On Mon, Sep 18, 2023 at 07:04:41PM -0600, Simon Glass wrote:
Hi Tom, Dan,
On Tue, 8 Aug 2023 at 19:39, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
The VNBYTES() macro needs to have parentheses to prevent some (harmless) macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)The * operation is done before the / operation. It still ends up with the same results, but it's not ideal.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
In that this seems correct:
Applied to u-boot/next, thanks!
But I want to note that with gcc-13.1 (and binutils 2.40) or more specifically the kernel.org arm/aarch64/etc toolchains, this causes the generated code to change: aarch64: (for 1/1 boards) all +8.0 text +8.0 bananapi-m5 : all +8 text +8 u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8) function old new delta video_post_probe 248 256 +8
So I don't know that this was a harmless bug.
Hmm I should have read the comment directly above...
"Note we omit the outer brackets to allow multiplication by fractional pixels."
Having said that, it doesn't seem to matter getting (if indeed we do) a less accurate result.
But we should fix the comment then, or decide no, this is intentional.
One year has passed and I just stumbled upon this incoherency. It would be nice to clarify the comment or otherwise to revert Dan's patch if judged irrelevant in the end.
I guess at this point I vote for fixing the comment.
participants (4)
-
Dan Carpenter -
Miquel Raynal -
Simon Glass -
Tom Rini