[PATCH v1 0/7] HID: playstation: DS4: LED bugfix, third-party gamepad support

[PATCH v1 0/7] HID: playstation: DS4: LED bugfix, third-party gamepad support

[Max Staudt]

Dear hid-playstation maintainers,

Could you please have a look at the enclosed patches for the DualShock 4
driver in hid-playstation, and upstream them if possible?


There is one bugfix, and a few small patches to enable third-party
controllers. They sometimes don't implement features that they
semantically "don't need", but which currently trip the driver.

For example, for the DualShock 4, we don't actually need to know the
firmware version in order to work with the gamepad - unlike with the
DualSense, which has different driver logic depending on the version.


Finally, there are two patches to add a DS4 compatible controller with
an unassigned VID/PID - I'd appreciate your thoughts on that.


If I can make it easier to upstream these patches, please let me know.

Thanks!

Max


Patches in this series:
  [PATCH v1 1/7] HID: playstation: DS4: Fix LED blinking
  [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address
  [PATCH v1 3/7] HID: playstation: DS4: Don't fail on FW/HW version
  [PATCH v1 4/7] HID: playstation: DS4: Don't fail on calibration data
  [PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01
  [PATCH v1 6/7] HID: playstation: Simplify device type ID
  [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER

[PATCH v1 1/7] HID: playstation: DS4: Fix LED blinking

[Max Staudt]

There was no way to disable blinking once enabled.
Disable it on brightness = 0, as per the Linux LED spec.

The driver reports back the values it sends to the controller, but they
need to be scaled back to milliseconds. Setting the LED blinking via
sysfs works as expected now.

Signed-off-by: Max Staudt <max@enpas.org>
---
 drivers/hid/hid-playstation.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 8ac8f7b8e317..7f50e13601f0 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2037,8 +2037,9 @@ static int dualshock4_led_set_blink(struct led_classdev *led, unsigned long *del
 
 	dualshock4_schedule_work(ds4);
 
-	*delay_on = ds4->lightbar_blink_on;
-	*delay_off = ds4->lightbar_blink_off;
+	/* Report scaled values back to LED subsystem */
+	*delay_on = ds4->lightbar_blink_on * 10;
+	*delay_off = ds4->lightbar_blink_off * 10;
 
 	return 0;
 }
@@ -2065,6 +2066,13 @@ static int dualshock4_led_set_brightness(struct led_classdev *led, enum led_brig
 		break;
 	case 3:
 		ds4->lightbar_enabled = !!value;
+
+		/* brightness = 0 also cancels blinking in Linux. */
+		if (!ds4->lightbar_enabled) {
+			ds4->lightbar_blink_off = 0;
+			ds4->lightbar_blink_on = 0;
+			ds4->update_lightbar_blink = true;
+		}
 	}
 
 	ds4->update_lightbar = true;
-- 
2.39.2

[PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address request

[Max Staudt]

Some third-party controllers can't report their MAC address.

Since a unique ID is needed for ps_devices_list_add() and
ps_device_register_battery(), let's use hdev->id for this when we don't
have a MAC address.

Signed-off-by: Max Staudt <max@enpas.org>
---
 drivers/hid/hid-playstation.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 7f50e13601f0..0a3c442af305 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1966,7 +1966,10 @@ static int dualshock4_get_mac_address(struct dualshock4 *ds4)
 				DS4_FEATURE_REPORT_PAIRING_INFO_SIZE, false);
 		if (ret) {
 			hid_err(hdev, "Failed to retrieve DualShock4 pairing info: %d\n", ret);
-			goto err_free;
+			hid_err(hdev, "Generating fake MAC address for this device.\n");
+			buf[1] = (hdev->id >>  0) & 0xff;
+			buf[2] = (hdev->id >>  8) & 0xff;
+			buf[3] = (hdev->id >> 16) & 0xff;
 		}
 
 		memcpy(ds4->base.mac_address, &buf[1], sizeof(ds4->base.mac_address));
@@ -1986,7 +1989,6 @@ static int dualshock4_get_mac_address(struct dualshock4 *ds4)
 		return 0;
 	}
 
-err_free:
 	kfree(buf);
 	return ret;
 }
@@ -2552,7 +2554,7 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
 	ret = dualshock4_get_mac_address(ds4);
 	if (ret) {
 		hid_err(hdev, "Failed to get MAC address from DualShock4\n");
-		return ERR_PTR(ret);
+		hid_err(hdev, "Can't detect simultaneous USB/BT connections from this device.\n");
 	}
 	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds4->base.mac_address);
 
-- 
2.39.2

Re: [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address request

[Roderick Colenbrander]

On Mon, Jan 15, 2024 at 6:58 AM Max Staudt <max@enpas.org> wrote:
>
> Some third-party controllers can't report their MAC address.
>
> Since a unique ID is needed for ps_devices_list_add() and
> ps_device_register_battery(), let's use hdev->id for this when we don't
> have a MAC address.
>
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
>  drivers/hid/hid-playstation.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 7f50e13601f0..0a3c442af305 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -1966,7 +1966,10 @@ static int dualshock4_get_mac_address(struct dualshock4 *ds4)
>                                 DS4_FEATURE_REPORT_PAIRING_INFO_SIZE, false);
>                 if (ret) {
>                         hid_err(hdev, "Failed to retrieve DualShock4 pairing info: %d\n", ret);
> -                       goto err_free;
> +                       hid_err(hdev, "Generating fake MAC address for this device.\n");
> +                       buf[1] = (hdev->id >>  0) & 0xff;
> +                       buf[2] = (hdev->id >>  8) & 0xff;
> +                       buf[3] = (hdev->id >> 16) & 0xff;
>                 }
>
>                 memcpy(ds4->base.mac_address, &buf[1], sizeof(ds4->base.mac_address));
> @@ -1986,7 +1989,6 @@ static int dualshock4_get_mac_address(struct dualshock4 *ds4)
>                 return 0;
>         }
>
> -err_free:
>         kfree(buf);
>         return ret;
>  }
> @@ -2552,7 +2554,7 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
>         ret = dualshock4_get_mac_address(ds4);
>         if (ret) {
>                 hid_err(hdev, "Failed to get MAC address from DualShock4\n");
> -               return ERR_PTR(ret);
> +               hid_err(hdev, "Can't detect simultaneous USB/BT connections from this device.\n");
>         }
>         snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds4->base.mac_address);
>
> --
> 2.39.2
>
>

