[PATCH v6 0/5] Enable Bluetooth functionality for WCN3990

[PATCH v6 0/5] Enable Bluetooth functionality for WCN3990

[Balakrishna Godavarthi]

v6:
  * Hooked up qca_power to qca_serdev.	
  * renamed all the naming inconsistency functions with qca_*
  * leveraged common code of ROME for wcn3990.
  * created wrapper functions for re-usable blocks.
  * updated function of _*regulator_enable and _*regualtor_disable.  
  * removed redundant comments and functions.
  * add code to derive the firmware files based on ROM Version.
  * created new patch for common function of ROME and wcn3990.
  * enables Qualcomm chip to operate at 3.2Mbps.
  * updated names of functions that are used for both the chips to keep this generic. 

v5:
  * updated with review comments for hci_qca and btqca
 
v4:
  * rebased all changes on top of bluetooth-next.
  * addressed review comments.
  * New patch created for firmware download.

v3:
  * Rebased all changes on top of bluetooth-next.
  * dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990
    - added dt-binding snippet for bluetooth chip wcn3990.
  * Bluetooth: Add support for wcn3990 soc.
    - updated BT_INFO with bt_dev_info, where ever necessary.

v2:
   * Add support for wcn3990:
    These changes are aligned as per serdev frame work.
          We implemented these changes on top of https://patchwork.kernel.org/patch/10316093/
        As this patch enable serdev for one of the Qualcomm BT chip.

        The changes in the patch include.
        - Adding support to voting up/down regulators for WCN3990.
           - Bluetooth firmware download for wcn3990.

  * Add device tree bindings for Atheros chips:
          These changes are on top of https://patchwork.kernel.org/patch/10316097/.
          - Description of device tree bindings.

Balakrishna Godavarthi (5):
  dt-bindings: net: bluetooth: Add device tree bindings for QTI chip
    wcn3990
  Bluetooth: btqca: Rename ROME related functions to Generic functions
  Bluetooth: hci_qca: Enable 3.2 Mbps operating speed.
  Bluetooth: btqca: Add wcn3990 firmware download support.
  Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

 .../bindings/net/qualcomm-bluetooth.txt       |  31 +-
 drivers/bluetooth/btqca.c                     |  59 ++-
 drivers/bluetooth/btqca.h                     |  20 +
 drivers/bluetooth/hci_qca.c                   | 423 ++++++++++++++++--
 4 files changed, 479 insertions(+), 54 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 1/5] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990

[Balakrishna Godavarthi]

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
Canges in v6:

	* Changed the oper-speed to max-speed.

Canges in v5:

	* Added entry for oper-speed and init-speed.

---
 .../bindings/net/qualcomm-bluetooth.txt       | 31 +++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
index 0ea18a53cc29..c4590bc87390 100644
--- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -10,12 +10,24 @@ device the slave device is attached to.
 Required properties:
  - compatible: should contain one of the following:
    * "qcom,qca6174-bt"
+   * "qcom,wcn3990-bt"
 
 Optional properties:
  - enable-gpios: gpio specifier used to enable chip
  - clocks: clock provided to the controller (SUSCLK_32KHZ)
-
-Example:
+ - vddpa-supply: Bluetooth VDD PA regulator handle
+ - vddio-supply: Bluetooth VDD IO regulator handle
+ - vddldo-supply: Bluetooth VDD LDO regulator handle. Kept under optional
+		  parameters as some of the chipsets doesn't require ldo or
+		  it may use from same vddio.
+ - vddxtal-supply: Bluetooth VDD XTAL regulator handle
+ - vddcore-supply: Bluetooth VDD CORE regulator handle
+ - vddpwd-supply: Chip power down gpio is required when bluetooth module
+		  and other modules like wifi co-exist in a singe chip and
+		  shares a common gpio to bring chip out of reset.
+ - max-speed: operating speed of the chip.
+
+Examples:
 
 serial@7570000 {
 	label = "BT-UART";
@@ -28,3 +40,18 @@ serial@7570000 {
 		clocks = <&divclk4>;
 	};
 };
+
+serial@898000 {
+	label = "BT-UART";
+	status = "okay";
+
+	bluetooth: bt_wcn3990 {
+		compatible = "qcom,wcn3990-bt";
+		vddio-supply = <&pm8998_s3>;
+		vddxtal-supply = <&pm8998_s5>;
+		vddcore-supply = <&pm8998_l7>;
+		vddpa-supply = <&pm8998_l17>;
+		vddldo-supply = <&pm8998_l25>;
+		max-speed = <3200000>;
+	};
+};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 2/5] Bluetooth: btqca: Rename ROME related functions to Generic functions

[Balakrishna Godavarthi]

Some of the QCA BTSoC ROME functions, are used for different versions
or different make of BTSoC's. Instead of duplicating the same functions
for new chip, updating names of the functions that are used for both
chip's to keep this generic and would help in future when we would have
new BT SoC.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---

Changes in v6:
	* initial patch
	* updated names of functions that are used for both the chips to 
	  keep this generic and would help in future when we would have 
	  new BT SoC.

---
 drivers/bluetooth/btqca.c | 14 +++++++-------
 drivers/bluetooth/btqca.h |  6 ++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 8219816c54a0..5c3551239b12 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -27,7 +27,7 @@
 
 #define VERSION "0.1"
 
-static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
+int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 {
 	struct sk_buff *skb;
 	struct edl_event_hdr *edl;
@@ -35,15 +35,14 @@ static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 	char cmd;
 	int err = 0;
 
-	BT_DBG("%s: ROME Patch Version Request", hdev->name);
+	bt_dev_dbg(hdev, "QCA BTSoC Patch Version Request");
 
 	cmd = EDL_PATCH_VER_REQ_CMD;
 	skb = __hci_cmd_sync_ev(hdev, EDL_PATCH_CMD_OPCODE, EDL_PATCH_CMD_LEN,
 				&cmd, HCI_VENDOR_PKT, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
-		BT_ERR("%s: Failed to read version of ROME (%d)", hdev->name,
-		       err);
+		bt_dev_err(hdev, "Failed to read version of BTSoC (%d)", err);
 		return err;
 	}
 
@@ -88,13 +87,14 @@ static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(qca_patch_ver_req);
 
 static int rome_reset(struct hci_dev *hdev)
 {
 	struct sk_buff *skb;
 	int err;
 
-	BT_DBG("%s: ROME HCI_RESET", hdev->name);
+	bt_dev_dbg(hdev, "QCA BTSoC HCI_RESET");
 
 	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
@@ -267,7 +267,7 @@ static int rome_download_firmware(struct hci_dev *hdev,
 	const u8 *segment;
 	int ret, remain, i = 0;
 
-	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
+	bt_dev_info(hdev, "QCA BTSoC Downloading %s", config->fwname);
 
 	ret = request_firmware(&fw, config->fwname, &hdev->dev);
 	if (ret) {
@@ -339,7 +339,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 	config.user_baud_rate = baudrate;
 
 	/* Get ROME version information */
-	err = rome_patch_ver_req(hdev, &rome_ver);
+	err = qca_patch_ver_req(hdev, &rome_ver);
 	if (err < 0 || rome_ver == 0) {
 		BT_ERR("%s: Failed to get version 0x%x", hdev->name, err);
 		return err;
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 13d77fd873b6..c1504a49fd79 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -128,6 +128,7 @@ struct tlv_type_hdr {
 
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate);
+int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version);
 
 #else
 
@@ -141,4 +142,9 @@ static inline int qca_uart_setup_rome(struct hci_dev *hdev, int speed)
 	return -EOPNOTSUPP;
 }
 
+static inline int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 3/5] Bluetooth: hci_qca: Enable 3.2 Mbps operating speed.

[Balakrishna Godavarthi]

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---

Changes in v6:
	* initial patch
	* enables Qualcomm chip to operate at 3.2Mbps. 

---
 drivers/bluetooth/hci_qca.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f05382b5a65d..cb1034998040 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -872,6 +872,8 @@ static uint8_t qca_get_baudrate_value(int speed)
 		return QCA_BAUDRATE_2000000;
 	case 3000000:
 		return QCA_BAUDRATE_3000000;
+	case 3200000:
+		return QCA_BAUDRATE_3200000;
 	case 3500000:
 		return QCA_BAUDRATE_3500000;
 	default:
@@ -886,7 +888,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	struct sk_buff *skb;
 	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
 
-	if (baudrate > QCA_BAUDRATE_3000000)
+	if (baudrate > QCA_BAUDRATE_3200000)
 		return -EINVAL;
 
 	cmd[4] = baudrate;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 4/5] Bluetooth: btqca: Add wcn3990 firmware download support.

[Balakrishna Godavarthi]

This patch enables the RAM and NV patch download for wcn3990.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
Changes in v6:
	* add code to derive the firmware files based on ROM Version.
	* created new patch for common function of ROME and wcn3990.

Changes in v5:
	* moved changes related to hci_qca.
	* addressed review comments.
 
Changes in v4:
	* initial patch.

---
 drivers/bluetooth/btqca.c | 45 +++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btqca.h |  8 +++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 5c3551239b12..cef3daf3285b 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -380,6 +380,51 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 }
 EXPORT_SYMBOL_GPL(qca_uart_setup_rome);
 
