diff mbox series

[04/13] HID: playstation: add DualSense touchpad support.

Message ID 20201219062336.72568-5-roderick@gaikai.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series HID: new driver for PS5 'DualSense' controller | expand

Commit Message

Roderick Colenbrander Dec. 19, 2020, 6:23 a.m. UTC 
From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Implement support for DualSense touchpad as a separate input device.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 59 +++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

Comments

Samuel Čavoj Dec. 26, 2020, 2:14 a.m. UTC | #1 
Hello,

thank you for this driver. It makes me really happy to see an official
one. Just a small thing I noticed while reading through it:


On 18.12.2020 22:23, Roderick Colenbrander wrote:
> @@ -311,6 +345,25 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
>  	input_sync(ds->gamepad);
>  
> +	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);

The above line is duplicated right before the input_sync (marked). Is
there any reason for this or is it accidental?

> +	for (i = 0; i < 2; i++) {
> +		bool active = (ds_report->points[i].contact & 0x80) ? false : true;
> +
> +		input_mt_slot(ds->touchpad, i);
> +		input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
> +
> +		if (active) {
> +			int x = (ds_report->points[i].x_hi << 8) | ds_report->points[i].x_lo;
> +			int y = (ds_report->points[i].y_hi << 4) | ds_report->points[i].y_lo;
> +
> +			input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
> +			input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
> +		}
> +	}
> +	input_mt_sync_frame(ds->touchpad);
> +	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);

Right here.

> +	input_sync(ds->touchpad);
> +

Regards,
Samuel
Roderick Colenbrander Dec. 26, 2020, 10:27 p.m. UTC | #2 
Hi Samuel,

Thanks for your review.


On Fri, Dec 25, 2020 at 6:14 PM Samuel Čavoj <samuel@cavoj.net> wrote:
>
> Hello,
>
> thank you for this driver. It makes me really happy to see an official
> one. Just a small thing I noticed while reading through it:
>
>
> On 18.12.2020 22:23, Roderick Colenbrander wrote:
> > @@ -311,6 +345,25 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> >       input_sync(ds->gamepad);
> >
> > +     input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
>
> The above line is duplicated right before the input_sync (marked). Is
> there any reason for this or is it accidental?

Good catch. It was purely accidental as the code went through many
rebases and code shuffling. So a little artifact..

>
> > +     for (i = 0; i < 2; i++) {
> > +             bool active = (ds_report->points[i].contact & 0x80) ? false : true;
> > +
> > +             input_mt_slot(ds->touchpad, i);
> > +             input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
> > +
> > +             if (active) {
> > +                     int x = (ds_report->points[i].x_hi << 8) | ds_report->points[i].x_lo;
> > +                     int y = (ds_report->points[i].y_hi << 4) | ds_report->points[i].y_lo;
> > +
> > +                     input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
> > +                     input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
> > +             }
> > +     }
> > +     input_mt_sync_frame(ds->touchpad);
> > +     input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
>
> Right here.

I will probably keep this one and take the other one out as this is
next to the other touchpad code.

>
> > +     input_sync(ds->touchpad);
> > +
>
> Regards,
> Samuel

Thanks,
Roderick
Barnabás Pőcze Dec. 29, 2020, 7:49 p.m. UTC | #3 
Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> +static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
> +		int num_contacts)

Very minor thing, but `input_mt_init_slots()` takes an `unsigned int`, so
wouldn't it be better if `num_contacts` was an `unsigned int`?


> +{
> +	struct input_dev *touchpad;
> +	int ret;
> +
> +	touchpad = ps_allocate_input_dev(hdev, "Touchpad");
> +	if (IS_ERR(touchpad))
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Map button underneath touchpad to BTN_LEFT. */
> +	input_set_capability(touchpad, EV_KEY, BTN_LEFT);
> +	__set_bit(INPUT_PROP_BUTTONPAD, touchpad->propbit);
> +
> +	input_set_abs_params(touchpad, ABS_MT_POSITION_X, 0, width, 0, 0);
> +	input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height, 0, 0);

Shouldn't it be `width - 1` and `height - 1`? Or what am I missing?


> +
> +	ret = input_mt_init_slots(touchpad, num_contacts, INPUT_MT_POINTER);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = input_register_device(touchpad);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return touchpad;
> +}
> +
>  static int dualsense_get_mac_address(struct dualsense *ds)
>  {
>  	uint8_t *buf;
> @@ -271,6 +304,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	uint8_t battery_data, battery_capacity, charging_status, value;
>  	int battery_status;
>  	unsigned long flags;
> +	int i;
>
>  	/* DualSense in USB uses the full HID report for reportID 1, but
>  	 * Bluetooth uses a minimal HID report for reportID 1 and reports
> @@ -311,6 +345,25 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
>  	input_sync(ds->gamepad);
>
> +	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> +	for (i = 0; i < 2; i++) {
> +		bool active = (ds_report->points[i].contact & 0x80) ? false : true;
> [...]

I believe it'd be preferable to give a name to that 0x80.


Regards,
Barnabás Pőcze
Roderick Colenbrander Dec. 29, 2020, 9:44 p.m. UTC | #4 
Hi Barnabás,

On Tue, Dec 29, 2020 at 11:49 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > +static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
> > +             int num_contacts)
>
> Very minor thing, but `input_mt_init_slots()` takes an `unsigned int`, so
> wouldn't it be better if `num_contacts` was an `unsigned int`?

Agreed.

>
> > +{
> > +     struct input_dev *touchpad;
> > +     int ret;
> > +
> > +     touchpad = ps_allocate_input_dev(hdev, "Touchpad");
> > +     if (IS_ERR(touchpad))
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     /* Map button underneath touchpad to BTN_LEFT. */
> > +     input_set_capability(touchpad, EV_KEY, BTN_LEFT);
> > +     __set_bit(INPUT_PROP_BUTTONPAD, touchpad->propbit);
> > +
> > +     input_set_abs_params(touchpad, ABS_MT_POSITION_X, 0, width, 0, 0);
> > +     input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height, 0, 0);
>
> Shouldn't it be `width - 1` and `height - 1`? Or what am I missing?

I suspect that's what it should be. The docs aren't very clear on that
and after glancing around other drivers (in particular
input/touchscreen) many seem to be off by 1 as well. I think it should
indeed be 'width - 1' as also related axes are created through
input_mt_init_slots like ABS_X and ABS_Y, which inherit the same
dimensions and which would also be off by 1. So yeah, good catch.

> > +
> > +     ret = input_mt_init_slots(touchpad, num_contacts, INPUT_MT_POINTER);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     ret = input_register_device(touchpad);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     return touchpad;
> > +}
> > +
> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -271,6 +304,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       uint8_t battery_data, battery_capacity, charging_status, value;
> >       int battery_status;
> >       unsigned long flags;
> > +     int i;
> >
> >       /* DualSense in USB uses the full HID report for reportID 1, but
> >        * Bluetooth uses a minimal HID report for reportID 1 and reports
> > @@ -311,6 +345,25 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> >       input_sync(ds->gamepad);
> >
> > +     input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> > +     for (i = 0; i < 2; i++) {
> > +             bool active = (ds_report->points[i].contact & 0x80) ? false : true;
> > [...]
>
> I believe it'd be preferable to give a name to that 0x80.

Will do.

>
> Regards,
> Barnabás Pőcze

Regards,
Roderick
diff mbox series

Patch

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 598d262e2a08..7e622b02ee30 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -56,9 +56,14 @@  struct ps_device {
 #define DS_STATUS_CHARGING		GENMASK(7, 4)
 #define DS_STATUS_CHARGING_SHIFT	4
 
+/* DualSense hardware limits */
+#define DS_TOUCHPAD_WIDTH	1920
+#define DS_TOUCHPAD_HEIGHT	1080
+
 struct dualsense {
 	struct ps_device base;
 	struct input_dev *gamepad;
+	struct input_dev *touchpad;
 };
 
 struct dualsense_touch_point {
@@ -239,6 +244,34 @@  static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 	return gamepad;
 }
 
+static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
+		int num_contacts)
+{
+	struct input_dev *touchpad;
+	int ret;
+
+	touchpad = ps_allocate_input_dev(hdev, "Touchpad");
+	if (IS_ERR(touchpad))
+		return ERR_PTR(-ENOMEM);
+
+	/* Map button underneath touchpad to BTN_LEFT. */
+	input_set_capability(touchpad, EV_KEY, BTN_LEFT);
+	__set_bit(INPUT_PROP_BUTTONPAD, touchpad->propbit);
+
+	input_set_abs_params(touchpad, ABS_MT_POSITION_X, 0, width, 0, 0);
+	input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height, 0, 0);
+
+	ret = input_mt_init_slots(touchpad, num_contacts, INPUT_MT_POINTER);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = input_register_device(touchpad);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return touchpad;
+}
+
 static int dualsense_get_mac_address(struct dualsense *ds)
 {
 	uint8_t *buf;
@@ -271,6 +304,7 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	uint8_t battery_data, battery_capacity, charging_status, value;
 	int battery_status;
 	unsigned long flags;
+	int i;
 
 	/* DualSense in USB uses the full HID report for reportID 1, but
 	 * Bluetooth uses a minimal HID report for reportID 1 and reports
@@ -311,6 +345,25 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
 	input_sync(ds->gamepad);
 
+	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
+	for (i = 0; i < 2; i++) {
+		bool active = (ds_report->points[i].contact & 0x80) ? false : true;
+
+		input_mt_slot(ds->touchpad, i);
+		input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
+
+		if (active) {
+			int x = (ds_report->points[i].x_hi << 8) | ds_report->points[i].x_lo;
+			int y = (ds_report->points[i].y_hi << 4) | ds_report->points[i].y_lo;
+
+			input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
+			input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
+		}
+	}
+	input_mt_sync_frame(ds->touchpad);
+	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
+	input_sync(ds->touchpad);
+
 	battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
 	charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
 
@@ -382,6 +435,12 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
 		goto err;
 	}
 
+	ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
+	if (IS_ERR(ds->touchpad)) {
+		ret = PTR_ERR(ds->touchpad);
+		goto err;
+	}
+
 	ret = ps_device_register_battery((struct ps_device *)ds);
 	if (ret < 0)
 		goto err;