All of lore.kernel.org
 help / color / mirror / Atom feed
From: Corentin Labbe <clabbe.montjoie@gmail.com>
To: Conley Lee <conleylee@foxmail.com>
Cc: davem@davemloft.net, kuba@kernel.org, mripard@kernel.org,
	wens@csie.org, 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
Date: Mon, 10 Jan 2022 14:56:35 +0100	[thread overview]
Message-ID: <Ydw7EzPvwArW/siQ@Red> (raw)
In-Reply-To: <tencent_AEEE0573A5455BBE4D5C05226C6C1E3AEF08@qq.com>

Le Mon, Jan 10, 2022 at 07:35:49PM +0800, Conley Lee a écrit :
> This patch remove magic numbers in sun4i-emac.c and replace with macros
> defined in sun4i-emac.h
> 
> Change since v1
> ---------------
> - reformat
> - merge commits
> - add commit message
> 
> Signed-off-by: Conley Lee <conleylee@foxmail.com>
> ---
>  drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
>  drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
>  2 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> index 849de4564709..98fd98feb439 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> @@ -106,9 +106,9 @@ static void emac_update_speed(struct net_device *dev)
>  
>  	/* set EMAC SPEED, depend on PHY  */
>  	reg_val = readl(db->membase + EMAC_MAC_SUPP_REG);
> -	reg_val &= ~(0x1 << 8);
> +	reg_val &= ~EMAC_MAC_SUPP_100M;
>  	if (db->speed == SPEED_100)
> -		reg_val |= 1 << 8;
> +		reg_val |= EMAC_MAC_SUPP_100M;
>  	writel(reg_val, db->membase + EMAC_MAC_SUPP_REG);
>  }
>  
> @@ -264,7 +264,7 @@ static void emac_dma_done_callback(void *arg)
>  
>  	/* re enable interrupt */
>  	reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> -	reg_val |= (0x01 << 8);
> +	reg_val |= EMAC_INT_CTL_RX_EN;
>  	writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  	db->emacrx_completed_flag = 1;
> @@ -429,7 +429,7 @@ static unsigned int emac_powerup(struct net_device *ndev)
>  	/* initial EMAC */
>  	/* flush RX FIFO */
>  	reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> -	reg_val |= 0x8;
> +	reg_val |= EMAC_RX_CTL_FLUSH_FIFO;
>  	writel(reg_val, db->membase + EMAC_RX_CTL_REG);
>  	udelay(1);
>  
> @@ -441,8 +441,8 @@ static unsigned int emac_powerup(struct net_device *ndev)
>  
>  	/* set MII clock */
>  	reg_val = readl(db->membase + EMAC_MAC_MCFG_REG);
> -	reg_val &= (~(0xf << 2));
> -	reg_val |= (0xD << 2);
> +	reg_val &= ~EMAC_MAC_MCFG_MII_CLKD_MASK;
> +	reg_val |= EMAC_MAC_MCFG_MII_CLKD_72;
>  	writel(reg_val, db->membase + EMAC_MAC_MCFG_REG);
>  
>  	/* clear RX counter */
> @@ -506,7 +506,7 @@ static void emac_init_device(struct net_device *dev)
>  
>  	/* enable RX/TX0/RX Hlevel interrup */
>  	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);
>  	writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  	spin_unlock_irqrestore(&db->lock, flags);
> @@ -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);
>  			writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  			/* had one stuck? */
> @@ -669,7 +671,9 @@ static void emac_rx(struct net_device *dev)
>  			writel(reg_val | EMAC_CTL_RX_EN,
>  			       db->membase + EMAC_CTL_REG);
>  			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);
>  			writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  			db->emacrx_completed_flag = 1;
> @@ -783,20 +787,20 @@ static irqreturn_t emac_interrupt(int irq, void *dev_id)
>  	}
>  
>  	/* Transmit Interrupt check */
> -	if (int_status & (0x01 | 0x02))
> +	if (int_status & EMAC_INT_STA_TX_COMPLETE)
>  		emac_tx_done(dev, db, int_status);
>  
> -	if (int_status & (0x04 | 0x08))
> +	if (int_status & EMAC_INT_STA_TX_ABRT)
>  		netdev_info(dev, " ab : %x\n", int_status);
>  
>  	/* Re-enable interrupt mask */
>  	if (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);
>  		writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  	} else {
>  		reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> -		reg_val |= (0xf << 0);
> +		reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN);
>  		writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  	}
>  
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.h b/drivers/net/ethernet/allwinner/sun4i-emac.h
> index 38c72d9ec600..90bd9ad77607 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.h
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.h
> @@ -38,6 +38,7 @@
>  #define EMAC_RX_CTL_REG		(0x3c)
>  #define EMAC_RX_CTL_AUTO_DRQ_EN		(1 << 1)
>  #define EMAC_RX_CTL_DMA_EN		(1 << 2)
> +#define EMAC_RX_CTL_FLUSH_FIFO		(1 << 3)
>  #define EMAC_RX_CTL_PASS_ALL_EN		(1 << 4)
>  #define EMAC_RX_CTL_PASS_CTL_EN		(1 << 5)
>  #define EMAC_RX_CTL_PASS_CRC_ERR_EN	(1 << 6)
> @@ -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)

Hello

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)

Regards

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Corentin Labbe <clabbe.montjoie@gmail.com>
To: Conley Lee <conleylee@foxmail.com>
Cc: davem@davemloft.net, kuba@kernel.org, mripard@kernel.org,
	wens@csie.org, 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
Date: Mon, 10 Jan 2022 14:56:35 +0100	[thread overview]
Message-ID: <Ydw7EzPvwArW/siQ@Red> (raw)
In-Reply-To: <tencent_AEEE0573A5455BBE4D5C05226C6C1E3AEF08@qq.com>

Le Mon, Jan 10, 2022 at 07:35:49PM +0800, Conley Lee a écrit :
> This patch remove magic numbers in sun4i-emac.c and replace with macros
> defined in sun4i-emac.h
> 
> Change since v1
> ---------------
> - reformat
> - merge commits
> - add commit message
> 
> Signed-off-by: Conley Lee <conleylee@foxmail.com>
> ---
>  drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
>  drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
>  2 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> index 849de4564709..98fd98feb439 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> @@ -106,9 +106,9 @@ static void emac_update_speed(struct net_device *dev)
>  
>  	/* set EMAC SPEED, depend on PHY  */
>  	reg_val = readl(db->membase + EMAC_MAC_SUPP_REG);
> -	reg_val &= ~(0x1 << 8);
> +	reg_val &= ~EMAC_MAC_SUPP_100M;
>  	if (db->speed == SPEED_100)
> -		reg_val |= 1 << 8;
> +		reg_val |= EMAC_MAC_SUPP_100M;
>  	writel(reg_val, db->membase + EMAC_MAC_SUPP_REG);
>  }
>  
> @@ -264,7 +264,7 @@ static void emac_dma_done_callback(void *arg)
>  
>  	/* re enable interrupt */
>  	reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> -	reg_val |= (0x01 << 8);
> +	reg_val |= EMAC_INT_CTL_RX_EN;
>  	writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  	db->emacrx_completed_flag = 1;
> @@ -429,7 +429,7 @@ static unsigned int emac_powerup(struct net_device *ndev)
>  	/* initial EMAC */
>  	/* flush RX FIFO */
>  	reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> -	reg_val |= 0x8;
> +	reg_val |= EMAC_RX_CTL_FLUSH_FIFO;
>  	writel(reg_val, db->membase + EMAC_RX_CTL_REG);
>  	udelay(1);
>  
> @@ -441,8 +441,8 @@ static unsigned int emac_powerup(struct net_device *ndev)
>  
>  	/* set MII clock */
>  	reg_val = readl(db->membase + EMAC_MAC_MCFG_REG);
> -	reg_val &= (~(0xf << 2));
> -	reg_val |= (0xD << 2);
> +	reg_val &= ~EMAC_MAC_MCFG_MII_CLKD_MASK;
> +	reg_val |= EMAC_MAC_MCFG_MII_CLKD_72;
>  	writel(reg_val, db->membase + EMAC_MAC_MCFG_REG);
>  
>  	/* clear RX counter */
> @@ -506,7 +506,7 @@ static void emac_init_device(struct net_device *dev)
>  
>  	/* enable RX/TX0/RX Hlevel interrup */
>  	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);
>  	writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  	spin_unlock_irqrestore(&db->lock, flags);
> @@ -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);
>  			writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  			/* had one stuck? */
> @@ -669,7 +671,9 @@ static void emac_rx(struct net_device *dev)
>  			writel(reg_val | EMAC_CTL_RX_EN,
>  			       db->membase + EMAC_CTL_REG);
>  			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);
>  			writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  
>  			db->emacrx_completed_flag = 1;
> @@ -783,20 +787,20 @@ static irqreturn_t emac_interrupt(int irq, void *dev_id)
>  	}
>  
>  	/* Transmit Interrupt check */
> -	if (int_status & (0x01 | 0x02))
> +	if (int_status & EMAC_INT_STA_TX_COMPLETE)
>  		emac_tx_done(dev, db, int_status);
>  
> -	if (int_status & (0x04 | 0x08))
> +	if (int_status & EMAC_INT_STA_TX_ABRT)
>  		netdev_info(dev, " ab : %x\n", int_status);
>  
>  	/* Re-enable interrupt mask */
>  	if (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);
>  		writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  	} else {
>  		reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> -		reg_val |= (0xf << 0);
> +		reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN);
>  		writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>  	}
>  
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.h b/drivers/net/ethernet/allwinner/sun4i-emac.h
> index 38c72d9ec600..90bd9ad77607 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.h
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.h
> @@ -38,6 +38,7 @@
>  #define EMAC_RX_CTL_REG		(0x3c)
>  #define EMAC_RX_CTL_AUTO_DRQ_EN		(1 << 1)
>  #define EMAC_RX_CTL_DMA_EN		(1 << 2)
> +#define EMAC_RX_CTL_FLUSH_FIFO		(1 << 3)
>  #define EMAC_RX_CTL_PASS_ALL_EN		(1 << 4)
>  #define EMAC_RX_CTL_PASS_CTL_EN		(1 << 5)
>  #define EMAC_RX_CTL_PASS_CRC_ERR_EN	(1 << 6)
> @@ -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)

Hello

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)

Regards

  parent reply	other threads:[~2022-01-10 13:59 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-10  7:23 [PATCH 0/2] sun4i-emac: replace magic number with marcos conleylee
2022-01-10  7:23 ` conleylee
2022-01-10  8:32 ` Corentin Labbe
2022-01-10  8:32   ` Corentin Labbe
2022-01-10 11:35 ` [PATCH v2] net: ethernet: sun4i-emac: replace magic number with macro Conley Lee
2022-01-10 11:35   ` Conley Lee
2022-01-10 13:05   ` Leon Romanovsky
2022-01-10 13:05     ` Leon Romanovsky
2022-01-10 14:42     ` Conley Lee
2022-01-10 14:42       ` Conley Lee
2022-01-10 13:31   ` Andrew Lunn
2022-01-10 13:31     ` Andrew Lunn
2022-01-10 14:40     ` Conley Lee
2022-01-10 14:40       ` Conley Lee
2022-01-10 13:56   ` Corentin Labbe [this message]
2022-01-10 13:56     ` Corentin Labbe
2022-01-10 17:28     ` Jakub Kicinski
2022-01-10 17:28       ` Jakub Kicinski
2022-01-11  3:19     ` Conley Lee
2022-01-11  3:19       ` Conley Lee
2022-01-11  3:05 ` [PATCH v3] " Conley Lee
2022-01-11  3:05   ` Conley Lee
2022-01-11 16:32   ` Corentin Labbe
2022-01-11 16:32     ` Corentin Labbe
2022-01-12  6:16     ` Conley Lee
2022-01-12  6:16       ` Conley Lee
2022-01-12  4:40   ` patchwork-bot+netdevbpf
2022-01-12  4:40     ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Ydw7EzPvwArW/siQ@Red \
    --to=clabbe.montjoie@gmail.com \
    --cc=conleylee@foxmail.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mripard@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.