A platform device can be used to provide some specific resources in
order to manage the controller. In this first patch we retrieve the
reset gpio which is used to power on/off the controller.
The main issue is to match the current tty with the correct pdev.
In case of ACPI, we can easily find the right tty/pdev pair because
they are both child of the same UART port.
If controller is powered-on from the driver, we need to wait for a
HCI boot event before being able to send any command.
Signed-off-by: Loic Poulain <[email protected]>
---
v2: v3: none, align patch version with patch 1/2 "Intel baudrate" patch
v4: remove device tree support, will be part of a different patch
v5: static declaration fixes
rework locking usage, remove confusing intel_device_lock/unlock
Add intel_acpi_probe, align acpi device table name
v6: intel_set_power refactoring
move idev search (intel_get_idev) direclty in intel_open
drivers/bluetooth/hci_intel.c | 216 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 215 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index da3192a..c060fd3 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -26,6 +26,10 @@
#include <linux/skbuff.h>
#include <linux/firmware.h>
#include <linux/wait.h>
+#include <linux/tty.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/acpi.h>
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
@@ -39,10 +43,20 @@
#define STATE_FIRMWARE_FAILED 3
#define STATE_BOOTING 4
+struct intel_device {
+ struct list_head list;
+ struct platform_device *pdev;
+ struct gpio_desc *reset;
+ struct hci_uart *hu;
+};
+static LIST_HEAD(intel_device_list);
+static DEFINE_SPINLOCK(intel_device_list_lock);
+
struct intel_data {
struct sk_buff *rx_skb;
struct sk_buff_head txq;
unsigned long flags;
+ struct intel_device *idev;
};
static u8 intel_convert_speed(unsigned int speed)
@@ -77,9 +91,52 @@ static u8 intel_convert_speed(unsigned int speed)
}
}
+static bool intel_device_exists(struct intel_device *idev)
+{
+ struct list_head *p;
+
+ list_for_each(p, &intel_device_list) {
+ struct intel_device *dev = list_entry(p, struct intel_device,
+ list);
+ if (dev == idev)
+ return true;
+ }
+
+ return false;
+}
+
+static int intel_set_power(struct hci_uart *hu, bool powered)
+{
+ struct intel_data *intel = hu->priv;
+ struct intel_device *idev = intel->idev;
+ int err = 0;
+
+ spin_lock(&intel_device_list_lock);
+
+ if (!intel_device_exists(idev)) {
+ err = -ENODEV;
+ goto done;
+ }
+
+ if (!idev->reset) {
+ err = -EINVAL;
+ goto done;
+ }
+
+ BT_DBG("%s: set power %d", hu->hdev->name, powered);
+
+ gpiod_set_value(idev->reset, powered);
+
+done:
+ spin_unlock(&intel_device_list_lock);
+
+ return err;
+}
+
static int intel_open(struct hci_uart *hu)
{
struct intel_data *intel;
+ struct list_head *p;
BT_DBG("hu %p", hu);
@@ -90,6 +147,29 @@ static int intel_open(struct hci_uart *hu)
skb_queue_head_init(&intel->txq);
hu->priv = intel;
+
+ spin_lock(&intel_device_list_lock);
+ list_for_each(p, &intel_device_list) {
+ struct intel_device *idev;
+
+ 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) {
+ BT_INFO("hu %p, Found compatible pm device (%s)", hu,
+ dev_name(&idev->pdev->dev));
+ intel->idev = idev;
+ intel->idev->hu = hu;
+ break;
+ }
+ }
+ spin_unlock(&intel_device_list_lock);
+
+ if (!intel_set_power(hu, true))
+ set_bit(STATE_BOOTING, &intel->flags);
+
return 0;
}
@@ -99,6 +179,13 @@ static int intel_close(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ intel_set_power(hu, false);
+
+ spin_lock(&intel_device_list_lock);
+ if (intel_device_exists(intel->idev))
+ intel->idev->hu = NULL;
+ spin_unlock(&intel_device_list_lock);
+
skb_queue_purge(&intel->txq);
kfree_skb(intel->rx_skb);
kfree(intel);
@@ -149,6 +236,26 @@ static int intel_set_baudrate(struct hci_uart *hu, unsigned int speed)
struct hci_dev *hdev = hu->hdev;
u8 speed_cmd[] = { 0x06, 0xfc, 0x01, 0x00 };
struct sk_buff *skb;
+ int err;
+
+ /* This can be the first command sent to the chip, check controller is
+ * ready
+ */
+ err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
+ TASK_INTERRUPTIBLE,
+ msecs_to_jiffies(1000));
+
+ if (err == 1) {
+ BT_ERR("%s: Device boot interrupted", hdev->name);
+ clear_bit(STATE_BOOTING, &intel->flags);
+ return -EINTR;
+ }
+
+ if (err) {
+ BT_ERR("%s: Device boot timeout", hdev->name);
+ clear_bit(STATE_BOOTING, &intel->flags);
+ /* Try to continue anyway */
+ }
BT_INFO("%s: Change controller speed to %d", hdev->name, speed);
@@ -231,6 +338,23 @@ static int intel_setup(struct hci_uart *hu)
if (oper_speed && init_speed && oper_speed != init_speed)
speed_change = 1;
+ /* Check controller is ready */
+ err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
+ TASK_INTERRUPTIBLE,
+ msecs_to_jiffies(1000));
+
+ if (err == 1) {
+ BT_ERR("%s: Device boot interrupted", hdev->name);
+ clear_bit(STATE_BOOTING, &intel->flags);
+ return -EINTR;
+ }
+
+ if (err) {
+ BT_ERR("%s: Device boot timeout", hdev->name);
+ clear_bit(STATE_BOOTING, &intel->flags);
+ /* Try to continue anyway */
+ }
+
set_bit(STATE_BOOTLOADER, &intel->flags);
/* Read the Intel version information to determine if the device
@@ -546,11 +670,13 @@ done:
if (err == 1) {
BT_ERR("%s: Device boot interrupted", hdev->name);
+ clear_bit(STATE_BOOTING, &intel->flags);
return -EINTR;
}
if (err) {
BT_ERR("%s: Device boot timeout", hdev->name);
+ clear_bit(STATE_BOOTING, &intel->flags);
return -ETIMEDOUT;
}
@@ -584,7 +710,8 @@ static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
struct intel_data *intel = hu->priv;
struct hci_event_hdr *hdr;
- if (!test_bit(STATE_BOOTLOADER, &intel->flags))
+ if (!test_bit(STATE_BOOTLOADER, &intel->flags) &&
+ !test_bit(STATE_BOOTING, &intel->flags))
goto recv;
hdr = (void *)skb->data;
@@ -700,12 +827,99 @@ static const struct hci_uart_proto intel_proto = {
.dequeue = intel_dequeue,
};
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id intel_acpi_match[] = {
+ { "INT33E1", 0 },
+ { },
+};
+
+static int intel_acpi_probe(struct intel_device *idev)
+{
+ const struct acpi_device_id *id;
+
+ id = acpi_match_device(intel_acpi_match, &idev->pdev->dev);
+ if (!id)
+ return -ENODEV;
+
+ return 0;
+}
+#else
+static int intel_acpi_probe(struct intel_device *idev)
+{
+ return -ENODEV;
+}
+#endif
+
+static int intel_probe(struct platform_device *pdev)
+{
+ struct intel_device *idev;
+
+ idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
+ if (!idev)
+ return -ENOMEM;
+
+ idev->pdev = pdev;
+
+ if (ACPI_HANDLE(&pdev->dev)) {
+ int err = intel_acpi_probe(idev);
+ if (err)
+ return err;
+ } else {
+ return -ENODEV;
+ }
+
+ idev->reset = devm_gpiod_get_optional(&pdev->dev, "reset",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(idev->reset)) {
+ dev_err(&pdev->dev, "Unable to retrieve gpio\n");
+ return PTR_ERR(idev->reset);
+ }
+
+ platform_set_drvdata(pdev, idev);
+
+ /* Place this instance on the device list */
+ spin_lock(&intel_device_list_lock);
+ list_add_tail(&idev->list, &intel_device_list);
+ spin_unlock(&intel_device_list_lock);
+
+ dev_info(&pdev->dev, "registered.\n");
+
+ return 0;
+}
+
+static int intel_remove(struct platform_device *pdev)
+{
+ struct intel_device *idev = platform_get_drvdata(pdev);
+
+ spin_lock(&intel_device_list_lock);
+ list_del(&idev->list);
+ spin_unlock(&intel_device_list_lock);
+
+ dev_info(&pdev->dev, "unregistered.\n");
+
+ return 0;
+}
+
+static struct platform_driver intel_driver = {
+ .probe = intel_probe,
+ .remove = intel_remove,
+ .driver = {
+ .name = "hci_intel",
+ .acpi_match_table = ACPI_PTR(intel_acpi_match),
+ .owner = THIS_MODULE,
+ },
+};
+
int __init intel_init(void)
{
+ platform_driver_register(&intel_driver);
+
return hci_uart_register_proto(&intel_proto);
}
int __exit intel_deinit(void)
{
+ platform_driver_unregister(&intel_driver);
+
return hci_uart_unregister_proto(&intel_proto);
}
--
1.9.1
Hi Loic,
>> A platform device can be used to provide some specific resources in
>> order to manage the controller. In this first patch we retrieve the
>> reset gpio which is used to power on/off the controller.
>>
>> The main issue is to match the current tty with the correct pdev.
>> In case of ACPI, we can easily find the right tty/pdev pair because
>> they are both child of the same UART port.
>>
>> If controller is powered-on from the driver, we need to wait for a
>> HCI boot event before being able to send any command.
>>
>> Signed-off-by: Loic Poulain <[email protected]>
>> ---
>> v2: v3: none, align patch version with patch 1/2 "Intel baudrate" patch
>> v4: remove device tree support, will be part of a different patch
>> v5: static declaration fixes
>> rework locking usage, remove confusing intel_device_lock/unlock
>> Add intel_acpi_probe, align acpi device table name
>> v6: intel_set_power refactoring
>> move idev search (intel_get_idev) direclty in intel_open
>>
>> drivers/bluetooth/hci_intel.c | 216 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 215 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
>> index da3192a..c060fd3 100644
>> --- a/drivers/bluetooth/hci_intel.c
>> +++ b/drivers/bluetooth/hci_intel.c
>> @@ -26,6 +26,10 @@
>> #include <linux/skbuff.h>
>> #include <linux/firmware.h>
>> #include <linux/wait.h>
>> +#include <linux/tty.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/acpi.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -39,10 +43,20 @@
>> #define STATE_FIRMWARE_FAILED 3
>> #define STATE_BOOTING 4
>>
>> +struct intel_device {
>> + struct list_head list;
>> + struct platform_device *pdev;
>> + struct gpio_desc *reset;
>> + struct hci_uart *hu;
>
> tell me where you are actually using hci_uart in the context of intel_device. I can not make out its usage and you are creating a race condition when you try to clear this. So as long as you do not need it, do not add this. From what I can tell this is dead code at the moment.
>
>> +};
>> +static LIST_HEAD(intel_device_list);
>> +static DEFINE_SPINLOCK(intel_device_list_lock);
>> +
>> struct intel_data {
>> struct sk_buff *rx_skb;
>> struct sk_buff_head txq;
>> unsigned long flags;
>> + struct intel_device *idev;
>> };
>>
>> static u8 intel_convert_speed(unsigned int speed)
>> @@ -77,9 +91,52 @@ static u8 intel_convert_speed(unsigned int speed)
>> }
>> }
>>
>> +static bool intel_device_exists(struct intel_device *idev)
>> +{
>> + struct list_head *p;
>> +
>> + list_for_each(p, &intel_device_list) {
>> + struct intel_device *dev = list_entry(p, struct intel_device,
>> + list);
>> + if (dev == idev)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int intel_set_power(struct hci_uart *hu, bool powered)
>> +{
>> + struct intel_data *intel = hu->priv;
>> + struct intel_device *idev = intel->idev;
>> + int err = 0;
>> +
>> + spin_lock(&intel_device_list_lock);
>> +
>> + if (!intel_device_exists(idev)) {
>> + err = -ENODEV;
>> + goto done;
>> + }
>> +
>> + if (!idev->reset) {
>> + err = -EINVAL;
>> + goto done;
>> + }
>> +
>> + BT_DBG("%s: set power %d", hu->hdev->name, powered);
>> +
>> + gpiod_set_value(idev->reset, powered);
>> +
>> +done:
>> + spin_unlock(&intel_device_list_lock);
>> +
>> + return err;
>> +}
>> +
>> static int intel_open(struct hci_uart *hu)
>> {
>> struct intel_data *intel;
>> + struct list_head *p;
>>
>> BT_DBG("hu %p", hu);
>>
>> @@ -90,6 +147,29 @@ static int intel_open(struct hci_uart *hu)
>> skb_queue_head_init(&intel->txq);
>>
>> hu->priv = intel;
>> +
>> + spin_lock(&intel_device_list_lock);
>> + list_for_each(p, &intel_device_list) {
>> + struct intel_device *idev;
>> +
>> + 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) {
>> + BT_INFO("hu %p, Found compatible pm device (%s)", hu,
>> + dev_name(&idev->pdev->dev));
>> + intel->idev = idev;
>> + intel->idev->hu = hu;
>> + break;
>> + }
>> + }
>> + spin_unlock(&intel_device_list_lock);
>> +
>> + if (!intel_set_power(hu, true))
>> + set_bit(STATE_BOOTING, &intel->flags);
>> +
>> return 0;
>> }
>>
>> @@ -99,6 +179,13 @@ static int intel_close(struct hci_uart *hu)
>>
>> BT_DBG("hu %p", hu);
>>
>> + intel_set_power(hu, false);
>> +
>> + spin_lock(&intel_device_list_lock);
>> + if (intel_device_exists(intel->idev))
>> + intel->idev->hu = NULL;
>> + spin_unlock(&intel_device_list_lock);
>> +
>> skb_queue_purge(&intel->txq);
>> kfree_skb(intel->rx_skb);
>> kfree(intel);
>> @@ -149,6 +236,26 @@ static int intel_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> struct hci_dev *hdev = hu->hdev;
>> u8 speed_cmd[] = { 0x06, 0xfc, 0x01, 0x00 };
>> struct sk_buff *skb;
>> + int err;
>> +
>> + /* This can be the first command sent to the chip, check controller is
>> + * ready
>> + */
>> + err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
>> + TASK_INTERRUPTIBLE,
>> + msecs_to_jiffies(1000));
>> +
>> + if (err == 1) {
>> + BT_ERR("%s: Device boot interrupted", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + return -EINTR;
>> + }
>> +
>> + if (err) {
>> + BT_ERR("%s: Device boot timeout", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + /* Try to continue anyway */
>> + }
>>
>> BT_INFO("%s: Change controller speed to %d", hdev->name, speed);
>>
>> @@ -231,6 +338,23 @@ static int intel_setup(struct hci_uart *hu)
>> if (oper_speed && init_speed && oper_speed != init_speed)
>> speed_change = 1;
>>
>> + /* Check controller is ready */
>> + err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
>> + TASK_INTERRUPTIBLE,
>> + msecs_to_jiffies(1000));
>> +
>> + if (err == 1) {
>> + BT_ERR("%s: Device boot interrupted", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + return -EINTR;
>> + }
>> +
>> + if (err) {
>> + BT_ERR("%s: Device boot timeout", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> + /* Try to continue anyway */
>> + }
>> +
>> set_bit(STATE_BOOTLOADER, &intel->flags);
>>
>> /* Read the Intel version information to determine if the device
>> @@ -546,11 +670,13 @@ done:
>>
>> if (err == 1) {
>> BT_ERR("%s: Device boot interrupted", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> return -EINTR;
>> }
>>
>> if (err) {
>> BT_ERR("%s: Device boot timeout", hdev->name);
>> + clear_bit(STATE_BOOTING, &intel->flags);
>> return -ETIMEDOUT;
>> }
>>
>> @@ -584,7 +710,8 @@ static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>> struct intel_data *intel = hu->priv;
>> struct hci_event_hdr *hdr;
>>
>> - if (!test_bit(STATE_BOOTLOADER, &intel->flags))
>> + if (!test_bit(STATE_BOOTLOADER, &intel->flags) &&
>> + !test_bit(STATE_BOOTING, &intel->flags))
>> goto recv;
>>
>> hdr = (void *)skb->data;
>> @@ -700,12 +827,99 @@ static const struct hci_uart_proto intel_proto = {
>> .dequeue = intel_dequeue,
>> };
>>
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id intel_acpi_match[] = {
>> + { "INT33E1", 0 },
>> + { },
>> +};
>> +
>> +static int intel_acpi_probe(struct intel_device *idev)
>> +{
>> + const struct acpi_device_id *id;
>> +
>> + id = acpi_match_device(intel_acpi_match, &idev->pdev->dev);
>> + if (!id)
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>> +#else
>> +static int intel_acpi_probe(struct intel_device *idev)
>> +{
>> + return -ENODEV;
>> +}
>> +#endif
>> +
>> +static int intel_probe(struct platform_device *pdev)
>> +{
>> + struct intel_device *idev;
>> +
>> + idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
>> + if (!idev)
>> + return -ENOMEM;
>> +
>> + idev->pdev = pdev;
>> +
>> + if (ACPI_HANDLE(&pdev->dev)) {
>> + int err = intel_acpi_probe(idev);
>> + if (err)
>> + return err;
>> + } else {
>> + return -ENODEV;
>> + }
>> +
>> + idev->reset = devm_gpiod_get_optional(&pdev->dev, "reset",
>> + GPIOD_OUT_LOW);
>> + if (IS_ERR(idev->reset)) {
>> + dev_err(&pdev->dev, "Unable to retrieve gpio\n");
>> + return PTR_ERR(idev->reset);
>> + }
>> +
>> + platform_set_drvdata(pdev, idev);
>> +
>> + /* Place this instance on the device list */
>> + spin_lock(&intel_device_list_lock);
>> + list_add_tail(&idev->list, &intel_device_list);
>> + spin_unlock(&intel_device_list_lock);
>> +
>> + dev_info(&pdev->dev, "registered.\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int intel_remove(struct platform_device *pdev)
>> +{
>> + struct intel_device *idev = platform_get_drvdata(pdev);
>> +
>> + spin_lock(&intel_device_list_lock);
>> + list_del(&idev->list);
>
> This is dangerous. If intel->idev still holds a reference, then this is blowing up in your face in case the platform device goes away first.
>
> I really wonder that instead of storing a reference in intel->idev, just don't do that and always try to find the device when you want to use it. I mean they are only two cases right now. When you attach the line discipline and when you release it.
>
> So what I would do is keep it simple. On intel_open / intel_close, you hold the list spinlock, then find a matching device, if found, set GPIO, release spinlock. So the set powered helper can do the whole thing.
>
> spin_lock(&intel_device_list_lock);
> list_for_each(p, &intel_device_list) {
> struct intel_device *idev;
>
> 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) {
> BT_INFO("hu %p, Switching compatible pm device (%s) to %u", hu,
> dev_name(&idev->pdev->dev), powered);
> gpiod_set_value(idev->reset, powered);
> break;
> }
> }
> spin_unlock(&intel_device_list_lock);
>
> At this point it turns into calling the set_powered with true or false from intel_open and intel_close.
actually I updated your patch to do it like this and posted it. Look at v2 since I forgot the module device table macro that is needed for module auto-loading.
It looks now pretty simple to me. The only thing that I don't know is if we can call gpiod_set_value while holding a spinlock. Then again, I am pretty sure we could just convert this into a mutex anyway since normally probe functions are allowed to sleep.
Please give the v2 patch a spin on real hardware. I do not have a board where I can test this on.
Regards
Marcel
Hi Loic,
> A platform device can be used to provide some specific resources in
> order to manage the controller. In this first patch we retrieve the
> reset gpio which is used to power on/off the controller.
>
> The main issue is to match the current tty with the correct pdev.
> In case of ACPI, we can easily find the right tty/pdev pair because
> they are both child of the same UART port.
>
> If controller is powered-on from the driver, we need to wait for a
> HCI boot event before being able to send any command.
>
> Signed-off-by: Loic Poulain <[email protected]>
> ---
> v2: v3: none, align patch version with patch 1/2 "Intel baudrate" patch
> v4: remove device tree support, will be part of a different patch
> v5: static declaration fixes
> rework locking usage, remove confusing intel_device_lock/unlock
> Add intel_acpi_probe, align acpi device table name
> v6: intel_set_power refactoring
> move idev search (intel_get_idev) direclty in intel_open
>
> drivers/bluetooth/hci_intel.c | 216 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 215 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index da3192a..c060fd3 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -26,6 +26,10 @@
> #include <linux/skbuff.h>
> #include <linux/firmware.h>
> #include <linux/wait.h>
> +#include <linux/tty.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/acpi.h>
>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -39,10 +43,20 @@
> #define STATE_FIRMWARE_FAILED 3
> #define STATE_BOOTING 4
>
> +struct intel_device {
> + struct list_head list;
> + struct platform_device *pdev;
> + struct gpio_desc *reset;
> + struct hci_uart *hu;
tell me where you are actually using hci_uart in the context of intel_device. I can not make out its usage and you are creating a race condition when you try to clear this. So as long as you do not need it, do not add this. From what I can tell this is dead code at the moment.
> +};
> +static LIST_HEAD(intel_device_list);
> +static DEFINE_SPINLOCK(intel_device_list_lock);
> +
> struct intel_data {
> struct sk_buff *rx_skb;
> struct sk_buff_head txq;
> unsigned long flags;
> + struct intel_device *idev;
> };
>
> static u8 intel_convert_speed(unsigned int speed)
> @@ -77,9 +91,52 @@ static u8 intel_convert_speed(unsigned int speed)
> }
> }
>
> +static bool intel_device_exists(struct intel_device *idev)
> +{
> + struct list_head *p;
> +
> + list_for_each(p, &intel_device_list) {
> + struct intel_device *dev = list_entry(p, struct intel_device,
> + list);
> + if (dev == idev)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int intel_set_power(struct hci_uart *hu, bool powered)
> +{
> + struct intel_data *intel = hu->priv;
> + struct intel_device *idev = intel->idev;
> + int err = 0;
> +
> + spin_lock(&intel_device_list_lock);
> +
> + if (!intel_device_exists(idev)) {
> + err = -ENODEV;
> + goto done;
> + }
> +
> + if (!idev->reset) {
> + err = -EINVAL;
> + goto done;
> + }
> +
> + BT_DBG("%s: set power %d", hu->hdev->name, powered);
> +
> + gpiod_set_value(idev->reset, powered);
> +
> +done:
> + spin_unlock(&intel_device_list_lock);
> +
> + return err;
> +}
> +
> static int intel_open(struct hci_uart *hu)
> {
> struct intel_data *intel;
> + struct list_head *p;
>
> BT_DBG("hu %p", hu);
>
> @@ -90,6 +147,29 @@ static int intel_open(struct hci_uart *hu)
> skb_queue_head_init(&intel->txq);
>
> hu->priv = intel;
> +
> + spin_lock(&intel_device_list_lock);
> + list_for_each(p, &intel_device_list) {
> + struct intel_device *idev;
> +
> + 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) {
> + BT_INFO("hu %p, Found compatible pm device (%s)", hu,
> + dev_name(&idev->pdev->dev));
> + intel->idev = idev;
> + intel->idev->hu = hu;
> + break;
> + }
> + }
> + spin_unlock(&intel_device_list_lock);
> +
> + if (!intel_set_power(hu, true))
> + set_bit(STATE_BOOTING, &intel->flags);
> +
> return 0;
> }
>
> @@ -99,6 +179,13 @@ static int intel_close(struct hci_uart *hu)
>
> BT_DBG("hu %p", hu);
>
> + intel_set_power(hu, false);
> +
> + spin_lock(&intel_device_list_lock);
> + if (intel_device_exists(intel->idev))
> + intel->idev->hu = NULL;
> + spin_unlock(&intel_device_list_lock);
> +
> skb_queue_purge(&intel->txq);
> kfree_skb(intel->rx_skb);
> kfree(intel);
> @@ -149,6 +236,26 @@ static int intel_set_baudrate(struct hci_uart *hu, unsigned int speed)
> struct hci_dev *hdev = hu->hdev;
> u8 speed_cmd[] = { 0x06, 0xfc, 0x01, 0x00 };
> struct sk_buff *skb;
> + int err;
> +
> + /* This can be the first command sent to the chip, check controller is
> + * ready
> + */
> + err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
> + TASK_INTERRUPTIBLE,
> + msecs_to_jiffies(1000));
> +
> + if (err == 1) {
> + BT_ERR("%s: Device boot interrupted", hdev->name);
> + clear_bit(STATE_BOOTING, &intel->flags);
> + return -EINTR;
> + }
> +
> + if (err) {
> + BT_ERR("%s: Device boot timeout", hdev->name);
> + clear_bit(STATE_BOOTING, &intel->flags);
> + /* Try to continue anyway */
> + }
>
> BT_INFO("%s: Change controller speed to %d", hdev->name, speed);
>
> @@ -231,6 +338,23 @@ static int intel_setup(struct hci_uart *hu)
> if (oper_speed && init_speed && oper_speed != init_speed)
> speed_change = 1;
>
> + /* Check controller is ready */
> + err = wait_on_bit_timeout(&intel->flags, STATE_BOOTING,
> + TASK_INTERRUPTIBLE,
> + msecs_to_jiffies(1000));
> +
> + if (err == 1) {
> + BT_ERR("%s: Device boot interrupted", hdev->name);
> + clear_bit(STATE_BOOTING, &intel->flags);
> + return -EINTR;
> + }
> +
> + if (err) {
> + BT_ERR("%s: Device boot timeout", hdev->name);
> + clear_bit(STATE_BOOTING, &intel->flags);
> + /* Try to continue anyway */
> + }
> +
> set_bit(STATE_BOOTLOADER, &intel->flags);
>
> /* Read the Intel version information to determine if the device
> @@ -546,11 +670,13 @@ done:
>
> if (err == 1) {
> BT_ERR("%s: Device boot interrupted", hdev->name);
> + clear_bit(STATE_BOOTING, &intel->flags);
> return -EINTR;
> }
>
> if (err) {
> BT_ERR("%s: Device boot timeout", hdev->name);
> + clear_bit(STATE_BOOTING, &intel->flags);
> return -ETIMEDOUT;
> }
>
> @@ -584,7 +710,8 @@ static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
> struct intel_data *intel = hu->priv;
> struct hci_event_hdr *hdr;
>
> - if (!test_bit(STATE_BOOTLOADER, &intel->flags))
> + if (!test_bit(STATE_BOOTLOADER, &intel->flags) &&
> + !test_bit(STATE_BOOTING, &intel->flags))
> goto recv;
>
> hdr = (void *)skb->data;
> @@ -700,12 +827,99 @@ static const struct hci_uart_proto intel_proto = {
> .dequeue = intel_dequeue,
> };
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id intel_acpi_match[] = {
> + { "INT33E1", 0 },
> + { },
> +};
> +
> +static int intel_acpi_probe(struct intel_device *idev)
> +{
> + const struct acpi_device_id *id;
> +
> + id = acpi_match_device(intel_acpi_match, &idev->pdev->dev);
> + if (!id)
> + return -ENODEV;
> +
> + return 0;
> +}
> +#else
> +static int intel_acpi_probe(struct intel_device *idev)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +static int intel_probe(struct platform_device *pdev)
> +{
> + struct intel_device *idev;
> +
> + idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
> + if (!idev)
> + return -ENOMEM;
> +
> + idev->pdev = pdev;
> +
> + if (ACPI_HANDLE(&pdev->dev)) {
> + int err = intel_acpi_probe(idev);
> + if (err)
> + return err;
> + } else {
> + return -ENODEV;
> + }
> +
> + idev->reset = devm_gpiod_get_optional(&pdev->dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(idev->reset)) {
> + dev_err(&pdev->dev, "Unable to retrieve gpio\n");
> + return PTR_ERR(idev->reset);
> + }
> +
> + platform_set_drvdata(pdev, idev);
> +
> + /* Place this instance on the device list */
> + spin_lock(&intel_device_list_lock);
> + list_add_tail(&idev->list, &intel_device_list);
> + spin_unlock(&intel_device_list_lock);
> +
> + dev_info(&pdev->dev, "registered.\n");
> +
> + return 0;
> +}
> +
> +static int intel_remove(struct platform_device *pdev)
> +{
> + struct intel_device *idev = platform_get_drvdata(pdev);
> +
> + spin_lock(&intel_device_list_lock);
> + list_del(&idev->list);
This is dangerous. If intel->idev still holds a reference, then this is blowing up in your face in case the platform device goes away first.
I really wonder that instead of storing a reference in intel->idev, just don't do that and always try to find the device when you want to use it. I mean they are only two cases right now. When you attach the line discipline and when you release it.
So what I would do is keep it simple. On intel_open / intel_close, you hold the list spinlock, then find a matching device, if found, set GPIO, release spinlock. So the set powered helper can do the whole thing.
spin_lock(&intel_device_list_lock);
list_for_each(p, &intel_device_list) {
struct intel_device *idev;
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) {
BT_INFO("hu %p, Switching compatible pm device (%s) to %u", hu,
dev_name(&idev->pdev->dev), powered);
gpiod_set_value(idev->reset, powered);
break;
}
}
spin_unlock(&intel_device_list_lock);
At this point it turns into calling the set_powered with true or false from intel_open and intel_close.
In addition, the hci_bcm.c variant is having a remove driver here. Don't we need the same?
> + dev_info(&pdev->dev, "unregistered.\n");
> +
> + return 0;
> +}
> +
> +static struct platform_driver intel_driver = {
> + .probe = intel_probe,
> + .remove = intel_remove,
> + .driver = {
> + .name = "hci_intel",
> + .acpi_match_table = ACPI_PTR(intel_acpi_match),
> + .owner = THIS_MODULE,
> + },
> +};
> +
> int __init intel_init(void)
> {
> + platform_driver_register(&intel_driver);
> +
> return hci_uart_register_proto(&intel_proto);
> }
>
> int __exit intel_deinit(void)
> {
> + platform_driver_unregister(&intel_driver);
> +
> return hci_uart_unregister_proto(&intel_proto);
> }
Regards
Marcel