2010-09-24 13:52:16

by Par-Gunnar HJALMDAHL

Subject: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900

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 <[email protected]>
---
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
+
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 ([email protected]) for
ST-Ericsson.
+ * Henrik Possung ([email protected]) for ST-Ericsson.
+ * Josef Kindberg ([email protected]) for ST-Ericsson.
+ * Dariusz Szymszak ([email protected]) for ST-Ericsson.
+ * Kjell Andersson ([email protected]) 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.
+ */
+
+#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)
+
+#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)
+
+/* HCI device type */
+#define HCI_CG2900 HCI_VIRTUAL
+
+/* 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)
+
+/* 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);
+
+/* 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);
+ }
+}
+
+/**
+ * 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);
+}
+
+/**
+ * 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;
+}
+
+/**
+ * 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;
+}
+
+/**
+ * hci_init() - Initialize module.
+ *
+ * Allocate and initialize private data. Register to BlueZ.
+ *
+ * Returns:
+ * 0 if there is no error.
+ * -ENOMEM if allocation fails.
+ * Error codes from register_to_bluez.
+ */
+static int __init hci_init(void)
+{
+ int err;
+ CG2900_INFO("hci_init");
+
+ /* Initialize private data. */
+ hci_info = kzalloc(sizeof(*hci_info), GFP_KERNEL);
+ if (!hci_info) {
+ CG2900_ERR("Could not alloc hci_info struct.");
+ return -ENOMEM;
+ }
+
+ /* Init and register hdev */
+ err = register_to_bluez();
+ if (err) {
+ CG2900_ERR("HCI Device registration error (%d)", err);
+ kfree(hci_info);
+ hci_info = NULL;
+ return err;
+ }
+
+ return 0;
+}
+
+/**
+ * hci_exit() - Remove module.
+ *
+ * Remove HCI device from CG2900 driver.
+ */
+static void __exit hci_exit(void)
+{
+ int err = 0;
+
+ CG2900_INFO("hci_exit");
+
+ if (!hci_info)
+ return;
+
+ if (!hci_info->hdev)
+ goto finished;
+
+ err = hci_unregister_dev(hci_info->hdev);
+ if (err)
+ CG2900_ERR("Can not unregister HCI device (%d)", err);
+ hci_free_dev(hci_info->hdev);
+
+finished:
+ kfree(hci_info);
+ hci_info = NULL;
+}
+
+module_init(hci_init);
+module_exit(hci_exit);
+
+MODULE_AUTHOR("Par-Gunnar Hjalmdahl ST-Ericsson");
+MODULE_AUTHOR("Henrik Possung ST-Ericsson");
+MODULE_AUTHOR("Josef Kindberg ST-Ericsson");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Linux Bluetooth HCI H:4 Driver for ST-Ericsson
controller");
--
1.6.3.3

2010-09-28 09:14:26

by Par-Gunnar Hjalmdahl

Subject: Re: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900

Hi Gustavo,

Thanks for your comments. See below for answers.

/P-G

2010/9/27 Gustavo F. Padovan <[email protected]>:
> Hi Par-Gunnar,
>
> * Par-Gunnar Hjalmdahl <[email protected]> [2010-09-24 15:52:16 +0200]:
>
>> 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.
>
> First of all your your commit message and subject should be improved.
> The subject should bee something like:
>
> "Bluetooth: Add support for ST-Ericsson CG2900"
>
> and in the commit message you explain the details of the patch.
> And normally we do not use the BlueZ word in kernelspace.
>

OK. This is the first patch I've created within the community and of
course there are bound to be errors... ;-)
Since it was part of a big patch delivery I used quite similar names
across the different patches and I understand that this is an error
since the different patches are targeting different communities.

>>
>> Signed-off-by: Par-Gunnar Hjalmdahl <[email protected]>
>> ---
>> ?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
>
> Your patch looks a way complicated to a UART driver. Look at the others
> drivers at drivers/bluetooth/ and see how we implemented other UART
> drivers.
>

Well, mainly that's because it is not a UART driver. It is a driver
against CG2900 which currently have only UART as transport but quite
soon will get SPI as transport as well. For this Bluetooth driver the
actual physical transport used is not known. It just knows it has a
reliable transport below which delivers complete Bluetooth packets.

>>
>> 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
>> +
>> ?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 ([email protected]) for
>> ST-Ericsson.
>> + * Henrik Possung ([email protected]) for ST-Ericsson.
>> + * Josef Kindberg ([email protected]) for ST-Ericsson.
>> + * Dariusz Szymszak ([email protected]) for ST-Ericsson.
>> + * Kjell Andersson ([email protected]) 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.
>> + */
>> +
>> +#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;
>
> You don't need that, just use dynamic debug instead
>

Regarding the debug we know we have made an debug implementation that
might be controversial. We have used defines to output debug at
different levels and a module param to control the debug level in
runtime. I know that rest of the Kernel use other ways for debug and
if our way is not accepted we will of course modify the debug in the
driver to use standard Kernel methods.

I personally prefer to use defines because it makes it easy to add a
file name or similar at the start of the printout and also always use
'\n' in the end of each debug line. But if you don't like it that way
we will of course change that as well.

>> +
>> +#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)i
>
> and BT_DBG, BT_INFO, BT_ERR instead of these macros.

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)
>
> Don't hide your operation with a macro, that is a simple attribution, so
> no need for a macro for that.
>

OK. I prefer to be able to get a printout 'automatically' every time
state is changed, but it is no big issue to do this directly in the
code instead.

>> +
>> +/* HCI device type */
>> +#define HCI_CG2900 ? ? ? ? ? HCI_VIRTUAL
>> +
>> +/* 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)
>
> Same here.
>

OK.

> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
>

2010-09-27 16:45:02

by Gustavo Padovan

Subject: Re: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900

Hi Par-Gunnar,

* Par-Gunnar Hjalmdahl <[email protected]> [2010-09-24 15:52:16 +0200]:

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

First of all your your commit message and subject should be improved.
The subject should bee something like:

"Bluetooth: Add support for ST-Ericsson CG2900"

and in the commit message you explain the details of the patch.
And normally we do not use the BlueZ word in kernelspace.

>
> Signed-off-by: Par-Gunnar Hjalmdahl <[email protected]>
> ---
> 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

Your patch looks a way complicated to a UART driver. Look at the others
drivers at drivers/bluetooth/ and see how we implemented other UART
drivers.

>
> 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
> +
> 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 ([email protected]) for
> ST-Ericsson.
> + * Henrik Possung ([email protected]) for ST-Ericsson.
> + * Josef Kindberg ([email protected]) for ST-Ericsson.
> + * Dariusz Szymszak ([email protected]) for ST-Ericsson.
> + * Kjell Andersson ([email protected]) 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.
> + */
> +
> +#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;

