2022-12-26 03:28:57

by Joshua Goins

Subject: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

From: Aren Villanueva <[email protected]>

Adds support for the XP-PEN Artist 22R Pro in uclogic, including the stylus,
frame and pen pressure support.

I did not do the research for this hardware, or the original patch - that work
has been done by Aren Villanueva. For some reason they decided not to merge
it. My changes include remapping the stupid amount of tablet frame buttons,
cleaning up the code to match kernel style, and other small stuff.

The tablet is (almost) fully functional even when uclogic doesn't handle it.
Without initialization, the tablet has some sort of "basic driverless mode"
that allows the tablet frame buttons to have some default keys associated with
them (CTRL-S, CTRL-Z, that kind of stuff), but unfortunately the stylus pen
semi-works. While pressure sensitivity works, only one stylus button functions
correctly. Since the initialization process differs for Pro series tablets, the
new function uclogic_params_init_ugee_xppen_pro had to be introduced. I also
added USB HID IDs for this tablet too, but it's classified under the UGEE
vendor ID.

One of the more strange things I had to do is figure out a way to remap the
buttons since there are 20 of them in total, and of course there are more
buttons than there are BTN constants defined for us. When running without
uclogic, it starts at BTN_0, ends at BTN_8 and the tablet starts reporting
nonsensical keycodes so just leaving it alone isn't an option. I'm testing
this under a libinput system, which has a list of buttons it considers "tablet
pad buttons" which are notably BTN_0, BTN_1, so on and some
gamepad/joystick buttons. So I created a new array called
uclogic_extra_input_mapping for 20 working inputs.

Another weird feature of this tablet is the second dial, which the original
patchset introduced a new uclogic_frame param to handle since it seems it
throws both dials values into one byte. The left wheel is considered EV_WHEEL
and the other, EV_HWHEEL which seems fine to me. I also added the new param to
the debug messages too.

Link: https://github.com/DIGImend/digimend-kernel-drivers/pull/557
Signed-off-by: Joshua Goins <[email protected]>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-uclogic-core.c | 68 +++++++++++++-
drivers/hid/hid-uclogic-params.c | 147 +++++++++++++++++++++++++++++++
drivers/hid/hid-uclogic-params.h | 5 ++
drivers/hid/hid-uclogic-rdesc.c | 119 +++++++++++++++++++++++++
drivers/hid/hid-uclogic-rdesc.h | 15 ++++
6 files changed, 351 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 82713ef3aaa6..81d04054229a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1298,6 +1298,7 @@
#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_L 0x0935
#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S 0x0909
#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06 0x0078
+#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO 0x091b
#define USB_DEVICE_ID_UGEE_TABLET_G5 0x0074
#define USB_DEVICE_ID_UGEE_TABLET_EX07S 0x0071
#define USB_DEVICE_ID_UGEE_TABLET_RAINBOW_CV720 0x0055
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index 7fa6fe04f1b2..ebc93b354f7a 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -81,6 +81,30 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
return rdesc;
}

+/* Buttons considered valid tablet pad inputs. */
+const unsigned int uclogic_extra_input_mapping[] = {
+ BTN_0,
+ BTN_1,
+ BTN_2,
+ BTN_3,
+ BTN_4,
+ BTN_5,
+ BTN_6,
+ BTN_7,
+ BTN_8,
+ BTN_RIGHT,
+ BTN_MIDDLE,
+ BTN_SIDE,
+ BTN_EXTRA,
+ BTN_FORWARD,
+ BTN_BACK,
+ BTN_B,
+ BTN_A,
+ BTN_BASE,
+ BTN_BASE2,
+ BTN_X
+};
+
static int uclogic_input_mapping(struct hid_device *hdev,
struct hid_input *hi,
struct hid_field *field,
@@ -91,9 +115,27 @@ static int uclogic_input_mapping(struct hid_device *hdev,
struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
struct uclogic_params *params = &drvdata->params;

- /* Discard invalid pen usages */
- if (params->pen.usage_invalid && (field->application == HID_DG_PEN))
- return -1;
+ if (field->application == HID_GD_KEYPAD) {
+ /*
+ * Remap input buttons to sensible ones that are not invalid.
+ * This only affects previous behavior for devices with more than ten or so buttons.
+ */
+ const int key = (usage->hid & HID_USAGE) - 1;
+
+ if (key > 0 && key < ARRAY_SIZE(uclogic_extra_input_mapping)) {
+ hid_map_usage(hi,
+ usage,
+ bit,
+ max,
+ EV_KEY,
+ uclogic_extra_input_mapping[key]);
+ return 1;
+ }
+ } else if (field->application == HID_DG_PEN) {
+ /* Discard invalid pen usages */
+ if (params->pen.usage_invalid)
+ return -1;
+ }

/* Let hid-core decide what to do */
return 0;
@@ -403,8 +445,24 @@ static int uclogic_raw_event_frame(

/* If need to, and can, transform the bitmap dial reports */
if (frame->bitmap_dial_byte > 0 && frame->bitmap_dial_byte < size) {
- if (data[frame->bitmap_dial_byte] == 2)
+ switch (data[frame->bitmap_dial_byte]) {
+ case 2:
data[frame->bitmap_dial_byte] = -1;
+ break;
+
+ /* Everything below here is for tablets that shove multiple dials into 1 byte */
+ case 4:
+ case 0x10:
+ data[frame->bitmap_dial_byte] = 0;
+ data[frame->bitmap_second_dial_destination_byte] = 1;
+ break;
+
+ case 8:
+ case 0x20:
+ data[frame->bitmap_dial_byte] = 0;
+ data[frame->bitmap_second_dial_destination_byte] = -1;
+ break;
+ }
}

return 0;
@@ -531,6 +589,8 @@ static const struct hid_device_id uclogic_devices[] = {
USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S) },
{ HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
+ USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO) },
{ }
};
MODULE_DEVICE_TABLE(hid, uclogic_devices);
diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
index cd1233d7e253..86a1dfa2a4c0 100644
--- a/drivers/hid/hid-uclogic-params.c
+++ b/drivers/hid/hid-uclogic-params.c
@@ -103,6 +103,8 @@ static void uclogic_params_frame_hid_dbg(
frame->touch_flip_at);
hid_dbg(hdev, "\t\t.bitmap_dial_byte = %u\n",
frame->bitmap_dial_byte);
+ hid_dbg(hdev, "\t\t.bitmap_second_dial_destination_byte = %u\n",
+ frame->bitmap_second_dial_destination_byte);
}

/**
@@ -1418,6 +1420,126 @@ static int uclogic_params_ugee_v2_init(struct uclogic_params *params,
return rc;
}

+
+/*
+ * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
+ *
+ * @hdev: The HID device of the tablet interface to initialize and get
+ * parameters from. Cannot be NULL.
+ * @params: Parameters to fill in (to be cleaned with
+ * uclogic_params_cleanup()). Not modified in case of error.
+ * Cannot be NULL.
+ *
+ * Returns:
+ * Zero, if successful. A negative errno code on error.
+ */
+static int uclogic_params_init_ugee_xppen_pro(struct hid_device *hdev,
+ struct uclogic_params *p,
+ const u8 probe_endpoint,
+ const u8 rdesc_init_packet[],
+ const size_t rdesc_init_size,
+ const u8 rdesc_tablet_arr[],
+ const size_t rdesc_tablet_size,
+ const u8 rdesc_frame_arr[],
+ const size_t rdesc_frame_size)
+{
+ const size_t str_desc_len = 12;
+ struct usb_device *udev = hid_to_usb_dev(hdev);
+ u8 *buf = kmemdup(rdesc_init_packet, rdesc_init_size, GFP_KERNEL);
+ s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
+ int actual_len, rc;
+ u16 resolution;
+
+ if (hdev == NULL || p == NULL)
+ return -EINVAL;
+
+ rc = usb_interrupt_msg(
+ udev,
+ usb_sndintpipe(udev, probe_endpoint),
+ buf,
+ rdesc_init_size,
+ &actual_len,
+ USB_CTRL_SET_TIMEOUT);
+ kfree(buf);
+ if (rc == -EPIPE) {
+ hid_err(hdev, "broken pipe sending init packet\n");
+ return rc;
+ } else if (rc < 0) {
+ hid_err(hdev, "failed sending init packet: %d\n", rc);
+ return rc;
+ } else if (actual_len != rdesc_init_size) {
+ hid_err(hdev,
+ "failed to transfer complete init packet, only %d bytes sent\n",
+ actual_len);
+ return -1;
+ }
+
+ rc = uclogic_params_get_str_desc(&buf, hdev, 100, str_desc_len);
+ if (rc != str_desc_len) {
+ if (rc == -EPIPE) {
+ hid_err(hdev,
+ "string descriptor with pen parameters not found\n");
+ } else if (rc < 0) {
+ hid_err(hdev,
+ "failed retrieving pen parameters: %d\n", rc);
+ } else {
+ hid_err(hdev,
+ "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
+ rc,
+ str_desc_len);
+ rc = -1;
+ }
+ kfree(buf);
+ return rc;
+ }
+
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = get_unaligned_le16(buf + 2);
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = get_unaligned_le16(buf + 4);
+ /* buf + 6 is the number of pad buttons? Its 0x0008 */
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] =
+ get_unaligned_le16(buf + 8);
+ resolution = get_unaligned_le16(buf + 10);
+ kfree(buf);
+ if (resolution == 0) {
+ hid_err(hdev, "resolution of 0 in descriptor string\n");
+ return -1;
+ }
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_PM] =
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] * 1000 / resolution;
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_PM] =
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] * 1000 / resolution;
+
+ hid_dbg(hdev,
+ "Received parameters: X: %d Y: %d Pressure: %d Resolution: %u\n",
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM],
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM],
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM],
+ resolution);
+
+ p->pen.desc_ptr = uclogic_rdesc_template_apply(
+ rdesc_tablet_arr,
+ rdesc_tablet_size,
+ desc_params,
+ ARRAY_SIZE(desc_params));
+ p->pen.desc_size = rdesc_tablet_size;
+ p->pen.id = 0x02;
+
+ rc = uclogic_params_frame_init_with_desc(
+ &p->frame_list[0],
+ rdesc_frame_arr,
+ rdesc_frame_size,
+ UCLOGIC_RDESC_V1_FRAME_ID);
+ if (rc < 0) {
+ hid_err(hdev, "initializing frame params failed: %d\n", rc);
+ return rc;
+ }
+
+ p->pen.subreport_list[0].value = 0xf0;
+ p->pen.subreport_list[0].id = p->frame_list[0].id;
+
+ return 0;
+}
+
/**
* uclogic_params_init() - initialize a tablet interface and discover its
* parameters.
@@ -1728,6 +1850,31 @@ int uclogic_params_init(struct uclogic_params *params,
uclogic_params_init_invalid(&p);
}

+ break;
+ case VID_PID(USB_VENDOR_ID_UGEE,
+ USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO):
+ /* Ignore non-pen interfaces */
+ if (bInterfaceNumber != 2) {
+ uclogic_params_init_invalid(&p);
+ break;
+ }
+
+ rc = uclogic_params_init_ugee_xppen_pro(
+ hdev, &p, UCLOGIC_RDESC_UGEE_XPPEN_PROBE_ENDPOINT_TYPE1,
+ uclogic_rdesc_xppen_init_packet_type1_arr,
+ uclogic_rdesc_xppen_init_packet_type1_size,
+ uclogic_rdesc_xppen_pro_stylus_type1_arr,
+ uclogic_rdesc_xppen_pro_stylus_type1_size,
+ uclogic_rdesc_xppen_artist_22r_pro_frame_arr,
+ uclogic_rdesc_xppen_artist_22r_pro_frame_size);
+ if (rc != 0) {
+ hid_err(hdev, "failed creating frame parameters: %d\n", rc);
+ goto cleanup;
+ }
+
+ p.frame_list[0].bitmap_dial_byte = 7;
+ p.frame_list[0].bitmap_second_dial_destination_byte = 8;
+
break;
}

diff --git a/drivers/hid/hid-uclogic-params.h b/drivers/hid/hid-uclogic-params.h
index a97477c02ff8..6621a75a4b1a 100644
--- a/drivers/hid/hid-uclogic-params.h
+++ b/drivers/hid/hid-uclogic-params.h
@@ -171,6 +171,11 @@ struct uclogic_params_frame {
* counterclockwise, as opposed to the normal 1 and -1.
*/
unsigned int bitmap_dial_byte;
+ /*
+ * Destination offset for the second bitmap dial byte, if the tablet
+ * supports a second dial at all.
+ */
+ unsigned int bitmap_second_dial_destination_byte;
};

/*
diff --git a/drivers/hid/hid-uclogic-rdesc.c b/drivers/hid/hid-uclogic-rdesc.c
index fb40775f5f5b..b55dbe5017c1 100644
--- a/drivers/hid/hid-uclogic-rdesc.c
+++ b/drivers/hid/hid-uclogic-rdesc.c
@@ -1185,6 +1185,125 @@ const __u8 uclogic_rdesc_xppen_deco01_frame_arr[] = {
const size_t uclogic_rdesc_xppen_deco01_frame_size =
sizeof(uclogic_rdesc_xppen_deco01_frame_arr);

+/* Fix report descriptor for XP-Pen init packet type 1 */
+const __u8 uclogic_rdesc_xppen_init_packet_type1_arr[] = {
+ 0x02, 0xb0, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+};
+
+const size_t uclogic_rdesc_xppen_init_packet_type1_size =
+ sizeof(uclogic_rdesc_xppen_init_packet_type1_arr);
+
+/* Fixed report descriptor for XP-Pen Pro Stylus type 1 (10 bytes) */
+const __u8 uclogic_rdesc_xppen_pro_stylus_type1_arr[] = {
+ 0x05, 0x0D, /* Usage Page (Digitizer), */
+ 0x09, 0x02, /* Usage (Pen), */
+ 0xA1, 0x01, /* Collection (Application), */
+ 0x85, 0x02, /* Report ID (2), */
+ 0x09, 0x20, /* Usage (Stylus), */
+ 0xA0, /* Collection (Physical), */
+ 0x14, /* Logical Minimum (0), */
+ 0x25, 0x01, /* Logical Maximum (1), */
+ 0x09, 0x42, /* Usage (Tip Switch), */
+ 0x09, 0x44, /* Usage (Barrel Switch), */
+ 0x09, 0x46, /* Usage (Tablet Pick), */
+ 0x75, 0x01, /* Report Size (1), */
+ 0x95, 0x03, /* Report Count (3), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0x95, 0x02, /* Report Count (2), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x09, 0x32, /* Usage (In Range), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0x95, 0x02, /* Report Count (2), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x75, 0x10, /* Report Size (16), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0xA4, /* Push, */
+ 0x05, 0x01, /* Usage Page (Desktop), */
+ 0x55, 0xFD, /* Unit Exponent (-3), */
+ 0x65, 0x13, /* Unit (Inch), */
+ 0x34, /* Physical Minimum (0), */
+ 0x09, 0x30, /* Usage (X), */
+ 0x27, UCLOGIC_RDESC_PEN_PH(X_LM),
+ /* Logical Maximum (PLACEHOLDER), */
+ 0x47, UCLOGIC_RDESC_PEN_PH(X_PM),
+ /* Physical Maximum (PLACEHOLDER), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0x09, 0x31, /* Usage (Y), */
+ 0x27, UCLOGIC_RDESC_PEN_PH(Y_LM),
+ /* Logical Maximum (PLACEHOLDER), */
+ 0x47, UCLOGIC_RDESC_PEN_PH(Y_PM),
+ /* Physical Maximum (PLACEHOLDER), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0xB4, /* Pop, */
+ 0x09, 0x30, /* Usage (Tip Pressure), */
+ 0x27, UCLOGIC_RDESC_PEN_PH(PRESSURE_LM),
+ /* Logical Maximum (PLACEHOLDER), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0xA4, /* Push, */
+ 0x54, /* Unit Exponent (0), */
+ 0x65, 0x14, /* Unit (Degrees), */
+ 0x35, 0xC3, /* Physical Minimum (-61), */
+ 0x45, 0x3C, /* Physical Maximum (60), */
+ 0x15, 0xC3, /* Logical Minimum (-61), */
+ 0x25, 0x3C, /* Logical Maximum (60), */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x02, /* Report Count (2), */
+ 0x09, 0x3D, /* Usage (X Tilt), */
+ 0x09, 0x3E, /* Usage (Y Tilt), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0xB4, /* Pop, */
+ 0xC0, /* End Collection, */
+ 0xC0 /* End Collection */
+};
+
+const size_t uclogic_rdesc_xppen_pro_stylus_type1_size =
+ sizeof(uclogic_rdesc_xppen_pro_stylus_type1_arr);
+
+/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
+const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[] = {
+ 0x05, 0x01, /* Usage Page (Desktop), */
+ 0x09, 0x07, /* Usage (Keypad), */
+ 0xA1, 0x01, /* Collection (Application), */
+ 0x85, UCLOGIC_RDESC_V1_FRAME_ID,
+ /* Report ID (Virtual report), */
+ 0x05, 0x0D, /* Usage Page (Digitizer), */
+ 0x09, 0x39, /* Usage (Tablet Function Keys), */
+ 0xA0, /* Collection (Physical), */
+ 0x14, /* Logical Minimum (0), */
+ 0x25, 0x01, /* Logical Maximum (1), */
+ 0x75, 0x01, /* Report Size (1), */
+ 0x95, 0x08, /* Report Count (8), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x05, 0x09, /* Usage Page (Button), */
+ 0x19, 0x01, /* Usage Minimum (01h), */
+ 0x29, 0x14, /* Usage Maximum (14h), */
+ 0x95, 0x14, /* Report Count (20), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0x95, 0x14, /* Report Count (20), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x05, 0x01, /* Usage Page (Desktop), */
+ 0x09, 0x38, /* Usage (Wheel), */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x15, 0xFF, /* Logical Minimum (-1), */
+ 0x25, 0x08, /* Logical Maximum (8), */
+ 0x81, 0x06, /* Input (Variable, Relative), */
+ 0x05, 0x0C, /* Usage Page (Consumer Devices), */
+ 0x0A, 0x38, 0x02, /* Usage (AC PAN), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x81, 0x06, /* Input (Variable, Relative), */
+ 0x26, 0xFF, 0x00, /* Logical Maximum (255), */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0xC0, /* End Collection */
+ 0xC0, /* End Collection */
+};
+
+const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size =
+ sizeof(uclogic_rdesc_xppen_artist_22r_pro_frame_arr);
+
/**
* uclogic_rdesc_template_apply() - apply report descriptor parameters to a
* report descriptor template, creating a report descriptor. Copies the
diff --git a/drivers/hid/hid-uclogic-rdesc.h b/drivers/hid/hid-uclogic-rdesc.h
index a1f78c07293f..9e4055a5016e 100644
--- a/drivers/hid/hid-uclogic-rdesc.h
+++ b/drivers/hid/hid-uclogic-rdesc.h
@@ -205,4 +205,19 @@ extern const size_t uclogic_rdesc_ugee_g5_frame_size;
/* Least-significant bit of Ugee G5 frame rotary encoder state */
#define UCLOGIC_RDESC_UGEE_G5_FRAME_RE_LSB 38

