2010-10-18 22:34:38

by Pavan Savoy

[permalink] [raw]
Subject: [PATCH v2] Bluetooth: btwilink driver

From: Pavan Savoy <[email protected]>

v2 comments

Review comments about blank lines, error message, CORE->core,
drv->driver, returning err, not initializing err have been taken care.

a. The HCI not running error during ti_st_close is seen
when I try and rmmod when hci0 is up, and hence thought of
keeping it in prevous version - But removed now.

b. The error codes returned in the send_frame don't seem to matter
since they aren't considered in hci_core.
So returning err as we receive it from TI-ST driver now.

c. Even if we return different error code in hci's open device, EIO
is returned from the hci_core, we have different error codes
in TI-ST driver because we want to,
1. prevent same protocol from registering twice - EAGAIN
2. it is not really an error if we return EINPROGRESS since it means
firmware download is in progress because other protocol requested it
and the transport will soon be available.

Please review & Thanks,
Pavan

-- 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 <[email protected]>
---
drivers/bluetooth/Kconfig | 10 +
drivers/bluetooth/Makefile | 1 +
drivers/bluetooth/btwilink.c | 417 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 428 insertions(+), 0 deletions(-)
create mode 100644 drivers/bluetooth/btwilink.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 02deef4..e0d67dd 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 "BlueZ bluetooth driver for TI ST"
+ 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..e6e2e64
--- /dev/null
+++ b/drivers/bluetooth/btwilink.c
@@ -0,0 +1,417 @@
+/*
+ * 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 <[email protected]>
+ * Pavan Savoy <[email protected]>
+ *
+ * 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"
+
+/* Defines number of seconds to wait for reg completion
+ * callback getting called from ST (in case,registration
+ * with ST returns PENDING status)
+ */
+#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
+
+/**
+ * struct ti_st - BT driver operation structure
+ * @hdev: hci device pointer which binds to bt driver
+ * @flags: used locally,to maintain various BT driver status
+ * @streg_cbdata: to hold ST registration callback status
+ * @st_write: write function pointer of ST driver
+ * @wait_reg_completion - completion sync between ti_st_open
+ * and ti_st_registration_completion_cb.
+ */
+struct ti_st {
+ struct hci_dev *hdev;
+ char streg_cbdata;
+ long (*st_write) (struct sk_buff *);
+ struct completion wait_reg_completion;
+};
+
+static int reset;
+
+/* 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;
+ 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 = (struct ti_st *)priv_data;
+
+ /* ti_st_open() function needs value of 'data' to know
+ * the registration status(success/fail),So have a back
+ * up of it.
+ */
+ lhst->streg_cbdata = data;
+
+ /* Got a feedback from ST for BT driver registration
+ * request.Wackup ti_st_open() function to continue
+ * it's open operation.
+ */
+ 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)
+{
+ int err;
+ struct ti_st *lhst = (struct ti_st *)priv_data;
+
+ if (!skb)
+ return -EFAULT;
+
+ if (!lhst) {
+ kfree_skb(skb);
+ return -EFAULT;
+ }
+
+ skb->dev = (struct net_device *)lhst->hdev;
+
+ /* Forward skb to HCI core layer */
+ err = hci_recv_frame(skb);
+ if (err) {
+ kfree_skb(skb);
+ 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,
+ .priv_data = NULL,
+};
+
+/* 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);
+
+ /* 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) {
+ /* Prepare wait-for-completion handler data structures.
+ * Needed to syncronize this and st_registration_completion_cb()
+ * functions.
+ */
+ 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->streg_cbdata = -EINPROGRESS;
+
+ /* ST is busy with other protocol registration(may be busy with
+ * firmware download).So,Wait till the registration callback
+ * (passed as a argument to st_register() function) getting
+ * called from ST.
+ */
+ BT_DBG(" waiting for reg completion signal from ST");
+
+ timeleft = wait_for_completion_timeout
+ (&hst->wait_reg_completion,
+ msecs_to_jiffies(BT_REGISTER_TIMEOUT));
+ if (!timeleft) {
+ 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 value? */
+ if (hst->streg_cbdata != 0) {
+ BT_ERR("ST reg completion CB called with invalid"
+ "status %d", hst->streg_cbdata);
+ return -EAGAIN;
+ }
+ err = 0;
+ } else if (err == -EPERM) {
+ BT_ERR("st_register failed %d", err);
+ return err;
+ }
+
+ /* Do we have proper ST write function? */
+ if (ti_st_proto.write != NULL) {
+ /* We need this pointer for sending any Bluetooth pkts */
+ hst->st_write = ti_st_proto.write;
+ } else {
+ BT_ERR("failed to get ST write func pointer");
+
+ /* Undo registration with ST */
+ err = st_unregister(ST_BT);
+ if (err)
+ BT_ERR("st_unregister failed %d", err);
+
+ hst->st_write = NULL;
+ return err;
+ }
+
+ /* Registration with ST layer is completed successfully,
+ * now chip is ready to accept commands from HCI core.
+ * Mark HCI Device flag as RUNNING
+ */
+ set_bit(HCI_RUNNING, &hdev->flags);
+ return err;
+}
+
+/* Close device */
+static int ti_st_close(struct hci_dev *hdev)
+{
+ int err;
+ struct ti_st *hst = hdev->driver_data;
+
+ /* continue to unregister from transport */
+ err = st_unregister(ST_BT);
+ if (err)
+ BT_ERR("st_unregister failed %d", err);
+
+ hst->st_write = NULL;
+ return err;
+}
+
+/* Called from HCI core, Sends frames to Shared Transport */
+static int ti_st_send_frame(struct sk_buff *skb)
+{
+ struct hci_dev *hdev;
+ struct ti_st *hst;
+ long len;
+
+ if (!skb)
+ return -ENOMEM;
+
+ hdev = (struct hci_dev *)skb->dev;
+ if (!hdev)
+ return -ENODEV;
+
+ if (!test_bit(HCI_RUNNING, &hdev->flags))
+ return -EBUSY;
+
+ hst = (struct ti_st *)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.
+ */
+ if (!hst->st_write) {
+ kfree_skb(skb);
+ BT_ERR(" Can't write to ST, st_write null?");
+ return -EAGAIN;
+ }
+
+ len = hst->st_write(skb);
+ if (len < 0) {
+ kfree_skb(skb);
+ BT_ERR(" ST write failed (%ld)", len);
+ 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)
+{
+ if (!hdev)
+ BT_ERR("Destruct called with invalid HCI Device"
+ "(hdev=NULL)");
+
+ BT_DBG("%s", hdev->name);
+
+ /* free ti_st memory */
+ kfree(hdev->driver_data);
+ return;
+}
+
+/* Creates new HCI device */
+static int ti_st_register_dev(struct ti_st *hst)
+{
+ int err;
+ struct hci_dev *hdev;
+
+ /* Initialize and register HCI device */
+ hdev = hci_alloc_dev();
+ if (!hdev)
+ 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;
+
+ if (reset)
+ set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
+
+ err = hci_register_dev(hdev);
+ if (err < 0) {
+ BT_ERR("Can't register HCI device");
+ hci_free_dev(hdev);
+ return err;
+ }
+
+ BT_DBG(" HCI device registered. hdev= %p", hdev);
+ return 0;
+}
+
+
+static int bt_ti_probe(struct platform_device *pdev)
+{
+ int err;
+ static struct ti_st *hst;
+
+ BT_DBG(" Bluetooth Driver Version %s", VERSION);
+
+ hst = kzalloc(sizeof(struct ti_st), GFP_KERNEL);
+ if (!hst)
+ return -ENOMEM;
+
+ /* Expose "hciX" device to user space */
+ err = ti_st_register_dev(hst);
+ if (err) {
+ kfree(hst);
+ return err;
+ }
+
+ dev_set_drvdata(&pdev->dev, hst);
+ return err;
+}
+
+static int bt_ti_remove(struct platform_device *pdev)
+{
+ struct ti_st *hst;
+
+ hst = dev_get_drvdata(&pdev->dev);
+ /* Deallocate local resource's memory */
+ if (hst) {
+ struct hci_dev *hdev = hst->hdev;
+ if (!hdev) {
+ BT_ERR("Invalid hdev memory");
+ kfree(hst);
+ } else {
+ ti_st_close(hdev);
+ hci_unregister_dev(hdev);
+ /* Free HCI device memory */
+ hci_free_dev(hdev);
+ }
+ }
+ 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 bt_drv_init(void)
+{
+ long ret;
+
+ ret = platform_driver_register(&btwilink_driver);
+ if (ret != 0) {
+ BT_ERR("btwilink platform driver registration failed");
+ return -EPERM;
+ }
+ return 0;
+}
+
+static void __exit bt_drv_exit(void)
+{
+ platform_driver_unregister(&btwilink_driver);
+}
+
+module_init(bt_drv_init);
+module_exit(bt_drv_exit);
+
+/* ------ Module Info ------ */
+
+module_param(reset, bool, 0644);
+MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
+MODULE_AUTHOR("Raja Mani <[email protected]>");
+MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
--
1.6.5


2010-10-19 14:56:00

by Anderson Lizardo

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: btwilink driver

On Mon, Oct 18, 2010 at 6:34 PM, <[email protected]> wrote:
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 02deef4..e0d67dd 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -219,4 +219,14 @@ config BT_ATH3K
> =A0 =A0 =A0 =A0 =A0Say Y here to compile support for "Atheros firmware do=
wnload driver"
> =A0 =A0 =A0 =A0 =A0into the kernel or say M to compile it as module (ath3=
k).
>
> +config BT_WILINK
> + =A0 =A0 =A0 tristate "BlueZ bluetooth driver for TI ST"

I think this has been mentioned before: "BlueZ" is not used in this
context on the kernel. It is also not consistent with the config name
"BT_WILINK". You can follow the pattern from the other entries and
use:

tristate "Texas Instruments WinLink7 driver"

> + =A0 =A0 =A0 depends on TI_ST
> + =A0 =A0 =A0 help
> + =A0 =A0 =A0 =A0 This enables the Bluetooth driver for Texas Instrument'=
s BT/FM/GPS
> + =A0 =A0 =A0 =A0 combo devices. This makes use of shared transport line =
discipline
> + =A0 =A0 =A0 =A0 core driver to communicate with the BT core of the comb=
o chip.
> +
> + =A0 =A0 =A0 =A0 Say Y here to compile support for Texas Instrument's Wi=
Link7 driver
> + =A0 =A0 =A0 =A0 into the kernel or say M to compile it as module.
> =A0endmenu
> [...]
> diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
> new file mode 100644
> index 0000000..e6e2e64
> --- /dev/null
> +++ b/drivers/bluetooth/btwilink.c
> [...]
> +/* Defines number of seconds to wait for reg completion
> + * callback getting called from ST (in case,registration
> + * with ST returns PENDING status)
> + */

I suggest rewriting this comment into:

"Number of seconds to wait for registration completion when ST returns
PENDING status"

> +#define BT_REGISTER_TIMEOUT =A0 6000 =A0 =A0 /* 6 sec */
> +
> +/**
> + * struct ti_st - BT driver operation structure
> + * @hdev: hci device pointer which binds to bt driver

Just drop "BT" and "bt" here. I think it is clear you are referring to
the Bluetooth driver.

> + * @flags: used locally,to maintain various BT driver status

Suggestion: @flags: driver status flags

(if you can be more specific to which kind of status, it would be even bett=
er)

> + * @streg_cbdata: to hold ST registration callback status

You can drop "to hold" here.

> + * @st_write: write function pointer of ST driver

IMHO this description does not add anything to what st_write is for.

> + * @wait_reg_completion - completion sync between ti_st_open
> + * =A0 =A0 and ti_st_registration_completion_cb.
> + */
> +struct ti_st {
> + =A0 =A0 =A0 struct hci_dev *hdev;
> + =A0 =A0 =A0 char streg_cbdata;
> + =A0 =A0 =A0 long (*st_write) (struct sk_buff *);
> + =A0 =A0 =A0 struct completion wait_reg_completion;
> +};
> +
> +static int reset;
> +
> +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> +static inline void ti_st_tx_complete(struct ti_st *hst, int pkt_type)
> +{
> + =A0 =A0 =A0 struct hci_dev *hdev;
> + =A0 =A0 =A0 hdev =3D hst->hdev;
> +
> + =A0 =A0 =A0 /* Update HCI stat counters */
> + =A0 =A0 =A0 switch (pkt_type) {
> + =A0 =A0 =A0 case HCI_COMMAND_PKT:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.cmd_tx++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> +
> + =A0 =A0 =A0 case HCI_ACLDATA_PKT:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.acl_tx++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> +
> + =A0 =A0 =A0 case HCI_SCODATA_PKT:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.sco_tx++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =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)
> +{
> + =A0 =A0 =A0 struct ti_st *lhst =3D (struct ti_st *)priv_data;

Is this explicit cast necessary?

> +
> + =A0 =A0 =A0 /* ti_st_open() function needs value of 'data' to know
> + =A0 =A0 =A0 =A0* the registration status(success/fail),So have a back
> + =A0 =A0 =A0 =A0* up of it.
> + =A0 =A0 =A0 =A0*/

Suggestion: /* Save registration status for use in ti_st_open() */

> + =A0 =A0 =A0 lhst->streg_cbdata =3D data;
> +
> + =A0 =A0 =A0 /* Got a feedback from ST for BT driver registration
> + =A0 =A0 =A0 =A0* request.Wackup ti_st_open() function to continue
> + =A0 =A0 =A0 =A0* it's open operation.
> + =A0 =A0 =A0 =A0*/

Too much BT here. If it means "Bluetooth", you don't need to use it
every time. Additionally, I don't get what the above comment means.

> + =A0 =A0 =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)

Can this function return "int" instead? It is more common for
functions which return just a error value.

> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 struct ti_st *lhst =3D (struct ti_st *)priv_data;

Again, is this cast necessary?

> +
> + =A0 =A0 =A0 if (!skb)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EFAULT;
> +
> + =A0 =A0 =A0 if (!lhst) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EFAULT;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 skb->dev =3D (struct net_device *)lhst->hdev;
> +
> + =A0 =A0 =A0 /* Forward skb to HCI core layer */
> + =A0 =A0 =A0 err =3D hci_recv_frame(skb);
> + =A0 =A0 =A0 if (err) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Unable to push skb to HCI core(%d)"=
, err);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 lhst->hdev->stat.byte_rx +=3D skb->len;

Add a blank like here.

> + =A0 =A0 =A0 return 0;
> +}
> +
> +/* ------- Interfaces to HCI layer ------ */
> +/* protocol structure registered with shared transport */
> +static struct st_proto_s ti_st_proto =3D {
> + =A0 =A0 =A0 .type =3D ST_BT,
> + =A0 =A0 =A0 .recv =3D st_receive,
> + =A0 =A0 =A0 .reg_complete_cb =3D st_registration_completion_cb,
> + =A0 =A0 =A0 .priv_data =3D NULL,
> +};
> +
> +/* Called from HCI core to initialize the device */
> +static int ti_st_open(struct hci_dev *hdev)
> +{
> + =A0 =A0 =A0 unsigned long timeleft;
> + =A0 =A0 =A0 struct ti_st *hst;
> + =A0 =A0 =A0 int err;
> +
> + =A0 =A0 =A0 BT_DBG("%s %p", hdev->name, hdev);
> +
> + =A0 =A0 =A0 /* provide contexts for callbacks from ST */
> + =A0 =A0 =A0 hst =3D hdev->driver_data;
> + =A0 =A0 =A0 ti_st_proto.priv_data =3D hst;
> +
> + =A0 =A0 =A0 err =3D st_register(&ti_st_proto);
> + =A0 =A0 =A0 if (err =3D=3D -EINPROGRESS) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Prepare wait-for-completion handler data=
structures.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* Needed to syncronize this and st_regis=
tration_completion_cb()
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* functions.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/

syncronize -> synchronize

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 init_completion(&hst->wait_reg_completion);
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Reset ST registration callback status fl=
ag , this value
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* will be updated in ti_st_registration_=
completion_cb()
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* function whenever it called from ST dr=
iver.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hst->streg_cbdata =3D -EINPROGRESS;

If this field is used solely for holding status values, why not call
it "reg_status" or something like that? "cbdata" is more for opaque
parameters IMHO.

> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* ST is busy with other protocol registrat=
ion(may be busy with
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* firmware download).So,Wait till the re=
gistration callback
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* (passed as a argument to st_register()=
function) getting
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* called from ST.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/

"ST is busy with either protocol registration or firmware download.
Wait until the registration callback is called."

Is it clearer?

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_DBG(" waiting for reg completion signal =
from ST");

BT_DBG("Waiting for registration completion signal from ST");

> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 timeleft =3D wait_for_completion_timeout
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (&hst->wait_reg_completion,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msecs_to_jiffies(BT_REGI=
STER_TIMEOUT));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!timeleft) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Timeout(%d sec),did=
n't get reg "
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 "completion signal from ST",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 BT_REGISTER_TIMEOUT / 1000);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ETIMEDOUT;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Is ST registration callback called with =
ERROR value? */

ERROR value -> "error status"

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (hst->streg_cbdata !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("ST reg completion C=
B called with invalid"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 "status %d", hst->streg_cbdata);

Too much abbreviations here:

reg -> registration
CB -> callback

Also you are truncating the C string wrong here (and maybe in other
places). It will be print as "invalidstatus". Use either
"...invalid<space>" (1st line) or "<space>status..." (2nd line).

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EAGAIN;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D 0;
> + =A0 =A0 =A0 } else if (err =3D=3D -EPERM) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_register failed %d", err);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +

You could assign hst->st_write right here:

hst->st_write =3D ti_st_proto.write;

And simplify the if/else below to:

if (!hst->st_write) {
BT_ERR(....);
...
}

(i.e. invert the check logic and drop the "!=3D NULL" case)

> + =A0 =A0 =A0 /* Do we have proper ST write function? */
> + =A0 =A0 =A0 if (ti_st_proto.write !=3D NULL) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* We need this pointer for sending any Blu=
etooth pkts */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hst->st_write =3D ti_st_proto.write;
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("failed to get ST write func pointer=
");

This error message could be:

BT_ERR("undefined ST write function");

*Although* I think the whole check is in the wrong place. I think this
check should be inside the function which sets ti_st_proto.write.

> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Undo registration with ST */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D st_unregister(ST_BT);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (err)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_unregister faile=
d %d", err);

Suggestion:

BT_ERR("st_unregister() failed with error %d", err);

> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hst->st_write =3D NULL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 /* Registration with ST layer is completed successfully,
> + =A0 =A0 =A0 =A0* now chip is ready to accept commands from HCI core.
> + =A0 =A0 =A0 =A0* Mark HCI Device flag as RUNNING
> + =A0 =A0 =A0 =A0*/

The comment above could be summed as:

/* Registration with ST layer was successful and hardware is ready to
accept commands from HCI core. */

> + =A0 =A0 =A0 set_bit(HCI_RUNNING, &hdev->flags);

Add a blank line here.

> + =A0 =A0 =A0 return err;
> +}
> +
> +/* Close device */
> +static int ti_st_close(struct hci_dev *hdev)
> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 struct ti_st *hst =3D hdev->driver_data;
> +
> + =A0 =A0 =A0 /* continue to unregister from transport */
> + =A0 =A0 =A0 err =3D st_unregister(ST_BT);
> + =A0 =A0 =A0 if (err)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("st_unregister failed %d", err);

BT_ERR("st_unregister() failed with error %d", err);

> +
> + =A0 =A0 =A0 hst->st_write =3D NULL;

Add a blank line here.

> + =A0 =A0 =A0 return err;
> +}
> +
> +/* Called from HCI core, Sends frames to Shared Transport */

IMHO the comment above can be dropped (it is too obvious).

> +static int ti_st_send_frame(struct sk_buff *skb)
> +{
> + =A0 =A0 =A0 struct hci_dev *hdev;
> + =A0 =A0 =A0 struct ti_st *hst;
> + =A0 =A0 =A0 long len;
> +
> + =A0 =A0 =A0 if (!skb)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 hdev =3D (struct hci_dev *)skb->dev;
> + =A0 =A0 =A0 if (!hdev)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;
> +
> + =A0 =A0 =A0 if (!test_bit(HCI_RUNNING, &hdev->flags))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
> +
> + =A0 =A0 =A0 hst =3D (struct ti_st *)hdev->driver_data;
> +
> + =A0 =A0 =A0 /* Prepend skb with frame type */
> + =A0 =A0 =A0 memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> + =A0 =A0 =A0 BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_t=
ype,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb->len);
> +
> + =A0 =A0 =A0 /* Insert skb to shared transport layer's transmit queue.
> + =A0 =A0 =A0 =A0* Freeing skb memory is taken care in shared transport l=
ayer,
> + =A0 =A0 =A0 =A0* so don't free skb memory here.
> + =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 if (!hst->st_write) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR(" Can't write to ST, st_write null?"=
);

Suggestion: BT_ERR("Could not write to ST (st_write is NULL)");

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EAGAIN;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 len =3D hst->st_write(skb);
> + =A0 =A0 =A0 if (len < 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree_skb(skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR(" ST write failed (%ld)", len);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EAGAIN;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 /* ST accepted our skb. So, Go ahead and do rest */
> + =A0 =A0 =A0 hdev->stat.byte_tx +=3D len;
> + =A0 =A0 =A0 ti_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> +
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static void ti_st_destruct(struct hci_dev *hdev)
> +{
> + =A0 =A0 =A0 if (!hdev)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Destruct called with invalid HCI De=
vice"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "(hdev=3DNU=
LL)");

There is a "return" missing here. Also I don't think this error
message is necessary at all. You could have just:

if (!hdev)
return;

> +
> + =A0 =A0 =A0 BT_DBG("%s", hdev->name);
> +
> + =A0 =A0 =A0 /* free ti_st memory */
> + =A0 =A0 =A0 kfree(hdev->driver_data);

add a blank line here.

> + =A0 =A0 =A0 return;
> +}
> +
> +/* Creates new HCI device */
> +static int ti_st_register_dev(struct ti_st *hst)
> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 struct hci_dev *hdev;
> +
> + =A0 =A0 =A0 /* Initialize and register HCI device */
> + =A0 =A0 =A0 hdev =3D hci_alloc_dev();
> + =A0 =A0 =A0 if (!hdev)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 BT_DBG("hdev=3D %p", hdev);

BT_DBG("hdev %p", hdev);

> +
> + =A0 =A0 =A0 hst->hdev =3D hdev;
> + =A0 =A0 =A0 hdev->bus =3D HCI_UART;
> + =A0 =A0 =A0 hdev->driver_data =3D hst;
> + =A0 =A0 =A0 hdev->open =3D ti_st_open;
> + =A0 =A0 =A0 hdev->close =3D ti_st_close;
> + =A0 =A0 =A0 hdev->flush =3D NULL;
> + =A0 =A0 =A0 hdev->send =3D ti_st_send_frame;
> + =A0 =A0 =A0 hdev->destruct =3D ti_st_destruct;
> + =A0 =A0 =A0 hdev->owner =3D THIS_MODULE;
> +
> + =A0 =A0 =A0 if (reset)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
> +
> + =A0 =A0 =A0 err =3D hci_register_dev(hdev);
> + =A0 =A0 =A0 if (err < 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Can't register HCI device");

Print the err value on the message above.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_free_dev(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 BT_DBG(" HCI device registered. hdev=3D %p", hdev);

Suggestion: BT_DBG("HCI device registered (hdev %p)", hdev);

> + =A0 =A0 =A0 return 0;
> +}
> +
> +
> +static int bt_ti_probe(struct platform_device *pdev)
> +{
> + =A0 =A0 =A0 int err;
> + =A0 =A0 =A0 static struct ti_st *hst;
> +
> + =A0 =A0 =A0 BT_DBG(" Bluetooth Driver Version %s", VERSION);
> +
> + =A0 =A0 =A0 hst =3D kzalloc(sizeof(struct ti_st), GFP_KERNEL);
> + =A0 =A0 =A0 if (!hst)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 /* Expose "hciX" device to user space */
> + =A0 =A0 =A0 err =3D ti_st_register_dev(hst);
> + =A0 =A0 =A0 if (err) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(hst);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 dev_set_drvdata(&pdev->dev, hst);
> + =A0 =A0 =A0 return err;
> +}
> +
> +static int bt_ti_remove(struct platform_device *pdev)
> +{
> + =A0 =A0 =A0 struct ti_st *hst;
> +
> + =A0 =A0 =A0 hst =3D dev_get_drvdata(&pdev->dev);
> + =A0 =A0 =A0 /* Deallocate local resource's memory =A0*/

You can invert the hst check:

if (!hst)
return <some_error_code>;

And reduce put the code below outside the if(). Also note that the
current code always returns 0. It should return error for hst =3D=3D NULL
and hdev =3D=3D NULL.

> + =A0 =A0 =A0 if (hst) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct hci_dev *hdev =3D hst->hdev;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!hdev) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Invalid hdev memory=
");

The error message above is not informative.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(hst);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ti_st_close(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_unregister_dev(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Free HCI device memory *=
/
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_free_dev(hdev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static struct platform_driver btwilink_driver =3D {
> + =A0 =A0 =A0 .probe =3D bt_ti_probe,
> + =A0 =A0 =A0 .remove =3D bt_ti_remove,
> + =A0 =A0 =A0 .driver =3D {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .name =3D "btwilink",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .owner =3D THIS_MODULE,
> + =A0 =A0 =A0 },
> +};
> +
> +/* ------- Module Init/Exit interfaces ------ */
> +static int __init bt_drv_init(void)
> +{
> + =A0 =A0 =A0 long ret;
> +
> + =A0 =A0 =A0 ret =3D platform_driver_register(&btwilink_driver);
> + =A0 =A0 =A0 if (ret !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("btwilink platform driver registrati=
on failed");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EPERM;

-EPERM for a registration failure? Looks strange to me.

> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static void __exit bt_drv_exit(void)
> +{
> + =A0 =A0 =A0 platform_driver_unregister(&btwilink_driver);
> +}
> +
> +module_init(bt_drv_init);
> +module_exit(bt_drv_exit);
> +
> +/* ------ Module Info ------ */
> +
> +module_param(reset, bool, 0644);
> +MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> +MODULE_AUTHOR("Raja Mani <[email protected]>");
> +MODULE_DESCRIPTION("Bluetooth Driver for TI Shared Transport" VERSION);
> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL");
> --
> 1.6.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>


Regards,
--=20
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil