Messages in this thread |  | | Date | Mon, 18 Jul 2022 21:33:08 +0200 | Subject | Re: [PATCH 1/7] staging: vt6655: Rename dwData to reg_value in four macros | From | 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.
|  |