RE: [PATCH v10 3/3] Bluetooth: hci_uart: Support firmware download for Marvell

2016-05-05 Thread Amitkumar Karwar 
Hi Alan,

> From: One Thousand Gnomes [mailto:gno...@lxorguk.ukuu.org.uk]
> Sent: Thursday, May 05, 2016 8:53 PM
> To: Amitkumar Karwar
> Cc: linux-blueto...@vger.kernel.org; linux-kernel@vger.kernel.org;
> Ganapathi Bhat
> Subject: Re: [PATCH v10 3/3] Bluetooth: hci_uart: Support firmware
> download for Marvell
> 
> > +/* Send ACK/NAK to the device */
> > +static void mrvl_send_ack(struct hci_uart *hu, unsigned char ack) {
> > +   struct tty_struct *tty = hu->tty;
> > +
> > +   tty->ops->write(tty, , sizeof(ack)); }
> 
> You don't know if the device has a write method, and it should be
> locked.
> This should go via your ldisc not directly.
> 

Thanks for review. We will take care of this.

Regards,
Amitkumar

RE: [PATCH v10 3/3] Bluetooth: hci_uart: Support firmware download for Marvell

2016-05-05 Thread Amitkumar Karwar 
Hi Alan,

> From: One Thousand Gnomes [mailto:gno...@lxorguk.ukuu.org.uk]
> Sent: Thursday, May 05, 2016 8:53 PM
> To: Amitkumar Karwar
> Cc: linux-blueto...@vger.kernel.org; linux-kernel@vger.kernel.org;
> Ganapathi Bhat
> Subject: Re: [PATCH v10 3/3] Bluetooth: hci_uart: Support firmware
> download for Marvell
> 
> > +/* Send ACK/NAK to the device */
> > +static void mrvl_send_ack(struct hci_uart *hu, unsigned char ack) {
> > +   struct tty_struct *tty = hu->tty;
> > +
> > +   tty->ops->write(tty, , sizeof(ack)); }
> 
> You don't know if the device has a write method, and it should be
> locked.
> This should go via your ldisc not directly.
> 

Thanks for review. We will take care of this.

Regards,
Amitkumar

Re: [PATCH v10 3/3] Bluetooth: hci_uart: Support firmware download for Marvell

2016-05-05 Thread One Thousand Gnomes 
> +/* Send ACK/NAK to the device */
> +static void mrvl_send_ack(struct hci_uart *hu, unsigned char ack)
> +{
> + struct tty_struct *tty = hu->tty;
> +
> + tty->ops->write(tty, , sizeof(ack));
> +}

You don't know if the device has a write method, and it should be locked.
This should go via your ldisc not directly.

Alan

Re: [PATCH v10 3/3] Bluetooth: hci_uart: Support firmware download for Marvell

2016-05-05 Thread One Thousand Gnomes 
> +/* Send ACK/NAK to the device */
> +static void mrvl_send_ack(struct hci_uart *hu, unsigned char ack)
> +{
> + struct tty_struct *tty = hu->tty;
> +
> + tty->ops->write(tty, , sizeof(ack));
> +}

You don't know if the device has a write method, and it should be locked.
This should go via your ldisc not directly.

Alan

[PATCH v10 3/3] Bluetooth: hci_uart: Support firmware download for Marvell

2016-05-05 Thread Amitkumar Karwar 
From: Ganapathi Bhat 

This patch implement firmware download feature for
Marvell Bluetooth devices. If firmware is already
downloaded, it will skip downloading.

Signed-off-by: Ganapathi Bhat 
Signed-off-by: Amitkumar Karwar 
---
v2: Fixed compilation warning reported by kbuild test robot
v3: Addressed review comments from Marcel Holtmann
  a) Removed vendor specific code from hci_ldisc.c
  b) Get rid of static forward declaration
  c) Removed unnecessary heavy nesting
  d) Git rid of module parameter and global variables
  e) Add logic to pick right firmware image
v4: Addresses review comments from Alan
  a) Use existing kernel helper APIs instead of writing own.
  b) Replace mdelay() with msleep()
v5: Addresses review comments from Loic Poulain
  a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
  b) Used static functions where required and removed forward delcarations
  c) Edited comments for the function hci_uart_recv_data
  d) Made HCI_UART_DNLD_FW flag a part of driver private data
v6: Addresses review comments from Loic Poulain
  a) Used skb instead of array to store firmware data during download
  b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write
  c) Used GFP_KERNEL instead of GFP_ATOMIC
v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change resolves
   errors reported by kbuild test robot.
v8: Addressed review comments from Marcel Holtmann
  a) Removed unnecessary memory allocation failure messages
  b) Get rid of btmrvl.h header file and add definitions in hci_mrvl.c file
v9: Addressed review comments from Marcel Holtmann
  a) Moved firmware download code from setup to prepare handler.
  b) Change messages from bt_dev_*->BT_*, as hdev isn't available during 
firmware
 download.
v10: Addressed review comments from Marcel Holtmann
  a) Added new callback recv_for_prepare to receive data from device
 during prepare phase
  b) Avoided using private flags (HCI_UART_DNLD_FW) as new receive callback is
 added for the same purpose
  c) Used kernel API to handle unaligned data
  d) Moved mrvl_set_baud functionality inside setup callback
---
 drivers/bluetooth/Kconfig |  11 +
 drivers/bluetooth/Makefile|   1 +
 drivers/bluetooth/hci_ldisc.c |   6 +
 drivers/bluetooth/hci_mrvl.c  | 535 ++
 drivers/bluetooth/hci_uart.h  |   8 +-
 5 files changed, 560 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/hci_mrvl.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index cf50fd2..daafd0c 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -180,6 +180,17 @@ config BT_HCIUART_AG6XX
 
  Say Y here to compile support for Intel AG6XX protocol.
 
+config BT_HCIUART_MRVL
+   bool "Marvell protocol support"
+   depends on BT_HCIUART
+   select BT_HCIUART_H4
+   help
+ Marvell is serial protocol for communication between Bluetooth
+ device and host. This protocol is required for most Marvell Bluetooth
+ devices with UART interface.
+
+ Say Y here to compile support for HCI MRVL protocol.
+
 config BT_HCIBCM203X
tristate "HCI BCM203x USB driver"
depends on USB
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 9c18939..364dbb6 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
 hci_uart-$(CONFIG_BT_HCIUART_BCM)  += hci_bcm.o
 hci_uart-$(CONFIG_BT_HCIUART_QCA)  += hci_qca.o
 hci_uart-$(CONFIG_BT_HCIUART_AG6XX)+= hci_ag6xx.o
+hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o
 hci_uart-objs  := $(hci_uart-y)
 
 ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 047e786..4896b6f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -821,6 +821,9 @@ static int __init hci_uart_init(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
ag6xx_init();
 #endif
+#ifdef CONFIG_BT_HCIUART_MRVL
+   mrvl_init();
+#endif
 
return 0;
 }
