[PATCH 1/5] Bluetooth: hci_intel: Retrieve host-wake IRQ

[PATCH 1/5] Bluetooth: hci_intel: Retrieve host-wake IRQ

[Loic Poulain]

An IRQ can be retrieved from the pdev resources. This irq will be used
in case of LPM suspend mode to wake-up the host and resume the link.
This resource can be declared as a GPIO-Interrupt which requires to be
converted into IRQ.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_intel.c | 45 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 46426ff..934b5a7 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -31,6 +31,7 @@
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/acpi.h>
+#include <linux/interrupt.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -48,6 +49,7 @@ struct intel_device {
 	struct list_head list;
 	struct platform_device *pdev;
 	struct gpio_desc *reset;
+	int irq;
 };
 
 static LIST_HEAD(intel_device_list);
@@ -789,6 +791,15 @@ static const struct hci_uart_proto intel_proto = {
 	.dequeue	= intel_dequeue,
 };
 
+static irqreturn_t intel_irq(int irq, void *dev_id)
+{
+	struct intel_device *idev = dev_id;
+
+	dev_info(&idev->pdev->dev, "hci_intel irq\n");
+
+	return IRQ_HANDLED;
+}
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id intel_acpi_match[] = {
 	{ "INT33E1", 0 },
@@ -816,6 +827,7 @@ static int intel_acpi_probe(struct intel_device *idev)
 static int intel_probe(struct platform_device *pdev)
 {
 	struct intel_device *idev;
+	int err;
 
 	idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
 	if (!idev)
@@ -838,6 +850,36 @@ static int intel_probe(struct platform_device *pdev)
 		return PTR_ERR(idev->reset);
 	}
 
+	idev->irq = platform_get_irq(pdev, 0);
+	if (idev->irq < 0) {
+		struct gpio_desc *host_wake;
+
+		dev_err(&pdev->dev, "No IRQ, falling back to gpio-irq\n");
+
+		host_wake = devm_gpiod_get_optional(&pdev->dev, "host-wake",
+						    GPIOD_IN);
+		if (IS_ERR(host_wake)) {
+			dev_err(&pdev->dev, "Unable to retrieve IRQ\n");
+			goto no_irq;
+		}
+
+		idev->irq = gpiod_to_irq(host_wake);
+		if (idev->irq < 0) {
+			dev_err(&pdev->dev, "No corresponding irq for gpio\n");
+			goto no_irq;
+		}
+	}
+
+	err = devm_request_threaded_irq(&pdev->dev, idev->irq, NULL, intel_irq,
+					IRQF_ONESHOT, "bt-host-wake", idev);
+	if (err) {
+		dev_err(&pdev->dev, "unable to allocate irq\n");
+		return err;
+	}
+
+	device_init_wakeup(&pdev->dev, true);
+
+no_irq:
 	platform_set_drvdata(pdev, idev);
 
 	/* Place this instance on the device list */
@@ -845,7 +887,8 @@ static int intel_probe(struct platform_device *pdev)
 	list_add_tail(&idev->list, &intel_device_list);
 	spin_unlock(&intel_device_list_lock);
 
-	dev_info(&pdev->dev, "registered.\n");
+	dev_info(&pdev->dev, "registered, gpio(%d)/irq(%d).\n",
+		 desc_to_gpio(idev->reset), idev->irq);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 2/5] Bluetooth: hci_intel: Add intel_device_get function

[Loic Poulain]

Move intel_device searching procedure in a standalone function since it
will be used from different places in the driver.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_intel.c | 49 +++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 934b5a7..26a1314 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -93,6 +93,24 @@ static u8 intel_convert_speed(unsigned int speed)
 	}
 }
 
+static struct intel_device *intel_device_get(struct hci_uart *hu)
+{
+	struct list_head *p;
+
+	list_for_each(p, &intel_device_list) {
+		struct intel_device *idev = list_entry(p, struct intel_device,
+						       list);
+
+		/* tty device and pdev device should share the same parent
+		 * which is the UART port.
+		 */
+		if (hu->tty->dev->parent == idev->pdev->dev.parent)
+			return idev;
+	}
+
+	return NULL;
+}
+
 static int intel_wait_booting(struct hci_uart *hu)
 {
 	struct intel_data *intel = hu->priv;
@@ -117,32 +135,27 @@ static int intel_wait_booting(struct hci_uart *hu)
 
 static int intel_set_power(struct hci_uart *hu, bool powered)
 {
-	struct list_head *p;
-	int err = -ENODEV;
+	struct intel_device *idev;
+	int err = 0;
 
 	spin_lock(&intel_device_list_lock);
 
-	list_for_each(p, &intel_device_list) {
-		struct intel_device *idev = list_entry(p, struct intel_device,
-						       list);
-
-		/* tty device and pdev device should share the same parent
-		 * which is the UART port.
-		 */
-		if (hu->tty->dev->parent != idev->pdev->dev.parent)
-			continue;
+	idev = intel_device_get(hu);
+	if (!idev)
+		err = -ENODEV;
+		goto done;
 
-		if (!idev->reset) {
-			err = -ENOTSUPP;
-			break;
-		}
+	if (!idev->reset) {
+		err = -ENOTSUPP;
+		goto done;
+	}
 
-		BT_INFO("hu %p, Switching compatible pm device (%s) to %u",
+	BT_INFO("hu %p, Switching compatible pm device (%s) to %u",
 			hu, dev_name(&idev->pdev->dev), powered);
 
-		gpiod_set_value(idev->reset, powered);
-	}
+	gpiod_set_value(idev->reset, powered);
 
+done:
 	spin_unlock(&intel_device_list_lock);
 
 	return err;
-- 
1.9.1

Re: [PATCH 2/5] Bluetooth: hci_intel: Add intel_device_get function

[Marcel Holtmann]

Hi Loic,

> Move intel_device searching procedure in a standalone function since it
> will be used from different places in the driver.
> 
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 49 +++++++++++++++++++++++++++----------------
> 1 file changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index 934b5a7..26a1314 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -93,6 +93,24 @@ static u8 intel_convert_speed(unsigned int speed)
> 	}
> }
> 
> +static struct intel_device *intel_device_get(struct hci_uart *hu)
> +{
> +	struct list_head *p;
> +
> +	list_for_each(p, &intel_device_list) {
> +		struct intel_device *idev = list_entry(p, struct intel_device,
> +						       list);
> +
> +		/* tty device and pdev device should share the same parent
> +		 * which is the UART port.
> +		 */
> +		if (hu->tty->dev->parent == idev->pdev->dev.parent)
> +			return idev;
> +	}
> +
> +	return NULL;
> +}
> +

lets see how many times this actually needs to be used. I really want to keep this rather simple.

Regards

Marcel

[PATCH 3/5] Bluetooth: hci_intel: Introduce LPM support

[Loic Poulain]

Enable controller Low-Power-Mode if we have a pdev to manage host
wake-up. Once LPM is enabled, controller notifies its TX status via
a vendor specific packet (tx_idle/tx_active).
tx_active means that there is more data upcoming from controller.
tx_idle means that controller can be put in suspended state.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_intel.c | 76 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 26a1314..f855515 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -44,6 +44,25 @@
 #define STATE_FIRMWARE_LOADED	2
 #define STATE_FIRMWARE_FAILED	3
 #define STATE_BOOTING		4
+#define STATE_TX_ACTIVE		5
+
+#define HCI_LPM_PKT 0xf1
+#define HCI_LPM_MAX_SIZE 10
+#define HCI_LPM_HDR_SIZE HCI_EVENT_HDR_SIZE
+#define H4_RECV_LPM \
+	.type = HCI_LPM_PKT, \
+	.hlen = HCI_LPM_HDR_SIZE, \
+	.loff = 1, \
+	.lsize = 1, \
+	.maxlen = HCI_LPM_MAX_SIZE
+
+#define LPM_OP_TX_NOTIFY 0x00
+
+struct hci_lpm_pkt {
+	__u8 opcode;
+	__u8 dlen;
+	__u8 data[0];
+} __packed;
 
 struct intel_device {
 	struct list_head list;
@@ -141,9 +160,10 @@ static int intel_set_power(struct hci_uart *hu, bool powered)
 	spin_lock(&intel_device_list_lock);
 
 	idev = intel_device_get(hu);
-	if (!idev)
+	if (!idev) {
 		err = -ENODEV;
 		goto done;
+	}
 
 	if (!idev->reset) {
 		err = -ENOTSUPP;
@@ -299,7 +319,9 @@ static int intel_setup(struct hci_uart *hu)
 {
 	static const u8 reset_param[] = { 0x00, 0x01, 0x00, 0x01,
 					  0x00, 0x08, 0x04, 0x00 };
+	static const u8 lpm_param[] = {0x03, 0x07, 0x01, 0x0b};
 	struct intel_data *intel = hu->priv;
+	struct intel_device *idev;
 	struct hci_dev *hdev = hu->hdev;
 	struct sk_buff *skb;
 	struct intel_version *ver;
@@ -663,6 +685,25 @@ done:
 
 	BT_INFO("%s: Device booted in %llu usecs", hdev->name, duration);
 
+	spin_lock(&intel_device_list_lock);
+	idev = intel_device_get(hu);
+	if (!idev || !device_may_wakeup(&idev->pdev->dev)) {
+		spin_unlock(&intel_device_list_lock);
+		goto no_lpm;
+	}
+	spin_unlock(&intel_device_list_lock);
+
+	BT_INFO("%s: Enabling LPM", hdev->name);
+
+	skb = __hci_cmd_sync(hdev, 0xfc8b, sizeof(lpm_param), lpm_param,
+			     HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: Failed to enable LPM", hdev->name);
+		goto no_lpm;
+	}
+	kfree_skb(skb);
+
+no_lpm:
 	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_CMD_TIMEOUT);
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
@@ -723,10 +764,43 @@ recv:
 	return hci_recv_frame(hdev, skb);
 }
 
+static void intel_recv_lpm_notification(struct hci_dev *hdev, int value)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct intel_data *intel = hu->priv;
+
+	BT_DBG("%s: TX idle notification (%d)", hdev->name, value);
+
+	if (value)
+		set_bit(STATE_TX_ACTIVE, &intel->flags);
+	else
+		clear_bit(STATE_TX_ACTIVE, &intel->flags);
+}
+
+static int intel_recv_lpm(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_lpm_pkt *lpm = (void *)skb->data;
+
+	switch (lpm->opcode) {
+	case LPM_OP_TX_NOTIFY:
+		if (lpm->dlen)
+			intel_recv_lpm_notification(hdev, lpm->data[0]);
+		break;
+	default:
+		BT_ERR("%s: unknown LPM opcode (%02x)", hdev->name,
+		       lpm->opcode);
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
 static const struct h4_recv_pkt intel_recv_pkts[] = {
 	{ H4_RECV_ACL,   .recv = hci_recv_frame },
 	{ H4_RECV_SCO,   .recv = hci_recv_frame },
 	{ H4_RECV_EVENT, .recv = intel_recv_event },
+	{ H4_RECV_LPM,   .recv = intel_recv_lpm },
 };
 
 static int intel_recv(struct hci_uart *hu, const void *data, int count)
-- 
1.9.1

Re: [PATCH 3/5] Bluetooth: hci_intel: Introduce LPM support

[Marcel Holtmann]

Hi Loic,

> Enable controller Low-Power-Mode if we have a pdev to manage host
> wake-up. Once LPM is enabled, controller notifies its TX status via
> a vendor specific packet (tx_idle/tx_active).
> tx_active means that there is more data upcoming from controller.
> tx_idle means that controller can be put in suspended state.
> 
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 76 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index 26a1314..f855515 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -44,6 +44,25 @@
> #define STATE_FIRMWARE_LOADED	2
> #define STATE_FIRMWARE_FAILED	3
> #define STATE_BOOTING		4
> +#define STATE_TX_ACTIVE		5
> +
> +#define HCI_LPM_PKT 0xf1
> +#define HCI_LPM_MAX_SIZE 10
> +#define HCI_LPM_HDR_SIZE HCI_EVENT_HDR_SIZE
> +#define H4_RECV_LPM \
> +	.type = HCI_LPM_PKT, \
> +	.hlen = HCI_LPM_HDR_SIZE, \
> +	.loff = 1, \
> +	.lsize = 1, \
> +	.maxlen = HCI_LPM_MAX_SIZE

lets name this INTEL_RECV_LPM and please put this one above intel_recv_pkts struct. Similar to what has been done in hci_qca.c.

> +
> +#define LPM_OP_TX_NOTIFY 0x00
> +
> +struct hci_lpm_pkt {
> +	__u8 opcode;
> +	__u8 dlen;
> +	__u8 data[0];
> +} __packed;
> 
> struct intel_device {
> 	struct list_head list;
> @@ -141,9 +160,10 @@ static int intel_set_power(struct hci_uart *hu, bool powered)
> 	spin_lock(&intel_device_list_lock);
> 
> 	idev = intel_device_get(hu);
> -	if (!idev)
> +	if (!idev) {
> 		err = -ENODEV;
> 		goto done;
> +	}
> 
> 	if (!idev->reset) {
> 		err = -ENOTSUPP;
> @@ -299,7 +319,9 @@ static int intel_setup(struct hci_uart *hu)
> {
> 	static const u8 reset_param[] = { 0x00, 0x01, 0x00, 0x01,
> 					  0x00, 0x08, 0x04, 0x00 };
> +	static const u8 lpm_param[] = {0x03, 0x07, 0x01, 0x0b};

Coding style.

> 	struct intel_data *intel = hu->priv;
> +	struct intel_device *idev;
> 	struct hci_dev *hdev = hu->hdev;
> 	struct sk_buff *skb;
> 	struct intel_version *ver;
> @@ -663,6 +685,25 @@ done:
> 
> 	BT_INFO("%s: Device booted in %llu usecs", hdev->name, duration);
> 
> +	spin_lock(&intel_device_list_lock);
> +	idev = intel_device_get(hu);
> +	if (!idev || !device_may_wakeup(&idev->pdev->dev)) {
> +		spin_unlock(&intel_device_list_lock);
> +		goto no_lpm;
> +	}
> +	spin_unlock(&intel_device_list_lock);

I really dislike having multiple unlock. This code needs to be restructured to become readable and easy to understand without having to think too much that it is correct.

> +
> +	BT_INFO("%s: Enabling LPM", hdev->name);
> +
> +	skb = __hci_cmd_sync(hdev, 0xfc8b, sizeof(lpm_param), lpm_param,
> +			     HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		BT_ERR("%s: Failed to enable LPM", hdev->name);
> +		goto no_lpm;
> +	}
> +	kfree_skb(skb);
> +
> +no_lpm:
> 	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_CMD_TIMEOUT);
> 	if (IS_ERR(skb))
> 		return PTR_ERR(skb);
> @@ -723,10 +764,43 @@ recv:
> 	return hci_recv_frame(hdev, skb);
> }
> 
> +static void intel_recv_lpm_notification(struct hci_dev *hdev, int value)
> +{

Shorten this name to ..notify instead of ..notification.

> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct intel_data *intel = hu->priv;
> +
> +	BT_DBG("%s: TX idle notification (%d)", hdev->name, value);
> +
> +	if (value)
> +		set_bit(STATE_TX_ACTIVE, &intel->flags);
> +	else
> +		clear_bit(STATE_TX_ACTIVE, &intel->flags);
> +}
> +
> +static int intel_recv_lpm(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct hci_lpm_pkt *lpm = (void *)skb->data;
> +
> +	switch (lpm->opcode) {
> +	case LPM_OP_TX_NOTIFY:
> +		if (lpm->dlen)
> +			intel_recv_lpm_notification(hdev, lpm->data[0]);
> +		break;
> +	default:
> +		BT_ERR("%s: unknown LPM opcode (%02x)", hdev->name,
> +		       lpm->opcode);

We even have break; here.

> +	}
> +
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> static const struct h4_recv_pkt intel_recv_pkts[] = {
> 	{ H4_RECV_ACL,   .recv = hci_recv_frame },
> 	{ H4_RECV_SCO,   .recv = hci_recv_frame },
> 	{ H4_RECV_EVENT, .recv = intel_recv_event },
> +	{ H4_RECV_LPM,   .recv = intel_recv_lpm },
> };
> 
> static int intel_recv(struct hci_uart *hu, const void *data, int count)

Regards

Marcel

[PATCH 4/5] Bluetooth: hci_intel: Add intel_data_list

[Loic Poulain]

In the same way as a intel_data (hu) needs to access its corresponding
intel_device (pdev). A intel_device may be required to access the
intel_data. We need to maintain a list of current intel_data and
protect it against concurrent access.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_intel.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index f855515..99f1307 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -75,11 +75,16 @@ static LIST_HEAD(intel_device_list);
 static DEFINE_SPINLOCK(intel_device_list_lock);
 
 struct intel_data {
+	struct list_head list;
 	struct sk_buff *rx_skb;
 	struct sk_buff_head txq;
 	unsigned long flags;
+	struct hci_uart *hu;
 };
 
+static LIST_HEAD(intel_data_list);
+static DEFINE_MUTEX(intel_data_list_lock);
+
 static u8 intel_convert_speed(unsigned int speed)
 {
 	switch (speed) {
@@ -130,6 +135,24 @@ static struct intel_device *intel_device_get(struct hci_uart *hu)
 	return NULL;
 }
 
+static struct intel_data *intel_data_get(struct intel_device *idev)
+{
+	struct list_head *p;
+
+	list_for_each(p, &intel_data_list) {
+		struct intel_data *intel = list_entry(p, struct intel_data,
+						      list);
+
+		/* tty device and pdev device should share the same parent
+		 * which is the UART port.
+		 */
+		if (intel->hu->tty->dev->parent == idev->pdev->dev.parent)
+			return intel;
+	}
+
+	return NULL;
+}
+
 static int intel_wait_booting(struct hci_uart *hu)
 {
 	struct intel_data *intel = hu->priv;
@@ -194,6 +217,11 @@ static int intel_open(struct hci_uart *hu)
 	skb_queue_head_init(&intel->txq);
 
 	hu->priv = intel;
+	intel->hu = hu;
+
+	mutex_lock(&intel_data_list_lock);
+	list_add_tail(&intel->list, &intel_data_list);
+	mutex_unlock(&intel_data_list_lock);
 
 	if (!intel_set_power(hu, true))
 		set_bit(STATE_BOOTING, &intel->flags);
@@ -209,6 +237,10 @@ static int intel_close(struct hci_uart *hu)
 
 	intel_set_power(hu, false);
 
+	mutex_lock(&intel_data_list_lock);
+	list_del(&intel->list);
+	mutex_unlock(&intel_data_list_lock);
+
 	skb_queue_purge(&intel->txq);
 	kfree_skb(intel->rx_skb);
 	kfree(intel);
-- 
1.9.1

Re: [PATCH 4/5] Bluetooth: hci_intel: Add intel_data_list

[Marcel Holtmann]

Hi Loic,

> In the same way as a intel_data (hu) needs to access its corresponding
> intel_device (pdev). A intel_device may be required to access the
> intel_data. We need to maintain a list of current intel_data and
> protect it against concurrent access.
> 
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index f855515..99f1307 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -75,11 +75,16 @@ static LIST_HEAD(intel_device_list);
> static DEFINE_SPINLOCK(intel_device_list_lock);
> 
> struct intel_data {
> +	struct list_head list;

this is the private data associated with the hci_uart. I am not turning this into a list.

> 	struct sk_buff *rx_skb;
> 	struct sk_buff_head txq;
> 	unsigned long flags;
> +	struct hci_uart *hu;
> };
> 
> +static LIST_HEAD(intel_data_list);
> +static DEFINE_MUTEX(intel_data_list_lock);
> +
> static u8 intel_convert_speed(unsigned int speed)
> {
> 	switch (speed) {
> @@ -130,6 +135,24 @@ static struct intel_device *intel_device_get(struct hci_uart *hu)
> 	return NULL;
> }
> 
> +static struct intel_data *intel_data_get(struct intel_device *idev)
> +{
> +	struct list_head *p;
> +
> +	list_for_each(p, &intel_data_list) {
> +		struct intel_data *intel = list_entry(p, struct intel_data,
> +						      list);
> +
> +		/* tty device and pdev device should share the same parent
> +		 * which is the UART port.
> +		 */
> +		if (intel->hu->tty->dev->parent == idev->pdev->dev.parent)
> +			return intel;
> +	}
> +
> +	return NULL;
> +}
> +

You really need to come up with something else. intel_data is not a list. It is private data for hci_uart.

> static int intel_wait_booting(struct hci_uart *hu)
> {
> 	struct intel_data *intel = hu->priv;
> @@ -194,6 +217,11 @@ static int intel_open(struct hci_uart *hu)
> 	skb_queue_head_init(&intel->txq);
> 
> 	hu->priv = intel;
> +	intel->hu = hu;
> +
> +	mutex_lock(&intel_data_list_lock);
> +	list_add_tail(&intel->list, &intel_data_list);
> +	mutex_unlock(&intel_data_list_lock);
> 
> 	if (!intel_set_power(hu, true))
> 		set_bit(STATE_BOOTING, &intel->flags);
> @@ -209,6 +237,10 @@ static int intel_close(struct hci_uart *hu)
> 
> 	intel_set_power(hu, false);
> 
> +	mutex_lock(&intel_data_list_lock);
> +	list_del(&intel->list);
> +	mutex_unlock(&intel_data_list_lock);
> +
> 	skb_queue_purge(&intel->txq);
> 	kfree_skb(intel->rx_skb);
> 	kfree(intel);

Regards

Marcel

Re: [PATCH 4/5] Bluetooth: hci_intel: Add intel_data_list

[Loic Poulain]

Hi Marcel,

>> struct intel_data {
>> +	struct list_head list;
>
> this is the private data associated with the hci_uart. I am not turning this into a list.

>> +static struct intel_data *intel_data_get(struct intel_device *idev)
>> +{
>> +	struct list_head *p;
>> +
>> +	list_for_each(p, &intel_data_list) {
>> +		struct intel_data *intel = list_entry(p, struct intel_data,
>> +						      list);
>> +
>> +		/* tty device and pdev device should share the same parent
>> +		 * which is the UART port.
>> +		 */
>> +		if (intel->hu->tty->dev->parent == idev->pdev->dev.parent)
>> +			return intel;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>
> You really need to come up with something else. intel_data is not a list. It is private data for hci_uart.
>

Do you mean having an other simple list:
struct intel_hu {
	struct list_head list;
	struct hci_uart *hu;
}

Regards,
Loic

-- 
Intel Open Source Technology Center
http://oss.intel.com/

Re: [PATCH 4/5] Bluetooth: hci_intel: Add intel_data_list

[Marcel Holtmann]

Hi Loic,

>>> struct intel_data {
>>> +	struct list_head list;
>> 
>> this is the private data associated with the hci_uart. I am not turning this into a list.
> 
>>> +static struct intel_data *intel_data_get(struct intel_device *idev)
>>> +{
>>> +	struct list_head *p;
>>> +
>>> +	list_for_each(p, &intel_data_list) {
>>> +		struct intel_data *intel = list_entry(p, struct intel_data,
>>> +						      list);
>>> +
>>> +		/* tty device and pdev device should share the same parent
>>> +		 * which is the UART port.
>>> +		 */
>>> +		if (intel->hu->tty->dev->parent == idev->pdev->dev.parent)
>>> +			return intel;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>> 
>> You really need to come up with something else. intel_data is not a list. It is private data for hci_uart.
>> 
> 
> Do you mean having an other simple list:
> struct intel_hu {
> 	struct list_head list;
> 	struct hci_uart *hu;
> }

this doesn't make it any better. I rather not have this in a list at all. I get the feeling that we really should look into getting UART Slave concept upstream. So the power part can become its own driver.

Regards

Marcel

[PATCH 5/5] Bluetooth: hci_intel: Implement suspend/resume

[Loic Poulain]

Add platform PM suspend/resume callbacks.
Chip can be suspended/resumed via specific vendor commands which need
to be acked by the controller.
Enable the IRQ wake capability in order to let the controller wake the
host in case of available data.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_intel.c | 176 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 176 insertions(+)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 99f1307..4fdcb5d 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -45,6 +45,10 @@
 #define STATE_FIRMWARE_FAILED	3
 #define STATE_BOOTING		4
 #define STATE_TX_ACTIVE		5
+#define STATE_SUSPENDED		7
+#define STATE_SUSPENDING	8
+#define STATE_RESUMING		9
+#define STATE_LPM_ENABLED	10
 
 #define HCI_LPM_PKT 0xf1
 #define HCI_LPM_MAX_SIZE 10
@@ -57,6 +61,8 @@
 	.maxlen = HCI_LPM_MAX_SIZE
 
 #define LPM_OP_TX_NOTIFY 0x00
+#define LPM_OP_SUSPEND_ACK 0x02
+#define LPM_OP_RESUME_ACK 0x03
 
 struct hci_lpm_pkt {
 	__u8 opcode;
@@ -735,6 +741,8 @@ done:
 	}
 	kfree_skb(skb);
 
+	set_bit(STATE_LPM_ENABLED, &intel->flags);
+
 no_lpm:
 	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_CMD_TIMEOUT);
 	if (IS_ERR(skb))
@@ -812,12 +820,28 @@ static void intel_recv_lpm_notification(struct hci_dev *hdev, int value)
 static int intel_recv_lpm(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_lpm_pkt *lpm = (void *)skb->data;
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct intel_data *intel = hu->priv;
 
 	switch (lpm->opcode) {
 	case LPM_OP_TX_NOTIFY:
 		if (lpm->dlen)
 			intel_recv_lpm_notification(hdev, lpm->data[0]);
 		break;
+	case LPM_OP_SUSPEND_ACK:
+		set_bit(STATE_SUSPENDED, &intel->flags);
+		if (test_and_clear_bit(STATE_SUSPENDING, &intel->flags)) {
+			smp_mb__after_atomic();
+			wake_up_bit(&intel->flags, STATE_SUSPENDING);
+		}
+		break;
+	case LPM_OP_RESUME_ACK:
+		clear_bit(STATE_SUSPENDED, &intel->flags);
+		if (test_and_clear_bit(STATE_RESUMING, &intel->flags)) {
+			smp_mb__after_atomic();
+			wake_up_bit(&intel->flags, STATE_SUSPENDING);
+		}
+		break;
 	default:
 		BT_ERR("%s: unknown LPM opcode (%02x)", hdev->name,
 		       lpm->opcode);
@@ -895,6 +919,111 @@ static struct sk_buff *intel_dequeue(struct hci_uart *hu)
 	return skb;
 }
 
+static int intel_lpm_suspend(struct hci_uart *hu)
+{
+	static const u8 suspend[] = {0x01, 0x01, 0x01};
+	struct intel_data *intel = hu->priv;
+	struct sk_buff *skb;
+	int err;
+
+	if (!test_bit(STATE_LPM_ENABLED, &intel->flags) ||
+	    test_bit(STATE_SUSPENDED, &intel->flags))
+		return 0;
+
+	if (test_bit(STATE_TX_ACTIVE, &intel->flags))
+		return -EAGAIN;
+
+	BT_DBG("%s: suspending", hu->hdev->name);
+
+	skb = bt_skb_alloc(sizeof(suspend), GFP_KERNEL);
+	if (!skb) {
+		BT_ERR("Failed to allocate memory for suspend packet");
+		return -ENOMEM;
+	}
+
+	memcpy(skb_put(skb, sizeof(suspend)), suspend, sizeof(suspend));
+	bt_cb(skb)->pkt_type = HCI_LPM_PKT;
+
+	set_bit(STATE_SUSPENDING, &intel->flags);
+
+	skb_queue_tail(&intel->txq, skb);
+	hci_uart_tx_wakeup(hu);
+
+	err = wait_on_bit_timeout(&intel->flags, STATE_SUSPENDING,
+				  TASK_INTERRUPTIBLE,
+				  msecs_to_jiffies(1000));
+	if (err == 1) {
+		BT_ERR("%s: Device suspend interrupted", hu->hdev->name);
+		clear_bit(STATE_SUSPENDING, &intel->flags);
+		return -EINTR;
+	}
+
+	if (err) {
+		BT_ERR("%s: Device suspend timeout", hu->hdev->name);
+		clear_bit(STATE_SUSPENDING, &intel->flags);
+		return -ETIMEDOUT;
+	}
+
+	if (!test_bit(STATE_SUSPENDED, &intel->flags)) {
+		BT_ERR("%s: Device suspend error", hu->hdev->name);
+		return -EINVAL;
+	}
+
+	BT_DBG("%s: suspended", hu->hdev->name);
+
+	return 0;
+}
+
+static int intel_lpm_resume(struct hci_uart *hu)
+{
+	struct intel_data *intel = hu->priv;
+	struct sk_buff *skb;
+	int err;
+
+	if (!test_bit(STATE_LPM_ENABLED, &intel->flags) ||
+	    !test_bit(STATE_SUSPENDED, &intel->flags))
+		return 0;
+
+	BT_DBG("%s: resuming", hu->hdev->name);
+
+	skb = bt_skb_alloc(0, GFP_KERNEL);
+	if (!skb) {
+		BT_ERR("Failed to allocate memory for resume packet");
+		return -ENOMEM;
+	}
+
+	bt_cb(skb)->pkt_type = 0xf0;
+
+	set_bit(STATE_RESUMING, &intel->flags);
+
+	skb_queue_tail(&intel->txq, skb);
+	hci_uart_tx_wakeup(hu);
+
+	err = wait_on_bit_timeout(&intel->flags, STATE_RESUMING,
+				  TASK_INTERRUPTIBLE,
+				  msecs_to_jiffies(1000));
+	if (err == 1) {
+		BT_ERR("%s: Device resume interrupted", hu->hdev->name);
+		clear_bit(STATE_RESUMING, &intel->flags);
+		return -EINTR;
+	}
+
+	if (err) {
+		BT_ERR("%s: Device resume timeout", hu->hdev->name);
+		clear_bit(STATE_RESUMING, &intel->flags);
+		return -ETIMEDOUT;
+	}
+
+	if (test_bit(STATE_SUSPENDED, &intel->flags)) {
+		BT_ERR("%s: Device resume error", hu->hdev->name);
+		return -EINVAL;
+	}
+
+	BT_DBG("%s: resumed", hu->hdev->name);
+
+	return 0;
+}
+
 static const struct hci_uart_proto intel_proto = {
 	.id		= HCI_UART_INTEL,
 	.name		= "Intel",
@@ -1025,6 +1154,52 @@ static int intel_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int intel_suspend(struct device *dev)
+{
+	struct intel_device *idev = dev_get_drvdata(dev);
+	struct intel_data *intel;
+
+	dev_dbg(dev, "intel_suspend\n");
+
+	if ((idev->irq < 0) || !device_may_wakeup(dev))
+		return 0;
+
+	mutex_lock(&intel_data_list_lock);
+	intel = intel_data_get(idev);
+	if (intel)
+		intel_lpm_suspend(intel->hu);
+	mutex_unlock(&intel_data_list_lock);
+
+	enable_irq_wake(idev->irq);
+
+	return 0;
+}
+
+static int intel_resume(struct device *dev)
+{
+	struct intel_device *idev = dev_get_drvdata(dev);
+	struct intel_data *intel;
+
+	dev_dbg(dev, "intel_resume\n");
+
+	if ((idev->irq < 0) || !device_may_wakeup(dev))
+		return 0;
+
+	disable_irq_wake(idev->irq);
+
+	mutex_lock(&intel_data_list_lock);
+	intel = intel_data_get(idev);
+	if (intel)
+		intel_lpm_resume(intel->hu);
+	mutex_unlock(&intel_data_list_lock);
+
+	return 0;
+}
+
+static const struct dev_pm_ops intel_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
+};
+
 static struct platform_driver intel_driver = {
 	.probe = intel_probe,
 	.remove = intel_remove,
@@ -1032,6 +1207,7 @@ static struct platform_driver intel_driver = {
 		.name = "hci_intel",
 		.acpi_match_table = ACPI_PTR(intel_acpi_match),
 		.owner = THIS_MODULE,
+		.pm = &intel_pm_ops,
 	},
 };
 
-- 
1.9.1

Re: [PATCH 5/5] Bluetooth: hci_intel: Implement suspend/resume

[Marcel Holtmann]

Hi Loic,

> Add platform PM suspend/resume callbacks.
> Chip can be suspended/resumed via specific vendor commands which need
> to be acked by the controller.
> Enable the IRQ wake capability in order to let the controller wake the
> host in case of available data.
> 
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 176 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 176 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index 99f1307..4fdcb5d 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -45,6 +45,10 @@
> #define STATE_FIRMWARE_FAILED	3
> #define STATE_BOOTING		4
> #define STATE_TX_ACTIVE		5
> +#define STATE_SUSPENDED		7
> +#define STATE_SUSPENDING	8
> +#define STATE_RESUMING		9
> +#define STATE_LPM_ENABLED	10

at some point we really need to think about how we can combine USB and UART into a unified code block. So we need to be careful with just stuffing new states in here.

> 
> #define HCI_LPM_PKT 0xf1
> #define HCI_LPM_MAX_SIZE 10
> @@ -57,6 +61,8 @@
> 	.maxlen = HCI_LPM_MAX_SIZE
> 
> #define LPM_OP_TX_NOTIFY 0x00
> +#define LPM_OP_SUSPEND_ACK 0x02
> +#define LPM_OP_RESUME_ACK 0x03
> 
> struct hci_lpm_pkt {
> 	__u8 opcode;
> @@ -735,6 +741,8 @@ done:
> 	}
> 	kfree_skb(skb);
> 
> +	set_bit(STATE_LPM_ENABLED, &intel->flags);
> +

This should have gone in the previous patch introducing LPM support.

> no_lpm:
> 	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_CMD_TIMEOUT);
> 	if (IS_ERR(skb))
> @@ -812,12 +820,28 @@ static void intel_recv_lpm_notification(struct hci_dev *hdev, int value)
> static int intel_recv_lpm(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	struct hci_lpm_pkt *lpm = (void *)skb->data;
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct intel_data *intel = hu->priv;
> 
> 	switch (lpm->opcode) {
> 	case LPM_OP_TX_NOTIFY:
> 		if (lpm->dlen)
> 			intel_recv_lpm_notification(hdev, lpm->data[0]);
> 		break;
> +	case LPM_OP_SUSPEND_ACK:
> +		set_bit(STATE_SUSPENDED, &intel->flags);
> +		if (test_and_clear_bit(STATE_SUSPENDING, &intel->flags)) {
> +			smp_mb__after_atomic();
> +			wake_up_bit(&intel->flags, STATE_SUSPENDING);
> +		}
> +		break;
> +	case LPM_OP_RESUME_ACK:
> +		clear_bit(STATE_SUSPENDED, &intel->flags);
> +		if (test_and_clear_bit(STATE_RESUMING, &intel->flags)) {
> +			smp_mb__after_atomic();
> +			wake_up_bit(&intel->flags, STATE_SUSPENDING);
> +		}

Is this a typo with RESUMING vs SUSPENDING? Otherwise better call this LPM_TRANSACTION or something that makes clear that we are doing a LPM change. Since we can only have one at a time.

> +		break;
> 	default:
> 		BT_ERR("%s: unknown LPM opcode (%02x)", hdev->name,
> 		       lpm->opcode);
> @@ -895,6 +919,111 @@ static struct sk_buff *intel_dequeue(struct hci_uart *hu)
> 	return skb;
> }
> 
> +static int intel_lpm_suspend(struct hci_uart *hu)
> +{
> +	static const u8 suspend[] = {0x01, 0x01, 0x01};
> +	struct intel_data *intel = hu->priv;
> +	struct sk_buff *skb;
> +	int err;
> +
> +	if (!test_bit(STATE_LPM_ENABLED, &intel->flags) ||
> +	    test_bit(STATE_SUSPENDED, &intel->flags))
> +		return 0;
> +
> +	if (test_bit(STATE_TX_ACTIVE, &intel->flags))
> +		return -EAGAIN;
> +
> +	BT_DBG("%s: suspending", hu->hdev->name);
> +
> +	skb = bt_skb_alloc(sizeof(suspend), GFP_KERNEL);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for suspend packet");
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(skb_put(skb, sizeof(suspend)), suspend, sizeof(suspend));
> +	bt_cb(skb)->pkt_type = HCI_LPM_PKT;
> +
> +	set_bit(STATE_SUSPENDING, &intel->flags);
> +
> +	skb_queue_tail(&intel->txq, skb);
> +	hci_uart_tx_wakeup(hu);
> +
> +	err = wait_on_bit_timeout(&intel->flags, STATE_SUSPENDING,
> +				  TASK_INTERRUPTIBLE,
> +				  msecs_to_jiffies(1000));
> +	if (err == 1) {
> +		BT_ERR("%s: Device suspend interrupted", hu->hdev->name);
> +		clear_bit(STATE_SUSPENDING, &intel->flags);
> +		return -EINTR;
> +	}
> +
> +	if (err) {
> +		BT_ERR("%s: Device suspend timeout", hu->hdev->name);
> +		clear_bit(STATE_SUSPENDING, &intel->flags);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (!test_bit(STATE_SUSPENDED, &intel->flags)) {
> +		BT_ERR("%s: Device suspend error", hu->hdev->name);
> +		return -EINVAL;
> +	}

This keeps repeating itself. Create a helper function like I did with the booting one.

> +
> +	BT_DBG("%s: suspended", hu->hdev->name);
> +
> +	return 0;
> +}
> +
> +static int intel_lpm_resume(struct hci_uart *hu)
> +{
> +	struct intel_data *intel = hu->priv;
> +	struct sk_buff *skb;
> +	int err;
> +
> +	if (!test_bit(STATE_LPM_ENABLED, &intel->flags) ||
> +	    !test_bit(STATE_SUSPENDED, &intel->flags))
> +		return 0;
> +
> +	BT_DBG("%s: resuming", hu->hdev->name);
> +
> +	skb = bt_skb_alloc(0, GFP_KERNEL);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for resume packet");
> +		return -ENOMEM;
> +	}
> +
> +	bt_cb(skb)->pkt_type = 0xf0;

Introduce a define for this type like you did for the other one.

> +
> +	set_bit(STATE_RESUMING, &intel->flags);
> +
> +	skb_queue_tail(&intel->txq, skb);
> +	hci_uart_tx_wakeup(hu);
> +
> +	err = wait_on_bit_timeout(&intel->flags, STATE_RESUMING,
> +				  TASK_INTERRUPTIBLE,
> +				  msecs_to_jiffies(1000));
> +	if (err == 1) {
> +		BT_ERR("%s: Device resume interrupted", hu->hdev->name);
> +		clear_bit(STATE_RESUMING, &intel->flags);
> +		return -EINTR;
> +	}
> +
> +	if (err) {
> +		BT_ERR("%s: Device resume timeout", hu->hdev->name);
> +		clear_bit(STATE_RESUMING, &intel->flags);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (test_bit(STATE_SUSPENDED, &intel->flags)) {
> +		BT_ERR("%s: Device resume error", hu->hdev->name);
> +		return -EINVAL;
> +	}
> +
> +	BT_DBG("%s: resumed", hu->hdev->name);
> +
> +	return 0;
> +}
> +
> static const struct hci_uart_proto intel_proto = {
> 	.id		= HCI_UART_INTEL,
> 	.name		= "Intel",
> @@ -1025,6 +1154,52 @@ static int intel_remove(struct platform_device *pdev)
> 	return 0;
> }
> 
> +static int intel_suspend(struct device *dev)
> +{
> +	struct intel_device *idev = dev_get_drvdata(dev);
> +	struct intel_data *intel;
> +
> +	dev_dbg(dev, "intel_suspend\n");
> +
> +	if ((idev->irq < 0) || !device_may_wakeup(dev))
> +		return 0;

No ( ) around idev->irq < 0.

> +
> +	mutex_lock(&intel_data_list_lock);
> +	intel = intel_data_get(idev);
> +	if (intel)
> +		intel_lpm_suspend(intel->hu);
> +	mutex_unlock(&intel_data_list_lock);
> +
> +	enable_irq_wake(idev->irq);
> +
> +	return 0;
> +}
> +
> +static int intel_resume(struct device *dev)
> +{
> +	struct intel_device *idev = dev_get_drvdata(dev);
> +	struct intel_data *intel;
> +
> +	dev_dbg(dev, "intel_resume\n");
> +
> +	if ((idev->irq < 0) || !device_may_wakeup(dev))
> +		return 0;
> +
> +	disable_irq_wake(idev->irq);
> +
> +	mutex_lock(&intel_data_list_lock);
> +	intel = intel_data_get(idev);
> +	if (intel)
> +		intel_lpm_resume(intel->hu);
> +	mutex_unlock(&intel_data_list_lock);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops intel_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
> +};
> +

You need #ifdef CONFIG_PM_SLEEP.

> static struct platform_driver intel_driver = {
> 	.probe = intel_probe,
> 	.remove = intel_remove,
> @@ -1032,6 +1207,7 @@ static struct platform_driver intel_driver = {
> 		.name = "hci_intel",
> 		.acpi_match_table = ACPI_PTR(intel_acpi_match),
> 		.owner = THIS_MODULE,
> +		.pm = &intel_pm_ops,
> 	},
> };

Regards

Marcel

Re: [PATCH 1/5] Bluetooth: hci_intel: Retrieve host-wake IRQ

[Marcel Holtmann]

Hi Loic,

> An IRQ can be retrieved from the pdev resources. This irq will be used
> in case of LPM suspend mode to wake-up the host and resume the link.
> This resource can be declared as a GPIO-Interrupt which requires to be
> converted into IRQ.
> 
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 45 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index 46426ff..934b5a7 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -31,6 +31,7 @@
> #include <linux/platform_device.h>
> #include <linux/gpio/consumer.h>
> #include <linux/acpi.h>
> +#include <linux/interrupt.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -48,6 +49,7 @@ struct intel_device {
> 	struct list_head list;
> 	struct platform_device *pdev;
> 	struct gpio_desc *reset;
> +	int irq;
> };
> 
> static LIST_HEAD(intel_device_list);
> @@ -789,6 +791,15 @@ static const struct hci_uart_proto intel_proto = {
> 	.dequeue	= intel_dequeue,
> };
> 
> +static irqreturn_t intel_irq(int irq, void *dev_id)
> +{
> +	struct intel_device *idev = dev_id;
> +
> +	dev_info(&idev->pdev->dev, "hci_intel irq\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id intel_acpi_match[] = {
> 	{ "INT33E1", 0 },
> @@ -816,6 +827,7 @@ static int intel_acpi_probe(struct intel_device *idev)
> static int intel_probe(struct platform_device *pdev)
> {
> 	struct intel_device *idev;
> +	int err;
> 
> 	idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
> 	if (!idev)
> @@ -838,6 +850,36 @@ static int intel_probe(struct platform_device *pdev)
> 		return PTR_ERR(idev->reset);
> 	}
> 
> +	idev->irq = platform_get_irq(pdev, 0);
> +	if (idev->irq < 0) {
> +		struct gpio_desc *host_wake;
> +
> +		dev_err(&pdev->dev, "No IRQ, falling back to gpio-irq\n");
> +
> +		host_wake = devm_gpiod_get_optional(&pdev->dev, "host-wake",
> +						    GPIOD_IN);
> +		if (IS_ERR(host_wake)) {
> +			dev_err(&pdev->dev, "Unable to retrieve IRQ\n");
> +			goto no_irq;
> +		}
> +
> +		idev->irq = gpiod_to_irq(host_wake);
> +		if (idev->irq < 0) {
> +			dev_err(&pdev->dev, "No corresponding irq for gpio\n");
> +			goto no_irq;
> +		}
> +	}
> +
> +	err = devm_request_threaded_irq(&pdev->dev, idev->irq, NULL, intel_irq,
> +					IRQF_ONESHOT, "bt-host-wake", idev);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to allocate irq\n");
> +		return err;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, true);

do you really want to do this in probe? Meaning that as soon as we boot, we claim this interrupt. Isn't it better to do this when the line discipline gets attached.

> +
> +no_irq:
> 	platform_set_drvdata(pdev, idev);
> 
> 	/* Place this instance on the device list */
> @@ -845,7 +887,8 @@ static int intel_probe(struct platform_device *pdev)
> 	list_add_tail(&idev->list, &intel_device_list);
> 	spin_unlock(&intel_device_list_lock);
> 
> -	dev_info(&pdev->dev, "registered.\n");
> +	dev_info(&pdev->dev, "registered, gpio(%d)/irq(%d).\n",
> +		 desc_to_gpio(idev->reset), idev->irq);
> 
> 	return 0;

Regards

Marcel

Re: [PATCH 1/5] Bluetooth: hci_intel: Retrieve host-wake IRQ

[Loic Poulain]

Hi Marcel,

>> +	idev->irq = platform_get_irq(pdev, 0);
>> +	if (idev->irq < 0) {
>> +		struct gpio_desc *host_wake;
>> +
>> +		dev_err(&pdev->dev, "No IRQ, falling back to gpio-irq\n");
>> +
>> +		host_wake = devm_gpiod_get_optional(&pdev->dev, "host-wake",
>> +						    GPIOD_IN);
>> +		if (IS_ERR(host_wake)) {
>> +			dev_err(&pdev->dev, "Unable to retrieve IRQ\n");
>> +			goto no_irq;
>> +		}
>> +
>> +		idev->irq = gpiod_to_irq(host_wake);
>> +		if (idev->irq < 0) {
>> +			dev_err(&pdev->dev, "No corresponding irq for gpio\n");
>> +			goto no_irq;
>> +		}
>> +	}
>> +
>> +	err = devm_request_threaded_irq(&pdev->dev, idev->irq, NULL, intel_irq,
>> +					IRQF_ONESHOT, "bt-host-wake", idev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "unable to allocate irq\n");
>> +		return err;
>> +	}
>> +
>> +	device_init_wakeup(&pdev->dev, true);
>
> do you really want to do this in probe? Meaning that as soon as we boot, we claim this interrupt. Isn't it better to do this when the line discipline gets attached.


Is it ok to move the irq request/free in intel_set_power?

Regards,
Loic

-- 
Intel Open Source Technology Center
http://oss.intel.com/

Re: [PATCH 1/5] Bluetooth: hci_intel: Retrieve host-wake IRQ

[Marcel Holtmann]

Hi Loic,

>>> +	idev->irq = platform_get_irq(pdev, 0);
>>> +	if (idev->irq < 0) {
>>> +		struct gpio_desc *host_wake;
>>> +
>>> +		dev_err(&pdev->dev, "No IRQ, falling back to gpio-irq\n");
>>> +
>>> +		host_wake = devm_gpiod_get_optional(&pdev->dev, "host-wake",
>>> +						    GPIOD_IN);
>>> +		if (IS_ERR(host_wake)) {
>>> +			dev_err(&pdev->dev, "Unable to retrieve IRQ\n");
>>> +			goto no_irq;
>>> +		}
>>> +
>>> +		idev->irq = gpiod_to_irq(host_wake);
>>> +		if (idev->irq < 0) {
>>> +			dev_err(&pdev->dev, "No corresponding irq for gpio\n");
>>> +			goto no_irq;
>>> +		}
>>> +	}
>>> +
>>> +	err = devm_request_threaded_irq(&pdev->dev, idev->irq, NULL, intel_irq,
>>> +					IRQF_ONESHOT, "bt-host-wake", idev);
>>> +	if (err) {
>>> +		dev_err(&pdev->dev, "unable to allocate irq\n");
>>> +		return err;
>>> +	}
>>> +
>>> +	device_init_wakeup(&pdev->dev, true);
>> 
>> do you really want to do this in probe? Meaning that as soon as we boot, we claim this interrupt. Isn't it better to do this when the line discipline gets attached.
> 
> 
> Is it ok to move the irq request/free in intel_set_power?

I would assume so.

Regards

Marcel