Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461583 > unrolled thread

[RFC 0/7] Nokia N9xx bluetooth driver

Back to article view | Back to linux.kernel

Contents

  [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel <sre@kernel.org> - 2016-08-13 05:20 +0200
    [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init Sebastian Reichel <sre@kernel.org> - 2016-08-13 05:20 +0200
      Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT  init Pavel Machek <pavel@ucw.cz> - 2016-08-14 11:00 +0200
        Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT  init Sebastian Reichel <sre@kernel.org> - 2016-08-16 10:20 +0200
    [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel <sre@kernel.org> - 2016-08-13 05:20 +0200
      Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-08-15 02:00 +0200
        Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel <sre@kernel.org> - 2016-08-15 03:20 +0200
      Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Marcel Holtmann <marcel@holtmann.org> - 2016-08-16 09:10 +0200
        Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Pali Rohár <pali.rohar@gmail.com> - 2016-08-16 10:00 +0200
          Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel <sre@kernel.org> - 2016-08-16 11:30 +0200
        Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel <sre@kernel.org> - 2016-08-16 11:20 +0200
          Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Marcel Holtmann <marcel@holtmann.org> - 2016-08-16 12:30 +0200
          Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Marcel Holtmann <marcel@holtmann.org> - 2016-08-16 12:30 +0200
            Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Pavel Machek <pavel@ucw.cz> - 2016-08-16 22:10 +0200
      Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Marcel Holtmann <marcel@holtmann.org> - 2016-08-16 10:20 +0200
        Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel <sre@kernel.org> - 2016-08-16 11:40 +0200
    Re: [RFC 0/7] Nokia N9xx bluetooth driver Marcel Holtmann <marcel@holtmann.org> - 2016-08-16 09:20 +0200
      Re: [RFC 0/7] Nokia N9xx bluetooth driver Rob Herring <robh+dt@kernel.org> - 2016-08-16 22:30 +0200

#1461583 — [RFC 0/7] Nokia N9xx bluetooth driver

Hi,

This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
N9xx devices. It has been tested on the Nokia N950, where it works
correctly. On Nokia N900 it currently fails during negotiation
(probably related to slightly incorrect serial settings/timings).
The N900's bcm2048 correctly answeres to alive check even before
negotiation (on N950 it does not work before negotiation), but replies
with an Hardware error event to the negotiation packet.

Apart from N900 support there are still two "features" missing in the
driver:

1. To save energy the bluetooth module can be put into sleep mode via a
   GPIO. This gpio should be enabled before sending data via UART and
   disabled once the transmission is done. I currently just keep the
   GPIO always enabled.
2. It would be nice to have a bluetooth device exposed by the kernel
   automatically without having to setup the tty disector first for
   proper configurationless out of the box support. I could not find
   a nice way to do this from the kernel, though.

On N950 the driver works with omap-serial and omap8250-serial drivers.

You can also find this series in the following branch:
https://git.kernel.org/cgit/linux/kernel/git/sre/linux-n900.git/log/?h=nokia-bluetooth

-- Sebastian

NeilBrown (1):
  tty: add support for "tty slave" devices

Sebastian Reichel (6):
  tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init
  dt: bindings: Add nokia-bluetooth
  Bluetooth: hci_uart: Add support for word alignment
  Bluetooth: hci_nokia: Introduce new driver
  ARM: dts: OMAP3-N900: Add bluetooth
  ARM: dts: OMAP3-N950: Add bluetooth

 .../devicetree/bindings/net/nokia-bluetooth.txt    |  43 ++
 Documentation/devicetree/bindings/serial/8250.txt  |   4 +
 arch/arm/boot/dts/omap3-n900.dts                   |  22 +-
 arch/arm/boot/dts/omap3-n950-n9.dtsi               |  34 +
 drivers/bluetooth/Kconfig                          |  10 +
 drivers/bluetooth/Makefile                         |   1 +
 drivers/bluetooth/hci_h4.c                         |  10 +
 drivers/bluetooth/hci_ldisc.c                      |   6 +
 drivers/bluetooth/hci_nokia.c                      | 734 +++++++++++++++++++++
 drivers/bluetooth/hci_nokia.h                      | 140 ++++
 drivers/bluetooth/hci_uart.h                       |   9 +-
 drivers/tty/serial/omap-serial.c                   |   3 +
 drivers/tty/tty_io.c                               |   6 +
 13 files changed, 1020 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt
 create mode 100644 drivers/bluetooth/hci_nokia.c
 create mode 100644 drivers/bluetooth/hci_nokia.h

-- 
2.8.1

[toc] | [next] | [standalone]

#1461584 — [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init

---
 drivers/tty/serial/omap-serial.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index a2a529994ba5..7c2c77789c2c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1542,6 +1542,9 @@ static struct omap_uart_port_info *of_get_uart_port_info(struct device *dev)
 
 	of_property_read_u32(dev->of_node, "clock-frequency",
 					 &omap_up_info->uartclk);
+
+	omap_up_info->flags = UPF_BOOT_AUTOCONF;
+
 	return omap_up_info;
 }
 
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1461726 — Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init

It would be nice to have a line about why it is needed, unfortunately
include/linux/serial_core.h is not exactly helpful.

Plus you'll need to sign off the patch.

Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1463525 — Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Sun, Aug 14, 2016 at 10:49:56AM +0200, Pavel Machek wrote:
> It would be nice to have a line about why it is needed,
> unfortunately include/linux/serial_core.h is not exactly helpful.
>
> Plus you'll need to sign off the patch.

I should have had another look at the long patch
descriptions before sending the patchset ;)

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1461586 — [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

This driver adds support for Nokia H4+ procotol used
for example by Nokia's internet tablets (N770 - N950).
---
 drivers/bluetooth/Kconfig     |  10 +
 drivers/bluetooth/Makefile    |   1 +
 drivers/bluetooth/hci_ldisc.c |   6 +
 drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_nokia.h | 140 ++++++++
 drivers/bluetooth/hci_uart.h  |   8 +-
 6 files changed, 898 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/hci_nokia.c
 create mode 100644 drivers/bluetooth/hci_nokia.h

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index cf50fd2e96df..c32d9d5ad1d2 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -86,6 +86,16 @@ config BT_HCIUART_H4
 
 	  Say Y here to compile support for HCI UART (H4) protocol.
 
+config BT_HCIUART_NOKIA
+	bool "UART Nokia H4+ protocol support"
+	depends on BT_HCIUART
+	help
+	  Nokia H4+ is serial protocol for communication between Bluetooth
+	  device and host. This protocol is required for Bluetooth devices
+	  with UART interface in Nokia devices.
+
+	  Say Y here to compile support for Nokia's H4+ protocol.
+
 config BT_HCIUART_BCSP
 	bool "BCSP protocol support"
 	depends on BT_HCIUART
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 9c18939fc5c9..f7951646ee14 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)	+= hci_intel.o
 hci_uart-$(CONFIG_BT_HCIUART_BCM)	+= hci_bcm.o
 hci_uart-$(CONFIG_BT_HCIUART_QCA)	+= hci_qca.o
 hci_uart-$(CONFIG_BT_HCIUART_AG6XX)	+= hci_ag6xx.o
+hci_uart-$(CONFIG_BT_HCIUART_NOKIA)	+= hci_nokia.o
 hci_uart-objs				:= $(hci_uart-y)
 
 ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index dda97398c59a..83d0de94bf35 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -810,6 +810,9 @@ static int __init hci_uart_init(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
 	ag6xx_init();
 #endif
+#ifdef CONFIG_BT_HCIUART_NOKIA
+	nokia_init();
+#endif
 
 	return 0;
 }
@@ -845,6 +848,9 @@ static void __exit hci_uart_exit(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
 	ag6xx_deinit();
 #endif
+#ifdef CONFIG_BT_HCIUART_NOKIA
+	nokia_deinit();
+#endif
 
 	/* Release tty registration of line discipline */
 	err = tty_unregister_ldisc(N_HCI);
diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
new file mode 100644
index 000000000000..efd4dd320838
--- /dev/null
+++ b/drivers/bluetooth/hci_nokia.c
@@ -0,0 +1,734 @@
+/*
+ *
+ *  Bluetooth HCI UART H4 driver with Nokia Extensions
+ *
+ *  Copyright (C) 2015 Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/fcntl.h>
+#include <linux/interrupt.h>
+#include <linux/ptrace.h>
+#include <linux/poll.h>
+#include <linux/pm_runtime.h>
+#include <linux/firmware.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/signal.h>
+#include <linux/ioctl.h>
+#include <linux/skbuff.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+
+#include <linux/gpio/consumer.h>
+
+#include <linux/unaligned/le_struct.h>
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "hci_uart.h"
+#include "hci_nokia.h"
+
+struct nokia_uart_dev {
+	struct device *dev;
+	struct tty_port *port;
+	struct gpio_desc *reset;
+	struct gpio_desc *wakeup_host;
+	struct gpio_desc *wakeup_bt;
+	unsigned long sysclk_speed;
+};
+
+struct nokia_bt_dev {
+	struct hci_uart *hu;
+	struct nokia_uart_dev *btdata;
+	int wake_irq;
+	bool wake_state;
+	struct sk_buff *rx_skb;
+	struct sk_buff_head txq;
+	bdaddr_t bdaddr;
+
+	int init_error;
+	struct completion init_completion;
+
+	uint8_t man_id;
+	uint8_t ver_id;
+};
+
+static char *nokia_get_fw_name(struct nokia_bt_dev *btdev)
+{
+	switch (btdev->man_id) {
+	case NOKIA_ID_CSR:
+		return FIRMWARE_CSR;
+	case NOKIA_ID_BCM2048:
+		return FIRMWARE_BCM2048;
+	case NOKIA_ID_TI1271:
+		return FIRMWARE_TI1271;
+	default:
+		return NULL;
+	}
+}
+
+static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state,
+				 int timeout_ms)
+{
+	unsigned long timeout;
+	int signal;
+
+	timeout = jiffies + msecs_to_jiffies(timeout_ms);
+	for (;;) {
+		signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS;
+		if (!!signal == !!state) {
+			dev_dbg(hu->tty->dev, "wait for cts... received!\n");
+			return 0;
+		}
+		if (time_after(jiffies, timeout)) {
+			dev_dbg(hu->tty->dev, "wait for cts... timeout!\n");
+			return -ETIMEDOUT;
+		}
+		usleep_range(1000, 2000);
+	}
+}
+
+static int btdev_match(struct device *child, void *data)
+{
+	if (!strcmp(child->driver->name, "nokia-bluetooth"))
+		return 1;
+	else
+		return 0;
+}
+
+static irqreturn_t wakeup_handler(int irq, void *data)
+{
+	struct nokia_bt_dev *btdev = data;
+	struct device *serialdev = btdev->hu->tty->dev;
+	int wake_state = gpiod_get_value(btdev->btdata->wakeup_host);
+
+	dev_dbg(serialdev, "wakeup received: %d -> %d\n",
+		btdev->wake_state, wake_state);
+
+	if (btdev->wake_state == wake_state)
+		return IRQ_HANDLED;
+
+	if (wake_state)
+		pm_runtime_get_sync(serialdev);
+	else if (!wake_state)
+		pm_runtime_put(serialdev);
+
+	btdev->wake_state = wake_state;
+
+	return IRQ_HANDLED;
+}
+
+static int nokia_reset(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	int err;
+
+	/* reset routine */
+	gpiod_set_value_cansleep(btdev->btdata->reset, 0);
+	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
+
+	msleep(50);
+
+	/* safety check */
+	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
+	if (err == 1) {
+		dev_err(hu->tty->dev, "reset: host wakeup not low!\n");
+		return -EPROTO;
+	}
+
+	/* flush queues */
+	tty_ldisc_flush(hu->tty);
+	tty_driver_flush_buffer(hu->tty);
+
+	/* init uart */
+	hci_uart_init_tty(hu);
+	hci_uart_set_flow_control(hu, true);
+	hci_uart_set_baudrate(hu, INIT_SPEED);
+
+	gpiod_set_value_cansleep(btdev->btdata->reset, 1);
+	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 0);
+
+	msleep(100);
+
+	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
+	if (err == 0) {
+		dev_err(hu->tty->dev, "reset: host wakeup not high!\n");
+		return -EPROTO;
+	}
+
+	/* wait for cts */
+	err = hci_uart_wait_for_cts(hu, true, 100);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "CTS not received: %d\n", err);
+		return err;
+	}
+
+	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
+	hci_uart_set_flow_control(hu, false);
+
+	return 0;
+}
+
+static int nokia_send_alive_packet(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_alive_hdr *hdr;
+	struct hci_nokia_alive_pkt *pkt;
+	struct sk_buff *skb;
+	int len;
+
+	dev_dbg(hu->tty->dev, "Sending alive packet...\n");
+
+	init_completion(&btdev->init_completion);
+
+	len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt);
+	skb = bt_skb_alloc(len, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT;
+	memset(skb->data, 0x00, len);
+
+	hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr));
+	hdr->dlen = sizeof(*pkt);
+	pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt));
+	pkt->mid = NOKIA_ALIVE_REQ;
+
+	hu->hdev->send(hu->hdev, skb);
+
+	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
+		msecs_to_jiffies(1000))) {
+		return -ETIMEDOUT;
+	}
+
+	if (btdev->init_error < 0)
+		return btdev->init_error;
+
+	return 0;
+}
+
+static int nokia_send_negotiation(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_neg_cmd *neg_cmd;
+	struct hci_nokia_neg_hdr *neg_hdr;
+	struct sk_buff *skb;
+	int len, err;
+	u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE);
+	int sysclk = btdev->btdata->sysclk_speed / 1000;
+
+	dev_dbg(hu->tty->dev, "Sending negotiation...\n");
+
+	len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd);
+	skb = bt_skb_alloc(len, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT;
+
+	neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
+	neg_hdr->dlen = sizeof(*neg_cmd);
+
+	neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
+	neg_cmd->ack = NOKIA_NEG_REQ;
+	neg_cmd->baud = cpu_to_le16(baud);
+	neg_cmd->unused1 = 0x0000;
+	neg_cmd->proto = NOKIA_PROTO_BYTE;
+	neg_cmd->sys_clk = cpu_to_le16(sysclk);
+	neg_cmd->unused2 = 0x0000;
+
+	btdev->init_error = 0;
+	init_completion(&btdev->init_completion);
+
+	hu->hdev->send(hu->hdev, skb);
+
+	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
+		msecs_to_jiffies(10000))) {
+		return -ETIMEDOUT;
+	}
+
+	if (btdev->init_error < 0)
+		return btdev->init_error;
+
+	/* Change to operational settings */
+	hci_uart_set_flow_control(hu, true); // disable flow control
+
+	/* setup negotiated max. baudrate */
+	hci_uart_set_baudrate(hu, MAX_BAUD_RATE);
+
+	err = hci_uart_wait_for_cts(hu, true, 100);
+	if (err < 0)
+		return err;
+
+	hci_uart_set_flow_control(hu, false); // re-enable flow control
+
+	dev_dbg(hu->tty->dev, "Negotiation successful...\n");
+
+	return 0;
+}
+
+static int nokia_setup_fw(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	const struct firmware *fw;
+	const u8 *fw_ptr;
+	size_t fw_size;
+	int err;
+
+	BT_DBG("hu %p", hu);
+
+	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);
+	if (err < 0) {
+		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
+		       hu->hdev->name, err);
+		return err;
+	}
+
+	fw_ptr = fw->data;
+	fw_size = fw->size;
+
+	while (fw_size >= 4) {
+		u16 pkt_size = get_unaligned_le16(fw_ptr);
+		u8 pkt_type = fw_ptr[2];
+		const struct hci_command_hdr *cmd;
+		u16 opcode;
+		struct sk_buff *skb;
+
+		switch (pkt_type) {
+		case HCI_COMMAND_PKT:
+			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
+			opcode = le16_to_cpu(cmd->opcode);
+
+			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
+					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
+					     HCI_INIT_TIMEOUT);
+			if (IS_ERR(skb)) {
+				err = PTR_ERR(skb);
+				BT_ERR("%s: Firmware command %04x failed (%d)",
+				       hu->hdev->name, opcode, err);
+				goto done;
+			}
+			kfree_skb(skb);
+			break;
+		case HCI_NOKIA_RADIO_PKT:
+		case HCI_NOKIA_NEG_PKT:
+		case HCI_NOKIA_ALIVE_PKT:
+			break;
+		}
+
+		fw_ptr += pkt_size + 2;
+		fw_size -= pkt_size + 2;
+	}
+
+done:
+	release_firmware(fw);
+	return err;
+}
+
+static int nokia_setup(struct hci_uart *hu)
+{
+	int err;
+
+	pm_runtime_get_sync(hu->tty->dev);
+
+	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n");
+
+	/* 0. reset connection */
+	err = nokia_reset(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Reset failed: %d\n", err);
+		goto out;
+	}
+
+	/* 1. negotiate speed etc */
+	err = nokia_send_negotiation(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Negotiation failed: %d\n", err);
+		goto out;
+	}
+
+	/* 2. verify correct setup using alive packet */
+	err = nokia_send_alive_packet(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Alive check failed: %d\n", err);
+		goto out;
+	}
+
+	/* 3. send firmware */
+	err = nokia_setup_fw(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Could not setup FW: %d\n", err);
+		goto out;
+	}
+
+	hci_uart_set_flow_control(hu, true);
+	hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE);
+	hci_uart_set_flow_control(hu, false);
+
+	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n");
+
+	/*
+	 * TODO:
+	 * disable wakeup_bt at this point and automatically enable it when
+	 * data is about to be written until all data has been written (+ some
+	 * delay).
+	 *
+	 * Since this is not yet support by the uart/tty kernel framework we
+	 * will always keep enabled the wakeup_bt gpio for now, so that the
+	 * bluetooth chip will never transit into idle modes.
+	 */
+
+out:
+	pm_runtime_put(hu->tty->dev);
+
+	return err;
+}
+
+static int nokia_open(struct hci_uart *hu)
+{
+	struct device *serialdev = hu->tty->dev;
+	struct nokia_bt_dev *btdev;
+	struct device *uartbtdev;
+	int err;
+
+	btdev = kzalloc(sizeof(*btdev), GFP_KERNEL);
+	if (!btdev)
+		return -ENOMEM;
+
+	btdev->hu = hu;
+
+	skb_queue_head_init(&btdev->txq);
+
+	uartbtdev = device_find_child(serialdev, NULL, btdev_match);
+	if (!uartbtdev) {
+		dev_err(serialdev, "bluetooth device node not found!\n");
+		return -ENODEV;
+	}
+
+	btdev->btdata = dev_get_drvdata(uartbtdev);
+	if (!btdev->btdata)
+		return -EINVAL;
+
+	hu->priv = btdev;
+
+	/* register handler for host wakeup gpio */
+	btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host);
+	err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler,
+		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+		"wakeup", btdev);
+	if (err) {
+		gpiod_set_value(btdev->btdata->reset, 0);
+		gpiod_set_value(btdev->btdata->wakeup_bt, 0);
+		return err;
+	}
+
+	dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n",
+		dev_name(uartbtdev));
+
+	pm_runtime_enable(hu->tty->dev);
+
+	return 0;
+}
+
+static int nokia_flush(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+
+	BT_DBG("hu %p", hu);
+
+	skb_queue_purge(&btdev->txq);
+
+	return 0;
+}
+
+static int nokia_close(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+
+	hu->priv = NULL;
+
+	BT_DBG("hu %p", hu);
+
+	skb_queue_purge(&btdev->txq);
+
+	kfree_skb(btdev->rx_skb);
+
+	free_irq(btdev->wake_irq, btdev);
+
+	/* disable module */
+	gpiod_set_value(btdev->btdata->reset, 0);
+	gpiod_set_value(btdev->btdata->wakeup_bt, 0);
+
+	hu->priv = NULL;
+	kfree(btdev);
+
+	pm_runtime_disable(hu->tty->dev);
+
+	return 0;
+}
+
+/* Enqueue frame for transmittion (padding, crc, etc) */
+static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	int err;
+
+	BT_DBG("hu %p skb %p", hu, skb);
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
+
+	/* Packets must be word aligned */
+	if (skb->len % 2) {
+		err = skb_pad(skb, 1);
+		if (err)
+			return err;
+		*skb_put(skb, 1) = 0x00;
+	}
+
+	skb_queue_tail(&btdev->txq, skb);
+
+	return 0;
+}
+
+static int nokia_recv_negotiation_packet(struct hci_dev *hdev,
+					 struct sk_buff *skb)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_neg_hdr *hdr;
+	struct hci_nokia_neg_evt *evt;
+	int ret = 0;
+
+	hdr = (struct hci_nokia_neg_hdr *)skb->data;
+	if (hdr->dlen != sizeof(*evt)) {
+		btdev->init_error = -EIO;
+		ret = -EIO;
+		goto finish_neg;
+	}
+
+	evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr));
+
+	if (evt->ack != NOKIA_NEG_ACK) {
+		dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n");
+		btdev->init_error = -EINVAL;
+	}
+
+	btdev->man_id = evt->man_id;
+	btdev->ver_id = evt->ver_id;
+
+	dev_dbg(hu->tty->dev, "NOKIA negotiation:\n");
+	dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud);
+	dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk);
+	dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id);
+	dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id);
+
+finish_neg:
+	complete(&btdev->init_completion);
+	kfree_skb(skb);
+	return ret;
+}
+
+static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_alive_hdr *hdr;
+	struct hci_nokia_alive_pkt *pkt;
+	int ret = 0;
+
+	hdr = (struct hci_nokia_alive_hdr *)skb->data;
+	if (hdr->dlen != sizeof(*pkt)) {
+		dev_err(hu->tty->dev, "Corrupted alive message\n");
+		btdev->init_error = -EIO;
+		ret = -EIO;
+		goto finish_alive;
+	}
+
+	pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr));
+
+	if (pkt->mid != NOKIA_ALIVE_RESP) {
+		dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n",
+			pkt->mid);
+		btdev->init_error = -EINVAL;
+		goto finish_alive;
+	}
+
+	dev_dbg(hu->tty->dev, "Received alive packet!\n");
+
+finish_alive:
+	complete(&btdev->init_completion);
+	kfree_skb(skb);
+	return ret;
+}
+
+static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* Packets received on the dedicated radio channel are
+	 * HCI events and so feed them back into the core.
+	 */
+	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
+	return hci_recv_frame(hdev, skb);
+}
+
+/* Recv data */
+static const struct h4_recv_pkt nokia_recv_pkts[] = {
+	{ NOKIA_RECV_ACL,	.recv = hci_recv_frame },
+	{ NOKIA_RECV_SCO,	.recv = hci_recv_frame },
+	{ NOKIA_RECV_EVENT,	.recv = hci_recv_frame },
+	{ NOKIA_RECV_ALIVE,	.recv = nokia_recv_alive_packet },
+	{ NOKIA_RECV_NEG,	.recv = nokia_recv_negotiation_packet },
+	{ NOKIA_RECV_RADIO,	.recv = nokia_recv_radio },
+};
+
+static int nokia_recv(struct hci_uart *hu, const void *data, int count)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	int err;
+
+	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
+		return -EUNATCH;
+
+	btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count,
+				  nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts));
+	if (IS_ERR(btdev->rx_skb)) {
+		err = PTR_ERR(btdev->rx_skb);
+		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
+		btdev->rx_skb = NULL;
+		return err;
+	}
+
+	return count;
+}
+
+static struct sk_buff *nokia_dequeue(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+
+	return skb_dequeue(&btdev->txq);
+}
+
+static const struct hci_uart_proto nokia_proto = {
+	.id		= HCI_UART_NOKIA,
+	.name		= "Nokia",
+	.open		= nokia_open,
+	.close		= nokia_close,
+	.recv		= nokia_recv,
+	.enqueue	= nokia_enqueue,
+	.dequeue	= nokia_dequeue,
+	.flush		= nokia_flush,
+	.setup		= nokia_setup,
+};
+
+static int nokia_bluetooth_probe(struct platform_device *pdev)
+{
+	struct nokia_uart_dev *btdata;
+	struct device *bcmdev = &pdev->dev;
+	struct clk *sysclk;
+	int err = 0;
+
+	if(!bcmdev->parent) {
+		dev_err(bcmdev, "parent device missing!\n");
+		return -ENODEV;
+	}
+
+	btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL);
+	if(!btdata)
+		return -ENOMEM;
+
+	btdata->dev = bcmdev;
+	dev_set_drvdata(bcmdev, btdata);
+
+	btdata->port = dev_get_drvdata(bcmdev->parent);
+	if(!btdata->port) {
+		dev_err(bcmdev, "port data missing in parent device!\n");
+		return -ENODEV;
+	}
+
+	btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(btdata->reset)) {
+		err = PTR_ERR(btdata->reset);
+		dev_err(bcmdev, "could not get reset gpio: %d\n", err);
+		return err;
+	}
+
+	btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN);
+	if (IS_ERR(btdata->wakeup_host)) {
+		err = PTR_ERR(btdata->wakeup_host);
+		dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err);
+		return err;
+	}
+
+
+	btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup",
+					    GPIOD_OUT_LOW);
+	if (IS_ERR(btdata->wakeup_bt)) {
+		err = PTR_ERR(btdata->wakeup_bt);
+		dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err);
+		return err;
+	}
+
+	sysclk = devm_clk_get(bcmdev, "sysclk");
+	if (IS_ERR(sysclk)) {
+		err = PTR_ERR(sysclk);
+		dev_err(bcmdev, "could not get sysclk: %d\n", err);
+		return err;
+	}
+
+	clk_prepare_enable(sysclk);
+	btdata->sysclk_speed = clk_get_rate(sysclk);
+	clk_disable_unprepare(sysclk);
+
+	dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent));
+	dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000);
+
+	/* TODO: open tty and setup line disector from kernel-side */
+
+	return err;
+}
+
+static const struct of_device_id nokia_bluetooth_of_match[] = {
+	{ .compatible = "nokia,brcm,bcm2048", },
+	{ .compatible = "nokia,ti,wl1271-bluetooth", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
+
+static struct platform_driver platform_nokia_driver = {
+	.driver = {
+		.name = "nokia-bluetooth",
+		.of_match_table = nokia_bluetooth_of_match,
+	},
+	.probe = nokia_bluetooth_probe,
+};
+
+int __init nokia_init(void)
+{
+	platform_driver_register(&platform_nokia_driver);
+	return hci_uart_register_proto(&nokia_proto);
+}
+
+int __exit nokia_deinit(void)
+{
+	platform_driver_unregister(&platform_nokia_driver);
+	return hci_uart_unregister_proto(&nokia_proto);
+}
diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h
new file mode 100644
index 000000000000..8c4d307840e5
--- /dev/null
+++ b/drivers/bluetooth/hci_nokia.h
@@ -0,0 +1,140 @@
+/*
+ *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#ifndef __HCI_NOKIA_H
+#define __HCI_NOKIA_H
+
+#define NOKIA_ID_CSR		0x02
+#define NOKIA_ID_BCM2048	0x04
+#define NOKIA_ID_TI1271		0x31
+
+#define FIRMWARE_CSR		"nokia/bc4fw.bin"
+#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
+#define FIRMWARE_TI1271		"nokia/ti1273.bin"
+
+#define NOKIA_BCM_BDADDR	0xfc01
+
+#define HCI_NOKIA_NEG_PKT	0x06
+#define HCI_NOKIA_ALIVE_PKT	0x07
+#define HCI_NOKIA_RADIO_PKT	0x08
+
+#define HCI_NOKIA_NEG_HDR_SIZE		1
+#define HCI_NOKIA_MAX_NEG_SIZE		255
+#define HCI_NOKIA_ALIVE_HDR_SIZE	1
+#define HCI_NOKIA_MAX_ALIVE_SIZE	255
+#define HCI_NOKIA_RADIO_HDR_SIZE	2
+#define HCI_NOKIA_MAX_RADIO_SIZE	255
+
+#define NOKIA_PROTO_PKT		0x44
+#define NOKIA_PROTO_BYTE	0x4c
+
+#define NOKIA_NEG_REQ		0x00
+#define NOKIA_NEG_ACK		0x20
+#define NOKIA_NEG_NAK		0x40
+
+#define H4_TYPE_SIZE		1
+
+#define NOKIA_RECV_ACL \
+	H4_RECV_ACL, \
+	.wordaligned = true
+
+#define NOKIA_RECV_SCO \
+	H4_RECV_SCO, \
+	.wordaligned = true
+
+#define NOKIA_RECV_EVENT \
+	H4_RECV_EVENT, \
+	.wordaligned = true
+
+#define NOKIA_RECV_ALIVE \
+	.type = HCI_NOKIA_ALIVE_PKT, \
+	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
+	.wordaligned = true
+
+#define NOKIA_RECV_NEG \
+	.type = HCI_NOKIA_NEG_PKT, \
+	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
+	.wordaligned = true
+
+#define NOKIA_RECV_RADIO \
+	.type = HCI_NOKIA_RADIO_PKT, \
+	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
+	.loff = 1, \
+	.lsize = 1, \
+	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
+	.wordaligned = true
+
+struct hci_nokia_neg_hdr {
+	__u8	dlen;
+} __packed;
+
+struct hci_nokia_neg_cmd {
+	__u8	ack;
+	__u16	baud;
+	__u16	unused1;
+	__u8	proto;
+	__u16	sys_clk;
+	__u16	unused2;
+} __packed;
+
+static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb)
+{
+	return (struct hci_nokia_neg_hdr *) skb->data;
+}
+
+#define NOKIA_ALIVE_REQ   0x55
+#define NOKIA_ALIVE_RESP  0xcc
+
+struct hci_nokia_alive_hdr {
+	__u8	dlen;
+} __packed;
+
+struct hci_nokia_alive_pkt {
+	__u8	mid;
+	__u8	unused;
+} __packed;
+
+static inline struct hci_nokia_alive_hdr *hci_nokia_alive_hdr(const struct sk_buff *skb)
+{
+	return (struct hci_nokia_alive_hdr *) skb->data;
+}
+
+struct hci_nokia_neg_evt {
+	__u8	ack;
+	__u16	baud;
+	__u16	unused1;
+	__u8	proto;
+	__u16	sys_clk;
+	__u16	unused2;
+	__u8	man_id;
+	__u8	ver_id;
+} __packed;
+
+#define BT_BAUDRATE_DIVIDER     384000000
+#define BC4_MAX_BAUD_RATE       3692300
+#define MAX_BAUD_RATE           921600
+#define INIT_SPEED              120000
+
+struct hci_nokia_radio_hdr {
+	__u8	evt;
+	__u8	dlen;
+} __packed;
+
+#endif
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index a7d67aec3632..314b243df996 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -35,7 +35,7 @@
 #define HCIUARTGETFLAGS		_IOR('U', 204, int)
 
 /* UART protocols */
-#define HCI_UART_MAX_PROTO	10
+#define HCI_UART_MAX_PROTO	11
 
 #define HCI_UART_H4	0
 #define HCI_UART_BCSP	1
@@ -47,6 +47,7 @@
 #define HCI_UART_BCM	7
 #define HCI_UART_QCA	8
 #define HCI_UART_AG6XX	9
+#define HCI_UART_NOKIA	10
 
 #define HCI_UART_RAW_DEVICE	0
 #define HCI_UART_RESET_ON_INIT	1
@@ -190,3 +191,8 @@ int qca_deinit(void);
 int ag6xx_init(void);
 int ag6xx_deinit(void);
 #endif
+
+#ifdef CONFIG_BT_HCIUART_NOKIA
+int nokia_init(void);
+int nokia_deinit(void);
+#endif
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1462507 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel <sre@kernel.org> wrote:
> This driver adds support for Nokia H4+ procotol used
> for example by Nokia's internet tablets (N770 - N950).
> ---
>  drivers/bluetooth/Kconfig     |  10 +
>  drivers/bluetooth/Makefile    |   1 +
>  drivers/bluetooth/hci_ldisc.c |   6 +
>  drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/bluetooth/hci_nokia.h | 140 ++++++++
>  drivers/bluetooth/hci_uart.h  |   8 +-
>  6 files changed, 898 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bluetooth/hci_nokia.c
>  create mode 100644 drivers/bluetooth/hci_nokia.h
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index cf50fd2e96df..c32d9d5ad1d2 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -86,6 +86,16 @@ config BT_HCIUART_H4
>
>           Say Y here to compile support for HCI UART (H4) protocol.
>
> +config BT_HCIUART_NOKIA
> +       bool "UART Nokia H4+ protocol support"

If the option is a bool, then you don't need module.h or any MODULE
macros, nor do you need any __exit or unregister related code.

Alternatively, if there is a use case for it to be modular, then maybe you
want to change the above to a tristate.

Paul.
--

> +       depends on BT_HCIUART
> +       help
> +         Nokia H4+ is serial protocol for communication between Bluetooth
> +         device and host. This protocol is required for Bluetooth devices
> +         with UART interface in Nokia devices.
> +
> +         Say Y here to compile support for Nokia's H4+ protocol.
> +
>  config BT_HCIUART_BCSP
>         bool "BCSP protocol support"
>         depends on BT_HCIUART


[...]

> +       { .compatible = "nokia,ti,wl1271-bluetooth", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> +
> +static struct platform_driver platform_nokia_driver = {
> +       .driver = {
> +               .name = "nokia-bluetooth",
> +               .of_match_table = nokia_bluetooth_of_match,
> +       },
> +       .probe = nokia_bluetooth_probe,
> +};
> +
> +int __init nokia_init(void)
> +{
> +       platform_driver_register(&platform_nokia_driver);
> +       return hci_uart_register_proto(&nokia_proto);
> +}
> +
> +int __exit nokia_deinit(void)
> +{
> +       platform_driver_unregister(&platform_nokia_driver);
> +       return hci_uart_unregister_proto(&nokia_proto);
> +}

[toc] | [prev] | [next] | [standalone]

#1462517 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Multipart message — attachments visible in raw view] — view raw

Hi Paul,

On Sun, Aug 14, 2016 at 07:54:28PM -0400, Paul Gortmaker wrote:
> On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > This driver adds support for Nokia H4+ procotol used
> > for example by Nokia's internet tablets (N770 - N950).
> >
> > [...]
> >
> > +config BT_HCIUART_NOKIA
> > +       bool "UART Nokia H4+ protocol support"
> 
> If the option is a bool, then you don't need module.h or any MODULE
> macros, nor do you need any __exit or unregister related code.
> 
> Alternatively, if there is a use case for it to be modular, then maybe you
> want to change the above to a tristate.

Actually it is bool and modular: It becomes part of the module
configured by the tristate CONFIG_BT_HCIUART option.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1463467 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

Hi Sebastian,

> This driver adds support for Nokia H4+ procotol used
> for example by Nokia's internet tablets (N770 - N950).
> ---
> drivers/bluetooth/Kconfig     |  10 +
> drivers/bluetooth/Makefile    |   1 +
> drivers/bluetooth/hci_ldisc.c |   6 +
> drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_nokia.h | 140 ++++++++
> drivers/bluetooth/hci_uart.h  |   8 +-
> 6 files changed, 898 insertions(+), 1 deletion(-)
> create mode 100644 drivers/bluetooth/hci_nokia.c
> create mode 100644 drivers/bluetooth/hci_nokia.h
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index cf50fd2e96df..c32d9d5ad1d2 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -86,6 +86,16 @@ config BT_HCIUART_H4
> 
> 	  Say Y here to compile support for HCI UART (H4) protocol.
> 
> +config BT_HCIUART_NOKIA
> +	bool "UART Nokia H4+ protocol support"
> +	depends on BT_HCIUART
> +	help
> +	  Nokia H4+ is serial protocol for communication between Bluetooth
> +	  device and host. This protocol is required for Bluetooth devices
> +	  with UART interface in Nokia devices.
> +
> +	  Say Y here to compile support for Nokia's H4+ protocol.
> +
> config BT_HCIUART_BCSP
> 	bool "BCSP protocol support"
> 	depends on BT_HCIUART
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 9c18939fc5c9..f7951646ee14 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)	+= hci_intel.o
> hci_uart-$(CONFIG_BT_HCIUART_BCM)	+= hci_bcm.o
> hci_uart-$(CONFIG_BT_HCIUART_QCA)	+= hci_qca.o
> hci_uart-$(CONFIG_BT_HCIUART_AG6XX)	+= hci_ag6xx.o
> +hci_uart-$(CONFIG_BT_HCIUART_NOKIA)	+= hci_nokia.o
> hci_uart-objs				:= $(hci_uart-y)
> 
> ccflags-y += -D__CHECK_ENDIAN__
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index dda97398c59a..83d0de94bf35 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -810,6 +810,9 @@ static int __init hci_uart_init(void)
> #ifdef CONFIG_BT_HCIUART_AG6XX
> 	ag6xx_init();
> #endif
> +#ifdef CONFIG_BT_HCIUART_NOKIA
> +	nokia_init();
> +#endif
> 
> 	return 0;
> }
> @@ -845,6 +848,9 @@ static void __exit hci_uart_exit(void)
> #ifdef CONFIG_BT_HCIUART_AG6XX
> 	ag6xx_deinit();
> #endif
> +#ifdef CONFIG_BT_HCIUART_NOKIA
> +	nokia_deinit();
> +#endif
> 
> 	/* Release tty registration of line discipline */
> 	err = tty_unregister_ldisc(N_HCI);
> diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
> new file mode 100644
> index 000000000000..efd4dd320838
> --- /dev/null
> +++ b/drivers/bluetooth/hci_nokia.c
> @@ -0,0 +1,734 @@
> +/*
> + *
> + *  Bluetooth HCI UART H4 driver with Nokia Extensions
> + *
> + *  Copyright (C) 2015 Marcel Holtmann <marcel@holtmann.org>
> + *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/fcntl.h>
> +#include <linux/interrupt.h>
> +#include <linux/ptrace.h>
> +#include <linux/poll.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/firmware.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/signal.h>
> +#include <linux/ioctl.h>
> +#include <linux/skbuff.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/gpio/consumer.h>
> +
> +#include <linux/unaligned/le_struct.h>

are you sure all these includes are needed?

> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include "hci_uart.h"
> +#include "hci_nokia.h"
> +
> +struct nokia_uart_dev {
> +	struct device *dev;
> +	struct tty_port *port;
> +	struct gpio_desc *reset;
> +	struct gpio_desc *wakeup_host;
> +	struct gpio_desc *wakeup_bt;
> +	unsigned long sysclk_speed;
> +};
> +
> +struct nokia_bt_dev {
> +	struct hci_uart *hu;
> +	struct nokia_uart_dev *btdata;
> +	int wake_irq;
> +	bool wake_state;
> +	struct sk_buff *rx_skb;
> +	struct sk_buff_head txq;
> +	bdaddr_t bdaddr;
> +
> +	int init_error;
> +	struct completion init_completion;
> +
> +	uint8_t man_id;
> +	uint8_t ver_id;
> +};
> +
> +static char *nokia_get_fw_name(struct nokia_bt_dev *btdev)
> +{
> +	switch (btdev->man_id) {
> +	case NOKIA_ID_CSR:
> +		return FIRMWARE_CSR;
> +	case NOKIA_ID_BCM2048:
> +		return FIRMWARE_BCM2048;
> +	case NOKIA_ID_TI1271:
> +		return FIRMWARE_TI1271;
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state,
> +				 int timeout_ms)
> +{
> +	unsigned long timeout;
> +	int signal;
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	for (;;) {
> +		signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS;
> +		if (!!signal == !!state) {
> +			dev_dbg(hu->tty->dev, "wait for cts... received!\n");
> +			return 0;
> +		}
> +		if (time_after(jiffies, timeout)) {
> +			dev_dbg(hu->tty->dev, "wait for cts... timeout!\n");
> +			return -ETIMEDOUT;
> +		}
> +		usleep_range(1000, 2000);
> +	}
> +}

This is a super odd function. No return value since we essentially have an endless loop.

> +
> +static int btdev_match(struct device *child, void *data)
> +{
> +	if (!strcmp(child->driver->name, "nokia-bluetooth"))
> +		return 1;
> +	else
> +		return 0;
> +}

Anything wrong with just return !strcmp?

> +
> +static irqreturn_t wakeup_handler(int irq, void *data)
> +{
> +	struct nokia_bt_dev *btdev = data;
> +	struct device *serialdev = btdev->hu->tty->dev;
> +	int wake_state = gpiod_get_value(btdev->btdata->wakeup_host);
> +
> +	dev_dbg(serialdev, "wakeup received: %d -> %d\n",
> +		btdev->wake_state, wake_state);
> +
> +	if (btdev->wake_state == wake_state)
> +		return IRQ_HANDLED;
> +
> +	if (wake_state)
> +		pm_runtime_get_sync(serialdev);
> +	else if (!wake_state)
> +		pm_runtime_put(serialdev);
> +
> +	btdev->wake_state = wake_state;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int nokia_reset(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	int err;
> +
> +	/* reset routine */
> +	gpiod_set_value_cansleep(btdev->btdata->reset, 0);
> +	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
> +
> +	msleep(50);
> +
> +	/* safety check */
> +	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
> +	if (err == 1) {
> +		dev_err(hu->tty->dev, "reset: host wakeup not low!\n");
> +		return -EPROTO;
> +	}
> +
> +	/* flush queues */
> +	tty_ldisc_flush(hu->tty);
> +	tty_driver_flush_buffer(hu->tty);
> +
> +	/* init uart */
> +	hci_uart_init_tty(hu);
> +	hci_uart_set_flow_control(hu, true);
> +	hci_uart_set_baudrate(hu, INIT_SPEED);
> +
> +	gpiod_set_value_cansleep(btdev->btdata->reset, 1);
> +	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 0);
> +
> +	msleep(100);
> +
> +	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
> +	if (err == 0) {
> +		dev_err(hu->tty->dev, "reset: host wakeup not high!\n");
> +		return -EPROTO;
> +	}
> +
> +	/* wait for cts */
> +	err = hci_uart_wait_for_cts(hu, true, 100);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "CTS not received: %d\n", err);
> +		return err;
> +	}
> +
> +	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
> +	hci_uart_set_flow_control(hu, false);
> +
> +	return 0;
> +}
> +
> +static int nokia_send_alive_packet(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_alive_hdr *hdr;
> +	struct hci_nokia_alive_pkt *pkt;
> +	struct sk_buff *skb;
> +	int len;
> +
> +	dev_dbg(hu->tty->dev, "Sending alive packet...\n");
> +
> +	init_completion(&btdev->init_completion);
> +
> +	len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt);
> +	skb = bt_skb_alloc(len, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT;
> +	memset(skb->data, 0x00, len);
> +
> +	hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr));
> +	hdr->dlen = sizeof(*pkt);
> +	pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt));
> +	pkt->mid = NOKIA_ALIVE_REQ;
> +
> +	hu->hdev->send(hu->hdev, skb);

I am not sure we want these to go through the Bluetooth core packet sending. They are not standard HCI packet and should stay within the driver. If you send them through the core they will cause problems with the monitor interface.

> +
> +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> +		msecs_to_jiffies(1000))) {
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (btdev->init_error < 0)
> +		return btdev->init_error;
> +
> +	return 0;
> +}
> +
> +static int nokia_send_negotiation(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_neg_cmd *neg_cmd;
> +	struct hci_nokia_neg_hdr *neg_hdr;
> +	struct sk_buff *skb;
> +	int len, err;
> +	u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE);
> +	int sysclk = btdev->btdata->sysclk_speed / 1000;
> +
> +	dev_dbg(hu->tty->dev, "Sending negotiation...\n");
> +
> +	len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd);
> +	skb = bt_skb_alloc(len, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT;
> +
> +	neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
> +	neg_hdr->dlen = sizeof(*neg_cmd);
> +
> +	neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
> +	neg_cmd->ack = NOKIA_NEG_REQ;
> +	neg_cmd->baud = cpu_to_le16(baud);
> +	neg_cmd->unused1 = 0x0000;
> +	neg_cmd->proto = NOKIA_PROTO_BYTE;
> +	neg_cmd->sys_clk = cpu_to_le16(sysclk);
> +	neg_cmd->unused2 = 0x0000;
> +
> +	btdev->init_error = 0;
> +	init_completion(&btdev->init_completion);
> +
> +	hu->hdev->send(hu->hdev, skb);
> +
> +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> +		msecs_to_jiffies(10000))) {
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (btdev->init_error < 0)
> +		return btdev->init_error;
> +
> +	/* Change to operational settings */
> +	hci_uart_set_flow_control(hu, true); // disable flow control

Please use a proper comment that explains also disabling flow control.
> +
> +	/* setup negotiated max. baudrate */
> +	hci_uart_set_baudrate(hu, MAX_BAUD_RATE);
> +
> +	err = hci_uart_wait_for_cts(hu, true, 100);
> +	if (err < 0)
> +		return err;
> +
> +	hci_uart_set_flow_control(hu, false); // re-enable flow control
> +
> +	dev_dbg(hu->tty->dev, "Negotiation successful...\n");
> +
> +	return 0;
> +}
> +
> +static int nokia_setup_fw(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	const struct firmware *fw;
> +	const u8 *fw_ptr;
> +	size_t fw_size;
> +	int err;
> +
> +	BT_DBG("hu %p", hu);
> +
> +	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);

