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

[Jean Delvare <jdelvare@suse.de>]

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 <wsa@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