Re: [PATCH i2c-tools] i2ctransfer: add support for I2C_M_RECV_LEN

Daniel Stodden
2020-08-02T20:28:32Z

> On Aug 2, 2020, at 12:28 PM, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
>
> Strictly spoken, this is for emulating SMBus transfers, not I2C. But
> hey, we still want to test devices supporting these transfers.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> I created this to be able to play around with I2C_M_RECV_LEN and with
> SMBUS2 and SMBUS3 max block sizes. Tested with a Renesas Lager board and
> my slave-testunit hacked with SMBus block transfer support (to be
> upstreamed later).
>
> Fun fact: printing the correct length in the output took longer than
> implementing the actual functionality.
>
> tools/i2ctransfer.8 | 1 +
> tools/i2ctransfer.c | 42 +++++++++++++++++++++++++++++++-----------
> 2 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/tools/i2ctransfer.8 b/tools/i2ctransfer.8
> index d16e34e..152d20d 100644
> --- a/tools/i2ctransfer.8
> +++ b/tools/i2ctransfer.8
> @@ -91,6 +91,7 @@ specifies if the message is read or write
> .B <length_of_message>
> specifies the number of bytes read or written in this message.
> It is parsed as an unsigned 16 bit integer, but note that the Linux Kernel applies an additional upper limit (8192 as of v4.10).
> +For read messages to targets which support SMBus Block transactions, it can also be '?', then the target will determine the length.
> .TP
> .B [@address]
> specifies the 7-bit address of the chip to be accessed for this message, and is an integer.
> diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
> index 7b95d48..2b8a3fb 100644
> --- a/tools/i2ctransfer.c
> +++ b/tools/i2ctransfer.c
> @@ -45,7 +45,8 @@ static void help(void)
> "Usage: i2ctransfer [-f] [-y] [-v] [-V] [-a] I2CBUS DESC [DATA] [DESC [DATA]]...\n"
> " I2CBUS is an integer or an I2C bus name\n"
> " DESC describes the transfer in the form: {r|w}LENGTH[@address]\n"
> - " 1) read/write-flag 2) LENGTH (range 0-65535) 3) I2C address (use last one if omitted)\n"
> + " 1) read/write-flag 2) LENGTH (range 0-65535, or '?')\n"
> + " 3) I2C address (use last one if omitted)\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"
> @@ -84,17 +85,24 @@ static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
>
> for (i = 0; i < nmsgs; i++) {
> int read = msgs[i].flags & I2C_M_RD;
> + int recv_len = msgs[i].flags & I2C_M_RECV_LEN;
> int print_buf = (read && (flags & PRINT_READ_BUF)) ||
> (!read && (flags & PRINT_WRITE_BUF));
> -
> - if (flags & PRINT_HEADER)
> - fprintf(output, "msg %u: addr 0x%02x, %s, len %u",
> - i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
> + __u16 len = recv_len ? msgs[i].buf[0] + 1 : msgs[i].len;

This is post-ioctl()? (Sorry, still not familiar enough with i2c-tools.)

It isn’t wrong, but shouldn’t be necessary.
Unless the adapter driver you’re using went astray. Not ruling that out.

Before I2C_RDWR:
- msg.len is sizeof(msg.buf)
- msg.block[0] is <extra_bytes>

After I2C_RDWR:
- msg.block[0] is <len> of transfer
- msg.len is <len> + <extra_bytes> (but never > sizeof(msg.buf))

Hth,
Daniel

> +
> + if (flags & PRINT_HEADER) {
> + fprintf(output, "msg %u: addr 0x%02x, %s, len ",
> + i, msgs[i].addr, read ? "read" : "write");
> + if (!recv_len || flags & PRINT_READ_BUF)
> + fprintf(output, "%u", len);
> + else
> + fprintf(output, "TBD");
> + }
>
> if (msgs[i].len && print_buf) {
> if (flags & PRINT_HEADER)
> fprintf(output, ", buf ");
> - for (j = 0; j < msgs[i].len - 1; j++)
> + for (j = 0; j < len - 1; j++)
> fprintf(output, "0x%02x ", msgs[i].buf[j]);
> /* Print final byte with newline */
> fprintf(output, "0x%02x\n", msgs[i].buf[j]);
> @@ -192,13 +200,23 @@ int main(int argc, char *argv[])
> goto err_out_with_arg;
> }
>
> - len = strtoul(arg_ptr, &end, 0);
> - if (len > 0xffff || arg_ptr == end) {
> - fprintf(stderr, "Error: Length invalid\n");
> - goto err_out_with_arg;
> + if (*arg_ptr == '?') {
> + if (!(flags & I2C_M_RD)) {
> + fprintf(stderr, "Error: variable length not allowed with write\n");
> + goto err_out_with_arg;
> + }
> + len = 256; /* SMBUS3_MAX_BLOCK_LEN + 1 */
> + flags |= I2C_M_RECV_LEN;
> + arg_ptr++;
> + } else {
> + len = strtoul(arg_ptr, &end, 0);
> + if (len > 0xffff || arg_ptr == end) {
> + fprintf(stderr, "Error: Length invalid\n");
> + goto err_out_with_arg;
> + }
> + arg_ptr = end;
> }
>
> - arg_ptr = end;
> if (*arg_ptr) {
> if (*arg_ptr++ != '@') {
> fprintf(stderr, "Error: Unknown separator after length\n");
> @@ -237,6 +255,8 @@ int main(int argc, char *argv[])
> }
> memset(buf, 0, len);
> msgs[nmsgs].buf = buf;
> + if (flags & I2C_M_RECV_LEN)
> + buf[0] = 1; /* number of extra bytes */
> }
>
> if (flags & I2C_M_RD || len == 0) {
> --
> 2.27.0
>