Bluetooth: hci_intel: enable on new platform

PATCH In review Andy Shevchenko 2 participants 8 messages Sep 3, 2020 – Sep 11, 2020 Bluetooth drivers

lore

Share & tools

lore

Andy Shevchenko

author

2020-09-03 18:48 UTC #0 lore raw

PATCH 1/4 Bluetooth: hci_intel: enable on new platform

On new Intel platform the device is provided with INT33E3 ID. Append it to the list.

This will require ACPI_GPIO_QUIRK_ONLY_GPIOIO to be enabled because the relevant ASL looks like:

	UartSerialBusV2 ( ... )
	GpioInt ( ... ) { ... }
	GpioIo ( ... ) { ... }

which means that first GPIO resource is an interrupt, while we are expecting it to be reset one (output). Do the same for host-wake because in case of GpioInt() the platform_get_irq() will do the job and should return correct Linux IRQ number. That said, host-wake GPIO can only be GpioIo() resource.

While here, drop commas in terminator lines.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/bluetooth/hci_intel.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/bluetooth/hci_intel.c +5 -4

index f1299da6eed8..703d774be5a6 100644
@@ -1076,7 +1076,8 @@ static const struct hci_uart_proto intel_proto = {
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id intel_acpi_match[] = {
 	{ "INT33E1", 0 },
-	{ },
+	{ "INT33E3", 0 },
+	{ }
 };
 MODULE_DEVICE_TABLE(acpi, intel_acpi_match);
 #endif

@@ -1138,9 +1139,9 @@ static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
 static const struct acpi_gpio_params host_wake_gpios = { 1, 0, false };
 
 static const struct acpi_gpio_mapping acpi_hci_intel_gpios[] = {
-	{ "reset-gpios", &reset_gpios, 1 },
-	{ "host-wake-gpios", &host_wake_gpios, 1 },
-	{ },
+	{ "reset-gpios", &reset_gpios, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
+	{ "host-wake-gpios", &host_wake_gpios, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
+	{ }
 };
 
 static int intel_probe(struct platform_device *pdev)

Signature · 1 line

2.28.0

Andy Shevchenko

author

2020-09-03 18:48 UTC #1 lore raw

PATCH 2/4 Bluetooth: hci_intel: drop strange le16_to_cpu() against u8 values

Sparse rightfully complains:

hci_intel.c:696:26: warning: cast to restricted __le16 hci_intel.c:701:26: warning: cast to restricted __le16 hci_intel.c:702:26: warning: cast to restricted __le16 hci_intel.c:703:26: warning: cast to restricted __le16 hci_intel.c:725:26: warning: cast to restricted __le16 hci_intel.c:730:26: warning: cast to restricted __le16 hci_intel.c:731:26: warning: cast to restricted __le16 hci_intel.c:732:26: warning: cast to restricted __le16

because we access non-restricted types with le16_to_cpu(). More confusion is added by using above against u8. On big-endian architecture we will get all zeroes. I bet it's not what should be in such case.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/bluetooth/hci_intel.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

drivers/bluetooth/hci_intel.c +4 ~~-10~~

index 703d774be5a6..50e4fc6813c2 100644
@@ -693,14 +693,11 @@ static int intel_setup(struct hci_uart *hu)
 	case 0x0b:      /* SfP */
 	case 0x0c:      /* WsP */
 		snprintf(fwname, sizeof(fwname), "intel/ibt-%u-%u.sfi",
-			 le16_to_cpu(ver.hw_variant),
-			 le16_to_cpu(params.dev_revid));
+			 ver.hw_variant, le16_to_cpu(params.dev_revid));
 		break;
 	case 0x12:      /* ThP */
 		snprintf(fwname, sizeof(fwname), "intel/ibt-%u-%u-%u.sfi",
-			 le16_to_cpu(ver.hw_variant),
-			 le16_to_cpu(ver.hw_revision),
-			 le16_to_cpu(ver.fw_revision));
+			 ver.hw_variant, ver.hw_revision, ver.fw_revision);
 		break;
 	default:
 		bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",

@@ -722,14 +719,11 @@ static int intel_setup(struct hci_uart *hu)
 	case 0x0b:      /* SfP */
 	case 0x0c:      /* WsP */
 		snprintf(fwname, sizeof(fwname), "intel/ibt-%u-%u.ddc",
-			 le16_to_cpu(ver.hw_variant),
-			 le16_to_cpu(params.dev_revid));
+			 ver.hw_variant, le16_to_cpu(params.dev_revid));
 		break;
 	case 0x12:      /* ThP */
 		snprintf(fwname, sizeof(fwname), "intel/ibt-%u-%u-%u.ddc",
-			 le16_to_cpu(ver.hw_variant),
-			 le16_to_cpu(ver.hw_revision),
-			 le16_to_cpu(ver.fw_revision));
+			 ver.hw_variant, ver.hw_revision, ver.fw_revision);
 		break;
 	default:
 		bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",

