diff mbox series

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

Message ID 20201219062336.72568-5-roderick@gaikai.com
State New
Headers show
Series [01/13] HID: playstation: initial DualSense USB support. | 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;