All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7] Bluetooth: btwilink driver
@ 2010-11-26  9:20 pavan_savoy
  2010-11-30  7:29   ` Pavan Savoy
  2010-11-30 15:46 ` Gustavo F. Padovan
  0 siblings, 2 replies; 15+ messages in thread
From: pavan_savoy @ 2010-11-26  9:20 UTC (permalink / raw)
  To: padovan, marcel; +Cc: linux-bluetooth, linux-kernel, Pavan Savoy

From: Pavan Savoy <pavan_savoy@ti.com>

Marcel, Gustavo,

comments attended to from v5 and v6,

1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
Now I handle for EINPROGRESS - which is not really an error and
return during all other error cases.

2. _write is still a function pointer and not an exported function, I
need to change the underlying driver's code for this.
However, previous lkml comments on the underlying driver's code
suggested it to be kept as a function pointer and not EXPORT.
Gustavo, Marcel - Please comment on this.
Is this absolutely required? If so why?

3. test_and_set_bit of HCI_RUNNING is done at beginning of
ti_st_open, and did not see issues during firmware download.
However ideally I would still like to set HCI_RUNNING once the firmware
download is done, because I don't want to allow a _send_frame during
firmware download - Marcel, Gustavo - Please comment.

4. test_and_clear of HCI_RUNNING now done @ beginning of close.

5. EAGAIN on failure of st_write is to suggest to try and write again.
I have never this happen - However only if UART goes bad this case may
occur.

6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
fact the code is pretty much borrowed from there.
Marcel, Gustavo - Please suggest where should it be done? If not here.

7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.

8. platform_driver registration inside module_init now is similar to
other drivers.

9. Dan Carpenter's comments on leaking hst memory on failed
hci_register_dev and empty space after quotes in debug statements
fixed.

Thanks for the comments...
Sorry, for previously not being very clear on which comments were
handled and which were not.

-- patch description --

This is the bluetooth protocol driver for the TI WiLink7 chipsets.
Texas Instrument's WiLink chipsets combine wireless technologies
like BT, FM, GPS and WLAN onto a single chip.

This Bluetooth driver works on top of the TI_ST shared transport
line discipline driver which also allows other drivers like
FM V4L2 and GPS character driver to make use of the same UART interface.

Kconfig and Makefile modifications to enable the Bluetooth
driver for Texas Instrument's WiLink 7 chipset.

Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
---
 drivers/bluetooth/Kconfig    |   10 ++
 drivers/bluetooth/Makefile   |    1 +
 drivers/bluetooth/btwilink.c |  363 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 374 insertions(+), 0 deletions(-)
 create mode 100644 drivers/bluetooth/btwilink.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 02deef4..8e0de9a 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -219,4 +219,14 @@ config BT_ATH3K
 	  Say Y here to compile support for "Atheros firmware download driver"
 	  into the kernel or say M to compile it as module (ath3k).
 
+config BT_WILINK
+	tristate "Texas Instruments WiLink7 driver"
+	depends on TI_ST
+	help
+	  This enables the Bluetooth driver for Texas Instrument's BT/FM/GPS
+	  combo devices. This makes use of shared transport line discipline
+	  core driver to communicate with the BT core of the combo chip.
+
+	  Say Y here to compile support for Texas Instrument's WiLink7 driver
+	  into the kernel or say M to compile it as module.
 endmenu
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 71bdf13..f4460f4 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO)	+= btsdio.o
 obj-$(CONFIG_BT_ATH3K)		+= ath3k.o
 obj-$(CONFIG_BT_MRVL)		+= btmrvl.o
 obj-$(CONFIG_BT_MRVL_SDIO)	+= btmrvl_sdio.o
+obj-$(CONFIG_BT_WILINK)		+= btwilink.o
 
 btmrvl-y			:= btmrvl_main.o
 btmrvl-$(CONFIG_DEBUG_FS)	+= btmrvl_debugfs.o
diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
new file mode 100644
index 0000000..71e69f8
--- /dev/null
+++ b/drivers/bluetooth/btwilink.c
@@ -0,0 +1,363 @@
+/*
+ *  Texas Instrument's Bluetooth Driver For Shared Transport.
+ *
+ *  Bluetooth Driver acts as interface between HCI core and
+ *  TI Shared Transport Layer.
+ *
+ *  Copyright (C) 2009-2010 Texas Instruments
+ *  Author: Raja Mani <raja_mani@ti.com>
+ *	Pavan Savoy <pavan_savoy@ti.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License 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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include <linux/ti_wilink_st.h>
+
+/* Bluetooth Driver Version */
+#define VERSION               "1.0"
+
+/* Number of seconds to wait for registration completion
+ * when ST returns PENDING status.
+ */
+#define BT_REGISTER_TIMEOUT   6000	/* 6 sec */
+
+/**
+ * struct ti_st - driver operation structure
+ * @hdev: hci device pointer which binds to bt driver
+ * @reg_status: ST registration callback status
+ * @st_write: write function provided by the ST driver
+ *	to be used by the driver during send_frame.
+ * @wait_reg_completion - completion sync between ti_st_open
+ *	and ti_st_registration_completion_cb.
+ */
+struct ti_st {
+	struct hci_dev *hdev;
+	char reg_status;
+	long (*st_write) (struct sk_buff *);
+	struct completion wait_reg_completion;
+};
+
+/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
+static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
+{
+	struct hci_dev *hdev = hst->hdev;
+
+	/* Update HCI stat counters */
+	switch (pkt_type) {
+	case HCI_COMMAND_PKT:
+		hdev->stat.cmd_tx++;
+		break;
+
+	case HCI_ACLDATA_PKT:
+		hdev->stat.acl_tx++;
+		break;
+
+	case HCI_SCODATA_PKT:
+		hdev->stat.sco_tx++;
+		break;
+	}
+}
+
+/* ------- Interfaces to Shared Transport ------ */
+
+/* Called by ST layer to indicate protocol registration completion
+ * status.ti_st_open() function will wait for signal from this
+ * API when st_register() function returns ST_PENDING.
+ */
+static void st_registration_completion_cb(void *priv_data, char data)
+{
+	struct ti_st *lhst = priv_data;
+
+	/* Save registration status for use in ti_st_open() */
+	lhst->reg_status = data;
+	/* complete the wait in ti_st_open() */
+	complete(&lhst->wait_reg_completion);
+}
+
+/* Called by Shared Transport layer when receive data is
+ * available */
+static long st_receive(void *priv_data, struct sk_buff *skb)
+{
+	struct ti_st *lhst = priv_data;
+	int err;
+
+	if (!skb)
+		return -EFAULT;
+
+	if (!lhst) {
+		kfree_skb(skb);
+		return -EFAULT;
+	}
+
+	skb->dev = (void *) lhst->hdev;
+
+	/* Forward skb to HCI core layer */
+	err = hci_recv_frame(skb);
+	if (err < 0) {
+		BT_ERR("Unable to push skb to HCI core(%d)", err);
+		return err;
+	}
+
+	lhst->hdev->stat.byte_rx += skb->len;
+
+	return 0;
+}
+
+/* ------- Interfaces to HCI layer ------ */
+/* protocol structure registered with shared transport */
+static struct st_proto_s ti_st_proto = {
+	.type = ST_BT,
+	.recv = st_receive,
+	.reg_complete_cb = st_registration_completion_cb,
+};
+
+/* Called from HCI core to initialize the device */
+static int ti_st_open(struct hci_dev *hdev)
+{
+	unsigned long timeleft;
+	struct ti_st *hst;
+	int err;
+
+	BT_DBG("%s %p", hdev->name, hdev);
+	if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
+		BT_ERR("btwilink already opened");
+		return -EBUSY;
+	}
+
+	/* provide contexts for callbacks from ST */
+	hst = hdev->driver_data;
+	ti_st_proto.priv_data = hst;
+
+	err = st_register(&ti_st_proto);
+	if (err == -EINPROGRESS) {
+		/* ST is busy with either protocol registration or firmware
+		 * download.
+		 */
+		/* Prepare wait-for-completion handler data structures.
+		 */
+		init_completion(&hst->wait_reg_completion);
+
+		/* Reset ST registration callback status flag , this value
+		 * will be updated in ti_st_registration_completion_cb()
+		 * function whenever it called from ST driver.
+		 */
+		hst->reg_status = -EINPROGRESS;
+
+		BT_DBG("waiting for registration completion signal from ST");
+		timeleft = wait_for_completion_timeout
+			(&hst->wait_reg_completion,
+			 msecs_to_jiffies(BT_REGISTER_TIMEOUT));
+		if (!timeleft) {
+			clear_bit(HCI_RUNNING, &hdev->flags);
+			BT_ERR("Timeout(%d sec),didn't get reg "
+					"completion signal from ST",
+					BT_REGISTER_TIMEOUT / 1000);
+			return -ETIMEDOUT;
+		}
+
+		/* Is ST registration callback called with ERROR status? */
+		if (hst->reg_status != 0) {
+			clear_bit(HCI_RUNNING, &hdev->flags);
+			BT_ERR("ST registration completed with invalid "
+					"status %d", hst->reg_status);
+			return -EAGAIN;
+		}
+		err = 0;
+	} else if (err != 0) {
+		clear_bit(HCI_RUNNING, &hdev->flags);
+		BT_ERR("st_register failed %d", err);
+		return err;
+	}
+
+	/* ti_st_proto.write is filled up by the underlying shared
+	 * transport driver upon registration
+	 */
+	hst->st_write = ti_st_proto.write;
+	if (!hst->st_write) {
+		BT_ERR("undefined ST write function");
+		clear_bit(HCI_RUNNING, &hdev->flags);
+
+		/* Undo registration with ST */
+		err = st_unregister(ST_BT);
+		if (err)
+			BT_ERR("st_unregister() failed with error %d", err);
+
+		hst->st_write = NULL;
+		return err;
+	}
+
+	return err;
+}
+
+/* Close device */
+static int ti_st_close(struct hci_dev *hdev)
+{
+	int err;
+	struct ti_st *hst = hdev->driver_data;
+
+	if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
+		return 0;
+
+	/* continue to unregister from transport */
+	err = st_unregister(ST_BT);
+	if (err)
+		BT_ERR("st_unregister() failed with error %d", err);
+
+	hst->st_write = NULL;
+
+	return err;
+}
+
+static int ti_st_send_frame(struct sk_buff *skb)
+{
+	struct hci_dev *hdev;
+	struct ti_st *hst;
+	long len;
+
+	hdev = (struct hci_dev *)skb->dev;
+
+	if (!test_bit(HCI_RUNNING, &hdev->flags))
+		return -EBUSY;
+
+	hst = hdev->driver_data;
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
+
+	BT_DBG("%s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
+			skb->len);
+
+	/* Insert skb to shared transport layer's transmit queue.
+	 * Freeing skb memory is taken care in shared transport layer,
+	 * so don't free skb memory here.
+	 */
+	len = hst->st_write(skb);
+	if (len < 0) {
+		kfree_skb(skb);
+		BT_ERR("ST write failed (%ld)", len);
+		/* Try Again, would only fail if UART has gone bad */
+		return -EAGAIN;
+	}
+
+	/* ST accepted our skb. So, Go ahead and do rest */
+	hdev->stat.byte_tx += len;
+	ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
+
+	return 0;
+}
+
+static void ti_st_destruct(struct hci_dev *hdev)
+{
+	BT_DBG("%s", hdev->name);
+	kfree(hdev->driver_data);
+}
+
+static int bt_ti_probe(struct platform_device *pdev)
+{
+	static struct ti_st *hst;
+	struct hci_dev *hdev;
+	int err;
+
+	hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
+	if (!hst)
+		return -ENOMEM;
+
+	/* Expose "hciX" device to user space */
+	hdev = hci_alloc_dev();
+	if (!hdev) {
+		kfree(hst);
+		return -ENOMEM;
+	}
+
+	BT_DBG("hdev %p", hdev);
+
+	hst->hdev = hdev;
+	hdev->bus = HCI_UART;
+	hdev->driver_data = hst;
+	hdev->open = ti_st_open;
+	hdev->close = ti_st_close;
+	hdev->flush = NULL;
+	hdev->send = ti_st_send_frame;
+	hdev->destruct = ti_st_destruct;
+	hdev->owner = THIS_MODULE;
+
+	err = hci_register_dev(hdev);
+	if (err < 0) {
+		BT_ERR("Can't register HCI device error %d", err);
+		kfree(hst);
+		hci_free_dev(hdev);
+		return err;
+	}
+
+	BT_DBG("HCI device registered (hdev %p)", hdev);
+
+	dev_set_drvdata(&pdev->dev, hst);
+	return err;
+}
+
+static int bt_ti_remove(struct platform_device *pdev)
+{
+	struct hci_dev *hdev;
+	struct ti_st *hst = dev_get_drvdata(&pdev->dev);
+
+	if (!hst)
+		return -EFAULT;
+
+	hdev = hst->hdev;
+	ti_st_close(hdev);
+	hci_unregister_dev(hdev);
+
+	hci_free_dev(hdev);
+	kfree(hst);
+
+	dev_set_drvdata(&pdev->dev, NULL);
+	return 0;
+}
+
+static struct platform_driver btwilink_driver = {
+	.probe = bt_ti_probe,
+	.remove = bt_ti_remove,
+	.driver = {
+		.name = "btwilink",
+		.owner = THIS_MODULE,
+	},
+};
+
+/* ------- Module Init/Exit interfaces ------ */
+static int __init btwilink_init(void)
+{
+	BT_INFO("Bluetooth Driver for TI WiLink - Version %s", VERSION);
+
+	return platform_driver_register(&btwilink_driver);
+}
+
+static void __exit btwilink_exit(void)
+{
+	platform_driver_unregister(&btwilink_driver);
+}
+
+module_init(btwilink_init);
+module_exit(btwilink_exit);
+
+/* ------ Module Info ------ */
+
+MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
+MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
-- 
1.5.6.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-11-26  9:20 [PATCH v7] Bluetooth: btwilink driver pavan_savoy
@ 2010-11-30  7:29   ` Pavan Savoy
  2010-11-30 15:46 ` Gustavo F. Padovan
  1 sibling, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-11-30  7:29 UTC (permalink / raw)
  To: padovan, marcel; +Cc: linux-bluetooth, linux-kernel, Pavan Savoy

On Fri, Nov 26, 2010 at 2:50 PM,  <pavan_savoy@ti.com> wrote:
> From: Pavan Savoy <pavan_savoy@ti.com>
>
Marcel, Gustavo,

Please find some time to comment and also answer some of the
concerns I have below,

Thanks & Regards,
Pavan Savoy.

> comments attended to from v5 and v6,
>
> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
> Now I handle for EINPROGRESS - which is not really an error and
> return during all other error cases.
>
> 2. _write is still a function pointer and not an exported function, I
> need to change the underlying driver's code for this.
> However, previous lkml comments on the underlying driver's code
> suggested it to be kept as a function pointer and not EXPORT.
> Gustavo, Marcel - Please comment on this.
> Is this absolutely required? If so why?
>
> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
> ti_st_open, and did not see issues during firmware download.
> However ideally I would still like to set HCI_RUNNING once the firmware
> download is done, because I don't want to allow a _send_frame during
> firmware download - Marcel, Gustavo - Please comment.
>
> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
>
> 5. EAGAIN on failure of st_write is to suggest to try and write again.
> I have never this happen - However only if UART goes bad this case may
> occur.
>
> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
> fact the code is pretty much borrowed from there.
> Marcel, Gustavo - Please suggest where should it be done? If not here.
>
> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.
>
> 8. platform_driver registration inside module_init now is similar to
> other drivers.
>
> 9. Dan Carpenter's comments on leaking hst memory on failed
> hci_register_dev and empty space after quotes in debug statements
> fixed.
>
> Thanks for the comments...
> Sorry, for previously not being very clear on which comments were
> handled and which were not.
>
> -- patch description --
>
> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> Texas Instrument's WiLink chipsets combine wireless technologies
> like BT, FM, GPS and WLAN onto a single chip.
>
> This Bluetooth driver works on top of the TI_ST shared transport
> line discipline driver which also allows other drivers like
> FM V4L2 and GPS character driver to make use of the same UART interface.
>
> Kconfig and Makefile modifications to enable the Bluetooth
> driver for Texas Instrument's WiLink 7 chipset.
>
> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> ---
>  drivers/bluetooth/Kconfig    |   10 ++
>  drivers/bluetooth/Makefile   |    1 +
>  drivers/bluetooth/btwilink.c |  363 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 374 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/bluetooth/btwilink.c
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..8e0de9a 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
>          Say Y here to compile support for "Atheros firmware download driver"
>          into the kernel or say M to compile it as module (ath3k).
>
> +config BT_WILINK
> +       tristate "Texas Instruments WiLink7 driver"
> +       depends on TI_ST
> +       help
> +         This enables the Bluetooth driver for Texas Instrument's BT/FM/GPS
> +         combo devices. This makes use of shared transport line discipline
> +         core driver to communicate with the BT core of the combo chip.
> +
> +         Say Y here to compile support for Texas Instrument's WiLink7 driver
> +         into the kernel or say M to compile it as module.
>  endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 71bdf13..f4460f4 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO)    += btsdio.o
>  obj-$(CONFIG_BT_ATH3K)         += ath3k.o
>  obj-$(CONFIG_BT_MRVL)          += btmrvl.o
>  obj-$(CONFIG_BT_MRVL_SDIO)     += btmrvl_sdio.o
> +obj-$(CONFIG_BT_WILINK)                += btwilink.o
>
>  btmrvl-y                       := btmrvl_main.o
>  btmrvl-$(CONFIG_DEBUG_FS)      += btmrvl_debugfs.o
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..71e69f8
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> @@ -0,0 +1,363 @@
> +/*
> + *  Texas Instrument's Bluetooth Driver For Shared Transport.
> + *
> + *  Bluetooth Driver acts as interface between HCI core and
> + *  TI Shared Transport Layer.
> + *
> + *  Copyright (C) 2009-2010 Texas Instruments
> + *  Author: Raja Mani <raja_mani@ti.com>
> + *     Pavan Savoy <pavan_savoy@ti.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License 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.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/ti_wilink_st.h>
> +
> +/* Bluetooth Driver Version */
> +#define VERSION               "1.0"
> +
> +/* Number of seconds to wait for registration completion
> + * when ST returns PENDING status.
> + */
> +#define BT_REGISTER_TIMEOUT   6000     /* 6 sec */
> +
> +/**
> + * struct ti_st - driver operation structure
> + * @hdev: hci device pointer which binds to bt driver
> + * @reg_status: ST registration callback status
> + * @st_write: write function provided by the ST driver
> + *     to be used by the driver during send_frame.
> + * @wait_reg_completion - completion sync between ti_st_open
> + *     and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> +       struct hci_dev *hdev;
> +       char reg_status;
> +       long (*st_write) (struct sk_buff *);
> +       struct completion wait_reg_completion;
> +};
> +
> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
> +{
> +       struct hci_dev *hdev = hst->hdev;
> +
> +       /* Update HCI stat counters */
> +       switch (pkt_type) {
> +       case HCI_COMMAND_PKT:
> +               hdev->stat.cmd_tx++;
> +               break;
> +
> +       case HCI_ACLDATA_PKT:
> +               hdev->stat.acl_tx++;
> +               break;
> +
> +       case HCI_SCODATA_PKT:
> +               hdev->stat.sco_tx++;
> +               break;
> +       }
> +}
> +
> +/* ------- Interfaces to Shared Transport ------ */
> +
> +/* Called by ST layer to indicate protocol registration completion
> + * status.ti_st_open() function will wait for signal from this
> + * API when st_register() function returns ST_PENDING.
> + */
> +static void st_registration_completion_cb(void *priv_data, char data)
> +{
> +       struct ti_st *lhst = priv_data;
> +
> +       /* Save registration status for use in ti_st_open() */
> +       lhst->reg_status = data;
> +       /* complete the wait in ti_st_open() */
> +       complete(&lhst->wait_reg_completion);
> +}
> +
> +/* Called by Shared Transport layer when receive data is
> + * available */
> +static long st_receive(void *priv_data, struct sk_buff *skb)
> +{
> +       struct ti_st *lhst = priv_data;
> +       int err;
> +
> +       if (!skb)
> +               return -EFAULT;
> +
> +       if (!lhst) {
> +               kfree_skb(skb);
> +               return -EFAULT;
> +       }
> +
> +       skb->dev = (void *) lhst->hdev;
> +
> +       /* Forward skb to HCI core layer */
> +       err = hci_recv_frame(skb);
> +       if (err < 0) {
> +               BT_ERR("Unable to push skb to HCI core(%d)", err);
> +               return err;
> +       }
> +
> +       lhst->hdev->stat.byte_rx += skb->len;
> +
> +       return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto = {
> +       .type = ST_BT,
> +       .recv = st_receive,
> +       .reg_complete_cb = st_registration_completion_cb,
> +};
> +
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> +       unsigned long timeleft;
> +       struct ti_st *hst;
> +       int err;
> +
> +       BT_DBG("%s %p", hdev->name, hdev);
> +       if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
> +               BT_ERR("btwilink already opened");
> +               return -EBUSY;
> +       }
> +
> +       /* provide contexts for callbacks from ST */
> +       hst = hdev->driver_data;
> +       ti_st_proto.priv_data = hst;
> +
> +       err = st_register(&ti_st_proto);
> +       if (err == -EINPROGRESS) {
> +               /* ST is busy with either protocol registration or firmware
> +                * download.
> +                */
> +               /* Prepare wait-for-completion handler data structures.
> +                */
> +               init_completion(&hst->wait_reg_completion);
> +
> +               /* Reset ST registration callback status flag , this value
> +                * will be updated in ti_st_registration_completion_cb()
> +                * function whenever it called from ST driver.
> +                */
> +               hst->reg_status = -EINPROGRESS;
> +
> +               BT_DBG("waiting for registration completion signal from ST");
> +               timeleft = wait_for_completion_timeout
> +                       (&hst->wait_reg_completion,
> +                        msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> +               if (!timeleft) {
> +                       clear_bit(HCI_RUNNING, &hdev->flags);
> +                       BT_ERR("Timeout(%d sec),didn't get reg "
> +                                       "completion signal from ST",
> +                                       BT_REGISTER_TIMEOUT / 1000);
> +                       return -ETIMEDOUT;
> +               }
> +
> +               /* Is ST registration callback called with ERROR status? */
> +               if (hst->reg_status != 0) {
> +                       clear_bit(HCI_RUNNING, &hdev->flags);
> +                       BT_ERR("ST registration completed with invalid "
> +                                       "status %d", hst->reg_status);
> +                       return -EAGAIN;
> +               }
> +               err = 0;
> +       } else if (err != 0) {
> +               clear_bit(HCI_RUNNING, &hdev->flags);
> +               BT_ERR("st_register failed %d", err);
> +               return err;
> +       }
> +
> +       /* ti_st_proto.write is filled up by the underlying shared
> +        * transport driver upon registration
> +        */
> +       hst->st_write = ti_st_proto.write;
> +       if (!hst->st_write) {
> +               BT_ERR("undefined ST write function");
> +               clear_bit(HCI_RUNNING, &hdev->flags);
> +
> +               /* Undo registration with ST */
> +               err = st_unregister(ST_BT);
> +               if (err)
> +                       BT_ERR("st_unregister() failed with error %d", err);
> +
> +               hst->st_write = NULL;
> +               return err;
> +       }
> +
> +       return err;
> +}
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> +       int err;
> +       struct ti_st *hst = hdev->driver_data;
> +
> +       if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
> +               return 0;
> +
> +       /* continue to unregister from transport */
> +       err = st_unregister(ST_BT);
> +       if (err)
> +               BT_ERR("st_unregister() failed with error %d", err);
> +
> +       hst->st_write = NULL;
> +
> +       return err;
> +}
> +
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> +       struct hci_dev *hdev;
> +       struct ti_st *hst;
> +       long len;
> +
> +       hdev = (struct hci_dev *)skb->dev;
> +
> +       if (!test_bit(HCI_RUNNING, &hdev->flags))
> +               return -EBUSY;
> +
> +       hst = hdev->driver_data;
> +
> +       /* Prepend skb with frame type */
> +       memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> +       BT_DBG("%s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
> +                       skb->len);
> +
> +       /* Insert skb to shared transport layer's transmit queue.
> +        * Freeing skb memory is taken care in shared transport layer,
> +        * so don't free skb memory here.
> +        */
> +       len = hst->st_write(skb);
> +       if (len < 0) {
> +               kfree_skb(skb);
> +               BT_ERR("ST write failed (%ld)", len);
> +               /* Try Again, would only fail if UART has gone bad */
> +               return -EAGAIN;
> +       }
> +
> +       /* ST accepted our skb. So, Go ahead and do rest */
> +       hdev->stat.byte_tx += len;
> +       ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> +       return 0;
> +}
> +
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> +       BT_DBG("%s", hdev->name);
> +       kfree(hdev->driver_data);
> +}
> +
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> +       static struct ti_st *hst;
> +       struct hci_dev *hdev;
> +       int err;
> +
> +       hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> +       if (!hst)
> +               return -ENOMEM;
> +
> +       /* Expose "hciX" device to user space */
> +       hdev = hci_alloc_dev();
> +       if (!hdev) {
> +               kfree(hst);
> +               return -ENOMEM;
> +       }
> +
> +       BT_DBG("hdev %p", hdev);
> +
> +       hst->hdev = hdev;
> +       hdev->bus = HCI_UART;
> +       hdev->driver_data = hst;
> +       hdev->open = ti_st_open;
> +       hdev->close = ti_st_close;
> +       hdev->flush = NULL;
> +       hdev->send = ti_st_send_frame;
> +       hdev->destruct = ti_st_destruct;
> +       hdev->owner = THIS_MODULE;
> +
> +       err = hci_register_dev(hdev);
> +       if (err < 0) {
> +               BT_ERR("Can't register HCI device error %d", err);
> +               kfree(hst);
> +               hci_free_dev(hdev);
> +               return err;
> +       }
> +
> +       BT_DBG("HCI device registered (hdev %p)", hdev);
> +
> +       dev_set_drvdata(&pdev->dev, hst);
> +       return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> +       struct hci_dev *hdev;
> +       struct ti_st *hst = dev_get_drvdata(&pdev->dev);
> +
> +       if (!hst)
> +               return -EFAULT;
> +
> +       hdev = hst->hdev;
> +       ti_st_close(hdev);
> +       hci_unregister_dev(hdev);
> +
> +       hci_free_dev(hdev);
> +       kfree(hst);
> +
> +       dev_set_drvdata(&pdev->dev, NULL);
> +       return 0;
> +}
> +
> +static struct platform_driver btwilink_driver = {
> +       .probe = bt_ti_probe,
> +       .remove = bt_ti_remove,
> +       .driver = {
> +               .name = "btwilink",
> +               .owner = THIS_MODULE,
> +       },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init btwilink_init(void)
> +{
> +       BT_INFO("Bluetooth Driver for TI WiLink - Version %s", VERSION);
> +
> +       return platform_driver_register(&btwilink_driver);
> +}
> +
> +static void __exit btwilink_exit(void)
> +{
> +       platform_driver_unregister(&btwilink_driver);
> +}
> +
> +module_init(btwilink_init);
> +module_exit(btwilink_exit);
> +
> +/* ------ Module Info ------ */
> +
> +MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
> +MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
> --
> 1.5.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
@ 2010-11-30  7:29   ` Pavan Savoy
  0 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-11-30  7:29 UTC (permalink / raw)
  To: padovan, marcel; +Cc: linux-bluetooth, linux-kernel, Pavan Savoy

On Fri, Nov 26, 2010 at 2:50 PM,  <pavan_savoy@ti.com> wrote:
> From: Pavan Savoy <pavan_savoy@ti.com>
>
Marcel, Gustavo,

Please find some time to comment and also answer some of the
concerns I have below,

Thanks & Regards,
Pavan Savoy.

> comments attended to from v5 and v6,
>
> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
> Now I handle for EINPROGRESS - which is not really an error and
> return during all other error cases.
>
> 2. _write is still a function pointer and not an exported function, I
> need to change the underlying driver's code for this.
> However, previous lkml comments on the underlying driver's code
> suggested it to be kept as a function pointer and not EXPORT.
> Gustavo, Marcel - Please comment on this.
> Is this absolutely required? If so why?
>
> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
> ti_st_open, and did not see issues during firmware download.
> However ideally I would still like to set HCI_RUNNING once the firmware
> download is done, because I don't want to allow a _send_frame during
> firmware download - Marcel, Gustavo - Please comment.
>
> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
>
> 5. EAGAIN on failure of st_write is to suggest to try and write again.
> I have never this happen - However only if UART goes bad this case may
> occur.
>
> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
> fact the code is pretty much borrowed from there.
> Marcel, Gustavo - Please suggest where should it be done? If not here.
>
> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.
>
> 8. platform_driver registration inside module_init now is similar to
> other drivers.
>
> 9. Dan Carpenter's comments on leaking hst memory on failed
> hci_register_dev and empty space after quotes in debug statements
> fixed.
>
> Thanks for the comments...
> Sorry, for previously not being very clear on which comments were
> handled and which were not.
>
> -- patch description --
>
> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> Texas Instrument's WiLink chipsets combine wireless technologies
> like BT, FM, GPS and WLAN onto a single chip.
>
> This Bluetooth driver works on top of the TI_ST shared transport
> line discipline driver which also allows other drivers like
> FM V4L2 and GPS character driver to make use of the same UART interface.
>
> Kconfig and Makefile modifications to enable the Bluetooth
> driver for Texas Instrument's WiLink 7 chipset.
>
> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> ---
> =C2=A0drivers/bluetooth/Kconfig =C2=A0 =C2=A0| =C2=A0 10 ++
> =C2=A0drivers/bluetooth/Makefile =C2=A0 | =C2=A0 =C2=A01 +
> =C2=A0drivers/bluetooth/btwilink.c | =C2=A0363 ++++++++++++++++++++++++++=
++++++++++++++++
> =C2=A03 files changed, 374 insertions(+), 0 deletions(-)
> =C2=A0create mode 100644 drivers/bluetooth/btwilink.c
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..8e0de9a 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Say Y here to compile support for "Athe=
ros firmware download driver"
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0into the kernel or say M to compile it =
as module (ath3k).
>
> +config BT_WILINK
> + =C2=A0 =C2=A0 =C2=A0 tristate "Texas Instruments WiLink7 driver"
> + =C2=A0 =C2=A0 =C2=A0 depends on TI_ST
> + =C2=A0 =C2=A0 =C2=A0 help
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 This enables the Bluetooth driver for Texas=
 Instrument's BT/FM/GPS
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 combo devices. This makes use of shared tra=
nsport line discipline
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 core driver to communicate with the BT core=
 of the combo chip.
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 Say Y here to compile support for Texas Ins=
trument's WiLink7 driver
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 into the kernel or say M to compile it as m=
odule.
> =C2=A0endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 71bdf13..f4460f4 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_BT_HCIBTSDIO) =C2=A0 =C2=A0+=3D btsdio.o
> =C2=A0obj-$(CONFIG_BT_ATH3K) =C2=A0 =C2=A0 =C2=A0 =C2=A0 +=3D ath3k.o
> =C2=A0obj-$(CONFIG_BT_MRVL) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0+=3D btmrvl=
.o
> =C2=A0obj-$(CONFIG_BT_MRVL_SDIO) =C2=A0 =C2=A0 +=3D btmrvl_sdio.o
> +obj-$(CONFIG_BT_WILINK) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0+=3D btwilink.o
>
> =C2=A0btmrvl-y =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 :=3D btmrvl_main.o
> =C2=A0btmrvl-$(CONFIG_DEBUG_FS) =C2=A0 =C2=A0 =C2=A0+=3D btmrvl_debugfs.o
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..71e69f8
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> @@ -0,0 +1,363 @@
> +/*
> + * =C2=A0Texas Instrument's Bluetooth Driver For Shared Transport.
> + *
> + * =C2=A0Bluetooth Driver acts as interface between HCI core and
> + * =C2=A0TI Shared Transport Layer.
> + *
> + * =C2=A0Copyright (C) 2009-2010 Texas Instruments
> + * =C2=A0Author: Raja Mani <raja_mani@ti.com>
> + * =C2=A0 =C2=A0 Pavan Savoy <pavan_savoy@ti.com>
> + *
> + * =C2=A0This program is free software; you can redistribute it and/or m=
odify
> + * =C2=A0it under the terms of the GNU General Public License version 2 =
as
> + * =C2=A0published by the Free Software Foundation.
> + *
> + * =C2=A0This program is distributed in the hope that it will be useful,
> + * =C2=A0but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * =C2=A0MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =C2=A0See =
the
> + * =C2=A0GNU General Public License for more details.
> + *
> + * =C2=A0You should have received a copy of the GNU General Public Licen=
se
> + * =C2=A0along with this program; if not, write to the Free Software
> + * =C2=A0Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA =C2=A0=
02111-1307 =C2=A0USA
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/ti_wilink_st.h>
> +
> +/* Bluetooth Driver Version */
> +#define VERSION =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "1.0"
> +
> +/* Number of seconds to wait for registration completion
> + * when ST returns PENDING status.
> + */
> +#define BT_REGISTER_TIMEOUT =C2=A0 6000 =C2=A0 =C2=A0 /* 6 sec */
> +
> +/**
> + * struct ti_st - driver operation structure
> + * @hdev: hci device pointer which binds to bt driver
> + * @reg_status: ST registration callback status
> + * @st_write: write function provided by the ST driver
> + * =C2=A0 =C2=A0 to be used by the driver during send_frame.
> + * @wait_reg_completion - completion sync between ti_st_open
> + * =C2=A0 =C2=A0 and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> + =C2=A0 =C2=A0 =C2=A0 char reg_status;
> + =C2=A0 =C2=A0 =C2=A0 long (*st_write) (struct sk_buff *);
> + =C2=A0 =C2=A0 =C2=A0 struct completion wait_reg_completion;
> +};
> +
> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev =3D hst->hdev;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Update HCI stat counters */
> + =C2=A0 =C2=A0 =C2=A0 switch (pkt_type) {
> + =C2=A0 =C2=A0 =C2=A0 case HCI_COMMAND_PKT:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.cmd_tx++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +
> + =C2=A0 =C2=A0 =C2=A0 case HCI_ACLDATA_PKT:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.acl_tx++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +
> + =C2=A0 =C2=A0 =C2=A0 case HCI_SCODATA_PKT:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hdev->stat.sco_tx++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> + =C2=A0 =C2=A0 =C2=A0 }
> +}
> +
> +/* ------- Interfaces to Shared Transport ------ */
> +
> +/* Called by ST layer to indicate protocol registration completion
> + * status.ti_st_open() function will wait for signal from this
> + * API when st_register() function returns ST_PENDING.
> + */
> +static void st_registration_completion_cb(void *priv_data, char data)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *lhst =3D priv_data;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Save registration status for use in ti_st_open(=
) */
> + =C2=A0 =C2=A0 =C2=A0 lhst->reg_status =3D data;
> + =C2=A0 =C2=A0 =C2=A0 /* complete the wait in ti_st_open() */
> + =C2=A0 =C2=A0 =C2=A0 complete(&lhst->wait_reg_completion);
> +}
> +
> +/* Called by Shared Transport layer when receive data is
> + * available */
> +static long st_receive(void *priv_data, struct sk_buff *skb)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *lhst =3D priv_data;
> + =C2=A0 =C2=A0 =C2=A0 int err;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!skb)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!lhst) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 skb->dev =3D (void *) lhst->hdev;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Forward skb to HCI core layer */
> + =C2=A0 =C2=A0 =C2=A0 err =3D hci_recv_frame(skb);
> + =C2=A0 =C2=A0 =C2=A0 if (err < 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("Unable to push=
 skb to HCI core(%d)", err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 lhst->hdev->stat.byte_rx +=3D skb->len;
> +
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto =3D {
> + =C2=A0 =C2=A0 =C2=A0 .type =3D ST_BT,
> + =C2=A0 =C2=A0 =C2=A0 .recv =3D st_receive,
> + =C2=A0 =C2=A0 =C2=A0 .reg_complete_cb =3D st_registration_completion_cb=
,
> +};
> +
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 unsigned long timeleft;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst;
> + =C2=A0 =C2=A0 =C2=A0 int err;
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("%s %p", hdev->name, hdev);
> + =C2=A0 =C2=A0 =C2=A0 if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("btwilink alrea=
dy opened");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EBUSY;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 /* provide contexts for callbacks from ST */
> + =C2=A0 =C2=A0 =C2=A0 hst =3D hdev->driver_data;
> + =C2=A0 =C2=A0 =C2=A0 ti_st_proto.priv_data =3D hst;
> +
> + =C2=A0 =C2=A0 =C2=A0 err =3D st_register(&ti_st_proto);
> + =C2=A0 =C2=A0 =C2=A0 if (err =3D=3D -EINPROGRESS) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* ST is busy with eit=
her protocol registration or firmware
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* download.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Prepare wait-for-co=
mpletion handler data structures.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 init_completion(&hst->=
wait_reg_completion);
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Reset ST registrati=
on callback status flag , this value
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* will be update=
d in ti_st_registration_completion_cb()
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* function whene=
ver it called from ST driver.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->reg_status =3D -E=
INPROGRESS;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_DBG("waiting for re=
gistration completion signal from ST");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timeleft =3D wait_for_=
completion_timeout
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 (&hst->wait_reg_completion,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!timeleft) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 clear_bit(HCI_RUNNING, &hdev->flags);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 BT_ERR("Timeout(%d sec),didn't get reg "
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "completion =
signal from ST",
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_REGISTER_=
TIMEOUT / 1000);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 return -ETIMEDOUT;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Is ST registration =
callback called with ERROR status? */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (hst->reg_status !=
=3D 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 clear_bit(HCI_RUNNING, &hdev->flags);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 BT_ERR("ST registration completed with invalid "
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "status %d",=
 hst->reg_status);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 return -EAGAIN;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D 0;
> + =C2=A0 =C2=A0 =C2=A0 } else if (err !=3D 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(HCI_RUNNING,=
 &hdev->flags);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("st_register fa=
iled %d", err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 /* ti_st_proto.write is filled up by the underlyin=
g shared
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* transport driver upon registration
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D ti_st_proto.write;
> + =C2=A0 =C2=A0 =C2=A0 if (!hst->st_write) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("undefined ST w=
rite function");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(HCI_RUNNING,=
 &hdev->flags);
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Undo registration w=
ith ST */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(=
ST_BT);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (err)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 BT_ERR("st_unregister() failed with error %d", err);
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL=
;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 return err;
> +}
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 int err;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst =3D hdev->driver_data;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags)=
)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* continue to unregister from transport */
> + =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(ST_BT);
> + =C2=A0 =C2=A0 =C2=A0 if (err)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("st_unregister(=
) failed with error %d", err);
> +
> + =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL;
> +
> + =C2=A0 =C2=A0 =C2=A0 return err;
> +}
> +
> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst;
> + =C2=A0 =C2=A0 =C2=A0 long len;
> +
> + =C2=A0 =C2=A0 =C2=A0 hdev =3D (struct hci_dev *)skb->dev;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!test_bit(HCI_RUNNING, &hdev->flags))
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EBUSY;
> +
> + =C2=A0 =C2=A0 =C2=A0 hst =3D hdev->driver_data;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Prepend skb with frame type */
> + =C2=A0 =C2=A0 =C2=A0 memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1)=
;
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("%s: type %d len %d", hdev->name, bt_cb(skb=
)->pkt_type,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 skb->len);
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Insert skb to shared transport layer's transmit=
 queue.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* Freeing skb memory is taken care in shared=
 transport layer,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* so don't free skb memory here.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 len =3D hst->st_write(skb);
> + =C2=A0 =C2=A0 =C2=A0 if (len < 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("ST write faile=
d (%ld)", len);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Try Again, would on=
ly fail if UART has gone bad */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EAGAIN;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 /* ST accepted our skb. So, Go ahead and do rest *=
/
> + =C2=A0 =C2=A0 =C2=A0 hdev->stat.byte_tx +=3D len;
> + =C2=A0 =C2=A0 =C2=A0 ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("%s", hdev->name);
> + =C2=A0 =C2=A0 =C2=A0 kfree(hdev->driver_data);
> +}
> +
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 static struct ti_st *hst;
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> + =C2=A0 =C2=A0 =C2=A0 int err;
> +
> + =C2=A0 =C2=A0 =C2=A0 hst =3D kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + =C2=A0 =C2=A0 =C2=A0 if (!hst)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
> +
> + =C2=A0 =C2=A0 =C2=A0 /* Expose "hciX" device to user space */
> + =C2=A0 =C2=A0 =C2=A0 hdev =3D hci_alloc_dev();
> + =C2=A0 =C2=A0 =C2=A0 if (!hdev) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree(hst);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("hdev %p", hdev);
> +
> + =C2=A0 =C2=A0 =C2=A0 hst->hdev =3D hdev;
> + =C2=A0 =C2=A0 =C2=A0 hdev->bus =3D HCI_UART;
> + =C2=A0 =C2=A0 =C2=A0 hdev->driver_data =3D hst;
> + =C2=A0 =C2=A0 =C2=A0 hdev->open =3D ti_st_open;
> + =C2=A0 =C2=A0 =C2=A0 hdev->close =3D ti_st_close;
> + =C2=A0 =C2=A0 =C2=A0 hdev->flush =3D NULL;
> + =C2=A0 =C2=A0 =C2=A0 hdev->send =3D ti_st_send_frame;
> + =C2=A0 =C2=A0 =C2=A0 hdev->destruct =3D ti_st_destruct;
> + =C2=A0 =C2=A0 =C2=A0 hdev->owner =3D THIS_MODULE;
> +
> + =C2=A0 =C2=A0 =C2=A0 err =3D hci_register_dev(hdev);
> + =C2=A0 =C2=A0 =C2=A0 if (err < 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("Can't register=
 HCI device error %d", err);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree(hst);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hci_free_dev(hdev);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return err;
> + =C2=A0 =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0 =C2=A0 BT_DBG("HCI device registered (hdev %p)", hdev);
> +
> + =C2=A0 =C2=A0 =C2=A0 dev_set_drvdata(&pdev->dev, hst);
> + =C2=A0 =C2=A0 =C2=A0 return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + =C2=A0 =C2=A0 =C2=A0 struct hci_dev *hdev;
> + =C2=A0 =C2=A0 =C2=A0 struct ti_st *hst =3D dev_get_drvdata(&pdev->dev);
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!hst)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
> +
> + =C2=A0 =C2=A0 =C2=A0 hdev =3D hst->hdev;
> + =C2=A0 =C2=A0 =C2=A0 ti_st_close(hdev);
> + =C2=A0 =C2=A0 =C2=A0 hci_unregister_dev(hdev);
> +
> + =C2=A0 =C2=A0 =C2=A0 hci_free_dev(hdev);
> + =C2=A0 =C2=A0 =C2=A0 kfree(hst);
> +
> + =C2=A0 =C2=A0 =C2=A0 dev_set_drvdata(&pdev->dev, NULL);
> + =C2=A0 =C2=A0 =C2=A0 return 0;
> +}
> +
> +static struct platform_driver btwilink_driver =3D {
> + =C2=A0 =C2=A0 =C2=A0 .probe =3D bt_ti_probe,
> + =C2=A0 =C2=A0 =C2=A0 .remove =3D bt_ti_remove,
> + =C2=A0 =C2=A0 =C2=A0 .driver =3D {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .name =3D "btwilink",
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .owner =3D THIS_MODULE=
,
> + =C2=A0 =C2=A0 =C2=A0 },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init btwilink_init(void)
> +{
> + =C2=A0 =C2=A0 =C2=A0 BT_INFO("Bluetooth Driver for TI WiLink - Version =
%s", VERSION);
> +
> + =C2=A0 =C2=A0 =C2=A0 return platform_driver_register(&btwilink_driver);
> +}
> +
> +static void __exit btwilink_exit(void)
> +{
> + =C2=A0 =C2=A0 =C2=A0 platform_driver_unregister(&btwilink_driver);
> +}
> +
> +module_init(btwilink_init);
> +module_exit(btwilink_exit);
> +
> +/* ------ Module Info ------ */
> +
> +MODULE_AUTHOR("Raja Mani <raja_mani@ti.com>");
> +MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
> --
> 1.5.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-11-26  9:20 [PATCH v7] Bluetooth: btwilink driver pavan_savoy
  2010-11-30  7:29   ` Pavan Savoy
@ 2010-11-30 15:46 ` Gustavo F. Padovan
  2010-11-30 16:00     ` Pavan Savoy
  1 sibling, 1 reply; 15+ messages in thread
From: Gustavo F. Padovan @ 2010-11-30 15:46 UTC (permalink / raw)
  To: pavan_savoy; +Cc: marcel, linux-bluetooth, linux-kernel

Hi Pavan,

* pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-26 04:20:57 -0500]:

> From: Pavan Savoy <pavan_savoy@ti.com>
> 
> Marcel, Gustavo,
> 
> comments attended to from v5 and v6,
> 
> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
> Now I handle for EINPROGRESS - which is not really an error and
> return during all other error cases.
> 
> 2. _write is still a function pointer and not an exported function, I
> need to change the underlying driver's code for this.
> However, previous lkml comments on the underlying driver's code
> suggested it to be kept as a function pointer and not EXPORT.
> Gustavo, Marcel - Please comment on this.
> Is this absolutely required? If so why?
> 
> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
> ti_st_open, and did not see issues during firmware download.
> However ideally I would still like to set HCI_RUNNING once the firmware
> download is done, because I don't want to allow a _send_frame during
> firmware download - Marcel, Gustavo - Please comment.
> 
> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
> 
> 5. EAGAIN on failure of st_write is to suggest to try and write again.
> I have never this happen - However only if UART goes bad this case may
> occur.
> 
> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
> fact the code is pretty much borrowed from there.
> Marcel, Gustavo - Please suggest where should it be done? If not here.
> 
> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.
> 
> 8. platform_driver registration inside module_init now is similar to
> other drivers.
> 
> 9. Dan Carpenter's comments on leaking hst memory on failed
> hci_register_dev and empty space after quotes in debug statements
> fixed.
> 
> Thanks for the comments...
> Sorry, for previously not being very clear on which comments were
> handled and which were not.
> 
> -- patch description --
> 
> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> Texas Instrument's WiLink chipsets combine wireless technologies
> like BT, FM, GPS and WLAN onto a single chip.
> 
> This Bluetooth driver works on top of the TI_ST shared transport
> line discipline driver which also allows other drivers like
> FM V4L2 and GPS character driver to make use of the same UART interface.
> 
> Kconfig and Makefile modifications to enable the Bluetooth
> driver for Texas Instrument's WiLink 7 chipset.
> 
> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> ---
>  drivers/bluetooth/Kconfig    |   10 ++
>  drivers/bluetooth/Makefile   |    1 +
>  drivers/bluetooth/btwilink.c |  363 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 374 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/bluetooth/btwilink.c

So as part of reviewing this I took a look at your underlying driver and
I didn't like what I saw there, you are handling Bluetooth stuff inside
the core driver and that is just wrong. You have a Bluetooth driver here
then you have to leave the Bluetooth data handling to the Bluetooth
driver and do not do that in the core. 

So I'm going to clear NACK this. Your TI ST driver architecture is
a mess, Ideally you should not have any #include <net/bluetoooth/...>
there. Implement a core driver that only gets the Shared Transport
data and pass it to the right driver without look into the data and
process it. Process the data is not the role of the core driver.

Please fix this and come back when you have a proper solution for your
driver.

-- 
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-11-30 15:46 ` Gustavo F. Padovan
@ 2010-11-30 16:00     ` Pavan Savoy
  0 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-11-30 16:00 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: marcel, linux-bluetooth, linux-kernel

Gustavo,

On Tue, Nov 30, 2010 at 9:16 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Pavan,
>
> * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-26 04:20:57 -0500]:
>
>> From: Pavan Savoy <pavan_savoy@ti.com>
>>
>> Marcel, Gustavo,
>>
>> comments attended to from v5 and v6,
>>
>> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
>> Now I handle for EINPROGRESS - which is not really an error and
>> return during all other error cases.
>>
>> 2. _write is still a function pointer and not an exported function, I
>> need to change the underlying driver's code for this.
>> However, previous lkml comments on the underlying driver's code
>> suggested it to be kept as a function pointer and not EXPORT.
>> Gustavo, Marcel - Please comment on this.
>> Is this absolutely required? If so why?
>>
>> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
>> ti_st_open, and did not see issues during firmware download.
>> However ideally I would still like to set HCI_RUNNING once the firmware
>> download is done, because I don't want to allow a _send_frame during
>> firmware download - Marcel, Gustavo - Please comment.
>>
>> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
>>
>> 5. EAGAIN on failure of st_write is to suggest to try and write again.
>> I have never this happen - However only if UART goes bad this case may
>> occur.
>>
>> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
>> fact the code is pretty much borrowed from there.
>> Marcel, Gustavo - Please suggest where should it be done? If not here.
>>
>> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.
>>
>> 8. platform_driver registration inside module_init now is similar to
>> other drivers.
>>
>> 9. Dan Carpenter's comments on leaking hst memory on failed
>> hci_register_dev and empty space after quotes in debug statements
>> fixed.
>>
>> Thanks for the comments...
>> Sorry, for previously not being very clear on which comments were
>> handled and which were not.
>>
>> -- patch description --
>>
>> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
>> Texas Instrument's WiLink chipsets combine wireless technologies
>> like BT, FM, GPS and WLAN onto a single chip.
>>
>> This Bluetooth driver works on top of the TI_ST shared transport
>> line discipline driver which also allows other drivers like
>> FM V4L2 and GPS character driver to make use of the same UART interface.
>>
>> Kconfig and Makefile modifications to enable the Bluetooth
>> driver for Texas Instrument's WiLink 7 chipset.
>>
>> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
>> ---
>>  drivers/bluetooth/Kconfig    |   10 ++
>>  drivers/bluetooth/Makefile   |    1 +
>>  drivers/bluetooth/btwilink.c |  363 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 374 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/bluetooth/btwilink.c
>
> So as part of reviewing this I took a look at your underlying driver and
> I didn't like what I saw there, you are handling Bluetooth stuff inside
> the core driver and that is just wrong. You have a Bluetooth driver here
> then you have to leave the Bluetooth data handling to the Bluetooth
> driver and do not do that in the core.

Thanks for reviewing this and the underlying driver.
yes, we do have Bluetooth/FM/GPS handling inside the TI ST driver, on
addition of further technologies
we do plan to have them inside the ST driver too.

The understanding of BT or FM or GPS is required for the ST driver
because, the data coming from the chip
can either be of these technologies, further-more the data might not
come in a set.
As in, an a2dp/ftp ACL frame might come in 2 frames instead of 1, and
in other cases,
there might be a HCI-EVENT + FM CH8 data in a single frame received by the UART.

> So I'm going to clear NACK this. Your TI ST driver architecture is
> a mess, Ideally you should not have any #include <net/bluetoooth/...>
> there. Implement a core driver that only gets the Shared Transport
> data and pass it to the right driver without look into the data and
> process it. Process the data is not the role of the core driver.

So as I see it the tty_receive which translates to st_int_recv() in
TI-ST is the area of concern for
you ...
So any suggestions as to how we can go about just abstracting the BT,
FM and GPS data part to their individual drivers?

> Please fix this and come back when you have a proper solution for your
> driver.
>
> --
> Gustavo F. Padovan
> http://profusion.mobi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
@ 2010-11-30 16:00     ` Pavan Savoy
  0 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-11-30 16:00 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: marcel, linux-bluetooth, linux-kernel

Gustavo,

On Tue, Nov 30, 2010 at 9:16 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Pavan,
>
> * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-26 04:20:57 -0500]:
>
>> From: Pavan Savoy <pavan_savoy@ti.com>
>>
>> Marcel, Gustavo,
>>
>> comments attended to from v5 and v6,
>>
>> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
>> Now I handle for EINPROGRESS - which is not really an error and
>> return during all other error cases.
>>
>> 2. _write is still a function pointer and not an exported function, I
>> need to change the underlying driver's code for this.
>> However, previous lkml comments on the underlying driver's code
>> suggested it to be kept as a function pointer and not EXPORT.
>> Gustavo, Marcel - Please comment on this.
>> Is this absolutely required? If so why?
>>
>> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
>> ti_st_open, and did not see issues during firmware download.
>> However ideally I would still like to set HCI_RUNNING once the firmware
>> download is done, because I don't want to allow a _send_frame during
>> firmware download - Marcel, Gustavo - Please comment.
>>
>> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
>>
>> 5. EAGAIN on failure of st_write is to suggest to try and write again.
>> I have never this happen - However only if UART goes bad this case may
>> occur.
>>
>> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
>> fact the code is pretty much borrowed from there.
>> Marcel, Gustavo - Please suggest where should it be done? If not here.
>>
>> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.
>>
>> 8. platform_driver registration inside module_init now is similar to
>> other drivers.
>>
>> 9. Dan Carpenter's comments on leaking hst memory on failed
>> hci_register_dev and empty space after quotes in debug statements
>> fixed.
>>
>> Thanks for the comments...
>> Sorry, for previously not being very clear on which comments were
>> handled and which were not.
>>
>> -- patch description --
>>
>> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
>> Texas Instrument's WiLink chipsets combine wireless technologies
>> like BT, FM, GPS and WLAN onto a single chip.
>>
>> This Bluetooth driver works on top of the TI_ST shared transport
>> line discipline driver which also allows other drivers like
>> FM V4L2 and GPS character driver to make use of the same UART interface.
>>
>> Kconfig and Makefile modifications to enable the Bluetooth
>> driver for Texas Instrument's WiLink 7 chipset.
>>
>> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
>> ---
>> =C2=A0drivers/bluetooth/Kconfig =C2=A0 =C2=A0| =C2=A0 10 ++
>> =C2=A0drivers/bluetooth/Makefile =C2=A0 | =C2=A0 =C2=A01 +
>> =C2=A0drivers/bluetooth/btwilink.c | =C2=A0363 +++++++++++++++++++++++++=
+++++++++++++++++
>> =C2=A03 files changed, 374 insertions(+), 0 deletions(-)
>> =C2=A0create mode 100644 drivers/bluetooth/btwilink.c
>
> So as part of reviewing this I took a look at your underlying driver and
> I didn't like what I saw there, you are handling Bluetooth stuff inside
> the core driver and that is just wrong. You have a Bluetooth driver here
> then you have to leave the Bluetooth data handling to the Bluetooth
> driver and do not do that in the core.

Thanks for reviewing this and the underlying driver.
yes, we do have Bluetooth/FM/GPS handling inside the TI ST driver, on
addition of further technologies
we do plan to have them inside the ST driver too.

The understanding of BT or FM or GPS is required for the ST driver
because, the data coming from the chip
can either be of these technologies, further-more the data might not
come in a set.
As in, an a2dp/ftp ACL frame might come in 2 frames instead of 1, and
in other cases,
there might be a HCI-EVENT + FM CH8 data in a single frame received by the =
UART.

> So I'm going to clear NACK this. Your TI ST driver architecture is
> a mess, Ideally you should not have any #include <net/bluetoooth/...>
> there. Implement a core driver that only gets the Shared Transport
> data and pass it to the right driver without look into the data and
> process it. Process the data is not the role of the core driver.

So as I see it the tty_receive which translates to st_int_recv() in
TI-ST is the area of concern for
you ...
So any suggestions as to how we can go about just abstracting the BT,
FM and GPS data part to their individual drivers?

> Please fix this and come back when you have a proper solution for your
> driver.
>
> --
> Gustavo F. Padovan
> http://profusion.mobi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-11-30 16:00     ` Pavan Savoy
@ 2010-12-02  0:48       ` Pavan Savoy
  -1 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-12-02  0:48 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: marcel, linux-bluetooth, linux-kernel

Gustavo,

On Tue, Nov 30, 2010 at 9:30 PM, Pavan Savoy <pavan_savoy@ti.com> wrote:
>>> Marcel, Gustavo,
>>> comments attended to from v5 and v6,

There was 1 solution which was proposed for this,
See in tty_receive/st_int_recv() - we pretty much want only the
packet_id, header length and
the offset of the payload in the header structure.
this is to pack together the data coming in single frame or sets of
multiple frames...

so if bt driver can do something like,
st_register(acl_proto) -> where ACL_PKT=0x2, acl_hdr.len and
acl_hdr.payload_offset is sent to ST
st_register(sco_proto) -> where SCO_PKT=0x3, sco_hdr.len and
sco_hdr.payload_offset is sent to ST
st_register(evt_proto) -> where EVT_PKT=0x2, sco_hdr.len and
evt_hdr.payload_offset is sent to ST

and all 3 map to same st_recv() functions from ST, then
the ST driver as you suggested can be generic enough not to include
any net/bluetooth/ stuff,
and also can be ignorant about any bluetooth, fm or gps stuff in general...

So please suggest if this seems a valid solution, we had previously
declined it because it made bt_driver
sort of register to ST 3 times instead of 1 time like now....

Thanks,
Pavan

>>> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
>>> Now I handle for EINPROGRESS - which is not really an error and
>>> return during all other error cases.
>>>
>>> 2. _write is still a function pointer and not an exported function, I
>>> need to change the underlying driver's code for this.
>>> However, previous lkml comments on the underlying driver's code
>>> suggested it to be kept as a function pointer and not EXPORT.
>>> Gustavo, Marcel - Please comment on this.
>>> Is this absolutely required? If so why?
>>>
>>> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
>>> ti_st_open, and did not see issues during firmware download.
>>> However ideally I would still like to set HCI_RUNNING once the firmware
>>> download is done, because I don't want to allow a _send_frame during
>>> firmware download - Marcel, Gustavo - Please comment.
>>>
>>> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
>>>
>>> 5. EAGAIN on failure of st_write is to suggest to try and write again.
>>> I have never this happen - However only if UART goes bad this case may
>>> occur.
>>>
>>> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
>>> fact the code is pretty much borrowed from there.
>>> Marcel, Gustavo - Please suggest where should it be done? If not here.
>>>
>>> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.
>>>
>>> 8. platform_driver registration inside module_init now is similar to
>>> other drivers.
>>>
>>> 9. Dan Carpenter's comments on leaking hst memory on failed
>>> hci_register_dev and empty space after quotes in debug statements
>>> fixed.
>>>
>>> Thanks for the comments...
>>> Sorry, for previously not being very clear on which comments were
>>> handled and which were not.
>>>
>>> -- patch description --
>>>
>>> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
>>> Texas Instrument's WiLink chipsets combine wireless technologies
>>> like BT, FM, GPS and WLAN onto a single chip.
>>>
>>> This Bluetooth driver works on top of the TI_ST shared transport
>>> line discipline driver which also allows other drivers like
>>> FM V4L2 and GPS character driver to make use of the same UART interface.
>>>
>>> Kconfig and Makefile modifications to enable the Bluetooth
>>> driver for Texas Instrument's WiLink 7 chipset.
>>>
>>> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
>>> ---
>>>  drivers/bluetooth/Kconfig    |   10 ++
>>>  drivers/bluetooth/Makefile   |    1 +
>>>  drivers/bluetooth/btwilink.c |  363 ++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 374 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/bluetooth/btwilink.c
>>
>> So as part of reviewing this I took a look at your underlying driver and
>> I didn't like what I saw there, you are handling Bluetooth stuff inside
>> the core driver and that is just wrong. You have a Bluetooth driver here
>> then you have to leave the Bluetooth data handling to the Bluetooth
>> driver and do not do that in the core.
>
> Thanks for reviewing this and the underlying driver.
> yes, we do have Bluetooth/FM/GPS handling inside the TI ST driver, on
> addition of further technologies
> we do plan to have them inside the ST driver too.
>
> The understanding of BT or FM or GPS is required for the ST driver
> because, the data coming from the chip
> can either be of these technologies, further-more the data might not
> come in a set.
> As in, an a2dp/ftp ACL frame might come in 2 frames instead of 1, and
> in other cases,
> there might be a HCI-EVENT + FM CH8 data in a single frame received by the UART.
>
>> So I'm going to clear NACK this. Your TI ST driver architecture is
>> a mess, Ideally you should not have any #include <net/bluetoooth/...>
>> there. Implement a core driver that only gets the Shared Transport
>> data and pass it to the right driver without look into the data and
>> process it. Process the data is not the role of the core driver.
>
> So as I see it the tty_receive which translates to st_int_recv() in
> TI-ST is the area of concern for
> you ...
> So any suggestions as to how we can go about just abstracting the BT,
> FM and GPS data part to their individual drivers?
>
>> Please fix this and come back when you have a proper solution for your
>> driver.
>>
>> --
>> Gustavo F. Padovan
>> http://profusion.mobi
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
@ 2010-12-02  0:48       ` Pavan Savoy
  0 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-12-02  0:48 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: marcel, linux-bluetooth, linux-kernel

