From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E7EDBC77B7F for ; Fri, 12 May 2023 11:14:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241037AbjELLOZ (ORCPT ); Fri, 12 May 2023 07:14:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39754 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232174AbjELLOY (ORCPT ); Fri, 12 May 2023 07:14:24 -0400 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8F31E4C1F for ; Fri, 12 May 2023 04:14:21 -0700 (PDT) Received: by mail-ed1-x52a.google.com with SMTP id 4fb4d7f45d1cf-50bc0117683so17328643a12.1 for ; Fri, 12 May 2023 04:14:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fairphone.com; s=fair; t=1683890060; x=1686482060; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ubvb9p88+qXd6VOrp401GthxX4aOEOtnz4kgi/kUzHM=; b=0DxQHN51nTkhDMihJi78b3ohzJMoM2rXRuga1g6tw0P6qNiVS4sVaDavvwMIT5/iJ0 Gyd4AtWdLZ5WJxMYmdxWl1sgVyeT/fA1AVb4pnZesPs4GAjkzKCCdzj13wDF3ZGCLXgP /cwdnqPJKz2mPmZYNAhyABgUXcoDYYThXQVYyJUox8JCl4sX4QDb29r863JZZN2x5CeK RKkhhEXl7XohTbmdb9Cms9xaWPK4UYp4HK4gfhE8VTjVlxxug2tdQhjIM10LhFv5XJkL XI+ANWqOR1T7qPIYvtmfC7JKuEzDCWvGXQ5LA0LBl7zQmc0N7GhgzmlhRA3//yPTDNA3 znnw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683890060; x=1686482060; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=ubvb9p88+qXd6VOrp401GthxX4aOEOtnz4kgi/kUzHM=; b=SBgUwEQdBHJtxSo0jAlyiw9x1Z55R1Y0T30rjLBo87s/yjcX9nowuTr/DUSAeh4vEY R+mHqj1vkNYt63g8yz0gis8Lxak0K1urnOkw8VPCS232Q0MsyYHMaSFLf/AV4uoiXfkm IedHKouVxHysg09eqOwm3f+FNVrbbDbJpEJwDBQP/51yd9ZVKuu5nWpl7hJMFBkJQBaR vKNpTyLhplLphBIJQdeIqxOAWtRiCumxuKNpFxDs7ob37MvFXHdgVHj7kRVJDt314C72 tWYElANo+c+r+7For8QPUSKC+fXjWz/q2q7zYFPEB4pWe21K+Qibuc2TLoyGHt5Usl7j yVqw== X-Gm-Message-State: AC+VfDxNuB2RCOBvQu5GDKRczi56Xcwtj53WR2ISewlcRn8F+FgPK6Mr +CAO6qQbY8n4Sb7rPxegRUwxTQ== X-Google-Smtp-Source: ACHHUZ52Czgk88thyOAzAK6r5gc3rGC6RgqVtFaULcxPliXgWW8mW3g/7ugZKJeeCj8A5U/8Lh8yPw== X-Received: by 2002:a17:907:a0e:b0:94e:e6b9:fef2 with SMTP id bb14-20020a1709070a0e00b0094ee6b9fef2mr20395577ejc.67.1683890060020; Fri, 12 May 2023 04:14:20 -0700 (PDT) Received: from localhost (144-178-202-138.static.ef-service.nl. [144.178.202.138]) by smtp.gmail.com with ESMTPSA id l21-20020a1709062a9500b00969f44bbef3sm4734097eje.11.2023.05.12.04.14.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 12 May 2023 04:14:19 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 12 May 2023 13:14:18 +0200 Message-Id: Cc: "David S. Miller" , "Eric Dumazet" , "Jakub Kicinski" , "Paolo Abeni" , "Rob Herring" , "Krzysztof Kozlowski" , "Balakrishna Godavarthi" , "Rocky Liao" , "Marcel Holtmann" , "Johan Hedberg" , "Luiz Augusto von Dentz" , "Andy Gross" , "Bjorn Andersson" , "Konrad Dybcio" , <~postmarketos/upstreaming@lists.sr.ht>, , , , , , Subject: Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support From: "Luca Weiss" To: "Simon Horman" X-Mailer: aerc 0.15.1 References: <20230421-fp4-bluetooth-v1-0-0430e3a7e0a2@fairphone.com> <20230421-fp4-bluetooth-v1-2-0430e3a7e0a2@fairphone.com> In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Hi Simon, On Mon May 1, 2023 at 3:11 PM CEST, Simon Horman wrote: > 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. > >=20 > > 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 > >=20 > > Signed-off-by: Luca Weiss > > --- > > drivers/bluetooth/btqca.c | 13 +++++++++++-- > > drivers/bluetooth/btqca.h | 12 ++++++++++-- > > drivers/bluetooth/hci_qca.c | 12 ++++++++++++ > > 3 files changed, 33 insertions(+), 4 deletions(-) > >=20 > > 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 =3D ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f)= ; > > + if (soc_type =3D=3D QCA_WCN3988) > > + rom_ver =3D ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f= ); > > + else > > + rom_ver =3D ((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 li= ttle. > > * 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 =3D soc_type =3D=3D QCA_WCN3988 ? 5 : 4; > rom_ver =3D ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f); > > Using some helpers such as GENMASK and FIELD_PREP might also be nice. While I'm not opposed to the idea, I'm not sure it's worth making beautiful macros for this since - to my eyes - how the mapping of soc_ver to firmware name works is rather obscure since the sources from Qualcomm just have a static lookup table of soc_ver to firmware name so doing this dynamically like here is different. And I haven't looked at other chips that are covered there to see if there's a pattern to this, for the most part it seems the original formula works for most chips and the one I added works for WCN3988 (and the other "APACHE" chips, whatever they are). If a third way is added then I would say for sure this line should be made nicer but for now I think it's easier to keep this as I sent it because we don't know what the future will hold. > > > =20 > > if (soc_type =3D=3D QCA_WCN6750) > > qca_send_patch_config_cmd(hdev); > > =20 > > /* Download rampatch file */ > > config.type =3D TLV_TYPE_PATCH; > > - if (qca_is_wcn399x(soc_type)) { > > + if (soc_type =3D=3D 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 =3D=3D QCA_QCA6390) { > > @@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t ba= udrate, > > if (firmware_name) > > snprintf(config.fwname, sizeof(config.fwname), > > "qca/%s", firmware_name); > > + else if (soc_type =3D=3D 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 =3D=3D QCA_WCN3991_SOC_ID) { > > Not strictly related to this patch, but while reviewing this I noticed th= at > 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? Good catch, as you've seen I sent a patch separately to fix that. :) Regards Luca > > > snprintf(config.fwname, sizeof(config.fwname), > > ...