linux-kernel - Re: [PATCH v3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <506df7b0-6ffb-c829-0c82-89a52f0b0892@molgen.mpg.de>
Date:   Wed, 17 May 2023 14:27:45 +0200
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     Tim Jiang <quic_tjiang@...cinc.com>
Cc:     marcel@...tmann.org, linux-kernel@...r.kernel.org,
        linux-bluetooth@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Balakrishna Godavarthi <quic_bgodavar@...cinc.com>,
        Hemant Gupta <quic_hemantg@...cinc.com>, mka@...omium.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@...gen.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.

>> Signed-off-by: Tim Jiang <quic_tjiang@...cinc.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]?

[…]

>> 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.

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


Kind regards,

Paul