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

["Brigham Campbell" <me@brighamcampbell.com>]

Hi Gero,

On Thu Jul 9, 2026 at 6:40 AM MDT, Gero Schwäricke wrote:
> I just finished reviewing your patch. Aaand I saw Wolfram beat me to it
> by 20 minutes. I hope you forgive me for sending it as is even though it
> may now contain duplicates to his findings.

Not at all! I understand that it takes a lot of effort to carefully
review changes. I appreciate your work.

> On Mon Jul 6, 2026 at 6:27 AM CEST, Brigham Campbell wrote:
> [...]
>> @@ -410,7 +406,7 @@ int parse_i2c_address(const char *address_arg, int all_addrs)
>>  	return address;
>>  }
>>  
>> -int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
>> +int open_i2c_dev_num(int i2cbus, char *filename, size_t size, int quiet)
>>  {
>>  	int file, len;
>>  
>
> The lookup function uses suffix `_by_name`, so for consistency maybe use
> `_by_nr` instead? That is what it's called in `struct i2c_adap`.

I don't have a strong opinion. Happy to change to `_by_nr`.

> This is a minimally invasive change that makes this work, but it also
> spaghettifies the code base a little. `lookup_i2c_bus()` tries to parse
> the input to a bus number or, if that doesn't work, tries to resolve it
> as a bus name. And if that fails we try to open as path. The issue is
> that `lookup_i2c_bus()` is not very descriptive. Without reading the
> 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.

You make some really good points. I'll make these changes in v5.

> I think we can also inline `open_i2c_dev_path()` and remove it's
> `fprintf()` in the failure case. We're just speculating here that it's a
> path, so printing at the end that the given input could not be found as
> bus number, bus name, or path is sufficient.

I agree that `open_i2c_dev_path()` should be inlined, but I don't think
it would be a good idea to remove its `fprintf()` altogether. How about
something like the following, which would make the error messages more
orthogonal?

	if (errno != ENOENT) {
		fprintf(stderr, "Error: Could not open file "
			"`%s': %s\n", i2cbus_arg, strerror(errno));
		if (errno == EACCES)
			fprintf(stderr, "Run as root?\n");
		return file;
	}

	fprintf(stderr, "Error: `%s' is not a bus number, name, or device file "
		"path!\n", i2cbus_arg);

If the i2cbus_arg parameter didn't appear to be a file (ENOENT), it will
print an error indicating that all three methods failed. If it did
appear to be a file but couldn't open the file for whatever reason, it
will print the error along with a suggestion to run as root if it's a
permissions issue. This behavior reflects the behavior of
`open_i2c_dev_by_nr()`.

> I don't think there's a need to expose _num() and _path(). In fact, you
> can even remove lookup_i2c_bus() as that is not used anymore by any
> tool.

I'll clean up the header and remove unnecessary header exports in v5.

-- 
Brigham Campbell
https://brighamcampbell.com