From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] staging: fbtft: avoid empty macro argument in define_fbtft_write_reg Date: Fri, 13 Mar 2026 14:18:06 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness**: The patch is functionally correct. Where `modifier` was em= pty, the macro expansion `modifier((data_type)va_arg(...))` would produce `= ((data_type)va_arg(...))` =E2=80=94 just the expression in parentheses. Wit= h `fbtft_identity`, it produces `(((data_type)va_arg(...)))` =E2=80=94 an e= xtra layer of parentheses that has no semantic effect. **Issue 1: Blank line removal** The patch removes the blank line between the `EXPORT_SYMBOL(func);` end of = the macro definition and the first invocation: ```c - -define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, ) +#define fbtft_identity(x) (x) +define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, fbtft_identity) ``` The `#define` is jammed right up against the end of the preceding macro. It= would be cleaner to keep a blank line separator before the new `#define`, = improving readability: ```c EXPORT_SYMBOL(func); #define fbtft_identity(x) (x) define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, fbtft_identity) ``` **Issue 2: Naming and scope** `fbtft_identity` is defined in a `.c` file and used only locally, so the sc= ope is fine. However, the name `fbtft_identity` is a bit unusual. A more co= nventional kernel approach might be to check if there's already a no-op/ide= ntity macro available, or to name it something more descriptive like `fbtft= _noop_modifier`. This is minor and stylistic. **Issue 3: Commit message** The commit message says "This fixes the error reported by scripts/checkpatc= h.pl" but doesn't quote the actual checkpatch warning. It would be better p= ractice to include the specific warning text so reviewers can evaluate whet= her this is the right fix. For staging code especially, checkpatch fixes ar= e common and welcome, but stating the exact warning is standard practice. **Overall**: The patch is a straightforward, correct cleanup. The suggestio= ns above are minor. It could also be worth checking if a series-wide cleanu= p of this file is planned (since this is staging code), but as a standalone= fix it's fine. --- Generated by Claude Code Patch Reviewer