| Message ID | ZxwciG6YeVFgfDRU@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID: multitouch: make mt_set_mode() less cryptic | expand |
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
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.
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; }
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(-)