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

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: "Roderick Colenbrander" <roderick@gaikai.com>,
	"Barnabás Pőcze" <pobrn@protonmail.com>
Cc: "Jiri Kosina" <jikos@kernel.org>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	"Chris Ye" <lzye@google.com>,
	"Samuel Čavoj" <sammko@sammserver.com>,
	"Florian Märkl" <linux@florianmaerkl.de>,
	"Roderick Colenbrander" <roderick.colenbrander@sony.com>
Subject: Re: [PATCH v5 00/13] HID: new driver for PS5 'DualSense' controller
Date: Fri, 5 Feb 2021 18:01:56 +0100	[thread overview]
Message-ID: <CAO-hwJLi+twcmSFkbPHHJ23CpF5P=EORsVauyhwvGt9dGoq++A@mail.gmail.com> (raw)
In-Reply-To: <20210128172657.24516-1-roderick@gaikai.com>

Hi,

On Thu, Jan 28, 2021 at 6:27 PM Roderick Colenbrander
<roderick@gaikai.com> wrote:
>
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> 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
>