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

[Uwe Kleine-König]

Hello,

On Mon, Mar 13, 2017 at 12:01:40PM +0100, Wolfram Sang wrote:
> 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.

It works fine with quoting:

	.RI [ "desc " [ data "]] ..."

. But ok, it's at least arguable if this is better than your solution.

> > > +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.

I'd say it must not. It should have logic to detect bus busy and delay
any transfers until the bus becomes idle. With a repeated start the bus
doesn't become idle between two transfers.

> > > +	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 ;)

ah, ok. BTW, I like tools that clean up after themselves. This way
debugging is much easier if you look for lost memory. And yes, this
doesn't matter much for short-living programs like i2c-tools, but I like
being consistent here and also do the cleanup for this this type of
program.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |