[PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

This patch adds UART support for the ST-Ericsson CG2900 Connectivity
Combo controller.
This patch registers to the TTY framework as a line discipline
driver for the N_HCI ldisc. When opened it registers as a transport
to the CG2900 framework. This patch also handles the low power
operation (suspend/resume) for the CG2900 when using UART transport.

Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
---
 drivers/mfd/Kconfig              |    7 +
 drivers/mfd/cg2900/Makefile      |    2 +
 drivers/mfd/cg2900/cg2900_uart.c | 1851 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 1860 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/cg2900/cg2900_uart.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index a8e790f..6fcd8b6 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -282,6 +282,13 @@ config MFD_STLC2690_CHIP
 	help
 	  Support for ST-Ericsson STLC2690 Connectivity Controller

+config MFD_CG2900_UART
+	tristate "Support CG2900 UART transport"
+	depends on MFD_CG2900
+	help
+	  Support for UART as transport for ST-Ericsson CG2900 Connectivity
+	  Controller
+
 config PMIC_DA903X
 	bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
 	depends on I2C=y
diff --git a/drivers/mfd/cg2900/Makefile b/drivers/mfd/cg2900/Makefile
index 4935daa..c8dd713 100644
--- a/drivers/mfd/cg2900/Makefile
+++ b/drivers/mfd/cg2900/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_MFD_CG2900)	+= cg2900_char_devices.o
 obj-$(CONFIG_MFD_CG2900_CHIP)	+= cg2900_chip.o
 obj-$(CONFIG_MFD_STLC2690_CHIP)	+= stlc2690_chip.o

