[RFC] i2c-tools: i2ctransfer: add new tool

[Wolfram Sang]

This tool allows to construct and concat multiple I2C messages into one
single transfer. Its aim is to test I2C master controllers, and so there
is no SMBus fallback.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

I've been missing such a tool a number of times now, so I finally got around to
writing it myself. As with all I2C tools, it can be dangerous, but it can also
be very useful when developing. I am not sure if distros should supply it, I'll
leave that to Jean's experience. For embedded build systems, I think this
should be selectable. It is RFC for now because it needs broader testing and some
more beautification. However, I've been using it already to test the i2c_quirk
infrastructure and Renesas I2C controllers.

tools/Module.mk | 8 +-
tools/i2ctransfer.c | 296 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 303 insertions(+), 1 deletion(-)
create mode 100644 tools/i2ctransfer.c

diff --git a/tools/Module.mk b/tools/Module.mk
index d14bb0c..62f1238 100644
--- a/tools/Module.mk
+++ b/tools/Module.mk
@@ -14,7 +14,7 @@ TOOLS_CFLAGS := -Wstrict-prototypes -Wshadow -Wpointer-arith -Wcast-qual \
-W -Wundef -Wmissing-prototypes -Iinclude
TOOLS_LDFLAGS := -Llib -li2c

-TOOLS_TARGETS := i2cdetect i2cdump i2cset i2cget
+TOOLS_TARGETS := i2cdetect i2cdump i2cset i2cget i2ctransfer

#
# Programs
@@ -32,6 +32,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)
$(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)

+$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
+ $(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
+
#
# Objects
#
@@ -48,6 +51,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI
$(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h
$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@

+$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h
+ $(CC) $(CFLAGS) -Wno-maybe-uninitialized $(TOOLS_CFLAGS) -c $< -o $@
+
$(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h
$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@

diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
new file mode 100644
index 0000000..30923f5
--- /dev/null
+++ b/tools/i2ctransfer.c
@@ -0,0 +1,296 @@
+/*
+ i2ctransfer.c - A user-space program to send concatenated i2c messages
+ Copyright (C) 2015 Wolfram Sang <w...@sang-engineering.com>
+ Copyright (C) 2015 Renesas Electronics Corporation
+
+ Based on i2cget.c:
+ Copyright (C) 2005-2012 Jean Delvare <jdel...@suse.de>
+
+ which is based on i2cset.c:
+ Copyright (C) 2001-2003 Frodo Looijaard <fro...@dds.nl>, and
+ Mark D. Studebaker <mdsx...@yahoo.com>
+ Copyright (C) 2004-2005 Jean Delvare
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+*/
+
+#include <sys/ioctl.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <linux/i2c.h>
+#include <linux/i2c-dev.h>
+#include "i2cbusses.h"
+#include "util.h"
+#include "../version.h"
+
+enum parse_state {
+ PARSE_GET_ADDR,
+ PARSE_GET_FLAGS,
+ PARSE_GET_LENGTH,
+ PARSE_GET_DATA
+};
+
+#define PRINT_STDERR (1 << 0)
+#define PRINT_READ_BUF (1 << 1)
+#define PRINT_WRITE_BUF (1 << 2)
+#define PRINT_HEADER (1 << 3)
+
+static void help(void)
+{
+ fprintf(stderr,
+ "Usage: i2ctransfer [-f] [-y] [-v] [-V] I2CBUS ADDRESS FLAGS LENGTH [DATA]...\n"
+ " I2CBUS is an integer or an I2C bus name\n"
+ " ADDRESS is an integer (0x03 - 0x77)\n"
+ " FLAGS is one of:\n"
+ " r (read)\n"
+ " w (write)\n"
+ " LENGTH is an integer (0 - 65535)\n"
+ " DATA are LENGTH bytes, for a write message. They can be shortened by a suffix:\n"
+ " = (keep value constant until LENGTH)\n"
+ " + (increase value by 1 until LENGTH)\n"
+ " - (decrease value by 1 until LENGTH)\n"
+ "\nExample (on bus 0, write 0xbd to 0xc0-0xcf of device 0x50, read a byte from device 0x51):\n"
+ " # i2ctransfer 0 0x50 w 0x11 0xc0 0xbd= 0x51 r 1\n"
+ );
+}
+
+static int check_funcs(int file)
+{
+ unsigned long funcs;
+
+ /* check adapter functionality */
+ if (ioctl(file, I2C_FUNCS, &funcs) < 0) {
+ fprintf(stderr, "Error: Could not get the adapter "
+ "functionality matrix: %s\n", strerror(errno));
+ return -1;
+ }
+
+ if (!(funcs & I2C_FUNC_I2C)) {
+ fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
+ return -1;
+ }
+
+ return 0;
+}
+
+static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
+{
+ __u32 i, j;
+ FILE *output = flags & PRINT_STDERR ? stderr : stdout;
+
+ for (i = 0; i < nmsgs; i++) {
+ int read = !!(msgs[i].flags & I2C_M_RD);
+ int newline = !!(flags & PRINT_HEADER);
+
+ if (flags & PRINT_HEADER)
+ fprintf(output, "Msg %u: addr 0x%04x, %s, len %u",
+ i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
+ if (read == !!(flags & PRINT_READ_BUF) ||
+ !read == !!(flags & PRINT_WRITE_BUF)) {
+ if (flags & PRINT_HEADER)
+ fprintf(output, ", buf ");
+ for (j = 0; j < msgs[i].len; j++)
+ fprintf(output, "0x%02x ", msgs[i].buf[j]);
+ newline = 1;
+ }
+ if (newline)
+ fprintf(output, "\n");
+ }
+}
+
+static int confirm(const char *filename, struct i2c_msg *msgs, __u32 nmsgs)
+{
+ fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
+ fprintf(stderr, "I will send the following messages to device file %s:\n", filename);
+ print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
+
+ fprintf(stderr, "Continue? [y/N] ");
+ fflush(stderr);
+ if (!user_ack(0)) {
+ fprintf(stderr, "Aborting on user request.\n");
+ return 0;
+ }
+
+ return 1;
+}
+
+int main(int argc, char *argv[])
+{
+ char c, filename[20];
+ char *end;
+ int i2cbus, address, file, arg_idx = 1;
+ int force = 0, yes = 0, version = 0, verbose = 0;
+ unsigned flag_idx = 0, buf_idx = 0, nmsgs = 0;
+ unsigned long len, raw_data;
+ __u8 data;
+ __u8 *buf;
+ __u16 flags;
+ struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
+ struct i2c_rdwr_ioctl_data rdwr;
+ enum parse_state state = PARSE_GET_ADDR;
+
+ /* handle (optional) arg_idx first */
+ while (arg_idx < argc && argv[arg_idx][0] == '-') {
+ switch (argv[arg_idx][1]) {
+ case 'V': version = 1; break;
+ case 'v': verbose = 1; break;
+ case 'f': force = 1; break;
+ case 'y': yes = 1; break;
+ default:
+ fprintf(stderr, "Error: Unsupported option "
+ "\"%s\"!\n", argv[arg_idx]);
+ help();
+ exit(1);
+ }
+ arg_idx++;
+ }
+
+ if (version) {
+ fprintf(stderr, "i2ctransfer version %s\n", VERSION);
+ exit(0);
+ }
+
+ if (arg_idx == argc) {
+ help();
+ exit(0);
+ }
+
+ i2cbus = lookup_i2c_bus(argv[arg_idx++]);
+ if (i2cbus < 0)
+ exit(1);
+
+ file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
+ if (file < 0 || check_funcs(file))
+ exit(1);
+
+ while (arg_idx < argc) {
+ switch (state) {
+ case PARSE_GET_ADDR:
+ address = parse_i2c_address(argv[arg_idx++]);
+ if (address < 0)
+ exit(1);
+
+ if (!force && set_slave_addr(file, address, 0))
+ exit(1);
+
+ msgs[nmsgs].addr = address;
+ state = PARSE_GET_FLAGS;
+ break;
+
+ case PARSE_GET_FLAGS:
+ flag_idx = 0;
+ flags = 0;
+ while ((c = argv[arg_idx][flag_idx])) {
+ switch (c) {
+ case 'r': flags |= I2C_M_RD; break;
+ case 'w': flags &= ~I2C_M_RD; break;
+ default:
+ fprintf(stderr, "Error: Invalid flag '%c'!\n", c);
+ exit(1);
+ }
+ flag_idx++;
+ }
+ msgs[nmsgs].flags = flags;
+ arg_idx++;
+ state = PARSE_GET_LENGTH;
+ break;
+
+ case PARSE_GET_LENGTH:
+ len = strtoul(argv[arg_idx++], &end, 0);
+ if (*end || len > 65535) {
+ fprintf(stderr, "Error: Length invalid!\n");
+ exit(1);
+ }
+
+ msgs[nmsgs].len = len;
+
+ buf = malloc(len);
+ if (!buf) {
+ fprintf(stderr, "Error: No memory for buffer!\n");
+ exit(ENOMEM);
+ }
+ memset(buf, 0, len);
+ msgs[nmsgs].buf = buf;
+
+ if (flags & I2C_M_RD) {
+ nmsgs++;
+ state = PARSE_GET_ADDR;
+ } else {
+ buf_idx = 0;
+ state = PARSE_GET_DATA;
+ }
+
+ break;
+
+ case PARSE_GET_DATA:
+ raw_data = strtoul(argv[arg_idx++], &end, 0);
+ if (raw_data > 255) {
+ fprintf(stderr, "Error: Data byte '%lu' invalid!\n", raw_data);
+ exit(1);
+ }
+ data = raw_data;
+ buf[buf_idx++] = data;
+
+ c = *end;
+ if (c) {
+ for (; buf_idx < len; buf_idx++) {
+ switch (c) {
+ case '+': data++; break;
+ case '-': data--; break;
+ case '=': break;
+ default:
+ fprintf(stderr, "Error: Invalid data byte suffix '%c'!\n", c);
+ exit(1);
+ }
+
+ buf[buf_idx] = data;
+ }
+ }
+
+ if (buf_idx == len) {
+ nmsgs++;
+ state = PARSE_GET_ADDR;
+ }
+
+ break;
+ }
+ }
+
+ if (state != PARSE_GET_ADDR) {
+ fprintf(stderr, "Error: Incomplete message\n");
+ exit(1);
+ }
+
+ if (nmsgs == 0) {
+ help();
+ exit(0);
+ }
+
+ if (!yes && !confirm(filename, msgs, nmsgs))
+ exit(0);
+
+ rdwr.msgs = msgs;
+ rdwr.nmsgs = nmsgs;
+ if (ioctl(file, I2C_RDWR, &rdwr) < 0) {
+ fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
+ exit(1);
+ }
+
+ close(file);
+
+ print_msgs(msgs, nmsgs, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
+
+ /* let Linux free malloced memory on termination */
+ exit(0);
+}
--
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Wolfram Sang]

On Fri, Feb 27, 2015 at 05:16:56PM +0100, Wolfram Sang wrote:
> This tool allows to construct and concat multiple I2C messages into one
> single transfer. Its aim is to test I2C master controllers, and so there
> is no SMBus fallback.
>
> Signed-off-by: Wolfram Sang <w...@the-dreams.de>
> ---
>
> I've been missing such a tool a number of times now, so I finally got around to
> writing it myself. As with all I2C tools, it can be dangerous, but it can also
> be very useful when developing. I am not sure if distros should supply it, I'll
> leave that to Jean's experience. For embedded build systems, I think this
> should be selectable. It is RFC for now because it needs broader testing and some
> more beautification. However, I've been using it already to test the i2c_quirk
> infrastructure and Renesas I2C controllers.

Jean, my tests went well and so I want to brush it up for inclusion into
i2c-tools upstream. Any show-stoppers you see from a high-level point of
view?

[Jean Delvare]

Hi Wolfram,

I think it is a good idea, just I couldn't find the time to review it,
sorry :(

--
Jean Delvare
SUSE L3 Support

[Wolfram Sang]

On Tue, Apr 21, 2015 at 07:25:51AM +0200, Jean Delvare wrote:
> Hi Wolfram,
>
> On Mon, 20 Apr 2015 19:36:38 +0200, Wolfram Sang wrote:
> > On Fri, Feb 27, 2015 at 05:16:56PM +0100, Wolfram Sang wrote:
> > > This tool allows to construct and concat multiple I2C messages into one
> > > single transfer. Its aim is to test I2C master controllers, and so there
> > > is no SMBus fallback.
> > >
> > > Signed-off-by: Wolfram Sang <w...@the-dreams.de>
> > > ---
> > >
> > > I've been missing such a tool a number of times now, so I finally got around to
> > > writing it myself. As with all I2C tools, it can be dangerous, but it can also
> > > be very useful when developing. I am not sure if distros should supply it, I'll
> > > leave that to Jean's experience. For embedded build systems, I think this
> > > should be selectable. It is RFC for now because it needs broader testing and some
> > > more beautification. However, I've been using it already to test the i2c_quirk
> > > infrastructure and Renesas I2C controllers.
> >
> > Jean, my tests went well and so I want to brush it up for inclusion into
> > i2c-tools upstream. Any show-stoppers you see from a high-level point of
> > view?
>
> I think it is a good idea, just I couldn't find the time to review it,
> sorry :(

No problem, no hurry. Glad to know that you like it in general.

[Jean Delvare]

Hi Wolfram,

On Fri, 27 Feb 2015 17:16:56 +0100, Wolfram Sang wrote:
> This tool allows to construct and concat multiple I2C messages into one
> single transfer. Its aim is to test I2C master controllers, and so there
> is no SMBus fallback.
>
> Signed-off-by: Wolfram Sang <w...@the-dreams.de>
> ---
>
> I've been missing such a tool a number of times now, so I finally got around to
> writing it myself. As with all I2C tools, it can be dangerous, but it can also
> be very useful when developing. I am not sure if distros should supply it, I'll
> leave that to Jean's experience. For embedded build systems, I think this
> should be selectable. It is RFC for now because it needs broader testing and some
> more beautification. However, I've been using it already to test the i2c_quirk
> infrastructure and Renesas I2C controllers.

I took a very quick look today and I am fine with the general idea but
one thing I don't like is the user interface. All parameters look the
same even though they have very different meanings. It's very easy to
get it wrong and guess what, at my very first attempt I managed to do
exactly that. I was lucky enough that my incorrect syntax was not
accepted by the tool, but with slightly different values it would have
been accepted and and the tool would have performed something different
from what I wanted to do. Possibly with tragic consequences.

So instead of:

# i2ctransfer 0 0x50 w 0x11 0xc0 0xbd= 0x51 r 1

I would have imagined something like:

# i2ctransfer 0 w@0x50 0x11 0xc0 0xbd= r@0x51 1

This at least avoids passing addresses as register values or
vice-versa. What do you think? Or maybe it's just me being an idiot and
other users would get it right even with your proposed syntax.

My alternative proposal leaves the potential problem of mixing length
and register values. I am not necessarily a fan of passing the write
length on the command line, but it is similar to the read commands, and
I see it allows writing the same value to a whole register range with
little effort, so it does have some value. So I guess it's OK.

BTW I'm curious if you actually needed the + and - suffixes in practice?
I can easily imagine how the = suffix will be useful in the real
world, but + and -... Or maybe just for bus driver testing purpose?

I don't have time for a full review today but just a couple things
which caught my eye:

> +static void help(void)
> +{
> + fprintf(stderr,
> + "Usage: i2ctransfer [-f] [-y] [-v] [-V] I2CBUS ADDRESS FLAGS LENGTH [DATA]...\n"

You should mention here that the ADDRESS FLAGS LENGTH [DATA] sequence
can be repeated, it's not obvious that the trailing "..." mean that.

> + " I2CBUS is an integer or an I2C bus name\n"
> + " ADDRESS is an integer (0x03 - 0x77)\n"
> + " FLAGS is one of:\n"
> + " r (read)\n"
> + " w (write)\n"

These are really directions not flags.

> + " LENGTH is an integer (0 - 65535)\n"
> + " DATA are LENGTH bytes, for a write message. They can be shortened by a suffix:\n"
> + " = (keep value constant until LENGTH)\n"
> + " + (increase value by 1 until LENGTH)\n"
> + " - (decrease value by 1 until LENGTH)\n"
> + "\nExample (on bus 0, write 0xbd to 0xc0-0xcf of device 0x50, read a byte from device 0x51):\n"
> + " # i2ctransfer 0 0x50 w 0x11 0xc0 0xbd= 0x51 r 1\n"
> + );

> (...)

> +}
> + /* let Linux free malloced memory on termination */

I don't like this. The memory allocated in i2cbusses.c is freed
explicitly, so it is inconsistent to not free yours. Freeing the memory
explicitly makes the code easier to read and debug as it documents the
lifetime of allocated buffers (for both humans and tools like
valgrind.) So please free your memory buffers explicitly.

> + exit(0);
> +}

--
Jean Delvare
SUSE L3 Support

[Jean Delvare]

Having slept over it, I came up with a 3rd proposal:

# i2ctransfer 0 w0x11@0x50 0xc0 0xbd= r1@0x51

That is, combining the slave address, direction and length into a
single parameter. The advantage is that this is all more explicit and
the risk of mixing up values is close to zero. Whether it is more or
less readable than the previous proposals is probably a matter of
taste. Also I suspect it would make the parsing and state machine more
simple, but that's only a nice side effect.

Wolfram (and others), please tell me what you think. I am not trying to
force my views here, just suggesting alternatives for your
consideration.

[Wolfram Sang]

> Having slept over it, I came up with a 3rd proposal:
>
> # i2ctransfer 0 w0x11@0x50 0xc0 0xbd= r1@0x51
>
> That is, combining the slave address, direction and length into a
> single parameter. The advantage is that this is all more explicit and
> the risk of mixing up values is close to zero. Whether it is more or
> less readable than the previous proposals is probably a matter of
> taste. Also I suspect it would make the parsing and state machine more
> simple, but that's only a nice side effect.
>
> Wolfram (and others), please tell me what you think. I am not trying to
> force my views here, just suggesting alternatives for your
> consideration.

I liked your proposal, so thanks for this input. I agree that the risk
of mixing something up is high, I was okay with the printout of the
messages to be sent, but a better syntax is very welcome, too. I need to
think about the flags a little bit, though. Although this isn't
implemented yet, PEC and 10-bit flags might be added in the future?
Handling R/W as "just another" flag made this option extremly simple.
But we probably can work something out.

So much for the quick response, I'll have a closer look later.

[Randy Grunwell]

I'm curious why this would not be an extension of the i2c read and write commands? Would it not make sense to add a tier above "Block" (perhaps "Extended"), and use the same syntax?

Forgive me if this is out of place - I'm quite new, both here and to Linux/C.

To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majo...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

[Uwe Kleine-König]

Hello Randy,

On Fri, May 08, 2015 at 03:28:19PM +0000, Randy Grunwell wrote:
> I'm curious why this would not be an extension of the i2c read and write commands? Would it not make sense to add a tier above "Block" (perhaps "Extended"), and use the same syntax?
>
> Forgive me if this is out of place - I'm quite new, both here and to Linux/C.

one thing you should look into is how you quote mails correctly and
please don't top-post.

Also breaking long lines after ~72 chars is usually done.

These requests might seem picky, but they simplify things for people
reading several hundred mails per day.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |

[Jean Delvare]

Hi Randy,

On Fri, 8 May 2015 15:28:19 +0000, Randy Grunwell wrote:
> I'm curious why this would not be an extension of the i2c read and write commands? Would it not make sense to add a tier above "Block" (perhaps "Extended"), and use the same syntax?
>
> Forgive me if this is out of place - I'm quite new, both here and to Linux/C.

No problem, asking questions is fine.

The thing is that this isn't only a question of maximum length. It is
also a question of which kernel interface is being used (ioctl I2C_RDWR
instead of ioctl I2C_SMBUS.) Additionally, i2ctransfer supports any
combination of reading and writing, so in essence it doesn't extend a
specific existing tool, it extends all of them. And the command line
interface will be completely different, whichever we settle for. So it
seems quite obvious that a separate tool is the best way to implement
the feature, as Wolfram did.

--
Jean Delvare
SUSE L3 Support
--

[Jean Delvare]

Hi Wolfram,

On Fri, 8 May 2015 16:38:26 +0200, Wolfram Sang wrote:
> > Having slept over it, I came up with a 3rd proposal:
> >
> > # i2ctransfer 0 w0x11@0x50 0xc0 0xbd= r1@0x51
> >
> > That is, combining the slave address, direction and length into a
> > single parameter. The advantage is that this is all more explicit and
> > the risk of mixing up values is close to zero. Whether it is more or
> > less readable than the previous proposals is probably a matter of
> > taste. Also I suspect it would make the parsing and state machine more
> > simple, but that's only a nice side effect.
> >
> > Wolfram (and others), please tell me what you think. I am not trying to
> > force my views here, just suggesting alternatives for your
> > consideration.
>
> I liked your proposal, so thanks for this input. I agree that the risk
> of mixing something up is high, I was okay with the printout of the
> messages to be sent, but a better syntax is very welcome, too. I need to
> think about the flags a little bit, though. Although this isn't
> implemented yet, PEC and 10-bit flags might be added in the future?

This is a good point, we need to think about it. Maybe not PEC, as
normally any PEC-enabled transaction would be handled by the other
tools already. And I don't think the kernel can handle PEC over ioctl
I2C_RDWR anyway. But 10-bit addresses, we already had a request to
support than and your new tool would be perfect for that.

One easy way would be to assume that the transaction either targets one
or more 10-bit addressed chips, or one or more 7-bit addressed chips,
but doesn't mix. In that case a simple flag (say -t) in front of the
transaction will do the trick. I'd think it is sufficient, and I even
suspect that some controllers may only support that, but OTOH I never
worked with 10-bit addressed chips so I can't really tell.

If you think it's not enough, then the address modifier could go
separately before or after the address byte, i.e. either r1@0x123t or
r1@t0x123. I suspect that the latter should be easier to implement.

> Handling R/W as "just another" flag made this option extremly simple.
> But we probably can work something out.

I think the proposal above makes more sense than grouping it with the
direction letter (r or w) even though it's also a letter, as it's
really an address modifier, which affects neither the direction nor the
length. But again it's really only a suggestion, if you can come up
with something clearer and/or easier to implement, please do.

> So much for the quick response, I'll have a closer look later.

I wouldn't call it "quick" ;-) but you're welcome.

[Wolfram Sang]

Hi Jean,

> If you think it's not enough, then the address modifier could go
> separately before or after the address byte, i.e. either r1@0x123t or
> r1@t0x123. I suspect that the latter should be easier to implement.

In deed, I would aim for maximum flexibility (i.e. possible to mix 7 and
10 bit addresses), and implement the latter proposal of yours.

> > Handling R/W as "just another" flag made this option extremly simple.
> > But we probably can work something out.
>
> I think the proposal above makes more sense than grouping it with the
> direction letter (r or w)

I agree.

[Wolfram Sang]

> BTW I'm curious if you actually needed the + and - suffixes in practice?
> I can easily imagine how the = suffix will be useful in the real
> world, but + and -... Or maybe just for bus driver testing purpose?

Exactly for that. While you can send complex messages with my tool, it
should be clear this is mainly for testing/developing. Once you know
what you need to do to access a slave, a specfic driver is the better
option, be it in userspace or kernel space.

Those patterns are easily recognizable when fed into an EEPROM. They can
already reveal quite some problems with bus drivers, e.g. off-by-one
errors when fetching data, sending STOP etc.

> > +}
> > + /* let Linux free malloced memory on termination */
>
> I don't like this. The memory allocated in i2cbusses.c is freed
> explicitly, so it is inconsistent to not free yours. Freeing the memory

Well, it is documented :)

> explicitly makes the code easier to read and debug as it documents the

I disagree about this one. Currently, we have a LOT of error paths when
parsing input fails. Getting all the error paths right with freeing
memory is usually error-prone (especially with later additions) and will
IMO make the code way less readable. Since the lifetime of those buffers
ends right before the exit(0), it seems unnecessarily complex to me.

However, I need to redo the parsing anyhow. Let's see how it looks like
in the next version. If it is easy/readable to free(), I'll do it.