[v5,00/13] HID: new driver for PS5 'DualSense' controller

Message

Roderick Colenbrander Jan. 28, 2021, 5:26 p.m. UTC

From: Roderick Colenbrander <[e-mail]>

Hi,

This is hopefully the final revision of this patch series. Patch v4 had
a rebase issue of a part of the sensors patch for which a part had moved
to the end of the series. This has been fixed. I have double, no triple
checked the patches. Made sure they build using a 'rebase -x' script
and also ran the hid-tools tests on the final driver.

Thanks to everyone who provided feedback through the mailing list or privately.
As suggested by Benjamin on the 'v4' version of this email, if you were
involed in the review or testing of this series and would like some credit,
please provide a reviewed-by or tested-by tag.

Changes since v4:
- Fixed bad rebase of ps_sensors_create, moved it to appropriate patch.

Thanks,

Roderick Colenbrander
Sony Interactive Entertainment, LLC

Roderick Colenbrander (13):
  HID: playstation: initial DualSense USB support.
  HID: playstation: use DualSense MAC address as unique identifier.
  HID: playstation: add DualSense battery support.
  HID: playstation: add DualSense touchpad support.
  HID: playstation: add DualSense accelerometer and gyroscope support.
  HID: playstation: track devices in list.
  HID: playstation: add DualSense Bluetooth support.
  HID: playstation: add DualSense classic rumble support.
  HID: playstation: add DualSense lightbar support
  HID: playstation: add microphone mute support for DualSense.
  HID: playstation: add DualSense player LEDs support.
  HID: playstation: DualSense set LEDs to default player id.
  HID: playstation: report DualSense hardware and firmware version.

 MAINTAINERS                   |    6 +
 drivers/hid/Kconfig           |   21 +
 drivers/hid/Makefile          |    1 +
 drivers/hid/hid-ids.h         |    1 +
 drivers/hid/hid-playstation.c | 1492 +++++++++++++++++++++++++++++++++
 5 files changed, 1521 insertions(+)
 create mode 100644 drivers/hid/hid-playstation.c

Comments

Benjamin Tissoires Feb. 5, 2021, 5:01 p.m. UTC | #1

Hi,

On Thu, Jan 28, 2021 at 6:27 PM Roderick Colenbrander
<[e-mail]> wrote:
>
> From: Roderick Colenbrander <[e-mail]>
>
> Hi,
>
> This is hopefully the final revision of this patch series. Patch v4 had
> a rebase issue of a part of the sensors patch for which a part had moved
> to the end of the series. This has been fixed. I have double, no triple
> checked the patches. Made sure they build using a 'rebase -x' script
> and also ran the hid-tools tests on the final driver.
>
> Thanks to everyone who provided feedback through the mailing list or privately.
> As suggested by Benjamin on the 'v4' version of this email, if you were
> involed in the review or testing of this series and would like some credit,
> please provide a reviewed-by or tested-by tag.
>
> Changes since v4:
> - Fixed bad rebase of ps_sensors_create, moved it to appropriate patch.

Barnabás, any comments on this version?

As soon as I get your rev-by, we can apply the series, just in time for 5.12.

Roderick, I do see a few checkpath errors that could be fixed, but I
won't hole the series against:
HID: playstation: add DualSense battery support. -> WARNING: Missing a
blank line after declarations
HID: playstation: report DualSense hardware and firmware version. ->
WARNING: Consider renaming function(s) 'ps_show_firmware_version' to
'firmware_version_show' (and same for ps_show_hardware_version)

Also, there is a weird sparse error:
+drivers/hid/hid-playstation.c:xxx:1:.error: static assertion failed:
"sizeof(struct dualsense_input_report) == DS_INPUT_REPORT_USB_SIZE -
1"
+drivers/hid/hid-playstation.c:xxx:1:.error: static assertion failed:
"sizeof(struct dualsense_output_report_bt) ==
DS_OUTPUT_REPORT_BT_SIZE"

