[U-Boot] [PATCH v6 03/23] Add abs() macro to return absolute value

[Albert ARIBAUD]

Hi Tom,

Le 10/05/2012 00:56, Tom Warren a écrit :
> Albert,
>
>> -----Original Message-----
>> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
>> Sent: Friday, May 04, 2012 12:28 PM
>> To: Simon Glass
>> Cc: U-Boot Mailing List; Tom Warren
>> Subject: Re: [U-Boot] [PATCH v6 03/23] Add abs() macro to return absolute
>> value
>>
>> Hi Simon,
>>
>> Le 04/05/2012 20:25, Simon Glass a écrit :
>>> This macro is generally useful to make it available in common.
>>>
>>> Signed-off-by: Simon Glass<sjg at chromium.org>
>>> ---
>>> Changes in v3:
>>> - Add new patch to put abs() in common.h
>>>
>>> Changes in v6:
>>> - Update x86emu and omap4 to use the abs() macro
>>>
>>>    arch/arm/cpu/armv7/omap4/clocks.c       |    2 --
>>>    drivers/bios_emulator/x86emu/prim_ops.c |    5 -----
>>>    include/common.h                        |   13 +++++++++++++
>>>    3 files changed, 13 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/omap4/clocks.c
>>> b/arch/arm/cpu/armv7/omap4/clocks.c
>>> index e2189f7..ce3f59c 100644
>>> --- a/arch/arm/cpu/armv7/omap4/clocks.c
>>> +++ b/arch/arm/cpu/armv7/omap4/clocks.c
>>> @@ -46,8 +46,6 @@
>>>    #define puts(s)
>>>    #endif
>>>
>>> -#define abs(x) (((x)<   0) ? ((x)*-1) : (x))
>>> -
>>>    struct omap4_prcm_regs *const prcm = (struct omap4_prcm_regs
>>> *)0x4A004100;
>>>
>>>    const u32 sys_clk_array[8] = {
>>> diff --git a/drivers/bios_emulator/x86emu/prim_ops.c
>>> b/drivers/bios_emulator/x86emu/prim_ops.c
>>> index 7553087..5f6c795 100644
>>> --- a/drivers/bios_emulator/x86emu/prim_ops.c
>>> +++ b/drivers/bios_emulator/x86emu/prim_ops.c
>>> @@ -118,11 +118,6 @@ static u32 x86emu_parity_tab[8] =
>>>
>>>    #define PARITY(x)   (((x86emu_parity_tab[(x) / 32]>>   ((x) % 32))&   1)
>> == 0)
>>>    #define XOR2(x)	    (((x) ^ ((x)>>1))&   0x1)
>>> -/*----------------------------- Implementation
>>> ----------------------------*/ -int abs(int v) -{
>>> -	return (v>0)?v:-v;
>>> -}
>>>
>>>    /*----------------------------- Implementation
>>> ----------------------------*/
>>>
>>> diff --git a/include/common.h b/include/common.h index
>>> 74d9704..92eac2c 100644
>>> --- a/include/common.h
>>> +++ b/include/common.h
>>> @@ -229,6 +229,19 @@ ulong timer_get_boot_us(void);
>>>    #define MIN(x, y)  min(x, y)
>>>    #define MAX(x, y)  max(x, y)
>>>
>>> +/*
>>> + * Return the absolute value of a number. This handles unsigned ints,
>>> +shorts
>>> + * and chars and returns a signed long.
>>> + */
>>
>> What is the rationale for forcing a signed long return type? Such macros as
>> max, min, abs etc usually try to avoid any forced typing. Can't you get rid
>> of the ret variable and use __x directly as the return value?
>> +#define abs(x) ({					\
>>> +		long ret;				\
>>> +		{					\
>>> +			typeof((x)) __x = (x);		\
>>> +			ret = (__x<   0) ? -__x : __x;	\
>>> +		}					\
>>> +		ret;					\
>>> +	})
>>> +
>>>    #if defined(CONFIG_ENV_IS_EMBEDDED)
>>>    #define TOTAL_MALLOC_LEN	CONFIG_SYS_MALLOC_LEN
>>>    #elif ( ((CONFIG_ENV_ADDR+CONFIG_ENV_SIZE)<
>>> CONFIG_SYS_MONITOR_BASE) || \
>>
> This is the last fix needed to get my pull request to compile w/o errors/warnings w/MAKEALL.
>
> To get this unstuck, how about if I change the macro to:
>
> #define abs(X)				\
> 	({ typeof (X) __x = (X);		\
> 		(__x<   0) ? -__x : __x; })
>
> And put it locally in drivers/power/tps6586x.c (the only file that
> uses it in that patchset). That way I get a clean build w/o the OMAP4
> warnings of a double-defined abs(), and we can move everybody to use
> abs() in include/common.h at a later date?

I don't have the final word on drivers/power :) but the best is you send 
out a patch and if no one rejects it, then it should go in.

> Tom

Amicalement,
-- 
Albert.