[PATCH v3 00/13] Bluetooth on 2015+ MacBook (Pro)

[PATCH v3 00/13] Bluetooth on 2015+ MacBook (Pro)

[Lukas Wunner]

Enable UART-attached Bluetooth on 2015+ Macs, v3.

The series has grown to 13 patches because bugs in the driver keep
cropping up.

Changes since v2:

- In patch [01/13] ("Mandate presence of shutdown and device wake GPIO"):
  Keep netdev indentation style. (Marcel)

- New patch [04/13] to fix an IRQ race on ->close.

- New patch [05/13] to fix an unbalanced pm_runtime_disable(). (Andy)

- New patch [06/13] to invalidate the IRQ if requesting it failed.

- New patch [08/13] to add callbacks to toggle GPIOs.
  This makes the subsequent addition of Mac support less intrusive.
  Replaces patch "Add helper to toggle device wake GPIO" in v2. (Andy)

- In patch [09/13] ("Handle errors properly"):
  Drop redundant assignment. (Andy)

- In patch [10/13] ("Support Apple GPIO handling"):
  Don't enable runtime PM on Macs for lack of usable host wake IRQ (Hans),
  s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)

- Move patch "Silence IRQ printk" to end of series as it's merely
  a cleanup and no longer necessary for Mac support.

- New patch [12/13] to use msleep() instead of mdelay()
  after toggling device wake pin. (Andy)

- New patch [13/13] to fix sleep mode struct ordering.

- Drop incorrect patch "Enable runtime PM despite absence of IRQ". (Hans)

Link to v2:
https://marc.info/?l=linux-bluetooth&m=151492012812029&w=4

Thanks,

Lukas


Lukas Wunner (12):
  Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO
  Bluetooth: hci_bcm: Clean up unnecessary #ifdef
  Bluetooth: hci_bcm: Fix race on close
  Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()
  Bluetooth: hci_bcm: Invalidate IRQ on request failure
  Bluetooth: hci_bcm: Document struct bcm_device
  Bluetooth: hci_bcm: Add callbacks to toggle GPIOs
  Bluetooth: hci_bcm: Handle errors properly
  Bluetooth: hci_bcm: Support Apple GPIO handling
  Bluetooth: hci_bcm: Silence IRQ printk
  Bluetooth: hci_bcm: Sleep instead of spinning
  Bluetooth: btbcm: Fix sleep mode struct ordering

Ronald Tschalär (1):
  Bluetooth: hci_bcm: Validate IRQ before using it

 drivers/bluetooth/btbcm.h   |   2 +-
 drivers/bluetooth/hci_bcm.c | 227 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 189 insertions(+), 40 deletions(-)

-- 
2.15.1

[PATCH v3 09/13] Bluetooth: hci_bcm: Handle errors properly

[Lukas Wunner]

A significant portion of this driver lacks error handling.  As a first
step, add error paths to bcm_gpio_set_power(), bcm_open(), bcm_close(),
bcm_suspend_device(), bcm_resume_device(), bcm_resume(), bcm_probe() and
bcm_serdev_probe().  (I've also scrutinized bcm_suspend() but think it's
fine as is.)

Those are all the functions accessing the device wake and shutdown GPIO.
On Apple Macs the pins are accessed through ACPI methods, which may fail
for various reasons, hence proper error handling is necessary.  Non-Macs
access the pins directly, which may fail as well but the GPIO core does
not yet pass back errors to consumers.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 90 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 74 insertions(+), 16 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 81eb6ac80de6..a8d12a53ea1c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -197,11 +197,21 @@ static bool bcm_device_exists(struct bcm_device *device)
 
 static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 {
-	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
-		clk_prepare_enable(dev->clk);
+	int err;
 
-	dev->set_shutdown(dev, powered);
-	dev->set_device_wakeup(dev, powered);
+	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled) {
+		err = clk_prepare_enable(dev->clk);
+		if (err)
+			return err;
+	}
+
+	err = dev->set_shutdown(dev, powered);
+	if (err)
+		goto err_clk_disable;
+
+	err = dev->set_device_wakeup(dev, powered);
+	if (err)
+		goto err_revert_shutdown;
 
 	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
 		clk_disable_unprepare(dev->clk);
@@ -209,6 +219,13 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	dev->clk_enabled = powered;
 
 	return 0;
+
+err_revert_shutdown:
+	dev->set_shutdown(dev, !powered);
+err_clk_disable:
+	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
+		clk_disable_unprepare(dev->clk);
+	return err;
 }
 
 #ifdef CONFIG_PM
@@ -331,6 +348,7 @@ static int bcm_open(struct hci_uart *hu)
 {
 	struct bcm_data *bcm;
 	struct list_head *p;
+	int err;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
@@ -345,7 +363,10 @@ static int bcm_open(struct hci_uart *hu)
 	mutex_lock(&bcm_device_lock);
 
 	if (hu->serdev) {
-		serdev_device_open(hu->serdev);
+		err = serdev_device_open(hu->serdev);
+		if (err)
+			goto err_free;
+
 		bcm->dev = serdev_device_get_drvdata(hu->serdev);
 		goto out;
 	}
@@ -373,17 +394,30 @@ static int bcm_open(struct hci_uart *hu)
 	if (bcm->dev) {
 		hu->init_speed = bcm->dev->init_speed;
 		hu->oper_speed = bcm->dev->oper_speed;
-		bcm_gpio_set_power(bcm->dev, true);
+		err = bcm_gpio_set_power(bcm->dev, true);
+		if (err)
+			goto err_unset_hu;
 	}
 
 	mutex_unlock(&bcm_device_lock);
 	return 0;
+
+err_unset_hu:
+#ifdef CONFIG_PM
+	bcm->dev->hu = NULL;
+#endif
+err_free:
+	mutex_unlock(&bcm_device_lock);
+	hu->priv = NULL;
+	kfree(bcm);
+	return err;
 }
 
 static int bcm_close(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
 	struct bcm_device *bdev = NULL;
+	int err;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
@@ -406,8 +440,10 @@ static int bcm_close(struct hci_uart *hu)
 			device_init_wakeup(bdev->dev, false);
 		}
 
-		bcm_gpio_set_power(bdev, false);
-		if (bdev->irq > 0) {
+		err = bcm_gpio_set_power(bdev, false);
+		if (err) {
+			bt_dev_err(hu->hdev, "Failed to power down");
+		} else if (bdev->irq > 0) {
 			pm_runtime_disable(bdev->dev);
 			pm_runtime_set_suspended(bdev->dev);
 		}
@@ -590,6 +626,7 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 static int bcm_suspend_device(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err;
 
 	bt_dev_dbg(bdev, "");
 
@@ -601,7 +638,15 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	bdev->set_device_wakeup(bdev, false);
+	err = bdev->set_device_wakeup(bdev, false);
+	if (err) {
+		if (bdev->is_suspended && bdev->hu) {
+			bdev->is_suspended = false;
+			hci_uart_set_flow_control(bdev->hu, false);
+		}
+		return -EBUSY;
+	}
+
 	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
 	mdelay(15);
 
@@ -611,10 +656,16 @@ static int bcm_suspend_device(struct device *dev)
 static int bcm_resume_device(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err;
 
 	bt_dev_dbg(bdev, "");
 
-	bdev->set_device_wakeup(bdev, true);
+	err = bdev->set_device_wakeup(bdev, true);
+	if (err) {
+		dev_err(dev, "Failed to power up\n");
+		return err;
+	}
+
 	bt_dev_dbg(bdev, "resume, delaying 15 ms");
 	mdelay(15);
 
@@ -668,6 +719,7 @@ static int bcm_suspend(struct device *dev)
 static int bcm_resume(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err = 0;
 
 	bt_dev_dbg(bdev, "resume: is_suspended %d", bdev->is_suspended);
 
@@ -687,14 +739,16 @@ static int bcm_resume(struct device *dev)
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
 
-	bcm_resume_device(dev);
+	err = bcm_resume_device(dev);
 
 unlock:
 	mutex_unlock(&bcm_device_lock);
 
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
+	if (!err) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
 
 	return 0;
 }
@@ -925,7 +979,9 @@ static int bcm_probe(struct platform_device *pdev)
 	list_add_tail(&dev->list, &bcm_device_list);
 	mutex_unlock(&bcm_device_lock);
 
-	bcm_gpio_set_power(dev, false);
+	ret = bcm_gpio_set_power(dev, false);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to power down\n");
 
 	return 0;
 }
@@ -1027,7 +1083,9 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
 	if (err)
 		return err;
 
-	bcm_gpio_set_power(bcmdev, false);
+	err = bcm_gpio_set_power(bcmdev, false);
+	if (err)
+		dev_err(&serdev->dev, "Failed to power down\n");
 
 	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
 }
-- 
2.15.1

Re: [PATCH v3 09/13] Bluetooth: hci_bcm: Handle errors properly

[Lukas Wunner]

On Sun, Jan 07, 2018 at 08:16:32PM +0100, Lukas Wunner wrote:
> @@ -406,8 +440,10 @@ static int bcm_close(struct hci_uart *hu)
>  			device_init_wakeup(bdev->dev, false);
>  		}
>  
> -		bcm_gpio_set_power(bdev, false);
> -		if (bdev->irq > 0) {
> +		err = bcm_gpio_set_power(bdev, false);
> +		if (err) {
> +			bt_dev_err(hu->hdev, "Failed to power down");
> +		} else if (bdev->irq > 0) {
>  			pm_runtime_disable(bdev->dev);
>  			pm_runtime_set_suspended(bdev->dev);
>  		}

Hm, I realize I've botched this (again):  If a valid IRQ was found
but bcm_gpio_set_power() fails here, then the pm_runtime_enable()
in bcm_request_irq() isn't balanced. :-(

So pm_runtime_disable() needs to be called even if err != 0
(but only if bdev->irq > 0).

Lukas

[PATCH v3 07/13] Bluetooth: hci_bcm: Document struct bcm_device

[Lukas Wunner]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index b5f8008758bc..888dcec56bc2 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -52,7 +52,30 @@
 
 #define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
 
-/* device driver resources */
+/**
+ * struct bcm_device - device driver resources
+ * @serdev_hu: HCI UART controller struct
+ * @list: bcm_device_list node
+ * @dev: physical UART slave
+ * @name: device name logged by bt_dev_*() functions
+ * @device_wakeup: BT_WAKE pin,
+ *	assert = Bluetooth device must wake up or remain awake,
+ *	deassert = Bluetooth device may sleep when sleep criteria are met
+ * @shutdown: BT_REG_ON pin,
+ *	power up or power down Bluetooth device internal regulators
+ * @clk: clock used by Bluetooth device
+ * @clk_enabled: whether @clk is prepared and enabled
+ * @init_speed: default baudrate of Bluetooth device;
+ *	the host UART is initially set to this baudrate so that
+ *	it can configure the Bluetooth device for @oper_speed
+ * @oper_speed: preferred baudrate of Bluetooth device;
+ *	set to 0 if @init_speed is already the preferred baudrate
+ * @irq: interrupt triggered by HOST_WAKE_BT pin
+ * @irq_active_low: whether @irq is active low
+ * @hu: pointer to HCI UART controller struct,
+ *	used to enable flow control during runtime suspend and system sleep
+ * @is_suspended: whether flow control is currently enabled
+ */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
 	struct hci_uart		serdev_hu;
@@ -74,7 +97,7 @@ struct bcm_device {
 
 #ifdef CONFIG_PM
 	struct hci_uart		*hu;
-	bool			is_suspended; /* suspend/resume flag */
+	bool			is_suspended;
 #endif
 };
 
-- 
2.15.1

Re: [PATCH v3 07/13] Bluetooth: hci_bcm: Document struct bcm_device

[Lukas Wunner]

On Sun, Jan 07, 2018 at 08:16:32PM +0100, Lukas Wunner wrote:
> + * @hu: pointer to HCI UART controller struct,
> + *	used to enable flow control during runtime suspend and system sleep
> + * @is_suspended: whether flow control is currently enabled

Botched this as well unfortunately, it should read "used to *disable*
flow control".

I didn't realize that the semantics of hci_uart_set_flow_control()
are reversed and that "false" actually *enables* flow control.
(What's the rationale for that anyway?)

Lukas

[PATCH v3 06/13] Bluetooth: hci_bcm: Invalidate IRQ on request failure

[Lukas Wunner]

If devm_request_irq() fails, the driver bails out of bcm_request_irq()
but continues to ->setup the device (because the IRQ is optional).

The driver subsequently calls devm_free_irq(), enable_irq_wake() and
disable_irq_wake() on the IRQ even though requesting it failed.

Avoid by invalidating the IRQ on request failure.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 991d429246f7..b5f8008758bc 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -216,8 +216,10 @@ static int bcm_request_irq(struct bcm_data *bcm)
 			       bdev->irq_active_low ? IRQF_TRIGGER_FALLING :
 						      IRQF_TRIGGER_RISING,
 			       "host_wake", bdev);
-	if (err)
+	if (err) {
+		bdev->irq = err;
 		goto unlock;
+	}
 
 	device_init_wakeup(bdev->dev, true);
 
-- 
2.15.1

[PATCH v3 11/13] Bluetooth: hci_bcm: Silence IRQ printk

[Lukas Wunner]

The host wake IRQ is optional, but if none is found, "BCM irq: -22" is
logged which may irritate users.  This is really a debug message, so use
dev_dbg() instead of dev_info().  If users are interested in the IRQ,
they can always consult /proc/interrupts.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 53615fd5e442..5252b3ef5ad4 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -942,7 +942,7 @@ static int bcm_get_resources(struct bcm_device *dev)
 		dev->irq = gpiod_to_irq(gpio);
 	}
 
-	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
+	dev_dbg(dev->dev, "BCM irq: %d\n", dev->irq);
 	return 0;
 }
 
-- 
2.15.1

[PATCH v3 13/13] Bluetooth: btbcm: Fix sleep mode struct ordering

[Lukas Wunner]

According to the documentation for Laird SD40 radio modules (which use
the BCM4329 chipset), the order of the Enable_BREAK_To_Host and
Pulsed_HOST_WAKE parameters in the sleep mode struct is reversed
vis-à-vis our struct declaration.  See page 46 of this PDF:

http://cdn.lairdtech.com/home/brandworld/files/Application%20Note%20-%2040%20Series%20Bluetooth.pdf

The documentation is dated Oct 2015, so fairly recent, making it appear
more likely that the documentation is correct and our code is wrong.
Amend our code to be in congruence with the documentation.

Cc: Sue White <sue.white@lairdtech.com>
Cc: Ilya Faenson <ifaenson@broadcom.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/btbcm.h   | 2 +-
 drivers/bluetooth/hci_bcm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index d9e6b41658e5..cfe6ad4cc621 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -44,8 +44,8 @@ struct bcm_set_sleep_mode {
 	__u8 tristate_control;
 	__u8 usb_auto_sleep;
 	__u8 usb_resume_timeout;
-	__u8 pulsed_host_wake;
 	__u8 break_to_host;
+	__u8 pulsed_host_wake;
 } __packed;
 
 struct bcm_set_pcm_int_params {
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 81e82ce2da96..7793a9847fdb 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -300,8 +300,8 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
 	/* Irrelevant USB flags */
 	.usb_auto_sleep = 0,
 	.usb_resume_timeout = 0,
+	.break_to_host = 0,
 	.pulsed_host_wake = 0,
-	.break_to_host = 0
 };
 
 static int bcm_setup_sleep(struct hci_uart *hu)
-- 
2.15.1

[PATCH v3 12/13] Bluetooth: hci_bcm: Sleep instead of spinning

[Lukas Wunner]

The driver calls mdelay(15) in the ->suspend, ->resume, ->runtime_suspend
and ->runtime_resume hook, however spinning for such a long period of
time is discouraged as per Documentation/timers/timers-howto.txt.

The use of mdelay() seems unnecessary, it is allowed to sleep in the
system sleep and runtime PM hooks (with the exception of ->suspend_noirq
and ->resume_noirq) and the driver itself also does not rely on a
non-sleeping ->runtime_resume as the only place where a synchronous
resume is performed, in bcm_dequeue(), is called from a work item in
hci_ldisc.c and hci_serdev.c.

So replace the mdelay(15) with msleep(15).

Note that the delay is inserted after asserting or deasserting the
device wake pin, but in bcm_gpio_set_power() that pin is asserted or
deasserted *without* observing a delay.  It is thus unclear if the delay
is necessary at all.  It is likewise unclear why it is exactly 15 ms,
the commit introducing it, 118612fb9165 ("Bluetooth: hci_bcm: Add
suspend/resume PM functions"), does not provide a rationale.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 5252b3ef5ad4..81e82ce2da96 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -657,7 +657,7 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-	mdelay(15);
+	msleep(15);
 
 	return 0;
 }
@@ -676,7 +676,7 @@ static int bcm_resume_device(struct device *dev)
 	}
 
 	bt_dev_dbg(bdev, "resume, delaying 15 ms");
-	mdelay(15);
+	msleep(15);
 
 	/* When this executes, the device has woken up already */
 	if (bdev->is_suspended && bdev->hu) {
-- 
2.15.1

[PATCH v3 02/13] Bluetooth: hci_bcm: Validate IRQ before using it

[Lukas Wunner]

From: Ronald Tschalär <ronald@innovation.ch>

The ->close, ->suspend and ->resume hooks assume presence of a valid IRQ
if the device is wakeup capable.  However it's entirely possible that
wakeup was enabled by some other entity besides this driver and in this
case the user will get a WARN splat if no valid IRQ was found.  Avoid by
checking if the IRQ is valid, i.e. > 0.

Case in point:  On recent MacBook Pros, the Bluetooth device lacks an
IRQ (because host wakeup is handled by the SMC, independently of the
operating system), but it does possess a _PRW method (which specifies
the SMC's GPE as wake event).  The ACPI core therefore automatically
marks the physical Bluetooth device wakeup capable upon binding it to
its ACPI companion:

device_set_wakeup_capable+0x96/0xb0
acpi_bind_one+0x28a/0x310
acpi_platform_notify+0x20/0xa0
device_add+0x215/0x690
serdev_device_add+0x57/0xf0
acpi_serdev_add_device+0xc9/0x110
acpi_ns_walk_namespace+0x131/0x280
acpi_walk_namespace+0xf5/0x13d
serdev_controller_add+0x6f/0x110
serdev_tty_port_register+0x98/0xf0
tty_port_register_device_attr_serdev+0x3a/0x70
uart_add_one_port+0x268/0x500
serial8250_register_8250_port+0x32e/0x490
dw8250_probe+0x46c/0x720
platform_drv_probe+0x35/0x90
driver_probe_device+0x300/0x450
bus_for_each_drv+0x67/0xb0
__device_attach+0xde/0x160
bus_probe_device+0x9c/0xb0
device_add+0x448/0x690
platform_device_add+0x10e/0x260
mfd_add_device+0x392/0x4c0
mfd_add_devices+0xb1/0x110
intel_lpss_probe+0x2a9/0x610 [intel_lpss]
intel_lpss_pci_probe+0x7a/0xa8 [intel_lpss_pci]

Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
[lukas: fix up ->suspend and ->resume as well, add commit message]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 7d059b35c63a..d9f71ed26667 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -377,7 +377,7 @@ static int bcm_close(struct hci_uart *hu)
 		pm_runtime_disable(bdev->dev);
 		pm_runtime_set_suspended(bdev->dev);
 
-		if (device_can_wakeup(bdev->dev)) {
+		if (bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
@@ -623,7 +623,7 @@ static int bcm_suspend(struct device *dev)
 	if (pm_runtime_active(dev))
 		bcm_suspend_device(dev);
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev) && bdev->irq > 0) {
 		error = enable_irq_wake(bdev->irq);
 		if (!error)
 			bt_dev_dbg(bdev, "BCM irq: enabled");
@@ -653,7 +653,7 @@ static int bcm_resume(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev) && bdev->irq > 0) {
 		disable_irq_wake(bdev->irq);
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
-- 
2.15.1

[PATCH v3 04/13] Bluetooth: hci_bcm: Fix race on close

[Lukas Wunner]

Upon ->close, the driver powers the Bluetooth controller down, deasserts
the device wake pin, updates the runtime PM status to "suspended" and
finally frees the IRQ.

Because the IRQ is freed last, a runtime resume can take place after
the controller was powered down.  The impact is not grave, the worst
thing that can happen is that the device wake pin is reasserted (should
have no effect while the regulator is off) and that setting the runtime
PM status to "suspended" does not reflect reality.

Still, it's wrong, so free the IRQ first.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index c551ef4c350f..6144a3f9c37a 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -372,14 +372,14 @@ static int bcm_close(struct hci_uart *hu)
 	}
 
 	if (bdev) {
-		bcm_gpio_set_power(bdev, false);
-		pm_runtime_disable(bdev->dev);
-		pm_runtime_set_suspended(bdev->dev);
-
 		if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
+
+		bcm_gpio_set_power(bdev, false);
+		pm_runtime_disable(bdev->dev);
+		pm_runtime_set_suspended(bdev->dev);
 	}
 	mutex_unlock(&bcm_device_lock);
 
-- 
2.15.1

[PATCH v3 05/13] Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()

[Lukas Wunner]

On ->setup, pm_runtime_enable() is only called if a valid IRQ was found,
but on ->close(), pm_runtime_disable() is called unconditionally.
Disablement of runtime PM is recorded in a counter, so every
pm_runtime_disable() needs to be balanced.  Fix it.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 6144a3f9c37a..991d429246f7 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -378,8 +378,10 @@ static int bcm_close(struct hci_uart *hu)
 		}
 
 		bcm_gpio_set_power(bdev, false);
-		pm_runtime_disable(bdev->dev);
-		pm_runtime_set_suspended(bdev->dev);
+		if (bdev->irq > 0) {
+			pm_runtime_disable(bdev->dev);
+			pm_runtime_set_suspended(bdev->dev);
+		}
 	}
 	mutex_unlock(&bcm_device_lock);
 
-- 
2.15.1

[PATCH v3 10/13] Bluetooth: hci_bcm: Support Apple GPIO handling

[Lukas Wunner]

Enable Bluetooth on the following Macs which provide custom ACPI methods
to toggle the GPIOs for device wake and shutdown instead of accessing
the pins directly:

    MacBook8,1     2015  12"
    MacBook9,1     2016  12"
    MacBook10,1    2017  12"
    MacBookPro13,1 2016  13"
    MacBookPro13,2 2016  13" with Touch Bar
    MacBookPro13,3 2016  15" with Touch Bar
    MacBookPro14,1 2017  13"
    MacBookPro14,2 2017  13" with Touch Bar
    MacBookPro14,3 2017  15" with Touch Bar

On the MacBook8,1 Bluetooth is muxed with a second device (a debug port
on the SSD) under the control of PCH GPIO 36.  Because serdev cannot
deal with multiple slaves yet, it is currently necessary to patch the
DSDT and remove the SSDC device.

The custom ACPI methods are called:

    BTLP (Low Power) takes one argument, toggles device wake GPIO
    BTPU (Power Up) tells SMC to drive shutdown GPIO high
    BTPD (Power Down) tells SMC to drive shutdown GPIO low
    BTRS (Reset) calls BTPD followed by BTPU
    BTRB unknown, not present on all MacBooks

Search for the BTLP, BTPU and BTPD methods on ->probe and cache them in
struct bcm_device if the machine is a Mac.

Additionally, set the init_speed based on a custom device property
provided by Apple in lieu of _CRS resources.  The Broadcom UART's speed
is fixed on Apple Macs:  Any attempt to change it results in Bluetooth
status code 0x0c and bcm_set_baudrate() thus always returns -EBUSY.
By setting only the init_speed and leaving oper_speed at zero, we can
achieve that the host UART's speed is adjusted but the Broadcom UART's
speed is left as is.

The host wake pin goes into the SMC which handles it independently
of the OS, so there's no IRQ for it.

Thanks to Ronald Tschalär who did extensive debugging and testing of
this patch and contributed fixes.

