From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Schocher <hs@denx.de>
Date: Mon, 22 Feb 2010 10:01:00 +0100
Subject: [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory
In-Reply-To: <20100222083531.CB118A87D18@gemini.denx.de>
References: <1266664483-19663-1-git-send-email-fransmeulenbroeks@gmail.com>
	<4B8231FF.1050605@denx.de> <20100222083531.CB118A87D18@gemini.denx.de>
Message-ID: <4B8247CC.5070508@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hello Wolfgang,

Wolfgang Denk schrieb:
> In message <4B8231FF.1050605@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.

Ah, Ok.

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

Ups, yes, you are right!

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

Then it is Ok for me.

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

Ack.

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

Yep you are right, that was no good idea. Also the "len" argument
in the "i2c md" command is optional!

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