LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller
@ 2021-08-12  8:50 Zijun Hu
  2021-08-12 12:30 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Zijun Hu @ 2021-08-12  8:50 UTC (permalink / raw)
  To: marcel, johan.hedberg, luiz.dentz
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, bgodavar, c-hbandi,
	hemantg, mka, rjliao, zijuhu, tjiang

From: Tim Jiang <tjiang@codeaurora.org>

we have different factory to produce wcn6855 soc chip, so we should
use different nvm file with suffix to distinguish them.

Signed-off-by: Tim Jiang <tjiang@codeaurora.org>
---
 drivers/bluetooth/btusb.c | 60 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index b1a05bb9f4bf..d7b4e0f1c3e3 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -4013,6 +4013,9 @@ static int btusb_set_bdaddr_wcn6855(struct hci_dev *hdev,
 #define QCA_DFU_TIMEOUT		3000
 #define QCA_FLAG_MULTI_NVM      0x80
 
+#define WCN6855_2_0_RAM_VERSION_GF 0x400c1200
+#define WCN6855_2_1_RAM_VERSION_GF 0x400c1211
+
 struct qca_version {
 	__le32	rom_version;
 	__le32	patch_version;
@@ -4044,6 +4047,7 @@ static const struct qca_device_info qca_devices_table[] = {
 	{ 0x00000302, 28, 4, 16 }, /* Rome 3.2 */
 	{ 0x00130100, 40, 4, 16 }, /* WCN6855 1.0 */
 	{ 0x00130200, 40, 4, 16 }, /* WCN6855 2.0 */
+	{ 0x00130201, 40, 4, 16 }, /* WCN6855 2.1 */
 };
 
 static int btusb_qca_send_vendor_req(struct usb_device *udev, u8 request,
@@ -4198,6 +4202,39 @@ static int btusb_setup_qca_load_rampatch(struct hci_dev *hdev,
 	return err;
 }
 
+static int btusb_setup_qca_form_nvm_name(char **fwname,
+					int max_size,
+					struct qca_version *ver,
+					char *factory)
+{
+	/* if boardid equal 0, use default nvm without suffix */
+	switch (le16_to_cpu(ver->board_id)) {
+	case 0x0:
+		/* we add suffix factory to distinguish with different factory. */
+		if (factory != NULL) {
+			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%s.bin",
+				 le32_to_cpu(ver->rom_version),
+				 factory);
+		} else {
+			snprintf(*fwname, max_size, "qca/nvm_usb_%08x.bin",
+				 le32_to_cpu(ver->rom_version));
+		}
+		break;
+	default:
+		if (factory != NULL) {
+			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%s_%04x.bin",
+				le32_to_cpu(ver->rom_version),
+				factory,
+				le16_to_cpu(ver->board_id));
+		} else {
+			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%04x.bin",
+				le32_to_cpu(ver->rom_version),
+				le16_to_cpu(ver->board_id));
+		}
+		break;
+	}
+}
+
 static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
 				    struct qca_version *ver,
 				    const struct qca_device_info *info)
@@ -4206,19 +4243,24 @@ static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
 	char fwname[64];
 	int err;
 
-	if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
-		/* if boardid equal 0, use default nvm without surfix */
-		if (le16_to_cpu(ver->board_id) == 0x0) {
+	switch (ver->ram_version) {
+	case WCN6855_2_0_RAM_VERSION_GF:
+	case WCN6855_2_1_RAM_VERSION_GF:
+		if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
+			btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, "gf");
+		} else {
 			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
 				 le32_to_cpu(ver->rom_version));
+		}
+		break;
+	default:
+		if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
+			btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, NULL);
 		} else {
-			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x_%04x.bin",
-				le32_to_cpu(ver->rom_version),
-				le16_to_cpu(ver->board_id));
+			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
+				 le32_to_cpu(ver->rom_version));
 		}
-	} else {
-		snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
-			 le32_to_cpu(ver->rom_version));
+		break;
 	}
 
 	err = request_firmware(&fw, fwname, &hdev->dev);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller
  2021-08-12  8:50 [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller Zijun Hu
@ 2021-08-12 12:30 ` kernel test robot
  2021-08-18  7:37 ` tjiang
  2021-08-18 13:58 ` Matthias Kaehlcke
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-08-12 12:30 UTC (permalink / raw)
  To: Zijun Hu, marcel, johan.hedberg, luiz.dentz
  Cc: kbuild-all, linux-kernel, linux-bluetooth, linux-arm-msm,
	bgodavar, c-hbandi, hemantg, mka

[-- Attachment #1: Type: text/plain, Size: 4959 bytes --]

Hi Zijun,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on linus/master v5.14-rc5 next-20210812]
[cannot apply to bluetooth/master linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Zijun-Hu/Bluetooth-btusb-Add-support-different-nvm-to-distinguish-different-factory-for-WCN6855-controller/20210812-165229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: alpha-randconfig-s032-20210811 (attached as .config)
compiler: alpha-linux-gcc (GCC) 10.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-348-gf0e6938b-dirty
        # https://github.com/0day-ci/linux/commit/c9758b351006b29772f27d1e5e7fe904270f1356
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zijun-Hu/Bluetooth-btusb-Add-support-different-nvm-to-distinguish-different-factory-for-WCN6855-controller/20210812-165229
        git checkout c9758b351006b29772f27d1e5e7fe904270f1356
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/bluetooth/btusb.c: In function 'btusb_setup_qca_form_nvm_name':
   drivers/bluetooth/btusb.c:3371:1: error: no return statement in function returning non-void [-Werror=return-type]
    3371 | }
         | ^
   drivers/bluetooth/btusb.c: In function 'btusb_setup_qca_load_nvm':
>> drivers/bluetooth/btusb.c:3385:34: error: passing argument 1 of 'btusb_setup_qca_form_nvm_name' from incompatible pointer type [-Werror=incompatible-pointer-types]
    3385 |    btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, "gf");
         |                                  ^~~~~~~
         |                                  |
         |                                  char (*)[64]
   drivers/bluetooth/btusb.c:3340:49: note: expected 'char **' but argument is of type 'char (*)[64]'
    3340 | static int btusb_setup_qca_form_nvm_name(char **fwname,
         |                                          ~~~~~~~^~~~~~
   drivers/bluetooth/btusb.c:3393:34: error: passing argument 1 of 'btusb_setup_qca_form_nvm_name' from incompatible pointer type [-Werror=incompatible-pointer-types]
    3393 |    btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, NULL);
         |                                  ^~~~~~~
         |                                  |
         |                                  char (*)[64]
   drivers/bluetooth/btusb.c:3340:49: note: expected 'char **' but argument is of type 'char (*)[64]'
    3340 | static int btusb_setup_qca_form_nvm_name(char **fwname,
         |                                          ~~~~~~~^~~~~~
   cc1: some warnings being treated as errors


vim +/btusb_setup_qca_form_nvm_name +3385 drivers/bluetooth/btusb.c

  3372	
  3373	static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
  3374					    struct qca_version *ver,
  3375					    const struct qca_device_info *info)
  3376	{
  3377		const struct firmware *fw;
  3378		char fwname[64];
  3379		int err;
  3380	
  3381		switch (ver->ram_version) {
  3382		case WCN6855_2_0_RAM_VERSION_GF:
  3383		case WCN6855_2_1_RAM_VERSION_GF:
  3384			if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> 3385				btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, "gf");
  3386			} else {
  3387				snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
  3388					 le32_to_cpu(ver->rom_version));
  3389			}
  3390			break;
  3391		default:
  3392			if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
  3393				btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, NULL);
  3394			} else {
  3395				snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
  3396					 le32_to_cpu(ver->rom_version));
  3397			}
  3398			break;
  3399		}
  3400	
  3401		err = request_firmware(&fw, fwname, &hdev->dev);
  3402		if (err) {
  3403			bt_dev_err(hdev, "failed to request NVM file: %s (%d)",
  3404				   fwname, err);
  3405			return err;
  3406		}
  3407	
  3408		bt_dev_info(hdev, "using NVM file: %s", fwname);
  3409	
  3410		err = btusb_setup_qca_download_fw(hdev, fw, info->nvm_hdr);
  3411	
  3412		release_firmware(fw);
  3413	
  3414		return err;
  3415	}
  3416	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31730 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller
  2021-08-12  8:50 [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller Zijun Hu
  2021-08-12 12:30 ` kernel test robot
@ 2021-08-18  7:37 ` tjiang
  2021-08-19 14:50   ` Marcel Holtmann
  2021-08-18 13:58 ` Matthias Kaehlcke
  2 siblings, 1 reply; 5+ messages in thread
From: tjiang @ 2021-08-18  7:37 UTC (permalink / raw)
  To: Zijun Hu
  Cc: marcel, johan.hedberg, luiz.dentz, linux-kernel, linux-bluetooth,
	linux-arm-msm, bgodavar, c-hbandi, hemantg, mka, rjliao


marcel:
   could you help review this patch ? thank you.
tjiang

On 2021-08-12 16:50, Zijun Hu wrote:
> From: Tim Jiang <tjiang@codeaurora.org>
> 
> we have different factory to produce wcn6855 soc chip, so we should
> use different nvm file with suffix to distinguish them.
> 
> Signed-off-by: Tim Jiang <tjiang@codeaurora.org>
> ---
>  drivers/bluetooth/btusb.c | 60 
> ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b1a05bb9f4bf..d7b4e0f1c3e3 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4013,6 +4013,9 @@ static int btusb_set_bdaddr_wcn6855(struct 
> hci_dev *hdev,
>  #define QCA_DFU_TIMEOUT		3000
>  #define QCA_FLAG_MULTI_NVM      0x80
> 
> +#define WCN6855_2_0_RAM_VERSION_GF 0x400c1200
> +#define WCN6855_2_1_RAM_VERSION_GF 0x400c1211
> +
>  struct qca_version {
>  	__le32	rom_version;
>  	__le32	patch_version;
> @@ -4044,6 +4047,7 @@ static const struct qca_device_info
> qca_devices_table[] = {
>  	{ 0x00000302, 28, 4, 16 }, /* Rome 3.2 */
>  	{ 0x00130100, 40, 4, 16 }, /* WCN6855 1.0 */
>  	{ 0x00130200, 40, 4, 16 }, /* WCN6855 2.0 */
> +	{ 0x00130201, 40, 4, 16 }, /* WCN6855 2.1 */
>  };
> 
>  static int btusb_qca_send_vendor_req(struct usb_device *udev, u8 
> request,
> @@ -4198,6 +4202,39 @@ static int btusb_setup_qca_load_rampatch(struct
> hci_dev *hdev,
>  	return err;
>  }
> 
> +static int btusb_setup_qca_form_nvm_name(char **fwname,
> +					int max_size,
> +					struct qca_version *ver,
> +					char *factory)
> +{
> +	/* if boardid equal 0, use default nvm without suffix */
> +	switch (le16_to_cpu(ver->board_id)) {
> +	case 0x0:
> +		/* we add suffix factory to distinguish with different factory. */
> +		if (factory != NULL) {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%s.bin",
> +				 le32_to_cpu(ver->rom_version),
> +				 factory);
> +		} else {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x.bin",
> +				 le32_to_cpu(ver->rom_version));
> +		}
> +		break;
> +	default:
> +		if (factory != NULL) {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%s_%04x.bin",
> +				le32_to_cpu(ver->rom_version),
> +				factory,
> +				le16_to_cpu(ver->board_id));
> +		} else {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%04x.bin",
> +				le32_to_cpu(ver->rom_version),
> +				le16_to_cpu(ver->board_id));
> +		}
> +		break;
> +	}
> +}
> +
>  static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
>  				    struct qca_version *ver,
>  				    const struct qca_device_info *info)
> @@ -4206,19 +4243,24 @@ static int btusb_setup_qca_load_nvm(struct
> hci_dev *hdev,
>  	char fwname[64];
>  	int err;
> 
> -	if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> -		/* if boardid equal 0, use default nvm without surfix */
> -		if (le16_to_cpu(ver->board_id) == 0x0) {
> +	switch (ver->ram_version) {
> +	case WCN6855_2_0_RAM_VERSION_GF:
> +	case WCN6855_2_1_RAM_VERSION_GF:
> +		if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> +			btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, "gf");
> +		} else {
>  			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
>  				 le32_to_cpu(ver->rom_version));
> +		}
> +		break;
> +	default:
> +		if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> +			btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, NULL);
>  		} else {
> -			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x_%04x.bin",
> -				le32_to_cpu(ver->rom_version),
> -				le16_to_cpu(ver->board_id));
> +			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
> +				 le32_to_cpu(ver->rom_version));
>  		}
> -	} else {
> -		snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
> -			 le32_to_cpu(ver->rom_version));
> +		break;
>  	}
> 
>  	err = request_firmware(&fw, fwname, &hdev->dev);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller
  2021-08-12  8:50 [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller Zijun Hu
  2021-08-12 12:30 ` kernel test robot
  2021-08-18  7:37 ` tjiang
@ 2021-08-18 13:58 ` Matthias Kaehlcke
  2 siblings, 0 replies; 5+ messages in thread
From: Matthias Kaehlcke @ 2021-08-18 13:58 UTC (permalink / raw)
  To: Zijun Hu
  Cc: marcel, johan.hedberg, luiz.dentz, linux-kernel, linux-bluetooth,
	linux-arm-msm, bgodavar, c-hbandi, hemantg, rjliao, tjiang

On Thu, Aug 12, 2021 at 04:50:16PM +0800, Zijun Hu wrote:
> From: Tim Jiang <tjiang@codeaurora.org>
> 
> we have different factory to produce wcn6855 soc chip, so we should
> use different nvm file with suffix to distinguish them.

What exactly does factory mean in this context, different production
facilities? Could this be just treated as a variant of the chip instead
of introducing the concept of 'factory'?

> 
> Signed-off-by: Tim Jiang <tjiang@codeaurora.org>
> ---
>  drivers/bluetooth/btusb.c | 60 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b1a05bb9f4bf..d7b4e0f1c3e3 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4013,6 +4013,9 @@ static int btusb_set_bdaddr_wcn6855(struct hci_dev *hdev,
>  #define QCA_DFU_TIMEOUT		3000
>  #define QCA_FLAG_MULTI_NVM      0x80
>  
> +#define WCN6855_2_0_RAM_VERSION_GF 0x400c1200
> +#define WCN6855_2_1_RAM_VERSION_GF 0x400c1211
> +
>  struct qca_version {
>  	__le32	rom_version;
>  	__le32	patch_version;
> @@ -4044,6 +4047,7 @@ static const struct qca_device_info qca_devices_table[] = {
>  	{ 0x00000302, 28, 4, 16 }, /* Rome 3.2 */
>  	{ 0x00130100, 40, 4, 16 }, /* WCN6855 1.0 */
>  	{ 0x00130200, 40, 4, 16 }, /* WCN6855 2.0 */
> +	{ 0x00130201, 40, 4, 16 }, /* WCN6855 2.1 */
>  };
>  
>  static int btusb_qca_send_vendor_req(struct usb_device *udev, u8 request,
> @@ -4198,6 +4202,39 @@ static int btusb_setup_qca_load_rampatch(struct hci_dev *hdev,
>  	return err;
>  }
>  
> +static int btusb_setup_qca_form_nvm_name(char **fwname,

How about btusb_generate/get_qca_fw_name()?

> +					int max_size,
> +					struct qca_version *ver,
> +					char *factory)
> +{
> +	/* if boardid equal 0, use default nvm without suffix */
> +	switch (le16_to_cpu(ver->board_id)) {
> +	case 0x0:
> +		/* we add suffix factory to distinguish with different factory. */
> +		if (factory != NULL) {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%s.bin",
> +				 le32_to_cpu(ver->rom_version),
> +				 factory);
> +		} else {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x.bin",
> +				 le32_to_cpu(ver->rom_version));
> +		}

how about:

	snprintf(*fwname, max_size, "qca/nvm_usb_%08x%s.bin",
		 le32_to_cpu(ver->rom_version),
		 factory);

And you either pass the 'factory' including the underscore to the function, or
an empty string (potentially with a suitable define). That would eliminate the
need for the if/else construct here and below.

Or alternatively:

	snprintf(*fwname, max_size, "qca/nvm_usb_%08x%s%s.bin",
		 le32_to_cpu(ver->rom_version),
		 separator, factory);

With separator defaulting to an empty string and being assigned to '_' when
'factory' is set.

> +		break;
> +	default:
> +		if (factory != NULL) {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%s_%04x.bin",
> +				le32_to_cpu(ver->rom_version),
> +				factory,
> +				le16_to_cpu(ver->board_id));
> +		} else {
> +			snprintf(*fwname, max_size, "qca/nvm_usb_%08x_%04x.bin",
> +				le32_to_cpu(ver->rom_version),
> +				le16_to_cpu(ver->board_id));
> +		}
> +		break;
> +	}
> +}

In case you keep the if/else constructs you should probably use local
variables to do the conversion of rom version and board id from LE to CPU
format once, instead of doing it 4 times (in terms of code, not execution).

> +
>  static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
>  				    struct qca_version *ver,
>  				    const struct qca_device_info *info)
> @@ -4206,19 +4243,24 @@ static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
>  	char fwname[64];
>  	int err;
>  
> -	if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> -		/* if boardid equal 0, use default nvm without surfix */
> -		if (le16_to_cpu(ver->board_id) == 0x0) {
> +	switch (ver->ram_version) {
> +	case WCN6855_2_0_RAM_VERSION_GF:
> +	case WCN6855_2_1_RAM_VERSION_GF:
> +		if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> +			btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, "gf");
> +		} else {
>  			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
>  				 le32_to_cpu(ver->rom_version));
> +		}

The addition of a function to generate the firmware names makes sense IMO if
that gets more complex, to separate it from the actual firmware loading. However
it seems odd to only outsource part of it. Wouldn't it make more sense to hide
the entire complexity in btusb_setup_qca_form_nvm_name()?

> +		break;
> +	default:
> +		if (((ver->flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> +			btusb_setup_qca_form_nvm_name(&fwname, sizeof(fwname), ver, NULL);
>  		} else {
> -			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x_%04x.bin",
> -				le32_to_cpu(ver->rom_version),
> -				le16_to_cpu(ver->board_id));
> +			snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
> +				 le32_to_cpu(ver->rom_version));
>  		}
> -	} else {
> -		snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
> -			 le32_to_cpu(ver->rom_version));
> +		break;
>  	}
>  
>  	err = request_firmware(&fw, fwname, &hdev->dev);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller
  2021-08-18  7:37 ` tjiang
@ 2021-08-19 14:50   ` Marcel Holtmann
  0 siblings, 0 replies; 5+ messages in thread
From: Marcel Holtmann @ 2021-08-19 14:50 UTC (permalink / raw)
  To: tjiang
  Cc: Zijun Hu, Johan Hedberg, Luiz Augusto von Dentz, open list,
	open list:BLUETOOTH SUBSYSTEM, MSM, Balakrishna Godavarthi,
	c-hbandi, Hemantg, Matthias Kaehlcke, rjliao

Hi Tim,

>  could you help review this patch ? thank you.
> tjiang

if the kernel test robot throws an error, I am not even looking at a patch. You need to fix these first.

Regards

Marcel


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-19 14:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12  8:50 [PATCH v3] Bluetooth: btusb: Add support different nvm to distinguish different factory for WCN6855 controller Zijun Hu
2021-08-12 12:30 ` kernel test robot
2021-08-18  7:37 ` tjiang
2021-08-19 14:50   ` Marcel Holtmann
2021-08-18 13:58 ` Matthias Kaehlcke

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).