[PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

From: Bjorn Andersson <bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

The Qualcomm WCNSS chip provides two SMD channels to the BT core; one
for command and one for event packets. This driver exposes the two
channels as a hci device.

Signed-off-by: Bjorn Andersson <bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 drivers/bluetooth/Kconfig   |  11 +++
 drivers/bluetooth/Makefile  |   1 +
 drivers/bluetooth/hci_smd.c | 222 ++++++++++++++++++++++++++++++++++++++++++++
 include/net/bluetooth/hci.h |   1 +
 4 files changed, 235 insertions(+)
 create mode 100644 drivers/bluetooth/hci_smd.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 3d480d8c6111..1a2658f373b5 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig

@@ -62,6 +62,17 @@ config BT_HCIBTSDIO
 	  Say Y here to compile support for Bluetooth SDIO devices into the
 	  kernel or say M to compile it as module (btsdio).
 
+config BT_HCISMD
+	tristate "HCI Qualcomm SMD driver"
+	depends on QCOM_SMD
+	help
+	  Qualcomm SMD HCI driver.
+	  This driver is used to bridge HCI data onto the shared memory
+	  channels to the WCNSS core.
+
+	  Say Y here to compile support for HCI over Qualcomm SMD into the
+	  kernelor say M to compile as a module.
+
 config BT_HCIUART
 	tristate "HCI UART driver"
 	depends on TTY

diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 07c9cf381e5a..43c7dc8641ff 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile

@@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART)	+= btuart_cs.o
 
 obj-$(CONFIG_BT_HCIBTUSB)	+= btusb.o
 obj-$(CONFIG_BT_HCIBTSDIO)	+= btsdio.o
+obj-$(CONFIG_BT_HCISMD)		+= hci_smd.o
 
 obj-$(CONFIG_BT_INTEL)		+= btintel.o
 obj-$(CONFIG_BT_ATH3K)		+= ath3k.o

diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c
new file mode 100644
index 000000000000..e5748da2f902
--- /dev/null
+++ b/drivers/bluetooth/hci_smd.c