+int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+			    u32 *soc_ver)
+{
+	struct rome_config config;
+	int err;
+	u8 rom_ver;
+
+	/* Which firmware files to download are based on ROM version.
+	 * ROM version is derived from last two bytes of soc_ver.
+	 */
+	rom_ver = ((*soc_ver & 0x00000f00) >> 0x04) | (*soc_ver & 0x0000000f);
+	config.user_baud_rate = baudrate;
+	/* Download rampatch file */
+	config.type = TLV_TYPE_PATCH;
+	snprintf(config.fwname, sizeof(config.fwname), "qca/crbtfw%02x.tlv",
+		 rom_ver);
+	err = rome_download_firmware(hdev, &config);
+	if (err < 0) {
+		BT_ERR("%s: Failed to download patch (%d)", hdev->name, err);
+		return err;
+	}
+
+	/* Download NVM configuration */
+	config.type = TLV_TYPE_NVM;
+	snprintf(config.fwname, sizeof(config.fwname), "qca/crnv%02x.bin",
+		 rom_ver);
+	err = rome_download_firmware(hdev, &config);
+	if (err < 0) {
+		BT_ERR("%s: Failed to download NVM (%d)", hdev->name, err);
+		return err;
+	}
+
+	/* Perform HCI reset */
+	err = rome_reset(hdev);
+	if (err < 0) {
+		BT_ERR("%s: Failed to run HCI_RESET (%d)", hdev->name, err);
+		return err;
+	}
+
+	bt_dev_info(hdev, "wcn3990 setup on UART is completed");
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(qca_uart_setup_cherokee);
+
 MODULE_AUTHOR("Ben Young Tae Kim <ytkim@qca.qualcomm.com>");
 MODULE_DESCRIPTION("Bluetooth support for Qualcomm Atheros family ver " VERSION);
 MODULE_VERSION(VERSION);
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index c1504a49fd79..e93e0660cf6c 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -129,6 +129,8 @@ struct tlv_type_hdr {
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate);
 int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version);
+int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+			    u32 *soc_ver);
 
 #else
 
@@ -147,4 +149,10 @@ static inline int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 	return -EOPNOTSUPP;
 }
 
+static int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+				   u32 *soc_ver)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 5/5] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[Balakrishna Godavarthi]

Add support to set voltage/current of various regulators
to power up/down Bluetooth chip wcn3990.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---

Changes in v6:
	* Hooked up qca_power to qca_serdev.	
	* renamed all the naming inconsistency functions with qca_*
	* leveraged common code of ROME for wcn3990.
	* created wrapper functions for re-usable blocks.
	* updated function of _*regulator_enable and _*regualtor_disable.  
	* removed redundant comments and functions.
	* addressed review comments.

Changes in v5:
	* updated regulator vddpa min_uV to 1304000.
  	* addressed review comments.
 
Changes in v4:
	* Segregated the changes of btqca from hci_qca
	* rebased all changes on top of bluetooth-next.
	* addressed review comments.

---
 drivers/bluetooth/btqca.h   |   6 +
 drivers/bluetooth/hci_qca.c | 419 ++++++++++++++++++++++++++++++++----
 2 files changed, 381 insertions(+), 44 deletions(-)

diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index e93e0660cf6c..2e551b029970 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -37,6 +37,9 @@
 #define EDL_TAG_ID_HCI			(17)
 #define EDL_TAG_ID_DEEP_SLEEP		(27)
 
+#define CHEROKEE_POWERON_PULSE          0xFC
+#define CHEROKEE_POWEROFF_PULSE         0xC0
+
 enum qca_bardrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,
@@ -124,6 +127,9 @@ struct tlv_type_hdr {
 	__u8   data[0];
 } __packed;
 
+int qca_btsoc_power_setup(struct hci_uart *hu, bool on);
+int qca_btsoc_shutdown(struct hci_dev *hdev);
+
 #if IS_ENABLED(CONFIG_BT_QCA)
 
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index cb1034998040..e235be0e5202 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -5,7 +5,7 @@
  *  protocol extension to H4.
  *
  *  Copyright (C) 2007 Texas Instruments, Inc.
- *  Copyright (c) 2010, 2012 The Linux Foundation. All rights reserved.
+ *  Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights reserved.
  *
  *  Acknowledgements:
  *  This file is based on hci_ll.c, which was...
@@ -31,9 +31,14 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/serdev.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -119,10 +124,48 @@ struct qca_data {
 	u64 votes_off;
 };
 
+enum qca_btsoc_type {
+	BTQCA_INVALID = -1,
+	BTQCA_AR3002,
+	BTQCA_ROME,
+	BTQCA_CHEROKEE
+};
+
+/*
+ * Voltage regulator information required for configuring the
+ * QCA Bluetooth chipset
+ */
+struct qca_vreg {
+	const char *name;
+	unsigned int min_uV;
+	unsigned int max_uV;
+	unsigned int load_uA;
+};
+
+struct qca_vreg_data {
+	enum qca_btsoc_type soc_type;
+	struct qca_vreg *vregs;
+	size_t num_vregs;
+};
+
+/*
+ * Platform data for the QCA Bluetooth power driver.
+ */
+struct qca_power {
+	struct device *dev;
+	const struct qca_vreg_data *vreg_data;
+	struct regulator_bulk_data *vreg_bulk;
+	bool vregs_on;
+};
+
 struct qca_serdev {
 	struct hci_uart	 serdev_hu;
 	struct gpio_desc *bt_en;
 	struct clk	 *susclk;
+	enum qca_btsoc_type btsoc_type;
+	struct qca_power *bt_power;
+	u32 init_speed;
+	u32 oper_speed;
 };
 
 static void __serial_clock_on(struct tty_struct *tty)
@@ -402,6 +445,7 @@ static int qca_open(struct hci_uart *hu)
 {
 	struct qca_serdev *qcadev;
 	struct qca_data *qca;
+	int ret = 0;
 
 	BT_DBG("hu %p qca_open", hu);
 
@@ -463,13 +507,19 @@ static int qca_open(struct hci_uart *hu)
 		serdev_device_open(hu->serdev);
 
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 1);
+		if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
+			hu->init_speed = qcadev->init_speed;
+			hu->oper_speed = qcadev->oper_speed;
+			ret = qca_btsoc_power_setup(hu, true);
+		} else {
+			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);
 
-	return 0;
+	return ret;
 }
 
 static void qca_debugfs_init(struct hci_dev *hdev)
@@ -552,7 +602,10 @@ static int qca_close(struct hci_uart *hu)
 		serdev_device_close(hu->serdev);
 
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 0);
+		if (qcadev->btsoc_type == BTQCA_CHEROKEE)
+			qca_btsoc_shutdown(hu->hdev);
+		else
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
 	}
 
 	kfree_skb(qca->rx_skb);
