[PATCH v1] Bluetooth: hci_qca: Make bt_en and susclk not mandatory for QCA Rome

[PATCH v1] Bluetooth: hci_qca: Make bt_en and susclk not mandatory for QCA Rome

[Rocky Liao]

On some platforms the bt_en pin and susclk are default on and there
is no exposed resource to control them. This patch makes the bt_en
and susclk not mandatory to have BT work. It also will not set the
HCI_QUIRK_NON_PERSISTENT_SETUP and shutdown() callback if bt_en is
not available.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index bf436d6e638e..325baa046c3a 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1562,9 +1562,11 @@ static int qca_power_on(struct hci_dev *hdev)
 		ret = qca_wcn3990_init(hu);
 	} else {
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 1);
-		/* Controller needs time to bootup. */
-		msleep(150);
+		if (!IS_ERR(qcadev->bt_en)) {
+			gpiod_set_value_cansleep(qcadev->bt_en, 1);
+			/* Controller needs time to bootup. */
+			msleep(150);
+		}
 	}
 
 	return ret;
@@ -1750,7 +1752,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
 		host_set_baudrate(hu, 2400);
 		qca_send_power_pulse(hu, false);
 		qca_regulator_disable(qcadev);
-	} else {
+	} else if (!IS_ERR(qcadev->bt_en)) {
 		gpiod_set_value_cansleep(qcadev->bt_en, 0);
 	}
 }
@@ -1852,6 +1854,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	struct hci_dev *hdev;
 	const struct qca_vreg_data *data;
 	int err;
+	bool power_ctrl_enabled = true;
 
 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
 	if (!qcadev)
@@ -1901,35 +1904,37 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en)) {
-			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
-			return PTR_ERR(qcadev->bt_en);
+			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
+			power_ctrl_enabled = false;
 		}
 
 		qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {
-			dev_err(&serdev->dev, "failed to acquire clk\n");
-			return PTR_ERR(qcadev->susclk);
-		}
-
-		err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
-		if (err)
-			return err;
+			dev_warn(&serdev->dev, "failed to acquire clk\n");
+		} else {
+			err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
+			if (err)
+				return err;
 
-		err = clk_prepare_enable(qcadev->susclk);
-		if (err)
-			return err;
+			err = clk_prepare_enable(qcadev->susclk);
+			if (err)
+				return err;
+		}
 
 		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
 		if (err) {
 			BT_ERR("Rome serdev registration failed");
-			clk_disable_unprepare(qcadev->susclk);
+			if (!IS_ERR(qcadev->susclk))
+				clk_disable_unprepare(qcadev->susclk);
 			return err;
 		}
 	}
 
-	hdev = qcadev->serdev_hu.hdev;
-	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
-	hdev->shutdown = qca_power_off;
+	if (power_ctrl_enabled) {
+		hdev = qcadev->serdev_hu.hdev;
+		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+		hdev->shutdown = qca_power_off;
+	}
 
 	return 0;
 }
@@ -1940,7 +1945,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
 
 	if (qca_is_wcn399x(qcadev->btsoc_type))
 		qca_power_shutdown(&qcadev->serdev_hu);
-	else
+	else if (!IS_ERR(qcadev->susclk))
 		clk_disable_unprepare(qcadev->susclk);
 
 	hci_uart_unregister_device(&qcadev->serdev_hu);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH v1] Bluetooth: hci_qca: Make bt_en and susclk not mandatory for QCA Rome

[Marcel Holtmann]

Hi Rocky,

> On some platforms the bt_en pin and susclk are default on and there
> is no exposed resource to control them. This patch makes the bt_en
> and susclk not mandatory to have BT work. It also will not set the
> HCI_QUIRK_NON_PERSISTENT_SETUP and shutdown() callback if bt_en is
> not available.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 21 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH v1] Bluetooth: hci_qca: Make bt_en and susclk not mandatory for QCA Rome

[Johan Hovold]

