* [PATCH v4 0/3] Enable Bluetooth functionality for WCN3990.
@ 2018-05-04 15:35 Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 1/3] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990 Balakrishna Godavarthi
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-04 15:35 UTC (permalink / raw)
To: marcel, johan.hedberg
Cc: linux-bluetooth, rtatiya, hemantg, linux-arm-msm, Balakrishna Godavarthi
These patches enable Bluetooth functionality for WCN3990 by
- adding support for voting of voltage regulators for WCN3990 attached to MSM
- Bluetooth firmware download support for WCN3990
- description of device tree bindings
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 (3):
dt-bindings: net: bluetooth: Add device tree bindings for QTI chip
wcn3990
Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
Bluetooth: btqca: Add wcn3990 firmware download support.
.../devicetree/bindings/net/qualcomm-bluetooth.txt | 31 +-
drivers/bluetooth/btqca.c | 45 +-
drivers/bluetooth/btqca.h | 19 +
drivers/bluetooth/hci_qca.c | 555 ++++++++++++++++++---
4 files changed, 575 insertions(+), 75 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 1/3] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990
2018-05-04 15:35 [PATCH v4 0/3] Enable Bluetooth functionality for WCN3990 Balakrishna Godavarthi
@ 2018-05-04 15:35 ` Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth " Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support Balakrishna Godavarthi
2 siblings, 0 replies; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-04 15:35 UTC (permalink / raw)
To: marcel, johan.hedberg
Cc: linux-bluetooth, rtatiya, hemantg, linux-arm-msm, Balakrishna Godavarthi
Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
.../devicetree/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 0ea18a5..63cae49 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.
+ - oper-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>;
+ oper-speed = <3200000>;
+ };
+};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-04 15:35 [PATCH v4 0/3] Enable Bluetooth functionality for WCN3990 Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 1/3] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990 Balakrishna Godavarthi
@ 2018-05-04 15:35 ` Balakrishna Godavarthi
2018-05-05 1:17 ` Matthias Kaehlcke
` (3 more replies)
2018-05-04 15:35 ` [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support Balakrishna Godavarthi
2 siblings, 4 replies; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-04 15:35 UTC (permalink / raw)
To: marcel, johan.hedberg
Cc: linux-bluetooth, rtatiya, hemantg, linux-arm-msm, Balakrishna Godavarthi
Add support to set voltage/current of various regulators
to power up/down Bluetooth chip wcn3990.
Add support to read baudrate from dts.
Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
drivers/bluetooth/hci_qca.c | 555 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 483 insertions(+), 72 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f05382b..075fab7 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...
@@ -35,6 +35,10 @@
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/serdev.h>
+#include <asm-generic/delay.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of_device.h>
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
@@ -119,12 +123,51 @@ struct qca_data {
u64 votes_off;
};
+enum btqca_soc_t {
+ BTQCA_INVALID = -1,
+ BTQCA_AR3002,
+ BTQCA_ROME,
+ BTQCA_CHEROKEE
+};
+
struct qca_serdev {
struct hci_uart serdev_hu;
struct gpio_desc *bt_en;
struct clk *susclk;
+ enum btqca_soc_t btsoc_type;
+ u32 init_speed;
+ u32 oper_speed;
+};
+
+/*
+ * voltage regulator information required for configuring the
+ * QCA bluetooth chipset
+ */
+struct btqca_vreg {
+ const char *name;
+ unsigned int min_v;
+ unsigned int max_v;
+ unsigned int load_ua;
+};
+
+struct btqca_vreg_data {
+ enum btqca_soc_t soc_type;
+ struct btqca_vreg *vregs;
+ size_t num_vregs;
};
+/*
+ * Platform data for the QCA bluetooth power driver.
+ */
+struct btqca_power {
+ struct device *dev;
+ const struct btqca_vreg_data *vreg_data;
+ struct regulator_bulk_data *vreg_bulk;
+ bool vreg_status;
+};
+
+static struct btqca_power *qca;
+
static void __serial_clock_on(struct tty_struct *tty)
{
/* TODO: Some chipset requires to enable UART clock on client
@@ -463,7 +506,12 @@ 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;
+ btqca_power_setup(true);
+ } else
+ gpiod_set_value_cansleep(qcadev->bt_en, 1);
}
BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
@@ -552,7 +600,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_cleanup(hu->hdev);
+ else
+ gpiod_set_value_cansleep(qcadev->bt_en, 0);
}
kfree_skb(qca->rx_skb);
@@ -872,6 +923,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 +939,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;
@@ -923,68 +976,292 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
hci_uart_set_baudrate(hu, speed);
}
+static int qca_send_poweron_cmd(struct hci_dev *hdev)
+{
+ struct hci_uart *hu = hci_get_drvdata(hdev);
+ struct qca_data *qca = hu->priv;
+ struct sk_buff *skb;
+ u8 cmd;
+
+ BT_DBG("%s sending power on command to btsoc", hdev->name);
+ /* By sending 0xFC host is trying to power up the soc */
+ cmd = CHEROKEE_POWERON_PULSE;
+ skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
+ if (!skb) {
+ BT_ERR("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 */
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ schedule_timeout(usecs_to_jiffies(100));
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ return 0;
+}
+
+static int qca_send_poweroff_cmd(struct hci_dev *hdev)
+{
+ struct hci_uart *hu = hci_get_drvdata(hdev);
+ struct qca_data *qca = hu->priv;
+ struct sk_buff *skb;
+ u8 cmd;
+
+ BT_DBG("%s sending power off command to btsoc", hdev->name);
+ /* By sending 0xC0 host is trying to power off the soc */
+ cmd = CHEROKEE_POWEROFF_PULSE;
+ skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
+ if (!skb) {
+ BT_ERR("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 */
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ schedule_timeout(usecs_to_jiffies(100));
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ return 0;
+}
+
+static int qca_serdev_open(struct hci_uart *hu)
+{
+ int ret = 0;
+
+ if (hu->serdev)
+ serdev_device_open(hu->serdev);
+ else {
+ bt_dev_err(hu->hdev, "open operation not supported");
+ ret = 1;
+ }
+
+ return ret;
+}
+
+int qca_btsoc_cleanup(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);
+ /* send 0xC0 command to btsoc before turning off regulators */
+ qca_send_poweroff_cmd(hdev);
+ /* turn off btsoc */
+ return btqca_power_setup(false);
+}
+
+static int qca_serdev_close(struct hci_uart *hu)
+{
+ int ret = 0;
+
+ if (hu->serdev)
+ serdev_device_close(hu->serdev);
+ else {
+ bt_dev_err(hu->hdev, "close operation not supported");
+ ret = 1;
+ }
+
+ return ret;
+}
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_info(hdev, "setting up wcn3990");
+ /* 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);
+ else {
+ bt_dev_err(hdev, "initial speed %u", speed);
+ return -1;
+ }
- bt_dev_info(hdev, "ROME setup");
+ /* disable flow control, as chip is still not turned on */
+ hci_uart_set_flow_control(hu, true);
+ /* send poweron command to btsoc */
+ ret = qca_send_poweron_cmd(hdev);
+ if (ret) {
+ bt_dev_err(hdev, "Failed to send power on command");
+ return ret;
+ }
- /* Patch downloading has to be done without IBS mode */
- clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+ /* close serial port */
+ ret = qca_serdev_close(hu);
+ if (ret)
+ return ret;
+ /* open serial port */
+ ret = qca_serdev_open(hu);
+ if (ret)
+ return ret;
- /* Setup initial baudrate */
- speed = 0;
- if (hu->init_speed)
- speed = hu->init_speed;
- else if (hu->proto->init_speed)
- speed = hu->proto->init_speed;
+ /* 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);
+ else {
+ BT_ERR("%s:initial speed %u", hdev->name, speed);
+ return -1;
+ }
- if (speed)
- host_set_baudrate(hu, speed);
+ /* Enable flow control */
+ hci_uart_set_flow_control(hu, false);
+ /* wait until flow control settled */
+ mdelay(100);
+ bt_dev_info(hdev, "wcn3990 Patch Version Request");
+ ret = rome_patch_ver_req(hdev, &soc_ver);
+ if (ret < 0 || soc_ver == 0) {
+ BT_ERR("%s: Failed to get version 0x%x", hdev->name,
+ ret);
+ return 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;
+ bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
+
+ /* clear flow control */
+ hci_uart_set_flow_control(hu, true);
+ /* set operating speed */
+ 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_ERR("%s:Failed to change the baud rate(%d)",
+ hdev->name, ret);
+ return ret;
+ }
+ if (speed)
+ host_set_baudrate(hu, speed);
+ else {
+ BT_ERR("%s:Error in setting operator speed:%u",
+ hdev->name, speed);
+ return -1;
+ }
+ }
- if (speed) {
- qca_baudrate = qca_get_baudrate_value(speed);
+ /* Set flow control */
+ hci_uart_set_flow_control(hu, false);
+ /*Setup patch and NVM configurations */
+ ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
+ if (!ret) {
+ set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+ qca_debugfs_init(hdev);
+ } else if (ret == -ENOENT) {
+ /* No patch/nvm-config found, run with original
+ * fw/config.
+ */
+ ret = 0;
+ } else if (ret == -EAGAIN) {
+ /*
+ * Userspace firmware loader will return -EAGAIN in
+ * case no patch/nvm-config is found, so run with
+ * original fw/config.
+ */
+ ret = 0;
+ }
- 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);
+ /* Setup wcn3990 bdaddr */
+ hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
+
+ return ret;
+
+ 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);
}
- 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);
- } else if (ret == -ENOENT) {
- /* No patch/nvm-config found, run with original fw/config */
- ret = 0;
- } else if (ret == -EAGAIN) {
- /*
- * Userspace firmware loader will return -EAGAIN in case no
- * patch/nvm-config is found, so run with original fw/config.
- */
- ret = 0;
- }
+ /* 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);
+ } else if (ret == -ENOENT) {
+ /* No patch/nvm-config found, run with original
+ * fw/config
+ */
+ ret = 0;
+ } else if (ret == -EAGAIN) {
+ /*
+ * Userspace firmware loader will return -EAGAIN in
+ * case no patch/nvm-config is found, so run with
+ * original fw/config.
+ */
+ ret = 0;
+ }
- /* Setup bdaddr */
- hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
+ /* Setup bdaddr */
+ hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
- return ret;
+ return ret;
+ }
}
static struct hci_uart_proto qca_proto = {
@@ -1002,44 +1279,174 @@ static int qca_setup(struct hci_uart *hu)
.dequeue = qca_dequeue,
};
+static const struct btqca_vreg_data cherokee_data = {
+ .soc_type = BTQCA_CHEROKEE,
+ .vregs = (struct btqca_vreg []) {
+ { "vddio", 1352000, 1352000, 0 },
+ { "vddxtal", 1904000, 2040000, 0 },
+ { "vddcore", 1800000, 1800000, 1 },
+ { "vddpa", 130400, 1304000, 1 },
+ { "vddldo", 3000000, 3312000, 1 },
+ },
+ .num_vregs = 5,
+};
+
+void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
+{
+ /* disable the regulator if requested by user
+ * or when fault in any regulator.
+ */
+ for (reg_nu = reg_nu - 1; reg_nu >= 0 ; reg_nu--) {
+ regulator_disable(qca->vreg_bulk[reg_nu].consumer);
+ regulator_set_voltage(qca->vreg_bulk[reg_nu].consumer, 0,
+ vregs[reg_nu].max_v);
+ if (vregs[reg_nu].load_ua)
+ regulator_set_load(qca->vreg_bulk[reg_nu].consumer, 0);
+ }
+}
+
+int btqca_power_setup(bool on)
+{
+ int ret = 0;
+ int i;
+ struct btqca_vreg *vregs;
+
+ if (!qca || !qca->vreg_data || !qca->vreg_bulk)
+ return -EINVAL;
+ vregs = qca->vreg_data->vregs;
+
+ BT_DBG("on: %d", on);
+ /* turn on if regualtors are off */
+ if (on == true && qca->vreg_status == false) {
+ qca->vreg_status = true;
+ for (i = 0; ret == 0 && i < qca->vreg_data->num_vregs; i++) {
+ regulator_set_voltage(qca->vreg_bulk[i].consumer,
+ vregs[i].min_v,
+ vregs[i].max_v);
+
+ if (vregs[i].load_ua)
+ regulator_set_load(qca->vreg_bulk[i].consumer,
+ vregs[i].load_ua);
+
+ ret = regulator_enable(qca->vreg_bulk[i].consumer);
+ }
+ } else if (on == false && qca->vreg_status == true) {
+ qca->vreg_status = false;
+ /* turn of regualtor in reverse order */
+ btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
+ }
+
+ /* regulatos fails to enable */
+ if (ret) {
+ qca->vreg_status = false;
+ BT_ERR("failed to enable regualtor:%s", vregs[i].name);
+ /* set regulator voltage and load to zero */
+ regulator_set_voltage(qca->vreg_bulk[i].consumer,
+ 0, vregs[i].max_v);
+ if (vregs[i].load_ua)
+ regulator_set_load(qca->vreg_bulk[i].consumer, 0);
+ /* turn off regulators which are enabled */
+ btqca_disable_regulators(i+1, vregs);
+ }
+
+ return ret;
+}
+
+static int init_regulators(struct btqca_power *qca,
+ const struct btqca_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;
- int err;
+ const struct btqca_vreg_data *data;
+ int err = 0;
qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
if (!qcadev)
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) {
+ qca = kzalloc(sizeof(struct btqca_power), GFP_KERNEL);
+ if (!qca)
+ return -ENOMEM;
+
+ qca->dev = &serdev->dev;
+ qca->vreg_data = data;
+ err = init_regulators(qca, data->vregs, data->num_vregs);
+ if (err) {
+ BT_ERR("Failed to init regualtors:%d", err);
+ kfree(qca);
+ 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);
- }
+ /* set voltage regulator status as false */
+ qca->vreg_status = false;
+ /* get operating speed */
+ device_property_read_u32(&serdev->dev, "oper-speed",
+ &qcadev->oper_speed);
+ device_property_read_u32(&serdev->dev, "init-speed",
+ &qcadev->init_speed);
+ if (!qcadev->oper_speed)
+ BT_INFO("DTS entry for operating speed is disabled");
+ err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+ if (err) {
+ BT_ERR("wcn3990 serdev registration failed");
+ kfree(qca);
+ goto out;
+ }
- 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);
- }
+ } 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);
+ }
- err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
- if (err)
- return err;
+ 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_prepare_enable(qcadev->susclk);
- if (err)
- return err;
+ err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
+ if (err)
+ return err;
- err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
- if (err)
- clk_disable_unprepare(qcadev->susclk);
+ 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);
+ }
+
+out: return err;
- return err;
}
static void qca_serdev_remove(struct serdev_device *serdev)
@@ -1047,12 +1454,16 @@ 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);
+ if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
+ btqca_power_setup(false);
+ kfree(qca);
+ } 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 = &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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support.
2018-05-04 15:35 [PATCH v4 0/3] Enable Bluetooth functionality for WCN3990 Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 1/3] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990 Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth " Balakrishna Godavarthi
@ 2018-05-04 15:35 ` Balakrishna Godavarthi
2018-05-05 19:11 ` kbuild test robot
2018-05-05 19:14 ` kbuild test robot
2 siblings, 2 replies; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-04 15:35 UTC (permalink / raw)
To: marcel, johan.hedberg
Cc: linux-bluetooth, rtatiya, hemantg, linux-arm-msm, Balakrishna Godavarthi
This patch enables the RAM and NV patch download for wcn3990.
Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
drivers/bluetooth/btqca.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 19 +++++++++++++++++++
2 files changed, 63 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 8219816..12b4dd3 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 rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
{
struct sk_buff *skb;
struct edl_event_hdr *edl;
@@ -380,6 +380,49 @@ 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;
+
+ /* we are using the existing funciton of ROME,
+ * instead of duplicating the function for wcn3990.
+ */
+ config.user_baud_rate = baudrate;
+ /* Download rampatch file */
+ config.type = TLV_TYPE_PATCH;
+ snprintf(config.fwname, sizeof(config.fwname), "qca/rampatch_%08x.tlv",
+ *soc_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/nvm_%08x.bin",
+ *soc_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 13d77fd..2b41987 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,10 +127,16 @@ struct tlv_type_hdr {
__u8 data[0];
} __packed;
+int qca_btsoc_cleanup(struct hci_dev *hdev);
+int btqca_power_setup(bool on);
+
#if IS_ENABLED(CONFIG_BT_QCA)
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_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+ u32 *soc_ver);
+int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version);
#else
@@ -141,4 +150,14 @@ static inline int qca_uart_setup_rome(struct hci_dev *hdev, int speed)
return -EOPNOTSUPP;
}
+static int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+ u32 *soc_ver)
+{
+ return -EOPNOTSUPP;
+}
+
+static int rome_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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-04 15:35 ` [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth " Balakrishna Godavarthi
@ 2018-05-05 1:17 ` Matthias Kaehlcke
2018-05-09 8:56 ` Balakrishna Godavarthi
` (2 more replies)
2018-05-05 19:21 ` kbuild test robot
` (2 subsequent siblings)
3 siblings, 3 replies; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-05-05 1:17 UTC (permalink / raw)
To: Balakrishna Godavarthi
Cc: marcel, johan.hedberg, linux-bluetooth, rtatiya, hemantg, linux-arm-msm
Hi,
On Fri, May 04, 2018 at 09:05:05PM +0530, Balakrishna Godavarthi wrote:
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
> Add support to read baudrate from dts.
>
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> drivers/bluetooth/hci_qca.c | 555 ++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 483 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f05382b..075fab7 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...
> @@ -35,6 +35,10 @@
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/serdev.h>
> +#include <asm-generic/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_device.h>
AFAIK the convention is to order includes alphabetically.
> +static struct btqca_power *qca;
This limits the driver to a single instance. This is probably the most
common use case, but in general it's preferable not to have this kind
of limitations.
> static void __serial_clock_on(struct tty_struct *tty)
> {
> /* TODO: Some chipset requires to enable UART clock on client
> @@ -463,7 +506,12 @@ 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;
> + btqca_power_setup(true);
> + } else
> + gpiod_set_value_cansleep(qcadev->bt_en, 1);
Use curly braces for the else branch too.
> +static int qca_send_poweron_cmd(struct hci_dev *hdev)
> +{
> + struct hci_uart *hu = hci_get_drvdata(hdev);
> + struct qca_data *qca = hu->priv;
> + struct sk_buff *skb;
> + u8 cmd;
> +
> + BT_DBG("%s sending power on command to btsoc", hdev->name);
Use bt_dev_dbg(), same for other BT_XXX().
> + /* By sending 0xFC host is trying to power up the soc */
nit: SoC, same a few lines below.
> + cmd = CHEROKEE_POWERON_PULSE;
> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
Use GFP_KERNEL, the code sleeps a few lines below, hence it must
definitely not be called in atomic context.
> + if (!skb) {
> + BT_ERR("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 */
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + schedule_timeout(usecs_to_jiffies(100));
> + set_current_state(TASK_INTERRUPTIBLE);
> +
> + return 0;
> +}
> +
> +static int qca_send_poweroff_cmd(struct hci_dev *hdev)
> +{
> + struct hci_uart *hu = hci_get_drvdata(hdev);
> + struct qca_data *qca = hu->priv;
> + struct sk_buff *skb;
> + u8 cmd;
> +
> + BT_DBG("%s sending power off command to btsoc", hdev->name);
> + /* By sending 0xC0 host is trying to power off the soc */
> + cmd = CHEROKEE_POWEROFF_PULSE;
> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
> + if (!skb) {
> + BT_ERR("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 */
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + schedule_timeout(usecs_to_jiffies(100));
> + set_current_state(TASK_INTERRUPTIBLE);
> +
> + return 0;
> +}
This function is almost a clone of qca_send_poweron_cmd(), move the
common code to something like qca_send_cmd() and call it from
qca_send_poweron/off_cmd().
> +static int qca_serdev_open(struct hci_uart *hu)
> +{
> + int ret = 0;
> +
> + if (hu->serdev)
> + serdev_device_open(hu->serdev);
Check return value.
Add curly braces to the if branch too.
> +static int qca_serdev_close(struct hci_uart *hu)
> +{
> + int ret = 0;
> +
> + if (hu->serdev)
> + serdev_device_close(hu->serdev);
Add curly braces to the if branch too.
> 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_info(hdev, "setting up wcn3990");
> + /* 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);
Add curly braces.
> + else {
> + bt_dev_err(hdev, "initial speed %u", speed);
> + return -1;
> + }
>
> - bt_dev_info(hdev, "ROME setup");
> + /* disable flow control, as chip is still not turned on */
> + hci_uart_set_flow_control(hu, true);
The interface of this function is confusing. enable = true disables
flow control ... Not the fault of this driver though :)
> + /* send poweron command to btsoc */
> + ret = qca_send_poweron_cmd(hdev);
> + if (ret) {
> + bt_dev_err(hdev, "Failed to send power on command");
> + return ret;
> + }
>
> - /* Patch downloading has to be done without IBS mode */
> - clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> + /* close serial port */
> + ret = qca_serdev_close(hu);
> + if (ret)
> + return ret;
> + /* open serial port */
> + ret = qca_serdev_open(hu);
> + if (ret)
> + return ret;
>
> - /* Setup initial baudrate */
> - speed = 0;
> - if (hu->init_speed)
> - speed = hu->init_speed;
> - else if (hu->proto->init_speed)
> - speed = hu->proto->init_speed;
> + /* 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);
Add curly braces.
> + else {
> + BT_ERR("%s:initial speed %u", hdev->name, speed);
> + return -1;
> + }
>
> - if (speed)
> - host_set_baudrate(hu, speed);
> + /* Enable flow control */
> + hci_uart_set_flow_control(hu, false);
> + /* wait until flow control settled */
> + mdelay(100);
A busy wait of 100ms doesn't seem a good idea. Use msleep()
instead. Is it really necessary to wait that long?
> + bt_dev_info(hdev, "wcn3990 Patch Version Request");
> + ret = rome_patch_ver_req(hdev, &soc_ver);
> + if (ret < 0 || soc_ver == 0) {
> + BT_ERR("%s: Failed to get version 0x%x", hdev->name,
> + ret);
> + return 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;
> + bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
> +
> + /* clear flow control */
> + hci_uart_set_flow_control(hu, true);
> + /* set operating speed */
> + 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_ERR("%s:Failed to change the baud rate(%d)",
> + hdev->name, ret);
> + return ret;
> + }
> + if (speed)
> + host_set_baudrate(hu, speed);
Add curly braces.
> + else {
> + BT_ERR("%s:Error in setting operator speed:%u",
> + hdev->name, speed);
> + return -1;
> + }
> + }
>
> - if (speed) {
> - qca_baudrate = qca_get_baudrate_value(speed);
> + /* Set flow control */
> + hci_uart_set_flow_control(hu, false);
> + /*Setup patch and NVM configurations */
Add blank before 'Setup' and remove one before 'NVM'.
> + ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
> + if (!ret) {
> + set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> + qca_debugfs_init(hdev);
> + } else if (ret == -ENOENT) {
> + /* No patch/nvm-config found, run with original
> + * fw/config.
> + */
> + ret = 0;
> + } else if (ret == -EAGAIN) {
> + /*
> + * Userspace firmware loader will return -EAGAIN in
> + * case no patch/nvm-config is found, so run with
> + * original fw/config.
> + */
> + ret = 0;
> + }
>
> - 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);
> + /* Setup wcn3990 bdaddr */
> + hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
> +
> + return ret;
> +
> + default:
What follows looks similar to the Cherokee path. I didn't look at all
the details, but it's probably possible to share some more code.
> + 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;
Fix indentation.
> +static const struct btqca_vreg_data cherokee_data = {
> + .soc_type = BTQCA_CHEROKEE,
> + .vregs = (struct btqca_vreg []) {
> + { "vddio", 1352000, 1352000, 0 },
> + { "vddxtal", 1904000, 2040000, 0 },
> + { "vddcore", 1800000, 1800000, 1 },
> + { "vddpa", 130400, 1304000, 1 },
0 missing for min_v?
> + { "vddldo", 3000000, 3312000, 1 },
> + },
> + .num_vregs = 5,
> +};
> +
> +void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
> +{
> + /* disable the regulator if requested by user
> + * or when fault in any regulator.
> + */
> + for (reg_nu = reg_nu - 1; reg_nu >= 0 ; reg_nu--) {
Better use a local variable for iteration instead of the function parameter
> +int btqca_power_setup(bool on)
> +{
> + int ret = 0;
> + int i;
> + struct btqca_vreg *vregs;
> +
> + if (!qca || !qca->vreg_data || !qca->vreg_bulk)
> + return -EINVAL;
> + vregs = qca->vreg_data->vregs;
> +
> + BT_DBG("on: %d", on);
> + /* turn on if regualtors are off */
> + if (on == true && qca->vreg_status == false) {
if (on && !qca->vreg_status)
You might also consider a more expressive name instead of vreg_status,
something like vregs_on.
> + qca->vreg_status = true;
> + for (i = 0; ret == 0 && i < qca->vreg_data->num_vregs; i++) {
Better check ret inline and break when an error is encountered
> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
> + vregs[i].min_v,
> + vregs[i].max_v);
Check return value.
> +
> + if (vregs[i].load_ua)
> + regulator_set_load(qca->vreg_bulk[i].consumer,
> + vregs[i].load_ua);
Check return value.
> + ret = regulator_enable(qca->vreg_bulk[i].consumer);
> + }
Fix indentation.
> + } else if (on == false && qca->vreg_status == true) {
(!on && qca->vreg_status)
> + qca->vreg_status = false;
> + /* turn of regualtor in reverse order */
'off, regulator'
> + btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
> + }
> +
> + /* regulatos fails to enable */
'regulators failed'
> + if (ret) {
> + qca->vreg_status = false;
> + BT_ERR("failed to enable regualtor:%s", vregs[i].name);
'regulator'
> + /* set regulator voltage and load to zero */
> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
> + 0, vregs[i].max_v);
check return value.
> static int qca_serdev_probe(struct serdev_device *serdev)
> {
> struct qca_serdev *qcadev;
> - int err;
> + const struct btqca_vreg_data *data;
> + int err = 0;
>
> qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> if (!qcadev)
> 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) {
> + qca = kzalloc(sizeof(struct btqca_power), GFP_KERNEL);
Use devm_kzalloc()
> + if (!qca)
> + return -ENOMEM;
> +
> + qca->dev = &serdev->dev;
> + qca->vreg_data = data;
> + err = init_regulators(qca, data->vregs, data->num_vregs);
> + if (err) {
> + BT_ERR("Failed to init regualtors:%d", err);
'regulators'
> + kfree(qca);
> + 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);
> - }
> + /* set voltage regulator status as false */
> + qca->vreg_status = false;
> + /* get operating speed */
> + device_property_read_u32(&serdev->dev, "oper-speed",
> + &qcadev->oper_speed);
> + device_property_read_u32(&serdev->dev, "init-speed",
> + &qcadev->init_speed);
> + if (!qcadev->oper_speed)
> + BT_INFO("DTS entry for operating speed is
> - disabled");
The message isn't very clear. The entry isn't disabled, it doesn't exist.
> static void qca_serdev_remove(struct serdev_device *serdev)
> @@ -1047,12 +1454,16 @@ 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);
> + if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
> + btqca_power_setup(false);
> + kfree(qca);
> + } else
> + clk_disable_unprepare(qcadev->susclk);
Add curly braces.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support.
2018-05-04 15:35 ` [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support Balakrishna Godavarthi
@ 2018-05-05 19:11 ` kbuild test robot
2018-05-05 19:14 ` kbuild test robot
1 sibling, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2018-05-05 19:11 UTC (permalink / raw)
To: Balakrishna Godavarthi
Cc: kbuild-all, marcel, johan.hedberg, linux-bluetooth, rtatiya,
hemantg, linux-arm-msm, Balakrishna Godavarthi
[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]
Hi Balakrishna,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on next-20180504]
[cannot apply to bluetooth/master v4.17-rc3]
[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/dt-bindings-net-bluetooth-Add-device-tree-bindings-for-QTI-chip-wcn3990/20180506-004430
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All errors (new ones prefixed by >>):
`.exit.data' referenced in section `.exit.text' of drivers/tty/n_hdlc.o: defined in discarded section `.exit.data' of drivers/tty/n_hdlc.o
`.exit.data' referenced in section `.exit.text' of drivers/tty/n_hdlc.o: defined in discarded section `.exit.data' of drivers/tty/n_hdlc.o
`.exit.data' referenced in section `.exit.text' of drivers/tty/n_hdlc.o: defined in discarded section `.exit.data' of drivers/tty/n_hdlc.o
`.exit.data' referenced in section `.exit.text' of drivers/tty/n_hdlc.o: defined in discarded section `.exit.data' of drivers/tty/n_hdlc.o
drivers/bluetooth/hci_qca.o: In function `qca_setup':
>> hci_qca.c:(.text+0x1004): undefined reference to `__bad_udelay'
---
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: 53282 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support.
2018-05-04 15:35 ` [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support Balakrishna Godavarthi
2018-05-05 19:11 ` kbuild test robot
@ 2018-05-05 19:14 ` kbuild test robot
1 sibling, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2018-05-05 19:14 UTC (permalink / raw)
To: Balakrishna Godavarthi
Cc: kbuild-all, marcel, johan.hedberg, linux-bluetooth, rtatiya,
hemantg, linux-arm-msm, Balakrishna Godavarthi
[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]
Hi Balakrishna,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on next-20180504]
[cannot apply to bluetooth/master v4.17-rc3]
[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/dt-bindings-net-bluetooth-Add-device-tree-bindings-for-QTI-chip-wcn3990/20180506-004430
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-randconfig-s0-05060048 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "rome_patch_ver_req" [drivers/bluetooth/hci_uart.ko] undefined!
---
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: 26555 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-04 15:35 ` [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth " Balakrishna Godavarthi
2018-05-05 1:17 ` Matthias Kaehlcke
@ 2018-05-05 19:21 ` kbuild test robot
2018-05-05 19:45 ` kbuild test robot
2018-05-05 20:11 ` [RFC PATCH] Bluetooth: hci_qca: btqca_disable_regulators() can be static kbuild test robot
3 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2018-05-05 19:21 UTC (permalink / raw)
To: Balakrishna Godavarthi
Cc: kbuild-all, marcel, johan.hedberg, linux-bluetooth, rtatiya,
hemantg, linux-arm-msm, Balakrishna Godavarthi
[-- Attachment #1: Type: text/plain, Size: 10191 bytes --]
Hi Balakrishna,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on next-20180504]
[cannot apply to bluetooth/master v4.17-rc3]
[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/dt-bindings-net-bluetooth-Add-device-tree-bindings-for-QTI-chip-wcn3990/20180506-004430
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-randconfig-x014-201818 (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
Note: the linux-review/Balakrishna-Godavarthi/dt-bindings-net-bluetooth-Add-device-tree-bindings-for-QTI-chip-wcn3990/20180506-004430 HEAD 8ea35b4e8656709a14f0170188ef9004ac96b4ac builds fine.
It only hurts bisectibility.
All error/warnings (new ones prefixed by >>):
drivers//bluetooth/hci_qca.c: In function 'qca_open':
>> drivers//bluetooth/hci_qca.c:512:4: error: implicit declaration of function 'btqca_power_setup'; did you mean 'ether_setup'? [-Werror=implicit-function-declaration]
btqca_power_setup(true);
^~~~~~~~~~~~~~~~~
ether_setup
drivers//bluetooth/hci_qca.c: In function 'qca_close':
>> drivers//bluetooth/hci_qca.c:604:4: error: implicit declaration of function 'qca_btsoc_cleanup'; did you mean 'hci_sock_cleanup'? [-Werror=implicit-function-declaration]
qca_btsoc_cleanup(hu->hdev);
^~~~~~~~~~~~~~~~~
hci_sock_cleanup
drivers//bluetooth/hci_qca.c: In function 'qca_send_poweron_cmd':
>> drivers//bluetooth/hci_qca.c:988:8: error: 'CHEROKEE_POWERON_PULSE' undeclared (first use in this function)
cmd = CHEROKEE_POWERON_PULSE;
^~~~~~~~~~~~~~~~~~~~~~
drivers//bluetooth/hci_qca.c:988:8: note: each undeclared identifier is reported only once for each function it appears in
drivers//bluetooth/hci_qca.c: In function 'qca_send_poweroff_cmd':
>> drivers//bluetooth/hci_qca.c:1018:8: error: 'CHEROKEE_POWEROFF_PULSE' undeclared (first use in this function)
cmd = CHEROKEE_POWEROFF_PULSE;
^~~~~~~~~~~~~~~~~~~~~~~
drivers//bluetooth/hci_qca.c: In function 'qca_setup':
>> drivers//bluetooth/hci_qca.c:1144:9: error: implicit declaration of function 'rome_patch_ver_req'; did you mean 'blk_path_error'? [-Werror=implicit-function-declaration]
ret = rome_patch_ver_req(hdev, &soc_ver);
^~~~~~~~~~~~~~~~~~
blk_path_error
>> drivers//bluetooth/hci_qca.c:1182:9: error: implicit declaration of function 'qca_uart_setup_cherokee'; did you mean 'qca_uart_setup_rome'? [-Werror=implicit-function-declaration]
ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
^~~~~~~~~~~~~~~~~~~~~~~
qca_uart_setup_rome
drivers//bluetooth/hci_qca.c: At top level:
>> drivers//bluetooth/hci_qca.c:1308:5: error: conflicting types for 'btqca_power_setup'
int btqca_power_setup(bool on)
^~~~~~~~~~~~~~~~~
drivers//bluetooth/hci_qca.c:1309:1: note: an argument type that has a default promotion can't match an empty parameter name list declaration
{
^
drivers//bluetooth/hci_qca.c:512:4: note: previous implicit declaration of 'btqca_power_setup' was here
btqca_power_setup(true);
^~~~~~~~~~~~~~~~~
drivers//bluetooth/hci_qca.c: In function 'btqca_power_setup':
>> drivers//bluetooth/hci_qca.c:1346:3: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (vregs[i].load_ua)
^~
drivers//bluetooth/hci_qca.c:1349:4: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
btqca_disable_regulators(i+1, vregs);
^~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +512 drivers//bluetooth/hci_qca.c
442
443 /* Initialize protocol */
444 static int qca_open(struct hci_uart *hu)
445 {
446 struct qca_serdev *qcadev;
447 struct qca_data *qca;
448
449 BT_DBG("hu %p qca_open", hu);
450
451 qca = kzalloc(sizeof(struct qca_data), GFP_ATOMIC);
452 if (!qca)
453 return -ENOMEM;
454
455 skb_queue_head_init(&qca->txq);
456 skb_queue_head_init(&qca->tx_wait_q);
457 spin_lock_init(&qca->hci_ibs_lock);
458 qca->workqueue = alloc_ordered_workqueue("qca_wq", 0);
459 if (!qca->workqueue) {
460 BT_ERR("QCA Workqueue not initialized properly");
461 kfree(qca);
462 return -ENOMEM;
463 }
464
465 INIT_WORK(&qca->ws_awake_rx, qca_wq_awake_rx);
466 INIT_WORK(&qca->ws_awake_device, qca_wq_awake_device);
467 INIT_WORK(&qca->ws_rx_vote_off, qca_wq_serial_rx_clock_vote_off);
468 INIT_WORK(&qca->ws_tx_vote_off, qca_wq_serial_tx_clock_vote_off);
469
470 qca->hu = hu;
471
472 /* Assume we start with both sides asleep -- extra wakes OK */
473 qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
474 qca->rx_ibs_state = HCI_IBS_RX_ASLEEP;
475
476 /* clocks actually on, but we start votes off */
477 qca->tx_vote = false;
478 qca->rx_vote = false;
479 qca->flags = 0;
480
481 qca->ibs_sent_wacks = 0;
482 qca->ibs_sent_slps = 0;
483 qca->ibs_sent_wakes = 0;
484 qca->ibs_recv_wacks = 0;
485 qca->ibs_recv_slps = 0;
486 qca->ibs_recv_wakes = 0;
487 qca->vote_last_jif = jiffies;
488 qca->vote_on_ms = 0;
489 qca->vote_off_ms = 0;
490 qca->votes_on = 0;
491 qca->votes_off = 0;
492 qca->tx_votes_on = 0;
493 qca->tx_votes_off = 0;
494 qca->rx_votes_on = 0;
495 qca->rx_votes_off = 0;
496
497 hu->priv = qca;
498
499 timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0);
500 qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS;
501
502 timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
503 qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
504
505 if (hu->serdev) {
506 serdev_device_open(hu->serdev);
507
508 qcadev = serdev_device_get_drvdata(hu->serdev);
509 if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
510 hu->init_speed = qcadev->init_speed;
511 hu->oper_speed = qcadev->oper_speed;
> 512 btqca_power_setup(true);
513 } else
514 gpiod_set_value_cansleep(qcadev->bt_en, 1);
515 }
516
517 BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
518 qca->tx_idle_delay, qca->wake_retrans);
519
520 return 0;
521 }
522
523 static void qca_debugfs_init(struct hci_dev *hdev)
524 {
525 struct hci_uart *hu = hci_get_drvdata(hdev);
526 struct qca_data *qca = hu->priv;
527 struct dentry *ibs_dir;
528 umode_t mode;
529
530 if (!hdev->debugfs)
531 return;
532
533 ibs_dir = debugfs_create_dir("ibs", hdev->debugfs);
534
535 /* read only */
536 mode = S_IRUGO;
537 debugfs_create_u8("tx_ibs_state", mode, ibs_dir, &qca->tx_ibs_state);
538 debugfs_create_u8("rx_ibs_state", mode, ibs_dir, &qca->rx_ibs_state);
539 debugfs_create_u64("ibs_sent_sleeps", mode, ibs_dir,
540 &qca->ibs_sent_slps);
541 debugfs_create_u64("ibs_sent_wakes", mode, ibs_dir,
542 &qca->ibs_sent_wakes);
543 debugfs_create_u64("ibs_sent_wake_acks", mode, ibs_dir,
544 &qca->ibs_sent_wacks);
545 debugfs_create_u64("ibs_recv_sleeps", mode, ibs_dir,
546 &qca->ibs_recv_slps);
547 debugfs_create_u64("ibs_recv_wakes", mode, ibs_dir,
548 &qca->ibs_recv_wakes);
549 debugfs_create_u64("ibs_recv_wake_acks", mode, ibs_dir,
550 &qca->ibs_recv_wacks);
551 debugfs_create_bool("tx_vote", mode, ibs_dir, &qca->tx_vote);
552 debugfs_create_u64("tx_votes_on", mode, ibs_dir, &qca->tx_votes_on);
553 debugfs_create_u64("tx_votes_off", mode, ibs_dir, &qca->tx_votes_off);
554 debugfs_create_bool("rx_vote", mode, ibs_dir, &qca->rx_vote);
555 debugfs_create_u64("rx_votes_on", mode, ibs_dir, &qca->rx_votes_on);
556 debugfs_create_u64("rx_votes_off", mode, ibs_dir, &qca->rx_votes_off);
557 debugfs_create_u64("votes_on", mode, ibs_dir, &qca->votes_on);
558 debugfs_create_u64("votes_off", mode, ibs_dir, &qca->votes_off);
559 debugfs_create_u32("vote_on_ms", mode, ibs_dir, &qca->vote_on_ms);
560 debugfs_create_u32("vote_off_ms", mode, ibs_dir, &qca->vote_off_ms);
561
562 /* read/write */
563 mode = S_IRUGO | S_IWUSR;
564 debugfs_create_u32("wake_retrans", mode, ibs_dir, &qca->wake_retrans);
565 debugfs_create_u32("tx_idle_delay", mode, ibs_dir,
566 &qca->tx_idle_delay);
567 }
568
569 /* Flush protocol data */
570 static int qca_flush(struct hci_uart *hu)
571 {
572 struct qca_data *qca = hu->priv;
573
574 BT_DBG("hu %p qca flush", hu);
575
576 skb_queue_purge(&qca->tx_wait_q);
577 skb_queue_purge(&qca->txq);
578
579 return 0;
580 }
581
582 /* Close protocol */
583 static int qca_close(struct hci_uart *hu)
584 {
585 struct qca_serdev *qcadev;
586 struct qca_data *qca = hu->priv;
587
588 BT_DBG("hu %p qca close", hu);
589
590 serial_clock_vote(HCI_IBS_VOTE_STATS_UPDATE, hu);
591
592 skb_queue_purge(&qca->tx_wait_q);
593 skb_queue_purge(&qca->txq);
594 del_timer(&qca->tx_idle_timer);
595 del_timer(&qca->wake_retrans_timer);
596 destroy_workqueue(qca->workqueue);
597 qca->hu = NULL;
598
599 if (hu->serdev) {
600 serdev_device_close(hu->serdev);
601
602 qcadev = serdev_device_get_drvdata(hu->serdev);
603 if (qcadev->btsoc_type == BTQCA_CHEROKEE)
> 604 qca_btsoc_cleanup(hu->hdev);
605 else
606 gpiod_set_value_cansleep(qcadev->bt_en, 0);
607 }
608
609 kfree_skb(qca->rx_skb);
610
611 hu->priv = NULL;
612
613 kfree(qca);
614
615 return 0;
616 }
617
---
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: 26941 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-04 15:35 ` [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth " Balakrishna Godavarthi
2018-05-05 1:17 ` Matthias Kaehlcke
2018-05-05 19:21 ` kbuild test robot
@ 2018-05-05 19:45 ` kbuild test robot
2018-05-05 20:11 ` [RFC PATCH] Bluetooth: hci_qca: btqca_disable_regulators() can be static kbuild test robot
3 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2018-05-05 19:45 UTC (permalink / raw)
To: Balakrishna Godavarthi
Cc: kbuild-all, marcel, johan.hedberg, linux-bluetooth, rtatiya,
hemantg, linux-arm-msm, Balakrishna Godavarthi
[-- Attachment #1: Type: text/plain, Size: 5950 bytes --]
Hi Balakrishna,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on next-20180504]
[cannot apply to bluetooth/master v4.17-rc3]
[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/dt-bindings-net-bluetooth-Add-device-tree-bindings-for-QTI-chip-wcn3990/20180506-004430
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-randconfig-x017-201818 (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 >>):
drivers//bluetooth/hci_qca.c: In function 'qca_open':
drivers//bluetooth/hci_qca.c:512:4: error: implicit declaration of function 'btqca_power_setup'; did you mean 'ether_setup'? [-Werror=implicit-function-declaration]
btqca_power_setup(true);
^~~~~~~~~~~~~~~~~
ether_setup
drivers//bluetooth/hci_qca.c: In function 'qca_close':
drivers//bluetooth/hci_qca.c:604:4: error: implicit declaration of function 'qca_btsoc_cleanup'; did you mean 'hci_sock_cleanup'? [-Werror=implicit-function-declaration]
qca_btsoc_cleanup(hu->hdev);
^~~~~~~~~~~~~~~~~
hci_sock_cleanup
drivers//bluetooth/hci_qca.c: In function 'qca_send_poweron_cmd':
drivers//bluetooth/hci_qca.c:988:8: error: 'CHEROKEE_POWERON_PULSE' undeclared (first use in this function)
cmd = CHEROKEE_POWERON_PULSE;
^~~~~~~~~~~~~~~~~~~~~~
drivers//bluetooth/hci_qca.c:988:8: note: each undeclared identifier is reported only once for each function it appears in
drivers//bluetooth/hci_qca.c: In function 'qca_send_poweroff_cmd':
drivers//bluetooth/hci_qca.c:1018:8: error: 'CHEROKEE_POWEROFF_PULSE' undeclared (first use in this function)
cmd = CHEROKEE_POWEROFF_PULSE;
^~~~~~~~~~~~~~~~~~~~~~~
drivers//bluetooth/hci_qca.c: In function 'qca_setup':
drivers//bluetooth/hci_qca.c:1144:9: error: implicit declaration of function 'rome_patch_ver_req'; did you mean 'blk_path_error'? [-Werror=implicit-function-declaration]
ret = rome_patch_ver_req(hdev, &soc_ver);
^~~~~~~~~~~~~~~~~~
blk_path_error
drivers//bluetooth/hci_qca.c:1182:9: error: implicit declaration of function 'qca_uart_setup_cherokee'; did you mean 'qca_uart_setup_rome'? [-Werror=implicit-function-declaration]
ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
^~~~~~~~~~~~~~~~~~~~~~~
qca_uart_setup_rome
drivers//bluetooth/hci_qca.c: At top level:
drivers//bluetooth/hci_qca.c:1308:5: error: conflicting types for 'btqca_power_setup'
int btqca_power_setup(bool on)
^~~~~~~~~~~~~~~~~
drivers//bluetooth/hci_qca.c:1309:1: note: an argument type that has a default promotion can't match an empty parameter name list declaration
{
^
drivers//bluetooth/hci_qca.c:512:4: note: previous implicit declaration of 'btqca_power_setup' was here
btqca_power_setup(true);
^~~~~~~~~~~~~~~~~
In file included from include/linux/kernel.h:10:0,
from drivers//bluetooth/hci_qca.c:31:
drivers//bluetooth/hci_qca.c: In function 'btqca_power_setup':
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> drivers//bluetooth/hci_qca.c:1346:3: note: in expansion of macro 'if'
if (vregs[i].load_ua)
^~
drivers//bluetooth/hci_qca.c:1349:4: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
btqca_disable_regulators(i+1, vregs);
^~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/if +1346 drivers//bluetooth/hci_qca.c
1307
1308 int btqca_power_setup(bool on)
1309 {
1310 int ret = 0;
1311 int i;
1312 struct btqca_vreg *vregs;
1313
1314 if (!qca || !qca->vreg_data || !qca->vreg_bulk)
1315 return -EINVAL;
1316 vregs = qca->vreg_data->vregs;
1317
1318 BT_DBG("on: %d", on);
1319 /* turn on if regualtors are off */
1320 if (on == true && qca->vreg_status == false) {
1321 qca->vreg_status = true;
1322 for (i = 0; ret == 0 && i < qca->vreg_data->num_vregs; i++) {
1323 regulator_set_voltage(qca->vreg_bulk[i].consumer,
1324 vregs[i].min_v,
1325 vregs[i].max_v);
1326
1327 if (vregs[i].load_ua)
1328 regulator_set_load(qca->vreg_bulk[i].consumer,
1329 vregs[i].load_ua);
1330
1331 ret = regulator_enable(qca->vreg_bulk[i].consumer);
1332 }
1333 } else if (on == false && qca->vreg_status == true) {
1334 qca->vreg_status = false;
1335 /* turn of regualtor in reverse order */
1336 btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
1337 }
1338
1339 /* regulatos fails to enable */
1340 if (ret) {
1341 qca->vreg_status = false;
1342 BT_ERR("failed to enable regualtor:%s", vregs[i].name);
1343 /* set regulator voltage and load to zero */
1344 regulator_set_voltage(qca->vreg_bulk[i].consumer,
1345 0, vregs[i].max_v);
> 1346 if (vregs[i].load_ua)
1347 regulator_set_load(qca->vreg_bulk[i].consumer, 0);
1348 /* turn off regulators which are enabled */
1349 btqca_disable_regulators(i+1, vregs);
1350 }
1351
1352 return ret;
1353 }
1354
---
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: 33451 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH] Bluetooth: hci_qca: btqca_disable_regulators() can be static
2018-05-04 15:35 ` [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth " Balakrishna Godavarthi
` (2 preceding siblings ...)
2018-05-05 19:45 ` kbuild test robot
@ 2018-05-05 20:11 ` kbuild test robot
3 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2018-05-05 20:11 UTC (permalink / raw)
To: Balakrishna Godavarthi
Cc: kbuild-all, marcel, johan.hedberg, linux-bluetooth, rtatiya,
hemantg, linux-arm-msm, Balakrishna Godavarthi
Fixes: 1bab29c5dbe1 ("Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.")
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
hci_qca.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 075fab7..ff9fbe0 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1291,7 +1291,7 @@ static const struct btqca_vreg_data cherokee_data = {
.num_vregs = 5,
};
-void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
+static void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
{
/* disable the regulator if requested by user
* or when fault in any regulator.
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-05 1:17 ` Matthias Kaehlcke
@ 2018-05-09 8:56 ` Balakrishna Godavarthi
2018-05-09 9:27 ` Balakrishna Godavarthi
2018-05-09 10:07 ` Balakrishna Godavarthi
2 siblings, 0 replies; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-09 8:56 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: marcel, johan.hedberg, linux-bluetooth, rtatiya, hemantg, linux-arm-msm
Hi,
On 2018-05-05 06:47, Matthias Kaehlcke wrote:
> Hi,
>
> On Fri, May 04, 2018 at 09:05:05PM +0530, Balakrishna Godavarthi wrote:
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> Add support to read baudrate from dts.
>>
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> drivers/bluetooth/hci_qca.c | 555
>> ++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 483 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f05382b..075fab7 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...
>> @@ -35,6 +35,10 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/module.h>
>> #include <linux/serdev.h>
>> +#include <asm-generic/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of_device.h>
>
> AFAIK the convention is to order includes alphabetically.
>
>> +static struct btqca_power *qca;
>
> This limits the driver to a single instance. This is probably the most
> common use case, but in general it's preferable not to have this kind
> of limitations.
>
>> static void __serial_clock_on(struct tty_struct *tty)
>> {
>> /* TODO: Some chipset requires to enable UART clock on client
>> @@ -463,7 +506,12 @@ 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;
>> + btqca_power_setup(true);
>> + } else
>> + gpiod_set_value_cansleep(qcadev->bt_en, 1);
>
> Use curly braces for the else branch too.
>
>> +static int qca_send_poweron_cmd(struct hci_dev *hdev)
>> +{
>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>> + struct qca_data *qca = hu->priv;
>> + struct sk_buff *skb;
>> + u8 cmd;
>> +
>> + BT_DBG("%s sending power on command to btsoc", hdev->name);
>
> Use bt_dev_dbg(), same for other BT_XXX().
>
>> + /* By sending 0xFC host is trying to power up the soc */
>
> nit: SoC, same a few lines below.
>
>> + cmd = CHEROKEE_POWERON_PULSE;
>> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
>
> Use GFP_KERNEL, the code sleeps a few lines below, hence it must
> definitely not be called in atomic context.
>
>> + if (!skb) {
>> + BT_ERR("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 */
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(usecs_to_jiffies(100));
>> + set_current_state(TASK_INTERRUPTIBLE);
>> +
>> + return 0;
>> +}
>> +
>> +static int qca_send_poweroff_cmd(struct hci_dev *hdev)
>> +{
>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>> + struct qca_data *qca = hu->priv;
>> + struct sk_buff *skb;
>> + u8 cmd;
>> +
>> + BT_DBG("%s sending power off command to btsoc", hdev->name);
>> + /* By sending 0xC0 host is trying to power off the soc */
>> + cmd = CHEROKEE_POWEROFF_PULSE;
>> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
>> + if (!skb) {
>> + BT_ERR("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 */
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(usecs_to_jiffies(100));
>> + set_current_state(TASK_INTERRUPTIBLE);
>> +
>> + return 0;
>> +}
>
> This function is almost a clone of qca_send_poweron_cmd(), move the
> common code to something like qca_send_cmd() and call it from
> qca_send_poweron/off_cmd().
>
>> +static int qca_serdev_open(struct hci_uart *hu)
>> +{
>> + int ret = 0;
>> +
>> + if (hu->serdev)
>> + serdev_device_open(hu->serdev);
>
> Check return value.
>
> Add curly braces to the if branch too.
>
>> +static int qca_serdev_close(struct hci_uart *hu)
>> +{
>> + int ret = 0;
>> +
>> + if (hu->serdev)
>> + serdev_device_close(hu->serdev);
>
> Add curly braces to the if branch too.
>
>> 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_info(hdev, "setting up wcn3990");
>> + /* 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);
>
> Add curly braces.
>
>> + else {
>> + bt_dev_err(hdev, "initial speed %u", speed);
>> + return -1;
>> + }
>>
>> - bt_dev_info(hdev, "ROME setup");
>> + /* disable flow control, as chip is still not turned on */
>> + hci_uart_set_flow_control(hu, true);
>
> The interface of this function is confusing. enable = true disables
> flow control ... Not the fault of this driver though :)
>
>> + /* send poweron command to btsoc */
>> + ret = qca_send_poweron_cmd(hdev);
>> + if (ret) {
>> + bt_dev_err(hdev, "Failed to send power on command");
>> + return ret;
>> + }
>>
>> - /* Patch downloading has to be done without IBS mode */
>> - clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> + /* close serial port */
>> + ret = qca_serdev_close(hu);
>> + if (ret)
>> + return ret;
>> + /* open serial port */
>> + ret = qca_serdev_open(hu);
>> + if (ret)
>> + return ret;
>>
>> - /* Setup initial baudrate */
>> - speed = 0;
>> - if (hu->init_speed)
>> - speed = hu->init_speed;
>> - else if (hu->proto->init_speed)
>> - speed = hu->proto->init_speed;
>> + /* 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);
>
> Add curly braces.
>
>> + else {
>> + BT_ERR("%s:initial speed %u", hdev->name, speed);
>> + return -1;
>> + }
>>
>> - if (speed)
>> - host_set_baudrate(hu, speed);
>> + /* Enable flow control */
>> + hci_uart_set_flow_control(hu, false);
>> + /* wait until flow control settled */
>> + mdelay(100);
>
> A busy wait of 100ms doesn't seem a good idea. Use msleep()
> instead. Is it really necessary to wait that long?
>
>> + bt_dev_info(hdev, "wcn3990 Patch Version Request");
>> + ret = rome_patch_ver_req(hdev, &soc_ver);
>> + if (ret < 0 || soc_ver == 0) {
>> + BT_ERR("%s: Failed to get version 0x%x", hdev->name,
>> + ret);
>> + return 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;
>> + bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
>> +
>> + /* clear flow control */
>> + hci_uart_set_flow_control(hu, true);
>> + /* set operating speed */
>> + 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_ERR("%s:Failed to change the baud rate(%d)",
>> + hdev->name, ret);
>> + return ret;
>> + }
>> + if (speed)
>> + host_set_baudrate(hu, speed);
>
> Add curly braces.
>
>> + else {
>> + BT_ERR("%s:Error in setting operator speed:%u",
>> + hdev->name, speed);
>> + return -1;
>> + }
>> + }
>>
>> - if (speed) {
>> - qca_baudrate = qca_get_baudrate_value(speed);
>> + /* Set flow control */
>> + hci_uart_set_flow_control(hu, false);
>> + /*Setup patch and NVM configurations */
>
> Add blank before 'Setup' and remove one before 'NVM'.
>
>> + ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
>> + if (!ret) {
>> + set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> + qca_debugfs_init(hdev);
>> + } else if (ret == -ENOENT) {
>> + /* No patch/nvm-config found, run with original
>> + * fw/config.
>> + */
>> + ret = 0;
>> + } else if (ret == -EAGAIN) {
>> + /*
>> + * Userspace firmware loader will return -EAGAIN in
>> + * case no patch/nvm-config is found, so run with
>> + * original fw/config.
>> + */
>> + ret = 0;
>> + }
>>
>> - 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);
>> + /* Setup wcn3990 bdaddr */
>> + hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
>> +
>> + return ret;
>> +
>> + default:
>
> What follows looks similar to the Cherokee path. I didn't look at all
> the details, but it's probably possible to share some more code.
>
>> + 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;
>
> Fix indentation.
>
>> +static const struct btqca_vreg_data cherokee_data = {
>> + .soc_type = BTQCA_CHEROKEE,
>> + .vregs = (struct btqca_vreg []) {
>> + { "vddio", 1352000, 1352000, 0 },
>> + { "vddxtal", 1904000, 2040000, 0 },
>> + { "vddcore", 1800000, 1800000, 1 },
>> + { "vddpa", 130400, 1304000, 1 },
>
> 0 missing for min_v?
>
>> + { "vddldo", 3000000, 3312000, 1 },
>> + },
>> + .num_vregs = 5,
>> +};
>> +
>> +void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
>> +{
>> + /* disable the regulator if requested by user
>> + * or when fault in any regulator.
>> + */
>> + for (reg_nu = reg_nu - 1; reg_nu >= 0 ; reg_nu--) {
>
> Better use a local variable for iteration instead of the function
> parameter
>
>> +int btqca_power_setup(bool on)
>> +{
>> + int ret = 0;
>> + int i;
>> + struct btqca_vreg *vregs;
>> +
>> + if (!qca || !qca->vreg_data || !qca->vreg_bulk)
>> + return -EINVAL;
>> + vregs = qca->vreg_data->vregs;
>> +
>> + BT_DBG("on: %d", on);
>> + /* turn on if regualtors are off */
>> + if (on == true && qca->vreg_status == false) {
>
> if (on && !qca->vreg_status)
>
> You might also consider a more expressive name instead of vreg_status,
> something like vregs_on.
>
>> + qca->vreg_status = true;
>> + for (i = 0; ret == 0 && i < qca->vreg_data->num_vregs; i++) {
>
> Better check ret inline and break when an error is encountered
>
>> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
>> + vregs[i].min_v,
>> + vregs[i].max_v);
>
> Check return value.
>
>> +
>> + if (vregs[i].load_ua)
>> + regulator_set_load(qca->vreg_bulk[i].consumer,
>> + vregs[i].load_ua);
>
> Check return value.
>
>> + ret = regulator_enable(qca->vreg_bulk[i].consumer);
>> + }
>
> Fix indentation.
>
>> + } else if (on == false && qca->vreg_status == true) {
>
> (!on && qca->vreg_status)
>
>> + qca->vreg_status = false;
>> + /* turn of regualtor in reverse order */
>
> 'off, regulator'
>
>> + btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
>> + }
>> +
>> + /* regulatos fails to enable */
>
> 'regulators failed'
>
>> + if (ret) {
>> + qca->vreg_status = false;
>> + BT_ERR("failed to enable regualtor:%s", vregs[i].name);
>
> 'regulator'
>
>> + /* set regulator voltage and load to zero */
>> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
>> + 0, vregs[i].max_v);
>
> check return value.
>
>> static int qca_serdev_probe(struct serdev_device *serdev)
>> {
>> struct qca_serdev *qcadev;
>> - int err;
>> + const struct btqca_vreg_data *data;
>> + int err = 0;
>>
>> qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>> if (!qcadev)
>> 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) {
>> + qca = kzalloc(sizeof(struct btqca_power), GFP_KERNEL);
>
> Use devm_kzalloc()
>
>> + if (!qca)
>> + return -ENOMEM;
>> +
>> + qca->dev = &serdev->dev;
>> + qca->vreg_data = data;
>> + err = init_regulators(qca, data->vregs, data->num_vregs);
>> + if (err) {
>> + BT_ERR("Failed to init regualtors:%d", err);
>
> 'regulators'
>
>> + kfree(qca);
>> + 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);
>> - }
>> + /* set voltage regulator status as false */
>> + qca->vreg_status = false;
>> + /* get operating speed */
>> + device_property_read_u32(&serdev->dev, "oper-speed",
>> + &qcadev->oper_speed);
>> + device_property_read_u32(&serdev->dev, "init-speed",
>> + &qcadev->init_speed);
>> + if (!qcadev->oper_speed)
>> + BT_INFO("DTS entry for operating speed is
>> - disabled");
>
> The message isn't very clear. The entry isn't disabled, it doesn't
> exist.
>
>> static void qca_serdev_remove(struct serdev_device *serdev)
>> @@ -1047,12 +1454,16 @@ 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);
>> + if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
>> + btqca_power_setup(false);
>> + kfree(qca);
>> + } else
>> + clk_disable_unprepare(qcadev->susclk);
>
> Add curly braces.
will send a incremental patches as per your comments.
--
Regards
Balakrishna.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-05 1:17 ` Matthias Kaehlcke
2018-05-09 8:56 ` Balakrishna Godavarthi
@ 2018-05-09 9:27 ` Balakrishna Godavarthi
2018-05-09 10:07 ` Balakrishna Godavarthi
2 siblings, 0 replies; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-09 9:27 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: marcel, johan.hedberg, linux-bluetooth, rtatiya, hemantg, linux-arm-msm
hi,
On 2018-05-05 06:47, Matthias Kaehlcke wrote:
> Hi,
>
> On Fri, May 04, 2018 at 09:05:05PM +0530, Balakrishna Godavarthi wrote:
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> Add support to read baudrate from dts.
>>
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> drivers/bluetooth/hci_qca.c | 555
>> ++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 483 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f05382b..075fab7 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...
>> @@ -35,6 +35,10 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/module.h>
>> #include <linux/serdev.h>
>> +#include <asm-generic/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of_device.h>
>
> AFAIK the convention is to order includes alphabetically.
[Bala]:will update.
>
>> +static struct btqca_power *qca;
>
> This limits the driver to a single instance. This is probably the most
> common use case, but in general it's preferable not to have this kind
> of limitations.
>
[Bala]:yes, as we need to limit driver fro single instance.
>> static void __serial_clock_on(struct tty_struct *tty)
>> {
>> /* TODO: Some chipset requires to enable UART clock on client
>> @@ -463,7 +506,12 @@ 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;
>> + btqca_power_setup(true);
>> + } else
>> + gpiod_set_value_cansleep(qcadev->bt_en, 1);
>
> Use curly braces for the else branch too.
[Bala]:will update.
>
>> +static int qca_send_poweron_cmd(struct hci_dev *hdev)
>> +{
>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>> + struct qca_data *qca = hu->priv;
>> + struct sk_buff *skb;
>> + u8 cmd;
>> +
>> + BT_DBG("%s sending power on command to btsoc", hdev->name);
>
> Use bt_dev_dbg(), same for other BT_XXX().
>
>> + /* By sending 0xFC host is trying to power up the soc */
>
> nit: SoC, same a few lines below.
>
>> + cmd = CHEROKEE_POWERON_PULSE;
>> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
>
> Use GFP_KERNEL, the code sleeps a few lines below, hence it must
> definitely not be called in atomic context.
>
>> + if (!skb) {
>> + BT_ERR("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 */
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(usecs_to_jiffies(100));
>> + set_current_state(TASK_INTERRUPTIBLE);
>> +
>> + return 0;
>> +}
>> +
>> +static int qca_send_poweroff_cmd(struct hci_dev *hdev)
>> +{
>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>> + struct qca_data *qca = hu->priv;
>> + struct sk_buff *skb;
>> + u8 cmd;
>> +
>> + BT_DBG("%s sending power off command to btsoc", hdev->name);
>> + /* By sending 0xC0 host is trying to power off the soc */
>> + cmd = CHEROKEE_POWEROFF_PULSE;
>> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
>> + if (!skb) {
>> + BT_ERR("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 */
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(usecs_to_jiffies(100));
>> + set_current_state(TASK_INTERRUPTIBLE);
>> +
>> + return 0;
>> +}
>
> This function is almost a clone of qca_send_poweron_cmd(), move the
> common code to something like qca_send_cmd() and call it from
> qca_send_poweron/off_cmd().
[Bala]: will make as one function.
>
>> +static int qca_serdev_open(struct hci_uart *hu)
>> +{
>> + int ret = 0;
>> +
>> + if (hu->serdev)
>> + serdev_device_open(hu->serdev);
>
> Check return value.
>
> Add curly braces to the if branch too.
>
>> +static int qca_serdev_close(struct hci_uart *hu)
>> +{
>> + int ret = 0;
>> +
>> + if (hu->serdev)
>> + serdev_device_close(hu->serdev);
>
> Add curly braces to the if branch too.
[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_info(hdev, "setting up wcn3990");
>> + /* 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);
>
> Add curly braces.
[Bala]- will update.
>
>> + else {
>> + bt_dev_err(hdev, "initial speed %u", speed);
>> + return -1;
>> + }
>>
>> - bt_dev_info(hdev, "ROME setup");
>> + /* disable flow control, as chip is still not turned on */
>> + hci_uart_set_flow_control(hu, true);
>
> The interface of this function is confusing. enable = true disables
> flow control ... Not the fault of this driver though :)
[Bala]- yes i feel it is reverse. but i don't want disturb the function
as some one may be using the same function
before calling these function, i have written a comment.
>
>> + /* send poweron command to btsoc */
>> + ret = qca_send_poweron_cmd(hdev);
>> + if (ret) {
>> + bt_dev_err(hdev, "Failed to send power on command");
>> + return ret;
>> + }
>>
>> - /* Patch downloading has to be done without IBS mode */
>> - clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> + /* close serial port */
>> + ret = qca_serdev_close(hu);
>> + if (ret)
>> + return ret;
>> + /* open serial port */
>> + ret = qca_serdev_open(hu);
>> + if (ret)
>> + return ret;
>>
>> - /* Setup initial baudrate */
>> - speed = 0;
>> - if (hu->init_speed)
>> - speed = hu->init_speed;
>> - else if (hu->proto->init_speed)
>> - speed = hu->proto->init_speed;
>> + /* 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);
>
> Add curly braces.
>
>> + else {
>> + BT_ERR("%s:initial speed %u", hdev->name, speed);
>> + return -1;
>> + }
>>
>> - if (speed)
>> - host_set_baudrate(hu, speed);
>> + /* Enable flow control */
>> + hci_uart_set_flow_control(hu, false);
>> + /* wait until flow control settled */
>> + mdelay(100);
>
> A busy wait of 100ms doesn't seem a good idea. Use msleep()
> instead. Is it really necessary to wait that long?
[Bala]- after long analysis we conclude to wait for 100ms to make SoC to
bootup.\
>
>> + bt_dev_info(hdev, "wcn3990 Patch Version Request");
>> + ret = rome_patch_ver_req(hdev, &soc_ver);
>> + if (ret < 0 || soc_ver == 0) {
>> + BT_ERR("%s: Failed to get version 0x%x", hdev->name,
>> + ret);
>> + return 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;
>> + bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
>> +
>> + /* clear flow control */
>> + hci_uart_set_flow_control(hu, true);
>> + /* set operating speed */
>> + 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_ERR("%s:Failed to change the baud rate(%d)",
>> + hdev->name, ret);
>> + return ret;
>> + }
>> + if (speed)
>> + host_set_baudrate(hu, speed);
>
> Add curly braces.
>
>> + else {
>> + BT_ERR("%s:Error in setting operator speed:%u",
>> + hdev->name, speed);
>> + return -1;
>> + }
>> + }
>>
>> - if (speed) {
>> - qca_baudrate = qca_get_baudrate_value(speed);
>> + /* Set flow control */
>> + hci_uart_set_flow_control(hu, false);
>> + /*Setup patch and NVM configurations */
>
> Add blank before 'Setup' and remove one before 'NVM'.
>
>> + ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
>> + if (!ret) {
>> + set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> + qca_debugfs_init(hdev);
>> + } else if (ret == -ENOENT) {
>> + /* No patch/nvm-config found, run with original
>> + * fw/config.
>> + */
>> + ret = 0;
>> + } else if (ret == -EAGAIN) {
>> + /*
>> + * Userspace firmware loader will return -EAGAIN in
>> + * case no patch/nvm-config is found, so run with
>> + * original fw/config.
>> + */
>> + ret = 0;
>> + }
>>
>> - 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);
>> + /* Setup wcn3990 bdaddr */
>> + hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
>> +
>> + return ret;
>> +
>> + default:
>
> What follows looks similar to the Cherokee path. I didn't look at all
> the details, but it's probably possible to share some more code.
[Bala]- Cherokee has a different way for setup.
ROME will not have flow control enabled.
>
>> + 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;
>
> Fix indentation.
>
>> +static const struct btqca_vreg_data cherokee_data = {
>> + .soc_type = BTQCA_CHEROKEE,
>> + .vregs = (struct btqca_vreg []) {
>> + { "vddio", 1352000, 1352000, 0 },
>> + { "vddxtal", 1904000, 2040000, 0 },
>> + { "vddcore", 1800000, 1800000, 1 },
>> + { "vddpa", 130400, 1304000, 1 },
>
> 0 missing for min_v?
>
>> + { "vddldo", 3000000, 3312000, 1 },
>> + },
>> + .num_vregs = 5,
>> +};
>> +
>> +void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
>> +{
>> + /* disable the regulator if requested by user
>> + * or when fault in any regulator.
>> + */
>> + for (reg_nu = reg_nu - 1; reg_nu >= 0 ; reg_nu--) {
>
> Better use a local variable for iteration instead of the function
> parameter
>
>> +int btqca_power_setup(bool on)
>> +{
>> + int ret = 0;
>> + int i;
>> + struct btqca_vreg *vregs;
>> +
>> + if (!qca || !qca->vreg_data || !qca->vreg_bulk)
>> + return -EINVAL;
>> + vregs = qca->vreg_data->vregs;
>> +
>> + BT_DBG("on: %d", on);
>> + /* turn on if regualtors are off */
>> + if (on == true && qca->vreg_status == false) {
>
> if (on && !qca->vreg_status)
>
> You might also consider a more expressive name instead of vreg_status,
> something like vregs_on.
>
>> + qca->vreg_status = true;
>> + for (i = 0; ret == 0 && i < qca->vreg_data->num_vregs; i++) {
>
> Better check ret inline and break when an error is encountered
>
>> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
>> + vregs[i].min_v,
>> + vregs[i].max_v);
>
> Check return value.
>
>> +
>> + if (vregs[i].load_ua)
>> + regulator_set_load(qca->vreg_bulk[i].consumer,
>> + vregs[i].load_ua);
>
> Check return value.
>
>> + ret = regulator_enable(qca->vreg_bulk[i].consumer);
>> + }
>
> Fix indentation.
>
>> + } else if (on == false && qca->vreg_status == true) {
>
> (!on && qca->vreg_status)
>
>> + qca->vreg_status = false;
>> + /* turn of regualtor in reverse order */
>
> 'off, regulator'
>
>> + btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
>> + }
>> +
>> + /* regulatos fails to enable */
>
> 'regulators failed'
>
>> + if (ret) {
>> + qca->vreg_status = false;
>> + BT_ERR("failed to enable regualtor:%s", vregs[i].name);
>
> 'regulator'
>
>> + /* set regulator voltage and load to zero */
>> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
>> + 0, vregs[i].max_v);
>
> check return value.
>
>> static int qca_serdev_probe(struct serdev_device *serdev)
>> {
>> struct qca_serdev *qcadev;
>> - int err;
>> + const struct btqca_vreg_data *data;
>> + int err = 0;
>>
>> qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>> if (!qcadev)
>> 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) {
>> + qca = kzalloc(sizeof(struct btqca_power), GFP_KERNEL);
>
> Use devm_kzalloc()
>
>> + if (!qca)
>> + return -ENOMEM;
>> +
>> + qca->dev = &serdev->dev;
>> + qca->vreg_data = data;
>> + err = init_regulators(qca, data->vregs, data->num_vregs);
>> + if (err) {
>> + BT_ERR("Failed to init regualtors:%d", err);
>
> 'regulators'
>
>> + kfree(qca);
>> + 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);
>> - }
>> + /* set voltage regulator status as false */
>> + qca->vreg_status = false;
>> + /* get operating speed */
>> + device_property_read_u32(&serdev->dev, "oper-speed",
>> + &qcadev->oper_speed);
>> + device_property_read_u32(&serdev->dev, "init-speed",
>> + &qcadev->init_speed);
>> + if (!qcadev->oper_speed)
>> + BT_INFO("DTS entry for operating speed is
>> - disabled");
>
> The message isn't very clear. The entry isn't disabled, it doesn't
> exist.
>
>> static void qca_serdev_remove(struct serdev_device *serdev)
>> @@ -1047,12 +1454,16 @@ 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);
>> + if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
>> + btqca_power_setup(false);
>> + kfree(qca);
>> + } else
>> + clk_disable_unprepare(qcadev->susclk);
>
> Add curly braces.
--
Regards
Balakrishna.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-05 1:17 ` Matthias Kaehlcke
2018-05-09 8:56 ` Balakrishna Godavarthi
2018-05-09 9:27 ` Balakrishna Godavarthi
@ 2018-05-09 10:07 ` Balakrishna Godavarthi
2018-05-09 17:24 ` Matthias Kaehlcke
2 siblings, 1 reply; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-09 10:07 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: marcel, johan.hedberg, linux-bluetooth, rtatiya, hemantg, linux-arm-msm
Hi,
Please find my comments inline.
On 2018-05-05 06:47, Matthias Kaehlcke wrote:
> Hi,
>
> On Fri, May 04, 2018 at 09:05:05PM +0530, Balakrishna Godavarthi wrote:
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> Add support to read baudrate from dts.
>>
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> drivers/bluetooth/hci_qca.c | 555
>> ++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 483 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f05382b..075fab7 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...
>> @@ -35,6 +35,10 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/module.h>
>> #include <linux/serdev.h>
>> +#include <asm-generic/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of_device.h>
>
> AFAIK the convention is to order includes alphabetically.
[Bala]: will update.
>
>> +static struct btqca_power *qca;
>
> This limits the driver to a single instance. This is probably the most
> common use case, but in general it's preferable not to have this kind
> of limitations.
[Bala]: yes, we need for one instance when the driver got probed.
>
>> static void __serial_clock_on(struct tty_struct *tty)
>> {
>> /* TODO: Some chipset requires to enable UART clock on client
>> @@ -463,7 +506,12 @@ 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;
>> + btqca_power_setup(true);
>> + } else
>> + gpiod_set_value_cansleep(qcadev->bt_en, 1);
>
> Use curly braces for the else branch too.
[Bala]: will update.
>
>> +static int qca_send_poweron_cmd(struct hci_dev *hdev)
>> +{
>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>> + struct qca_data *qca = hu->priv;
>> + struct sk_buff *skb;
>> + u8 cmd;
>> +
>> + BT_DBG("%s sending power on command to btsoc", hdev->name);
>
> Use bt_dev_dbg(), same for other BT_XXX().
>
>> + /* By sending 0xFC host is trying to power up the soc */
>
> nit: SoC, same a few lines below.
>
>> + cmd = CHEROKEE_POWERON_PULSE;
>> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
>
> Use GFP_KERNEL, the code sleeps a few lines below, hence it must
> definitely not be called in atomic context.
[Bala]: will update.
>
>> + if (!skb) {
>> + BT_ERR("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 */
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(usecs_to_jiffies(100));
>> + set_current_state(TASK_INTERRUPTIBLE);
>> +
>> + return 0;
>> +}
>> +
>> +static int qca_send_poweroff_cmd(struct hci_dev *hdev)
>> +{
>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>> + struct qca_data *qca = hu->priv;
>> + struct sk_buff *skb;
>> + u8 cmd;
>> +
>> + BT_DBG("%s sending power off command to btsoc", hdev->name);
>> + /* By sending 0xC0 host is trying to power off the soc */
>> + cmd = CHEROKEE_POWEROFF_PULSE;
>> + skb = bt_skb_alloc(sizeof(cmd), GFP_ATOMIC);
>> + if (!skb) {
>> + BT_ERR("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 */
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(usecs_to_jiffies(100));
>> + set_current_state(TASK_INTERRUPTIBLE);
>> +
>> + return 0;
>> +}
>
> This function is almost a clone of qca_send_poweron_cmd(), move the
> common code to something like qca_send_cmd() and call it from
> qca_send_poweron/off_cmd().
[Bala]: will create on function for both ON and OFF.
>
>> +static int qca_serdev_open(struct hci_uart *hu)
>> +{
>> + int ret = 0;
>> +
>> + if (hu->serdev)
>> + serdev_device_open(hu->serdev);
>
> Check return value.
[Bala]: as we are not checking in qca_open, the return in this function
is to know the serdev function availability.
>
> Add curly braces to the if branch too.
[Bala]: will update.
>
>> +static int qca_serdev_close(struct hci_uart *hu)
>> +{
>> + int ret = 0;
>> +
>> + if (hu->serdev)
>> + serdev_device_close(hu->serdev);
>
> Add curly braces to the if branch too.
[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_info(hdev, "setting up wcn3990");
>> + /* 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);
>
> Add curly braces.
[Bala]: will update.
>
>> + else {
>> + bt_dev_err(hdev, "initial speed %u", speed);
>> + return -1;
>> + }
>>
>> - bt_dev_info(hdev, "ROME setup");
>> + /* disable flow control, as chip is still not turned on */
>> + hci_uart_set_flow_control(hu, true);
>
> The interface of this function is confusing. enable = true disables
> flow control ... Not the fault of this driver though :)
[Bala]: yes it is bit confusing. i taught to update.. but not sure of
impact.
so instead of updating, using the same function and written a
comment above.
>
>> + /* send poweron command to btsoc */
>> + ret = qca_send_poweron_cmd(hdev);
>> + if (ret) {
>> + bt_dev_err(hdev, "Failed to send power on command");
>> + return ret;
>> + }
>>
>> - /* Patch downloading has to be done without IBS mode */
>> - clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> + /* close serial port */
>> + ret = qca_serdev_close(hu);
>> + if (ret)
>> + return ret;
>> + /* open serial port */
>> + ret = qca_serdev_open(hu);
>> + if (ret)
>> + return ret;
>>
>> - /* Setup initial baudrate */
>> - speed = 0;
>> - if (hu->init_speed)
>> - speed = hu->init_speed;
>> - else if (hu->proto->init_speed)
>> - speed = hu->proto->init_speed;
>> + /* 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);
>
> Add curly braces.
[Bala]: will update.
>
>> + else {
>> + BT_ERR("%s:initial speed %u", hdev->name, speed);
>> + return -1;
>> + }
>>
>> - if (speed)
>> - host_set_baudrate(hu, speed);
>> + /* Enable flow control */
>> + hci_uart_set_flow_control(hu, false);
>> + /* wait until flow control settled */
>> + mdelay(100);
>
> A busy wait of 100ms doesn't seem a good idea. Use msleep()
> instead. Is it really necessary to wait that long?
[Bala]: yes this delay is required for BT Soc to bootup and settle down.
>
>> + bt_dev_info(hdev, "wcn3990 Patch Version Request");
>> + ret = rome_patch_ver_req(hdev, &soc_ver);
>> + if (ret < 0 || soc_ver == 0) {
>> + BT_ERR("%s: Failed to get version 0x%x", hdev->name,
>> + ret);
>> + return 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;
>> + bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
>> +
>> + /* clear flow control */
>> + hci_uart_set_flow_control(hu, true);
>> + /* set operating speed */
>> + 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_ERR("%s:Failed to change the baud rate(%d)",
>> + hdev->name, ret);
>> + return ret;
>> + }
>> + if (speed)
>> + host_set_baudrate(hu, speed);
>
> Add curly braces.
[Bala]: will update.
>
>> + else {
>> + BT_ERR("%s:Error in setting operator speed:%u",
>> + hdev->name, speed);
>> + return -1;
>> + }
>> + }
>>
>> - if (speed) {
>> - qca_baudrate = qca_get_baudrate_value(speed);
>> + /* Set flow control */
>> + hci_uart_set_flow_control(hu, false);
>> + /*Setup patch and NVM configurations */
>
> Add blank before 'Setup' and remove one before 'NVM'.
[Bala]: will update.
>
>> + ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &soc_ver);
>> + if (!ret) {
>> + set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> + qca_debugfs_init(hdev);
>> + } else if (ret == -ENOENT) {
>> + /* No patch/nvm-config found, run with original
>> + * fw/config.
>> + */
>> + ret = 0;
>> + } else if (ret == -EAGAIN) {
>> + /*
>> + * Userspace firmware loader will return -EAGAIN in
>> + * case no patch/nvm-config is found, so run with
>> + * original fw/config.
>> + */
>> + ret = 0;
>> + }
>>
>> - 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);
>> + /* Setup wcn3990 bdaddr */
>> + hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
>> +
>> + return ret;
>> +
>> + default:
>
> What follows looks similar to the Cherokee path. I didn't look at all
> the details, but it's probably possible to share some more code.
[Bala]: yes these are two different chip.
we also have difference in setup. the follwing are major
differences
Cherokee uses flow control. where ROME flow control is disabled.
ON sequence also varies.
>
>> + 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;
>
> Fix indentation.
[Bala]: will update.
>
>> +static const struct btqca_vreg_data cherokee_data = {
>> + .soc_type = BTQCA_CHEROKEE,
>> + .vregs = (struct btqca_vreg []) {
>> + { "vddio", 1352000, 1352000, 0 },
>> + { "vddxtal", 1904000, 2040000, 0 },
>> + { "vddcore", 1800000, 1800000, 1 },
>> + { "vddpa", 130400, 1304000, 1 },
>
> 0 missing for min_v?
[Bala]: min_v is the minimum voltage required for BT chip.
>
>> + { "vddldo", 3000000, 3312000, 1 },
>> + },
>> + .num_vregs = 5,
>> +};
>> +
>> +void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
>> +{
>> + /* disable the regulator if requested by user
>> + * or when fault in any regulator.
>> + */
>> + for (reg_nu = reg_nu - 1; reg_nu >= 0 ; reg_nu--) {
>
> Better use a local variable for iteration instead of the function
> parameter
[Bala]: will update.
>
>> +int btqca_power_setup(bool on)
>> +{
>> + int ret = 0;
>> + int i;
>> + struct btqca_vreg *vregs;
>> +
>> + if (!qca || !qca->vreg_data || !qca->vreg_bulk)
>> + return -EINVAL;
>> + vregs = qca->vreg_data->vregs;
>> +
>> + BT_DBG("on: %d", on);
>> + /* turn on if regualtors are off */
>> + if (on == true && qca->vreg_status == false) {
>
> if (on && !qca->vreg_status)
>
> You might also consider a more expressive name instead of vreg_status,
> something like vregs_on.
[Bala]: will update.
>
>> + qca->vreg_status = true;
>> + for (i = 0; ret == 0 && i < qca->vreg_data->num_vregs; i++) {
>
> Better check ret inline and break when an error is encountered
[Bala]: will update.
>
>> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
>> + vregs[i].min_v,
>> + vregs[i].max_v);
>
> Check return value.
[Bala]: i am checking the status for regulator enable
do we really require to check the status. as this same
regulators are used by different sub systems.
we will get an error during regulator enable when voltages are
not settled down.
>
>> +
>> + if (vregs[i].load_ua)
>> + regulator_set_load(qca->vreg_bulk[i].consumer,
>> + vregs[i].load_ua);
>
> Check return value.
>
>> + ret = regulator_enable(qca->vreg_bulk[i].consumer);
>> + }
>
> Fix indentation.
[Bala]: will update.
>
>> + } else if (on == false && qca->vreg_status == true) {
>
> (!on && qca->vreg_status)
>
>> + qca->vreg_status = false;
>> + /* turn of regualtor in reverse order */
>
> 'off, regulator'
[Bala]: will update.
>
>> + btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
>> + }
>> +
>> + /* regulatos fails to enable */
>
> 'regulators failed'
[Bala]: will update.
>
>> + if (ret) {
>> + qca->vreg_status = false;
>> + BT_ERR("failed to enable regualtor:%s", vregs[i].name);
>
> 'regulator'
>
>> + /* set regulator voltage and load to zero */
>> + regulator_set_voltage(qca->vreg_bulk[i].consumer,
>> + 0, vregs[i].max_v);
>
> check return value.
>
>> static int qca_serdev_probe(struct serdev_device *serdev)
>> {
>> struct qca_serdev *qcadev;
>> - int err;
>> + const struct btqca_vreg_data *data;
>> + int err = 0;
>>
>> qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>> if (!qcadev)
>> 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) {
>> + qca = kzalloc(sizeof(struct btqca_power), GFP_KERNEL);
>
> Use devm_kzalloc()
[Bala]: will update.
>
>> + if (!qca)
>> + return -ENOMEM;
>> +
>> + qca->dev = &serdev->dev;
>> + qca->vreg_data = data;
>> + err = init_regulators(qca, data->vregs, data->num_vregs);
>> + if (err) {
>> + BT_ERR("Failed to init regualtors:%d", err);
>
> 'regulators'
[Bala]: will update.
>
>> + kfree(qca);
>> + 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);
>> - }
>> + /* set voltage regulator status as false */
>> + qca->vreg_status = false;
>> + /* get operating speed */
>> + device_property_read_u32(&serdev->dev, "oper-speed",
>> + &qcadev->oper_speed);
>> + device_property_read_u32(&serdev->dev, "init-speed",
>> + &qcadev->init_speed);
>> + if (!qcadev->oper_speed)
>> + BT_INFO("DTS entry for operating speed is
>> - disabled");
>
> The message isn't very clear. The entry isn't disabled, it doesn't
> exist.
[Bala]: will update.
>
>> static void qca_serdev_remove(struct serdev_device *serdev)
>> @@ -1047,12 +1454,16 @@ 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);
>> + if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
>> + btqca_power_setup(false);
>> + kfree(qca);
>> + } else
>> + clk_disable_unprepare(qcadev->susclk);
>
> Add curly braces.
[Bala]: will update.
Thanks for code review. will send a updated patch with the changes.
--
Regards
Balakrishna.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-09 10:07 ` Balakrishna Godavarthi
@ 2018-05-09 17:24 ` Matthias Kaehlcke
2018-05-10 16:01 ` Balakrishna Godavarthi
0 siblings, 1 reply; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-05-09 17:24 UTC (permalink / raw)
To: Balakrishna Godavarthi
Cc: marcel, johan.hedberg, linux-bluetooth, rtatiya, hemantg, linux-arm-msm
Hi Balakrishna,
On Wed, May 09, 2018 at 03:37:34PM +0530, Balakrishna Godavarthi wrote:
> Hi,
> Please find my comments inline.
>
> On 2018-05-05 06:47, Matthias Kaehlcke wrote:
> > Hi,
> >
> > On Fri, May 04, 2018 at 09:05:05PM +0530, Balakrishna Godavarthi wrote:
> > > Add support to set voltage/current of various regulators
> > > to power up/down Bluetooth chip wcn3990.
> > > Add support to read baudrate from dts.
> > >
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > ---
> > > drivers/bluetooth/hci_qca.c | 555
> > > ++++++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 483 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index f05382b..075fab7 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > ...
> > > - bt_dev_info(hdev, "ROME setup");
> > > + /* disable flow control, as chip is still not turned on */
> > > + hci_uart_set_flow_control(hu, true);
> >
> > The interface of this function is confusing. enable = true disables
> > flow control ... Not the fault of this driver though :)
> [Bala]: yes it is bit confusing. i taught to update.. but not sure of
> impact.
> so instead of updating, using the same function and written a
> comment above.
Sure, not much you can do about it. Just mentioned it since I was
about to comment that things are done inversely, but tean checked
hci_uart_set_flow_control() and saw that it has this odd
interface. Folks more familiar with the Bluetooth subsystem are
probably already used to it ;-)
> > > - if (speed)
> > > - host_set_baudrate(hu, speed);
> > > + /* Enable flow control */
> > > + hci_uart_set_flow_control(hu, false);
> > > + /* wait until flow control settled */
> > > + mdelay(100);
> >
> > A busy wait of 100ms doesn't seem a good idea. Use msleep()
> > instead. Is it really necessary to wait that long?
> [Bala]: yes this delay is required for BT Soc to bootup and settle down.
:(
> > What follows looks similar to the Cherokee path. I didn't look at all
> > the details, but it's probably possible to share some more code.
> [Bala]: yes these are two different chip.
> we also have difference in setup. the follwing are major differences
> Cherokee uses flow control. where ROME flow control is disabled.
> ON sequence also varies.
Ok, I'll give it another pass in the next revision to see if I have
suggestions to make it at least a bit less redundant.
> > > +static const struct btqca_vreg_data cherokee_data = {
> > > + .soc_type = BTQCA_CHEROKEE,
> > > + .vregs = (struct btqca_vreg []) {
> > > + { "vddio", 1352000, 1352000, 0 },
> > > + { "vddxtal", 1904000, 2040000, 0 },
> > > + { "vddcore", 1800000, 1800000, 1 },
> > > + { "vddpa", 130400, 1304000, 1 },
> >
> > 0 missing for min_v?
>
> [Bala]: min_v is the minimum voltage required for BT chip.
I referred to the value '130400', which happens to be exactly 1/10 of
the max_v '1304000', so I suspect the voltages are the same and a zero
is missing.
> > > + regulator_set_voltage(qca->vreg_bulk[i].consumer,
> > > + vregs[i].min_v,
> > > + vregs[i].max_v);
> >
> > Check return value.
> [Bala]: i am checking the status for regulator enable
> do we really require to check the status. as this same regulators
> are used by different sub systems.
> we will get an error during regulator enable when voltages are not
> settled down.
regulator_set_voltage() could fail due to requesting a voltage range
not supported by the regulator. In this case regulator_enable() could
still succeed, but not supply the requested voltage.
Thanks
Matthias
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.
2018-05-09 17:24 ` Matthias Kaehlcke
@ 2018-05-10 16:01 ` Balakrishna Godavarthi
0 siblings, 0 replies; 15+ messages in thread
From: Balakrishna Godavarthi @ 2018-05-10 16:01 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: marcel, johan.hedberg, linux-bluetooth, rtatiya, hemantg, linux-arm-msm
Hi Matthias,
On 2018-05-09 22:54, Matthias Kaehlcke wrote:
> Hi Balakrishna,
>
> On Wed, May 09, 2018 at 03:37:34PM +0530, Balakrishna Godavarthi wrote:
>> Hi,
>> Please find my comments inline.
>>
>> On 2018-05-05 06:47, Matthias Kaehlcke wrote:
>> > Hi,
>> >
>> > On Fri, May 04, 2018 at 09:05:05PM +0530, Balakrishna Godavarthi wrote:
>> > > Add support to set voltage/current of various regulators
>> > > to power up/down Bluetooth chip wcn3990.
>> > > Add support to read baudrate from dts.
>> > >
>> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > ---
>> > > drivers/bluetooth/hci_qca.c | 555
>> > > ++++++++++++++++++++++++++++++++++++++------
>> > > 1 file changed, 483 insertions(+), 72 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index f05382b..075fab7 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > > ...
>> > > - bt_dev_info(hdev, "ROME setup");
>> > > + /* disable flow control, as chip is still not turned on */
>> > > + hci_uart_set_flow_control(hu, true);
>> >
>> > The interface of this function is confusing. enable = true disables
>> > flow control ... Not the fault of this driver though :)
>> [Bala]: yes it is bit confusing. i taught to update.. but not sure of
>> impact.
>> so instead of updating, using the same function and written a
>> comment above.
>
> Sure, not much you can do about it. Just mentioned it since I was
> about to comment that things are done inversely, but tean checked
> hci_uart_set_flow_control() and saw that it has this odd
> interface. Folks more familiar with the Bluetooth subsystem are
> probably already used to it ;-)
>
>> > > - if (speed)
>> > > - host_set_baudrate(hu, speed);
>> > > + /* Enable flow control */
>> > > + hci_uart_set_flow_control(hu, false);
>> > > + /* wait until flow control settled */
>> > > + mdelay(100);
>> >
>> > A busy wait of 100ms doesn't seem a good idea. Use msleep()
>> > instead. Is it really necessary to wait that long?
>> [Bala]: yes this delay is required for BT Soc to bootup and settle
>> down.
>
> :(
>
>> > What follows looks similar to the Cherokee path. I didn't look at all
>> > the details, but it's probably possible to share some more code.
>> [Bala]: yes these are two different chip.
>> we also have difference in setup. the follwing are major
>> differences
>> Cherokee uses flow control. where ROME flow control is
>> disabled.
>> ON sequence also varies.
>
> Ok, I'll give it another pass in the next revision to see if I have
> suggestions to make it at least a bit less redundant.
>
>> > > +static const struct btqca_vreg_data cherokee_data = {
>> > > + .soc_type = BTQCA_CHEROKEE,
>> > > + .vregs = (struct btqca_vreg []) {
>> > > + { "vddio", 1352000, 1352000, 0 },
>> > > + { "vddxtal", 1904000, 2040000, 0 },
>> > > + { "vddcore", 1800000, 1800000, 1 },
>> > > + { "vddpa", 130400, 1304000, 1 },
>> >
>> > 0 missing for min_v?
>>
>> [Bala]: min_v is the minimum voltage required for BT chip.
>
> I referred to the value '130400', which happens to be exactly 1/10 of
> the max_v '1304000', so I suspect the voltages are the same and a zero
> is missing.
>
[Bala]: Good catch, the same regulator is used for WLAN too. that could
be reason i could not able to catch this issue during testing
will update and retest it again.
>> > > + regulator_set_voltage(qca->vreg_bulk[i].consumer,
>> > > + vregs[i].min_v,
>> > > + vregs[i].max_v);
>> >
>> > Check return value.
>> [Bala]: i am checking the status for regulator enable
>> do we really require to check the status. as this same
>> regulators
>> are used by different sub systems.
>> we will get an error during regulator enable when voltages are
>> not
>> settled down.
>
> regulator_set_voltage() could fail due to requesting a voltage range
> not supported by the regulator. In this case regulator_enable() could
> still succeed, but not supply the requested voltage.
>
[Bala]: will catch the return status.
> Thanks
>
> Matthias
I will send the patch after testing these changes.
--
Regards
Balakrishna.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-05-10 16:01 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-04 15:35 [PATCH v4 0/3] Enable Bluetooth functionality for WCN3990 Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 1/3] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990 Balakrishna Godavarthi
2018-05-04 15:35 ` [PATCH v4 2/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth " Balakrishna Godavarthi
2018-05-05 1:17 ` Matthias Kaehlcke
2018-05-09 8:56 ` Balakrishna Godavarthi
2018-05-09 9:27 ` Balakrishna Godavarthi
2018-05-09 10:07 ` Balakrishna Godavarthi
2018-05-09 17:24 ` Matthias Kaehlcke
2018-05-10 16:01 ` Balakrishna Godavarthi
2018-05-05 19:21 ` kbuild test robot
2018-05-05 19:45 ` kbuild test robot
2018-05-05 20:11 ` [RFC PATCH] Bluetooth: hci_qca: btqca_disable_regulators() can be static kbuild test robot
2018-05-04 15:35 ` [PATCH v4 3/3] Bluetooth: btqca: Add wcn3990 firmware download support Balakrishna Godavarthi
2018-05-05 19:11 ` kbuild test robot
2018-05-05 19:14 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.