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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Par-Gunnar,

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

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

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

Actually btcg2900.ko sounds good to me.

> >>  btmrvl-y                     := btmrvl_main.o
> >>  btmrvl-$(CONFIG_DEBUG_FS)    += btmrvl_debugfs.o
> >>
> >> diff --git a/drivers/bluetooth/cg2900_hci.c b/drivers/bluetooth/cg2900_hci.c
> >> new file mode 100644
> >> index 0000000..de1ada8
> >> --- /dev/null
> >> +++ b/drivers/bluetooth/cg2900_hci.c
> >> @@ -0,0 +1,896 @@
> >> +/*
> >> + * drivers/bluetooth/cg2900_hci.c
> >> + *
> >> + * Copyright (C) ST-Ericsson SA 2010
> >> + * Authors:
> >> + * Par-Gunnar Hjalmdahl (par-gunnar.p.hjalmdahl@xxxxxxxxxxxxxx) for
> >> ST-Ericsson.
> >> + * Henrik Possung (henrik.possung@xxxxxxxxxxxxxx) for ST-Ericsson.
> >> + * Josef Kindberg (josef.kindberg@xxxxxxxxxxxxxx) for ST-Ericsson.
> >> + * Dariusz Szymszak (dariusz.xd.szymczak@xxxxxxxxxxxxxx) for ST-Ericsson.
> >> + * Kjell Andersson (kjell.k.andersson@xxxxxxxxxxxxxx) 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


--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux