[U-Boot] [PATCH] cmd_i2c.c: added command to read to memory
From: Frans <frans@frans.(none)>
Added a new function i2c read to read to memory. That way it becomes possible to test against a value and use that to influence the boot process.
Design decision was to stay close to the i2c md command with respect to command syntax.
Signed-off-by: Frans Meulenbroeks fransmeulenbroeks@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) { + 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); + + if (i2c_read(chip, devaddr, alen, memaddr, length) != 0) + { + puts ("Error reading the chip.\n"); + return 1; + } + return 0; +} + +/* + * 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); #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); #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); 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); - if (!strncmp(argv[0], "re", 2)) { + if (!strncmp(argv[0], "reset", 5)) { i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); return 0; } - if (!strncmp(argv[0], "lo", 2)) + if (!strncmp(argv[0], "loop", 4)) return do_i2c_loop(cmdtp, flag, argc, argv); #if defined(CONFIG_CMD_SDRAM) - if (!strncmp(argv[0], "sd", 2)) + if (!strncmp(argv[0], "sdram", 5)) return do_sdram(cmdtp, flag, argc, argv); #endif cmd_usage(cmdtp); @@ -1296,6 +1355,7 @@ U_BOOT_CMD( #if defined(CONFIG_I2C_MULTI_BUS) "i2c dev [dev] - show or set current I2C bus\n" #endif /* CONFIG_I2C_MULTI_BUS */ + "i2c read chip address[.0, .1, .2] #_of_objects memaddr - read from I2C device to memory\n" "i2c md chip address[.0, .1, .2] [# of objects] - read from I2C device\n" "i2c mm chip address[.0, .1, .2] - write to I2C device (auto-incrementing)\n" "i2c mw chip address[.0, .1, .2] value [count] - write to I2C device (fill)\n"
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@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
Dear Heiko Schocher,
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.
- /*
* 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
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
Hi Frans,
From: Frans <frans@frans.(none)>
You should adjust your git settings before committing to git, i.e. do a "git config --global user.email <your e-mail>". Otherwise "git email sends mails to "frans@frans" as already happened if you check the headers.
Cheers Detlev
participants (4)
-
Detlev Zundel -
Frans Meulenbroeks -
Heiko Schocher -
Wolfgang Denk