+/* Probe endpoints for XP-Pen key activators */
+#define UCLOGIC_RDESC_UGEE_XPPEN_PROBE_ENDPOINT_TYPE1 0x03
+
+/* Fix report descriptor for XP-Pen init packet type 1 */
+extern const __u8 uclogic_rdesc_xppen_init_packet_type1_arr[];
+extern const size_t uclogic_rdesc_xppen_init_packet_type1_size;
+
+/* Fixed report descriptor for XP-Pen Pro Stylus type 1 (10 bytes) */
+extern const __u8 uclogic_rdesc_xppen_pro_stylus_type1_arr[];
+extern const size_t uclogic_rdesc_xppen_pro_stylus_type1_size;
+
+/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
+extern const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[];
+extern const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size;
+
#endif /* _HID_UCLOGIC_RDESC_H */
--
2.38.2

2022-12-26 09:38:41

by kernel test robot

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi Joshua,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20221226-112302
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/2068502.VLH7GnMWUR%40adrastea
patch subject: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro
config: arm-randconfig-r021-20221226
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project f5700e7b69048de958172fb513b336564e7f8709)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/51d8c9b14fc55dcb5b8e3f7f4e0cc582ce9097d7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20221226-112302
git checkout 51d8c9b14fc55dcb5b8e3f7f4e0cc582ce9097d7
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/hid/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/hid/hid-uclogic-params.c:1489:5: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
str_desc_len);
^~~~~~~~~~~~
include/linux/hid.h:1206:30: note: expanded from macro 'hid_err'
dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:144:65: note: expanded from macro 'dev_err'
dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
_p_func(dev, fmt, ##__VA_ARGS__); \
~~~ ^~~~~~~~~~~
1 warning generated.


vim +1489 drivers/hid/hid-uclogic-params.c

1422
1423
1424 /*
1425 * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
1426 *
1427 * @hdev: The HID device of the tablet interface to initialize and get
1428 * parameters from. Cannot be NULL.
1429 * @params: Parameters to fill in (to be cleaned with
1430 * uclogic_params_cleanup()). Not modified in case of error.
1431 * Cannot be NULL.
1432 *
1433 * Returns:
1434 * Zero, if successful. A negative errno code on error.
1435 */
1436 static int uclogic_params_init_ugee_xppen_pro(struct hid_device *hdev,
1437 struct uclogic_params *p,
1438 const u8 probe_endpoint,
1439 const u8 rdesc_init_packet[],
1440 const size_t rdesc_init_size,
1441 const u8 rdesc_tablet_arr[],
1442 const size_t rdesc_tablet_size,
1443 const u8 rdesc_frame_arr[],
1444 const size_t rdesc_frame_size)
1445 {
1446 const size_t str_desc_len = 12;
1447 struct usb_device *udev = hid_to_usb_dev(hdev);
1448 u8 *buf = kmemdup(rdesc_init_packet, rdesc_init_size, GFP_KERNEL);
1449 s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
1450 int actual_len, rc;
1451 u16 resolution;
1452
1453 if (hdev == NULL || p == NULL)
1454 return -EINVAL;
1455
1456 rc = usb_interrupt_msg(
1457 udev,
1458 usb_sndintpipe(udev, probe_endpoint),
1459 buf,
1460 rdesc_init_size,
1461 &actual_len,
1462 USB_CTRL_SET_TIMEOUT);
1463 kfree(buf);
1464 if (rc == -EPIPE) {
1465 hid_err(hdev, "broken pipe sending init packet\n");
1466 return rc;
1467 } else if (rc < 0) {
1468 hid_err(hdev, "failed sending init packet: %d\n", rc);
1469 return rc;
1470 } else if (actual_len != rdesc_init_size) {
1471 hid_err(hdev,
1472 "failed to transfer complete init packet, only %d bytes sent\n",
1473 actual_len);
1474 return -1;
1475 }
1476
1477 rc = uclogic_params_get_str_desc(&buf, hdev, 100, str_desc_len);
1478 if (rc != str_desc_len) {
1479 if (rc == -EPIPE) {
1480 hid_err(hdev,
1481 "string descriptor with pen parameters not found\n");
1482 } else if (rc < 0) {
1483 hid_err(hdev,
1484 "failed retrieving pen parameters: %d\n", rc);
1485 } else {
1486 hid_err(hdev,
1487 "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
1488 rc,
> 1489 str_desc_len);
1490 rc = -1;
1491 }
1492 kfree(buf);
1493 return rc;
1494 }
1495
1496 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = get_unaligned_le16(buf + 2);
1497 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = get_unaligned_le16(buf + 4);
1498 /* buf + 6 is the number of pad buttons? Its 0x0008 */
1499 desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] =
1500 get_unaligned_le16(buf + 8);
1501 resolution = get_unaligned_le16(buf + 10);
1502 kfree(buf);
1503 if (resolution == 0) {
1504 hid_err(hdev, "resolution of 0 in descriptor string\n");
1505 return -1;
1506 }
1507 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_PM] =
1508 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] * 1000 / resolution;
1509 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_PM] =
1510 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] * 1000 / resolution;
1511
1512 hid_dbg(hdev,
1513 "Received parameters: X: %d Y: %d Pressure: %d Resolution: %u\n",
1514 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM],
1515 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM],
1516 desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM],
1517 resolution);
1518
1519 p->pen.desc_ptr = uclogic_rdesc_template_apply(
1520 rdesc_tablet_arr,
1521 rdesc_tablet_size,
1522 desc_params,
1523 ARRAY_SIZE(desc_params));
1524 p->pen.desc_size = rdesc_tablet_size;
1525 p->pen.id = 0x02;
1526
1527 rc = uclogic_params_frame_init_with_desc(
1528 &p->frame_list[0],
1529 rdesc_frame_arr,
1530 rdesc_frame_size,
1531 UCLOGIC_RDESC_V1_FRAME_ID);
1532 if (rc < 0) {
1533 hid_err(hdev, "initializing frame params failed: %d\n", rc);
1534 return rc;
1535 }
1536
1537 p->pen.subreport_list[0].value = 0xf0;
1538 p->pen.subreport_list[0].id = p->frame_list[0].id;
1539
1540 return 0;
1541 }
1542

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

2022-12-26 10:47:25

by kernel test robot

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi Joshua,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20221226-112302
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/2068502.VLH7GnMWUR%40adrastea
patch subject: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro
config: mips-allmodconfig
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/51d8c9b14fc55dcb5b8e3f7f4e0cc582ce9097d7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20221226-112302
git checkout 51d8c9b14fc55dcb5b8e3f7f4e0cc582ce9097d7
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/device.h:15,
from include/linux/usb.h:19,
from drivers/hid/hid-uclogic-params.h:19,
from drivers/hid/hid-uclogic-params.c:16:
drivers/hid/hid-uclogic-params.c: In function 'uclogic_params_init_ugee_xppen_pro':
>> drivers/hid/hid-uclogic-params.c:1487:33: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
1487 | "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
include/linux/hid.h:1206:9: note: in expansion of macro 'dev_err'
1206 | dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
| ^~~~~~~
drivers/hid/hid-uclogic-params.c:1486:25: note: in expansion of macro 'hid_err'
1486 | hid_err(hdev,
| ^~~~~~~
drivers/hid/hid-uclogic-params.c:1487:111: note: format string is defined here
1487 | "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
| ~~^
| |
| long unsigned int
| %u


vim +1487 drivers/hid/hid-uclogic-params.c

1422
1423
1424 /*
1425 * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
1426 *
1427 * @hdev: The HID device of the tablet interface to initialize and get
1428 * parameters from. Cannot be NULL.
1429 * @params: Parameters to fill in (to be cleaned with
1430 * uclogic_params_cleanup()). Not modified in case of error.
1431 * Cannot be NULL.
1432 *
1433 * Returns:
1434 * Zero, if successful. A negative errno code on error.
1435 */
1436 static int uclogic_params_init_ugee_xppen_pro(struct hid_device *hdev,
1437 struct uclogic_params *p,
1438 const u8 probe_endpoint,
1439 const u8 rdesc_init_packet[],
1440 const size_t rdesc_init_size,
1441 const u8 rdesc_tablet_arr[],
1442 const size_t rdesc_tablet_size,
1443 const u8 rdesc_frame_arr[],
1444 const size_t rdesc_frame_size)
1445 {
1446 const size_t str_desc_len = 12;
1447 struct usb_device *udev = hid_to_usb_dev(hdev);
1448 u8 *buf = kmemdup(rdesc_init_packet, rdesc_init_size, GFP_KERNEL);
1449 s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
1450 int actual_len, rc;
1451 u16 resolution;
1452
1453 if (hdev == NULL || p == NULL)
1454 return -EINVAL;
1455
1456 rc = usb_interrupt_msg(
1457 udev,
1458 usb_sndintpipe(udev, probe_endpoint),
1459 buf,
1460 rdesc_init_size,
1461 &actual_len,
1462 USB_CTRL_SET_TIMEOUT);
1463 kfree(buf);
1464 if (rc == -EPIPE) {
1465 hid_err(hdev, "broken pipe sending init packet\n");
1466 return rc;
1467 } else if (rc < 0) {
1468 hid_err(hdev, "failed sending init packet: %d\n", rc);
1469 return rc;
1470 } else if (actual_len != rdesc_init_size) {
1471 hid_err(hdev,
1472 "failed to transfer complete init packet, only %d bytes sent\n",
1473 actual_len);
1474 return -1;
1475 }
1476
1477 rc = uclogic_params_get_str_desc(&buf, hdev, 100, str_desc_len);
1478 if (rc != str_desc_len) {
1479 if (rc == -EPIPE) {
1480 hid_err(hdev,
1481 "string descriptor with pen parameters not found\n");
1482 } else if (rc < 0) {
1483 hid_err(hdev,
1484 "failed retrieving pen parameters: %d\n", rc);
1485 } else {
1486 hid_err(hdev,
> 1487 "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
1488 rc,
1489 str_desc_len);
1490 rc = -1;
1491 }
1492 kfree(buf);
1493 return rc;
1494 }
1495
1496 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = get_unaligned_le16(buf + 2);
1497 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = get_unaligned_le16(buf + 4);
1498 /* buf + 6 is the number of pad buttons? Its 0x0008 */
1499 desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] =
1500 get_unaligned_le16(buf + 8);
1501 resolution = get_unaligned_le16(buf + 10);
1502 kfree(buf);
1503 if (resolution == 0) {
1504 hid_err(hdev, "resolution of 0 in descriptor string\n");
1505 return -1;
1506 }
1507 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_PM] =
1508 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] * 1000 / resolution;
1509 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_PM] =
1510 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] * 1000 / resolution;
1511
1512 hid_dbg(hdev,
1513 "Received parameters: X: %d Y: %d Pressure: %d Resolution: %u\n",
1514 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM],
1515 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM],
1516 desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM],
1517 resolution);
1518
1519 p->pen.desc_ptr = uclogic_rdesc_template_apply(
1520 rdesc_tablet_arr,
1521 rdesc_tablet_size,
1522 desc_params,
1523 ARRAY_SIZE(desc_params));
1524 p->pen.desc_size = rdesc_tablet_size;
1525 p->pen.id = 0x02;
1526
1527 rc = uclogic_params_frame_init_with_desc(
1528 &p->frame_list[0],
1529 rdesc_frame_arr,
1530 rdesc_frame_size,
1531 UCLOGIC_RDESC_V1_FRAME_ID);
1532 if (rc < 0) {
1533 hid_err(hdev, "initializing frame params failed: %d\n", rc);
1534 return rc;
1535 }
1536
1537 p->pen.subreport_list[0].value = 0xf0;
1538 p->pen.subreport_list[0].id = p->frame_list[0].id;
1539
1540 return 0;
1541 }
1542

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

2022-12-26 11:46:17

by kernel test robot

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi Joshua,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20221226-112302
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/2068502.VLH7GnMWUR%40adrastea
patch subject: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro
config: arc-randconfig-r003-20221226
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/51d8c9b14fc55dcb5b8e3f7f4e0cc582ce9097d7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20221226-112302
git checkout 51d8c9b14fc55dcb5b8e3f7f4e0cc582ce9097d7
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/hid/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/device.h:15,
from include/linux/usb.h:19,
from drivers/hid/hid-uclogic-params.h:19,
from drivers/hid/hid-uclogic-params.c:16:
drivers/hid/hid-uclogic-params.c: In function 'uclogic_params_init_ugee_xppen_pro':
>> drivers/hid/hid-uclogic-params.c:1487:33: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
1487 | "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
include/linux/hid.h:1206:9: note: in expansion of macro 'dev_err'
1206 | dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
| ^~~~~~~
drivers/hid/hid-uclogic-params.c:1486:25: note: in expansion of macro 'hid_err'
1486 | hid_err(hdev,
| ^~~~~~~
drivers/hid/hid-uclogic-params.c:1487:111: note: format string is defined here
1487 | "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
| ~~^
| |
| long unsigned int
| %u


vim +1487 drivers/hid/hid-uclogic-params.c

1422
1423
1424 /*
1425 * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
1426 *
1427 * @hdev: The HID device of the tablet interface to initialize and get
1428 * parameters from. Cannot be NULL.
1429 * @params: Parameters to fill in (to be cleaned with
1430 * uclogic_params_cleanup()). Not modified in case of error.
1431 * Cannot be NULL.
1432 *
1433 * Returns:
1434 * Zero, if successful. A negative errno code on error.
1435 */
1436 static int uclogic_params_init_ugee_xppen_pro(struct hid_device *hdev,
1437 struct uclogic_params *p,
1438 const u8 probe_endpoint,
1439 const u8 rdesc_init_packet[],
1440 const size_t rdesc_init_size,
1441 const u8 rdesc_tablet_arr[],
1442 const size_t rdesc_tablet_size,
1443 const u8 rdesc_frame_arr[],
1444 const size_t rdesc_frame_size)
1445 {
1446 const size_t str_desc_len = 12;
1447 struct usb_device *udev = hid_to_usb_dev(hdev);
1448 u8 *buf = kmemdup(rdesc_init_packet, rdesc_init_size, GFP_KERNEL);
1449 s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
1450 int actual_len, rc;
1451 u16 resolution;
1452
1453 if (hdev == NULL || p == NULL)
1454 return -EINVAL;
1455
1456 rc = usb_interrupt_msg(
1457 udev,
1458 usb_sndintpipe(udev, probe_endpoint),
1459 buf,
1460 rdesc_init_size,
1461 &actual_len,
1462 USB_CTRL_SET_TIMEOUT);
1463 kfree(buf);
1464 if (rc == -EPIPE) {
1465 hid_err(hdev, "broken pipe sending init packet\n");
1466 return rc;
1467 } else if (rc < 0) {
1468 hid_err(hdev, "failed sending init packet: %d\n", rc);
1469 return rc;
1470 } else if (actual_len != rdesc_init_size) {
1471 hid_err(hdev,
1472 "failed to transfer complete init packet, only %d bytes sent\n",
1473 actual_len);
1474 return -1;
1475 }
1476
1477 rc = uclogic_params_get_str_desc(&buf, hdev, 100, str_desc_len);
1478 if (rc != str_desc_len) {
1479 if (rc == -EPIPE) {
1480 hid_err(hdev,
1481 "string descriptor with pen parameters not found\n");
1482 } else if (rc < 0) {
1483 hid_err(hdev,
1484 "failed retrieving pen parameters: %d\n", rc);
1485 } else {
1486 hid_err(hdev,
> 1487 "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
1488 rc,
1489 str_desc_len);
1490 rc = -1;
1491 }
1492 kfree(buf);
1493 return rc;
1494 }
1495
1496 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = get_unaligned_le16(buf + 2);
1497 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = get_unaligned_le16(buf + 4);
1498 /* buf + 6 is the number of pad buttons? Its 0x0008 */
1499 desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] =
1500 get_unaligned_le16(buf + 8);
1501 resolution = get_unaligned_le16(buf + 10);
1502 kfree(buf);
1503 if (resolution == 0) {
1504 hid_err(hdev, "resolution of 0 in descriptor string\n");
1505 return -1;
1506 }
1507 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_PM] =
1508 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] * 1000 / resolution;
1509 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_PM] =
1510 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] * 1000 / resolution;
1511
1512 hid_dbg(hdev,
1513 "Received parameters: X: %d Y: %d Pressure: %d Resolution: %u\n",
1514 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM],
1515 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM],
1516 desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM],
1517 resolution);
1518
1519 p->pen.desc_ptr = uclogic_rdesc_template_apply(
1520 rdesc_tablet_arr,
1521 rdesc_tablet_size,
1522 desc_params,
1523 ARRAY_SIZE(desc_params));
1524 p->pen.desc_size = rdesc_tablet_size;
1525 p->pen.id = 0x02;
1526
1527 rc = uclogic_params_frame_init_with_desc(
1528 &p->frame_list[0],
1529 rdesc_frame_arr,
1530 rdesc_frame_size,
1531 UCLOGIC_RDESC_V1_FRAME_ID);
1532 if (rc < 0) {
1533 hid_err(hdev, "initializing frame params failed: %d\n", rc);
1534 return rc;
1535 }
1536
1537 p->pen.subreport_list[0].value = 0xf0;
1538 p->pen.subreport_list[0].id = p->frame_list[0].id;
1539
1540 return 0;
1541 }
1542

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

