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

["Peter Krystad" <pkrystad@codeaurora.org>]

Hi Andrei & Marcel,

> Hi Andrei,
> 
> > > > 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.

> So just split it into bredr_init and amp_init.
> 
> > > > -	if (test_bit(HCI_INIT, &hdev->flags))
> > > > +	if (hdev->dev_type == HCI_BREDR && test_bit(HCI_INIT, &hdev-
> >flags))
> > > >  		hci_setup(hdev);
> > > >  }
> > > This is ugly. Why don't we set HCI_INIT for AMP controllers as well?
> >
> > The purpose of the hunk is to avoid hci_setup for other controllers than
> > BR/EDR. Maybe later on we will come up with something like amp_setup for
> > AMP controllers.
> >
> > Maybe I have to create special patch for it?
> 
> I like to not see a temporary hack and just init the AMP controller.
> This needs to be done anyway. So lets just do it.
> 
> Regards
> 
> Marcel
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,

Peter.

--Peter Krystad
  Employee of Qualcomm Innovation Center, Inc.
  Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum