From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nishanth Menon Date: Tue, 3 Nov 2015 15:13:09 -0600 Subject: [U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM In-Reply-To: <563921FC.5060602@compulab.co.il> References: <1446553362-29732-1-git-send-email-s-kipisz2@ti.com> <1446553362-29732-4-git-send-email-s-kipisz2@ti.com> <5638B398.8010904@compulab.co.il> <5638CF08.2070701@ti.com> <5638D234.7060604@ti.com> <5638E8BF.7090603@compulab.co.il> <5638F2D6.60203@ti.com> <563921FC.5060602@compulab.co.il> Message-ID: <56392365.7020308@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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