Signature · 1 line

2.28.0

Marcel Holtmann

maintainer

2020-09-11 06:51 UTC #2 lore raw

Re: [PATCH v1 2/4] Bluetooth: hci_intel: drop strange le16_to_cpu() against u8 values

Hi Andy,

Quoted 16 lines

Sparse rightfully complains:
hci_intel.c:696:26: warning: cast to restricted __le16 hci_intel.c:701:26: warning: cast to restricted __le16 hci_intel.c:702:26: warning: cast to restricted __le16 hci_intel.c:703:26: warning: cast to restricted __le16 hci_intel.c:725:26: warning: cast to restricted __le16 hci_intel.c:730:26: warning: cast to restricted __le16 hci_intel.c:731:26: warning: cast to restricted __le16 hci_intel.c:732:26: warning: cast to restricted __le16
because we access non-restricted types with le16_to_cpu(). More confusion is added by using above against u8. On big-endian architecture we will get all zeroes. I bet it's not what should be in such case.

Quoted 4 lines

Signed-off-by Andy Shevchenko <[email protected]>
---
drivers/bluetooth/hci_intel.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Andy Shevchenko

author

2020-09-03 18:49 UTC #3 lore raw

PATCH 3/4 Bluetooth: hci_intel: switch to list_for_each_entry()

In reply to Andy Shevchenko

There is no need to have list_for_each() followed by list_entry() when we simply may use list_for_each_entry() directly.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/bluetooth/hci_intel.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

drivers/bluetooth/hci_intel.c +10 ~~-21~~