It's weird because it only fails while running sparse, when the normal
compilation is just fine, and the assert is correctly evaluated.

Anyway, the series is good from my Point of View, but I'd like to get
the reviewers some credits.

Cheers,
Benjamin

>
> Thanks,
>
> Roderick Colenbrander
> Sony Interactive Entertainment, LLC
>
> Roderick Colenbrander (13):
>   HID: playstation: initial DualSense USB support.
>   HID: playstation: use DualSense MAC address as unique identifier.
>   HID: playstation: add DualSense battery support.
>   HID: playstation: add DualSense touchpad support.
>   HID: playstation: add DualSense accelerometer and gyroscope support.
>   HID: playstation: track devices in list.
>   HID: playstation: add DualSense Bluetooth support.
>   HID: playstation: add DualSense classic rumble support.
>   HID: playstation: add DualSense lightbar support
>   HID: playstation: add microphone mute support for DualSense.
>   HID: playstation: add DualSense player LEDs support.
>   HID: playstation: DualSense set LEDs to default player id.
>   HID: playstation: report DualSense hardware and firmware version.
>
>  MAINTAINERS                   |    6 +
>  drivers/hid/Kconfig           |   21 +
>  drivers/hid/Makefile          |    1 +
>  drivers/hid/hid-ids.h         |    1 +
>  drivers/hid/hid-playstation.c | 1492 +++++++++++++++++++++++++++++++++
>  5 files changed, 1521 insertions(+)
>  create mode 100644 drivers/hid/hid-playstation.c
>
> --
> 2.26.2
>

Barnabás Pőcze Feb. 6, 2021, 4:03 a.m. UTC | #2

Hi


2021. február 5., péntek 18:01 keltezéssel, Benjamin Tissoires írta:

> Hi,
>
> On Thu, Jan 28, 2021 at 6:27 PM Roderick Colenbrander
> [e-mail] wrote:
>
> > From: Roderick Colenbrander <[e-mail]>
> > Hi,
> > This is hopefully the final revision of this patch series. Patch v4 had
> > a rebase issue of a part of the sensors patch for which a part had moved
> > to the end of the series. This has been fixed. I have double, no triple
> > checked the patches. Made sure they build using a 'rebase -x' script
> > and also ran the hid-tools tests on the final driver.
> > Thanks to everyone who provided feedback through the mailing list or privately.
> > As suggested by Benjamin on the 'v4' version of this email, if you were
> > involed in the review or testing of this series and would like some credit,
> > please provide a reviewed-by or tested-by tag.
> > Changes since v4:
> >
> > -   Fixed bad rebase of ps_sensors_create, moved it to appropriate patch.
>
> Barnabás, any comments on this version?
>
> As soon as I get your rev-by, we can apply the series, just in time for 5.12.
>

Sorry for not responding earlier, I have been relatively busy lately. I have taken
another look at the final source file. I have a couple comments for Roderick:

 - `player_ids` array should be `static const` as far as I can see;
 - there are a couple devm_kasprintf() calls which are not checked;
 - power_supply_powers() call is not checked - I think either a comment
   should mention that it's not considered a fatal error, or checked

There are also other more minor things, formatting inconsistencies, but I
cannot see anything else, so with the aforementioned things fixed, if you want:

Reviewed-by: Barnabás Pőcze <[e-mail]>


Regards,
Barnabás Pőcze