@@ -925,20 +978,36 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
-static int qca_setup(struct hci_uart *hu)
+static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd)
 {
-	struct hci_dev *hdev = hu->hdev;
+	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
-	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
-	int ret;
+	struct sk_buff *skb;
 
-	bt_dev_info(hdev, "ROME setup");
+	bt_dev_dbg(hdev, "sending command %02x to SoC", cmd);
 
-	/* Patch downloading has to be done without IBS mode */
-	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
+	if (!skb) {
+		bt_dev_err(hdev, "Failed to allocate memory for skb packet");
+		return -ENOMEM;
+	}
+
+	skb_put_data(skb, &cmd, sizeof(cmd));
+	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
+
+	skb_queue_tail(&qca->txq, skb);
+	hci_uart_tx_wakeup(hu);
+
+	/* Wait for 100 uS for SoC to settle down */
+	usleep_range(100, 200);
+
+	return 0;
+}
+
+static void qca_set_init_speed(struct hci_uart *hu)
+{
+	unsigned int speed = 0;
 
-	/* Setup initial baudrate */
-	speed = 0;
 	if (hu->init_speed)
 		speed = hu->init_speed;
 	else if (hu->proto->init_speed)
@@ -946,29 +1015,136 @@ static int qca_setup(struct hci_uart *hu)
 
 	if (speed)
 		host_set_baudrate(hu, speed);
+}
+
+static int qca_set_operating_speed(struct hci_uart *hu, u32 *qca_baudrate)
+{
+	unsigned int speed = 0;
+	int ret;
 
-	/* Setup user speed if needed */
-	speed = 0;
 	if (hu->oper_speed)
 		speed = hu->oper_speed;
 	else if (hu->proto->oper_speed)
 		speed = hu->proto->oper_speed;
 
 	if (speed) {
-		qca_baudrate = qca_get_baudrate_value(speed);
-
-		bt_dev_info(hdev, "Set UART speed to %d", speed);
-		ret = qca_set_baudrate(hdev, qca_baudrate);
+		*qca_baudrate = qca_get_baudrate_value(speed);
+		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
+		ret = qca_set_baudrate(hu->hdev, *qca_baudrate);
 		if (ret) {
-			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
+			bt_dev_err(hu->hdev, "Failed to change the baud rate (%d)",
 				   ret);
 			return ret;
 		}
 		host_set_baudrate(hu, speed);
 	}
 
-	/* Setup patch / NVM configurations */
-	ret = qca_uart_setup_rome(hdev, qca_baudrate);
+	return ret;
+}
+
+int qca_btsoc_shutdown(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+
+	/* change host baud rate before sending power off command */
+	host_set_baudrate(hu, 2400);
+	qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);
+	/* turn off btsoc */
+	return qca_btsoc_power_setup(hu, false);
+}
+
+static int qca_setup(struct hci_uart *hu)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct qca_data *qca = hu->priv;
+	struct qca_serdev *qcadev;
+	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
+	int ret;
+	int soc_ver;
+
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+
+	switch (qcadev->btsoc_type) {
+	case BTQCA_CHEROKEE:
+		bt_dev_dbg(hdev, "setting up wcn3990");
+		/* Patch downloading has to be done without IBS mode */
+		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+		qca_set_init_speed(hu);
+		hci_uart_set_flow_control(hu, true);
+		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
+		if (ret) {
+			bt_dev_err(hdev, "failed to send power on command");
+			return ret;
+		}
+
+		/* Close and re-open the port */
+		serdev_device_close(hu->serdev);
+		ret = serdev_device_open(hu->serdev);
+		if (ret) {
+			bt_dev_err(hdev, "failed to open port");
+			return ret;
+		}
+
+		qca_set_init_speed(hu);
+		hci_uart_set_flow_control(hu, false);
+		msleep(100);
+		ret = qca_patch_ver_req(hdev, &soc_ver);
+		if (ret < 0 || soc_ver == 0) {
+			bt_dev_err(hdev, "Failed to get version 0x%x", ret);
+			return ret;
+		}
+
+		bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
+		hci_uart_set_flow_control(hu, true);
+		ret = qca_set_operating_speed(hu, &qca_baudrate);
+		if (ret)
+			return ret;
+		hci_uart_set_flow_control(hu, false);
+		/* Setup patch and NVM configurations */
+		ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
+
+		break;
+
+	default:
+		bt_dev_info(hdev, "ROME setup");
+
+		/* Patch downloading has to be done without IBS mode */
+		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+
+		/* Setup initial baudrate */
+		speed = 0;
+		if (hu->init_speed)
+			speed = hu->init_speed;
+		else if (hu->proto->init_speed)
+			speed = hu->proto->init_speed;
+
+		if (speed)
+			host_set_baudrate(hu, speed);
+
+		/* Setup user speed if needed */
+		speed = 0;
+		if (hu->oper_speed)
+			speed = hu->oper_speed;
+		else if (hu->proto->oper_speed)
+			speed = hu->proto->oper_speed;
+
+		if (speed) {
+			qca_baudrate = qca_get_baudrate_value(speed);
+
+			bt_dev_info(hdev, "Set UART speed to %d", speed);
+			ret = qca_set_baudrate(hdev, qca_baudrate);
+			if (ret) {
+				bt_dev_err(hdev, "Failed to change the baud rate (%d)",
+					    ret);
+				return ret;
+			}
+			host_set_baudrate(hu, speed);
+		}
+
+		/* Setup patch / NVM configurations */
+		ret = qca_uart_setup_rome(hdev, qca_baudrate);
+	}
+
 	if (!ret) {
 		set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
@@ -1004,9 +1180,120 @@ static struct hci_uart_proto qca_proto = {
 	.dequeue	= qca_dequeue,
 };
 
+static const struct qca_vreg_data qca_cherokee_data = {
+	.soc_type = BTQCA_CHEROKEE,
+	.vregs = (struct qca_vreg []) {
+		{ "vddio",   1352000, 1352000,  0 },
+		{ "vddxtal", 1904000, 2040000,  0 },
+		{ "vddcore", 1800000, 1800000,  1 },
+		{ "vddpa",   1304000, 1304000,  1 },
+		{ "vddldo",  3000000, 3312000,  1 },
+	},
+	.num_vregs = 5,
+};
+
+static int qca_enable_regulator(struct qca_vreg vregs,
+				struct regulator *regulator)
+{
+	int ret;
+
+	ret = regulator_set_voltage(regulator, vregs.min_uV,
+				    vregs.max_uV);
+	if (ret)
+		goto out;
+
+	if (vregs.load_uA)
+		ret = regulator_set_load(regulator,
+					 vregs.load_uA);
+
+	if (ret)
+		goto out;
+
+	ret = regulator_enable(regulator);
+
+out:
+	return ret;
+
+}
+
+static void qca_disable_regulator(struct qca_vreg vregs,
+				  struct regulator *regulator)
+{
+	/* Disable the regulator if requested by user
+	 * or when fault to enable any regulator.
+	 */
+	regulator_disable(regulator);
+	regulator_set_voltage(regulator, 0, vregs.max_uV);
+	if (vregs.load_uA)
+		regulator_set_load(regulator, 0);
+
+}
+
+int qca_btsoc_power_setup(struct hci_uart *hu, bool on)
+{
+	struct qca_vreg *vregs;
+	struct regulator_bulk_data *vreg_bulk;
+	struct qca_serdev *qcadev;
+	int i, num_vregs, ret = 0;
+
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+	if (!qcadev || !qcadev->bt_power || !qcadev->bt_power->vreg_data ||
+	    !qcadev->bt_power->vreg_bulk)
+		return -EINVAL;
+
+	vregs = qcadev->bt_power->vreg_data->vregs;
+	vreg_bulk = qcadev->bt_power->vreg_bulk;
+	num_vregs = qcadev->bt_power->vreg_data->num_vregs;
+	BT_DBG("on: %d", on);
+	if (on  && !qcadev->bt_power->vregs_on) {
+		for (i = 0; i < num_vregs; i++) {
+			ret = qca_enable_regulator(vregs[i],
+						   vreg_bulk[i].consumer);
+			if (ret)
+				break;
+		}
+		/* regulators failed */
+		if (ret) {
+			BT_ERR("failed to enable regulator:%s", vregs[i].name);
+			/* turn off regulators which are enabled */
+			for (i = i - 1; i >= 0; i--)
+				qca_disable_regulator(vregs[i],
+						      vreg_bulk[i].consumer);
+		} else {
+			qcadev->bt_power->vregs_on = true;
+		}
+	} else if (!on && qcadev->bt_power->vregs_on) {
+		/* turn off regulator in reverse order */
+		i = qcadev->bt_power->vreg_data->num_vregs - 1;
+		for ( ; i >= 0; i--)
+			qca_disable_regulator(vregs[i], vreg_bulk[i].consumer);
+		qcadev->bt_power->vregs_on = false;
+	}
+
+	return ret;
+}
+
+static int qca_init_regulators(struct qca_power *qca,
+			       const struct qca_vreg *vregs, size_t num_vregs)
+{
+	int i;
+
+	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
+				      sizeof(struct regulator_bulk_data),
+				      GFP_KERNEL);
+	if (!qca->vreg_bulk)
+		return -ENOMEM;
+
+	for (i = 0; i < num_vregs; i++)
+		qca->vreg_bulk[i].supply = vregs[i].name;
+
+	return devm_regulator_bulk_get(qca->dev, num_vregs, qca->vreg_bulk);
+}
+
 static int qca_serdev_probe(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev;
+	const struct qca_vreg_data *data;
 	int err;
 
 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
@@ -1014,34 +1301,72 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		return -ENOMEM;
 
 	qcadev->serdev_hu.serdev = serdev;
+	data = of_device_get_match_data(&serdev->dev);
+	if (data && data->soc_type == BTQCA_CHEROKEE)
+		qcadev->btsoc_type = BTQCA_CHEROKEE;
+	else
+		qcadev->btsoc_type = BTQCA_ROME;
+
 	serdev_device_set_drvdata(serdev, qcadev);
+	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
+		qcadev->bt_power = devm_kzalloc(&serdev->dev,
+						sizeof(struct qca_power),
+						GFP_KERNEL);
+		if (!qcadev->bt_power)
+			return -ENOMEM;
+
+		qcadev->bt_power->dev = &serdev->dev;
+		qcadev->bt_power->vreg_data = data;
+		err = qca_init_regulators(qcadev->bt_power, data->vregs,
+					  data->num_vregs);
+		if (err) {
+			BT_ERR("Failed to init regulators:%d", err);
+			devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
+			devm_kfree(&serdev->dev, qcadev->bt_power);
+			goto out;
+		}
 
-	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);
-	}
+		qcadev->bt_power->vregs_on = false;
+		device_property_read_u32(&serdev->dev, "max-speed",
+					 &qcadev->oper_speed);
+		if (!qcadev->oper_speed)
+			BT_INFO("UART will pick default operating speed");
+		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+		if (err) {
+			BT_ERR("wcn3990 serdev registration failed");
+			devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
+			devm_kfree(&serdev->dev, qcadev->bt_power);
+			goto out;
+		}
+	} else {
+		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);
+		}
 
