i2c(8) diagnostic tool for review
Stanislav Sedov
stas at FreeBSD.org
Sat Dec 6 06:45:58 PST 2008
On Fri, 05 Dec 2008 16:08:44 +0100
Rafal Jaworowski <raj at 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
More information about the freebsd-embedded
mailing list