So does this nokia_get_fw_name really needs to be a separate function? Or can this just be done right here in this function? I prefer it to be done where it is actually used. Unless you use that name in many places.

> +	if (err < 0) {
> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
> +		       hu->hdev->name, err);
> +		return err;
> +	}
> +
> +	fw_ptr = fw->data;
> +	fw_size = fw->size;
> +
> +	while (fw_size >= 4) {
> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
> +		u8 pkt_type = fw_ptr[2];
> +		const struct hci_command_hdr *cmd;
> +		u16 opcode;
> +		struct sk_buff *skb;
> +
> +		switch (pkt_type) {
> +		case HCI_COMMAND_PKT:
> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
> +			opcode = le16_to_cpu(cmd->opcode);
> +
> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
> +					     HCI_INIT_TIMEOUT);
> +			if (IS_ERR(skb)) {
> +				err = PTR_ERR(skb);
> +				BT_ERR("%s: Firmware command %04x failed (%d)",
> +				       hu->hdev->name, opcode, err);
> +				goto done;
> +			}
> +			kfree_skb(skb);
> +			break;
> +		case HCI_NOKIA_RADIO_PKT:

Are you sure you can ignore the RADIO_PKT commands. They are used to set up the FM radio parts of the chip. They are standard HCI commands (in the case of Broadcom at least). At minimum it should be added a comment here that you are ignoring them on purpose.

> +		case HCI_NOKIA_NEG_PKT:
> +		case HCI_NOKIA_ALIVE_PKT:

And here I would also a comment on why are we ignore these commands and driving this all by ourselves.

> +			break;
> +		}
> +
> +		fw_ptr += pkt_size + 2;
> +		fw_size -= pkt_size + 2;
> +	}
> +
> +done:
> +	release_firmware(fw);
> +	return err;
> +}
> +
> +static int nokia_setup(struct hci_uart *hu)
> +{
> +	int err;
> +
> +	pm_runtime_get_sync(hu->tty->dev);
> +
> +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n");
> +
> +	/* 0. reset connection */
> +	err = nokia_reset(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Reset failed: %d\n", err);
> +		goto out;
> +	}
> +
> +	/* 1. negotiate speed etc */
> +	err = nokia_send_negotiation(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Negotiation failed: %d\n", err);
> +		goto out;
> +	}
> +
> +	/* 2. verify correct setup using alive packet */
> +	err = nokia_send_alive_packet(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Alive check failed: %d\n", err);
> +		goto out;
> +	}
> +
> +	/* 3. send firmware */
> +	err = nokia_setup_fw(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Could not setup FW: %d\n", err);
> +		goto out;
> +	}
> +
> +	hci_uart_set_flow_control(hu, true);
> +	hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE);