Gustavo,

On Tue, Nov 30, 2010 at 9:30 PM, Pavan Savoy <pavan_savoy@ti.com> wrote:
>>> Marcel, Gustavo,
>>> comments attended to from v5 and v6,

There was 1 solution which was proposed for this,
See in tty_receive/st_int_recv() - we pretty much want only the
packet_id, header length and
the offset of the payload in the header structure.
this is to pack together the data coming in single frame or sets of
multiple frames...

so if bt driver can do something like,
st_register(acl_proto) -> where ACL_PKT=3D0x2, acl_hdr.len and
acl_hdr.payload_offset is sent to ST
st_register(sco_proto) -> where SCO_PKT=3D0x3, sco_hdr.len and
sco_hdr.payload_offset is sent to ST
st_register(evt_proto) -> where EVT_PKT=3D0x2, sco_hdr.len and
evt_hdr.payload_offset is sent to ST

and all 3 map to same st_recv() functions from ST, then
the ST driver as you suggested can be generic enough not to include
any net/bluetooth/ stuff,
and also can be ignorant about any bluetooth, fm or gps stuff in general...

So please suggest if this seems a valid solution, we had previously
declined it because it made bt_driver
sort of register to ST 3 times instead of 1 time like now....

Thanks,
Pavan

>>> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM=
,
>>> Now I handle for EINPROGRESS - which is not really an error and
>>> return during all other error cases.
>>>
>>> 2. _write is still a function pointer and not an exported function, I
>>> need to change the underlying driver's code for this.
>>> However, previous lkml comments on the underlying driver's code
>>> suggested it to be kept as a function pointer and not EXPORT.
>>> Gustavo, Marcel - Please comment on this.
>>> Is this absolutely required? If so why?
>>>
>>> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
>>> ti_st_open, and did not see issues during firmware download.
>>> However ideally I would still like to set HCI_RUNNING once the firmware
>>> download is done, because I don't want to allow a _send_frame during
>>> firmware download - Marcel, Gustavo - Please comment.
>>>
>>> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
>>>
>>> 5. EAGAIN on failure of st_write is to suggest to try and write again.
>>> I have never this happen - However only if UART goes bad this case may
>>> occur.
>>>
>>> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
>>> fact the code is pretty much borrowed from there.
>>> Marcel, Gustavo - Please suggest where should it be done? If not here.
>>>
>>> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails=
.
>>>
>>> 8. platform_driver registration inside module_init now is similar to
>>> other drivers.
>>>
>>> 9. Dan Carpenter's comments on leaking hst memory on failed
>>> hci_register_dev and empty space after quotes in debug statements
>>> fixed.
>>>
>>> Thanks for the comments...
>>> Sorry, for previously not being very clear on which comments were
>>> handled and which were not.
>>>
>>> -- patch description --
>>>
>>> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
>>> Texas Instrument's WiLink chipsets combine wireless technologies
>>> like BT, FM, GPS and WLAN onto a single chip.
>>>
>>> This Bluetooth driver works on top of the TI_ST shared transport
>>> line discipline driver which also allows other drivers like
>>> FM V4L2 and GPS character driver to make use of the same UART interface=
.
>>>
>>> Kconfig and Makefile modifications to enable the Bluetooth
>>> driver for Texas Instrument's WiLink 7 chipset.
>>>
>>> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
>>> ---
>>> =C2=A0drivers/bluetooth/Kconfig =C2=A0 =C2=A0| =C2=A0 10 ++
>>> =C2=A0drivers/bluetooth/Makefile =C2=A0 | =C2=A0 =C2=A01 +
>>> =C2=A0drivers/bluetooth/btwilink.c | =C2=A0363 ++++++++++++++++++++++++=
++++++++++++++++++
>>> =C2=A03 files changed, 374 insertions(+), 0 deletions(-)
>>> =C2=A0create mode 100644 drivers/bluetooth/btwilink.c
>>
>> So as part of reviewing this I took a look at your underlying driver and
>> I didn't like what I saw there, you are handling Bluetooth stuff inside
>> the core driver and that is just wrong. You have a Bluetooth driver here
>> then you have to leave the Bluetooth data handling to the Bluetooth
>> driver and do not do that in the core.
>
> Thanks for reviewing this and the underlying driver.
> yes, we do have Bluetooth/FM/GPS handling inside the TI ST driver, on
> addition of further technologies
> we do plan to have them inside the ST driver too.
>
> The understanding of BT or FM or GPS is required for the ST driver
> because, the data coming from the chip
> can either be of these technologies, further-more the data might not
> come in a set.
> As in, an a2dp/ftp ACL frame might come in 2 frames instead of 1, and
> in other cases,
> there might be a HCI-EVENT + FM CH8 data in a single frame received by th=
e UART.
>
>> So I'm going to clear NACK this. Your TI ST driver architecture is
>> a mess, Ideally you should not have any #include <net/bluetoooth/...>
>> there. Implement a core driver that only gets the Shared Transport
>> data and pass it to the right driver without look into the data and
>> process it. Process the data is not the role of the core driver.
>
> So as I see it the tty_receive which translates to st_int_recv() in
> TI-ST is the area of concern for
> you ...
> So any suggestions as to how we can go about just abstracting the BT,
> FM and GPS data part to their individual drivers?
>
>> Please fix this and come back when you have a proper solution for your
>> driver.
>>
>> --
>> Gustavo F. Padovan
>> http://profusion.mobi
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoot=
h" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-11-30 16:00     ` Pavan Savoy
@ 2010-12-06 21:23       ` Gustavo F. Padovan
  -1 siblings, 0 replies; 15+ messages in thread
From: Gustavo F. Padovan @ 2010-12-06 21:23 UTC (permalink / raw)
  To: Pavan Savoy; +Cc: marcel, linux-bluetooth, linux-kernel

Hi Pavan,

* Pavan Savoy <pavan_savoy@ti.com> [2010-11-30 21:30:47 +0530]:

> Gustavo,
> 
> On Tue, Nov 30, 2010 at 9:16 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > Hi Pavan,
> >
> > * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-26 04:20:57 -0500]:
> >
> >> From: Pavan Savoy <pavan_savoy@ti.com>
> >>
> >> Marcel, Gustavo,
> >>
> >> comments attended to from v5 and v6,
> >>
> >> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPERM,
> >> Now I handle for EINPROGRESS - which is not really an error and
> >> return during all other error cases.
> >>
> >> 2. _write is still a function pointer and not an exported function, I
> >> need to change the underlying driver's code for this.
> >> However, previous lkml comments on the underlying driver's code
> >> suggested it to be kept as a function pointer and not EXPORT.
> >> Gustavo, Marcel - Please comment on this.
> >> Is this absolutely required? If so why?
> >>
> >> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
> >> ti_st_open, and did not see issues during firmware download.
> >> However ideally I would still like to set HCI_RUNNING once the firmware
> >> download is done, because I don't want to allow a _send_frame during
> >> firmware download - Marcel, Gustavo - Please comment.
> >>
> >> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
> >>
> >> 5. EAGAIN on failure of st_write is to suggest to try and write again.
> >> I have never this happen - However only if UART goes bad this case may
> >> occur.
> >>
> >> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
> >> fact the code is pretty much borrowed from there.
> >> Marcel, Gustavo - Please suggest where should it be done? If not here.
> >>
> >> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fails.
> >>
> >> 8. platform_driver registration inside module_init now is similar to
> >> other drivers.
> >>
> >> 9. Dan Carpenter's comments on leaking hst memory on failed
> >> hci_register_dev and empty space after quotes in debug statements
> >> fixed.
> >>
> >> Thanks for the comments...
> >> Sorry, for previously not being very clear on which comments were
> >> handled and which were not.
> >>
> >> -- patch description --
> >>
> >> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> >> Texas Instrument's WiLink chipsets combine wireless technologies
> >> like BT, FM, GPS and WLAN onto a single chip.
> >>
> >> This Bluetooth driver works on top of the TI_ST shared transport
> >> line discipline driver which also allows other drivers like
> >> FM V4L2 and GPS character driver to make use of the same UART interface.
> >>
> >> Kconfig and Makefile modifications to enable the Bluetooth
> >> driver for Texas Instrument's WiLink 7 chipset.
> >>
> >> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> >> ---
> >>  drivers/bluetooth/Kconfig    |   10 ++
> >>  drivers/bluetooth/Makefile   |    1 +
> >>  drivers/bluetooth/btwilink.c |  363 ++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 374 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/bluetooth/btwilink.c
> >
> > So as part of reviewing this I took a look at your underlying driver and
> > I didn't like what I saw there, you are handling Bluetooth stuff inside
> > the core driver and that is just wrong. You have a Bluetooth driver here
> > then you have to leave the Bluetooth data handling to the Bluetooth
> > driver and do not do that in the core.
> 
> Thanks for reviewing this and the underlying driver.
> yes, we do have Bluetooth/FM/GPS handling inside the TI ST driver, on
> addition of further technologies
> we do plan to have them inside the ST driver too.
> 
> The understanding of BT or FM or GPS is required for the ST driver
> because, the data coming from the chip
> can either be of these technologies, further-more the data might not
> come in a set.
> As in, an a2dp/ftp ACL frame might come in 2 frames instead of 1, and
> in other cases,
> there might be a HCI-EVENT + FM CH8 data in a single frame received by the UART.

Can't you differentiate Bluetooth data in a generic way, withou looking if it
is ACL, SCO or HCI EVENT? That done, you can just accumulate in a buffer all
the Bluetooth data you received in that stream then send it to Bluetooth
driver after finish that stream processing.

-- 
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
@ 2010-12-06 21:23       ` Gustavo F. Padovan
  0 siblings, 0 replies; 15+ messages in thread
