[U-Boot] [PATCH V2 3/4] compulab: eeprom: add support for obtaining product name

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Nikita,

On 09/06/15 11:48, Nikita Kiryanov wrote:
> Introduce cl_eeprom_get_product_name() for obtaining product name
> from the eeprom.
> 
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> ---
> Changes in V2:
> 	- s/BOARD_PRODUCT_NAME_*/PRODUCT_NAME_*
> 	- Added documentation
> 	- rewrote cl_eeprom_get_product_name() to take a buffer parameter
> 
>  board/compulab/common/eeprom.c | 30 ++++++++++++++++++++++++++++++
>  board/compulab/common/eeprom.h |  6 ++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/board/compulab/common/eeprom.c b/board/compulab/common/eeprom.c
> index 9f18a3d..6304468 100644
> --- a/board/compulab/common/eeprom.c
> +++ b/board/compulab/common/eeprom.c
> @@ -9,6 +9,7 @@
>  
>  #include <common.h>
>  #include <i2c.h>
> +#include "eeprom.h"
>  
>  #ifndef CONFIG_SYS_I2C_EEPROM_ADDR
>  # define CONFIG_SYS_I2C_EEPROM_ADDR	0x50
> @@ -25,6 +26,8 @@
>  #define BOARD_REV_OFFSET		0
>  #define BOARD_REV_OFFSET_LEGACY		6
>  #define BOARD_REV_SIZE			2
> +#define PRODUCT_NAME_OFFSET		128
> +#define PRODUCT_NAME_SIZE		16
>  #define MAC_ADDR_OFFSET			4
>  #define MAC_ADDR_OFFSET_LEGACY		0

It seems that the time has come to move the above constants out of this file
into the eeprom.h, so they can be used by users of this "lib".
Don't you think?

>  
> @@ -151,3 +154,30 @@ u32 cl_eeprom_get_board_rev(uint eeprom_bus)
>  
>  	return board_rev;
>  };
> +
> +/*
> + * Routine: cl_eeprom_get_board_rev
> + * Description: read system revision from eeprom

Seems like you have a successful copy/paste problem ;-)

> + *
> + * @buf: buffer to store the product name
> + * @eeprom_bus: i2c bus num of the eeprom
> + *
> + * @return: 0 on success, < 0 on failure
> + */
> +int cl_eeprom_get_product_name(uchar *buf, uint eeprom_bus)
> +{
> +	int err;
> +
> +	if (buf == NULL)
> +		return -EINVAL;

I think that this check is not really necessary.
If someone passes NULL instead of buf - let it crash, no?

> +
> +	err = cl_eeprom_setup(eeprom_bus);
> +	if (err)
> +		return err;
> +
> +	err = cl_eeprom_read(PRODUCT_NAME_OFFSET, buf, PRODUCT_NAME_SIZE);
> +	if (!err) /* Protect ourselves from invalid data (unterminated str) */

Why do you need the above check?
I think, you can just write '\0' anyway, no?

> +		buf[PRODUCT_NAME_SIZE - 1] = '\0';
> +
> +	return err;
> +}
> diff --git a/board/compulab/common/eeprom.h b/board/compulab/common/eeprom.h
> index e74c379..c0b4739 100644
> --- a/board/compulab/common/eeprom.h
> +++ b/board/compulab/common/eeprom.h
> @@ -9,10 +9,12 @@
>  
>  #ifndef _EEPROM_
>  #define _EEPROM_
> +#include <errno.h>
>  
>  #ifdef CONFIG_SYS_I2C
>  int cl_eeprom_read_mac_addr(uchar *buf, uint eeprom_bus);
>  u32 cl_eeprom_get_board_rev(uint eeprom_bus);
> +int cl_eeprom_get_product_name(uchar *buf, uint eeprom_bus);
>  #else
>  static inline int cl_eeprom_read_mac_addr(uchar *buf, uint eeprom_bus)
>  {
> @@ -22,6 +24,10 @@ static inline u32 cl_eeprom_get_board_rev(uint eeprom_bus)
>  {
>  	return 0;
>  }
> +static inline int cl_eeprom_get_product_name(uchar *buf, uint eeprom_bus)
> +{
> +	return -ENOSYS;
> +}
>  #endif
>  
>  #endif
> 

-- 
Regards,
Igor.