Hi Fred, -----Original Message----- From: linux-bluetooth-owner@xxxxxxxxxxxxxxx [mailto:linux-bluetooth-owner@xxxxxxxxxxxxxxx] On Behalf Of Frederic Danis Sent: Thursday, July 16, 2015 12:10 PM To: linux-bluetooth@xxxxxxxxxxxxxxx Subject: Re: [PATCH 1/3] Bluetooth: hci_uart: Add PM for BCM2E39 Ping ? On 03/07/2015 15:22, Frederic Danis wrote: > Retrieve "shutdown" and "device_wakeup" GPIOs from ACPI. > Set device off during platform device enumeration. > Set device on only when it is attached. IF: Sorry for the delay in responding. I am not actively working on the similar DT driver at the moment as we have hit a DT maintainer roadblock for its development. I do not read this bulletin board much therefore. Great seeing a first take on the Broadcom BT UART ACPI driver! Looks like a good start to me. A few comments: if01. I would turn the device on when the BCM protocol is opened and turn the device off when the protocol is closed. if02. I would strongly encourage you to start supporting the runtime suspend upon a period of inactivity, resume and the interrupt-triggered wake as related power savings are significant. You would obviously need to set reasonable sleep parameters via the VSC then. Of course, we would also want to flow control the device in suspend time to avoid character loss. See my DT patches for the VSC and UART flow control particulars. if03. We would also want at least to suspend and resume the device when the platform suspends/resumes as a whole. if04. A great enhancement would have also been to make a decision whether to suspend or turn off the device at the time of the platform suspend. That would require BT knowledge of what's paired etc. We would obviously need to resume or turn it back on at the platform resume time accordingly. if05. We should ideally support running on the Broadcom BT UART ACPI node as it ships now in hundreds of laptop/tablet/phone models while you apparently rely on Linux specific enhancements to this extent. We want to let users dual boot with BT working fine in either operating system. Would you publish a relevant DSL snippet of what you've done in the ACPI on your test platform? Thanks, -Ilya > > Signed-off-by: Frederic Danis <frederic.danis@xxxxxxxxxxxxxxx> > --- > drivers/bluetooth/hci_bcm.c | 191 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 189 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c > index 23523e1..856cb48 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,33 @@ 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 }, > + { }, > +}; > +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); > } > -- Frederic Danis Open Source Technology Center frederic.danis@xxxxxxxxx Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n�����{��� �^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
