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 <loic.poulain@xxxxxxxxx>
> ---
> 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
--
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