Hi Max,

For what type of devices is this not working? This one example of this
request which is very foundational for a controller working on even
the game console. Are this perhaps USB-only devices? If the case maybe
some kind of error is only needed for USB connections.

Thanks,
Roderick

Re: [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address request

[Max Staudt]

On 1/25/24 09:39, Roderick Colenbrander wrote:
> On Mon, Jan 15, 2024 at 6:58 AM Max Staudt <max@enpas.org> wrote:
>>
>> Some third-party controllers can't report their MAC address.
>>
> For what type of devices is this not working? This one example of this
> request which is very foundational for a controller working on even
> the game console. Are this perhaps USB-only devices? If the case maybe
> some kind of error is only needed for USB connections.

IIRC I've only seen this quirk with the oddball VID/PID 7545:0104 that my patch 7/7 adds. It is indeed a USB-only controller. I have not tried this device (or any, really) with an actual console, my focus is just on Linux. Admittedly, I'd also like to know what happens on a real PS4 :)

My intention was to keep the error message and the solution as universal as possible, in case a controller comes along that has both this quirk *and* Bluetooth. It's a bit of an ugly workaround though - if you (or anyone else) have an idea for a nicer solution, I'd be really glad.


Max

Re: [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address request

[Roderick Colenbrander]

On Sat, Jan 27, 2024 at 12:51 AM Max Staudt <max@enpas.org> wrote:
>
> On 1/25/24 09:39, Roderick Colenbrander wrote:
> > On Mon, Jan 15, 2024 at 6:58 AM Max Staudt <max@enpas.org> wrote:
> >>
> >> Some third-party controllers can't report their MAC address.
> >>
> > For what type of devices is this not working? This one example of this
> > request which is very foundational for a controller working on even
> > the game console. Are this perhaps USB-only devices? If the case maybe
> > some kind of error is only needed for USB connections.
>
> IIRC I've only seen this quirk with the oddball VID/PID 7545:0104 that my patch 7/7 adds. It is indeed a USB-only controller. I have not tried this device (or any, really) with an actual console, my focus is just on Linux. Admittedly, I'd also like to know what happens on a real PS4 :)
>
> My intention was to keep the error message and the solution as universal as possible, in case a controller comes along that has both this quirk *and* Bluetooth. It's a bit of an ugly workaround though - if you (or anyone else) have an idea for a nicer solution, I'd be really glad.
>
>
> Max
>

I remember on the console side that we support a number of controllers
including our official model and some licensed controllers. I recall
them taking some different codepaths and HID reports differently. It
has been a while, so I don't recall the details. If I remember it
could be that all of the licensed ones were USB-only (of course there
are some Bluetooth capable clones).

For this reason I think not all DS4-compatible devices have a MAC
address or handle this request. (We made some fixes in hid-playstation
relative to hid-sony, where hid-sony used another less known HID
report for the MAC address. Now we use the more commonly known one and
that helped other clone devices).

I'm not sure about the best way to handle this. I have kind of been
leaning towards doing a vid/pid like check for this case even though I
really hate it. It could be within dualshock4_get_mac_address as we do
some other special handling there too (although having the caller of
dualshock4_get_mac_address do it is an option too, but I think within
get_mac_address is slightly nicer for now).

Thanks,
Roderick

