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

Hi Barnabás,

On Tue, Dec 29, 2020 at 11:49 AM Barnabás Pőcze [off-list ref] wrote:

Hi


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

quoted

[...]
+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.

quoted

+{
+     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.

quoted

+
+     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