| Message ID | 20210525090612.26157-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | [i2c-tools] tools: i2cbusses: Handle bus names like /dev/i2c-0 | expand |
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
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).
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
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.
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; };
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(-)