[PATCH v5 0/3] Bluetooth: hci_qca: Add serdev support

[PATCH v5 0/3] Bluetooth: hci_qca: Add serdev support

[Thierry Escande]

Hi,

This patchset enables the Qualcomm BT controller QCA6174 node in the
device tree of the db820c board. This allows the bluetooth chipset to
be probed and registered against the hci layer by using the serdev
framework.

This patchset also contains the documentation for the compatible
string "qcom,qca6174-bt" related to this chipset.

v5:
- Rename 'bt-disable-n' gpio as 'enable'

v4:
- Fix dt binding documentation
- Address some other issues in patch #3

v3:
- Address comments for patch #3 (details in patch)

v2:
- Fix author email

Thierry Escande (3):
  arm64: dts: apq8096-db820c: enable bluetooth node
  dt-bindings: net: bluetooth: Add qualcomm-bluetooth
  Bluetooth: hci_qca: Add serdev support

 .../devicetree/bindings/net/qualcomm-bluetooth.txt |  34 +++++++
 arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi  |  14 +++
 .../boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi    |  17 ++++
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi       |  33 +++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi              |  10 ++
 drivers/bluetooth/Kconfig                          |   1 +
 drivers/bluetooth/hci_qca.c                        | 109 ++++++++++++++++++++-
 7 files changed, 216 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt

-- 
2.14.1

[PATCH v5 1/3] arm64: dts: apq8096-db820c: enable bluetooth node

[Thierry Escande]

Add a new serial node for the Qualcomm BT controller QCA6174. This
allows automatic probing and hci registration through the serdev
framework instead of relying on the userspace helpers.

Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
---

v5:
- Rename 'bt-disable-n' gpio as 'enable'

v4: no change
v3: no change

v2:
- Fix author email

 arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi  | 14 +++++++++
 .../boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi    | 17 +++++++++++
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi       | 33 ++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi              | 10 +++++++
 4 files changed, 74 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi
index 24552f19b3fa..172165d84669 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi
@@ -36,4 +36,18 @@
 			drive-strength = <2>;	/* 2 MA */
 		};
 	};
+
+	blsp1_uart1_default: blsp1_uart1_default {
+		function = "blsp_uart2";
+		pins = "gpio41", "gpio42", "gpio43", "gpio44";
+		drive-strength = <16>;
+		bias-disable;
+	};
+
+	blsp1_uart1_sleep: blsp1_uart1_sleep {
+		function = "gpio";
+		pins = "gpio41", "gpio42", "gpio43", "gpio44";
+		drive-strength = <2>;
+		bias-disable;
+	};
 };
diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi
index 59b29ddfb6e9..f8d2a3b10b1f 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi
@@ -26,6 +26,23 @@
 		};
 	};
 
