Received-SPF: pass (google.com: domain of linux-kernel+bounces-37838-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240115144538.12018-1-max@enpas.org> <20240115144538.12018-6-max@enpas.org>
In-Reply-To: <20240115144538.12018-6-max@enpas.org>
From: Roderick Colenbrander <thunderbird2k@gmail.com>
Date: Wed, 24 Jan 2024 16:54:24 -0800
Message-ID: <CAEc3jaCYWpXX=YgeNzXJd0Pg4yn4WKqL=GBSAzaWNzo=3RSw1A@mail.gmail.com>
Subject: Re: [PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01
To: Max Staudt <max@enpas.org>
Cc: Roderick Colenbrander <roderick.colenbrander@sony.com>, Jiri Kosina <jikos@kernel.org>, 
	Benjamin Tissoires <benjamin.tissoires@redhat.com>, linux-input@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Max,

On Mon, Jan 15, 2024 at 6:52=E2=80=AFAM 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 =3D false;
>
>         /*
>          * DualShock4 in USB uses the full HID report for reportID 1, but
> @@ -2225,6 +2228,18 @@ static int dualshock4_parse_report(struct ps_devic=
e *ps_dev, struct hid_report *
>                 ds4_report =3D &bt->common;
>                 num_touch_reports =3D bt->num_touch_reports;
>                 touch_reports =3D bt->touch_reports;
> +       } else if (hdev->bus =3D=3D BUS_BLUETOOTH &&
> +                  report->id =3D=3D DS4_INPUT_REPORT_BT_MINIMAL &&
> +                        size =3D=3D DS4_INPUT_REPORT_BT_MINIMAL_SIZE) {
> +               /* Some third-party pads never switch to the full 0x11 re=
port.
> +                * The short 0x01 report is 10 bytes long:
> +                *   u8 report_id =3D=3D 0x01
> +                *   u8 first_bytes_of_full_report[9]
> +                * So let's reuse the full report parser, and stop it aft=
er
> +                * parsing the buttons.
> +                */
> +               ds4_report =3D (struct dualshock4_input_report_common *)&=
data[1];
> +               is_minimal =3D true;
>         } else {
>                 hid_err(hdev, "Unhandled reportID=3D%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 =3D 0; i < ARRAY_SIZE(ds4_report->gyro); i++) {
>                 int raw_data =3D (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 =3D battery_status;
>         spin_unlock_irqrestore(&ps_dev->lock, flags);
>
> +finish_minimal:
>         return 0;
>  }
>
> --
> 2.39.2
>
>

Thanks,
Roderick