I think this variable needs a better name if it is common for all vendors.

> +	hci_uart_set_flow_control(hu, false);
> +
> +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n");
> +
> +	/*
> +	 * TODO:
> +	 * disable wakeup_bt at this point and automatically enable it when
> +	 * data is about to be written until all data has been written (+ some
> +	 * delay).
> +	 *
> +	 * Since this is not yet support by the uart/tty kernel framework we
> +	 * will always keep enabled the wakeup_bt gpio for now, so that the
> +	 * bluetooth chip will never transit into idle modes.
> +	 */
> +
> +out:
> +	pm_runtime_put(hu->tty->dev);
> +
> +	return err;
> +}
> +
> +static int nokia_open(struct hci_uart *hu)
> +{
> +	struct device *serialdev = hu->tty->dev;
> +	struct nokia_bt_dev *btdev;
> +	struct device *uartbtdev;
> +	int err;
> +
> +	btdev = kzalloc(sizeof(*btdev), GFP_KERNEL);
> +	if (!btdev)
> +		return -ENOMEM;
> +
> +	btdev->hu = hu;
> +
> +	skb_queue_head_init(&btdev->txq);
> +
> +	uartbtdev = device_find_child(serialdev, NULL, btdev_match);
> +	if (!uartbtdev) {
> +		dev_err(serialdev, "bluetooth device node not found!\n");
> +		return -ENODEV;
> +	}
> +
> +	btdev->btdata = dev_get_drvdata(uartbtdev);
> +	if (!btdev->btdata)
> +		return -EINVAL;
> +
> +	hu->priv = btdev;
> +
> +	/* register handler for host wakeup gpio */
> +	btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host);
> +	err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler,
> +		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +		"wakeup", btdev);
> +	if (err) {
> +		gpiod_set_value(btdev->btdata->reset, 0);
> +		gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> +		return err;
> +	}
> +
> +	dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n",
> +		dev_name(uartbtdev));
> +
> +	pm_runtime_enable(hu->tty->dev);
> +
> +	return 0;
> +}
> +
> +static int nokia_flush(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +
> +	BT_DBG("hu %p", hu);
> +
> +	skb_queue_purge(&btdev->txq);
> +
> +	return 0;
> +}
> +
> +static int nokia_close(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +
> +	hu->priv = NULL;
> +
> +	BT_DBG("hu %p", hu);
> +
> +	skb_queue_purge(&btdev->txq);
> +
> +	kfree_skb(btdev->rx_skb);
> +
> +	free_irq(btdev->wake_irq, btdev);
> +
> +	/* disable module */
> +	gpiod_set_value(btdev->btdata->reset, 0);
> +	gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> +
> +	hu->priv = NULL;
> +	kfree(btdev);
> +
> +	pm_runtime_disable(hu->tty->dev);
> +
> +	return 0;
> +}
> +
> +/* Enqueue frame for transmittion (padding, crc, etc) */
> +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	int err;
> +
> +	BT_DBG("hu %p skb %p", hu, skb);
> +
> +	/* Prepend skb with frame type */
> +	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> +	/* Packets must be word aligned */
> +	if (skb->len % 2) {
> +		err = skb_pad(skb, 1);
> +		if (err)
> +			return err;
> +		*skb_put(skb, 1) = 0x00;
> +	}
> +
> +	skb_queue_tail(&btdev->txq, skb);
> +
> +	return 0;
> +}
> +
> +static int nokia_recv_negotiation_packet(struct hci_dev *hdev,
> +					 struct sk_buff *skb)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_neg_hdr *hdr;
> +	struct hci_nokia_neg_evt *evt;
> +	int ret = 0;
> +
> +	hdr = (struct hci_nokia_neg_hdr *)skb->data;
> +	if (hdr->dlen != sizeof(*evt)) {
> +		btdev->init_error = -EIO;
> +		ret = -EIO;
> +		goto finish_neg;
> +	}
> +
> +	evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr));
> +
> +	if (evt->ack != NOKIA_NEG_ACK) {
> +		dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n");
> +		btdev->init_error = -EINVAL;
> +	}
> +
> +	btdev->man_id = evt->man_id;
> +	btdev->ver_id = evt->ver_id;
> +
> +	dev_dbg(hu->tty->dev, "NOKIA negotiation:\n");
> +	dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud);
> +	dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk);
> +	dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id);
> +	dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id);
> +
> +finish_neg:
> +	complete(&btdev->init_completion);
> +	kfree_skb(skb);
> +	return ret;
> +}
> +
> +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_alive_hdr *hdr;
> +	struct hci_nokia_alive_pkt *pkt;
> +	int ret = 0;
> +
> +	hdr = (struct hci_nokia_alive_hdr *)skb->data;
> +	if (hdr->dlen != sizeof(*pkt)) {
> +		dev_err(hu->tty->dev, "Corrupted alive message\n");
> +		btdev->init_error = -EIO;
> +		ret = -EIO;
> +		goto finish_alive;
> +	}
> +
> +	pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr));
> +
> +	if (pkt->mid != NOKIA_ALIVE_RESP) {
> +		dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n",
> +			pkt->mid);
> +		btdev->init_error = -EINVAL;
> +		goto finish_alive;
> +	}
> +
> +	dev_dbg(hu->tty->dev, "Received alive packet!\n");
> +
> +finish_alive:
> +	complete(&btdev->init_completion);
> +	kfree_skb(skb);
> +	return ret;
> +}
> +
> +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	/* Packets received on the dedicated radio channel are
> +	 * HCI events and so feed them back into the core.
> +	 */
> +	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;

I think using hci_skb_pkt_type(skb) is correct here as well.

> +	return hci_recv_frame(hdev, skb);
> +}
> +
> +/* Recv data */
> +static const struct h4_recv_pkt nokia_recv_pkts[] = {
> +	{ NOKIA_RECV_ACL,	.recv = hci_recv_frame },
> +	{ NOKIA_RECV_SCO,	.recv = hci_recv_frame },
> +	{ NOKIA_RECV_EVENT,	.recv = hci_recv_frame },
> +	{ NOKIA_RECV_ALIVE,	.recv = nokia_recv_alive_packet },
> +	{ NOKIA_RECV_NEG,	.recv = nokia_recv_negotiation_packet },
> +	{ NOKIA_RECV_RADIO,	.recv = nokia_recv_radio },
> +};
> +
> +static int nokia_recv(struct hci_uart *hu, const void *data, int count)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	int err;
> +
> +	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> +		return -EUNATCH;
> +
> +	btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count,
> +				  nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts));
> +	if (IS_ERR(btdev->rx_skb)) {
> +		err = PTR_ERR(btdev->rx_skb);
> +		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
> +		btdev->rx_skb = NULL;
> +		return err;
> +	}
> +
> +	return count;
> +}
> +
> +static struct sk_buff *nokia_dequeue(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +
> +	return skb_dequeue(&btdev->txq);
> +}
> +
> +static const struct hci_uart_proto nokia_proto = {
> +	.id		= HCI_UART_NOKIA,
> +	.name		= "Nokia",
> +	.open		= nokia_open,
> +	.close		= nokia_close,
> +	.recv		= nokia_recv,
> +	.enqueue	= nokia_enqueue,
> +	.dequeue	= nokia_dequeue,
> +	.flush		= nokia_flush,
> +	.setup		= nokia_setup,
> +};
> +
> +static int nokia_bluetooth_probe(struct platform_device *pdev)
> +{
> +	struct nokia_uart_dev *btdata;
> +	struct device *bcmdev = &pdev->dev;
> +	struct clk *sysclk;
> +	int err = 0;
> +
> +	if(!bcmdev->parent) {
> +		dev_err(bcmdev, "parent device missing!\n");
> +		return -ENODEV;
> +	}
> +
> +	btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL);
> +	if(!btdata)
> +		return -ENOMEM;
> +
> +	btdata->dev = bcmdev;
> +	dev_set_drvdata(bcmdev, btdata);
> +
> +	btdata->port = dev_get_drvdata(bcmdev->parent);
> +	if(!btdata->port) {
> +		dev_err(bcmdev, "port data missing in parent device!\n");
> +		return -ENODEV;
> +	}
> +
> +	btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(btdata->reset)) {
> +		err = PTR_ERR(btdata->reset);
> +		dev_err(bcmdev, "could not get reset gpio: %d\n", err);
> +		return err;
> +	}
> +
> +	btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN);
> +	if (IS_ERR(btdata->wakeup_host)) {
> +		err = PTR_ERR(btdata->wakeup_host);
> +		dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err);
> +		return err;
> +	}
> +
> +
> +	btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup",
> +					    GPIOD_OUT_LOW);
> +	if (IS_ERR(btdata->wakeup_bt)) {
> +		err = PTR_ERR(btdata->wakeup_bt);
> +		dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err);
> +		return err;
> +	}
> +
> +	sysclk = devm_clk_get(bcmdev, "sysclk");
> +	if (IS_ERR(sysclk)) {
> +		err = PTR_ERR(sysclk);
> +		dev_err(bcmdev, "could not get sysclk: %d\n", err);
> +		return err;
> +	}
> +
> +	clk_prepare_enable(sysclk);
> +	btdata->sysclk_speed = clk_get_rate(sysclk);
> +	clk_disable_unprepare(sysclk);
> +
> +	dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent));
> +	dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000);
> +
> +	/* TODO: open tty and setup line disector from kernel-side */
> +
> +	return err;
> +}
> +
> +static const struct of_device_id nokia_bluetooth_of_match[] = {
> +	{ .compatible = "nokia,brcm,bcm2048", },
> +	{ .compatible = "nokia,ti,wl1271-bluetooth", },

Where is the CSR BC4 one here? I prefer if we only have support for the ones that are actually supported and detected. We can easily extend things later.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> +
> +static struct platform_driver platform_nokia_driver = {
> +	.driver = {
> +		.name = "nokia-bluetooth",
> +		.of_match_table = nokia_bluetooth_of_match,
> +	},
> +	.probe = nokia_bluetooth_probe,
> +};
> +
> +int __init nokia_init(void)
> +{
> +	platform_driver_register(&platform_nokia_driver);
> +	return hci_uart_register_proto(&nokia_proto);
> +}
> +
> +int __exit nokia_deinit(void)
> +{
> +	platform_driver_unregister(&platform_nokia_driver);
> +	return hci_uart_unregister_proto(&nokia_proto);
> +}
> diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h
> new file mode 100644
> index 000000000000..8c4d307840e5
> --- /dev/null
> +++ b/drivers/bluetooth/hci_nokia.h
> @@ -0,0 +1,140 @@
> +/*
> + *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#ifndef __HCI_NOKIA_H
> +#define __HCI_NOKIA_H

Lets not do a separate header here. Just move this all into hci_nokia.c. There is really zero benefit in the header.

> +
> +#define NOKIA_ID_CSR		0x02
> +#define NOKIA_ID_BCM2048	0x04
> +#define NOKIA_ID_TI1271		0x31
> +
> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"

If the CSR ones are not yet supported, then leave them out for now. We can add this later.

> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
> +
> +#define NOKIA_BCM_BDADDR	0xfc01

We have btbcm.[ch] for this.

> +
> +#define HCI_NOKIA_NEG_PKT	0x06
> +#define HCI_NOKIA_ALIVE_PKT	0x07
> +#define HCI_NOKIA_RADIO_PKT	0x08
> +
> +#define HCI_NOKIA_NEG_HDR_SIZE		1
> +#define HCI_NOKIA_MAX_NEG_SIZE		255
> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
> +
> +#define NOKIA_PROTO_PKT		0x44
> +#define NOKIA_PROTO_BYTE	0x4c
> +
> +#define NOKIA_NEG_REQ		0x00
> +#define NOKIA_NEG_ACK		0x20
> +#define NOKIA_NEG_NAK		0x40
> +
> +#define H4_TYPE_SIZE		1

I am not sure this define adds any overall value to the code.

> +
> +#define NOKIA_RECV_ACL \
> +	H4_RECV_ACL, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_SCO \
> +	H4_RECV_SCO, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_EVENT \
> +	H4_RECV_EVENT, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_ALIVE \
> +	.type = HCI_NOKIA_ALIVE_PKT, \
> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_NEG \
> +	.type = HCI_NOKIA_NEG_PKT, \
> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_RADIO \
> +	.type = HCI_NOKIA_RADIO_PKT, \
> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
> +	.loff = 1, \
> +	.lsize = 1, \
> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
> +	.wordaligned = true

For this ones I would have use the HCI event ones. My original patch had this:

+#define NOK_RECV_NEG \
+	.type = NOK_NEG_PKT, \
+	.hlen = NOK_NEG_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_ALIVE \
+	.type = NOK_ALIVE_PKT, \
+	.hlen = NOK_ALIVE_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_RADIO \
+	.type = NOK_RADIO_PKT, \
+	.hlen = HCI_EVENT_HDR_SIZE, \
+	.loff = 1, \
+	.lsize = 1, \
+	.maxlen = HCI_MAX_EVENT_SIZE
+
+static const struct h4_recv_pkt nok_recv_pkts[] = {
+	{ H4_RECV_ACL,    .recv = hci_recv_frame },
+	{ H4_RECV_SCO,    .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
+	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
+	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
+	{ NOK_RECV_RADIO, .recv = nok_recv_radio },

With just these simple defines at the top:

+#define NOK_NEG_PKT	0x06
+#define NOK_ALIVE_PKT	0x07
+#define NOK_RADIO_PKT	0x08
+
+#define NOK_NEG_HDR_SIZE	1
+#define NOK_ALIVE_HDR_SIZE	1

And I would prefer if we keep it like that.

> +
> +struct hci_nokia_neg_hdr {
> +	__u8	dlen;
> +} __packed;
> +
> +struct hci_nokia_neg_cmd {
> +	__u8	ack;
> +	__u16	baud;
> +	__u16	unused1;
> +	__u8	proto;
> +	__u16	sys_clk;
> +	__u16	unused2;
> +} __packed;
> +
> +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb)
> +{
> +	return (struct hci_nokia_neg_hdr *) skb->data;
> +}

What good is this inline? A define would be way better, if really needed.

> +
> +#define NOKIA_ALIVE_REQ   0x55
> +#define NOKIA_ALIVE_RESP  0xcc
> +
> +struct hci_nokia_alive_hdr {
> +	__u8	dlen;
> +} __packed;
> +
> +struct hci_nokia_alive_pkt {
> +	__u8	mid;
> +	__u8	unused;
> +} __packed;
> +
> +static inline struct hci_nokia_alive_hdr *hci_nokia_alive_hdr(const struct sk_buff *skb)
> +{
> +	return (struct hci_nokia_alive_hdr *) skb->data;
> +}
> +
> +struct hci_nokia_neg_evt {
> +	__u8	ack;
> +	__u16	baud;
> +	__u16	unused1;
> +	__u8	proto;
> +	__u16	sys_clk;
> +	__u16	unused2;
> +	__u8	man_id;
> +	__u8	ver_id;
> +} __packed;
> +
> +#define BT_BAUDRATE_DIVIDER     384000000
> +#define BC4_MAX_BAUD_RATE       3692300
> +#define MAX_BAUD_RATE           921600
> +#define INIT_SPEED              120000
> +
> +struct hci_nokia_radio_hdr {
> +	__u8	evt;
> +	__u8	dlen;
> +} __packed;
> +
> +#endif
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index a7d67aec3632..314b243df996 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -35,7 +35,7 @@
> #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> 
> /* UART protocols */
> -#define HCI_UART_MAX_PROTO	10
> +#define HCI_UART_MAX_PROTO	11
> 
> #define HCI_UART_H4	0
> #define HCI_UART_BCSP	1
> @@ -47,6 +47,7 @@
> #define HCI_UART_BCM	7
> #define HCI_UART_QCA	8
> #define HCI_UART_AG6XX	9
> +#define HCI_UART_NOKIA	10
> 
> #define HCI_UART_RAW_DEVICE	0
> #define HCI_UART_RESET_ON_INIT	1
> @@ -190,3 +191,8 @@ int qca_deinit(void);
> int ag6xx_init(void);
> int ag6xx_deinit(void);
> #endif
> +
> +#ifdef CONFIG_BT_HCIUART_NOKIA
> +int nokia_init(void);
> +int nokia_deinit(void);
> +#endif

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1463513 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

On Tuesday 16 August 2016 09:02:14 Marcel Holtmann wrote:
> > +static int nokia_setup_fw(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	const struct firmware *fw;
> > +	const u8 *fw_ptr;
> > +	size_t fw_size;
> > +	int err;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);
> 
> So does this nokia_get_fw_name really needs to be a separate function? Or can this just be done right here in this function? I prefer it to be done where it is actually used. Unless you use that name in many places.
> 
> > +	if (err < 0) {
> > +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
> > +		       hu->hdev->name, err);
> > +		return err;
> > +	}
> > +
> > +	fw_ptr = fw->data;
> > +	fw_size = fw->size;
> > +
> > +	while (fw_size >= 4) {
> > +		u16 pkt_size = get_unaligned_le16(fw_ptr);
> > +		u8 pkt_type = fw_ptr[2];
> > +		const struct hci_command_hdr *cmd;
> > +		u16 opcode;
> > +		struct sk_buff *skb;
> > +
> > +		switch (pkt_type) {
> > +		case HCI_COMMAND_PKT:
> > +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
> > +			opcode = le16_to_cpu(cmd->opcode);
> > +
> > +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
> > +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
> > +					     HCI_INIT_TIMEOUT);
> > +			if (IS_ERR(skb)) {
> > +				err = PTR_ERR(skb);
> > +				BT_ERR("%s: Firmware command %04x failed (%d)",
> > +				       hu->hdev->name, opcode, err);
> > +				goto done;
> > +			}
> > +			kfree_skb(skb);
> > +			break;
> > +		case HCI_NOKIA_RADIO_PKT:
> 
> Are you sure you can ignore the RADIO_PKT commands. They are used to set up the FM radio parts of the chip. They are standard HCI commands (in the case of Broadcom at least). At minimum it should be added a comment here that you are ignoring them on purpose.
> 
> > +		case HCI_NOKIA_NEG_PKT:
> > +		case HCI_NOKIA_ALIVE_PKT:
> 
> And here I would also a comment on why are we ignore these commands and driving this all by ourselves.
> 

Good question... In Pavel's version of bluetooth driver, which is
working on Nokia N900, is sent whole firmware at one __hci_cmd_sync
step. It does not skip any packets, plus he added this comment:

/* Note that this is timing-critical. If sending packets takes
 * too long, initialization will fail.
 */

So really, can we skip those packets? And is not this reason why
this bluetooth driver does not work on Nokia N900?

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1463593 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Tue, Aug 16, 2016 at 09:52:17AM +0200, Pali Rohár wrote:
> > > +		case HCI_NOKIA_RADIO_PKT:
> > 
> > Are you sure you can ignore the RADIO_PKT commands. They are
> > used to set up the FM radio parts of the chip. They are standard
> > HCI commands (in the case of Broadcom at least). At minimum it
> > should be added a comment here that you are ignoring them on
> > purpose.
> > 
> > > +		case HCI_NOKIA_NEG_PKT:
> > > +		case HCI_NOKIA_ALIVE_PKT:
> > 
> > And here I would also a comment on why are we ignore these
> > commands and driving this all by ourselves.
> > 
> 
> Good question... In Pavel's version of bluetooth driver, which is
> working on Nokia N900, is sent whole firmware at one __hci_cmd_sync
> step. It does not skip any packets, plus he added this comment:
> 
> /* Note that this is timing-critical. If sending packets takes
>  * too long, initialization will fail.
>  */
> 
> So really, can we skip those packets? And is not this reason why
> this bluetooth driver does not work on Nokia N900?

Let's have a look - here is Pavel's version:
https://lwn.net/Articles/627201/

In pseudocode:

while(true) {
    cmd = get_cmd_from_firmware();
    if (!cmd) break;
    __hci_cmd_sync(cmd);
}

This is not "whole firmware at one __hci_cmd_sync step", is it?
And obviously that wouldn't work. Next let's have a look at
"It does not skip any packets":

/* Skip first two packets */
if (++num <= 2)
    continue;

Which are HCI_NOKIA_NEG_PKT and HCI_NOKIA_ALIVE_PKT. Those are
open-coded. By using the packets from the firmware we could drop the
negotiation/alive functions from the driver and remove quite a few
lines of code. I think it should only be done after finding the N900
bug, though. I found it quite useful, that first communication does not
happen through the firmware file.

Surely the radio packet is not ignored, but that part is not used on
N950 and N900 fails at first packet, so no way to test the radio
packet handling. I probably should add a /* TODO: check how to
handle radio packets */ for the radio packet entry. Or implement it
the way Marcel suggested and hope that it just works once the other
bug is found.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1463579 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Multipart message — attachments visible in raw view] — view raw

