[U-Boot] [PATCH] cmd_i2c.c: added command to read to memory

[Heiko Schocher]

Hello Frans,

Frans Meulenbroeks wrote:
> Added a new function i2c read to read to memory.

Why is this function needed? Do you read from an EEprom?
If so, you can use the eeprom command, or?

> That way it becomes possible to test against a value and
> use that to influence the boot process.

Ah, I see, but again, if you read from an eeprom, use the eeprom
command.

> Design decision was to stay close to the i2c md command with
> respect to command syntax.
> 
> Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks at gmail.com>
> ---
>  common/cmd_i2c.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 68 insertions(+), 8 deletions(-)
> 
> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
> index 62cbd33..0100aa9 100644
> --- a/common/cmd_i2c.c
> +++ b/common/cmd_i2c.c
> @@ -150,6 +150,64 @@ int i2c_set_bus_speed(unsigned int)
>  
>  /*
>   * Syntax:
> + *	i2c read {i2c_chip} {devaddr}{.0, .1, .2} {len} {memaddr}
> + */
> +
> +int do_i2c_read ( cmd_tbl_t *cmdtp, int argc, char *argv[])
> +{
> +	u_char	chip;
> +	uint	devaddr, alen, length;
> +	u_char  *memaddr;
> +	int	j;
> +
> +	if (argc != 5) {
> +		cmd_usage(cmdtp);
> +		return 1;
> +	}
> +
> +	/*
> +	 * I2C chip address
> +	 */
> +	chip = simple_strtoul(argv[1], NULL, 16);
> +
> +	/*
> +	 * I2C data address within the chip.  This can be 1 or
> +	 * 2 bytes long.  Some day it might be 3 bytes long :-).
> +	 */
> +	devaddr = simple_strtoul(argv[2], NULL, 16);
> +	alen = 1;
> +	for (j = 0; j < 8; j++) {
> +		if (argv[2][j] == '.') {
> +			alen = argv[2][j+1] - '0';
> +			if (alen > 4) {

shouldn;t it be "if (alen > 3) {" ?

> +				cmd_usage(cmdtp);
> +				return 1;
> +			}
> +			break;
> +		} else if (argv[2][j] == '\0')
> +			break;
> +	}
> +
> +	/*
> +	 * Length is the number of objects, not number of bytes.
> +	 */
> +	length = simple_strtoul(argv[3], NULL, 16);
> +
> +	/*
> +	 * memaddr is the address where to store things in memory
> +	 */
> +	memaddr = (u_char *)simple_strtoul(argv[4], NULL, 16);

Please add a check, if it is a valid address (not NULL).

> +
> +	if (i2c_read(chip, devaddr, alen, memaddr, length) != 0)
> +	{
> +		puts ("Error reading the chip.\n");
> +		return 1;
> +	}
> +	return 0;
> +}

Hmm... and what is, if you read from an eeprom, and you cross pages?
You don;t get what you expect!

> +/*
> + * Syntax:
>   *	i2c md {i2c_chip} {addr}{.0, .1, .2} {len}
>   */
>  #define DISP_LINE_LEN	16
> @@ -1249,15 +1306,17 @@ int do_i2c(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>  	argv++;
>  
>  #if defined(CONFIG_I2C_MUX)
> -	if (!strncmp(argv[0], "bu", 2))
> +	if (!strcmp(argv[0], "bus", 3))
>  		return do_i2c_add_bus(cmdtp, flag, argc, argv);

Why this?

>  #endif  /* CONFIG_I2C_MUX */
> -	if (!strncmp(argv[0], "sp", 2))
> +	if (!strncmp(argv[0], "speed", 5))
>  		return do_i2c_bus_speed(cmdtp, flag, argc, argv);

and this ... and all other?

Keep in mind, that maybe there are at least default environments, which
uses i2c commands, and so you have to check, if you don;t break existing
board support, if you change this!

>  #if defined(CONFIG_I2C_MULTI_BUS)
> -	if (!strncmp(argv[0], "de", 2))
> +	if (!strncmp(argv[0], "dev", 3))
>  		return do_i2c_bus_num(cmdtp, flag, argc, argv);
>  #endif  /* CONFIG_I2C_MULTI_BUS */
> +	if (!strncmp(argv[0], "read", 4))
> +		return do_i2c_read(cmdtp, argc, argv);

Please sort alphabetical!

>  	if (!strncmp(argv[0], "md", 2))
>  		return do_i2c_md(cmdtp, flag, argc, argv);
>  	if (!strncmp(argv[0], "mm", 2))
> @@ -1266,18 +1325,18 @@ int do_i2c(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>  		return do_i2c_mw(cmdtp, flag, argc, argv);
>  	if (!strncmp(argv[0], "nm", 2))
>  		return mod_i2c_mem (cmdtp, 0, flag, argc, argv);
> -	if (!strncmp(argv[0], "cr", 2))
> +	if (!strncmp(argv[0], "crc", 3))
>  		return do_i2c_crc(cmdtp, flag, argc, argv);
> -	if (!strncmp(argv[0], "pr", 2))
> +	if (!strncmp(argv[0], "probe", 5))
>  		return do_i2c_probe(cmdtp, flag, argc, argv);

Add the new command here ...

> -	if (!strncmp(argv[0], "re", 2)) {
> +	if (!strncmp(argv[0], "reset", 5)) {

... and you have only here to change the command check length from 2 -> 3!

Or, you convert it, as Detlev suggested here:

http://lists.denx.de/pipermail/u-boot/2010-February/067893.html

This would be the preferred way.

While writting here, and your code is just a copy from "i2c md"
maybe you can just modify the i2c md command, to something like that:

"i2c md {i2c_chip} {addr}{.0, .1, .2} {len} {memaddr}"

If there is a "memaddr", the i2c md command don;t print the values,
but it writes them to the memaddr ...

bye
heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany