* [PATCH] HID: wacom: Improve generic name generation @ 2017-03-28 21:15 Jason Gerecke 2017-03-29 7:30 ` Benjamin Tissoires 0 siblings, 1 reply; 6+ messages in thread From: Jason Gerecke @ 2017-03-28 21:15 UTC (permalink / raw) To: linux-input, Jiri Kosina Cc: Benjamin Tissoires, Ping Cheng, Aaron Skomra, Jason Gerecke, Jason Gerecke The 'wacom_update_name' function is responsible for producing names for the input device nodes based on the hardware device name. Commit f2209d4 added the ability to strip off prefixes like "Wacom Co.,Ltd." where the prefix was immediately (and redundantly) followed by "Wacom". The 2nd-generation Intuos Pro 2 has such a prefix, but with a small error (the period and comma are swapped) that prevents the existing code from matching it. We're loath to extend the number of cases out endlessly and so instead try to be smarter about name generation. We observe that the cause of the redundant prefixes is HID combining the manufacturer and product strings of USB devices together. By using the original product name (with "Wacom" prefixed, if it does not already exist in the string) we can bypass the gyrations to find and remove redundant prefixes. For devices connected by other busses, the problem either doesn't exist (e.g. BUS_BLUETOOTH) or the name should be replaced with a generic one entirely (e.g. BUS_I2C, BUS_INTEL_ISHTP). Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> --- drivers/hid/wacom_sys.c | 65 +++++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 037b9c04745a..ad5d8722fa84 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -2026,41 +2026,42 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix) /* Generic devices name unspecified */ if ((features->type == HID_GENERIC) && !strcmp("Wacom HID", features->name)) { - if (strstr(wacom->hdev->name, "Wacom") || - strstr(wacom->hdev->name, "wacom") || - strstr(wacom->hdev->name, "WACOM")) { - /* name is in HID descriptor, use it */ - strlcpy(name, wacom->hdev->name, sizeof(name)); - - /* strip out excess whitespaces */ - while (1) { - char *gap = strstr(name, " "); - if (gap == NULL) - break; - /* shift everything including the terminator */ - memmove(gap, gap+1, strlen(gap)); - } + char *product_name = NULL; - /* strip off excessive prefixing */ - if (strstr(name, "Wacom Co.,Ltd. Wacom ") == name) { - int n = strlen(name); - int x = strlen("Wacom Co.,Ltd. "); - memmove(name, name+x, n-x+1); - } - if (strstr(name, "Wacom Co., Ltd. Wacom ") == name) { - int n = strlen(name); - int x = strlen("Wacom Co., Ltd. "); - memmove(name, name+x, n-x+1); - } + if (wacom->hdev->bus == BUS_USB) { + struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); + struct usb_device *dev = interface_to_usbdev(intf); + product_name = dev->product; + } + else if (wacom->hdev->bus == BUS_BLUETOOTH) { + product_name = wacom->hdev->name; + } - /* get rid of trailing whitespace */ - if (name[strlen(name)-1] == ' ') - name[strlen(name)-1] = '\0'; - } else { - /* no meaningful name retrieved. use product ID */ - snprintf(name, sizeof(name), - "%s %X", features->name, wacom->hdev->product); + if (!product_name) { + snprintf(name, sizeof(name), "%s %X", + features->name, wacom->hdev->product); } + else if (strstr(product_name, "Wacom") || + strstr(product_name, "wacom") || + strstr(product_name, "WACOM")) { + strlcpy(name, product_name, sizeof(name)); + } + else { + snprintf(name, sizeof(name), "Wacom %s", product_name); + } + + /* strip out excess whitespaces */ + while (1) { + char *gap = strstr(name, " "); + if (gap == NULL) + break; + /* shift everything including the terminator */ + memmove(gap, gap+1, strlen(gap)); + } + + /* get rid of trailing whitespace */ + if (name[strlen(name)-1] == ' ') + name[strlen(name)-1] = '\0'; } else { strlcpy(name, features->name, sizeof(name)); } -- 2.12.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: wacom: Improve generic name generation 2017-03-28 21:15 [PATCH] HID: wacom: Improve generic name generation Jason Gerecke @ 2017-03-29 7:30 ` Benjamin Tissoires 2017-03-31 16:17 ` Jason Gerecke 0 siblings, 1 reply; 6+ messages in thread From: Benjamin Tissoires @ 2017-03-29 7:30 UTC (permalink / raw) To: Jason Gerecke Cc: linux-input, Jiri Kosina, Ping Cheng, Aaron Skomra, Jason Gerecke On Mar 28 2017 or thereabouts, Jason Gerecke wrote: > The 'wacom_update_name' function is responsible for producing names for > the input device nodes based on the hardware device name. Commit f2209d4 > added the ability to strip off prefixes like "Wacom Co.,Ltd." where the > prefix was immediately (and redundantly) followed by "Wacom". The > 2nd-generation Intuos Pro 2 has such a prefix, but with a small error > (the period and comma are swapped) that prevents the existing code from > matching it. We're loath to extend the number of cases out endlessly and > so instead try to be smarter about name generation. > > We observe that the cause of the redundant prefixes is HID combining the > manufacturer and product strings of USB devices together. By using the > original product name (with "Wacom" prefixed, if it does not already > exist in the string) we can bypass the gyrations to find and remove > redundant prefixes. For devices connected by other busses, the problem > either doesn't exist (e.g. BUS_BLUETOOTH) or the name should be replaced > with a generic one entirely (e.g. BUS_I2C, BUS_INTEL_ISHTP). Ouch. I hope Wacom doesn't plan on using BUS_INTEL_ISHTP for new devices :) > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > --- > drivers/hid/wacom_sys.c | 65 +++++++++++++++++++++++++------------------------ > 1 file changed, 33 insertions(+), 32 deletions(-) > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > index 037b9c04745a..ad5d8722fa84 100644 > --- a/drivers/hid/wacom_sys.c > +++ b/drivers/hid/wacom_sys.c > @@ -2026,41 +2026,42 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix) > > /* Generic devices name unspecified */ > if ((features->type == HID_GENERIC) && !strcmp("Wacom HID", features->name)) { > - if (strstr(wacom->hdev->name, "Wacom") || > - strstr(wacom->hdev->name, "wacom") || > - strstr(wacom->hdev->name, "WACOM")) { > - /* name is in HID descriptor, use it */ > - strlcpy(name, wacom->hdev->name, sizeof(name)); > - > - /* strip out excess whitespaces */ > - while (1) { > - char *gap = strstr(name, " "); > - if (gap == NULL) > - break; > - /* shift everything including the terminator */ > - memmove(gap, gap+1, strlen(gap)); > - } > + char *product_name = NULL; > > - /* strip off excessive prefixing */ > - if (strstr(name, "Wacom Co.,Ltd. Wacom ") == name) { > - int n = strlen(name); > - int x = strlen("Wacom Co.,Ltd. "); > - memmove(name, name+x, n-x+1); > - } > - if (strstr(name, "Wacom Co., Ltd. Wacom ") == name) { > - int n = strlen(name); > - int x = strlen("Wacom Co., Ltd. "); > - memmove(name, name+x, n-x+1); > - } > + if (wacom->hdev->bus == BUS_USB) { > + struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); > + struct usb_device *dev = interface_to_usbdev(intf); > + product_name = dev->product; This will break uhid emulated devices in a very bad way (oops). How about you either add a stub to get the product name from the low-level driver, or simply add 2 pointers to strings to store the product_name and the vendor_name in struct hid_device? Bonus point if you let hid-core decide on the name based on the vendor ID with a table of commonly used companies. hid-core could also clean up the product name so you won't have to do it in each HID driver. The choice of using a weird name for I2C devices was a lazy one from me, and I am starting to hope for something better in the future. Cheers, Benjamin > + } > + else if (wacom->hdev->bus == BUS_BLUETOOTH) { > + product_name = wacom->hdev->name; > + } > > - /* get rid of trailing whitespace */ > - if (name[strlen(name)-1] == ' ') > - name[strlen(name)-1] = '\0'; > - } else { > - /* no meaningful name retrieved. use product ID */ > - snprintf(name, sizeof(name), > - "%s %X", features->name, wacom->hdev->product); > + if (!product_name) { > + snprintf(name, sizeof(name), "%s %X", > + features->name, wacom->hdev->product); > } > + else if (strstr(product_name, "Wacom") || > + strstr(product_name, "wacom") || > + strstr(product_name, "WACOM")) { > + strlcpy(name, product_name, sizeof(name)); > + } > + else { > + snprintf(name, sizeof(name), "Wacom %s", product_name); > + } > + > + /* strip out excess whitespaces */ > + while (1) { > + char *gap = strstr(name, " "); > + if (gap == NULL) > + break; > + /* shift everything including the terminator */ > + memmove(gap, gap+1, strlen(gap)); > + } > + > + /* get rid of trailing whitespace */ > + if (name[strlen(name)-1] == ' ') > + name[strlen(name)-1] = '\0'; > } else { > strlcpy(name, features->name, sizeof(name)); > } > -- > 2.12.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: wacom: Improve generic name generation 2017-03-29 7:30 ` Benjamin Tissoires @ 2017-03-31 16:17 ` Jason Gerecke 2017-04-28 23:07 ` Jason Gerecke 2017-05-15 19:55 ` Ping Cheng 0 siblings, 2 replies; 6+ messages in thread From: Jason Gerecke @ 2017-03-31 16:17 UTC (permalink / raw) To: Benjamin Tissoires Cc: linux-input, Jiri Kosina, Ping Cheng, Aaron Skomra, Jason Gerecke On 03/29/2017 12:30 AM, Benjamin Tissoires wrote: > On Mar 28 2017 or thereabouts, Jason Gerecke wrote: >> The 'wacom_update_name' function is responsible for producing names for >> the input device nodes based on the hardware device name. Commit f2209d4 >> added the ability to strip off prefixes like "Wacom Co.,Ltd." where the >> prefix was immediately (and redundantly) followed by "Wacom". The >> 2nd-generation Intuos Pro 2 has such a prefix, but with a small error >> (the period and comma are swapped) that prevents the existing code from >> matching it. We're loath to extend the number of cases out endlessly and >> so instead try to be smarter about name generation. >> >> We observe that the cause of the redundant prefixes is HID combining the >> manufacturer and product strings of USB devices together. By using the >> original product name (with "Wacom" prefixed, if it does not already >> exist in the string) we can bypass the gyrations to find and remove >> redundant prefixes. For devices connected by other busses, the problem >> either doesn't exist (e.g. BUS_BLUETOOTH) or the name should be replaced >> with a generic one entirely (e.g. BUS_I2C, BUS_INTEL_ISHTP). > > Ouch. I hope Wacom doesn't plan on using BUS_INTEL_ISHTP for new devices > :) > Just me parroting bus names at random. Perhaps BUS_INTEL_ISHTP wasn't a great example ;) >> >> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> >> --- >> drivers/hid/wacom_sys.c | 65 +++++++++++++++++++++++++------------------------ >> 1 file changed, 33 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c >> index 037b9c04745a..ad5d8722fa84 100644 >> --- a/drivers/hid/wacom_sys.c >> +++ b/drivers/hid/wacom_sys.c >> @@ -2026,41 +2026,42 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix) >> >> /* Generic devices name unspecified */ >> if ((features->type == HID_GENERIC) && !strcmp("Wacom HID", features->name)) { >> - if (strstr(wacom->hdev->name, "Wacom") || >> - strstr(wacom->hdev->name, "wacom") || >> - strstr(wacom->hdev->name, "WACOM")) { >> - /* name is in HID descriptor, use it */ >> - strlcpy(name, wacom->hdev->name, sizeof(name)); >> - >> - /* strip out excess whitespaces */ >> - while (1) { >> - char *gap = strstr(name, " "); >> - if (gap == NULL) >> - break; >> - /* shift everything including the terminator */ >> - memmove(gap, gap+1, strlen(gap)); >> - } >> + char *product_name = NULL; >> >> - /* strip off excessive prefixing */ >> - if (strstr(name, "Wacom Co.,Ltd. Wacom ") == name) { >> - int n = strlen(name); >> - int x = strlen("Wacom Co.,Ltd. "); >> - memmove(name, name+x, n-x+1); >> - } >> - if (strstr(name, "Wacom Co., Ltd. Wacom ") == name) { >> - int n = strlen(name); >> - int x = strlen("Wacom Co., Ltd. "); >> - memmove(name, name+x, n-x+1); >> - } >> + if (wacom->hdev->bus == BUS_USB) { >> + struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); >> + struct usb_device *dev = interface_to_usbdev(intf); >> + product_name = dev->product; > > This will break uhid emulated devices in a very bad way (oops). > How about you either add a stub to get the product name from the > low-level driver, or simply add 2 pointers to strings to store the > product_name and the vendor_name in struct hid_device? > Good call on the uhid emulation. I'll write up an alternate patch as suggested. Do you have any particular preference either way? I suppose the former would be more amenable to the extra credit, but... > Bonus point if you let hid-core decide on the name based on the vendor > ID with a table of commonly used companies. hid-core could also clean up > the product name so you won't have to do it in each HID driver. > The choice of using a weird name for I2C devices was a lazy one from me, > and I am starting to hope for something better in the future. > > Cheers, > Benjamin > ...I probably won't go for the extra-credit here. Our driver seems to be pretty unique in how it tries to prettify the vendor; others either replace the name wholesale (like we do in the non-generic codepath) or just append suffixes where necessary. Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... >> + } >> + else if (wacom->hdev->bus == BUS_BLUETOOTH) { >> + product_name = wacom->hdev->name; >> + } >> >> - /* get rid of trailing whitespace */ >> - if (name[strlen(name)-1] == ' ') >> - name[strlen(name)-1] = '\0'; >> - } else { >> - /* no meaningful name retrieved. use product ID */ >> - snprintf(name, sizeof(name), >> - "%s %X", features->name, wacom->hdev->product); >> + if (!product_name) { >> + snprintf(name, sizeof(name), "%s %X", >> + features->name, wacom->hdev->product); >> } >> + else if (strstr(product_name, "Wacom") || >> + strstr(product_name, "wacom") || >> + strstr(product_name, "WACOM")) { >> + strlcpy(name, product_name, sizeof(name)); >> + } >> + else { >> + snprintf(name, sizeof(name), "Wacom %s", product_name); >> + } >> + >> + /* strip out excess whitespaces */ >> + while (1) { >> + char *gap = strstr(name, " "); >> + if (gap == NULL) >> + break; >> + /* shift everything including the terminator */ >> + memmove(gap, gap+1, strlen(gap)); >> + } >> + >> + /* get rid of trailing whitespace */ >> + if (name[strlen(name)-1] == ' ') >> + name[strlen(name)-1] = '\0'; >> } else { >> strlcpy(name, features->name, sizeof(name)); >> } >> -- >> 2.12.0 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: wacom: Improve generic name generation 2017-03-31 16:17 ` Jason Gerecke @ 2017-04-28 23:07 ` Jason Gerecke 2017-05-15 19:55 ` Ping Cheng 1 sibling, 0 replies; 6+ messages in thread From: Jason Gerecke @ 2017-04-28 23:07 UTC (permalink / raw) To: Benjamin Tissoires Cc: Linux Input, Jiri Kosina, Ping Cheng, Aaron Skomra, Jason Gerecke On Fri, Mar 31, 2017 at 9:17 AM, Jason Gerecke <killertofu@gmail.com> wrote: > On 03/29/2017 12:30 AM, Benjamin Tissoires wrote: >> On Mar 28 2017 or thereabouts, Jason Gerecke wrote: >>> The 'wacom_update_name' function is responsible for producing names for >>> the input device nodes based on the hardware device name. Commit f2209d4 >>> added the ability to strip off prefixes like "Wacom Co.,Ltd." where the >>> prefix was immediately (and redundantly) followed by "Wacom". The >>> 2nd-generation Intuos Pro 2 has such a prefix, but with a small error >>> (the period and comma are swapped) that prevents the existing code from >>> matching it. We're loath to extend the number of cases out endlessly and >>> so instead try to be smarter about name generation. >>> >>> We observe that the cause of the redundant prefixes is HID combining the >>> manufacturer and product strings of USB devices together. By using the >>> original product name (with "Wacom" prefixed, if it does not already >>> exist in the string) we can bypass the gyrations to find and remove >>> redundant prefixes. For devices connected by other busses, the problem >>> either doesn't exist (e.g. BUS_BLUETOOTH) or the name should be replaced >>> with a generic one entirely (e.g. BUS_I2C, BUS_INTEL_ISHTP). >> >> Ouch. I hope Wacom doesn't plan on using BUS_INTEL_ISHTP for new devices >> :) >> > > Just me parroting bus names at random. Perhaps BUS_INTEL_ISHTP wasn't a > great example ;) > >>> >>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> >>> --- >>> drivers/hid/wacom_sys.c | 65 +++++++++++++++++++++++++------------------------ >>> 1 file changed, 33 insertions(+), 32 deletions(-) >>> >>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c >>> index 037b9c04745a..ad5d8722fa84 100644 >>> --- a/drivers/hid/wacom_sys.c >>> +++ b/drivers/hid/wacom_sys.c >>> @@ -2026,41 +2026,42 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix) >>> >>> /* Generic devices name unspecified */ >>> if ((features->type == HID_GENERIC) && !strcmp("Wacom HID", features->name)) { >>> - if (strstr(wacom->hdev->name, "Wacom") || >>> - strstr(wacom->hdev->name, "wacom") || >>> - strstr(wacom->hdev->name, "WACOM")) { >>> - /* name is in HID descriptor, use it */ >>> - strlcpy(name, wacom->hdev->name, sizeof(name)); >>> - >>> - /* strip out excess whitespaces */ >>> - while (1) { >>> - char *gap = strstr(name, " "); >>> - if (gap == NULL) >>> - break; >>> - /* shift everything including the terminator */ >>> - memmove(gap, gap+1, strlen(gap)); >>> - } >>> + char *product_name = NULL; >>> >>> - /* strip off excessive prefixing */ >>> - if (strstr(name, "Wacom Co.,Ltd. Wacom ") == name) { >>> - int n = strlen(name); >>> - int x = strlen("Wacom Co.,Ltd. "); >>> - memmove(name, name+x, n-x+1); >>> - } >>> - if (strstr(name, "Wacom Co., Ltd. Wacom ") == name) { >>> - int n = strlen(name); >>> - int x = strlen("Wacom Co., Ltd. "); >>> - memmove(name, name+x, n-x+1); >>> - } >>> + if (wacom->hdev->bus == BUS_USB) { >>> + struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); >>> + struct usb_device *dev = interface_to_usbdev(intf); >>> + product_name = dev->product; >> >> This will break uhid emulated devices in a very bad way (oops). >> How about you either add a stub to get the product name from the >> low-level driver, or simply add 2 pointers to strings to store the >> product_name and the vendor_name in struct hid_device? >> > > Good call on the uhid emulation. I'll write up an alternate patch as > suggested. Do you have any particular preference either way? I suppose > the former would be more amenable to the extra credit, but... > >> Bonus point if you let hid-core decide on the name based on the vendor >> ID with a table of commonly used companies. hid-core could also clean up >> the product name so you won't have to do it in each HID driver. >> The choice of using a weird name for I2C devices was a lazy one from me, >> and I am starting to hope for something better in the future. >> >> Cheers, >> Benjamin >> > > ...I probably won't go for the extra-credit here. Our driver seems to be > pretty unique in how it tries to prettify the vendor; others either > replace the name wholesale (like we do in the non-generic codepath) or > just append suffixes where necessary. > > Jason > --- > Now instead of four in the eights place / > you’ve got three, ‘Cause you added one / > (That is to say, eight) to the two, / > But you can’t take seven from three, / > So you look at the sixty-fours.... > I've spent a little bit of time on this and I'm not sure that this plan actually makes sense: * USB: Provides reasonably-friendly vendor and product names. * I2C: Provides only a (not terribly friendly) chip name: e.g. "WCOM4824:00" * Bluetooth: Provides "ProviderName" and "ServiceName", but its not clear that I can actually ask BlueZ for this information. * ISHTP: Let us not speak of this ;) I could still take this route, but it seems like the only use would be USB (and /maybe/ Bluetooth if I can figure out how to prod BlueZ). Alternatively, I could try to make the conditional in this patch stronger, but I'm not sure how kosher checking something like "wacom->hdev->ll_driver == &usb_hid_driver" is... Thoughts? Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... >>> + } >>> + else if (wacom->hdev->bus == BUS_BLUETOOTH) { >>> + product_name = wacom->hdev->name; >>> + } >>> >>> - /* get rid of trailing whitespace */ >>> - if (name[strlen(name)-1] == ' ') >>> - name[strlen(name)-1] = '\0'; >>> - } else { >>> - /* no meaningful name retrieved. use product ID */ >>> - snprintf(name, sizeof(name), >>> - "%s %X", features->name, wacom->hdev->product); >>> + if (!product_name) { >>> + snprintf(name, sizeof(name), "%s %X", >>> + features->name, wacom->hdev->product); >>> } >>> + else if (strstr(product_name, "Wacom") || >>> + strstr(product_name, "wacom") || >>> + strstr(product_name, "WACOM")) { >>> + strlcpy(name, product_name, sizeof(name)); >>> + } >>> + else { >>> + snprintf(name, sizeof(name), "Wacom %s", product_name); >>> + } >>> + >>> + /* strip out excess whitespaces */ >>> + while (1) { >>> + char *gap = strstr(name, " "); >>> + if (gap == NULL) >>> + break; >>> + /* shift everything including the terminator */ >>> + memmove(gap, gap+1, strlen(gap)); >>> + } >>> + >>> + /* get rid of trailing whitespace */ >>> + if (name[strlen(name)-1] == ' ') >>> + name[strlen(name)-1] = '\0'; >>> } else { >>> strlcpy(name, features->name, sizeof(name)); >>> } >>> -- >>> 2.12.0 >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: wacom: Improve generic name generation 2017-03-31 16:17 ` Jason Gerecke 2017-04-28 23:07 ` Jason Gerecke @ 2017-05-15 19:55 ` Ping Cheng 2017-05-16 7:18 ` Benjamin Tissoires 1 sibling, 1 reply; 6+ messages in thread From: Ping Cheng @ 2017-05-15 19:55 UTC (permalink / raw) To: Jason Gerecke Cc: Benjamin Tissoires, linux-input, Jiri Kosina, Aaron Skomra, Jason Gerecke On Fri, Mar 31, 2017 at 9:17 AM, Jason Gerecke <killertofu@gmail.com> wrote: > On 03/29/2017 12:30 AM, Benjamin Tissoires wrote: >> On Mar 28 2017 or thereabouts, Jason Gerecke wrote: >>> The 'wacom_update_name' function is responsible for producing names for >>> the input device nodes based on the hardware device name. Commit f2209d4 >>> added the ability to strip off prefixes like "Wacom Co.,Ltd." where the >>> prefix was immediately (and redundantly) followed by "Wacom". The >>> 2nd-generation Intuos Pro 2 has such a prefix, but with a small error >>> (the period and comma are swapped) that prevents the existing code from >>> matching it. We're loath to extend the number of cases out endlessly and >>> so instead try to be smarter about name generation. >>> >>> We observe that the cause of the redundant prefixes is HID combining the >>> manufacturer and product strings of USB devices together. By using the >>> original product name (with "Wacom" prefixed, if it does not already >>> exist in the string) we can bypass the gyrations to find and remove >>> redundant prefixes. For devices connected by other busses, the problem >>> either doesn't exist (e.g. BUS_BLUETOOTH) or the name should be replaced >>> with a generic one entirely (e.g. BUS_I2C, BUS_INTEL_ISHTP). >> >> Ouch. I hope Wacom doesn't plan on using BUS_INTEL_ISHTP for new devices >> :) >> > > Just me parroting bus names at random. Perhaps BUS_INTEL_ISHTP wasn't a > great example ;) > >>> >>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> >>> --- >>> drivers/hid/wacom_sys.c | 65 +++++++++++++++++++++++++------------------------ >>> 1 file changed, 33 insertions(+), 32 deletions(-) >>> >>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c >>> index 037b9c04745a..ad5d8722fa84 100644 >>> --- a/drivers/hid/wacom_sys.c >>> +++ b/drivers/hid/wacom_sys.c >>> @@ -2026,41 +2026,42 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix) >>> >>> /* Generic devices name unspecified */ >>> if ((features->type == HID_GENERIC) && !strcmp("Wacom HID", features->name)) { >>> - if (strstr(wacom->hdev->name, "Wacom") || >>> - strstr(wacom->hdev->name, "wacom") || >>> - strstr(wacom->hdev->name, "WACOM")) { >>> - /* name is in HID descriptor, use it */ >>> - strlcpy(name, wacom->hdev->name, sizeof(name)); >>> - >>> - /* strip out excess whitespaces */ >>> - while (1) { >>> - char *gap = strstr(name, " "); >>> - if (gap == NULL) >>> - break; >>> - /* shift everything including the terminator */ >>> - memmove(gap, gap+1, strlen(gap)); >>> - } >>> + char *product_name = NULL; >>> >>> - /* strip off excessive prefixing */ >>> - if (strstr(name, "Wacom Co.,Ltd. Wacom ") == name) { >>> - int n = strlen(name); >>> - int x = strlen("Wacom Co.,Ltd. "); >>> - memmove(name, name+x, n-x+1); >>> - } >>> - if (strstr(name, "Wacom Co., Ltd. Wacom ") == name) { >>> - int n = strlen(name); >>> - int x = strlen("Wacom Co., Ltd. "); >>> - memmove(name, name+x, n-x+1); >>> - } >>> + if (wacom->hdev->bus == BUS_USB) { >>> + struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); >>> + struct usb_device *dev = interface_to_usbdev(intf); >>> + product_name = dev->product; >> >> This will break uhid emulated devices in a very bad way (oops). >> How about you either add a stub to get the product name from the >> low-level driver, or simply add 2 pointers to strings to store the >> product_name and the vendor_name in struct hid_device? >> > > Good call on the uhid emulation. I'll write up an alternate patch as > suggested. Do you have any particular preference either way? I suppose > the former would be more amenable to the extra credit, but... Since Benjamin didn't have a preference (he didn't reply so I assumed the answer ;), I think you can update the patch with your own choice. > >> Bonus point if you let hid-core decide on the name based on the vendor >> ID with a table of commonly used companies. hid-core could also clean up >> the product name so you won't have to do it in each HID driver. >> The choice of using a weird name for I2C devices was a lazy one from me, >> and I am starting to hope for something better in the future. >> >> Cheers, >> Benjamin >> > > ...I probably won't go for the extra-credit here. Our driver seems to be > pretty unique in how it tries to prettify the vendor; others either > replace the name wholesale (like we do in the non-generic codepath) or > just append suffixes where necessary. I agree. I don't think it is necessary to touch hid-core. Cheers, Ping > > Jason > --- > Now instead of four in the eights place / > you’ve got three, ‘Cause you added one / > (That is to say, eight) to the two, / > But you can’t take seven from three, / > So you look at the sixty-fours.... > >>> + } >>> + else if (wacom->hdev->bus == BUS_BLUETOOTH) { >>> + product_name = wacom->hdev->name; >>> + } >>> >>> - /* get rid of trailing whitespace */ >>> - if (name[strlen(name)-1] == ' ') >>> - name[strlen(name)-1] = '\0'; >>> - } else { >>> - /* no meaningful name retrieved. use product ID */ >>> - snprintf(name, sizeof(name), >>> - "%s %X", features->name, wacom->hdev->product); >>> + if (!product_name) { >>> + snprintf(name, sizeof(name), "%s %X", >>> + features->name, wacom->hdev->product); >>> } >>> + else if (strstr(product_name, "Wacom") || >>> + strstr(product_name, "wacom") || >>> + strstr(product_name, "WACOM")) { >>> + strlcpy(name, product_name, sizeof(name)); >>> + } >>> + else { >>> + snprintf(name, sizeof(name), "Wacom %s", product_name); >>> + } >>> + >>> + /* strip out excess whitespaces */ >>> + while (1) { >>> + char *gap = strstr(name, " "); >>> + if (gap == NULL) >>> + break; >>> + /* shift everything including the terminator */ >>> + memmove(gap, gap+1, strlen(gap)); >>> + } >>> + >>> + /* get rid of trailing whitespace */ >>> + if (name[strlen(name)-1] == ' ') >>> + name[strlen(name)-1] = '\0'; >>> } else { >>> strlcpy(name, features->name, sizeof(name)); >>> } >>> -- >>> 2.12.0 >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: wacom: Improve generic name generation 2017-05-15 19:55 ` Ping Cheng @ 2017-05-16 7:18 ` Benjamin Tissoires 0 siblings, 0 replies; 6+ messages in thread From: Benjamin Tissoires @ 2017-05-16 7:18 UTC (permalink / raw) To: Ping Cheng Cc: Jason Gerecke, linux-input, Jiri Kosina, Aaron Skomra, Jason Gerecke On May 15 2017 or thereabouts, Ping Cheng wrote: > On Fri, Mar 31, 2017 at 9:17 AM, Jason Gerecke <killertofu@gmail.com> wrote: > > On 03/29/2017 12:30 AM, Benjamin Tissoires wrote: > >> On Mar 28 2017 or thereabouts, Jason Gerecke wrote: > >>> The 'wacom_update_name' function is responsible for producing names for > >>> the input device nodes based on the hardware device name. Commit f2209d4 > >>> added the ability to strip off prefixes like "Wacom Co.,Ltd." where the > >>> prefix was immediately (and redundantly) followed by "Wacom". The > >>> 2nd-generation Intuos Pro 2 has such a prefix, but with a small error > >>> (the period and comma are swapped) that prevents the existing code from > >>> matching it. We're loath to extend the number of cases out endlessly and > >>> so instead try to be smarter about name generation. > >>> > >>> We observe that the cause of the redundant prefixes is HID combining the > >>> manufacturer and product strings of USB devices together. By using the > >>> original product name (with "Wacom" prefixed, if it does not already > >>> exist in the string) we can bypass the gyrations to find and remove > >>> redundant prefixes. For devices connected by other busses, the problem > >>> either doesn't exist (e.g. BUS_BLUETOOTH) or the name should be replaced > >>> with a generic one entirely (e.g. BUS_I2C, BUS_INTEL_ISHTP). > >> > >> Ouch. I hope Wacom doesn't plan on using BUS_INTEL_ISHTP for new devices > >> :) > >> > > > > Just me parroting bus names at random. Perhaps BUS_INTEL_ISHTP wasn't a > > great example ;) > > > >>> > >>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > >>> --- > >>> drivers/hid/wacom_sys.c | 65 +++++++++++++++++++++++++------------------------ > >>> 1 file changed, 33 insertions(+), 32 deletions(-) > >>> > >>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > >>> index 037b9c04745a..ad5d8722fa84 100644 > >>> --- a/drivers/hid/wacom_sys.c > >>> +++ b/drivers/hid/wacom_sys.c > >>> @@ -2026,41 +2026,42 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix) > >>> > >>> /* Generic devices name unspecified */ > >>> if ((features->type == HID_GENERIC) && !strcmp("Wacom HID", features->name)) { > >>> - if (strstr(wacom->hdev->name, "Wacom") || > >>> - strstr(wacom->hdev->name, "wacom") || > >>> - strstr(wacom->hdev->name, "WACOM")) { > >>> - /* name is in HID descriptor, use it */ > >>> - strlcpy(name, wacom->hdev->name, sizeof(name)); > >>> - > >>> - /* strip out excess whitespaces */ > >>> - while (1) { > >>> - char *gap = strstr(name, " "); > >>> - if (gap == NULL) > >>> - break; > >>> - /* shift everything including the terminator */ > >>> - memmove(gap, gap+1, strlen(gap)); > >>> - } > >>> + char *product_name = NULL; > >>> > >>> - /* strip off excessive prefixing */ > >>> - if (strstr(name, "Wacom Co.,Ltd. Wacom ") == name) { > >>> - int n = strlen(name); > >>> - int x = strlen("Wacom Co.,Ltd. "); > >>> - memmove(name, name+x, n-x+1); > >>> - } > >>> - if (strstr(name, "Wacom Co., Ltd. Wacom ") == name) { > >>> - int n = strlen(name); > >>> - int x = strlen("Wacom Co., Ltd. "); > >>> - memmove(name, name+x, n-x+1); > >>> - } > >>> + if (wacom->hdev->bus == BUS_USB) { > >>> + struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); > >>> + struct usb_device *dev = interface_to_usbdev(intf); > >>> + product_name = dev->product; > >> > >> This will break uhid emulated devices in a very bad way (oops). > >> How about you either add a stub to get the product name from the > >> low-level driver, or simply add 2 pointers to strings to store the > >> product_name and the vendor_name in struct hid_device? > >> > > > > Good call on the uhid emulation. I'll write up an alternate patch as > > suggested. Do you have any particular preference either way? I suppose > > the former would be more amenable to the extra credit, but... > > Since Benjamin didn't have a preference (he didn't reply so I assumed > the answer ;), I think you can update the patch with your own choice. > > > > >> Bonus point if you let hid-core decide on the name based on the vendor > >> ID with a table of commonly used companies. hid-core could also clean up > >> the product name so you won't have to do it in each HID driver. > >> The choice of using a weird name for I2C devices was a lazy one from me, > >> and I am starting to hope for something better in the future. > >> > >> Cheers, > >> Benjamin > >> > > > > ...I probably won't go for the extra-credit here. Our driver seems to be > > pretty unique in how it tries to prettify the vendor; others either > > replace the name wholesale (like we do in the non-generic codepath) or > > just append suffixes where necessary. > > I agree. I don't think it is necessary to touch hid-core. Yeah, sorry, this email went deep into my mailbox. A hid-core cleanup would be nice, but if you are the only one in need for that, then you can have a special case in your module. Cheers, Benjamin > > Cheers, > Ping > > > > > Jason > > --- > > Now instead of four in the eights place / > > you’ve got three, ‘Cause you added one / > > (That is to say, eight) to the two, / > > But you can’t take seven from three, / > > So you look at the sixty-fours.... > > > >>> + } > >>> + else if (wacom->hdev->bus == BUS_BLUETOOTH) { > >>> + product_name = wacom->hdev->name; > >>> + } > >>> > >>> - /* get rid of trailing whitespace */ > >>> - if (name[strlen(name)-1] == ' ') > >>> - name[strlen(name)-1] = '\0'; > >>> - } else { > >>> - /* no meaningful name retrieved. use product ID */ > >>> - snprintf(name, sizeof(name), > >>> - "%s %X", features->name, wacom->hdev->product); > >>> + if (!product_name) { > >>> + snprintf(name, sizeof(name), "%s %X", > >>> + features->name, wacom->hdev->product); > >>> } > >>> + else if (strstr(product_name, "Wacom") || > >>> + strstr(product_name, "wacom") || > >>> + strstr(product_name, "WACOM")) { > >>> + strlcpy(name, product_name, sizeof(name)); > >>> + } > >>> + else { > >>> + snprintf(name, sizeof(name), "Wacom %s", product_name); > >>> + } > >>> + > >>> + /* strip out excess whitespaces */ > >>> + while (1) { > >>> + char *gap = strstr(name, " "); > >>> + if (gap == NULL) > >>> + break; > >>> + /* shift everything including the terminator */ > >>> + memmove(gap, gap+1, strlen(gap)); > >>> + } > >>> + > >>> + /* get rid of trailing whitespace */ > >>> + if (name[strlen(name)-1] == ' ') > >>> + name[strlen(name)-1] = '\0'; > >>> } else { > >>> strlcpy(name, features->name, sizeof(name)); > >>> } > >>> -- > >>> 2.12.0 > >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-16 7:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-28 21:15 [PATCH] HID: wacom: Improve generic name generation Jason Gerecke 2017-03-29 7:30 ` Benjamin Tissoires 2017-03-31 16:17 ` Jason Gerecke 2017-04-28 23:07 ` Jason Gerecke 2017-05-15 19:55 ` Ping Cheng 2017-05-16 7:18 ` Benjamin Tissoires
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.