diff mbox series

[1/3] Bluetooth: btusb: Refactor gpio reset

Message ID 20210119124258.1.Ide97dd8adadf50af66738852c3c68861734cca1c@changeid (mailing list archive)
State New, archived
Headers show
Series Bluetooth: btusb: Expose hw reset to debugfs | expand

Commit Message

Abhishek Pandit-Subedi Jan. 19, 2021, 8:43 p.m. UTC 
Refactor gpio reset to use a common gpio reset function.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 drivers/bluetooth/btusb.c | 59 +++++++++++++--------------------------
 1 file changed, 19 insertions(+), 40 deletions(-)

Comments

bluez.test.bot@gmail.com Jan. 19, 2021, 9:42 p.m. UTC | #1 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=417689

---Test result---

##############################
    Test: CheckPatch - PASS
    

    ##############################
    Test: CheckGitLint - FAIL
    workflow: Add workflow files for ci
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing

Bluetooth: btusb: Refactor gpio reset
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing

Bluetooth: btusb: Trigger gpio reset quicker
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing

Bluetooth: btusb: Expose reset gpio to debugfs
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing


    ##############################
    Test: CheckBuildK - PASS
    

    ##############################
    Test: CheckTestRunner: Setup - PASS
    

    ##############################
    Test: CheckTestRunner: l2cap-tester - PASS
    Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

    ##############################
    Test: CheckTestRunner: bnep-tester - PASS
    Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: mgmt-tester - PASS
    Total: 416, Passed: 402 (96.6%), Failed: 0, Not Run: 14

    ##############################
    Test: CheckTestRunner: rfcomm-tester - PASS
    Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: sco-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: smp-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: userchan-tester - PASS
    Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0

    

---
Regards,
Linux Bluetooth
Marcel Holtmann Jan. 25, 2021, 3:19 p.m. UTC | #2 
Hi Abhishek,

> Refactor gpio reset to use a common gpio reset function.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> drivers/bluetooth/btusb.c | 59 +++++++++++++--------------------------
> 1 file changed, 19 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b14102fba6018..03341e6cbf3ed 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -547,6 +547,7 @@ struct btusb_data {
> 	struct usb_endpoint_descriptor *diag_rx_ep;
> 
> 	struct gpio_desc *reset_gpio;
> +	unsigned int reset_duration_ms;
> 
> 	__u8 cmdreq_type;
> 	__u8 cmdreq;
> @@ -566,15 +567,21 @@ struct btusb_data {
> 	unsigned cmd_timeout_cnt;
> };
> 
> -static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
> +static void btusb_toggle_gpio(struct gpio_desc *desc, unsigned int duration)
> +{
> +	gpiod_set_value_cansleep(desc, 1);
> +	msleep(duration);
> +	gpiod_set_value_cansleep(desc, 0);
> +}
> +
> +static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
> {
> 	struct btusb_data *data = hci_get_drvdata(hdev);
> -	struct gpio_desc *reset_gpio = data->reset_gpio;
> 
> 	if (++data->cmd_timeout_cnt < 5)
> 		return;
> 
> -	if (!reset_gpio) {
> +	if (!data->reset_gpio) {
> 		bt_dev_err(hdev, "No way to reset. Ignoring and continuing");
> 		return;
> 	}
> @@ -592,39 +599,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
> 	}
> 
> 	bt_dev_err(hdev, "Initiating HW reset via gpio");
> -	gpiod_set_value_cansleep(reset_gpio, 1);
> -	msleep(100);
> -	gpiod_set_value_cansleep(reset_gpio, 0);
> -}
> -
> -static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
> -{
> -	struct btusb_data *data = hci_get_drvdata(hdev);
> -	struct gpio_desc *reset_gpio = data->reset_gpio;
> -
> -	if (++data->cmd_timeout_cnt < 5)
> -		return;
> -
> -	if (!reset_gpio) {
> -		bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring");
> -		return;
> -	}
> -
> -	/* Toggle the hard reset line. The Realtek device is going to
> -	 * yank itself off the USB and then replug. The cleanup is handled
> -	 * correctly on the way out (standard USB disconnect), and the new
> -	 * device is detected cleanly and bound to the driver again like
> -	 * it should be.
> -	 */
> -	if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
> -		bt_dev_err(hdev, "last reset failed? Not resetting again");
> -		return;
> -	}
> -
> -	bt_dev_err(hdev, "Reset Realtek device via gpio");
> -	gpiod_set_value_cansleep(reset_gpio, 1);
> -	msleep(200);
> -	gpiod_set_value_cansleep(reset_gpio, 0);
> +	btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
> }

