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

['Emeltchenko Andrei' <Andrei.Emeltchenko.news@gmail.com>]

Hi Marcel,

On Fri, Nov 18, 2011 at 06:14:13AM +0100, Marcel Holtmann wrote:
> Hi Peter,
> 
> > > > > > 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)
> > > > > >  			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