Re: [PATCH] HID: multitouch: make mt_set_mode() less cryptic

On Mon, Oct 28, 2024 at 04:47:55PM +0100, Benjamin Tissoires wrote:

On Oct 25 2024, Dmitry Torokhov wrote:

quoted

mt_set_mode() accepts 2 boolean switches indicating whether the device
(if it follows Windows Precision Touchpad specification) should report
hardware buttons and/or surface contacts. For a casual reader it is
completely not clear, as they look at the call site, which exact mode
is being requested.

Define report_mode enum and change mt_set_mode() to accept is as
an argument instead. This allows to write:

	mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);

or

	mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_BUTTONS);

which makes intent much more clear.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-multitouch.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 99812c0f830b..e4bb2fb5596d 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -83,6 +83,13 @@ enum latency_mode {
 	HID_LATENCY_HIGH = 1,
 };
 
+enum report_mode {
+	TOUCHPAD_REPORT_NONE = 0,
+	TOUCHPAD_REPORT_BUTTONS = 1,
+	TOUCHPAD_REPORT_CONTACTS = 2,

Maybe to be more obvious, BIT(0) and BIT(1) for the 2 values above?

I'm just concerned that someone adds "3" if we ever need to add a new
value.

Right, I'll change it.

quoted

+	TOUCHPAD_REPORT_ALL = TOUCHPAD_REPORT_BUTTONS | TOUCHPAD_REPORT_CONTACTS,
+};
+
 #define MT_IO_FLAGS_RUNNING		0
 #define MT_IO_FLAGS_ACTIVE_SLOTS	1
 #define MT_IO_FLAGS_PENDING_SLOTS	2

@@ -1486,8 +1493,7 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev,
 				     struct hid_field *field,
 				     struct hid_usage *usage,
 				     enum latency_mode latency,
-				     bool surface_switch,
-				     bool button_switch,
+				     enum report_mode report_mode,
 				     bool *inputmode_found)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);

@@ -1542,11 +1548,11 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev,
 		return true;
 
 	case HID_DG_SURFACESWITCH:
-		field->value[index] = surface_switch;
+		field->value[index] = report_mode & TOUCHPAD_REPORT_CONTACTS;

Just to be on the safe side:
!!(report_mode & TOUCHPAD_REPORT_CONTACTS);

Oh, yes, that makes sense. I'll send an updated patch in a minute.

Thanks.

-- 
Dmitry