Re: [PATCH 5/7] staging: vt6655: Replace MACvReceive1 with function vt6655_mac_dma_ctl

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Jul 18, 2022 at 10:00:59PM +0200, Philipp Hortmann wrote:
> > > diff --git a/drivers/staging/vt6655/mac.h b/drivers/staging/vt6655/mac.h
> > > index 5747de436911..129a6602f6f0 100644
> > > --- a/drivers/staging/vt6655/mac.h
> > > +++ b/drivers/staging/vt6655/mac.h
> > > @@ -537,16 +537,6 @@
> > >   /*---------------------  Export Macros ------------------------------*/
> > > -#define MACvReceive1(iobase)						\
> > > -do {									\
> > > -	unsigned long reg_value;					\
> > > -	reg_value = ioread32(iobase + MAC_REG_RXDMACTL1);		\
> > > -	if (reg_value & DMACTL_RUN)					\
> > > -		iowrite32(DMACTL_WAKE, iobase + MAC_REG_RXDMACTL1);	\
> > > -	else								\
> > > -		iowrite32(DMACTL_RUN, iobase + MAC_REG_RXDMACTL1);	\
> > > -} while (0)
> > > -
> > >   #define MACvTransmit0(iobase)						\
> > >   do {									\
> > >   	unsigned long reg_value;					\
> > 
> 
> I was asking in kernelnewbies what to do with multi line macros as
> checkpatch.pl warnings cannot be totally avoided.
> 
> Greg replied to make functions out of them.
> 
> Please find the full email under:
> 
> https://www.mail-archive.com/kernelnewbies@kernelnewbies.org/msg22042.html
> 
> In this case I really like the static function solution much more than the
> macros.

Yes.  We all like the static function.  It's the commit message, I'm not
so keen on.

You could have avoided the checkpatch warning with an assignment at the
start of the macro:

	typeof(iobase) base = (iobase);

#define MACvReceive1(iobase)						\
do {									\
	typeof(iobase) base = (iobase);					\
	unsigned long reg_value = ioread32(base + MAC_REG_RXDMACTL1);	\
	if (reg_value & DMACTL_RUN)					\
		iowrite32(DMACTL_WAKE, base + MAC_REG_RXDMACTL1);	\
	else								\
		iowrite32(DMACTL_RUN, base + MAC_REG_RXDMACTL1);	\
} while (0)

It's not a *good* solution, but it works.

The means the "iobase" argument would only be executed one time.
Imagine if someone passed "iobase++" to the original function.  It
would have incremented twice instead of once as expected.  That's what
the checkpatch warning is saying.  Nothing to do with multiple lines.

regards,
dan carpenter