board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

PATCH Superseded Hannes Petermaier 4 participants 7 messages Mar 7, 2014 – Mar 12, 2014

lore

Share & tools

lore

Hannes Petermaier

author

2014-03-07 17:56 UTC #0 lore raw

PATCH 1/1 board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong

Signed-off-by Hannes Petermaier <[email protected]>

---
 board/BuR/kwb/board.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

board/BuR/kwb/board.c +2 -2

index 8aa16bc..8fb5e68 100644
@@ -120,7 +120,7 @@ void am33xx_spl_board_init(void)
 
 	/* power-ON  3V3 via Resetcontroller */
 	oldspeed = i2c_get_bus_speed();
-	if (0 != i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
+	if (0 <= i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
 		buf = RSTCTRL_FORCE_PWR_NEN;
 		i2c_write(RSTCTRL_ADDR, RSTCTRL_CTRLREG, 1,
 			  (uint8_t *)&buf, sizeof(buf));

@@ -221,7 +221,7 @@ int board_late_init(void)
 			   TPS65217_WLEDCTRL1, 0x09, 0xFF);
 	/* write bootinfo into scratchregister of resetcontroller */
 	oldspeed = i2c_get_bus_speed();
-	if (0 != i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
+	if (0 <= i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
 		i2c_write(RSTCTRL_ADDR, RSTCTRL_SCRATCHREG, 1,
 			  (uint8_t *)&buf, sizeof(buf));
 		i2c_set_bus_speed(oldspeed);

Signature · 1 line

1.7.10.4

Gerhard Sittig

2014-03-08 17:38 UTC #1 lore raw

PATCH 1/1 board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

Quoted 13 lines · Hannes Petermaier

- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
Signed-off-by Hannes Petermaier <[email protected]>
---
 board/BuR/kwb/board.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
board/BuR/kwb/board.c +0 -0
index 8aa16bc..8fb5e68 100644

board/BuR/kwb/board.c void am33xx_spl_board_init(void) · line 126

Quoted 8 lines

board/BuR/kwb/board.c +1 -1

@@ -120,7 +120,7 @@ void am33xx_spl_board_init(void)
 
 	/* power-ON  3V3 via Resetcontroller */
 	oldspeed = i2c_get_bus_speed();
-	if (0 != i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
+	if (0 <= i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
 		buf = RSTCTRL_FORCE_PWR_NEN;
 		i2c_write(RSTCTRL_ADDR, RSTCTRL_CTRLREG, 1,
 			  (uint8_t *)&buf, sizeof(buf));

While you are at it, can you fixup this Yoda programming style and use the regular idiom instead? It hurts the brain to have to stop and read code "backwards" before seeing what's going on.

virtually yours
Gerhard Sittig

Signature · 3 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

Hannes Petermaier

2014-03-08 18:14 UTC #2 lore raw

PATCH 1/1 board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

Hi Gerhard,

Quoted 14 lines · Gerhard Sittig

Quoted 13 lines · Hannes Petermaier
- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
Signed-off-by Hannes Petermaier <[email protected]>
---
  board/BuR/kwb/board.c |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
board/BuR/kwb/board.c +0 -0
index 8aa16bc..8fb5e68 100644
@@ -120,7 +120,7 @@ void am33xx_spl_board_init(void)
 

Quoted 10 lines

board/BuR/kwb/board.c void am33xx_spl_board_init(void) · line 126
Quoted 7 lines
board/BuR/kwb/board.c +1 -1
  	/* power-ON  3V3 via Resetcontroller */
  	oldspeed = i2c_get_bus_speed();
-	if (0 != i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
+	if (0 <= i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
  		buf = RSTCTRL_FORCE_PWR_NEN;
  		i2c_write(RSTCTRL_ADDR, RSTCTRL_CTRLREG, 1,
  			  (uint8_t *)&buf, sizeof(buf));
While you are at it, can you fixup this Yoda programming style and use the regular idiom instead? It hurts the brain to have to stop and read code "backwards" before seeing what's going on.

OK. I'll send v2. Maybe i can change my personal idiom.
I've learned to do this so to avoid a unintended assignment to a variable. For exmaple:
if (var = 10)
compiler will do assignment to variable.
if (10 = var)
compiler will generate error.

But in these days "checkpatch.pl" will warn in such case.

Quoted 2 lines

virtually yours
Gerhard Sittig

best regards,
Hannes

Gerhard Sittig

2014-03-08 19:45 UTC #3 lore raw

PATCH 1/1 board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

Quoted 27 lines · Hannes Petermaier

Hi Gerhard,
Quoted 8 lines · Gerhard Sittig
Quoted 7 lines · Hannes Petermaier
- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
Signed-off-by Hannes Petermaier <[email protected]>
---
 board/BuR/kwb/board.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 15 lines
board/BuR/kwb/board.c void am33xx_spl_board_init(void) · line 125
Quoted 12 lines
board/BuR/kwb/board.c +1 -1
index 8aa16bc..8fb5e68 100644
@@ -120,7 +120,7 @@ void am33xx_spl_board_init(void)
 	/* power-ON  3V3 via Resetcontroller */
 	oldspeed = i2c_get_bus_speed();
-	if (0 != i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
+	if (0 <= i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
 		buf = RSTCTRL_FORCE_PWR_NEN;
 		i2c_write(RSTCTRL_ADDR, RSTCTRL_CTRLREG, 1,
 			  (uint8_t *)&buf, sizeof(buf));
While you are at it, can you fixup this Yoda programming style and use the regular idiom instead? It hurts the brain to have to stop and read code "backwards" before seeing what's going on.

Quoted 7 lines

OK. I'll send v2. Maybe i can change my personal idiom.
I've learned to do this so to avoid a unintended assignment to a variable. For exmaple:
if (var = 10)
compiler will do assignment to variable.
if (10 = var)
compiler will generate error.

(This is not personally pointing at you, but is meant for the archives and for others to see why I feel this way about it, and why it's not just cosmetics.)

Years ago I was on a similar trip, but there was one reply which made me realize how weird it is: "They can remember to put the constant in front, but cannot remember to use the correct operator." I felt embarassed, and woke up. :)

Some two months ago I learned about the "Yoda programming" name, which I feel a very good match for this style. It's really sick to have to read such code. I don't care whether a constant is in some relation to something else, I care about whether the result of calling a function means success or error. Putting things backwards only "works" for those who had intense exposure to and are trained for this style, to everyone else it feels totally unnatural and obfuscates what's happening, and unnecessarily requires mental resources to parse what happens and whether it's correct. Code should not be written for compilers, but for humans to read and verify and maintain the source.

BTW have C compilers been warning about "if (a = b)" for some decades by now. There really is no point in this reverse idiom.

virtually yours
Gerhard Sittig

Signature · 3 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

Hannes Petermaier

author

2014-03-08 18:09 UTC #4 lore raw

PATCH v2 1/1 board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

In reply to Hannes Petermaier

- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong

Signed-off-by Hannes Petermaier <[email protected]>

---
Changes for v2:
- fixed up Yoda programming style at testing return value of

     i2c_set_bus_speed :-)
---
 board/BuR/kwb/board.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

board/BuR/kwb/board.c +2 -2

index 8aa16bc..804765a 100644
@@ -120,7 +120,7 @@ void am33xx_spl_board_init(void)
 
 	/* power-ON  3V3 via Resetcontroller */
 	oldspeed = i2c_get_bus_speed();
-	if (0 != i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
+	if (i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC) >= 0) {
 		buf = RSTCTRL_FORCE_PWR_NEN;
 		i2c_write(RSTCTRL_ADDR, RSTCTRL_CTRLREG, 1,
 			  (uint8_t *)&buf, sizeof(buf));

@@ -221,7 +221,7 @@ int board_late_init(void)
 			   TPS65217_WLEDCTRL1, 0x09, 0xFF);
 	/* write bootinfo into scratchregister of resetcontroller */
 	oldspeed = i2c_get_bus_speed();
-	if (0 != i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC)) {
+	if (i2c_set_bus_speed(CONFIG_SYS_OMAP24_I2C_SPEED_PSOC) >= 0) {
 		i2c_write(RSTCTRL_ADDR, RSTCTRL_SCRATCHREG, 1,
 			  (uint8_t *)&buf, sizeof(buf));
 		i2c_set_bus_speed(oldspeed);

Signature · 1 line

1.7.9.5

Tom Rini

2014-03-12 20:23 UTC #5 lore raw

PATCH v2 1/1 board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

Quoted 3 lines · Hannes Petermaier

- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
Signed-off-by Hannes Petermaier <[email protected]>

Applied to u-boot-ti/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140312/6d1e609a/attachment.pgp>

Tom Rini

2014-03-12 20:04 UTC #6 lore raw

In reply to Hannes Petermaier

Quoted 3 lines · Hannes Petermaier

- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
Signed-off-by Hannes Petermaier <[email protected]>

Applied to u-boot-ti/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140312/9a705bff/attachment.pgp>