[i2c-tools PATCH] tools: i2cbusses: Handle bus names like /dev/i2c-0

[i2c-tools PATCH] tools: i2cbusses: Handle bus names like /dev/i2c-0

[Chris Packham]

File based tab completion means it's easy to do something like
i2cdump /dev/i2c-0 0x52. Accept this method of specifying the i2c bus
device.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 tools/i2cbusses.c | 12 ++++++++++--
 tools/i2cbusses.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/i2cbusses.c b/tools/i2cbusses.c
index b4f00ae..5cb6e93 100644
--- a/tools/i2cbusses.c
+++ b/tools/i2cbusses.c
@@ -25,6 +25,7 @@
 /* For strdup and snprintf */
 #define _BSD_SOURCE 1 /* for glibc <= 2.19 */
 #define _DEFAULT_SOURCE 1 /* for glibc >= 2.19 */
+#define _GNU_SOURCE 1 /* for asprintf */
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -104,8 +105,10 @@ void free_adapters(struct i2c_adap *adapters)
 {
 	int i;
 
-	for (i = 0; adapters[i].name; i++)
+	for (i = 0; adapters[i].name; i++) {
 		free(adapters[i].name);
+		free(adapters[i].devname);
+	}
 	free(adapters);
 }
 
@@ -306,6 +309,10 @@ found:
 				free_adapters(adapters);
 				return NULL;
 			}
+			if (asprintf(&adapters[count].devname, "/dev/i2c-%d", i2cbus) < 0) {
+				free_adapters(adapters);
+				return NULL;
+			}
 			adapters[count].funcs = adap_types[type].funcs;
 			adapters[count].algo = adap_types[type].algo;
 			count++;
@@ -331,7 +338,8 @@ static int lookup_i2c_bus_by_name(const char *bus_name)
 	/* Walk the list of i2c busses, looking for the one with the
 	   right name */
 	for (i = 0; adapters[i].name; i++) {
-		if (strcmp(adapters[i].name, bus_name) == 0) {
+		if (strcmp(adapters[i].name, bus_name) == 0 ||
+		    strcmp(adapters[i].devname, bus_name) == 0) {
 			if (i2cbus >= 0) {
 				fprintf(stderr,
 					"Error: I2C bus name is not unique!\n");
diff --git a/tools/i2cbusses.h b/tools/i2cbusses.h
index a192c7f..77e1b8e 100644
--- a/tools/i2cbusses.h
+++ b/tools/i2cbusses.h
@@ -27,6 +27,7 @@
 struct i2c_adap {
 	int nr;
 	char *name;
+	char *devname;
 	const char *funcs;
 	const char *algo;
 };
-- 
2.31.1

Re: [i2c-tools PATCH] tools: i2cbusses: Handle bus names like /dev/i2c-0

[Jean Delvare]

Hi Chris,

On Tue, 25 May 2021 21:06:12 +1200, Chris Packham wrote:
> File based tab completion means it's easy to do something like
> i2cdump /dev/i2c-0 0x52. Accept this method of specifying the i2c bus
> device.

I can't really see the value of this change, sorry. You want to use a
longer parameter so you can tab-complete it. The original parameter was
a 1- or 2-digit number, which is faster to type than /d<tab>i2<tab>.
Plus if you have multiple i2c buses, tab completion can't guess which
one you want anyway, so you'll have to type the bus number eventually.

So, what do we actually win here?

-- 
Jean Delvare
SUSE L3 Support

Re: [i2c-tools PATCH] tools: i2cbusses: Handle bus names like /dev/i2c-0

[Chris Packham]

On 26/05/21 7:39 pm, Jean Delvare wrote:
> Hi Chris,
>
> On Tue, 25 May 2021 21:06:12 +1200, Chris Packham wrote:
>> File based tab completion means it's easy to do something like
>> i2cdump /dev/i2c-0 0x52. Accept this method of specifying the i2c bus
>> device.
> I can't really see the value of this change, sorry. You want to use a
> longer parameter so you can tab-complete it. The original parameter was
> a 1- or 2-digit number, which is faster to type than /d<tab>i2<tab>.
> Plus if you have multiple i2c buses, tab completion can't guess which
> one you want anyway, so you'll have to type the bus number eventually.
>
> So, what do we actually win here?

My main motivation was to replace an in-house tool that is provides 
similar functionality but it currently takes the bus as a path. At first 
I even thought there was a bug because I thought "or an I2C bus name" 
meant the path, it wasn't until I looked at the code that I realised 
this was the name used in the kernel.

One advantage I can see is that the /d<tab>/i2<tab> implicitly validates 
that the bus actually exists (assuming /dev is managed by devtmpfs 
and/or udev).

Re: [i2c-tools PATCH] tools: i2cbusses: Handle bus names like /dev/i2c-0

[Jean Delvare]

Hi Chris,

On Wed, 26 May 2021 21:23:07 +0000, Chris Packham wrote:
> On 26/05/21 7:39 pm, Jean Delvare wrote:
> > I can't really see the value of this change, sorry. You want to use a
> > longer parameter so you can tab-complete it. The original parameter was
> > a 1- or 2-digit number, which is faster to type than /d<tab>i2<tab>.
> > Plus if you have multiple i2c buses, tab completion can't guess which
> > one you want anyway, so you'll have to type the bus number eventually.
> >
> > So, what do we actually win here?  
> 
> My main motivation was to replace an in-house tool that is provides 
> similar functionality but it currently takes the bus as a path. At first 
> I even thought there was a bug because I thought "or an I2C bus name" 
> meant the path, it wasn't until I looked at the code that I realised 
> this was the name used in the kernel.

OK, that's a better explanation. But I'm still not convinced by the
benefit. I'm sure you guys can learn quickly to pass just the i2c bus
number as the first parameter. Plus I don't like your implementation
for various technical reasons anyway (like allocating extra memory for
every bus when you may never actually need it, and hard-coding the
/dev/i2c-<N> pattern when there's at least one alternative supported by
i2c-tools at the moment - although I'm unsure if anyone still uses it).
So I'm not going to apply your patch, sorry.

> One advantage I can see is that the /d<tab>/i2<tab> implicitly validates 
> that the bus actually exists (assuming /dev is managed by devtmpfs 
> and/or udev).

That's not an advantage. Running the command on the wrong I2C bus could
have bad consequences. The only safe way to use the tool without
checking the list of available i2c buses first is to select the I2C bus
by name.

-- 
Jean Delvare
SUSE L3 Support

Re: [i2c-tools PATCH] tools: i2cbusses: Handle bus names like /dev/i2c-0

[Chris Packham]

On 2/06/21 7:25 pm, Jean Delvare wrote:
> Hi Chris,
>
> On Wed, 26 May 2021 21:23:07 +0000, Chris Packham wrote:
>> On 26/05/21 7:39 pm, Jean Delvare wrote:
>>> I can't really see the value of this change, sorry. You want to use a
>>> longer parameter so you can tab-complete it. The original parameter was
>>> a 1- or 2-digit number, which is faster to type than /d<tab>i2<tab>.
>>> Plus if you have multiple i2c buses, tab completion can't guess which
>>> one you want anyway, so you'll have to type the bus number eventually.
>>>
>>> So, what do we actually win here?
>> My main motivation was to replace an in-house tool that is provides
>> similar functionality but it currently takes the bus as a path. At first
>> I even thought there was a bug because I thought "or an I2C bus name"
>> meant the path, it wasn't until I looked at the code that I realised
>> this was the name used in the kernel.
> OK, that's a better explanation. But I'm still not convinced by the
> benefit. I'm sure you guys can learn quickly to pass just the i2c bus
> number as the first parameter. Plus I don't like your implementation
> for various technical reasons anyway (like allocating extra memory for
> every bus when you may never actually need it, and hard-coding the
> /dev/i2c-<N> pattern when there's at least one alternative supported by
> i2c-tools at the moment - although I'm unsure if anyone still uses it).
> So I'm not going to apply your patch, sorry.

Yeah I had a few goes at implementing this. It seemed the simplest 
despite the memory use (which I thought probably wouldn't be a problem 
given the fact these tools run an then stop). I also thought about 
passing the argument down to the underlying open() if parsing as a 
number or name failed but again that would involve a fair bit of churn.

>> One advantage I can see is that the /d<tab>/i2<tab> implicitly validates
>> that the bus actually exists (assuming /dev is managed by devtmpfs
>> and/or udev).
> That's not an advantage. Running the command on the wrong I2C bus could
> have bad consequences. The only safe way to use the tool without
> checking the list of available i2c buses first is to select the I2C bus
> by name.

Yes I see your point. Mostly we use these tools for debugging broken 
systems anyway so "bad consequences" have already happened.

Sounds like some re-training of fingers is the best approach.