+obj-$(CONFIG_MFD_CG2900_UART)	+= cg2900_uart.o
+
diff --git a/drivers/mfd/cg2900/cg2900_uart.c b/drivers/mfd/cg2900/cg2900_uart.c
new file mode 100644
index 0000000..f5e287d
--- /dev/null
+++ b/drivers/mfd/cg2900/cg2900_uart.c
@@ -0,0 +1,1851 @@
+/*
+ * drivers/mfd/cg2900/cg2900_uart.c
+ *
+ * Copyright (C) ST-Ericsson SA 2010
+ * Authors:
+ * Par-Gunnar Hjalmdahl (par-gunnar.p.hjalmdahl@stericsson.com) for
ST-Ericsson.
+ * Henrik Possung (henrik.possung@stericsson.com) for ST-Ericsson.
+ * Josef Kindberg (josef.kindberg@stericsson.com) for ST-Ericsson.
+ * Dariusz Szymszak (dariusz.xd.szymczak@stericsson.com) for ST-Ericsson.
+ * Kjell Andersson (kjell.k.andersson@stericsson.com) for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ *
+ * Linux Bluetooth UART Driver for ST-Ericsson CG2900 connectivity controller.
+ */
+
+#include <asm/byteorder.h>
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/skbuff.h>
+#include <linux/timer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/tty.h>
+#include <linux/tty_ldisc.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+#include <linux/mfd/cg2900.h>
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci.h>
+
+#include "cg2900_chip.h"
+#include "cg2900_core.h"
+#include "cg2900_debug.h"
+#include "hci_defines.h"
+
+/* Workqueues' names */
+#define UART_WQ_NAME		"cg2900_uart_wq"
+#define UART_NAME		"cg2900_uart"
+
+/* Standardized Bluetooth command channels */
+#define HCI_BT_CMD_H4_CHANNEL	0x01
+#define HCI_BT_ACL_H4_CHANNEL	0x02
+#define HCI_BT_EVT_H4_CHANNEL	0x04
+
+/* H4 channels specific for CG2900 */
+#define HCI_FM_RADIO_H4_CHANNEL	0x08
+#define HCI_GNSS_H4_CHANNEL	0x09
+
+/* Timers used in milliseconds */
+#define UART_TX_TIMEOUT		100
+#define UART_RESP_TIMEOUT	1000
+
+/* State-setting defines */
+#define SET_BAUD_STATE(__new_state) \
+	CG2900_SET_STATE("baud_rate_state", uart_info->baud_rate_state, \
+			 __new_state)
+#define SET_SLEEP_STATE(__new_state) \
+	CG2900_SET_STATE("sleep_state", uart_info->sleep_state, __new_state)
+
+/* Number of bytes to reserve at start of sk_buffer when receiving packet */
+#define RX_SKB_RESERVE		8
+/* Max size of received packet (not including reserved bytes) */
+#define RX_SKB_MAX_SIZE		1024
+
+/* Max size of bytes we can receive on the UART */
+#define UART_RECEIVE_ROOM	65536
+
+/* Size of the header in the different packets */
+#define HCI_BT_EVT_HDR_SIZE	2
+#define HCI_BT_ACL_HDR_SIZE	4
+#define HCI_FM_RADIO_HDR_SIZE	1
+#define HCI_GNSS_HDR_SIZE	3
+
+/* Position of length field in the different packets */
+#define HCI_EVT_LEN_POS		2
+#define HCI_ACL_LEN_POS		3
+#define FM_RADIO_LEN_POS	1
+#define GNSS_LEN_POS		2
+
+/* Bytes in the command Hci_Cmd_ST_Set_Uart_Baud_Rate */
+#define SET_BAUD_RATE_LSB	0x09
+#define SET_BAUD_RATE_MSB	0xFC
+#define SET_BAUD_RATE_PAYL_LEN	0x01
+#define SET_BAUD_RATE_LEN	0x04
+#define BAUD_RATE_57600		0x03
+#define BAUD_RATE_115200	0x02
+#define BAUD_RATE_230400	0x01
+#define BAUD_RATE_460800	0x00
+#define BAUD_RATE_921600	0x20
+#define BAUD_RATE_2000000	0x25
+#define BAUD_RATE_3000000	0x27
+#define BAUD_RATE_4000000	0x2B
+
+/* Baud rate defines */
+#define ZERO_BAUD_RATE		0
+#define DEFAULT_BAUD_RATE	115200
+#define HIGH_BAUD_RATE		3000000
+
+/* HCI TTY line discipline value */
+#ifndef N_HCI
+#define N_HCI			15
+#endif
+
+/* IOCTLs for UART */
+#define HCIUARTSETPROTO		_IOW('U', 200, int)
+#define HCIUARTGETPROTO		_IOR('U', 201, int)
+#define HCIUARTGETDEVICE	_IOR('U', 202, int)
+#define HCIUARTSETFD		_IOW('U', 203, int)
+
+
+/* UART break control parameters */
+#define TTY_BREAK_ON		(-1)
+#define TTY_BREAK_OFF		(0)
+
+/**
+ * enum uart_rx_state - UART RX-state for UART.
+ * @W4_PACKET_TYPE:	Waiting for packet type.
+ * @W4_EVENT_HDR:	Waiting for BT event header.
+ * @W4_ACL_HDR:		Waiting for BT ACL header.
+ * @W4_FM_RADIO_HDR:	Waiting for FM header.
+ * @W4_GNSS_HDR:	Waiting for GNSS header.
+ * @W4_DATA:		Waiting for data in rest of the packet (after header).
+ */
+enum uart_rx_state {
+	W4_PACKET_TYPE,
+	W4_EVENT_HDR,
+	W4_ACL_HDR,
+	W4_FM_RADIO_HDR,
+	W4_GNSS_HDR,
+	W4_DATA
+};
+
+/**
+  * enum sleep_state - Sleep-state for UART.
+  * @CHIP_AWAKE:  Chip is awake.
+  * @CHIP_FALLING_ASLEEP:  Chip is falling asleep.
+  * @CHIP_ASLEEP: Chip is asleep.
+  * @CHIP_SUSPENDED: Chip in suspend state.
+  * @CHIP_POWERED_DOWN: Chip is off.
+  */
+enum sleep_state {
+	CHIP_AWAKE,
+	CHIP_FALLING_ASLEEP,
+	CHIP_ASLEEP,
+	CHIP_SUSPENDED,
+	CHIP_POWERED_DOWN
+};
+
+/**
+ * enum baud_rate_change_state - Baud rate-state for UART.
+ * @BAUD_IDLE:		No baud rate change is ongoing.
+ * @BAUD_SENDING_RESET:	HCI reset has been sent. Waiting for command complete
+ *			event.
+ * @BAUD_START:		Set baud rate cmd scheduled for sending.
+ * @BAUD_SENDING:	Set baud rate cmd sending in progress.
+ * @BAUD_WAITING:	Set baud rate cmd sent, waiting for command complete
+ *			event.
+ * @BAUD_SUCCESS:	Baud rate change has succeeded.
+ * @BAUD_FAIL:		Baud rate change has failed.
+ */
+enum baud_rate_change_state {
+	BAUD_IDLE,
+	BAUD_SENDING_RESET,
+	BAUD_START,
+	BAUD_SENDING,
+	BAUD_WAITING,
+	BAUD_SUCCESS,
+	BAUD_FAIL
+};
+
+/**
+ * struct uart_work_struct - Work structure for UART module.
+ * @work:	Work structure.
+ * @data:	Pointer to private data.
+ *
+ * This structure is used to pack work for work queue.
+ */
+struct uart_work_struct{
+	struct work_struct	work;
+	void			*data;
+};
+
+/**
+ * struct test_char_dev_info - Main UART info structure.
+ * @wq:			UART work queue.
+ * @tx_queue:		TX queue for sending data to chip.
+ * @tty:		TTY info structure.
+ * @rx_lock:		RX spin lock.
+ * @rx_state:		Current RX state.
+ * @rx_count:		Number of bytes left to receive.
+ * @rx_skb:		SK_buffer to store the received data into.
+ * @tx_mutex:		TX mutex.
+ * @baud_rate_state:	UART baud rate change state.
+ * @baud_rate:		Current baud rate setting.
+ * @sleep_state:	UART sleep state.
+ * @timer:		UART timer (for chip sleep).
+ * @fd:			File object to device.
+ * @sleep_state_lock:	Used to protect chip state.
+ * @sleep_allowed:	Indicate if tty has functions needed for sleep mode.
+ * @regulator:		Regulator.
+ * @regulator_enabled:	True if regulator is enabled.
+ * @dev:		Pointer to CG2900 uart device.
+ */
+struct uart_info {
+	struct workqueue_struct		*wq;
+	struct sk_buff_head		tx_queue;
+	struct tty_struct		*tty;
+	spinlock_t			rx_lock;
+	enum uart_rx_state		rx_state;
+	unsigned long			rx_count;
+	struct sk_buff			*rx_skb;
+	struct mutex			tx_mutex;
+	enum baud_rate_change_state	baud_rate_state;
+	int				baud_rate;
+	enum sleep_state		sleep_state;
+	struct timer_list		timer;
+	struct file			*fd;
+	struct mutex			sleep_state_lock;
+	bool				sleep_allowed;
+	struct regulator		*regulator;
+	bool				regulator_enabled;
+	struct device			*dev;
+};
+
+static struct uart_info *uart_info;
+
+/* Module parameters */
+static int uart_default_baud = DEFAULT_BAUD_RATE;
+static int uart_high_baud = HIGH_BAUD_RATE;
+
+static DECLARE_WAIT_QUEUE_HEAD(uart_wait_queue);
+
+static void update_timer(void);
+
+/**
+ * is_chip_flow_off() - Check if chip has set flow off.
+ * @tty:	Pointer to tty.
+ *
+ * Returns:
+ *   true - chip flows off.
+ *   false - chip flows on.
+ */
+static bool is_chip_flow_off(struct tty_struct *tty)
+{
+	int lines;
+
+	lines = tty->ops->tiocmget(tty, uart_info->fd);
+
+	if (lines & TIOCM_CTS)
+		return false;
+	else
+		return true;
+}
+
+/**
+ * create_work_item() - Create work item and add it to the work queue.
+ * @wq:		work queue struct where the work will be added.
+ * @work_func:	Work function.
+ * @data:	Private data for the work.
+ *
+ * Returns:
+ *   0 if there is no error.
+ *   -EBUSY if not possible to queue work.
+ *   -ENOMEM if allocation fails.
+ */
+static int create_work_item(struct workqueue_struct *wq, work_func_t work_func,
+			    void *data)
+{
+	struct uart_work_struct *new_work;
+	int err;
+
+	new_work = kmalloc(sizeof(*new_work), GFP_ATOMIC);
+	if (!new_work) {
+		CG2900_ERR("Failed to alloc memory for uart_work_struct!");
+		return -ENOMEM;
+	}
+
+	new_work->data = data;
+	INIT_WORK(&new_work->work, work_func);
+
+	err = queue_work(wq, &new_work->work);
+	if (!err) {
+		CG2900_ERR("Failed to queue work_struct because it's already "
+			   "in the queue!");
+		kfree(new_work);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+/**
+ * set_tty_baud() - Called to set specific baud in TTY.
+ * @tty:	Tty device.
+ * @baud:	Baud to set.
+ *
+ * Returns:
+ *   true - baudrate set with success.
+ *   false - baundrate set failure.
+ */
+static bool set_tty_baud(struct tty_struct *tty, int baud)
+{
+	struct ktermios *old_termios;
+	bool retval = true;
+
+	old_termios = kmalloc(sizeof(*old_termios), GFP_ATOMIC);
+	if (!old_termios) {
+		CG2900_ERR("Could not allocate termios");
+		return false;
+	}
+
+	mutex_lock(&(tty->termios_mutex));
+	/* Start by storing the old termios. */
+	memcpy(old_termios, tty->termios, sizeof(*old_termios));
+
+	/* Let's mark that CG2900 driver uses c_ispeed and c_ospeed fields. */
+	tty->termios->c_cflag |= BOTHER;
+
+	tty_encode_baud_rate(tty, baud, baud);
+
+	/* Finally inform the driver */
+	if (tty->ops->set_termios)
+		tty->ops->set_termios(tty, old_termios);
+	else {
+		CG2900_ERR("Can not set new baudrate.");
+		/* Copy back the old termios to restore old setting. */
+		memcpy(tty->termios, old_termios, sizeof(*old_termios));
+		retval = false;
+	}
+
+	tty->termios->c_cflag &= ~BOTHER;
+
+	mutex_unlock(&(tty->termios_mutex));
+	kfree(old_termios);
+
+	return retval;
+}
+
+/**
+ * handle_cts_irq() - Called to handle CTS interrupt in work context.
+ * @work:	work which needs to be done.
+ *
+ * The handle_cts_irq() function is a work handler called if interrupt on CTS
+ * occurred. It updates the sleep timer which will wake up the transport.
+ */
+static void handle_cts_irq(struct work_struct *work)
+{
+	/* Restart timer and disable interrupt. */
+	update_timer();
+}
+
+/**
+ * cts_interrupt() - Called to handle CTS interrupt.
+ * @irq:	Interrupt that occurred.
+ * @dev_id:	Device ID where interrupt occurred (not used).
+ *
+ * The handle_cts_irq() function is called if interrupt on CTS occurred.
+ * It disables the interrupt and starts a new work thread to handle
+ * the interrupt.
+ */
+static irqreturn_t cts_interrupt(int irq, void *dev_id)
+{
+#ifdef CONFIG_PM
+	disable_irq_wake(irq);
+#endif
+	disable_irq_nosync(irq);
+
+	/* If chip is suspended, resume callback will be called. */
+	if (CHIP_SUSPENDED != uart_info->sleep_state)
+		/* Create work and leave IRQ context. */
+		(void)create_work_item(uart_info->wq, handle_cts_irq, NULL);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * set_cts_irq() - Enable interrupt on CTS.
+ *
+ * Returns:
+ *   0 if there is no error.
+ *   Error codes from request_irq and disable_uart.
+ */
+static int set_cts_irq(void)
+{
+	int err;
+	struct cg2900_platform_data *pf_data;
+
+	pf_data = dev_get_platdata(uart_info->dev->parent);
+
+	/* First disable the UART so we can use IRQ on the GPIOs */
+	if (pf_data->uart.disable_uart) {
+		err = pf_data->uart.disable_uart();
+		if (err) {
+			CG2900_ERR("Could not disable UART (%d)", err);
+			goto error;
+		}
+	}
+
+	/* Set IRQ on CTS. */
+	err = request_irq(pf_data->uart.cts_irq,
+			  cts_interrupt,
+			  IRQF_TRIGGER_FALLING,
+			  UART_NAME,
+			  NULL);
+	if (err) {
+		CG2900_ERR("Could not request CTS IRQ (%d)", err);
+		goto error;
+	}
+
+#ifdef CONFIG_PM
+	enable_irq_wake(pf_data->uart.cts_irq);
+#endif
+	return 0;
+
+error:
+	if (pf_data->uart.enable_uart)
+		(void)pf_data->uart.enable_uart();
+	return err;
+}
+
+/**
+ * unset_cts_irq() - Disable interrupt on CTS.
+ */
+static void unset_cts_irq(void)
+{
+	int err = 0;
+	struct cg2900_platform_data *pf_data;
+
+	pf_data = dev_get_platdata(uart_info->dev->parent);
+
+	/* Free CTS interrupt and restore UART settings. */
+	free_irq(pf_data->uart.cts_irq, NULL);
+
+	if (pf_data->uart.enable_uart) {
+		err = pf_data->uart.enable_uart();
+		if (err)
+			CG2900_ERR("Unable to enable UART Hardware (%d)", err);
+	}
+}
+
+/**
+ * update_timer() - Updates or starts the sleep timer.
+ *
+ * Updates or starts the sleep timer used to detect when there are no current
+ * data transmissions.
+ */
+static void update_timer(void)
+{
+	unsigned long timeout_jiffies = cg2900_get_sleep_timeout();
+	struct tty_struct *tty;
+
+	if ((!timeout_jiffies || !uart_info->fd || !uart_info->sleep_allowed)
+		&& (uart_info->sleep_state != CHIP_SUSPENDED))
+		return;
+
+	mutex_lock(&(uart_info->sleep_state_lock));
+	/*
+	 * This function indicates data is transmitted.
+	 * Therefore see to that the chip is awake.
+	 */
+	if (CHIP_AWAKE == uart_info->sleep_state)
+		goto finished;
+
+	tty = uart_info->tty;
+
+	if (CHIP_ASLEEP == uart_info->sleep_state ||
+		CHIP_SUSPENDED == uart_info->sleep_state) {
+		/* Disable IRQ only when it was enabled. */
+		unset_cts_irq();
+		(void)set_tty_baud(tty, uart_info->baud_rate);
+	}
+	/* Set FLOW on. */
+	tty_unthrottle(tty);
+
+	/* Unset BREAK. */
+	CG2900_DBG("Clear break");
+	tty->ops->break_ctl(tty, TTY_BREAK_OFF);
+
+	SET_SLEEP_STATE(CHIP_AWAKE);
+
+finished:
+	mutex_unlock(&(uart_info->sleep_state_lock));
+	/*
+	 * If timer is running restart it. If not, start it.
+	 * All this is handled by mod_timer().
+	 */
+	mod_timer(&(uart_info->timer), jiffies + timeout_jiffies);
+}
+
+/**
+ * sleep_timer_expired() - Called when sleep timer expires.
+ * @data:	Value supplied when starting the timer.
+ *
+ * The sleep_timer_expired() function is called if there are no ongoing data
+ * transmissions. It tries to put the chip in sleep mode.
+ *
+ */
+static void sleep_timer_expired(unsigned long data)
+{
+	unsigned long timeout_jiffies = cg2900_get_sleep_timeout();
+	struct tty_struct *tty;
+
+	if (!timeout_jiffies || !uart_info->sleep_allowed || !uart_info->fd)
+		return;
+
+	mutex_lock(&(uart_info->sleep_state_lock));
+
+	tty = uart_info->tty;
+
+	switch (uart_info->sleep_state) {
+	case CHIP_FALLING_ASLEEP:
+		if (!is_chip_flow_off(tty))
+			goto run_timer;
+
+		/* Flow OFF. */
+		tty_throttle(tty);
+
+		/*
+		 * Set baud zero.
+		 * This cause shut off UART clock as well.
+		 */
+		(void)set_tty_baud(tty, ZERO_BAUD_RATE);
+
+		if (set_cts_irq() < 0) {
+			CG2900_ERR("Can not set interrupt on CTS.");
+			(void)set_tty_baud(tty, uart_info->baud_rate);
+			tty_unthrottle(tty);
+			SET_SLEEP_STATE(CHIP_AWAKE);
+			goto error;
+		}
+
+		SET_SLEEP_STATE(CHIP_ASLEEP);
+		break;
+	case CHIP_AWAKE:
+
+		CG2900_DBG("Set break");
+		tty->ops->break_ctl(tty, TTY_BREAK_ON);
+
+		SET_SLEEP_STATE(CHIP_FALLING_ASLEEP);
+		goto run_timer;
+
+	case CHIP_POWERED_DOWN:
+	case CHIP_SUSPENDED:
+	case CHIP_ASLEEP: /* Fallthrough. */
+	default:
+		CG2900_DBG("Chip sleeps, is suspended or powered down.");
+		break;
+	}
+
+	mutex_unlock(&(uart_info->sleep_state_lock));
+
+	return;
+
+run_timer:
+	mutex_unlock(&(uart_info->sleep_state_lock));
+	mod_timer(&(uart_info->timer), jiffies + timeout_jiffies);
+	return;
+error:
+	/* Disable sleep mode.*/
+	CG2900_ERR("Disable sleep mode.");
+	uart_info->sleep_allowed = false;
+	uart_info->fd = NULL;
+	mutex_unlock(&(uart_info->sleep_state_lock));
+}
+
+#ifdef CONFIG_PM
+/**
+ * cg2900_uart_suspend() - Called by Linux PM to put the device in a
low power mode.
+ * @pdev:	Pointer to platform device.
+ * @state:	New state.
+ *
+ * In UART case, CG2900 driver does nothing on suspend.
+ *
+ * Returns:
+ *   0 - Success.
+ */
+static int cg2900_uart_suspend(struct platform_device *pdev,
pm_message_t state)
+{
+	if (uart_info->sleep_state == CHIP_POWERED_DOWN)
+		return 0;
+
+	/* Timer is mostlikely running. Delete it. */
+	del_timer(&uart_info->timer);
+
+	if (CHIP_ASLEEP == uart_info->sleep_state)
+		goto finished;
+
+	if (CHIP_AWAKE == uart_info->sleep_state) {
+		uart_info->tty->ops->break_ctl(uart_info->tty, TTY_BREAK_ON);
+		SET_SLEEP_STATE(CHIP_FALLING_ASLEEP);
+		msleep(10);
+	}
+
+	if (CHIP_FALLING_ASLEEP == uart_info->sleep_state) {
+		int err;
+
+		/* Flow OFF. */
+		tty_throttle(uart_info->tty);
+		(void)set_tty_baud(uart_info->tty, ZERO_BAUD_RATE);
+
+		err = set_cts_irq();
+		if (err < 0) {
+			CG2900_ERR("Can not suspend");
+			SET_SLEEP_STATE(CHIP_AWAKE);
+			return err;
+		}
+	}
+
+finished:
+	SET_SLEEP_STATE(CHIP_SUSPENDED);
+	return 0;
+}
+
+/**
+ * cg2900_uart_resume() - Called to bring a device back from a low power state.
+ * @pdev:	Pointer to platform device.
+ *
+ * In UART case, CG2900 driver does nothing on resume.
+ *
+ * Returns:
+ *   0 - Success.
+ */
+static int cg2900_uart_resume(struct platform_device *pdev)
+{
+	if (uart_info->sleep_state != CHIP_POWERED_DOWN)
+		update_timer();
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+/**
+ * cg2900_enable_regulator() - Enable regulator.
+ *
+ * Returns:
+ *   0 - Success.
+ *   Error from regulator_get, regulator_enable.
+ */
+static int cg2900_enable_regulator(void)
+{
+#ifdef CONFIG_REGULATOR
+	int err;
+
+	/* Get and enable regulator. */
+	uart_info->regulator = regulator_get(uart_info->dev->parent, "gbf_1v8");
+	if (IS_ERR(uart_info->regulator)) {
+		CG2900_ERR("Not able to find regulator.");
+		err = PTR_ERR(uart_info->regulator);
+	} else {
+		err = regulator_enable(uart_info->regulator);
+		if (err)
+			CG2900_ERR("Not able to enable regulator.");
+		else
+			uart_info->regulator_enabled = true;
+	}
+	return err;
+#else
+	return 0;
+#endif
+}
+
+/**
+ * cg2900_disable_regulator() - Disable regulator.
+ *
+ */
+static void cg2900_disable_regulator(void)
+{
+#ifdef CONFIG_REGULATOR
+	/* Disable and put regulator. */
+	if (uart_info->regulator && uart_info->regulator_enabled) {
+		regulator_disable(uart_info->regulator);
+		uart_info->regulator_enabled = false;
+	}
+	regulator_put(uart_info->regulator);
+	uart_info->regulator = NULL;
+#endif
+}
+
+/**
+ * is_set_baud_rate_cmd() - Checks if data contains set baud rate hci cmd.
+ * @data:	Pointer to data array to check.
+ *
+ * Returns:
+ *   true - if cmd found;
+ *   false - otherwise.
+ */
+static bool is_set_baud_rate_cmd(const char *data)
+{
+	bool cmd_match = false;
+
+	if ((data[0] == HCI_BT_CMD_H4_CHANNEL) &&
+	    (data[1] == SET_BAUD_RATE_LSB) &&
+	    (data[2] == SET_BAUD_RATE_MSB) &&
+	    (data[3] == SET_BAUD_RATE_PAYL_LEN)) {
+		cmd_match = true;
+	}
+	return cmd_match;
+}
+
+/**
+ * is_bt_cmd_complete_no_param() - Checks if data contains command
complete event for a certain command.
+ * @skb:	sk_buffer containing the data including H:4 header.
+ * @cmd_lsb:	Command LSB.
+ * @cmd_msb:	Command MSB.
+ *
+ * Returns:
+ *   true - If this is the command complete we were looking for;
+ *   false - otherwise.
+ */
+static bool is_bt_cmd_complete_no_param(struct sk_buff *skb, u8 cmd_lsb,
+					u8 cmd_msb)
+{
+	if ((HCI_BT_EVT_H4_CHANNEL == skb->data[0]) &&
+	    (HCI_BT_EVT_CMD_COMPLETE == skb->data[1]) &&
+	    (HCI_BT_CMD_COMPLETE_NO_PARAM_LEN == skb->data[2]) &&
+	    (cmd_lsb == skb->data[4]) &&
+	    (cmd_msb == skb->data[5]))
+		return true;
+
+	return false;
+}
+
+/**
+ * alloc_rx_skb() - Alloc an sk_buff structure for receiving data
from controller.
+ * @size:	Size in number of octets.
+ * @priority:	Allocation priority, e.g. GFP_KERNEL.
+ *
+ * Returns:
+ *   Pointer to sk_buff structure.
+ */
+static struct sk_buff *alloc_rx_skb(unsigned int size, gfp_t priority)
+{
+	struct sk_buff *skb;
+
+	/* Allocate the SKB and reserve space for the header */
+	skb = alloc_skb(size + RX_SKB_RESERVE, priority);
+	if (skb)
+		skb_reserve(skb, RX_SKB_RESERVE);
+
+	return skb;
+}
+
+/**
+ * finish_setting_baud_rate() - Handles sending the ste baud rate hci cmd.
+ * @tty:	Pointer to a tty_struct used to communicate with tty driver.
+ *
+ * finish_setting_baud_rate() makes sure that the set baud rate cmd has
+ * been really sent out on the wire and then switches the tty driver to new
+ * baud rate.
+ */
+static void finish_setting_baud_rate(struct tty_struct *tty)
+{
+	/*
+	 * Give the tty driver time to send data and proceed. If it hasn't
+	 * been sent we can't do much about it anyway.
+	 */
+	schedule_timeout_interruptible(msecs_to_jiffies(UART_TX_TIMEOUT));
+
+	/*
+	 * Now set the termios struct to the new baudrate. Start by storing
+	 * the old termios.
+	 */
+	if (set_tty_baud(tty, uart_info->baud_rate)) {
+		CG2900_DBG("Setting termios to new baud rate");
+		SET_BAUD_STATE(BAUD_WAITING);
+	} else
+		SET_BAUD_STATE(BAUD_IDLE);
+
+	tty_unthrottle(tty);
+}
+
+/**
+ * alloc_set_baud_rate_cmd() - Allocates new sk_buff and fills in the
change baud rate hci cmd.
+ * @baud:	(in/out) Requested new baud rate. Updated to default baud rate
+ *		upon invalid value.
+ *
+ * Returns:
+ *   Pointer to allocated sk_buff if successful;
+ *   NULL otherwise.
+ */
+static struct sk_buff *alloc_set_baud_rate_cmd(int *baud)
+{
+	struct sk_buff *skb;
+	u8 data[SET_BAUD_RATE_LEN];
+	u8 *h4;
+
+	skb = cg2900_alloc_skb(SET_BAUD_RATE_LEN, GFP_ATOMIC);
+	if (!skb) {
+		CG2900_ERR("Failed to alloc skb!");
+		return NULL;
+	}
+
+	/* Create the Hci_Cmd_ST_Set_Uart_Baud_Rate packet */
+	data[0] = SET_BAUD_RATE_LSB;
+	data[1] = SET_BAUD_RATE_MSB;
+	data[2] = SET_BAUD_RATE_PAYL_LEN;
+
+	switch (*baud) {
+	case 57600:
+		data[3] = BAUD_RATE_57600;
+		break;
+	case 115200:
+		data[3] = BAUD_RATE_115200;
+		break;
+	case 230400:
+		data[3] = BAUD_RATE_230400;
+		break;
+	case 460800:
+		data[3] = BAUD_RATE_460800;
+		break;
+	case 921600:
+		data[3] = BAUD_RATE_921600;
+		break;
+	case 2000000:
+		data[3] = BAUD_RATE_2000000;
+		break;
+	case 3000000:
+		data[3] = BAUD_RATE_3000000;
+		break;
+	case 4000000:
+		data[3] = BAUD_RATE_4000000;
+		break;
+	default:
+		CG2900_ERR("Invalid speed requested (%d), using 115200 bps "
+			   "instead\n", *baud);
+		data[3] = BAUD_RATE_115200;
+		*baud = 115200;
+		break;
+	};
+
+	memcpy(skb_put(skb, SET_BAUD_RATE_LEN), data, SET_BAUD_RATE_LEN);
+	h4 = skb_push(skb, HCI_H4_SIZE);
+	*h4 = HCI_BT_CMD_H4_CHANNEL;
+
+	return skb;
+}
+
+/**
+ * work_do_transmit() - Transmit data packet to connectivity
controller over UART.
+ * @work:	Pointer to work info structure. Contains uart_info structure
+ *		pointer.
+ */
+static void work_do_transmit(struct work_struct *work)
+{
+	struct sk_buff *skb;
+	struct tty_struct *tty;
+	struct uart_work_struct *current_work;
+
+	if (!work) {
+		CG2900_ERR("work == NULL");
+		return;
+	}
+
+	/* Restart timer. */
+	update_timer();
+
+	current_work = container_of(work, struct uart_work_struct, work);
+
+	if (uart_info->tty)
+		tty = uart_info->tty;
+	else {
+		CG2900_ERR("Important structs not allocated!");
+		goto finished;
+	}
+
+	mutex_lock(&uart_info->tx_mutex);
+
+	/* Retrieve the first packet in the queue */
+	skb = skb_dequeue(&uart_info->tx_queue);
+	while (skb) {
+		int len;
+
+		/*
+		 * Tell TTY that there is data waiting and call the write
+		 * function.
+		 */
+		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+		len = tty->ops->write(tty, skb->data, skb->len);
+		CG2900_INFO("Written %d bytes to UART of %d bytes in packet",
+			    len, skb->len);
+
+		/*
+		 * If it's set baud rate cmd set correct baud state and after
+		 * sending is finished inform the tty driver about the new
+		 * baud rate.
+		 */
+		if ((BAUD_START == uart_info->baud_rate_state) &&
+		    (is_set_baud_rate_cmd(skb->data))) {
+			CG2900_INFO("UART set baud rate cmd found.");
+			SET_BAUD_STATE(BAUD_SENDING);
+		}
+
+		/* Remove the bytes written from the sk_buffer */
+		skb_pull(skb, len);
+
+		/*
+		 * If there is more data in this sk_buffer, put it at the start
+		 * of the list and exit the loop
+		 */
+		if (skb->len) {
+			skb_queue_head(&uart_info->tx_queue, skb);
+			break;
+		}
+		/*
+		 * No more data in the sk_buffer. Free it and get next packet in
+		 * queue.
+		 * Check if set baud rate cmd is in sending progress, if so call
+		 * proper function to handle that cmd since it requires special
+		 * attention.
+		 */
+		if (BAUD_SENDING == uart_info->baud_rate_state)
+			finish_setting_baud_rate(tty);
+
+		kfree_skb(skb);
+		skb = skb_dequeue(&uart_info->tx_queue);
+	}
+
+	mutex_unlock(&uart_info->tx_mutex);
+
+finished:
+	kfree(current_work);
+}
+
+/**
+ * work_hw_deregistered() - Handle HW deregistered.
+ * @work: Reference to work data.
+ */
+static void work_hw_deregistered(struct work_struct *work)
+{
+	struct uart_work_struct *current_work;
+	int err;
+
+	if (!work) {
+		CG2900_ERR("work == NULL");
+		return;
+	}
+
+	current_work = container_of(work, struct uart_work_struct, work);
+
+	/* Purge any stored sk_buffers */
+	skb_queue_purge(&uart_info->tx_queue);
+	if (uart_info->rx_skb) {
+		kfree_skb(uart_info->rx_skb);
+		uart_info->rx_skb = NULL;
+	}
+
+	err = cg2900_deregister_trans_driver();
+	if (err)
+		CG2900_ERR("Could not deregister UART from Core (%d)", err);
+
+	kfree(current_work);
+}
+
+/**
+ * set_baud_rate() - Sets new baud rate for the UART.
+ * @baud:	New baud rate.
+ *
+ * This function first sends the HCI command
+ * Hci_Cmd_ST_Set_Uart_Baud_Rate. It then changes the baud rate in HW, and
+ * finally it waits for the Command Complete event for the
+ * Hci_Cmd_ST_Set_Uart_Baud_Rate command.
+ *
+ * Returns:
+ *   0 if there is no error.
+ *   -EALREADY if baud rate change is already in progress.
+ *   -EFAULT if one or more of the UART related structs is not allocated.
+ *   -ENOMEM if skb allocation has failed.
+ *   -EPERM if setting the new baud rate has failed.
+ *   Error codes generated by create_work_item.
+ */
+static int set_baud_rate(int baud)
+{
+	struct tty_struct *tty = NULL;
+	int err = 0;
+	struct sk_buff *skb;
+	int old_baud_rate;
+
+	CG2900_INFO("set_baud_rate (%d baud)", baud);
+
+	if (uart_info->baud_rate_state != BAUD_IDLE) {
+		CG2900_ERR("Trying to set new baud rate before old setting "
+			   "is finished");
+		return -EALREADY;
+	}
+
+	if (uart_info->tty)
+		tty = uart_info->tty;
+	else {
+		CG2900_ERR("Important structs not allocated!");
+		return -EFAULT;
+	}
+
+	tty_throttle(tty);
+
+	/*
+	 * Store old baud rate so that we can restore it if something goes
+	 * wrong.
+	 */
+	old_baud_rate = uart_info->baud_rate;
+
+	skb = alloc_set_baud_rate_cmd(&baud);
+	if (!skb) {
+		CG2900_ERR("alloc_set_baud_rate_cmd failed");
+		return -ENOMEM;
+	}
+
+	SET_BAUD_STATE(BAUD_START);
+	uart_info->baud_rate = baud;
+
+	/* Queue the sk_buffer... */
+	skb_queue_tail(&uart_info->tx_queue, skb);
+
+	/* ... and call the common UART TX function */
+	err = create_work_item(uart_info->wq, work_do_transmit, NULL);
+	if (err) {
+		CG2900_ERR("Failed to send change baud rate cmd, freeing "
+			   "skb.");
+		skb = skb_dequeue_tail(&uart_info->tx_queue);
+		SET_BAUD_STATE(BAUD_IDLE);
+		uart_info->baud_rate = old_baud_rate;
+		kfree_skb(skb);
+		return err;
+	}
+
+	CG2900_DBG("Set baud rate cmd scheduled for sending.");
+
+	/*
+	 * Now wait for the command complete.
+	 * It will come at the new baudrate.
+	 */
+	wait_event_interruptible_timeout(uart_wait_queue,
+				((BAUD_SUCCESS == uart_info->baud_rate_state) ||
+				 (BAUD_FAIL    == uart_info->baud_rate_state)),
+				 msecs_to_jiffies(UART_RESP_TIMEOUT));
+	if (BAUD_SUCCESS == uart_info->baud_rate_state)
+		CG2900_DBG("Baudrate changed to %d baud", baud);
+	else {
+		CG2900_ERR("Failed to set new baudrate (%d)",
+			   uart_info->baud_rate_state);
+		err = -EPERM;
+	}
+
+	/* Finally flush the TTY so we are sure that is no bad data there */
+	if (tty->ops->flush_buffer) {
+		CG2900_DBG("Flushing TTY after baud rate change");
+		tty->ops->flush_buffer(tty);
+	}
+
+	/* Finished. Set state to IDLE */
+	SET_BAUD_STATE(BAUD_IDLE);
+
+	return err;
+}
+
+/**
+ * uart_open() - Open the CG2900 UART for data transfers.
+ * @dev:	Transport device information.
+ *
+ * Returns:
+ *   0 if there is no error,
+ *   -EACCES if write to transport failed,
+ *   -EIO if chip did not answer to commands.
+ */
+static int uart_open(struct cg2900_trans_dev *dev)
+{
+	u8 data[HCI_BT_RESET_LEN + HCI_H4_SIZE];
+	struct tty_struct *tty;
+	int bytes_written;
+
+	/*
+	 * Chip has just been started up. It has a system to autodetect
+	 * exact baud rate and transport to use. There are only a few commands
+	 * it will recognize and HCI Reset is one of them.
+	 * We therefore start with sending that before actually changing
+	 * baud rate.
+	 *
+	 * Create the Hci_Reset packet
+	 */
+	data[0] = HCI_BT_CMD_H4_CHANNEL;
+	data[1] = HCI_BT_RESET_CMD_LSB;
+	data[2] = HCI_BT_RESET_CMD_MSB;
+	data[3] = HCI_BT_RESET_PARAM_LEN;
+
+	/* Get the TTY info and send the packet */
+	tty = uart_info->tty;
+	SET_BAUD_STATE(BAUD_SENDING_RESET);
+	CG2900_DBG("Sending HCI reset before baud rate change");
+	bytes_written = tty->ops->write(tty, data,
+					HCI_BT_RESET_LEN + HCI_H4_SIZE);
+	if (bytes_written != HCI_BT_RESET_LEN + HCI_H4_SIZE) {
+		CG2900_ERR("Only wrote %d bytes", bytes_written);
+		SET_BAUD_STATE(BAUD_IDLE);
+		return -EACCES;
+	}
+
+	/*
+	 * Wait for command complete. If error, exit without changing
+	 * baud rate.
+	 */
+	wait_event_interruptible_timeout(uart_wait_queue,
+					BAUD_IDLE == uart_info->baud_rate_state,
+					msecs_to_jiffies(UART_RESP_TIMEOUT));
+	if (BAUD_IDLE != uart_info->baud_rate_state) {
+		CG2900_ERR("Failed to send HCI Reset");
+		SET_BAUD_STATE(BAUD_IDLE);
+		return -EIO;
+	}
+
+	/* Just return if there will be no change of baud rate */
+	if (uart_default_baud != uart_high_baud)
+		return set_baud_rate(uart_high_baud);
+	else
+		return 0;
+}
+
+/**
+ * uart_set_chip_power() - Enable or disable the CG2900.
+ * @chip_on:	true if chip shall be enabled, false otherwise.
+ */
+static void uart_set_chip_power(bool chip_on)
+{
+	int uart_baudrate = uart_default_baud;
+	struct tty_struct *tty;
+	struct cg2900_platform_data *pf_data;
+
+	CG2900_INFO("uart_set_chip_power: %s",
+		    (chip_on ? "ENABLE" : "DISABLE"));
+
+	if (uart_info->tty)
+		tty = uart_info->tty;
+	else {
+		CG2900_ERR("Important structs not allocated!");
+		return;
+	}
+
+	pf_data = dev_get_platdata(uart_info->dev->parent);
+
+	if (chip_on) {
+		if (cg2900_enable_regulator())
+			return;
+		if (pf_data->enable_chip) {
+			pf_data->enable_chip();
+			SET_SLEEP_STATE(CHIP_AWAKE);
+		}
+	} else {
+		if (pf_data->disable_chip) {
+			pf_data->disable_chip();
+			SET_SLEEP_STATE(CHIP_POWERED_DOWN);
+		}
+
+		cg2900_disable_regulator();
+		/*
+		 * Setting baud rate to 0 will tell UART driver to shut off its
+		 * clocks.
+		 */
+		uart_baudrate = ZERO_BAUD_RATE;
+	}
+	/*
+	 * Now we have to set the digital baseband UART
+	 * to default baudrate if chip is ON or to zero baudrate if
+	 * chip is turning OFF.
+	 */
+	 (void)set_tty_baud(tty, uart_baudrate);
+}
+
+/**
+ * uart_chip_startup_finished() - CG2900 startup finished.
+ */
+static void uart_chip_startup_finished(void)
+{
+	/* Run the timer. */
+	update_timer();
+}
+/**
+ * uart_close() - Close the CG2900 UART for data transfers.
+ * @dev:	Transport device information.
+ *
+ * Returns:
+ *   0 if there is no error.
+ */
+static int uart_close(struct cg2900_trans_dev *dev)
+{
+	/* The chip is already shut down. Power off the chip. */
+	uart_set_chip_power(false);
+
+	return 0;
+}
+
+/**
+ * uart_write() - Transmit data to CG2900 over UART.
+ * @dev:	Transport device information.
+ * @skb:	SK buffer to transmit.
+ *
+ * Returns:
+ *   0 if there is no error.
+ *   Errors from create_work_item.
+ */
+static int uart_write(struct cg2900_trans_dev *dev, struct sk_buff *skb)
+{
+	int err;
+
+	/* Delete sleep timer. */
+	(void)del_timer(&uart_info->timer);
+
+	CG2900_DBG_DATA_CONTENT("uart_write", skb->data, skb->len);
+
+	/* Queue the sk_buffer... */
+	skb_queue_tail(&uart_info->tx_queue, skb);
+
+	/* ...and start TX operation */
+	err = create_work_item(uart_info->wq, work_do_transmit, NULL);
+	if (err)
+		CG2900_ERR("Failed to create work item (%d) uart_tty_wakeup",
+			   err);
+
+	return err;
+}
+
+/**
+ * send_skb_to_core() - Sends packet received from UART to CG2900 Core.
+ * @skb:	Received data packet.
+ *
+ * This function checks if UART is waiting for Command complete event,
+ * see set_baud_rate.
+ * If it is waiting it checks if it is the expected packet and the status.
+ * If not is passes the packet to CG2900 Core.
+ */
+static void send_skb_to_core(struct sk_buff *skb)
+{
+	u8 status;
+
+	if (!skb) {
+		CG2900_ERR("Received NULL as skb");
+		return;
+	}
+
+	if (BAUD_WAITING == uart_info->baud_rate_state) {
+		/*
+		 * Should only really be one packet received now:
+		 * the CmdComplete for the SetBaudrate command
+		 * Let's see if this is the packet we are waiting for.
+		 */
+		if (!is_bt_cmd_complete_no_param(skb, SET_BAUD_RATE_LSB,
+						 SET_BAUD_RATE_MSB)) {
+			/*
+			 * Received other event. Should not really happen,
+			 * but pass the data to CG2900 Core anyway.
+			 */
+			CG2900_DBG_DATA("Sending packet to CG2900 Core while "
+					"waiting for CmdComplete");
+			cg2900_data_from_chip(skb);
+			return;
+		}
+
+		/*
+		 * We have received complete event for our baud rate
+		 * change command
+		 */
+		status = skb->data[HCI_BT_EVT_CMD_COMPL_STATUS_POS +
+				   HCI_H4_SIZE];
+		if (HCI_BT_ERROR_NO_ERROR == status) {
+			CG2900_DBG("Received baud rate change complete "
+				   "event OK");
+			SET_BAUD_STATE(BAUD_SUCCESS);
+		} else {
+			CG2900_ERR("Received baud rate change complete event "
+				   "with status 0x%X", status);
+			SET_BAUD_STATE(BAUD_FAIL);
+		}
+		wake_up_interruptible(&uart_wait_queue);
+		kfree_skb(skb);
+	} else if (BAUD_SENDING_RESET == uart_info->baud_rate_state) {
+		/*
+		 * Should only really be one packet received now:
+		 * the CmdComplete for the Reset command
+		 * Let's see if this is the packet we are waiting for.
+		 */
+		if (!is_bt_cmd_complete_no_param(skb, HCI_BT_RESET_CMD_LSB,
+						 HCI_BT_RESET_CMD_MSB)) {
+			/*
+			 * Received other event. Should not really happen,
+			 * but pass the data to CG2900 Core anyway.
+			 */
+			CG2900_DBG_DATA("Sending packet to CG2900 Core while "
+					"waiting for CmdComplete");
+			cg2900_data_from_chip(skb);
+			return;
+		}
+
+		/*
+		 * We have received complete event for our baud rate
+		 * change command
+		 */
+		status = skb->data[HCI_BT_EVT_CMD_COMPL_STATUS_POS +
+				   HCI_H4_SIZE];
+		if (HCI_BT_ERROR_NO_ERROR == status) {
+			CG2900_DBG("Received HCI reset complete event OK");
+			/*
+			 * Go back to BAUD_IDLE since this was not really
+			 * baud rate change but just a preparation of the chip
+			 * to be ready to receive commands.
+			 */
+			SET_BAUD_STATE(BAUD_IDLE);
+		} else {
+			CG2900_ERR("Received HCI reset complete event with "
+				   "status 0x%X", status);
+			SET_BAUD_STATE(BAUD_FAIL);
+		}
+		wake_up_interruptible(&uart_wait_queue);
+		kfree_skb(skb);
+	} else {
+		/* Just pass data to CG2900 Core */
+		cg2900_data_from_chip(skb);
+	}
+}
+
+static struct cg2900_trans_callbacks uart_cb = {
+	.open = uart_open,
+	.close = uart_close,
+	.write = uart_write,
+	.set_chip_power = uart_set_chip_power,
+	.chip_startup_finished  = uart_chip_startup_finished
+};
+
+/**
+ * check_data_len() - Check number of bytes to receive.
+ * @len:	Number of bytes left to receive.
+ */
+static void check_data_len(int len)
+{
+	/* First get number of bytes left in the sk_buffer */
+	register int room = skb_tailroom(uart_info->rx_skb);
+
+	if (!len) {
+		/* No data left to receive. Transmit to CG2900 Core */
+		send_skb_to_core(uart_info->rx_skb);
+	} else if (len > room) {
+		CG2900_ERR("Data length is too large (%d > %d)", len, room);
+		kfree_skb(uart_info->rx_skb);
+	} else {
+		/*
+		 * "Normal" case. Switch to data receiving state and store
+		 * data length.
+		 */
+		uart_info->rx_state = W4_DATA;
+		uart_info->rx_count = len;
+		return;
+	}
+
+	uart_info->rx_state = W4_PACKET_TYPE;
+	uart_info->rx_skb   = NULL;
+	uart_info->rx_count = 0;
+}
+
+/**
+ * uart_receive_skb() - Handles received UART data.
+ * @data:	Data received
+ * @count:	Number of bytes received
+ *
+ * The uart_receive_skb() function handles received UART data and puts it
+ * together to one complete packet.
+ *
+ * Returns:
+ *   Number of bytes not handled, i.e. 0 = no error.
+ */
+static int uart_receive_skb(const u8 *data, int count)
+{
+	const u8 *r_ptr;
+	u8 *w_ptr;
+	int len;
+	struct hci_event_hdr	*evt;
+	struct hci_acl_hdr	*acl;
+	union fm_leg_evt_or_irq	*fm;
+	struct gnss_hci_hdr	*gnss;
+	u8 *tmp;
+
+	r_ptr = data;
+	/* Continue while there is data left to handle */
+	while (count) {
+		/*
+		 * If we have already received a packet we know how many bytes
+		 * there are left.
+		 */
+		if (!uart_info->rx_count)
+			goto check_h4_header;
+
+		/* First copy received data into the skb_rx */
+		len = min_t(unsigned int, uart_info->rx_count, count);
+		memcpy(skb_put(uart_info->rx_skb, len), r_ptr, len);
+		/* Update counters from the length and step the data pointer */
+		uart_info->rx_count -= len;
+		count -= len;
+		r_ptr += len;
+
+		if (uart_info->rx_count)
+			/*
+			 * More data to receive to current packet. Break and
+			 * wait for next data on the UART.
+			 */
+			break;
+
+		/* Handle the different states */
+		tmp = uart_info->rx_skb->data + CG2900_SKB_RESERVE;
+		switch (uart_info->rx_state) {
+		case W4_DATA:
+			/*
+			 * Whole data packet has been received.
+			 * Transmit it to CG2900 Core.
+			 */
+			send_skb_to_core(uart_info->rx_skb);
+
+			uart_info->rx_state = W4_PACKET_TYPE;
+			uart_info->rx_skb = NULL;
+			continue;
+
+		case W4_EVENT_HDR:
+			evt = (struct hci_event_hdr *)tmp;
+			check_data_len(evt->plen);
+			/* Header read. Continue with next bytes */
+			continue;
+
+		case W4_ACL_HDR:
+			acl = (struct hci_acl_hdr *)tmp;
+			check_data_len(le16_to_cpu(acl->dlen));
+			/* Header read. Continue with next bytes */
+			continue;
+
+		case W4_FM_RADIO_HDR:
+			fm = (union fm_leg_evt_or_irq *)tmp;
+			check_data_len(fm->param_length);
+			/* Header read. Continue with next bytes */
+			continue;
+
+		case W4_GNSS_HDR:
+			gnss = (struct gnss_hci_hdr *)tmp;
+			check_data_len(le16_to_cpu(gnss->plen));
+			/* Header read. Continue with next bytes */
+			continue;
+
+		default:
+			CG2900_ERR("Bad state indicating memory overwrite "
+				   "(0x%X)", (u8)(uart_info->rx_state));
+			break;
+		}
+
+check_h4_header:
+		/* Check which H:4 packet this is and update RX states */
+		if (*r_ptr == HCI_BT_EVT_H4_CHANNEL) {
+			uart_info->rx_state = W4_EVENT_HDR;
+			uart_info->rx_count = HCI_BT_EVT_HDR_SIZE;
+		} else if (*r_ptr == HCI_BT_ACL_H4_CHANNEL) {
+			uart_info->rx_state = W4_ACL_HDR;
+			uart_info->rx_count = HCI_BT_ACL_HDR_SIZE;
+		} else if (*r_ptr == HCI_FM_RADIO_H4_CHANNEL) {
+			uart_info->rx_state = W4_FM_RADIO_HDR;
+			uart_info->rx_count = HCI_FM_RADIO_HDR_SIZE;
+		} else if (*r_ptr == HCI_GNSS_H4_CHANNEL) {
+			uart_info->rx_state = W4_GNSS_HDR;
+			uart_info->rx_count = HCI_GNSS_HDR_SIZE;
+		} else {
+			CG2900_ERR("Unknown HCI packet type 0x%X", (u8)*r_ptr);
+			r_ptr++;
+			count--;
+			continue;
+		}
+
+		/*
+		 * Allocate packet. We do not yet know the size and therefore
+		 * allocate max size.
+		 */
+		uart_info->rx_skb = alloc_rx_skb(RX_SKB_MAX_SIZE, GFP_ATOMIC);
+		if (!uart_info->rx_skb) {
+			CG2900_ERR("Can't allocate memory for new packet");
+			uart_info->rx_state = W4_PACKET_TYPE;
+			uart_info->rx_count = 0;
+			return 0;
+		}
+
+		/* Write the H:4 header first in the sk_buffer */
+		w_ptr = skb_put(uart_info->rx_skb, 1);
+		*w_ptr = *r_ptr;
+
+		/* First byte (H4 header) read. Goto next byte */
+		r_ptr++;
+		count--;
+	}
+
+	return count;
+}
+
+/**
+ * uart_tty_open() - Called when UART line discipline changed to N_HCI.
+ * @tty:	Pointer to associated TTY instance data.
+ *
+ * Returns:
+ *   0 if there is no error.
+ *   Errors from cg2900_register_trans_driver.
+ */
+static int uart_tty_open(struct tty_struct *tty)
+{
+	int err;
+
+	CG2900_INFO("uart_tty_open");
+
+	/* Set the structure pointers and set the UART receive room */
+	uart_info->tty = tty;
+	tty->disc_data = NULL;
+	tty->receive_room = UART_RECEIVE_ROOM;
+
+	/*
+	 * Flush any pending characters in the driver and line discipline.
+	 * Don't use ldisc_ref here as the open path is before the ldisc is
+	 * referencable.
+	 */
+	if (tty->ldisc->ops->flush_buffer)
+		tty->ldisc->ops->flush_buffer(tty);
+
+	tty_driver_flush_buffer(tty);
+
+	/* Tell CG2900 Core that UART is connected */
+	err = cg2900_register_trans_driver(&uart_cb, NULL);
+	if (err)
+		CG2900_ERR("Could not register transport driver (%d)", err);
+
+	if (tty->ops->tiocmget && tty->ops->break_ctl)
+		uart_info->sleep_allowed = true;
+	else
+		CG2900_ERR("Sleep mode not available.");
+
+	return err;
+
+}
+
+/**
+ * uart_tty_close() - Close UART tty.
+ * @tty:	Pointer to associated TTY instance data.
+ *
+ * The uart_tty_close() function is called when the line discipline is changed
+ * to something else, the TTY is closed, or the TTY detects a hangup.
+ */
+static void uart_tty_close(struct tty_struct *tty)
+{
+	int err;
+
+	CG2900_INFO("uart_tty_close");
+
+	BUG_ON(!uart_info);
+	BUG_ON(!uart_info->wq);
+
+	err = create_work_item(uart_info->wq, work_hw_deregistered, NULL);
+	if (err)
+		CG2900_ERR("Failed to create work item (%d) "
+			   "work_hw_deregistered", err);
+}
+
+/**
+ * uart_tty_wakeup() - Callback function for transmit wake up.
+ * @tty:	Pointer to associated TTY instance data.
+ *
+ * The uart_tty_wakeup() callback function is called when low level
+ * device driver can accept more send data.
+ */
+static void uart_tty_wakeup(struct tty_struct *tty)
+{
+	int err;
+
+	CG2900_INFO("uart_tty_wakeup");
+
+	/*
+	 * Clear the TTY_DO_WRITE_WAKEUP bit that is set in
+	 * work_do_transmit().
+	 */
+	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+
+	if (tty != uart_info->tty)
+		return;
+
+	/* Delete sleep timer. */
+	(void)del_timer(&uart_info->timer);
+
+	/* Start TX operation */
+	err = create_work_item(uart_info->wq, work_do_transmit, NULL);
+	if (err)
+		CG2900_ERR("Failed to create work item (%d) uart_tty_wakeup",
+			   err);
+}
+
+/**
+ * uart_tty_receive() - Called by TTY low level driver when receive
data is available.
+ * @tty:	Pointer to TTY instance data
+ * @data:	Pointer to received data
+ * @flags:	Pointer to flags for data
+ * @count:	Count of received data in bytes
+ */
+static void uart_tty_receive(struct tty_struct *tty, const u8 *data,
+			     char *flags, int count)
+{
+	CG2900_INFO("uart_tty_receive");
+
+	if (tty != uart_info->tty)
+		return;
+
+	CG2900_DBG_DATA("Received data with length = %d and first byte 0x%02X",
+			count, data[0]);
+	CG2900_DBG_DATA_CONTENT("uart_tty_receive", data, count);
+
+	/* Restart data timer */
+	update_timer();
+	spin_lock(&uart_info->rx_lock);
+	uart_receive_skb(data, count);
+	spin_unlock(&uart_info->rx_lock);
+
+}
+
+/**
+ * uart_tty_ioctl() -  Process IOCTL system call for the TTY device.
+ * @tty:   Pointer to TTY instance data.
+ * @file:  Pointer to open file object for device.
+ * @cmd:   IOCTL command code.
+ * @arg:   Argument for IOCTL call (cmd dependent).
+ *
+ * Returns:
+ *   0 if there is no error.
+ *   -EBADF if supplied TTY struct is not correct.
+ *   Error codes from n_tty_iotcl_helper.
+ */
+static int uart_tty_ioctl(struct tty_struct *tty, struct file *file,
+			  unsigned int cmd, unsigned long arg)
+{
+	int err = 0;
+
+	CG2900_INFO("uart_tty_ioctl cmd %d", cmd);
+	CG2900_DBG("DIR: %d, TYPE: %d, NR: %d, SIZE: %d", _IOC_DIR(cmd),
+		   _IOC_TYPE(cmd), _IOC_NR(cmd), _IOC_SIZE(cmd));
+
+
+
+	switch (cmd) {
+	case HCIUARTSETFD:
+		/* Save file object to device. */
+		if (!uart_info->fd)
+			uart_info->fd = file;
+		else
+			CG2900_DBG("Cannot store file object to device.");
+		break;
+	case HCIUARTSETPROTO: /* Fallthrough */
+	case HCIUARTGETPROTO:
+	case HCIUARTGETDEVICE:
+		/*
+		 * We don't do anything special here, but we have to show we
+		 * handle it.
+		 */
+		break;
+
+	default:
+		err = n_tty_ioctl_helper(tty, file, cmd, arg);
+		break;
+	};
+
+	return err;
+}
+
+/*
+ * We don't provide read/write/poll interface for user space.
+ */
+static ssize_t uart_tty_read(struct tty_struct *tty, struct file *file,
+			     unsigned char __user *buf, size_t nr)
+{
+	CG2900_INFO("uart_tty_read");
+	return 0;
+}
+
+static ssize_t uart_tty_write(struct tty_struct *tty, struct file *file,
+			      const unsigned char *data, size_t count)
+{
+	CG2900_INFO("uart_tty_write");
+	return count;
+}
+
+static unsigned int uart_tty_poll(struct tty_struct *tty, struct file *filp,
+				  poll_table *wait)
+{
+	return 0;
+}
+
+/* Generic functions */
+
+/* The uart_ldisc structure is used when registering to the UART framework. */
+static struct tty_ldisc_ops uart_ldisc = {
+	.magic        = TTY_LDISC_MAGIC,
+	.name         = "n_hci",
+	.open         = uart_tty_open,
+	.close        = uart_tty_close,
+	.read         = uart_tty_read,
+	.write        = uart_tty_write,
+	.ioctl        = uart_tty_ioctl,
+	.poll         = uart_tty_poll,
+	.receive_buf  = uart_tty_receive,
+	.write_wakeup = uart_tty_wakeup,
+	.owner        = THIS_MODULE
+};
+
+/**
+ * cg2900_uart_probe() - Initialize CG2900 UART resources.
+ * @pdev:	Platform device.
+ *
+ * This function initializes the module and registers to the UART framework.
+ *
+ * Returns:
+ *   0 if success.
+ *   -ENOMEM for failed alloc or structure creation.
+ *   -ECHILD for failed work queue creation.
+ *   Error codes generated by tty_register_ldisc.
+ */
+static int __devinit cg2900_uart_probe(struct platform_device *pdev)
+{
+	int err = 0;
+
+	CG2900_INFO("cg2900_uart_probe");
+
+	uart_info = kzalloc(sizeof(*uart_info), GFP_KERNEL);
+	if (!uart_info) {
+		CG2900_ERR("Couldn't allocate uart_info");
+		return -ENOMEM;
+	}
+
+	uart_info->sleep_state = CHIP_POWERED_DOWN;
+	skb_queue_head_init(&uart_info->tx_queue);
+	mutex_init(&uart_info->tx_mutex);
+	spin_lock_init(&uart_info->rx_lock);
+	mutex_init(&(uart_info->sleep_state_lock));
+
+	/* Init UART TX work queue */
+	uart_info->wq = create_singlethread_workqueue(UART_WQ_NAME);
+	if (!uart_info->wq) {
+		CG2900_ERR("Could not create workqueue");
+		err = -ECHILD; /* No child processes */
+		goto error_handling_wq;
+	}
+	init_timer(&uart_info->timer);
+	uart_info->timer.function = sleep_timer_expired;
+	uart_info->timer.expires  = jiffies + cg2900_get_sleep_timeout();
+
+	/* Register the tty discipline. We will see what will be used. */
+	err = tty_register_ldisc(N_HCI, &uart_ldisc);
+	if (err) {
+		CG2900_ERR("HCI line discipline registration failed. (0x%X)",
+			   err);
+		goto error_handling_register;
+	}
+
+	uart_info->dev = &pdev->dev;
+
+	goto finished;
+
+error_handling_register:
+	destroy_workqueue(uart_info->wq);
+error_handling_wq:
+	mutex_destroy(&uart_info->tx_mutex);
+	kfree(uart_info);
+	uart_info = NULL;
+finished:
+	return err;
+}
+
+/**
+ * cg2900_uart_remove() - Release CG2900 UART resources.
+ * @pdev:	Platform device.
+ *
+ * Returns:
+ *   0 if success.
+ *   Error codes generated by tty_unregister_ldisc.
+ */
+static int __devexit cg2900_uart_remove(struct platform_device *pdev)
+{
+	int err;
+
+	CG2900_INFO("cg2900_uart_remove");
+
+	/* Release tty registration of line discipline */
+	err = tty_unregister_ldisc(N_HCI);
+	if (err)
+		CG2900_ERR("Can't unregister HCI line discipline (%d)", err);
+
+	if (!uart_info)
+		return err;
+
+	destroy_workqueue(uart_info->wq);
+	mutex_destroy(&uart_info->tx_mutex);
+
+	kfree(uart_info);
+	uart_info = NULL;
+	return err;
+}
+
+static struct platform_driver cg2900_uart_driver = {
+	.driver = {
+		.name	= "cg2900-uart",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= cg2900_uart_probe,
+	.remove	= __devexit_p(cg2900_uart_remove),
+#ifdef CONFIG_PM
+	.suspend = cg2900_uart_suspend,
+	.resume = cg2900_uart_resume
+#endif
+};
+
+/**
+ * cg2900_uart_init() - Initialize module.
+ *
+ * Registers platform driver.
+ */
+static int __init cg2900_uart_init(void)
+{
+	CG2900_INFO("cg2900_uart_init");
+	return platform_driver_register(&cg2900_uart_driver);
+}
+
+/**
+ * cg2900_uart_exit() - Remove module.
+ *
+ * Unregisters platform driver.
+ */
+static void __exit cg2900_uart_exit(void)
+{
+	CG2900_INFO("cg2900_uart_exit");
+	platform_driver_unregister(&cg2900_uart_driver);
+}
+
+module_init(cg2900_uart_init);
+module_exit(cg2900_uart_exit);
+
+module_param(uart_default_baud, int, S_IRUGO);
+MODULE_PARM_DESC(uart_default_baud,
+		 "Default UART baud rate, e.g. 115200. If not set 115200 will "
+		 "be used.");
+
+module_param(uart_high_baud, int, S_IRUGO | S_IWUSR | S_IWGRP);
+MODULE_PARM_DESC(uart_high_baud,
+		 "High speed UART baud rate, e.g. 4000000.  If not set 3000000 "
+		 "will be used.");
+
+MODULE_AUTHOR("Par-Gunnar Hjalmdahl ST-Ericsson");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ST-Ericsson CG2900 UART Driver");
-- 
1.6.3.3

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Alan Cox]

> + * is_chip_flow_off() - Check if chip has set flow off.
> + * @tty:	Pointer to tty.
> + *
> + * Returns:
> + *   true - chip flows off.
> + *   false - chip flows on.
> + */
> +static bool is_chip_flow_off(struct tty_struct *tty)
> +{
> +	int lines;
> +
> +	lines = tty->ops->tiocmget(tty, uart_info->fd);
> +
> +	if (lines & TIOCM_CTS)
> +		return false;
> +	else
> +		return true;
> +}

What if the device doesn't have a tiocmget ? You must check this. If you
want to call into the ttys own tiocmget froma sleeping context fine, but
see tty_tiocmget and you'll notice you need to check the op is non NULL
somewhere. You could do this when the ldisc is opened and refuse to open
- some ldiscs do that


> +
> +/**
> + * create_work_item() - Create work item and add it to the work queue.
> + * @wq:		work queue struct where the work will be added.
> + * @work_func:	Work function.
> + * @data:	Private data for the work.
> + *
> + * Returns:
> + *   0 if there is no error.
> + *   -EBUSY if not possible to queue work.
> + *   -ENOMEM if allocation fails.
> + */
> +static int create_work_item(struct workqueue_struct *wq, work_func_t work_func,
> +			    void *data)
> +{
> +	struct uart_work_struct *new_work;
> +	int err;
> +
> +	new_work = kmalloc(sizeof(*new_work), GFP_ATOMIC);

So instead of a tiny object embedded in your device you've got a whole
error path to worry about, a printk disguised in a macro and a text
string longer than the struct size ? Surely this should be part of the
device

> +	if (!new_work) {
> +		CG2900_ERR("Failed to alloc memory for uart_work_struct!");

Please use the standard dev_dbg etc functionality - or pr_err etc when
you have no device pointer. The newest kernel tty object has a device
pointer added so you could use that.


> + * set_tty_baud() - Called to set specific baud in TTY.
> + * @tty:	Tty device.
> + * @baud:	Baud to set.
> + *
> + * Returns:
> + *   true - baudrate set with success.
> + *   false - baundrate set failure.
> + */
> +static bool set_tty_baud(struct tty_struct *tty, int baud)
> +{
> +	struct ktermios *old_termios;
> +	bool retval = true;
> +
> +	old_termios = kmalloc(sizeof(*old_termios), GFP_ATOMIC);

termios struct is easily small enough to be fine on the stack

> +	/* Let's mark that CG2900 driver uses c_ispeed and c_ospeed fields. */
> +	tty->termios->c_cflag |= BOTHER;

This shouldn't be needed - the tty_encode_baud_rate logic works out of
BOTHER must be set

> +	tty->termios->c_cflag &= ~BOTHER;

And your termios is now potentially invalid


> +	/* Set IRQ on CTS. */
> +	err = request_irq(pf_data->uart.cts_irq,
> +			  cts_interrupt,
> +			  IRQF_TRIGGER_FALLING,
> +			  UART_NAME,
> +			  NULL);

So we now ave terminal tty/ldisc confusion

> +	if (err) {
> +		CG2900_ERR("Could not request CTS IRQ (%d)", err);
> +		goto error;
> +	}
> +
> +#ifdef CONFIG_PM
> +	enable_irq_wake(pf_data->uart.cts_irq);
> +#endif
> +	return 0;
> +
> +error:
> +	if (pf_data->uart.enable_uart)
> +		(void)pf_data->uart.enable_uart();
> +	return err;

> +}
> +
> +/**
> + * unset_cts_irq() - Disable interrupt on CTS.
> + */
> +static void unset_cts_irq(void)

And no ability to support multiple devices

> +	CG2900_DBG("Clear break");
> +	tty->ops->break_ctl(tty, TTY_BREAK_OFF);

What if the tty doesn't have one ?


> +	if (CHIP_AWAKE == uart_info->sleep_state) {
> +		uart_info->tty->ops->break_ctl(uart_info->tty, TTY_BREAK_ON);

Ditto


> +		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +		len = tty->ops->write(tty, skb->data, skb->len);

A tty isn't required to have a write function

> +		if ((BAUD_START == uart_info->baud_rate_state) &&
> +		    (is_set_baud_rate_cmd(skb->data))) {
> +			CG2900_INFO("UART set baud rate cmd found.");
> +			SET_BAUD_STATE(BAUD_SENDING);

Do we really need this all in capitals ?



> + * uart_tty_open() - Called when UART line discipline changed to N_HCI.
> + * @tty:	Pointer to associated TTY instance data.
> + *
> + * Returns:
> + *   0 if there is no error.
> + *   Errors from cg2900_register_trans_driver.
> + */
> +static int uart_tty_open(struct tty_struct *tty)
> +{
> +	int err;
> +
> +	CG2900_INFO("uart_tty_open");
> +
> +	/* Set the structure pointers and set the UART receive room */
> +	uart_info->tty = tty;

You must respect the kref handling in the kernel tty code. Take
references by all means but do it properly.


> +static void uart_tty_wakeup(struct tty_struct *tty)
> +{
> +	int err;
> +
> +	CG2900_INFO("uart_tty_wakeup");
> +
> +	/*
> +	 * Clear the TTY_DO_WRITE_WAKEUP bit that is set in
> +	 * work_do_transmit().
> +	 */
> +	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +
> +	if (tty != uart_info->tty)
> +		return;

How can this occur - and why check it after you've changed tty->flags ???


> +/**
> + * uart_tty_receive() - Called by TTY low level driver when receive
> data is available.
> + * @tty:	Pointer to TTY instance data
> + * @data:	Pointer to received data
> + * @flags:	Pointer to flags for data
> + * @count:	Count of received data in bytes
> + */
> +static void uart_tty_receive(struct tty_struct *tty, const u8 *data,
> +			     char *flags, int count)
> +{
> +	CG2900_INFO("uart_tty_receive");
> +
> +	if (tty != uart_info->tty)
> +		return;

Again this is nonsense


> +/*
> + * We don't provide read/write/poll interface for user space.
> + */
> +static ssize_t uart_tty_read(struct tty_struct *tty, struct file *file,
> +			     unsigned char __user *buf, size_t nr)
> +{
> +	CG2900_INFO("uart_tty_read");
> +	return 0;
> +}

-EINVAL then

> +
> +static ssize_t uart_tty_write(struct tty_struct *tty, struct file *file,
> +			      const unsigned char *data, size_t count)
> +{
> +	CG2900_INFO("uart_tty_write");
> +	return count;

This is wrong - you can't jusr vanish the data
> +}



This needs some restructuring I think

A line discipline should contain no hardware awareness, that goes in the
actual uart hardware driver. So we shouldn't have magic irq code in this
part of things.

You also need to sort out the tty kref handling in open/close and the
fact you've got magic hardcoded single instance stuff buried i nit.

Finally tty ldiscs don't go buried in the mfd directory, or they'll get
missed during chanegs/updates. The ldisc probably belongs in bluetooth a
and the hardware support in the mfd directory.


So - NAK this for the moment, it needs to be split cleanly into ldisc
(thing which speaks the protocol and eg sees "speed change required" and
acts on it) and device (thing which knows about the hardware).

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Hi and thanks for your comments Alan.
See below for answers.

/P-G

2010/10/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>
>> + * is_chip_flow_off() - Check if chip has set flow off.
>> + * @tty:     Pointer to tty.
>> + *
>> + * Returns:
>> + *   true - chip flows off.
>> + *   false - chip flows on.
>> + */
>> +static bool is_chip_flow_off(struct tty_struct *tty)
>> +{
>> +     int lines;
>> +
>> +     lines = tty->ops->tiocmget(tty, uart_info->fd);
>> +
>> +     if (lines & TIOCM_CTS)
>> +             return false;
>> +     else
>> +             return true;
>> +}
>
> What if the device doesn't have a tiocmget ? You must check this. If you
> want to call into the ttys own tiocmget froma sleeping context fine, but
> see tty_tiocmget and you'll notice you need to check the op is non NULL
> somewhere. You could do this when the ldisc is opened and refuse to open
> - some ldiscs do that
>

The existence of the callback is checked in the function
uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
sleep and this code will never run.

>
>> +
>> +/**
>> + * create_work_item() - Create work item and add it to the work queue.
>> + * @wq:              work queue struct where the work will be added.
>> + * @work_func:       Work function.
>> + * @data:    Private data for the work.
>> + *
>> + * Returns:
>> + *   0 if there is no error.
>> + *   -EBUSY if not possible to queue work.
>> + *   -ENOMEM if allocation fails.
>> + */
>> +static int create_work_item(struct workqueue_struct *wq, work_func_t work_func,
>> +                         void *data)
>> +{
>> +     struct uart_work_struct *new_work;
>> +     int err;
>> +
>> +     new_work = kmalloc(sizeof(*new_work), GFP_ATOMIC);
>
> So instead of a tiny object embedded in your device you've got a whole
> error path to worry about, a printk disguised in a macro and a text
> string longer than the struct size ? Surely this should be part of the
> device
>

OK. We can embed the work struct in the device structure.

>> +     if (!new_work) {
>> +             CG2900_ERR("Failed to alloc memory for uart_work_struct!");
>
> Please use the standard dev_dbg etc functionality - or pr_err etc when
> you have no device pointer. The newest kernel tty object has a device
> pointer added so you could use that.
>

OK. I like the debug system we have now (using module parameter to set
debug level in runtime), but I've received comments regarding this
before so I assume we will have to switch to standard printouts.
Is it OK to use defines or inline functions to add "CG2900" before and
'\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?
>
>> + * set_tty_baud() - Called to set specific baud in TTY.
>> + * @tty:     Tty device.
>> + * @baud:    Baud to set.
>> + *
>> + * Returns:
>> + *   true - baudrate set with success.
>> + *   false - baundrate set failure.
>> + */
>> +static bool set_tty_baud(struct tty_struct *tty, int baud)
>> +{
>> +     struct ktermios *old_termios;
>> +     bool retval = true;
>> +
>> +     old_termios = kmalloc(sizeof(*old_termios), GFP_ATOMIC);
>
> termios struct is easily small enough to be fine on the stack
>

OK.

>> +     /* Let's mark that CG2900 driver uses c_ispeed and c_ospeed fields. */
>> +     tty->termios->c_cflag |= BOTHER;
>
> This shouldn't be needed - the tty_encode_baud_rate logic works out of
> BOTHER must be set
>

I will have to check with the guy who wrote this code. I think he put
it there for a reason.

>> +     tty->termios->c_cflag &= ~BOTHER;
>
> And your termios is now potentially invalid
>

As above, I will check.

>
>> +     /* Set IRQ on CTS. */
>> +     err = request_irq(pf_data->uart.cts_irq,
>> +                       cts_interrupt,
>> +                       IRQF_TRIGGER_FALLING,
>> +                       UART_NAME,
>> +                       NULL);
>
> So we now ave terminal tty/ldisc confusion
>

The guy responsible for this is thinking through a new design, where
we can move the hardware specific handling to another place.

>> +     if (err) {
>> +             CG2900_ERR("Could not request CTS IRQ (%d)", err);
>> +             goto error;
>> +     }
>> +
>> +#ifdef CONFIG_PM
>> +     enable_irq_wake(pf_data->uart.cts_irq);
>> +#endif
>> +     return 0;
>> +
>> +error:
>> +     if (pf_data->uart.enable_uart)
>> +             (void)pf_data->uart.enable_uart();
>> +     return err;
>
>> +}
>> +
>> +/**
>> + * unset_cts_irq() - Disable interrupt on CTS.
>> + */
>> +static void unset_cts_irq(void)
>
> And no ability to support multiple devices
>

OK. We will try to fix this.

>> +     CG2900_DBG("Clear break");
>> +     tty->ops->break_ctl(tty, TTY_BREAK_OFF);
>
> What if the tty doesn't have one ?
>

See answer above. It is checked in another place.

>
>> +     if (CHIP_AWAKE == uart_info->sleep_state) {
>> +             uart_info->tty->ops->break_ctl(uart_info->tty, TTY_BREAK_ON);
>
> Ditto
>

And ditto as well :-)

>
>> +             set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> +             len = tty->ops->write(tty, skb->data, skb->len);
>
> A tty isn't required to have a write function

I don't quite understand your comment here. This particular code is
inspired of the Bluetooth ldisc code and there it is used like. It
seems to work fine so we do it the same way.
How would you prefer it to be?

>
>> +             if ((BAUD_START == uart_info->baud_rate_state) &&
>> +                 (is_set_baud_rate_cmd(skb->data))) {
>> +                     CG2900_INFO("UART set baud rate cmd found.");
>> +                     SET_BAUD_STATE(BAUD_SENDING);
>
> Do we really need this all in capitals ?
>

We use a define to get a debug printout when setting state, but I
understand it's not so popular so I guess we will have to skip it.
I guess it won't help if we would use an inline function instead of a
define (which would mean we could skip the capitals)?

>
>
>> + * uart_tty_open() - Called when UART line discipline changed to N_HCI.
>> + * @tty:     Pointer to associated TTY instance data.
>> + *
>> + * Returns:
>> + *   0 if there is no error.
>> + *   Errors from cg2900_register_trans_driver.
>> + */
>> +static int uart_tty_open(struct tty_struct *tty)
>> +{
>> +     int err;
>> +
>> +     CG2900_INFO("uart_tty_open");
>> +
>> +     /* Set the structure pointers and set the UART receive room */
>> +     uart_info->tty = tty;
>
> You must respect the kref handling in the kernel tty code. Take
> references by all means but do it properly.
>

OK. We will add tty_kref_get/put handling.

>
>> +static void uart_tty_wakeup(struct tty_struct *tty)
>> +{
>> +     int err;
>> +
>> +     CG2900_INFO("uart_tty_wakeup");
>> +
>> +     /*
>> +      * Clear the TTY_DO_WRITE_WAKEUP bit that is set in
>> +      * work_do_transmit().
>> +      */
>> +     clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> +
>> +     if (tty != uart_info->tty)
>> +             return;
>
> How can this occur - and why check it after you've changed tty->flags ???
>

OK. Will be removed.

>
>> +/**
>> + * uart_tty_receive() - Called by TTY low level driver when receive
>> data is available.
>> + * @tty:     Pointer to TTY instance data
>> + * @data:    Pointer to received data
>> + * @flags:   Pointer to flags for data
>> + * @count:   Count of received data in bytes
>> + */
>> +static void uart_tty_receive(struct tty_struct *tty, const u8 *data,
>> +                          char *flags, int count)
>> +{
>> +     CG2900_INFO("uart_tty_receive");
>> +
>> +     if (tty != uart_info->tty)
>> +             return;
>
> Again this is nonsense
>

OK.Will be removed.

>
>> +/*
>> + * We don't provide read/write/poll interface for user space.
>> + */
>> +static ssize_t uart_tty_read(struct tty_struct *tty, struct file *file,
>> +                          unsigned char __user *buf, size_t nr)
>> +{
>> +     CG2900_INFO("uart_tty_read");
>> +     return 0;
>> +}
>
> -EINVAL then
>

OK. We can probably skip registering the callback function completely otherwise.

>> +
>> +static ssize_t uart_tty_write(struct tty_struct *tty, struct file *file,
>> +                           const unsigned char *data, size_t count)
>> +{
>> +     CG2900_INFO("uart_tty_write");
>> +     return count;
>
> This is wrong - you can't jusr vanish the data

OK. To be changed.

>> +}
>
>
>
> This needs some restructuring I think
>
> A line discipline should contain no hardware awareness, that goes in the
> actual uart hardware driver. So we shouldn't have magic irq code in this
> part of things.
>
> You also need to sort out the tty kref handling in open/close and the
> fact you've got magic hardcoded single instance stuff buried i nit.
>
> Finally tty ldiscs don't go buried in the mfd directory, or they'll get
> missed during chanegs/updates. The ldisc probably belongs in bluetooth a
> and the hardware support in the mfd directory.
>
>
> So - NAK this for the moment, it needs to be split cleanly into ldisc
> (thing which speaks the protocol and eg sees "speed change required" and
> acts on it) and device (thing which knows about the hardware).
>
>

OK. We will try to figure out a new design.
I'm not too happy about putting the ldisc part in Bluetooth though
since it is only partly Bluetooth, it is also GPS and FM. Better could
maybe be under char/?

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Alan Cox]

> The existence of the callback is checked in the function
> uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
> sleep and this code will never run.

OK yes see this now.

> >> +             CG2900_ERR("Failed to alloc memory for uart_work_struct!");
> >
> > Please use the standard dev_dbg etc functionality - or pr_err etc when
> > you have no device pointer. The newest kernel tty object has a device
> > pointer added so you could use that.
> >
> 
> OK. I like the debug system we have now (using module parameter to set
> debug level in runtime), but I've received comments regarding this
> before so I assume we will have to switch to standard printouts.
> Is it OK to use defines or inline functions to add "CG2900" before and
> '\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?

The pr_fmt bit will do what you want for a non dev_dbg type thing. See
include/linux/kernel.h

> >> + * unset_cts_irq() - Disable interrupt on CTS.
> >> + */
> >> +static void unset_cts_irq(void)
> >
> > And no ability to support multiple devices
> >
> 
> OK. We will try to fix this.

That may go away when you clean up the device side. The line discipline
can be attached to any device so must be multi-device aware, the hardware
driver can certainly be single device only if you can only ever have one

[Although its a good idea to design it so it can be fixed because you
 never know when you'll find your sales team just sold someone a two
 device solution ;) ]

> >> +             set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> >> +             len = tty->ops->write(tty, skb->data, skb->len);
> >
> > A tty isn't required to have a write function
> 
> I don't quite understand your comment here. This particular code is
> inspired of the Bluetooth ldisc code and there it is used like. It
> seems to work fine so we do it the same way.

You copied a security hole from the bluetooth driver which we found a
couple of weeks ago

> How would you prefer it to be?

Check it is valid, in your case probably on open just refuse to attach to
a read only port.

> OK. We will try to figure out a new design.
> I'm not too happy about putting the ldisc part in Bluetooth though
> since it is only partly Bluetooth, it is also GPS and FM. Better could
> maybe be under char/?

Works for me - there is an ongoing "we must move tty ldiscs and core tty
code somewhere more sensible of their own" discussion, so if it is
dropped into char, it'll move with them just fine.

Alan

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Thanks again for your comments Alan.

Next patch set will contain resolution to all your comments. See below.

/P-G

2010/10/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> The existence of the callback is checked in the function
>> uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
>> sleep and this code will never run.
>
> OK yes see this now.
>
>> >> +             CG2900_ERR("Failed to alloc memory for uart_work_struct!");
>> >
>> > Please use the standard dev_dbg etc functionality - or pr_err etc when
>> > you have no device pointer. The newest kernel tty object has a device
>> > pointer added so you could use that.
>> >
>>
>> OK. I like the debug system we have now (using module parameter to set
>> debug level in runtime), but I've received comments regarding this
>> before so I assume we will have to switch to standard printouts.
>> Is it OK to use defines or inline functions to add "CG2900" before and
>> '\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?
>
> The pr_fmt bit will do what you want for a non dev_dbg type thing. See
> include/linux/kernel.h
>

OK. Added. I'm however using dev_err, dev_dbg, etc where possible.

>> >> + * unset_cts_irq() - Disable interrupt on CTS.
>> >> + */
>> >> +static void unset_cts_irq(void)
>> >
>> > And no ability to support multiple devices
>> >
>>
>> OK. We will try to fix this.
>
> That may go away when you clean up the device side. The line discipline
> can be attached to any device so must be multi-device aware, the hardware
> driver can certainly be single device only if you can only ever have one
>
> [Although its a good idea to design it so it can be fixed because you
>  never know when you'll find your sales team just sold someone a two
>  device solution ;) ]
>

OK. Design still ongoing, but will be fixed in some way.

>> >> +             set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> >> +             len = tty->ops->write(tty, skb->data, skb->len);
>> >
>> > A tty isn't required to have a write function
>>
>> I don't quite understand your comment here. This particular code is
>> inspired of the Bluetooth ldisc code and there it is used like. It
>> seems to work fine so we do it the same way.
>
> You copied a security hole from the bluetooth driver which we found a
> couple of weeks ago
>
>> How would you prefer it to be?
>
> Check it is valid, in your case probably on open just refuse to attach to
> a read only port.
>

OK. Done.

>> OK. We will try to figure out a new design.
>> I'm not too happy about putting the ldisc part in Bluetooth though
>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>> maybe be under char/?
>
> Works for me - there is an ongoing "we must move tty ldiscs and core tty
> code somewhere more sensible of their own" discussion, so if it is
> dropped into char, it'll move with them just fine.
>
> Alan
>

Again, thanks for the comments.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:

> >
> > So - NAK this for the moment, it needs to be split cleanly into ldisc
> > (thing which speaks the protocol and eg sees "speed change required" and
> > acts on it) and device (thing which knows about the hardware).
> 
> OK. We will try to figure out a new design.
> I'm not too happy about putting the ldisc part in Bluetooth though
> since it is only partly Bluetooth, it is also GPS and FM. Better could
> maybe be under char/?

After getting a better idea of what the base mfd driver does, my impression
is now that you should not register a second N_HCI line discipline at all,
but instead extend the existing line discipline with this number.

I'm not sure what happens if you need two modules that try to register
the same ldisc number, but I imagine it is not good.

Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

2010/10/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> +
>> +/**
>> + * create_work_item() - Create work item and add it to the work queue.
>> + * @wq:              work queue struct where the work will be added.
>> + * @work_func:       Work function.
>> + * @data:    Private data for the work.
>> + *
>> + * Returns:
>> + *   0 if there is no error.
>> + *   -EBUSY if not possible to queue work.
>> + *   -ENOMEM if allocation fails.
>> + */
>> +static int create_work_item(struct workqueue_struct *wq, work_func_t work_func,
>> +                         void *data)
>> +{
>> +     struct uart_work_struct *new_work;
>> +     int err;
>> +
>> +     new_work = kmalloc(sizeof(*new_work), GFP_ATOMIC);
>
> So instead of a tiny object embedded in your device you've got a whole
> error path to worry about, a printk disguised in a macro and a text
> string longer than the struct size ? Surely this should be part of the
> device
>

I've tried now to use 3 separate work_structs in the device (one for
each work function used), but this doesn't work out.
The reason is that the work is generated so often that a work is not
finished before next work of same type comes. This is especially true
for transmit and receive. Then I get 0 back when queuing the work and
there is no real way to solve it from what I can see than to allocate
new work structures every time.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

2010/10/28 Arnd Bergmann <arnd@arndb.de>:
> On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:
>
>> >
>> > So - NAK this for the moment, it needs to be split cleanly into ldisc
>> > (thing which speaks the protocol and eg sees "speed change required" and
>> > acts on it) and device (thing which knows about the hardware).
>>
>> OK. We will try to figure out a new design.
>> I'm not too happy about putting the ldisc part in Bluetooth though
>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>> maybe be under char/?
>
> After getting a better idea of what the base mfd driver does, my impression
> is now that you should not register a second N_HCI line discipline at all,
> but instead extend the existing line discipline with this number.
>
> I'm not sure what happens if you need two modules that try to register
> the same ldisc number, but I imagine it is not good.
>
> Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
>
>        Arnd
>

We also need the ldisc code to handle events from FM and GPS and since
that is chip specific we cannot add that to the generic hci_ldisc
code.
I agree that we might run into problems if two drivers try to register
the same line discipline. It might then be better to introduce a new
line discipline then even though that could cause other problems. I do
not know if it is possible to add a condition in Kconfig otherwise so
the CG2900 ldisc cannot be active while the "normal" ldisc driver is
selected.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

2010/10/29 Par-Gunnar Hjalmdahl <pghatwork@gmail.com>:
> 2010/10/28 Arnd Bergmann <arnd@arndb.de>:
>> On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:
>>
>>> >
>>> > So - NAK this for the moment, it needs to be split cleanly into ldisc
>>> > (thing which speaks the protocol and eg sees "speed change required" and
>>> > acts on it) and device (thing which knows about the hardware).
>>>
>>> OK. We will try to figure out a new design.
>>> I'm not too happy about putting the ldisc part in Bluetooth though
>>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>>> maybe be under char/?
>>
>> After getting a better idea of what the base mfd driver does, my impression
>> is now that you should not register a second N_HCI line discipline at all,
>> but instead extend the existing line discipline with this number.
>>
>> I'm not sure what happens if you need two modules that try to register
>> the same ldisc number, but I imagine it is not good.
>>
>> Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
>>
>>        Arnd
>>
>
> We also need the ldisc code to handle events from FM and GPS and since
> that is chip specific we cannot add that to the generic hci_ldisc
> code.
> I agree that we might run into problems if two drivers try to register
> the same line discipline. It might then be better to introduce a new
> line discipline then even though that could cause other problems. I do
> not know if it is possible to add a condition in Kconfig otherwise so
> the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> selected.
>
> /P-G
>

Hi again,

I might have been a bit too quick there.
The actual channel matching and packet creation is done in hci_h4.c
while ldisc registration is done in hci_ldisc.c.
So it might to be enough to create a new hci_h4-cg2900.c (or similar
name) that can separate the right channels. We must however do changes
to hci_ldisc as well since it seems to always register to the
Bluetooth stack here, which we definitely don't want since that is
handled by btcg2900.c.
Also note that this ldisc issue is only valid when using UART as
transport. We will also support SPI and then we will probably run into
completely new, interesting problems. :-)

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Alan Cox]

> > Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
> 
> We also need the ldisc code to handle events from FM and GPS and since
> that is chip specific we cannot add that to the generic hci_ldisc
> code.

Agreed - it's a different protocol.

> I agree that we might run into problems if two drivers try to register
> the same line discipline. It might then be better to introduce a new

If your ldisc is written properly it shouldn't matter. Each tty has a
private ldisc pointer to keep the per ldisc instance data.

> line discipline then even though that could cause other problems. I do
> not know if it is possible to add a condition in Kconfig otherwise so
> the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> selected.

Not sure I follow the concern here ?

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Alan Cox]

> The reason is that the work is generated so often that a work is not
> finished before next work of same type comes. This is especially true
> for transmit and receive. Then I get 0 back when queuing the work and
> there is no real way to solve it from what I can see than to allocate
> new work structures every time.

So if that is the case what bounds your memory usage - can a busy box end
up with thousands of work queue slos used ? It sounds like your model is
perhaps wrong - if there is a continual stream of work maybe you should
simply have a kernel thread to handle it if it cannot be deferred
- remember ldisc code is able to sleep in most paths.

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Friday 29 October 2010, Alan Cox wrote:

> > line discipline then even though that could cause other problems. I do
> > not know if it is possible to add a condition in Kconfig otherwise so
> > the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> > selected.
> 
> Not sure I follow the concern here ?

It's about the ldisc number. Both bluetooth and cg2900 register themselves
to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.

The solution would not be to make the two ldisc implementations mutually
exclusive, but to make cg2900 use a new ldisc number, e.g. N_HCI_CG2900.
However, I'm still not convinced that this is actually necessary, we might
be able to add hooks into the existing N_HCI implementation for the
extensions in cg2900.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Friday 29 October 2010, Par-Gunnar Hjalmdahl wrote:
> I might have been a bit too quick there.
> The actual channel matching and packet creation is done in hci_h4.c
> while ldisc registration is done in hci_ldisc.c.
> So it might to be enough to create a new hci_h4-cg2900.c (or similar
> name) that can separate the right channels.

That sounds good, but would that still be h4?

There are currently six UART protocols that have drivers in Linux:
H4, bcsp, 3Weire, h4ds, ll and ath3k.

Can cg2900 be simply another one of those, or is it different
from the others?

> We must however do changes
> to hci_ldisc as well since it seems to always register to the
> Bluetooth stack here, which we definitely don't want since that is
> handled by btcg2900.c.

Can you elaborate? You said earlier that cg2900 is a standard HCI
with some extensions. If that's the case, why do you need your own
btcg2900 driver to handle bluetooth instead of the regular hci code?

> Also note that this ldisc issue is only valid when using UART as
> transport. We will also support SPI and then we will probably run into
> completely new, interesting problems. :-)

Is the link layer on SPI different from the UART variant? Maybe you cna
just add a SPI TTY driver if that doesn't exist yet and bind the same
ldisc to the SPI device.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Alan Cox]

> It's about the ldisc number. Both bluetooth and cg2900 register themselves
> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.

Ah I see - I had assumed any actual final merge would be assigning it a
new LDISC code as is required.

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

2010/10/31, Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>
> Ah I see - I had assumed any actual final merge would be assigning it a
> new LDISC code as is required.
>

Hi,

Sorry for not answering quicker. We in my department have been
discussing new design as well as trying out some solutions.

As an answer to previous comments, both from Arnd and Alan, we would
like to do the following:
 - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
accordingly that will be placed under drivers/char
 - Keep CG2900 driver as an MFD. We will however register the MFD
cells in each driver and remove the function API. The functions
(write, reset, etc) will instead be placed in each MFD cell using
function pointers. This way we can use different functions for
different channels as needed. By placing the MFD cell registration in
each chip driver we will also only expose the channels that are
actually supported by each chip. This way we can also remove the
pretty ugly matching between the devices and respective H4 channel as
well as the add_h4_user function and the similar other functions.

We were thinking about if we could re-use the existing hci_ldisc
driver. As stated before the big problem here is however that
hci_ldisc directly registers to the Bluetooth stack. We could separate
the data for FM and GPS in the protocol driver, but it is pretty ugly
to have two separate data paths within the same driver.
As I've state earlier this would also not work with other transports
such as SPI.I appreciate Arnd's suggestion to create a TTY for SPI,
but I think that would overcomplicate the situation and create a
solution more complex than actually needed. We think that creating a
new ldisc creates a cleaner solution. It will to some extent remind of
hci_ldisc, but it will not register to any framework except tty.

We've thought about if we should switch cg2900 to a bus, but after
discussing this we feel that CG2900 has such individual
characteristics that it could be hard to create a bus that would suite
it. We therefore will keep the MFD type. We might in the future add
new chips to the driver, but they will most likely not require so much
rework of the driver that we will have to create something completely
new. We will rather create a lib file that contains common code to
reduce total code size of the driver.

Do you see any problems with our suggestions?

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Alan Cox]

>  - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
> accordingly that will be placed under drivers/char

That seems sensible. That code should also be entirely hardware
independent.

> solution more complex than actually needed. We think that creating a
> new ldisc creates a cleaner solution. It will to some extent remind of
> hci_ldisc, but it will not register to any framework except tty.

Ok. So presumably both the SPI and the tty interfaces would then use the
same library code to do the demux - or are the protocols different ?
 
> We've thought about if we should switch cg2900 to a bus, but after
> discussing this we feel that CG2900 has such individual
> characteristics that it could be hard to create a bus that would suite
> it. We therefore will keep the MFD type. We might in the future add
> new chips to the driver, but they will most likely not require so much
> rework of the driver that we will have to create something completely
> new. We will rather create a lib file that contains common code to
> reduce total code size of the driver.

No particular opinion either way. Whatever works best and keeps it
modular.

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
> 2010/10/31, Alan Cox <alan@lxorguk.ukuu.org.uk>:
> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
> >
> > Ah I see - I had assumed any actual final merge would be assigning it a
> > new LDISC code as is required.
> 
> Sorry for not answering quicker. We in my department have been
> discussing new design as well as trying out some solutions.
> 
> As an answer to previous comments, both from Arnd and Alan, we would
> like to do the following:
>  - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
> accordingly that will be placed under drivers/char

I'm not convinced, although that's what Alan was suggesting. I'd like
to hear from the bluetooth people what they think about this.

Could you summarize why you think that cg2900 is different enough from
an HCI to require a different line discipline? From your previous
explanation it sounded like it was mostly added functionality,
not something entirely different.

>  - Keep CG2900 driver as an MFD. We will however register the MFD
> cells in each driver and remove the function API. The functions
> (write, reset, etc) will instead be placed in each MFD cell using
> function pointers. This way we can use different functions for
> different channels as needed. By placing the MFD cell registration in
> each chip driver we will also only expose the channels that are
> actually supported by each chip. This way we can also remove the
> pretty ugly matching between the devices and respective H4 channel as
> well as the add_h4_user function and the similar other functions.

Ok, excellent.

> We were thinking about if we could re-use the existing hci_ldisc
> driver. As stated before the big problem here is however that
> hci_ldisc directly registers to the Bluetooth stack. We could separate
> the data for FM and GPS in the protocol driver, but it is pretty ugly
> to have two separate data paths within the same driver.

One of us must be misreading the code. As far as I can tell, hci_ldisc
does not register to the bluetooth stack at all, it just provides
the basic multiplex for multiple HCI protocols, while hci_h4.c
communicates to the bluetooth stack.

If I read it correctly, that means that you can still use hci_ldisc,
but need to add another protocol next to hci_h4 and hci_bcsp for
your cg2900.

> As I've state earlier this would also not work with other transports
> such as SPI.I appreciate Arnd's suggestion to create a TTY for SPI,
> but I think that would overcomplicate the situation and create a
> solution more complex than actually needed. We think that creating a
> new ldisc creates a cleaner solution. It will to some extent remind of
> hci_ldisc, but it will not register to any framework except tty.

How would registering ony to tty solve the problem of SPI?

If you just add another hci_cg2900 module, it can register to both
the hci_ldisc and the spi code.

> We've thought about if we should switch cg2900 to a bus, but after
> discussing this we feel that CG2900 has such individual
> characteristics that it could be hard to create a bus that would suite
> it. We therefore will keep the MFD type. We might in the future add
> new chips to the driver, but they will most likely not require so much
> rework of the driver that we will have to create something completely
> new. We will rather create a lib file that contains common code to
> reduce total code size of the driver.

Yes, makes sense.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

2010/11/8 Arnd Bergmann <arnd@arndb.de>:
> On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
>> 2010/10/31, Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>> >
>> > Ah I see - I had assumed any actual final merge would be assigning it a
>> > new LDISC code as is required.
>>
>> Sorry for not answering quicker. We in my department have been
>> discussing new design as well as trying out some solutions.
>>
>> As an answer to previous comments, both from Arnd and Alan, we would
>> like to do the following:
>>  - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
>> accordingly that will be placed under drivers/char
>
> I'm not convinced, although that's what Alan was suggesting. I'd like
> to hear from the bluetooth people what they think about this.
>
> Could you summarize why you think that cg2900 is different enough from
> an HCI to require a different line discipline? From your previous
> explanation it sounded like it was mostly added functionality,
> not something entirely different.
>
>> We were thinking about if we could re-use the existing hci_ldisc
>> driver. As stated before the big problem here is however that
>> hci_ldisc directly registers to the Bluetooth stack. We could separate
>> the data for FM and GPS in the protocol driver, but it is pretty ugly
>> to have two separate data paths within the same driver.
>
> One of us must be misreading the code. As far as I can tell, hci_ldisc
> does not register to the bluetooth stack at all, it just provides
> the basic multiplex for multiple HCI protocols, while hci_h4.c
> communicates to the bluetooth stack.
>
> If I read it correctly, that means that you can still use hci_ldisc,
> but need to add another protocol next to hci_h4 and hci_bcsp for
> your cg2900.
>

If you look in function hci_ldisc.c/hci_uart_register_dev(), it here
registers as a driver to the Bluetooth stack. This means that received
Bluetooth packets would go ldisc->protocol->ldisc->bluetooth, while FM
and GPS would go ldisc->protocol->(FM/GPS)stack. I think it's quite
bad to have two different data paths like this. The new ldisc we're
creating looks a lot like hci_ldisc, but it does not register itself
to an overlaying stack directly.
One option would of course be to modify the existing hci_ldisc.c but I
feel that be rather dangerous and which could create a lot of
problems.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

2010/11/11 Par-Gunnar Hjalmdahl <pghatwork@gmail.com>:
> 2010/11/8 Arnd Bergmann <arnd@arndb.de>:
>> On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
>>> 2010/10/31, Alan Cox <alan@lxorguk.ukuu.org.uk>:
>>> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>>> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>>> >
>>> > Ah I see - I had assumed any actual final merge would be assigning it a
>>> > new LDISC code as is required.
>>>
>>> Sorry for not answering quicker. We in my department have been
>>> discussing new design as well as trying out some solutions.
>>>
>>> As an answer to previous comments, both from Arnd and Alan, we would
>>> like to do the following:
>>>  - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
>>> accordingly that will be placed under drivers/char
>>
>> I'm not convinced, although that's what Alan was suggesting. I'd like
>> to hear from the bluetooth people what they think about this.
>>
>> Could you summarize why you think that cg2900 is different enough from
>> an HCI to require a different line discipline? From your previous
>> explanation it sounded like it was mostly added functionality,
>> not something entirely different.
>>
>>> We were thinking about if we could re-use the existing hci_ldisc
>>> driver. As stated before the big problem here is however that
>>> hci_ldisc directly registers to the Bluetooth stack. We could separate
>>> the data for FM and GPS in the protocol driver, but it is pretty ugly
>>> to have two separate data paths within the same driver.
>>
>> One of us must be misreading the code. As far as I can tell, hci_ldisc
>> does not register to the bluetooth stack at all, it just provides
>> the basic multiplex for multiple HCI protocols, while hci_h4.c
>> communicates to the bluetooth stack.
>>
>> If I read it correctly, that means that you can still use hci_ldisc,
>> but need to add another protocol next to hci_h4 and hci_bcsp for
>> your cg2900.
>>
>
> If you look in function hci_ldisc.c/hci_uart_register_dev(), it here
> registers as a driver to the Bluetooth stack. This means that received
> Bluetooth packets would go ldisc->protocol->ldisc->bluetooth, while FM
> and GPS would go ldisc->protocol->(FM/GPS)stack. I think it's quite
> bad to have two different data paths like this. The new ldisc we're
> creating looks a lot like hci_ldisc, but it does not register itself
> to an overlaying stack directly.
> One option would of course be to modify the existing hci_ldisc.c but I
> feel that be rather dangerous and which could create a lot of
> problems.
>
> /P-G
>

After looking again at the code I see that I was wrong.
For the receiving path the data will go ldics->protocol->stack. It's
actually the TX path (to the chip) that is a bit strange where
Bluetooth data is going to stack->ldisc->protocol->ldisc->uart. Here
we would have a separate path for FM and GPS.
I still don't like the idea of making a tty/ldisc for the SPI
transport. I definitely would prefer instead a new ldisc which doesn't
register to any stack on top. My preference would be to keep the
solutions independent, where we use tty/ldisc for UART and a direct
transport protocol driver for SPI (i.e. registering using
spi_register_driver).

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Thursday 11 November 2010, Par-Gunnar Hjalmdahl wrote:
> > One option would of course be to modify the existing hci_ldisc.c but I
> > feel that be rather dangerous and which could create a lot of
> > problems.
>
> After looking again at the code I see that I was wrong.
> For the receiving path the data will go ldics->protocol->stack. It's
> actually the TX path (to the chip) that is a bit strange where
> Bluetooth data is going to stack->ldisc->protocol->ldisc->uart. Here
> we would have a separate path for FM and GPS.

Ok, but is that a problem? You really should not be afraid of
touching existing code in order to make it more generic or removing
strange code paths like the one you just described. Cleaning up
code is usually better than duplicating it when you notice something
wrong with the existing implementation.

Simply calling the underlying ldisc module from the FM and GPS modules
should not even require changes, right?

> I still don't like the idea of making a tty/ldisc for the SPI
> transport. I definitely would prefer instead a new ldisc which doesn't
> register to any stack on top.

Yes, agreed. You had already convinced me in the previous reply, sorry
if that wasn't clear.

> My preference would be to keep the
> solutions independent, where we use tty/ldisc for UART and a direct
> transport protocol driver for SPI (i.e. registering using
> spi_register_driver).

Yes. However, you can probably share substantial parts of the code
between the spi_driver and the hci_uart_proto implementations.
You can do this either by making them a single module that registers
to both hci_ldisc and spi_bus, or having a common cg2900 base module
that does the common parts and add separate modules for spi and
hci_uart that register a small wrapper to the respective subsystems.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

2010/10/29 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> The reason is that the work is generated so often that a work is not
>> finished before next work of same type comes. This is especially true
>> for transmit and receive. Then I get 0 back when queuing the work and
>> there is no real way to solve it from what I can see than to allocate
>> new work structures every time.
>
> So if that is the case what bounds your memory usage - can a busy box end
> up with thousands of work queue slos used ? It sounds like your model is
> perhaps wrong - if there is a continual stream of work maybe you should
> simply have a kernel thread to handle it if it cannot be deferred
> - remember ldisc code is able to sleep in most paths.
>
>

Hi Alan,

I went through my old mails here and realized I hadn't answered you
for this question.
Basically most of the time we are able to handle the works in time for
the next work to be created. But there are occasions where next work
is created really quickly and we end up with an error situation when
starting the work. But if we allocate the work it will then be handled
when the first one is ready. It's not like we will never handle it.
The data transfers can be received in a bit bursty way and then there
are no interrupts for a while. So in the end all works are handled
rather quickly and queue will never build up. We have tested with
several large file transfers and data pumps without issues.

By the way, I will soon release a new patch set for CG2900 (hopefully
next week), which contains major rework. It's hard to explain
everything here and now but changes include:
 - Reuse of existing HCI line discipline under /drivers/bluetooth/.
Line discipline has been modified so it is selectable from protocol if
line discipline should register to Bluetooth stack or not.
 - Architecture modification: core, chip, and transport is new created
from platform specific files (/arch/). Each chip file then spawns MFD
devices for the channels it support.
 - Core file now only handles detection of connected chip. All chip
dependent code is moved to transport and chip files.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

Hi Par,

On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
<pghatwork@gmail.com> wrote:
> 2010/10/29 Alan Cox <alan@lxorguk.ukuu.org.uk>:

> By the way, I will soon release a new patch set for CG2900 (hopefully
> next week), which contains major rework. It's hard to explain
> everything here and now but changes include:
>  - Reuse of existing HCI line discipline under /drivers/bluetooth/.
> Line discipline has been modified so it is selectable from protocol if
> line discipline should register to Bluetooth stack or not.

So is it true that if there's the other chip with similar
functionality I have to implement support for, I'll have to pretty
much duplicate your line discipline driver?

Isn't it better to have a new line discipline that the standard
drivers (H4, LL, ...) will be applicable to?

~Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

Hi Par,

On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
<pghatwork@gmail.com> wrote:
> 2010/10/29 Alan Cox <alan@lxorguk.ukuu.org.uk>:

> By the way, I will soon release a new patch set for CG2900 (hopefully
> next week), which contains major rework. It's hard to explain
> everything here and now but changes include:
> =A0- Reuse of existing HCI line discipline under /drivers/bluetooth/.
> Line discipline has been modified so it is selectable from protocol if
> line discipline should register to Bluetooth stack or not.

So is it true that if there's the other chip with similar
functionality I have to implement support for, I'll have to pretty
much duplicate your line discipline driver?

Isn't it better to have a new line discipline that the standard
drivers (H4, LL, ...) will be applicable to?

~Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Hi Vitaly,

2010/12/3 Vitaly Wool <vitalywool@gmail.com>:
> Hi Par,
>
> On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
> <pghatwork@gmail.com> wrote:
>> 2010/10/29 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>
>> By the way, I will soon release a new patch set for CG2900 (hopefully
>> next week), which contains major rework. It's hard to explain
>> everything here and now but changes include:
>>  - Reuse of existing HCI line discipline under /drivers/bluetooth/.
>> Line discipline has been modified so it is selectable from protocol if
>> line discipline should register to Bluetooth stack or not.
>
> So is it true that if there's the other chip with similar
> functionality I have to implement support for, I'll have to pretty
> much duplicate your line discipline driver?
>

What will be in the protocol file, i.e. the same level as hci_h4.c,
hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
that are in a standardized specification are the Bluetooth channels
1-4. Other channels such as FM and GPS are vendor specific. I've seen
that for example TI has the same channels for FM and GPS, but these
channels, 8 and 9, are in no way standardized. The CG2900 also carries
a number of other channels, such as debug and device channels, which
I'm pretty sure is individual for this chip.
This identification of the channels is also only one function, even
though it is an important function. There are also a lot of other code
regarding baud rate settings, low power handling, etc that I'm
positive will not apply to chips of other vendors. It is of course
possible that other chips from ST-Ericsson, current and future, will
be able to reuse this file, but as I said I doubt that other vendors
might be able to use it.

> Isn't it better to have a new line discipline that the standard
> drivers (H4, LL, ...) will be applicable to?
>
> ~Vitaly
>

I'm not certain what you mean. We are modifying the line discipline a
bit, but the only thing we have needed to do with existing protocol
drivers has been to add a boolean parameter for the protocol settings
(so they will register to the Bluetooth stack). I don't see why we
should create a new line discipline driver and then move the existing
protocol drivers to this.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Hi Vitaly,

2010/12/3 Vitaly Wool <vitalywool@gmail.com>:
> Hi Par,
>
> On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
> <pghatwork@gmail.com> wrote:
>> 2010/10/29 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>
>> By the way, I will soon release a new patch set for CG2900 (hopefully
>> next week), which contains major rework. It's hard to explain
>> everything here and now but changes include:
>> =A0- Reuse of existing HCI line discipline under /drivers/bluetooth/.
>> Line discipline has been modified so it is selectable from protocol if
>> line discipline should register to Bluetooth stack or not.
>
> So is it true that if there's the other chip with similar
> functionality I have to implement support for, I'll have to pretty
> much duplicate your line discipline driver?
>

What will be in the protocol file, i.e. the same level as hci_h4.c,
hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
that are in a standardized specification are the Bluetooth channels
1-4. Other channels such as FM and GPS are vendor specific. I've seen
that for example TI has the same channels for FM and GPS, but these
channels, 8 and 9, are in no way standardized. The CG2900 also carries
a number of other channels, such as debug and device channels, which
I'm pretty sure is individual for this chip.
This identification of the channels is also only one function, even
though it is an important function. There are also a lot of other code
regarding baud rate settings, low power handling, etc that I'm
positive will not apply to chips of other vendors. It is of course
possible that other chips from ST-Ericsson, current and future, will
be able to reuse this file, but as I said I doubt that other vendors
might be able to use it.

> Isn't it better to have a new line discipline that the standard
> drivers (H4, LL, ...) will be applicable to?
>
> ~Vitaly
>

I'm not certain what you mean. We are modifying the line discipline a
bit, but the only thing we have needed to do with existing protocol
drivers has been to add a boolean parameter for the protocol settings
(so they will register to the Bluetooth stack). I don't see why we
should create a new line discipline driver and then move the existing
protocol drivers to this.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

Hi Par,

On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
<pghatwork@gmail.com> wrote:

>> So is it true that if there's the other chip with similar
>> functionality I have to implement support for, I'll have to pretty
>> much duplicate your line discipline driver?
>>
>
> What will be in the protocol file, i.e. the same level as hci_h4.c,
> hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
> that are in a standardized specification are the Bluetooth channels
> 1-4. Other channels such as FM and GPS are vendor specific. I've seen
> that for example TI has the same channels for FM and GPS, but these
> channels, 8 and 9, are in no way standardized. The CG2900 also carries
> a number of other channels, such as debug and device channels, which
> I'm pretty sure is individual for this chip.
> This identification of the channels is also only one function, even
> though it is an important function. There are also a lot of other code
> regarding baud rate settings, low power handling, etc that I'm
> positive will not apply to chips of other vendors. It is of course
> possible that other chips from ST-Ericsson, current and future, will
> be able to reuse this file, but as I said I doubt that other vendors
> might be able to use it.

Wait. What level of abstraction is this file at if even some future
STE chips are not guaranteed to work with it? Is it at a level of
hardcoding the GPIO numbers?

>> Isn't it better to have a new line discipline that the standard
>> drivers (H4, LL, ...) will be applicable to?

> I'm not certain what you mean. We are modifying the line discipline a
> bit, but the only thing we have needed to do with existing protocol
> drivers has been to add a boolean parameter for the protocol settings
> (so they will register to the Bluetooth stack). I don't see why we
> should create a new line discipline driver and then move the existing
> protocol drivers to this.

Okay, let me try to be more specific. There's for instance
drivers/bluetooth/hci_ll.c which is the line discipline driver that is
meant to work with any BT chip supporting LL protocol. Your solution
seems to imply that one will have to create a variation of this
implementation for each BT chip supporting LL that will use your
shared transport implementation. Is it really the case?

Thanks,
   Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Hi Vitaly,

2010/12/6 Vitaly Wool <vitalywool@gmail.com>:
> Hi Par,
>
> On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
> <pghatwork@gmail.com> wrote:
>
>>> So is it true that if there's the other chip with similar
>>> functionality I have to implement support for, I'll have to pretty
>>> much duplicate your line discipline driver?
>>>
>>
>> What will be in the protocol file, i.e. the same level as hci_h4.c,
>> hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
>> that are in a standardized specification are the Bluetooth channels
>> 1-4. Other channels such as FM and GPS are vendor specific. I've seen
>> that for example TI has the same channels for FM and GPS, but these
>> channels, 8 and 9, are in no way standardized. The CG2900 also carries
>> a number of other channels, such as debug and device channels, which
>> I'm pretty sure is individual for this chip.
>> This identification of the channels is also only one function, even
>> though it is an important function. There are also a lot of other code
>> regarding baud rate settings, low power handling, etc that I'm
>> positive will not apply to chips of other vendors. It is of course
>> possible that other chips from ST-Ericsson, current and future, will
>> be able to reuse this file, but as I said I doubt that other vendors
>> might be able to use it.
>
> Wait. What level of abstraction is this file at if even some future
> STE chips are not guaranteed to work with it? Is it at a level of
> hardcoding the GPIO numbers?
>

There are no GPIOs or similar in the driver code; those are defined
and used in the board specific files under the /arch/ files. What
could be a problem with a future chip would be if for example the HCI
command to change baud rate would change. We have no indication that
this will change, but I cannot give a lifetime guarantee that nothing
will ever change with how the chip startup is handled.

>>> Isn't it better to have a new line discipline that the standard
>>> drivers (H4, LL, ...) will be applicable to?
>
>> I'm not certain what you mean. We are modifying the line discipline a
>> bit, but the only thing we have needed to do with existing protocol
>> drivers has been to add a boolean parameter for the protocol settings
>> (so they will register to the Bluetooth stack). I don't see why we
>> should create a new line discipline driver and then move the existing
>> protocol drivers to this.
>
> Okay, let me try to be more specific. There's for instance
> drivers/bluetooth/hci_ll.c which is the line discipline driver that is
> meant to work with any BT chip supporting LL protocol. Your solution
> seems to imply that one will have to create a variation of this
> implementation for each BT chip supporting LL that will use your
> shared transport implementation. Is it really the case?
>
> Thanks,
>   Vitaly
>

Well, from what I can see LL is an extension of the H4, basically it
adds sleep mode handling in a vendor specific way to the normal H4
protocol. So it is also based on the hci_h4 just as our file is. But
our file has basically nothing in common with what has been for the LL
file. We don't support any of the LL sleep commands for example.
So if I would make a driver for a combo chip supporting LL, I would
either modify the existing hci_ll.c or I would make a new file based
on hci_ll.c. There is not much you could really reuse from our new
file. Basically it would be the handling of any common channels, so if
you would for example have the same specification of FM and GPS you
could maybe save around 20 lines of common code, but you would
probably have to add a lot of more code just to keep the solution
generic.
One major difference is also that hci_ll never changes baud rate or
other settings. I assume that is done from hciattach during startup
instead. But we cannot run with that since we have to shut down the
chip when no user is connected in order to save power. This means that
we have to add vendor specific commands in order to for example set
baud rate. And then you run into these vendor specific problems. If
there would be a standardized specification on how to set baud rate
and how to put chip in sleep I assume things could be solved
differently, but that is not the case.

As a quick answer to your question: that would depend on the
difference between the different controllers, I guess. But CG2900
doesn't support the LL protocol so it is not an issue for that.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Hi Vitaly,

2010/12/6 Vitaly Wool <vitalywool@gmail.com>:
> Hi Par,
>
> On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
> <pghatwork@gmail.com> wrote:
>
>>> So is it true that if there's the other chip with similar
>>> functionality I have to implement support for, I'll have to pretty
>>> much duplicate your line discipline driver?
>>>
>>
>> What will be in the protocol file, i.e. the same level as hci_h4.c,
>> hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
>> that are in a standardized specification are the Bluetooth channels
>> 1-4. Other channels such as FM and GPS are vendor specific. I've seen
>> that for example TI has the same channels for FM and GPS, but these
>> channels, 8 and 9, are in no way standardized. The CG2900 also carries
>> a number of other channels, such as debug and device channels, which
>> I'm pretty sure is individual for this chip.
>> This identification of the channels is also only one function, even
>> though it is an important function. There are also a lot of other code
>> regarding baud rate settings, low power handling, etc that I'm
>> positive will not apply to chips of other vendors. It is of course
>> possible that other chips from ST-Ericsson, current and future, will
>> be able to reuse this file, but as I said I doubt that other vendors
>> might be able to use it.
>
> Wait. What level of abstraction is this file at if even some future
> STE chips are not guaranteed to work with it? Is it at a level of
> hardcoding the GPIO numbers?
>

There are no GPIOs or similar in the driver code; those are defined
and used in the board specific files under the /arch/ files. What
could be a problem with a future chip would be if for example the HCI
command to change baud rate would change. We have no indication that
this will change, but I cannot give a lifetime guarantee that nothing
will ever change with how the chip startup is handled.

>>> Isn't it better to have a new line discipline that the standard
>>> drivers (H4, LL, ...) will be applicable to?
>
>> I'm not certain what you mean. We are modifying the line discipline a
>> bit, but the only thing we have needed to do with existing protocol
>> drivers has been to add a boolean parameter for the protocol settings
>> (so they will register to the Bluetooth stack). I don't see why we
>> should create a new line discipline driver and then move the existing
>> protocol drivers to this.
>
> Okay, let me try to be more specific. There's for instance
> drivers/bluetooth/hci_ll.c which is the line discipline driver that is
> meant to work with any BT chip supporting LL protocol. Your solution
> seems to imply that one will have to create a variation of this
> implementation for each BT chip supporting LL that will use your
> shared transport implementation. Is it really the case?
>
> Thanks,
> =A0 Vitaly
>

Well, from what I can see LL is an extension of the H4, basically it
adds sleep mode handling in a vendor specific way to the normal H4
protocol. So it is also based on the hci_h4 just as our file is. But
our file has basically nothing in common with what has been for the LL
file. We don't support any of the LL sleep commands for example.
So if I would make a driver for a combo chip supporting LL, I would
either modify the existing hci_ll.c or I would make a new file based
on hci_ll.c. There is not much you could really reuse from our new
file. Basically it would be the handling of any common channels, so if
you would for example have the same specification of FM and GPS you
could maybe save around 20 lines of common code, but you would
probably have to add a lot of more code just to keep the solution
generic.
One major difference is also that hci_ll never changes baud rate or
other settings. I assume that is done from hciattach during startup
instead. But we cannot run with that since we have to shut down the
chip when no user is connected in order to save power. This means that
we have to add vendor specific commands in order to for example set
baud rate. And then you run into these vendor specific problems. If
there would be a standardized specification on how to set baud rate
and how to put chip in sleep I assume things could be solved
differently, but that is not the case.

As a quick answer to your question: that would depend on the
difference between the different controllers, I guess. But CG2900
doesn't support the LL protocol so it is not an issue for that.

/P-G

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

Par,

> Well, from what I can see LL is an extension of the H4, basically it
> adds sleep mode handling in a vendor specific way to the normal H4
> protocol. So it is also based on the hci_h4 just as our file is. But
> our file has basically nothing in common with what has been for the LL
> file. We don't support any of the LL sleep commands for example.
> So if I would make a driver for a combo chip supporting LL, I would
> either modify the existing hci_ll.c or I would make a new file based
> on hci_ll.c. There is not much you could really reuse from our new
> file. Basically it would be the handling of any common channels, so if
> you would for example have the same specification of FM and GPS you
> could maybe save around 20 lines of common code, but you would
> probably have to add a lot of more code just to keep the solution
> generic.

Right, but this gives me the hard time seeing how your implementation
is applicable to other multi-functional chips with similar
functionality.

> One major difference is also that hci_ll never changes baud rate or
> other settings. I assume that is done from hciattach during startup
> instead. But we cannot run with that since we have to shut down the
> chip when no user is connected in order to save power. This means that
> we have to add vendor specific commands in order to for example set
> baud rate. And then you run into these vendor specific problems. If
> there would be a standardized specification on how to set baud rate
> and how to put chip in sleep I assume things could be solved
> differently, but that is not the case.

Again, there are at least TI and Broadcom chips that support HCI_LL
and if they were to use your implementation of the core, they would
have had to add 2 more implementations of the corresponding line
discipline driver.

> As a quick answer to your question: that would depend on the
> difference between the different controllers, I guess. But CG2900
> doesn't support the LL protocol so it is not an issue for that.

Right, but if you are only aiming cg2000, why would you create a
framework for that?  I initially thought your solution was generic
enough to handle other "many-in-one" Bluetooth chips but I'm
completely unsure about that now.

Thanks,
   Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Monday 06 December 2010, Vitaly Wool wrote:

> On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
> <pghatwork@gmail.com> wrote:

> >> Isn't it better to have a new line discipline that the standard
> >> drivers (H4, LL, ...) will be applicable to?
> 
> > I'm not certain what you mean. We are modifying the line discipline a
> > bit, but the only thing we have needed to do with existing protocol
> > drivers has been to add a boolean parameter for the protocol settings
> > (so they will register to the Bluetooth stack). I don't see why we
> > should create a new line discipline driver and then move the existing
> > protocol drivers to this.
> 
> Okay, let me try to be more specific. There's for instance
> drivers/bluetooth/hci_ll.c which is the line discipline driver that is
> meant to work with any BT chip supporting LL protocol. Your solution
> seems to imply that one will have to create a variation of this
> implementation for each BT chip supporting LL that will use your
> shared transport implementation. Is it really the case?

hci_ll is not the line discipline but the a TI specific uart protocol.
The line discipline driver (hci_ldisc.c) is shared across all protocols
(h4, ll, ...) and gets extended slightly so it can deal with cg2900
in addition to the existing HCIs. The rest of the cg2900 support is
about adding more hci_uart_protos.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Monday 06 December 2010, Vitaly Wool wrote:
> > As a quick answer to your question: that would depend on the
> > difference between the different controllers, I guess. But CG2900
> > doesn't support the LL protocol so it is not an issue for that.
> 
> Right, but if you are only aiming cg2000, why would you create a
> framework for that?  I initially thought your solution was generic
> enough to handle other "many-in-one" Bluetooth chips but I'm
> completely unsure about that now.

As far as I understand, the point is that it's no longer a 'solution'
at the core, i.e. there is no replacement for hci_ldisc or any
of these, just modules for the additional h4 protocols that don't
have a linux implementation yet.

The patch set that was originally posted here had a new framework,
but after the comments from Alan and me, Par-Gunnar agreed to use
the existing framework instead and extend it in a useful way.
Please read all of the discussion we already had. You made a good
point here, but I fear you had both Par-Gunnar and me confused
because it was a point that already got resolved.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

Hi Arnd,

On Mon, Dec 6, 2010 at 3:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> hci_ll is not the line discipline but the a TI specific uart protocol.
> The line discipline driver (hci_ldisc.c) is shared across all protocols
> (h4, ll, ...) and gets extended slightly so it can deal with cg2900
> in addition to the existing HCIs. The rest of the cg2900 support is
> about adding more hci_uart_protos.

I was referring to hci_ll as to the line discipline driver, not the
line discipline itself.

The thing is, there are different Bluetooth combo chips which use HCI
to encapsulate commands to the sub-chips behind, and there's also the
implementation for TI chip doing that which is in the mainline. So
instead of keeping reinventing the wheel, I think it's fairly
beneficial for everything if we finally agree on something that works
for all the combo chips of the type.

Thanks,
   Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

On Mon, Dec 6, 2010 at 3:49 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> As far as I understand, the point is that it's no longer a 'solution'
> at the core, i.e. there is no replacement for hci_ldisc or any
> of these, just modules for the additional h4 protocols that don't
> have a linux implementation yet.
>
> The patch set that was originally posted here had a new framework,
> but after the comments from Alan and me, Par-Gunnar agreed to use
> the existing framework instead and extend it in a useful way.
> Please read all of the discussion we already had. You made a good
> point here, but I fear you had both Par-Gunnar and me confused
> because it was a point that already got resolved.

Yeah I've read through that thread but the only conclusion I'd been
able to make was that you agreed on not introducing yet another line
discipline. I'm not sure if I could make a conclusion that Par gave up
on his attempt to make his patchset generic/extensible/reusable for
other chips of similar type. But if that's really the case, I see no
point in this patchset at all, as opposed to generalizing the TI
shared transport thingie and using that one.

Thanks,
   Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Monday 06 December 2010, Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 3:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > hci_ll is not the line discipline but the a TI specific uart protocol.
> > The line discipline driver (hci_ldisc.c) is shared across all protocols
> > (h4, ll, ...) and gets extended slightly so it can deal with cg2900
> > in addition to the existing HCIs. The rest of the cg2900 support is
> > about adding more hci_uart_protos.
> 
> I was referring to hci_ll as to the line discipline driver, not the
> line discipline itself.
> 
> The thing is, there are different Bluetooth combo chips which use HCI
> to encapsulate commands to the sub-chips behind, and there's also the
> implementation for TI chip doing that which is in the mainline. So
> instead of keeping reinventing the wheel, I think it's fairly
> beneficial for everything if we finally agree on something that works
> for all the combo chips of the type.

Yes, that makes sense. I'm probably missing something here, but
it seems to me that hci_ll is only about the power management stuff
on TI (and broadcom, as you say) chips, and the multi-protocol
support is currently handled in hci_ldisc by allowing multiple
protocols like h4 and ll to be registered. It that correct?

One aspect that Par-Gunnar mentioned was that the multi-protocol
stuff for cg2900 and I suspect other similar devices would also
need to work with non-UART-based HCIs, which don't use hci_uart_proto
but would need something similar. Also, hci_uart is currently not
modular, e.g. you cannot build hci_ll as a loadable module
as you'd need for dynamic registration.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

On Mon, Dec 6, 2010 at 4:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Yes, that makes sense. I'm probably missing something here, but
> it seems to me that hci_ll is only about the power management stuff
> on TI (and broadcom, as you say) chips, and the multi-protocol
> support is currently handled in hci_ldisc by allowing multiple
> protocols like h4 and ll to be registered. It that correct?

Yeah, it's basic for TI and it's supported by Broadcom although they
have their own protocol for power saving.

But I was trying to make a different point here. On a basic level,
there's this cg2000 chip from STE that does BT, FM and GPS. There's
the chip from TI that does BT, FM and GPS, and there's the Broadcom
chip that does BT+FM. They all use HCI to access the other functions
of the combo chip and they do it in a really simiar way, with the
differences mostly in power management techniques. So I think it's
quite sensible to have some kind of framework that is suitable for
such devices.

> One aspect that Par-Gunnar mentioned was that the multi-protocol
> stuff for cg2900 and I suspect other similar devices would also
> need to work with non-UART-based HCIs, which don't use hci_uart_proto
> but would need something similar. Also, hci_uart is currently not
> modular, e.g. you cannot build hci_ll as a loadable module
> as you'd need for dynamic registration.

But generally speaking, isn't a line discipline/driver attached to a
tty? We can use dumb tty for e. g. SPI and still be able to use
hci_ll, right?

Thanks,
   Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Monday 06 December 2010, Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 4:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > Yes, that makes sense. I'm probably missing something here, but
> > it seems to me that hci_ll is only about the power management stuff
> > on TI (and broadcom, as you say) chips, and the multi-protocol
> > support is currently handled in hci_ldisc by allowing multiple
> > protocols like h4 and ll to be registered. It that correct?
> 
> Yeah, it's basic for TI and it's supported by Broadcom although they
> have their own protocol for power saving.
> 
> But I was trying to make a different point here. On a basic level,
> there's this cg2000 chip from STE that does BT, FM and GPS. There's
> the chip from TI that does BT, FM and GPS, and there's the Broadcom
> chip that does BT+FM. They all use HCI to access the other functions
> of the combo chip and they do it in a really simiar way, with the
> differences mostly in power management techniques. So I think it's
> quite sensible to have some kind of framework that is suitable for
> such devices.

Yes, I agree 100% in principle. I could not find the code that 
Broadcom/TI FM and GPS stuff so far, can you point us to that?

The cg2900 solution for this was to use MFD (plus another layer
in the posted version, but that will go away I assume). Using
MFD is not the only possibility here, but I could not see anything
wrong with it either. Do you think we can move them all over to
use MFD infrastructure?

> > One aspect that Par-Gunnar mentioned was that the multi-protocol
> > stuff for cg2900 and I suspect other similar devices would also
> > need to work with non-UART-based HCIs, which don't use hci_uart_proto
> > but would need something similar. Also, hci_uart is currently not
> > modular, e.g. you cannot build hci_ll as a loadable module
> > as you'd need for dynamic registration.
> 
> But generally speaking, isn't a line discipline/driver attached to a
> tty? We can use dumb tty for e. g. SPI and still be able to use
> hci_ll, right?

I suggested that as well, but the point was made that this would
add an unnecessary indirection for the SPI case, which is not
really much like a serial port. It's certainly possible to do it
like you say, but if we add a way to register the high-level
protocols with an HCI-like multi-function device, we could
also do it in a way that does not rely on tty-ldisc but keeps it
as one of the options.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Vitaly Wool]

On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:

>> But I was trying to make a different point here. On a basic level,
>> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>> chip that does BT+FM. They all use HCI to access the other functions
>> of the combo chip and they do it in a really simiar way, with the
>> differences mostly in power management techniques. So I think it's
>> quite sensible to have some kind of framework that is suitable for
>> such devices.
>
> Yes, I agree 100% in principle. I could not find the code that
> Broadcom/TI FM and GPS stuff so far, can you point us to that?

Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.

Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
sure about the mainline support for those.

> The cg2900 solution for this was to use MFD (plus another layer
> in the posted version, but that will go away I assume). Using
> MFD is not the only possibility here, but I could not see anything
> wrong with it either. Do you think we can move them all over to
> use MFD infrastructure?

I guess so but it's probably more of a detail than what I'm up to now :)

>> But generally speaking, isn't a line discipline/driver attached to a
>> tty? We can use dumb tty for e. g. SPI and still be able to use
>> hci_ll, right?
>
> I suggested that as well, but the point was made that this would
> add an unnecessary indirection for the SPI case, which is not
> really much like a serial port. It's certainly possible to do it
> like you say, but if we add a way to register the high-level
> protocols with an HCI-like multi-function device, we could
> also do it in a way that does not rely on tty-ldisc but keeps it
> as one of the options.

I actually don't think it's such a big indirection, I prefer to think
of it more as of the abstraction layer. If not use this, are we going
to have direct SPI device drivers? I'm afraid we might end up with a
huge duplication of code in that case.

Thanks,
   Vitaly

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> >> But I was trying to make a different point here. On a basic level,
> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
> >> chip that does BT+FM. They all use HCI to access the other functions
> >> of the combo chip and they do it in a really simiar way, with the
> >> differences mostly in power management techniques. So I think it's
> >> quite sensible to have some kind of framework that is suitable for
> >> such devices.
> >
> > Yes, I agree 100% in principle. I could not find the code that
> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
> 
> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
> 
> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
> sure about the mainline support for those.

Ah, it had not occured to me to look in drivers/misc. Looking at the
code over there, it becomes much clearer what your point is. Evidently
the ti-st driver implements a line discipline similar to, but conflicting
with hci_ldisc, and it has its own copy of the hci_ll code.

I guess this is exactly what we're trying to avoid having with the
cg2900 code ;-).

> > The cg2900 solution for this was to use MFD (plus another layer
> > in the posted version, but that will go away I assume). Using
> > MFD is not the only possibility here, but I could not see anything
> > wrong with it either. Do you think we can move them all over to
> > use MFD infrastructure?
> 
> I guess so but it's probably more of a detail than what I'm up to now :)

Agreed.
 
> >> But generally speaking, isn't a line discipline/driver attached to a
> >> tty? We can use dumb tty for e. g. SPI and still be able to use
> >> hci_ll, right?
> >
> > I suggested that as well, but the point was made that this would
> > add an unnecessary indirection for the SPI case, which is not
> > really much like a serial port. It's certainly possible to do it
> > like you say, but if we add a way to register the high-level
> > protocols with an HCI-like multi-function device, we could
> > also do it in a way that does not rely on tty-ldisc but keeps it
> > as one of the options.
> 
> I actually don't think it's such a big indirection, I prefer to think
> of it more as of the abstraction layer. If not use this, are we going
> to have direct SPI device drivers? I'm afraid we might end up with a
> huge duplication of code in that case.

The question is how it should be modeled in a better way than today.

I believe we already agree it should be something like

   bt   ti-radio    st-e-radio    ti-gps   st-e-gps  broadcom-radio ...
   |         |          |            |          |          |         |
   +---------+----------+---------+--+----------+----------+---------+
                                  |
                          common-hci-module
                                  |
                      +-----------+-----------+
                      |        |       |      |
                    serial    spi     i2c    ...


Any objections to this?

If you agree, I think we should discuss the alternatives for the common
module. I think you are saying we should make the common module a single
ldisc doing the multiplexing and have all transports register as ttys into
it, right?

What we discussed before was to split it into multiple modules, one for
the tty ldisc and one for the common code, and then have the spi/i2c/...
drivers feed into the common multiplexer without going through tty.

I don't currently see a significant advantage or disadvantage with either
approach.

	Arnd

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Pavan Savoy]

On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> >> But I was trying to make a different point here. On a basic level,
>> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>> >> chip that does BT+FM. They all use HCI to access the other functions
>> >> of the combo chip and they do it in a really simiar way, with the
>> >> differences mostly in power management techniques. So I think it's
>> >> quite sensible to have some kind of framework that is suitable for
>> >> such devices.
>> >
>> > Yes, I agree 100% in principle. I could not find the code that
>> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
>>
>> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
>>
>> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
>> sure about the mainline support for those.
>
> Ah, it had not occured to me to look in drivers/misc. Looking at the
> code over there, it becomes much clearer what your point is. Evidently
> the ti-st driver implements a line discipline similar to, but conflicting
> with hci_ldisc, and it has its own copy of the hci_ll code.
>
> I guess this is exactly what we're trying to avoid having with the
> cg2900 code ;-).
>
>> > The cg2900 solution for this was to use MFD (plus another layer
>> > in the posted version, but that will go away I assume). Using
>> > MFD is not the only possibility here, but I could not see anything
>> > wrong with it either. Do you think we can move them all over to
>> > use MFD infrastructure?
>>
>> I guess so but it's probably more of a detail than what I'm up to now :)
>
> Agreed.
>
>> >> But generally speaking, isn't a line discipline/driver attached to a
>> >> tty? We can use dumb tty for e. g. SPI and still be able to use
>> >> hci_ll, right?
>> >
>> > I suggested that as well, but the point was made that this would
>> > add an unnecessary indirection for the SPI case, which is not
>> > really much like a serial port. It's certainly possible to do it
>> > like you say, but if we add a way to register the high-level
>> > protocols with an HCI-like multi-function device, we could
>> > also do it in a way that does not rely on tty-ldisc but keeps it
>> > as one of the options.
>>
>> I actually don't think it's such a big indirection, I prefer to think
>> of it more as of the abstraction layer. If not use this, are we going
>> to have direct SPI device drivers? I'm afraid we might end up with a
>> huge duplication of code in that case.
>
> The question is how it should be modeled in a better way than today.
>
> I believe we already agree it should be something like
>
>   bt   ti-radio    st-e-radio    ti-gps   st-e-gps  broadcom-radio ...
>   |         |          |            |          |          |         |
>   +---------+----------+---------+--+----------+----------+---------+
>                                  |
>                          common-hci-module
>                                  |
>                      +-----------+-----------+
>                      |        |       |      |
>                    serial    spi     i2c    ...

Yes, this sort of architecture would certainly help...
However, I suppose the most common question as one of you stated above
was how can the channel -ID and other factors such as offset-header
packet format be generalized?

As in over here, will the common-hci-module work fine, If say ti-radio
says he is expecting packets starting from id-8 which is of length 10?
and also work for st-e-radio which would say expecting data from id-6
with 15 max bytes?
Answering this I suppose would solve a lot of our problems....

Marcel did previously suggest a bus-driver sort of a structure, where
the transport are sort of adapter drivers, the data interpretation
logic like splitting HCI-events, FM CH-8 packets all fall into the
algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
drivers falling into the client/protocol drivers....



> Any objections to this?
>
> If you agree, I think we should discuss the alternatives for the common
> module. I think you are saying we should make the common module a single
> ldisc doing the multiplexing and have all transports register as ttys into
> it, right?
>
> What we discussed before was to split it into multiple modules, one for
> the tty ldisc and one for the common code, and then have the spi/i2c/...
> drivers feed into the common multiplexer without going through tty.
>
> I don't currently see a significant advantage or disadvantage with either
> approach.
>
>        Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Pavan Savoy]

On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> >> But I was trying to make a different point here. On a basic level,
>> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>> >> chip that does BT+FM. They all use HCI to access the other functions
>> >> of the combo chip and they do it in a really simiar way, with the
>> >> differences mostly in power management techniques. So I think it's
>> >> quite sensible to have some kind of framework that is suitable for
>> >> such devices.
>> >
>> > Yes, I agree 100% in principle. I could not find the code that
>> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
>>
>> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
>>
>> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
>> sure about the mainline support for those.
>
> Ah, it had not occured to me to look in drivers/misc. Looking at the
> code over there, it becomes much clearer what your point is. Evidently
> the ti-st driver implements a line discipline similar to, but conflicting
> with hci_ldisc, and it has its own copy of the hci_ll code.
>
> I guess this is exactly what we're trying to avoid having with the
> cg2900 code ;-).
>
>> > The cg2900 solution for this was to use MFD (plus another layer
>> > in the posted version, but that will go away I assume). Using
>> > MFD is not the only possibility here, but I could not see anything
>> > wrong with it either. Do you think we can move them all over to
>> > use MFD infrastructure?
>>
>> I guess so but it's probably more of a detail than what I'm up to now :)
>
> Agreed.
>
>> >> But generally speaking, isn't a line discipline/driver attached to a
>> >> tty? We can use dumb tty for e. g. SPI and still be able to use
>> >> hci_ll, right?
>> >
>> > I suggested that as well, but the point was made that this would
>> > add an unnecessary indirection for the SPI case, which is not
>> > really much like a serial port. It's certainly possible to do it
>> > like you say, but if we add a way to register the high-level
>> > protocols with an HCI-like multi-function device, we could
>> > also do it in a way that does not rely on tty-ldisc but keeps it
>> > as one of the options.
>>
>> I actually don't think it's such a big indirection, I prefer to think
>> of it more as of the abstraction layer. If not use this, are we going
>> to have direct SPI device drivers? I'm afraid we might end up with a
>> huge duplication of code in that case.
>
> The question is how it should be modeled in a better way than today.
>
> I believe we already agree it should be something like
>
> =C2=A0 bt =C2=A0 ti-radio =C2=A0 =C2=A0st-e-radio =C2=A0 =C2=A0ti-gps =C2=
=A0 st-e-gps =C2=A0broadcom-radio ...
> =C2=A0 | =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
| =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0 |
> =C2=A0 +---------+----------+---------+--+----------+----------+---------=
+
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0common-hci-module
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0+-----------+-----------+
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A0 =C2=
=A0|
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0seri=
al =C2=A0 =C2=A0spi =C2=A0 =C2=A0 i2c =C2=A0 =C2=A0...