@@ -856,6 +859,9 @@ static void __exit hci_uart_exit(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
ag6xx_deinit();
 #endif
+#ifdef CONFIG_BT_HCIUART_MRVL
+   mrvl_deinit();
+#endif
 
/* Release tty registration of line discipline */
err = tty_unregister_ldisc(N_HCI);
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
new file mode 100644
index 000..8416014
--- /dev/null
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -0,0 +1,535 @@
+/* Bluetooth HCI UART driver for Marvell devices
+ *
+ * Copyright (C) 2016, Marvell International Ltd.
+ *
+ *  Acknowledgements:
+ *  This file is based on hci_h4.c, which was written
+ *  by Maxim Krasnyansky and Marcel Holtmann.
+ *
+ * This software file (the "File") is distributed by Marvell International

[PATCH v10 3/3] Bluetooth: hci_uart: Support firmware download for Marvell

2016-05-05 Thread Amitkumar Karwar 
From: Ganapathi Bhat 

This patch implement firmware download feature for
Marvell Bluetooth devices. If firmware is already
downloaded, it will skip downloading.

Signed-off-by: Ganapathi Bhat 
Signed-off-by: Amitkumar Karwar 
---
v2: Fixed compilation warning reported by kbuild test robot
v3: Addressed review comments from Marcel Holtmann
  a) Removed vendor specific code from hci_ldisc.c
  b) Get rid of static forward declaration
  c) Removed unnecessary heavy nesting
  d) Git rid of module parameter and global variables
  e) Add logic to pick right firmware image
v4: Addresses review comments from Alan
  a) Use existing kernel helper APIs instead of writing own.
  b) Replace mdelay() with msleep()
v5: Addresses review comments from Loic Poulain
  a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
  b) Used static functions where required and removed forward delcarations
  c) Edited comments for the function hci_uart_recv_data
  d) Made HCI_UART_DNLD_FW flag a part of driver private data
v6: Addresses review comments from Loic Poulain
  a) Used skb instead of array to store firmware data during download
  b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write
  c) Used GFP_KERNEL instead of GFP_ATOMIC
v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change resolves
   errors reported by kbuild test robot.
v8: Addressed review comments from Marcel Holtmann
  a) Removed unnecessary memory allocation failure messages
  b) Get rid of btmrvl.h header file and add definitions in hci_mrvl.c file
v9: Addressed review comments from Marcel Holtmann
  a) Moved firmware download code from setup to prepare handler.
  b) Change messages from bt_dev_*->BT_*, as hdev isn't available during 
firmware
 download.
v10: Addressed review comments from Marcel Holtmann
  a) Added new callback recv_for_prepare to receive data from device
 during prepare phase
  b) Avoided using private flags (HCI_UART_DNLD_FW) as new receive callback is
 added for the same purpose
  c) Used kernel API to handle unaligned data
  d) Moved mrvl_set_baud functionality inside setup callback
---
 drivers/bluetooth/Kconfig |  11 +
 drivers/bluetooth/Makefile|   1 +
 drivers/bluetooth/hci_ldisc.c |   6 +
 drivers/bluetooth/hci_mrvl.c  | 535 ++
 drivers/bluetooth/hci_uart.h  |   8 +-
 5 files changed, 560 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/hci_mrvl.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index cf50fd2..daafd0c 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -180,6 +180,17 @@ config BT_HCIUART_AG6XX
 
  Say Y here to compile support for Intel AG6XX protocol.
 
+config BT_HCIUART_MRVL
+   bool "Marvell protocol support"
+   depends on BT_HCIUART
+   select BT_HCIUART_H4
+   help
+ Marvell is serial protocol for communication between Bluetooth
+ device and host. This protocol is required for most Marvell Bluetooth
+ devices with UART interface.
+
+ Say Y here to compile support for HCI MRVL protocol.
+
 config BT_HCIBCM203X
tristate "HCI BCM203x USB driver"
depends on USB
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 9c18939..364dbb6 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
 hci_uart-$(CONFIG_BT_HCIUART_BCM)  += hci_bcm.o
 hci_uart-$(CONFIG_BT_HCIUART_QCA)  += hci_qca.o
 hci_uart-$(CONFIG_BT_HCIUART_AG6XX)+= hci_ag6xx.o
+hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o
 hci_uart-objs  := $(hci_uart-y)
 
 ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 047e786..4896b6f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -821,6 +821,9 @@ static int __init hci_uart_init(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
ag6xx_init();
 #endif
+#ifdef CONFIG_BT_HCIUART_MRVL
+   mrvl_init();
+#endif
 
return 0;
 }
@@ -856,6 +859,9 @@ static void __exit hci_uart_exit(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
ag6xx_deinit();
 #endif
+#ifdef CONFIG_BT_HCIUART_MRVL
+   mrvl_deinit();
+#endif
 
/* Release tty registration of line discipline */
err = tty_unregister_ldisc(N_HCI);
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
new file mode 100644
index 000..8416014
--- /dev/null
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -0,0 +1,535 @@
+/* Bluetooth HCI UART driver for Marvell devices
+ *
+ * Copyright (C) 2016, Marvell International Ltd.
+ *
+ *  Acknowledgements:
+ *  This file is based on hci_h4.c, which was written
+ *  by Maxim Krasnyansky and Marcel Holtmann.
+ *
+ * This software file (the "File") is distributed by Marvell International
+ * Ltd. under the terms of the GNU General Public License