-	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);
-	}
+		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_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)
-		clk_disable_unprepare(qcadev->susclk);
+		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+		if (err)
+			clk_disable_unprepare(qcadev->susclk);
+	}
+
+out:	return err;
 
-	return err;
 }
 
 static void qca_serdev_remove(struct serdev_device *serdev)
@@ -1050,11 +1375,17 @@ static void qca_serdev_remove(struct serdev_device *serdev)
 
 	hci_uart_unregister_device(&qcadev->serdev_hu);
 
-	clk_disable_unprepare(qcadev->susclk);
+	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
+		devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
+		devm_kfree(&serdev->dev, qcadev->bt_power);
+	} else {
+		clk_disable_unprepare(qcadev->susclk);
+	}
 }
 
 static const struct of_device_id qca_bluetooth_of_match[] = {
 	{ .compatible = "qcom,qca6174-bt" },
+	{ .compatible = "qcom,wcn3990-bt", .data = &qca_cherokee_data},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 2/5] Bluetooth: btqca: Rename ROME related functions to Generic functions

[Matthias Kaehlcke]

On Thu, May 24, 2018 at 09:30:48PM +0530, Balakrishna Godavarthi wrote:
> Some of the QCA BTSoC ROME functions, are used for different versions
> or different make of BTSoC's. Instead of duplicating the same functions
> for new chip, updating names of the functions that are used for both
> chip's to keep this generic and would help in future when we would have
> new BT SoC.

The commit message talks about 'functions', however only the name of
one function (rome_patch_ver_req()) has changed. From the changes in
log messages I assume you also intended to rename
rome_reset() and rome_download_firmware(). 

> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> 
> Changes in v6:
> 	* initial patch
> 	* updated names of functions that are used for both the chips to 
> 	  keep this generic and would help in future when we would have 
> 	  new BT SoC.
> 
> ---
>  drivers/bluetooth/btqca.c | 14 +++++++-------
>  drivers/bluetooth/btqca.h |  6 ++++++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 8219816c54a0..5c3551239b12 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -27,7 +27,7 @@
>  
>  #define VERSION "0.1"
>  
> -static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
> +int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)

The different version names are confusing. The name of the function
calls it 'patch version', the parameter 'rome_version' and the callers
vary between 'soc_version' and 'rome_version'.

It would be unfair to ask you to 'fix the world' in this patch series,
but please stick at least to a consistent naming scheme in the code
you change. Obviously feel free to squeeze in another cleanup patch :)

