Re: [PATCH v4 1/2] i2c-tools: Allow passing device file paths

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

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


> code it's not understandable what's happening. Instead I would propose
> to inline `lookup_i2c_bus()` and then have the logic inside
> `open_i2c_dev()` be imparative like:
> 
> 1. try to convert input to i2cbus bus number, if it worked
>    `return open_i2c_dev_by_nr()` with the bus number, otherwise,
> 2. try `lookup_i2c_bus_by_name()`, if it worked we now have the bus
>    number, so we `return open_i2c_dev_by_nr()` with it, otherwise,
> 3. try to open as path.

I like this approach.

> > -	i2cbus = lookup_i2c_bus(argv[optind]);
> > -	if (i2cbus < 0) {
> > -		help();
> > -		exit(1);
> > -	}
> >  
> >  	/* read address range if present */
> >  	if (argc == optind + 3 && mode != MODE_FUNC) {
> > @@ -321,7 +317,7 @@ int main(int argc, char *argv[])
> >  		exit(1);
> >  	}
> >  
> > -	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
> > +	file = open_i2c_dev(argv[optind], &filename, sizeof(filename_buf), 0);
> >  	if (file < 0) {
> >  		exit(1);
> >  	}
> 
> Removing this will change the error path, so a call that would
> previously fail in lookup_i2c_bus() may now fail somewhere else,
> changing error code and message. Same for all other tools.

You mean that help() is not printed in some cases then? The error code
is 1 in all cases, or am I missing something?

> @Jean Is that acceptable or against the targeted stability requirements
> of i2c-tools?

I am also curious about Jean's opinion here. In my book it is okay. If
not, we could also release the new version as 5.0, I'd say. Would even
match adding the DDR5 support nicely ;)


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