Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20230924140927.9844-2-tinozzo123@gmail.com>
In-Reply-To: <20230924140927.9844-2-tinozzo123@gmail.com>
From:   Daniel Ogorchock <djogorchock@gmail.com>
Date:   Sat, 30 Sep 2023 15:55:11 -0400
Message-ID: <CAEVj2tnbFrdUPmNWWb2ePoh-6-OTqLmv1n6OcmJeUgBrY6w1FA@mail.gmail.com>
Subject: Re: [PATCH v3] HID: nintendo: reinitialize USB Pro Controller after
 resuming from suspend
To:     Martino Fontana <tinozzo123@gmail.com>
Cc:     jikos@kernel.org, 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
Precedence: bulk

Hi Martino,

On Sun, Sep 24, 2023 at 10:13=E2=80=AFAM Martino Fontana <tinozzo123@gmail.=
com> wrote:
>
> When suspending the computer, a Switch Pro Controller connected via USB w=
ill
> lose its internal status. However, because the USB connection was technic=
ally
> never lost, when resuming the computer, the driver will attempt to commun=
icate
> with the controller as if nothing happened (and fail).
> Because of this, the user was forced to manually disconnect the controlle=
r
> (or to press the sync button on the controller to power it off), so that =
it
> can be re-initialized.
>
> With this patch, the controller will be automatically re-initialized afte=
r
> resuming from suspend.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=3D216233
>
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
>
> ---
> Changes for v2 and v3: Applied suggestions
>
>  drivers/hid/hid-nintendo.c | 175 ++++++++++++++++++++++---------------
>  1 file changed, 103 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 250f5d2f8..10468f727 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctl=
r)
>         struct joycon_input_report *report;
>
>         req.subcmd_id =3D JC_SUBCMD_REQ_DEV_INFO;
> +       mutex_lock(&ctlr->output_mutex);
>         ret =3D joycon_send_subcmd(ctlr, &req, 0, HZ);
> +       mutex_unlock(&ctlr->output_mutex);
>         if (ret) {
>                 hid_err(ctlr->hdev, "Failed to get joycon info; ret=3D%d\=
n", ret);
>                 return ret;
> @@ -2117,6 +2119,85 @@ static int joycon_read_info(struct joycon_ctlr *ct=
lr)
>         return 0;
>  }
>
> +static int joycon_init(struct hid_device *hdev)
> +{
> +       struct joycon_ctlr *ctlr =3D hid_get_drvdata(hdev);
> +       int ret =3D 0;
> +
> +       mutex_lock(&ctlr->output_mutex);
> +       /* if handshake command fails, assume ble pro controller */
> +       if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> +           !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> +               hid_dbg(hdev, "detected USB controller\n");
> +               /* set baudrate for improved latency */
> +               ret =3D joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ)=
;
> +               if (ret) {
> +                       hid_err(hdev, "Failed to set baudrate; ret=3D%d\n=
", ret);
> +                       goto out_unlock;
> +               }
> +               /* handshake */
> +               ret =3D joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> +               if (ret) {
> +                       hid_err(hdev, "Failed handshake; ret=3D%d\n", ret=
);
> +                       goto out_unlock;
> +               }
> +               /*
> +                * Set no timeout (to keep controller in USB mode).
> +                * This doesn't send a response, so ignore the timeout.
> +                */
> +               joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> +       } else if (jc_type_is_chrggrip(ctlr)) {
> +               hid_err(hdev, "Failed charging grip handshake\n");
> +               ret =3D -ETIMEDOUT;
> +               goto out_unlock;
> +       }
> +
> +       /* get controller calibration data, and parse it */
> +       ret =3D joycon_request_calibration(ctlr);
> +       if (ret) {
> +               /*
> +                * We can function with default calibration, but it may b=
e
> +                * inaccurate. Provide a warning, and continue on.
> +                */
> +               hid_warn(hdev, "Analog stick positions may be inaccurate\=
n");
> +       }
> +
> +       /* get IMU calibration data, and parse it */
> +       ret =3D joycon_request_imu_calibration(ctlr);
> +       if (ret) {
> +               /*
> +                * We can function with default calibration, but it may b=
e
> +                * inaccurate. Provide a warning, and continue on.
> +                */
> +               hid_warn(hdev, "Unable to read IMU calibration data\n");
> +       }
> +
> +       /* Set the reporting mode to 0x30, which is the full report mode =
*/
> +       ret =3D joycon_set_report_mode(ctlr);
> +       if (ret) {
> +               hid_err(hdev, "Failed to set report mode; ret=3D%d\n", re=
t);
> +               goto out_unlock;
> +       }
> +
> +       /* Enable rumble */
> +       ret =3D joycon_enable_rumble(ctlr);
> +       if (ret) {
> +               hid_err(hdev, "Failed to enable rumble; ret=3D%d\n", ret)=
;
> +               goto out_unlock;
> +       }
> +
> +       /* Enable the IMU */
> +       ret =3D joycon_enable_imu(ctlr);
> +       if (ret) {
> +               hid_err(hdev, "Failed to enable the IMU; ret=3D%d\n", ret=
);
> +               goto out_unlock;
> +       }
> +
> +out_unlock:
> +       mutex_unlock(&ctlr->output_mutex);
> +       return ret;
> +}
> +
>  /* Common handler for parsing inputs */
>  static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
>                                                               int size)
> @@ -2248,85 +2329,19 @@ static int nintendo_hid_probe(struct hid_device *=
hdev,
>
>         hid_device_io_start(hdev);
>
> -       /* Initialize the controller */
> -       mutex_lock(&ctlr->output_mutex);
> -       /* if handshake command fails, assume ble pro controller */
> -       if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> -           !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> -               hid_dbg(hdev, "detected USB controller\n");
> -               /* set baudrate for improved latency */
> -               ret =3D joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ)=
;
> -               if (ret) {
> -                       hid_err(hdev, "Failed to set baudrate; ret=3D%d\n=
", ret);
> -                       goto err_mutex;
> -               }
> -               /* handshake */
> -               ret =3D joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> -               if (ret) {
> -                       hid_err(hdev, "Failed handshake; ret=3D%d\n", ret=
);
> -                       goto err_mutex;
> -               }
> -               /*
> -                * Set no timeout (to keep controller in USB mode).
> -                * This doesn't send a response, so ignore the timeout.
> -                */
> -               joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> -       } else if (jc_type_is_chrggrip(ctlr)) {
> -               hid_err(hdev, "Failed charging grip handshake\n");
> -               ret =3D -ETIMEDOUT;
> -               goto err_mutex;
> -       }
> -
> -       /* get controller calibration data, and parse it */
> -       ret =3D joycon_request_calibration(ctlr);
> +       ret =3D joycon_init(hdev);
>         if (ret) {
> -               /*
> -                * We can function with default calibration, but it may b=
e
> -                * inaccurate. Provide a warning, and continue on.
> -                */
> -               hid_warn(hdev, "Analog stick positions may be inaccurate\=
n");
> -       }
> -
> -       /* get IMU calibration data, and parse it */
> -       ret =3D joycon_request_imu_calibration(ctlr);
> -       if (ret) {
> -               /*
> -                * We can function with default calibration, but it may b=
e
> -                * inaccurate. Provide a warning, and continue on.
> -                */
> -               hid_warn(hdev, "Unable to read IMU calibration data\n");
> -       }
> -
> -       /* Set the reporting mode to 0x30, which is the full report mode =
*/
> -       ret =3D joycon_set_report_mode(ctlr);
> -       if (ret) {
> -               hid_err(hdev, "Failed to set report mode; ret=3D%d\n", re=
t);
> -               goto err_mutex;
> -       }
> -
> -       /* Enable rumble */
> -       ret =3D joycon_enable_rumble(ctlr);
> -       if (ret) {
> -               hid_err(hdev, "Failed to enable rumble; ret=3D%d\n", ret)=
;
> -               goto err_mutex;
> -       }
> -
> -       /* Enable the IMU */
> -       ret =3D joycon_enable_imu(ctlr);
> -       if (ret) {
> -               hid_err(hdev, "Failed to enable the IMU; ret=3D%d\n", ret=
);
> -               goto err_mutex;
> +               hid_err(hdev, "Failed to initialize controller; ret=3D%d\=
n", ret);
> +               goto err_close;
>         }
>
>         ret =3D joycon_read_info(ctlr);
>         if (ret) {
>                 hid_err(hdev, "Failed to retrieve controller info; ret=3D=
%d\n",
>                         ret);
> -               goto err_mutex;
> +               goto err_close;
>         }
>
> -       mutex_unlock(&ctlr->output_mutex);
> -
>         /* Initialize the leds */
>         ret =3D joycon_leds_create(ctlr);
>         if (ret) {
> @@ -2352,8 +2367,6 @@ static int nintendo_hid_probe(struct hid_device *hd=
ev,
>         hid_dbg(hdev, "probe - success\n");
>         return 0;
>
> -err_mutex:
> -       mutex_unlock(&ctlr->output_mutex);
>  err_close:
>         hid_hw_close(hdev);
>  err_stop:
> @@ -2383,6 +2396,20 @@ static void nintendo_hid_remove(struct hid_device =
*hdev)
>         hid_hw_stop(hdev);
>  }
>
> +#ifdef CONFIG_PM
> +
> +static int nintendo_hid_resume(struct hid_device *hdev)
> +{
> +       int ret =3D joycon_init(hdev);
> +
> +       if (ret)
> +               hid_err(hdev, "Failed to restore controller after resume"=
);
> +
> +       return ret;
> +}
> +
> +#endif
> +
>  static const struct hid_device_id nintendo_hid_devices[] =3D {
>         { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
>                          USB_DEVICE_ID_NINTENDO_PROCON) },
> @@ -2404,6 +2431,10 @@ static struct hid_driver nintendo_hid_driver =3D {
>         .probe          =3D nintendo_hid_probe,
>         .remove         =3D nintendo_hid_remove,
>         .raw_event      =3D nintendo_hid_event,
> +
> +#ifdef CONFIG_PM
> +       .resume         =3D nintendo_hid_resume,
> +#endif
>  };
>  module_hid_driver(nintendo_hid_driver);
>
> --
> 2.42.0
>

Thanks for adding the resume hook for usb controllers. Looks good to me.

Reviewed-by: Daniel J. Ogorchock <djogorchock@gmail.com>