[PATCH v1 1/4] Bluetooth: hci_intel: enable on new platform

[PATCH v1 1/4] Bluetooth: hci_intel: enable on new platform

[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 ( ... ) { ... }

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 <andriy.shevchenko@linux.intel.com>
---
 drivers/bluetooth/hci_intel.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index f1299da6eed8..703d774be5a6 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -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)
-- 
2.28.0

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

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
---
 drivers/bluetooth/hci_intel.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 703d774be5a6..50e4fc6813c2 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -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)",
-- 
2.28.0

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

[Marcel Holtmann]

Hi Andy,

> 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 <andriy.shevchenko@linux.intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

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

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/bluetooth/hci_intel.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 50e4fc6813c2..b20a40fab83e 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -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);
-- 
2.28.0

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

[Marcel Holtmann]

Hi Andy,

> 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 <andriy.shevchenko@linux.intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 31 ++++++++++---------------------
> 1 file changed, 10 insertions(+), 21 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH v1 4/4] Bluetooth: hci_intel: sort headers alphabetically

[Andy Shevchenko]

Sort headers alphabetically to increase readability
and make maintenance easier.

While here, update copyright year.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/bluetooth/hci_intel.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index b20a40fab83e..17f51bc17bb5 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -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>
-- 
2.28.0

Re: [PATCH v1 1/4] Bluetooth: hci_intel: enable on new platform

[Andy Shevchenko]

On Thu, Sep 03, 2020 at 09:48:47PM +0300, Andy Shevchenko wrote:
> 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.

Any comments?

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/bluetooth/hci_intel.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index f1299da6eed8..703d774be5a6 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -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)
> -- 
> 2.28.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] Bluetooth: hci_intel: enable on new platform

[Marcel Holtmann]

Hi Andy,

> 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 <andriy.shevchenko@linux.intel.com>
> ---
> drivers/bluetooth/hci_intel.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel