Hi Vikram,
> drivers/bluetooth/Kconfig | 10 +
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/hci_ath.c | 545 +++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_ath.h | 72 ++++++
> drivers/bluetooth/hci_ldisc.c | 6 +
> drivers/bluetooth/hci_uart.h | 8 +-
> 6 files changed, 641 insertions(+), 1 deletions(-)
> create mode 100755 drivers/bluetooth/hci_ath.c
> create mode 100755 drivers/bluetooth/hci_ath.h
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 058fbcc..32b98a4 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -58,6 +58,16 @@ config BT_HCIUART_BCSP
>
> Say Y here to compile support for HCI BCSP protocol.
>
> +config BT_HCIUART_ATH
> + bool "Atheros AR3001 Board support"
> + depends on BT_HCIUART
> + help
> + HCIATH (HCI Atheros) is a serial protocol for communication
> + between Bluetooth device and host. This protocol is required for
> + serial Bluetooth devices that are based on Atheros AR3001 chips.
> +
> + Say Y here to compile support for HCIATH protocol.
> +
it would be nice if you add a document explaining this special Atheros
protocol that is used.
> +static void ath_context_switch(struct work_struct *work)
> +{
> + int status;
> + struct ath_struct *ath;
> + struct hci_uart *hu;
> + struct tty_struct *tty;
> + struct sk_buff *skbuf;
> + ath = container_of(work, struct ath_struct, ctxtsw);
> + hu = ath->hu;
> + tty = hu->tty;
> + status = ath_wakeup_ar3001(tty);
> + if ((status & TIOCM_CTS)) {
> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
> + skb_queue_tail(&ath->txq, skbuf);
> +
> + /* Ready to send Data */
> + clear_bit(HCI_UART_SENDING, &hu->tx_state);
> + hci_uart_tx_wakeup(hu);
> + }
> +}
> +
> +/* Initialize protocol */
> +static int ath_open(struct hci_uart *hu)
> +{
> + struct ath_struct *ath;
> + BT_DBG("hu %p", hu);
> + ath = kzalloc(sizeof(*ath), GFP_ATOMIC);
> + if (!ath)
> + return -ENOMEM;
> + skb_queue_head_init(&ath->txq);
> + skb_queue_head_init(&ath->tx_wait_q);
> + skb_queue_head_init(&ath->tx_cmd_wait_q);
> + spin_lock_init(&ath->hciath_lock);
> + ath->cur_sleep = 0;
> + ath->num_cmds_complete = 1;
> + hu->priv = ath;
> + ath->hu = hu;
> + init_waitqueue_head(&ath->wqevt);
> + INIT_WORK(&ath->ctxtsw, ath_context_switch);
> + return 0;
> +}
In all the function, use at least some empty lines in between so that
somebody can try to read these function. Cramping everything in less
space doesn't make it simpler.
> +/* Enqueue frame for transmittion (padding, crc, etc) */
> +/* may be called from two simultaneous tasklets */
Why are you stealing the comments from hci_ll.c. Are you using actually
padding and CRC?
> +static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> +{
> + struct ath_struct *ath;
> + ath = hu->priv;
Direct assignment on declaration.
> + if (HCI_SCODATA_PKT == bt_cb(skb)->pkt_type) {
Always the other way around. Variable compares to constant. Not the
constant to variable.
> +static struct sk_buff *ath_dequeue(struct hci_uart *hu)
> +{
> + struct ath_struct *ath;
> + struct sk_buff *skbuff;
> + ath = hu->priv;
> + skbuff = skb_dequeue(&ath->txq);
> + if (NULL != skbuff) {
Never ever use the variable name skbuff and also if (skbuff) here.
However preferable this is if (!skb) return NULL;
> + ath_handle_host_data(ath, bt_cb(skbuff)->pkt_type,
> + &skbuff->data[1], skbuff->len - 1);
> + }
> + return skbuff;
> +}
> +
> +static inline int ath_check_data_len(struct ath_struct *ath, int len)
> +{
> + register int room = skb_tailroom(ath->rx_skb);
> + BT_DBG("len %d room %d", len, room);
> + if (!len) {
> + hci_recv_frame(ath->rx_skb);
> + } else if (len > room) {
> + BT_ERR("Data length is too large");
> + kfree_skb(ath->rx_skb);
> + } else {
> + ath->rx_state = HCIATH_W4_DATA;
> + ath->rx_count = len;
> + return len;
> + }
How is this if statement even suppose to work?
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_skb = NULL;
> + ath->rx_count = 0;
> + return 0;
> +}
> +
> +/* Recv data */
> +static int ath_recv(struct hci_uart *hu, void *data, int count)
> +{
> + struct ath_struct *ath = hu->priv;
> + register char *ptr;
> + struct hci_event_hdr *eh;
> + struct hci_acl_hdr *ah;
> + struct hci_sco_hdr *sh;
> + struct sk_buff *skbuff;
> + register int len, type, dlen;
> + skbuff = NULL;
> + BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count,
> + ath->rx_state, ath->rx_count);
> + ptr = data;
> + while (count) {
> + if (ath->rx_count) {
> + len = min_t(unsigned int, ath->rx_count, count);
> + memcpy(skb_put(ath->rx_skb, len), ptr, len);
> + ath->rx_count -= len;
> + count -= len;
> + ptr += len;
> + if (ath->rx_count)
> + continue;
> + switch (ath->rx_state) {
> + case HCIATH_W4_DATA:
> + ath_handle_data_from_controller(ath,
> + bt_cb
> + (ath->rx_skb)->
> + pkt_type,
> + ath->rx_skb->
> + data,
> + ath->rx_skb->
> + len);
> + if (HCI_EVENT_PKT ==
> + bt_cb(ath->rx_skb)->pkt_type
> + && ath->num_cmds_complete > 0) {
> +
> + skbuff =
> + skb_dequeue(&ath->tx_cmd_wait_q);
> + if (skbuff != NULL) {
> + skb_queue_tail(&ath->txq,
> + skbuff);
> + schedule_work(&ath->ctxtsw);
> + }
> + }
> + if (ath_verify_event_discardable
> + (hu, bt_cb(ath->rx_skb)->pkt_type,
> + ath->rx_skb->data)) {
> + kfree(ath->rx_skb);
> + ath->rx_skb = NULL;
> + } else {
> + hci_recv_frame(ath->rx_skb);
> + }
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_skb = NULL;
> + ath->rx_count = 0;
> + continue;
> + case HCIATH_W4_EVENT_HDR:
> + eh = (struct hci_event_hdr *)ath->rx_skb->data;
> + BT_DBG("Event header: evt 0x%2.2x plen %d",
> + eh->evt, eh->plen);
> + ath_check_data_len(ath, eh->plen);
> + continue;
> + case HCIATH_W4_ACL_HDR:
> + ah = (struct hci_acl_hdr *)ath->rx_skb->data;
> + dlen = __le16_to_cpu(ah->dlen);
> + BT_DBG("ACL header: dlen %d", dlen);
> + ath_check_data_len(ath, dlen);
> + continue;
> + case HCIATH_W4_SCO_HDR:
> + sh = (struct hci_sco_hdr *)ath->rx_skb->data;
> + BT_DBG("SCO header: dlen %d", sh->dlen);
> + ath_check_data_len(ath, sh->dlen);
> + continue;
> + }
> + }
> +
> + /* HCIATH_W4_PACKET_TYPE */
> + switch (*ptr) {
> + case HCI_EVENT_PKT:
> + BT_DBG("Event packet");
> + ath->rx_state = HCIATH_W4_EVENT_HDR;
> + ath->rx_count = HCI_EVENT_HDR_SIZE;
> + type = HCI_EVENT_PKT;
> + break;
> + case HCI_ACLDATA_PKT:
> + BT_DBG("ACL packet");
> + ath->rx_state = HCIATH_W4_ACL_HDR;
> + ath->rx_count = HCI_ACL_HDR_SIZE;
> + type = HCI_ACLDATA_PKT;
> + break;
> + case HCI_SCODATA_PKT:
> + BT_DBG("SCO packet");
> + ath->rx_state = HCIATH_W4_SCO_HDR;
> + ath->rx_count = HCI_SCO_HDR_SIZE;
> + type = HCI_SCODATA_PKT;
> + break;
> + default:
> + BT_ERR("Unknown HCI packet type %2.2x", (__u8) *ptr);
> + hu->hdev->stat.err_rx++;
> + ptr++;
> + count--;
> + continue;
> + };
> + ptr++;
> + count--;
> +
> + /* Allocate packet */
> + ath->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE, GFP_ATOMIC);
> + if (!ath->rx_skb) {
> + BT_ERR("Can't allocate mem for new packet");
> + ath->rx_state = HCIATH_W4_PACKET_TYPE;
> + ath->rx_count = 0;
> + return 0;
> + }
> + ath->rx_skb->dev = (void *)hu->hdev;
> + bt_cb(ath->rx_skb)->pkt_type = type;
> + } return count;
> +}
We have a common exported function called hci_recv_fragment that should
be doing exactly this.
> +static void ath_controller_sleep_mode(struct hci_uart *hu, bool enable)
> +{
> + unsigned char sleepcmd[] = { 0x01, 0x04, 0xFC, 0x01, 0x00 };
> + sleepcmd[4] = enable;
> + ath_write_data_to_cntrlr(hu->hdev, sleepcmd, sizeof(sleepcmd));
> +}
Special vendor commands need a comment. Explain the data structure of
them and what are the options.
> +int ath_wakeup_ar3001(struct tty_struct *tty)
> +{
> + struct termios settings;
> + int status = 0x00;
> + mm_segment_t oldfs;
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + if ((status & TIOCM_CTS))
> + return status;
> +
> + oldfs = get_fs();
> + set_fs(KERNEL_DS);
> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
> +
> + settings.c_cflag &= ~CRTSCTS;
> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
> + set_fs(oldfs);
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS);
> + mdelay(20);
> +
> + status = tty->driver->ops->tiocmget(tty, NULL);
> +
> + tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00);
> + mdelay(20);
> +
> + status = tty->driver->ops->tiocmget(tty, NULL);
> + oldfs = get_fs();
> + set_fs(KERNEL_DS);
> + n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
> +
> + settings.c_cflag |= CRTSCTS;
> + n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
> + set_fs(oldfs);
> + return status;
> +}
Why is this magic in kernel space and not inside hciattach?
> +int ath_fullboot_config(struct hci_uart *hu, int current_event)
> +{
> + struct sk_buff *skbuf;
> + struct ath_struct *ath = hu->priv;
> + static int autosleep;
> + unsigned char rstevt[] = { 0x1, 0x3, 0xc, 0x0 };
> + if (current_event == HCI_RESET) {
> +
> + if (ath->cur_sleep) {
> + autosleep = 1;
> + ath_controller_sleep_mode(hu, 1);
> + return 1;
> + } else {
> + return 0;
> + }
> + }
> +
> + if (current_event == HCI_SET_SLEEP_MODE) {
> +
> + if (autosleep == 0)
> + return 1;
> +
> + while ((skbuf = skb_dequeue(&ath->tx_wait_q)))
> + skb_queue_tail(&ath->txq, skbuf);
> +
> + ath_write_data_to_host(hu->hdev, rstevt, sizeof(rstevt));
> + autosleep = 0;
> + return 1;
> + }
> + return 0;
> +}
Why is this function not static? And all the others?
> +int ath_write_data_to_host(void *dev, unsigned char *data, u8 length)
> +{
> + struct sk_buff *skbuf;
> + struct hci_dev *hdev;
> + hdev = (struct hci_dev *)dev;
> + skbuf = bt_skb_alloc(length, GFP_ATOMIC);
> + if (!skbuf) {
> + BT_ERR("Memory allocation failed");
> + return -1;
> + }
> + skb_orphan(skbuf);
> +
> + /* First byte will be added as packet type */
> + bt_cb(skbuf)->pkt_type = data[0];
> + skbuf->dev = (void *)hdev;
> + memcpy(skb_put(skbuf, length - 1), &data[1], length - 1);
> + hci_recv_frame(skbuf);
> + return length;
> +}
What is this exactly for? Do you have a secondary receive path?
> +int ath_write_data_to_cntrlr(void *dev, unsigned char *Data, u8 len)
> +{
> + struct sk_buff *skbuff;
> + struct ath_struct *ath;
> + struct hci_uart *hu;
> + struct hci_dev *hdev;
> + if (NULL == dev) {
> + BT_DBG("NULL handle received %p \n", dev);
> + return 0;
> + }
> + hdev = (struct hci_dev *)dev;
> + hu = (struct hci_uart *)hdev->driver_data;
> + if (hu == NULL) {
> + BT_DBG("NULL handle received %p \n", hdev);
> + return 0;
> + }
> + ath = hu->priv;
> + if (ath == NULL) {
> + BT_DBG("NULL handle received \n");
> + return 0;
> + }
> + skbuff = bt_skb_alloc(len, GFP_ATOMIC);
> + if (skbuff == NULL) {
> + BT_DBG("Malloc Fail memory %p \n", skbuff);
> + return 0;
> + }
> + skb_orphan(skbuff);
> +
> + if (len != 0)
> + memcpy(skb_put(skbuff, len), Data, len);
> +
> + bt_cb(skbuff)->pkt_type = HCI_COMMAND_PKT;
> + skbuff->dev = dev;
> + if (ath->num_cmds_complete > 0) {
> + skb_queue_tail(&ath->txq, skbuff);
> + schedule_work(&ath->ctxtsw);
> + } else {
> + skb_queue_tail(&ath->tx_cmd_wait_q, skbuff);
> + }
> + return len;
> +}
Messing with num_cmds_complete inside the driver is wrong. What are you
doing here?
> +int ath_check_sleep_cmd(struct ath_struct *ath, unsigned char *packet)
> +{
> + if (packet[0] == HCI_EVENT_PKT && packet[1] == 0xFC)
> + ath->cur_sleep = packet[3];
How is this working. Every vendor event is sleep event?
> +int ath_verify_event_discardable(struct hci_uart *hu, unsigned char pkt_type,
> + unsigned char *packet)
> +{
> + if (pkt_type != HCI_EVENT_PKT)
> + return 0;
> +
> + switch (packet[0]) {
> + case 0x0E: /* Command Complete Event */
> + if (packet[3] == 0x03 && packet[4] == 0x0C) {
> +
> + /* Command complete for HCI Reset Received */
> + return ath_fullboot_config(hu, HCI_RESET);
> + } else if (packet[3] == 0x04 && packet[4] == 0xFC) {
> + return ath_fullboot_config(hu, HCI_SET_SLEEP_MODE);
> + }
> + break;
> + }
> + return 0;
> +}
What are you doing here?
> +void ath_handle_hci_event(struct ath_struct *ath, u8 * packet)
> +{
> + switch (packet[0]) {
> + case 0x05: /* ACL Disconnection Complete Event */
> + break;
> + case 0x03: /* ACL Connection Complete Event */
> + break;
> + case 0x0E: /* Command Complete Event */
> + spin_lock(&ath->hciath_lock);
> + ath->num_cmds_complete = packet[2];
> + spin_unlock(&ath->hciath_lock);
> + break;
> + case 0x0F: /* Command Status Event */
> + spin_lock(&ath->hciath_lock);
> + ath->num_cmds_complete = packet[3];
> + spin_unlock(&ath->hciath_lock);
> + break;
> + }
> +}
Something is really wrong in this driver? Are you really know how this
is suppose to work if you have to interfere this much?
> +void ath_handle_host_data(struct ath_struct *ath, u8 pktType, u8 * packet,
> + unsigned int len)
> +{
> + switch (pktType) {
> + case HCI_ACLDATA_PKT: /* ACL packets */
> + break;
> + case HCI_COMMAND_PKT: /* HCI Commands */
> + ath_handle_hci_cmd(ath, packet);
> + ath_check_sleep_cmd(ath, packet);
> + break;
> + }
> +}
> +
> +void ath_handle_data_from_controller(struct ath_struct *ath, u8 pktType,
> + u8 *packet, unsigned int len)
> +{
> + switch (pktType) {
> + case HCI_ACLDATA_PKT: /* ACL packets */
> + break;
> + case HCI_EVENT_PKT: /* HCI Events */
> + ath_handle_hci_event(ath, packet);
> + break;
> + }
> +}
Seriously? are you implemented your own Bluetooth stack? Please explain
what you really need. And then the host stack can provide it for you.
This is not acceptable.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
