Paul:
Inline comments.
Regards.
Tim
-----Original Message-----
From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
Sent: Wednesday, May 17, 2023 8:28 PM
To: Tim Jiang (QUIC) <quic_tjiang@xxxxxxxxxxx>
Cc: marcel@xxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-bluetooth@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; Balakrishna Godavarthi (QUIC) <quic_bgodavar@xxxxxxxxxxx>; Hemant Gupta (QUIC) <quic_hemantg@xxxxxxxxxxx>; mka@xxxxxxxxxxxx
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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxx>
>> ---
>> 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
