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

Hi Marcel,

On Fri, Nov 18, 2011 at 06:14:13AM +0100, Marcel Holtmann wrote:

Hi Peter,

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);
|  }
|
|  /* 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<------------------------------------------------------

AMP - specific part:

<------8<---------------------------------------------------
|  /* AMP initialization */
|  /* Connection accept timeout ~5 secs */
|  param = cpu_to_le16(0x1f40);
|  hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
|
|  /* Read AMP Info */
|  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
<------8<---------------------------------------------------

Source:
git://codeaurora.org/kernel/msm.git

Best regards 
Andrei Emeltchenko