diff mbox

[dpdk-dev,3/8] drivers/net/e1000: Fix missing brackets

Message ID 1456426121-21423-4-git-send-email-aconole@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Aaron Conole Feb. 25, 2016, 6:48 p.m. UTC 
  The register read/write mphy functions have misleading whitespace around
the locked check. This cleanup merely preserves the existing functionality
while improving the ready check.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Wenzhuo Lu Feb. 26, 2016, 1:02 a.m. UTC | #1 
Hi Aaron,


> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo; Zhang, Helin; Ananyev, Konstantin; Richardson, Bruce
> Subject: [PATCH 3/8] drivers/net/e1000: Fix missing brackets
> 
> The register read/write mphy functions have misleading whitespace around the
> locked check. This cleanup merely preserves the existing functionality while
> improving the ready check.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/e1000/base/e1000_phy.c
> b/drivers/net/e1000/base/e1000_phy.c
> index d43b7ce..8642d38 100644
> --- a/drivers/net/e1000/base/e1000_phy.c
> +++ b/drivers/net/e1000/base/e1000_phy.c
> @@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 *data)
>  	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
>  	return E1000_SUCCESS;
>  }
> 
> @@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 data,
>  	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
>  	return E1000_SUCCESS;
>  }
> 
> --
> 2.5.0
Normally we will not maintain the base code. It's just taken from kernel driver.
Agree with you that the whitespace is misleading. But as it's no real impact. I'd like to say not a big deal, better not  change it. :)
Zhang, Helin March 18, 2016, 1:03 a.m. UTC | #2 
> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 3/8] drivers/net/e1000: Fix missing brackets
> 
> The register read/write mphy functions have misleading whitespace around the
> locked check. This cleanup merely preserves the existing functionality while
> improving the ready check.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/e1000/base/e1000_phy.c
> b/drivers/net/e1000/base/e1000_phy.c
> index d43b7ce..8642d38 100644
> --- a/drivers/net/e1000/base/e1000_phy.c
> +++ b/drivers/net/e1000/base/e1000_phy.c
> @@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 *data)
>  	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
I think the above line should be included in brackets above.
Though we should avoid modifying any code in base driver, I'd
suggest you to fix the issue here. Thank you!

>  	return E1000_SUCCESS;
>  }
> 
> @@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 data,
>  	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
>  	return E1000_SUCCESS;
The same comments as above. Thanks!

>  }
> 
> --
> 2.5.0
diff mbox

Patch

diff --git a/drivers/net/e1000/base/e1000_phy.c b/drivers/net/e1000/base/e1000_phy.c
index d43b7ce..8642d38 100644
--- a/drivers/net/e1000/base/e1000_phy.c
+++ b/drivers/net/e1000/base/e1000_phy.c
@@ -4153,13 +4153,13 @@  s32 e1000_read_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 *data)
 	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+	}
 
+	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
 	return E1000_SUCCESS;
 }
 
@@ -4218,13 +4218,13 @@  s32 e1000_write_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 data,
 	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+	}
 
+	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
 	return E1000_SUCCESS;
 }