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

[Nishanth Menon]

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 at ti.com>
>>
>> [...]
>>
>>>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
>>> Signed-off-by: Steve Kipisz <s-kipisz2 at 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.


-- 
Regards,
Nishanth Menon