Re: [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address request

[Max Staudt]

On 1/31/24 05:59, Roderick Colenbrander wrote:
> I remember on the console side that we support a number of controllers
> including our official model and some licensed controllers. I recall
> them taking some different codepaths and HID reports differently. It
> has been a while, so I don't recall the details. If I remember it
> could be that all of the licensed ones were USB-only (of course there
> are some Bluetooth capable clones).

Now that is an interesting tidbit... if you learn more, I'd be curious to hear about it if possible!


> I'm not sure about the best way to handle this. I have kind of been
> leaning towards doing a vid/pid like check for this case even though I
> really hate it. It could be within dualshock4_get_mac_address as we do
> some other special handling there too (although having the caller of
> dualshock4_get_mac_address do it is an option too, but I think within
> get_mac_address is slightly nicer for now).

As suggested in the 7545:0104 patch, how about dropping this patch as well, until we encounter a device we really want to add and that does not provide a MAC address?


If it's okay for you, I'd send a v2 of everything after hearing your comment on the patch to make controllers work that don't provide gyro calibration data.


Max

[PATCH v1 3/7] HID: playstation: DS4: Don't fail on FW/HW version request

[Max Staudt]

Some third-party controllers can't report firmware/hardware version.

Unlike for the DualSense, the driver does not use these values for
anything in the DualShock 4 case, but merely exposes them via sysfs.
They will simply be 0x0.

Signed-off-by: Max Staudt <max@enpas.org>
---
 drivers/hid/hid-playstation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 0a3c442af305..12321cae4416 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2561,7 +2561,7 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
 	ret = dualshock4_get_firmware_info(ds4);
 	if (ret) {
 		hid_err(hdev, "Failed to get firmware info from DualShock4\n");
-		return ERR_PTR(ret);
+		hid_err(hdev, "HW/FW version data in sysfs will be invalid.\n");
 	}
 
 	ret = ps_devices_list_add(ps_dev);
-- 
2.39.2

Re: [PATCH v1 3/7] HID: playstation: DS4: Don't fail on FW/HW version request

[Roderick Colenbrander]

On Mon, Jan 15, 2024 at 6:51 AM Max Staudt <max@enpas.org> wrote:
>
> Some third-party controllers can't report firmware/hardware version.
>
> Unlike for the DualSense, the driver does not use these values for
> anything in the DualShock 4 case, but merely exposes them via sysfs.
> They will simply be 0x0.
>
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
>  drivers/hid/hid-playstation.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 0a3c442af305..12321cae4416 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -2561,7 +2561,7 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
>         ret = dualshock4_get_firmware_info(ds4);
>         if (ret) {
>                 hid_err(hdev, "Failed to get firmware info from DualShock4\n");
> -               return ERR_PTR(ret);
> +               hid_err(hdev, "HW/FW version data in sysfs will be invalid.\n");
>         }
>
>         ret = ps_devices_list_add(ps_dev);
> --
> 2.39.2
>
>

This looks good. Perhaps could have been a hid_warn then, but err is
probably fine.

Roderick

Re: [PATCH v1 3/7] HID: playstation: DS4: Don't fail on FW/HW version request

[Max Staudt]

On 1/25/24 09:43, Roderick Colenbrander wrote:
> On Mon, Jan 15, 2024 at 6:51 AM Max Staudt <max@enpas.org> wrote:
>>
>>          ret = dualshock4_get_firmware_info(ds4);
>>          if (ret) {
>>                  hid_err(hdev, "Failed to get firmware info from DualShock4\n");
>> -               return ERR_PTR(ret);
>> +               hid_err(hdev, "HW/FW version data in sysfs will be invalid.\n");
>>          }
> 
> This looks good. Perhaps could have been a hid_warn then, but err is
> probably fine.

I didn't think about it and kept the hid_err that was already there :)

I agree, and maybe we can take this further: If this code block is not a fatal error anymore, maybe it makes sense to change *both* lines to hid_warn?

If you prefer this, then I'll update the other patches accordingly.


Max

[PATCH v1 4/7] HID: playstation: DS4: Don't fail on calibration data request

[Max Staudt]

Some third-party controllers can't report calibration data for the
gyro/accelerometer.

We can still use the gamepad as-is, so let's do that.

Signed-off-by: Max Staudt <max@enpas.org>
---
 drivers/hid/hid-playstation.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 12321cae4416..2bf44bd3cc8a 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1778,8 +1778,10 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 		int retries;
 
 		buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
+		if (!buf) {
+			ret = -ENOMEM;
+			goto no_buffer_tail_check;
+		}
 
 		/* We should normally receive the feature report data we asked
 		 * for, but hidraw applications such as Steam can issue feature
@@ -1798,24 +1800,25 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 
 				hid_err(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
 				ret = -EILSEQ;
-				goto err_free;
 			} else {
 				break;
 			}
 		}
 	} else { /* Bluetooth */
 		buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
+		if (!buf) {
+			ret = -ENOMEM;
+			goto no_buffer_tail_check;
+		}
 
 		ret = ps_get_report(hdev, DS4_FEATURE_REPORT_CALIBRATION_BT, buf,
 				DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, true);
-		if (ret) {
+
+		if (ret)
 			hid_err(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
-			goto err_free;
-		}
 	}
 
+	/* Parse buffer. If the transfer failed, this safely copies zeroes. */
 	gyro_pitch_bias  = get_unaligned_le16(&buf[1]);
 	gyro_yaw_bias    = get_unaligned_le16(&buf[3]);
 	gyro_roll_bias   = get_unaligned_le16(&buf[5]);
@@ -1867,6 +1870,11 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	ds4->gyro_calib_data[2].sens_denom = abs(gyro_roll_plus - gyro_roll_bias) +
 			abs(gyro_roll_minus - gyro_roll_bias);
 
+	/* Done parsing the buffer, so let's free it. */
+	kfree(buf);
+
+no_buffer_tail_check:
+
 	/*
 	 * Sanity check gyro calibration data. This is needed to prevent crashes
 	 * during report handling of virtual, clone or broken devices not implementing
@@ -1919,8 +1927,6 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 		}
 	}
 
-err_free:
-	kfree(buf);
 	return ret;
 }
 
@@ -2571,7 +2577,7 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
 	ret = dualshock4_get_calibration_data(ds4);
 	if (ret) {
 		hid_err(hdev, "Failed to get calibration data from DualShock4\n");
-		goto err;
+		hid_err(hdev, "Gyroscope and accelerometer will be inaccurate.\n");
 	}
 
 	ds4->gamepad = ps_gamepad_create(hdev, dualshock4_play_effect);
-- 
2.39.2

[PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01

[Max Staudt]

Some third-party controllers never switch to the full 0x11 report.

They keep sending the short 0x01 report, so let's parse that instead.

Signed-off-by: Max Staudt <max@enpas.org>
---
 drivers/hid/hid-playstation.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 2bf44bd3cc8a..086b0768fa51 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -287,6 +287,8 @@ struct dualsense_output_report {
 
 #define DS4_INPUT_REPORT_USB			0x01
 #define DS4_INPUT_REPORT_USB_SIZE		64
+#define DS4_INPUT_REPORT_BT_MINIMAL		0x01
+#define DS4_INPUT_REPORT_BT_MINIMAL_SIZE	10
 #define DS4_INPUT_REPORT_BT			0x11
 #define DS4_INPUT_REPORT_BT_SIZE		78
 #define DS4_OUTPUT_REPORT_USB			0x05
@@ -2198,6 +2200,7 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
 	int battery_status, i, j;
 	uint16_t sensor_timestamp;
 	unsigned long flags;
+	bool is_minimal = false;
 
 	/*
 	 * DualShock4 in USB uses the full HID report for reportID 1, but
@@ -2225,6 +2228,18 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
 		ds4_report = &bt->common;
 		num_touch_reports = bt->num_touch_reports;
 		touch_reports = bt->touch_reports;
+	} else if (hdev->bus == BUS_BLUETOOTH &&
+		   report->id == DS4_INPUT_REPORT_BT_MINIMAL &&
+			 size == DS4_INPUT_REPORT_BT_MINIMAL_SIZE) {
+		/* Some third-party pads never switch to the full 0x11 report.
+		 * The short 0x01 report is 10 bytes long:
+		 *   u8 report_id == 0x01
+		 *   u8 first_bytes_of_full_report[9]
+		 * So let's reuse the full report parser, and stop it after
+		 * parsing the buttons.
+		 */
+		ds4_report = (struct dualshock4_input_report_common *)&data[1];
+		is_minimal = true;
 	} else {
 		hid_err(hdev, "Unhandled reportID=%d\n", report->id);
 		return -1;
@@ -2258,6 +2273,9 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
 	input_report_key(ds4->gamepad, BTN_MODE,   ds4_report->buttons[2] & DS_BUTTONS2_PS_HOME);
 	input_sync(ds4->gamepad);
 
+	if (is_minimal)
+		goto finish_minimal;
+
 	/* Parse and calibrate gyroscope data. */
 	for (i = 0; i < ARRAY_SIZE(ds4_report->gyro); i++) {
 		int raw_data = (short)le16_to_cpu(ds4_report->gyro[i]);
@@ -2365,6 +2383,7 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
 	ps_dev->battery_status = battery_status;
 	spin_unlock_irqrestore(&ps_dev->lock, flags);
 
+finish_minimal:
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01

[Roderick Colenbrander]

Hi Max,

On Mon, Jan 15, 2024 at 6:52 AM Max Staudt <max@enpas.org> wrote:
>
> Some third-party controllers never switch to the full 0x11 report.
>
> They keep sending the short 0x01 report, so let's parse that instead.
>
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
>  drivers/hid/hid-playstation.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 2bf44bd3cc8a..086b0768fa51 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -287,6 +287,8 @@ struct dualsense_output_report {
>
>  #define DS4_INPUT_REPORT_USB                   0x01
>  #define DS4_INPUT_REPORT_USB_SIZE              64
> +#define DS4_INPUT_REPORT_BT_MINIMAL            0x01
> +#define DS4_INPUT_REPORT_BT_MINIMAL_SIZE       10
>  #define DS4_INPUT_REPORT_BT                    0x11
>  #define DS4_INPUT_REPORT_BT_SIZE               78
>  #define DS4_OUTPUT_REPORT_USB                  0x05
> @@ -2198,6 +2200,7 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
>         int battery_status, i, j;
>         uint16_t sensor_timestamp;
>         unsigned long flags;
> +       bool is_minimal = false;
>
>         /*
>          * DualShock4 in USB uses the full HID report for reportID 1, but
> @@ -2225,6 +2228,18 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
>                 ds4_report = &bt->common;
>                 num_touch_reports = bt->num_touch_reports;
>                 touch_reports = bt->touch_reports;
> +       } else if (hdev->bus == BUS_BLUETOOTH &&
> +                  report->id == DS4_INPUT_REPORT_BT_MINIMAL &&
> +                        size == DS4_INPUT_REPORT_BT_MINIMAL_SIZE) {
> +               /* Some third-party pads never switch to the full 0x11 report.
> +                * The short 0x01 report is 10 bytes long:
> +                *   u8 report_id == 0x01
> +                *   u8 first_bytes_of_full_report[9]
> +                * So let's reuse the full report parser, and stop it after
> +                * parsing the buttons.
> +                */
> +               ds4_report = (struct dualshock4_input_report_common *)&data[1];
> +               is_minimal = true;
>         } else {
>                 hid_err(hdev, "Unhandled reportID=%d\n", report->id);
>                 return -1;
> @@ -2258,6 +2273,9 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
>         input_report_key(ds4->gamepad, BTN_MODE,   ds4_report->buttons[2] & DS_BUTTONS2_PS_HOME);
>         input_sync(ds4->gamepad);
>
> +       if (is_minimal)
> +               goto finish_minimal;
> +

I would say let's turn this into a 'return 0'. The goto is not useful
since there is no need for any common cleanup or some other common
logic later.

>         /* Parse and calibrate gyroscope data. */
>         for (i = 0; i < ARRAY_SIZE(ds4_report->gyro); i++) {
>                 int raw_data = (short)le16_to_cpu(ds4_report->gyro[i]);
> @@ -2365,6 +2383,7 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
>         ps_dev->battery_status = battery_status;
>         spin_unlock_irqrestore(&ps_dev->lock, flags);
>
> +finish_minimal:
>         return 0;
>  }
>
> --
> 2.39.2
>
>

Thanks,
Roderick

Re: [PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01

[Max Staudt]

On 1/25/24 09:54, Roderick Colenbrander wrote:
> I would say let's turn this into a 'return 0'. The goto is not useful
> since there is no need for any common cleanup or some other common
> logic later.

Oops, that one slipped through the cracks.

Thank you, will do!


Max

[PATCH v1 6/7] HID: playstation: Simplify device type ID

[Max Staudt]

Distinguish PS4/PS5 type controllers using .driver_data in
MODULE_DEVICE_TABLE rather than by VID/PID.

This allows adding compatible controllers with different VID/PID.

Signed-off-by: Max Staudt <max@enpas.org>
---
 drivers/hid/hid-playstation.c | 40 +++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 086b0768fa51..a0eb36d695d9 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -27,6 +27,11 @@ static DEFINE_IDA(ps_player_id_allocator);
 
 #define HID_PLAYSTATION_VERSION_PATCH 0x8000
 
+enum PS_TYPE {
+	PS_TYPE_PS4_DUALSHOCK4,
+	PS_TYPE_PS5_DUALSENSE,
+};
+
 /* Base class for playstation devices. */
 struct ps_device {
 	struct list_head list;
@@ -2690,17 +2695,14 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_stop;
 	}
 
-	if (hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER ||
-		hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_2 ||
-		hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE) {
+	if (id->driver_data == PS_TYPE_PS4_DUALSHOCK4) {
 		dev = dualshock4_create(hdev);
 		if (IS_ERR(dev)) {
 			hid_err(hdev, "Failed to create dualshock4.\n");
 			ret = PTR_ERR(dev);
 			goto err_close;
 		}
-	} else if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER ||
-		hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER_2) {
+	} else if (id->driver_data == PS_TYPE_PS5_DUALSENSE) {
 		dev = dualsense_create(hdev);
 		if (IS_ERR(dev)) {
 			hid_err(hdev, "Failed to create dualsense.\n");
@@ -2734,16 +2736,26 @@ static void ps_remove(struct hid_device *hdev)
 
 static const struct hid_device_id ps_devices[] = {
 	/* Sony DualShock 4 controllers for PS4 */
-	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
-	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
+		.driver_data = PS_TYPE_PS4_DUALSHOCK4 },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
+		.driver_data = PS_TYPE_PS4_DUALSHOCK4 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2),
+		.driver_data = PS_TYPE_PS4_DUALSHOCK4 },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2),
+		.driver_data = PS_TYPE_PS4_DUALSHOCK4 },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE),
+		.driver_data = PS_TYPE_PS4_DUALSHOCK4 },
+
 	/* Sony DualSense controllers for PS5 */
