atl1c: Add support for Atheros AR8152 and AR8152

PATCH Ben Hutchings 4 participants 7 messages Oct 11, 2010 – Oct 22, 2010 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Ben Hutchings

author

2010-10-11 01:18 UTC #0 lore raw

Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support for Atheros AR8152 and AR8152" included the following changes:

Quoted 3 lines

drivers/net/atl1c/atl1c_hw.c +0 -0

@@ -70,17 +70,39 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)

[...]

Quoted 15 lines

drivers/net/atl1c/atl1c_hw.c +9 -6

-		/* Enable OTP CLK */
-		if (!(otp_ctrl_data & OTP_CTRL_CLK_EN)) {
-			otp_ctrl_data |= OTP_CTRL_CLK_EN;
-			AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
-			AT_WRITE_FLUSH(hw);
-			msleep(1);
+		if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
+			/* Enable OTP CLK */
+			if (!(otp_ctrl_data & OTP_CTRL_CLK_EN)) {
+				otp_ctrl_data |= OTP_CTRL_CLK_EN;
+				AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
+				AT_WRITE_FLUSH(hw);
+				msleep(1);
+			}
+		}

[...]

Quoted 7 lines

drivers/net/atl1c/atl1c_hw.c +0 -3

@@ -96,11 +118,31 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
 			return -1;
 	}
 	/* Disable OTP_CLK */
-	if (otp_ctrl_data & OTP_CTRL_CLK_EN) {
-		otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
-		AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);

drivers/net/atl1c/atl1c_hw.c static int atl1c_get_permanent_address(struct atl1c_hw *hw) · line 128

Quoted 10 lines

drivers/net/atl1c/atl1c_hw.c +8 -2

-		AT_WRITE_FLUSH(hw);
-		msleep(1);
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
+		if (otp_ctrl_data & OTP_CTRL_CLK_EN) {
+			otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
+			AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
+			AT_WRITE_FLUSH(hw);
+			msleep(1);
+		}
+	}

Shouldn't the first if-statement use the same condition as the second i.e. matching the previously-defined hardware types athr_l1c and athr_l2c?

Ben.

Signature · 2 lines

Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

David Miller

maintainer

2010-10-11 04:02 UTC #1 lore raw

From: Ben Hutchings <[email protected]>
Date: Mon, 11 Oct 2010 02:18:50 +0100

Quoted 2 lines

Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support for Atheros AR8152 and AR8152" included the following changes:

...

Quoted 1 line

Quoted 1 line
(quoted diff) +1 -0
+		if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {

...

Quoted 1 line

Quoted 1 line
(quoted diff) +1 -0
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {

...

Quoted 3 lines

Shouldn't the first if-statement use the same condition as the second i.e. matching the previously-defined hardware types athr_l1c and athr_l2c?

Yeah that definitely looks like a bug to me.

Luis R. Rodriguez

2010-10-11 18:48 UTC #2 lore raw

Quoted 8 lines · David Miller

From: Ben Hutchings <[email protected]>
Date: Mon, 11 Oct 2010 02:18:50 +0100
Quoted 2 lines
Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support for Atheros AR8152 and AR8152" included the following changes:
...
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+		if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
...

Quoted 7 lines

Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
...
Quoted 3 lines
Shouldn't the first if-statement use the same condition as the second i.e. matching the previously-defined hardware types athr_l1c and athr_l2c?
Yeah that definitely looks like a bug to me.

Good catch, unfortunatley I don't have the source code I used to port this work the day I did this anymore locally, so adding Jie Yang who is actually our maintainer for this driver.

Jie, can you please confirm if this patch is correct?

drivers/net/atl1c/atl1c_hw.c +1 -1

index d8501f0..0a7b786 100644
@@ -132,7 +132,7 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
 			return -1;
 	}
 	/* Disable OTP_CLK */
-	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b)) {
 		otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
 		AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
 		msleep(1);

Luis

Stephen Hemminger

2010-10-11 19:01 UTC #3 lore raw

Quoted 29 lines · On Mon, 11 Oct 2010 11:48:35 -0700 "Luis R. Rodriguez"…

Quoted 8 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Mon, 11 Oct 2010 02:18:50 +0100
Quoted 2 lines
Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support for Atheros AR8152 and AR8152" included the following changes:
...
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+		if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
...
Quoted 7 lines
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
...
Quoted 3 lines
Shouldn't the first if-statement use the same condition as the second i.e. matching the previously-defined hardware types athr_l1c and athr_l2c?
Yeah that definitely looks like a bug to me.
Good catch, unfortunatley I don't have the source code I used to port this work the day I did this anymore locally, so adding Jie Yang who is actually our maintainer for this driver.
Jie, can you please confirm if this patch is correct?
drivers/net/atl1c/atl1c_hw.c +0 -0
index d8501f0..0a7b786 100644

drivers/net/atl1c/atl1c_hw.c static int atl1c_get_permanent_address(struct atl1c_hw *hw) · line 138

Quoted 8 lines

drivers/net/atl1c/atl1c_hw.c +1 -1

@@ -132,7 +132,7 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
 			return -1;
 	}
 	/* Disable OTP_CLK */