You need to explain why this patch is correct. You are removing more code here. And there is an extra check in case of Realtek and a large comment.

Regards

Marcel
diff mbox series

Patch

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index b14102fba6018..03341e6cbf3ed 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -547,6 +547,7 @@  struct btusb_data {
 	struct usb_endpoint_descriptor *diag_rx_ep;
 
 	struct gpio_desc *reset_gpio;
+	unsigned int reset_duration_ms;
 
 	__u8 cmdreq_type;
 	__u8 cmdreq;
@@ -566,15 +567,21 @@  struct btusb_data {
 	unsigned cmd_timeout_cnt;
 };
 
-static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
+static void btusb_toggle_gpio(struct gpio_desc *desc, unsigned int duration)
+{
+	gpiod_set_value_cansleep(desc, 1);
+	msleep(duration);
+	gpiod_set_value_cansleep(desc, 0);
+}
+
+static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
-	struct gpio_desc *reset_gpio = data->reset_gpio;
 
 	if (++data->cmd_timeout_cnt < 5)
 		return;
 
-	if (!reset_gpio) {
+	if (!data->reset_gpio) {
 		bt_dev_err(hdev, "No way to reset. Ignoring and continuing");
 		return;
 	}
@@ -592,39 +599,7 @@  static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
 	}
 
 	bt_dev_err(hdev, "Initiating HW reset via gpio");
-	gpiod_set_value_cansleep(reset_gpio, 1);
-	msleep(100);
-	gpiod_set_value_cansleep(reset_gpio, 0);
-}
-
-static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
-{
-	struct btusb_data *data = hci_get_drvdata(hdev);
-	struct gpio_desc *reset_gpio = data->reset_gpio;
-
-	if (++data->cmd_timeout_cnt < 5)
-		return;
-
-	if (!reset_gpio) {
-		bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring");
-		return;
-	}
-
-	/* Toggle the hard reset line. The Realtek device is going to
-	 * yank itself off the USB and then replug. The cleanup is handled
-	 * correctly on the way out (standard USB disconnect), and the new
-	 * device is detected cleanly and bound to the driver again like
-	 * it should be.
-	 */
-	if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
-		bt_dev_err(hdev, "last reset failed? Not resetting again");
-		return;
-	}
-
-	bt_dev_err(hdev, "Reset Realtek device via gpio");
-	gpiod_set_value_cansleep(reset_gpio, 1);
-	msleep(200);
-	gpiod_set_value_cansleep(reset_gpio, 0);
+	btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
 }
 
 static void btusb_qca_cmd_timeout(struct hci_dev *hdev)
@@ -4462,7 +4437,8 @@  static int btusb_probe(struct usb_interface *intf,
 		hdev->shutdown = btusb_shutdown_intel;
 		hdev->set_diag = btintel_set_diag_mfg;
 		hdev->set_bdaddr = btintel_set_bdaddr;
-		hdev->cmd_timeout = btusb_intel_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 100;
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -4476,7 +4452,8 @@  static int btusb_probe(struct usb_interface *intf,
 		hdev->hw_error = btintel_hw_error;
 		hdev->set_diag = btintel_set_diag;
 		hdev->set_bdaddr = btintel_set_bdaddr;
-		hdev->cmd_timeout = btusb_intel_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 100;
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -4490,7 +4467,8 @@  static int btusb_probe(struct usb_interface *intf,
 		hdev->hw_error = btintel_hw_error;
 		hdev->set_diag = btintel_set_diag;
 		hdev->set_bdaddr = btintel_set_bdaddr;
-		hdev->cmd_timeout = btusb_intel_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 100;
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -4557,7 +4535,8 @@  static int btusb_probe(struct usb_interface *intf,
 	    (id->driver_info & BTUSB_REALTEK)) {
 		hdev->setup = btrtl_setup_realtek;
 		hdev->shutdown = btrtl_shutdown_realtek;
-		hdev->cmd_timeout = btusb_rtl_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 200;
 
 		/* Realtek devices lose their updated firmware over global
 		 * suspend that means host doesn't send SET_FEATURE