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@xxxxxxxxx>
> > ---
> > 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
