Return-Path: Subject: Re: [PATCHv4 1/5] Bluetooth: Define AMP controller statuses From: Marcel Holtmann To: Emeltchenko Andrei Cc: linux-bluetooth@vger.kernel.org Date: Wed, 16 Nov 2011 22:26:41 +0900 In-Reply-To: <20111116100438.GI30662@aemeltch-MOBL1> References: <1321366547-29462-1-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1321366547-29462-2-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1321422613.15441.513.camel@aeonflux> <20111116090325.GF30662@aemeltch-MOBL1> <1321434990.15441.526.camel@aeonflux> <20111116100438.GI30662@aemeltch-MOBL1> Content-Type: text/plain; charset="UTF-8" Message-ID: <1321450004.15441.546.camel@aeonflux> Mime-Version: 1.0 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Andrei, > > > > > AMP status codes copied from Bluez patch sent by Peter Krystad > > > > > . > > > > > > > > > > Signed-off-by: Andrei Emeltchenko > > > > > --- > > > > > include/net/bluetooth/hci.h | 9 +++++++++ > > > > > 1 files changed, 9 insertions(+), 0 deletions(-) > > > > > > > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > > > > index 139ce2a..e79ed67 100644 > > > > > --- a/include/net/bluetooth/hci.h > > > > > +++ b/include/net/bluetooth/hci.h > > > > > @@ -56,6 +56,15 @@ > > > > > #define HCI_BREDR 0x00 > > > > > #define HCI_AMP 0x01 > > > > > > > > > > +/* AMP controller status */ > > > > > +#define AMP_CTRL_POWERED_DOWN 0x00 > > > > > +#define AMP_CTRL_BLUETOOTH_ONLY 0x01 > > > > > +#define AMP_CTRL_NO_CAPACITY 0x02 > > > > > +#define AMP_CTRL_LOW_CAPACITY 0x03 > > > > > +#define AMP_CTRL_MEDIUM_CAPACITY 0x04 > > > > > +#define AMP_CTRL_HIGH_CAPACITY 0x05 > > > > > +#define AMP_CTRL_FULL_CAPACITY 0x06 > > > > > + > > > > > > > > is hci.h really the right place for these? It is not HCI specific > > > > per-se. It is A2MP detail. And as mentioned earlier, I do not believe we > > > > should do it like this. > > > > > > I believe that it is HCI device specific since hci_dev structure is > > > accountable for BR/EDR and AMP controllers and we currently keep > > > controller-specific information in hci_dev. > > > > > > Those defines indicate AMP controller status like powered or not. > > > > > > What would be the better place? > > > > > > include/net/bluetooth/hci_core.h > > > include/net/bluetooth/amp.h > > > include/net/bluetooth/a2mp.h > > > > > > > > > > I think we need to expose some sort of functionality that lets the AMP > > > > drivers handle this dynamically. > > > > > > This status and other AMP parameters would be normally returned when > > > "read local amp info" HCI command. > > > > if these information come from the AMP controller, then this is clearly > > an A2MP specific detail. There is no point in storing them in hci_dev at > > all. > > I thought that A2MP specific details are details related to A2MP > connection. Information about status of AMP controller does not depend on > A2MP connection. the only part using this information will be A2MP and it has to read it from the controller fresh almost every time. So yes, it belongs there and not inside HCI. At least that is how I read the AMP controller specification and how it is suppose to be used. > > I think these definition can stay local in net/bluetooth/a2mp.c for now. > > maybe a2mp.h then? As we use existing HCI command infrastructure to handle > AMP specific HCI commands we might need to source the header. As I said, leave it local for now. We can always move them around later. Regards Marcel