On Fri, Apr 21, 2023 at 04:11:39PM +0200, Luca Weiss wrote:
> Add support for the Bluetooth chip codenamed APACHE which is part of
> WCN3988.
>
> The firmware for this chip has a slightly different naming scheme
> compared to most others. For ROM Version 0x0200 we need to use
> apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv +
> apnv11.bin
>
> Signed-off-by: Luca Weiss <luca.weiss@xxxxxxxxxxxxx>
> ---
> drivers/bluetooth/btqca.c | 13 +++++++++++--
> drivers/bluetooth/btqca.h | 12 ++++++++++--
> drivers/bluetooth/hci_qca.c | 12 ++++++++++++
> 3 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index fd0941fe8608..3ee1ef88a640 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> /* Firmware files to download are based on ROM version.
> * ROM version is derived from last two bytes of soc_ver.
> */
> - rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> + if (soc_type == QCA_WCN3988)
> + rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f);
> + else
> + rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
Hi Luca,
perhaps it's just me. But I was wondering if this can be improved on a little.
* Move the common portion outside of the conditional
* And also, I think it's normal to use decimal for shift values.
e.g.
unsigned shift;
...
shift = soc_type == QCA_WCN3988 ? 5 : 4;
rom_ver = ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f);
Using some helpers such as GENMASK and FIELD_PREP might also be nice.
>
> if (soc_type == QCA_WCN6750)
> qca_send_patch_config_cmd(hdev);
>
> /* Download rampatch file */
> config.type = TLV_TYPE_PATCH;
> - if (qca_is_wcn399x(soc_type)) {
> + if (soc_type == QCA_WCN3988) {
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/apbtfw%02x.tlv", rom_ver);
> + } else if (qca_is_wcn399x(soc_type)) {
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/crbtfw%02x.tlv", rom_ver);
> } else if (soc_type == QCA_QCA6390) {
> @@ -636,6 +642,9 @@ 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 (soc_type == QCA_WCN3988)
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/apnv%02x.bin", rom_ver);
> else if (qca_is_wcn399x(soc_type)) {
> if (ver.soc_id == QCA_WCN3991_SOC_ID) {
Not strictly related to this patch, but while reviewing this I noticed that
ver.soc_id is __le32 but QCA_WCN3991_SOC_ID is in host byteorder.
Perhaps a cpu_to_le32() or le32_to_cpu() call is in order here?
> snprintf(config.fwname, sizeof(config.fwname),
...
