Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver
On Thu, Oct 08, 2015 at 10:07:44AM -0700, Bjorn Andersson wrote: > On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote: > > > Hi Bjorn, > > > [..] > > >> 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. > > >>> 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 > > > [..] > > >>> +#include > > >> > > >> The hci.h include is not needed. If you do, then you are doing > > >> something fishy. > > >> > > > -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver
On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote:
> Hi Bjorn,
>
[..]
> >> 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?
> >>> 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
> > [..]
> >>> +#include
> >>
> >> 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.
> >
> >>> +
> >>> +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.
> >
> >>> +
> >>> +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.
> >
> >>> + 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.
[..]
> >>> +
> >>> + 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.
> I
Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver
Hi Bjorn,
>>> 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.
>>>
> [..]
>>> 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.
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.
>>> 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
> [..]
>>> +#include
>>
>> 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.
>
>>> +
>>> +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.
>
>>> +
>>> +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.
>
>>> + 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.
>>> +
>>> + skb->dev = (void *)smd_hci.hci;
>>
>> This is no longer needed.
>>
>
> Thanks
>
>>> + 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
>
>>> +
>>> + return ret;
>>> +}
>>> +
> [..]
>>> +
>>> +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);
>
Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver
On Wed 30 Sep 23:54 PDT 2015, Marcel Holtmann wrote:
> Hi Bjorn,
>
> > 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.
> >
[..]
> > 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.
> > 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
[..]
> > +#include
>
> 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.
> > +
> > +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.
> > +
> > +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.
> > + 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.
> > +
> > + skb->dev = (void *)smd_hci.hci;
>
> This is no longer needed.
>
Thanks
> > + 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
> > +
> > + return ret;
> > +}
> > +
[..]
> > +
> > +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.
> > +
> > + 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 t
Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver
Hi Bjorn,
> 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
> ---
> 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
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.
> 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 ..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
> +#include
> +#include
> +#include
> +#include
> +#include
The hci.h include is not needed. If you do, then you are doing something fishy.
> +
> +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.
> +
> +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?
> + if (!skb)
> + return -ENOMEM;
> +
> + buf = skb_put(skb, count);
> + memcpy_fromio(buf, data, count);
Why is this a memcpy_fromio here?
> +
> + skb->dev = (void *)smd_hci.hci;
This is no longer needed.
> + 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.
> +
> + 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);
> +
Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver
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 >>):
>> drivers/bluetooth/hci_smd.c:186:33: error: array type has incomplete element
>> type
static const struct qcom_smd_id smd_hci_acl_match[] = {
^
>> 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')
>> drivers/bluetooth/hci_smd.c:200:2: error: unknown field 'smd_match_table'
>> specified in initializer
.smd_match_table = smd_hci_acl_match,
^
>> 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
>> 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:
>> include/linux/module.h:128:27: error: redefinition of '__inittest'
static inline initcall_t __inittest(void) \
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> 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: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) \
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> 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);
^
>> include/linux/module.h:130:6: error: redefinition of 'init_module'
int init_module(void) __attribute__((alias(#initfn)));
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> 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: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)));
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> 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);
^
>> include/linux/module.h:134:27: error: redefinition of '__exittest'
static inline exitcall_t __exittest(void) \
^
>> 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 };
[PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver
From: Bjorn Andersson
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
---
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 ..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
+#include
+#include
+#include
+#include
+#include
+
+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 chann
