From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Kipisz Date: Tue, 3 Nov 2015 09:13:12 -0600 Subject: [U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM In-Reply-To: <5638B398.8010904@compulab.co.il> References: <1446553362-29732-1-git-send-email-s-kipisz2@ti.com> <1446553362-29732-4-git-send-email-s-kipisz2@ti.com> <5638B398.8010904@compulab.co.il> Message-ID: <5638CF08.2070701@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 11/03/2015 07:16 AM, Igor Grinberg wrote: > Hi Steve, > > On 11/03/15 14:22, Steve Kipisz wrote: >> From: Lokesh Vutla > > [...] > >> >> Signed-off-by: Lokesh Vutla >> Signed-off-by: Steve Kipisz >> --- >> 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.