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

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4B8231FF.1050605 at denx.de> you wrote:
> 
> > 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.

The intention is to be able to read from / write to any I2C device,
not only EEPROMs. For example, assume to read a time from a RTC, or a
temperature from a DTT.

> > +	/*
> > +	 * 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).

No such check should be added.

Note that 0 _is_ a valid address in many systems (for example, on all
Power Architecture systems, 0 is the begin of the system RAM and can
be read and written without problems).

> > +	if (i2c_read(chip, devaddr, alen, memaddr, length) != 0)
> > +	{

BTW: incorrect brace style.

> > +		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!

We are not talking about EEPROMs here. If you want to access EEPROMs,
you can use the EEPROM command. The "i2c" command set is for low-level
access to I2C devices, and you are supposed to know what you are
doing.

> >  #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?

Agreed - these changes are bogus. As Detlev pointed out, the
subcommand handling should be reworked.

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

This seems unlogical to me. First, this would cover only reading, but
we also want to add support for writing. Second, what would "md" store
to memory? The raw content or an ASCII hex dump of it? From the
command name, it should store a hex dump - which is not what we are
looking for.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the bathtub of history the truth is harder to hold than the  soap,
and much more difficult to find ...     - Terry Pratchett, _Sourcery_