Message ID | 49983DB0.6040008@gmail.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
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. In message <49983DB0.6040008@gmail.com>,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 <roel.kluin@gmail.com> >--- >diff --git a/drivers/atm/lanai.c b/drivers/atm/lanai.c >index 144a49f..317fb5a 100644 >--- a/drivers/atm/lanai.c >+++ b/drivers/atm/lanai.c >@@ -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 majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 16, 2009 at 05:02:36PM -0500, Chas Williams (CONTRACTOR) wrote: > 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. 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/atm/lanai.c b/drivers/atm/lanai.c index 144a49f..317fb5a 100644 --- a/drivers/atm/lanai.c +++ b/drivers/atm/lanai.c @@ -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 */
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 <roel.kluin@gmail.com> --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html