Re: [PATCH net-next v2] net: smc91x: Refactor SMC_* macros

[Simon Horman]

On Fri, May 31, 2024 at 02:01:04PM +0200, Thorsten Blum wrote:
> Use the macro parameter lp directly instead of relying on ioaddr being
> defined in the surrounding scope.
> 
> The macros SMC_CURRENT_BANK(), SMC_SELECT_BANK(), SMC_GET_BASE(), and
> SMC_GET_REV() take an additional parameter ioaddr to use a different
> address if necessary (e.g., as in smc_probe()).
> 
> Relying on implicitly defined variable names in C macros is generally
> considered bad practice and can be avoided by using explicit parameters.
> 
> Compile-tested only.
> 
> Suggested-by: Andrew Lunn <andrew@xxxxxxx>
> Signed-off-by: Thorsten Blum <thorsten.blum@xxxxxxxxxx>
> ---
> Changes in v2:
> - Add macro parameter ioaddr where necessary to fix smc_probe() after
>   feedback from Jakub Kicinski
> - Update patch description
> - Link to v1: https://lore.kernel.org/linux-kernel/20240528104421.399885-3-thorsten.blum@xxxxxxxxxx/
> ---
>  drivers/net/ethernet/smsc/smc91x.c | 132 +++++++++++--------------
>  drivers/net/ethernet/smsc/smc91x.h | 152 ++++++++++++++---------------
>  2 files changed, 131 insertions(+), 153 deletions(-)

Hi Thorsten,

This is a large and repetitive patch, which makes it hard to spot any errors
(I couldn't see any :)

I'm not sure if it is worth doing at this point,
but it might have been worth splitting the patch up,
f.e. by addressing one MACRO at a time, removing
local ioaddr variables as they become unused.

As highlighted bellow, checkpatch flags a lot of issues with
the code updated by this patch. Perhaps they could be addressed
as the lines with issues are updated. Or by patch(es) before
those that make the changes made by this patch. Or some combination
of the two.

...

> diff --git a/drivers/net/ethernet/smsc/smc91x.h b/drivers/net/ethernet/smsc/smc91x.h

...

> @@ -839,64 +839,64 @@ static const char * chip_ids[ 16 ] =  {
>  #define SMC_MUST_ALIGN_WRITE(lp)	SMC_32BIT(lp)
>  
>  #define SMC_GET_PN(lp)						\
> -	(SMC_8BIT(lp)	? (SMC_inb(ioaddr, PN_REG(lp)))	\
> -				: (SMC_inw(ioaddr, PN_REG(lp)) & 0xFF))
> +	(SMC_8BIT(lp)	? (SMC_inb(lp->base, PN_REG(lp)))	\
> +				: (SMC_inw(lp->base, PN_REG(lp)) & 0xFF))
>  
>  #define SMC_SET_PN(lp, x)						\
>  	do {								\
>  		if (SMC_MUST_ALIGN_WRITE(lp))				\
> -			SMC_outl((x)<<16, ioaddr, SMC_REG(lp, 0, 2));	\
> +			SMC_outl((x)<<16, lp->base, SMC_REG(lp, 0, 2));	\

While we are here, I think we can address the whitespace issues
flagged by checkpatch - there should be spaces around '<<'.

Likewise elsewhere.

...

> -#define SMC_CURRENT_BANK(lp)	SMC_inw(ioaddr, BANK_SELECT)
> +#define SMC_CURRENT_BANK(lp, ioaddr)	SMC_inw(ioaddr, BANK_SELECT)

lp is not used in this macro, can it be removed?
Possibly as a follow-up?

Flagged by checkpatch.

>  
> -#define SMC_SELECT_BANK(lp, x)					\
> +#define SMC_SELECT_BANK(lp, x, ioaddr)					\
>  	do {								\
>  		if (SMC_MUST_ALIGN_WRITE(lp))				\
>  			SMC_outl((x)<<16, ioaddr, 12<<SMC_IO_SHIFT);	\
> @@ -904,125 +904,125 @@ static const char * chip_ids[ 16 ] =  {
>  			SMC_outw(lp, x, ioaddr, BANK_SELECT);		\
>  	} while (0)
>  
> -#define SMC_GET_BASE(lp)		SMC_inw(ioaddr, BASE_REG(lp))
> +#define SMC_GET_BASE(lp, ioaddr)	SMC_inw(ioaddr, BASE_REG(lp))
>  
> -#define SMC_SET_BASE(lp, x)	SMC_outw(lp, x, ioaddr, BASE_REG(lp))
> +#define SMC_SET_BASE(lp, x)	SMC_outw(lp, x, lp->base, BASE_REG(lp))

lp is now evaluated twice in this macro.
As the motivation of this patch seems to be about good practice,
perhaps we should avoid introducing a bad one?

Likewise in several other macros.

Flagged by checkpatch.

...

> -#define SMC_GET_CTL(lp)		SMC_inw(ioaddr, CTL_REG(lp))
> +#define SMC_GET_CTL(lp)		SMC_inw(lp->base, CTL_REG(lp))

I don't think this is introduced by this patch,
but perhaps it could be addressed.

Checkpatch says: Macro argument 'lp' may be better as '(lp)' to avoid precedence issues

Likewise elsewhere.

...