* [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.