Re: [PATCH v4 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>,
	"Samuel Čavoj" <sammko@sammserver.com>,
	"Florian Märkl" <linux@florianmaerkl.de>
Cc: Jiri Kosina <jikos@kernel.org>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	Chris Ye <lzye@google.com>,
	Roderick Colenbrander <roderick.colenbrander@sony.com>
Subject: Re: [PATCH v4 00/13] HID: new driver for PS5 'DualSense' controller
Date: Thu, 28 Jan 2021 09:31:43 +0100	[thread overview]
Message-ID: <CAO-hwJJ5r0hBNEhKvZkLevyG8mf6rQVL_7nf4XcjUi0mgErF5w@mail.gmail.com> (raw)
In-Reply-To: <20210117234435.180294-1-roderick@gaikai.com>

Hi Roderick,

On Mon, Jan 18, 2021 at 12:44 AM Roderick Colenbrander
<roderick@gaikai.com> wrote:
>
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Hi,
>
> This is the same code as v3. Due to a misstake during a last minute
> rebase, the touchpad and sensors patch got combined while fixing a conflict.
> The new v4 corrects that issue. There are no additional code changes.
>
> This new revision contains a few bug fixes, but mostly features small
> code changes and minor improvements relative to v2.
>
> In terms of bugs there were bugs in the sensor code. There was an overflow
> issue and EV_MSC/MSC_TIMESTAMP were not set on the device. In addition,
> the ps_device spinlock was not initialized.
>
> The biggest change in the driver was the addition of a new 'ps_get_report'
> helper function. It handles GET_FEATURE report retrieval and any error handling
> including CRC checks for PlayStation Bluetooth devices. This greatly simplified
> all the functions (dualsense_get_mac_address, dualsense_calibration_info, ..)
> dealing, which used their own report handling and error checking.
>
> Aside for these changes, there were mostly little code style changes like defining
> magic constants, cleaning up comments, cleaning up log messages, static_assert
> checks etcetera.
>
> Thanks to everyone who provided feedback through the mailing list or privately.

From a rough review, the code looks good to me. However, I'd like to
have Baranabás reviewed-by tag at least given all the work he has been
doing. There were other people involved in the various versions, and
it would be nice if we can get some credits for them too.

So for anyone involved in the discussions, could you give us your
reviewed-by or tested-by if you feel like?

[Roderick, as a general rule of thumb, it's better IMO to keep Cc-ed
the people who gave feedback, so they are notified of a new version.]

Cheers,
Benjamin

>
> Changes since v3:
> - Separated touchpad and sensors into separate patches due to rebase misstake.
>
> Changes since v2:
> - Removed !Expert setting for hid-playstation from Kconfig.
> - Removed DualSense from hid-quirks table.
> - Added report size checks to dualsense_parse_report.
> - Moved mac address endianess comment to struct ps_device.
> - Added static_asserts for packed structure size checks.
> - Improved readability of battery capacity calculation using 'min'.
> - Added spin_lock_init to dualsense_create to initialize ps_device lock.
> - Fixed sensors timestamp overflow.
> - Fixed missing MSC_TIMESTAMP and EV_MSC capabilities in ps_sensors_create.
> - Used DIV_ROUND_CLOSEST for timestamp calculations to minimize rounding errors.
> - Switched to devm_kmalloc_array for lightbar allocation.
> - Added CRC32 and NEW_LEDS dependency to Kconfig.
> - Added defines for crc32 seed constants.
> - Added crc32 check for dualsense_get_mac_address and increased report size to 20.
> - Added new ps_get_report call to obtain feature reports.
> - Switched to ARRAY_SIZE in dualsense_parse_reports for touch points, accel and gyro data.
> - Changed touch point parse loop to use "struct dualsense_touch_point".
> - Improved consistency of info and error messages.
> - Unified comment style.
>
>
> 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 | 1485 +++++++++++++++++++++++++++++++++
>  5 files changed, 1514 insertions(+)
>  create mode 100644 drivers/hid/hid-playstation.c
>
> --
> 2.26.2
>