Hi Matthias,
please find my comment inline.
On 2018-05-12 02:55, Matthias Kaehlcke wrote:
Hi Bala,
On Fri, May 11, 2018 at 05:51:03PM +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@xxxxxxxxxxxxxx>
---
Please include a change log also in the individual patches, not just
in the cover letter. In a revision after many comments it may be
sufficient to say "addressed <reviewers> comments", if the number if
changes is limited it is preferable to briefly list the individual
changes.
[Bala]: will add from next patch set.
drivers/bluetooth/btqca.h | 6 +
drivers/bluetooth/hci_qca.c | 547
++++++++++++++++++++++++++++++++++++++------
2 files changed, 480 insertions(+), 73 deletions(-)
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 2a7366b..8e2142a 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)
The parentheses are not needed around a literal. That they are used
for the other values is IMO no good reason to introduce more of them.
You might want to squeeze in a clean up patch that removes them for
the other defines.
+int btqca_power_setup(bool on);
+int qca_btsoc_cleanup(struct hci_dev *hdev);
nit: inconsistent naming.
If maintainers and authors have no objections you might want to
squeeze in a patch that renames everything to btqca. Just a suggestion
though.
[bala]: changed the name of function to qca_power_setup
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f05382b..71f9591 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,11 @@
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/serdev.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of_device.h>
+#include <linux/device.h>
As I mentioned on v4, the includes should be in alphabetical order.
[Bala]: some thing like this?
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/regulator/consumer.h>
#include <linux/serdev.h>
+enum btqca_soc_t {
nit: Again, inconsistent naming, some functions/structs are called
qca_... other btqca_... Personally I prefer btqca_ to avoid clashes.
[Bala]: will append qca_.* for the function and structure which are
newly added. to aling with already existing functions and strcutures
so btqca_soc_t will be qca_soc_type.
let me know your comments on this.
+/*
+ * voltage regulator information required for configuring the
+ * QCA bluetooth chipset
nit: Voltage, Bluetooth
[Bala]: will update
+ */
+struct btqca_vreg {
+ const char *name;
+ unsigned int min_v;
+ unsigned int max_v;
+ unsigned int load_ua;
+};
nit: consider min_uV, max_uV, load_uA (as in the regulator framework).
[Bala]: will update
+
+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.
nit: Bluetooth
[Bala]: will update
+static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd)
+{
+ struct hci_uart *hu = hci_get_drvdata(hdev);
+ struct qca_data *qca = hu->priv;
+ struct sk_buff *skb;
+
+ BT_DBG("%s:sending command %02x to SoC", hdev->name, cmd);
bt_dev_dbg()
[Bala]: will update
+
+ skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
+ if (!skb) {
+ BT_ERR("Failed to allocate memory for skb packet");
bt_dev_err()
[Bala]: will update
+ /* Wait for 100 us for soc to settle down */
nit: uS, SoC
[Bala]: will update
+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;
+}
From v4:
> Check return value.
[Bala]: as we are not checking in qca_open, the return in this
function is to know the serdev function availability.
Sorry, your comment didn't really clarify things for me. The function
is called from qca_setup() apparently with the intention to open the
serial port, not to check if the function is available. If the serial
port can't be opened for whatever reason the function should return an
error (typically a negative value).
Also the error message is misleading, the underlying problem is not
that the open operations is not supported, but that no serdev device
is associated with the hci_uart. Actually it seems this could never
happen:
In qca_serdev_probe():
qcadev->serdev_hu.serdev = serdev;
And qca_serdev_open() is only called from qca_setup(). Basically the
first thing qca_setup() does is:
qcadev = serdev_device_get_drvdata(hu->serdev);
Thus if hu->serdev is NULL qca_setup() will crash shortly after:
switch (qcadev->btsoc_type) {
I think we can get rid of qca_serdev_open/close() and just call
directly serdev_device_open().
[Bala]: yes your correct. will call the serdev_.* close and open from
qca_setup()
Do we really need to check hu->serdev == NULL in qca_setup?
+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_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);
The comment doesn't provide any useful information. From the code it's
evident that a CHEROKEE_POWEROFF_PULSE is sent, if anybody is
interested in the hex code they can look up the definition of
CHEROKEE_POWEROFF_PULSE.
[Bala]: will remove the comment.
+ /* turn off btsoc */
+ return btqca_power_setup(false);
This comment laso doesn't provide any value.
[Bala]: will remove
+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;
+}
Same commants as for open().
Preferably keep open() and close() together, without squeezing the
cleanup function in between.
[Bala]: will remove these functions and directly call them from serdev_*
close or open from qca_setup()
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:
I still thinks this is has a lot of common code with the default/Rome
case, but let's first clarify and possibly improve a few things.
[Bala]: actually i haven't edited any part of code for ROME i.e. default
case expect the indentation.
Yes we have little common code, but if i do any changes to ROME
code i.e. default case,i don't have ROME Chip to test.
i am little bit worried if some thing in ROME got broken with my
change. that will create problem.
+ 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;
This pattern is repeated a several times for initial and operating
speeds. Helper functions like btqca_get_init_speed() and
btqca_get_oper_speed() would make the code more readable and compact.
[Bala]: will create a new functions.
+
+ if (speed) {
+ host_set_baudrate(hu, speed);
+ } else {
+ bt_dev_err(hdev, "initial speed %u", speed);
+ return -1;
+ }
Note: Up to here Rome and Cherokee do exactly the same.
+ /* disable flow control, as chip is still not turned on */
+ hci_uart_set_flow_control(hu, true);
+ /* send 0xFC command to btsoc */
+ ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
Delete useless comment.
[Bala]: will update.
+ if (ret) {
+ bt_dev_err(hdev, "Failed to send power on command");
+ return ret;
+ }
+
+ /* close serial port */
+ ret = qca_serdev_close(hu);
+ if (ret)
+ return ret;
+ /* open serial port */
+ ret = qca_serdev_open(hu);
+ if (ret)
+ return ret;
The comments are pointless, it is evident from the code that the
serial port is opened/closed. Much more interesting would be to
explain why it is necessary to close and re-open the port.
[Bala]: will update.
- bt_dev_info(hdev, "ROME setup");
+ /* 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);
bt_dev_err()
+ return -1;
+ }
The initial baudrate has already been set above, is it necessary to
configure it again because the port was closed?
[Bala]: for safer side we setting init speed again after close and open.
This baudrate code is extremely noisy. Besides introducing the helper
functions mentioned above you could log the error message in
host_set_baudrate() or even have an addtional wrapper that determines
and sets the baudrate. The latter would reduce the above to:
if (xyz_set_baudrate(hu, INIT_RATE))
return -1;
And that multiple times.
[Bala]: will create wrapper to handle baudrate setting.
+ /* clear flow control */
+ hci_uart_set_flow_control(hu, true);
+ /* set operating speed */
Note: Starting from here the code for Cherokee and Rome is essentially
the same, except for enabling flow control.
If you prefer keep the somewhat redundant code paths in the next
revision, but please consider the improvements I suggested. With less
noisy code it will be easier to determine if it is reasonable to join
the two code paths.
[Bala]: will try to use common code.
+static int btqca_enable_regulators(int i, struct btqca_vreg *vregs)
The common convention is to pass first the structure a function acts
on, then other parameters. Use a more expressive name for the
number of regulators, like 'num_regs' or 'nregs'.
[Bala]:will update.
+{
+ int ret = 0;
Initialization is not necessary, ret is assigned in the next line.
[Bala]:will remove.
+static void btqca_disable_regulators(int reg_nu, struct btqca_vreg
*vregs)
+{
Same as for the enable function. Please use the same name for the
argument with the number of regulators on both functions, personally I
don't think 'reg_nu' is a great choice, but that's just my opinion ;-)
[Bala]: in btqca_enable_regulator() we are enabling one regulator. i.e.
this is called by btqc_power_setup() function.
in btqca_disable_regulator() we are disabling all the
regulators.
i think it require a checkup. will update both in the same way.
+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 */
regulators
Consider dropping the comment, IMO the code speaks for itself.
[Bala]: will remove
+ if (on == true && qca->vregs_on == false) {
if (on && !qca->vreg_on) {
+ qca->vregs_on = true;
Typically you'd change the variable after having performed the
operation with success.
[Bala]: will update.
+ for (i = 0; i < qca->vreg_data->num_vregs; i++) {
+ ret = btqca_enable_regulators(i, vregs);
+ if (ret)
+ break;
+ }
+ } else if (on == false && qca->vregs_on == true) {
if (!on && qca->vreg_on) {
+ qca->vregs_on = false;
Better done after having disabled the regulators.
[Bala]: will update.
+ /* turn off regulator in reverse order */
+ btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
+ }
+
+ /* regulators failed */
+ if (ret) {
+ qca->vregs_on = false;
+ BT_ERR("failed to enable regulator:%s", vregs[i].name);
+ /* turn off regulators which are enabled */
+ btqca_disable_regulators(i, vregs);
+ }
Why not do this directly when the problem is detected? The code also
relies on 'ret' only being set in the 'on' path. Which isn't intuitive
for the reader and *might* change in the future.
[Bala]: will copy the same in on block.
+
+ return ret;
+}
+
+static int init_regulators(struct btqca_power *qca,
Preferably keep use the same prefix (btqca_) consistently, unless
names become awfully long or otherwise unreadable.
[Bala]: will rename as qca_init_regulators() and also changes functions
starting with btqca_.* to qca_.*
static int qca_serdev_probe(struct serdev_device *serdev)
{
struct qca_serdev *qcadev;
const struct btqca_vreg_data *data;
int err = 0;
unnecessary initialization
[Bala]: will remove.
/* set voltage regulator status as false */
qca->vregs_on = false;
delete pointless comment
[Bala]: will remove
/* get operating speed */
device_property_read_u32(&serdev->dev, "oper-speed",
comment doesn't add much value
[Bala]: will remove
+ err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+ if (err) {
+ BT_ERR("wcn3990 serdev registration failed");
+ kfree(qca);
+ goto out;
disable regulators
[Bala]: we didn't enable regulators yet. we just get the regulators
address. explicitly disabling the regulators is not safe.
Thanks
Matthias
--
Regards
Balakrishna.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
