Re: [PATCH 1/7] staging: vt6655: Rename dwData to reg_value in four macros

[Philipp Hortmann]

On 7/18/22 08:07, Joe Perches wrote:
> Please remember that checkpatch is a stupid little scripted tool
> and the actual goal is to have readable code.
Understood.
> Look a bit beyond the code and see if and how you could make the
> code better.
>
> All of these macros have the same form and logic.
That is the reason why I put them all together in one static function:
static void vt6655_mac_dma_ctl(void __iomem *iobase, u8 reg_index)
{
	unsigned long reg_value;

	reg_value = ioread32(iobase + reg_index);
	if (reg_value & DMACTL_RUN)
		iowrite32(DMACTL_WAKE, iobase + reg_index);
	else
		iowrite32(DMACTL_RUN, iobase + reg_index);
}

> Perhaps it'd be better to use another indirect macro and define
> all of these with that new macro.
>
> Something like:
>
> #define mac_v(iobase, reg)						\
> do {									\
> 	void __iomem *addr = (iobase) + (reg);				\
> 	iowrite32(ioread32(addr) & DMACTL_RUN ? DMACTL_WAKE : DMACTL_RUN,\
> 		  addr);						\
> } while (0)
>
> #define MACvReceive0(iobase)	mac_v(iobase, MAC_REG_RXDMACTL0)
> #define MACvReceive1(iobase)	mac_v(iobase, MAC_REG_RXDMACTL1)
> #define MACvTransmit0(iobase)	mac_v(iobase, MAC_REG_TXDMACTL0)
> #define MACvTransmitAC0(iobase)	mac_v(iobase, MAC_REG_AC0DMACTL)

That is an interesting solution. But for me this code is not as good 
readable as my proposal. Reason is that I struggle with the function in 
function with condition broken into two lines.