+	divclk4_pin_a: divclk4 {
+		pins = "gpio18";
+		function = "func2";
+
+		bias-disable;
+		power-source = <PM8994_GPIO_S4>;
+	};
+
+	bt_en_pin_a: bt-en-active {
+		pins = "gpio19";
+		function = "normal";
+
+		output-low;
+		power-source = <PM8994_GPIO_S4>;
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
+	};
+
 	usb3_vbus_det_gpio: pm8996_gpio22 {
 		pinconf {
 			pins = "gpio22";
diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
index 1c8f1b86472d..8b3bcdf0d718 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
@@ -23,6 +23,7 @@
 	aliases {
 		serial0 = &blsp2_uart1;
 		serial1 = &blsp2_uart2;
+		serial2 = &blsp1_uart1;
 		i2c0	= &blsp1_i2c2;
 		i2c1	= &blsp2_i2c1;
 		i2c2	= &blsp2_i2c0;
@@ -34,7 +35,39 @@
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		divclk4: divclk4 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32768>;
+			clock-output-names = "divclk4";
+
+			pinctrl-names = "default";
+			pinctrl-0 = <&divclk4_pin_a>;
+		};
+	};
+
 	soc {
+		serial@7570000 {
+			label = "BT-UART";
+			status = "okay";
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&blsp1_uart1_default>;
+			pinctrl-1 = <&blsp1_uart1_sleep>;
+
+			bluetooth {
+				compatible = "qcom,qca6174-bt";
+
+				/* bt_disable_n gpio */
+				enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
+
+				pinctrl-names = "default";
+				pinctrl-0 = <&bt_en_pin_a>;
+
+				clocks = <&divclk4>;
+			};
+		};
+
 		serial@75b0000 {
 			label = "LS-UART1";
 			status = "okay";
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 0a6f7952bbb1..2d54a86a027f 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -408,6 +408,16 @@
 			#clock-cells = <1>;
 		};
 
+		blsp1_uart1: serial@7570000 {
+			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
+			reg = <0x07570000 0x1000>;
+			interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>,
+				 <&gcc GCC_BLSP1_AHB_CLK>;
+			clock-names = "core", "iface";
+			status = "disabled";
+		};
+
 		blsp1_spi0: spi@7575000 {
 			compatible = "qcom,spi-qup-v2.2.1";
 			reg = <0x07575000 0x600>;
-- 
2.14.1

[PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Thierry Escande]

Add binding document for serial bluetooth chips using Qualcomm protocol.

Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
---

v5:
- Rename 'bt-disable-n' gpio as 'enable'

v4:
- Move bt-disable-n-gpios to required properties section
- Add clocks and pinctrl-0 as required properties

v3: no change
v2: no change

 .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt

diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
new file mode 100644
index 000000000000..bbc2973634b2
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -0,0 +1,34 @@
+Qualcomm Bluetooth Chips
+---------------------
+
+This documents the binding structure and common properties for serial
+attached Qualcomm devices.
+
+Serial attached Qualcomm devices shall be a child node of the host UART
+device the slave device is attached to.
+
+Required properties:
+ - compatible: should contain one of the following:
+   * "qcom,qca6174-bt"
+ - enable-gpios: gpio specifier used to enable chip
+ - pinctrl-0: pin phandle for bt_en gpio
+ - clocks: clock phandle for SUSCLK_32KHZ
+
+Example:
+
+serial@7570000 {
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&blsp1_uart1_default>;
+	pinctrl-1 = <&blsp1_uart1_sleep>;
+
+	bluetooth {
+		compatible = "qcom,qca6174-bt";
+
+		enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_en_pin_a>;
+
+		clocks = <&divclk4>;
+	};
+};
-- 
2.14.1

[PATCH v5 3/3] Bluetooth: hci_qca: Add serdev support

[Thierry Escande]

Add support for Qualcomm serial slave devices. Probe the serial device,
retrieve its maximum speed and register a new hci uart device.

Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
---

v5:
- Use gpio new name 'enable'

v4:
- Rename divclk4 as susclk (its name in the bt chip)
- Use gpiod_set_value_cansleep()
- Replace #include <linux/of.h> with <linux/mod_devicetable.h>
- Restore dependency on BT_HCIUART

v3:
- Remove redundant call to gpiod_set_value() after devm_gpiod_get()
- Check returned values for clk_set_rate() and clk_prepare_enable()
- Use clk_disable_unprepare()

v2:
- Fix author email

 drivers/bluetooth/Kconfig   |   1 +
 drivers/bluetooth/hci_qca.c | 109 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 07e55cd8f8c8..e0f1a6609b68 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -196,6 +196,7 @@ config BT_HCIUART_BCM
 config BT_HCIUART_QCA
 	bool "Qualcomm Atheros protocol support"
 	depends on BT_HCIUART
+	depends on BT_HCIUART_SERDEV
 	select BT_HCIUART_H4
 	select BT_QCA
 	help
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 05ec530b8a3a..6e6042f77784 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -29,7 +29,11 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/clk.h>
 #include <linux/debugfs.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/serdev.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -50,6 +54,9 @@
 #define IBS_TX_IDLE_TIMEOUT_MS		2000
 #define BAUDRATE_SETTLE_TIMEOUT_MS	300
 
+/* susclk rate */
+#define SUSCLK_RATE_32KHZ	32768
+
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -111,6 +118,12 @@ struct qca_data {
 	u64 votes_off;
 };
 
+struct qca_serdev {
+	struct hci_uart	 serdev_hu;
+	struct gpio_desc *bt_en;
+	struct clk	 *susclk;
+};
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -386,6 +399,7 @@ static void hci_ibs_wake_retrans_timeout(struct timer_list *t)
 /* Initialize protocol */
 static int qca_open(struct hci_uart *hu)
 {
+	struct qca_serdev *qcadev;
 	struct qca_data *qca;
 
 	BT_DBG("hu %p qca_open", hu);
@@ -444,6 +458,13 @@ static int qca_open(struct hci_uart *hu)
 	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
 	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
 
+	if (hu->serdev) {
+		serdev_device_open(hu->serdev);
+
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+		gpiod_set_value_cansleep(qcadev->bt_en, 1);
+	}
+
 	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
 	       qca->tx_idle_delay, qca->wake_retrans);
 
@@ -512,6 +533,7 @@ static int qca_flush(struct hci_uart *hu)
 /* Close protocol */
 static int qca_close(struct hci_uart *hu)
 {
+	struct qca_serdev *qcadev;
 	struct qca_data *qca = hu->priv;
 
 	BT_DBG("hu %p qca close", hu);
@@ -525,6 +547,13 @@ static int qca_close(struct hci_uart *hu)
 	destroy_workqueue(qca->workqueue);
 	qca->hu = NULL;
 
+	if (hu->serdev) {
+		serdev_device_close(hu->serdev);
+
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+		gpiod_set_value_cansleep(qcadev->bt_en, 0);
+	}
+
 	kfree_skb(qca->rx_skb);
 
 	hu->priv = NULL;
@@ -885,6 +914,14 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	return 0;
 }
 
+static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
+{
+	if (hu->serdev)
+		serdev_device_set_baudrate(hu->serdev, speed);
+	else
+		hci_uart_set_baudrate(hu, speed);
+}
+
 static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -905,7 +942,7 @@ static int qca_setup(struct hci_uart *hu)
 		speed = hu->proto->init_speed;
 
 	if (speed)
-		hci_uart_set_baudrate(hu, speed);
+		host_set_baudrate(hu, speed);
 
 	/* Setup user speed if needed */
 	speed = 0;
@@ -924,7 +961,7 @@ static int qca_setup(struct hci_uart *hu)
 				   ret);
 			return ret;
 		}
-		hci_uart_set_baudrate(hu, speed);
+		host_set_baudrate(hu, speed);
 	}
 
 	/* Setup patch / NVM configurations */