> Roderick, I do see a few checkpath errors that could be fixed, but I
> won't hole the series against:
> HID: playstation: add DualSense battery support. -> WARNING: Missing a
> blank line after declarations
> HID: playstation: report DualSense hardware and firmware version. ->
> WARNING: Consider renaming function(s) 'ps_show_firmware_version' to
> 'firmware_version_show' (and same for ps_show_hardware_version)
>
> Also, there is a weird sparse error:
> +drivers/hid/hid-playstation.c:xxx:1:.error: static assertion failed:
> "sizeof(struct dualsense_input_report) == DS_INPUT_REPORT_USB_SIZE -
> 1"
> +drivers/hid/hid-playstation.c:xxx:1:.error: static assertion failed:
> "sizeof(struct dualsense_output_report_bt) ==
> DS_OUTPUT_REPORT_BT_SIZE"
>
> It's weird because it only fails while running sparse, when the normal
> compilation is just fine, and the assert is correctly evaluated.
>
> Anyway, the series is good from my Point of View, but I'd like to get
> the reviewers some credits.
>
> Cheers,
> Benjamin
>
> > Thanks,
> > Roderick Colenbrander
> > Sony Interactive Entertainment, LLC
> > Roderick Colenbrander (13):
> > HID: playstation: initial DualSense USB support.
> > HID: playstation: use DualSense MAC address as unique identifier.
> > HID: playstation: add DualSense battery support.
> > HID: playstation: add DualSense touchpad support.
> > HID: playstation: add DualSense accelerometer and gyroscope support.
> > HID: playstation: track devices in list.
> > HID: playstation: add DualSense Bluetooth support.
> > HID: playstation: add DualSense classic rumble support.
> > HID: playstation: add DualSense lightbar support
> > HID: playstation: add microphone mute support for DualSense.
> > HID: playstation: add DualSense player LEDs support.
> > HID: playstation: DualSense set LEDs to default player id.
> > HID: playstation: report DualSense hardware and firmware version.
> > MAINTAINERS | 6 +
> > drivers/hid/Kconfig | 21 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-ids.h | 1 +
> > drivers/hid/hid-playstation.c | 1492 +++++++++++++++++++++++++++++++++
> > 5 files changed, 1521 insertions(+)
> > create mode 100644 drivers/hid/hid-playstation.c
> > --
> > 2.26.2

Roderick Colenbrander Feb. 7, 2021, 6:05 a.m. UTC | #3

Hi Barnabás and Benjamin,

On Fri, Feb 5, 2021 at 8:03 PM Barnabás Pőcze <[e-mail]> wrote:
>
> Hi
>
>
> 2021. február 5., péntek 18:01 keltezéssel, Benjamin Tissoires írta:
>
> > Hi,
> >
> > On Thu, Jan 28, 2021 at 6:27 PM Roderick Colenbrander
> > [e-mail] wrote:
> >
> > > From: Roderick Colenbrander <[e-mail]>
> > > Hi,
> > > This is hopefully the final revision of this patch series. Patch v4 had
> > > a rebase issue of a part of the sensors patch for which a part had moved
> > > to the end of the series. This has been fixed. I have double, no triple
> > > checked the patches. Made sure they build using a 'rebase -x' script
> > > and also ran the hid-tools tests on the final driver.
> > > Thanks to everyone who provided feedback through the mailing list or privately.
> > > As suggested by Benjamin on the 'v4' version of this email, if you were
> > > involed in the review or testing of this series and would like some credit,
> > > please provide a reviewed-by or tested-by tag.
> > > Changes since v4:
> > >
> > > -   Fixed bad rebase of ps_sensors_create, moved it to appropriate patch.
> >
> > Barnabás, any comments on this version?
> >
> > As soon as I get your rev-by, we can apply the series, just in time for 5.12.
> >
>
> Sorry for not responding earlier, I have been relatively busy lately. I have taken
> another look at the final source file. I have a couple comments for Roderick:
>
>  - `player_ids` array should be `static const` as far as I can see;
>  - there are a couple devm_kasprintf() calls which are not checked;
>  - power_supply_powers() call is not checked - I think either a comment
>    should mention that it's not considered a fatal error, or checked
>
> There are also other more minor things, formatting inconsistencies, but I
> cannot see anything else, so with the aforementioned things fixed, if you want:
>
> Reviewed-by: Barnabás Pőcze <[e-mail]>
>

Thanks for the additional feedback. I fixed a few additional checkpath
warnings and also included these other fixes. I will send out a v6
series tomorrow morning when I'm fresh :)

Thanks,
Roderick