-	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
-	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
+		.driver_data = PS_TYPE_PS5_DUALSENSE },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
+		.driver_data = PS_TYPE_PS5_DUALSENSE },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2),
+		.driver_data = PS_TYPE_PS5_DUALSENSE },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2),
+		.driver_data = PS_TYPE_PS5_DUALSENSE },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, ps_devices);
-- 
2.39.2

Re: [PATCH v1 6/7] HID: playstation: Simplify device type ID

[Roderick Colenbrander]

On Mon, Jan 15, 2024 at 6:55 AM Max Staudt <max@enpas.org> wrote:
>
> Distinguish PS4/PS5 type controllers using .driver_data in
> MODULE_DEVICE_TABLE rather than by VID/PID.
>
> This allows adding compatible controllers with different VID/PID.
>
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
>  drivers/hid/hid-playstation.c | 40 +++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 086b0768fa51..a0eb36d695d9 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -27,6 +27,11 @@ static DEFINE_IDA(ps_player_id_allocator);
>
>  #define HID_PLAYSTATION_VERSION_PATCH 0x8000
>
> +enum PS_TYPE {
> +       PS_TYPE_PS4_DUALSHOCK4,
> +       PS_TYPE_PS5_DUALSENSE,
> +};
> +
>  /* Base class for playstation devices. */
>  struct ps_device {
>         struct list_head list;
> @@ -2690,17 +2695,14 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 goto err_stop;
>         }
>
> -       if (hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER ||
> -               hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_2 ||
> -               hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE) {
> +       if (id->driver_data == PS_TYPE_PS4_DUALSHOCK4) {
>                 dev = dualshock4_create(hdev);
>                 if (IS_ERR(dev)) {
>                         hid_err(hdev, "Failed to create dualshock4.\n");
>                         ret = PTR_ERR(dev);
>                         goto err_close;
>                 }
> -       } else if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER ||
> -               hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER_2) {
> +       } else if (id->driver_data == PS_TYPE_PS5_DUALSENSE) {
>                 dev = dualsense_create(hdev);
>                 if (IS_ERR(dev)) {
>                         hid_err(hdev, "Failed to create dualsense.\n");
> @@ -2734,16 +2736,26 @@ static void ps_remove(struct hid_device *hdev)
>
>  static const struct hid_device_id ps_devices[] = {
>         /* Sony DualShock 4 controllers for PS4 */
> -       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
> -       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
> +               .driver_data = PS_TYPE_PS4_DUALSHOCK4 },
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
> +               .driver_data = PS_TYPE_PS4_DUALSHOCK4 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2),
> +               .driver_data = PS_TYPE_PS4_DUALSHOCK4 },
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2),
> +               .driver_data = PS_TYPE_PS4_DUALSHOCK4 },
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE),
> +               .driver_data = PS_TYPE_PS4_DUALSHOCK4 },
> +
>         /* Sony DualSense controllers for PS5 */
> -       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
> -       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
> +               .driver_data = PS_TYPE_PS5_DUALSENSE },
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
> +               .driver_data = PS_TYPE_PS5_DUALSENSE },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2),
> +               .driver_data = PS_TYPE_PS5_DUALSENSE },
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER_2),
> +               .driver_data = PS_TYPE_PS5_DUALSENSE },
>         { }
>  };
>  MODULE_DEVICE_TABLE(hid, ps_devices);
> --
> 2.39.2
>
>

Hi Max,

This one looks good to me.

Roderick

[PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers

[Max Staudt]

It seems like this USB VID is not officially assigned, so let's create a
hid-ids.h entry without a vendor or product name.

Signed-off-by: Max Staudt <max@enpas.org>
---
 drivers/hid/hid-ids.h         | 3 +++
 drivers/hid/hid-playstation.c | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 72046039d1be..df831ab464a4 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -22,6 +22,9 @@
 #define USB_DEVICE_ID_3M2256		0x0502
 #define USB_DEVICE_ID_3M3266		0x0506
 
+#define USB_VENDOR_ID_7545		0x7545
+#define USB_DEVICE_ID_7545_0104		0x0104
+
 #define USB_VENDOR_ID_A4TECH		0x09da
 #define USB_DEVICE_ID_A4TECH_WCP32PU	0x0006
 #define USB_DEVICE_ID_A4TECH_X5_005D	0x000a
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index a0eb36d695d9..0aa474f1e96f 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2747,6 +2747,10 @@ static const struct hid_device_id ps_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE),
 		.driver_data = PS_TYPE_PS4_DUALSHOCK4 },
 
+	/* Third-party controllers identifying as "SZ-MYPOWER" */
+	{ HID_USB_DEVICE(USB_VENDOR_ID_7545, USB_DEVICE_ID_7545_0104),
+		.driver_data = PS_TYPE_PS4_DUALSHOCK4 },
+
 	/* Sony DualSense controllers for PS5 */
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
 		.driver_data = PS_TYPE_PS5_DUALSENSE },
