Re: [PATCH v3] Added support for Atheros AR300x Bluetooth Chip

[Suraj Sumangala <suraj@atheros.com>]

Hi Luis,

Luis Rodriguez wrote:
> On Tue, Apr 20, 2010 at 03:20:32AM -0700, Suraj Sumangala wrote:
>> This protocol implements support for power management feature provided by AR300x chip.
>> This lets the controller chip go to sleep mode if there is no Bluetooth
>> activity for some time.
>> It then wakes up the chip in case of a Bluetooth activity.
> 
> The above commit log needs some more work. Try to keep the
> commit log entry subject to about 50 characters, the context should
> not pass around 75 characters. Your commit also indicates this is a
> "protocol" ? This is driver, and you do have some hacks for enhancing
> power saving, but that is not that relevant to the commit log.
> 
> How about:
> 
> ---
> Add support for the Atheros AR300x Bluetooth Chip
> 
> This adds support for the Atheros Bluetooth serial protocol to
> support the AR300x chipsets. The serial protocol implements
> enhanced power management features for the AR300x chipsets.
> 
> Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> Signed-off-by: Suraj <suraj@atheros.com>
> ---
> 
> Of course you will need to adjust the subject and prepend it with
> PATCH v4, just as you did with a v3 for this patch.
> 
> Then, this stuff:
> 
>> * Third version
>>
>> ** Updated with extra spacing and indentation
> 
> Do you use checkpatch.pl for your patches? If not please
> add check your patches after committing them with:
> 
> git show ./scripts/checkpatch.pl -
> vi drivers/bluetooth/hci_ath.c
> git commit -a --amend
> 
> And repeat until checkpatch.pl is git happy :)

Yes, I had done a checkpatch round with the patch. it looked happy. Did 
you see any issue that could should have been caught by it?

