lkml.org 
[lkml]   [2022]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/7] staging: vt6655: Rename dwData to reg_value in four macros
From
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.

\
 
 \ /
  Last update: 2022-07-18 21:35    [W:0.049 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site