Re: [PATCH i2c-tools v2 2/2] i2ctransfer: Add optional message modifier flags

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

> --- a/tools/i2ctransfer.8
> +++ b/tools/i2ctransfer.8
> @@ -96,8 +96,11 @@ The number of blocks is limited by the Linux Kernel and defined by I2C_RDWR_IOCT
>  .I desc
>  blocks are composed like this:
>  
> -.I {r|w}<length_of_message>[@address]
> +.I [inpst]{r|w}<length_of_message>[@address]
>  
> +.TP
> +.B [inpst]
> +specifies optional MESSAGE MODIFIER FLAGS. See the section below for details.

The documentation needs just a slight update like "if supported". It
depends not only on the Linux version but the controller driver anyhow.

> +++ b/tools/i2ctransfer.c
> @@ -52,9 +52,16 @@ static void help(void)
>  		"           -V version info\n"
>  		"           -y yes to all confirmations\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, or '?')\n"
> -		"    3) I2C address (use last one if omitted)\n"
> +		"  DESC describes the transfer in the form: [inpst]{r|w}LENGTH[@address]\n"
> +		"    1) optional message modifier flags\n"
> +		"       i: ignore NACK from client\n"
> +		"       n: no master ACK/NACK bit in a read message\n"
> +		"       p: emit a STOP after the message\n"
> +		"       s: skip repeated start\n"
> +		"       t: toggle read/write bit\n"
> +		"    2) mandatory read/write flag\n"
> +		"    3) LENGTH (range 0-65535, or '?')\n"
> +		"    4) I2C address (use last one if omitted)\n"

Same here. I think the help text can be static by saying "if supported"

>  		"  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"
> @@ -202,12 +209,21 @@ int main(int argc, char *argv[])
>  		case PARSE_GET_DESC:
>  			flags = 0;
>  
> -			switch (*arg_ptr++) {
> -			case 'r': flags |= I2C_M_RD; break;
> -			case 'w': break;
> -			default:
> -				fprintf(stderr, "Error: Invalid direction\n");
> -				goto err_out_with_arg;
> +			for (int done = 0; !done; ) {
> +				switch (*arg_ptr++) {
> +				/* optional flags */
> +				case 'i': flags |= I2C_M_IGNORE_NAK; break;
> +				case 'n': flags |= I2C_M_NO_RD_ACK; break;
> +				case 'p': flags |= I2C_M_STOP; break;
> +				case 's': flags |= I2C_M_NOSTART; break;
> +				case 't': flags |= I2C_M_REV_DIR_ADDR; break;

Brainstorming here: maybe a macro could help:

	case 'i': add_flag_if_supported(flags, I2C_M_IGNORE_NAK);

?

Thanks and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]