lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZE+6e7ZxJ2s9DHI1@corigine.com> Date: Mon, 1 May 2023 15:11:23 +0200 From: Simon Horman <simon.horman@...igine.com> To: Luca Weiss <luca.weiss@...rphone.com> Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Balakrishna Godavarthi <bgodavar@...eaurora.org>, Rocky Liao <rjliao@...eaurora.org>, Marcel Holtmann <marcel@...tmann.org>, Johan Hedberg <johan.hedberg@...il.com>, Luiz Augusto von Dentz <luiz.dentz@...il.com>, Andy Gross <agross@...nel.org>, Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org, netdev@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org, linux-arm-msm@...r.kernel.org Subject: Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support 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@...rphone.com> > --- > 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), ...
Powered by blists - more mailing lists