RE: [PATCH v3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

["Tim Jiang (QUIC)" <quic_tjiang@quicinc.com>]

Paul:
  Inline comments.
Regards.
Tim


-----Original Message-----
From: Paul Menzel <pmenzel@molgen.mpg.de> 
Sent: Wednesday, May 17, 2023 8:28 PM
To: Tim Jiang (QUIC) <quic_tjiang@quicinc.com>
Cc: marcel@holtmann.org; linux-kernel@vger.kernel.org; linux-bluetooth@vger.kernel.org; linux-arm-msm@vger.kernel.org; Balakrishna Godavarthi (QUIC) <quic_bgodavar@quicinc.com>; Hemant Gupta (QUIC) <quic_hemantg@quicinc.com>; mka@chromium.org
Subject: Re: [PATCH v3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

Dear Tim,


Am 17.05.23 um 04:46 schrieb Tim Jiang (QUIC):
> Paul :
>    Thanks for comments, please see inline comments.

Thank you for your reply. (It’d be great, if you used an email client, that can properly quote/cite like Mozilla Thunderbird.)

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Tuesday, May 16, 2023 7:00 PM

> Am 16.05.23 um 12:41 schrieb Tim Jiang:
>> This patch adds support for QCA2066 firmware patch and nvm downloading.
> 
> Could you please elaborate, what new features are needed for this as you add common functions?
> 
> Please document the datasheet.
> [Tim] no new feature, only support new chip qca2066 btfw downloading

As I wrote, you add common functions like `qca_read_fw_board_id()`, which were not required before. So please elaborate in the commit message.
[Tim] got it , will address it in v5 version

>> Signed-off-by: Tim Jiang <quic_tjiang@quicinc.com>
>> ---
>>    drivers/bluetooth/btqca.c   | 77 ++++++++++++++++++++++++++++++++++++-
>>    drivers/bluetooth/btqca.h   |  4 ++
>>    drivers/bluetooth/hci_qca.c |  8 +++-
>>    3 files changed, 87 insertions(+), 2 deletions(-)

[…]

>> @@ -574,6 +616,30 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>    }
>>    EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
>>    
>> +static void qca_generate_nvm_name(struct hci_dev *hdev, char *fwname,
>> +		   size_t max_size, struct qca_btsoc_version ver, u16 bid) {
>> +	u8 rom_ver = 0;
>> +	u32 soc_ver;
> 
> Any reason to fix the size of the variables?
> [Tim] sorry , I does not got your point

Why can’t you simply use `unsigned int` [1]?
[Tim] you can refer to function qca_uart_setup , which also use u32

[…]

>> diff --git a/drivers/bluetooth/hci_qca.c 
>> b/drivers/bluetooth/hci_qca.c index 1b064504b388..ec24ce451568 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1729,7 +1729,7 @@ static int qca_setup(struct hci_uart *hu)
>>    	bt_dev_info(hdev, "setting up %s",
>>    		qca_is_wcn399x(soc_type) ? "wcn399x" :
>>    		(soc_type == QCA_WCN6750) ? "wcn6750" :
>> -		(soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390");
>> +		(soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390/QCA2066");
>>    
>>    	qca->memdump_state = QCA_MEMDUMP_IDLE;
>>    
>> @@ -1874,6 +1874,11 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>>    	.num_vregs = 0,
>>    };
>>    
>> +static const struct qca_device_data qca_soc_data_qca2066 = {
>> +	.soc_type = QCA_QCA2066,
>> +	.num_vregs = 0,
>> +};
>> +
>>    static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>>    	.soc_type = QCA_WCN6750,
>>    	.vregs = (struct qca_vreg []) {
>> @@ -2364,6 +2369,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>>    	{ .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
>>    	{ .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750},
>>    	{ .compatible = "qcom,wcn6855-bt", .data = 
>> &qca_soc_data_wcn6855},
>> +	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
> 
> Sort it?
> [Tim] it have been sorted or please guide me how to sort it ?

Sort it lexicographically, that means, q goes before w.
[Tim] will address in v5 version.


>>    	{ /* sentinel */ }
>>    };
>>    MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);


Kind regards,

Paul