-	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b)) {
 		otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
 		AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
 		msleep(1);

Did you notice ((gratuitous extra parens))

David Miller

maintainer

2010-10-11 19:02 UTC #4 lore raw

From: Stephen Hemminger <[email protected]>
Date: Mon, 11 Oct 2010 12:01:28 -0700

Quoted 14 lines

drivers/net/atl1c/atl1c_hw.c static int atl1c_get_permanent_address(struct atl1c_hw *hw) · line 138
Quoted 10 lines · On Mon, 11 Oct 2010 11:48:35 -0700 "Luis R. Rodriguez"…
drivers/net/atl1c/atl1c_hw.c +1 -1
@@ -132,7 +132,7 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
 			return -1;
 	}
 	/* Disable OTP_CLK */
-	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b)) {
 		otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
 		AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
 		msleep(1);
Did you notice ((gratuitous extra parens))

Yeah let's kill that too while we're changing this.

Ben Hutchings

author

2010-10-11 22:28 UTC #5 lore raw

In reply to Luis R. Rodriguez

Quoted 15 lines · Luis R. Rodriguez

Quoted 14 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Mon, 11 Oct 2010 02:18:50 +0100
Quoted 2 lines
Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support for Atheros AR8152 and AR8152" included the following changes:
...
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+		if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
...
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
...
Quoted 3 lines
Shouldn't the first if-statement use the same condition as the second i.e. matching the previously-defined hardware types athr_l1c and athr_l2c?

Quoted 7 lines

Quoted 1 line
Yeah that definitely looks like a bug to me.
Good catch, unfortunatley I don't have the source code I used to port this work the day I did this anymore locally, so adding Jie Yang who is actually our maintainer for this driver.
Jie, can you please confirm if this patch is correct?

I was suggesting that the first condition was wrong and the second was right.

Ben.

Quoted 16 lines

drivers/net/atl1c/atl1c_hw.c +1 -1

index d8501f0..0a7b786 100644
@@ -132,7 +132,7 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
 			return -1;
 	}
 	/* Disable OTP_CLK */
-	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b)) {
 		otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
 		AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
 		msleep(1);

Luis

Signature · 2 lines

Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Luis R. Rodriguez

2010-10-22 20:37 UTC #7 lore raw

In reply to a message not in the archive

Quoted 23 lines · Luis Rodriguez

Quoted 22 lines · Ben Hutchings
Quoted 21 lines · Luis R. Rodriguez
Quoted 8 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Mon, 11 Oct 2010 02:18:50 +0100
Quoted 2 lines
Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support for Atheros AR8152 and AR8152" included the following changes:
...
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+		if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
...
Quoted 7 lines
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+	if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
...
Quoted 3 lines
Shouldn't the first if-statement use the same condition as the second i.e. matching the previously-defined hardware types athr_l1c and athr_l2c?
Yeah that definitely looks like a bug to me.
Good catch, unfortunatley I don't have the source code I used to port this work the day I did this anymore locally, so adding Jie Yang who is actually our maintainer for this driver.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Jie, can you please confirm if this patch is correct?
I was suggesting that the first condition was wrong and the second was right.
Heh, thanks, Jie can you review?

Jie, have you had any chance to review?

Luis