Return-Path: Content-Type: text/plain; charset=US-ASCII Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\)) Subject: Re: [PATCH 4/5] Broadcom Bluetooth protocol UART support From: Marcel Holtmann In-Reply-To: <5572AC25.5020508@broadcom.com> Date: Sat, 6 Jun 2015 10:31:44 +0200 Cc: Ilya Faenson , linux-bluetooth@vger.kernel.org Message-Id: References: <1433365304-16707-1-git-send-email-ifaenson@broadcom.com> <1433365304-16707-5-git-send-email-ifaenson@broadcom.com> <5572AC25.5020508@broadcom.com> To: Arend van Spriel Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Arend, >>> Merge of the Broadcom logic into the Intel's btbcm implementation. >>> >>> Signed-off-by: Ilya Faenson >>> --- >>> drivers/bluetooth/btbcm.c | 142 ++++++++++++++++++++++++++++++++++++---------- >>> drivers/bluetooth/btbcm.h | 21 ++++++- >>> 2 files changed, 132 insertions(+), 31 deletions(-) >>> >>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c >>> index 728fce3..e1d5ad0 100644 >>> --- a/drivers/bluetooth/btbcm.c >>> +++ b/drivers/bluetooth/btbcm.c >>> @@ -3,6 +3,7 @@ >>> * Bluetooth support for Broadcom devices >>> * >>> * Copyright (C) 2015 Intel Corporation >>> + * Copyright (C) 2015 Broadcom Corporation >>> * >>> * >>> * This program is free software; you can redistribute it and/or modify >>> @@ -32,7 +33,8 @@ >>> >>> #define VERSION "0.1" >>> >>> -#define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}}) >>> +#define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00} }) >>> +#define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43} }) >> >> something funky is going on with your editor that insist on fixing this ;) >> >>> >>> int btbcm_check_bdaddr(struct hci_dev *hdev) >>> { >>> @@ -43,6 +45,7 @@ int btbcm_check_bdaddr(struct hci_dev *hdev) >>> HCI_INIT_TIMEOUT); >>> if (IS_ERR(skb)) { >>> int err = PTR_ERR(skb); >>> + >> >> In this specific case, please do not fix it. I like to keep the simple error path sections small. >> >>> BT_ERR("%s: BCM: Reading device address failed (%d)", >>> hdev->name, err); >>> return err; >>> @@ -56,10 +59,11 @@ int btbcm_check_bdaddr(struct hci_dev *hdev) >>> >>> bda = (struct hci_rp_read_bd_addr *)skb->data; >>> >>> - /* The address 00:20:70:02:A0:00 indicates a BCM20702A0 controller >>> - * with no configured address. >>> + /* Check if the address indicates a controller with no configured >>> + * address. >>> */ >>> - if (!bacmp(&bda->bdaddr, BDADDR_BCM20702A0)) { >>> + if (!bacmp(&bda->bdaddr, BDADDR_BCM20702A0) || >>> + !bacmp(&bda->bdaddr, BDADDR_BCM4324B3)) { >>> BT_INFO("%s: BCM: Using default device address (%pMR)", >>> hdev->name,&bda->bdaddr); >>> set_bit(HCI_QUIRK_INVALID_BDADDR,&hdev->quirks); >>> @@ -89,21 +93,14 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr) >>> } >>> EXPORT_SYMBOL_GPL(btbcm_set_bdaddr); >>> >>> -int btbcm_patchram(struct hci_dev *hdev, const char *firmware) >>> +int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw) >>> { >>> const struct hci_command_hdr *cmd; >>> - const struct firmware *fw; >>> const u8 *fw_ptr; >>> size_t fw_size; >>> struct sk_buff *skb; >>> u16 opcode; >>> - int err; >>> - >>> - err = request_firmware(&fw, firmware,&hdev->dev); >>> - if (err< 0) { >>> - BT_INFO("%s: BCM: Patch %s not found", hdev->name, firmware); >>> - return err; >>> - } >>> + int err = 0; >>> >>> /* Start Download */ >>> skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT); >>> @@ -129,8 +126,7 @@ int btbcm_patchram(struct hci_dev *hdev, const char *firmware) >>> fw_size -= sizeof(*cmd); >>> >>> if (fw_size< cmd->plen) { >>> - BT_ERR("%s: BCM: Patch %s is corrupted", hdev->name, >>> - firmware); >>> + BT_ERR("%s: BCM: Patch is corrupted", hdev->name); >>> err = -EINVAL; >>> goto done; >>> } >>> @@ -156,7 +152,6 @@ int btbcm_patchram(struct hci_dev *hdev, const char *firmware) >>> msleep(250); >>> >>> done: >>> - release_firmware(fw); >>> return err; >>> } >>> EXPORT_SYMBOL(btbcm_patchram); >>> @@ -168,6 +163,7 @@ static int btbcm_reset(struct hci_dev *hdev) >>> skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT); >>> if (IS_ERR(skb)) { >>> int err = PTR_ERR(skb); >>> + >>> BT_ERR("%s: BCM: Reset failed (%d)", hdev->name, err); >>> return err; >>> } >>> @@ -242,9 +238,101 @@ static const struct { >>> const char *name; >>> } bcm_uart_subver_table[] = { >>> { 0x410e, "BCM43341B0" }, /* 002.001.014 */ >>> + { 0x4406, "BCM4324B3" }, /* 002.004.006 */ >>> + { 0x610c, "BCM4354_003.001.012.0306.0659"}, /* 003.001.012 */ >> >> Please just BCM4354 here. >> >> And I have initial patches for the manifest file to map modalias to firmware names. That will then hopefully solve all of these and we can just update this easily from userspace. I will send patches as soon as I have cleaned them up a bit. > > Is using modalias sufficient. At least for our wifi chips it is not as new chip revisions with same modalias require different firmware. Same may be true for bt chips. let me put it this way, the Windows driver maps the firmware based on USB VID/PID and thus that seems fine then. Also it seems that every ACPI based platform gets a proper ID assigned. So that makes sense as well there. So there are plenty of USB dongles that share the same firmware, but it seems to be manual mapping based on who tested and certified a given firmware for a given piece of hardware. It is just too many that I really want to know. I rather have them all listed and then someone can update it from userspace instead of having to deal with it in the kernel. Regards Marcel