Hi Par-Gunnar,
* Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@xxxxxxxxxxxxxx> [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 <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
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 (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.
> + */
> +
> +#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
--
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
