Return-Path: Subject: Re: [PATCH] Added support for Atheros AR300x UART Bluetooth Chip From: Marcel Holtmann To: Vikram Kandukuri Cc: linux-bluetooth@vger.kernel.org, suraj@atheros.com, lrodriguez@atheros.com In-Reply-To: <20100209114231.GA6587@ATH-LT-538> References: <20100209114231.GA6587@ATH-LT-538> Content-Type: text/plain; charset="UTF-8" Date: Mon, 22 Feb 2010 03:29:59 +0100 Message-ID: <1266805799.18491.33.camel@violet> Mime-Version: 1.0 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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