Yes, this sort of architecture would certainly help...
However, I suppose the most common question as one of you stated above
was how can the channel -ID and other factors such as offset-header
packet format be generalized?

As in over here, will the common-hci-module work fine, If say ti-radio
says he is expecting packets starting from id-8 which is of length 10?
and also work for st-e-radio which would say expecting data from id-6
with 15 max bytes?
Answering this I suppose would solve a lot of our problems....

Marcel did previously suggest a bus-driver sort of a structure, where
the transport are sort of adapter drivers, the data interpretation
logic like splitting HCI-events, FM CH-8 packets all fall into the
algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
drivers falling into the client/protocol drivers....



> Any objections to this?
>
> If you agree, I think we should discuss the alternatives for the common
> module. I think you are saying we should make the common module a single
> ldisc doing the multiplexing and have all transports register as ttys int=
o
> it, right?
>
> What we discussed before was to split it into multiple modules, one for
> the tty ldisc and one for the common code, and then have the spi/i2c/...
> drivers feed into the common multiplexer without going through tty.
>
> I don't currently see a significant advantage or disadvantage with either
> approach.
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Hi Pavan et al,

2010/12/8 Pavan Savoy <pavan_savoy@sify.com>:
> On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> >> But I was trying to make a different point here. On a basic level,
>>> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>>> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>>> >> chip that does BT+FM. They all use HCI to access the other functions
>>> >> of the combo chip and they do it in a really simiar way, with the
>>> >> differences mostly in power management techniques. So I think it's
>>> >> quite sensible to have some kind of framework that is suitable for
>>> >> such devices.
>>> >
>>> > Yes, I agree 100% in principle. I could not find the code that
>>> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
>>>
>>> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
>>>
>>> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
>>> sure about the mainline support for those.
>>
>> Ah, it had not occured to me to look in drivers/misc. Looking at the
>> code over there, it becomes much clearer what your point is. Evidently
>> the ti-st driver implements a line discipline similar to, but conflicting
>> with hci_ldisc, and it has its own copy of the hci_ll code.
>>
>> I guess this is exactly what we're trying to avoid having with the
>> cg2900 code ;-).
>>
>>> > The cg2900 solution for this was to use MFD (plus another layer
>>> > in the posted version, but that will go away I assume). Using
>>> > MFD is not the only possibility here, but I could not see anything
>>> > wrong with it either. Do you think we can move them all over to
>>> > use MFD infrastructure?
>>>
>>> I guess so but it's probably more of a detail than what I'm up to now :)
>>
>> Agreed.
>>
>>> >> But generally speaking, isn't a line discipline/driver attached to a
>>> >> tty? We can use dumb tty for e. g. SPI and still be able to use
>>> >> hci_ll, right?
>>> >
>>> > I suggested that as well, but the point was made that this would
>>> > add an unnecessary indirection for the SPI case, which is not
>>> > really much like a serial port. It's certainly possible to do it
>>> > like you say, but if we add a way to register the high-level
>>> > protocols with an HCI-like multi-function device, we could
>>> > also do it in a way that does not rely on tty-ldisc but keeps it
>>> > as one of the options.
>>>
>>> I actually don't think it's such a big indirection, I prefer to think
>>> of it more as of the abstraction layer. If not use this, are we going
>>> to have direct SPI device drivers? I'm afraid we might end up with a
>>> huge duplication of code in that case.
>>
>> The question is how it should be modeled in a better way than today.
>>
>> I believe we already agree it should be something like
>>
>>   bt   ti-radio    st-e-radio    ti-gps   st-e-gps  broadcom-radio ...
>>   |         |          |            |          |          |         |
>>   +---------+----------+---------+--+----------+----------+---------+
>>                                  |
>>                          common-hci-module
>>                                  |
>>                      +-----------+-----------+
>>                      |        |       |      |
>>                    serial    spi     i2c    ...
>
> Yes, this sort of architecture would certainly help...
> However, I suppose the most common question as one of you stated above
> was how can the channel -ID and other factors such as offset-header
> packet format be generalized?
>
> As in over here, will the common-hci-module work fine, If say ti-radio
> says he is expecting packets starting from id-8 which is of length 10?
> and also work for st-e-radio which would say expecting data from id-6
> with 15 max bytes?
> Answering this I suppose would solve a lot of our problems....
>
> Marcel did previously suggest a bus-driver sort of a structure, where
> the transport are sort of adapter drivers, the data interpretation
> logic like splitting HCI-events, FM CH-8 packets all fall into the
> algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
> drivers falling into the client/protocol drivers....
>