>  static int rome_reset(struct hci_dev *hdev)
>  {
>  	struct sk_buff *skb;
>  	int err;
>  
> -	BT_DBG("%s: ROME HCI_RESET", hdev->name);
> +	bt_dev_dbg(hdev, "QCA BTSoC HCI_RESET");
>  
>  	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>  	if (IS_ERR(skb)) {
> @@ -267,7 +267,7 @@ static int rome_download_firmware(struct hci_dev *hdev,
>  	const u8 *segment;
>  	int ret, remain, i = 0;
>  
> -	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
> +	bt_dev_info(hdev, "QCA BTSoC Downloading %s", config->fwname);
>  
>  	ret = request_firmware(&fw, config->fwname, &hdev->dev);
>  	if (ret) {
> @@ -339,7 +339,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
>  	config.user_baud_rate = baudrate;
>  
>  	/* Get ROME version information */

'patch version'/'SoC version'/... , whatever the chosen consistent
name is.

> -	err = rome_patch_ver_req(hdev, &rome_ver);
> +	err = qca_patch_ver_req(hdev, &rome_ver);

rome_ver => ${consistent_name}_ver

Thanks

Matthias

Re: [PATCH v6 3/5] Bluetooth: hci_qca: Enable 3.2 Mbps operating speed.

[Matthias Kaehlcke]

On Thu, May 24, 2018 at 09:30:49PM +0530, Balakrishna Godavarthi wrote:
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> 
> Changes in v6:
> 	* initial patch
> 	* enables Qualcomm chip to operate at 3.2Mbps.

The second point should be rather part of the commit message, but
since the summary basically states the same I guess it's fine to
omit it.

> 
> ---
>  drivers/bluetooth/hci_qca.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f05382b5a65d..cb1034998040 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -872,6 +872,8 @@ static uint8_t qca_get_baudrate_value(int speed)
>  		return QCA_BAUDRATE_2000000;
>  	case 3000000:
>  		return QCA_BAUDRATE_3000000;
> +	case 3200000:
> +		return QCA_BAUDRATE_3200000;
>  	case 3500000:
>  		return QCA_BAUDRATE_3500000;
>  	default:
> @@ -886,7 +888,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	struct sk_buff *skb;
>  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>  
> -	if (baudrate > QCA_BAUDRATE_3000000)
> +	if (baudrate > QCA_BAUDRATE_3200000)
>  		return -EINVAL;
>  
>  	cmd[4] = baudrate;

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v6 5/5] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

Hi Balakrishna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[cannot apply to bluetooth/master v4.17-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Balakrishna-Godavarthi/Enable-Bluetooth-functionality-for-WCN3990/20180527-135618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-randconfig-x005-201821 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from drivers/bluetooth/btqca.c:26:0:
>> drivers/bluetooth/btqca.h:130:34: warning: 'struct hci_uart' declared inside parameter list will not be visible outside of this definition or declaration
    int qca_btsoc_power_setup(struct hci_uart *hu, bool on);
                                     ^~~~~~~~

vim +130 drivers/bluetooth/btqca.h

   129	
 > 130	int qca_btsoc_power_setup(struct hci_uart *hu, bool on);
   131	int qca_btsoc_shutdown(struct hci_dev *hdev);
   132	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26221 bytes --]

Re: [PATCH v6 5/5] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

Hi Balakrishna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[cannot apply to bluetooth/master v4.17-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Balakrishna-Godavarthi/Enable-Bluetooth-functionality-for-WCN3990/20180527-135618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: i386-randconfig-b0-05271406 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from drivers/bluetooth/btqca.c:26:0:
>> drivers/bluetooth/btqca.h:130:34: warning: 'struct hci_uart' declared inside parameter list
    int qca_btsoc_power_setup(struct hci_uart *hu, bool on);
                                     ^
>> drivers/bluetooth/btqca.h:130:34: warning: its scope is only this definition or declaration, which is probably not what you want

vim +130 drivers/bluetooth/btqca.h

   129	
 > 130	int qca_btsoc_power_setup(struct hci_uart *hu, bool on);
   131	int qca_btsoc_shutdown(struct hci_dev *hdev);
   132	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25780 bytes --]

Re: [PATCH v6 2/5] Bluetooth: btqca: Rename ROME related functions to Generic functions

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-05-26 03:27, Matthias Kaehlcke wrote:
> On Thu, May 24, 2018 at 09:30:48PM +0530, Balakrishna Godavarthi wrote:
>> Some of the QCA BTSoC ROME functions, are used for different versions
>> or different make of BTSoC's. Instead of duplicating the same 
>> functions
>> for new chip, updating names of the functions that are used for both
>> chip's to keep this generic and would help in future when we would 
>> have
>> new BT SoC.
> 
> The commit message talks about 'functions', however only the name of
> one function (rome_patch_ver_req()) has changed. From the changes in
> log messages I assume you also intended to rename
> rome_reset() and rome_download_firmware().
> 
[Bala]: may be commit message is confusing, my initial intention in 
changing function names are restricted to functions which are exported 
to hci_qca.c and logs.
         But as per your suggestion i can also change the rome_reset() 
(qca_send_reset) and rome_download_firmware() (qca_download_firmware()) 
too.
         Will update the max possible function names in next incremental 
patch.
         But i will restrict my changes to only required function names. 
will not change the function names related to ROME (common to wnc3990) 
which are called in other files (some function are used in btusb.c)and 
also
         the structure defined with name rome_.*. Changing these name 
could break some functionalities of already available code.

>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> 
>> Changes in v6:
>> 	* initial patch
>> 	* updated names of functions that are used for both the chips to
>> 	  keep this generic and would help in future when we would have
>> 	  new BT SoC.
>> 
>> ---
>>  drivers/bluetooth/btqca.c | 14 +++++++-------
>>  drivers/bluetooth/btqca.h |  6 ++++++
>>  2 files changed, 13 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index 8219816c54a0..5c3551239b12 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -27,7 +27,7 @@
>> 
>>  #define VERSION "0.1"
>> 
>> -static int rome_patch_ver_req(struct hci_dev *hdev, u32 
>> *rome_version)
>> +int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
> 
> The different version names are confusing. The name of the function
> calls it 'patch version', the parameter 'rome_version' and the callers
> vary between 'soc_version' and 'rome_version'.
> 
> It would be unfair to ask you to 'fix the world' in this patch series,
> but please stick at least to a consistent naming scheme in the code
> you change. Obviously feel free to squeeze in another cleanup patch :)
> 

[Bala]: will update function and argument names.

>>  static int rome_reset(struct hci_dev *hdev)
>>  {
>>  	struct sk_buff *skb;
>>  	int err;
>> 
>> -	BT_DBG("%s: ROME HCI_RESET", hdev->name);
>> +	bt_dev_dbg(hdev, "QCA BTSoC HCI_RESET");
>> 
>>  	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>  	if (IS_ERR(skb)) {
>> @@ -267,7 +267,7 @@ static int rome_download_firmware(struct hci_dev 
>> *hdev,
>>  	const u8 *segment;
>>  	int ret, remain, i = 0;
>> 
>> -	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
>> +	bt_dev_info(hdev, "QCA BTSoC Downloading %s", config->fwname);
>> 
>>  	ret = request_firmware(&fw, config->fwname, &hdev->dev);
>>  	if (ret) {
>> @@ -339,7 +339,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, 
>> uint8_t baudrate)
>>  	config.user_baud_rate = baudrate;
>> 
>>  	/* Get ROME version information */
> 
> 'patch version'/'SoC version'/... , whatever the chosen consistent
> name is.
> 
>> -	err = rome_patch_ver_req(hdev, &rome_ver);
>> +	err = qca_patch_ver_req(hdev, &rome_ver);
> 
> rome_ver => ${consistent_name}_ver
> 
> Thanks
> 
> Matthias

Can you please review other patches too so that i can address review 
comments in next patch set.

-- 
Regards
Balakrishna.

Re: [PATCH v6 5/5] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[Matthias Kaehlcke]

On Thu, May 24, 2018 at 09:30:51PM +0530, Balakrishna Godavarthi wrote:
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> 
> Changes in v6:
> 	* Hooked up qca_power to qca_serdev.	
> 	* renamed all the naming inconsistency functions with qca_*
> 	* leveraged common code of ROME for wcn3990.
> 	* created wrapper functions for re-usable blocks.
> 	* updated function of _*regulator_enable and _*regualtor_disable.  
> 	* removed redundant comments and functions.
> 	* addressed review comments.
> 
> Changes in v5:
> 	* updated regulator vddpa min_uV to 1304000.
>   	* addressed review comments.
>  
> Changes in v4:
> 	* Segregated the changes of btqca from hci_qca
> 	* rebased all changes on top of bluetooth-next.
> 	* addressed review comments.
> ---
>
> ...
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index cb1034998040..e235be0e5202 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> ...
> +static void qca_set_init_speed(struct hci_uart *hu)
> +{
> +	unsigned int speed = 0;
>  
> -	/* Setup initial baudrate */
> -	speed = 0;
>  	if (hu->init_speed)
>  		speed = hu->init_speed;
>  	else if (hu->proto->init_speed)
> @@ -946,29 +1015,136 @@ static int qca_setup(struct hci_uart *hu)
>  
>  	if (speed)
>  		host_set_baudrate(hu, speed);
> +}
> +
> +static int qca_set_operating_speed(struct hci_uart *hu, u32 *qca_baudrate)
> +{

This is a bit convoluted, with the function setting the speed and
returning it. I would suggest a qca_get_oper_speed() and
qca_get_init_speed(), and just have them return the value instead of
passing it through a pointer. You could then have a qca_set_speed()
which converts the baudrate to the value the chip understands, calls
qca_set_baudrate() and host_set_baudrate()

> +static int qca_setup(struct hci_uart *hu)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	struct qca_data *qca = hu->priv;
> +	struct qca_serdev *qcadev;
> +	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> +	int ret;
> +	int soc_ver;
> +
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
> +
> +	switch (qcadev->btsoc_type) {
> +	case BTQCA_CHEROKEE:
> +		bt_dev_dbg(hdev, "setting up wcn3990");
> +		/* Patch downloading has to be done without IBS mode */
> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +		qca_set_init_speed(hu);
> +		hci_uart_set_flow_control(hu, true);
> +		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
> +		if (ret) {
> +			bt_dev_err(hdev, "failed to send power on command");
> +			return ret;
> +		}
> +
> +		/* Close and re-open the port */

As mentioned earlier, this comment doesn't provide any useful
information, it's evident from the code. Rather explain *why* the
close/open is needed.

> +		serdev_device_close(hu->serdev);
> +		ret = serdev_device_open(hu->serdev);
> +		if (ret) {
> +			bt_dev_err(hdev, "failed to open port");
> +			return ret;
> +		}
> +
> +		qca_set_init_speed(hu);
> +		hci_uart_set_flow_control(hu, false);
> +		msleep(100);

Which step makes the delay necessary? I guess it's not enabling flow
control but probably sending the power on command. If that is correct
the delay should be done after the corresponding command.

> +		ret = qca_patch_ver_req(hdev, &soc_ver);
> +		if (ret < 0 || soc_ver == 0) {
> +			bt_dev_err(hdev, "Failed to get version 0x%x", ret);

Probably better: "Failed to get version: %d" since ret contains an
errno and the problem wasn't that we couldn't get "version 0x<errno>".

> +			return ret;
> +		}
> +
> +		bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
> +		hci_uart_set_flow_control(hu, true);

Repeatedly switching on and off of flow control is a bit noisy, I
wonder if it could be hidden in a wrapper. From the code it seems that
flow control is disabled (true) for init speed and and enabled (false)
for operating speed. If this is correct the hci_uart_set_flow_control()
calls could be moved inside qca_set_oper/init_speed(). struct qca_serdev
could have a flag indicating if flow control is supported at all to
skip the hci_uart_set_flow_control() calls for Rome.

Just an idea, no objections if you prefer to leave it as is and Marcel
is ok with it.

> +		ret = qca_set_operating_speed(hu, &qca_baudrate);
> +		if (ret)
> +			return ret;
> +		hci_uart_set_flow_control(hu, false);
> +		/* Setup patch and NVM configurations */
> +		ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
> +
> +		break;
> +
> +	default:
> +		bt_dev_info(hdev, "ROME setup");
> +
> +		/* Patch downloading has to be done without IBS mode */
> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +
> +		/* Setup initial baudrate */
> +		speed = 0;
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;

If you introduce helpers to determine the init/oper speed these should
also be used here. Probably best to do this in a separate patch.

> +
> +		if (speed)
> +			host_set_baudrate(hu, speed);
> +
> +		/* Setup user speed if needed */
> +		speed = 0;
> +		if (hu->oper_speed)
> +			speed = hu->oper_speed;
> +		else if (hu->proto->oper_speed)
> +			speed = hu->proto->oper_speed;

ditto

> +static void qca_disable_regulator(struct qca_vreg vregs,
> +				  struct regulator *regulator)
> +{
> +	/* Disable the regulator if requested by user
> +	 * or when fault to enable any regulator.
> +	 */

This comment is not useful. The following code simply disables the
regulator, why this is done is a question of the caller.

> +	regulator_disable(regulator);
> +	regulator_set_voltage(regulator, 0, vregs.max_uV);
> +	if (vregs.load_uA)
> +		regulator_set_load(regulator, 0);
> +
> +}
> +
> +int qca_btsoc_power_setup(struct hci_uart *hu, bool on)
> +{
> +	struct qca_vreg *vregs;
> +	struct regulator_bulk_data *vreg_bulk;
> +	struct qca_serdev *qcadev;
> +	int i, num_vregs, ret = 0;
> +
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
> +	if (!qcadev || !qcadev->bt_power || !qcadev->bt_power->vreg_data ||
> +	    !qcadev->bt_power->vreg_bulk)
> +		return -EINVAL;
> +
> +	vregs = qcadev->bt_power->vreg_data->vregs;
> +	vreg_bulk = qcadev->bt_power->vreg_bulk;
> +	num_vregs = qcadev->bt_power->vreg_data->num_vregs;
> +	BT_DBG("on: %d", on);
> +	if (on  && !qcadev->bt_power->vregs_on) {
> +		for (i = 0; i < num_vregs; i++) {
> +			ret = qca_enable_regulator(vregs[i],
> +						   vreg_bulk[i].consumer);
> +			if (ret)
> +				break;
> +		}
> +		/* regulators failed */

Comment is not useful, BT_ERR below leaves things clear.

> +		if (ret) {
> +			BT_ERR("failed to enable regulator:%s", vregs[i].name);
> +			/* turn off regulators which are enabled */
> +			for (i = i - 1; i >= 0; i--)
> +				qca_disable_regulator(vregs[i],
> +						      vreg_bulk[i].consumer);
> +		} else {
> +			qcadev->bt_power->vregs_on = true;
> +		}
> +	} else if (!on && qcadev->bt_power->vregs_on) {
> +		/* turn off regulator in reverse order */
> +		i = qcadev->bt_power->vreg_data->num_vregs - 1;
> +		for ( ; i >= 0; i--)
> +			qca_disable_regulator(vregs[i], vreg_bulk[i].consumer);
> +		qcadev->bt_power->vregs_on = false;
> +	}
> +
> +	return ret;
> +}
> +
> +static int qca_init_regulators(struct qca_power *qca,
> +			       const struct qca_vreg *vregs, size_t num_vregs)
> +{
> +	int i;
> +
> +	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
> +				      sizeof(struct regulator_bulk_data),
> +				      GFP_KERNEL);
> +	if (!qca->vreg_bulk)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_vregs; i++)
> +		qca->vreg_bulk[i].supply = vregs[i].name;
> +
> +	return devm_regulator_bulk_get(qca->dev, num_vregs, qca->vreg_bulk);
> +}
> +
>  static int qca_serdev_probe(struct serdev_device *serdev)
>  {
>  	struct qca_serdev *qcadev;
> +	const struct qca_vreg_data *data;
>  	int err;
>  
>  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> @@ -1014,34 +1301,72 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		return -ENOMEM;
>  
>  	qcadev->serdev_hu.serdev = serdev;
> +	data = of_device_get_match_data(&serdev->dev);
> +	if (data && data->soc_type == BTQCA_CHEROKEE)
> +		qcadev->btsoc_type = BTQCA_CHEROKEE;
> +	else
> +		qcadev->btsoc_type = BTQCA_ROME;
> +
>  	serdev_device_set_drvdata(serdev, qcadev);
> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
> +		qcadev->bt_power = devm_kzalloc(&serdev->dev,
> +						sizeof(struct qca_power),
> +						GFP_KERNEL);
> +		if (!qcadev->bt_power)
> +			return -ENOMEM;
> +
> +		qcadev->bt_power->dev = &serdev->dev;
> +		qcadev->bt_power->vreg_data = data;
> +		err = qca_init_regulators(qcadev->bt_power, data->vregs,
> +					  data->num_vregs);
> +		if (err) {
> +			BT_ERR("Failed to init regulators:%d", err);
> +			devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
> +			devm_kfree(&serdev->dev, qcadev->bt_power);

Not necessary, the memory is allocated with devm_kzalloc(&serdev->dev,
...), therefore it is freed if probe() fails.

> +			goto out;
> +		}
>  
> -	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);
> -	}
> +		qcadev->bt_power->vregs_on = false;
> +		device_property_read_u32(&serdev->dev, "max-speed",
> +					 &qcadev->oper_speed);
> +		if (!qcadev->oper_speed)
> +			BT_INFO("UART will pick default operating speed");
> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> +		if (err) {
> +			BT_ERR("wcn3990 serdev registration failed");
> +			devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
> +			devm_kfree(&serdev->dev, qcadev->bt_power);

no need to free memory.

> +			goto out;
> +		}
> +	} else {
> +		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);
> +		}
>  
> -	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);
> -	}
> +		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_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)
> -		clk_disable_unprepare(qcadev->susclk);
> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> +		if (err)
> +			clk_disable_unprepare(qcadev->susclk);
> +	}
> +
> +out:	return err;
>  
> -	return err;
>  }
>  
>  static void qca_serdev_remove(struct serdev_device *serdev)
> @@ -1050,11 +1375,17 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>  
>  	hci_uart_unregister_device(&qcadev->serdev_hu);
>  
> -	clk_disable_unprepare(qcadev->susclk);
> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
> +		devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
> +		devm_kfree(&serdev->dev, qcadev->bt_power);

