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

[Jean Delvare <jdelvare@suse.de>]

Hi Benoît,

On Fri, 16 Jan 2026 14:01:57 +0100, Benoît Monin wrote:
> On Tuesday, 13 January 2026 at 18:33:53 CET, Wolfram Sang wrote:
> [...]
> > >  		"  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);
> > 
> > ?
> >   
> I am having a hard time coming up with something here. There is the
> __is_defined() macro in the kernel source[1], it only returns 0 or 1, not
> the value of the define.
> 
> Maybe defining as zero the flags that are undefined would be acceptable?
> E.g.
> 
> #ifndef I2C_M_IGNORE_NAK
> #define I2C_M_IGNORE_NAK 0
> #endif

I don't think that's a good idea, because then you would simply ignore
the flag that was passed by the user if it's not supported? If so, this
is bad user experience. If the user asks for something unsupported, the
tool should report the lack of support, and fail.

> Or you have another idea?

I honestly think that a bunch of ifdefs in this part of the code would
work just fine and be acceptable. We don't plan to add more flags in
the future, and I think that's better than preprocessor magic which may
work or break depending on the compiler.

Something like:

				/* optional flags */
				case 'i':
#ifdef I2C_M_IGNORE_NAK
					flags |= I2C_M_IGNORE_NAK;
#else
					fprintf(stderr, "Error: Unsupported flag '%c'\n", 'i');
#endif
					break;
				case 'n':
#ifdef I2C_M_NO_RD_ACK
					flags |= I2C_M_NO_RD_ACK;
#else
					fprintf(stderr, "Error: Unsupported flag '%c'\n", 'n');
#endif
					break;
				case 'p':
#ifdef I2C_M_STOP
					flags |= I2C_M_STOP;
#else
					fprintf(stderr, "Error: Unsupported flag '%c'\n", 'p');
#endif
					break;
				case 's':
#ifdef I2C_M_NOSTART
					flags |= I2C_M_NOSTART;
#else
					fprintf(stderr, "Error: Unsupported flag '%c'\n", 's');
#endif
					break;
				case 't':
#ifdef I2C_M_REV_DIR_ADDR
					flags |= I2C_M_REV_DIR_ADDR;
#else
					fprintf(stderr, "Error: Unsupported flag '%c'\n", 't');
#endif
					break;


That being said, I think you also need a second level of support check.
The above only checks if the flags are defined in the kernel headers,
which should generally correspond to the flags being supported by the
kernel. But just because the flags are known to the kernel, doesn't
mean that the i2c adapter you are operating on, supports protocol
mangling.

So I believe that you also need to extend check_funcs() to return the
supported functionality, and then check for I2C_FUNC_PROTOCOL_MANGLING
and I2C_FUNC_NOSTART after parsing the optional flags. If mangling
flags are passed by the user but the adapter's driver does not support
protocol mangling, the tool should report the incompatibility and stop.

-- 
Jean Delvare
SUSE L3 Support