Re: [RFC 3/3] Bluetooth: Add AMP initialization

Hi Andrei,

quoted

quoted

quoted

quoted

quoted

quoted

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d1eef7c..290acb2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c

@@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev,

unsigned long opt)

quoted

quoted

quoted

 			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
 	}

-	/* Read Local Supported Features */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
-
 	/* Read Local Version */
 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);

 	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
 	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);

-	/* Read BD Address */
-	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
+	switch (hdev->dev_type) {
+	case HCI_BREDR:

stop right here and first split this out into proper helper functions.
Making this part longer and more convoluted is not going to work well in
the long run.

Would following functions work:

__hci_common_init
__hci_bredr_init
__hci_amp_init

I would not bother with common init. I think there is nothing really
common between AMP and BR/EDR/LE controllers.

The commands quoted above are the common part of init between BR-EDR and AMP, plus 
there will be a few additional common commands when block-based flow control is added.
Makes sense to me to split the controller-specific sequences into two helper funcs 
called at this switch statement.

with our current approach on handling this, I rather copy hci_send_cmd
than making this code more complicated.

However since we are talking about controller init. Can someone please
first propose a list of how we init AMP vs BR/EDR/LE controllers instead
of just dumping code here. I think it needs some clear strategy on how
to do it.

AMP controller is initialized following way in Code Aurora code:

Common part:

<------8<--------------------------------------------------
|  /* Mandatory initialization */
|
|  /* Reset */
|  if (!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
|          set_bit(HCI_RESET, &hdev->flags);
|          hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
|  }

this check for QUIRK_NO_RESET is pointless. That is just there for old
Bluetooth 1.0b controllers. Or broken hardware. Let's assume the AMP
controllers are fine and just issue OP_RESET.

|  /* Read Local Version */
|  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
|
|  /* Read HCI Flow Control Mode */
|  hci_send_cmd(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, 0, NULL);
|
|  /* Read Buffer Size (ACL mtu, max pkt, etc.) */
|  hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
|
|  /* Read Data Block Size (ACL mtu, max pkt, etc.) */
|  hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
<------8<------------------------------------------------------

This is not a generic sequence. We will not be reading flow control or
data block size commands. It will heavily fail on older controllers.

Regards

Marcel