[PATCH 1/2] Bluetooth: hci open callback for hci UART transport driver

[PATCH 1/2] Bluetooth: hci open callback for hci UART transport driver

[Suraj Sumangala]

This patch provides option for hci transport driver protocol implementation
to have a callback for hci open.

Signed-off-by: Suraj Sumangala <suraj@atheros.com>
---
 drivers/bluetooth/hci_ldisc.c |    5 ++++-
 drivers/bluetooth/hci_uart.h  |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 998833d..5e02501 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -162,9 +162,12 @@ restart:
 /* Initialize device */
 static int hci_uart_open(struct hci_dev *hdev)
 {
+	struct hci_uart *hu  = (struct hci_uart *) hdev->driver_data;
+
 	BT_DBG("%s %p", hdev->name, hdev);
 
-	/* Nothing to do for UART driver */
+	if (hu->proto->hci_open)
+		hu->proto->hci_open(hu);
 
 	set_bit(HCI_RUNNING, &hdev->flags);
 
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 99fb352..d0198ec 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -51,6 +51,7 @@ struct hci_uart;
 struct hci_uart_proto {
 	unsigned int id;
 	int (*open)(struct hci_uart *hu);
+	int (*hci_open)(struct hci_uart *hu);
 	int (*close)(struct hci_uart *hu);
 	int (*flush)(struct hci_uart *hu);
 	int (*recv)(struct hci_uart *hu, void *data, int len);
-- 
1.7.0.4

[PATCH 2/2] Bluetooth: support to send power management enable during hci open

[Suraj Sumangala]

This patch enables HCI_UART_ATH3K transport driver to support
sending Vendor specific hci commands during hci open
to enable or disable power management feature.

Signed-off-by: Suraj Sumangala <suraj@atheros.com>
---
 drivers/bluetooth/hci_ath.c |   59 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index 6a160c1..d4b0653 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -49,6 +49,37 @@ struct ath_struct {
 	struct work_struct ctxtsw;
 };
 
+/* Form HCI command */
+static struct sk_buff *form_hci_cmd(struct hci_dev *hdev, __u16 opcode,
+						 __u32 plen, void *param)
+{
+	int len = HCI_COMMAND_HDR_SIZE + plen;
+	struct hci_command_hdr *hdr;
+	struct sk_buff *skb;
+
+	BT_DBG("%s opcode 0x%x plen %d", hdev->name, opcode, plen);
+
+	skb = bt_skb_alloc(len, GFP_ATOMIC);
+	if (!skb) {
+		BT_ERR("%s no memory for command", hdev->name);
+		return NULL;
+	}
+
+	hdr = (struct hci_command_hdr *) skb_put(skb, HCI_COMMAND_HDR_SIZE);
+	hdr->opcode = cpu_to_le16(opcode);
+	hdr->plen   = plen;
+
+	if (plen)
+		memcpy(skb_put(skb, plen), param, plen);
+
+	BT_DBG("skb len %d", skb->len);
+
+	bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
+	skb->dev = (void *) hdev;
+
+	return skb;
+}
+
 static int ath_wakeup_ar3k(struct tty_struct *tty)
 {
 	struct termios settings;
@@ -81,6 +112,26 @@ static int ath_wakeup_ar3k(struct tty_struct *tty)
 	return status;
 }
 
+#define HCI_OP_SLEEP_SET			0xFC04
+static void ath_hci_init_driver(struct hci_uart *hu)
+{
+	struct ath_struct *ath = hu->priv;
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+
+	if (!hdev)
+		return;
+
+	if (!ath)
+		return;
+
+	skb = form_hci_cmd(hdev, HCI_OP_SLEEP_SET, 1, &ath->cur_sleep);
+	if (!skb)
+		return;
+
+	skb_queue_head(&hdev->driver_init, skb);
+}
+
 static void ath_hci_uart_work(struct work_struct *work)
 {
 	int status;
@@ -126,6 +177,13 @@ static int ath_open(struct hci_uart *hu)
 	return 0;
 }
 
+static int ath_hci_open(struct hci_uart *hu)
+{
+	ath_hci_init_driver(hu);
+
+	return 0;
+}
+
 /* Flush protocol data */
 static int ath_flush(struct hci_uart *hu)
 {
@@ -210,6 +268,7 @@ static int ath_recv(struct hci_uart *hu, void *data, int count)
 static struct hci_uart_proto athp = {
 	.id = HCI_UART_ATH3K,
 	.open = ath_open,
+	.hci_open = ath_hci_open,
 	.close = ath_close,
 	.recv = ath_recv,
 	.enqueue = ath_enqueue,
-- 
1.7.0.4

Re: [PATCH 2/2] Bluetooth: support to send power management enable during hci open

[Pavan Savoy]

On Tue, Sep 21, 2010 at 8:33 AM, Suraj Sumangala <suraj@atheros.com> wrote:
> This patch enables HCI_UART_ATH3K transport driver to support
> sending Vendor specific hci commands during hci open
> to enable or disable power management feature.

Why? shouldn't this be done from the hciattach? like for the other
manufacturers?
If you want it to be sent before hci0 interface is exposed, send it
over ttyXX, you have your _init function and if you require it to be
sent after the hci0 is exposed - do it in the _post function.

> Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> ---
>  drivers/bluetooth/hci_ath.c |   59 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> index 6a160c1..d4b0653 100644
> --- a/drivers/bluetooth/hci_ath.c
> +++ b/drivers/bluetooth/hci_ath.c
> @@ -49,6 +49,37 @@ struct ath_struct {
>        struct work_struct ctxtsw;
>  };
>
> +/* Form HCI command */
> +static struct sk_buff *form_hci_cmd(struct hci_dev *hdev, __u16 opcode,
> +                                                __u32 plen, void *param)
> +{
> +       int len = HCI_COMMAND_HDR_SIZE + plen;
> +       struct hci_command_hdr *hdr;
> +       struct sk_buff *skb;
> +
> +       BT_DBG("%s opcode 0x%x plen %d", hdev->name, opcode, plen);
> +
> +       skb = bt_skb_alloc(len, GFP_ATOMIC);
> +       if (!skb) {
> +               BT_ERR("%s no memory for command", hdev->name);
> +               return NULL;
> +       }
> +
> +       hdr = (struct hci_command_hdr *) skb_put(skb, HCI_COMMAND_HDR_SIZE);
> +       hdr->opcode = cpu_to_le16(opcode);
> +       hdr->plen   = plen;
> +
> +       if (plen)
> +               memcpy(skb_put(skb, plen), param, plen);
> +
> +       BT_DBG("skb len %d", skb->len);
> +
> +       bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
> +       skb->dev = (void *) hdev;
> +
> +       return skb;
> +}
> +
>  static int ath_wakeup_ar3k(struct tty_struct *tty)
>  {
>        struct termios settings;
> @@ -81,6 +112,26 @@ static int ath_wakeup_ar3k(struct tty_struct *tty)
>        return status;
>  }
>
> +#define HCI_OP_SLEEP_SET                       0xFC04
> +static void ath_hci_init_driver(struct hci_uart *hu)
> +{
> +       struct ath_struct *ath = hu->priv;
> +       struct hci_dev *hdev = hu->hdev;
> +       struct sk_buff *skb;
> +
> +       if (!hdev)
> +               return;
> +
> +       if (!ath)
> +               return;
> +
> +       skb = form_hci_cmd(hdev, HCI_OP_SLEEP_SET, 1, &ath->cur_sleep);
> +       if (!skb)
> +               return;
> +
> +       skb_queue_head(&hdev->driver_init, skb);
> +}
> +
>  static void ath_hci_uart_work(struct work_struct *work)
>  {
>        int status;
> @@ -126,6 +177,13 @@ static int ath_open(struct hci_uart *hu)
>        return 0;
>  }
>
> +static int ath_hci_open(struct hci_uart *hu)
> +{
> +       ath_hci_init_driver(hu);
> +
> +       return 0;
> +}
> +
>  /* Flush protocol data */
>  static int ath_flush(struct hci_uart *hu)
>  {
> @@ -210,6 +268,7 @@ static int ath_recv(struct hci_uart *hu, void *data, int count)
>  static struct hci_uart_proto athp = {
>        .id = HCI_UART_ATH3K,
>        .open = ath_open,
> +       .hci_open = ath_hci_open,
>        .close = ath_close,
>        .recv = ath_recv,
>        .enqueue = ath_enqueue,
> --
> 1.7.0.4
>
> --
> 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
>

Re: [PATCH 2/2] Bluetooth: support to send power management enable during hci open

[Suraj Sumangala]

Hi Pavan,

On 9/22/2010 1:22 AM, Pavan Savoy wrote:
> On Tue, Sep 21, 2010 at 8:33 AM, Suraj Sumangala<suraj@atheros.com>  wrote:
>> This patch enables HCI_UART_ATH3K transport driver to support
>> sending Vendor specific hci commands during hci open
>> to enable or disable power management feature.
>
> Why? shouldn't this be done from the hciattach? like for the other
> manufacturers?
> If you want it to be sent before hci0 interface is exposed, send it
> over ttyXX, you have your _init function and if you require it to be
> sent after the hci0 is exposed - do it in the _post function.
>
We are already using the _init and _post of hciattach.

The mentioned feature will get disabled in the controller on receiving a 
HCI RESET command.

If the user does an HCI close, this feature will be disabled and we need 
to enable it again when the user opens the HCI device again.

I guess the "hdev->driver_init" queue is provided for that reason.

An hciattach is called only once but hci open/close can be done multiple 
times.

Regards
Suraj

Re: [PATCH 2/2] Bluetooth: support to send power management enable during hci open

[Pavan Savoy]

Suraj,

On Tue, Sep 21, 2010 at 11:01 PM, Suraj Sumangala <suraj@atheros.com> wrote:
> Hi Pavan,
>
> On 9/22/2010 1:22 AM, Pavan Savoy wrote:
>>
>> On Tue, Sep 21, 2010 at 8:33 AM, Suraj Sumangala<suraj@atheros.com>
>>  wrote:
>>>
>>> This patch enables HCI_UART_ATH3K transport driver to support
>>> sending Vendor specific hci commands during hci open
>>> to enable or disable power management feature.
>>
>> Why? shouldn't this be done from the hciattach? like for the other
>> manufacturers?
>> If you want it to be sent before hci0 interface is exposed, send it
>> over ttyXX, you have your _init function and if you require it to be
>> sent after the hci0 is exposed - do it in the _post function.
>>
> We are already using the _init and _post of hciattach.
>
> The mentioned feature will get disabled in the controller on receiving a HCI
> RESET command.
>
> If the user does an HCI close, this feature will be disabled and we need to
> enable it again when the user opens the HCI device again.
>
> I guess the "hdev->driver_init" queue is provided for that reason.
>
> An hciattach is called only once but hci open/close can be done multiple
> times.

Well this is debatable, I guess we had this discussion when our
manufacturer came up with PM feature like this - As to what is the
right BT on procedure?
Should hciattach be terminated when BT is Off or is it just a
hciconfig hci0 down - So we decided to get rid of hciattach way of
doing things.

Also HCI_QUIRK_NO_RESET allows you to not to have reset .. in certain
cases, I guess with this your chip's firmware is able to remember the
PM settings previously sent across?

> Regards
> Suraj
>

Re: [PATCH 2/2] Bluetooth: support to send power management enable during hci open

[Suraj Sumangala]

Hi Pavan,

On 9/23/2010 2:52 AM, Pavan Savoy wrote:
> Suraj,
>
> On Tue, Sep 21, 2010 at 11:01 PM, Suraj Sumangala<suraj@atheros.com>  wrote:
>> Hi Pavan,
>>
>> On 9/22/2010 1:22 AM, Pavan Savoy wrote:
>>>
>>> On Tue, Sep 21, 2010 at 8:33 AM, Suraj Sumangala<suraj@atheros.com>
>>>   wrote:
>>>>
>>>> This patch enables HCI_UART_ATH3K transport driver to support
>>>> sending Vendor specific hci commands during hci open
>>>> to enable or disable power management feature.
>>>
>>> Why? shouldn't this be done from the hciattach? like for the other
>>> manufacturers?
>>> If you want it to be sent before hci0 interface is exposed, send it
>>> over ttyXX, you have your _init function and if you require it to be
>>> sent after the hci0 is exposed - do it in the _post function.
>>>
>> We are already using the _init and _post of hciattach.
>>
>> The mentioned feature will get disabled in the controller on receiving a HCI
>> RESET command.
>>
>> If the user does an HCI close, this feature will be disabled and we need to
>> enable it again when the user opens the HCI device again.
>>
>> I guess the "hdev->driver_init" queue is provided for that reason.
>>
>> An hciattach is called only once but hci open/close can be done multiple
>> times.
>
> Well this is debatable, I guess we had this discussion when our
> manufacturer came up with PM feature like this - As to what is the
> right BT on procedure?
> Should hciattach be terminated when BT is Off or is it just a
> hciconfig hci0 down - So we decided to get rid of hciattach way of
> doing things.

Yes, it would work fine for me too on a customer platform. but my idea 
was to get it working on a generic x86 system running a normal distribution.

Moreover, if you looks at the "hdev->driver_init" queue, it looks like 
it is meant for exactly this requirement. Bluez already have code in 
hci_init_req() to take care of this scenario.


>
> Also HCI_QUIRK_NO_RESET allows you to not to have reset .. in certain
> cases, I guess with this your chip's firmware is able to remember the
> PM settings previously sent across?

HCI_QUIRK_NO_RESET only lets you not send a HCI reset during 
initialization. I have no issues there, I am using the _post callback in 
hciattach to configure PM.

I am doing the testing in Ubuntu 10.04 using the Blueman as my Bluetooth 
agent.

 From the UI if the user does a BT off. It sends an HCI reset command 
followed by ah HCI close. This is causing the firmware forget the PM 
parameters.
>
>> Regards
>> Suraj
>>

Regards
Suraj

Re: [PATCH 1/2] Bluetooth: hci open callback for hci UART transport driver

[Suraj Sumangala]

*ping*

On 9/21/2010 7:03 PM, Suraj Sumangala wrote:
> This patch provides option for hci transport driver protocol implementation
> to have a callback for hci open.
>
> Signed-off-by: Suraj Sumangala<suraj@atheros.com>
> ---
>   drivers/bluetooth/hci_ldisc.c |    5 ++++-
>   drivers/bluetooth/hci_uart.h  |    1 +
>   2 files changed, 5 insertions(+), 1 deletions(-)
>

Regards
Suraj

Re: [PATCH 1/2] Bluetooth: hci open callback for hci UART transport driver

[Suraj Sumangala]

Hi,

On 9/30/2010 2:01 PM, Suraj Sumangala wrote:
> *ping*
>
> On 9/21/2010 7:03 PM, Suraj Sumangala wrote:
>> This patch provides option for hci transport driver protocol implementation
>> to have a callback for hci open.
>>
>> Signed-off-by: Suraj Sumangala<suraj@atheros.com>
>> ---
>>    drivers/bluetooth/hci_ldisc.c |    5 ++++-
>>    drivers/bluetooth/hci_uart.h  |    1 +
>>    2 files changed, 5 insertions(+), 1 deletions(-)
>>
>
> Regards
> Suraj

Is there any comments on this patch?

Regards
Suraj

Re: [PATCH 1/2] Bluetooth: hci open callback for hci UART transport driver

[Marcel Holtmann]

Hi Suraj,

> This patch provides option for hci transport driver protocol implementation
> to have a callback for hci open.
> 
> Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> ---
>  drivers/bluetooth/hci_ldisc.c |    5 ++++-
>  drivers/bluetooth/hci_uart.h  |    1 +
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 998833d..5e02501 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -162,9 +162,12 @@ restart:
>  /* Initialize device */
>  static int hci_uart_open(struct hci_dev *hdev)
>  {
> +	struct hci_uart *hu  = (struct hci_uart *) hdev->driver_data;
> +
>  	BT_DBG("%s %p", hdev->name, hdev);
>  
> -	/* Nothing to do for UART driver */
> +	if (hu->proto->hci_open)
> +		hu->proto->hci_open(hu);
>  
>  	set_bit(HCI_RUNNING, &hdev->flags);
>  
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 99fb352..d0198ec 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -51,6 +51,7 @@ struct hci_uart;
>  struct hci_uart_proto {
>  	unsigned int id;
>  	int (*open)(struct hci_uart *hu);
> +	int (*hci_open)(struct hci_uart *hu);
>  	int (*close)(struct hci_uart *hu);
>  	int (*flush)(struct hci_uart *hu);
>  	int (*recv)(struct hci_uart *hu, void *data, int len);

I don't like this. It is not what any other driver is doing. For me this
is just hacking around something that would need to be solved for all
Bluetooth transport drivers. Just trying to solve this for UART based
drivers is not going to help.

So we talked about adding a setup stage to the HCI driver interface.
That is the way you should proceed if doing this in hciattach is not
enough for you.

Regards

Marcel