mbox series

[v13,0/2] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

Message ID 20230727044011.965205-1-quic_tjiang@quicinc.com
Headers show
Series Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066 | expand

Message

Tim Jiang July 27, 2023, 4:40 a.m. UTC 
This series adds support for qualcomm bluetooth soc qca2066

Changes in v13
 - change the subject name for patch 1/2
 - solve review comments for patch 2/2

Changes in v12
 - fix compile error issue for patch 1/2

Changes in v11
 - reverse two patches order

Changes in v10
 - break out btsoc type print into seperate patch

Changes in v2-v9
 - solve review comments for code style and commit message context


Tim Jiang (2):
  Bluetooth: hci_qca: adjust qca btsoc type print expression
  Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

 drivers/bluetooth/btqca.c   | 76 ++++++++++++++++++++++++++++++++++++-
 drivers/bluetooth/btqca.h   |  4 +-
 drivers/bluetooth/hci_qca.c | 42 ++++++++++++++++++--
 3 files changed, 115 insertions(+), 7 deletions(-)

Comments

Johan Hovold July 27, 2023, 7:25 a.m. UTC | #1 
On Thu, Jul 27, 2023 at 12:40:09PM +0800, Tim Jiang wrote:
> This series adds support for qualcomm bluetooth soc qca2066
> 
> Changes in v13
>  - change the subject name for patch 1/2
>  - solve review comments for patch 2/2

Again, this is not specific enough and essentially only explains why you
changed something, but doesn't say what you changed.

You also again ignored some of my review comments without even
explaining why.

Seriously, you Qualcomm engineers really need to get your act together
and stop wasting other people's time.

Johan
Johan Hovold July 27, 2023, 7:50 a.m. UTC | #2 
On Thu, Jul 27, 2023 at 03:34:53PM +0800, Tim Jiang wrote:
> On 7/27/23 15:27, Johan Hovold wrote:
> > On Thu, Jul 27, 2023 at 12:40:10PM +0800, Tim Jiang wrote:

> >> @@ -1762,10 +1763,32 @@ static int qca_setup(struct hci_uart *hu)
> >>   	 */
> >>   	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> >>   
> >> -	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");
> >> +	switch (soc_type) {
> >> +	case QCA_AR3002:
> >> +		soc_name = "ar300x";
> >> +		break;
> >> +	case QCA_ROME:
> >> +		soc_name = "ROME";
> >> +		break;
> >> +	case QCA_QCA6390:
> >> +		soc_name = "QCA6390";
> >> +		break;
> >> +	case QCA_WCN3990:
> >> +	case QCA_WCN3991:
> >> +	case QCA_WCN3998:
> >> +		soc_name = "wcn399x";
> >> +		break;
> >> +	case QCA_WCN6750:
> >> +		soc_name = "wcn6750";
> >> +		break;
> >> +	case QCA_WCN6855:
> >> +		soc_name = "wcn6855";
> >> +		break;
> > I still think the above should be sorted (alphabetically) as maintaining
> > these lists otherwise soon becomes harder than it should be. And similar
> > throughout the driver.

> [Tim] Hi Johan: I think we no need to sort it, we only add the new btsoc 
> name following the older one, for example , ar300x is the oldest , ROME 
> is new than ar300x, actually qca2066 is newer version chip than qca6390, 
> so I does not think we need to sort it.

Possibly, but generally this becomes hard to maintain and eventually
someone will need to sort these entries anyway. Therefore it's generally
a good idea to just do so from the start.

But it was good that you replied so that we know that this comment was
not just missed or ignored.

> >> +	default:
> >> +		soc_name = "unknown soc";
> >> +		break;
> >> +	}
> >> +	bt_dev_info(hdev, "setting up %s", soc_name);

Johan