Date: Sat, 6 Dec 2008 17:18:29 +0300 From: Stanislav Sedov <stas@FreeBSD.org> To: Rafal Jaworowski <raj@semihalf.com> Cc: embedded@freebsd.org Subject: Re: i2c(8) diagnostic tool for review Message-ID: <20081206171829.82f0618a.stas@FreeBSD.org> In-Reply-To: <493943FC.8080001@semihalf.com> References: <493943FC.8080001@semihalf.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 05 Dec 2008 16:08:44 +0100 Rafal Jaworowski <raj@semihalf.com> mentioned: > This nice little program is helpful with inspecting an I2C bus, when bringing > up a new system, or just for diagnostic purposes: > http://people.freebsd.org/~raj/patches/misc/i2c.diff > > Note the patch extends the /dev/iicX interface with a ioctl for the 'repeated > start' method. > > More detailed description of the tool is in the manual page: > http://people.freebsd.org/~raj/patches/misc/i2c-man.txt > > Any comments welcome. > Great! I haven't tried the tool itself yet, but there're some comments for the source itself. Hopefully, it'll be useful. > +The options are as follows: > +.Bl -tag -width ".Fl d Ar direction" > +.It Fl a Ar address > +7-bit address on the I2C device to operate on (hex). > +.It Fl f Ar device > +I2C bus to use (default is /dev/iic0). > +.It Fl d Ar r|w > +transfer direction: r - read, w - write. > +.It Fl w Ar 0|8|16 > +device addressing width (in bits). > +.It Fl o Ar offset > +offset within the device for data transfer (hex). > +.It Fl c Ar count > +number of bytes to transfer (dec). > +.It Fl m Ar ss|rs|no > +addressing mode, i.e., I2C bus operations performed after the offset for the > +transfer has been written to the device and before the actual read/write > +operation. rs - repeated start; ss - stop start; no - none. > +.It Fl v > +be verbose I think options here should come sorted if there're no specific needs. At least -v in the middle looks weird. > +.if ${MACHINE_ARCH} == "arm" > +_i2c= i2c > +.endif It there any specific reason the utility is limited to arm? I2C interface is generic enough, and I think the utility will be useful for other platforms as well. > +#define I2C_DEV "/dev/iic0" > +#define I2C_MODE_NOTSET 0 > +#define I2C_MODE_NONE 1 > +#define I2C_MODE_STOP_START 2 > +#define I2C_MODE_REPEATED_START 3 #define and name should delimited by tab according to style(9) > +static void > +usage(void) > +{ > + > + fprintf(stderr, "usage: %s -a addr [-f device] [-d [r|w]] [-o offset] " > + "[-w [0|8|16]] [-c count] [-m [ss|rs|no]] [-b] [-v]\n", > + getprogname()); > + fprintf(stderr, " %s -s [-f device] [-n skip_addr] -v\n", > + getprogname()); > + fprintf(stderr, " %s -r [-f device] -v\n", getprogname()); > + exit(1); > +} You're using sysexit codes everywhere, I suppose the exit code here should be EX_USAGE too? At least, it looks inconsistent to other parts of the code. Also it might make sense to mark the function as __dead2 as it never returns. This will enable a number of possible optimisations to gcc. > + token = strsep(&skip_addr, ".."); > + if (token) > + addr_range.end = strtoul(token, 0, 16); > + } I think there's a check missing around strtoul? > + if (token == NULL) > + break; > + sk_addr[i] = strtoul(token, 0, 16); > + } The same as above. > + tokens = (int *)malloc((len / 2 + 1) * sizeof(int)); > + index = skip_get_tokens(skip_addr, tokens, > + len / 2 + 1); > + } The check around malloc is missing. Also, len should be checked to be non-zero. > +prepare_buf(int size, uint32_t off) > +{ > + char *buf; > + > + buf = malloc(size); > + if (buf == NULL) > + return (buf); Consider adding a check around size. > +static int > +i2c_write(char *dev, struct options i2c_opt, char *i2c_buf) > +{ > + struct iiccmd cmd; > + int ch, i, error, fd, bufsize; > + char *err_msg, *buf; > + > + /* > + * Read data to be written to the chip from stdin > + */ > + if (i2c_opt.verbose && !i2c_opt.binary) > + fprintf(stderr, "Enter %u bytes of data: ", i2c_opt.count); > + > + for (i = 0; i < i2c_opt.count; i++) { > + ch = getchar(); > + if (ch == EOF) { > + free(i2c_buf); > + err(1, "not enough data, exiting\n"); > + } > + i2c_buf[i] = ch; > + } > + > + fd = open(dev, O_RDWR); > + if (fd == -1) { > + free(i2c_buf); > + err(1, "open failed"); > + } > + > + /* > + * Write offset where the data will go > + */ > + cmd.slave = i2c_opt.addr; > + error = ioctl(fd, I2CSTART, &cmd); > + if (error == -1) { > + err_msg = "ioctl: error sending start condition"; > + goto err1; > + } > + > + if (i2c_opt.width) { > + bufsize = i2c_opt.width / 8; > + buf = prepare_buf(bufsize, i2c_opt.off); > + if (buf == NULL) { > + err_msg = "error: offset malloc"; > + goto err1; > + } > + > + cmd.count = bufsize; > + cmd.buf = buf; > + error = ioctl(fd, I2CWRITE, &cmd); > + free(buf); > + if (error == -1) { > + err_msg = "ioctl: error when write offset"; > + goto err1; > + } > + } You're freeing i2c_buf on every exit, but not here. Why? Probably, it'll be better to move the buffer freeing duty to the calling function as the i2c_buf comes as an argument. It'll also help to eliminate a lot of code here. There're also a lot of places where buffer is not freed later in this function. > + i2c_opt.addr = (strtoul(optarg, 0, 16) << 1); > + i2c_opt.addr_set = 1; > + break; > + case 'f': > + dev = optarg; > + break; > + case 'd': > + i2c_opt.dir = optarg[0]; > + break; > + case 'o': > + i2c_opt.off = strtoul(optarg, 0, 16); > + break; > + case 'w': > + i2c_opt.width = atoi(optarg); > + break; > + case 'c': > + i2c_opt.count = atoi(optarg); > + break; > + case 'm': I think the checks around strtoul and atoi should be added to properly inform user about bad arguments passed, instead of just silently ignoring junk. > + if (i2c_opt.scan) { > + if (i2c_opt.addr_set) > + usage(); > + } else if (i2c_opt.reset) { > + if (i2c_opt.addr_set) > + usage(); > + } else if ((i2c_opt.dir == 'r' || i2c_opt.dir == 'w')) { > + if ((i2c_opt.addr_set == 0) || > + !(i2c_opt.width == 0 || i2c_opt.width == 8 || > + i2c_opt.width == 16)) > + usage(); > + } else > + usage(); I think this code could be simplifed, e.g. checks could be combined into a single if, like else if (i2c_opt.reset && i2c_opt.addr_set) an eliminating the default usage() case. There're also a number of minor spelling errors and extra spaces, I can look for them if needed. Thanks for a good work! -- Stanislav Sedov ST4096-RIPE
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081206171829.82f0618a.stas>