Message-ID: <20160816090952.ur35t5bbcr7hsxvj@earth>
Date:	Tue, 16 Aug 2016 11:09:52 +0200
From:	Sebastian Reichel <sre@...nel.org>
To:	Marcel Holtmann <marcel@...tmann.org>
Cc:	Tony Lindgren <tony@...mide.com>, Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>, Ville Tervo <ville.tervo@....fi>,
	Filip Matijević <filip.matijevic.pz@...il.com>,
	Aaro Koskinen <aaro.koskinen@....fi>,
	Pavel Machek <pavel@....cz>,
	Pali Rohár <pali.rohar@...il.com>,
	ivo.g.dimitrov.75@...il.com, linux-bluetooth@...r.kernel.org,
	linux-serial@...r.kernel.org, linux-omap@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

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@...nel.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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists