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

[Igor Grinberg]

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).

>  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...

> +{
> +	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.

> +#endif	/* __ASSEMBLY__ */
> +
>  #endif /* _OMAP_COMMON_H_ */
> 

-- 
Regards,
Igor.