XP-PEN G430S works in v9 -- broken in v10 by #464

[tornaria]

I have a XP-PEN G430S which works perfectly with v9 but doesn't work at all with v10.

I tracked the issue to #464 (commit ec5c16d).

Indeed, with the following patch applied the pen works fine.

--- a/hid-uclogic-rdesc.c
+++ b/hid-uclogic-rdesc.c
@@ -534,7 +534,7 @@ const size_t uclogic_rdesc_twha60_fixed1_size =
 /* Fixed report descriptor template for (tweaked) v1 pen reports */
 const __u8 uclogic_rdesc_v1_pen_template_arr[] = {
        0x05, 0x0D,             /*  Usage Page (Digitizer),                 */
-       0x09, 0x01,             /*  Usage (Digitizer),                      */
+       0x09, 0x02,             /*  Usage (Digitizer),                      */
        0xA1, 0x01,             /*  Collection (Application),               */
        0x85, 0x07,             /*      Report ID (7),                      */
        0x09, 0x20,             /*      Usage (Stylus),                     */

I'm not sure what would be the proper fix for this issue.

Note that G430S has the same usbid as G540 and it's handled by USB_DEVICE_ID_UGEE_XPPEN_TABLET_G540.

[tornaria]

In case it's useful, I tested 3335f96, which is the first commit to support my tablet, and it works. But if I change the rdesc from "Pen" to "Digitizer" (as in ec5c16d) then it stops working.

[tornaria]

Here's a different patch that makes the pen work:

diff --git a/hid-uclogic-params.c b/hid-uclogic-params.c
index b13ae12..0d5b1c6 100644
--- a/hid-uclogic-params.c
+++ b/hid-uclogic-params.c
@@ -1204,6 +1204,7 @@ int uclogic_params_init(struct uclogic_params *params,
                                hid_warn(hdev, "pen parameters not found");
                                uclogic_params_init_invalid(&p);
                        }
+                       hdev->quirks |= HID_QUIRK_HIDINPUT_FORCE;
                } else {
                        uclogic_params_init_invalid(&p);
                }

For some reason, with Digitizer usage the interface is not recognized as an input device; forcing it makes it work.

Is this patch reasonable?

[tornaria]

It might be that the actual bug is in the kernel, in the IS_INPUT_APPLICATION macro (include/linux/hid.h):

/* Applications from HID Usage Tables 4/8/99 Version 1.1 */
/* We ignore a few input applications that are not widely used */
#define IS_INPUT_APPLICATION(a) \
		(((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
		|| ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \
		|| (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \
		|| (a == HID_GD_WIRELESS_RADIO_CTLS))

This definition gives IS_INPUT_APPLICATION(HID_DG_DIGITIZER) false which makes hidinput_connect() to skip this tablet, unless HID_QUIRK_HIDINPUT_FORCE is used.

Since HID_DG_PEN = 0x000d0002 and HID_DG_DIGITIZER = 0x000d0001, I believe a proper fix in the kernel would be

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 85bedeb9ca9f..93b62631d011 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -838,7 +838,7 @@ static inline bool hid_is_using_ll_driver(struct hid_device *hdev,
 /* We ignore a few input applications that are not widely used */
 #define IS_INPUT_APPLICATION(a) \
                (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
-               || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \
+               || ((a >= HID_DG_DIGITIZER) && (a <= HID_DG_WHITEBOARD)) \
                || (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \
                || (a == HID_GD_WIRELESS_RADIO_CTLS))
 

Meanwhile, using HID_QUIRK_HIDINPUT_FORCE seems a good workaround, at least for the broken tablet, but it seems safe to use it for all tablets as in:

diff --git a/hid-uclogic-core.c b/hid-uclogic-core.c
index ce30c45..e805ff8 100644
--- a/hid-uclogic-core.c
+++ b/hid-uclogic-core.c
@@ -203,6 +203,7 @@ static int uclogic_probe(struct hid_device *hdev,
 #ifdef HID_QUIRK_NO_EMPTY_INPUT
        hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;
 #endif
+       hdev->quirks |= HID_QUIRK_HIDINPUT_FORCE;
 
        /* Allocate and assign driver data */
        drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);

[tornaria]

May I also suggest the following cosmetic patch:

diff --git a/hid-uclogic-core.c b/hid-uclogic-core.c
index ce30c45..2fe9af9 100644
--- a/hid-uclogic-core.c
+++ b/hid-uclogic-core.c
@@ -163,6 +163,9 @@ static int uclogic_input_configured(struct hid_device *hdev,
                case HID_GD_KEYPAD:
                        suffix = "Pad";
                        break;
+               case HID_DG_DIGITIZER:
+                       suffix = "Digitizer";
+                       break;
                case HID_DG_PEN:
                        suffix = "Pen";
                        break;

Without this patch, my tablet shows in dmesg as:

input: XP-PEN G430S as /devices/pci0000...

With the patch, it shows as:

input: XP-PEN G430S Digitizer as /devices/pci0000...

Edit: this also affects the name of the device as shown by libinput or xinput.

---

Finally, the following line in hid-uclogic-core.c feels strange to me:

	/* Discard invalid pen usages */
	if (params->pen.usage_invalid && (field->application == HID_DG_PEN))
		return -1;

I'm thinking if pen.usage_invalid is true you want to discard this interface, regardless of field->application (which could also be HID_DG_DIGITIZER?)