> 
>> ** made function definitions static
>> ** Removed inline and register keyword usage.
>> ** Removed unused return calls.
>> ** Incorporated code comments by Luis and Gustavo
>>
>> Thanks Luis and Gustavo for your comments
> 
> 
> Remove it from the commit log, if you do want to add some extra text
> to the patch youc an put it below the three dashes ("-") where the
> diff stat goes:
> 
>>
>> Signed-off-by: Suraj <suraj@atheros.com>
>>
>> ---
>   ^^^
> 
> These are the three lines, anything below is ignored by git am
> when the maintainer applies the patch. So you can add anything you
> want ignored by git am here.
> 
>>  drivers/bluetooth/Kconfig     |   11 ++
>>  drivers/bluetooth/Makefile    |    1 +
>>  drivers/bluetooth/hci_ath.c   |  384 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/bluetooth/hci_ldisc.c |    6 +
>>  drivers/bluetooth/hci_uart.h  |    8 +-
>>  5 files changed, 409 insertions(+), 1 deletions(-)
>>  create mode 100755 drivers/bluetooth/hci_ath.c
>>
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 058fbcc..81abeff 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -58,6 +58,17 @@ config BT_HCIUART_BCSP
>>
>>         Say Y here to compile support for HCI BCSP protocol.
>>
>> +config BT_HCIUART_ATH
>> +     bool "Atheros AR300x Board support"
>> +     depends on BT_HCIUART
>> +     help
>> +       HCIATH (HCI Atheros) is a serial protocol for communication
>> +       between Bluetooth device and host with support for Atheros AR300x
>> +       power management feature. This protocol is required for
>> +       serial Bluetooth devices that are based on Atheros AR300x chips.
> 
> Please adjust the description as well. How about:
> 
> +         HCIATH (HCI Atheros) is a serial protocol for communication
> +         between the host and Atheros AR300x Bluetooth devices. The
> +         protocol implements enhaned power management features for the
> +         the AR300x chipsets, it lets the controller chip go to sleep
> +         mode if there is no Bluetooth activity for some time and wakes
> +         up the chip in case of a Bluetooth activity. Enabling this
> +         option will build HCI Atheros support into the hci_uart driver.
> +         Enable this option if you have an UART Atheros AR300x serial
> +         device.
> 
> 
>> +
>> +       Say Y here to compile support for HCIATH protocol.
>> +
>>  config BT_HCIUART_LL
>>       bool "HCILL protocol support"
>>       depends on BT_HCIUART
>> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
>> index 7e5aed5..1481faa 100644
>> --- a/drivers/bluetooth/Makefile
>> +++ b/drivers/bluetooth/Makefile
>> @@ -26,4 +26,5 @@ hci_uart-y                          := hci_ldisc.o
>>  hci_uart-$(CONFIG_BT_HCIUART_H4)     += hci_h4.o
>>  hci_uart-$(CONFIG_BT_HCIUART_BCSP)   += hci_bcsp.o
>>  hci_uart-$(CONFIG_BT_HCIUART_LL)     += hci_ll.o
>> +hci_uart-$(CONFIG_BT_HCIUART_ATH)    += hci_ath.o
>>  hci_uart-objs                                := $(hci_uart-y)
>> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
>> new file mode 100755
>> index 0000000..2f91954
>> --- /dev/null
>> +++ b/drivers/bluetooth/hci_ath.c
>> @@ -0,0 +1,384 @@
>> +/*
>> + * Copyright (c) 2009-2010 Atheros Communications Inc.
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; either version 2 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  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/module.h>
>> +#include <linux/kernel.h>
>> +
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/errno.h>
>> +#include <linux/ioctl.h>
>> +#include <linux/skbuff.h>
>> +
>> +#include <net/bluetooth/bluetooth.h>
>> +#include <net/bluetooth/hci_core.h>
>> +
>> +#include "hci_uart.h"
>> +
>> +
>> +/* HCIATH receiver States */
>> +#define HCIATH_W4_PACKET_TYPE                        0
>> +#define HCIATH_W4_EVENT_HDR                  1
>> +#define HCIATH_W4_ACL_HDR                    2
>> +#define HCIATH_W4_SCO_HDR                    3
>> +#define HCIATH_W4_DATA                               4
>> +
>> +struct ath_struct {
>> +     struct hci_uart *hu;
>> +     unsigned int rx_state;
>> +     unsigned int rx_count;
>> +     unsigned int cur_sleep;
>> +
>> +     spinlock_t hciath_lock;
>> +     struct sk_buff *rx_skb;
>> +     struct sk_buff_head txq;
>> +     wait_queue_head_t wqevt;
>> +     struct work_struct ctxtsw;
>> +};
>> +
>> +static int ath_wakeup_ar3001(struct tty_struct *tty)
>> +{
>> +     struct termios settings;
>> +     int status = 0x00;
>> +
>> +     status = tty->driver->ops->tiocmget(tty, NULL);
>> +
>> +     if ((status & TIOCM_CTS))
>> +             return status;
> 
> No need for double () parens here. This should be fine:
> 
> +       if (status & TIOCM_CTS)
> +               return status;
> 
> You would use double parens if you are doing a check against
> another flag as well.
> 
>> +
>> +     n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
>> +
>> +     /* Disable Automatic RTSCTS */
>> +     settings.c_cflag &= ~CRTSCTS;
>> +     n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
>> +
>> +     status = tty->driver->ops->tiocmget(tty, NULL);
>> +
>> +     /* Clear RTS first */
>> +     tty->driver->ops->tiocmset(tty, NULL, 0x00, TIOCM_RTS);
>> +     mdelay(20);
>> +
>> +     status = tty->driver->ops->tiocmget(tty, NULL);
>> +
>> +     /* Set RTS, wake up board */
>> +     tty->driver->ops->tiocmset(tty, NULL, TIOCM_RTS, 0x00);
>> +     mdelay(20);
>> +
>> +     status = tty->driver->ops->tiocmget(tty, NULL);
>> +
>> +     n_tty_ioctl_helper(tty, NULL, TCGETS, (unsigned long)&settings);
>> +
>> +     settings.c_cflag |= CRTSCTS;
>> +     n_tty_ioctl_helper(tty, NULL, TCSETS, (unsigned long)&settings);
>> +
>> +     return status;
>> +}
>> +
>> +static void ath_context_switch(struct work_struct *work)
> 
> Can you please rename this to ath_hci_uart_work()
> 
>> +{
>> +     int status;
>> +     struct ath_struct *ath;
>> +     struct hci_uart *hu;
>> +     struct tty_struct *tty;
>> +
>> +     ath = container_of(work, struct ath_struct, ctxtsw);
>> +
>> +     hu = ath->hu;
>> +     tty = hu->tty;
>> +
>> +     /* verify and wake up controller */
>> +     if (ath->cur_sleep) {
>> +
>> +             status = ath_wakeup_ar3001(tty);
>> +
>> +             if (!(status & TIOCM_CTS))
>> +                     return;
>> +     }
>> +
>> +     /* 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);
>> +     spin_lock_init(&ath->hciath_lock);
>> +
>> +     hu->priv = ath;
>> +     ath->hu = hu;
>> +
>> +     init_waitqueue_head(&ath->wqevt);
>> +     INIT_WORK(&ath->ctxtsw, ath_context_switch);
>> +
>> +     return 0;
>> +}
>> +
>> +/* Flush protocol data */
>> +static int ath_flush(struct hci_uart *hu)
>> +{
>> +     struct ath_struct *ath = hu->priv;
>> +
>> +     BT_DBG("hu %p", hu);
>> +
>> +     skb_queue_purge(&ath->txq);
>> +
>> +     return 0;
>> +}
>> +
>> +/* Close protocol */
>> +static int ath_close(struct hci_uart *hu)
>> +{
>> +     struct ath_struct *ath = hu->priv;
>> +
>> +     BT_DBG("hu %p", hu);
>> +
>> +     skb_queue_purge(&ath->txq);
>> +
>> +     if (ath->rx_skb)
>> +             kfree_skb(ath->rx_skb);
> 
> No need for the check, kfree_skb() does that for you
> and it has it optimized for the case where the skb you
> pass is NULL. You can just do:
> 
> +       kfree_skb(ath->rx_skb);
> 
>> +
>> +     cancel_work_sync(&ath->ctxtsw);
>> +
>> +     hu->priv = NULL;
>> +     kfree(ath);
>> +
>> +     return 0;
>> +}
> 
> Did you give this new patch a spin by looping bringing up,
> scanning, bringing the interface down? Or have a loop doing
> a scan while in another window you bring the interface up
> and down?
> 
Yep, Have been testing it for sometime. Haven't seen any issue yet (Hope 
I dont see any :-) )
>> +
>> +/* Enqueue frame for transmittion */
>> +static int ath_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>> +{
>> +     struct ath_struct *ath = hu->priv;
>> +
>> +     if (bt_cb(skb)->pkt_type == HCI_SCODATA_PKT) {
>> +
>> +             /* Discard SCO packet.AR3001 does not support SCO over HCI */
> 
> Add a space after packet. Why does it not support SCO over HCI BTW?
> Just curious. I'm new to BT :) Is this common? If this is common can't
> the BT stack be informed of these things so that they don't pass the skbs
> to the driver?
> 
 From my experience, it is common for smaller BT devices (headsets) and 
devices with memory limitations.

I guess, there is no way to tell SCO routing using any HCI cmd/Event.

In the Linux Ubuntu distro that I use, it is controlled using the 
"SCORouting" flag in "/etc/bluetooth/audio.conf". Since it is a user 
configuration, driver can not assume that the flag will be set.

So, this is like a precautionary check.



> This could be done in a separate patch though if this is the case
> though.
> 
>> +             BT_DBG("SCO Packet over HCI received Dropping");
>> +
>> +             kfree(skb);
>> +
>> +             return 0;
>> +     }
>> +
>> +     BT_DBG("hu %p skb %p", hu, skb);
>> +
>> +     /* Prepend skb with frame type */
>> +     memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
>> +
>> +     skb_queue_tail(&ath->txq, skb);
>> +     set_bit(HCI_UART_SENDING, &hu->tx_state);
>> +
>> +     schedule_work(&ath->ctxtsw);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct sk_buff *ath_dequeue(struct hci_uart *hu)
>> +{
>> +     struct ath_struct *ath = hu->priv;
>> +     struct sk_buff *skbuf;
>> +
>> +     skbuf = skb_dequeue(&ath->txq);
>> +
>> +     if (!skbuf)
>> +             return NULL;
>> +
>> +
>> +     /*
>> +      * Check if the HCI command is  HCI sleep enable and
>> +      * update the sleep enable flag with command parameter.
>> +      *
>> +      * Value of sleep enable flag will be used later
>> +      * to verify if controller has to be woken up before
>> +      * sending any packet.
>> +      */
>> +     if (skbuf->data[0] == 0x01 && skbuf->data[1] == 0x04 &&
>> +         skbuf->data[2] == 0xFC)
>> +             ath->cur_sleep = skbuf->data[4];
> 
> Might as well just do this:
> 
> +       if (skbuf->data[0] == 0x01 &&
> +           skbuf->data[1] == 0x04 &&
> +           skbuf->data[2] == 0xFC)
> +               ath->cur_sleep = skbuf->data[4];
> 
> Is this sort of check done in any other drivers/protocols? If so a
> helper could be added to hci_uart.h if this is the case, but likely
> better though a separate patch.

No, this check is done only for this specific driver/protocol.

This is the reason why we had to go for board specific driver change. 
Otherways, we could have used the default HCI driver implementation.

> 
>> +
>> +     return skbuf;
>> +}
>> +
>> +static void ath_check_data_len(struct ath_struct *ath, int len)
>> +{
>> +     int room = skb_tailroom(ath->rx_skb);
>> +
>> +     BT_DBG("len %d room %d", len, room);
>> +
>> +     if (len > room) {
>> +             BT_ERR("Data length is too large");
>> +             kfree_skb(ath->rx_skb);
>> +             ath->rx_state = HCIATH_W4_PACKET_TYPE;
>> +             ath->rx_skb = NULL;
>> +             ath->rx_count = 0;
>> +     } else {
>> +             ath->rx_state = HCIATH_W4_DATA;
>> +             ath->rx_count = len;
>> +     }
>> +}
>> +
>> +/* Recv data */
>> +static int ath_recv(struct hci_uart *hu, void *data, int count)
>> +{
>> +     struct ath_struct *ath = hu->priv;
>> +     char *ptr = data;
>> +     struct hci_event_hdr *eh;
>> +     struct hci_acl_hdr *ah;
>> +     struct hci_sco_hdr *sh;
>> +     int len, type, dlen;
>> +
>> +
>> +     BT_DBG("hu %p count %d rx_state %d rx_count %d", hu, count,
>> +            ath->rx_state, ath->rx_count);
>> +
>> +     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:
>> +                             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 -ENOMEM;
>> +             }
>> +             ath->rx_skb->dev = (void *)hu->hdev;
>> +             bt_cb(ath->rx_skb)->pkt_type = type;
>> +     }
>> +
>> +     return count;
>> +}
>> +
>> +static struct hci_uart_proto athp = {
>> +     .id = HCI_UART_ATH,
>> +     .open = ath_open,
>> +     .close = ath_close,
>> +     .recv = ath_recv,
>> +     .enqueue = ath_enqueue,
>> +     .dequeue = ath_dequeue,
>> +     .flush = ath_flush,
>> +};
>> +
>> +int ath_init(void)
>> +{
>> +     int err = hci_uart_register_proto(&athp);
>> +
>> +     if (!err)
>> +             BT_INFO("HCIATH protocol initialized");
>> +     else
>> +             BT_ERR("HCIATH protocol registration failed");
>> +
>> +     return err;
>> +}
>> +
>> +int ath_deinit(void)
>> +{
>> +     return hci_uart_unregister_proto(&athp);
>> +}
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index 76a1abb..7dd76d1 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -542,6 +542,9 @@ static int __init hci_uart_init(void)
>>  #ifdef CONFIG_BT_HCIUART_LL
>>       ll_init();
>>  #endif
>> +#ifdef CONFIG_BT_HCIUART_ATH
>> +     ath_init();
>> +#endif
>>
>>       return 0;
>>  }
>> @@ -559,6 +562,9 @@ static void __exit hci_uart_exit(void)
>>  #ifdef CONFIG_BT_HCIUART_LL
>>       ll_deinit();
>>  #endif
>> +#ifdef CONFIG_BT_HCIUART_ATH
>> +     ath_deinit();
>> +#endif
>>
>>       /* Release tty registration of line discipline */
>>       if ((err = tty_unregister_ldisc(N_HCI)))
>> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
>> index 50113db..385537f 100644
>> --- a/drivers/bluetooth/hci_uart.h
>> +++ b/drivers/bluetooth/hci_uart.h
>> @@ -33,13 +33,14 @@
>>  #define HCIUARTGETDEVICE     _IOR('U', 202, int)
>>
>>  /* UART protocols */
>> -#define HCI_UART_MAX_PROTO   5
>> +#define HCI_UART_MAX_PROTO   6
>>
>>  #define HCI_UART_H4  0
>>  #define HCI_UART_BCSP        1
>>  #define HCI_UART_3WIRE       2
>>  #define HCI_UART_H4DS        3
>>  #define HCI_UART_LL  4
>> +#define HCI_UART_ATH 5
>>
>>  struct hci_uart;
>>
>> @@ -91,3 +92,8 @@ int bcsp_deinit(void);
>>  int ll_init(void);
>>  int ll_deinit(void);
>>  #endif
>> +
>> +#ifdef CONFIG_BT_HCIUART_ATH
>> +int ath_init(void);
>> +int ath_deinit(void);
>> +#endif
>> --
>> 1.7.0
>>
>>
>>

Thanks for the comments.

Regards,
Suraj