Add a new template placeholder to allow configuring the number of
buttons in the drawing tablet frame and update the KUnit tests to
cover the new case.
Signed-off-by: José Expósito <[email protected]>
---
drivers/hid/hid-uclogic-rdesc-test.c | 36 ++++++++++++++++++++++++++++
drivers/hid/hid-uclogic-rdesc.c | 14 ++++++++---
drivers/hid/hid-uclogic-rdesc.h | 6 +++++
3 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-uclogic-rdesc-test.c b/drivers/hid/hid-uclogic-rdesc-test.c
index ded59e226230..ebebffef5f8a 100644
--- a/drivers/hid/hid-uclogic-rdesc-test.c
+++ b/drivers/hid/hid-uclogic-rdesc-test.c
@@ -31,6 +31,10 @@ static const s32 params_pen_some[] = {
[UCLOGIC_RDESC_PEN_PH_ID_X_PM] = 0xBB,
};
+static const s32 params_frame_all[UCLOGIC_RDESC_PH_ID_NUM] = {
+ [UCLOGIC_RDESC_FRAME_PH_ID_UM] = 0xFF,
+};
+
static const __u8 template_empty[] = { };
static const __u8 template_small[] = { 0x00 };
static const __u8 template_no_ph[] = { 0xAA, 0xFE, 0xAA, 0xED, 0x1D };
@@ -39,6 +43,10 @@ static const __u8 template_pen_ph_end[] = {
0xAA, 0xBB, UCLOGIC_RDESC_PEN_PH_HEAD
};
+static const __u8 template_btn_ph_end[] = {
+ 0xAA, 0xBB, UCLOGIC_RDESC_FRAME_PH_BTN_HEAD
+};
+
static const __u8 template_pen_all_params[] = {
UCLOGIC_RDESC_PEN_PH(X_LM),
0x47, UCLOGIC_RDESC_PEN_PH(X_PM),
@@ -55,6 +63,18 @@ static const __u8 expected_pen_all_params[] = {
0x00, 0xEE, 0x00, 0x00, 0x00,
};
+static const __u8 template_frame_all_params[] = {
+ 0x01, 0x02,
+ UCLOGIC_RDESC_FRAME_PH_BTN,
+ 0x99,
+};
+
+static const __u8 expected_frame_all_params[] = {
+ 0x01, 0x02,
+ 0x2A, 0xFF, 0x00,
+ 0x99,
+};
+
static const __u8 template_pen_some_params[] = {
0x01, 0x02,
UCLOGIC_RDESC_PEN_PH(X_LM),
@@ -108,6 +128,14 @@ static struct uclogic_template_case uclogic_template_cases[] = {
.param_num = ARRAY_SIZE(params_pen_all),
.expected = template_pen_ph_end,
},
+ {
+ .name = "Frame button placeholder at the end, without ID",
+ .template = template_btn_ph_end,
+ .template_size = sizeof(template_btn_ph_end),
+ .param_list = params_frame_all,
+ .param_num = ARRAY_SIZE(params_frame_all),
+ .expected = template_btn_ph_end,
+ },
{
.name = "All params present in the pen template",
.template = template_pen_all_params,
@@ -116,6 +144,14 @@ static struct uclogic_template_case uclogic_template_cases[] = {
.param_num = ARRAY_SIZE(params_pen_all),
.expected = expected_pen_all_params,
},
+ {
+ .name = "All params present in the frame template",
+ .template = template_frame_all_params,
+ .template_size = sizeof(template_frame_all_params),
+ .param_list = params_frame_all,
+ .param_num = ARRAY_SIZE(params_frame_all),
+ .expected = expected_frame_all_params,
+ },
{
.name = "Some params present in the pen template (complete param list)",
.template = template_pen_some_params,
diff --git a/drivers/hid/hid-uclogic-rdesc.c b/drivers/hid/hid-uclogic-rdesc.c
index 7126fba80968..3fb84ac492b4 100644
--- a/drivers/hid/hid-uclogic-rdesc.c
+++ b/drivers/hid/hid-uclogic-rdesc.c
@@ -979,7 +979,7 @@ const size_t uclogic_rdesc_xppen_deco01_frame_size =
* uclogic_rdesc_template_apply() - apply report descriptor parameters to a
* report descriptor template, creating a report descriptor. Copies the
* template over to the new report descriptor and replaces every occurrence of
- * UCLOGIC_RDESC_PEN_PH_HEAD, followed by an index byte, with the value from the
+ * the template placeholders, followed by an index byte, with the value from the
* parameter list at that index.
*
* @template_ptr: Pointer to the template buffer.
@@ -996,6 +996,7 @@ __u8 *uclogic_rdesc_template_apply(const __u8 *template_ptr,
const s32 *param_list,
size_t param_num)
{
+ static const __u8 btn_head[] = {UCLOGIC_RDESC_FRAME_PH_BTN_HEAD};
static const __u8 pen_head[] = {UCLOGIC_RDESC_PEN_PH_HEAD};
__u8 *rdesc_ptr;
__u8 *p;
@@ -1005,12 +1006,19 @@ __u8 *uclogic_rdesc_template_apply(const __u8 *template_ptr,
if (rdesc_ptr == NULL)
return NULL;
- for (p = rdesc_ptr; p + sizeof(pen_head) < rdesc_ptr + template_size;) {
- if (memcmp(p, pen_head, sizeof(pen_head)) == 0 &&
+ for (p = rdesc_ptr; p + sizeof(btn_head) < rdesc_ptr + template_size;) {
+ if (p + sizeof(pen_head) < rdesc_ptr + template_size &&
+ memcmp(p, pen_head, sizeof(pen_head)) == 0 &&
p[sizeof(pen_head)] < param_num) {
v = param_list[p[sizeof(pen_head)]];
put_unaligned(cpu_to_le32(v), (s32 *)p);
p += sizeof(pen_head) + 1;
+ } else if (memcmp(p, btn_head, sizeof(btn_head)) == 0 &&
+ p[sizeof(btn_head)] < param_num) {
+ v = param_list[p[sizeof(btn_head)]];
+ put_unaligned((__u8)0x2A, p); /* Usage Maximum */
+ put_unaligned_le16((__force u16)cpu_to_le16(v), p + 1);
+ p += sizeof(btn_head) + 1;
} else {
p++;
}
diff --git a/drivers/hid/hid-uclogic-rdesc.h b/drivers/hid/hid-uclogic-rdesc.h
index 9d37090c39d1..3d78299f082d 100644
--- a/drivers/hid/hid-uclogic-rdesc.h
+++ b/drivers/hid/hid-uclogic-rdesc.h
@@ -82,6 +82,7 @@ extern const size_t uclogic_rdesc_twha60_fixed1_size;
/* Report descriptor template placeholder head */
#define UCLOGIC_RDESC_PEN_PH_HEAD 0xFE, 0xED, 0x1D
+#define UCLOGIC_RDESC_FRAME_PH_BTN_HEAD 0xFE, 0xED
/* Apply report descriptor parameters to a report descriptor template */
extern __u8 *uclogic_rdesc_template_apply(const __u8 *template_ptr,
@@ -96,6 +97,7 @@ enum uclogic_rdesc_ph_id {
UCLOGIC_RDESC_PEN_PH_ID_Y_LM,
UCLOGIC_RDESC_PEN_PH_ID_Y_PM,
UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM,
+ UCLOGIC_RDESC_FRAME_PH_ID_UM,
UCLOGIC_RDESC_PH_ID_NUM
};
@@ -103,6 +105,10 @@ enum uclogic_rdesc_ph_id {
#define UCLOGIC_RDESC_PEN_PH(_ID) \
UCLOGIC_RDESC_PEN_PH_HEAD, UCLOGIC_RDESC_PEN_PH_ID_##_ID
+/* Report descriptor frame buttons template placeholder */
+#define UCLOGIC_RDESC_FRAME_PH_BTN \
+ UCLOGIC_RDESC_FRAME_PH_BTN_HEAD, UCLOGIC_RDESC_FRAME_PH_ID_UM
+
/* Report ID for v1 pen reports */
#define UCLOGIC_RDESC_V1_PEN_ID 0x07
--
2.25.1
The while in question does nothing except provide the possibility
to have an infinite loop in case the subreport id is actually the same
as the pen id.
Signed-off-by: Stefan Berzl <[email protected]>
---
drivers/hid/hid-uclogic-core.c | 55 ++++++++++++++++------------------
1 file changed, 25 insertions(+), 30 deletions(-)
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index c0fe66e50c58..1a6b941f3964 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -423,40 +423,35 @@ static int uclogic_raw_event(struct hid_device *hdev,
if (report->type != HID_INPUT_REPORT)
return 0;
- while (true) {
- /* Tweak pen reports, if necessary */
- if ((report_id == params->pen.id) && (size >= 2)) {
- subreport_list_end =
- params->pen.subreport_list +
- ARRAY_SIZE(params->pen.subreport_list);
- /* Try to match a subreport */
- for (subreport = params->pen.subreport_list;
- subreport < subreport_list_end; subreport++) {
- if (subreport->value != 0 &&
- subreport->value == data[1]) {
- break;
- }
- }
- /* If a subreport matched */
- if (subreport < subreport_list_end) {
- /* Change to subreport ID, and restart */
- report_id = data[0] = subreport->id;
- continue;
- } else {
- return uclogic_raw_event_pen(drvdata, data, size);
+ /* Tweak pen reports, if necessary */
+ if ((report_id == params->pen.id) && (size >= 2)) {
+ subreport_list_end =
+ params->pen.subreport_list +
+ ARRAY_SIZE(params->pen.subreport_list);
+ /* Try to match a subreport */
+ for (subreport = params->pen.subreport_list;
+ subreport < subreport_list_end; subreport++) {
+ if (subreport->value != 0 &&
+ subreport->value == data[1]) {
+ break;
}
}
-
- /* Tweak frame control reports, if necessary */
- for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
- if (report_id == params->frame_list[i].id) {
- return uclogic_raw_event_frame(
- drvdata, ¶ms->frame_list[i],
- data, size);
- }
+ /* If a subreport matched */
+ if (subreport < subreport_list_end) {
+ /* Change to subreport ID, and restart */
+ report_id = data[0] = subreport->id;
+ } else {
+ return uclogic_raw_event_pen(drvdata, data, size);
}
+ }
- break;
+ /* Tweak frame control reports, if necessary */
+ for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
+ if (report_id == params->frame_list[i].id) {
+ return uclogic_raw_event_frame(
+ drvdata, ¶ms->frame_list[i],
+ data, size);
+ }
}
return 0;
--
2.36.1
Hi Stefan,
Thanks for the patch. You can send it as an standalone patch rather
than as a response to my patches, I don't know if it could be missed by
maintaners this way.
On Sun, May 29, 2022 at 11:49:46PM +0200, Stefan Berzl wrote:
> The while in question does nothing except provide the possibility
> to have an infinite loop in case the subreport id is actually the same
> as the pen id.
>
> Signed-off-by: Stefan Berzl <[email protected]>
>
> ---
> drivers/hid/hid-uclogic-core.c | 55 ++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> index c0fe66e50c58..1a6b941f3964 100644
> --- a/drivers/hid/hid-uclogic-core.c
> +++ b/drivers/hid/hid-uclogic-core.c
> @@ -423,40 +423,35 @@ static int uclogic_raw_event(struct hid_device *hdev,
> if (report->type != HID_INPUT_REPORT)
> return 0;
>
> - while (true) {
> - /* Tweak pen reports, if necessary */
> - if ((report_id == params->pen.id) && (size >= 2)) {
> - subreport_list_end =
> - params->pen.subreport_list +
> - ARRAY_SIZE(params->pen.subreport_list);
> - /* Try to match a subreport */
> - for (subreport = params->pen.subreport_list;
> - subreport < subreport_list_end; subreport++) {
> - if (subreport->value != 0 &&
> - subreport->value == data[1]) {
> - break;
> - }
> - }
> - /* If a subreport matched */
> - if (subreport < subreport_list_end) {
> - /* Change to subreport ID, and restart */
> - report_id = data[0] = subreport->id;
> - continue;
Here, in the previous code, the "report_id" is set to the subreport ID
and the while loop is executed again with the new ID. The loop acts as
a recursive function.
Isn't this behaviour removed by your patch?
Jose
> - } else {
> - return uclogic_raw_event_pen(drvdata, data, size);
> + /* Tweak pen reports, if necessary */
> + if ((report_id == params->pen.id) && (size >= 2)) {
> + subreport_list_end =
> + params->pen.subreport_list +
> + ARRAY_SIZE(params->pen.subreport_list);
> + /* Try to match a subreport */
> + for (subreport = params->pen.subreport_list;
> + subreport < subreport_list_end; subreport++) {
> + if (subreport->value != 0 &&
> + subreport->value == data[1]) {
> + break;
> }
> }
> -
> - /* Tweak frame control reports, if necessary */
> - for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> - if (report_id == params->frame_list[i].id) {
> - return uclogic_raw_event_frame(
> - drvdata, ¶ms->frame_list[i],
> - data, size);
> - }
> + /* If a subreport matched */
> + if (subreport < subreport_list_end) {
> + /* Change to subreport ID, and restart */
> + report_id = data[0] = subreport->id;
> + } else {
> + return uclogic_raw_event_pen(drvdata, data, size);
> }
> + }
>
> - break;
> + /* Tweak frame control reports, if necessary */
> + for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> + if (report_id == params->frame_list[i].id) {
> + return uclogic_raw_event_frame(
> + drvdata, ¶ms->frame_list[i],
> + data, size);
> + }
> }
>
> return 0;
> --
> 2.36.1
>
>
On Mon, May 30, 2022 at 06:46:38PM +0200, Stefan Berzl wrote:
> Think about what this behavior really achieves. In the first iteration,
> we check if params->pen.id equals the report_id, which is the actual
> report id from the usb message. If that is the case, we check if the
> second byte of the message is such that we need an updated "subreport"
> for this particular message. Therefore, the report_id is set to the
> subreport->id. This subreport->id is by design supposed to be different
> from the original params->pen.id, because otherwise, why would we need
> this update? If we then "continue" with this useless loop, either one of
> two cases can happen:
>
> The best case is that the (report_id = subreport->id) != params->pen.id
> in which case the if-block won't be executed and we only wasted time.
>
> If the (report_id = subreport->id) == params->pen.id however, things get
> interesting. The "subreport_list_end" and "subreport" variables will
> again be set to entries based on "params->pen.subreport_list", which is
> totally unchanged from the last iteration. We will iterate the same
> subreports, find the same result, set report_id to the same
> subreport->id and, that's the beauty of it, "continue" this ingenious
> loop, creating an infinite loop.
True, data[1] doesn't change, so an extra if is executed for no reason.
I mean, it is not dramatic, but I guess the while loop could be removed
for clarity.
I wonder why it was implemented in a loop though, check commit
8b013098be ("HID: uclogic: Replace pen_frame_flag with subreport_list").
The while loop is intrudeced there and I can imagine that for a good
reason... However, I can not think in a case where removing the loop
could cause issues.
> This contraption is in the best case only wasteful, yet it has been
> accepted all willy-nilly like. Really gets the noggin joggin.
>
> >
> >> - } else {
> >> - return uclogic_raw_event_pen(drvdata, data, size);
> >> + /* Tweak pen reports, if necessary */
> >> + if ((report_id == params->pen.id) && (size >= 2)) {
> >> + subreport_list_end =
> >> + params->pen.subreport_list +
> >> + ARRAY_SIZE(params->pen.subreport_list);
> >> + /* Try to match a subreport */
> >> + for (subreport = params->pen.subreport_list;
> >> + subreport < subreport_list_end; subreport++) {
> >> + if (subreport->value != 0 &&
> >> + subreport->value == data[1]) {
> >> + break;
> >> }
> >> }
> >> -
> >> - /* Tweak frame control reports, if necessary */
> >> - for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> >> - if (report_id == params->frame_list[i].id) {
> >> - return uclogic_raw_event_frame(
> >> - drvdata, ¶ms->frame_list[i],
> >> - data, size);
> >> - }
> >> + /* If a subreport matched */
> >> + if (subreport < subreport_list_end) {
> >> + /* Change to subreport ID, and restart */
> >> + report_id = data[0] = subreport->id;
> >> + } else {
> >> + return uclogic_raw_event_pen(drvdata, data, size);
> >> }
> >> + }
> >>
> >> - break;
> >> + /* Tweak frame control reports, if necessary */
> >> + for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> >> + if (report_id == params->frame_list[i].id) {
> >> + return uclogic_raw_event_frame(
> >> + drvdata, ¶ms->frame_list[i],
> >> + data, size);
> >> + }
> >> }
> >>
> >> return 0;
> >> --
> >> 2.36.1
> >>
> >>
>
> Bye bye
>
> Stefan Berzl
Hello everynyan!
> Hi Stefan,
>
> Thanks for the patch. You can send it as an standalone patch rather
> than as a response to my patches, I don't know if it could be missed by
> maintaners this way.
You are right about that, I'll keep it in mind. But for consistency,
I think it's better to reply here for now.
>> The while in question does nothing except provide the possibility
>> to have an infinite loop in case the subreport id is actually the same
>> as the pen id.
>>
>> Signed-off-by: Stefan Berzl <[email protected]>
>>
>> ---
>> drivers/hid/hid-uclogic-core.c | 55 ++++++++++++++++------------------
>> 1 file changed, 25 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
>> index c0fe66e50c58..1a6b941f3964 100644
>> --- a/drivers/hid/hid-uclogic-core.c
>> +++ b/drivers/hid/hid-uclogic-core.c
>> @@ -423,40 +423,35 @@ static int uclogic_raw_event(struct hid_device *hdev,
>> if (report->type != HID_INPUT_REPORT)
>> return 0;
>>
>> - while (true) {
>> - /* Tweak pen reports, if necessary */
>> - if ((report_id == params->pen.id) && (size >= 2)) {
>> - subreport_list_end =
>> - params->pen.subreport_list +
>> - ARRAY_SIZE(params->pen.subreport_list);
>> - /* Try to match a subreport */
>> - for (subreport = params->pen.subreport_list;
>> - subreport < subreport_list_end; subreport++) {
>> - if (subreport->value != 0 &&
>> - subreport->value == data[1]) {
>> - break;
>> - }
>> - }
>> - /* If a subreport matched */
>> - if (subreport < subreport_list_end) {
>> - /* Change to subreport ID, and restart */
>> - report_id = data[0] = subreport->id;
>> - continue;
>
> Here, in the previous code, the "report_id" is set to the subreport ID
> and the while loop is executed again with the new ID. The loop acts as
> a recursive function.
>
> Isn't this behaviour removed by your patch?
>
> Jose
Think about what this behavior really achieves. In the first iteration,
we check if params->pen.id equals the report_id, which is the actual
report id from the usb message. If that is the case, we check if the
second byte of the message is such that we need an updated "subreport"
for this particular message. Therefore, the report_id is set to the
subreport->id. This subreport->id is by design supposed to be different
from the original params->pen.id, because otherwise, why would we need
this update? If we then "continue" with this useless loop, either one of
two cases can happen:
The best case is that the (report_id = subreport->id) != params->pen.id
in which case the if-block won't be executed and we only wasted time.
If the (report_id = subreport->id) == params->pen.id however, things get
interesting. The "subreport_list_end" and "subreport" variables will
again be set to entries based on "params->pen.subreport_list", which is
totally unchanged from the last iteration. We will iterate the same
subreports, find the same result, set report_id to the same
subreport->id and, that's the beauty of it, "continue" this ingenious
loop, creating an infinite loop.
This contraption is in the best case only wasteful, yet it has been
accepted all willy-nilly like. Really gets the noggin joggin.
>
>> - } else {
>> - return uclogic_raw_event_pen(drvdata, data, size);
>> + /* Tweak pen reports, if necessary */
>> + if ((report_id == params->pen.id) && (size >= 2)) {
>> + subreport_list_end =
>> + params->pen.subreport_list +
>> + ARRAY_SIZE(params->pen.subreport_list);
>> + /* Try to match a subreport */
>> + for (subreport = params->pen.subreport_list;
>> + subreport < subreport_list_end; subreport++) {
>> + if (subreport->value != 0 &&
>> + subreport->value == data[1]) {
>> + break;
>> }
>> }
>> -
>> - /* Tweak frame control reports, if necessary */
>> - for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
>> - if (report_id == params->frame_list[i].id) {
>> - return uclogic_raw_event_frame(
>> - drvdata, ¶ms->frame_list[i],
>> - data, size);
>> - }
>> + /* If a subreport matched */
>> + if (subreport < subreport_list_end) {
>> + /* Change to subreport ID, and restart */
>> + report_id = data[0] = subreport->id;
>> + } else {
>> + return uclogic_raw_event_pen(drvdata, data, size);
>> }
>> + }
>>
>> - break;
>> + /* Tweak frame control reports, if necessary */
>> + for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
>> + if (report_id == params->frame_list[i].id) {
>> + return uclogic_raw_event_frame(
>> + drvdata, ¶ms->frame_list[i],
>> + data, size);
>> + }
>> }
>>
>> return 0;
>> --
>> 2.36.1
>>
>>
Bye bye
Stefan Berzl