-- 
2.39.2

Re: [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers

[Roderick Colenbrander]

On Mon, Jan 15, 2024 at 6:52 AM Max Staudt <max@enpas.org> wrote:
>
> It seems like this USB VID is not officially assigned, so let's create a
> hid-ids.h entry without a vendor or product name.
>
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
>  drivers/hid/hid-ids.h         | 3 +++
>  drivers/hid/hid-playstation.c | 4 ++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 72046039d1be..df831ab464a4 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -22,6 +22,9 @@
>  #define USB_DEVICE_ID_3M2256           0x0502
>  #define USB_DEVICE_ID_3M3266           0x0506
>
> +#define USB_VENDOR_ID_7545             0x7545
> +#define USB_DEVICE_ID_7545_0104                0x0104
> +
>  #define USB_VENDOR_ID_A4TECH           0x09da
>  #define USB_DEVICE_ID_A4TECH_WCP32PU   0x0006
>  #define USB_DEVICE_ID_A4TECH_X5_005D   0x000a
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index a0eb36d695d9..0aa474f1e96f 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -2747,6 +2747,10 @@ static const struct hid_device_id ps_devices[] = {
>         { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE),
>                 .driver_data = PS_TYPE_PS4_DUALSHOCK4 },
>
> +       /* Third-party controllers identifying as "SZ-MYPOWER" */
> +       { HID_USB_DEVICE(USB_VENDOR_ID_7545, USB_DEVICE_ID_7545_0104),
> +               .driver_data = PS_TYPE_PS4_DUALSHOCK4 },
> +
>         /* Sony DualSense controllers for PS5 */
>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
>                 .driver_data = PS_TYPE_PS5_DUALSENSE },
> --
> 2.39.2
>
>

I'm not familiar with this device, but if it indeed works. Then I'm
okay with it.

Thanks,
Roderick

Re: [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers

[Max Staudt]

On 1/25/24 10:03, Roderick Colenbrander wrote:
> I'm not familiar with this device, but if it indeed works. Then I'm
> okay with it.

Thanks!


I've just tried this patch on real hardware again, and there's a tradeoff here - it improves the situation for one 7545:0104 controller, and worsens it for another.

Up to you, and if you don't want to think about it, then let's shelve this patch :)



Details follow, if you're curious.


I have two controllers with VID/PID 7545:0104, and they're both very quirky multi-emulation devices. One is shaped like a PS4 controller, the other like a hybrid between a PS4 and a Switch controller. Since these controllers exhibit all of the USB related quirks in this series, I've kept them as reproducers. Other controllers that passed through my hands only had a subset of the quirks.

Up until now, both controllers worked with hid-sony as PS3 controllers. With this patch, the PS4 controller gains LED support and fine-grained control of the weak rumble motor. The "Switch (?) controller" on the other hand errors out, becomes 0079:181c, and loses the Home key and the accelerometer. This is a user facing change, and the question is how much we really care about these controllers.



More details, if you're still reading:


Both are "multi-purpose" controllers, appearing as PS4/PS3/Switch/other controllers in sequence. They advertise themselves as one USB device, and if there is no driver sending whatever init sequence they expect, they disconnect and try emulating a different controller.

The PS4 controller has rumble and an RGB LED, and this patch series improves its functionality. It cannot emulate a Switch controller.

The Switch (?) controller has no rumble and four multicolour player LEDs, but it adds Switch compatibility including accelerometer and gyro.


For the PS4 mode, which is the first that they try, and which would unify most functions, they use 7545:0104 instead of cloning a DS4 VID/PID. So I took a guess and found that it works fine with hid-playstation if I add the VID/PID and the init quirks in patches 2/3/4. Well, to be precise, I've only made the DS4 shaped one work in PS4 mode, the Switch controller isn't happy and errors out, see below.


On the PS4 controller, this makes the RGB LED work, rumble works, but the gyro and touchpad don't send HID updates. The touchpad can click though, so maybe the controller I have has a hardware defect.

The Switch (?) controller is where things get weird. It disconnects, even though it is initialised by hid-playstation, and transitions into a generic controller with VID/PID 0079:181c. This mode is *not* on the list of emulations it usually tries. It's as if the "unfinished" PS4 initialisation transitions it into a hidden fifth emulation mode. In this mode, the home key does not send any HID event, and there are no accelerometer updates that hid-sony would receive in PS3 mode.


So, with this patch, the PS4 controller works better on Linux, while the Switch controller works worse. Both were seen as PS3 controllers up until now. I see no way to discern them at driver probe time.

Any preference on what to do...?



Max

