* [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory
@ 2010-02-20 11:14 Frans Meulenbroeks
2010-02-22 7:27 ` Heiko Schocher
2010-02-22 11:05 ` Detlev Zundel
0 siblings, 2 replies; 5+ messages in thread
From: Frans Meulenbroeks @ 2010-02-20 11:14 UTC (permalink / raw)
To: u-boot
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"
--
1.5.4.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory
2010-02-20 11:14 [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory Frans Meulenbroeks
@ 2010-02-22 7:27 ` Heiko Schocher
2010-02-22 8:35 ` Wolfgang Denk
2010-02-22 11:05 ` Detlev Zundel
1 sibling, 1 reply; 5+ messages in thread
From: Heiko Schocher @ 2010-02-22 7:27 UTC (permalink / raw)
To: u-boot
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory
2010-02-22 7:27 ` Heiko Schocher
@ 2010-02-22 8:35 ` Wolfgang Denk
2010-02-22 9:01 ` Heiko Schocher
0 siblings, 1 reply; 5+ messages in thread
From: Wolfgang Denk @ 2010-02-22 8:35 UTC (permalink / raw)
To: u-boot
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
--
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_
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory
2010-02-22 8:35 ` Wolfgang Denk
@ 2010-02-22 9:01 ` Heiko Schocher
0 siblings, 0 replies; 5+ messages in thread
From: Heiko Schocher @ 2010-02-22 9:01 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory
2010-02-20 11:14 [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory Frans Meulenbroeks
2010-02-22 7:27 ` Heiko Schocher
@ 2010-02-22 11:05 ` Detlev Zundel
1 sibling, 0 replies; 5+ messages in thread
From: Detlev Zundel @ 2010-02-22 11:05 UTC (permalink / raw)
To: u-boot
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 at frans" as already happened if you check the
headers.
Cheers
Detlev
--
He thinks he's really smooth, but he's only C^1.
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-02-22 11:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-20 11:14 [U-Boot] [PATCH] cmd_i2c.c: added command to read to memory Frans Meulenbroeks
2010-02-22 7:27 ` Heiko Schocher
2010-02-22 8:35 ` Wolfgang Denk
2010-02-22 9:01 ` Heiko Schocher
2010-02-22 11:05 ` Detlev Zundel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox