[PATCH v2 0/4] Bluetooth: hci_bcm: Add ACPI and PM support

[PATCH v2 0/4] Bluetooth: hci_bcm: Add ACPI and PM support

[Frederic Danis]

These patches move PM support from rfkill-gpio to hci_uart module.
On, off, suspend and resume functions are supported.
They also parse the ACPI entry to find initial UART baud rate.

v1->v2:
- Add BCM2E67 device in acpi_device_id table
- Add suspend/resume functions

Frederic Danis (4):
  Bluetooth: hci_bcm: Add PM for BCM devices
  net: rfkill: gpio: Make BCM2E39 support optional
  Bluetooth: hci_bcm: Retrieve UART speed from ACPI
  Bluetooth: hci_bcm: Add suspend/resume PM functions

 drivers/bluetooth/hci_bcm.c | 279 +++++++++++++++++++++++++++++++++++++++++++-
 net/rfkill/rfkill-gpio.c    |   2 +
 2 files changed, 279 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] Bluetooth: hci_bcm: Add PM for BCM devices

[Frederic Danis]

Retrieve "shutdown" and "device_wakeup" GPIOs from ACPI.
Set device off during platform device enumeration.
Set device on only when attached.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 192 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 190 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 23523e1..fbe1cab 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -25,6 +25,12 @@
 #include <linux/errno.h>
 #include <linux/skbuff.h>
 #include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/tty.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -32,11 +38,30 @@
 #include "btbcm.h"
 #include "hci_uart.h"
 
+struct bcm_device {
+	struct list_head	list;
+
+	struct platform_device	*pdev;
+
+	const char		*name;
+	struct gpio_desc	*device_wakeup;
+	struct gpio_desc	*shutdown;
+
+	struct clk		*clk;
+	bool			clk_enabled;
+};
+
 struct bcm_data {
-	struct sk_buff *rx_skb;
-	struct sk_buff_head txq;
+	struct sk_buff		*rx_skb;
+	struct sk_buff_head	txq;
+
+	struct bcm_device	*dev;
 };
 
+/* List of BCM BT UART devices */
+static DEFINE_SPINLOCK(device_list_lock);
+static LIST_HEAD(device_list);
+
 static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -86,9 +111,26 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
 	return 0;
 }
 
+static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
+{
+	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
+		clk_enable(dev->clk);
+
+	gpiod_set_value_cansleep(dev->shutdown, powered);
+	gpiod_set_value_cansleep(dev->device_wakeup, powered);
+
+	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
+		clk_disable(dev->clk);
+
+	dev->clk_enabled = powered;
+
+	return 0;
+}
+
 static int bcm_open(struct hci_uart *hu)
 {
 	struct bcm_data *bcm;
+	struct list_head *ptr;
 
 	BT_DBG("hu %p", hu);
 
@@ -99,6 +141,22 @@ static int bcm_open(struct hci_uart *hu)
 	skb_queue_head_init(&bcm->txq);
 
 	hu->priv = bcm;
+
+	spin_lock(&device_list_lock);
+	list_for_each(ptr, &device_list) {
+		struct bcm_device *dev;
+
+		dev = list_entry(ptr, struct bcm_device, list);
+		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
+			bcm->dev = dev;
+			break;
+		}
+	}
+	spin_unlock(&device_list_lock);
+
+	if (bcm->dev)
+		bcm_gpio_set_power(bcm->dev, true);
+
 	return 0;
 }
 
@@ -108,6 +166,9 @@ static int bcm_close(struct hci_uart *hu)
 
 	BT_DBG("hu %p", hu);
 
+	if (bcm->dev)
+		bcm_gpio_set_power(bcm->dev, false);
+
 	skb_queue_purge(&bcm->txq);
 	kfree_skb(bcm->rx_skb);
 	kfree(bcm);
@@ -232,6 +293,111 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 	return skb_dequeue(&bcm->txq);
 }
 
+static const struct acpi_gpio_params device_wakeup_gpios = { 0, 0, false };
+static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
+
+static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
+	{ "device-wakeup-gpios", &device_wakeup_gpios, 1 },
+	{ "shutdown-gpios", &shutdown_gpios, 1 },
+	{ },
+};
+
+static int bcm_acpi_probe(struct bcm_device *dev)
+{
+	struct platform_device *pdev = dev->pdev;
+	const struct acpi_device_id *id;
+	struct gpio_desc *gpio;
+	int ret;
+
+	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	/* Retrieve GPIO data */
+	dev->name = dev_name(&pdev->dev);
+	ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev),
+					acpi_bcm_default_gpios);
+	if (ret)
+		return ret;
+
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+
+	gpio = devm_gpiod_get(&pdev->dev, "device-wakeup");
+	if (!IS_ERR(gpio)) {
+		ret = gpiod_direction_output(gpio, 0);
+		if (ret)
+			return ret;
+		dev->device_wakeup = gpio;
+	}
+
+	gpio = devm_gpiod_get(&pdev->dev, "shutdown");
+	if (!IS_ERR(gpio)) {
+		ret = gpiod_direction_output(gpio, 0);
+		if (ret)
+			return ret;
+		dev->shutdown = gpio;
+	}
+
+	/* 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;
+	}
+
+	return 0;
+}
+
+static int bcm_probe(struct platform_device *pdev)
+{
+	struct bcm_device *dev;
+	struct acpi_device_id *pdata = pdev->dev.platform_data;
+	int ret;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->pdev = pdev;
+
+	if (ACPI_HANDLE(&pdev->dev)) {
+		ret = bcm_acpi_probe(dev);
+		if (ret)
+			return ret;
+	} else if (pdata) {
+		dev->name = pdata->id;
+	} else {
+		return -ENODEV;
+	}
+
+	platform_set_drvdata(pdev, dev);
+
+	dev_info(&pdev->dev, "%s device registered.\n", dev->name);
+
+	/* Place this instance on the device list */
+	spin_lock(&device_list_lock);
+	list_add_tail(&dev->list, &device_list);
+	spin_unlock(&device_list_lock);
+
+	bcm_gpio_set_power(dev, false);
+
+	return 0;
+}
+
+static int bcm_remove(struct platform_device *pdev)
+{
+	struct bcm_device *dev = platform_get_drvdata(pdev);
+
+	spin_lock(&device_list_lock);
+	list_del(&dev->list);
+	spin_unlock(&device_list_lock);
+
+	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
+
+	return 0;
+}
+
 static const struct hci_uart_proto bcm_proto = {
 	.id		= HCI_UART_BCM,
 	.name		= "BCM",
@@ -247,12 +413,34 @@ static const struct hci_uart_proto bcm_proto = {
 	.dequeue	= bcm_dequeue,
 };
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id bcm_acpi_match[] = {
+	{ "BCM2E39", 0 },
+	{ "BCM2E67", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
+#endif
+
+static struct platform_driver bcm_driver = {
+	.probe = bcm_probe,
+	.remove = bcm_remove,
+	.driver = {
+		.name = "hci_bcm",
+		.acpi_match_table = ACPI_PTR(bcm_acpi_match),
+	},
+};
+
 int __init bcm_init(void)
 {
+	platform_driver_register(&bcm_driver);
+
 	return hci_uart_register_proto(&bcm_proto);
 }
 
 int __exit bcm_deinit(void)
 {
+	platform_driver_unregister(&bcm_driver);
+
 	return hci_uart_unregister_proto(&bcm_proto);
 }
-- 
1.9.1

Re: [PATCH v2 1/4] Bluetooth: hci_bcm: Add PM for BCM devices

[Marcel Holtmann]

Hi Fred,

> Retrieve "shutdown" and "device_wakeup" GPIOs from ACPI.
> Set device off during platform device enumeration.
> Set device on only when attached.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 192 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 190 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 23523e1..fbe1cab 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -25,6 +25,12 @@
> #include <linux/errno.h>
> #include <linux/skbuff.h>
> #include <linux/firmware.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/tty.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -32,11 +38,30 @@
> #include "btbcm.h"
> #include "hci_uart.h"
> 
> +struct bcm_device {
> +	struct list_head	list;
> +
> +	struct platform_device	*pdev;
> +
> +	const char		*name;
> +	struct gpio_desc	*device_wakeup;
> +	struct gpio_desc	*shutdown;
> +
> +	struct clk		*clk;
> +	bool			clk_enabled;
> +};
> +
> struct bcm_data {
> -	struct sk_buff *rx_skb;
> -	struct sk_buff_head txq;
> +	struct sk_buff		*rx_skb;
> +	struct sk_buff_head	txq;
> +
> +	struct bcm_device	*dev;
> };
> 
> +/* List of BCM BT UART devices */
> +static DEFINE_SPINLOCK(device_list_lock);
> +static LIST_HEAD(device_list);
> +
> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> 	struct hci_dev *hdev = hu->hdev;
> @@ -86,9 +111,26 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
> 	return 0;
> }
> 
> +static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
> +{
> +	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
> +		clk_enable(dev->clk);
> +
> +	gpiod_set_value_cansleep(dev->shutdown, powered);
> +	gpiod_set_value_cansleep(dev->device_wakeup, powered);
> +
> +	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
> +		clk_disable(dev->clk);
> +
> +	dev->clk_enabled = powered;
> +
> +	return 0;
> +}
> +
> static int bcm_open(struct hci_uart *hu)
> {
> 	struct bcm_data *bcm;
> +	struct list_head *ptr;

Lets just use p here instead of ptr. We have either used p as short enumeration variable (similar to i and n) in other parts of the Bluetooth subsystem.

> 
> 	BT_DBG("hu %p", hu);
> 
> @@ -99,6 +141,22 @@ static int bcm_open(struct hci_uart *hu)
> 	skb_queue_head_init(&bcm->txq);
> 
> 	hu->priv = bcm;
> +
> +	spin_lock(&device_list_lock);
> +	list_for_each(ptr, &device_list) {
> +		struct bcm_device *dev;
> +
> +		dev = list_entry(ptr, struct bcm_device, list);

If it fits into a single line, assign struct bcm_device *dev = list_entry directly.

If you only need to get a few extra characters we could even shorten it to struct bcm_dev. If we do that then obvious bcm_dev_list_lock change has to happen as well. If it fits in a single line, then leave it as is.

> +		if (hu->tty->dev->parent == dev->pdev->dev.parent) {

I would add a comment above the if statement explaining what we are looking for here. Also please double check that we do not have to take any extra locks before accessing both struct device.

> +			bcm->dev = dev;
> +			break;
> +		}
> +	}
> +	spin_unlock(&device_list_lock);
> +
> +	if (bcm->dev)
> +		bcm_gpio_set_power(bcm->dev, true);
> +

I think yo need to protect bcm->dev with at least the list lock or a mutex. If the platform device or driver gets removed mid way, then the bcm->dev value is no longer valid.

> 	return 0;
> }
> 
> @@ -108,6 +166,9 @@ static int bcm_close(struct hci_uart *hu)
> 
> 	BT_DBG("hu %p", hu);
> 
> +	if (bcm->dev)
> +		bcm_gpio_set_power(bcm->dev, false);
> +

Same as above, I think we need to protect bcm->dev against removal of the device or driver.

In case this is not an issue, then I would prefer we add a comment why not. So when we ever change anything, we remember to look at this again.

> 	skb_queue_purge(&bcm->txq);
> 	kfree_skb(bcm->rx_skb);
> 	kfree(bcm);
> @@ -232,6 +293,111 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
> 	return skb_dequeue(&bcm->txq);
> }
> 
> +static const struct acpi_gpio_params device_wakeup_gpios = { 0, 0, false };
> +static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
> +
> +static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
> +	{ "device-wakeup-gpios", &device_wakeup_gpios, 1 },
> +	{ "shutdown-gpios", &shutdown_gpios, 1 },
> +	{ },
> +};
> +
> +static int bcm_acpi_probe(struct bcm_device *dev)
> +{
> +	struct platform_device *pdev = dev->pdev;
> +	const struct acpi_device_id *id;
> +	struct gpio_desc *gpio;
> +	int ret;
> +
> +	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	/* Retrieve GPIO data */
> +	dev->name = dev_name(&pdev->dev);
> +	ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev),
> +					acpi_bcm_default_gpios);
> +	if (ret)
> +		return ret;
> +
> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
> +
> +	gpio = devm_gpiod_get(&pdev->dev, "device-wakeup");
> +	if (!IS_ERR(gpio)) {
> +		ret = gpiod_direction_output(gpio, 0);
> +		if (ret)
> +			return ret;
> +		dev->device_wakeup = gpio;
> +	}
> +
> +	gpio = devm_gpiod_get(&pdev->dev, "shutdown");
> +	if (!IS_ERR(gpio)) {
> +		ret = gpiod_direction_output(gpio, 0);
> +		if (ret)
> +			return ret;
> +		dev->shutdown = gpio;
> +	}
> +
> +	/* 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;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bcm_probe(struct platform_device *pdev)
> +{
> +	struct bcm_device *dev;
> +	struct acpi_device_id *pdata = pdev->dev.platform_data;
> +	int ret;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->pdev = pdev;
> +
> +	if (ACPI_HANDLE(&pdev->dev)) {
> +		ret = bcm_acpi_probe(dev);
> +		if (ret)
> +			return ret;
> +	} else if (pdata) {
> +		dev->name = pdata->id;
> +	} else {
> +		return -ENODEV;

Am I assuming correctly that devm_kzalloc allocated memory gets automatically cleaned up when probe fails?

> +	}
> +
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev_info(&pdev->dev, "%s device registered.\n", dev->name);
> +
> +	/* Place this instance on the device list */
> +	spin_lock(&device_list_lock);
> +	list_add_tail(&dev->list, &device_list);
> +	spin_unlock(&device_list_lock);
> +
> +	bcm_gpio_set_power(dev, false);
> +
> +	return 0;
> +}
> +
> +static int bcm_remove(struct platform_device *pdev)
> +{
> +	struct bcm_device *dev = platform_get_drvdata(pdev);
> +
> +	spin_lock(&device_list_lock);
> +	list_del(&dev->list);
> +	spin_unlock(&device_list_lock);

I think here we need to check to clean bcm->dev reference if one exists. Do we?

Another option might be to just enforce holding a reference of the platform device when the line discipline is active. So that the device model makes sure we are not accessing invalid memory. I don't know if this is possible and valid operation, but it sounds like the right thing to do here.

> +
> +	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
> +
> +	return 0;
> +}
> +
> static const struct hci_uart_proto bcm_proto = {
> 	.id		= HCI_UART_BCM,
> 	.name		= "BCM",
> @@ -247,12 +413,34 @@ static const struct hci_uart_proto bcm_proto = {
> 	.dequeue	= bcm_dequeue,
> };
> 
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id bcm_acpi_match[] = {
> +	{ "BCM2E39", 0 },
> +	{ "BCM2E67", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
> +#endif
> +
> +static struct platform_driver bcm_driver = {
> +	.probe = bcm_probe,
> +	.remove = bcm_remove,
> +	.driver = {
> +		.name = "hci_bcm",
> +		.acpi_match_table = ACPI_PTR(bcm_acpi_match),
> +	},
> +};
> +
> int __init bcm_init(void)
> {
> +	platform_driver_register(&bcm_driver);
> +
> 	return hci_uart_register_proto(&bcm_proto);
> }
> 
> int __exit bcm_deinit(void)
> {
> +	platform_driver_unregister(&bcm_driver);
> +
> 	return hci_uart_unregister_proto(&bcm_proto);
> }

Regards

Marcel

Re: [PATCH v2 1/4] Bluetooth: hci_bcm: Add PM for BCM devices

[Frederic Danis]

Hello Marcel,

On 07/08/2015 18:41, Marcel Holtmann wrote:
> Hi Fred,
>
>> Retrieve "shutdown" and "device_wakeup" GPIOs from ACPI.
>> Set device off during platform device enumeration.
>> Set device on only when attached.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 192 +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 190 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 23523e1..fbe1cab 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -25,6 +25,12 @@
>> #include <linux/errno.h>
>> #include <linux/skbuff.h>
>> #include <linux/firmware.h>
>> +#include <linux/module.h>
>> +#include <linux/acpi.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/tty.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -32,11 +38,30 @@
>> #include "btbcm.h"
>> #include "hci_uart.h"
>>
>> +struct bcm_device {
>> +	struct list_head	list;
>> +
>> +	struct platform_device	*pdev;
>> +
>> +	const char		*name;
>> +	struct gpio_desc	*device_wakeup;
>> +	struct gpio_desc	*shutdown;
>> +
>> +	struct clk		*clk;
>> +	bool			clk_enabled;
>> +};
>> +
>> struct bcm_data {
>> -	struct sk_buff *rx_skb;
>> -	struct sk_buff_head txq;
>> +	struct sk_buff		*rx_skb;
>> +	struct sk_buff_head	txq;
>> +
>> +	struct bcm_device	*dev;
>> };
>>
>> +/* List of BCM BT UART devices */
>> +static DEFINE_SPINLOCK(device_list_lock);
>> +static LIST_HEAD(device_list);
>> +
>> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> {
>> 	struct hci_dev *hdev = hu->hdev;
>> @@ -86,9 +111,26 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> 	return 0;
>> }
>>
>> +static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
>> +{
>> +	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
>> +		clk_enable(dev->clk);
>> +
>> +	gpiod_set_value_cansleep(dev->shutdown, powered);
>> +	gpiod_set_value_cansleep(dev->device_wakeup, powered);
>> +
>> +	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
>> +		clk_disable(dev->clk);
>> +
>> +	dev->clk_enabled = powered;
>> +
>> +	return 0;
>> +}
>> +
>> static int bcm_open(struct hci_uart *hu)
>> {
>> 	struct bcm_data *bcm;
>> +	struct list_head *ptr;
>
> Lets just use p here instead of ptr. We have either used p as short enumeration variable (similar to i and n) in other parts of the Bluetooth subsystem.

OK

>>
>> 	BT_DBG("hu %p", hu);
>>
>> @@ -99,6 +141,22 @@ static int bcm_open(struct hci_uart *hu)
>> 	skb_queue_head_init(&bcm->txq);
>>
>> 	hu->priv = bcm;
>> +
>> +	spin_lock(&device_list_lock);
>> +	list_for_each(ptr, &device_list) {
>> +		struct bcm_device *dev;
>> +
>> +		dev = list_entry(ptr, struct bcm_device, list);
>
> If it fits into a single line, assign struct bcm_device *dev = list_entry directly.
>
> If you only need to get a few extra characters we could even shorten it to struct bcm_dev. If we do that then obvious bcm_dev_list_lock change has to happen as well. If it fits in a single line, then leave it as is.

OK

>> +		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
>
> I would add a comment above the if statement explaining what we are looking for here. Also please double check that we do not have to take any extra locks before accessing both struct device.

OK, I will add an explanation.
I do not found extra locks.

>> +			bcm->dev = dev;
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock(&device_list_lock);
>> +
>> +	if (bcm->dev)
>> +		bcm_gpio_set_power(bcm->dev, true);
>> +
>
> I think yo need to protect bcm->dev with at least the list lock or a mutex. If the platform device or driver gets removed mid way, then the bcm->dev value is no longer valid.

Yes, I will check if bcm_device is still in device_list and protect it 
with device_list_lock before calling bcm_gpio_set_power.

>> 	return 0;
>> }
>>
>> @@ -108,6 +166,9 @@ static int bcm_close(struct hci_uart *hu)
>>
>> 	BT_DBG("hu %p", hu);
>>
>> +	if (bcm->dev)
>> +		bcm_gpio_set_power(bcm->dev, false);
>> +
>
> Same as above, I think we need to protect bcm->dev against removal of the device or driver.

OK

> In case this is not an issue, then I would prefer we add a comment why not. So when we ever change anything, we remember to look at this again.
>
>> 	skb_queue_purge(&bcm->txq);
>> 	kfree_skb(bcm->rx_skb);
>> 	kfree(bcm);
>> @@ -232,6 +293,111 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
>> 	return skb_dequeue(&bcm->txq);
>> }
>>
>> +static const struct acpi_gpio_params device_wakeup_gpios = { 0, 0, false };
>> +static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
>> +
>> +static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
>> +	{ "device-wakeup-gpios", &device_wakeup_gpios, 1 },
>> +	{ "shutdown-gpios", &shutdown_gpios, 1 },
>> +	{ },
>> +};
>> +
>> +static int bcm_acpi_probe(struct bcm_device *dev)
>> +{
>> +	struct platform_device *pdev = dev->pdev;
>> +	const struct acpi_device_id *id;
>> +	struct gpio_desc *gpio;
>> +	int ret;
>> +
>> +	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
>> +	if (!id)
>> +		return -ENODEV;
>> +
>> +	/* Retrieve GPIO data */
>> +	dev->name = dev_name(&pdev->dev);
>> +	ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev),
>> +					acpi_bcm_default_gpios);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
>> +
>> +	gpio = devm_gpiod_get(&pdev->dev, "device-wakeup");
>> +	if (!IS_ERR(gpio)) {
>> +		ret = gpiod_direction_output(gpio, 0);
>> +		if (ret)
>> +			return ret;
>> +		dev->device_wakeup = gpio;
>> +	}
>> +
>> +	gpio = devm_gpiod_get(&pdev->dev, "shutdown");
>> +	if (!IS_ERR(gpio)) {
>> +		ret = gpiod_direction_output(gpio, 0);
>> +		if (ret)
>> +			return ret;
>> +		dev->shutdown = gpio;
>> +	}
>> +
>> +	/* 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;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int bcm_probe(struct platform_device *pdev)
>> +{
>> +	struct bcm_device *dev;
>> +	struct acpi_device_id *pdata = pdev->dev.platform_data;
>> +	int ret;
>> +
>> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> +	if (!dev)
>> +		return -ENOMEM;
>> +
>> +	dev->pdev = pdev;
>> +
>> +	if (ACPI_HANDLE(&pdev->dev)) {
>> +		ret = bcm_acpi_probe(dev);
>> +		if (ret)
>> +			return ret;
>> +	} else if (pdata) {
>> +		dev->name = pdata->id;
>> +	} else {
>> +		return -ENODEV;
>
> Am I assuming correctly that devm_kzalloc allocated memory gets automatically cleaned up when probe fails?

Yes, you're right

>> +	}
>> +
>> +	platform_set_drvdata(pdev, dev);
>> +
>> +	dev_info(&pdev->dev, "%s device registered.\n", dev->name);
>> +
>> +	/* Place this instance on the device list */
>> +	spin_lock(&device_list_lock);
>> +	list_add_tail(&dev->list, &device_list);
>> +	spin_unlock(&device_list_lock);
>> +
>> +	bcm_gpio_set_power(dev, false);
>> +
>> +	return 0;
>> +}
>> +
>> +static int bcm_remove(struct platform_device *pdev)
>> +{
>> +	struct bcm_device *dev = platform_get_drvdata(pdev);
>> +
>> +	spin_lock(&device_list_lock);
>> +	list_del(&dev->list);
>> +	spin_unlock(&device_list_lock);
>
> I think here we need to check to clean bcm->dev reference if one exists. Do we?
>
> Another option might be to just enforce holding a reference of the platform device when the line discipline is active. So that the device model makes sure we are not accessing invalid memory. I don't know if this is possible and valid operation, but it sounds like the right thing to do here.

I do not found a way to hold a reference of the platform device.

Regards

Fred

Re: [PATCH v2 1/4] Bluetooth: hci_bcm: Add PM for BCM devices

[Luka Karinja]

Hey Frederic
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id bcm_acpi_match[] = {
> +    { "BCM2E39", 0 },
> +    { "BCM2E67", 0 },
> +    { },
> +};
could you also add:

+     { "BCM2E65", 0 },

This is the ACPI name the Asus T100TAF uses
Thanks

Re: [PATCH v2 1/4] Bluetooth: hci_bcm: Add PM for BCM devices

[Frederic Danis]

Hello Luka,

On 10/08/2015 10:00, Luka Karinja wrote:
> Hey Frederic
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id bcm_acpi_match[] = {
>> +    { "BCM2E39", 0 },
>> +    { "BCM2E67", 0 },
>> +    { },
>> +};
> could you also add:
>
> +     { "BCM2E65", 0 },
>
> This is the ACPI name the Asus T100TAF uses

In addition to this, bcm_uart_subver_table, btbcm_initialize() and 
btbcm_check_bdaddr() may need to be patched to support new device.
All is located in btbcm.c file.

Regards

Fred

Re: [PATCH v2 1/4] Bluetooth: hci_bcm: Add PM for BCM devices

[Luka Karinja]

Hey Frederic.

> In addition to this, bcm_uart_subver_table, btbcm_initialize() and 
> btbcm_check_bdaddr() may need to be patched to support new device.
> All is located in btbcm.c file.
>

Could you give me some pointers of how to find those values for my 
device? the chip is a BCM43341

Thanks

Re: [PATCH v2 1/4] Bluetooth: hci_bcm: Add PM for BCM devices

[Frederic Danis]

Hello Luka,

On 23/08/2015 12:44, Luka Karinja wrote:
> Hey Frederic.
>
>> In addition to this, bcm_uart_subver_table, btbcm_initialize() and
>> btbcm_check_bdaddr() may need to be patched to support new device.
>> All is located in btbcm.c file.
>>
>
> Could you give me some pointers of how to find those values for my
> device? the chip is a BCM43341

Regarding bcm_uart_subver_table, you can retrieve Subversion value by 
performing "hciconfig -a".

For btbcm_initialize(), you should check Revision displayed by 
hciconfig. You may have to add a "case" to "switch ((rev & 0xf000) >> 12)".

After this, you should be able to use btattach with your chipset.

If you have a firmware for the chipset, you could also check if it uses 
a default device address and add it to btbcm_check_bdaddr().

Regards

Fred

[PATCH v2 2/4] net: rfkill: gpio: Make BCM2E39 support optional

[Frederic Danis]

In case of kernel configured to build Bluetooth BCM UART driver, do not
support BCM2E39 in rfkill-gpio.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 net/rfkill/rfkill-gpio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index d5d58d9..16d7a3a 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -164,7 +164,9 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id rfkill_acpi_match[] = {
 	{ "BCM2E1A", RFKILL_TYPE_BLUETOOTH },
+#ifndef CONFIG_BT_HCIUART_BCM
 	{ "BCM2E39", RFKILL_TYPE_BLUETOOTH },
+#endif
 	{ "BCM2E3D", RFKILL_TYPE_BLUETOOTH },
 	{ "BCM2E40", RFKILL_TYPE_BLUETOOTH },
 	{ "BCM2E64", RFKILL_TYPE_BLUETOOTH },
-- 
1.9.1

Re: [PATCH v2 2/4] net: rfkill: gpio: Make BCM2E39 support optional

[Marcel Holtmann]

Hi Fred,

> In case of kernel configured to build Bluetooth BCM UART driver, do not
> support BCM2E39 in rfkill-gpio.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> net/rfkill/rfkill-gpio.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
> index d5d58d9..16d7a3a 100644
> --- a/net/rfkill/rfkill-gpio.c
> +++ b/net/rfkill/rfkill-gpio.c
> @@ -164,7 +164,9 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id rfkill_acpi_match[] = {
> 	{ "BCM2E1A", RFKILL_TYPE_BLUETOOTH },
> +#ifndef CONFIG_BT_HCIUART_BCM
> 	{ "BCM2E39", RFKILL_TYPE_BLUETOOTH },
> +#endif

since we are supporting the Broadcom device properly now, lets just remove that entry here. No need to make it optional.

Regards

Marcel

[PATCH v2 3/4] Bluetooth: hci_bcm: Retrieve UART speed from ACPI

[Frederic Danis]

Parse platform_device's ACPI to retrieve UART init speed.
When BCM device is open, check if its TTY has same parent as one of the
platform devices saved. If yes, use platform_device's init speed.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index fbe1cab..8e1eacd 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -49,6 +49,8 @@ struct bcm_device {
 
 	struct clk		*clk;
 	bool			clk_enabled;
+
+	u32			init_speed;
 };
 
 struct bcm_data {
@@ -149,6 +151,7 @@ static int bcm_open(struct hci_uart *hu)
 		dev = list_entry(ptr, struct bcm_device, list);
 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
 			bcm->dev = dev;
+			hu->init_speed = dev->init_speed;
 			break;
 		}
 	}
@@ -302,11 +305,29 @@ static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
 	{ },
 };
 
+static int bcm_resource(struct acpi_resource *ares, void *data)
+{
+	struct bcm_device *dev = data;
+
+	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
+		struct acpi_resource_uart_serialbus *sb;
+
+		sb = &ares->data.uart_serial_bus;
+		if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_UART)
+			dev->init_speed = sb->default_baud_rate;
+	}
+
+	/* Always tell the ACPI core to skip this resource */
+	return 1;
+}
+
 static int bcm_acpi_probe(struct bcm_device *dev)
 {
 	struct platform_device *pdev = dev->pdev;
 	const struct acpi_device_id *id;
 	struct gpio_desc *gpio;
+	struct acpi_device *adev;
+	LIST_HEAD(resources);
 	int ret;
 
 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
@@ -346,6 +367,13 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 		return -EINVAL;
 	}
 
+	/* Retrieve UART ACPI info */
+	adev = ACPI_COMPANION(&dev->pdev->dev);
+	if (!adev)
+		return 0;
+
+	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH v2 4/4] Bluetooth: hci_bcm: Add suspend/resume PM functions

[Frederic Danis]

Add reference to hci_uart structure to bcm_device.
This allows suspend/resume callbacks to manage UART flow control.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 8e1eacd..8d8f21c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -51,6 +51,9 @@ struct bcm_device {
 	bool			clk_enabled;
 
 	u32			init_speed;
+
+	struct hci_uart		*hu;
+	bool			is_suspended; /* suspend/resume flag */
 };
 
 struct bcm_data {
@@ -152,6 +155,7 @@ static int bcm_open(struct hci_uart *hu)
 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
 			bcm->dev = dev;
 			hu->init_speed = dev->init_speed;
+			dev->hu = hu;
 			break;
 		}
 	}
@@ -169,8 +173,10 @@ static int bcm_close(struct hci_uart *hu)
 
 	BT_DBG("hu %p", hu);
 
-	if (bcm->dev)
+	if (bcm->dev) {
 		bcm_gpio_set_power(bcm->dev, false);
+		bcm->dev->hu = NULL;
+	}
 
 	skb_queue_purge(&bcm->txq);
 	kfree_skb(bcm->rx_skb);
@@ -296,6 +302,53 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 	return skb_dequeue(&bcm->txq);
 }
 
+/* Platform suspend callback */
+static int bcm_suspend(struct device *pdev)
+{
+	struct bcm_device *dev = platform_get_drvdata(to_platform_device(pdev));
+
+	BT_DBG("suspend (%p): is_suspended %d", dev, dev->is_suspended);
+
+	if (!dev->is_suspended) {
+		hci_uart_set_flow_control(dev->hu, true);
+
+		/* Once this callback returns, driver suspends BT via GPIO */
+		dev->is_suspended = true;
+	}
+
+	/* Suspend the device */
+	if (dev->device_wakeup) {
+		gpiod_set_value(dev->device_wakeup, false);
+		BT_DBG("suspend, delaying 15 ms");
+		mdelay(15);
+	}
+
+	return 0;
+}
+
+/* Platform resume callback */
+static int bcm_resume(struct device *pdev)
+{
+	struct bcm_device *dev = platform_get_drvdata(to_platform_device(pdev));
+
+	BT_DBG("resume (%p): is_suspended %d", dev, dev->is_suspended);
+
+	if (dev->device_wakeup) {
+		gpiod_set_value(dev->device_wakeup, true);
+		BT_DBG("resume, delaying 15 ms");
+		mdelay(15);
+	}
+
+	/* When this callback executes, the device has woken up already */
+	if (dev->is_suspended) {
+		dev->is_suspended = false;
+
+		hci_uart_set_flow_control(dev->hu, false);
+	}
+
+	return 0;
+}
+
 static const struct acpi_gpio_params device_wakeup_gpios = { 0, 0, false };
 static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
 
@@ -450,12 +503,18 @@ static const struct acpi_device_id bcm_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
 #endif
 
+/* Platform suspend and resume callbacks */
+static const struct dev_pm_ops bcm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
+};
+
 static struct platform_driver bcm_driver = {
 	.probe = bcm_probe,
 	.remove = bcm_remove,
 	.driver = {
 		.name = "hci_bcm",
 		.acpi_match_table = ACPI_PTR(bcm_acpi_match),
+		.pm = &bcm_pm_ops,
 	},
 };
 
-- 
1.9.1

Re: [PATCH v2 0/4] Bluetooth: hci_bcm: Add ACPI and PM support

[Marcel Holtmann]

Hi Fred,

> These patches move PM support from rfkill-gpio to hci_uart module.
> On, off, suspend and resume functions are supported.
> They also parse the ACPI entry to find initial UART baud rate.
> 
> v1->v2:
> - Add BCM2E67 device in acpi_device_id table
> - Add suspend/resume functions

looks pretty good. My only concern is the race between bcm_data and bcm_device. Is that a valid concern from my side or can that never happen? Meaning do we need to protect against removal / unbinding of the platform device while the line discipline is active.

Regards

Marcel

Re: [PATCH v2 0/4] Bluetooth: hci_bcm: Add ACPI and PM support

[Frederic Danis]

Hello Marcel,

On 07/08/2015 18:45, Marcel Holtmann wrote:
> Hi Fred,
>
>> These patches move PM support from rfkill-gpio to hci_uart module.
>> On, off, suspend and resume functions are supported.
>> They also parse the ACPI entry to find initial UART baud rate.
>>
>> v1->v2:
>> - Add BCM2E67 device in acpi_device_id table
>> - Add suspend/resume functions
>
> looks pretty good. My only concern is the race between bcm_data and bcm_device. Is that a valid concern from my side or can that never happen? Meaning do we need to protect against removal / unbinding of the platform device while the line discipline is active.

I do not found a way to protect against device unbinding, so I will 
check if bcm_device is still in bsm_device_list before using it.

Regards

Fred