* [PATCH] HID: multitouch: make mt_set_mode() less cryptic
@ 2024-10-25 22:32 Dmitry Torokhov
2024-10-28 15:47 ` Benjamin Tissoires
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2024-10-25 22:32 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
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,
+ 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;
return true;
case HID_DG_BUTTONSWITCH:
- field->value[index] = button_switch;
+ field->value[index] = report_mode & TOUCHPAD_REPORT_BUTTONS;
return true;
}
@@ -1554,7 +1560,7 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev,
}
static void mt_set_modes(struct hid_device *hdev, enum latency_mode latency,
- bool surface_switch, bool button_switch)
+ enum report_mode report_mode)
{
struct hid_report_enum *rep_enum;
struct hid_report *rep;
@@ -1579,8 +1585,7 @@ static void mt_set_modes(struct hid_device *hdev, enum latency_mode latency,
rep->field[i],
usage,
latency,
- surface_switch,
- button_switch,
+ report_mode,
&inputmode_found))
update_report = true;
}
@@ -1820,7 +1825,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
dev_warn(&hdev->dev, "Cannot allocate sysfs group for %s\n",
hdev->name);
- mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
+ mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
return 0;
}
@@ -1832,9 +1837,9 @@ static int mt_suspend(struct hid_device *hdev, pm_message_t state)
/* High latency is desirable for power savings during S3/S0ix */
if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
!hid_hw_may_wakeup(hdev))
- mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
+ mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_NONE);
else
- mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
+ mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_ALL);
return 0;
}
@@ -1842,7 +1847,7 @@ static int mt_suspend(struct hid_device *hdev, pm_message_t state)
static int mt_reset_resume(struct hid_device *hdev)
{
mt_release_contacts(hdev);
- mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
+ mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
return 0;
}
@@ -1854,7 +1859,7 @@ static int mt_resume(struct hid_device *hdev)
hid_hw_idle(hdev, 0, 0, HID_REQ_SET_IDLE);
- mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
+ mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
return 0;
}
--
2.47.0.163.g1226f6d8fa-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] HID: multitouch: make mt_set_mode() less cryptic
2024-10-25 22:32 [PATCH] HID: multitouch: make mt_set_mode() less cryptic Dmitry Torokhov
@ 2024-10-28 15:47 ` Benjamin Tissoires
2024-10-28 19:04 ` Dmitry Torokhov
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Tissoires @ 2024-10-28 15:47 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Jiri Kosina, linux-input, linux-kernel
On Oct 25 2024, Dmitry Torokhov wrote:
> 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.
> + 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);
> return true;
>
> case HID_DG_BUTTONSWITCH:
> - field->value[index] = button_switch;
> + field->value[index] = report_mode & TOUCHPAD_REPORT_BUTTONS;
same here.
> return true;
> }
>
> @@ -1554,7 +1560,7 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev,
> }
>
> static void mt_set_modes(struct hid_device *hdev, enum latency_mode latency,
> - bool surface_switch, bool button_switch)
> + enum report_mode report_mode)
> {
> struct hid_report_enum *rep_enum;
> struct hid_report *rep;
> @@ -1579,8 +1585,7 @@ static void mt_set_modes(struct hid_device *hdev, enum latency_mode latency,
> rep->field[i],
> usage,
> latency,
> - surface_switch,
> - button_switch,
> + report_mode,
> &inputmode_found))
> update_report = true;
> }
> @@ -1820,7 +1825,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> dev_warn(&hdev->dev, "Cannot allocate sysfs group for %s\n",
> hdev->name);
>
> - mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
> + mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
>
> return 0;
> }
> @@ -1832,9 +1837,9 @@ static int mt_suspend(struct hid_device *hdev, pm_message_t state)
> /* High latency is desirable for power savings during S3/S0ix */
> if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
> !hid_hw_may_wakeup(hdev))
> - mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
> + mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_NONE);
> else
> - mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
> + mt_set_modes(hdev, HID_LATENCY_HIGH, TOUCHPAD_REPORT_ALL);
>
> return 0;
> }
> @@ -1842,7 +1847,7 @@ static int mt_suspend(struct hid_device *hdev, pm_message_t state)
> static int mt_reset_resume(struct hid_device *hdev)
> {
> mt_release_contacts(hdev);
> - mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
> + mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
heh, I wonder if we actually need the split buttons/touches, because in
all cases, we are either reporting None or All.
Anyway, with the couple of nitpicks:
Reviewed-by: Benjamin Tissoires <bentiss@kernel.org>
Cheers,
Benjamin
> return 0;
> }
>
> @@ -1854,7 +1859,7 @@ static int mt_resume(struct hid_device *hdev)
>
> hid_hw_idle(hdev, 0, 0, HID_REQ_SET_IDLE);
>
> - mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
> + mt_set_modes(hdev, HID_LATENCY_NORMAL, TOUCHPAD_REPORT_ALL);
>
> return 0;
> }
> --
> 2.47.0.163.g1226f6d8fa-goog
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] HID: multitouch: make mt_set_mode() less cryptic
2024-10-28 15:47 ` Benjamin Tissoires
@ 2024-10-28 19:04 ` Dmitry Torokhov
0 siblings, 0 replies; 3+ messages in thread
From: Dmitry Torokhov @ 2024-10-28 19:04 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, linux-kernel
On Mon, Oct 28, 2024 at 04:47:55PM +0100, Benjamin Tissoires wrote:
> On Oct 25 2024, Dmitry Torokhov wrote:
> > 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.
>
> > + 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-28 19:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-25 22:32 [PATCH] HID: multitouch: make mt_set_mode() less cryptic Dmitry Torokhov
2024-10-28 15:47 ` Benjamin Tissoires
2024-10-28 19:04 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).