Skip to contentSkip to conversation

ATM: misplaced parentheses?

PATCH Applied Roel Kluin 4 participants 5 messages Feb 15, 2009 – Feb 19, 2009

Subsystems (3)
Networking Network drivers ATM
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 ATM: misplaced parentheses?

I think below is what was intended? otherwise we could as well have written:

tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) ||

	(data & i) ? CONFIG1_PROMDATA : 0;

Does this fix the parsing of the EEPROM?

please review. -------------------------->8------------------8<--------------------------- Fix misplaced parentheses

Signed-off-by Roel Kluin <[email protected]>

---

drivers/atm/lanai.c +1 -1
index 144a49f..317fb5a 100644
@@ -901,7 +901,7 @@ static int __devinit eeprom_read(struct lanai_dev *lanai)
 		clock_l(); udelay(5);
 		for (i = 128; i != 0; i >>= 1) {   /* write command out */
 			tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) |
-			    (data & i) ? CONFIG1_PROMDATA : 0;
+			    (data & i ? CONFIG1_PROMDATA : 0);
 			if (lanai->conf1 != tmp) {
 				set_config1(tmp);
 				udelay(5);	/* Let new data settle */
#1 lore raw

that doesnt seem to make sense either. the original code is:

for (i = 128; i != 0; i >>= 1) { /* write command out */
...
tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) |

			    (data & i) ? CONFIG1_PROMDATA : 0;

since i is always positive here, you wouldnt need the ?: if your suggested fix is the original intent. it looks like setting CONFIG1_PROMDATA means '1' and not setting it means '0' when writing the value of data to the register.

Quoted 31 lines · In message <[email protected]>,Roel Kluin writes:

I think below is what was intended? otherwise we could as well have written:

tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) ||

	(data & i) ? CONFIG1_PROMDATA : 0;

Does this fix the parsing of the EEPROM?

please review. -------------------------->8------------------8<--------------------------- Fix misplaced parentheses

Signed-off-by Roel Kluin <[email protected]>

---

drivers/atm/lanai.c +1 -2
index 144a49f..317fb5a 100644
@@ -901,7 +901,7 @@ static int __devinit eeprom_read(struct lanai_dev *lanai)
 		clock_l(); udelay(5);
 		for (i = 128; i != 0; i >>= 1) {   /* write command out */
 			tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) |
-			    (data & i) ? CONFIG1_PROMDATA : 0;
+			    (data & i ? CONFIG1_PROMDATA : 0);
 			if (lanai->conf1 != tmp) {
 				set_config1(tmp);
 				udelay(5);	/* Let new data settle */
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 8 lines · Chas Williams (CONTRACTOR)

that doesnt seem to make sense either. the original code is:

drivers/atm/lanai.c +0 -0
 		for (i = 128; i != 0; i >>= 1) {   /* write command out */
...
 			tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) |

			    (data & i) ? CONFIG1_PROMDATA : 0;
Quoted 4 lines

since i is always positive here, you wouldnt need the ?: if your suggested fix is the original intent. it looks like setting CONFIG1_PROMDATA means '1' and not setting it means '0' when writing the value of data to the register.

What the hell does it have to do with i being positive? || variant is, of course, silly; it's very obvious what's going on here. Garden-variety bit-banging, IOW
tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) |
((data & i) ? CONFIG1_PROMDATA : 0);
and while you technically only need parens around ?:, in this case it's better to keep it fully parenthesised - more readable that way.

The lack of parens around ?: in the current tree is an obvious bug.

author
#3 lore raw
Quoted 4 lines · Al Viro

while you technically only need parens around ?:, in this case it's better to keep it fully parenthesised - more readable that way.

The lack of parens around ?: in the current tree is an obvious bug.

Ok, with extra for clarity:

-------------------------->8------------------8<--------------------------- Add missing parentheses

Signed-off-by Roel Kluin <[email protected]>

---

drivers/atm/lanai.c +1 -1
index 144a49f..8733a2e 100644
@@ -901,7 +901,7 @@ static int __devinit eeprom_read(struct lanai_dev *lanai)
 		clock_l(); udelay(5);
 		for (i = 128; i != 0; i >>= 1) {   /* write command out */
 			tmp = (lanai->conf1 & ~CONFIG1_PROMDATA) |
-			    (data & i) ? CONFIG1_PROMDATA : 0;
+			    ((data & i) ? CONFIG1_PROMDATA : 0);
 			if (lanai->conf1 != tmp) {
 				set_config1(tmp);
 				udelay(5);	/* Let new data settle */