[PATCH] i2c_tools.c: add i2ctransfer utility
Nikolaus Voss
nv at vosn.de
Mon Jan 7 11:32:50 UTC 2019
Hi Xabier,
many thanks for reviewing the patch.
On Mon, 17 Dec 2018, Xabier Oneca -- xOneca wrote:
> Hi Nikolaus,
>
> Some comments about your patch follow:
>
>> i2ctransfer sends and receives user defined i2c messages
>> ---
>> miscutils/i2c_tools.c | 216 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 215 insertions(+), 1 deletion(-)
>>
>> diff --git a/miscutils/i2c_tools.c b/miscutils/i2c_tools.c
>> index 6a2134063..76f128e6d 100644
>> --- a/miscutils/i2c_tools.c
>> +++ b/miscutils/i2c_tools.c
>> @@ -36,17 +36,26 @@
>> //config: help
>> //config: Detect I2C chips.
>> //config:
>> +//config:config I2CTRANSFER
>> +//config: bool "i2ctransfer (4.0 kb)"
>> +//config: default y
>> +//config: select PLATFORM_LINUX
>> +//config: help
>> +//config: Send user-defined I2C messages in one transfer.
>> +//config:
>>
>> //applet:IF_I2CGET(APPLET(i2cget, BB_DIR_USR_SBIN, BB_SUID_DROP))
>> //applet:IF_I2CSET(APPLET(i2cset, BB_DIR_USR_SBIN, BB_SUID_DROP))
>> //applet:IF_I2CDUMP(APPLET(i2cdump, BB_DIR_USR_SBIN, BB_SUID_DROP))
>> //applet:IF_I2CDETECT(APPLET(i2cdetect, BB_DIR_USR_SBIN, BB_SUID_DROP))
>> +//applet:IF_I2CTRANSFER(APPLET(i2ctransfer, BB_DIR_USR_SBIN, BB_SUID_DROP))
>> /* not NOEXEC: if hw operation stalls, use less memory in "hung" process */
>>
>> //kbuild:lib-$(CONFIG_I2CGET) += i2c_tools.o
>> //kbuild:lib-$(CONFIG_I2CSET) += i2c_tools.o
>> //kbuild:lib-$(CONFIG_I2CDUMP) += i2c_tools.o
>> //kbuild:lib-$(CONFIG_I2CDETECT) += i2c_tools.o
>> +//kbuild:lib-$(CONFIG_I2CTRANSFER) += i2c_tools.o
>>
>> /*
>> * Unsupported stuff:
>> @@ -80,12 +89,18 @@
>> #define I2C_FUNCS 0x0705
>> #define I2C_PEC 0x0708
>> #define I2C_SMBUS 0x0720
>> +#define I2C_RDWR 0x0707
>> +#define I2C_RDWR_IOCTL_MAX_MSGS 42
>> struct i2c_smbus_ioctl_data {
>> __u8 read_write;
>> __u8 command;
>> __u32 size;
>> union i2c_smbus_data *data;
>> };
>> +struct i2c_rdwr_ioctl_data {
>> + struct i2c_msg *msgs; /* pointers to i2c_msgs */
>> + __u32 nmsgs; /* number of i2c_msgs */
>> +};
>> /* end linux/i2c-dev.h */
>>
>> /*
>> @@ -262,7 +277,7 @@ static int i2c_bus_lookup(const char *bus_str)
>> return xstrtou_range(bus_str, 10, 0, 0xfffff);
>> }
>>
>> -#if ENABLE_I2CGET || ENABLE_I2CSET || ENABLE_I2CDUMP
>> +#if ENABLE_I2CGET || ENABLE_I2CSET || ENABLE_I2CDUMP || ENABLE_I2CTRANSFER
>> static int i2c_parse_bus_addr(const char *addr_str)
>> {
>> /* Slave address must be in range 0x03 - 0x77. */
>> @@ -1373,3 +1388,202 @@ int i2cdetect_main(int argc UNUSED_PARAM, char **argv)
>> return 0;
>> }
>> #endif /* ENABLE_I2CDETECT */
>> +
>> +#if ENABLE_I2CTRANSFER
>> +static void check_i2c_func(int fd)
>> +{
>> + unsigned long funcs;
>> +
>> + get_funcs_matrix(fd, &funcs);
>> +
>> + if (!(funcs & I2C_FUNC_I2C))
>> + bb_error_msg_and_die("warning: adapter does not support I2C transfers");
>> +}
>> +
>> +//usage:#define i2ctransfer_trivial_usage
>> +//usage: "[-f] [-y] I2CBUS DESC [DATA] [DESC [DATA]]..."
>> +//usage:#define i2ctransfer_full_usage "\n\n"
>> +//usage: "Send user-defined I2C messages in one transfer"
>> +//usage: "\n"
>> +//usage: "\nI2CBUS I2C bus number"
>> +//usage: "\nDESC describes the transfer in the form: {r|w}LENGTH[@address]"
>> +int i2ctransfer_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
>> +int i2ctransfer_main(int argc UNUSED_PARAM, char **argv)
>> +{
>> + const unsigned opt_f = (1 << 0), opt_y = (1 << 1);
>> +
>> + int bus_num, bus_addr = - 1;
>> + int fd;
>> + unsigned opts;
>> + int nmsgs = 0, nmsgs_sent, i, j;
>> + struct i2c_msg msgs[I2C_RDWR_IOCTL_MAX_MSGS];
>> + unsigned buf_idx = 0;
>> + struct i2c_rdwr_ioctl_data rdwr;
>> + enum parse_state {
>> + PARSE_GET_DESC,
>> + PARSE_GET_DATA,
>> + } state = PARSE_GET_DESC;
>> +
>> + for (i = 0; i < I2C_RDWR_IOCTL_MAX_MSGS; i++)
>> + msgs[i].buf = NULL;
>> +
>> + opts = getopt32(argv, "^"
>> + "fy"
>> + "\0" "-2" /* minimum 2 args */
>> + );
>> + argv += optind;
>> + argc -= optind;
>> + argc--; /* now argv[argc] is last arg */
>
> argc is not used. No need to fix its value.
Ok, fixed.
>
>> +
>> + bus_num = i2c_bus_lookup(argv[0]);
>> + fd = i2c_dev_open(bus_num);
>> + check_i2c_func(fd);
>> +
>> + while (*++argv) {
>> + char *arg_ptr = *argv;
>> + unsigned long len, raw_data;
>> + __u16 flags;
>> + __u8 data, *buf;
>> + char *end;
>> +
>> + if (nmsgs > I2C_RDWR_IOCTL_MAX_MSGS)
>> + bb_error_msg_and_die("Error: Too many messages (max: %d)\n",
>> + I2C_RDWR_IOCTL_MAX_MSGS);
>
> Off-by-one error? Seems you meant to use >=.
The code is actually correct, as nmsgs is incremented below after
writing to msgs[nmsgs] and nmsgs == I2C_RDWR_IOCTL_MAX_MSGS is valid.
>
>> +
>> + switch (state) {
>> + case PARSE_GET_DESC:
>> + flags = 0;
>> +
>> + switch (*arg_ptr++) {
>> + case 'r': flags |= I2C_M_RD; break;
>> + case 'w': break;
>> + default:
>> + bb_error_msg_and_die("Error: Invalid direction: %s\n",
>> + *argv);
>
> Maybe bb_show_usage() here?
Yes, applied.
>
>> + }
>> +
>> + len = strtoul(arg_ptr, &end, 0);
>> + if (len > 0xffff || arg_ptr == end)
>> + bb_error_msg_and_die("Error: Length invalid: %s\n", *argv);
>> +
>> + arg_ptr = end;
>> + if (*arg_ptr) {
>> + if (*arg_ptr++ != '@')
>> + bb_error_msg_and_die("Error: Unknown separator after length: %s\n",
>> + *argv);
>> +
>> + /* We skip 10-bit support for now. If we want it,
>> + * it should be marked with a 't' flag before
>> + * the address here.
>> + */
>> +
>> + if (!(opts & opt_f)) {
>> + bus_addr = i2c_parse_bus_addr(arg_ptr);
>> + i2c_set_slave_addr(fd, bus_addr, opts & opt_f);
>
> Here opts & opt_f == 0 always.
Ok.
>
>> + } else {
>> + /* 'force' allows whole address range */
>
> Not sure about this statement. It seems to be a '-a' for this:
> https://manpages.debian.org/unstable/i2c-tools/i2ctransfer.8.en.html#OPTIONS
> It seems none of the I2C functions of Busybox but i2cdetect support -a.
My version of i2ctransfer for i2c-tools does not (yet) support -a, see
https://manpages.debian.org/testing/i2c-tools/i2ctransfer.8.en.html#OPTIONS
but I think it is a good extension, so I added support for it.
>
>> + bus_addr = strtol(arg_ptr, &end, 0);
>
> Can xstrtou_range(arg_ptr, 0, 0, 0x7f) be used here?
Yes, applied.
>
>> + if (arg_ptr == end || *end || bus_addr > 0x7f)
>> + bb_error_msg_and_die("Error: Invalid chip address: %s\n",
>> + *argv);
>> + }
>> + } else {
>> + /* Reuse last address if possible */
>> + if (bus_addr < 0)
>> + bb_error_msg_and_die("Error: No address given: %s\n",
>> + *argv);
>> + }
>> +
>> + msgs[nmsgs].addr = bus_addr;
>> + msgs[nmsgs].flags = flags;
>> + msgs[nmsgs].len = len;
>> +
>> + if (len) {
>> + buf = xmalloc(len);
>> + if (!buf)
>> + bb_error_msg_and_die("Error: No memory for buffer: %s\n", *argv);
>
> xmalloc dies on error. No need to check buf.
>
>> + memset(buf, 0, len);
>
> xmalloc + memset = xzalloc
Ok, applied.
>
>> + msgs[nmsgs].buf = buf;
>> + }
>> +
>> + if (flags & I2C_M_RD || len == 0) {
>> + nmsgs++;
>> + } else {
>> + buf_idx = 0;
>> + state = PARSE_GET_DATA;
>> + }
>> + break;
>> +
>> + case PARSE_GET_DATA:
>> + raw_data = strtoul(arg_ptr, &end, 0);
>> + if (raw_data > 0xff || arg_ptr == end)
>> + bb_error_msg_and_die("Error: Invalid data byte: %s\n", *argv);
>> +
>> + data = raw_data;
>> + len = msgs[nmsgs].len;
>> +
>> + while (buf_idx < len) {
>> + msgs[nmsgs].buf[buf_idx++] = data;
>> +
>> + if (!*end)
>> + break;
>> +
>> + switch (*end) {
>> + /* Pseudo randomness (8 bit AXR with a=13 and b=27) */
>> + case 'p':
>> + data = (data ^ 27) + 13;
>> + data = (data << 1) | (data >> 7);
>> + break;
>> + case '+': data++; break;
>> + case '-': data--; break;
>> + case '=': break;
>> + default:
>> + bb_error_msg_and_die("Error: Invalid data byte suffix: %s\n",
>> + *argv);
>> + }
>> + }
>> +
>> + if (buf_idx == len) {
>> + nmsgs++;
>> + state = PARSE_GET_DESC;
>> + }
>> +
>> + break;
>> +
>> + default:
>> + /* Should never happen */
>> + bb_error_msg_and_die("Internal Error: Unknown state in state machine!\n");
>> + }
>> + }
>> +
>> + if (state != PARSE_GET_DESC || nmsgs == 0)
>> + bb_error_msg_and_die("Error: Incomplete message\n");
>> +
>> + if (!(opts & opt_y))
>> + confirm_action(bus_addr, 0, 0, 0);
>> +
>> + rdwr.msgs = msgs;
>> + rdwr.nmsgs = nmsgs;
>> + nmsgs_sent = ioctl(fd, I2C_RDWR, &rdwr);
>> + if (nmsgs_sent < 0)
>> + bb_error_msg("Error: Sending messages failed: %s\n", strerror(errno));
>> + else if (nmsgs_sent < nmsgs)
>> + bb_error_msg("Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
>> +
>> + for (i = 0; i < nmsgs_sent; i++) {
>> + if (msgs[i].len && msgs[i].flags & I2C_M_RD) {
>> + for (j = 0; j < msgs[i].len - 1; j++)
>> + printf("0x%02x ", msgs[i].buf[j]);
>> + /* Print final byte with newline */
>> + printf("0x%02x\n", msgs[i].buf[j]);
>> + }
>> + }
>> +
>> + close(fd);
>> +
>> + for (i = 0; i < nmsgs; i++)
>> + free(msgs[i].buf);
>> +
>> + return 0;
>> +}
>> +#endif /* ENABLE_I2CTRANSFER */
>> --
>> 2.17.1
>
> Cheers,
>
> Xabier Oneca_,,_
>
I will post a fixed version.
Nikolaus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3493 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20190107/8d64ac9a/attachment-0001.p7s>
More information about the busybox
mailing list