What the cg2900 driver has now turned into is almost like the
cg2900_core acting as a bus, identifying which chip is connected and
choosing correct chip driver from this. This cg2900_core module should
not contain any vendor specific code. When identified, the chip driver
then uses MFD to setup channels according to the functionality it
supports (one MFD device per H4 channel). It's hard to explain
everything of the new architecture here. I would like you to check the
new patch set I'm trying hard to create at the moment. I will try to
get it out on Friday, but it's hard to promise anything (there is a
lot of work to do with it). There you could then see if it is
something you could consider useful.

/P-G

>
>
>> Any objections to this?
>>
>> If you agree, I think we should discuss the alternatives for the common
>> module. I think you are saying we should make the common module a single
>> ldisc doing the multiplexing and have all transports register as ttys into
>> it, right?
>>
>> What we discussed before was to split it into multiple modules, one for
>> the tty ldisc and one for the common code, and then have the spi/i2c/...
>> drivers feed into the common multiplexer without going through tty.
>>
>> I don't currently see a significant advantage or disadvantage with either
>> approach.
>>
>>        Arnd
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Par-Gunnar Hjalmdahl]

Hi Pavan et al,

2010/12/8 Pavan Savoy <pavan_savoy@sify.com>:
> On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> >> But I was trying to make a different point here. On a basic level,
>>> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>>> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>>> >> chip that does BT+FM. They all use HCI to access the other functions
>>> >> of the combo chip and they do it in a really simiar way, with the
>>> >> differences mostly in power management techniques. So I think it's
>>> >> quite sensible to have some kind of framework that is suitable for
>>> >> such devices.
>>> >
>>> > Yes, I agree 100% in principle. I could not find the code that
>>> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
>>>
>>> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
>>>
>>> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
>>> sure about the mainline support for those.
>>
>> Ah, it had not occured to me to look in drivers/misc. Looking at the
>> code over there, it becomes much clearer what your point is. Evidently
>> the ti-st driver implements a line discipline similar to, but conflictin=
g
>> with hci_ldisc, and it has its own copy of the hci_ll code.
>>
>> I guess this is exactly what we're trying to avoid having with the
>> cg2900 code ;-).
>>
>>> > The cg2900 solution for this was to use MFD (plus another layer
>>> > in the posted version, but that will go away I assume). Using
>>> > MFD is not the only possibility here, but I could not see anything
>>> > wrong with it either. Do you think we can move them all over to
>>> > use MFD infrastructure?
>>>
>>> I guess so but it's probably more of a detail than what I'm up to now :=
)
>>
>> Agreed.
>>
>>> >> But generally speaking, isn't a line discipline/driver attached to a
>>> >> tty? We can use dumb tty for e. g. SPI and still be able to use
>>> >> hci_ll, right?
>>> >
>>> > I suggested that as well, but the point was made that this would
>>> > add an unnecessary indirection for the SPI case, which is not
>>> > really much like a serial port. It's certainly possible to do it
>>> > like you say, but if we add a way to register the high-level
>>> > protocols with an HCI-like multi-function device, we could
>>> > also do it in a way that does not rely on tty-ldisc but keeps it
>>> > as one of the options.
>>>
>>> I actually don't think it's such a big indirection, I prefer to think
>>> of it more as of the abstraction layer. If not use this, are we going
>>> to have direct SPI device drivers? I'm afraid we might end up with a
>>> huge duplication of code in that case.
>>
>> The question is how it should be modeled in a better way than today.
>>
>> I believe we already agree it should be something like
>>
>> =A0 bt =A0 ti-radio =A0 =A0st-e-radio =A0 =A0ti-gps =A0 st-e-gps =A0broa=
dcom-radio ...
>> =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 =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 =A0common-hci-module
>> =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 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0| =A0 =A0 =
=A0 | =A0 =A0 =A0|
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0serial =A0 =A0spi =A0 =A0 i2c =A0=
 =A0...