Hi Marcel,

On Tue, Aug 16, 2016 at 09:02:14AM +0200, Marcel Holtmann wrote:
> [...]
> > +#include <linux/module.h>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/fcntl.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/ptrace.h>
> > +#include <linux/poll.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/firmware.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <linux/errno.h>
> > +#include <linux/string.h>
> > +#include <linux/signal.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/gpio/consumer.h>
> > +
> > +#include <linux/unaligned/le_struct.h>
> 
> are you sure all these includes are needed?

No. A few of them are from previous version of
the driver. I will clean this up in the next
version.

> [...]
> > +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state,
> > +				 int timeout_ms)
> > +{
> > +	unsigned long timeout;
> > +	int signal;
> > +
> > +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> > +	for (;;) {
> > +		signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS;
> > +		if (!!signal == !!state) {
> > +			dev_dbg(hu->tty->dev, "wait for cts... received!\n");
> > +			return 0;
> > +		}
> > +		if (time_after(jiffies, timeout)) {
> > +			dev_dbg(hu->tty->dev, "wait for cts... timeout!\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +		usleep_range(1000, 2000);
> > +	}
> > +}
> 
> This is a super odd function. No return value since we essentially
> have an endless loop.

I will rewrite it, so that the loop condition checks for timeout.

> > +
> > +static int btdev_match(struct device *child, void *data)
> > +{
> > +	if (!strcmp(child->driver->name, "nokia-bluetooth"))
> > +		return 1;
> > +	else
> > +		return 0;
> > +}
> 
> Anything wrong with just return !strcmp?

No. I had initially debug prints in the cases.

> > +static int nokia_send_alive_packet(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_alive_hdr *hdr;
> > +	struct hci_nokia_alive_pkt *pkt;
> > +	struct sk_buff *skb;
> > +	int len;
> > +
> > +	dev_dbg(hu->tty->dev, "Sending alive packet...\n");
> > +
> > +	init_completion(&btdev->init_completion);
> > +
> > +	len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt);
> > +	skb = bt_skb_alloc(len, GFP_KERNEL);
> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT;
> > +	memset(skb->data, 0x00, len);
> > +
> > +	hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr));
> > +	hdr->dlen = sizeof(*pkt);
> > +	pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt));
> > +	pkt->mid = NOKIA_ALIVE_REQ;
> > +
> > +	hu->hdev->send(hu->hdev, skb);
> 
> I am not sure we want these to go through the Bluetooth core
> packet sending. They are not standard HCI packet and should stay
> within the driver. If you send them through the core they will
> cause problems with the monitor interface.

ok. I will directly call nokia_enqueue().

> > +
> > +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> > +		msecs_to_jiffies(1000))) {
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (btdev->init_error < 0)
> > +		return btdev->init_error;
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_send_negotiation(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_neg_cmd *neg_cmd;
> > +	struct hci_nokia_neg_hdr *neg_hdr;
> > +	struct sk_buff *skb;
> > +	int len, err;
> > +	u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE);
> > +	int sysclk = btdev->btdata->sysclk_speed / 1000;
> > +
> > +	dev_dbg(hu->tty->dev, "Sending negotiation...\n");
> > +
> > +	len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd);
> > +	skb = bt_skb_alloc(len, GFP_KERNEL);
> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT;
> > +
> > +	neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
> > +	neg_hdr->dlen = sizeof(*neg_cmd);
> > +
> > +	neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
> > +	neg_cmd->ack = NOKIA_NEG_REQ;
> > +	neg_cmd->baud = cpu_to_le16(baud);
> > +	neg_cmd->unused1 = 0x0000;
> > +	neg_cmd->proto = NOKIA_PROTO_BYTE;
> > +	neg_cmd->sys_clk = cpu_to_le16(sysclk);
> > +	neg_cmd->unused2 = 0x0000;
> > +
> > +	btdev->init_error = 0;
> > +	init_completion(&btdev->init_completion);
> > +
> > +	hu->hdev->send(hu->hdev, skb);
> > +
> > +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> > +		msecs_to_jiffies(10000))) {
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (btdev->init_error < 0)
> > +		return btdev->init_error;
> > +
> > +	/* Change to operational settings */
> > +	hci_uart_set_flow_control(hu, true); // disable flow control
> 
> Please use a proper comment that explains also
> disabling flow control.

ok.

> > +
> > +	/* setup negotiated max. baudrate */
> > +	hci_uart_set_baudrate(hu, MAX_BAUD_RATE);
> > +
> > +	err = hci_uart_wait_for_cts(hu, true, 100);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	hci_uart_set_flow_control(hu, false); // re-enable flow control
> > +
> > +	dev_dbg(hu->tty->dev, "Negotiation successful...\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_setup_fw(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	const struct firmware *fw;
> > +	const u8 *fw_ptr;
> > +	size_t fw_size;
> > +	int err;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);
> 
> So does this nokia_get_fw_name really needs to be a separate
> function? Or can this just be done right here in this function? I
> prefer it to be done where it is actually used. Unless you use
> that name in many places.

I inlined it and dropped CSR support.

> > +	if (err < 0) {
> > +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
> > +		       hu->hdev->name, err);
> > +		return err;
> > +	}
> > +
> > +	fw_ptr = fw->data;
> > +	fw_size = fw->size;
> > +
> > +	while (fw_size >= 4) {
> > +		u16 pkt_size = get_unaligned_le16(fw_ptr);
> > +		u8 pkt_type = fw_ptr[2];
> > +		const struct hci_command_hdr *cmd;
> > +		u16 opcode;
> > +		struct sk_buff *skb;
> > +
> > +		switch (pkt_type) {
> > +		case HCI_COMMAND_PKT:
> > +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
> > +			opcode = le16_to_cpu(cmd->opcode);
> > +
> > +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
> > +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
> > +					     HCI_INIT_TIMEOUT);
> > +			if (IS_ERR(skb)) {
> > +				err = PTR_ERR(skb);
> > +				BT_ERR("%s: Firmware command %04x failed (%d)",
> > +				       hu->hdev->name, opcode, err);
> > +				goto done;
> > +			}
> > +			kfree_skb(skb);
> > +			break;
> > +		case HCI_NOKIA_RADIO_PKT:
> 
> Are you sure you can ignore the RADIO_PKT commands. They are used
> to set up the FM radio parts of the chip. They are standard HCI
> commands (in the case of Broadcom at least). At minimum it should
> be added a comment here that you are ignoring them on purpose.

I got the driver working on N950. I think it does not make use of
the radio packets at all. On N900 they may be needed, though. I do
not reach far enough in the firmware loading process to know for
sure.

If I remember correctly your template driver does bundle it together
with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
opcode size is u8 instead of u16. I ignored it for now, since I
could not properly test it.

> > +		case HCI_NOKIA_NEG_PKT:
> > +		case HCI_NOKIA_ALIVE_PKT:
> 
> And here I would also a comment on why are we ignore these
> commands and driving this all by ourselves.

I think we could use the packets from the firmware instead
of doing it manually (On N900 they are bit identical to the
manually generated one - On N950 I have not yet checked), but
until N900 works having it coded explicitly helps debugging.

> > +			break;
> > +		}
> > +
> > +		fw_ptr += pkt_size + 2;
> > +		fw_size -= pkt_size + 2;
> > +	}
> > +
> > +done:
> > +	release_firmware(fw);
> > +	return err;
> > +}
> > +
> > +static int nokia_setup(struct hci_uart *hu)
> > +{
> > +	int err;
> > +
> > +	pm_runtime_get_sync(hu->tty->dev);
> > +
> > +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n");
> > +
> > +	/* 0. reset connection */
> > +	err = nokia_reset(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Reset failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 1. negotiate speed etc */
> > +	err = nokia_send_negotiation(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Negotiation failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 2. verify correct setup using alive packet */
> > +	err = nokia_send_alive_packet(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Alive check failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 3. send firmware */
> > +	err = nokia_setup_fw(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Could not setup FW: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	hci_uart_set_flow_control(hu, true);
> > +	hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE);
> 
> I think this variable needs a better name if
> it is common for all vendors.

It is common. I will rename it to MAX_BAUD_RATE and
old MAX_BAUD_RATE to SETUP_BAUD_RATE.

> > +	hci_uart_set_flow_control(hu, false);
> > +
> > +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n");
> > +
> > +	/*
> > +	 * TODO:
> > +	 * disable wakeup_bt at this point and automatically enable it when
> > +	 * data is about to be written until all data has been written (+ some
> > +	 * delay).
> > +	 *
> > +	 * Since this is not yet support by the uart/tty kernel framework we
> > +	 * will always keep enabled the wakeup_bt gpio for now, so that the
> > +	 * bluetooth chip will never transit into idle modes.
> > +	 */
> > +
> > +out:
> > +	pm_runtime_put(hu->tty->dev);
> > +
> > +	return err;
> > +}
> > +
> > +static int nokia_open(struct hci_uart *hu)
> > +{
> > +	struct device *serialdev = hu->tty->dev;
> > +	struct nokia_bt_dev *btdev;
> > +	struct device *uartbtdev;
> > +	int err;
> > +
> > +	btdev = kzalloc(sizeof(*btdev), GFP_KERNEL);
> > +	if (!btdev)
> > +		return -ENOMEM;
> > +
> > +	btdev->hu = hu;
> > +
> > +	skb_queue_head_init(&btdev->txq);
> > +
> > +	uartbtdev = device_find_child(serialdev, NULL, btdev_match);
> > +	if (!uartbtdev) {
> > +		dev_err(serialdev, "bluetooth device node not found!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdev->btdata = dev_get_drvdata(uartbtdev);
> > +	if (!btdev->btdata)
> > +		return -EINVAL;
> > +
> > +	hu->priv = btdev;
> > +
> > +	/* register handler for host wakeup gpio */
> > +	btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host);
> > +	err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler,
> > +		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +		"wakeup", btdev);
> > +	if (err) {
> > +		gpiod_set_value(btdev->btdata->reset, 0);
> > +		gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> > +		return err;
> > +	}
> > +
> > +	dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n",
> > +		dev_name(uartbtdev));
> > +
> > +	pm_runtime_enable(hu->tty->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_flush(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	skb_queue_purge(&btdev->txq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_close(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	hu->priv = NULL;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	skb_queue_purge(&btdev->txq);
> > +
> > +	kfree_skb(btdev->rx_skb);
> > +
> > +	free_irq(btdev->wake_irq, btdev);
> > +
> > +	/* disable module */
> > +	gpiod_set_value(btdev->btdata->reset, 0);
> > +	gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> > +
> > +	hu->priv = NULL;
> > +	kfree(btdev);
> > +
> > +	pm_runtime_disable(hu->tty->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Enqueue frame for transmittion (padding, crc, etc) */
> > +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	int err;
> > +
> > +	BT_DBG("hu %p skb %p", hu, skb);
> > +
> > +	/* Prepend skb with frame type */
> > +	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> > +
> > +	/* Packets must be word aligned */
> > +	if (skb->len % 2) {
> > +		err = skb_pad(skb, 1);
> > +		if (err)
> > +			return err;
> > +		*skb_put(skb, 1) = 0x00;
> > +	}
> > +
> > +	skb_queue_tail(&btdev->txq, skb);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_recv_negotiation_packet(struct hci_dev *hdev,
> > +					 struct sk_buff *skb)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_neg_hdr *hdr;
> > +	struct hci_nokia_neg_evt *evt;
> > +	int ret = 0;
> > +
> > +	hdr = (struct hci_nokia_neg_hdr *)skb->data;
> > +	if (hdr->dlen != sizeof(*evt)) {
> > +		btdev->init_error = -EIO;
> > +		ret = -EIO;
> > +		goto finish_neg;
> > +	}
> > +
> > +	evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr));
> > +
> > +	if (evt->ack != NOKIA_NEG_ACK) {
> > +		dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n");
> > +		btdev->init_error = -EINVAL;
> > +	}
> > +
> > +	btdev->man_id = evt->man_id;
> > +	btdev->ver_id = evt->ver_id;
> > +
> > +	dev_dbg(hu->tty->dev, "NOKIA negotiation:\n");
> > +	dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud);
> > +	dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk);
> > +	dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id);
> > +	dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id);
> > +
> > +finish_neg:
> > +	complete(&btdev->init_completion);
> > +	kfree_skb(skb);
> > +	return ret;
> > +}
> > +
> > +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_alive_hdr *hdr;
> > +	struct hci_nokia_alive_pkt *pkt;
> > +	int ret = 0;
> > +
> > +	hdr = (struct hci_nokia_alive_hdr *)skb->data;
> > +	if (hdr->dlen != sizeof(*pkt)) {
> > +		dev_err(hu->tty->dev, "Corrupted alive message\n");
> > +		btdev->init_error = -EIO;
> > +		ret = -EIO;
> > +		goto finish_alive;
> > +	}
> > +
> > +	pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr));
> > +
> > +	if (pkt->mid != NOKIA_ALIVE_RESP) {
> > +		dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n",
> > +			pkt->mid);
> > +		btdev->init_error = -EINVAL;
> > +		goto finish_alive;
> > +	}
> > +
> > +	dev_dbg(hu->tty->dev, "Received alive packet!\n");
> > +
> > +finish_alive:
> > +	complete(&btdev->init_completion);
> > +	kfree_skb(skb);
> > +	return ret;
> > +}
> > +
> > +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	/* Packets received on the dedicated radio channel are
> > +	 * HCI events and so feed them back into the core.
> > +	 */
> > +	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
> 
> I think using hci_skb_pkt_type(skb) is correct here as well.

ok.

> > +	return hci_recv_frame(hdev, skb);
> > +}
> > +
> > +/* Recv data */
> > +static const struct h4_recv_pkt nokia_recv_pkts[] = {
> > +	{ NOKIA_RECV_ACL,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_SCO,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_EVENT,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_ALIVE,	.recv = nokia_recv_alive_packet },
> > +	{ NOKIA_RECV_NEG,	.recv = nokia_recv_negotiation_packet },
> > +	{ NOKIA_RECV_RADIO,	.recv = nokia_recv_radio },
> > +};
> > +
> > +static int nokia_recv(struct hci_uart *hu, const void *data, int count)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	int err;
> > +
> > +	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> > +		return -EUNATCH;
> > +
> > +	btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count,
> > +				  nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts));
> > +	if (IS_ERR(btdev->rx_skb)) {
> > +		err = PTR_ERR(btdev->rx_skb);
> > +		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
> > +		btdev->rx_skb = NULL;
> > +		return err;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> > +static struct sk_buff *nokia_dequeue(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	return skb_dequeue(&btdev->txq);
> > +}
> > +
> > +static const struct hci_uart_proto nokia_proto = {
> > +	.id		= HCI_UART_NOKIA,
> > +	.name		= "Nokia",
> > +	.open		= nokia_open,
> > +	.close		= nokia_close,
> > +	.recv		= nokia_recv,
> > +	.enqueue	= nokia_enqueue,
> > +	.dequeue	= nokia_dequeue,
> > +	.flush		= nokia_flush,
> > +	.setup		= nokia_setup,
> > +};
> > +
> > +static int nokia_bluetooth_probe(struct platform_device *pdev)
> > +{
> > +	struct nokia_uart_dev *btdata;
> > +	struct device *bcmdev = &pdev->dev;
> > +	struct clk *sysclk;
> > +	int err = 0;
> > +
> > +	if(!bcmdev->parent) {
> > +		dev_err(bcmdev, "parent device missing!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL);
> > +	if(!btdata)
> > +		return -ENOMEM;
> > +
> > +	btdata->dev = bcmdev;
> > +	dev_set_drvdata(bcmdev, btdata);
> > +
> > +	btdata->port = dev_get_drvdata(bcmdev->parent);
> > +	if(!btdata->port) {
> > +		dev_err(bcmdev, "port data missing in parent device!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(btdata->reset)) {
> > +		err = PTR_ERR(btdata->reset);
> > +		dev_err(bcmdev, "could not get reset gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN);
> > +	if (IS_ERR(btdata->wakeup_host)) {
> > +		err = PTR_ERR(btdata->wakeup_host);
> > +		dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +
> > +	btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup",
> > +					    GPIOD_OUT_LOW);
> > +	if (IS_ERR(btdata->wakeup_bt)) {
> > +		err = PTR_ERR(btdata->wakeup_bt);
> > +		dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	sysclk = devm_clk_get(bcmdev, "sysclk");
> > +	if (IS_ERR(sysclk)) {
> > +		err = PTR_ERR(sysclk);
> > +		dev_err(bcmdev, "could not get sysclk: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	clk_prepare_enable(sysclk);
> > +	btdata->sysclk_speed = clk_get_rate(sysclk);
> > +	clk_disable_unprepare(sysclk);
> > +
> > +	dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent));
> > +	dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000);
> > +
> > +	/* TODO: open tty and setup line disector from kernel-side */
> > +
> > +	return err;
> > +}
> > +
> > +static const struct of_device_id nokia_bluetooth_of_match[] = {
> > +	{ .compatible = "nokia,brcm,bcm2048", },
> > +	{ .compatible = "nokia,ti,wl1271-bluetooth", },
> 
> Where is the CSR BC4 one here? I prefer if we only have support
> for the ones that are actually supported and detected. We can
> easily extend things later.

I will drop CSR stuff. I don't have a device to test it.

> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> > +
> > +static struct platform_driver platform_nokia_driver = {
> > +	.driver = {
> > +		.name = "nokia-bluetooth",
> > +		.of_match_table = nokia_bluetooth_of_match,
> > +	},
> > +	.probe = nokia_bluetooth_probe,
> > +};
> > +
> > +int __init nokia_init(void)
> > +{
> > +	platform_driver_register(&platform_nokia_driver);
> > +	return hci_uart_register_proto(&nokia_proto);
> > +}
> > +
> > +int __exit nokia_deinit(void)
> > +{
> > +	platform_driver_unregister(&platform_nokia_driver);
> > +	return hci_uart_unregister_proto(&nokia_proto);
> > +}
> > diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h
> > new file mode 100644
> > index 000000000000..8c4d307840e5
> > --- /dev/null
> > +++ b/drivers/bluetooth/hci_nokia.h
> > @@ -0,0 +1,140 @@
> > +/*
> > + *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation; either version 2 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __HCI_NOKIA_H
> > +#define __HCI_NOKIA_H
> 
> Lets not do a separate header here. Just move this all into
> hci_nokia.c. There is really zero benefit in the header.

ok.

> > +
> > +#define NOKIA_ID_CSR		0x02
> > +#define NOKIA_ID_BCM2048	0x04
> > +#define NOKIA_ID_TI1271		0x31
> > +
> > +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
> 
> If the CSR ones are not yet supported, then leave them out for
> now. We can add this later.
> 
> > +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
> > +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
> > +
> > +#define NOKIA_BCM_BDADDR	0xfc01
> 
> We have btbcm.[ch] for this.

ah this is a leftover. Currently the driver does not set
set_bdaddr() callback, since it differs between ti and bcm backend.
It looks like btbcm_set_bdaddr() can be used for the broadcom based
chips, though.

> > +#define HCI_NOKIA_NEG_PKT	0x06
> > +#define HCI_NOKIA_ALIVE_PKT	0x07
> > +#define HCI_NOKIA_RADIO_PKT	0x08
> > +
> > +#define HCI_NOKIA_NEG_HDR_SIZE		1
> > +#define HCI_NOKIA_MAX_NEG_SIZE		255
> > +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
> > +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
> > +#define HCI_NOKIA_RADIO_HDR_SIZE	2
> > +#define HCI_NOKIA_MAX_RADIO_SIZE	255
> > +
> > +#define NOKIA_PROTO_PKT		0x44
> > +#define NOKIA_PROTO_BYTE	0x4c
> > +
> > +#define NOKIA_NEG_REQ		0x00
> > +#define NOKIA_NEG_ACK		0x20
> > +#define NOKIA_NEG_NAK		0x40
> > +
> > +#define H4_TYPE_SIZE		1
> 
> I am not sure this define adds any overall value to the code.
> 
> > +
> > +#define NOKIA_RECV_ACL \
> > +	H4_RECV_ACL, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_SCO \
> > +	H4_RECV_SCO, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_EVENT \
> > +	H4_RECV_EVENT, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_ALIVE \
> > +	.type = HCI_NOKIA_ALIVE_PKT, \
> > +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
> > +	.loff = 0, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_NEG \
> > +	.type = HCI_NOKIA_NEG_PKT, \
> > +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
> > +	.loff = 0, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_RADIO \
> > +	.type = HCI_NOKIA_RADIO_PKT, \
> > +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
> > +	.loff = 1, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
> > +	.wordaligned = true
> 
> For this ones I would have use the HCI event ones.
> My original patch had this:
> 
> +#define NOK_RECV_NEG \
> +	.type = NOK_NEG_PKT, \
> +	.hlen = NOK_NEG_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +#define NOK_RECV_ALIVE \
> +	.type = NOK_ALIVE_PKT, \
> +	.hlen = NOK_ALIVE_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +#define NOK_RECV_RADIO \
> +	.type = NOK_RADIO_PKT, \
> +	.hlen = HCI_EVENT_HDR_SIZE, \
> +	.loff = 1, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +static const struct h4_recv_pkt nok_recv_pkts[] = {
> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
> 
> With just these simple defines at the top:
> 
> +#define NOK_NEG_PKT	0x06
> +#define NOK_ALIVE_PKT	0x07
> +#define NOK_RADIO_PKT	0x08
> +
> +#define NOK_NEG_HDR_SIZE	1
> +#define NOK_ALIVE_HDR_SIZE	1
> 
> And I would prefer if we keep it like that.

ok. I used explicit defines, since it looks like
a copy/paste error otherwise.

> > +
> > +struct hci_nokia_neg_hdr {
> > +	__u8	dlen;
> > +} __packed;
> > +
> > +struct hci_nokia_neg_cmd {
> > +	__u8	ack;
> > +	__u16	baud;
> > +	__u16	unused1;
> > +	__u8	proto;
> > +	__u16	sys_clk;
> > +	__u16	unused2;
> > +} __packed;
> > +
> > +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb)
> > +{
> > +	return (struct hci_nokia_neg_hdr *) skb->data;
> > +}
> 
> What good is this inline? A define would be way better, if really
> needed.

I will drop hci_nokia_neg_hdr() and hci_nokia_alive_hdr() inlines

> [...]

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1463649 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

Hi Sebastian,

>>> +	if (err < 0) {
>>> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
>>> +		       hu->hdev->name, err);
>>> +		return err;
>>> +	}
>>> +
>>> +	fw_ptr = fw->data;
>>> +	fw_size = fw->size;
>>> +
>>> +	while (fw_size >= 4) {
>>> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
>>> +		u8 pkt_type = fw_ptr[2];
>>> +		const struct hci_command_hdr *cmd;
>>> +		u16 opcode;
>>> +		struct sk_buff *skb;
>>> +
>>> +		switch (pkt_type) {
>>> +		case HCI_COMMAND_PKT:
>>> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
>>> +			opcode = le16_to_cpu(cmd->opcode);
>>> +
>>> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
>>> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
>>> +					     HCI_INIT_TIMEOUT);
>>> +			if (IS_ERR(skb)) {
>>> +				err = PTR_ERR(skb);
>>> +				BT_ERR("%s: Firmware command %04x failed (%d)",
>>> +				       hu->hdev->name, opcode, err);
>>> +				goto done;
>>> +			}
>>> +			kfree_skb(skb);
>>> +			break;
>>> +		case HCI_NOKIA_RADIO_PKT:
>> 
>> Are you sure you can ignore the RADIO_PKT commands. They are used
>> to set up the FM radio parts of the chip. They are standard HCI
>> commands (in the case of Broadcom at least). At minimum it should
>> be added a comment here that you are ignoring them on purpose.
> 
> I got the driver working on N950. I think it does not make use of
> the radio packets at all. On N900 they may be needed, though. I do
> not reach far enough in the firmware loading process to know for
> sure.
> 
> If I remember correctly your template driver does bundle it together
> with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
> opcode size is u8 instead of u16. I ignored it for now, since I
> could not properly test it.

that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.

My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.

> 
>>> +		case HCI_NOKIA_NEG_PKT:
>>> +		case HCI_NOKIA_ALIVE_PKT:
>> 
>> And here I would also a comment on why are we ignore these
>> commands and driving this all by ourselves.
> 
> I think we could use the packets from the firmware instead
> of doing it manually (On N900 they are bit identical to the
> manually generated one - On N950 I have not yet checked), but
> until N900 works having it coded explicitly helps debugging.

We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.

>>> +
>>> +#define NOKIA_ID_CSR		0x02
>>> +#define NOKIA_ID_BCM2048	0x04
>>> +#define NOKIA_ID_TI1271		0x31
>>> +
>>> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
>> 
>> If the CSR ones are not yet supported, then leave them out for
>> now. We can add this later.
>> 
>>> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
>>> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
>>> +
>>> +#define NOKIA_BCM_BDADDR	0xfc01
>> 
>> We have btbcm.[ch] for this.
> 
> ah this is a leftover. Currently the driver does not set
> set_bdaddr() callback, since it differs between ti and bcm backend.
> It looks like btbcm_set_bdaddr() can be used for the broadcom based
> chips, though.

Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.

> 
>>> +#define HCI_NOKIA_NEG_PKT	0x06
>>> +#define HCI_NOKIA_ALIVE_PKT	0x07
>>> +#define HCI_NOKIA_RADIO_PKT	0x08
>>> +
>>> +#define HCI_NOKIA_NEG_HDR_SIZE		1
>>> +#define HCI_NOKIA_MAX_NEG_SIZE		255
>>> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
>>> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
>>> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
>>> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
>>> +
>>> +#define NOKIA_PROTO_PKT		0x44
>>> +#define NOKIA_PROTO_BYTE	0x4c
>>> +
>>> +#define NOKIA_NEG_REQ		0x00
>>> +#define NOKIA_NEG_ACK		0x20
>>> +#define NOKIA_NEG_NAK		0x40
>>> +
>>> +#define H4_TYPE_SIZE		1
>> 
>> I am not sure this define adds any overall value to the code.
>> 
>>> +
>>> +#define NOKIA_RECV_ACL \
>>> +	H4_RECV_ACL, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_SCO \
>>> +	H4_RECV_SCO, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_EVENT \
>>> +	H4_RECV_EVENT, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_ALIVE \
>>> +	.type = HCI_NOKIA_ALIVE_PKT, \
>>> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_NEG \
>>> +	.type = HCI_NOKIA_NEG_PKT, \
>>> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_RADIO \
>>> +	.type = HCI_NOKIA_RADIO_PKT, \
>>> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
>>> +	.loff = 1, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
>>> +	.wordaligned = true
>> 
>> For this ones I would have use the HCI event ones.
>> My original patch had this:
>> 
>> +#define NOK_RECV_NEG \
>> +	.type = NOK_NEG_PKT, \
>> +	.hlen = NOK_NEG_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_ALIVE \
>> +	.type = NOK_ALIVE_PKT, \
>> +	.hlen = NOK_ALIVE_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_RADIO \
>> +	.type = NOK_RADIO_PKT, \
>> +	.hlen = HCI_EVENT_HDR_SIZE, \
>> +	.loff = 1, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +static const struct h4_recv_pkt nok_recv_pkts[] = {
>> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
>> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
>> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
>> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
>> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
>> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
>> 
>> With just these simple defines at the top:
>> 
>> +#define NOK_NEG_PKT	0x06
>> +#define NOK_ALIVE_PKT	0x07
>> +#define NOK_RADIO_PKT	0x08
>> +
>> +#define NOK_NEG_HDR_SIZE	1
>> +#define NOK_ALIVE_HDR_SIZE	1
>> 
>> And I would prefer if we keep it like that.
> 
> ok. I used explicit defines, since it looks like
> a copy/paste error otherwise.

With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.

An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1463651 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

Hi Sebastian,

>>> +	if (err < 0) {
>>> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
>>> +		       hu->hdev->name, err);
>>> +		return err;
>>> +	}
>>> +
>>> +	fw_ptr = fw->data;
>>> +	fw_size = fw->size;
>>> +
>>> +	while (fw_size >= 4) {
>>> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
>>> +		u8 pkt_type = fw_ptr[2];
>>> +		const struct hci_command_hdr *cmd;
>>> +		u16 opcode;
>>> +		struct sk_buff *skb;
>>> +
>>> +		switch (pkt_type) {
>>> +		case HCI_COMMAND_PKT:
>>> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
>>> +			opcode = le16_to_cpu(cmd->opcode);
>>> +
>>> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
>>> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
>>> +					     HCI_INIT_TIMEOUT);
>>> +			if (IS_ERR(skb)) {
>>> +				err = PTR_ERR(skb);
>>> +				BT_ERR("%s: Firmware command %04x failed (%d)",
>>> +				       hu->hdev->name, opcode, err);
>>> +				goto done;
>>> +			}
>>> +			kfree_skb(skb);
>>> +			break;
>>> +		case HCI_NOKIA_RADIO_PKT:
>> 
>> Are you sure you can ignore the RADIO_PKT commands. They are used
>> to set up the FM radio parts of the chip. They are standard HCI
>> commands (in the case of Broadcom at least). At minimum it should
>> be added a comment here that you are ignoring them on purpose.
> 
> I got the driver working on N950. I think it does not make use of
> the radio packets at all. On N900 they may be needed, though. I do
> not reach far enough in the firmware loading process to know for
> sure.
> 
> If I remember correctly your template driver does bundle it together
> with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
> opcode size is u8 instead of u16. I ignored it for now, since I
> could not properly test it.

that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.

My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.

> 
>>> +		case HCI_NOKIA_NEG_PKT:
>>> +		case HCI_NOKIA_ALIVE_PKT:
>> 
>> And here I would also a comment on why are we ignore these
>> commands and driving this all by ourselves.
> 
> I think we could use the packets from the firmware instead
> of doing it manually (On N900 they are bit identical to the
> manually generated one - On N950 I have not yet checked), but
> until N900 works having it coded explicitly helps debugging.

We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.

>>> +
>>> +#define NOKIA_ID_CSR		0x02
>>> +#define NOKIA_ID_BCM2048	0x04
>>> +#define NOKIA_ID_TI1271		0x31
>>> +
>>> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
>> 
>> If the CSR ones are not yet supported, then leave them out for
>> now. We can add this later.
>> 
>>> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
>>> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
>>> +
>>> +#define NOKIA_BCM_BDADDR	0xfc01
>> 
>> We have btbcm.[ch] for this.
> 
> ah this is a leftover. Currently the driver does not set
> set_bdaddr() callback, since it differs between ti and bcm backend.
> It looks like btbcm_set_bdaddr() can be used for the broadcom based
> chips, though.

Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.

> 
>>> +#define HCI_NOKIA_NEG_PKT	0x06
>>> +#define HCI_NOKIA_ALIVE_PKT	0x07
>>> +#define HCI_NOKIA_RADIO_PKT	0x08
>>> +
>>> +#define HCI_NOKIA_NEG_HDR_SIZE		1
>>> +#define HCI_NOKIA_MAX_NEG_SIZE		255
>>> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
>>> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
>>> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
>>> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
>>> +
>>> +#define NOKIA_PROTO_PKT		0x44
>>> +#define NOKIA_PROTO_BYTE	0x4c
>>> +
>>> +#define NOKIA_NEG_REQ		0x00
>>> +#define NOKIA_NEG_ACK		0x20
>>> +#define NOKIA_NEG_NAK		0x40
>>> +
>>> +#define H4_TYPE_SIZE		1
>> 
>> I am not sure this define adds any overall value to the code.
>> 
>>> +
>>> +#define NOKIA_RECV_ACL \
>>> +	H4_RECV_ACL, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_SCO \
>>> +	H4_RECV_SCO, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_EVENT \
>>> +	H4_RECV_EVENT, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_ALIVE \
>>> +	.type = HCI_NOKIA_ALIVE_PKT, \
>>> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_NEG \
>>> +	.type = HCI_NOKIA_NEG_PKT, \
>>> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_RADIO \
>>> +	.type = HCI_NOKIA_RADIO_PKT, \
>>> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
>>> +	.loff = 1, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
>>> +	.wordaligned = true
>> 
>> For this ones I would have use the HCI event ones.
>> My original patch had this:
>> 
>> +#define NOK_RECV_NEG \
>> +	.type = NOK_NEG_PKT, \
>> +	.hlen = NOK_NEG_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_ALIVE \
>> +	.type = NOK_ALIVE_PKT, \
>> +	.hlen = NOK_ALIVE_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_RADIO \
>> +	.type = NOK_RADIO_PKT, \
>> +	.hlen = HCI_EVENT_HDR_SIZE, \
>> +	.loff = 1, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +static const struct h4_recv_pkt nok_recv_pkts[] = {
>> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
>> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
>> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
>> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
>> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
>> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
>> 
>> With just these simple defines at the top:
>> 
>> +#define NOK_NEG_PKT	0x06
>> +#define NOK_ALIVE_PKT	0x07
>> +#define NOK_RADIO_PKT	0x08
>> +
>> +#define NOK_NEG_HDR_SIZE	1
>> +#define NOK_ALIVE_HDR_SIZE	1
>> 
>> And I would prefer if we keep it like that.
> 
> ok. I used explicit defines, since it looks like
> a copy/paste error otherwise.

With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.

An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1464042 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

Hi!

> > I think we could use the packets from the firmware instead
> > of doing it manually (On N900 they are bit identical to the
> > manually generated one - On N950 I have not yet checked), but
> > until N900 works having it coded explicitly helps debugging.
> 
> We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.
> 

Editing firmware may raise some "interesting" legal questions... and
its redistribution might be tricky, too. Microsoft is not likely to be
too helpful :-(.

Best,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1463522 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

Hi Sebastien,

> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index a7d67aec3632..314b243df996 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -35,7 +35,7 @@
> #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> 
> /* UART protocols */
> -#define HCI_UART_MAX_PROTO	10
> +#define HCI_UART_MAX_PROTO	11
> 
> #define HCI_UART_H4	0
> #define HCI_UART_BCSP	1
> @@ -47,6 +47,7 @@
> #define HCI_UART_BCM	7
> #define HCI_UART_QCA	8
> #define HCI_UART_AG6XX	9
> +#define HCI_UART_NOKIA	10

since it seems the driver is getting closer to be ready, lets merge this extra protocol identifier as a separate patch. Then we can adapt btattach as well for it.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1463605 — Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Multipart message — attachments visible in raw view] — view raw

Hi Marcel,

On Tue, Aug 16, 2016 at 10:10:07AM +0200, Marcel Holtmann wrote:
> > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> > index a7d67aec3632..314b243df996 100644
> > --- a/drivers/bluetooth/hci_uart.h
> > +++ b/drivers/bluetooth/hci_uart.h
> > @@ -35,7 +35,7 @@
> > #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> > 
> > /* UART protocols */
> > -#define HCI_UART_MAX_PROTO	10
> > +#define HCI_UART_MAX_PROTO	11
> > 
> > #define HCI_UART_H4	0
> > #define HCI_UART_BCSP	1
> > @@ -47,6 +47,7 @@
> > #define HCI_UART_BCM	7
> > #define HCI_UART_QCA	8
> > #define HCI_UART_AG6XX	9
> > +#define HCI_UART_NOKIA	10
> 
> since it seems the driver is getting closer to be ready,
> lets merge this extra protocol identifier as a separate patch.

depends on the definition of "the driver". It requires
the gpio information from DT, so it depends on the new
serial bus. I still have to implement that and it has
the potential to trigger a bit of discussion :)

Anyways I will split this out into its own patch later. 

> Then we can adapt btattach as well for it.

It's enough to set ldisc=N_HCI and proto=HCI_UART_NOKIA.
Everything else is done by the protocol driver. I still
think it should be done from the kernel and will try to
come up with something in the serial bus.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1463475

Hi Sebastian,

> This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
> N9xx devices. It has been tested on the Nokia N950, where it works
> correctly. On Nokia N900 it currently fails during negotiation
> (probably related to slightly incorrect serial settings/timings).
> The N900's bcm2048 correctly answeres to alive check even before
> negotiation (on N950 it does not work before negotiation), but replies
> with an Hardware error event to the negotiation packet.
> 
> Apart from N900 support there are still two "features" missing in the
> driver:
> 
> 1. To save energy the bluetooth module can be put into sleep mode via a
>   GPIO. This gpio should be enabled before sending data via UART and
>   disabled once the transmission is done. I currently just keep the
>   GPIO always enabled.
> 2. It would be nice to have a bluetooth device exposed by the kernel
>   automatically without having to setup the tty disector first for
>   proper configurationless out of the box support. I could not find
>   a nice way to do this from the kernel, though.

currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.

However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.

This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.

This goes along with the weird fetish to expose certain Bluetooth GPIOs as RFKILL switches. They are not that either since they just power the UART. They have nothing to do with a radio RFKILL switch. We fixed the Intel and Broadcom ones to be mapped into the driver. Having a proper bus would make this one also a lot easier.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1464063

On Tue, Aug 16, 2016 at 2:10 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Sebastian,
>
>> This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
>> N9xx devices. It has been tested on the Nokia N950, where it works
>> correctly. On Nokia N900 it currently fails during negotiation
>> (probably related to slightly incorrect serial settings/timings).
>> The N900's bcm2048 correctly answeres to alive check even before
>> negotiation (on N950 it does not work before negotiation), but replies
>> with an Hardware error event to the negotiation packet.
>>
>> Apart from N900 support there are still two "features" missing in the
>> driver:
>>
>> 1. To save energy the bluetooth module can be put into sleep mode via a
>>   GPIO. This gpio should be enabled before sending data via UART and
>>   disabled once the transmission is done. I currently just keep the
>>   GPIO always enabled.
>> 2. It would be nice to have a bluetooth device exposed by the kernel
>>   automatically without having to setup the tty disector first for
>>   proper configurationless out of the box support. I could not find
>>   a nice way to do this from the kernel, though.
>
> currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.
>
> However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.
>
> This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.

To add to this, my initial thought has been to split off uart_port ops
for use by the uart bus/subsystem so we can reuse all the uart
drivers. Then driver ports can be registered either with the tty layer
or the uart bus (or perhaps registered with both and claimed by one
later). This is somewhat already done with the serio subsys (see
drivers/tty/serial/sunsu.c). I thought extending serio might be an
option, but it's data handling is pretty limited.

The transmit side seems pretty straightforward. The receive side is a
bit more complex as the buffering is all in the tty layer. It doesn't
seem so straightforward to share the tty buffer handling code. Perhaps
at least initially, a more simple buffering scheme can be used given
the packet oriented nature of the device protocols.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web