On Fri, Mar 31, 2017 at 9:17 AM, Jason Gerecke <killertofu@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