>
> Yes, this sort of architecture would certainly help...
> However, I suppose the most common question as one of you stated above
> was how can the channel -ID and other factors such as offset-header
> packet format be generalized?
>
> As in over here, will the common-hci-module work fine, If say ti-radio
> says he is expecting packets starting from id-8 which is of length 10?
> and also work for st-e-radio which would say expecting data from id-6
> with 15 max bytes?
> Answering this I suppose would solve a lot of our problems....
>
> Marcel did previously suggest a bus-driver sort of a structure, where
> the transport are sort of adapter drivers, the data interpretation
> logic like splitting HCI-events, FM CH-8 packets all fall into the
> algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
> drivers falling into the client/protocol drivers....
>

What the cg2900 driver has now turned into is almost like the
cg2900_core acting as a bus, identifying which chip is connected and
choosing correct chip driver from this. This cg2900_core module should
not contain any vendor specific code. When identified, the chip driver
then uses MFD to setup channels according to the functionality it
supports (one MFD device per H4 channel). It's hard to explain
everything of the new architecture here. I would like you to check the
new patch set I'm trying hard to create at the moment. I will try to
get it out on Friday, but it's hard to promise anything (there is a
lot of work to do with it). There you could then see if it is
something you could consider useful.

/P-G

>
>
>> Any objections to this?
>>
>> If you agree, I think we should discuss the alternatives for the common
>> module. I think you are saying we should make the common module a single
>> ldisc doing the multiplexing and have all transports register as ttys in=
to
>> it, right?
>>
>> What we discussed before was to split it into multiple modules, one for
>> the tty ldisc and one for the common code, and then have the spi/i2c/...
>> drivers feed into the common multiplexer without going through tty.
>>
>> I don't currently see a significant advantage or disadvantage with eithe=
r
>> approach.
>>
>> =A0 =A0 =A0 =A0Arnd
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoot=
h" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[Arnd Bergmann]

On Wednesday 08 December 2010, Par-Gunnar Hjalmdahl wrote:
> > Marcel did previously suggest a bus-driver sort of a structure, where
> > the transport are sort of adapter drivers, the data interpretation
> > logic like splitting HCI-events, FM CH-8 packets all fall into the
> > algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
> > drivers falling into the client/protocol drivers....
> 
> What the cg2900 driver has now turned into is almost like the
> cg2900_core acting as a bus, identifying which chip is connected and
> choosing correct chip driver from this. This cg2900_core module should
> not contain any vendor specific code. When identified, the chip driver
> then uses MFD to setup channels according to the functionality it
> supports (one MFD device per H4 channel). It's hard to explain
> everything of the new architecture here. I would like you to check the
> new patch set I'm trying hard to create at the moment. I will try to
> get it out on Friday, but it's hard to promise anything (there is a
> lot of work to do with it). There you could then see if it is
> something you could consider useful.

It sounds really good, I'm looking forward to your new patch set!
If it's generic enough to replace the ti-st code, that would be a
clear sign that you are on the right track ;-)

I'm not asking to fix their code yourself, but it might be good to
look at it and see if it would work.

	Arnd