From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751511AbbEGUIV (ORCPT ); Thu, 7 May 2015 16:08:21 -0400 Received: from cantor2.suse.de ([195.135.220.15]:46943 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751023AbbEGUIT (ORCPT ); Thu, 7 May 2015 16:08:19 -0400 Date: Thu, 7 May 2015 22:08:12 +0200 From: Jean Delvare To: Wolfram Sang Cc: linux-i2c@vger.kernel.org, linux-sh@vger.kernel.org, Magnus Damm , Simon Horman , Laurent Pinchart , Geert Uytterhoeven , linux-kernel@vger.kernel.org Subject: Re: [RFC] i2c-tools: i2ctransfer: add new tool Message-ID: <20150507220812.3776bb83@endymion.delvare> In-Reply-To: <1425053816-19804-1-git-send-email-wsa@the-dreams.de> References: <1425053816-19804-1-git-send-email-wsa@the-dreams.de> Organization: SUSE Linux X-Mailer: Claws Mail 3.10.1 (GTK+ 2.24.23; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > > 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