Re: [i2c-tools PATCH v2] i2ctransfer: add new tool

[Wolfram Sang]

Hi Uwe,

thanks for the review!

> > +.RI [ data ]
> > +.RI [ desc
> > +.RI [ data ]]
> 
> You could join the previous two lines.

Try it. You will miss some spaces, then.

> > +Also, you cannot be interrupted by another I2C master during one transfer, but it might happen between multiple transfers.
> 
> Well, unless you loose arbitration. Maybe this is too picky to be
> relevant here?

I wonder: will another I2C master start a transfer on a repeated start?
Need to investigate.

> Also in single-master setups you can be interrupted if a driver chooses
> to start sending a transfer between two of yours. I think this is the
> more relevant reason you want to use a single transfer.

Yes, true. I updated the paragraph.

> > +	if (!(funcs & I2C_FUNC_I2C)) {
> > +		fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
> > +		return -1;
> > +	}
> 
> Do you need this check? I hope the kernel doesn't rely on userspace to
> not send a transfer the adapter doesn't support? If the kernel checks
> appropriatly it's a waste of time to duplicate the check in i2ctransfer?

Other I2C tools do it also, so I did as well for consistency reasons. I'd
think, if we fix it, we do it altogether on all tools. In a seperate
series.

> > +	fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
> 
> Does it kill kittens? :-)

I hope not! :) Again, I copied this line from other I2C tools.

> > +	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
> 
> Should this limit be described in the man page?

Good idea, done now.

> > +			switch (*arg_ptr++) {
> > +			case 'r': flags |= I2C_M_RD; break;
> 
> This doesn't match kernel coding style and I'd put it on separate lines.

It's i2c-tools coding style ;)

> > +	exit(0);
> 
> return EXIT_SUCCESS; ?

Maybe. I'd vote for a seperate series for that again, though.

> > +	for (i = 0; i <= nmsgs; i++)
> > +		free(msgs[i].buf);
> > +
> > +	exit(1);
> 
> return EXIT_FAILURE; ?
> 
> Apart from the exit code this is exactly the trailer of the good path,
> so these could share code.

No! One has '< nmsgs', the other one '<= nmsgs'. Friendly rant: It was
all easier and less subtle before Jean wanted the 'don't rely on the OS
for cleanup' additions ;)

Regards,

   Wolfram