@@ -958,12 +995,80 @@ static struct hci_uart_proto qca_proto = {
 	.dequeue	= qca_dequeue,
 };
 
+static int qca_serdev_probe(struct serdev_device *serdev)
+{
+	struct qca_serdev *qcadev;
+	int err;
+
+	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
+	if (!qcadev)
+		return -ENOMEM;
+
+	qcadev->serdev_hu.serdev = serdev;
+	serdev_device_set_drvdata(serdev, qcadev);
+
+	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 bt-disable-n gpio\n");
+		return PTR_ERR(qcadev->bt_en);
+	}
+
+	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;
+
+	err = clk_prepare_enable(qcadev->susclk);
+	if (err)
+		return err;
+
+	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+	if (err)
+		clk_disable_unprepare(qcadev->susclk);
+
+	return err;
+}
+
+static void qca_serdev_remove(struct serdev_device *serdev)
+{
+	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
+
+	hci_uart_unregister_device(&qcadev->serdev_hu);
+
+	clk_disable_unprepare(qcadev->susclk);
+}
+
+static const struct of_device_id qca_bluetooth_of_match[] = {
+	{ .compatible = "qcom,qca6174-bt" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
+
+static struct serdev_device_driver qca_serdev_driver = {
+	.probe = qca_serdev_probe,
+	.remove = qca_serdev_remove,
+	.driver = {
+		.name = "hci_uart_qca",
+		.of_match_table = qca_bluetooth_of_match,
+	},
+};
+
 int __init qca_init(void)
 {
+	serdev_device_driver_register(&qca_serdev_driver);
+
 	return hci_uart_register_proto(&qca_proto);
 }
 
 int __exit qca_deinit(void)
 {
+	serdev_device_driver_unregister(&qca_serdev_driver);
+
 	return hci_uart_unregister_proto(&qca_proto);
 }
-- 
2.14.1

Re: [PATCH v5 3/3] Bluetooth: hci_qca: Add serdev support

[Andy Shevchenko]

On Tue, 2018-03-20 at 04:23 +0100, Thierry Escande wrote:
> Add support for Qualcomm serial slave devices. Probe the serial
> device,
> retrieve its maximum speed and register a new hci uart device.
> 

Tell me, if I'm not mistaken, I gave you my tag for this.

If no, FWIW,

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> ---
> 
> v5:
> - Use gpio new name 'enable'
> 
> v4:
> - Rename divclk4 as susclk (its name in the bt chip)
> - Use gpiod_set_value_cansleep()
> - Replace #include <linux/of.h> with <linux/mod_devicetable.h>
> - Restore dependency on BT_HCIUART
> 
> v3:
> - Remove redundant call to gpiod_set_value() after devm_gpiod_get()
> - Check returned values for clk_set_rate() and clk_prepare_enable()
> - Use clk_disable_unprepare()
> 
> v2:
> - Fix author email
> 
>  drivers/bluetooth/Kconfig   |   1 +
>  drivers/bluetooth/hci_qca.c | 109
> +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 07e55cd8f8c8..e0f1a6609b68 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -196,6 +196,7 @@ config BT_HCIUART_BCM
>  config BT_HCIUART_QCA
>  	bool "Qualcomm Atheros protocol support"
>  	depends on BT_HCIUART
> +	depends on BT_HCIUART_SERDEV
>  	select BT_HCIUART_H4
>  	select BT_QCA
>  	help
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 05ec530b8a3a..6e6042f77784 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -29,7 +29,11 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/clk.h>
>  #include <linux/debugfs.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/serdev.h>
>  
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> @@ -50,6 +54,9 @@
>  #define IBS_TX_IDLE_TIMEOUT_MS		2000
>  #define BAUDRATE_SETTLE_TIMEOUT_MS	300
>  
> +/* susclk rate */
> +#define SUSCLK_RATE_32KHZ	32768
> +
>  /* HCI_IBS transmit side sleep protocol states */
>  enum tx_ibs_states {
>  	HCI_IBS_TX_ASLEEP,
> @@ -111,6 +118,12 @@ struct qca_data {
>  	u64 votes_off;
>  };
>  
> +struct qca_serdev {
> +	struct hci_uart	 serdev_hu;
> +	struct gpio_desc *bt_en;
> +	struct clk	 *susclk;
> +};
> +
>  static void __serial_clock_on(struct tty_struct *tty)
>  {
>  	/* TODO: Some chipset requires to enable UART clock on client
> @@ -386,6 +399,7 @@ static void hci_ibs_wake_retrans_timeout(struct
> timer_list *t)
>  /* Initialize protocol */
>  static int qca_open(struct hci_uart *hu)
>  {
> +	struct qca_serdev *qcadev;
>  	struct qca_data *qca;
>  
>  	BT_DBG("hu %p qca_open", hu);
> @@ -444,6 +458,13 @@ static int qca_open(struct hci_uart *hu)
>  	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
>  	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
>  
> +	if (hu->serdev) {
> +		serdev_device_open(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 1);
> +	}
> +
>  	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u,
> wake_retrans=%u",
>  	       qca->tx_idle_delay, qca->wake_retrans);
>  
> @@ -512,6 +533,7 @@ static int qca_flush(struct hci_uart *hu)
>  /* Close protocol */
>  static int qca_close(struct hci_uart *hu)
>  {
> +	struct qca_serdev *qcadev;
>  	struct qca_data *qca = hu->priv;
>  
>  	BT_DBG("hu %p qca close", hu);
> @@ -525,6 +547,13 @@ static int qca_close(struct hci_uart *hu)
>  	destroy_workqueue(qca->workqueue);
>  	qca->hu = NULL;
>  
> +	if (hu->serdev) {
> +		serdev_device_close(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> +	}
> +
>  	kfree_skb(qca->rx_skb);
>  
>  	hu->priv = NULL;
> @@ -885,6 +914,14 @@ static int qca_set_baudrate(struct hci_dev *hdev,
> uint8_t baudrate)
>  	return 0;
>  }
>  
> +static inline void host_set_baudrate(struct hci_uart *hu, unsigned
> int speed)
> +{
> +	if (hu->serdev)
> +		serdev_device_set_baudrate(hu->serdev, speed);
> +	else
> +		hci_uart_set_baudrate(hu, speed);
> +}
> +
>  static int qca_setup(struct hci_uart *hu)
>  {
>  	struct hci_dev *hdev = hu->hdev;
> @@ -905,7 +942,7 @@ static int qca_setup(struct hci_uart *hu)
>  		speed = hu->proto->init_speed;
>  
>  	if (speed)
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
>  
>  	/* Setup user speed if needed */
>  	speed = 0;
> @@ -924,7 +961,7 @@ static int qca_setup(struct hci_uart *hu)
>  				   ret);
>  			return ret;
>  		}
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
>  	}
>  
>  	/* Setup patch / NVM configurations */
> @@ -958,12 +995,80 @@ static struct hci_uart_proto qca_proto = {
>  	.dequeue	= qca_dequeue,
>  };
>  
> +static int qca_serdev_probe(struct serdev_device *serdev)
> +{
> +	struct qca_serdev *qcadev;
> +	int err;
> +
> +	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev),
> GFP_KERNEL);
> +	if (!qcadev)
> +		return -ENOMEM;
> +
> +	qcadev->serdev_hu.serdev = serdev;
> +	serdev_device_set_drvdata(serdev, qcadev);
> +
> +	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 bt-disable-n 
> gpio\n");
> +		return PTR_ERR(qcadev->bt_en);
> +	}
> +
> +	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;
> +
> +	err = clk_prepare_enable(qcadev->susclk);
> +	if (err)
> +		return err;
> +
> +	err = hci_uart_register_device(&qcadev->serdev_hu,
> &qca_proto);
> +	if (err)
> +		clk_disable_unprepare(qcadev->susclk);
> +
> +	return err;
> +}
> +
> +static void qca_serdev_remove(struct serdev_device *serdev)
> +{
> +	struct qca_serdev *qcadev =
> serdev_device_get_drvdata(serdev);
> +
> +	hci_uart_unregister_device(&qcadev->serdev_hu);
> +
> +	clk_disable_unprepare(qcadev->susclk);
> +}
> +
> +static const struct of_device_id qca_bluetooth_of_match[] = {
> +	{ .compatible = "qcom,qca6174-bt" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
> +
> +static struct serdev_device_driver qca_serdev_driver = {
> +	.probe = qca_serdev_probe,
> +	.remove = qca_serdev_remove,
> +	.driver = {
> +		.name = "hci_uart_qca",
> +		.of_match_table = qca_bluetooth_of_match,
> +	},
> +};
> +
>  int __init qca_init(void)
>  {
> +	serdev_device_driver_register(&qca_serdev_driver);
> +
>  	return hci_uart_register_proto(&qca_proto);
>  }
>  
>  int __exit qca_deinit(void)
>  {
> +	serdev_device_driver_unregister(&qca_serdev_driver);
> +
>  	return hci_uart_unregister_proto(&qca_proto);
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v5 3/3] Bluetooth: hci_qca: Add serdev support

[Marcel Holtmann]

Hi Thierry,

> Add support for Qualcomm serial slave devices. Probe the serial device,
> retrieve its maximum speed and register a new hci uart device.
> 
> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> ---
> 
> v5:
> - Use gpio new name 'enable'
> 
> v4:
> - Rename divclk4 as susclk (its name in the bt chip)
> - Use gpiod_set_value_cansleep()
> - Replace #include <linux/of.h> with <linux/mod_devicetable.h>
> - Restore dependency on BT_HCIUART
> 
> v3:
> - Remove redundant call to gpiod_set_value() after devm_gpiod_get()
> - Check returned values for clk_set_rate() and clk_prepare_enable()
> - Use clk_disable_unprepare()
> 
> v2:
> - Fix author email
> 
> drivers/bluetooth/Kconfig   |   1 +
> drivers/bluetooth/hci_qca.c | 109 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 07e55cd8f8c8..e0f1a6609b68 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -196,6 +196,7 @@ config BT_HCIUART_BCM
> config BT_HCIUART_QCA
> 	bool "Qualcomm Atheros protocol support"
> 	depends on BT_HCIUART
> +	depends on BT_HCIUART_SERDEV
> 	select BT_HCIUART_H4
> 	select BT_QCA
> 	help
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 05ec530b8a3a..6e6042f77784 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -29,7 +29,11 @@
>  */
> 
> #include <linux/kernel.h>
> +#include <linux/clk.h>
> #include <linux/debugfs.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/serdev.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -50,6 +54,9 @@
> #define IBS_TX_IDLE_TIMEOUT_MS		2000
> #define BAUDRATE_SETTLE_TIMEOUT_MS	300
> 
> +/* susclk rate */
> +#define SUSCLK_RATE_32KHZ	32768
> +
> /* HCI_IBS transmit side sleep protocol states */
> enum tx_ibs_states {
> 	HCI_IBS_TX_ASLEEP,
> @@ -111,6 +118,12 @@ struct qca_data {
> 	u64 votes_off;
> };
> 
> +struct qca_serdev {
> +	struct hci_uart	 serdev_hu;
> +	struct gpio_desc *bt_en;
> +	struct clk	 *susclk;
> +};
> +
> static void __serial_clock_on(struct tty_struct *tty)
> {
> 	/* TODO: Some chipset requires to enable UART clock on client
> @@ -386,6 +399,7 @@ static void hci_ibs_wake_retrans_timeout(struct timer_list *t)
> /* Initialize protocol */
> static int qca_open(struct hci_uart *hu)
> {
> +	struct qca_serdev *qcadev;
> 	struct qca_data *qca;
> 
> 	BT_DBG("hu %p qca_open", hu);
> @@ -444,6 +458,13 @@ static int qca_open(struct hci_uart *hu)
> 	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
> 	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
> 
> +	if (hu->serdev) {
> +		serdev_device_open(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 1);
> +	}
> +
> 	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
> 	       qca->tx_idle_delay, qca->wake_retrans);
> 
> @@ -512,6 +533,7 @@ static int qca_flush(struct hci_uart *hu)
> /* Close protocol */
> static int qca_close(struct hci_uart *hu)
> {
> +	struct qca_serdev *qcadev;
> 	struct qca_data *qca = hu->priv;
> 
> 	BT_DBG("hu %p qca close", hu);
> @@ -525,6 +547,13 @@ static int qca_close(struct hci_uart *hu)
> 	destroy_workqueue(qca->workqueue);
> 	qca->hu = NULL;
> 
> +	if (hu->serdev) {
> +		serdev_device_close(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> +	}
> +
> 	kfree_skb(qca->rx_skb);
> 
> 	hu->priv = NULL;
> @@ -885,6 +914,14 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
> 	return 0;
> }
> 
> +static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> +{
> +	if (hu->serdev)
> +		serdev_device_set_baudrate(hu->serdev, speed);
> +	else
> +		hci_uart_set_baudrate(hu, speed);
> +}
> +
> static int qca_setup(struct hci_uart *hu)
> {
> 	struct hci_dev *hdev = hu->hdev;
> @@ -905,7 +942,7 @@ static int qca_setup(struct hci_uart *hu)
> 		speed = hu->proto->init_speed;
> 
> 	if (speed)
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
> 
> 	/* Setup user speed if needed */
> 	speed = 0;
> @@ -924,7 +961,7 @@ static int qca_setup(struct hci_uart *hu)
> 				   ret);
> 			return ret;
> 		}
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
> 	}
> 
> 	/* Setup patch / NVM configurations */
> @@ -958,12 +995,80 @@ static struct hci_uart_proto qca_proto = {
> 	.dequeue	= qca_dequeue,
> };
> 
> +static int qca_serdev_probe(struct serdev_device *serdev)
> +{
> +	struct qca_serdev *qcadev;
> +	int err;
> +
> +	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> +	if (!qcadev)
> +		return -ENOMEM;
> +
> +	qcadev->serdev_hu.serdev = serdev;
> +	serdev_device_set_drvdata(serdev, qcadev);
> +
> +	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 bt-disable-n gpio\n");
> +		return PTR_ERR(qcadev->bt_en);
> +	}
> +
> +	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;
> +
> +	err = clk_prepare_enable(qcadev->susclk);
> +	if (err)
> +		return err;
> +
> +	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> +	if (err)
> +		clk_disable_unprepare(qcadev->susclk);
> +
> +	return err;
> +}

so this a more generic question. Does the clk setup has to be done in serdev probe or can we just do that within qca_open callback. I asked because I really want to move towards btuart.c and integrate the vendor specific pieces there nicely. So what I did was that I posted a v2 that has the vendor abstraction build in and it would be super simple to add qca support to it. However I have no vendor specific handling from within the probe callback. If that is not needed and we can do all the clk and GPIO setup in the vendor open callback, then it should be fairly simple to do (I am ignoring IBS support for now, but I realize it is there).

That all said, the hci_qca.c code has __serial_clock_on() and __serial_clock_off() empty stubs. Is this about the susclk or is that something totally different?

Regards

Marcel

Re: [PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Marcel Holtmann]

Hi Thierry,

> Add binding document for serial bluetooth chips using Qualcomm protocol.
> 
> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> ---
> 
> v5:
> - Rename 'bt-disable-n' gpio as 'enable'
> 
> v4:
> - Move bt-disable-n-gpios to required properties section
> - Add clocks and pinctrl-0 as required properties
> 
> v3: no change
> v2: no change
> 
> .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> new file mode 100644
> index 000000000000..bbc2973634b2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> @@ -0,0 +1,34 @@
> +Qualcomm Bluetooth Chips
> +---------------------
> +
> +This documents the binding structure and common properties for serial
> +attached Qualcomm devices.
> +
> +Serial attached Qualcomm devices shall be a child node of the host UART
> +device the slave device is attached to.
> +
> +Required properties:
> + - compatible: should contain one of the following:
> +   * "qcom,qca6174-bt"
> + - enable-gpios: gpio specifier used to enable chip
> + - pinctrl-0: pin phandle for bt_en gpio
> + - clocks: clock phandle for SUSCLK_32KHZ

if I compare this with broadcom-bluetooth.txt or ti-bluetooth.txt then besides compatible, everything else is optional. The nokia-bluetooth.txt has everything required, but that is also a really specific platform.

Can we be less restrictive for a QCA general purpose chip?

> +
> +Example:
> +
> +serial@7570000 {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&blsp1_uart1_default>;
> +	pinctrl-1 = <&blsp1_uart1_sleep>;
> +
> +	bluetooth {
> +		compatible = "qcom,qca6174-bt";
> +
> +		enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&bt_en_pin_a>;

This one I do not understand and you might want to shed some light into why this is done that way.

> +
> +		clocks = <&divclk4>;

No clock-names?

Regards

Marcel

Re: [PATCH v5 3/3] Bluetooth: hci_qca: Add serdev support

[Thierry Escande]

Hi Marcel,

On 20/03/2018 16:49, Marcel Holtmann wrote:
> Hi Thierry,
> 
>> Add support for Qualcomm serial slave devices. Probe the serial device,
>> retrieve its maximum speed and register a new hci uart device.
>>
>> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
>> ---
>>
>> v5:
>> - Use gpio new name 'enable'
>>
>> v4:
>> - Rename divclk4 as susclk (its name in the bt chip)
>> - Use gpiod_set_value_cansleep()
>> - Replace #include <linux/of.h> with <linux/mod_devicetable.h>
>> - Restore dependency on BT_HCIUART
>>
>> v3:
>> - Remove redundant call to gpiod_set_value() after devm_gpiod_get()
>> - Check returned values for clk_set_rate() and clk_prepare_enable()
>> - Use clk_disable_unprepare()
>>
>> v2:
>> - Fix author email
>>
>> drivers/bluetooth/Kconfig   |   1 +
>> drivers/bluetooth/hci_qca.c | 109 +++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 108 insertions(+), 2 deletions(-)
>>

<snip>

> 
> so this a more generic question. Does the clk setup has to be done in serdev probe or can we just do that within qca_open callback. I asked because I really want to move towards btuart.c and integrate the vendor specific pieces there nicely. So what I did was that I posted a v2 that has the vendor abstraction build in and it would be super simple to add qca support to it. However I have no vendor specific handling from within the probe callback. If that is not needed and we can do all the clk and GPIO setup in the vendor open callback, then it should be fairly simple to do (I am ignoring IBS support for now, but I realize it is there).
I did test that and doing clk and gpio setups in qca_open seems ok.

> 
> That all said, the hci_qca.c code has __serial_clock_on() and __serial_clock_off() empty stubs. Is this about the susclk or is that something totally different?
afaiu these stubs are used to control host UART clock. The susclk 
concerns the bt chip itself.

Regards,
Thierry

> 
> Regards
> 
> Marcel
> 

Re: [PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Thierry Escande]

Hi Marcel,

On 20/03/2018 16:58, Marcel Holtmann wrote:
> Hi Thierry,
> 
>> Add binding document for serial bluetooth chips using Qualcomm protocol.
>>
>> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
>> ---
>>
>> v5:
>> - Rename 'bt-disable-n' gpio as 'enable'
>>
>> v4:
>> - Move bt-disable-n-gpios to required properties section
>> - Add clocks and pinctrl-0 as required properties
>>
>> v3: no change
>> v2: no change
>>
>> .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> new file mode 100644
>> index 000000000000..bbc2973634b2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> @@ -0,0 +1,34 @@
>> +Qualcomm Bluetooth Chips
>> +---------------------
>> +
>> +This documents the binding structure and common properties for serial
>> +attached Qualcomm devices.
>> +
>> +Serial attached Qualcomm devices shall be a child node of the host UART
>> +device the slave device is attached to.
>> +
>> +Required properties:
>> + - compatible: should contain one of the following:
>> +   * "qcom,qca6174-bt"
>> + - enable-gpios: gpio specifier used to enable chip
>> + - pinctrl-0: pin phandle for bt_en gpio
>> + - clocks: clock phandle for SUSCLK_32KHZ
> 
> if I compare this with broadcom-bluetooth.txt or ti-bluetooth.txt then besides compatible, everything else is optional. The nokia-bluetooth.txt has everything required, but that is also a really specific platform.
> 
> Can we be less restrictive for a QCA general purpose chip?
Ok. To me, at least the enable gpio seems required.

> 
>> +
>> +Example:
>> +
>> +serial@7570000 {
>> +	pinctrl-names = "default", "sleep";
>> +	pinctrl-0 = <&blsp1_uart1_default>;
>> +	pinctrl-1 = <&blsp1_uart1_sleep>;
>> +
>> +	bluetooth {
>> +		compatible = "qcom,qca6174-bt";
>> +
>> +		enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
>> +
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&bt_en_pin_a>;
> 
> This one I do not understand and you might want to shed some light into why this is done that way.
Sure. It reclaims this gpio pin for this device. Will add it to the 
documentation.

> 
>> +
>> +		clocks = <&divclk4>;
> 
> No clock-names?
No need for a name as there is only one clk obtained by passing a NULL 
id to devm_gpiod_get().

Regards,
Thierry

> 
> Regards
> 
> Marcel
> 

Re: [PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Rob Herring]

On Tue, Mar 20, 2018 at 04:23:30AM +0100, Thierry Escande wrote:
> Add binding document for serial bluetooth chips using Qualcomm protocol.
> 
> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> ---
> 
> v5:
> - Rename 'bt-disable-n' gpio as 'enable'
> 
> v4:
> - Move bt-disable-n-gpios to required properties section
> - Add clocks and pinctrl-0 as required properties
> 
> v3: no change
> v2: no change
> 
>  .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Bjorn Andersson]

On Tue 20 Mar 23:58 HKT 2018, Marcel Holtmann wrote:
> > Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
[..]
> > + - clocks: clock phandle for SUSCLK_32KHZ
> 
> if I compare this with broadcom-bluetooth.txt or ti-bluetooth.txt then
> besides compatible, everything else is optional. The
> nokia-bluetooth.txt has everything required, but that is also a really
> specific platform.
> 
> Can we be less restrictive for a QCA general purpose chip?
> 

The way we deal with this in other bindings is that we tie such
requirements to the compatible; i.e. we would say that qcom,qca6174-bt
requires a clock and we would have something like qcom,qca-bt that makes
it optional.

The beauty of this is that the driver will tell you if you forgot to
specify the clock when it actually is required, which saves you
considerable amount of debugging time.


NB. The way the bcm driver handles this is insufficient, as it treats
any error from clk_get as "there's no clock specified". The driver
should accept a clock not being specified, but should fail properly when
a clock is specified but can't be acquired (e.g. due to clk_get()
returning EPROBE_DEFER).

> > +
> > +Example:
> > +
> > +serial@7570000 {
> > +	pinctrl-names = "default", "sleep";
> > +	pinctrl-0 = <&blsp1_uart1_default>;
> > +	pinctrl-1 = <&blsp1_uart1_sleep>;
> > +
> > +	bluetooth {
> > +		compatible = "qcom,qca6174-bt";
> > +
> > +		enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&bt_en_pin_a>;
> 
> This one I do not understand and you might want to shed some light
> into why this is done that way.
> 

This is completely generic and only relates to getting the electrical
properties of the gpio pin set up correctly. So I would recommend that
we omit this from the binding and example (including the pinctrl
properties for the serial above).

Regards,
Bjorn

Re: [PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Thierry Escande]

Hi Bjorn,

On 27/03/2018 00:51, Bjorn Andersson wrote:
> On Tue 20 Mar 23:58 HKT 2018, Marcel Holtmann wrote:
>>> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> [..]
>>> + - clocks: clock phandle for SUSCLK_32KHZ
>>
>> if I compare this with broadcom-bluetooth.txt or ti-bluetooth.txt then
>> besides compatible, everything else is optional. The
>> nokia-bluetooth.txt has everything required, but that is also a really
>> specific platform.
>>
>> Can we be less restrictive for a QCA general purpose chip?
>>
> 
> The way we deal with this in other bindings is that we tie such
> requirements to the compatible; i.e. we would say that qcom,qca6174-bt
> requires a clock and we would have something like qcom,qca-bt that makes
> it optional.
> 
> The beauty of this is that the driver will tell you if you forgot to
> specify the clock when it actually is required, which saves you
> considerable amount of debugging time.
> 
> 
> NB. The way the bcm driver handles this is insufficient, as it treats
> any error from clk_get as "there's no clock specified". The driver
> should accept a clock not being specified, but should fail properly when
> a clock is specified but can't be acquired (e.g. due to clk_get()
> returning EPROBE_DEFER).
> 
>>> +
>>> +Example:
>>> +
>>> +serial@7570000 {
>>> +	pinctrl-names = "default", "sleep";
>>> +	pinctrl-0 = <&blsp1_uart1_default>;
>>> +	pinctrl-1 = <&blsp1_uart1_sleep>;
>>> +
>>> +	bluetooth {
>>> +		compatible = "qcom,qca6174-bt";
>>> +
>>> +		enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
>>> +
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&bt_en_pin_a>;
>>
>> This one I do not understand and you might want to shed some light
>> into why this is done that way.
>>
> 
> This is completely generic and only relates to getting the electrical
> properties of the gpio pin set up correctly. So I would recommend that
> we omit this from the binding and example (including the pinctrl
> properties for the serial above).

If I remove the pinctrl entry in the bluetooth node, the gpio19 is then 
marked as unclaimed. The drive strength also defaults to low but that 
doesn't seem to be an issue and the the chip can still be enabled 
through gpio19. Is it ok to have it unclaimed? If so I can remove it 
from the binding and the doc then.

Regarding the blsp1_uart1_default of the serial node, I can still enable 
the chip if I remove it but the hci commands all end in timeout. It 
seems that the function for these pins has to be explicitly set to 
blsp_uart2. So at least, the default pinctrl seems mandatory.

Regards,
Thierry

> 
> Regards,
> Bjorn
> 

Re: [PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Bjorn Andersson]

On Tue 27 Mar 08:56 PDT 2018, Thierry Escande wrote:

> Hi Bjorn,
> 
> On 27/03/2018 00:51, Bjorn Andersson wrote:
> > On Tue 20 Mar 23:58 HKT 2018, Marcel Holtmann wrote:
> > > > Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> > [..]
> > > > + - clocks: clock phandle for SUSCLK_32KHZ
> > > 
> > > if I compare this with broadcom-bluetooth.txt or ti-bluetooth.txt then
> > > besides compatible, everything else is optional. The
> > > nokia-bluetooth.txt has everything required, but that is also a really
> > > specific platform.
> > > 
> > > Can we be less restrictive for a QCA general purpose chip?
> > > 
> > 
> > The way we deal with this in other bindings is that we tie such
> > requirements to the compatible; i.e. we would say that qcom,qca6174-bt
> > requires a clock and we would have something like qcom,qca-bt that makes
> > it optional.
> > 
> > The beauty of this is that the driver will tell you if you forgot to
> > specify the clock when it actually is required, which saves you
> > considerable amount of debugging time.
> > 
> > 
> > NB. The way the bcm driver handles this is insufficient, as it treats
> > any error from clk_get as "there's no clock specified". The driver
> > should accept a clock not being specified, but should fail properly when
> > a clock is specified but can't be acquired (e.g. due to clk_get()
> > returning EPROBE_DEFER).
> > 
> > > > +
> > > > +Example:
> > > > +
> > > > +serial@7570000 {
> > > > +	pinctrl-names = "default", "sleep";
> > > > +	pinctrl-0 = <&blsp1_uart1_default>;
> > > > +	pinctrl-1 = <&blsp1_uart1_sleep>;
> > > > +
> > > > +	bluetooth {
> > > > +		compatible = "qcom,qca6174-bt";
> > > > +
> > > > +		enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
> > > > +
> > > > +		pinctrl-names = "default";
> > > > +		pinctrl-0 = <&bt_en_pin_a>;
> > > 
> > > This one I do not understand and you might want to shed some light
> > > into why this is done that way.
> > > 
> > 
> > This is completely generic and only relates to getting the electrical
> > properties of the gpio pin set up correctly. So I would recommend that
> > we omit this from the binding and example (including the pinctrl
> > properties for the serial above).
> 
> If I remove the pinctrl entry in the bluetooth node, the gpio19 is then
> marked as unclaimed. The drive strength also defaults to low but that
> doesn't seem to be an issue and the the chip can still be enabled through
> gpio19. Is it ok to have it unclaimed? If so I can remove it from the
> binding and the doc then.
> 
> Regarding the blsp1_uart1_default of the serial node, I can still enable the
> chip if I remove it but the hci commands all end in timeout. It seems that
> the function for these pins has to be explicitly set to blsp_uart2. So at
> least, the default pinctrl seems mandatory.
> 

Our board needs these properties to get the uart and gpio in the right
state, but this is unrelated to BT - that's why I suggested that you
omit these properties from the BT binding.

Regards,
Bjorn