2022-12-29 10:03:19

by Dan Carpenter

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi Joshua,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20221226-112302
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/2068502.VLH7GnMWUR%40adrastea
patch subject: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro
config: i386-randconfig-m021-20221226
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>

New smatch warnings:
drivers/hid/hid-uclogic-params.c:1453 uclogic_params_init_ugee_xppen_pro() warn: variable dereferenced before check 'hdev' (see line 1447)
drivers/hid/hid-uclogic-params.c:1454 uclogic_params_init_ugee_xppen_pro() warn: possible memory leak of 'buf'
drivers/hid/hid-uclogic-params.c:1492 uclogic_params_init_ugee_xppen_pro() error: double free of 'buf'

Old smatch warnings:
drivers/hid/hid-uclogic-params.c:1502 uclogic_params_init_ugee_xppen_pro() error: double free of 'buf'

vim +/hdev +1453 drivers/hid/hid-uclogic-params.c

51d8c9b14fc55dc Aren Villanueva 2022-12-25 1436 static int uclogic_params_init_ugee_xppen_pro(struct hid_device *hdev,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1437 struct uclogic_params *p,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1438 const u8 probe_endpoint,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1439 const u8 rdesc_init_packet[],
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1440 const size_t rdesc_init_size,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1441 const u8 rdesc_tablet_arr[],
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1442 const size_t rdesc_tablet_size,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1443 const u8 rdesc_frame_arr[],
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1444 const size_t rdesc_frame_size)
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1445 {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1446 const size_t str_desc_len = 12;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 @1447 struct usb_device *udev = hid_to_usb_dev(hdev);
^^^^
Dereference.

51d8c9b14fc55dc Aren Villanueva 2022-12-25 1448 u8 *buf = kmemdup(rdesc_init_packet, rdesc_init_size, GFP_KERNEL);

Never put functions which can fail in the declaration block. This
allocation has no check for NULL (common problem when done in
declaration block).

51d8c9b14fc55dc Aren Villanueva 2022-12-25 1449 s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1450 int actual_len, rc;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1451 u16 resolution;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1452
51d8c9b14fc55dc Aren Villanueva 2022-12-25 @1453 if (hdev == NULL || p == NULL)
^^^^^^^^^^^^
Checked to late.

51d8c9b14fc55dc Aren Villanueva 2022-12-25 @1454 return -EINVAL;

Needs a kfree(buf);

51d8c9b14fc55dc Aren Villanueva 2022-12-25 1455
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1456 rc = usb_interrupt_msg(
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1457 udev,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1458 usb_sndintpipe(udev, probe_endpoint),
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1459 buf,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1460 rdesc_init_size,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1461 &actual_len,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1462 USB_CTRL_SET_TIMEOUT);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1463 kfree(buf);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1464 if (rc == -EPIPE) {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1465 hid_err(hdev, "broken pipe sending init packet\n");
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1466 return rc;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1467 } else if (rc < 0) {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1468 hid_err(hdev, "failed sending init packet: %d\n", rc);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1469 return rc;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1470 } else if (actual_len != rdesc_init_size) {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1471 hid_err(hdev,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1472 "failed to transfer complete init packet, only %d bytes sent\n",
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1473 actual_len);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1474 return -1;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1475 }
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1476
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1477 rc = uclogic_params_get_str_desc(&buf, hdev, 100, str_desc_len);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1478 if (rc != str_desc_len) {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1479 if (rc == -EPIPE) {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1480 hid_err(hdev,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1481 "string descriptor with pen parameters not found\n");
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1482 } else if (rc < 0) {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1483 hid_err(hdev,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1484 "failed retrieving pen parameters: %d\n", rc);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1485 } else {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1486 hid_err(hdev,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1487 "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1488 rc,
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1489 str_desc_len);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1490 rc = -1;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1491 }
51d8c9b14fc55dc Aren Villanueva 2022-12-25 @1492 kfree(buf);

If uclogic_params_get_str_desc() fails then this is a double free.

51d8c9b14fc55dc Aren Villanueva 2022-12-25 1493 return rc;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1494 }
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1495
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1496 desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = get_unaligned_le16(buf + 2);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1497 desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = get_unaligned_le16(buf + 4);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1498 /* buf + 6 is the number of pad buttons? Its 0x0008 */
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1499 desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] =
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1500 get_unaligned_le16(buf + 8);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1501 resolution = get_unaligned_le16(buf + 10);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1502 kfree(buf);
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1503 if (resolution == 0) {
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1504 hid_err(hdev, "resolution of 0 in descriptor string\n");
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1505 return -1;
51d8c9b14fc55dc Aren Villanueva 2022-12-25 1506 }

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-12-29 19:24:54

by José Expósito

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi Joshua,

Thanks a lot for your patch!

It might cause conflicts with [1] and [2], so we'll need to rebase on
top of each other work at some point.

[1] https://lore.kernel.org/linux-input/[email protected]/T/
[2] https://lore.kernel.org/linux-input/[email protected]/T/

> From: Aren Villanueva <[email protected]>
>
> Adds support for the XP-PEN Artist 22R Pro in uclogic, including the stylus,
> frame and pen pressure support.
>
> I did not do the research for this hardware, or the original patch - that work
> has been done by Aren Villanueva. For some reason they decided not to merge
> it. My changes include remapping the stupid amount of tablet frame buttons,
> cleaning up the code to match kernel style, and other small stuff.
>
> The tablet is (almost) fully functional even when uclogic doesn't handle it.
> Without initialization, the tablet has some sort of "basic driverless mode"
> that allows the tablet frame buttons to have some default keys associated with
> them (CTRL-S, CTRL-Z, that kind of stuff), but unfortunately the stylus pen
> semi-works. While pressure sensitivity works, only one stylus button functions
> correctly. Since the initialization process differs for Pro series tablets, the
> new function uclogic_params_init_ugee_xppen_pro had to be introduced. I also
> added USB HID IDs for this tablet too, but it's classified under the UGEE
> vendor ID.
>
> One of the more strange things I had to do is figure out a way to remap the
> buttons since there are 20 of them in total, and of course there are more
> buttons than there are BTN constants defined for us. When running without
> uclogic, it starts at BTN_0, ends at BTN_8 and the tablet starts reporting
> nonsensical keycodes so just leaving it alone isn't an option. I'm testing
> this under a libinput system, which has a list of buttons it considers "tablet
> pad buttons" which are notably BTN_0, BTN_1, so on and some
> gamepad/joystick buttons. So I created a new array called
> uclogic_extra_input_mapping for 20 working inputs.
>
> Another weird feature of this tablet is the second dial, which the original
> patchset introduced a new uclogic_frame param to handle since it seems it
> throws both dials values into one byte. The left wheel is considered EV_WHEEL
> and the other, EV_HWHEEL which seems fine to me. I also added the new param to
> the debug messages too.
>
> Link: https://github.com/DIGImend/digimend-kernel-drivers/pull/557
> Signed-off-by: Joshua Goins <[email protected]>
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-uclogic-core.c | 68 +++++++++++++-
> drivers/hid/hid-uclogic-params.c | 147 +++++++++++++++++++++++++++++++
> drivers/hid/hid-uclogic-params.h | 5 ++
> drivers/hid/hid-uclogic-rdesc.c | 119 +++++++++++++++++++++++++
> drivers/hid/hid-uclogic-rdesc.h | 15 ++++
> 6 files changed, 351 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 82713ef3aaa6..81d04054229a 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1298,6 +1298,7 @@
> #define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_L 0x0935
> #define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S 0x0909
> #define USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06 0x0078
> +#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO 0x091b
> #define USB_DEVICE_ID_UGEE_TABLET_G5 0x0074
> #define USB_DEVICE_ID_UGEE_TABLET_EX07S 0x0071
> #define USB_DEVICE_ID_UGEE_TABLET_RAINBOW_CV720 0x0055
> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> index 7fa6fe04f1b2..ebc93b354f7a 100644
> --- a/drivers/hid/hid-uclogic-core.c
> +++ b/drivers/hid/hid-uclogic-core.c
> @@ -81,6 +81,30 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> return rdesc;
> }
>
> +/* Buttons considered valid tablet pad inputs. */
> +const unsigned int uclogic_extra_input_mapping[] = {

Sparse nitpick: "uclogic_extra_input_mapping" can be made static.

> + BTN_0,
> + BTN_1,
> + BTN_2,
> + BTN_3,
> + BTN_4,
> + BTN_5,
> + BTN_6,
> + BTN_7,
> + BTN_8,
> + BTN_RIGHT,
> + BTN_MIDDLE,
> + BTN_SIDE,
> + BTN_EXTRA,
> + BTN_FORWARD,
> + BTN_BACK,
> + BTN_B,
> + BTN_A,
> + BTN_BASE,
> + BTN_BASE2,
> + BTN_X
> +};
> +
> static int uclogic_input_mapping(struct hid_device *hdev,
> struct hid_input *hi,
> struct hid_field *field,
> @@ -91,9 +115,27 @@ static int uclogic_input_mapping(struct hid_device *hdev,
> struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
> struct uclogic_params *params = &drvdata->params;
>
> - /* Discard invalid pen usages */
> - if (params->pen.usage_invalid && (field->application == HID_DG_PEN))
> - return -1;
> + if (field->application == HID_GD_KEYPAD) {
> + /*
> + * Remap input buttons to sensible ones that are not invalid.
> + * This only affects previous behavior for devices with more than ten or so buttons.
> + */
> + const int key = (usage->hid & HID_USAGE) - 1;
> +
> + if (key > 0 && key < ARRAY_SIZE(uclogic_extra_input_mapping)) {
> + hid_map_usage(hi,
> + usage,
> + bit,
> + max,
> + EV_KEY,
> + uclogic_extra_input_mapping[key]);
> + return 1;
> + }
> + } else if (field->application == HID_DG_PEN) {
> + /* Discard invalid pen usages */
> + if (params->pen.usage_invalid)
> + return -1;
> + }
>
> /* Let hid-core decide what to do */
> return 0;
> @@ -403,8 +445,24 @@ static int uclogic_raw_event_frame(
>
> /* If need to, and can, transform the bitmap dial reports */
> if (frame->bitmap_dial_byte > 0 && frame->bitmap_dial_byte < size) {
> - if (data[frame->bitmap_dial_byte] == 2)
> + switch (data[frame->bitmap_dial_byte]) {
> + case 2:
> data[frame->bitmap_dial_byte] = -1;
> + break;
> +
> + /* Everything below here is for tablets that shove multiple dials into 1 byte */
> + case 4:
> + case 0x10:
> + data[frame->bitmap_dial_byte] = 0;
> + data[frame->bitmap_second_dial_destination_byte] = 1;
> + break;
> +
> + case 8:
> + case 0x20:
> + data[frame->bitmap_dial_byte] = 0;
> + data[frame->bitmap_second_dial_destination_byte] = -1;
> + break;
> + }

Is it possible to receive a report with information from both dials at
the same time?

I'm asking because I'm trying to understand what is the meaning of the
0x10 and 0x20 values and I wonder if they are generated when both dials
are used at the same time.

> }
>
> return 0;
> @@ -531,6 +589,8 @@ static const struct hid_device_id uclogic_devices[] = {
> USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S) },
> { HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
> USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
> + USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO) },
> { }
> };
> MODULE_DEVICE_TABLE(hid, uclogic_devices);
> diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
> index cd1233d7e253..86a1dfa2a4c0 100644
> --- a/drivers/hid/hid-uclogic-params.c
> +++ b/drivers/hid/hid-uclogic-params.c
> @@ -103,6 +103,8 @@ static void uclogic_params_frame_hid_dbg(
> frame->touch_flip_at);
> hid_dbg(hdev, "\t\t.bitmap_dial_byte = %u\n",
> frame->bitmap_dial_byte);
> + hid_dbg(hdev, "\t\t.bitmap_second_dial_destination_byte = %u\n",
> + frame->bitmap_second_dial_destination_byte);
> }
>
> /**
> @@ -1418,6 +1420,126 @@ static int uclogic_params_ugee_v2_init(struct uclogic_params *params,
> return rc;
> }
>
> +
> +/*
> + * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
> + *
> + * @hdev: The HID device of the tablet interface to initialize and get
> + * parameters from. Cannot be NULL.
> + * @params: Parameters to fill in (to be cleaned with
> + * uclogic_params_cleanup()). Not modified in case of error.
> + * Cannot be NULL.
> + *
> + * Returns:
> + * Zero, if successful. A negative errno code on error.
> + */
> +static int uclogic_params_init_ugee_xppen_pro(struct hid_device *hdev,

You can probably reuse uclogic_params_ugee_v2_init() or at least reuse
uclogic_probe_interface() and uclogic_params_parse_ugee_v2_desc() if
for some reason we need custom logic for this tablet.

> + struct uclogic_params *p,
> + const u8 probe_endpoint,
> + const u8 rdesc_init_packet[],
> + const size_t rdesc_init_size,
> + const u8 rdesc_tablet_arr[],
> + const size_t rdesc_tablet_size,
> + const u8 rdesc_frame_arr[],
> + const size_t rdesc_frame_size)
> +{
> + const size_t str_desc_len = 12;
> + struct usb_device *udev = hid_to_usb_dev(hdev);
> + u8 *buf = kmemdup(rdesc_init_packet, rdesc_init_size, GFP_KERNEL);
> + s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
> + int actual_len, rc;
> + u16 resolution;
> +
> + if (hdev == NULL || p == NULL)
> + return -EINVAL;
> +
> + rc = usb_interrupt_msg(
> + udev,
> + usb_sndintpipe(udev, probe_endpoint),
> + buf,
> + rdesc_init_size,
> + &actual_len,
> + USB_CTRL_SET_TIMEOUT);
> + kfree(buf);
> + if (rc == -EPIPE) {
> + hid_err(hdev, "broken pipe sending init packet\n");
> + return rc;
> + } else if (rc < 0) {
> + hid_err(hdev, "failed sending init packet: %d\n", rc);
> + return rc;
> + } else if (actual_len != rdesc_init_size) {
> + hid_err(hdev,
> + "failed to transfer complete init packet, only %d bytes sent\n",
> + actual_len);
> + return -1;
> + }
> +
> + rc = uclogic_params_get_str_desc(&buf, hdev, 100, str_desc_len);
> + if (rc != str_desc_len) {
> + if (rc == -EPIPE) {
> + hid_err(hdev,
> + "string descriptor with pen parameters not found\n");
> + } else if (rc < 0) {
> + hid_err(hdev,
> + "failed retrieving pen parameters: %d\n", rc);
> + } else {
> + hid_err(hdev,
> + "string descriptor with pen parameters has invalid length (got %d, expected %lu)\n",
> + rc,
> + str_desc_len);
> + rc = -1;
> + }
> + kfree(buf);
> + return rc;
> + }
> +
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = get_unaligned_le16(buf + 2);
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = get_unaligned_le16(buf + 4);
> + /* buf + 6 is the number of pad buttons? Its 0x0008 */

Is this value 8? In all the models I have seen so far this is indeed
the number of buttons.

Also, what's the value of buf[6]? As you can see in uclogic_params_parse_ugee_v2_desc(),
this field is the frame type. I'd be nice to know whether a different
frame type is reported when 2 dials are present or not.

Could you attach the contents of the 14 bytes of "buf", please? I'd be
nice to have a look and see if we can reuse as much code as possible.

> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] =
> + get_unaligned_le16(buf + 8);
> + resolution = get_unaligned_le16(buf + 10);
> + kfree(buf);
> + if (resolution == 0) {
> + hid_err(hdev, "resolution of 0 in descriptor string\n");
> + return -1;
> + }
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_PM] =
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] * 1000 / resolution;
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_PM] =
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] * 1000 / resolution;
> +
> + hid_dbg(hdev,
> + "Received parameters: X: %d Y: %d Pressure: %d Resolution: %u\n",
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM],
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM],
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM],
> + resolution);
> +
> + p->pen.desc_ptr = uclogic_rdesc_template_apply(
> + rdesc_tablet_arr,
> + rdesc_tablet_size,
> + desc_params,
> + ARRAY_SIZE(desc_params));
> + p->pen.desc_size = rdesc_tablet_size;
> + p->pen.id = 0x02;
> +
> + rc = uclogic_params_frame_init_with_desc(
> + &p->frame_list[0],
> + rdesc_frame_arr,
> + rdesc_frame_size,
> + UCLOGIC_RDESC_V1_FRAME_ID);
> + if (rc < 0) {
> + hid_err(hdev, "initializing frame params failed: %d\n", rc);
> + return rc;
> + }
> +
> + p->pen.subreport_list[0].value = 0xf0;
> + p->pen.subreport_list[0].id = p->frame_list[0].id;
> +
> + return 0;
> +}
> +
> /**
> * uclogic_params_init() - initialize a tablet interface and discover its
> * parameters.
> @@ -1728,6 +1850,31 @@ int uclogic_params_init(struct uclogic_params *params,
> uclogic_params_init_invalid(&p);
> }
>
> + break;
> + case VID_PID(USB_VENDOR_ID_UGEE,
> + USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO):

Ideally, we should be able to handle this tablet with the other UGEE v2
tablets.

> + /* Ignore non-pen interfaces */
> + if (bInterfaceNumber != 2) {
> + uclogic_params_init_invalid(&p);
> + break;
> + }
> +
> + rc = uclogic_params_init_ugee_xppen_pro(
> + hdev, &p, UCLOGIC_RDESC_UGEE_XPPEN_PROBE_ENDPOINT_TYPE1,
> + uclogic_rdesc_xppen_init_packet_type1_arr,
> + uclogic_rdesc_xppen_init_packet_type1_size,
> + uclogic_rdesc_xppen_pro_stylus_type1_arr,
> + uclogic_rdesc_xppen_pro_stylus_type1_size,
> + uclogic_rdesc_xppen_artist_22r_pro_frame_arr,
> + uclogic_rdesc_xppen_artist_22r_pro_frame_size);
> + if (rc != 0) {
> + hid_err(hdev, "failed creating frame parameters: %d\n", rc);
> + goto cleanup;
> + }
> +
> + p.frame_list[0].bitmap_dial_byte = 7;
> + p.frame_list[0].bitmap_second_dial_destination_byte = 8;
> +
> break;
> }
>
> diff --git a/drivers/hid/hid-uclogic-params.h b/drivers/hid/hid-uclogic-params.h
> index a97477c02ff8..6621a75a4b1a 100644
> --- a/drivers/hid/hid-uclogic-params.h
> +++ b/drivers/hid/hid-uclogic-params.h
> @@ -171,6 +171,11 @@ struct uclogic_params_frame {
> * counterclockwise, as opposed to the normal 1 and -1.
> */
> unsigned int bitmap_dial_byte;
> + /*
> + * Destination offset for the second bitmap dial byte, if the tablet
> + * supports a second dial at all.
> + */
> + unsigned int bitmap_second_dial_destination_byte;
> };
>
> /*
> diff --git a/drivers/hid/hid-uclogic-rdesc.c b/drivers/hid/hid-uclogic-rdesc.c
> index fb40775f5f5b..b55dbe5017c1 100644
> --- a/drivers/hid/hid-uclogic-rdesc.c
> +++ b/drivers/hid/hid-uclogic-rdesc.c
> @@ -1185,6 +1185,125 @@ const __u8 uclogic_rdesc_xppen_deco01_frame_arr[] = {
> const size_t uclogic_rdesc_xppen_deco01_frame_size =
> sizeof(uclogic_rdesc_xppen_deco01_frame_arr);
>
> +/* Fix report descriptor for XP-Pen init packet type 1 */
> +const __u8 uclogic_rdesc_xppen_init_packet_type1_arr[] = {
> + 0x02, 0xb0, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +};

This array is already declared in uclogic_params_ugee_v2_init(), which,
hopefully, we will be able to reuse. Otherwise, you might be interested
in this commit (not merged yet):
https://lore.kernel.org/linux-input/[email protected]/T/#u

> +
> +const size_t uclogic_rdesc_xppen_init_packet_type1_size =
> + sizeof(uclogic_rdesc_xppen_init_packet_type1_arr);
> +
> +/* Fixed report descriptor for XP-Pen Pro Stylus type 1 (10 bytes) */
> +const __u8 uclogic_rdesc_xppen_pro_stylus_type1_arr[] = {

Can't "uclogic_rdesc_ugee_v2_pen_template_arr" be used instead?

> + 0x05, 0x0D, /* Usage Page (Digitizer), */
> + 0x09, 0x02, /* Usage (Pen), */
> + 0xA1, 0x01, /* Collection (Application), */
> + 0x85, 0x02, /* Report ID (2), */
> + 0x09, 0x20, /* Usage (Stylus), */
> + 0xA0, /* Collection (Physical), */
> + 0x14, /* Logical Minimum (0), */
> + 0x25, 0x01, /* Logical Maximum (1), */
> + 0x09, 0x42, /* Usage (Tip Switch), */
> + 0x09, 0x44, /* Usage (Barrel Switch), */
> + 0x09, 0x46, /* Usage (Tablet Pick), */
> + 0x75, 0x01, /* Report Size (1), */
> + 0x95, 0x03, /* Report Count (3), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0x95, 0x02, /* Report Count (2), */
> + 0x81, 0x01, /* Input (Constant), */
> + 0x09, 0x32, /* Usage (In Range), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0x95, 0x02, /* Report Count (2), */
> + 0x81, 0x01, /* Input (Constant), */
> + 0x75, 0x10, /* Report Size (16), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0xA4, /* Push, */
> + 0x05, 0x01, /* Usage Page (Desktop), */
> + 0x55, 0xFD, /* Unit Exponent (-3), */
> + 0x65, 0x13, /* Unit (Inch), */
> + 0x34, /* Physical Minimum (0), */
> + 0x09, 0x30, /* Usage (X), */
> + 0x27, UCLOGIC_RDESC_PEN_PH(X_LM),
> + /* Logical Maximum (PLACEHOLDER), */
> + 0x47, UCLOGIC_RDESC_PEN_PH(X_PM),
> + /* Physical Maximum (PLACEHOLDER), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0x09, 0x31, /* Usage (Y), */
> + 0x27, UCLOGIC_RDESC_PEN_PH(Y_LM),
> + /* Logical Maximum (PLACEHOLDER), */
> + 0x47, UCLOGIC_RDESC_PEN_PH(Y_PM),
> + /* Physical Maximum (PLACEHOLDER), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0xB4, /* Pop, */
> + 0x09, 0x30, /* Usage (Tip Pressure), */
> + 0x27, UCLOGIC_RDESC_PEN_PH(PRESSURE_LM),
> + /* Logical Maximum (PLACEHOLDER), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0xA4, /* Push, */
> + 0x54, /* Unit Exponent (0), */
> + 0x65, 0x14, /* Unit (Degrees), */
> + 0x35, 0xC3, /* Physical Minimum (-61), */
> + 0x45, 0x3C, /* Physical Maximum (60), */
> + 0x15, 0xC3, /* Logical Minimum (-61), */
> + 0x25, 0x3C, /* Logical Maximum (60), */
> + 0x75, 0x08, /* Report Size (8), */
> + 0x95, 0x02, /* Report Count (2), */
> + 0x09, 0x3D, /* Usage (X Tilt), */
> + 0x09, 0x3E, /* Usage (Y Tilt), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0xB4, /* Pop, */
> + 0xC0, /* End Collection, */
> + 0xC0 /* End Collection */
> +};
> +
> +const size_t uclogic_rdesc_xppen_pro_stylus_type1_size =
> + sizeof(uclogic_rdesc_xppen_pro_stylus_type1_arr);
> +
> +/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
> +const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[] = {

Have a look to "uclogic_rdesc_ugee_v2_frame_dial_template_arr", I don't
know if it could be used for your tablet.

> + 0x05, 0x01, /* Usage Page (Desktop), */
> + 0x09, 0x07, /* Usage (Keypad), */
> + 0xA1, 0x01, /* Collection (Application), */
> + 0x85, UCLOGIC_RDESC_V1_FRAME_ID,
> + /* Report ID (Virtual report), */
> + 0x05, 0x0D, /* Usage Page (Digitizer), */
> + 0x09, 0x39, /* Usage (Tablet Function Keys), */
> + 0xA0, /* Collection (Physical), */
> + 0x14, /* Logical Minimum (0), */
> + 0x25, 0x01, /* Logical Maximum (1), */
> + 0x75, 0x01, /* Report Size (1), */
> + 0x95, 0x08, /* Report Count (8), */
> + 0x81, 0x01, /* Input (Constant), */
> + 0x05, 0x09, /* Usage Page (Button), */
> + 0x19, 0x01, /* Usage Minimum (01h), */
> + 0x29, 0x14, /* Usage Maximum (14h), */

If your tablet reports its number of buttons, UCLOGIC_RDESC_FRAME_PH_BTN
can be used here.

> + 0x95, 0x14, /* Report Count (20), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0x95, 0x14, /* Report Count (20), */
> + 0x81, 0x01, /* Input (Constant), */
> + 0x05, 0x01, /* Usage Page (Desktop), */
> + 0x09, 0x38, /* Usage (Wheel), */
> + 0x75, 0x08, /* Report Size (8), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0x15, 0xFF, /* Logical Minimum (-1), */
> + 0x25, 0x08, /* Logical Maximum (8), */
> + 0x81, 0x06, /* Input (Variable, Relative), */
> + 0x05, 0x0C, /* Usage Page (Consumer Devices), */
> + 0x0A, 0x38, 0x02, /* Usage (AC PAN), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0x81, 0x06, /* Input (Variable, Relative), */
> + 0x26, 0xFF, 0x00, /* Logical Maximum (255), */
> + 0x75, 0x08, /* Report Size (8), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0xC0, /* End Collection */
> + 0xC0, /* End Collection */
> +};
> +
> +const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size =
> + sizeof(uclogic_rdesc_xppen_artist_22r_pro_frame_arr);
> +
> /**
> * uclogic_rdesc_template_apply() - apply report descriptor parameters to a
> * report descriptor template, creating a report descriptor. Copies the
> diff --git a/drivers/hid/hid-uclogic-rdesc.h b/drivers/hid/hid-uclogic-rdesc.h
> index a1f78c07293f..9e4055a5016e 100644
> --- a/drivers/hid/hid-uclogic-rdesc.h
> +++ b/drivers/hid/hid-uclogic-rdesc.h
> @@ -205,4 +205,19 @@ extern const size_t uclogic_rdesc_ugee_g5_frame_size;
> /* Least-significant bit of Ugee G5 frame rotary encoder state */
> #define UCLOGIC_RDESC_UGEE_G5_FRAME_RE_LSB 38
>
> +/* Probe endpoints for XP-Pen key activators */
> +#define UCLOGIC_RDESC_UGEE_XPPEN_PROBE_ENDPOINT_TYPE1 0x03
> +
> +/* Fix report descriptor for XP-Pen init packet type 1 */
> +extern const __u8 uclogic_rdesc_xppen_init_packet_type1_arr[];
> +extern const size_t uclogic_rdesc_xppen_init_packet_type1_size;
> +
> +/* Fixed report descriptor for XP-Pen Pro Stylus type 1 (10 bytes) */
> +extern const __u8 uclogic_rdesc_xppen_pro_stylus_type1_arr[];
> +extern const size_t uclogic_rdesc_xppen_pro_stylus_type1_size;
> +
> +/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
> +extern const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[];
> +extern const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size;
> +
> #endif /* _HID_UCLOGIC_RDESC_H */

2022-12-30 20:35:24

by Joshua Goins

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

> Hi Joshua,
>
> Thanks a lot for your patch!
>
> It might cause conflicts with [1] and [2], so we'll need to rebase on
> top of each other work at some point.
>
> [1]
> https://lore.kernel.org/linux-input/20221226123456.14822-1-jose.exposito89@
> gmail.com/T/ [2]
> https://lore.kernel.org/linux-input/20221226125454.16106-1-jose.exposito89@
> gmail.com/T/

I've seen those patches in the mailing list (and I thought they already got
merged heh) so I'll keep a close eye and rebase as needed.

>
> Sparse nitpick: "uclogic_extra_input_mapping" can be made static.
>

I somehow missed that, will change.

>
> Is it possible to receive a report with information from both dials at
> the same time?
>
> I'm asking because I'm trying to understand what is the meaning of the
> 0x10 and 0x20 values and I wonder if they are generated when both dials
> are used at the same time.
>

According to Aren's reference sheet he wrote up for this tablet, the 0x10 and
0x20 values are reported by the right dial (left frame:
02:f0:00:00:00:00:00:20:00:14, right frame: 02:f0:00:00:00:00:00:10:00:14).
Dumping the contents of data[frame->bitmap_dial_byte] yields the same result.
The left dial works like usual, 02 for left and 01 for right.

As for recieving reports of the two dials turning at the same time... I can't
seem to make it do that. This tablet is pretty big, and I can't turn the dials
fast enough.

>
> You can probably reuse uclogic_params_ugee_v2_init() or at least reuse
> uclogic_probe_interface() and uclogic_params_parse_ugee_v2_desc() if
> for some reason we need custom logic for this tablet.
>

I actually looked a little into the UGEE v2 init functions to see if I could
reuse anything (but to be honest, I really just skimmed) but I will take a
second look to see if I can consolidate it.

>
> Is this value 8? In all the models I have seen so far this is indeed
> the number of buttons.
>
> Also, what's the value of buf[6]? As you can see in
> uclogic_params_parse_ugee_v2_desc(), this field is the frame type. I'd be
> nice to know whether a different frame type is reported when 2 dials are
> present or not.
>
> Could you attach the contents of the 14 bytes of "buf", please? I'd be
> nice to have a look and see if we can reuse as much code as possible.
>

Yeah here's the 14 bytes of "buf" here:
buf[0] = 000c
buf[1] = 0003
buf[2] = 0030
buf[3] = 00ba
buf[4] = 009a
buf[5] = 0068
buf[6] = 0006
buf[7] = 0000
buf[8] = 00ff
buf[9] = 001f
buf[10] = 00ec
buf[11] = 0009
buf[12] = 0080
buf[13] = 0072

I'm not sure you made a typo, but buf[6] in uclogic_params_parse_ugee_v2_desc
is reading the button count, which reports as 6 for some reason. buf[7] is 0
though, so it appears that its still reporting as UCLOGIC_PARAMS_FRAME_BUTTONS
(I could just be misunderstanding the strdescs) or the frames are completely
different.

>
> Ideally, we should be able to handle this tablet with the other UGEE v2
> tablets.
>

Yeah I will consolidate this case if I manage to merge this with the other
UGEE v2 tablet support.

>
> This array is already declared in uclogic_params_ugee_v2_init(), which,
> hopefully, we will be able to reuse. Otherwise, you might be interested
> in this commit (not merged yet):
> https://lore.kernel.org/linux-input/20221226125454.16106-4-jose.exposito89@g
> mail.com/T/#u

