Hi Marcel,

On Tue, Aug 16, 2016 at 09:02:14AM +0200, Marcel Holtmann wrote:

[...]

quoted

+#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.

[...]

quoted

+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.

quoted

+
+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.

quoted

+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().

quoted

+
+	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.

quoted

+
+	/* 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.

quoted

+	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.

quoted

+		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.

quoted

+			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.

quoted

+	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.

quoted

+	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.

quoted

+	{},
+};
+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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

quoted

+
+#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.

quoted

+#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.

quoted

+#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.

quoted

+
+#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.

quoted

+
+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