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

[Igor Grinberg <grinberg@compulab.co.il>]

On 11/03/15 17:26, Nishanth Menon wrote:
> On 11/03/2015 09:13 AM, Steven Kipisz wrote:
>> On 11/03/2015 07:16 AM, Igor Grinberg wrote:
>>> Hi Steve,
>>>
>>> On 11/03/15 14:22, Steve Kipisz wrote:
>>>> From: Lokesh Vutla <lokeshvutla@ti.com>
>>>
>>> [...]
>>>
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>>>> ---
>>>> v2 Based on
>>>>   master      a6104737  ARM: at91: sama5: change the environment
>>>> address to 0x6000
>>>>
>>>> Changes in v2 (since v1)
>>>>     - make the EEPROM code mor generic for TI EVMs
>>>>     - rename structures/subroutines to ti_am_xxxxx
>>>>     - add routines to access the EEPROM data
>>>>     - redo commit message to be more clear
>>>>
>>>> v1:   http://marc.info/?t=144608007900001&r=1&w=2
>>>>     (mailing list squashed original submission)
>>>>
>>>>   arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
>>>>   arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148
>>>> +++++++++++++++++++++++++
>>>>   arch/arm/include/asm/omap_common.h             | 130
>>>> +++++++++++++++++++++-
>>>>   3 files changed, 278 insertions(+), 1 deletion(-)
>>>>   create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile
>>>> b/arch/arm/cpu/armv7/omap-common/Makefile
>>>> index 464a5d1d732a..53a9fdb81100 100644
>>>> --- a/arch/arm/cpu/armv7/omap-common/Makefile
>>>> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
>>>> @@ -15,6 +15,7 @@ obj-y    += clocks-common.o
>>>>   obj-y    += emif-common.o
>>>>   obj-y    += vc.o
>>>>   obj-y    += abb.o
>>>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
>>>
>>> This makes this module compile on all TI SoC based boards enabling I2C.
>>> AFAIU, this is a separate chip (not inside the SoC), so this module will
>>> also compile on non-TI boards that do not have this EEPROM.
>>> I think, it should be more fine grained (e.g. have its own symbol).
>>>
>> Can you give a suggestion?
> 
> Are you sure this will be built into non-ti SoCs with I2C enabled if you
> are not using the function? I assume  __maybe_unused should take care of
> that, no - let the compiler do the gc anyways?
> 
> 
> It should have been documented on commit message as well.
> 
> 
>>>> + * @revision:    Revision of the board
>>>> + * @serial:    Serial Number of the board
>>>> + *
>>>> + * In case of NULL revision or serial information "unknown" is setup.
>>>> + * If name is NULL, default_name is used.
>>>> + */
>>>> +void set_board_info_env(char *name, char *default_name,
>>>> +            char *revision, char *serial);
>>>> +
>>>> +/**
>>>> + * board_am_is() - Generic Board detection logic
>>>> + * @name_tag:    Tag used in eeprom for the board
>>>> + *
>>>> + * Return: false if board information does not match OR eeprom
>>>> was'nt read.
>>>> + *       true otherwise
>>>> + */
>>>> +static inline bool board_am_is(char *name_tag)
>>>> +{
>>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>> +
>>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>> +        return false;
>>>> +    return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
>>>> +}
>>>
>>> Why do you need to place non trivial function implementation inside the
>>> header file?
>>>
>>>> +
>>>> +/**
>>>> + * 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?

Also, AFAIR, we try to not place code inside headers, unless the code
is a stub.

-- 
Regards,
Igor.