[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