Re: [PATCH v2] net: ethernet: sun4i-emac: replace magic number with macro

From: Jakub Kicinski
Date: Mon Jan 10 2022 - 12:28:43 EST


On Mon, 10 Jan 2022 14:56:35 +0100 Corentin Labbe wrote:
> > @@ -61,7 +62,21 @@
> > #define EMAC_RX_IO_DATA_STATUS_OK (1 << 7)
> > #define EMAC_RX_FBC_REG (0x50)
> > #define EMAC_INT_CTL_REG (0x54)
> > +#define EMAC_INT_CTL_RX_EN (1 << 8)
> > +#define EMAC_INT_CTL_TX0_EN (1)
> > +#define EMAC_INT_CTL_TX1_EN (1 << 1)
> > +#define EMAC_INT_CTL_TX_EN (EMAC_INT_CTL_TX0_EN | EMAC_INT_CTL_TX1_EN)
> > +#define EMAC_INT_CTL_TX0_ABRT_EN (0x1 << 2)
> > +#define EMAC_INT_CTL_TX1_ABRT_EN (0x1 << 3)
> > +#define EMAC_INT_CTL_TX_ABRT_EN (EMAC_INT_CTL_TX0_ABRT_EN | EMAC_INT_CTL_TX1_ABRT_EN)
> > #define EMAC_INT_STA_REG (0x58)
> > +#define EMAC_INT_STA_TX0_COMPLETE (0x1)
> > +#define EMAC_INT_STA_TX1_COMPLETE (0x1 << 1)
> > +#define EMAC_INT_STA_TX_COMPLETE (EMAC_INT_STA_TX0_COMPLETE | EMAC_INT_STA_TX1_COMPLETE)
> > +#define EMAC_INT_STA_TX0_ABRT (0x1 << 2)
> > +#define EMAC_INT_STA_TX1_ABRT (0x1 << 3)
> > +#define EMAC_INT_STA_TX_ABRT (EMAC_INT_STA_TX0_ABRT | EMAC_INT_STA_TX1_ABRT)
> > +#define EMAC_INT_STA_RX_COMPLETE (0x1 << 8)
>
> As proposed by checkpatch, I thing there are several place (like all
> EMAC_INT_STA) where you could use BIT(x) instead of (0xX << x)

That's not a hard requirement, if the driver already uses the shift you
can leave your code as is, some upstream developers actually prefer to
avoid the BIT() macro.