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

[Nishanth Menon <nm@ti.com>]

On 11/03/2015 03:07 PM, Igor Grinberg wrote:
> 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...
> 

:) OK, agreed. makes sense. I think Steve can definitely push it off
into the board/ti/common/ file :).

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

Fair enough. Thanks for reviewing it.

-- 
Regards,
Nishanth Menon