From: Gustavo F. Padovan @ 2010-12-06 21:23 UTC (permalink / raw)
  To: Pavan Savoy; +Cc: marcel, linux-bluetooth, linux-kernel

Hi Pavan,

* Pavan Savoy <pavan_savoy@ti.com> [2010-11-30 21:30:47 +0530]:

> Gustavo,
>=20
> On Tue, Nov 30, 2010 at 9:16 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > Hi Pavan,
> >
> > * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-11-26 04:20:57 -0500]:
> >
> >> From: Pavan Savoy <pavan_savoy@ti.com>
> >>
> >> Marcel, Gustavo,
> >>
> >> comments attended to from v5 and v6,
> >>
> >> 1. Inside ti_st_open, I previously only checked for EINPROGRESS & EPER=
M,
> >> Now I handle for EINPROGRESS - which is not really an error and
> >> return during all other error cases.
> >>
> >> 2. _write is still a function pointer and not an exported function, I
> >> need to change the underlying driver's code for this.
> >> However, previous lkml comments on the underlying driver's code
> >> suggested it to be kept as a function pointer and not EXPORT.
> >> Gustavo, Marcel - Please comment on this.
> >> Is this absolutely required? If so why?
> >>
> >> 3. test_and_set_bit of HCI_RUNNING is done at beginning of
> >> ti_st_open, and did not see issues during firmware download.
> >> However ideally I would still like to set HCI_RUNNING once the firmware
> >> download is done, because I don't want to allow a _send_frame during
> >> firmware download - Marcel, Gustavo - Please comment.
> >>
> >> 4. test_and_clear of HCI_RUNNING now done @ beginning of close.
> >>
> >> 5. EAGAIN on failure of st_write is to suggest to try and write again.
> >> I have never this happen - However only if UART goes bad this case may
> >> occur.
> >>
> >> 6. ti_st_tx_complete is very similar to hci_ldisc's tx_complete - in
> >> fact the code is pretty much borrowed from there.
> >> Marcel, Gustavo - Please suggest where should it be done? If not here.
> >>
> >> 7. comments cleaned-up + hst memory leak fixed when hci_alloc_dev fail=
s.
> >>
> >> 8. platform_driver registration inside module_init now is similar to
> >> other drivers.
> >>
> >> 9. Dan Carpenter's comments on leaking hst memory on failed
> >> hci_register_dev and empty space after quotes in debug statements
> >> fixed.
> >>
> >> Thanks for the comments...
> >> Sorry, for previously not being very clear on which comments were
> >> handled and which were not.
> >>
> >> -- patch description --
> >>
> >> This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> >> Texas Instrument's WiLink chipsets combine wireless technologies
> >> like BT, FM, GPS and WLAN onto a single chip.
> >>
> >> This Bluetooth driver works on top of the TI_ST shared transport
> >> line discipline driver which also allows other drivers like
> >> FM V4L2 and GPS character driver to make use of the same UART interfac=
e.
> >>
> >> Kconfig and Makefile modifications to enable the Bluetooth
> >> driver for Texas Instrument's WiLink 7 chipset.
> >>
> >> Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> >> ---
> >> =A0drivers/bluetooth/Kconfig =A0 =A0| =A0 10 ++
> >> =A0drivers/bluetooth/Makefile =A0 | =A0 =A01 +
> >> =A0drivers/bluetooth/btwilink.c | =A0363 +++++++++++++++++++++++++++++=
+++++++++++++
> >> =A03 files changed, 374 insertions(+), 0 deletions(-)
> >> =A0create mode 100644 drivers/bluetooth/btwilink.c
> >
> > So as part of reviewing this I took a look at your underlying driver and
> > I didn't like what I saw there, you are handling Bluetooth stuff inside
> > the core driver and that is just wrong. You have a Bluetooth driver here
> > then you have to leave the Bluetooth data handling to the Bluetooth
> > driver and do not do that in the core.
>=20
> Thanks for reviewing this and the underlying driver.
> yes, we do have Bluetooth/FM/GPS handling inside the TI ST driver, on
> addition of further technologies
> we do plan to have them inside the ST driver too.
>=20
> The understanding of BT or FM or GPS is required for the ST driver
> because, the data coming from the chip
> can either be of these technologies, further-more the data might not
> come in a set.
> As in, an a2dp/ftp ACL frame might come in 2 frames instead of 1, and
> in other cases,
> there might be a HCI-EVENT + FM CH8 data in a single frame received by th=
e UART.