no need to free memory

Re: [PATCH v6 2/5] Bluetooth: btqca: Rename ROME related functions to Generic functions

[Matthias Kaehlcke]

On Mon, May 28, 2018 at 04:34:18PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-05-26 03:27, Matthias Kaehlcke wrote:
> > On Thu, May 24, 2018 at 09:30:48PM +0530, Balakrishna Godavarthi wrote:
> > > Some of the QCA BTSoC ROME functions, are used for different versions
> > > or different make of BTSoC's. Instead of duplicating the same
> > > functions
> > > for new chip, updating names of the functions that are used for both
> > > chip's to keep this generic and would help in future when we would
> > > have
> > > new BT SoC.
> > 
> > The commit message talks about 'functions', however only the name of
> > one function (rome_patch_ver_req()) has changed. From the changes in
> > log messages I assume you also intended to rename
> > rome_reset() and rome_download_firmware().
> > 
> [Bala]: may be commit message is confusing, my initial intention in changing
> function names are restricted to functions which are exported to hci_qca.c
> and logs.
>         But as per your suggestion i can also change the rome_reset()
> (qca_send_reset) and rome_download_firmware() (qca_download_firmware()) too.
>         Will update the max possible function names in next incremental
> patch.
>         But i will restrict my changes to only required function names. will
> not change the function names related to ROME (common to wnc3990) which are
> called in other files (some function are used in btusb.c)and also
>         the structure defined with name rome_.*. Changing these name could
> break some functionalities of already available code.
> 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > ---
> > > 
> > > Changes in v6:
> > > 	* initial patch
> > > 	* updated names of functions that are used for both the chips to
> > > 	  keep this generic and would help in future when we would have
> > > 	  new BT SoC.
> > > 
> > > ---
> > >  drivers/bluetooth/btqca.c | 14 +++++++-------
> > >  drivers/bluetooth/btqca.h |  6 ++++++
> > >  2 files changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > index 8219816c54a0..5c3551239b12 100644
> > > --- a/drivers/bluetooth/btqca.c
> > > +++ b/drivers/bluetooth/btqca.c
> > > @@ -27,7 +27,7 @@
> > > 
> > >  #define VERSION "0.1"
> > > 
> > > -static int rome_patch_ver_req(struct hci_dev *hdev, u32
> > > *rome_version)
> > > +int qca_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
> > 
> > The different version names are confusing. The name of the function
> > calls it 'patch version', the parameter 'rome_version' and the callers
> > vary between 'soc_version' and 'rome_version'.
> > 
> > It would be unfair to ask you to 'fix the world' in this patch series,
> > but please stick at least to a consistent naming scheme in the code
> > you change. Obviously feel free to squeeze in another cleanup patch :)
> > 
> 
> [Bala]: will update function and argument names.
> 
> > >  static int rome_reset(struct hci_dev *hdev)
> > >  {
> > >  	struct sk_buff *skb;
> > >  	int err;
> > > 
> > > -	BT_DBG("%s: ROME HCI_RESET", hdev->name);
> > > +	bt_dev_dbg(hdev, "QCA BTSoC HCI_RESET");
> > > 
> > >  	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
> > >  	if (IS_ERR(skb)) {
> > > @@ -267,7 +267,7 @@ static int rome_download_firmware(struct hci_dev
> > > *hdev,
> > >  	const u8 *segment;
> > >  	int ret, remain, i = 0;
> > > 
> > > -	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
> > > +	bt_dev_info(hdev, "QCA BTSoC Downloading %s", config->fwname);
> > > 
> > >  	ret = request_firmware(&fw, config->fwname, &hdev->dev);
> > >  	if (ret) {
> > > @@ -339,7 +339,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev,
> > > uint8_t baudrate)
> > >  	config.user_baud_rate = baudrate;
> > > 
> > >  	/* Get ROME version information */
> > 
> > 'patch version'/'SoC version'/... , whatever the chosen consistent
> > name is.
> > 
> > > -	err = rome_patch_ver_req(hdev, &rome_ver);
> > > +	err = qca_patch_ver_req(hdev, &rome_ver);
> > 
> > rome_ver => ${consistent_name}_ver
> > 
> > Thanks
> > 
> > Matthias
> 
> Can you please review other patches too so that i can address review
> comments in next patch set.

Done

Sorry, for the delay, reviews can be time intensive and I have a
limited bandwidth for it. In any case it's good to also give Marcel
and Johan some time to look at it before respinning, after all it's
their opinion what counts most, I'm just trying to (hopefully) help a
bit.

Re: [PATCH v6 5/5] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[Balakrishna Godavarthi]

HI Matthias,

Pls find my comments inline.

On 2018-05-29 23:11, Matthias Kaehlcke wrote:
> On Thu, May 24, 2018 at 09:30:51PM +0530, Balakrishna Godavarthi wrote:
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> 
>> Changes in v6:
>> 	* Hooked up qca_power to qca_serdev.
>> 	* renamed all the naming inconsistency functions with qca_*
>> 	* leveraged common code of ROME for wcn3990.
>> 	* created wrapper functions for re-usable blocks.
>> 	* updated function of _*regulator_enable and _*regualtor_disable.
>> 	* removed redundant comments and functions.
>> 	* addressed review comments.
>> 
>> Changes in v5:
>> 	* updated regulator vddpa min_uV to 1304000.
>>   	* addressed review comments.
>> 
>> Changes in v4:
>> 	* Segregated the changes of btqca from hci_qca
>> 	* rebased all changes on top of bluetooth-next.
>> 	* addressed review comments.
>> ---
>> 
>> ...
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index cb1034998040..e235be0e5202 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> ...
>> +static void qca_set_init_speed(struct hci_uart *hu)
>> +{
>> +	unsigned int speed = 0;
>> 
>> -	/* Setup initial baudrate */
>> -	speed = 0;
>>  	if (hu->init_speed)
>>  		speed = hu->init_speed;
>>  	else if (hu->proto->init_speed)
>> @@ -946,29 +1015,136 @@ static int qca_setup(struct hci_uart *hu)
>> 
>>  	if (speed)
>>  		host_set_baudrate(hu, speed);
>> +}
>> +
>> +static int qca_set_operating_speed(struct hci_uart *hu, u32 
>> *qca_baudrate)
>> +{
> 
> This is a bit convoluted, with the function setting the speed and
> returning it. I would suggest a qca_get_oper_speed() and
> qca_get_init_speed(), and just have them return the value instead of
> passing it through a pointer. You could then have a qca_set_speed()
> which converts the baudrate to the value the chip understands, calls
> qca_set_baudrate() and host_set_baudrate()
> 

[Bala]: will update.

>> +static int qca_setup(struct hci_uart *hu)
>> +{
>> +	struct hci_dev *hdev = hu->hdev;
>> +	struct qca_data *qca = hu->priv;
>> +	struct qca_serdev *qcadev;
>> +	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>> +	int ret;
>> +	int soc_ver;
>> +
>> +	qcadev = serdev_device_get_drvdata(hu->serdev);
>> +
>> +	switch (qcadev->btsoc_type) {
>> +	case BTQCA_CHEROKEE:
>> +		bt_dev_dbg(hdev, "setting up wcn3990");
>> +		/* Patch downloading has to be done without IBS mode */
>> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +		qca_set_init_speed(hu);
>> +		hci_uart_set_flow_control(hu, true);
>> +		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
>> +		if (ret) {
>> +			bt_dev_err(hdev, "failed to send power on command");
>> +			return ret;
>> +		}
>> +
>> +		/* Close and re-open the port */
> 
> As mentioned earlier, this comment doesn't provide any useful
> information, it's evident from the code. Rather explain *why* the
> close/open is needed.
> 
>> +		serdev_device_close(hu->serdev);
>> +		ret = serdev_device_open(hu->serdev);
>> +		if (ret) {
>> +			bt_dev_err(hdev, "failed to open port");
>> +			return ret;
>> +		}
>> +
>> +		qca_set_init_speed(hu);
>> +		hci_uart_set_flow_control(hu, false);
>> +		msleep(100);
> 
> Which step makes the delay necessary? I guess it's not enabling flow
> control but probably sending the power on command. If that is correct
> the delay should be done after the corresponding command.
> 
>> +		ret = qca_patch_ver_req(hdev, &soc_ver);
>> +		if (ret < 0 || soc_ver == 0) {
>> +			bt_dev_err(hdev, "Failed to get version 0x%x", ret);
> 
> Probably better: "Failed to get version: %d" since ret contains an
> errno and the problem wasn't that we couldn't get "version 0x<errno>".
> 
>> +			return ret;
>> +		}
>> +
>> +		bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
>> +		hci_uart_set_flow_control(hu, true);
> 
> Repeatedly switching on and off of flow control is a bit noisy, I
> wonder if it could be hidden in a wrapper. From the code it seems that
> flow control is disabled (true) for init speed and and enabled (false)
> for operating speed. If this is correct the hci_uart_set_flow_control()
> calls could be moved inside qca_set_oper/init_speed(). struct 
> qca_serdev
> could have a flag indicating if flow control is supported at all to
> skip the hci_uart_set_flow_control() calls for Rome.
> 
> Just an idea, no objections if you prefer to leave it as is and Marcel
> is ok with it.
> 

[Bala]: will update.

>> +		ret = qca_set_operating_speed(hu, &qca_baudrate);
>> +		if (ret)
>> +			return ret;
>> +		hci_uart_set_flow_control(hu, false);
>> +		/* Setup patch and NVM configurations */
>> +		ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
>> +
>> +		break;
>> +
>> +	default:
>> +		bt_dev_info(hdev, "ROME setup");
>> +
>> +		/* Patch downloading has to be done without IBS mode */
>> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +
>> +		/* Setup initial baudrate */
>> +		speed = 0;
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
> 
> If you introduce helpers to determine the init/oper speed these should
> also be used here. Probably best to do this in a separate patch.
> 

[Bala]: will update ROME related stuff in other patch.

>> +
>> +		if (speed)
>> +			host_set_baudrate(hu, speed);
>> +
>> +		/* Setup user speed if needed */
>> +		speed = 0;
>> +		if (hu->oper_speed)
>> +			speed = hu->oper_speed;
>> +		else if (hu->proto->oper_speed)
>> +			speed = hu->proto->oper_speed;
> 
> ditto
> 

[Bala]: will update.

>> +static void qca_disable_regulator(struct qca_vreg vregs,
>> +				  struct regulator *regulator)
>> +{
>> +	/* Disable the regulator if requested by user
>> +	 * or when fault to enable any regulator.
>> +	 */
> 
> This comment is not useful. The following code simply disables the
> regulator, why this is done is a question of the caller.
> 

[Bala]: will update.

>> +	regulator_disable(regulator);
>> +	regulator_set_voltage(regulator, 0, vregs.max_uV);
>> +	if (vregs.load_uA)
>> +		regulator_set_load(regulator, 0);
>> +
>> +}
>> +
>> +int qca_btsoc_power_setup(struct hci_uart *hu, bool on)
>> +{
>> +	struct qca_vreg *vregs;
>> +	struct regulator_bulk_data *vreg_bulk;
>> +	struct qca_serdev *qcadev;
>> +	int i, num_vregs, ret = 0;
>> +
>> +	qcadev = serdev_device_get_drvdata(hu->serdev);
>> +	if (!qcadev || !qcadev->bt_power || !qcadev->bt_power->vreg_data ||
>> +	    !qcadev->bt_power->vreg_bulk)
>> +		return -EINVAL;
>> +
>> +	vregs = qcadev->bt_power->vreg_data->vregs;
>> +	vreg_bulk = qcadev->bt_power->vreg_bulk;
>> +	num_vregs = qcadev->bt_power->vreg_data->num_vregs;
>> +	BT_DBG("on: %d", on);
>> +	if (on  && !qcadev->bt_power->vregs_on) {
>> +		for (i = 0; i < num_vregs; i++) {
>> +			ret = qca_enable_regulator(vregs[i],
>> +						   vreg_bulk[i].consumer);
>> +			if (ret)
>> +				break;
>> +		}
>> +		/* regulators failed */
> 
> Comment is not useful, BT_ERR below leaves things clear.
[Bala]: will update.

> 
>> +		if (ret) {
>> +			BT_ERR("failed to enable regulator:%s", vregs[i].name);
>> +			/* turn off regulators which are enabled */
>> +			for (i = i - 1; i >= 0; i--)
>> +				qca_disable_regulator(vregs[i],
>> +						      vreg_bulk[i].consumer);
>> +		} else {
>> +			qcadev->bt_power->vregs_on = true;
>> +		}
>> +	} else if (!on && qcadev->bt_power->vregs_on) {
>> +		/* turn off regulator in reverse order */
>> +		i = qcadev->bt_power->vreg_data->num_vregs - 1;
>> +		for ( ; i >= 0; i--)
>> +			qca_disable_regulator(vregs[i], vreg_bulk[i].consumer);
>> +		qcadev->bt_power->vregs_on = false;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int qca_init_regulators(struct qca_power *qca,
>> +			       const struct qca_vreg *vregs, size_t num_vregs)
>> +{
>> +	int i;
>> +
>> +	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
>> +				      sizeof(struct regulator_bulk_data),
>> +				      GFP_KERNEL);
>> +	if (!qca->vreg_bulk)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < num_vregs; i++)
>> +		qca->vreg_bulk[i].supply = vregs[i].name;
>> +
>> +	return devm_regulator_bulk_get(qca->dev, num_vregs, qca->vreg_bulk);
>> +}
>> +
>>  static int qca_serdev_probe(struct serdev_device *serdev)
>>  {
>>  	struct qca_serdev *qcadev;
>> +	const struct qca_vreg_data *data;
>>  	int err;
>> 
>>  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>> @@ -1014,34 +1301,72 @@ static int qca_serdev_probe(struct 
>> serdev_device *serdev)
>>  		return -ENOMEM;
>> 
>>  	qcadev->serdev_hu.serdev = serdev;
>> +	data = of_device_get_match_data(&serdev->dev);
>> +	if (data && data->soc_type == BTQCA_CHEROKEE)
>> +		qcadev->btsoc_type = BTQCA_CHEROKEE;
>> +	else
>> +		qcadev->btsoc_type = BTQCA_ROME;
>> +
>>  	serdev_device_set_drvdata(serdev, qcadev);
>> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
>> +		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>> +						sizeof(struct qca_power),
>> +						GFP_KERNEL);
>> +		if (!qcadev->bt_power)
>> +			return -ENOMEM;
>> +
>> +		qcadev->bt_power->dev = &serdev->dev;
>> +		qcadev->bt_power->vreg_data = data;
>> +		err = qca_init_regulators(qcadev->bt_power, data->vregs,
>> +					  data->num_vregs);
>> +		if (err) {
>> +			BT_ERR("Failed to init regulators:%d", err);
>> +			devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
>> +			devm_kfree(&serdev->dev, qcadev->bt_power);
> 
> Not necessary, the memory is allocated with devm_kzalloc(&serdev->dev,
> ...), therefore it is freed if probe() fails.

[Bala]: will update.

> 
>> +			goto out;
>> +		}
>> 
>> -	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);
>> -	}
>> +		qcadev->bt_power->vregs_on = false;
>> +		device_property_read_u32(&serdev->dev, "max-speed",
>> +					 &qcadev->oper_speed);
>> +		if (!qcadev->oper_speed)
>> +			BT_INFO("UART will pick default operating speed");
>> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
>> +		if (err) {
>> +			BT_ERR("wcn3990 serdev registration failed");
>> +			devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
>> +			devm_kfree(&serdev->dev, qcadev->bt_power);
> 
> no need to free memory.

[Bala]: will update.

> 
>> +			goto out;
>> +		}
>> +	} else {
>> +		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);
>> +		}
>> 
>> -	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);
>> -	}
>> +		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_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)
>> -		clk_disable_unprepare(qcadev->susclk);
>> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
>> +		if (err)
>> +			clk_disable_unprepare(qcadev->susclk);
>> +	}
>> +
>> +out:	return err;
>> 
>> -	return err;
>>  }
>> 
>>  static void qca_serdev_remove(struct serdev_device *serdev)
>> @@ -1050,11 +1375,17 @@ static void qca_serdev_remove(struct 
>> serdev_device *serdev)
>> 
>>  	hci_uart_unregister_device(&qcadev->serdev_hu);
>> 
>> -	clk_disable_unprepare(qcadev->susclk);
>> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
>> +		devm_kfree(&serdev->dev, qcadev->bt_power->vreg_bulk);
>> +		devm_kfree(&serdev->dev, qcadev->bt_power);
> 
> no need to free memory

[Bala]: will update.

Thanks for reviewing will update the about comments in next patch set.


-- 
Regards
Balakrishna.