[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

On 11/03/15 19:45, Nishanth Menon wrote:
> On 11/03/2015 11:02 AM, Igor Grinberg wrote:
> 
>>>>>> +
>>>>>> +/**
>>>>>> + * board_am_rev_is() - Compare board revision
>>>>>> + * @rev_tag:    Revision tag to check in eeprom
>>>>>> + * @cmp_len:    How many chars to compare?
>>>>>> + *
>>>>>> + * NOTE: revision information is often messed up (hence the str len
>>>>>> match) :(
>>>>>> + *
>>>>>> + * Return: false if board information does not match OR eeprom
>>>>>> was'nt read.
>>>>>> + *       true otherwise
>>>>>> + */
>>>>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>>>>> +{
>>>>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>>>> +    int l;
>>>>>> +
>>>>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>>>> +        return false;
>>>>>> +
>>>>>> +    l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>>>>> cmp_len;
>>>>>> +    return !strncmp(ep->version, rev_tag, l);
>>>>>> +}
>>>>>
>>>>> Same here.
>>>>>
>>>> I thought by making them static inline would save space.
>>>
>>> I prefer that myself as well.
>>
>> I'm not sure I understand what space will it save?
>> AFAIK, inline places the function code inside the the caller function
>> and thus spreads into each caller, no? It probably saves some branches,
>> but how does that save space?
> 
> I dont think it saves space, but rather a function call overhead for
> trivial code as above.

I'm sorry, IMO the above code is not trivial enough... 
or just it is not trivial: several variables on the stack,
some logic which is not that straight forward for someone who is
not familiar with the code and defines that are mapped into an SRAM...
and we also have strings comparison in the end.
Come on... that is not trivial at all. Moreover it gives a very bad
example to any newcomer...

> 
>> Also, AFAIR, we try to not place code inside headers, unless the code
>> is a stub.
> 
> 
> That does not always make sense. here it is a straight forward
> comparison.. why hide it a function call deep when you can inline it?

Well, at least because header files are not meant to hold code.
Accepting this gives a bad example and reduces code quality...

-- 
Regards,
Igor.