Can't you differentiate Bluetooth data in a generic way, withou looking if =
it
is ACL, SCO or HCI EVENT? That done, you can just accumulate in a buffer all
the Bluetooth data you received in that stream then send it to Bluetooth
driver after finish that stream processing.

--=20
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-12-06 21:23       ` Gustavo F. Padovan
  (?)
@ 2010-12-06 21:35       ` Vitaly Wool
  2010-12-09  7:47           ` Pavan Savoy
  -1 siblings, 1 reply; 15+ messages in thread
From: Vitaly Wool @ 2010-12-06 21:35 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: Pavan Savoy, marcel, linux-bluetooth, linux-kernel

Hi Gustavo,

On Mon, Dec 6, 2010 at 10:23 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:

> Can't you differentiate Bluetooth data in a generic way, withou looking if it
> is ACL, SCO or HCI EVENT? That done, you can just accumulate in a buffer all
> the Bluetooth data you received in that stream then send it to Bluetooth
> driver after finish that stream processing.

I'm afraid he can't do this because he needs to route events to the
appropriate entity (BT/FM/GPS). I'm not sure how it can be done
without analyzing the incoming packet.

Thanks,
   Vitaly

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-12-06 21:35       ` Vitaly Wool
@ 2010-12-09  7:47           ` Pavan Savoy
  0 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-12-09  7:47 UTC (permalink / raw)
  To: Vitaly Wool, Gustavo F. Padovan; +Cc: marcel, linux-bluetooth, linux-kernel

Gustavo,

On Tue, Dec 7, 2010 at 3:05 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> Hi Gustavo,
>
> On Mon, Dec 6, 2010 at 10:23 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
>
>> Can't you differentiate Bluetooth data in a generic way, withou looking if it
>> is ACL, SCO or HCI EVENT? That done, you can just accumulate in a buffer all
>> the Bluetooth data you received in that stream then send it to Bluetooth
>> driver after finish that stream processing.
>
> I'm afraid he can't do this because he needs to route events to the
> appropriate entity (BT/FM/GPS). I'm not sure how it can be done
> without analyzing the incoming packet.

Think of TI-ST driver as a extension to the HCI-H4 driver or HCI-LL
with FM and GPS being the additional protocols, and more protocols
coming in future...
So some driver has to have a knowledge of the protocols which are on chip.
the basic arch can be found @ http://omappedia.org/wiki/Wilink_ST

As Vitaly rightly pointed out, the TI-ST driver needs to peek into all
the protocol's data be it BT, FM or GPS to assemble fragmented data
(say ACL data coming out of TTY in 2 fragments) or fragment multiple
protocol data (say HCI-Event + FM Channel 8 event data)....
Note: we include even the FM and GPS headers to understand protocol
frames, but they lack a standard unlike Bluetooth...

Since there lacks a generic way to differentiate BT, FM or GPS data at
TI-ST driver layer,  please suggest what can be done ...


> Thanks,
>   Vitaly
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
@ 2010-12-09  7:47           ` Pavan Savoy
  0 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-12-09  7:47 UTC (permalink / raw)
  To: Vitaly Wool, Gustavo F. Padovan; +Cc: marcel, linux-bluetooth, linux-kernel

Gustavo,

On Tue, Dec 7, 2010 at 3:05 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> Hi Gustavo,
>
> On Mon, Dec 6, 2010 at 10:23 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
>
>> Can't you differentiate Bluetooth data in a generic way, withou looking =
if it
>> is ACL, SCO or HCI EVENT? That done, you can just accumulate in a buffer=
 all
>> the Bluetooth data you received in that stream then send it to Bluetooth
>> driver after finish that stream processing.
>
> I'm afraid he can't do this because he needs to route events to the
> appropriate entity (BT/FM/GPS). I'm not sure how it can be done
> without analyzing the incoming packet.

Think of TI-ST driver as a extension to the HCI-H4 driver or HCI-LL
with FM and GPS being the additional protocols, and more protocols
coming in future...
So some driver has to have a knowledge of the protocols which are on chip.
the basic arch can be found @ http://omappedia.org/wiki/Wilink_ST

As Vitaly rightly pointed out, the TI-ST driver needs to peek into all
the protocol's data be it BT, FM or GPS to assemble fragmented data
(say ACL data coming out of TTY in 2 fragments) or fragment multiple
protocol data (say HCI-Event + FM Channel 8 event data)....
Note: we include even the FM and GPS headers to understand protocol
frames, but they lack a standard unlike Bluetooth...

Since there lacks a generic way to differentiate BT, FM or GPS data at
TI-ST driver layer,  please suggest what can be done ...


> Thanks,
> =C2=A0 Vitaly
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
  2010-12-09  7:47           ` Pavan Savoy
@ 2010-12-16  6:09             ` Pavan Savoy
  -1 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-12-16  6:09 UTC (permalink / raw)
  To: Gustavo F. Padovan, marcel; +Cc: linux-bluetooth, linux-kernel

On Thu, Dec 9, 2010 at 1:17 PM, Pavan Savoy <pavan_savoy@sify.com> wrote:
> Gustavo,
>
> On Tue, Dec 7, 2010 at 3:05 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
>> Hi Gustavo,
>>
>> On Mon, Dec 6, 2010 at 10:23 PM, Gustavo F. Padovan
>> <padovan@profusion.mobi> wrote:
>>
>>> Can't you differentiate Bluetooth data in a generic way, withou looking if it
>>> is ACL, SCO or HCI EVENT? That done, you can just accumulate in a buffer all
>>> the Bluetooth data you received in that stream then send it to Bluetooth
>>> driver after finish that stream processing.
>>
>> I'm afraid he can't do this because he needs to route events to the
>> appropriate entity (BT/FM/GPS). I'm not sure how it can be done
>> without analyzing the incoming packet.
>
> Think of TI-ST driver as a extension to the HCI-H4 driver or HCI-LL
> with FM and GPS being the additional protocols, and more protocols
> coming in future...
> So some driver has to have a knowledge of the protocols which are on chip.
> the basic arch can be found @ http://omappedia.org/wiki/Wilink_ST

Gustavo, Marcel,

Any suggestion to this?
Is including net/bluetooth headers a problem?
and is this a big blocking factor to try and push it to mainline?

I know it would be a dumb question, but do you want me to redeclare
the ACL, SCO and Event headers structures?
All I need is the off-set of the structure where the payload length resides....

Please suggest ...

regards,
Pavan

> As Vitaly rightly pointed out, the TI-ST driver needs to peek into all
> the protocol's data be it BT, FM or GPS to assemble fragmented data
> (say ACL data coming out of TTY in 2 fragments) or fragment multiple
> protocol data (say HCI-Event + FM Channel 8 event data)....
> Note: we include even the FM and GPS headers to understand protocol
> frames, but they lack a standard unlike Bluetooth...
>
> Since there lacks a generic way to differentiate BT, FM or GPS data at
> TI-ST driver layer,  please suggest what can be done ...
>
>
>> Thanks,
>>   Vitaly
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v7] Bluetooth: btwilink driver
@ 2010-12-16  6:09             ` Pavan Savoy
  0 siblings, 0 replies; 15+ messages in thread
From: Pavan Savoy @ 2010-12-16  6:09 UTC (permalink / raw)
  To: Gustavo F. Padovan, marcel; +Cc: linux-bluetooth, linux-kernel

On Thu, Dec 9, 2010 at 1:17 PM, Pavan Savoy <pavan_savoy@sify.com> wrote:
> Gustavo,
>
> On Tue, Dec 7, 2010 at 3:05 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
>> Hi Gustavo,
>>
>> On Mon, Dec 6, 2010 at 10:23 PM, Gustavo F. Padovan
>> <padovan@profusion.mobi> wrote:
>>
>>> Can't you differentiate Bluetooth data in a generic way, withou looking=
 if it
>>> is ACL, SCO or HCI EVENT? That done, you can just accumulate in a buffe=
r all
>>> the Bluetooth data you received in that stream then send it to Bluetoot=
h
>>> driver after finish that stream processing.
>>
>> I'm afraid he can't do this because he needs to route events to the
>> appropriate entity (BT/FM/GPS). I'm not sure how it can be done
>> without analyzing the incoming packet.
>
> Think of TI-ST driver as a extension to the HCI-H4 driver or HCI-LL
> with FM and GPS being the additional protocols, and more protocols
> coming in future...
> So some driver has to have a knowledge of the protocols which are on chip=
.
> the basic arch can be found @ http://omappedia.org/wiki/Wilink_ST

Gustavo, Marcel,

Any suggestion to this?
Is including net/bluetooth headers a problem?
and is this a big blocking factor to try and push it to mainline?

I know it would be a dumb question, but do you want me to redeclare
the ACL, SCO and Event headers structures?
All I need is the off-set of the structure where the payload length resides=
....

Please suggest ...

regards,
Pavan

> As Vitaly rightly pointed out, the TI-ST driver needs to peek into all
> the protocol's data be it BT, FM or GPS to assemble fragmented data
> (say ACL data coming out of TTY in 2 fragments) or fragment multiple
> protocol data (say HCI-Event + FM Channel 8 event data)....
> Note: we include even the FM and GPS headers to understand protocol
> frames, but they lack a standard unlike Bluetooth...
>
> Since there lacks a generic way to differentiate BT, FM or GPS data at
> TI-ST driver layer, =C2=A0please suggest what can be done ...
>
>
>> Thanks,
>> =C2=A0 Vitaly
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoot=
h" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2010-12-16  6:09 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-26  9:20 [PATCH v7] Bluetooth: btwilink driver pavan_savoy
2010-11-30  7:29 ` Pavan Savoy
2010-11-30  7:29   ` Pavan Savoy
2010-11-30 15:46 ` Gustavo F. Padovan
2010-11-30 16:00   ` Pavan Savoy
2010-11-30 16:00     ` Pavan Savoy
2010-12-02  0:48     ` Pavan Savoy
2010-12-02  0:48       ` Pavan Savoy
2010-12-06 21:23     ` Gustavo F. Padovan
2010-12-06 21:23       ` Gustavo F. Padovan
2010-12-06 21:35       ` Vitaly Wool
2010-12-09  7:47         ` Pavan Savoy
2010-12-09  7:47           ` Pavan Savoy
2010-12-16  6:09           ` Pavan Savoy
2010-12-16  6:09             ` Pavan Savoy

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.