[U-Boot] [PATCH] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[U-Boot] [PATCH] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[Hannes Petermaier]

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

Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>
---
 board/BuR/kwb/board.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/board/BuR/kwb/board.c b/board/BuR/kwb/board.c
index 8aa16bc..8fb5e68 100644
--- a/board/BuR/kwb/board.c
+++ b/board/BuR/kwb/board.c
@@ -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);
-- 
1.7.10.4

[U-Boot] [PATCH] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[Gerhard Sittig]

On Fri, Mar 07, 2014 at 18:56 +0100, Hannes Petermaier wrote:
> 
> - fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
> 
> Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>
> ---
>  board/BuR/kwb/board.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/board/BuR/kwb/board.c b/board/BuR/kwb/board.c
> index 8aa16bc..8fb5e68 100644
> --- a/board/BuR/kwb/board.c
> +++ b/board/BuR/kwb/board.c
> @@ -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
-- 
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

[U-Boot] [PATCH] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[Hannes Petermaier]

Hi Gerhard,

On 2014-03-08 18:38, Gerhard Sittig wrote:
> On Fri, Mar 07, 2014 at 18:56 +0100, Hannes Petermaier wrote:
>> - fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
>>
>> Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>
>> ---
>>   board/BuR/kwb/board.c |    4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/board/BuR/kwb/board.c b/board/BuR/kwb/board.c
>> index 8aa16bc..8fb5e68 100644
>> --- a/board/BuR/kwb/board.c
>> +++ b/board/BuR/kwb/board.c
>> @@ -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.
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.
> virtually yours
> Gerhard Sittig
best regards,
Hannes

[U-Boot] [PATCH] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[Gerhard Sittig]

On Sat, Mar 08, 2014 at 19:14 +0100, Hannes Petermaier wrote:
> 
> Hi Gerhard,
> 
> On 2014-03-08 18:38, Gerhard Sittig wrote:
> >On Fri, Mar 07, 2014 at 18:56 +0100, Hannes Petermaier wrote:
> >>- fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
> >>
> >>Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>
> >>---
> >>  board/BuR/kwb/board.c |    4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/board/BuR/kwb/board.c b/board/BuR/kwb/board.c
> >>index 8aa16bc..8fb5e68 100644
> >>--- a/board/BuR/kwb/board.c
> >>+++ b/board/BuR/kwb/board.c
> >>@@ -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.
> 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
-- 
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

[U-Boot] [PATCH v2] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[Hannes Petermaier]

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

Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>

---
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(-)

diff --git a/board/BuR/kwb/board.c b/board/BuR/kwb/board.c
index 8aa16bc..804765a 100644
--- a/board/BuR/kwb/board.c
+++ b/board/BuR/kwb/board.c
@@ -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);
-- 
1.7.9.5

[U-Boot] [U-Boot, v2] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[Tom Rini]

On Sat, Mar 08, 2014 at 07:09:32PM +0100, Hannes Petermaier wrote:

> - fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
> 
> Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>

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

-- 
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>

[U-Boot] board/BuR/kwb: fix usage of 'i2c_set_bus_speed'

[Tom Rini]

On Fri, Mar 07, 2014 at 06:56:55PM +0100, Hannes Petermaier wrote:

> - fix: return-value of 'i2c_set_bus_speed' was interpreted wrong
> 
> Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>

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

-- 
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>