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

["Gero Schwäricke" <gero.schwaericke@sevenlab.de>]

Hi Brigham,

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.

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

> @@ -446,6 +442,38 @@ int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
>  	return file;
>  }
>  
> +int open_i2c_dev_path(const char *i2cbus_arg, int quiet)
> +{
> +	int file = open(i2cbus_arg, O_RDWR);
> +
> +	if (file < 0 && !quiet) {
> +		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;
> +}
> +
> +int open_i2c_dev(char *i2cbus_arg, char **filename, size_t size, int quiet)
> +{
> +	int file, i2cbus;
> +
> +	i2cbus = lookup_i2c_bus(i2cbus_arg);
> +
> +	if (i2cbus < 0) {
> +		*filename = i2cbus_arg;
> +		file = open_i2c_dev_path(i2cbus_arg, quiet);
> +		if (file < 0)
> +			fprintf(stderr, "Failed to open `%s' as a bus name "
> +					"and as a path.\n", i2cbus_arg);
> +		return file;
> +	}
> +
> +	return open_i2c_dev_num(i2cbus, *filename, size, quiet);
> +}
> +

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.

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.

>  int set_slave_addr(int file, int address, int force)
>  {
>  	/* With force, let the user read from/write to the registers
> diff --git a/tools/i2cbusses.h b/tools/i2cbusses.h
> index 6f901b6..5de3056 100644
> --- a/tools/i2cbusses.h
> +++ b/tools/i2cbusses.h
> @@ -31,7 +31,9 @@ void free_adapters(struct i2c_adap *adapters);
>  
>  int lookup_i2c_bus(const char *i2cbus_arg);
>  int parse_i2c_address(const char *address_arg, int all_addrs);
> -int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet);
> +int open_i2c_dev(char *i2cbus_arg, char **filename, size_t size, int quiet);
> +int open_i2c_dev_num(int i2cbus, char *filename, size_t size, int quiet);
> +int open_i2c_dev_path(const char *i2cbus_arg, int quiet);
>  int set_slave_addr(int file, int address, int force);
>  
>  #define MISSING_FUNC_FMT	"Error: Adapter does not have %s capability\n"

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.

> diff --git a/tools/i2cdetect.c b/tools/i2cdetect.c
> index 3c22f17..b62f395 100644
> --- a/tools/i2cdetect.c
> +++ b/tools/i2cdetect.c
> @@ -213,8 +213,9 @@ static void print_i2c_busses(void)
>  int main(int argc, char *argv[])
>  {
>  	char *end;
> -	int i2cbus, file, res;
> -	char filename[20];
> +	int file, res;
> +	char filename_buf[20];
> +	char *filename = filename_buf;
>  	unsigned long funcs;
>  	int mode = MODE_AUTO;
>  	int first = 0x08, last = 0x77;
> @@ -277,11 +278,6 @@ int main(int argc, char *argv[])
>  		help();
>  		exit(1);
>  	}
> -	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.

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

Best,
Gero

[...] the rest is tool changes similar to i2cdetect

-- 


sevenlab engineering GmbH <https://sevenlab.de>
serious engineering.

Geschäftsführer: Christian J. Pereira
Amtsgericht Köln, HRB 121730
Anschrift: Hansaring 20, 50670 Köln