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

[Steven Kipisz]

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?
>>   endif
>>
>>   ifneq ($(CONFIG_OMAP54XX),)
>> diff --git a/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>> new file mode 100644
>> index 000000000000..f59ebbdb4ee8
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>
> [...]
>
>> +void __maybe_unused set_board_info_env(char *name, char *default_name,
>> +				       char *revision, char *serial)
>
> That looks really weird API to me...
> You have name and default_name? Why would you need both...
>
Default to beagle_x15 if the EEPROM isn't programmed. Looking at your 
other post, I think I see how to remove default_name.
>> +{
>> +	char *unknown = "unknown";
>> +
>> +	if (name)
>> +		setenv("board_name", name);
>> +	else
>> +		setenv("board_name", default_name);
>> +
>> +	if (revision)
>> +		setenv("board_revision", revision);
>> +	else
>> +		setenv("board_revision", unknown);
>> +
>> +	if (serial)
>> +		setenv("board_serial", serial);
>> +	else
>> +		setenv("board_serial", unknown);
>> +}
>
> [...]
>
>> diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
>> index d773b0430ad4..a76c67a85d37 100644
>> --- a/arch/arm/include/asm/omap_common.h
>> +++ b/arch/arm/include/asm/omap_common.h
>
> [...]
>
>> +/**
>> + * set_board_info_env() - Setup commonly used board information environment vars
>> + * @name:	Name of the board
>> + * @default_name: In case of empty string, what name to use?
>
> That seems redundant.
> The caller knows the board name, and if it does not, then it can place
> an arbitrary name (like unknown) into name parameter.
>
>> + * @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.
>> +#endif	/* __ASSEMBLY__ */
>> +
>>   #endif /* _OMAP_COMMON_H_ */
>>
>
Steve K.