I didn't see that array in ugee_v2_init, what I'll do is match the variables
defined in your cleanup patch (I believe it's closely identical already), so it
makes rebasing easier no matter what order they're merged in.

>
> Can't "uclogic_rdesc_ugee_v2_pen_template_arr" be used instead?
>

Yeah I think so, at first I didn't consider it but on closer inspection - the
offsets are the same (just for some reason, out of order). I'll be looking into
testing the pen using this descriptor on the second revision, maybe Pro-series
and UGEE v2 aren't so different after all!

>
> Have a look to "uclogic_rdesc_ugee_v2_frame_dial_template_arr", I don't
> know if it could be used for your tablet.
>

Same as above, same structure - different order (but it's all semantic, same
offsets) so I'll be consolidating if it works out.

>
> If your tablet reports its number of buttons, UCLOGIC_RDESC_FRAME_PH_BTN
> can be used here.
>

I'll be reviewing the descriptor data, ideally it should but I don't have high
hopes (as shown above) but I'm going to be testing it more.

Thank you for the review! I'll be resubmitting a second version of the patch
with your suggestions and making sure the kernel test bot is happy - and
hopefully there will be a lot less duplication structure-wise.

2023-01-01 16:14:11

by José Expósito

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi!

On Fri, Dec 30, 2022 at 03:02:41PM -0500, redstrate wrote:
> > Hi Joshua,
> >
> > Thanks a lot for your patch!
> >
> > It might cause conflicts with [1] and [2], so we'll need to rebase on
> > top of each other work at some point.
> >
> > [1]
> > https://lore.kernel.org/linux-input/20221226123456.14822-1-jose.exposito89@
> > gmail.com/T/ [2]
> > https://lore.kernel.org/linux-input/20221226125454.16106-1-jose.exposito89@
> > gmail.com/T/
>
> I've seen those patches in the mailing list (and I thought they already got
> merged heh) so I'll keep a close eye and rebase as needed.
>
> >
> > Sparse nitpick: "uclogic_extra_input_mapping" can be made static.
> >
>
> I somehow missed that, will change.
>
> >
> > Is it possible to receive a report with information from both dials at
> > the same time?
> >
> > I'm asking because I'm trying to understand what is the meaning of the
> > 0x10 and 0x20 values and I wonder if they are generated when both dials
> > are used at the same time.
> >
>
> According to Aren's reference sheet he wrote up for this tablet, the 0x10 and
> 0x20 values are reported by the right dial (left frame:
> 02:f0:00:00:00:00:00:20:00:14, right frame: 02:f0:00:00:00:00:00:10:00:14).
> Dumping the contents of data[frame->bitmap_dial_byte] yields the same result.
> The left dial works like usual, 02 for left and 01 for right.
>
> As for recieving reports of the two dials turning at the same time... I can't
> seem to make it do that. This tablet is pretty big, and I can't turn the dials
> fast enough.

Ah cool, then I guess we can remove the cases for "4" and "8"? I'd be
nice to stick with decimal numbers in all cases for consistency.

> >
> > You can probably reuse uclogic_params_ugee_v2_init() or at least reuse
> > uclogic_probe_interface() and uclogic_params_parse_ugee_v2_desc() if
> > for some reason we need custom logic for this tablet.
> >
>
> I actually looked a little into the UGEE v2 init functions to see if I could
> reuse anything (but to be honest, I really just skimmed) but I will take a
> second look to see if I can consolidate it.
>
> >
> > Is this value 8? In all the models I have seen so far this is indeed
> > the number of buttons.
> >
> > Also, what's the value of buf[6]? As you can see in
> > uclogic_params_parse_ugee_v2_desc(), this field is the frame type. I'd be
> > nice to know whether a different frame type is reported when 2 dials are
> > present or not.
> >
> > Could you attach the contents of the 14 bytes of "buf", please? I'd be
> > nice to have a look and see if we can reuse as much code as possible.
> >
>
> Yeah here's the 14 bytes of "buf" here:
> buf[0] = 000c
> buf[1] = 0003
> buf[2] = 0030
> buf[3] = 00ba
> buf[4] = 009a
> buf[5] = 0068
> buf[6] = 0006
> buf[7] = 0000
> buf[8] = 00ff
> buf[9] = 001f
> buf[10] = 00ec
> buf[11] = 0009
> buf[12] = 0080
> buf[13] = 0072
>
> I'm not sure you made a typo, but buf[6] in uclogic_params_parse_ugee_v2_desc
> is reading the button count, which reports as 6 for some reason. buf[7] is 0
> though, so it appears that its still reporting as UCLOGIC_PARAMS_FRAME_BUTTONS
> (I could just be misunderstanding the strdescs) or the frames are completely
> different.

Yes, my bad, I meant buf[7]. For the tablets I added so far it
indicated the frame type. However, it doesn't seem to be the case for
your tablet.

Also, buf[6] does not indicate the number of buttons (20?).

Did you check with Wireshark if the Windows driver is doing something
different for your tablet? It'd be nice to avoid adding quirks but it
might not be possible :S

We can ignore buf[12] and buf[14]. buf[0] indicates the size of the
descriptor (12), so the last two bytes are random memory.

> >
> > Ideally, we should be able to handle this tablet with the other UGEE v2
> > tablets.
> >
>
> Yeah I will consolidate this case if I manage to merge this with the other
> UGEE v2 tablet support.
>
> >
> > This array is already declared in uclogic_params_ugee_v2_init(), which,
> > hopefully, we will be able to reuse. Otherwise, you might be interested
> > in this commit (not merged yet):
> > https://lore.kernel.org/linux-input/20221226125454.16106-4-jose.exposito89@g
> > mail.com/T/#u
>
> I didn't see that array in ugee_v2_init, what I'll do is match the variables
> defined in your cleanup patch (I believe it's closely identical already), so it
> makes rebasing easier no matter what order they're merged in.
>
> >
> > Can't "uclogic_rdesc_ugee_v2_pen_template_arr" be used instead?
> >
>
> Yeah I think so, at first I didn't consider it but on closer inspection - the
> offsets are the same (just for some reason, out of order). I'll be looking into
> testing the pen using this descriptor on the second revision, maybe Pro-series
> and UGEE v2 aren't so different after all!
>
> >
> > Have a look to "uclogic_rdesc_ugee_v2_frame_dial_template_arr", I don't
> > know if it could be used for your tablet.
> >
>
> Same as above, same structure - different order (but it's all semantic, same
> offsets) so I'll be consolidating if it works out.
>
> >
> > If your tablet reports its number of buttons, UCLOGIC_RDESC_FRAME_PH_BTN
> > can be used here.
> >
>
> I'll be reviewing the descriptor data, ideally it should but I don't have high
> hopes (as shown above) but I'm going to be testing it more.
>
> Thank you for the review! I'll be resubmitting a second version of the patch
> with your suggestions and making sure the kernel test bot is happy - and
> hopefully there will be a lot less duplication structure-wise.
>
>
>
>

2023-01-01 16:21:59

by Joshua Goins

Subject: Re: [PATCH] HID: uclogic: Add support for XP-PEN Artist 22R Pro

>
> Ah cool, then I guess we can remove the cases for "4" and "8"? I'd be
> nice to stick with decimal numbers in all cases for consistency.
>

Oh yeah we totally could, I'm not sure why the double cases were there in the
first place - it might have been for another device included in the original
patch.

>
> Also, buf[6] does not indicate the number of buttons (20?).
>
> Did you check with Wireshark if the Windows driver is doing something
> different for your tablet? It'd be nice to avoid adding quirks but it
> might not be possible :S
>
> We can ignore buf[12] and buf[14]. buf[0] indicates the size of the
> descriptor (12), so the last two bytes are random memory.
>

I don't even think I have to drop into Windows, we have a userspace linux
program for XP-PEN but I haven't tested it with this tablet yet. Yeah, I'd
also like to avoid implementing device-specific quirks - hopefully I'll be able
to shrink the init code but I might not be able to get rid of all of it. It's
really weird that almost everything else (data frame wise) is the same, with
the exception of init. And yes, this tablet contains 20 buttons and nothing in
that buffer indicates button count :-/

2023-01-02 19:56:45

by Joshua Goins

Subject: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Adds support for the XP-PEN Artist 22R Pro, including stylus, tablet frame
and pen pressure.

The tablet has 20 buttons, but need to be remapped in order since the
device reports invalid keycodes. Existing tablet functionality should
not be inhibited, as BTN0-8 is still used.

New initialization functions are added since the device differs slightly
from other UGEE v2 devices.

Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Reported-by: José Expósito <[email protected]>
Signed-off-by: Joshua Goins <[email protected]>
---
v2 changes:
- rewrote initialization code to be more concise, and less error-prone, thanks
Dan Carpenter & José Expósito for pointers!
- less struct duplication, it now uses compatiable UGEE v2 ones.
- uclogic_extra_input_mapping is now static, thanks José again!
- straightened out dial transformation, now there's just the two cases and
they're decimal.
- fixed the patch formatting, it should be easier to apply now.
- rebased onto 6.2-rc2

I kept the new initialization functions seperate, please review and tell me if
they are better suited being merged into their UGEE v2 counterparts (which would
involve a lot of branching). I don't have another UGEE v2 tablet to test, so I
won't be able to test for regressions for those devices myself, so I refrained
from those changes in this patch.

Original message:
I did not do the research for this hardware, or the original patch - that work
has been done by Aren Villanueva. For some reason they decided not to merge
it. My changes include remapping the stupid amount of tablet frame buttons,
cleaning up the code to match kernel style, and other small stuff.

The tablet is (almost) fully functional even when uclogic doesn't handle it.
Without initialization, the tablet has some sort of "basic driverless mode"
that allows the tablet frame buttons to have some default keys associated with
them (CTRL-S, CTRL-Z, that kind of stuff), but unfortunately the stylus pen
semi-works. While pressure sensitivity works, only one stylus button functions
correctly. Since the initialization process differs for Pro series tablets, the
new function uclogic_params_init_ugee_xppen_pro had to be introduced. I also
added USB HID IDs for this tablet too, but it's classified under the UGEE
vendor ID.

One of the more strange things I had to do is figure out a way to remap the
buttons since there are 20 of them in total, and of course there are more
buttons than there are BTN constants defined for us. When running without
uclogic, it starts at BTN_0, ends at BTN_8 and the tablet starts reporting
nonsensical keycodes so just leaving it alone isn't an option. I'm testing
this under a libinput system, which has a list of buttons it considers "tablet
pad buttons" which are notably BTN_0, BTN_1, so on and some
gamepad/joystick buttons. So I created a new array called
uclogic_extra_input_mapping for 20 working inputs.

Another weird feature of this tablet is the second dial, which the original
patchset introduced a new uclogic_frame param to handle since it seems it
throws both dials values into one byte. The left wheel is considered EV_WHEEL
and the other, EV_HWHEEL which seems fine to me. I also added the new param to
the debug messages too.


drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-uclogic-core.c | 66 +++++++++++-
drivers/hid/hid-uclogic-params.c | 180 +++++++++++++++++++++++++++++++
drivers/hid/hid-uclogic-params.h | 5 +
drivers/hid/hid-uclogic-rdesc.c | 50 +++++++++
drivers/hid/hid-uclogic-rdesc.h | 9 ++
6 files changed, 307 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 82713ef3aaa6..81d04054229a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1298,6 +1298,7 @@
#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_L 0x0935
#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S 0x0909
#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06 0x0078
+#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO 0x091b
#define USB_DEVICE_ID_UGEE_TABLET_G5 0x0074
#define USB_DEVICE_ID_UGEE_TABLET_EX07S 0x0071
#define USB_DEVICE_ID_UGEE_TABLET_RAINBOW_CV720 0x0055
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index 7fa6fe04f1b2..8d92e984f984 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -81,6 +81,30 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
return rdesc;
}

+/* Buttons considered valid tablet pad inputs. */
+static const unsigned int uclogic_extra_input_mapping[] = {
+ BTN_0,
+ BTN_1,
+ BTN_2,
+ BTN_3,
+ BTN_4,
+ BTN_5,
+ BTN_6,
+ BTN_7,
+ BTN_8,
+ BTN_RIGHT,
+ BTN_MIDDLE,
+ BTN_SIDE,
+ BTN_EXTRA,
+ BTN_FORWARD,
+ BTN_BACK,
+ BTN_B,
+ BTN_A,
+ BTN_BASE,
+ BTN_BASE2,
+ BTN_X
+};
+
static int uclogic_input_mapping(struct hid_device *hdev,
struct hid_input *hi,
struct hid_field *field,
@@ -91,9 +115,27 @@ static int uclogic_input_mapping(struct hid_device *hdev,
struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
struct uclogic_params *params = &drvdata->params;

- /* Discard invalid pen usages */
- if (params->pen.usage_invalid && (field->application == HID_DG_PEN))
- return -1;
+ if (field->application == HID_GD_KEYPAD) {
+ /*
+ * Remap input buttons to sensible ones that are not invalid.
+ * This only affects previous behavior for devices with more than ten or so buttons.
+ */
+ const int key = (usage->hid & HID_USAGE) - 1;
+
+ if (key > 0 && key < ARRAY_SIZE(uclogic_extra_input_mapping)) {
+ hid_map_usage(hi,
+ usage,
+ bit,
+ max,
+ EV_KEY,
+ uclogic_extra_input_mapping[key]);
+ return 1;
+ }
+ } else if (field->application == HID_DG_PEN) {
+ /* Discard invalid pen usages */
+ if (params->pen.usage_invalid)
+ return -1;
+ }

/* Let hid-core decide what to do */
return 0;
@@ -403,8 +445,22 @@ static int uclogic_raw_event_frame(

/* If need to, and can, transform the bitmap dial reports */
if (frame->bitmap_dial_byte > 0 && frame->bitmap_dial_byte < size) {
- if (data[frame->bitmap_dial_byte] == 2)
+ switch (data[frame->bitmap_dial_byte]) {
+ case 2:
data[frame->bitmap_dial_byte] = -1;
+ break;
+
+ /* Everything below here is for tablets that shove multiple dials into 1 byte */
+ case 16:
+ data[frame->bitmap_dial_byte] = 0;
+ data[frame->bitmap_second_dial_destination_byte] = 1;
+ break;
+
+ case 32:
+ data[frame->bitmap_dial_byte] = 0;
+ data[frame->bitmap_second_dial_destination_byte] = -1;
+ break;
+ }
}

return 0;
@@ -531,6 +587,8 @@ static const struct hid_device_id uclogic_devices[] = {
USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S) },
{ HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
+ USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO) },
{ }
};
MODULE_DEVICE_TABLE(hid, uclogic_devices);
diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
index cd1233d7e253..d53c464a1b15 100644
--- a/drivers/hid/hid-uclogic-params.c
+++ b/drivers/hid/hid-uclogic-params.c
@@ -103,6 +103,8 @@ static void uclogic_params_frame_hid_dbg(
frame->touch_flip_at);
hid_dbg(hdev, "\t\t.bitmap_dial_byte = %u\n",
frame->bitmap_dial_byte);
+ hid_dbg(hdev, "\t\t.bitmap_second_dial_destination_byte = %u\n",
+ frame->bitmap_second_dial_destination_byte);
}

