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

[Paul Menzel]

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.

> > 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]?

[…]

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