Re: [PATCH] staging: rts5208: Add parenthesis to macro arguments

[Julia Lawall]

On Thu, 19 Oct 2023, kenechukwu maduechesi wrote:

> Checkpatch suggests using (reg) and (host) instead of reg and host
>
> The use of parenthesis in the macro argument '(reg)' ensures proper
> precedence and resolves potential issues that may arise due to the
> surrounding code context. This modification adheres to the recommended
> coding style and improves the readability or maintainability of the
> code.
>
> Signed-off-by: kenechukwu maduechesi <maduechesik@xxxxxxxxx>
> ---
>  drivers/staging/rts5208/rtsx.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.h b/drivers/staging/rts5208/rtsx.h
> index 2e101da83220..7d3373797eb4 100644
> --- a/drivers/staging/rts5208/rtsx.h
> +++ b/drivers/staging/rts5208/rtsx.h
> @@ -39,17 +39,17 @@
>  /*
>   * macros for easy use
>   */
> -#define rtsx_writel(chip, reg, value) \
> +#define rtsx_writel(chip, (reg), value) \
>  	iowrite32(value, (chip)->rtsx->remap_addr + reg)

I don't think this code has been subjected to the compiler.  Note that you
can't comiple a .h file directly; you have to find some other file that
includes it.  It may be useful to run make path/to/cfile.i (for some
cfile.c) to see if the specific code you changed it included in the
configuration or is discarded by the preprocessor.

In any case, you have put () in the argument list of a macro, for reg,
which is not correct.  The parentheses for chip are ok.

> -#define rtsx_readl(chip, reg) \
> +#define rtsx_readl(chip, (reg) \

Here you have broken the argument list of the macro completely, because
the parentheses are not balanced.

>  	ioread32((chip)->rtsx->remap_addr + reg)
> -#define rtsx_writew(chip, reg, value) \
> +#define rtsx_writew(chip, (reg), value) \
>  	iowrite16(value, (chip)->rtsx->remap_addr + reg)
> -#define rtsx_readw(chip, reg) \
> +#define rtsx_readw(chip, (reg)) \
>  	ioread16((chip)->rtsx->remap_addr + reg)
> -#define rtsx_writeb(chip, reg, value) \
> +#define rtsx_writeb(chip, (reg), value) \
>  	iowrite8(value, (chip)->rtsx->remap_addr + reg)
> -#define rtsx_readb(chip, reg) \
> +#define rtsx_readb(chip, (reg)) \
>  	ioread8((chip)->rtsx->remap_addr + reg)
>
>  #define rtsx_read_config_byte(chip, where, val) \
> @@ -131,8 +131,8 @@ static inline struct rtsx_dev *host_to_rtsx(struct Scsi_Host *host)
>   * The scsi_lock() and scsi_unlock() macros protect the sm_state and the
>   * single queue element srb for write access
>   */
> -#define scsi_unlock(host)	spin_unlock_irq(host->host_lock)
> -#define scsi_lock(host)		spin_lock_irq(host->host_lock)
> +#define scsi_unlock(host)	spin_unlock_irq((host)->host_lock)
> +#define scsi_lock(host)		spin_lock_irq((host)->host_lock)

I also wonder if someone has worked on this code already?  There was
a suggestion to just drop these macros and use the standard kernel
functions.  Maybe your kernel tree is not up to date.

julia

>  #define lock_state(chip)	spin_lock_irq(&((chip)->rtsx->reg_lock))
>  #define unlock_state(chip)	spin_unlock_irq(&((chip)->rtsx->reg_lock))
> --
> 2.25.1
>
>
>