Hi Suraj,
* Suraj Sumangala <suraj@xxxxxxxxxxx> [2010-04-22 12:24:04 +0530]:
>
> Hi Gustavo,
>
> Gustavo F. Padovan wrote:
> >* suraj <suraj@xxxxxxxxxxx> [2010-04-21 15:52:17 +0530]:
> >
> >>This implements the Atheros Bluetooth serial protocol to
> >>support the AR300x Bluetooth chipsets.
> >>The serial protocol implements enhanced power management
> >>features for the AR300x chipsets.
> >>
> >>Reviewed-by: Luis R. Rodriguez <lrodriguez@xxxxxxxxxxx>
> >>Signed-off-by: Suraj <suraj@xxxxxxxxxxx>
> >>
> >>---
> >> drivers/bluetooth/Kconfig | 14 ++
> >> drivers/bluetooth/Makefile | 1 +
> >> drivers/bluetooth/hci_ath.c | 378 +++++++++++++++++++++++++++++++++++++++++
> >> drivers/bluetooth/hci_ldisc.c | 6 +
> >> drivers/bluetooth/hci_uart.h | 8 +-
> >> 5 files changed, 406 insertions(+), 1 deletions(-)
> >> create mode 100755 drivers/bluetooth/hci_ath.c
> >>
..snip..
> >>+
> >>+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;
> >
> >Use hci_event_hdr() here like hci_h4.c does.
> >
> >>+
> >>+ 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;
> >
> >And hci_acl_hdr() here.
> >
> >>+ 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;
> >
> >hci_sco_hdr() here.
> >
> >>+
> >>+ 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;
> >>+}
> >
> >Just out of curiosity. I never worked in the driver world, but is it ok
> >to duplicate lots of code when working with drivers? hci_h4.c and this
> >patch share lots of similar code. ath_recv() and h4_recv() are exactly
> >the same except for one line, the ath->rx_count = 0; at case HCIATH_W4_DATA.
> >Does this line makes real difference? recv() is not the only function
> >duplicating code here.
>
> Let me try to anwer you here. I am also new to the driver world, so
> correct me if I am wrong.
>
> Both drivers do the same job, expect that the ATH driver does
> something more on the data transmit path. So, the receive path does
> the same thing. That is the reason why both looks same. This could
> possibly change later as new feature will be added to the ATH
> protocol.
Ok. I'm thinking if it is not possible create a separated 'lib' with the
common code. That's why I asked about the code duplication. I don't know
how worth it is, since we have only 3 drivers using similar code.
>
>
> The function "hci_recv_fragment()" could potentially replace most of
> the code in the ath_recv() function. But, unfortunately this
> function require the caller to provide the HCI Packet type as
> parameter.
>
> This defeats all the advantage of "hci_recv_fragment()" in a UART
> HCI transport driver as all types of packets are received through
> the same callback. So the caller will have to write the same messy
> code in ath_recv() to find out the packet type negating all the
> advantages of "hci_recv_fragment()".
>
>
> >
> >>+
> >>+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;
> >>+}
> >
> >BTW, we never check this return value on hci_ldisc.c, why?
>
> you, are correct. Just thought of keeping the same signature used by
> other protocol. moreover hci_ldisc.c being a common file used by all
> protocol it is possible that somebody want to check the return value
> at later point of time. So, kept it that way so that we do not have
> a problem then :-D
>
> Your thoughts?
Yes, your code is right, the return value should be kept. The problem
is that hci_uart_init() doesn't check any other protocol initialization
(h4, bcsp and ll). My thought is: we really need to check?
Marcel, can you answer that?
> >
> >>+
> >>+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
> >>
> >>
> >>
> >>
> >>
> >
> >--
> >Gustavo F. Padovan
> >http://padovan.org
>
> --
> 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
--
Gustavo F. Padovan
http://padovan.org
--
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