Re: [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers

[Roderick Colenbrander]

On Sat, Jan 27, 2024 at 3:16 AM Max Staudt <max@enpas.org> wrote:
>
> On 1/25/24 10:03, Roderick Colenbrander wrote:
> > I'm not familiar with this device, but if it indeed works. Then I'm
> > okay with it.
>
> Thanks!
>
>
> I've just tried this patch on real hardware again, and there's a tradeoff here - it improves the situation for one 7545:0104 controller, and worsens it for another.
>
> Up to you, and if you don't want to think about it, then let's shelve this patch :)
>
>
>
> Details follow, if you're curious.
>
>
> I have two controllers with VID/PID 7545:0104, and they're both very quirky multi-emulation devices. One is shaped like a PS4 controller, the other like a hybrid between a PS4 and a Switch controller. Since these controllers exhibit all of the USB related quirks in this series, I've kept them as reproducers. Other controllers that passed through my hands only had a subset of the quirks.
>
> Up until now, both controllers worked with hid-sony as PS3 controllers. With this patch, the PS4 controller gains LED support and fine-grained control of the weak rumble motor. The "Switch (?) controller" on the other hand errors out, becomes 0079:181c, and loses the Home key and the accelerometer. This is a user facing change, and the question is how much we really care about these controllers.
>
>
>
> More details, if you're still reading:
>
>
> Both are "multi-purpose" controllers, appearing as PS4/PS3/Switch/other controllers in sequence. They advertise themselves as one USB device, and if there is no driver sending whatever init sequence they expect, they disconnect and try emulating a different controller.
>
> The PS4 controller has rumble and an RGB LED, and this patch series improves its functionality. It cannot emulate a Switch controller.
>
> The Switch (?) controller has no rumble and four multicolour player LEDs, but it adds Switch compatibility including accelerometer and gyro.
>
>
> For the PS4 mode, which is the first that they try, and which would unify most functions, they use 7545:0104 instead of cloning a DS4 VID/PID. So I took a guess and found that it works fine with hid-playstation if I add the VID/PID and the init quirks in patches 2/3/4. Well, to be precise, I've only made the DS4 shaped one work in PS4 mode, the Switch controller isn't happy and errors out, see below.
>
>
> On the PS4 controller, this makes the RGB LED work, rumble works, but the gyro and touchpad don't send HID updates. The touchpad can click though, so maybe the controller I have has a hardware defect.
>
> The Switch (?) controller is where things get weird. It disconnects, even though it is initialised by hid-playstation, and transitions into a generic controller with VID/PID 0079:181c. This mode is *not* on the list of emulations it usually tries. It's as if the "unfinished" PS4 initialisation transitions it into a hidden fifth emulation mode. In this mode, the home key does not send any HID event, and there are no accelerometer updates that hid-sony would receive in PS3 mode.
>
>
> So, with this patch, the PS4 controller works better on Linux, while the Switch controller works worse. Both were seen as PS3 controllers up until now. I see no way to discern them at driver probe time.
>
> Any preference on what to do...?
>
>
>
> Max
>

Hmpf, euhm euhm I'm not entirely sure what makes sense. From the
sounds of it are somewhat broken devices (buggy firmwares on them) or
perhaps one of your controllers (the one with not working touch) is
perhaps broken.

Some of the patches like handling report id 0x1 (minimal) won't hurt,
the LED fixes won't either. It makes it easier to add more devices. I
wonder if we have fully have enough data.

Need to think a bit...

Roderick

