Hi Venkata, I agree with Marcel that the version magic is confusing ...
On Tue, Sep 15, 2020 at 09:13:25PM +0530, Venkata Lakshmi Narayana Gubba wrote: > This change will allow to use different NVM file based > on WCN3991 BT SoC ID.Need to use different NVM file based on > fab location for WCN3991 BT SoC. > > Signed-off-by: Venkata Lakshmi Narayana Gubba <gubba...@codeaurora.org> > --- > drivers/bluetooth/btqca.c | 41 +++++++++++++++++++++++++---------------- > drivers/bluetooth/btqca.h | 13 ++++++++----- > drivers/bluetooth/hci_qca.c | 11 +++++------ > 3 files changed, 38 insertions(+), 27 deletions(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index ce9dcff..a7e72f1 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > @@ -14,12 +14,11 @@ > > #define VERSION "0.1" > > -int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version, > +int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver, > enum qca_btsoc_type soc_type) > { > struct sk_buff *skb; > struct edl_event_hdr *edl; > - struct qca_btsoc_version *ver; > char cmd; > int err = 0; > u8 event_type = HCI_EV_VENDOR; > @@ -70,9 +69,9 @@ int qca_read_soc_version(struct hci_dev *hdev, u32 > *soc_version, > } > > if (soc_type >= QCA_WCN3991) > - memmove(&edl->data, &edl->data[1], sizeof(*ver)); > - > - ver = (struct qca_btsoc_version *)(edl->data); > + memcpy(ver, &edl->data[1], sizeof(*ver)); > + else > + memcpy(ver, &edl->data, sizeof(*ver)); > > bt_dev_info(hdev, "QCA Product ID :0x%08x", > le32_to_cpu(ver->product_id)); > @@ -83,13 +82,7 @@ int qca_read_soc_version(struct hci_dev *hdev, u32 > *soc_version, > bt_dev_info(hdev, "QCA Patch Version:0x%08x", > le16_to_cpu(ver->patch_ver)); > > - /* QCA chipset version can be decided by patch and SoC > - * version, combination with upper 2 bytes from SoC > - * and lower 2 bytes from patch will be used. > - */ > - *soc_version = (le32_to_cpu(ver->soc_id) << 16) | > - (le16_to_cpu(ver->rom_ver) & 0x0000ffff); > - if (*soc_version == 0) > + if (le32_to_cpu(ver->soc_id) == 0 || le16_to_cpu(ver->rom_ver) == 0) > err = -EILSEQ; > > out: > @@ -446,15 +439,25 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const > bdaddr_t *bdaddr) > EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome); > > int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > - enum qca_btsoc_type soc_type, u32 soc_ver, > + enum qca_btsoc_type soc_type, struct qca_btsoc_version ver, > const char *firmware_name) > { > struct qca_fw_config config; > int err; > u8 rom_ver = 0; > + u32 soc_ver; > > bt_dev_dbg(hdev, "QCA setup on UART"); > > + /* QCA chipset version can be decided by patch and SoC > + * version, combination with upper 2 bytes from SoC > + * and lower 2 bytes from patch will be used. > + */ > + soc_ver = (le32_to_cpu(ver.soc_id) << 16) | > + (le16_to_cpu(ver.rom_ver) & 0x0000ffff); > + Can we at least do the leN_to_cpu conversions in qca_read_soc_version() as previously to make this less clunky? And/or define a macro to extract 'soc_ver' to unclunkify this further. > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > + > config.user_baud_rate = baudrate; > > /* Download rampatch file */ > @@ -491,9 +494,15 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t > baudrate, > if (firmware_name) > snprintf(config.fwname, sizeof(config.fwname), > "qca/%s", firmware_name); > - else if (qca_is_wcn399x(soc_type)) > - snprintf(config.fwname, sizeof(config.fwname), > - "qca/crnv%02x.bin", rom_ver); > + else if (qca_is_wcn399x(soc_type)) { > + if (ver.soc_id == QCA_WCN3991_SOC_ID) { > + snprintf(config.fwname, sizeof(config.fwname), > + "qca/crnv%02xu.bin", rom_ver); > + } else { > + snprintf(config.fwname, sizeof(config.fwname), > + "qca/crnv%02x.bin", rom_ver); > + } > + } > else if (soc_type == QCA_QCA6390) > snprintf(config.fwname, sizeof(config.fwname), > "qca/htnv%02x.bin", rom_ver); > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h > index d81b74c..d01a9f5 100644 > --- a/drivers/bluetooth/btqca.h > +++ b/drivers/bluetooth/btqca.h > @@ -34,6 +34,8 @@ > #define QCA_HCI_CC_OPCODE 0xFC00 > #define QCA_HCI_CC_SUCCESS 0x00 > > +#define QCA_WCN3991_SOC_ID (0x40014320) The QCA_ prefix seems a bit verbose, given that this is a QCA driver and WCN3991 uniquely identifies the chip. Having the prefix just needlessly clutters conditions, I suggest to just call it SOC_ID_WCN3991.