lkml.org 
[lkml]   [2010]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900
    From
    Date
    Hi Par-Gunnar,

    > Sorry for not answering to your mail earlier. I've been on a business
    > trip whole last week where I just did not have the possibility to
    > answer.
    >
    > We are currently working on a new version of our driver, both the MFD
    > and the Bluetooth part, where we address the comments that we have so
    > far received. Hopefully I will able to send it later this week.
    >
    > As answer to last mail: yes, we will change our debug system to be
    > reuse existing functionality in the Kernel.

    sounds good to me. dynamic debug is actually pretty nice :)

    >
    > /P-G
    >
    > 2010/10/5 Marcel Holtmann <marcel@holtmann.org>:
    > > Hi Par-Gunnar,
    > >
    > >> This patch adds support for using the ST-Ericsson CG2900
    > >> connectivity controller as a driver for the BlueZ Bluetooth
    > >> stack.
    > >> This patch registers as a driver into the BlueZ framework and, when
    > >> opened by BlueZ, it registers as user for bt_cmd, bt_acl, and bt_evt
    > >> channels.
    > >>
    > >> Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
    > >> ---
    > >> drivers/bluetooth/Kconfig | 7 +
    > >> drivers/bluetooth/Makefile | 2 +
    > >> drivers/bluetooth/cg2900_hci.c | 896 ++++++++++++++++++++++++++++++++++++++++
    > >> 3 files changed, 905 insertions(+), 0 deletions(-)
    > >> create mode 100644 drivers/bluetooth/cg2900_hci.c
    > >>
    > >> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
    > >> index 02deef4..9ca8d69 100644
    > >> --- a/drivers/bluetooth/Kconfig
    > >> +++ b/drivers/bluetooth/Kconfig
    > >> @@ -219,4 +219,11 @@ config BT_ATH3K
    > >> Say Y here to compile support for "Atheros firmware download driver"
    > >> into the kernel or say M to compile it as module (ath3k).
    > >>
    > >> +config BT_CG2900
    > >> + tristate "ST-Ericsson CG2900 driver"
    > >> + depends on MFD_CG2900 && BT
    > >> + help
    > >> + Select if ST-Ericsson CG2900 Connectivity controller shall be used as
    > >> + Bluetooth controller for BlueZ.
    > >> +
    > >> endmenu
    > >> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
    > >> index 71bdf13..a479c16 100644
    > >> --- a/drivers/bluetooth/Makefile
    > >> +++ b/drivers/bluetooth/Makefile
    > >> @@ -19,6 +19,8 @@ obj-$(CONFIG_BT_ATH3K) += ath3k.o
    > >> obj-$(CONFIG_BT_MRVL) += btmrvl.o
    > >> obj-$(CONFIG_BT_MRVL_SDIO) += btmrvl_sdio.o
    > >>
    > >> +obj-$(CONFIG_BT_CG2900) += cg2900_hci.o
    > >> +
    > >
    > > Please sort this after ath3k and before btmvrl config.
    > >
    > > And for the name either just use cg2900 if it uniquely identifies the
    > > Bluetooth chip used in the combo or use btcg2900.o as module name if it
    > > is the name of the combo module.
    > >
    > > We clearly moved into the direction of either unique hardware names
    > > without bt prefix or we have the bt prefix in front of it. The fully
    > > generic term hci will be phased out of the driver names.
    > >
    >
    > I can change the name to btcg2900.o since the whole chip is called
    > cg2900 and this file is just for the BT part of it.
    > I will also sort it correctly into the file.

    Actually btcg2900.ko sounds good to me.

    > >> btmrvl-y := btmrvl_main.o
    > >> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
    > >>
    > >> diff --git a/drivers/bluetooth/cg2900_hci.c b/drivers/bluetooth/cg2900_hci.c
    > >> new file mode 100644
    > >> index 0000000..de1ada8
    > >> --- /dev/null
    > >> +++ b/drivers/bluetooth/cg2900_hci.c
    > >> @@ -0,0 +1,896 @@
    > >> +/*
    > >> + * drivers/bluetooth/cg2900_hci.c
    > >> + *
    > >> + * Copyright (C) ST-Ericsson SA 2010
    > >> + * Authors:
    > >> + * Par-Gunnar Hjalmdahl (par-gunnar.p.hjalmdahl@stericsson.com) for
    > >> ST-Ericsson.
    > >> + * Henrik Possung (henrik.possung@stericsson.com) for ST-Ericsson.
    > >> + * Josef Kindberg (josef.kindberg@stericsson.com) for ST-Ericsson.
    > >> + * Dariusz Szymszak (dariusz.xd.szymczak@stericsson.com) for ST-Ericsson.
    > >> + * Kjell Andersson (kjell.k.andersson@stericsson.com) for ST-Ericsson.
    > >> + * License terms: GNU General Public License (GPL), version 2
    > >> + *
    > >> + * Linux Bluetooth HCI H:4 Driver for ST-Ericsson CG2900 connectivity
    > >> controller
    > >> + * towards the BlueZ Bluetooth stack.
    > >> + */
    > >
    > > Drop the filename in this header and don't bother with the description
    > > towards BlueZ or Bluetooth subsystem. That is clear from the location of
    > > the file.
    > >
    > > Also what is up with the "for ST-Ericsson"?
    > >
    >
    > OK regarding the header comments. We have been instructed to use "for
    > ST-Ericsson" when writing author name. Is that a problem?

    I find this fully pointless since a) you have an stericsson.com email
    address and b) the code is copyright by ST-Ericsson. So my take is, yes,
    we get it ST-Ericsson has written this code.

    > >> +
    > >> +#include <linux/module.h>
    > >> +#include <linux/types.h>
    > >> +#include <linux/skbuff.h>
    > >> +#include <asm/byteorder.h>
    > >> +#include <net/bluetooth/bluetooth.h>
    > >> +#include <net/bluetooth/hci.h>
    > >> +#include <net/bluetooth/hci_core.h>
    > >> +
    > >> +#include <linux/workqueue.h>
    > >> +#include <linux/wait.h>
    > >> +#include <linux/time.h>
    > >> +#include <linux/jiffies.h>
    > >> +#include <linux/sched.h>
    > >> +#include <linux/timer.h>
    > >> +
    > >> +#include <linux/mfd/cg2900.h>
    > >> +#include <mach/cg2900_devices.h>
    > >> +
    > >> +/* module_param declared in cg2900_core.c */
    > >> +extern int cg2900_debug_level;
    > >> +
    > >> +#define NAME "CG2900 HCI"
    > >> +
    > >> +/* Debug defines */
    > >> +#define CG2900_DBG_DATA(fmt, arg...) \
    > >> +do { \
    > >> + if (cg2900_debug_level >= 25) \
    > >> + printk(KERN_DEBUG NAME " %s: " fmt "\n" , __func__ , ## arg); \
    > >> +} while (0)
    > >> +
    > >> +#define CG2900_DBG(fmt, arg...) \
    > >> +do { \
    > >> + if (cg2900_debug_level >= 20) \
    > >> + printk(KERN_DEBUG NAME " %s: " fmt "\n" , __func__ , ## arg); \
    > >> +} while (0)
    > >> +
    > >> +#define CG2900_INFO(fmt, arg...) \
    > >> +do { \
    > >> + if (cg2900_debug_level >= 10) \
    > >> + printk(KERN_INFO NAME ": " fmt "\n" , ## arg); \
    > >> +} while (0)
    > >> +
    > >> +#define CG2900_ERR(fmt, arg...) \
    > >> +do { \
    > >> + if (cg2900_debug_level >= 1) \
    > >> + printk(KERN_ERR NAME " %s: " fmt "\n" , __func__ , ## arg); \
    > >> +} while (0)
    > >
    > > Remove all this debug stuff. Use BT_DBG, BT_ERR etc.
    >
    > OK
    >
    > >
    > >> +#define CG2900_SET_STATE(__name, __var, __new_state) \
    > >> +do { \
    > >> + CG2900_DBG("New %s: 0x%X", __name, (uint32_t)__new_state); \
    > >> + __var = __new_state; \
    > >> +} while (0)
    > >
    > > What is this for? Please don't do that. It just obfuscates the code.
    > >
    >
    > Is it OK to use inline functions instead of defines? It's pretty
    > useful to be able to printout when changing state.
    > So something like:
    >
    > static inline void set_reset_state(enum reset_state new_state)
    > {
    > BT_DBG("New reset state: %x", new_state);
    > hci_info->reset_state = new_state;
    > }

    Not really. It just clutters the code. Do the debugging with dynamic
    debug and keep the code readable.

    I do see where you are coming from and it might be useful during initial
    development. For longterm maintainability this is not helping.

    > >> +/* HCI device type */
    > >> +#define HCI_CG2900 HCI_VIRTUAL
    > >
    > > This is the wrong type. Don't do that. And don't create a new define for
    > > it. Use the standard types. I assume that HCI_UART is most likely what
    > > you want here.
    > >
    >
    > The problem here is that the physical transport used is not shown for
    > this module. It is handled with the CG2900 MFD driver. But I guess I
    > could expose the type through an API function.

    Yes, please do that instead, but essentially HCI_UART is a way better
    fit than HCI_VIRTUAL if you are in doubt.

    > >> +/* Wait for 5 seconds for a response to our requests */
    > >> +#define RESP_TIMEOUT 5000
    > >> +
    > >> +/* State-setting defines */
    > >> +#define SET_RESET_STATE(__hci_reset_new_state) \
    > >> + CG2900_SET_STATE("reset_state", hci_info->reset_state, \
    > >> + __hci_reset_new_state)
    > >> +#define SET_ENABLE_STATE(__hci_enable_new_state) \
    > >> + CG2900_SET_STATE("enable_state", hci_info->enable_state, \
    > >> + __hci_enable_new_state)
    > >
    > > Don't do this. It is just highly obfuscating the code flow.
    > >
    >
    > As stated above, is it OK to use static inline functions instead?

    No. Just fix the code to do it properly there. You will see that it is
    just fine.

    > >> +/* Bluetooth error codes */
    > >> +#define HCI_ERR_NO_ERROR 0x00
    > >> +#define HCI_ERR_CMD_DISALLOWED 0x0C
    > >> +
    > >> +/**
    > >> + * enum reset_state - RESET-states of the HCI driver.
    > >> + *
    > >> + * @RESET_IDLE: No reset in progress.
    > >> + * @RESET_ACTIVATED: Reset in progress.
    > >> + * @RESET_UNREGISTERED: hdev is unregistered.
    > >> + */
    > >> +
    > >> +enum reset_state {
    > >> + RESET_IDLE,
    > >> + RESET_ACTIVATED,
    > >> + RESET_UNREGISTERED
    > >> +};
    > >> +
    > >> +/**
    > >> + * enum enable_state - ENABLE-states of the HCI driver.
    > >> + *
    > >> + * @ENABLE_IDLE: The HCI driver is loaded but not opened.
    > >> + * @ENABLE_WAITING_BT_ENABLED_CC: The HCI driver is waiting for a command
    > >> + * complete event from the BT chip as a
    > >> + * response to a BT Enable (true) command.
    > >> + * @ENABLE_BT_ENABLED: The BT chip is enabled.
    > >> + * @ENABLE_WAITING_BT_DISABLED_CC: The HCI driver is waiting for a command
    > >> + * complete event from the BT chip as a
    > >> + * response to a BT Enable (false) command.
    > >> + * @ENABLE_BT_DISABLED: The BT chip is disabled.
    > >> + * @ENABLE_BT_ERROR: The HCI driver is in a bad state, some
    > >> + * thing has failed and is not expected to
    > >> + * work properly.
    > >> + */
    > >> +enum enable_state {
    > >> + ENABLE_IDLE,
    > >> + ENABLE_WAITING_BT_ENABLED_CC,
    > >> + ENABLE_BT_ENABLED,
    > >> + ENABLE_WAITING_BT_DISABLED_CC,
    > >> + ENABLE_BT_DISABLED,
    > >> + ENABLE_BT_ERROR
    > >> +};
    > >> +
    > >> +/**
    > >> + * struct hci_info - Specifies HCI driver private data.
    > >> + *
    > >> + * This type specifies CG2900 HCI driver private data.
    > >> + *
    > >> + * @bt_cmd: Device structure for BT command channel.
    > >> + * @bt_evt: Device structure for BT event channel.
    > >> + * @bt_acl: Device structure for BT ACL channel.
    > >> + * @hdev: Device structure for HCI device.
    > >> + * @reset_state: Device enum for HCI driver reset state.
    > >> + * @enable_state: Device enum for HCI driver BT enable state.
    > >> + */
    > >> +struct hci_info {
    > >> + struct cg2900_device *bt_cmd;
    > >> + struct cg2900_device *bt_evt;
    > >> + struct cg2900_device *bt_acl;
    > >> + struct hci_dev *hdev;
    > >> + enum reset_state reset_state;
    > >> + enum enable_state enable_state;
    > >> +};
    > >> +
    > >> +/**
    > >> + * struct dev_info - Specifies private data used when receiving
    > >> callbacks from CPD.
    > >> + *
    > >> + * @hdev: Device structure for HCI device.
    > >> + * @hci_data_type: Type of data according to BlueZ.
    > >> + */
    > >> +struct dev_info {
    > >> + struct hci_dev *hdev;
    > >> + u8 hci_data_type;
    > >> +};
    > >> +
    > >> +/* Variables where LSB and MSB for bt_enable command is stored */
    > >> +static u16 bt_enable_cmd;
    > >> +
    > >> +static struct hci_info *hci_info;
    > >> +
    > >> +/*
    > >> + * hci_wait_queue - Main Wait Queue in HCI driver.
    > >> + */
    > >> +static DECLARE_WAIT_QUEUE_HEAD(hci_wait_queue);
    > >> +
    > >> +/* Internal function declarations */
    > >> +static int register_to_bluez(void);
    > >
    > > Don't use bluez in kernel code. Just use bluetooth or bt.
    > >
    >
    > OK
    >
    > >> +/* Internal functions */
    > >> +
    > >> +/**
    > >> + * remove_bt_users() - Unregister and remove any existing BT users.
    > >> + * @info: HCI driver info structure.
    > >> + */
    > >> +static void remove_bt_users(struct hci_info *info)
    > >> +{
    > >> + if (info->bt_cmd) {
    > >> + kfree(info->bt_cmd->user_data);
    > >> + info->bt_cmd->user_data = NULL;
    > >> + cg2900_deregister_user(info->bt_cmd);
    > >> + info->bt_cmd = NULL;
    > >> + }
    > >> +
    > >> + if (info->bt_evt) {
    > >> + kfree(info->bt_evt->user_data);
    > >> + info->bt_evt->user_data = NULL;
    > >> + cg2900_deregister_user(info->bt_evt);
    > >> + info->bt_evt = NULL;
    > >> + }
    > >> +
    > >> + if (info->bt_acl) {
    > >> + kfree(info->bt_acl->user_data);
    > >> + info->bt_acl->user_data = NULL;
    > >> + cg2900_deregister_user(info->bt_acl);
    > >> + info->bt_acl = NULL;
    > >> + }
    > >> +}
    > >> +
    > >> +/**
    > >> + * hci_read_cb() - Callback for handling data received from CG2900 driver.
    > >> + * @dev: Device receiving data.
    > >> + * @skb: Buffer with data coming from device.
    > >> + */
    > >> +static void hci_read_cb(struct cg2900_device *dev, struct sk_buff *skb)
    > >> +{
    > >> + int err = 0;
    > >> + struct dev_info *dev_info;
    > >> + struct hci_event_hdr *evt;
    > >> + struct hci_ev_cmd_complete *cmd_complete;
    > >> + struct hci_ev_cmd_status *cmd_status;
    > >> + u8 status;
    > >> +
    > >> + if (!skb) {
    > >> + CG2900_ERR("NULL supplied for skb");
    > >> + return;
    > >> + }
    > >> +
    > >> + if (!dev) {
    > >> + CG2900_ERR("dev == NULL");
    > >> + goto fin_free_skb;
    > >> + }
    > >> +
    > >> + dev_info = (struct dev_info *)dev->user_data;
    > >> +
    > >> + if (!dev_info) {
    > >> + CG2900_ERR("dev_info == NULL");
    > >> + goto fin_free_skb;
    > >> + }
    > >> +
    > >> + evt = (struct hci_event_hdr *)skb->data;
    > >> + cmd_complete = (struct hci_ev_cmd_complete *)(skb->data + sizeof(*evt));
    > >> + cmd_status = (struct hci_ev_cmd_status *)(skb->data + sizeof(*evt));
    > >> +
    > >> + /*
    > >> + * Check if HCI Driver it self is expecting a Command Complete packet
    > >> + * from the chip after a BT Enable command.
    > >> + */
    > >> + if ((hci_info->enable_state == ENABLE_WAITING_BT_ENABLED_CC ||
    > >> + hci_info->enable_state == ENABLE_WAITING_BT_DISABLED_CC) &&
    > >> + hci_info->bt_evt->h4_channel == dev->h4_channel &&
    > >> + evt->evt == HCI_EV_CMD_COMPLETE &&
    > >> + le16_to_cpu(cmd_complete->opcode) == bt_enable_cmd) {
    > >> + /*
    > >> + * This is the command complete event for
    > >> + * the HCI_Cmd_VS_Bluetooth_Enable.
    > >> + * Check result and update state.
    > >> + *
    > >> + * The BT chip is enabled/disabled. Either it was enabled/
    > >> + * disabled now (status NO_ERROR) or it was already enabled/
    > >> + * disabled (assuming status CMD_DISALLOWED is already enabled/
    > >> + * disabled).
    > >> + */
    > >> + status = *(skb->data + sizeof(*evt) + sizeof(*cmd_complete));
    > >> + if (status != HCI_ERR_NO_ERROR &&
    > >> + status != HCI_ERR_CMD_DISALLOWED) {
    > >> + CG2900_ERR("Could not enable/disable BT core (0x%X)",
    > >> + status);
    > >> + SET_ENABLE_STATE(ENABLE_BT_ERROR);
    > >> + goto fin_free_skb;
    > >> + }
    > >> +
    > >> + if (hci_info->enable_state == ENABLE_WAITING_BT_ENABLED_CC) {
    > >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
    > >> + CG2900_INFO("BT core is enabled");
    > >> + } else {
    > >> + SET_ENABLE_STATE(ENABLE_BT_DISABLED);
    > >> + CG2900_INFO("BT core is disabled");
    > >> + }
    > >> +
    > >> + /* Wake up whom ever is waiting for this result. */
    > >> + wake_up_interruptible(&hci_wait_queue);
    > >> + goto fin_free_skb;
    > >> + } else if ((hci_info->enable_state == ENABLE_WAITING_BT_DISABLED_CC ||
    > >> + hci_info->enable_state == ENABLE_WAITING_BT_ENABLED_CC) &&
    > >> + hci_info->bt_evt->h4_channel == dev->h4_channel &&
    > >> + evt->evt == HCI_EV_CMD_STATUS &&
    > >> + le16_to_cpu(cmd_status->opcode) == bt_enable_cmd) {
    > >> + /*
    > >> + * Clear the status events since the Bluez is not expecting
    > >> + * them.
    > >> + */
    > >> + CG2900_DBG("HCI Driver received Command Status(for "
    > >> + "BT enable): 0x%X", cmd_status->status);
    > >> + /*
    > >> + * This is the command status event for
    > >> + * the HCI_Cmd_VS_Bluetooth_Enable.
    > >> + * Just free the packet.
    > >> + */
    > >> + goto fin_free_skb;
    > >> + } else {
    > >> + bt_cb(skb)->pkt_type = dev_info->hci_data_type;
    > >> + skb->dev = (struct net_device *)dev_info->hdev;
    > >> + /* Update BlueZ stats */
    > >> + dev_info->hdev->stat.byte_rx += skb->len;
    > >> + if (bt_cb(skb)->pkt_type == HCI_ACLDATA_PKT)
    > >> + dev_info->hdev->stat.acl_rx++;
    > >> + else
    > >> + dev_info->hdev->stat.evt_rx++;
    > >> +
    > >> + CG2900_DBG_DATA("Data receive %d bytes", skb->len);
    > >> +
    > >> + /* Provide BlueZ with received frame*/
    > >> + err = hci_recv_frame(skb);
    > >> + /* If err, skb have been freed in hci_recv_frame() */
    > >> + if (err)
    > >> + CG2900_ERR("Failed in supplying packet to BlueZ (%d)",
    > >> + err);
    > >> + }
    > >> +
    > >> + return;
    > >> +
    > >> +fin_free_skb:
    > >> + kfree_skb(skb);
    > >> +}
    > >> +
    > >> +/**
    > >> + * hci_reset_cb() - Callback for handling reset from CG2900 driver.
    > >> + * @dev: CPD device resetting.
    > >> + */
    > >> +static void hci_reset_cb(struct cg2900_device *dev)
    > >> +{
    > >> + int err;
    > >> + struct hci_dev *hdev;
    > >> + struct dev_info *dev_info;
    > >> + struct hci_info *info;
    > >> +
    > >> + CG2900_INFO("BluezDriver: hci_reset_cb");
    > >> +
    > >> + if (!dev) {
    > >> + CG2900_ERR("NULL supplied for dev");
    > >> + return;
    > >> + }
    > >> +
    > >> + dev_info = (struct dev_info *)dev->user_data;
    > >> + if (!dev_info) {
    > >> + CG2900_ERR("NULL supplied for dev_info");
    > >> + return;
    > >> + }
    > >> +
    > >> + hdev = dev_info->hdev;
    > >> + if (!hdev) {
    > >> + CG2900_ERR("NULL supplied for hdev");
    > >> + return;
    > >> + }
    > >> +
    > >> + info = (struct hci_info *)hdev->driver_data;
    > >> + if (!info) {
    > >> + CG2900_ERR("NULL supplied for driver_data");
    > >> + return;
    > >> + }
    > >> +
    > >> + switch (dev_info->hci_data_type) {
    > >> +
    > >> + case HCI_EVENT_PKT:
    > >> + info->bt_evt = NULL;
    > >> + break;
    > >> +
    > >> + case HCI_COMMAND_PKT:
    > >> + info->bt_cmd = NULL;
    > >> + break;
    > >> +
    > >> + case HCI_ACLDATA_PKT:
    > >> + info->bt_acl = NULL;
    > >> + break;
    > >> +
    > >> + default:
    > >> + CG2900_ERR("Unknown HCI data type:%d",
    > >> + dev_info->hci_data_type);
    > >> + return;
    > >> + }
    > >> +
    > >> + SET_RESET_STATE(RESET_ACTIVATED);
    > >> +
    > >> + /* Free userdata as device info structure will be freed by CG2900
    > >> + * when this callback returns */
    > >> + kfree(dev->user_data);
    > >> + dev->user_data = NULL;
    > >> +
    > >> + /*
    > >> + * Continue to deregister hdev if all channels has been reset else
    > >> + * return.
    > >> + */
    > >> + if (info->bt_evt || info->bt_cmd || info->bt_acl)
    > >> + return;
    > >> +
    > >> + /*
    > >> + * Deregister HCI device. Close and Destruct functions should
    > >> + * in turn be called by BlueZ.
    > >> + */
    > >> + CG2900_INFO("Deregister HCI device");
    > >> + err = hci_unregister_dev(hdev);
    > >> + if (err)
    > >> + CG2900_ERR("Can not deregister HCI device! (%d)", err);
    > >> + /*
    > >> + * Now we are in trouble. Try to register a new hdev
    > >> + * anyway even though this will cost some memory.
    > >> + */
    > >> +
    > >> + wait_event_interruptible_timeout(hci_wait_queue,
    > >> + (RESET_UNREGISTERED == hci_info->reset_state),
    > >> + msecs_to_jiffies(RESP_TIMEOUT));
    > >> + if (RESET_UNREGISTERED != hci_info->reset_state)
    > >> + /*
    > >> + * Now we are in trouble. Try to register a new hdev
    > >> + * anyway even though this will cost some memory.
    > >> + */
    > >> + CG2900_ERR("Timeout expired. Could not deregister HCI device");
    > >> +
    > >> + /* Init and register hdev */
    > >> + CG2900_INFO("Register HCI device");
    > >> + err = register_to_bluez();
    > >> + if (err)
    > >> + CG2900_ERR("HCI Device registration error (%d).", err);
    > >> +}
    > >> +
    > >> +/*
    > >> + * struct cg2900_cb - Specifies callback structure for CG2900 user.
    > >> + *
    > >> + * @read_cb: Callback function called when data is received from
    > >> + * the controller.
    > >> + * @reset_cb: Callback function called when the controller has been reset.
    > >> + */
    > >> +static struct cg2900_callbacks cg2900_cb = {
    > >> + .read_cb = hci_read_cb,
    > >> + .reset_cb = hci_reset_cb
    > >> +};
    > >> +
    > >> +/**
    > >> + * open_from_hci() - Open HCI interface.
    > >> + * @hdev: HCI device being opened.
    > >> + *
    > >> + * BlueZ callback function for opening HCI interface to device.
    > >> + *
    > >> + * Returns:
    > >> + * 0 if there is no error.
    > >> + * -EINVAL if NULL pointer is supplied.
    > >> + * -EOPNOTSUPP if supplied packet type is not supported.
    > >> + * -EBUSY if device is already opened.
    > >> + * -EACCES if opening of channels failed.
    > >> + */
    > >> +static int open_from_hci(struct hci_dev *hdev)
    > >> +{
    > >> + struct hci_info *info;
    > >> + struct dev_info *dev_info;
    > >> + struct sk_buff *enable_cmd;
    > >> + int err;
    > >> +
    > >> + CG2900_INFO("Open ST-Ericsson connectivity HCI driver");
    > >> +
    > >> + if (!hdev) {
    > >> + CG2900_ERR("NULL supplied for hdev");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + info = (struct hci_info *)hdev->driver_data;
    > >> + if (!info) {
    > >> + CG2900_ERR("NULL supplied for driver_data");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + if (test_and_set_bit(HCI_RUNNING, &(hdev->flags))) {
    > >> + CG2900_ERR("Device already opened!");
    > >> + return -EBUSY;
    > >> + }
    > >> +
    > >> + if (!(info->bt_cmd)) {
    > >> + info->bt_cmd = cg2900_register_user(CG2900_BT_CMD,
    > >> + &cg2900_cb);
    > >> + if (info->bt_cmd) {
    > >> + dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
    > >> + if (dev_info) {
    > >> + dev_info->hdev = hdev;
    > >> + dev_info->hci_data_type = HCI_COMMAND_PKT;
    > >> + }
    > >> + info->bt_cmd->user_data = dev_info;
    > >> + } else {
    > >> + CG2900_ERR("Couldn't register CG2900_BT_CMD to CG2900");
    > >> + err = -EACCES;
    > >> + goto handle_error;
    > >> + }
    > >> + }
    > >> +
    > >> + if (!(info->bt_evt)) {
    > >> + info->bt_evt = cg2900_register_user(CG2900_BT_EVT,
    > >> + &cg2900_cb);
    > >> + if (info->bt_evt) {
    > >> + dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
    > >> + if (dev_info) {
    > >> + dev_info->hdev = hdev;
    > >> + dev_info->hci_data_type = HCI_EVENT_PKT;
    > >> + }
    > >> + info->bt_evt->user_data = dev_info;
    > >> + } else {
    > >> + CG2900_ERR("Couldn't register CG2900_BT_EVT to CG2900");
    > >> + err = -EACCES;
    > >> + goto handle_error;
    > >> + }
    > >> + }
    > >> +
    > >> + if (!(info->bt_acl)) {
    > >> + info->bt_acl = cg2900_register_user(CG2900_BT_ACL,
    > >> + &cg2900_cb);
    > >> + if (info->bt_acl) {
    > >> + dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
    > >> + if (dev_info) {
    > >> + dev_info->hdev = hdev;
    > >> + dev_info->hci_data_type = HCI_ACLDATA_PKT;
    > >> + }
    > >> + info->bt_acl->user_data = dev_info;
    > >> + } else {
    > >> + CG2900_ERR("Couldn't register CG2900_BT_ACL to CG2900");
    > >> + err = -EACCES;
    > >> + goto handle_error;
    > >> + }
    > >> + }
    > >> +
    > >> + if (info->reset_state == RESET_ACTIVATED)
    > >> + SET_RESET_STATE(RESET_IDLE);
    > >> +
    > >> + /*
    > >> + * Call platform specific function that returns the chip dependent
    > >> + * bt enable(true) HCI command.
    > >> + * If NULL is returned, then no bt_enable command should be sent to the
    > >> + * chip.
    > >> + */
    > >> + enable_cmd = cg2900_devices_get_bt_enable_cmd(&bt_enable_cmd, true);
    > >> + if (!enable_cmd) {
    > >> + /* The chip is enabled by default */
    > >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
    > >> + return 0;
    > >> + }
    > >> +
    > >> + /* Set the HCI state before sending command to chip. */
    > >> + SET_ENABLE_STATE(ENABLE_WAITING_BT_ENABLED_CC);
    > >> +
    > >> + /* Send command to chip */
    > >> + cg2900_write(info->bt_cmd, enable_cmd);
    > >> +
    > >> + /*
    > >> + * Wait for callback to receive command complete and then wake us up
    > >> + * again.
    > >> + */
    > >> + wait_event_interruptible_timeout(hci_wait_queue,
    > >> + (info->enable_state == ENABLE_BT_ENABLED),
    > >> + msecs_to_jiffies(RESP_TIMEOUT));
    > >> + /* Check the current state to se that it worked. */
    > >> + if (info->enable_state != ENABLE_BT_ENABLED) {
    > >> + CG2900_ERR("Could not enable BT core (%d)", info->enable_state);
    > >> + err = -EACCES;
    > >> + SET_ENABLE_STATE(ENABLE_BT_DISABLED);
    > >> + goto handle_error;
    > >> + }
    > >> +
    > >> + return 0;
    > >> +
    > >> +handle_error:
    > >> + remove_bt_users(info);
    > >> + clear_bit(HCI_RUNNING, &(hdev->flags));
    > >> + return err;
    > >> +
    > >> +}
    > >> +
    > >> +/**
    > >> + * flush_from_hci() - Flush HCI interface.
    > >> + * @hdev: HCI device being flushed.
    > >> + *
    > >> + * Returns:
    > >> + * 0 if there is no error.
    > >> + * -EINVAL if NULL pointer is supplied.
    > >> + */
    > >> +static int flush_from_hci(struct hci_dev *hdev)
    > >> +{
    > >> + CG2900_INFO("flush_from_hci");
    > >> +
    > >> + if (!hdev) {
    > >> + CG2900_ERR("NULL supplied for hdev");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + return 0;
    > >> +}
    > >> +
    > >> +/**
    > >> + * close_from_hci() - Close HCI interface.
    > >> + * @hdev: HCI device being closed.
    > >> + *
    > >> + * BlueZ callback function for closing HCI interface.
    > >> + * It flushes the interface first.
    > >> + *
    > >> + * Returns:
    > >> + * 0 if there is no error.
    > >> + * -EINVAL if NULL pointer is supplied.
    > >> + * -EOPNOTSUPP if supplied packet type is not supported.
    > >> + * -EBUSY if device is not opened.
    > >> + */
    > >> +static int close_from_hci(struct hci_dev *hdev)
    > >> +{
    > >> + struct hci_info *info = NULL;
    > >> + struct sk_buff *enable_cmd;
    > >> +
    > >> + CG2900_INFO("close_from_hci");
    > >> +
    > >> + if (!hdev) {
    > >> + CG2900_ERR("NULL supplied for hdev");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + info = (struct hci_info *)hdev->driver_data;
    > >> + if (!info) {
    > >> + CG2900_ERR("NULL supplied for driver_data");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + if (!test_and_clear_bit(HCI_RUNNING, &(hdev->flags))) {
    > >> + CG2900_ERR("Device already closed!");
    > >> + return -EBUSY;
    > >> + }
    > >> +
    > >> + flush_from_hci(hdev);
    > >> +
    > >> + /* Do not do this if there is an reset ongoing */
    > >> + if (hci_info->reset_state == RESET_ACTIVATED)
    > >> + goto remove_users;
    > >> +
    > >> + /*
    > >> + * Get the chip dependent BT Enable HCI command. The command parameter
    > >> + * shall be set to false to disable the BT core.
    > >> + * If NULL is returned, then no BT Enable command should be sent to the
    > >> + * chip.
    > >> + */
    > >> + enable_cmd = cg2900_devices_get_bt_enable_cmd(&bt_enable_cmd, false);
    > >> + if (!enable_cmd) {
    > >> + /*
    > >> + * The chip is enabled by default and we should not disable it.
    > >> + */
    > >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
    > >> + goto remove_users;
    > >> + }
    > >> +
    > >> + /* Set the HCI state before sending command to chip */
    > >> + SET_ENABLE_STATE(ENABLE_WAITING_BT_DISABLED_CC);
    > >> +
    > >> + /* Send command to chip */
    > >> + cg2900_write(info->bt_cmd, enable_cmd);
    > >> +
    > >> + /*
    > >> + * Wait for callback to receive command complete and then wake us up
    > >> + * again.
    > >> + */
    > >> + wait_event_interruptible_timeout(hci_wait_queue,
    > >> + (info->enable_state == ENABLE_BT_DISABLED),
    > >> + msecs_to_jiffies(RESP_TIMEOUT));
    > >> + /* Check the current state to se that it worked. */
    > >> + if (info->enable_state != ENABLE_BT_DISABLED) {
    > >> + CG2900_ERR("Could not disable BT core.");
    > >> + SET_ENABLE_STATE(ENABLE_BT_ENABLED);
    > >> + }
    > >> +
    > >> +remove_users:
    > >> + /* Finally deregister all users and free allocated data */
    > >> + remove_bt_users(info);
    > >> + return 0;
    > >> +}
    > >> +
    > >> +/**
    > >> + * send_from_hci() - Send packet to device.
    > >> + * @skb: sk buffer to be sent.
    > >> + *
    > >> + * BlueZ callback function for sending sk buffer.
    > >> + *
    > >> + * Returns:
    > >> + * 0 if there is no error.
    > >> + * -EINVAL if NULL pointer is supplied.
    > >> + * -EOPNOTSUPP if supplied packet type is not supported.
    > >> + * Error codes from cg2900_write.
    > >> + */
    > >> +static int send_from_hci(struct sk_buff *skb)
    > >> +{
    > >> + struct hci_dev *hdev;
    > >> + struct hci_info *info;
    > >> + int err = 0;
    > >> +
    > >> + if (!skb) {
    > >> + CG2900_ERR("NULL supplied for skb");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + hdev = (struct hci_dev *)(skb->dev);
    > >> + if (!hdev) {
    > >> + CG2900_ERR("NULL supplied for hdev");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + info = (struct hci_info *)hdev->driver_data;
    > >> + if (!info) {
    > >> + CG2900_ERR("NULL supplied for info");
    > >> + return -EINVAL;
    > >> + }
    > >> +
    > >> + /* Update BlueZ stats */
    > >> + hdev->stat.byte_tx += skb->len;
    > >> +
    > >> + CG2900_DBG_DATA("Data transmit %d bytes", skb->len);
    > >> +
    > >> + switch (bt_cb(skb)->pkt_type) {
    > >> + case HCI_COMMAND_PKT:
    > >> + CG2900_DBG("Sending HCI_COMMAND_PKT");
    > >> + err = cg2900_write(info->bt_cmd, skb);
    > >> + hdev->stat.cmd_tx++;
    > >> + break;
    > >> + case HCI_ACLDATA_PKT:
    > >> + CG2900_DBG("Sending HCI_ACLDATA_PKT");
    > >> + err = cg2900_write(info->bt_acl, skb);
    > >> + hdev->stat.acl_tx++;
    > >> + break;
    > >> + default:
    > >> + CG2900_ERR("Trying to transmit unsupported packet type "
    > >> + "(0x%.2X)", bt_cb(skb)->pkt_type);
    > >> + err = -EOPNOTSUPP;
    > >> + break;
    > >> + };
    > >> +
    > >> + return err;
    > >> +}
    > >> +
    > >> +/**
    > >> + * destruct_from_hci() - Destruct HCI interface.
    > >> + * @hdev: HCI device being destructed.
    > >> + */
    > >> +static void destruct_from_hci(struct hci_dev *hdev)
    > >> +{
    > >> + CG2900_INFO("destruct_from_hci");
    > >> +
    > >> + if (!hci_info)
    > >> + return;
    > >> +
    > >> + /* When being reset, register a new hdev when hdev is deregistered */
    > >> + if (hci_info->reset_state == RESET_ACTIVATED) {
    > >> + if (hci_info->hdev)
    > >> + hci_free_dev(hci_info->hdev);
    > >> + SET_RESET_STATE(RESET_UNREGISTERED);
    > >> + }
    > >> +}
    > >
    > > Please name thse cg2900_destruct or btcg2900_destruct. And not from_hci.
    > > Follow how the other Bluetooth drivers do the naming.
    > >
    > > And the destruct callback is for freeing memory. I am also not sure how
    > > reset and unregister/re-register HCI is a good idea.
    > >
    >
    > Regarding naming: no problem, we'll change that. We will see how we
    > solve the reset. It's a bit tricky to get it working correctly, but
    > we'll see what we can do. We have to be able in some way to handle if
    > another stack, such as GPS or FM, suddenly reset the chip.

    It sounds wrong to me, but please enlighten me.

    > >> +/**
    > >> + * notify_from_hci() - Notification from the HCI interface.
    > >> + * @hdev: HCI device notifying.
    > >> + * @evt: Notification event.
    > >> + */
    > >> +static void notify_from_hci(struct hci_dev *hdev, unsigned int evt)
    > >> +{
    > >> + CG2900_INFO("notify_from_hci - evt = 0x%.2X", evt);
    > >> +}
    > >
    > > If you don't use it, then just leave it out.
    > >
    >
    > OK
    >
    > >> +/**
    > >> + * ioctl_from_hci() - Handle IOCTL call to the HCI interface.
    > >> + * @hdev: HCI device.
    > >> + * @cmd: IOCTL command.
    > >> + * @arg: IOCTL argument.
    > >> + *
    > >> + * Returns:
    > >> + * -EINVAL if NULL is supplied for hdev.
    > >> + * -EPERM otherwise since current driver supports no IOCTL.
    > >> + */
    > >> +static int ioctl_from_hci(struct hci_dev *hdev, unsigned int cmd,
    > >> + unsigned long arg)
    > >> +{
    > >> + CG2900_INFO("ioctl_from_hci - cmd 0x%X", cmd);
    > >> + CG2900_DBG("DIR: %d, TYPE: %d, NR: %d, SIZE: %d",
    > >> + _IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd),
    > >> + _IOC_SIZE(cmd));
    > >> +
    > >> + if (!hdev) {
    > >> + CG2900_ERR("NULL supplied for hdev");
    > >> + return -EINVAL;;
    > >> + }
    > >> +
    > >> + return -EPERM;
    > >> +}
    > >
    > > Since you are not using anything with the ioctl, just leave it out. The
    > > Bluetooth subsystem will do the right thing.
    > >
    >
    > OK
    >
    > >> +/**
    > >> + * register_to_bluez() - Initialize module.
    > >> + *
    > >> + * Alloc, init, and register HCI device to BlueZ.
    > >> + *
    > >> + * Returns:
    > >> + * 0 if there is no error.
    > >> + * -ENOMEM if allocation fails.
    > >> + * Error codes from hci_register_dev.
    > >> + */
    > >> +static int register_to_bluez(void)
    > >> +{
    > >> + int err;
    > >> +
    > >> + hci_info->hdev = hci_alloc_dev();
    > >> + if (!hci_info->hdev) {
    > >> + CG2900_ERR("Could not allocate mem for HCI driver");
    > >> + return -ENOMEM;
    > >> + }
    > >> +
    > >> + hci_info->hdev->bus = HCI_CG2900;
    > >> + hci_info->hdev->driver_data = hci_info;
    > >> + hci_info->hdev->owner = THIS_MODULE;
    > >> + hci_info->hdev->open = open_from_hci;
    > >> + hci_info->hdev->close = close_from_hci;
    > >> + hci_info->hdev->flush = flush_from_hci;
    > >> + hci_info->hdev->send = send_from_hci;
    > >> + hci_info->hdev->destruct = destruct_from_hci;
    > >> + hci_info->hdev->notify = notify_from_hci;
    > >> + hci_info->hdev->ioctl = ioctl_from_hci;
    > >> +
    > >> + err = hci_register_dev(hci_info->hdev);
    > >> + if (err) {
    > >> + CG2900_ERR("Can not register HCI device (%d)", err);
    > >> + hci_free_dev(hci_info->hdev);
    > >> + }
    > >> +
    > >> + SET_ENABLE_STATE(ENABLE_IDLE);
    > >> + SET_RESET_STATE(RESET_IDLE);
    > >> +
    > >> + return err;
    > >> +}
    > >
    > > So whenever the module is loaded it registers the HCI device. That is
    > > not really useful here. This driver needs to be able to be compiled into
    > > the kernel (or as module) and run on hardware that doesn't have this
    > > chip built in.
    > >
    >
    > OK. We will think this through and solve it in a better way.

    The TI guys seem to favoring a platform device. So either you have to do
    that or create your own bus.

    Regards

    Marcel




    \
     
     \ /
      Last update: 2010-10-13 23:51    [W:0.144 / U:5.288 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site