ACPI snippet containing the custom methods and device properties
(taken from a MacBook8,1):

    Method (BTLP, 1, Serialized)
    {
        If (LEqual (Arg0, 0x00))
        {
            Store (0x01, GD54) /* set PCH GPIO 54 direction to input */
        }

        If (LEqual (Arg0, 0x01))
        {
            Store (0x00, GD54) /* set PCH GPIO 54 direction to output */
            Store (0x00, GP54) /* set PCH GPIO 54 value to low */
        }
    }

    Method (BTPU, 0, Serialized)
    {
        Store (0x01, \_SB.PCI0.LPCB.EC.BTPC)
        Sleep (0x0A)
    }

    Method (BTPD, 0, Serialized)
    {
        Store (0x00, \_SB.PCI0.LPCB.EC.BTPC)
        Sleep (0x0A)
    }

    Method (BTRS, 0, Serialized)
    {
        BTPD ()
        BTPU ()
    }

    Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
    {
        If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
        {
            Store (Package (0x08)
                {
                    "baud",
                    Buffer (0x08)
                    { 0xC0, 0xC6, 0x2D, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "parity",
                    Buffer (0x08)
                    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "dataBits",
                    Buffer (0x08)
                    { 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "stopBits",
                    Buffer (0x08)
                    { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }
                }, Local0)
            DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
            Return (Local0)
        }
        Return (0x00)
    }

Link: https://github.com/Dunedan/mbp-2016-linux/issues/29
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=110901
Reported-by: Leif Liddy <leif.liddy@gmail.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Max Shavrick <mxms@me.com>                     [MacBook8,1]
Tested-by: Leif Liddy <leif.liddy@gmail.com>              [MacBook9,1]
Tested-by: Daniel Roschka <danielroschka@phoenitydawn.de> [MacBookPro13,2]
Tested-by: Ronald Tschalär <ronald@innovation.ch>         [MacBookPro13,3]
Tested-by: Peter Y. Chuang <peteryuchuang@gmail.com>      [MacBookPro14,1]
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index a8d12a53ea1c..53615fd5e442 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -29,6 +29,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/property.h>
+#include <linux/platform_data/x86/apple.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
@@ -64,7 +65,12 @@
  * @shutdown: BT_REG_ON pin,
  *	power up or power down Bluetooth device internal regulators
  * @set_device_wakeup: callback to toggle BT_WAKE pin
+ *	either by accessing @device_wakeup or by calling @btlp
  * @set_shutdown: callback to toggle BT_REG_ON pin
+ *	either by accessing @shutdown or by calling @btpu/@btpd
+ * @btlp: Apple ACPI method to toggle BT_WAKE pin ("Bluetooth Low Power")
+ * @btpu: Apple ACPI method to drive BT_REG_ON pin high ("Bluetooth Power Up")
+ * @btpd: Apple ACPI method to drive BT_REG_ON pin low ("Bluetooth Power Down")
  * @clk: clock used by Bluetooth device
  * @clk_enabled: whether @clk is prepared and enabled
  * @init_speed: default baudrate of Bluetooth device;
@@ -90,6 +96,9 @@ struct bcm_device {
 	struct gpio_desc	*shutdown;
 	int			(*set_device_wakeup)(struct bcm_device *, bool);
 	int			(*set_shutdown)(struct bcm_device *, bool);
+#ifdef CONFIG_ACPI
+	acpi_handle		btlp, btpu, btpd;
+#endif
 
 	struct clk		*clk;
 	bool			clk_enabled;
@@ -845,6 +854,47 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 
 	return 0;
 }
+
+static int bcm_apple_set_device_wakeup(struct bcm_device *dev, bool awake)
+{
+	if (ACPI_FAILURE(acpi_execute_simple_method(dev->btlp, NULL, !awake)))
+		return -EIO;
+
+	return 0;
+}
+
+static int bcm_apple_set_shutdown(struct bcm_device *dev, bool powered)
+{
+	if (ACPI_FAILURE(acpi_evaluate_object(powered ? dev->btpu : dev->btpd,
+					      NULL, NULL, NULL)))
+		return -EIO;
+
+	return 0;
+}
+
+static int bcm_apple_get_resources(struct bcm_device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev->dev);
+	const union acpi_object *obj;
+
+	if (!adev ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTLP", &dev->btlp)) ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTPU", &dev->btpu)) ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTPD", &dev->btpd)))
+		return -ENODEV;
+
+	if (!acpi_dev_get_property(adev, "baud", ACPI_TYPE_BUFFER, &obj) &&
+	    obj->buffer.length == 8)
+		dev->init_speed = *(u64 *)obj->buffer.pointer;
+
+	dev->set_device_wakeup = bcm_apple_set_device_wakeup;
+	dev->set_shutdown = bcm_apple_set_shutdown;
+
+	return 0;
+}
+#else
+static inline int bcm_apple_get_resources(struct bcm_device *dev)
+{ return -EOPNOTSUPP; }
 #endif /* CONFIG_ACPI */
 
 static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
@@ -863,6 +913,9 @@ static int bcm_get_resources(struct bcm_device *dev)
 {
 	dev->name = dev_name(dev->dev);
 
+	if (x86_apple_machine && !bcm_apple_get_resources(dev))
+		return 0;
+
 	dev->clk = devm_clk_get(dev->dev, NULL);
 
 	dev->device_wakeup = devm_gpiod_get(dev->dev, "device-wakeup",
-- 
2.15.1

Re: [PATCH v3 10/13] Bluetooth: hci_bcm: Support Apple GPIO handling

[Andy Shevchenko]

On Sun, 2018-01-07 at 20:16 +0100, Lukas Wunner wrote:

It looks like you are going to send another version, thus...

> +#else
> +static inline int bcm_apple_get_resources(struct bcm_device *dev)
> +{ return -EOPNOTSUPP; }

I would not do this kind of style, better to unroll to normal, like

{
	return -EOPNOTSUPP;
}

>  #endif /* CONFIG_ACPI */

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 08/13] Bluetooth: hci_bcm: Add callbacks to toggle GPIOs

[Lukas Wunner]

MacBooks provides custom ACPI methods to toggle the GPIOs for device
wake and shutdown instead of accessing the pins directly.  Prepare for
their support by adding callbacks to toggle the GPIOs, which on non-Macs
do nothing more but call gpiod_set_value().

No functional change intended.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 888dcec56bc2..81eb6ac80de6 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -63,6 +63,8 @@
  *	deassert = Bluetooth device may sleep when sleep criteria are met
  * @shutdown: BT_REG_ON pin,
  *	power up or power down Bluetooth device internal regulators
+ * @set_device_wakeup: callback to toggle BT_WAKE pin
+ * @set_shutdown: callback to toggle BT_REG_ON pin
  * @clk: clock used by Bluetooth device
  * @clk_enabled: whether @clk is prepared and enabled
  * @init_speed: default baudrate of Bluetooth device;
@@ -86,6 +88,8 @@ struct bcm_device {
 	const char		*name;
 	struct gpio_desc	*device_wakeup;
 	struct gpio_desc	*shutdown;
+	int			(*set_device_wakeup)(struct bcm_device *, bool);
+	int			(*set_shutdown)(struct bcm_device *, bool);
 
 	struct clk		*clk;
 	bool			clk_enabled;
@@ -196,8 +200,8 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
 		clk_prepare_enable(dev->clk);
 
-	gpiod_set_value(dev->shutdown, powered);
-	gpiod_set_value(dev->device_wakeup, powered);
+	dev->set_shutdown(dev, powered);
+	dev->set_device_wakeup(dev, powered);
 
 	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
 		clk_disable_unprepare(dev->clk);
@@ -597,7 +601,7 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	gpiod_set_value(bdev->device_wakeup, false);
+	bdev->set_device_wakeup(bdev, false);
 	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
 	mdelay(15);
 
@@ -610,7 +614,7 @@ static int bcm_resume_device(struct device *dev)
 
 	bt_dev_dbg(bdev, "");
 
-	gpiod_set_value(bdev->device_wakeup, true);
+	bdev->set_device_wakeup(bdev, true);
 	bt_dev_dbg(bdev, "resume, delaying 15 ms");
 	mdelay(15);
 
@@ -789,6 +793,18 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 }
 #endif /* CONFIG_ACPI */
 
+static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
+{
+	gpiod_set_value(dev->device_wakeup, awake);
+	return 0;
+}
+
+static int bcm_gpio_set_shutdown(struct bcm_device *dev, bool powered)
+{
+	gpiod_set_value(dev->shutdown, powered);
+	return 0;
+}
+
 static int bcm_get_resources(struct bcm_device *dev)
 {
 	dev->name = dev_name(dev->dev);
@@ -804,6 +820,9 @@ static int bcm_get_resources(struct bcm_device *dev)
 	if (IS_ERR(dev->shutdown))
 		return PTR_ERR(dev->shutdown);
 
+	dev->set_device_wakeup = bcm_gpio_set_device_wakeup;
+	dev->set_shutdown = bcm_gpio_set_shutdown;
+
 	/* IRQ can be declared in ACPI table as Interrupt or GpioInt */
 	if (dev->irq <= 0) {
 		struct gpio_desc *gpio;
-- 
2.15.1

[PATCH v3 03/13] Bluetooth: hci_bcm: Clean up unnecessary #ifdef

[Lukas Wunner]

pm_runtime_disable() and pm_runtime_set_suspended() are replaced with
empty inlines if CONFIG_PM is disabled, so there's no need to #ifdef
them.

device_init_wakeup() is likewise replaced with an inline, though it's
not empty, but it and devm_free_irq() can be made conditional on
IS_ENABLED(CONFIG_PM), which is preferable to #ifdef as per section 20
of Documentation/process/coding-style.rst.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index d9f71ed26667..c551ef4c350f 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -373,15 +373,13 @@ static int bcm_close(struct hci_uart *hu)
 
 	if (bdev) {
 		bcm_gpio_set_power(bdev, false);
-#ifdef CONFIG_PM
 		pm_runtime_disable(bdev->dev);
 		pm_runtime_set_suspended(bdev->dev);
 
-		if (bdev->irq > 0) {
+		if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
-#endif
 	}
 	mutex_unlock(&bcm_device_lock);
 
-- 
2.15.1

[PATCH v3 01/13] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO

[Lukas Wunner]

Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
amended this driver to request a shutdown and device wake GPIO on probe,
but mandated that only one of them need to be present:

	/* Make sure at-least one of the GPIO is defined and that
	 * a name is specified for this instance
	 */
	if ((!dev->device_wakeup && !dev->shutdown) || !dev->name) {
		dev_err(&pdev->dev, "invalid platform data\n");
		return -EINVAL;
	}

However the same commit added a call to bcm_gpio_set_power() to the
->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
the resulting NULL pointer deref was never reported, suggesting there's
no machine where either GPIO is missing.

Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
serdev driver") removed the check whether at least one of the GPIOs is
present without specifying a reason.

Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
API") refactored the driver to use devm_gpiod_get_optional() instead of
devm_gpiod_get(), one is now tempted to believe that the driver doesn't
require *any* of the two GPIOs.

Which is wrong, the driver still requires both GPIOs to avoid a NULL
pointer deref.  To this end, establish the status quo ante and request
the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
of them is missing.

Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
unconditionally, bcm_suspend_device() and bcm_resume_device() do check
for its presence before accessing it.  Those checks are superfluous,
so remove them.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 1fc604a0d870..7d059b35c63a 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -572,11 +572,9 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, false);
-		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-		mdelay(15);
-	}
+	gpiod_set_value(bdev->device_wakeup, false);
+	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
+	mdelay(15);
 
 	return 0;
 }
@@ -587,11 +585,9 @@ static int bcm_resume_device(struct device *dev)
 
 	bt_dev_dbg(bdev, "");
 
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, true);
-		bt_dev_dbg(bdev, "resume, delaying 15 ms");
-		mdelay(15);
-	}
+	gpiod_set_value(bdev->device_wakeup, true);
+	bt_dev_dbg(bdev, "resume, delaying 15 ms");
+	mdelay(15);
 
 	/* When this executes, the device has woken up already */
 	if (bdev->is_suspended && bdev->hu) {
@@ -774,14 +770,12 @@ static int bcm_get_resources(struct bcm_device *dev)
 
 	dev->clk = devm_clk_get(dev->dev, NULL);
 
-	dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
-						     "device-wakeup",
-						     GPIOD_OUT_LOW);
+	dev->device_wakeup = devm_gpiod_get(dev->dev, "device-wakeup",
+					    GPIOD_OUT_LOW);
 	if (IS_ERR(dev->device_wakeup))
 		return PTR_ERR(dev->device_wakeup);
 
-	dev->shutdown = devm_gpiod_get_optional(dev->dev, "shutdown",
-						GPIOD_OUT_LOW);
+	dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
 	if (IS_ERR(dev->shutdown))
 		return PTR_ERR(dev->shutdown);
 
-- 
2.15.1

Re: [PATCH v3 00/13] Bluetooth on 2015+ MacBook (Pro)

[Andy Shevchenko]

On Sun, 2018-01-07 at 20:16 +0100, Lukas Wunner wrote:
> Enable UART-attached Bluetooth on 2015+ Macs, v3.
> 
> The series has grown to 13 patches because bugs in the driver keep
> cropping up.
> 
> Changes since v2:
> 
> - In patch [01/13] ("Mandate presence of shutdown and device wake
> GPIO"):
>   Keep netdev indentation style. (Marcel)
> 
> - New patch [04/13] to fix an IRQ race on ->close.
> 
> - New patch [05/13] to fix an unbalanced pm_runtime_disable(). (Andy)
> 
> - New patch [06/13] to invalidate the IRQ if requesting it failed.
> 
> - New patch [08/13] to add callbacks to toggle GPIOs.
>   This makes the subsequent addition of Mac support less intrusive.
>   Replaces patch "Add helper to toggle device wake GPIO" in v2. (Andy)
> 
> - In patch [09/13] ("Handle errors properly"):
>   Drop redundant assignment. (Andy)
> 
> - In patch [10/13] ("Support Apple GPIO handling"):
>   Don't enable runtime PM on Macs for lack of usable host wake IRQ
> (Hans),
>   s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)
> 
> - Move patch "Silence IRQ printk" to end of series as it's merely
>   a cleanup and no longer necessary for Mac support.
> 
> - New patch [12/13] to use msleep() instead of mdelay()
>   after toggling device wake pin. (Andy)
> 
> - New patch [13/13] to fix sleep mode struct ordering.
> 
> - Drop incorrect patch "Enable runtime PM despite absence of IRQ".
> (Hans)
> 

I didn't test this myself (yet), the code looks more or less fine to me,
 FWIW, 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Link to v2:
> https://marc.info/?l=linux-bluetooth&m=151492012812029&w=4
> 
> Thanks,
> 
> Lukas
> 
> 
> Lukas Wunner (12):
>   Bluetooth: hci_bcm: Mandate presence of shutdown and device wake
> GPIO
>   Bluetooth: hci_bcm: Clean up unnecessary #ifdef
>   Bluetooth: hci_bcm: Fix race on close
>   Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()
>   Bluetooth: hci_bcm: Invalidate IRQ on request failure
>   Bluetooth: hci_bcm: Document struct bcm_device
>   Bluetooth: hci_bcm: Add callbacks to toggle GPIOs
>   Bluetooth: hci_bcm: Handle errors properly
>   Bluetooth: hci_bcm: Support Apple GPIO handling
>   Bluetooth: hci_bcm: Silence IRQ printk
>   Bluetooth: hci_bcm: Sleep instead of spinning
>   Bluetooth: btbcm: Fix sleep mode struct ordering
> 
> Ronald Tschalär (1):
>   Bluetooth: hci_bcm: Validate IRQ before using it
> 
>  drivers/bluetooth/btbcm.h   |   2 +-
>  drivers/bluetooth/hci_bcm.c | 227
> ++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 189 insertions(+), 40 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 00/13] Bluetooth on 2015+ MacBook (Pro)

[Marcel Holtmann]

Hi Lukas,

>> Enable UART-attached Bluetooth on 2015+ Macs, v3.
>> 
>> The series has grown to 13 patches because bugs in the driver keep
>> cropping up.
>> 
>> Changes since v2:
>> 
>> - In patch [01/13] ("Mandate presence of shutdown and device wake
>> GPIO"):
>>  Keep netdev indentation style. (Marcel)
>> 
>> - New patch [04/13] to fix an IRQ race on ->close.
>> 
>> - New patch [05/13] to fix an unbalanced pm_runtime_disable(). (Andy)
>> 
>> - New patch [06/13] to invalidate the IRQ if requesting it failed.
>> 
>> - New patch [08/13] to add callbacks to toggle GPIOs.
>>  This makes the subsequent addition of Mac support less intrusive.
>>  Replaces patch "Add helper to toggle device wake GPIO" in v2. (Andy)
>> 
>> - In patch [09/13] ("Handle errors properly"):
>>  Drop redundant assignment. (Andy)
>> 
>> - In patch [10/13] ("Support Apple GPIO handling"):
>>  Don't enable runtime PM on Macs for lack of usable host wake IRQ
>> (Hans),
>>  s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)
>> 
>> - Move patch "Silence IRQ printk" to end of series as it's merely
>>  a cleanup and no longer necessary for Mac support.
>> 
>> - New patch [12/13] to use msleep() instead of mdelay()
>>  after toggling device wake pin. (Andy)
>> 
>> - New patch [13/13] to fix sleep mode struct ordering.
>> 
>> - Drop incorrect patch "Enable runtime PM despite absence of IRQ".
>> (Hans)
>> 
> 
> I didn't test this myself (yet), the code looks more or less fine to me,
> FWIW, 
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

can we get a v4 with the known fixes?

Regards

Marcel