[PATCH] video: fbdev: sm712: Fix operator precedence in big_swap macro

[PATCH] video: fbdev: sm712: Fix operator precedence in big_swap macro

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

The big_swap(p) macro was intended to swap bytes within 16-bit halves
of a 32-bit value. However, because the bitwise shift operators (<<, >>)
have higher precedence than the bitwise AND operator (&), the original
code failed to perform any shifting on the masked bits.

For example, 'p & 0xff00ff00 >> 8' was evaluated as 'p &
(0xff00ff00 >> 8)', effectively neutralizing the intended swap.

Fix this by adding parentheses to ensure the bitwise AND is performed
before the shift, correctly implementing the byte swap logic.

Fixes: 1461d66728648 ("staging: sm7xxfb: merge sm712fb with fbdev")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/video/fbdev/sm712.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/sm712.h b/drivers/video/fbdev/sm712.h
index c7ebf03..83fe25f 100644
--- a/drivers/video/fbdev/sm712.h
+++ b/drivers/video/fbdev/sm712.h
@@ -101,7 +101,7 @@ struct modeinit {
 #define mmio_addr		0x00800000
 #define seqw17()		smtc_seqw(0x17, 0x30)
 #define big_pixel_depth(p, d)	{if (p == 24) {p = 32; d = 32; } }
-#define big_swap(p)		((p & 0xff00ff00 >> 8) | (p & 0x00ff00ff << 8))
+#define big_swap(p)		(((p & 0xff00ff00) >> 8) | ((p & 0x00ff00ff) << 8))
 #else
 #define pal_rgb(r, g, b, val)	val
 #define big_addr		0
-- 
2.9.4

Claude review: video: fbdev: sm712: Fix operator precedence in big_swap macro

[Claude Code Review Bot]

Overall Series Review

Subject: video: fbdev: sm712: Fix operator precedence in big_swap macro
Author: lirongqing <lirongqing@baidu.com>
Patches: 1
Reviewed: 2026-05-16T10:14:54.297228

---

This is a single-patch series fixing a genuine operator precedence bug in the `big_swap` macro in the SM712 fbdev driver. The fix is correct and straightforward. The bug has been present since the code was merged from staging (`1461d66728648`), meaning the big-endian byte swap path has been silently broken for the entire lifetime of this driver in the fbdev tree.

The fix is low-risk and clearly correct. **Recommend: Accept.**

One minor observation: the macro arguments still lack parenthesizing of `p` itself (i.e., it should ideally be `(((p) & 0xff00ff00) >> 8) | (((p) & 0x00ff00ff) << 8)`) to protect against macro argument expansion issues if `p` is an expression. However, the existing callers at `sm712fb.c:1010`, `sm712fb.c:1069`, and `sm712fb.c:1141` all pass simple identifiers (`val`, `val`, `*src`), so this is a pre-existing style issue rather than a bug, and it's reasonable not to address it here.

---
Generated by Claude Code Patch Reviewer

Claude review: video: fbdev: sm712: Fix operator precedence in big_swap macro

[Claude Code Review Bot]

Patch Review

**Status: Correct fix for a real bug.**

The commit message clearly explains the problem: C operator precedence means `<<` and `>>` bind tighter than `&`, so the original code:

```c
#define big_swap(p)		((p & 0xff00ff00 >> 8) | (p & 0x00ff00ff << 8))
```

was parsed as:

```c
((p & (0xff00ff00 >> 8)) | (p & (0x00ff00ff << 8)))
```

which is `((p & 0x00ff00ff) | (p & 0xff00ff00))` — effectively just `p` (identity), completely defeating the byte swap.

The fix:

```c
#define big_swap(p)		(((p & 0xff00ff00) >> 8) | ((p & 0x00ff00ff) << 8))
```

correctly masks first, then shifts, implementing a proper swap of the high and low bytes within each 16-bit half of a 32-bit value.

**Verification:** All three call sites in `sm712fb.c` (lines 1010, 1069, 1141) use this for big-endian palette and framebuffer operations. The fix restores the intended byte-swap behavior that has been broken since the original merge.

**Nit (not blocking):** The macro argument `p` is not wrapped in parentheses (e.g., `(p)`) to guard against complex expressions. Since all current callers pass simple lvalues, this is a pre-existing cosmetic issue and not worth blocking the fix.

**Verdict: Reviewed-by worthy.** The Fixes tag correctly identifies the originating commit, and the fix is minimal and correct.

---
Generated by Claude Code Patch Reviewer