index 50e4fc6813c2..b20a40fab83e 100644
@@ -288,7 +288,7 @@ static irqreturn_t intel_irq(int irq, void *dev_id)
 
 static int intel_set_power(struct hci_uart *hu, bool powered)
 {
-	struct list_head *p;
+	struct intel_device *idev;
 	int err = -ENODEV;
 
 	if (!hu->tty->dev)

@@ -296,10 +296,7 @@ static int intel_set_power(struct hci_uart *hu, bool powered)
 
 	mutex_lock(&intel_device_list_lock);
 
-	list_for_each(p, &intel_device_list) {
-		struct intel_device *idev = list_entry(p, struct intel_device,
-						       list);
-
+	list_for_each_entry(idev, &intel_device_list, list) {
 		/* tty device and pdev device should share the same parent
 		 * which is the UART port.
 		 */

@@ -362,19 +359,16 @@ static int intel_set_power(struct hci_uart *hu, bool powered)
 
 static void intel_busy_work(struct work_struct *work)
 {
-	struct list_head *p;
 	struct intel_data *intel = container_of(work, struct intel_data,
 						busy_work);
+	struct intel_device *idev;
 
 	if (!intel->hu->tty->dev)
 		return;
 
 	/* Link is busy, delay the suspend */
 	mutex_lock(&intel_device_list_lock);
-	list_for_each(p, &intel_device_list) {
-		struct intel_device *idev = list_entry(p, struct intel_device,
-						       list);
-
+	list_for_each_entry(idev, &intel_device_list, list) {
 		if (intel->hu->tty->dev->parent == idev->pdev->dev.parent) {
 			pm_runtime_get(&idev->pdev->dev);
 			pm_runtime_mark_last_busy(&idev->pdev->dev);

@@ -533,7 +527,7 @@ static int intel_setup(struct hci_uart *hu)
 	struct sk_buff *skb;
 	struct intel_version ver;
 	struct intel_boot_params params;
-	struct list_head *p;
+	struct intel_device *idev;
 	const struct firmware *fw;
 	char fwname[64];
 	u32 boot_param;

@@ -833,13 +827,11 @@ static int intel_setup(struct hci_uart *hu)
 	 * until further LPM TX notification.
 	 */
 	mutex_lock(&intel_device_list_lock);
-	list_for_each(p, &intel_device_list) {
-		struct intel_device *dev = list_entry(p, struct intel_device,
-						      list);
+	list_for_each_entry(idev, &intel_device_list, list) {
 		if (!hu->tty->dev)
 			break;
-		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
-			if (device_may_wakeup(&dev->pdev->dev)) {
+		if (hu->tty->dev->parent == idev->pdev->dev.parent) {
+			if (device_may_wakeup(&idev->pdev->dev)) {
 				set_bit(STATE_LPM_ENABLED, &intel->flags);
 				set_bit(STATE_TX_ACTIVE, &intel->flags);
 			}

@@ -993,7 +985,7 @@ static int intel_recv(struct hci_uart *hu, const void *data, int count)
 static int intel_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 {
 	struct intel_data *intel = hu->priv;
-	struct list_head *p;
+	struct intel_device *idev;
 
 	BT_DBG("hu %p skb %p", hu, skb);

@@ -1004,10 +996,7 @@ static int intel_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 	 * completed before enqueuing any packet.
 	 */
 	mutex_lock(&intel_device_list_lock);
-	list_for_each(p, &intel_device_list) {
-		struct intel_device *idev = list_entry(p, struct intel_device,
-						       list);
-
+	list_for_each_entry(idev, &intel_device_list, list) {
 		if (hu->tty->dev->parent == idev->pdev->dev.parent) {
 			pm_runtime_get_sync(&idev->pdev->dev);
 			pm_runtime_mark_last_busy(&idev->pdev->dev);

Signature · 1 line

2.28.0

Marcel Holtmann

maintainer

2020-09-11 06:51 UTC #4 lore raw

Re: [PATCH v1 3/4] Bluetooth: hci_intel: switch to list_for_each_entry()

Hi Andy,

Quoted 7 lines

There is no need to have list_for_each() followed by list_entry() when we simply may use list_for_each_entry() directly.
Signed-off-by Andy Shevchenko <[email protected]>
---
drivers/bluetooth/hci_intel.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Andy Shevchenko

author

2020-09-03 18:49 UTC #5 lore raw

PATCH 4/4 Bluetooth: hci_intel: sort headers alphabetically

In reply to Andy Shevchenko

Sort headers alphabetically to increase readability and make maintenance easier.

While here, update copyright year.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/bluetooth/hci_intel.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/bluetooth/hci_intel.c +8 -8

index b20a40fab83e..17f51bc17bb5 100644
@@ -3,21 +3,21 @@
  *
  *  Bluetooth HCI UART driver for Intel devices
  *
- *  Copyright (C) 2015  Intel Corporation
+ *  Copyright (C) 2015,2020 Intel Corporation
  */
 
-#include <linux/kernel.h>
+#include <linux/acpi.h>
 #include <linux/errno.h>
-#include <linux/skbuff.h>
 #include <linux/firmware.h>
-#include <linux/module.h>
-#include <linux/wait.h>
-#include <linux/tty.h>
-#include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
-#include <linux/acpi.h>
 #include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/skbuff.h>
+#include <linux/tty.h>
+#include <linux/wait.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>

Signature · 1 line

2.28.0

Andy Shevchenko

author

2020-09-09 16:02 UTC #6 lore raw

In reply to Andy Shevchenko

Quoted 10 lines · Andy Shevchenko

On new Intel platform the device is provided with INT33E3 ID. Append it to the list.
This will require ACPI_GPIO_QUIRK_ONLY_GPIOIO to be enabled because the relevant ASL looks like:
	UartSerialBusV2 ( ... )
	GpioInt ( ... ) { ... }
	GpioIo ( ... ) { ... }

Quoted 6 lines

which means that first GPIO resource is an interrupt, while we are expecting it to be reset one (output). Do the same for host-wake because in case of GpioInt() the platform_get_irq() will do the job and should return correct Linux IRQ number. That said, host-wake GPIO can only be GpioIo() resource.
While here, drop commas in terminator lines.

Any comments?

Quoted 35 lines

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/bluetooth/hci_intel.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/bluetooth/hci_intel.c +5 -4

index f1299da6eed8..703d774be5a6 100644
@@ -1076,7 +1076,8 @@ static const struct hci_uart_proto intel_proto = {
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id intel_acpi_match[] = {
 	{ "INT33E1", 0 },
-	{ },
+	{ "INT33E3", 0 },
+	{ }
 };
 MODULE_DEVICE_TABLE(acpi, intel_acpi_match);
 #endif

@@ -1138,9 +1139,9 @@ static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
 static const struct acpi_gpio_params host_wake_gpios = { 1, 0, false };
 
 static const struct acpi_gpio_mapping acpi_hci_intel_gpios[] = {
-	{ "reset-gpios", &reset_gpios, 1 },
-	{ "host-wake-gpios", &host_wake_gpios, 1 },
-	{ },
+	{ "reset-gpios", &reset_gpios, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
+	{ "host-wake-gpios", &host_wake_gpios, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
+	{ }
 };
 
 static int intel_probe(struct platform_device *pdev)

Signature · 1 line

2.28.0

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Marcel Holtmann

maintainer

2020-09-11 07:04 UTC #7 lore raw

In reply to Andy Shevchenko

Hi Andy,

Quoted 15 lines

On new Intel platform the device is provided with INT33E3 ID. Append it to the list.
This will require ACPI_GPIO_QUIRK_ONLY_GPIOIO to be enabled because the relevant ASL looks like:
	UartSerialBusV2 ( ... )
	GpioInt ( ... ) { ... }
	GpioIo ( ... ) { ... }
which means that first GPIO resource is an interrupt, while we are expecting it to be reset one (output). Do the same for host-wake because in case of GpioInt() the platform_get_irq() will do the job and should return correct Linux IRQ number. That said, host-wake GPIO can only be GpioIo() resource.

Quoted 6 lines

While here, drop commas in terminator lines.
Signed-off-by Andy Shevchenko <[email protected]>
---
drivers/bluetooth/hci_intel.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel