Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver
From: Sebastian Reichel
Date: Tue Aug 16 2016 - 05:10:21 EST
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@xxxxxxxxxx>
> > + *
> > + * 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
Attachment:
signature.asc
Description: PGP signature