@@ -0,0 +1,222 @@
+/*
+ * Copyright (c) 2015, Sony Mobile Communications Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/smd.h>
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+#include <net/bluetooth/hci.h>
+
+static struct {
+	struct qcom_smd_channel *acl_channel;
+	struct qcom_smd_channel *cmd_channel;
+
+	struct hci_dev *hci;
+} smd_hci;
+
+static int smd_hci_recv(unsigned type, const void *data, size_t count)
+{
+	struct sk_buff *skb;
+	void *buf;
+	int ret;
+
+	skb = bt_skb_alloc(count, GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	buf = skb_put(skb, count);
+	memcpy_fromio(buf, data, count);
+
+	skb->dev = (void *)smd_hci.hci;
+	bt_cb(skb)->pkt_type = type;
+	skb_orphan(skb);
+
+	ret = hci_recv_frame(smd_hci.hci, skb);
+	if (ret < 0)
+		kfree_skb(skb);
+
+	return ret;
+}
+
+static int smd_hci_acl_callback(struct qcom_smd_device *qsdev,
+				const void *data,
+				size_t count)
+{
+	return smd_hci_recv(HCI_ACLDATA_PKT, data, count);
+}
+
+static int smd_hci_cmd_callback(struct qcom_smd_device *qsdev,
+				const void *data,
+				size_t count)
+{
+	return smd_hci_recv(HCI_EVENT_PKT, data, count);
+}
+
+static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	int ret;
+
+	switch (bt_cb(skb)->pkt_type) {
+	case HCI_ACLDATA_PKT:
+	case HCI_SCODATA_PKT:
+		ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len);
+		break;
+	case HCI_COMMAND_PKT:
+		ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len);
+		break;
+	default:
+		ret = -ENODEV;
+		break;
+	}
+
+	kfree_skb(skb);
+
+	return ret;
+}
+
+static int smd_hci_open(struct hci_dev *hci)
+{
+	return 0;
+}
+
+static int smd_hci_close(struct hci_dev *hci)
+{
+	return 0;
+}
+
+static int smd_hci_set_bdaddr(struct hci_dev *hci,
+			      const bdaddr_t *bdaddr)
+{
+	u8 buf[12];
+
+	buf[0] = 0x0b;
+	buf[1] = 0xfc;
+	buf[2] = 0x9;
+	buf[3] = 0x1;
+	buf[4] = 0x2;
+	buf[5] = sizeof(bdaddr_t);
+	memcpy(buf + 6, bdaddr, sizeof(bdaddr_t));
+
+	return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf));
+}
+
+static int smd_hci_register(void)
+{
+	struct hci_dev *hci;
+	int ret;
+
+	if (smd_hci.hci)
+		return 0;
+
+	/* Wait for both channels to probe before registering */
+	if (!smd_hci.acl_channel || !smd_hci.cmd_channel)
+		return 0;
+
+	hci = hci_alloc_dev();
+	if (!hci)
+		return -ENOMEM;
+
+	hci->bus = HCI_SMD;
+	hci->open = smd_hci_open;
+	hci->close = smd_hci_close;
+	hci->send = smd_hci_send;
+	hci->set_bdaddr = smd_hci_set_bdaddr;
+
+	ret = hci_register_dev(hci);
+	if (ret < 0) {
+		hci_free_dev(hci);
+		return ret;
+	}
+
+	smd_hci.hci = hci;
+
+	return 0;
+}
+
+static void smd_hci_unregister(void)
+{
+	/* Only unregister on the first remove call */
+	if (!smd_hci.hci)
+		return;
+
+	hci_unregister_dev(smd_hci.hci);
+	hci_free_dev(smd_hci.hci);
+	smd_hci.hci = NULL;
+}
+
+static int smd_hci_acl_probe(struct qcom_smd_device *sdev)
+{
+	smd_hci.acl_channel = sdev->channel;
+	smd_hci_register();
+
+	return 0;
+}
+
+static int smd_hci_cmd_probe(struct qcom_smd_device *sdev)
+{
+	smd_hci.cmd_channel = sdev->channel;
+	smd_hci_register();
+
+	return 0;
+}
+
+static void smd_hci_acl_remove(struct qcom_smd_device *sdev)
+{
+	smd_hci.acl_channel = NULL;
+	smd_hci_unregister();
+}
+
+static void smd_hci_cmd_remove(struct qcom_smd_device *sdev)
+{
+	smd_hci.cmd_channel = NULL;
+	smd_hci_unregister();
+}
+
+static const struct qcom_smd_id smd_hci_acl_match[] = {
+	{ .name = "APPS_RIVA_BT_ACL" },
+	{}
+};
+
+static const struct qcom_smd_id smd_hci_cmd_match[] = {
+	{ .name = "APPS_RIVA_BT_CMD" },
+	{}
+};
+
+static struct qcom_smd_driver smd_hci_acl_driver = {
+	.probe = smd_hci_acl_probe,
+	.remove = smd_hci_acl_remove,
+	.callback = smd_hci_acl_callback,
+	.smd_match_table = smd_hci_acl_match,
+	.driver  = {
+		.name  = "qcom_smd_hci_acl",
+		.owner = THIS_MODULE,
+	},
+};
+
+static struct qcom_smd_driver smd_hci_cmd_driver = {
+	.probe = smd_hci_cmd_probe,
+	.remove = smd_hci_cmd_remove,
+	.callback = smd_hci_cmd_callback,
+	.smd_match_table = smd_hci_cmd_match,
+	.driver  = {
+		.name  = "qcom_smd_hci_cmd",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_qcom_smd_driver(smd_hci_acl_driver);
+module_qcom_smd_driver(smd_hci_cmd_driver);
+
+MODULE_DESCRIPTION("Qualcomm SMD HCI driver");
+MODULE_LICENSE("GPL v2");

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7ca6690355ea..ee5b2dd922f6 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h

@@ -58,6 +58,7 @@
 #define HCI_RS232	4
 #define HCI_PCI		5
 #define HCI_SDIO	6
+#define HCI_SMD		7
 
 /* HCI controller types */
 #define HCI_BREDR	0x00

-- 
2.4.2

Hi Bjorn,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: arm-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git checkout a380238e176a2641aa74bac37f4e735fcec4d854
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

quoted

drivers/bluetooth/hci_smd.c:186:33: error: array type has incomplete element type

    static const struct qcom_smd_id smd_hci_acl_match[] = {
                                    ^

quoted

drivers/bluetooth/hci_smd.c:187:2: error: field name not in record or union initializer

     { .name = "APPS_RIVA_BT_ACL" },
     ^
   drivers/bluetooth/hci_smd.c:187:2: error: (near initialization for 'smd_hci_acl_match')
   drivers/bluetooth/hci_smd.c:191:33: error: array type has incomplete element type
    static const struct qcom_smd_id smd_hci_cmd_match[] = {
                                    ^
   drivers/bluetooth/hci_smd.c:192:2: error: field name not in record or union initializer
     { .name = "APPS_RIVA_BT_CMD" },
     ^
   drivers/bluetooth/hci_smd.c:192:2: error: (near initialization for 'smd_hci_cmd_match')

quoted

drivers/bluetooth/hci_smd.c:200:2: error: unknown field 'smd_match_table' specified in initializer

     .smd_match_table = smd_hci_acl_match,
     ^

quoted

drivers/bluetooth/hci_smd.c:200:2: warning: excess elements in struct initializer
drivers/bluetooth/hci_smd.c:200:2: warning: (near initialization for 'smd_hci_acl_driver')

   drivers/bluetooth/hci_smd.c:211:2: error: unknown field 'smd_match_table' specified in initializer
     .smd_match_table = smd_hci_cmd_match,
     ^
   drivers/bluetooth/hci_smd.c:211:2: warning: excess elements in struct initializer

quoted

drivers/bluetooth/hci_smd.c:211:2: warning: (near initialization for 'smd_hci_cmd_driver')

   In file included from drivers/bluetooth/hci_smd.c:14:0:

quoted

include/linux/module.h:128:27: error: redefinition of '__inittest'

     static inline initcall_t __inittest(void)  \
                              ^

quoted

include/linux/device.h:1321:1: note: in expansion of macro 'module_init'

    module_init(__driver##_init); \
    ^

quoted

include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'

     module_driver(__smd_driver, qcom_smd_driver_register, \
     ^

quoted

drivers/bluetooth/hci_smd.c:219:1: note: in expansion of macro 'module_qcom_smd_driver'

    module_qcom_smd_driver(smd_hci_cmd_driver);
    ^
   include/linux/module.h:128:27: note: previous definition of '__inittest' was here
     static inline initcall_t __inittest(void)  \
                              ^

quoted

include/linux/device.h:1321:1: note: in expansion of macro 'module_init'

    module_init(__driver##_init); \
    ^

quoted

include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'

     module_driver(__smd_driver, qcom_smd_driver_register, \
     ^
   drivers/bluetooth/hci_smd.c:218:1: note: in expansion of macro 'module_qcom_smd_driver'
    module_qcom_smd_driver(smd_hci_acl_driver);
    ^

quoted

include/linux/module.h:130:6: error: redefinition of 'init_module'

     int init_module(void) __attribute__((alias(#initfn)));
         ^

quoted

include/linux/device.h:1321:1: note: in expansion of macro 'module_init'

    module_init(__driver##_init); \
    ^

quoted

include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'

     module_driver(__smd_driver, qcom_smd_driver_register, \
     ^

quoted

drivers/bluetooth/hci_smd.c:219:1: note: in expansion of macro 'module_qcom_smd_driver'

    module_qcom_smd_driver(smd_hci_cmd_driver);
    ^
   include/linux/module.h:130:6: note: previous definition of 'init_module' was here
     int init_module(void) __attribute__((alias(#initfn)));
         ^

quoted

include/linux/device.h:1321:1: note: in expansion of macro 'module_init'

    module_init(__driver##_init); \
    ^

quoted

include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'

     module_driver(__smd_driver, qcom_smd_driver_register, \
     ^
   drivers/bluetooth/hci_smd.c:218:1: note: in expansion of macro 'module_qcom_smd_driver'
    module_qcom_smd_driver(smd_hci_acl_driver);
    ^

quoted

include/linux/module.h:134:27: error: redefinition of '__exittest'

     static inline exitcall_t __exittest(void)  \
                              ^

quoted

include/linux/device.h:1326:1: note: in expansion of macro 'module_exit'

    module_exit(__driver##_exit);
    ^

vim +186 drivers/bluetooth/hci_smd.c

   180	static void smd_hci_cmd_remove(struct qcom_smd_device *sdev)
   181	{
   182		smd_hci.cmd_channel = NULL;
   183		smd_hci_unregister();
   184	}
   185	
 > 186	static const struct qcom_smd_id smd_hci_acl_match[] = {
 > 187		{ .name = "APPS_RIVA_BT_ACL" },
   188		{}
   189	};
   190	
 > 191	static const struct qcom_smd_id smd_hci_cmd_match[] = {
 > 192		{ .name = "APPS_RIVA_BT_CMD" },
   193		{}
   194	};
   195	
   196	static struct qcom_smd_driver smd_hci_acl_driver = {
   197		.probe = smd_hci_acl_probe,
   198		.remove = smd_hci_acl_remove,
   199		.callback = smd_hci_acl_callback,
 > 200		.smd_match_table = smd_hci_acl_match,
   201		.driver  = {
   202			.name  = "qcom_smd_hci_acl",
   203			.owner = THIS_MODULE,
   204		},
   205	};
   206	
   207	static struct qcom_smd_driver smd_hci_cmd_driver = {
   208		.probe = smd_hci_cmd_probe,
   209		.remove = smd_hci_cmd_remove,
   210		.callback = smd_hci_cmd_callback,
 > 211		.smd_match_table = smd_hci_cmd_match,
   212		.driver  = {
   213			.name  = "qcom_smd_hci_cmd",
   214			.owner = THIS_MODULE,
   215		},
   216	};
   217	
 > 218	module_qcom_smd_driver(smd_hci_acl_driver);
 > 219	module_qcom_smd_driver(smd_hci_cmd_driver);
   220	
   221	MODULE_DESCRIPTION("Qualcomm SMD HCI driver");
   222	MODULE_LICENSE("GPL v2");

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

Hi Bjorn,

I wonder if choosing a name like btqcomsmd.ko would not be better here. For now I am fine with keeping hci_smd.ko since there are other issues here to be fixed first. Especially the kbuild test robot complained loudly.

The hci.h include is not needed. If you do, then you are doing something fishy.

A driver that only supports a single instance of a device and uses a global variable to do so is not a good idea. There should be no reason for this. Why is this done this way.

Is it required that this is GFP_ATOMIC or can this actually be GFP_KERNEL?

Why is this a memcpy_fromio here?

+
+	skb->dev = (void *)smd_hci.hci;

This is no longer needed.

This is a double kfree_skb here. The hci_recv_frame will consume the skb no matter what.

If you return an error, then the caller of hdev->send will free the skb.

These two need to at least handling the HCI_RUNNING flag setting and clearing like all the other drivers do.

Use full 0x00 syntax here.

+	buf[5] = sizeof(bdaddr_t);
+	memcpy(buf + 6, bdaddr, sizeof(bdaddr_t));

We also need to be sure that this command only changes the BD_ADDR in RAM. It can not be persistent. When resetting / power cycle the controller it will fallback to its default.

+
+	return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf));

Use __hci_cmd_sync since that is what is suppose to be used in this callback. See the other drivers on how this is done.

I wonder if qca_set_bdaddr_rome is not actually the same command and you could just use that instead.

This is not going to fly. This turns it into two module_{init,exit} calls. That really does not work this way.

I mean why do you need two different drivers in the first place. I should be matching via a single qcom_smd_id in the first place. And you handle everything else in the probe() callback.

For me this is similar to the USB case where you actually have to claim to interfaces. We just lookup the other interface and claim it. It looks like SMD core functionality needs the same feature.

I would prefer if this come in a separate patch from the driver.

Regards

Marcel

On Wed 30 Sep 23:54 PDT 2015, Marcel Holtmann wrote:

Hi Bjorn,

quoted

The Qualcomm WCNSS chip provides two SMD channels to the BT core; one
for command and one for event packets. This driver exposes the two
channels as a hci device.

[..]

quoted

diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 07c9cf381e5a..43c7dc8641ff 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile

@@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART)	+= btuart_cs.o

obj-$(CONFIG_BT_HCIBTUSB)	+= btusb.o
obj-$(CONFIG_BT_HCIBTSDIO)	+= btsdio.o
+obj-$(CONFIG_BT_HCISMD)		+= hci_smd.o

I wonder if choosing a name like btqcomsmd.ko would not be better
here. For now I am fine with keeping hci_smd.ko since there are other
issues here to be fixed first.

I had some problems figuring out the naming scheme here, but btqcomsmd
does seem reasonable, I'll update it in the next set.

Especially the kbuild test robot complained loudly.

Sorry, I forgot to mention in the patch that the patch depends on the
qcom_smd_id patch - that's available in linux-next. I will take another
look, but I think that was the cause of all the issues.

quoted

obj-$(CONFIG_BT_INTEL)		+= btintel.o
obj-$(CONFIG_BT_ATH3K)		+= ath3k.o

diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c

[..]

quoted

+#include <net/bluetooth/hci.h>

The hci.h include is not needed. If you do, then you are doing
something fishy.

Nothing fishy going on here, so I'll drop it.

quoted

+
+static struct {
+	struct qcom_smd_channel *acl_channel;
+	struct qcom_smd_channel *cmd_channel;
+
+	struct hci_dev *hci;
+} smd_hci;

A driver that only supports a single instance of a device and uses a
global variable to do so is not a good idea. There should be no reason
for this. Why is this done this way.

There's two channels coming up, each probing the associated driver that
combines these into 1 hci device.

So I have two options;

* the original idea was to implement multi-channel-per-device support in
  SMD, but as this is the only known case where that's needed I really
  don't want to add all the extra complexity to SMD.

* I can accept the fact that this is silicon inside the MSM SoC and
  should never exist in more than one instance and make this hack. The
  first version I implemented had this structure allocated on the first
  probe, but that didn't really add any value to the static struct.

I picked the second option due to the fact that the patch to SMD was way
bigger then this patch, and full of nasty logic.

quoted

+
+static int smd_hci_recv(unsigned type, const void *data, size_t count)
+{
+	struct sk_buff *skb;
+	void *buf;
+	int ret;
+
+	skb = bt_skb_alloc(count, GFP_ATOMIC);

Is it required that this is GFP_ATOMIC or can this actually be
GFP_KERNEL?

This is being called from IRQ context upon receiving data on the
channels.

quoted

+	if (!skb)
+		return -ENOMEM;
+
+	buf = skb_put(skb, count);
+	memcpy_fromio(buf, data, count);

Why is this a memcpy_fromio here?

A memcpy here doesn't seem to work on ARM64, probably because the
pointer references ioremapped memory. We are discussing either marking
the data __iomem or perhaps using memremap rather then ioremap.

quoted

+
+	skb->dev = (void *)smd_hci.hci;

This is no longer needed.

Thanks

quoted

+	bt_cb(skb)->pkt_type = type;
+	skb_orphan(skb);
+
+	ret = hci_recv_frame(smd_hci.hci, skb);
+	if (ret < 0)
+		kfree_skb(skb);

This is a double kfree_skb here. The hci_recv_frame will consume the
skb no matter what.

Thanks

quoted

+
+	return ret;
+}
+

[..]

quoted

+
+static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	int ret;
+
+	switch (bt_cb(skb)->pkt_type) {
+	case HCI_ACLDATA_PKT:
+	case HCI_SCODATA_PKT:
+		ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len);
+		break;
+	case HCI_COMMAND_PKT:
+		ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len);
+		break;
+	default:
+		ret = -ENODEV;
+		break;
+	}
+
+	kfree_skb(skb);

If you return an error, then the caller of hdev->send will free the
skb.

Must have looked in the wrong kernel tree :(

Thanks.

quoted

+
+	return ret;
+}
+
+static int smd_hci_open(struct hci_dev *hci)
+{
+	return 0;
+}
+
+static int smd_hci_close(struct hci_dev *hci)
+{
+	return 0;
+}

These two need to at least handling the HCI_RUNNING flag setting and
clearing like all the other drivers do.

I'm puzzled to the purpose of this, is this only for indication to user
space or am I missing something?

I found a couple of others that does nothing but set and clear this bit
in their open and close. Would you be open for a framework patch that
provides this functionality to drivers that doesn't need anything else?

quoted

+
+static int smd_hci_set_bdaddr(struct hci_dev *hci,
+			      const bdaddr_t *bdaddr)
+{
+	u8 buf[12];
+
+	buf[0] = 0x0b;
+	buf[1] = 0xfc;
+	buf[2] = 0x9;
+	buf[3] = 0x1;
+	buf[4] = 0x2;

Use full 0x00 syntax here.

Ok

quoted

+	buf[5] = sizeof(bdaddr_t);
+	memcpy(buf + 6, bdaddr, sizeof(bdaddr_t));

We also need to be sure that this command only changes the BD_ADDR in
RAM. It can not be persistent. When resetting / power cycle the
controller it will fallback to its default.

There seems to be no persistent storage at all in this chip, the BD_ADDR
being used comes from one of the firmware files, so this just adjust the
in-memory configuration.

quoted

+
+	return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf));

Use __hci_cmd_sync since that is what is suppose to be used in this
callback. See the other drivers on how this is done.

I did see this, and I first implemented it like that. Unfortunately the
firmware does not ack the command and __hci_cmd_sync() just times out.

I didn't manage to find a way around this.

I wonder if qca_set_bdaddr_rome is not actually the same command and
you could just use that instead.

Looks to be exactly the same thing and gives insight in what those first
5 bytes are. I'll update the magics based on this information.

If I didn't have the issue of the timeout it seems like I should be able
to just call it.

quoted

+}
+
+static int smd_hci_register(void)
+{
+	struct hci_dev *hci;
+	int ret;
+
+	if (smd_hci.hci)
+		return 0;
+
+	/* Wait for both channels to probe before registering */
+	if (!smd_hci.acl_channel || !smd_hci.cmd_channel)
+		return 0;
+
+	hci = hci_alloc_dev();
+	if (!hci)
+		return -ENOMEM;
+
+	hci->bus = HCI_SMD;
+	hci->open = smd_hci_open;
+	hci->close = smd_hci_close;
+	hci->send = smd_hci_send;
+	hci->set_bdaddr = smd_hci_set_bdaddr;
+
+	ret = hci_register_dev(hci);
+	if (ret < 0) {
+		hci_free_dev(hci);
+		return ret;
+	}
+
+	smd_hci.hci = hci;
+
+	return 0;
+}
+
+static void smd_hci_unregister(void)
+{
+	/* Only unregister on the first remove call */
+	if (!smd_hci.hci)
+		return;
+
+	hci_unregister_dev(smd_hci.hci);
+	hci_free_dev(smd_hci.hci);
+	smd_hci.hci = NULL;
+}
+
+static int smd_hci_acl_probe(struct qcom_smd_device *sdev)
+{
+	smd_hci.acl_channel = sdev->channel;
+	smd_hci_register();
+
+	return 0;
+}
+
+static int smd_hci_cmd_probe(struct qcom_smd_device *sdev)
+{
+	smd_hci.cmd_channel = sdev->channel;
+	smd_hci_register();
+
+	return 0;
+}
+
+static void smd_hci_acl_remove(struct qcom_smd_device *sdev)
+{
+	smd_hci.acl_channel = NULL;
+	smd_hci_unregister();
+}
+
+static void smd_hci_cmd_remove(struct qcom_smd_device *sdev)
+{
+	smd_hci.cmd_channel = NULL;
+	smd_hci_unregister();
+}
+
+static const struct qcom_smd_id smd_hci_acl_match[] = {
+	{ .name = "APPS_RIVA_BT_ACL" },
+	{}
+};
+
+static const struct qcom_smd_id smd_hci_cmd_match[] = {
+	{ .name = "APPS_RIVA_BT_CMD" },
+	{}
+};
+
+static struct qcom_smd_driver smd_hci_acl_driver = {
+	.probe = smd_hci_acl_probe,
+	.remove = smd_hci_acl_remove,
+	.callback = smd_hci_acl_callback,
+	.smd_match_table = smd_hci_acl_match,
+	.driver  = {
+		.name  = "qcom_smd_hci_acl",
+		.owner = THIS_MODULE,
+	},
+};
+
+static struct qcom_smd_driver smd_hci_cmd_driver = {
+	.probe = smd_hci_cmd_probe,
+	.remove = smd_hci_cmd_remove,
+	.callback = smd_hci_cmd_callback,
+	.smd_match_table = smd_hci_cmd_match,
+	.driver  = {
+		.name  = "qcom_smd_hci_cmd",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_qcom_smd_driver(smd_hci_acl_driver);
+module_qcom_smd_driver(smd_hci_cmd_driver);

This is not going to fly. This turns it into two module_{init,exit}
calls. That really does not work this way.

Right, I would need to at least fill in my own init and exit functions
with the two registers/unregisters calls...

I mean why do you need two different drivers in the first place. I
should be matching via a single qcom_smd_id in the first place. And
you handle everything else in the probe() callback.

The first iteration had 1 probe, 1 remove and 1 callback. I pulled the
match-data in probe and made the rest use this information by drvdata.
It was the same blocks of code as now, but accessed via the wrapper
drvdata object - so although one shouldn't do like this, this is much
cleaner.

For me this is similar to the USB case where you actually have to
claim to interfaces. We just lookup the other interface and claim it.
It looks like SMD core functionality needs the same feature.

Indeed, as I said above the plan was to implement something where I
either only probe when all channels where up (turned out to be a mess to
implement) or have some accessor function that can be called from probe
to open a second channel.

The latter seems reasonable to do, but it will be way more code than
there's in this driver - which seems to be the only case where that
feature would be needed.

quoted

+
+MODULE_DESCRIPTION("Qualcomm SMD HCI driver");
+MODULE_LICENSE("GPL v2");

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7ca6690355ea..ee5b2dd922f6 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h

@@ -58,6 +58,7 @@

#define HCI_RS232	4
#define HCI_PCI		5
#define HCI_SDIO	6
+#define HCI_SMD		7

I would prefer if this come in a separate patch from the driver.

Sure thing.


Thanks for your review Marcel.

Regards,
Born

Hi Bjorn,

quoted

quoted

The Qualcomm WCNSS chip provides two SMD channels to the BT core; one
for command and one for event packets. This driver exposes the two
channels as a hci device.

[..]

quoted

quoted

diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 07c9cf381e5a..43c7dc8641ff 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile

@@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART)	+= btuart_cs.o

obj-$(CONFIG_BT_HCIBTUSB)	+= btusb.o
obj-$(CONFIG_BT_HCIBTSDIO)	+= btsdio.o
+obj-$(CONFIG_BT_HCISMD)		+= hci_smd.o

I wonder if choosing a name like btqcomsmd.ko would not be better
here. For now I am fine with keeping hci_smd.ko since there are other
issues here to be fixed first.

I had some problems figuring out the naming scheme here, but btqcomsmd
does seem reasonable, I'll update it in the next set.

quoted

Especially the kbuild test robot complained loudly.

Sorry, I forgot to mention in the patch that the patch depends on the
qcom_smd_id patch - that's available in linux-next. I will take another
look, but I think that was the cause of all the issues.

which means, I can only merge this driver when this other patch has hit net-next tree. Unless I take the qcom_smd_id through bluetooth-next tree which doesn't look like a good idea.

quoted

quoted

obj-$(CONFIG_BT_INTEL)		+= btintel.o
obj-$(CONFIG_BT_ATH3K)		+= ath3k.o

diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c

[..]

quoted

quoted

+#include <net/bluetooth/hci.h>

The hci.h include is not needed. If you do, then you are doing
something fishy.

Nothing fishy going on here, so I'll drop it.

quoted

quoted

+
+static struct {
+	struct qcom_smd_channel *acl_channel;
+	struct qcom_smd_channel *cmd_channel;
+
+	struct hci_dev *hci;
+} smd_hci;

A driver that only supports a single instance of a device and uses a
global variable to do so is not a good idea. There should be no reason
for this. Why is this done this way.

There's two channels coming up, each probing the associated driver that
combines these into 1 hci device.

So I have two options;

* the original idea was to implement multi-channel-per-device support in
 SMD, but as this is the only known case where that's needed I really
 don't want to add all the extra complexity to SMD.

* I can accept the fact that this is silicon inside the MSM SoC and
 should never exist in more than one instance and make this hack. The
 first version I implemented had this structure allocated on the first
 probe, but that didn't really add any value to the static struct.

I picked the second option due to the fact that the patch to SMD was way
bigger then this patch, and full of nasty logic.

Writing a driver that assume it is a single instance of a given device is never a good idea. While you might find some instances in the kernel if you look hard enough, but that doesn't mean we should keep doing this. So this needs addressing.

quoted

quoted

+
+static int smd_hci_recv(unsigned type, const void *data, size_t count)
+{
+	struct sk_buff *skb;
+	void *buf;
+	int ret;
+
+	skb = bt_skb_alloc(count, GFP_ATOMIC);

Is it required that this is GFP_ATOMIC or can this actually be
GFP_KERNEL?

This is being called from IRQ context upon receiving data on the
channels.

I wonder why that is needed, but seems an issue in the SMD subsystem. So this is fine, might want to add a comment above the bt_skb_alloc to remind us.

quoted

quoted

+	if (!skb)
+		return -ENOMEM;
+
+	buf = skb_put(skb, count);
+	memcpy_fromio(buf, data, count);

Why is this a memcpy_fromio here?

A memcpy here doesn't seem to work on ARM64, probably because the
pointer references ioremapped memory. We are discussing either marking
the data __iomem or perhaps using memremap rather then ioremap.

I would add a comment here as well to remind everybody why this is used. And marking up the pointers is a good idea as well.

quoted

quoted

+
+	skb->dev = (void *)smd_hci.hci;

This is no longer needed.

Thanks

quoted

quoted

+	bt_cb(skb)->pkt_type = type;
+	skb_orphan(skb);
+
+	ret = hci_recv_frame(smd_hci.hci, skb);
+	if (ret < 0)
+		kfree_skb(skb);

This is a double kfree_skb here. The hci_recv_frame will consume the
skb no matter what.

Thanks

quoted

quoted

+
+	return ret;
+}
+

[..]

quoted

quoted

+
+static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	int ret;
+
+	switch (bt_cb(skb)->pkt_type) {
+	case HCI_ACLDATA_PKT:
+	case HCI_SCODATA_PKT:
+		ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len);
+		break;
+	case HCI_COMMAND_PKT:
+		ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len);
+		break;
+	default:
+		ret = -ENODEV;
+		break;
+	}
+
+	kfree_skb(skb);

quoted

If you return an error, then the caller of hdev->send will free the
skb.

Must have looked in the wrong kernel tree :(

Thanks.

quoted

quoted

+
+	return ret;
+}
+
+static int smd_hci_open(struct hci_dev *hci)
+{
+	return 0;
+}
+
+static int smd_hci_close(struct hci_dev *hci)
+{
+	return 0;
+}

These two need to at least handling the HCI_RUNNING flag setting and
clearing like all the other drivers do.

I'm puzzled to the purpose of this, is this only for indication to user
space or am I missing something?

I found a couple of others that does nothing but set and clear this bit
in their open and close. Would you be open for a framework patch that
provides this functionality to drivers that doesn't need anything else?

This has been in the drivers for as long as the Bluetooth subsystem has been merged upstream. So Linux 2.4.6 to be precise. I had plans to move this code into the core, but that has not happened yet. So until then, lets just include the HCI_RUNNING part.

If there are drivers not doing this, then that is a bug and needs to be fixed. Please point them out to me.

quoted

quoted

+
+static int smd_hci_set_bdaddr(struct hci_dev *hci,
+			      const bdaddr_t *bdaddr)
+{
+	u8 buf[12];
+
+	buf[0] = 0x0b;
+	buf[1] = 0xfc;
+	buf[2] = 0x9;
+	buf[3] = 0x1;
+	buf[4] = 0x2;

Use full 0x00 syntax here.

Ok

quoted

quoted

+	buf[5] = sizeof(bdaddr_t);
+	memcpy(buf + 6, bdaddr, sizeof(bdaddr_t));

We also need to be sure that this command only changes the BD_ADDR in
RAM. It can not be persistent. When resetting / power cycle the
controller it will fallback to its default.

There seems to be no persistent storage at all in this chip, the BD_ADDR
being used comes from one of the firmware files, so this just adjust the
in-memory configuration.

quoted

quoted

+
+	return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf));

Use __hci_cmd_sync since that is what is suppose to be used in this
callback. See the other drivers on how this is done.

I did see this, and I first implemented it like that. Unfortunately the
firmware does not ack the command and __hci_cmd_sync() just times out.

I didn't manage to find a way around this.

What to you mean by this? It does not send a Command Complete? Does it send a Vendor Event instead?

The real problem is that this callback is on purpose synchronous. The core relies on the fact that you only return when it actually completed.

quoted

I wonder if qca_set_bdaddr_rome is not actually the same command and
you could just use that instead.

Looks to be exactly the same thing and gives insight in what those first
5 bytes are. I'll update the magics based on this information.

If I didn't have the issue of the timeout it seems like I should be able
to just call it.

Has something tried to talk to Qualcomm why that timeout happens? They have something like this for the UART speed change which is changed in NVRAM. Maybe this is a more generic way on how they handle things. It would be good if guys from Qualcomm could give some insights here. However in case of their UART driver, this seems to work as it should. Maybe just a newer firmware is needed.

quoted

quoted

+}
+
+static int smd_hci_register(void)
+{
+	struct hci_dev *hci;
+	int ret;
+
+	if (smd_hci.hci)
+		return 0;
+
+	/* Wait for both channels to probe before registering */
+	if (!smd_hci.acl_channel || !smd_hci.cmd_channel)
+		return 0;
+
+	hci = hci_alloc_dev();
+	if (!hci)
+		return -ENOMEM;
+
+	hci->bus = HCI_SMD;
+	hci->open = smd_hci_open;
+	hci->close = smd_hci_close;
+	hci->send = smd_hci_send;
+	hci->set_bdaddr = smd_hci_set_bdaddr;
+
+	ret = hci_register_dev(hci);
+	if (ret < 0) {
+		hci_free_dev(hci);
+		return ret;
+	}
+
+	smd_hci.hci = hci;
+
+	return 0;
+}
+
+static void smd_hci_unregister(void)
+{
+	/* Only unregister on the first remove call */
+	if (!smd_hci.hci)
+		return;
+
+	hci_unregister_dev(smd_hci.hci);
+	hci_free_dev(smd_hci.hci);
+	smd_hci.hci = NULL;
+}
+
+static int smd_hci_acl_probe(struct qcom_smd_device *sdev)
+{
+	smd_hci.acl_channel = sdev->channel;
+	smd_hci_register();
+
+	return 0;
+}
+
+static int smd_hci_cmd_probe(struct qcom_smd_device *sdev)
+{
+	smd_hci.cmd_channel = sdev->channel;
+	smd_hci_register();
+
+	return 0;
+}
+
+static void smd_hci_acl_remove(struct qcom_smd_device *sdev)
+{
+	smd_hci.acl_channel = NULL;
+	smd_hci_unregister();
+}
+
+static void smd_hci_cmd_remove(struct qcom_smd_device *sdev)
+{
+	smd_hci.cmd_channel = NULL;
+	smd_hci_unregister();
+}
+
+static const struct qcom_smd_id smd_hci_acl_match[] = {
+	{ .name = "APPS_RIVA_BT_ACL" },
+	{}
+};
+
+static const struct qcom_smd_id smd_hci_cmd_match[] = {
+	{ .name = "APPS_RIVA_BT_CMD" },
+	{}
+};
+
+static struct qcom_smd_driver smd_hci_acl_driver = {
+	.probe = smd_hci_acl_probe,
+	.remove = smd_hci_acl_remove,
+	.callback = smd_hci_acl_callback,
+	.smd_match_table = smd_hci_acl_match,
+	.driver  = {
+		.name  = "qcom_smd_hci_acl",
+		.owner = THIS_MODULE,
+	},
+};
+
+static struct qcom_smd_driver smd_hci_cmd_driver = {
+	.probe = smd_hci_cmd_probe,
+	.remove = smd_hci_cmd_remove,
+	.callback = smd_hci_cmd_callback,
+	.smd_match_table = smd_hci_cmd_match,
+	.driver  = {
+		.name  = "qcom_smd_hci_cmd",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_qcom_smd_driver(smd_hci_acl_driver);
+module_qcom_smd_driver(smd_hci_cmd_driver);

quoted

This is not going to fly. This turns it into two module_{init,exit}
calls. That really does not work this way.

Right, I would need to at least fill in my own init and exit functions
with the two registers/unregisters calls...

quoted

I mean why do you need two different drivers in the first place. I
should be matching via a single qcom_smd_id in the first place. And
you handle everything else in the probe() callback.

The first iteration had 1 probe, 1 remove and 1 callback. I pulled the
match-data in probe and made the rest use this information by drvdata.
It was the same blocks of code as now, but accessed via the wrapper
drvdata object - so although one shouldn't do like this, this is much
cleaner.

quoted

For me this is similar to the USB case where you actually have to
claim to interfaces. We just lookup the other interface and claim it.
It looks like SMD core functionality needs the same feature.

Indeed, as I said above the plan was to implement something where I
either only probe when all channels where up (turned out to be a mess to
implement) or have some accessor function that can be called from probe
to open a second channel.

The latter seems reasonable to do, but it will be way more code than
there's in this driver - which seems to be the only case where that
feature would be needed.

That is what USB does when claiming a second interface. I would do exactly that and especially be able to make sure that both channels have the same struct device as parent.

Regards

Marcel

On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote:

Hi Bjorn,

[..]

quoted

quoted

Especially the kbuild test robot complained loudly.

Sorry, I forgot to mention in the patch that the patch depends on the
qcom_smd_id patch - that's available in linux-next. I will take another
look, but I think that was the cause of all the issues.

which means, I can only merge this driver when this other patch has
hit net-next tree. Unless I take the qcom_smd_id through
bluetooth-next tree which doesn't look like a good idea.

I was going to suggest that we just wait for -rc1, but the new additions
to SMD adds a bunch of new items on the dependency list.

Perhaps once we agree we can get your Ack and have Andy pull it through
the qcom-soc tree together with the SMD patches?

quoted

quoted

quoted

obj-$(CONFIG_BT_INTEL)              += btintel.o
obj-$(CONFIG_BT_ATH3K)              += ath3k.o

diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c

[..]

quoted

quoted

+#include <net/bluetooth/hci.h>

The hci.h include is not needed. If you do, then you are doing
something fishy.

Nothing fishy going on here, so I'll drop it.

quoted

quoted

+
+static struct {
+   struct qcom_smd_channel *acl_channel;
+   struct qcom_smd_channel *cmd_channel;
+
+   struct hci_dev *hci;
+} smd_hci;

A driver that only supports a single instance of a device and uses a
global variable to do so is not a good idea. There should be no reason
for this. Why is this done this way.

There's two channels coming up, each probing the associated driver that
combines these into 1 hci device.

So I have two options;

* the original idea was to implement multi-channel-per-device support in
 SMD, but as this is the only known case where that's needed I really
 don't want to add all the extra complexity to SMD.

* I can accept the fact that this is silicon inside the MSM SoC and
 should never exist in more than one instance and make this hack. The
 first version I implemented had this structure allocated on the first
 probe, but that didn't really add any value to the static struct.

I picked the second option due to the fact that the patch to SMD was way
bigger then this patch, and full of nasty logic.

Writing a driver that assume it is a single instance of a given device
is never a good idea. While you might find some instances in the
kernel if you look hard enough, but that doesn't mean we should keep
doing this. So this needs addressing.

Fair enough.

quoted

quoted

quoted

+
+static int smd_hci_recv(unsigned type, const void *data, size_t count)
+{
+   struct sk_buff *skb;
+   void *buf;
+   int ret;
+
+   skb = bt_skb_alloc(count, GFP_ATOMIC);

Is it required that this is GFP_ATOMIC or can this actually be
GFP_KERNEL?

This is being called from IRQ context upon receiving data on the
channels.

I wonder why that is needed, but seems an issue in the SMD subsystem.
So this is fine, might want to add a comment above the bt_skb_alloc to
remind us.

It's a design choice of mine, to deliver the incoming data directly from
the interrupt handler. Sure enough we could have had a worker thread or
something do it.

I'll add a comment.

quoted

quoted

quoted

+   if (!skb)
+           return -ENOMEM;
+
+   buf = skb_put(skb, count);
+   memcpy_fromio(buf, data, count);

Why is this a memcpy_fromio here?

A memcpy here doesn't seem to work on ARM64, probably because the
pointer references ioremapped memory. We are discussing either marking
the data __iomem or perhaps using memremap rather then ioremap.

I would add a comment here as well to remind everybody why this is
used. And marking up the pointers is a good idea as well.

Yeah, I have this on the todo.
Will put a comment here for now.

[..]

quoted

quoted

quoted

+
+   return ret;
+}
+
+static int smd_hci_open(struct hci_dev *hci)
+{
+   return 0;
+}
+
+static int smd_hci_close(struct hci_dev *hci)
+{
+   return 0;
+}

These two need to at least handling the HCI_RUNNING flag setting and
clearing like all the other drivers do.

I'm puzzled to the purpose of this, is this only for indication to user
space or am I missing something?

I found a couple of others that does nothing but set and clear this bit
in their open and close. Would you be open for a framework patch that
provides this functionality to drivers that doesn't need anything else?

This has been in the drivers for as long as the Bluetooth subsystem
has been merged upstream. So Linux 2.4.6 to be precise. I had plans to
move this code into the core, but that has not happened yet. So until
then, lets just include the HCI_RUNNING part.

I'll add the pieces.

If there are drivers not doing this, then that is a bug and needs to
be fixed. Please point them out to me.

What I meant was that I couldn't find anyone consuming this bit, the
cases I looked at all fiddled with it in various ways.

But I presume it can be seen from userspace.

quoted

quoted

quoted

+
+static int smd_hci_set_bdaddr(struct hci_dev *hci,
+                         const bdaddr_t *bdaddr)
+{
+   u8 buf[12];
+
+   buf[0] = 0x0b;
+   buf[1] = 0xfc;
+   buf[2] = 0x9;
+   buf[3] = 0x1;
+   buf[4] = 0x2;

Use full 0x00 syntax here.

Ok

quoted

quoted

+   buf[5] = sizeof(bdaddr_t);
+   memcpy(buf + 6, bdaddr, sizeof(bdaddr_t));

We also need to be sure that this command only changes the BD_ADDR in
RAM. It can not be persistent. When resetting / power cycle the
controller it will fallback to its default.

There seems to be no persistent storage at all in this chip, the BD_ADDR
being used comes from one of the firmware files, so this just adjust the
in-memory configuration.

quoted

quoted

+
+   return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf));

Use __hci_cmd_sync since that is what is suppose to be used in this
callback. See the other drivers on how this is done.

I did see this, and I first implemented it like that. Unfortunately the
firmware does not ack the command and __hci_cmd_sync() just times out.

I didn't manage to find a way around this.

What to you mean by this? It does not send a Command Complete? Does it
send a Vendor Event instead?

The immediate problem I had seems to be that I specified the wrong event
type. With this corrected it passes most of the time - but in some cases
I don't get the command complete packet back.

I don't know what causes this and I don't have access to the source code
of the other side :/

The real problem is that this callback is on purpose synchronous. The
core relies on the fact that you only return when it actually
completed.

Makes sense.

quoted

quoted

I wonder if qca_set_bdaddr_rome is not actually the same command and
you could just use that instead.

Looks to be exactly the same thing and gives insight in what those first
5 bytes are. I'll update the magics based on this information.

If I didn't have the issue of the timeout it seems like I should be able
to just call it.

Has something tried to talk to Qualcomm why that timeout happens? They
have something like this for the UART speed change which is changed in
NVRAM. Maybe this is a more generic way on how they handle things. It
would be good if guys from Qualcomm could give some insights here.
However in case of their UART driver, this seems to work as it should.
Maybe just a newer firmware is needed.

Unfortunately this part of Qualcomm hasn't joined the dance yet.

When booting the chip we load a file with "NV values", so I presume that
this command allows us to patch those entries in runtime.

[..]

That is what USB does when claiming a second interface. I would do
exactly that and especially be able to make sure that both channels
have the same struct device as parent.

I was hoping to not have to implement this, as it does complicates the
life cycle management of things in the SMD core but agrees with you that
it's cleaner.

I'll finish up the rewrite and send out a new version based on this.

Thanks for the review Marcel!

Regards,
Bjorn

On Thu, Oct 08, 2015 at 10:07:44AM -0700, Bjorn Andersson wrote:

On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote:

quoted

Hi Bjorn,

[..]

quoted

quoted

quoted

Especially the kbuild test robot complained loudly.

Sorry, I forgot to mention in the patch that the patch depends on the
qcom_smd_id patch - that's available in linux-next. I will take another
look, but I think that was the cause of all the issues.

which means, I can only merge this driver when this other patch has
hit net-next tree. Unless I take the qcom_smd_id through
bluetooth-next tree which doesn't look like a good idea.

I was going to suggest that we just wait for -rc1, but the new additions
to SMD adds a bunch of new items on the dependency list.

Perhaps once we agree we can get your Ack and have Andy pull it through
the qcom-soc tree together with the SMD patches?

This is fine by me.

quoted

quoted

quoted

quoted

obj-$(CONFIG_BT_INTEL)              += btintel.o
obj-$(CONFIG_BT_ATH3K)              += ath3k.o

diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c

[..]

quoted

quoted

+#include <net/bluetooth/hci.h>

The hci.h include is not needed. If you do, then you are doing
something fishy.

<snip>

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