Re: [PATCH v4] Bluetooth: hci_uart: Support firmware download for Marvell
flat view
From: Loic Poulain <hidden>
Date: 2016-03-14 15:54:49
Also in:
lkml
Hi Amitkumar, Quick review inline. On 03/03/2016 12:56, Amitkumar Karwar wrote:
From: Ganapathi Bhat <redacted>
This patch implement firmware download feature for
Marvell Bluetooth devices. If firmware is already
downloaded, it will skip downloading.
Signed-off-by: Ganapathi Bhat <redacted>
Signed-off-by: Amitkumar Karwar <redacted>
---
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()
---
drivers/bluetooth/Kconfig | 10 +
drivers/bluetooth/Makefile | 1 +
drivers/bluetooth/btmrvl.h | 41 +++
drivers/bluetooth/hci_ldisc.c | 6 +
drivers/bluetooth/hci_mrvl.c | 585 ++++++++++++++++++++++++++++++++++++++++++
drivers/bluetooth/hci_uart.h | 13 +-
6 files changed, 655 insertions(+), 1 deletion(-)
create mode 100644 drivers/bluetooth/btmrvl.h
create mode 100644 drivers/bluetooth/hci_mrvl.cquoted hunk ↗ jump to hunk
+ +/* Download firmware to the device */ +static int mrvl_dnld_fw(struct hci_uart *hu, const char *file_name) +{ + struct hci_dev *hdev = hu->hdev; + const struct firmware *fw; + struct sk_buff *skb = NULL; + int offset = 0; + int ret, tx_len; + struct mrvl_data *mrvl = hu->priv; + struct fw_data *fw_data = mrvl->fwdata; + + ret = request_firmware(&fw, file_name, &hdev->dev); + if (ret < 0) { + BT_ERR("request_firmware() failed");
Overall comment, You should use bt_dev_err/warn/dbg helpers when relevant.
+ +void hci_uart_recv_data(struct hci_uart *hu, u8 *buf, int len)
Why this is not a static function ?
quoted hunk ↗ jump to hunk
+{ + struct mrvl_data *mrvl = hu->priv; + struct fw_data *fw_data = mrvl->fwdata; + int i = 0; + + if (len < 5) {
Be careful, here you seem to suppose that tty layer provides well shaped/non-split marvell packets. But this is just a byte stream, you can receive bytes one by one or more than you expect.
quoted hunk ↗ jump to hunk
+ if ((!fw_data->next_index) && + (buf[0] != fw_data->expected_ack)) { + /*ex: XX XX XX*/ + return; + } + } + + if (len == 5) { + if (buf[0] != fw_data->expected_ack) { + /*ex: XX XX XX XX XX*/ + return; + } + /*ex: 5A LL LL LL LL*/ + fw_data->next_index = 0; + mrvl_validate_hdr_and_len(hu, buf); + return; + } + + if (len > 5) { + i = 0; + + while ((i < len) && (buf[i] != fw_data->expected_ack)) + i++; + + if (i == len) { + /* Could not find a header */ + return; + } + + if ((len - i) >= 5) { + /*ex: 00 00 00 00 a5 LL LL LL LL*/ + /*ex: a5 LL LL LL LL 00 00 00 00*/ + /*ex: 00 00 a5 LL LL LL LL 00 00*/ + /*ex: a5 LL LL LL LL*/
Check all your comments and respect Linux Kernel coding style. Add short explanation of the expected data format.
quoted hunk ↗ jump to hunk
+ fw_data->next_index = 0; + mrvl_validate_hdr_and_len(hu, buf + i); + return; + } + + /*ex: 00 00 00 00 a5 LL LL*/ + hci_uart_recv_data(hu, buf + i, len - i); + return; + } + + for (i = 0; i < len; i++) { + fw_data->five_bytes[fw_data->next_index] = buf[i]; + if (++fw_data->next_index == 5) { + fw_data->next_index = 0; + mrvl_validate_hdr_and_len(hu, fw_data->five_bytes); + return; + } + } +}
I think you should rework this function and make it more comprehensible. h4_recv_buf or h5_recv are good examples.
quoted hunk ↗ jump to hunk
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h index 4814ff0..245cab58 100644 --- a/drivers/bluetooth/hci_uart.h +++ b/drivers/bluetooth/hci_uart.h@@ -35,7 +35,7 @@ #define HCIUARTGETFLAGS _IOR('U', 204, int) /* UART protocols */ -#define HCI_UART_MAX_PROTO 10 +#define HCI_UART_MAX_PROTO 11 #define HCI_UART_H4 0 #define HCI_UART_BCSP 1@@ -47,6 +47,7 @@ #define HCI_UART_BCM 7 #define HCI_UART_QCA 8 #define HCI_UART_AG6XX 9 +#define HCI_UART_MRVL 10 #define HCI_UART_RAW_DEVICE 0 #define HCI_UART_RESET_ON_INIT 1@@ -95,11 +96,16 @@ struct hci_uart { /* HCI_UART proto flag bits */ #define HCI_UART_PROTO_SET 0 #define HCI_UART_REGISTERED 1 +#define HCI_UART_DNLD_FW 2
This flag is specific and should be part of your proto private data (mrvl_data).
quoted hunk ↗ jump to hunk
/* TX states */ #define HCI_UART_SENDING 1 #define HCI_UART_TX_WAKEUP 2 +#ifdef CONFIG_BT_HCIUART_MRVL +void hci_uart_recv_data(struct hci_uart *hu, u8 *buf, int len); +int hci_uart_dnld_fw(struct hci_uart *hu);
Why you declare these functions here ? Regards, Loic -- Intel Open Source Technology Center http://oss.intel.com/