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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