/**
@@ -1418,6 +1420,174 @@ static int uclogic_params_ugee_v2_init(struct uclogic_params *params,
return rc;
}

+/**
+ * uclogic_params_parse_ugee_xppen_pro_desc - parse the string descriptor
+ * containing pen and frame parameters returned by XP-PEN Pro devices.
+ *
+ * @str_desc: String descriptor, cannot be NULL.
+ * @str_desc_size: Size of the string descriptor.
+ * @desc_params: Output description params list.
+ * @desc_params_size: Size of the output description params list.
+ *
+ * Returns:
+ * Zero, if successful. A negative errno code on error.
+ */
+static int uclogic_params_parse_ugee_xppen_pro_desc(const __u8 *str_desc,
+ size_t str_desc_size,
+ s32 *desc_params,
+ size_t desc_params_size)
+{
+ s32 pen_x_lm, pen_y_lm;
+ s32 pen_x_pm, pen_y_pm;
+ s32 pen_pressure_lm;
+ s32 resolution;
+
+ /* Minimum descriptor length required, maximum seen so far is 14 */
+ const int min_str_desc_size = 12;
+
+ if (!str_desc || str_desc_size < min_str_desc_size)
+ return -EINVAL;
+
+ if (desc_params_size != UCLOGIC_RDESC_PH_ID_NUM)
+ return -EINVAL;
+
+ pen_x_lm = get_unaligned_le16(str_desc + 2);
+ pen_y_lm = get_unaligned_le16(str_desc + 4);
+ pen_pressure_lm = get_unaligned_le16(str_desc + 8);
+
+ resolution = get_unaligned_le16(str_desc + 10);
+ if (resolution == 0) {
+ pen_x_pm = 0;
+ pen_y_pm = 0;
+ } else {
+ pen_x_pm = pen_x_lm * 1000 / resolution;
+ pen_y_pm = pen_y_lm * 1000 / resolution;
+ }
+
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = pen_x_lm;
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_PM] = pen_x_pm;
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = pen_y_lm;
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_PM] = pen_y_pm;
+ desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] = pen_pressure_lm;
+
+ return 0;
+}
+
+/*
+ * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
+ *
+ * @hdev: The HID device of the tablet interface to initialize and get
+ * parameters from. Cannot be NULL.
+ * @params: Parameters to fill in (to be cleaned with
+ * uclogic_params_cleanup()). Not modified in case of error.
+ * Cannot be NULL.
+ *
+ * Returns:
+ * Zero, if successful. A negative errno code on error.
+ */
+static int uclogic_params_init_ugee_xppen_pro(struct uclogic_params *params,
+ struct hid_device *hdev,
+ const u8 rdesc_frame_arr[],
+ const size_t rdesc_frame_size)
+{
+ int rc = 0;
+ struct usb_interface *iface;
+ __u8 bInterfaceNumber;
+ const int str_desc_len = 12;
+ u8 *str_desc = NULL;
+ __u8 *rdesc_pen = NULL;
+ s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
+ /* The resulting parameters (noop) */
+ struct uclogic_params p = {0, };
+
+ if (!hdev || !params) {
+ rc = -EINVAL;
+ goto cleanup;
+ }
+
+ iface = to_usb_interface(hdev->dev.parent);
+ bInterfaceNumber = iface->cur_altsetting->desc.bInterfaceNumber;
+
+ /* Ignore non-pen interfaces */
+ if (bInterfaceNumber != 2) {
+ uclogic_params_init_invalid(&p);
+ goto output;
+ }
+
+ /*
+ * Initialize the interface by sending magic data.
+ * This magic data is the same as other UGEE v2 tablets.
+ */
+ rc = uclogic_probe_interface(hdev,
+ uclogic_ugee_v2_probe_arr,
+ uclogic_ugee_v2_probe_size,
+ 0x03);
+ if (rc) {
+ uclogic_params_init_invalid(&p);
+ goto output;
+ }
+
+ /**
+ * Read the string descriptor containing pen and frame parameters.
+ * These are slightly different than typical UGEE v2 devices.
+ */
+ rc = uclogic_params_get_str_desc(&str_desc, hdev, 100, str_desc_len);
+ if (rc != str_desc_len) {
+ hid_err(hdev, "failed retrieving pen and frame parameters: %d\n", rc);
+ uclogic_params_init_invalid(&p);
+ goto output;
+ }
+
+ rc = uclogic_params_parse_ugee_xppen_pro_desc(str_desc, str_desc_len,
+ desc_params,
+ ARRAY_SIZE(desc_params));
+ if (rc)
+ goto cleanup;
+
+ kfree(str_desc);
+ str_desc = NULL;
+
+ /* Initialize the pen interface */
+ rdesc_pen = uclogic_rdesc_template_apply(
+ uclogic_rdesc_ugee_v2_pen_template_arr,
+ uclogic_rdesc_ugee_v2_pen_template_size,
+ desc_params, ARRAY_SIZE(desc_params));
+ if (!rdesc_pen) {
+ rc = -ENOMEM;
+ goto cleanup;
+ }
+
+ p.pen.desc_ptr = rdesc_pen;
+ p.pen.desc_size = uclogic_rdesc_ugee_v2_pen_template_size;
+ p.pen.id = 0x02;
+ p.pen.subreport_list[0].value = 0xf0;
+ p.pen.subreport_list[0].id = UCLOGIC_RDESC_V1_FRAME_ID;
+
+ /* Initialize the frame interface */
+ rc = uclogic_params_frame_init_with_desc(
+ &p.frame_list[0],
+ rdesc_frame_arr,
+ rdesc_frame_size,
+ UCLOGIC_RDESC_V1_FRAME_ID);
+ if (rc < 0) {
+ hid_err(hdev, "initializing frame params failed: %d\n", rc);
+ goto output;
+ }
+
+ p.frame_list[0].bitmap_dial_byte = 7;
+ p.frame_list[0].bitmap_second_dial_destination_byte = 8;
+
+output:
+ /* Output parameters */
+ memcpy(params, &p, sizeof(*params));
+ memset(&p, 0, sizeof(p));
+ rc = 0;
+cleanup:
+ kfree(str_desc);
+ uclogic_params_cleanup(&p);
+ return rc;
+}
+
/**
* uclogic_params_init() - initialize a tablet interface and discover its
* parameters.
@@ -1728,6 +1898,16 @@ int uclogic_params_init(struct uclogic_params *params,
uclogic_params_init_invalid(&p);
}

+ break;
+ case VID_PID(USB_VENDOR_ID_UGEE,
+ USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO):
+ rc = uclogic_params_init_ugee_xppen_pro(&p,
+ hdev,
+ uclogic_rdesc_xppen_artist_22r_pro_frame_arr,
+ uclogic_rdesc_xppen_artist_22r_pro_frame_size);
+ if (rc != 0)
+ goto cleanup;
+
break;
}

diff --git a/drivers/hid/hid-uclogic-params.h b/drivers/hid/hid-uclogic-params.h
index a97477c02ff8..6621a75a4b1a 100644
--- a/drivers/hid/hid-uclogic-params.h
+++ b/drivers/hid/hid-uclogic-params.h
@@ -171,6 +171,11 @@ struct uclogic_params_frame {
* counterclockwise, as opposed to the normal 1 and -1.
*/
unsigned int bitmap_dial_byte;
+ /*
+ * Destination offset for the second bitmap dial byte, if the tablet
+ * supports a second dial at all.
+ */
+ unsigned int bitmap_second_dial_destination_byte;
};

/*
diff --git a/drivers/hid/hid-uclogic-rdesc.c b/drivers/hid/hid-uclogic-rdesc.c
index fb40775f5f5b..86293ae8c995 100644
--- a/drivers/hid/hid-uclogic-rdesc.c
+++ b/drivers/hid/hid-uclogic-rdesc.c
@@ -859,6 +859,12 @@ const __u8 uclogic_rdesc_v2_frame_dial_arr[] = {
const size_t uclogic_rdesc_v2_frame_dial_size =
sizeof(uclogic_rdesc_v2_frame_dial_arr);

+const __u8 uclogic_ugee_v2_probe_arr[] = {
+ 0x02, 0xb0, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+};
+const size_t uclogic_ugee_v2_probe_size = sizeof(uclogic_ugee_v2_probe_arr);
+const int uclogic_ugee_v2_probe_endpoint = 0x03;
+
/* Fixed report descriptor template for UGEE v2 pen reports */
const __u8 uclogic_rdesc_ugee_v2_pen_template_arr[] = {
0x05, 0x0d, /* Usage Page (Digitizers), */
@@ -1185,6 +1191,50 @@ const __u8 uclogic_rdesc_xppen_deco01_frame_arr[] = {
const size_t uclogic_rdesc_xppen_deco01_frame_size =
sizeof(uclogic_rdesc_xppen_deco01_frame_arr);

+/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
+const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[] = {
+ 0x05, 0x01, /* Usage Page (Desktop), */
+ 0x09, 0x07, /* Usage (Keypad), */
+ 0xA1, 0x01, /* Collection (Application), */
+ 0x85, UCLOGIC_RDESC_V1_FRAME_ID,
+ /* Report ID (Virtual report), */
+ 0x05, 0x0D, /* Usage Page (Digitizer), */
+ 0x09, 0x39, /* Usage (Tablet Function Keys), */
+ 0xA0, /* Collection (Physical), */
+ 0x14, /* Logical Minimum (0), */
+ 0x25, 0x01, /* Logical Maximum (1), */
+ 0x75, 0x01, /* Report Size (1), */
+ 0x95, 0x08, /* Report Count (8), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x05, 0x09, /* Usage Page (Button), */
+ 0x19, 0x01, /* Usage Minimum (01h), */
+ 0x29, 0x14, /* Usage Maximum (14h), */
+ 0x95, 0x14, /* Report Count (20), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0x95, 0x14, /* Report Count (20), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x05, 0x01, /* Usage Page (Desktop), */
+ 0x09, 0x38, /* Usage (Wheel), */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x15, 0xFF, /* Logical Minimum (-1), */
+ 0x25, 0x08, /* Logical Maximum (8), */
+ 0x81, 0x06, /* Input (Variable, Relative), */
+ 0x05, 0x0C, /* Usage Page (Consumer Devices), */
+ 0x0A, 0x38, 0x02, /* Usage (AC PAN), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x81, 0x06, /* Input (Variable, Relative), */
+ 0x26, 0xFF, 0x00, /* Logical Maximum (255), */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0xC0, /* End Collection */
+ 0xC0, /* End Collection */
+};
+
+const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size =
+ sizeof(uclogic_rdesc_xppen_artist_22r_pro_frame_arr);
+
/**
* uclogic_rdesc_template_apply() - apply report descriptor parameters to a
* report descriptor template, creating a report descriptor. Copies the
diff --git a/drivers/hid/hid-uclogic-rdesc.h b/drivers/hid/hid-uclogic-rdesc.h
index a1f78c07293f..c3cb2c75dda5 100644
--- a/drivers/hid/hid-uclogic-rdesc.h
+++ b/drivers/hid/hid-uclogic-rdesc.h
@@ -164,6 +164,11 @@ extern const size_t uclogic_rdesc_v2_frame_dial_size;
/* Report ID for tweaked UGEE v2 battery reports */
#define UCLOGIC_RDESC_UGEE_V2_BATTERY_ID 0xba

+/* Magic data expected by UGEEv2 devices on probe */
+extern const __u8 uclogic_ugee_v2_probe_arr[];
+extern const size_t uclogic_ugee_v2_probe_size;
+extern const int uclogic_ugee_v2_probe_endpoint;
+
/* Fixed report descriptor template for UGEE v2 pen reports */
extern const __u8 uclogic_rdesc_ugee_v2_pen_template_arr[];
extern const size_t uclogic_rdesc_ugee_v2_pen_template_size;
@@ -205,4 +210,8 @@ extern const size_t uclogic_rdesc_ugee_g5_frame_size;
/* Least-significant bit of Ugee G5 frame rotary encoder state */
#define UCLOGIC_RDESC_UGEE_G5_FRAME_RE_LSB 38

+/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
+extern const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[];
+extern const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size;
+
#endif /* _HID_UCLOGIC_RDESC_H */

base-commit: 88603b6dc419445847923fcb7fe5080067a30f98
--
2.38.2

2023-01-02 23:05:13

by kernel test robot

Subject: Re: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi Joshua,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 88603b6dc419445847923fcb7fe5080067a30f98]

url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20230103-035221
base: 88603b6dc419445847923fcb7fe5080067a30f98
patch link: https://lore.kernel.org/r/20230102194911.56083-1-josh%40redstrate.com
patch subject: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/544bdd996d164d0202d03318e29f3ec570f88b16
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20230103-035221
git checkout 544bdd996d164d0202d03318e29f3ec570f88b16
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/hid/hid-uclogic-params.c: In function 'uclogic_params_init_ugee_xppen_pro':
>> drivers/hid/hid-uclogic-params.c:1522:38: warning: passing argument 2 of 'uclogic_probe_interface' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
1522 | uclogic_ugee_v2_probe_arr,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/hid/hid-uclogic-params.c:1026:65: note: expected 'u8 *' {aka 'unsigned char *'} but argument is of type 'const __u8 *' {aka 'const unsigned char *'}
1026 | static int uclogic_probe_interface(struct hid_device *hdev, u8 *magic_arr,
| ~~~~^~~~~~~~~


vim +1522 drivers/hid/hid-uclogic-params.c

1475
1476 /*
1477 * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
1478 *
1479 * @hdev: The HID device of the tablet interface to initialize and get
1480 * parameters from. Cannot be NULL.
1481 * @params: Parameters to fill in (to be cleaned with
1482 * uclogic_params_cleanup()). Not modified in case of error.
1483 * Cannot be NULL.
1484 *
1485 * Returns:
1486 * Zero, if successful. A negative errno code on error.
1487 */
1488 static int uclogic_params_init_ugee_xppen_pro(struct uclogic_params *params,
1489 struct hid_device *hdev,
1490 const u8 rdesc_frame_arr[],
1491 const size_t rdesc_frame_size)
1492 {
1493 int rc = 0;
1494 struct usb_interface *iface;
1495 __u8 bInterfaceNumber;
1496 const int str_desc_len = 12;
1497 u8 *str_desc = NULL;
1498 __u8 *rdesc_pen = NULL;
1499 s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
1500 /* The resulting parameters (noop) */
1501 struct uclogic_params p = {0, };
1502
1503 if (!hdev || !params) {
1504 rc = -EINVAL;
1505 goto cleanup;
1506 }
1507
1508 iface = to_usb_interface(hdev->dev.parent);
1509 bInterfaceNumber = iface->cur_altsetting->desc.bInterfaceNumber;
1510
1511 /* Ignore non-pen interfaces */
1512 if (bInterfaceNumber != 2) {
1513 uclogic_params_init_invalid(&p);
1514 goto output;
1515 }
1516
1517 /*
1518 * Initialize the interface by sending magic data.
1519 * This magic data is the same as other UGEE v2 tablets.
1520 */
1521 rc = uclogic_probe_interface(hdev,
> 1522 uclogic_ugee_v2_probe_arr,
1523 uclogic_ugee_v2_probe_size,
1524 0x03);
1525 if (rc) {
1526 uclogic_params_init_invalid(&p);
1527 goto output;
1528 }
1529
1530 /**
1531 * Read the string descriptor containing pen and frame parameters.
1532 * These are slightly different than typical UGEE v2 devices.
1533 */
1534 rc = uclogic_params_get_str_desc(&str_desc, hdev, 100, str_desc_len);
1535 if (rc != str_desc_len) {
1536 hid_err(hdev, "failed retrieving pen and frame parameters: %d\n", rc);
1537 uclogic_params_init_invalid(&p);
1538 goto output;
1539 }
1540
1541 rc = uclogic_params_parse_ugee_xppen_pro_desc(str_desc, str_desc_len,
1542 desc_params,
1543 ARRAY_SIZE(desc_params));
1544 if (rc)
1545 goto cleanup;
1546
1547 kfree(str_desc);
1548 str_desc = NULL;
1549
1550 /* Initialize the pen interface */
1551 rdesc_pen = uclogic_rdesc_template_apply(
1552 uclogic_rdesc_ugee_v2_pen_template_arr,
1553 uclogic_rdesc_ugee_v2_pen_template_size,
1554 desc_params, ARRAY_SIZE(desc_params));
1555 if (!rdesc_pen) {
1556 rc = -ENOMEM;
1557 goto cleanup;
1558 }
1559
1560 p.pen.desc_ptr = rdesc_pen;
1561 p.pen.desc_size = uclogic_rdesc_ugee_v2_pen_template_size;
1562 p.pen.id = 0x02;
1563 p.pen.subreport_list[0].value = 0xf0;
1564 p.pen.subreport_list[0].id = UCLOGIC_RDESC_V1_FRAME_ID;
1565
1566 /* Initialize the frame interface */
1567 rc = uclogic_params_frame_init_with_desc(
1568 &p.frame_list[0],
1569 rdesc_frame_arr,
1570 rdesc_frame_size,
1571 UCLOGIC_RDESC_V1_FRAME_ID);
1572 if (rc < 0) {
1573 hid_err(hdev, "initializing frame params failed: %d\n", rc);
1574 goto output;
1575 }
1576
1577 p.frame_list[0].bitmap_dial_byte = 7;
1578 p.frame_list[0].bitmap_second_dial_destination_byte = 8;
1579
1580 output:
1581 /* Output parameters */
1582 memcpy(params, &p, sizeof(*params));
1583 memset(&p, 0, sizeof(p));
1584 rc = 0;
1585 cleanup:
1586 kfree(str_desc);
1587 uclogic_params_cleanup(&p);
1588 return rc;
1589 }
1590

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

2023-01-03 02:10:42

by kernel test robot

Subject: Re: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro

Hi Joshua,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 88603b6dc419445847923fcb7fe5080067a30f98]

url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20230103-035221
base: 88603b6dc419445847923fcb7fe5080067a30f98
patch link: https://lore.kernel.org/r/20230102194911.56083-1-josh%40redstrate.com
patch subject: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro
config: i386-randconfig-a013-20230102
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/544bdd996d164d0202d03318e29f3ec570f88b16
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Joshua-Goins/HID-uclogic-Add-support-for-XP-PEN-Artist-22R-Pro/20230103-035221
git checkout 544bdd996d164d0202d03318e29f3ec570f88b16
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/hid/hid-uclogic-params.c:1522:10: error: passing 'const __u8[]' (aka 'const unsigned char[]') to parameter of type 'u8 *' (aka 'unsigned char *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
uclogic_ugee_v2_probe_arr,
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/hid/hid-uclogic-params.c:1026:65: note: passing argument to parameter 'magic_arr' here
static int uclogic_probe_interface(struct hid_device *hdev, u8 *magic_arr,
^
1 error generated.


vim +1522 drivers/hid/hid-uclogic-params.c

1475
1476 /*
1477 * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
1478 *
1479 * @hdev: The HID device of the tablet interface to initialize and get
1480 * parameters from. Cannot be NULL.
1481 * @params: Parameters to fill in (to be cleaned with
1482 * uclogic_params_cleanup()). Not modified in case of error.
1483 * Cannot be NULL.
1484 *
1485 * Returns:
1486 * Zero, if successful. A negative errno code on error.
1487 */
1488 static int uclogic_params_init_ugee_xppen_pro(struct uclogic_params *params,
1489 struct hid_device *hdev,
1490 const u8 rdesc_frame_arr[],
1491 const size_t rdesc_frame_size)
1492 {
1493 int rc = 0;
1494 struct usb_interface *iface;
1495 __u8 bInterfaceNumber;
1496 const int str_desc_len = 12;
1497 u8 *str_desc = NULL;
1498 __u8 *rdesc_pen = NULL;
1499 s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
1500 /* The resulting parameters (noop) */
1501 struct uclogic_params p = {0, };
1502
1503 if (!hdev || !params) {
1504 rc = -EINVAL;
1505 goto cleanup;
1506 }
1507
1508 iface = to_usb_interface(hdev->dev.parent);
1509 bInterfaceNumber = iface->cur_altsetting->desc.bInterfaceNumber;
1510
1511 /* Ignore non-pen interfaces */
1512 if (bInterfaceNumber != 2) {
1513 uclogic_params_init_invalid(&p);
1514 goto output;
1515 }
1516
1517 /*
1518 * Initialize the interface by sending magic data.
1519 * This magic data is the same as other UGEE v2 tablets.
1520 */
1521 rc = uclogic_probe_interface(hdev,
> 1522 uclogic_ugee_v2_probe_arr,
1523 uclogic_ugee_v2_probe_size,
1524 0x03);
1525 if (rc) {
1526 uclogic_params_init_invalid(&p);
1527 goto output;
1528 }
1529
1530 /**
1531 * Read the string descriptor containing pen and frame parameters.
1532 * These are slightly different than typical UGEE v2 devices.
1533 */
1534 rc = uclogic_params_get_str_desc(&str_desc, hdev, 100, str_desc_len);
1535 if (rc != str_desc_len) {
1536 hid_err(hdev, "failed retrieving pen and frame parameters: %d\n", rc);
1537 uclogic_params_init_invalid(&p);
1538 goto output;
1539 }
1540
1541 rc = uclogic_params_parse_ugee_xppen_pro_desc(str_desc, str_desc_len,
1542 desc_params,
1543 ARRAY_SIZE(desc_params));
1544 if (rc)
1545 goto cleanup;
1546
1547 kfree(str_desc);
1548 str_desc = NULL;
1549
1550 /* Initialize the pen interface */
1551 rdesc_pen = uclogic_rdesc_template_apply(
1552 uclogic_rdesc_ugee_v2_pen_template_arr,
1553 uclogic_rdesc_ugee_v2_pen_template_size,
1554 desc_params, ARRAY_SIZE(desc_params));
1555 if (!rdesc_pen) {
1556 rc = -ENOMEM;
1557 goto cleanup;
1558 }
1559
1560 p.pen.desc_ptr = rdesc_pen;
1561 p.pen.desc_size = uclogic_rdesc_ugee_v2_pen_template_size;
1562 p.pen.id = 0x02;
1563 p.pen.subreport_list[0].value = 0xf0;
1564 p.pen.subreport_list[0].id = UCLOGIC_RDESC_V1_FRAME_ID;
1565
1566 /* Initialize the frame interface */
1567 rc = uclogic_params_frame_init_with_desc(
1568 &p.frame_list[0],
1569 rdesc_frame_arr,
1570 rdesc_frame_size,
1571 UCLOGIC_RDESC_V1_FRAME_ID);
1572 if (rc < 0) {
1573 hid_err(hdev, "initializing frame params failed: %d\n", rc);
1574 goto output;
1575 }
1576
1577 p.frame_list[0].bitmap_dial_byte = 7;
1578 p.frame_list[0].bitmap_second_dial_destination_byte = 8;
1579
1580 output:
1581 /* Output parameters */
1582 memcpy(params, &p, sizeof(*params));
1583 memset(&p, 0, sizeof(p));
1584 rc = 0;
1585 cleanup:
1586 kfree(str_desc);
1587 uclogic_params_cleanup(&p);
1588 return rc;
1589 }
1590

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

2023-01-03 08:56:45

by Dan Carpenter

Subject: Re: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro

On Mon, Jan 02, 2023 at 02:49:10PM -0500, Joshua Goins wrote:
> @@ -91,9 +115,27 @@ static int uclogic_input_mapping(struct hid_device *hdev,
> struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
> struct uclogic_params *params = &drvdata->params;
>
> - /* Discard invalid pen usages */
> - if (params->pen.usage_invalid && (field->application == HID_DG_PEN))
> - return -1;
> + if (field->application == HID_GD_KEYPAD) {
> + /*
> + * Remap input buttons to sensible ones that are not invalid.
> + * This only affects previous behavior for devices with more than ten or so buttons.
> + */
> + const int key = (usage->hid & HID_USAGE) - 1;
> +
> + if (key > 0 && key < ARRAY_SIZE(uclogic_extra_input_mapping)) {

It's an unusual that zero is not valid but I don't know the code at all.

> + hid_map_usage(hi,
> + usage,
> + bit,
> + max,
> + EV_KEY,
> + uclogic_extra_input_mapping[key]);
> + return 1;
> + }
> + } else if (field->application == HID_DG_PEN) {
> + /* Discard invalid pen usages */
> + if (params->pen.usage_invalid)
> + return -1;
> + }
>
> /* Let hid-core decide what to do */
> return 0;

[ snip ]

> +/*
> + * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
> + *
> + * @hdev: The HID device of the tablet interface to initialize and get
> + * parameters from. Cannot be NULL.
> + * @params: Parameters to fill in (to be cleaned with
> + * uclogic_params_cleanup()). Not modified in case of error.
> + * Cannot be NULL.
> + *
> + * Returns:
> + * Zero, if successful. A negative errno code on error.
> + */
> +static int uclogic_params_init_ugee_xppen_pro(struct uclogic_params *params,
> + struct hid_device *hdev,
> + const u8 rdesc_frame_arr[],
> + const size_t rdesc_frame_size)
> +{
> + int rc = 0;
> + struct usb_interface *iface;
> + __u8 bInterfaceNumber;
> + const int str_desc_len = 12;
> + u8 *str_desc = NULL;
> + __u8 *rdesc_pen = NULL;
> + s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
> + /* The resulting parameters (noop) */
> + struct uclogic_params p = {0, };
> +
> + if (!hdev || !params) {
> + rc = -EINVAL;
> + goto cleanup;
> + }
> +
> + iface = to_usb_interface(hdev->dev.parent);
> + bInterfaceNumber = iface->cur_altsetting->desc.bInterfaceNumber;
> +
> + /* Ignore non-pen interfaces */
> + if (bInterfaceNumber != 2) {
> + uclogic_params_init_invalid(&p);
> + goto output;

So this is a success path? "ret = 0?" The comments kind of suggest
that but I want to be sure.

> + }
> +
> + /*
> + * Initialize the interface by sending magic data.
> + * This magic data is the same as other UGEE v2 tablets.
> + */
> + rc = uclogic_probe_interface(hdev,
> + uclogic_ugee_v2_probe_arr,
> + uclogic_ugee_v2_probe_size,
> + 0x03);
> + if (rc) {
> + uclogic_params_init_invalid(&p);
> + goto output;
> + }
> +
> + /**
> + * Read the string descriptor containing pen and frame parameters.
> + * These are slightly different than typical UGEE v2 devices.
> + */
> + rc = uclogic_params_get_str_desc(&str_desc, hdev, 100, str_desc_len);
> + if (rc != str_desc_len) {
> + hid_err(hdev, "failed retrieving pen and frame parameters: %d\n", rc);
> + uclogic_params_init_invalid(&p);
> + goto output;

This isn't correct. You need to do something like:

rc = (rc < 0) ? rc : -EINVAL;

regards,
dan carpenter

> + }
> +
> + rc = uclogic_params_parse_ugee_xppen_pro_desc(str_desc, str_desc_len,
> + desc_params,
> + ARRAY_SIZE(desc_params));
> + if (rc)
> + goto cleanup;
> +
> + kfree(str_desc);
> + str_desc = NULL;
> +
> + /* Initialize the pen interface */
> + rdesc_pen = uclogic_rdesc_template_apply(
> + uclogic_rdesc_ugee_v2_pen_template_arr,
> + uclogic_rdesc_ugee_v2_pen_template_size,
> + desc_params, ARRAY_SIZE(desc_params));
> + if (!rdesc_pen) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
> +
> + p.pen.desc_ptr = rdesc_pen;
> + p.pen.desc_size = uclogic_rdesc_ugee_v2_pen_template_size;
> + p.pen.id = 0x02;
> + p.pen.subreport_list[0].value = 0xf0;
> + p.pen.subreport_list[0].id = UCLOGIC_RDESC_V1_FRAME_ID;
> +
> + /* Initialize the frame interface */
> + rc = uclogic_params_frame_init_with_desc(
> + &p.frame_list[0],
> + rdesc_frame_arr,
> + rdesc_frame_size,
> + UCLOGIC_RDESC_V1_FRAME_ID);
> + if (rc < 0) {
> + hid_err(hdev, "initializing frame params failed: %d\n", rc);
> + goto output;
> + }
> +
> + p.frame_list[0].bitmap_dial_byte = 7;
> + p.frame_list[0].bitmap_second_dial_destination_byte = 8;
> +
> +output:
> + /* Output parameters */
> + memcpy(params, &p, sizeof(*params));
> + memset(&p, 0, sizeof(p));
> + rc = 0;
> +cleanup:
> + kfree(str_desc);
> + uclogic_params_cleanup(&p);
> + return rc;
> +}

2023-01-05 18:07:19

by José Expósito

Subject: Re: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro

On Mon, Jan 02, 2023 at 02:49:10PM -0500, Joshua Goins wrote:
> Adds support for the XP-PEN Artist 22R Pro, including stylus, tablet frame
> and pen pressure.
>
> The tablet has 20 buttons, but need to be remapped in order since the
> device reports invalid keycodes. Existing tablet functionality should
> not be inhibited, as BTN0-8 is still used.
>
> New initialization functions are added since the device differs slightly
> from other UGEE v2 devices.
>
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Reported-by: Jos? Exp?sito <[email protected]>
> Signed-off-by: Joshua Goins <[email protected]>
> ---
> v2 changes:
> - rewrote initialization code to be more concise, and less error-prone, thanks
> Dan Carpenter & Jos? Exp?sito for pointers!
> - less struct duplication, it now uses compatiable UGEE v2 ones.
> - uclogic_extra_input_mapping is now static, thanks Jos? again!
> - straightened out dial transformation, now there's just the two cases and
> they're decimal.
> - fixed the patch formatting, it should be easier to apply now.
> - rebased onto 6.2-rc2
>
> I kept the new initialization functions seperate, please review and tell me if
> they are better suited being merged into their UGEE v2 counterparts (which would
> involve a lot of branching). I don't have another UGEE v2 tablet to test, so I
> won't be able to test for regressions for those devices myself, so I refrained
> from those changes in this patch.
>
> Original message:
> I did not do the research for this hardware, or the original patch - that work
> has been done by Aren Villanueva. For some reason they decided not to merge
> it. My changes include remapping the stupid amount of tablet frame buttons,
> cleaning up the code to match kernel style, and other small stuff.
>
> The tablet is (almost) fully functional even when uclogic doesn't handle it.
> Without initialization, the tablet has some sort of "basic driverless mode"
> that allows the tablet frame buttons to have some default keys associated with
> them (CTRL-S, CTRL-Z, that kind of stuff), but unfortunately the stylus pen
> semi-works. While pressure sensitivity works, only one stylus button functions
> correctly. Since the initialization process differs for Pro series tablets, the
> new function uclogic_params_init_ugee_xppen_pro had to be introduced. I also
> added USB HID IDs for this tablet too, but it's classified under the UGEE
> vendor ID.
>
> One of the more strange things I had to do is figure out a way to remap the
> buttons since there are 20 of them in total, and of course there are more
> buttons than there are BTN constants defined for us. When running without
> uclogic, it starts at BTN_0, ends at BTN_8 and the tablet starts reporting
> nonsensical keycodes so just leaving it alone isn't an option. I'm testing
> this under a libinput system, which has a list of buttons it considers "tablet
> pad buttons" which are notably BTN_0, BTN_1, so on and some
> gamepad/joystick buttons. So I created a new array called
> uclogic_extra_input_mapping for 20 working inputs.
>
> Another weird feature of this tablet is the second dial, which the original
> patchset introduced a new uclogic_frame param to handle since it seems it
> throws both dials values into one byte. The left wheel is considered EV_WHEEL
> and the other, EV_HWHEEL which seems fine to me. I also added the new param to
> the debug messages too.
>
>
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-uclogic-core.c | 66 +++++++++++-
> drivers/hid/hid-uclogic-params.c | 180 +++++++++++++++++++++++++++++++
> drivers/hid/hid-uclogic-params.h | 5 +
> drivers/hid/hid-uclogic-rdesc.c | 50 +++++++++
> drivers/hid/hid-uclogic-rdesc.h | 9 ++
> 6 files changed, 307 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 82713ef3aaa6..81d04054229a 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1298,6 +1298,7 @@
> #define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_L 0x0935
> #define USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S 0x0909
> #define USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06 0x0078
> +#define USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO 0x091b
> #define USB_DEVICE_ID_UGEE_TABLET_G5 0x0074
> #define USB_DEVICE_ID_UGEE_TABLET_EX07S 0x0071
> #define USB_DEVICE_ID_UGEE_TABLET_RAINBOW_CV720 0x0055
> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> index 7fa6fe04f1b2..8d92e984f984 100644
> --- a/drivers/hid/hid-uclogic-core.c
> +++ b/drivers/hid/hid-uclogic-core.c
> @@ -81,6 +81,30 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> return rdesc;
> }
>
> +/* Buttons considered valid tablet pad inputs. */
> +static const unsigned int uclogic_extra_input_mapping[] = {
> + BTN_0,
> + BTN_1,
> + BTN_2,
> + BTN_3,
> + BTN_4,
> + BTN_5,
> + BTN_6,
> + BTN_7,
> + BTN_8,

I see this event codes in a test device mock in libinput, but I wonder
if we could/should use BTN_9 instead of skiping a few IDs here.

Honestly, I don't now what should be the right approach in this case,
let's see if someone else comments on this topic.

> + BTN_RIGHT,
> + BTN_MIDDLE,
> + BTN_SIDE,
> + BTN_EXTRA,
> + BTN_FORWARD,
> + BTN_BACK,
> + BTN_B,
> + BTN_A,
> + BTN_BASE,
> + BTN_BASE2,
> + BTN_X
> +};
> +
> static int uclogic_input_mapping(struct hid_device *hdev,
> struct hid_input *hi,
> struct hid_field *field,
> @@ -91,9 +115,27 @@ static int uclogic_input_mapping(struct hid_device *hdev,
> struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
> struct uclogic_params *params = &drvdata->params;
>
> - /* Discard invalid pen usages */
> - if (params->pen.usage_invalid && (field->application == HID_DG_PEN))
> - return -1;
> + if (field->application == HID_GD_KEYPAD) {
> + /*
> + * Remap input buttons to sensible ones that are not invalid.
> + * This only affects previous behavior for devices with more than ten or so buttons.
> + */
> + const int key = (usage->hid & HID_USAGE) - 1;
> +
> + if (key > 0 && key < ARRAY_SIZE(uclogic_extra_input_mapping)) {
> + hid_map_usage(hi,
> + usage,
> + bit,
> + max,
> + EV_KEY,
> + uclogic_extra_input_mapping[key]);
> + return 1;
> + }
> + } else if (field->application == HID_DG_PEN) {
> + /* Discard invalid pen usages */
> + if (params->pen.usage_invalid)
> + return -1;
> + }
>
> /* Let hid-core decide what to do */
> return 0;
> @@ -403,8 +445,22 @@ static int uclogic_raw_event_frame(
>
> /* If need to, and can, transform the bitmap dial reports */
> if (frame->bitmap_dial_byte > 0 && frame->bitmap_dial_byte < size) {
> - if (data[frame->bitmap_dial_byte] == 2)
> + switch (data[frame->bitmap_dial_byte]) {
> + case 2:
> data[frame->bitmap_dial_byte] = -1;
> + break;
> +
> + /* Everything below here is for tablets that shove multiple dials into 1 byte */
> + case 16:
> + data[frame->bitmap_dial_byte] = 0;
> + data[frame->bitmap_second_dial_destination_byte] = 1;
> + break;
> +
> + case 32:
> + data[frame->bitmap_dial_byte] = 0;
> + data[frame->bitmap_second_dial_destination_byte] = -1;
> + break;
> + }
> }
>
> return 0;
> @@ -531,6 +587,8 @@ static const struct hid_device_id uclogic_devices[] = {
> USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_S) },
> { HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
> USB_DEVICE_ID_UGEE_XPPEN_TABLET_STAR06) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_UGEE,
> + USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO) },
> { }
> };
> MODULE_DEVICE_TABLE(hid, uclogic_devices);
> diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
> index cd1233d7e253..d53c464a1b15 100644
> --- a/drivers/hid/hid-uclogic-params.c
> +++ b/drivers/hid/hid-uclogic-params.c
> @@ -103,6 +103,8 @@ static void uclogic_params_frame_hid_dbg(
> frame->touch_flip_at);
> hid_dbg(hdev, "\t\t.bitmap_dial_byte = %u\n",
> frame->bitmap_dial_byte);
> + hid_dbg(hdev, "\t\t.bitmap_second_dial_destination_byte = %u\n",
> + frame->bitmap_second_dial_destination_byte);
> }
>
> /**
> @@ -1418,6 +1420,174 @@ static int uclogic_params_ugee_v2_init(struct uclogic_params *params,
> return rc;
> }
>
> +/**
> + * uclogic_params_parse_ugee_xppen_pro_desc - parse the string descriptor
> + * containing pen and frame parameters returned by XP-PEN Pro devices.
> + *
> + * @str_desc: String descriptor, cannot be NULL.
> + * @str_desc_size: Size of the string descriptor.
> + * @desc_params: Output description params list.
> + * @desc_params_size: Size of the output description params list.
> + *
> + * Returns:
> + * Zero, if successful. A negative errno code on error.
> + */
> +static int uclogic_params_parse_ugee_xppen_pro_desc(const __u8 *str_desc,

I think that you could use uclogic_params_parse_ugee_v2_desc() and
change the number of buttons in the template afterwards. It'd avoid
some code duplication.

> + size_t str_desc_size,
> + s32 *desc_params,
> + size_t desc_params_size)
> +{
> + s32 pen_x_lm, pen_y_lm;
> + s32 pen_x_pm, pen_y_pm;
> + s32 pen_pressure_lm;
> + s32 resolution;
> +
> + /* Minimum descriptor length required, maximum seen so far is 14 */
> + const int min_str_desc_size = 12;
> +
> + if (!str_desc || str_desc_size < min_str_desc_size)
> + return -EINVAL;
> +
> + if (desc_params_size != UCLOGIC_RDESC_PH_ID_NUM)
> + return -EINVAL;
> +
> + pen_x_lm = get_unaligned_le16(str_desc + 2);
> + pen_y_lm = get_unaligned_le16(str_desc + 4);
> + pen_pressure_lm = get_unaligned_le16(str_desc + 8);
> +
> + resolution = get_unaligned_le16(str_desc + 10);
> + if (resolution == 0) {
> + pen_x_pm = 0;
> + pen_y_pm = 0;
> + } else {
> + pen_x_pm = pen_x_lm * 1000 / resolution;
> + pen_y_pm = pen_y_lm * 1000 / resolution;
> + }
> +
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_LM] = pen_x_lm;
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_X_PM] = pen_x_pm;
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_LM] = pen_y_lm;
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_Y_PM] = pen_y_pm;
> + desc_params[UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM] = pen_pressure_lm;
> +
> + return 0;
> +}
> +
> +/*
> + * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
> + *
> + * @hdev: The HID device of the tablet interface to initialize and get
> + * parameters from. Cannot be NULL.
> + * @params: Parameters to fill in (to be cleaned with
> + * uclogic_params_cleanup()). Not modified in case of error.
> + * Cannot be NULL.
> + *
> + * Returns:
> + * Zero, if successful. A negative errno code on error.
> + */
> +static int uclogic_params_init_ugee_xppen_pro(struct uclogic_params *params,

There are some XP-Pen PRO devices handled by the other init function.
Maybe we could rename this function to something more specific to your
device.

> + struct hid_device *hdev,
> + const u8 rdesc_frame_arr[],
> + const size_t rdesc_frame_size)
> +{
> + int rc = 0;
> + struct usb_interface *iface;
> + __u8 bInterfaceNumber;
> + const int str_desc_len = 12;
> + u8 *str_desc = NULL;
> + __u8 *rdesc_pen = NULL;
> + s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
> + /* The resulting parameters (noop) */
> + struct uclogic_params p = {0, };
> +
> + if (!hdev || !params) {
> + rc = -EINVAL;
> + goto cleanup;
> + }
> +
> + iface = to_usb_interface(hdev->dev.parent);
> + bInterfaceNumber = iface->cur_altsetting->desc.bInterfaceNumber;
> +
> + /* Ignore non-pen interfaces */
> + if (bInterfaceNumber != 2) {
> + uclogic_params_init_invalid(&p);
> + goto output;
> + }
> +
> + /*
> + * Initialize the interface by sending magic data.
> + * This magic data is the same as other UGEE v2 tablets.
> + */
> + rc = uclogic_probe_interface(hdev,
> + uclogic_ugee_v2_probe_arr,
> + uclogic_ugee_v2_probe_size,
> + 0x03);

You can use "uclogic_ugee_v2_probe_endpoint" here.

> + if (rc) {
> + uclogic_params_init_invalid(&p);
> + goto output;
> + }
> +
> + /**
> + * Read the string descriptor containing pen and frame parameters.
> + * These are slightly different than typical UGEE v2 devices.
> + */
> + rc = uclogic_params_get_str_desc(&str_desc, hdev, 100, str_desc_len);
> + if (rc != str_desc_len) {
> + hid_err(hdev, "failed retrieving pen and frame parameters: %d\n", rc);
> + uclogic_params_init_invalid(&p);
> + goto output;
> + }
> +
> + rc = uclogic_params_parse_ugee_xppen_pro_desc(str_desc, str_desc_len,
> + desc_params,
> + ARRAY_SIZE(desc_params));
> + if (rc)
> + goto cleanup;
> +
> + kfree(str_desc);
> + str_desc = NULL;
> +
> + /* Initialize the pen interface */
> + rdesc_pen = uclogic_rdesc_template_apply(
> + uclogic_rdesc_ugee_v2_pen_template_arr,
> + uclogic_rdesc_ugee_v2_pen_template_size,
> + desc_params, ARRAY_SIZE(desc_params));
> + if (!rdesc_pen) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
> +
> + p.pen.desc_ptr = rdesc_pen;
> + p.pen.desc_size = uclogic_rdesc_ugee_v2_pen_template_size;
> + p.pen.id = 0x02;
> + p.pen.subreport_list[0].value = 0xf0;
> + p.pen.subreport_list[0].id = UCLOGIC_RDESC_V1_FRAME_ID;
> +
> + /* Initialize the frame interface */
> + rc = uclogic_params_frame_init_with_desc(
> + &p.frame_list[0],
> + rdesc_frame_arr,
> + rdesc_frame_size,
> + UCLOGIC_RDESC_V1_FRAME_ID);
> + if (rc < 0) {
> + hid_err(hdev, "initializing frame params failed: %d\n", rc);
> + goto output;
> + }
> +
> + p.frame_list[0].bitmap_dial_byte = 7;
> + p.frame_list[0].bitmap_second_dial_destination_byte = 8;
> +
> +output:
> + /* Output parameters */
> + memcpy(params, &p, sizeof(*params));
> + memset(&p, 0, sizeof(p));
> + rc = 0;
> +cleanup:
> + kfree(str_desc);
> + uclogic_params_cleanup(&p);
> + return rc;
> +}
> +
> /**
> * uclogic_params_init() - initialize a tablet interface and discover its
> * parameters.
> @@ -1728,6 +1898,16 @@ int uclogic_params_init(struct uclogic_params *params,
> uclogic_params_init_invalid(&p);
> }
>
> + break;
> + case VID_PID(USB_VENDOR_ID_UGEE,
> + USB_DEVICE_ID_UGEE_XPPEN_TABLET_22R_PRO):
> + rc = uclogic_params_init_ugee_xppen_pro(&p,
> + hdev,
> + uclogic_rdesc_xppen_artist_22r_pro_frame_arr,
> + uclogic_rdesc_xppen_artist_22r_pro_frame_size);
> + if (rc != 0)
> + goto cleanup;
> +
> break;
> }
>
> diff --git a/drivers/hid/hid-uclogic-params.h b/drivers/hid/hid-uclogic-params.h
> index a97477c02ff8..6621a75a4b1a 100644
> --- a/drivers/hid/hid-uclogic-params.h
> +++ b/drivers/hid/hid-uclogic-params.h
> @@ -171,6 +171,11 @@ struct uclogic_params_frame {
> * counterclockwise, as opposed to the normal 1 and -1.
> */
> unsigned int bitmap_dial_byte;
> + /*
> + * Destination offset for the second bitmap dial byte, if the tablet
> + * supports a second dial at all.
> + */
> + unsigned int bitmap_second_dial_destination_byte;
> };
>
> /*
> diff --git a/drivers/hid/hid-uclogic-rdesc.c b/drivers/hid/hid-uclogic-rdesc.c
> index fb40775f5f5b..86293ae8c995 100644
> --- a/drivers/hid/hid-uclogic-rdesc.c
> +++ b/drivers/hid/hid-uclogic-rdesc.c
> @@ -859,6 +859,12 @@ const __u8 uclogic_rdesc_v2_frame_dial_arr[] = {
> const size_t uclogic_rdesc_v2_frame_dial_size =
> sizeof(uclogic_rdesc_v2_frame_dial_arr);
>
> +const __u8 uclogic_ugee_v2_probe_arr[] = {
> + 0x02, 0xb0, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +};
> +const size_t uclogic_ugee_v2_probe_size = sizeof(uclogic_ugee_v2_probe_arr);
> +const int uclogic_ugee_v2_probe_endpoint = 0x03;
> +
> /* Fixed report descriptor template for UGEE v2 pen reports */
> const __u8 uclogic_rdesc_ugee_v2_pen_template_arr[] = {
> 0x05, 0x0d, /* Usage Page (Digitizers), */
> @@ -1185,6 +1191,50 @@ const __u8 uclogic_rdesc_xppen_deco01_frame_arr[] = {
> const size_t uclogic_rdesc_xppen_deco01_frame_size =
> sizeof(uclogic_rdesc_xppen_deco01_frame_arr);
>
> +/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
> +const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[] = {

User-space lacks support for dials, but, with this descriptor, would it
be possible to differenciate between the 2 dials to, for example,
assign them different actions? Or would them be exposed as the same
dial?

I have no idea how would user-space see this, but it'd interesting to
see how libinput handles it.

> + 0x05, 0x01, /* Usage Page (Desktop), */
> + 0x09, 0x07, /* Usage (Keypad), */
> + 0xA1, 0x01, /* Collection (Application), */
> + 0x85, UCLOGIC_RDESC_V1_FRAME_ID,
> + /* Report ID (Virtual report), */
> + 0x05, 0x0D, /* Usage Page (Digitizer), */
> + 0x09, 0x39, /* Usage (Tablet Function Keys), */
> + 0xA0, /* Collection (Physical), */
> + 0x14, /* Logical Minimum (0), */
> + 0x25, 0x01, /* Logical Maximum (1), */
> + 0x75, 0x01, /* Report Size (1), */
> + 0x95, 0x08, /* Report Count (8), */
> + 0x81, 0x01, /* Input (Constant), */
> + 0x05, 0x09, /* Usage Page (Button), */
> + 0x19, 0x01, /* Usage Minimum (01h), */
> + 0x29, 0x14, /* Usage Maximum (14h), */
> + 0x95, 0x14, /* Report Count (20), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0x95, 0x14, /* Report Count (20), */
> + 0x81, 0x01, /* Input (Constant), */
> + 0x05, 0x01, /* Usage Page (Desktop), */
> + 0x09, 0x38, /* Usage (Wheel), */
> + 0x75, 0x08, /* Report Size (8), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0x15, 0xFF, /* Logical Minimum (-1), */
> + 0x25, 0x08, /* Logical Maximum (8), */
> + 0x81, 0x06, /* Input (Variable, Relative), */
> + 0x05, 0x0C, /* Usage Page (Consumer Devices), */
> + 0x0A, 0x38, 0x02, /* Usage (AC PAN), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0x81, 0x06, /* Input (Variable, Relative), */
> + 0x26, 0xFF, 0x00, /* Logical Maximum (255), */
> + 0x75, 0x08, /* Report Size (8), */
> + 0x95, 0x01, /* Report Count (1), */
> + 0x81, 0x02, /* Input (Variable), */
> + 0xC0, /* End Collection */
> + 0xC0, /* End Collection */
> +};
> +
> +const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size =
> + sizeof(uclogic_rdesc_xppen_artist_22r_pro_frame_arr);
> +
> /**
> * uclogic_rdesc_template_apply() - apply report descriptor parameters to a
> * report descriptor template, creating a report descriptor. Copies the
> diff --git a/drivers/hid/hid-uclogic-rdesc.h b/drivers/hid/hid-uclogic-rdesc.h
> index a1f78c07293f..c3cb2c75dda5 100644
> --- a/drivers/hid/hid-uclogic-rdesc.h
> +++ b/drivers/hid/hid-uclogic-rdesc.h
> @@ -164,6 +164,11 @@ extern const size_t uclogic_rdesc_v2_frame_dial_size;
> /* Report ID for tweaked UGEE v2 battery reports */
> #define UCLOGIC_RDESC_UGEE_V2_BATTERY_ID 0xba
>
> +/* Magic data expected by UGEEv2 devices on probe */
> +extern const __u8 uclogic_ugee_v2_probe_arr[];
> +extern const size_t uclogic_ugee_v2_probe_size;
> +extern const int uclogic_ugee_v2_probe_endpoint;
> +

You can cherry-pick my patch refactoring this variables and send it as
part of your series. I think that it might help maintainers with the
merge and it'd also fix the problem reported by the test robot.

> /* Fixed report descriptor template for UGEE v2 pen reports */
> extern const __u8 uclogic_rdesc_ugee_v2_pen_template_arr[];
> extern const size_t uclogic_rdesc_ugee_v2_pen_template_size;
> @@ -205,4 +210,8 @@ extern const size_t uclogic_rdesc_ugee_g5_frame_size;
> /* Least-significant bit of Ugee G5 frame rotary encoder state */
> #define UCLOGIC_RDESC_UGEE_G5_FRAME_RE_LSB 38
>
> +/* Fixed report descriptor for XP-Pen Arist 22R Pro frame */
> +extern const __u8 uclogic_rdesc_xppen_artist_22r_pro_frame_arr[];
> +extern const size_t uclogic_rdesc_xppen_artist_22r_pro_frame_size;
> +
> #endif /* _HID_UCLOGIC_RDESC_H */
>
> base-commit: 88603b6dc419445847923fcb7fe5080067a30f98
> --
> 2.38.2
>

2023-01-05 22:57:27

by Joshua Goins

Subject: Re: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro

> I see this event codes in a test device mock in libinput, but I wonder
> if we could/should use BTN_9 instead of skiping a few IDs here.
>
> Honestly, I don't now what should be the right approach in this case,
> let's see if someone else comments on this topic.

I forgot about BTN_9, that should be an easy change. I already merged changes
upstream in systemd/udev to mark devices with these buttons correctly - so,
sunken cost and all that :-)

> I think that you could use uclogic_params_parse_ugee_v2_desc() and
> change the number of buttons in the template afterwards. It'd avoid
> some code duplication.

Yeah I think that's what I'll do, I was thinking of reusing it and just
overriding parameters if needed.

> There are some XP-Pen PRO devices handled by the other init function.
> Maybe we could rename this function to something more specific to your
> device.

Ah okay, it's pretty specific to this device anyway - so I'll rename the
function so it's clear what it's purpose is. I wasn't sure if future XP-PEN
Pro work was going to reuse this function, but I guess we'll rename it if that
happens!

> You can use "uclogic_ugee_v2_probe_endpoint" here.

Good catch! Will change.

> User-space lacks support for dials, but, with this descriptor, would it
> be possible to differenciate between the 2 dials to, for example,
> assign them different actions? Or would them be exposed as the same
> dial?
>
> I have no idea how would user-space see this, but it'd interesting to
> see how libinput handles it.

Currently userspace sees this as REL_WHEEL and REL_HWHEEL, but like
mentioned libinput currently rejects mouse wheel events from tablet pads.
The fact that they previously worked before these patches is because udev
misclassified the pad device as a mouse. I'm working upstream to expose dials
on tablet pads in libinput (I just got the green light so I'll be working on
that shortly!)

> You can cherry-pick my patch refactoring this variables and send it as
> part of your series. I think that it might help maintainers with the
> merge and it'd also fix the problem reported by the test robot.

Ooh good point, I didn't even consider doing that. Next version will have it
split up into two then.