On Wed, Mar 04, 2020 at 09:54:29AM +0800, Rocky Liao wrote:
> On some platforms the bt_en pin and susclk are default on and there
> is no exposed resource to control them. This patch makes the bt_en
> and susclk not mandatory to have BT work. It also will not set the
> HCI_QUIRK_NON_PERSISTENT_SETUP and shutdown() callback if bt_en is
> not available.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
>  drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index bf436d6e638e..325baa046c3a 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1562,9 +1562,11 @@ static int qca_power_on(struct hci_dev *hdev)
>  		ret = qca_wcn3990_init(hu);
>  	} else {
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
> -		gpiod_set_value_cansleep(qcadev->bt_en, 1);
> -		/* Controller needs time to bootup. */
> -		msleep(150);
> +		if (!IS_ERR(qcadev->bt_en)) {
> +			gpiod_set_value_cansleep(qcadev->bt_en, 1);
> +			/* Controller needs time to bootup. */
> +			msleep(150);
> +		}
>  	}
>  
>  	return ret;
> @@ -1750,7 +1752,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  		host_set_baudrate(hu, 2400);
>  		qca_send_power_pulse(hu, false);
>  		qca_regulator_disable(qcadev);
> -	} else {
> +	} else if (!IS_ERR(qcadev->bt_en)) {
>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>  	}
>  }
> @@ -1852,6 +1854,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	struct hci_dev *hdev;
>  	const struct qca_vreg_data *data;
>  	int err;
> +	bool power_ctrl_enabled = true;
>  
>  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>  	if (!qcadev)
> @@ -1901,35 +1904,37 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);

Shouldn't you use devm_gpiod_get_optional()?

>  		if (IS_ERR(qcadev->bt_en)) {
> -			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> -			return PTR_ERR(qcadev->bt_en);
> +			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> +			power_ctrl_enabled = false;

And bail out on errors, but treat NULL as !port_ctrl_enabled?

>  		}
>  
>  		qcadev->susclk = devm_clk_get(&serdev->dev, NULL);

And devm_clk_get_optional() here.

Etc.

Also does the devicetree binding need to be updated to reflect this
change?

Johan

Re: [PATCH v1] Bluetooth: hci_qca: Make bt_en and susclk not mandatory for QCA Rome

[Rocky Liao]

在 2020-03-04 16:53，Johan Hovold 写道：
> On Wed, Mar 04, 2020 at 09:54:29AM +0800, Rocky Liao wrote:
>> On some platforms the bt_en pin and susclk are default on and there
>> is no exposed resource to control them. This patch makes the bt_en
>> and susclk not mandatory to have BT work. It also will not set the
>> HCI_QUIRK_NON_PERSISTENT_SETUP and shutdown() callback if bt_en is
>> not available.
>> 
>> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 47 
>> ++++++++++++++++++++-----------------
>>  1 file changed, 26 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index bf436d6e638e..325baa046c3a 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1562,9 +1562,11 @@ static int qca_power_on(struct hci_dev *hdev)
>>  		ret = qca_wcn3990_init(hu);
>>  	} else {
>>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>> -		gpiod_set_value_cansleep(qcadev->bt_en, 1);
>> -		/* Controller needs time to bootup. */
>> -		msleep(150);
>> +		if (!IS_ERR(qcadev->bt_en)) {
>> +			gpiod_set_value_cansleep(qcadev->bt_en, 1);
>> +			/* Controller needs time to bootup. */
>> +			msleep(150);
>> +		}
>>  	}
>> 
>>  	return ret;
>> @@ -1750,7 +1752,7 @@ static void qca_power_shutdown(struct hci_uart 
>> *hu)
>>  		host_set_baudrate(hu, 2400);
>>  		qca_send_power_pulse(hu, false);
>>  		qca_regulator_disable(qcadev);
>> -	} else {
>> +	} else if (!IS_ERR(qcadev->bt_en)) {
>>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>>  	}
>>  }
>> @@ -1852,6 +1854,7 @@ static int qca_serdev_probe(struct serdev_device 
>> *serdev)
>>  	struct hci_dev *hdev;
>>  	const struct qca_vreg_data *data;
>>  	int err;
>> +	bool power_ctrl_enabled = true;
>> 
>>  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>>  	if (!qcadev)
>> @@ -1901,35 +1904,37 @@ static int qca_serdev_probe(struct 
>> serdev_device *serdev)
>>  		qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
>>  					       GPIOD_OUT_LOW);
> 
> Shouldn't you use devm_gpiod_get_optional()?
> 
>>  		if (IS_ERR(qcadev->bt_en)) {
>> -			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>> -			return PTR_ERR(qcadev->bt_en);
>> +			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>> +			power_ctrl_enabled = false;
> 
> And bail out on errors, but treat NULL as !port_ctrl_enabled?
> 
>>  		}
>> 
>>  		qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
> 
> And devm_clk_get_optional() here.
> 
> Etc.
> 
> Also does the devicetree binding need to be updated to reflect this
> change?
> 
OK, I will update to use the suggested API.
The devicetress binding doesn't need to be updated as they are already 
optional there.

> Johan