Re: [PATCH v2] HID: wacom: Improve generic name generation

On May 17 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. Other devices either don't have a manufacturer string
that needs to be removed (Bluetooth, uhid) or should have their name
generated from scratch (I2C).

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---

Thanks Jason.

I am not a big fan of the "real USB" check (I would believe enhancing
struct hid_device would make something more generic), but it seems to be
doing the job OK.

Just giving out 2 ideas to achieve the same, so you could decide keeping
this or not:

- adding .product_name to struct hid_device directly
- exporting usb_hid_driver in usbhid/usbhid.h and comparing hdev->ll_driver
  with it (this can be achieved in an inlined function in usbhid.h).

I would believe having the second option above would provide several
benefits:
- the wacom module will not have to check against all usb devices
- other drivers would be able to quickly tell if they are using a low
  level USB device, and not a uhid one. This way, we can remove some
  errors when we emulate a HID device on a HID driver that expects USB
  to be the actual transport layer

Your function works too, but I would believe this will be faster and
better (I also get the concern of having something self consistent for
easier backporting).

Besides that, small nitpicks:

wacom_is_real_usb() already checks for BUS_USB, so the first test is
redundant.

this else shoule be on the previous line

likewise

+			product_name = wacom->hdev->name;
+		}

How about?:
	 if (wacom_is_real_usb(wacom)) {
		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 ||
		   wacom->hdev->bus == BUS_USB) {
		product_name = wacom->hdev->name;
	}

else should be on the previous line

+			 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.2

Cheers,
Benjamin