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 <lokeshvu...@ti.com>
>>
>> [...]
>>
>>>
>>> Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com>
>>> Signed-off-by: Steve Kipisz <s-kipi...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