You don't need that, just use dynamic debug instead

> +
> +#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)i

and BT_DBG, BT_INFO, BT_ERR instead of these macros.

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

Don't hide your operation with a macro, that is a simple attribution, so
no need for a macro for that.

> +
> +/* HCI device type */
> +#define HCI_CG2900 HCI_VIRTUAL
> +
> +/* 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)

Same here.

--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

2010-10-13 21:48:36

by Marcel Holtmann

Subject: Re: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900

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 <[email protected]>:
> > 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 <[email protected]>
> >> ---
> >> 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 ([email protected]) for
> >> ST-Ericsson.
> >> + * Henrik Possung ([email protected]) for ST-Ericsson.
> >> + * Josef Kindberg ([email protected]) for ST-Ericsson.
> >> + * Dariusz Szymszak ([email protected]) for ST-Ericsson.
> >> + * Kjell Andersson ([email protected]) 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

2010-10-11 13:00:01

by Par-Gunnar Hjalmdahl

Subject: Re: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900

Thanks for your comments Marcel,

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.

/P-G

2010/10/5 Marcel Holtmann <[email protected]>:
> Hi Par-Gunnar,
>
>> This patch adds support for using the ST-Ericsson CG2900
>> =A0connectivity controller as a driver for the BlueZ Bluetooth
>> =A0stack.
>> =A0This patch registers as a driver into the BlueZ framework and, when
>> =A0opened by BlueZ, it registers as user for bt_cmd, bt_acl, and bt_evt
>> =A0channels.
>>
>> Signed-off-by: Par-Gunnar Hjalmdahl <[email protected]=
om>
>> ---
>> =A0drivers/bluetooth/Kconfig =A0 =A0 =A0| =A0 =A07 +
>> =A0drivers/bluetooth/Makefile =A0 =A0 | =A0 =A02 +
>> =A0drivers/bluetooth/cg2900_hci.c | =A0896 +++++++++++++++++++++++++++++=
+++++++++++
>> =A03 files changed, 905 insertions(+), 0 deletions(-)
>> =A0create 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
>> =A0 =A0 =A0 =A0 Say Y here to compile support for "Atheros firmware down=
load driver"
>> =A0 =A0 =A0 =A0 into the kernel or say M to compile it as module (ath3k)=
.
>>
>> +config BT_CG2900
>> + =A0 =A0 tristate "ST-Ericsson CG2900 driver"
>> + =A0 =A0 depends on MFD_CG2900 && BT
>> + =A0 =A0 help
>> + =A0 =A0 =A0 Select if ST-Ericsson CG2900 Connectivity controller shall=
be used as
>> + =A0 =A0 =A0 Bluetooth controller for BlueZ.
>> +
>> =A0endmenu
>> 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) =A0 =A0 =A0 =A0 =A0 =A0 =A0+=3D=
ath3k.o
>> =A0obj-$(CONFIG_BT_MRVL) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0+=3D btmrvl.o
>> =A0obj-$(CONFIG_BT_MRVL_SDIO) =A0 +=3D btmrvl_sdio.o
>>
>> +obj-$(CONFIG_BT_CG2900) =A0 =A0 =A0 =A0 =A0 =A0 =A0+=3D 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.

>> =A0btmrvl-y =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 :=3D btmrvl_main.o
>> =A0btmrvl-$(CONFIG_DEBUG_FS) =A0 =A0+=3D btmrvl_debugfs.o
>>
>> diff --git a/drivers/bluetooth/cg2900_hci.c b/drivers/bluetooth/cg2900_h=
ci.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 ([email protected]) for
>> ST-Ericsson.
>> + * Henrik Possung ([email protected]) for ST-Ericsson.
>> + * Josef Kindberg ([email protected]) for ST-Ericsson.
>> + * Dariusz Szymszak ([email protected]) for ST-Ericsso=
n.
>> + * Kjell Andersson ([email protected]) for ST-Ericsson.
>> + * License terms: =A0GNU 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?

>> +
>> +#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 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "CG2900 HCI"
>> +
>> +/* Debug defines */
>> +#define CG2900_DBG_DATA(fmt, arg...) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 \
>> +do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 if (cg2900_debug_level >=3D 25) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_DEBUG NAME " %s: " fmt "\n" , __fu=
nc__ , ## arg); \
>> +} while (0)
>> +
>> +#define CG2900_DBG(fmt, arg...) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0\
>> +do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 if (cg2900_debug_level >=3D 20) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_DEBUG NAME " %s: " fmt "\n" , __fu=
nc__ , ## arg); \
>> +} while (0)
>> +
>> +#define CG2900_INFO(fmt, arg...) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 \
>> +do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 if (cg2900_debug_level >=3D 10) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_INFO NAME ": " fmt "\n" , ## arg);=
\
>> +} while (0)
>> +
>> +#define CG2900_ERR(fmt, arg...) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> +do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 if (cg2900_debug_level >=3D 1) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0\
>> + =A0 =A0 =A0 =A0 =A0 =A0 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) =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 \
>> +do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
>> + =A0 =A0 CG2900_DBG("New %s: 0x%X", __name, (uint32_t)__new_state); =A0=
=A0 =A0\
>> + =A0 =A0 __var =3D __new_state; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
>> +} 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 =3D new_state;
}

>> +/* HCI device type */
>> +#define HCI_CG2900 =A0 =A0 =A0 =A0 =A0 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.

>> +/* Wait for 5 seconds for a response to our requests */
>> +#define RESP_TIMEOUT =A0 =A0 =A0 =A0 5000
>> +
>> +/* State-setting defines */
>> +#define SET_RESET_STATE(__hci_reset_new_state) \
>> + =A0 =A0 CG2900_SET_STATE("reset_state", hci_info->reset_state, \
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0__hci_reset_new_state)
>> +#define SET_ENABLE_STATE(__hci_enable_new_state) \
>> + =A0 =A0 CG2900_SET_STATE("enable_state", hci_info->enable_state, \
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0__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?

>> +/* Bluetooth error codes */
>> +#define HCI_ERR_NO_ERROR =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 0x00
>> +#define HCI_ERR_CMD_DISALLOWED =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 0x0C
>> +
>> +/**
>> + * enum reset_state - RESET-states of the HCI driver.
>> + *
>> + * @RESET_IDLE: =A0 =A0 =A0 =A0 =A0 =A0 =A0No reset in progress.
>> + * @RESET_ACTIVATED: Reset in progress.
>> + * @RESET_UNREGISTERED: =A0 =A0 =A0hdev is unregistered.
>> + */
>> +
>> +enum reset_state {
>> + =A0 =A0 RESET_IDLE,
>> + =A0 =A0 RESET_ACTIVATED,
>> + =A0 =A0 RESET_UNREGISTERED
>> +};
>> +
>> +/**
>> + * enum enable_state - ENABLE-states of the HCI driver.
>> + *
>> + * @ENABLE_IDLE: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 The HCI driver=
is loaded but not opened.
>> + * @ENABLE_WAITING_BT_ENABLED_CC: =A0 =A0The HCI driver is waiting for =
a command
>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
complete event from the BT chip as a
>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
response to a BT Enable (true) command.
>> + * @ENABLE_BT_ENABLED: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 The =
BT chip is enabled.
>> + * @ENABLE_WAITING_BT_DISABLED_CC: =A0 The HCI driver is waiting for a =
command
>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
complete event from the BT chip as a
>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
response to a BT Enable (false) command.
>> + * @ENABLE_BT_DISABLED: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0The =
BT chip is disabled.
>> + * @ENABLE_BT_ERROR: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 The HCI driver is =
in a bad state, some
>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
thing has failed and is not expected to
>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
work properly.
>> + */
>> +enum enable_state {
>> + =A0 =A0 ENABLE_IDLE,
>> + =A0 =A0 ENABLE_WAITING_BT_ENABLED_CC,
>> + =A0 =A0 ENABLE_BT_ENABLED,
>> + =A0 =A0 ENABLE_WAITING_BT_DISABLED_CC,
>> + =A0 =A0 ENABLE_BT_DISABLED,
>> + =A0 =A0 ENABLE_BT_ERROR
>> +};
>> +
>> +/**
>> + * struct hci_info - Specifies HCI driver private data.
>> + *
>> + * This type specifies CG2900 HCI driver private data.
>> + *
>> + * @bt_cmd: =A0 =A0 =A0 =A0 =A0Device structure for BT command channel.
>> + * @bt_evt: =A0 =A0 =A0 =A0 =A0Device structure for BT event channel.
>> + * @bt_acl: =A0 =A0 =A0 =A0 =A0Device structure for BT ACL channel.
>> + * @hdev: =A0 =A0 =A0 =A0 =A0 =A0Device structure for HCI device.
>> + * @reset_state: =A0 =A0 Device enum for HCI driver reset state.
>> + * @enable_state: =A0 =A0Device enum for HCI driver BT enable state.
>> + */
>> +struct hci_info {
>> + =A0 =A0 struct cg2900_device =A0 =A0*bt_cmd;
>> + =A0 =A0 struct cg2900_device =A0 =A0*bt_evt;
>> + =A0 =A0 struct cg2900_device =A0 =A0*bt_acl;
>> + =A0 =A0 struct hci_dev =A0 =A0 =A0 =A0 =A0*hdev;
>> + =A0 =A0 enum reset_state =A0 =A0 =A0 =A0reset_state;
>> + =A0 =A0 enum enable_state =A0 =A0 =A0 enable_state;
>> +};
>> +
>> +/**
>> + * struct dev_info - Specifies private data used when receiving
>> callbacks from CPD.
>> + *
>> + * @hdev: =A0 =A0 =A0 =A0 =A0 =A0Device structure for HCI device.
>> + * @hci_data_type: =A0 Type of data according to BlueZ.
>> + */
>> +struct dev_info {
>> + =A0 =A0 struct hci_dev =A0*hdev;
>> + =A0 =A0 u8 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_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: =A0 =A0HCI driver info structure.
>> + */
>> +static void remove_bt_users(struct hci_info *info)
>> +{
>> + =A0 =A0 if (info->bt_cmd) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 kfree(info->bt_cmd->user_data);
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_cmd->user_data =3D NULL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 cg2900_deregister_user(info->bt_cmd);
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_cmd =3D NULL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (info->bt_evt) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 kfree(info->bt_evt->user_data);
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_evt->user_data =3D NULL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 cg2900_deregister_user(info->bt_evt);
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_evt =3D NULL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (info->bt_acl) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 kfree(info->bt_acl->user_data);
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_acl->user_data =3D NULL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 cg2900_deregister_user(info->bt_acl);
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_acl =3D NULL;
>> + =A0 =A0 }
>> +}
>> +
>> +/**
>> + * hci_read_cb() - Callback for handling data received from CG2900 driv=
er.
>> + * @dev: =A0 =A0 Device receiving data.
>> + * @skb: =A0 =A0 Buffer with data coming from device.
>> + */
>> +static void hci_read_cb(struct cg2900_device *dev, struct sk_buff *skb)
>> +{
>> + =A0 =A0 int err =3D 0;
>> + =A0 =A0 struct dev_info *dev_info;
>> + =A0 =A0 struct hci_event_hdr *evt;
>> + =A0 =A0 struct hci_ev_cmd_complete *cmd_complete;
>> + =A0 =A0 struct hci_ev_cmd_status *cmd_status;
>> + =A0 =A0 u8 status;
>> +
>> + =A0 =A0 if (!skb) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for skb");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (!dev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("dev =3D=3D NULL");
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto fin_free_skb;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 dev_info =3D (struct dev_info *)dev->user_data;
>> +
>> + =A0 =A0 if (!dev_info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("dev_info =3D=3D NULL");
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto fin_free_skb;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 evt =3D (struct hci_event_hdr *)skb->data;
>> + =A0 =A0 cmd_complete =3D (struct hci_ev_cmd_complete *)(skb->data + si=
zeof(*evt));
>> + =A0 =A0 cmd_status =3D (struct hci_ev_cmd_status *)(skb->data + sizeof=
(*evt));
>> +
>> + =A0 =A0 /*
>> + =A0 =A0 =A0* Check if HCI Driver it self is expecting a Command Comple=
te packet
>> + =A0 =A0 =A0* from the chip after a BT Enable command.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 if ((hci_info->enable_state =3D=3D ENABLE_WAITING_BT_ENABLED_C=
C ||
>> + =A0 =A0 =A0 =A0 =A0hci_info->enable_state =3D=3D ENABLE_WAITING_BT_DIS=
ABLED_CC) &&
>> + =A0 =A0 =A0 =A0 hci_info->bt_evt->h4_channel =3D=3D dev->h4_channel &&
>> + =A0 =A0 =A0 =A0 evt->evt =3D=3D HCI_EV_CMD_COMPLETE &&
>> + =A0 =A0 =A0 =A0 le16_to_cpu(cmd_complete->opcode) =3D=3D bt_enable_cmd=
) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 /*
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* This is the command complete event for
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* the HCI_Cmd_VS_Bluetooth_Enable.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* Check result and update state.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* The BT chip is enabled/disabled. Either i=
t was enabled/
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* disabled now (status NO_ERROR) or it was =
already enabled/
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* disabled (assuming status CMD_DISALLOWED =
is already enabled/
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* disabled).
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> + =A0 =A0 =A0 =A0 =A0 =A0 status =3D *(skb->data + sizeof(*evt) + sizeof=
(*cmd_complete));
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (status !=3D HCI_ERR_NO_ERROR &&
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 status !=3D HCI_ERR_CMD_DISALLOWED) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Could not enable/d=
isable BT core (0x%X)",
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0status)=
;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 SET_ENABLE_STATE(ENABLE_BT_ERR=
OR);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto fin_free_skb;
>> + =A0 =A0 =A0 =A0 =A0 =A0 }
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (hci_info->enable_state =3D=3D ENABLE_WAITI=
NG_BT_ENABLED_CC) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 SET_ENABLE_STATE(ENABLE_BT_ENA=
BLED);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 CG2900_INFO("BT core is enable=
d");
>> + =A0 =A0 =A0 =A0 =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 SET_ENABLE_STATE(ENABLE_BT_DIS=
ABLED);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 CG2900_INFO("BT core is disabl=
ed");
>> + =A0 =A0 =A0 =A0 =A0 =A0 }
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Wake up whom ever is waiting for this resul=
t. */
>> + =A0 =A0 =A0 =A0 =A0 =A0 wake_up_interruptible(&hci_wait_queue);
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto fin_free_skb;
>> + =A0 =A0 } else if ((hci_info->enable_state =3D=3D ENABLE_WAITING_BT_DI=
SABLED_CC ||
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_info->enable_state =3D=3D ENABLE_W=
AITING_BT_ENABLED_CC) &&
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_info->bt_evt->h4_channel =3D=3D dev=
->h4_channel &&
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0evt->evt =3D=3D HCI_EV_CMD_STATUS &&
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0le16_to_cpu(cmd_status->opcode) =3D=3D =
bt_enable_cmd) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 /*
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* Clear the status events since the Bluez i=
s not expecting
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* them.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_DBG("HCI Driver received Command Status=
(for "
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"BT enable): 0x%X", cmd=
_status->status);
>> + =A0 =A0 =A0 =A0 =A0 =A0 /*
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* This is the command status event for
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* the HCI_Cmd_VS_Bluetooth_Enable.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* Just free the packet.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto fin_free_skb;
>> + =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 bt_cb(skb)->pkt_type =3D dev_info->hci_data_ty=
pe;
>> + =A0 =A0 =A0 =A0 =A0 =A0 skb->dev =3D (struct net_device *)dev_info->hd=
ev;
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Update BlueZ stats */
>> + =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hdev->stat.byte_rx +=3D skb->len;
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (bt_cb(skb)->pkt_type =3D=3D HCI_ACLDATA_PK=
T)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hdev->stat.acl_rx++;
>> + =A0 =A0 =A0 =A0 =A0 =A0 else
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hdev->stat.evt_rx++;
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_DBG_DATA("Data receive %d bytes", skb->=
len);
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Provide BlueZ with received frame*/
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D hci_recv_frame(skb);
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* If err, skb have been freed in hci_recv_fra=
me() */
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (err)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Failed in supplyin=
g packet to BlueZ (%d)",
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0err);
>> + =A0 =A0 }
>> +
>> + =A0 =A0 return;
>> +
>> +fin_free_skb:
>> + =A0 =A0 kfree_skb(skb);
>> +}
>> +
>> +/**
>> + * hci_reset_cb() - Callback for handling reset from CG2900 driver.
>> + * @dev: =A0 =A0 CPD device resetting.
>> + */
>> +static void hci_reset_cb(struct cg2900_device *dev)
>> +{
>> + =A0 =A0 int err;
>> + =A0 =A0 struct hci_dev *hdev;
>> + =A0 =A0 struct dev_info *dev_info;
>> + =A0 =A0 struct hci_info *info;
>> +
>> + =A0 =A0 CG2900_INFO("BluezDriver: hci_reset_cb");
>> +
>> + =A0 =A0 if (!dev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for dev");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 dev_info =3D (struct dev_info *)dev->user_data;
>> + =A0 =A0 if (!dev_info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for dev_info");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 hdev =3D dev_info->hdev;
>> + =A0 =A0 if (!hdev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for hdev");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 info =3D (struct hci_info *)hdev->driver_data;
>> + =A0 =A0 if (!info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for driver_data");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 switch (dev_info->hci_data_type) {
>> +
>> + =A0 =A0 case HCI_EVENT_PKT:
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_evt =3D NULL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 break;
>> +
>> + =A0 =A0 case HCI_COMMAND_PKT:
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_cmd =3D NULL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 break;
>> +
>> + =A0 =A0 case HCI_ACLDATA_PKT:
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_acl =3D NULL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 break;
>> +
>> + =A0 =A0 default:
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Unknown HCI data type:%d",
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_info->hci_data_type=
);
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 SET_RESET_STATE(RESET_ACTIVATED);
>> +
>> + =A0 =A0 /* Free userdata as device info structure will be freed by CG2=
900
>> + =A0 =A0 =A0* when this callback returns */
>> + =A0 =A0 kfree(dev->user_data);
>> + =A0 =A0 dev->user_data =3D NULL;
>> +
>> + =A0 =A0 /*
>> + =A0 =A0 =A0* Continue to deregister hdev if all channels has been rese=
t else
>> + =A0 =A0 =A0* return.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 if (info->bt_evt || info->bt_cmd || info->bt_acl)
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> +
>> + =A0 =A0 /*
>> + =A0 =A0 =A0* Deregister HCI device. Close and Destruct functions shoul=
d
>> + =A0 =A0 =A0* in turn be called by BlueZ.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 CG2900_INFO("Deregister HCI device");
>> + =A0 =A0 err =3D hci_unregister_dev(hdev);
>> + =A0 =A0 if (err)
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Can not deregister HCI device! (%d=
)", err);
>> + =A0 =A0 =A0 =A0 =A0 =A0 /*
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* Now we are in trouble. Try to register a =
new hdev
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* anyway even though this will cost some me=
mory.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> +
>> + =A0 =A0 wait_event_interruptible_timeout(hci_wait_queue,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (RESET_UNREGIS=
TERED =3D=3D hci_info->reset_state),
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 msecs_to_jiffi=
es(RESP_TIMEOUT));
>> + =A0 =A0 if (RESET_UNREGISTERED !=3D hci_info->reset_state)
>> + =A0 =A0 =A0 =A0 =A0 =A0 /*
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* Now we are in trouble. Try to register a =
new hdev
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* anyway even though this will cost some me=
mory.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Timeout expired. Could not deregis=
ter HCI device");
>> +
>> + =A0 =A0 /* Init and register hdev */
>> + =A0 =A0 CG2900_INFO("Register HCI device");
>> + =A0 =A0 err =3D register_to_bluez();
>> + =A0 =A0 if (err)
>> + =A0 =A0 =A0 =A0 =A0 =A0 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
>> + * =A0 =A0 =A0 =A0 =A0 the controller.
>> + * @reset_cb: =A0 =A0 =A0 =A0Callback function called when the controll=
er has been reset.
>> + */
>> +static struct cg2900_callbacks cg2900_cb =3D {
>> + =A0 =A0 .read_cb =3D hci_read_cb,
>> + =A0 =A0 .reset_cb =3D hci_reset_cb
>> +};
>> +
>> +/**
>> + * open_from_hci() - Open HCI interface.
>> + * @hdev: =A0 =A0HCI device being opened.
>> + *
>> + * BlueZ callback function for opening HCI interface to device.
>> + *
>> + * Returns:
>> + * =A0 0 if there is no error.
>> + * =A0 -EINVAL if NULL pointer is supplied.
>> + * =A0 -EOPNOTSUPP if supplied packet type is not supported.
>> + * =A0 -EBUSY if device is already opened.
>> + * =A0 -EACCES if opening of channels failed.
>> + */
>> +static int open_from_hci(struct hci_dev *hdev)
>> +{
>> + =A0 =A0 struct hci_info *info;
>> + =A0 =A0 struct dev_info *dev_info;
>> + =A0 =A0 struct sk_buff *enable_cmd;
>> + =A0 =A0 int err;
>> +
>> + =A0 =A0 CG2900_INFO("Open ST-Ericsson connectivity HCI driver");
>> +
>> + =A0 =A0 if (!hdev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for hdev");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 info =3D (struct hci_info *)hdev->driver_data;
>> + =A0 =A0 if (!info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for driver_data");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (test_and_set_bit(HCI_RUNNING, &(hdev->flags))) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Device already opened!");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (!(info->bt_cmd)) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_cmd =3D cg2900_register_user(CG2900_B=
T_CMD,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&cg2900_cb);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (info->bt_cmd) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info =3D kmalloc(sizeof(*d=
ev_info), GFP_KERNEL);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (dev_info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hdev=
=3D hdev;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hci_=
data_type =3D HCI_COMMAND_PKT;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 info->bt_cmd->user_data =3D de=
v_info;
>> + =A0 =A0 =A0 =A0 =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Couldn't register =
CG2900_BT_CMD to CG2900");
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EACCES;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto handle_error;
>> + =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (!(info->bt_evt)) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_evt =3D cg2900_register_user(CG2900_B=
T_EVT,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&cg2900_cb);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (info->bt_evt) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info =3D kmalloc(sizeof(*d=
ev_info), GFP_KERNEL);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (dev_info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hdev=
=3D hdev;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hci_=
data_type =3D HCI_EVENT_PKT;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 info->bt_evt->user_data =3D de=
v_info;
>> + =A0 =A0 =A0 =A0 =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Couldn't register =
CG2900_BT_EVT to CG2900");
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EACCES;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto handle_error;
>> + =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (!(info->bt_acl)) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 info->bt_acl =3D cg2900_register_user(CG2900_B=
T_ACL,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&cg2900_cb);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (info->bt_acl) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info =3D kmalloc(sizeof(*d=
ev_info), GFP_KERNEL);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (dev_info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hdev=
=3D hdev;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info->hci_=
data_type =3D HCI_ACLDATA_PKT;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 info->bt_acl->user_data =3D de=
v_info;
>> + =A0 =A0 =A0 =A0 =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Couldn't register =
CG2900_BT_ACL to CG2900");
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EACCES;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto handle_error;
>> + =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (info->reset_state =3D=3D RESET_ACTIVATED)
>> + =A0 =A0 =A0 =A0 =A0 =A0 SET_RESET_STATE(RESET_IDLE);
>> +
>> + =A0 =A0 /*
>> + =A0 =A0 =A0* Call platform specific function that returns the chip dep=
endent
>> + =A0 =A0 =A0* bt enable(true) HCI command.
>> + =A0 =A0 =A0* If NULL is returned, then no bt_enable command should be =
sent to the
>> + =A0 =A0 =A0* chip.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 enable_cmd =3D cg2900_devices_get_bt_enable_cmd(&bt_enable_cmd=
, true);
>> + =A0 =A0 if (!enable_cmd) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* The chip is enabled by default */
>> + =A0 =A0 =A0 =A0 =A0 =A0 SET_ENABLE_STATE(ENABLE_BT_ENABLED);
>> + =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 /* Set the HCI state before sending command to chip. */
>> + =A0 =A0 SET_ENABLE_STATE(ENABLE_WAITING_BT_ENABLED_CC);
>> +
>> + =A0 =A0 /* Send command to chip */
>> + =A0 =A0 cg2900_write(info->bt_cmd, enable_cmd);
>> +
>> + =A0 =A0 /*
>> + =A0 =A0 =A0* Wait for callback to receive command complete and then wa=
ke us up
>> + =A0 =A0 =A0* again.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 wait_event_interruptible_timeout(hci_wait_queue,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (info->enable_=
state =3D=3D ENABLE_BT_ENABLED),
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 msecs_to_jiffi=
es(RESP_TIMEOUT));
>> + =A0 =A0 /* Check the current state to se that it worked. */
>> + =A0 =A0 if (info->enable_state !=3D ENABLE_BT_ENABLED) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Could not enable BT core (%d)", in=
fo->enable_state);
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EACCES;
>> + =A0 =A0 =A0 =A0 =A0 =A0 SET_ENABLE_STATE(ENABLE_BT_DISABLED);
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto handle_error;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 return 0;
>> +
>> +handle_error:
>> + =A0 =A0 remove_bt_users(info);
>> + =A0 =A0 clear_bit(HCI_RUNNING, &(hdev->flags));
>> + =A0 =A0 return err;
>> +
>> +}
>> +
>> +/**
>> + * flush_from_hci() - Flush HCI interface.
>> + * @hdev: =A0 =A0HCI device being flushed.
>> + *
>> + * Returns:
>> + * =A0 0 if there is no error.
>> + * =A0 -EINVAL if NULL pointer is supplied.
>> + */
>> +static int flush_from_hci(struct hci_dev *hdev)
>> +{
>> + =A0 =A0 CG2900_INFO("flush_from_hci");
>> +
>> + =A0 =A0 if (!hdev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for hdev");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 return 0;
>> +}
>> +
>> +/**
>> + * close_from_hci() - Close HCI interface.
>> + * @hdev: =A0 =A0HCI device being closed.
>> + *
>> + * BlueZ callback function for closing HCI interface.
>> + * It flushes the interface first.
>> + *
>> + * Returns:
>> + * =A0 0 if there is no error.
>> + * =A0 -EINVAL if NULL pointer is supplied.
>> + * =A0 -EOPNOTSUPP if supplied packet type is not supported.
>> + * =A0 -EBUSY if device is not opened.
>> + */
>> +static int close_from_hci(struct hci_dev *hdev)
>> +{
>> + =A0 =A0 struct hci_info *info =3D NULL;
>> + =A0 =A0 struct sk_buff *enable_cmd;
>> +
>> + =A0 =A0 CG2900_INFO("close_from_hci");
>> +
>> + =A0 =A0 if (!hdev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for hdev");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 info =3D (struct hci_info *)hdev->driver_data;
>> + =A0 =A0 if (!info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for driver_data");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 if (!test_and_clear_bit(HCI_RUNNING, &(hdev->flags))) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Device already closed!");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 flush_from_hci(hdev);
>> +
>> + =A0 =A0 /* Do not do this if there is an reset ongoing */
>> + =A0 =A0 if (hci_info->reset_state =3D=3D RESET_ACTIVATED)
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto remove_users;
>> +
>> + =A0 =A0 /*
>> + =A0 =A0 =A0* Get the chip dependent BT Enable HCI command. The command=
parameter
>> + =A0 =A0 =A0* shall be set to false to disable the BT core.
>> + =A0 =A0 =A0* If NULL is returned, then no BT Enable command should be =
sent to the
>> + =A0 =A0 =A0* chip.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 enable_cmd =3D cg2900_devices_get_bt_enable_cmd(&bt_enable_cmd=
, false);
>> + =A0 =A0 if (!enable_cmd) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 /*
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* The chip is enabled by default and we sho=
uld not disable it.
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> + =A0 =A0 =A0 =A0 =A0 =A0 SET_ENABLE_STATE(ENABLE_BT_ENABLED);
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto remove_users;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 /* Set the HCI state before sending command to chip */
>> + =A0 =A0 SET_ENABLE_STATE(ENABLE_WAITING_BT_DISABLED_CC);
>> +
>> + =A0 =A0 /* Send command to chip */
>> + =A0 =A0 cg2900_write(info->bt_cmd, enable_cmd);
>> +
>> + =A0 =A0 /*
>> + =A0 =A0 =A0* Wait for callback to receive command complete and then wa=
ke us up
>> + =A0 =A0 =A0* again.
>> + =A0 =A0 =A0*/
>> + =A0 =A0 wait_event_interruptible_timeout(hci_wait_queue,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (info->enable_=
state =3D=3D ENABLE_BT_DISABLED),
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 msecs_to_jiffi=
es(RESP_TIMEOUT));
>> + =A0 =A0 /* Check the current state to se that it worked. */
>> + =A0 =A0 if (info->enable_state !=3D ENABLE_BT_DISABLED) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Could not disable BT core.");
>> + =A0 =A0 =A0 =A0 =A0 =A0 SET_ENABLE_STATE(ENABLE_BT_ENABLED);
>> + =A0 =A0 }
>> +
>> +remove_users:
>> + =A0 =A0 /* Finally deregister all users and free allocated data */
>> + =A0 =A0 remove_bt_users(info);
>> + =A0 =A0 return 0;
>> +}
>> +
>> +/**
>> + * send_from_hci() - Send packet to device.
>> + * @skb: =A0 =A0 sk buffer to be sent.
>> + *
>> + * BlueZ callback function for sending sk buffer.
>> + *
>> + * Returns:
>> + * =A0 0 if there is no error.
>> + * =A0 -EINVAL if NULL pointer is supplied.
>> + * =A0 -EOPNOTSUPP if supplied packet type is not supported.
>> + * =A0 Error codes from cg2900_write.
>> + */
>> +static int send_from_hci(struct sk_buff *skb)
>> +{
>> + =A0 =A0 struct hci_dev *hdev;
>> + =A0 =A0 struct hci_info *info;
>> + =A0 =A0 int err =3D 0;
>> +
>> + =A0 =A0 if (!skb) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for skb");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 hdev =3D (struct hci_dev *)(skb->dev);
>> + =A0 =A0 if (!hdev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for hdev");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 info =3D (struct hci_info *)hdev->driver_data;
>> + =A0 =A0 if (!info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for info");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 /* Update BlueZ stats */
>> + =A0 =A0 hdev->stat.byte_tx +=3D skb->len;
>> +
>> + =A0 =A0 CG2900_DBG_DATA("Data transmit %d bytes", skb->len);
>> +
>> + =A0 =A0 switch (bt_cb(skb)->pkt_type) {
>> + =A0 =A0 case HCI_COMMAND_PKT:
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_DBG("Sending HCI_COMMAND_PKT");
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D cg2900_write(info->bt_cmd, skb);
>> + =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.cmd_tx++;
>> + =A0 =A0 =A0 =A0 =A0 =A0 break;
>> + =A0 =A0 case HCI_ACLDATA_PKT:
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_DBG("Sending HCI_ACLDATA_PKT");
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D cg2900_write(info->bt_acl, skb);
>> + =A0 =A0 =A0 =A0 =A0 =A0 hdev->stat.acl_tx++;
>> + =A0 =A0 =A0 =A0 =A0 =A0 break;
>> + =A0 =A0 default:
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Trying to transmit unsupported pac=
ket type "
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"(0x%.2X)", bt_cb(skb)-=
>pkt_type);
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EOPNOTSUPP;
>> + =A0 =A0 =A0 =A0 =A0 =A0 break;
>> + =A0 =A0 };
>> +
>> + =A0 =A0 return err;
>> +}
>> +
>> +/**
>> + * destruct_from_hci() - Destruct HCI interface.
>> + * @hdev: =A0 =A0HCI device being destructed.
>> + */
>> +static void destruct_from_hci(struct hci_dev *hdev)
>> +{
>> + =A0 =A0 CG2900_INFO("destruct_from_hci");
>> +
>> + =A0 =A0 if (!hci_info)
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> +
>> + =A0 =A0 /* When being reset, register a new hdev when hdev is deregist=
ered */
>> + =A0 =A0 if (hci_info->reset_state =3D=3D RESET_ACTIVATED) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (hci_info->hdev)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_free_dev(hci_info->hdev);
>> + =A0 =A0 =A0 =A0 =A0 =A0 SET_RESET_STATE(RESET_UNREGISTERED);
>> + =A0 =A0 }
>> +}
>
> 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.

>> +/**
>> + * notify_from_hci() - Notification from the HCI interface.
>> + * @hdev: =A0 =A0HCI device notifying.
>> + * @evt: =A0 =A0 Notification event.
>> + */
>> +static void notify_from_hci(struct hci_dev *hdev, unsigned int evt)
>> +{
>> + =A0 =A0 CG2900_INFO("notify_from_hci - evt =3D 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: =A0IOCTL command.
>> + * @arg: =A0IOCTL argument.
>> + *
>> + * Returns:
>> + * =A0 -EINVAL if NULL is supplied for hdev.
>> + * =A0 -EPERM otherwise since current driver supports no IOCTL.
>> + */
>> +static int ioctl_from_hci(struct hci_dev *hdev, unsigned int cmd,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned long arg)
>> +{
>> + =A0 =A0 CG2900_INFO("ioctl_from_hci - cmd 0x%X", cmd);
>> + =A0 =A0 CG2900_DBG("DIR: %d, TYPE: %d, NR: %d, SIZE: %d",
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(=
cmd),
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0_IOC_SIZE(cmd));
>> +
>> + =A0 =A0 if (!hdev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("NULL supplied for hdev");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 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:
>> + * =A0 0 if there is no error.
>> + * =A0 -ENOMEM if allocation fails.
>> + * =A0 Error codes from hci_register_dev.
>> + */
>> +static int register_to_bluez(void)
>> +{
>> + =A0 =A0 int err;
>> +
>> + =A0 =A0 hci_info->hdev =3D hci_alloc_dev();
>> + =A0 =A0 if (!hci_info->hdev) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Could not allocate mem for HCI dri=
ver");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 hci_info->hdev->bus =3D HCI_CG2900;
>> + =A0 =A0 hci_info->hdev->driver_data =3D hci_info;
>> + =A0 =A0 hci_info->hdev->owner =3D THIS_MODULE;
>> + =A0 =A0 hci_info->hdev->open =3D open_from_hci;
>> + =A0 =A0 hci_info->hdev->close =3D close_from_hci;
>> + =A0 =A0 hci_info->hdev->flush =3D flush_from_hci;
>> + =A0 =A0 hci_info->hdev->send =3D send_from_hci;
>> + =A0 =A0 hci_info->hdev->destruct =3D destruct_from_hci;
>> + =A0 =A0 hci_info->hdev->notify =3D notify_from_hci;
>> + =A0 =A0 hci_info->hdev->ioctl =3D ioctl_from_hci;
>> +
>> + =A0 =A0 err =3D hci_register_dev(hci_info->hdev);
>> + =A0 =A0 if (err) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Can not register HCI device (%d)",=
err);
>> + =A0 =A0 =A0 =A0 =A0 =A0 hci_free_dev(hci_info->hdev);
>> + =A0 =A0 }
>> +
>> + =A0 =A0 SET_ENABLE_STATE(ENABLE_IDLE);
>> + =A0 =A0 SET_RESET_STATE(RESET_IDLE);
>> +
>> + =A0 =A0 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.

>> +/**
>> + * hci_init() - Initialize module.
>> + *
>> + * Allocate and initialize private data. Register to BlueZ.
>> + *
>> + * Returns:
>> + * =A0 0 if there is no error.
>> + * =A0 -ENOMEM if allocation fails.
>> + * =A0 Error codes from register_to_bluez.
>> + */
>> +static int __init hci_init(void)
>> +{
>> + =A0 =A0 int err;
>> + =A0 =A0 CG2900_INFO("hci_init");
>> +
>> + =A0 =A0 /* Initialize private data. */
>> + =A0 =A0 hci_info =3D kzalloc(sizeof(*hci_info), GFP_KERNEL);
>> + =A0 =A0 if (!hci_info) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Could not alloc hci_info struct.")=
;
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 /* Init and register hdev */
>> + =A0 =A0 err =3D register_to_bluez();
>> + =A0 =A0 if (err) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("HCI Device registration error (%d)=
", err);
>> + =A0 =A0 =A0 =A0 =A0 =A0 kfree(hci_info);
>> + =A0 =A0 =A0 =A0 =A0 =A0 hci_info =3D NULL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 return err;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 return 0;
>> +}
>> +
>> +/**
>> + * hci_exit() - Remove module.
>> + *
>> + * Remove HCI device from CG2900 driver.
>> + */
>> +static void __exit hci_exit(void)
>> +{
>> + =A0 =A0 int err =3D 0;
>> +
>> + =A0 =A0 CG2900_INFO("hci_exit");
>> +
>> + =A0 =A0 if (!hci_info)
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> +
>> + =A0 =A0 if (!hci_info->hdev)
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto finished;
>> +
>> + =A0 =A0 err =3D hci_unregister_dev(hci_info->hdev);
>> + =A0 =A0 if (err)
>> + =A0 =A0 =A0 =A0 =A0 =A0 CG2900_ERR("Can not unregister HCI device (%d)=
", err);
>> + =A0 =A0 hci_free_dev(hci_info->hdev);
>> +
>> +finished:
>> + =A0 =A0 kfree(hci_info);
>> + =A0 =A0 hci_info =3D NULL;
>> +}
>> +
>> +module_init(hci_init);
>> +module_exit(hci_exit);
>
> Please use more clear names like cg2900_init or similar.
>

OK

>> +MODULE_AUTHOR("Par-Gunnar Hjalmdahl ST-Ericsson");
>> +MODULE_AUTHOR("Henrik Possung ST-Ericsson");
>> +MODULE_AUTHOR("Josef Kindberg ST-Ericsson");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Linux Bluetooth HCI H:4 Driver for ST-Ericsson
>> controller");
>
> Regards
>
> Marcel
>
>
>

2010-10-05 09:20:43

by Marcel Holtmann

Subject: Re: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900

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 <[email protected]>
> ---
> 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.

> 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 ([email protected]) for
> ST-Ericsson.
> + * Henrik Possung ([email protected]) for ST-Ericsson.
> + * Josef Kindberg ([email protected]) for ST-Ericsson.
> + * Dariusz Szymszak ([email protected]) for ST-Ericsson.
> + * Kjell Andersson ([email protected]) 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"?

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

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

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

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

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

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

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

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

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

> +/**
> + * hci_init() - Initialize module.
> + *
> + * Allocate and initialize private data. Register to BlueZ.
> + *
> + * Returns:
> + * 0 if there is no error.
> + * -ENOMEM if allocation fails.
> + * Error codes from register_to_bluez.
> + */
> +static int __init hci_init(void)
> +{
> + int err;
> + CG2900_INFO("hci_init");
> +
> + /* Initialize private data. */
> + hci_info = kzalloc(sizeof(*hci_info), GFP_KERNEL);
> + if (!hci_info) {
> + CG2900_ERR("Could not alloc hci_info struct.");
> + return -ENOMEM;
> + }
> +
> + /* Init and register hdev */
> + err = register_to_bluez();
> + if (err) {
> + CG2900_ERR("HCI Device registration error (%d)", err);
> + kfree(hci_info);
> + hci_info = NULL;
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * hci_exit() - Remove module.
> + *
> + * Remove HCI device from CG2900 driver.
> + */
> +static void __exit hci_exit(void)
> +{
> + int err = 0;
> +
> + CG2900_INFO("hci_exit");
> +
> + if (!hci_info)
> + return;
> +
> + if (!hci_info->hdev)
> + goto finished;
> +
> + err = hci_unregister_dev(hci_info->hdev);
> + if (err)
> + CG2900_ERR("Can not unregister HCI device (%d)", err);
> + hci_free_dev(hci_info->hdev);
> +
> +finished:
> + kfree(hci_info);
> + hci_info = NULL;
> +}
> +
> +module_init(hci_init);
> +module_exit(hci_exit);

Please use more clear names like cg2900_init or similar.

> +MODULE_AUTHOR("Par-Gunnar Hjalmdahl ST-Ericsson");
> +MODULE_AUTHOR("Henrik Possung ST-Ericsson");
> +MODULE_AUTHOR("Josef Kindberg ST-Ericsson");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Linux Bluetooth HCI H:4 Driver for ST-Ericsson
> controller");

Regards

Marcel

2010-10-05 08:27:50

by Marcel Holtmann

Subject: Re: [PATCH 6/6] This patch adds support for using the ST-Ericsson CG2900

Hi Par-Gunnar,

> > * Par-Gunnar Hjalmdahl <[email protected]> [2010-09-24 15:52:16 +0200]:
> >
> >> 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.
> >
> > First of all your your commit message and subject should be improved.
> > The subject should bee something like:
> >
> > "Bluetooth: Add support for ST-Ericsson CG2900"
> >
> > and in the commit message you explain the details of the patch.
> > And normally we do not use the BlueZ word in kernelspace.
> >
>
> OK. This is the first patch I've created within the community and of
> course there are bound to be errors... ;-)
> Since it was part of a big patch delivery I used quite similar names
> across the different patches and I understand that this is an error
> since the different patches are targeting different communities.
>
> >>
> >> Signed-off-by: Par-Gunnar Hjalmdahl <[email protected]>
> >> ---
> >> 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
> >
> > Your patch looks a way complicated to a UART driver. Look at the others
> > drivers at drivers/bluetooth/ and see how we implemented other UART
> > drivers.
> >
>
> Well, mainly that's because it is not a UART driver. It is a driver
> against CG2900 which currently have only UART as transport but quite
> soon will get SPI as transport as well. For this Bluetooth driver the
> actual physical transport used is not known. It just knows it has a
> reliable transport below which delivers complete Bluetooth packets.

inside the Bluetooth driver please just use BT_DBG. That is hooked up
into dynamic_debug feature of the kernel and you can do whatever you
need for debugging.

I will not accept huge debug frameworks in the Bluetooth subsystem. Use
what the kernel offers you. You selective control based on every single
debug statement. It doesn't get better than that.

Regards

Marcel