1. Patchew
  2. linux
  3. View series

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

Dmitry Torokhov posted 1 patch 1 month ago
There is a newer version of this series
drivers/hid/hid-multitouch.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
[PATCH] HID: multitouch: make mt_set_mode() less cryptic
Posted by Dmitry Torokhov 1 month ago 
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
Re: [PATCH] HID: multitouch: make mt_set_mode() less cryptic
Posted by Benjamin Tissoires 3 weeks, 6 days ago 
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
Re: [PATCH] HID: multitouch: make mt_set_mode() less cryptic
Posted by Dmitry Torokhov 3 weeks, 6 days ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.