Messages in this thread | | | Date | Mon, 10 Jan 2022 22:40:38 +0800 | From | Conley Lee <> | Subject | Re: [PATCH v2] net: ethernet: sun4i-emac: replace magic number with macro |
| |
On 01/10/22 at 02:31下午, Andrew Lunn wrote: > Date: Mon, 10 Jan 2022 14:31:28 +0100 > From: Andrew Lunn <andrew@lunn.ch> > To: Conley Lee <conleylee@foxmail.com> > Cc: davem@davemloft.net, kuba@kernel.org, mripard@kernel.org, > wens@csie.org, clabbe.montjoie@gmail.com, netdev@vger.kernel.org, > linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] net: ethernet: sun4i-emac: replace magic number > with macro > > > @@ -637,7 +637,9 @@ static void emac_rx(struct net_device *dev) > > if (!rxcount) { > > db->emacrx_completed_flag = 1; > > reg_val = readl(db->membase + EMAC_INT_CTL_REG); > > - reg_val |= (0xf << 0) | (0x01 << 8); > > + reg_val |= > > + (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | > > + EMAC_INT_CTL_RX_EN); > > Putting the first value on the next line is a bit odd. This would be > preferred: > > + reg_val |= (EMAC_INT_CTL_TX_EN | > + EMAC_INT_CTL_TX_ABRT_EN | > + EMAC_INT_CTL_RX_EN); > > I also have to wonder why two | have become three? (0x01 << 8) is > clearly a single value. (0xf << 0) should either be a single macro, or > 4 macros since 0xf is four bits. Without looking into the details, i > cannot say this is wrong, but it does look strange. > > Andrew > Thanks for your suggestion. The (0xf << 0) mask enable tx finish and tx abort interrupts at hardware level. And the reason this mask has 4 bits is that sun4i emac has 2 tx channels. I reduce it into two macros EMAC_INT_CTL_TX_EN and EMAC_INT_CTL_TX_ABRT_EN, this may be more readable, since we always enable both tx channels in the driver.
| |