Re: [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers

[Max Staudt]

On 1/31/24 06:07, Roderick Colenbrander wrote:
> Hmpf, euhm euhm I'm not entirely sure what makes sense. From the
> sounds of it are somewhat broken devices (buggy firmwares on them) or
> perhaps one of your controllers (the one with not working touch) is
> perhaps broken.
> 
> Some of the patches like handling report id 0x1 (minimal) won't hurt,
> the LED fixes won't either. It makes it easier to add more devices. I
> wonder if we have fully have enough data.
> 
> Need to think a bit...

Yeah, maybe for now, let's focus on the patches that you don't see much trouble with. Better to enable some devices, than none.

I suggest dropping this here patch (enabling 7545:0104) for now, and if there's ever a change of mind, we can just pick it from the LKML archives :)


You can also drop the other patch that you're uneasy about, since I believe these 7545:0104 devices are the only ones I've observed to not implement the what-is-your-MAC-address command.



Max

Re: [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers

[Roderick Colenbrander]

On Wed, Jan 31, 2024 at 6:48 AM Max Staudt <max@enpas.org> wrote:
>
> On 1/31/24 06:07, Roderick Colenbrander wrote:
> > Hmpf, euhm euhm I'm not entirely sure what makes sense. From the
> > sounds of it are somewhat broken devices (buggy firmwares on them) or
> > perhaps one of your controllers (the one with not working touch) is
> > perhaps broken.
> >
> > Some of the patches like handling report id 0x1 (minimal) won't hurt,
> > the LED fixes won't either. It makes it easier to add more devices. I
> > wonder if we have fully have enough data.
> >
> > Need to think a bit...
>
> Yeah, maybe for now, let's focus on the patches that you don't see much trouble with. Better to enable some devices, than none.
>
> I suggest dropping this here patch (enabling 7545:0104) for now, and if there's ever a change of mind, we can just pick it from the LKML archives :)
>
>
> You can also drop the other patch that you're uneasy about, since I believe these 7545:0104 devices are the only ones I've observed to not implement the what-is-your-MAC-address command.
>
>
>
> Max
>

I agree it sounds like a good idea for now to drop just those 2 paches
and just see what other devices are out there (SDL2 has a good ds4
implementation too and they dealt with a lot of devices). May need to
get some of the 8bitdo and many others to see some patterns.

Thanks,
Roderick

Re: [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers

[Max Staudt]

On 2/1/24 01:34, Roderick Colenbrander wrote:
> I agree it sounds like a good idea for now to drop just those 2 paches
> and just see what other devices are out there (SDL2 has a good ds4
> implementation too and they dealt with a lot of devices). May need to
> get some of the 8bitdo and many others to see some patterns.

8BitDo seem to be interesting indeed. IIRC they need the patches for the firmware version, the gyro calibration, and 0x01 events.


Max

Re: [PATCH v1 0/7] HID: playstation: DS4: LED bugfix, third-party gamepad support

[Jiri Kosina]

On Mon, 15 Jan 2024, Max Staudt wrote:

> Dear hid-playstation maintainers,
> 
> Could you please have a look at the enclosed patches for the DualShock 4
> driver in hid-playstation, and upstream them if possible?
> 
> There is one bugfix, and a few small patches to enable third-party
> controllers. They sometimes don't implement features that they
> semantically "don't need", but which currently trip the driver.
> 
> For example, for the DualShock 4, we don't actually need to know the
> firmware version in order to work with the gamepad - unlike with the
> DualSense, which has different driver logic depending on the version.
> 
> Finally, there are two patches to add a DS4 compatible controller with
> an unassigned VID/PID - I'd appreciate your thoughts on that.
> 
> If I can make it easier to upstream these patches, please let me know.
> 
> Thanks!
> 
> Max
> 
> Patches in this series:
>   [PATCH v1 1/7] HID: playstation: DS4: Fix LED blinking
>   [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address
>   [PATCH v1 3/7] HID: playstation: DS4: Don't fail on FW/HW version
>   [PATCH v1 4/7] HID: playstation: DS4: Don't fail on calibration data
>   [PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01
>   [PATCH v1 6/7] HID: playstation: Simplify device type ID
>   [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER

Roderick, any word on this series, please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v1 0/7] HID: playstation: DS4: LED bugfix, third-party gamepad support

[Roderick Colenbrander]

On Tue, Jan 23, 2024 at 1:51 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Mon, 15 Jan 2024, Max Staudt wrote:
>
> > Dear hid-playstation maintainers,
> >
> > Could you please have a look at the enclosed patches for the DualShock 4
> > driver in hid-playstation, and upstream them if possible?
> >
> > There is one bugfix, and a few small patches to enable third-party
> > controllers. They sometimes don't implement features that they
> > semantically "don't need", but which currently trip the driver.
> >
> > For example, for the DualShock 4, we don't actually need to know the
> > firmware version in order to work with the gamepad - unlike with the
> > DualSense, which has different driver logic depending on the version.
> >
> > Finally, there are two patches to add a DS4 compatible controller with
> > an unassigned VID/PID - I'd appreciate your thoughts on that.
> >
> > If I can make it easier to upstream these patches, please let me know.
> >
> > Thanks!
> >
> > Max
> >
> > Patches in this series:
> >   [PATCH v1 1/7] HID: playstation: DS4: Fix LED blinking
> >   [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address
> >   [PATCH v1 3/7] HID: playstation: DS4: Don't fail on FW/HW version
> >   [PATCH v1 4/7] HID: playstation: DS4: Don't fail on calibration data
> >   [PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01
> >   [PATCH v1 6/7] HID: playstation: Simplify device type ID
> >   [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER
>
> Roderick, any word on this series, please?
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
>

Sorry for the late reply. I had glanced over them, but didn't have an
opportunity for a detailed review yet.

I will have some input (there was a goto I remember not being needed).
My general fear is a balance between supporting clone devices vs
reliability. This driver is heavily used in devices (phones, tablets,
TVs, cars). There have been bug reports in the past and just getting
the fixes downstream takes a lot of time (e.g. Android devices).

One of the key things I really would like to see enhanced are the unit
tests (hid-tools / kernel side now). To really make sure we emulate
behavior of these other devices well. The tricky part is that they
don't always support all the HID requests of the real device (which is
weird as the game console does use those HID reports and others and I
don't know how it would have worked there).

That's in general the key feedback about the tests. A question for
Max: do you have access to all the devices being added?

Thanks,
Roderick

Re: [PATCH v1 0/7] HID: playstation: DS4: LED bugfix, third-party gamepad support

[Max Staudt]

On 1/25/24 07:24, Roderick Colenbrander wrote:
> Sorry for the late reply. I had glanced over them, but didn't have an
> opportunity for a detailed review yet.

No worries, thank you for giving the patches a chance.


> My general fear is a balance between supporting clone devices vs
> reliability. This driver is heavily used in devices (phones, tablets,
> TVs, cars). There have been bug reports in the past and just getting
> the fixes downstream takes a lot of time (e.g. Android devices).

I understand this very well, which is why I hope I've kept the patches small enough to be alright to follow. If you have a bad feeling about something in particular, please let me know, and maybe we can find a better solution or alleviate concerns.

On my end, I am working (currently as a hobby) on an appliance that aims to support as many controllers as possible, hence I know the choices you face in order to keep user expectations in check, and user experience consistent. See my comment on patch 7/7 - what if someone uses a third-party controller that happens to work by chance, and a kernel update breaks stuff?

So I am with you on focusing on the original devices, which have predictable behaviour and quality. That said, hopefully some or all of these patches are trivial enough to be included upstream. I feel that it's beautiful to plug random stuff into a Linux box and to find that it just works :)


> One of the key things I really would like to see enhanced are the unit
> tests (hid-tools / kernel side now). To really make sure we emulate
> behavior of these other devices well. The tricky part is that they
> don't always support all the HID requests of the real device (which is
> weird as the game console does use those HID reports and others and I
> don't know how it would have worked there).

I've been wondering the same, but without a PS4 of my own, I didn't try such controllers on a real console. That would be interesting indeed!


> That's in general the key feedback about the tests. A question for
> Max: do you have access to all the devices being added?

I don't have all devices I've ever tested, but I do have at least one reproducer for each patch in this series.



Thanks,

Max