[ patch 1/7] drivers/serial/jsm: new serial device driver

[ patch 1/7] drivers/serial/jsm: new serial device driver

[Wen Xiong]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

Hi All,

We are submitting a new serial driver for the 2.6 kernel. This device 
driver is for the Digi Neo serial port adapter.

We made some changes based on great comments from linux community. We 
 used the Russell's serial_core interface on 2.6 kernel, handled all 
initialization of  module correctly and used fs/seq_file.c interface for 
/proc entry.  And we made some changes based on Greg KH's comments also 
including removing whitespace, changing some function comments, removing 
msleep() etc.

Per requests, I split it up in smaller chunks and sent them in several 
emails for you. You are able to read the code and comment the code 
easily. I added more adapter descriptions for you.

Neo adapter description:
This adapter is a 920K baud non-intelligent asynchronous serial 
communications adapter for PCI bus. The adapter is based on Exar 17D152 
Universal PCI Dual UART IC. The adapter is mapped into the PCI bus 
memory space, and offers extened UART register mapping beyond the 
standard 16c554 registers.  This driver used serial_core.c interface on 
2.6 serials of kernel.

Thanks for all your help!
wendy

This first patch included jsm_driver.c. This routine included all PCI 
initialization functions for this driver.

Signed-off-by: Wen Xiong <wendyx@us.ltcfwd.linux.ibm.com>



[-- Attachment #2: patch1.jasmine --]
[-- Type: text/plain, Size: 18882 bytes --]

diff -Nuar linux-2.6.9.orig/drivers/serial/jsm/jsm_driver.c linux-2.6.9.new/drivers/serial/jsm/jsm_driver.c
--- linux-2.6.9.orig/drivers/serial/jsm/jsm_driver.c	1969-12-31 18:00:00.000000000 -0600
+++ linux-2.6.9.new/drivers/serial/jsm/jsm_driver.c	2005-02-27 17:11:25.501022152 -0600
@@ -0,0 +1,733 @@
+/*
+ * Copyright 2003 Digi International (www.digi.com)
+ *	Scott H Kilau <Scott_Kilau at digi dot com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the 
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR 
+ * PURPOSE.  See the GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ *
+ *	NOTE TO LINUX KERNEL HACKERS:  DO NOT REFORMAT THIS CODE!
+ *
+ *	This is shared code between Digi's CVS archive and the
+ *	Linux Kernel sources.
+ *	Changing the source just for reformatting needlessly breaks
+ *	our CVS diff history.
+ *
+ *	Send any bug fixes/changes to:  Eng.Linux at digi dot com.
+ *	Thank you.
+ *
+ * $Id: jsm_driver.c,v 1.57 2004/08/31 19:41:34 scottk Exp $
+ *
+ */
+
+
+#include <linux/kernel.h>
+#include <linux/version.h>
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/moduleparam.h>
+
+#include "jsm_driver.h"
+#include "jsm_pci.h"
+#include "dpacompat.h"
+#include "jsm_mgmt.h"
+#include "jsm_tty.h"
+#include "jsm_neo.h"
+
+MODULE_AUTHOR("Digi International, http://www.digi.com");
+MODULE_DESCRIPTION("Driver for the Digi International Neo and Classic PCI based product line");
+MODULE_SUPPORTED_DEVICE("jsm");
+
+#define JSM_DRIVER_NAME "jsm"
+#define NR_PORTS	32 
+#define JSM_MINOR_START	0 
+
+struct uart_driver jsm_uart_driver = {
+	.owner = THIS_MODULE,
+	.driver_name = JSM_DRIVER_NAME,
+	.dev_name = "ttyn", 
+	.major = 253,
+	.minor = JSM_MINOR_START, 
+	.nr = NR_PORTS,
+	.cons = NULL,
+};
+
+/*
+ * insmod command line overrideable parameters
+ *
+ * NOTE: we use a set of macros to create the variables, which allows
+ * us to specify the variable type, name, initial value, and description.
+ */
+static int debug;
+static int rawreadok;
+static int trcbuf_size;
+module_param(debug, int, 0);
+module_param(rawreadok, int, 1);
+module_param(trcbuf_size, int, 0x100000);
+MODULE_PARM_DESC(debug, "Driver debugging level");
+MODULE_PARM_DESC(rawreadok, "Bypass flip buffers on input");
+MODULE_PARM_DESC(trcbuf_size, "Debugging trace buffer size. 1M");
+
+static int jsm_init_module(void);
+static void jsm_exit_module(void);
+static void jsm_poll_handler(ulong dummy);
+static int jsm_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static void jsm_remove_one(struct pci_dev *dev);
+
+/*
+ * File operations permitted on Control/Management major.
+ */
+static struct file_operations jsm_BoardFops =
+{
+	.owner		=	THIS_MODULE,
+	.ioctl		=	jsm_mgmt_ioctl,
+	.open		=	jsm_mgmt_open,
+	.release	=	jsm_mgmt_close
+};
+
+/*
+ * Globals
+ */
+uint		jsm_NumBoards,current_NumBoards;
+struct board_t	*jsm_Board[MAXBOARDS];
+char		*jsm_board_slot[MAXBOARDS];
+spinlock_t	jsm_global_lock = SPIN_LOCK_UNLOCKED;
+int		jsm_driver_state = DRIVER_INITIALIZED;
+ulong		jsm_poll_counter;
+uint		jsm_Major;
+int		jsm_debug;
+int		jsm_rawreadok;
+int		jsm_trcbuf_size;
+
+/*
+ * Static vars.
+ */
+static uint	jsm_major_control_registered = FALSE;
+static uint	jsm_driver_start = FALSE;
+
+/*
+ * Poller stuff
+ */
+static spinlock_t	jsm_poll_lock = SPIN_LOCK_UNLOCKED;
+static ulong		jsm_poll_time;				/* Time of next poll */
+static ulong		jsm_poll_tick = 50;			/* Poll interval - 20 ms */
+
+static struct timer_list jsm_poll_timer = {
+	.function =	jsm_poll_handler
+};
+
+static struct pci_device_id jsm_pci_tbl[] = {
+	{	DIGI_VID, PCI_DEVICE_NEO_4_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,		0 },
+	{	DIGI_VID, PCI_DEVICE_NEO_8_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,		1 },
+	{	DIGI_VID, PCI_DEVICE_NEO_2DB9_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	2 },
+	{	DIGI_VID, PCI_DEVICE_NEO_2DB9PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	3 },
+	{	DIGI_VID, PCI_DEVICE_NEO_2RJ45_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	4 },
+	{	DIGI_VID, PCI_DEVICE_NEO_2RJ45PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	5 },
+	{	DIGI_VID, PCI_DEVICE_NEO_1_422_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	6 },
+	{	DIGI_VID, PCI_DEVICE_NEO_1_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	7 },
+	{	DIGI_VID, PCI_DEVICE_NEO_2_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	8 },
+	{0,}						/* 0 terminated list. */
+};
+MODULE_DEVICE_TABLE(pci, jsm_pci_tbl);
+
+struct board_id {
+	uchar *name;
+	uint maxports;
+};
+
+static struct board_id jsm_Ids[] = {	
+	{	PCI_DEVICE_NEO_4_PCI_NAME,		4	},
+	{	PCI_DEVICE_NEO_8_PCI_NAME,		8	},
+	{	PCI_DEVICE_NEO_2DB9_PCI_NAME,		2	},
+	{	PCI_DEVICE_NEO_2DB9PRI_PCI_NAME,	2	},
+	{	PCI_DEVICE_NEO_2RJ45_PCI_NAME,		2	},
+	{	PCI_DEVICE_NEO_2RJ45PRI_PCI_NAME,	2	},
+	{	PCI_DEVICE_NEO_1_422_PCI_NAME,		1	},
+	{	PCI_DEVICE_NEO_1_422_485_PCI_NAME,	1	},
+	{	PCI_DEVICE_NEO_2_422_485_PCI_NAME,	2	},
+	{	NULL,					0	}
+};
+
+struct pci_driver jsm_driver = {
+	.name		= "jsm",
+	.probe		= jsm_init_one,
+	.id_table	= jsm_pci_tbl,
+	.remove		= __devexit_p(jsm_remove_one),
+};
+
+char *jsm_state_text[] = {
+	"Board Failed",
+	"Board Found",
+	"Board READY",
+};
+
+char *jsm_driver_state_text[] = {
+	"Driver Initialized",
+	"Driver Ready."
+};
+
+/*
+ * jsm_init_globals()
+ *
+ * This is where we initialize the globals from the static insmod
+ * configuration variables.  These are declared near the head of
+ * this file.
+ */
+static void jsm_init_globals(void)
+{
+	int i = 0;
+
+	jsm_rawreadok		= rawreadok;
+	jsm_trcbuf_size		= trcbuf_size;
+	jsm_debug		= debug;
+
+	for (i = 0; i < MAXBOARDS; i++) {
+		jsm_Board[i] = NULL;
+		jsm_board_slot[i] = (char *)kmalloc(20, GFP_KERNEL);
+		memset(jsm_board_slot[i], 0, 20);
+	}
+
+	init_timer(&jsm_poll_timer);
+}
+
+/*
+ * jsm_poll_handler
+ *
+ *	As each timer expires, it determines (a) whether the "transmit"
+ * waiter needs to be woken up, and (b) whether the poller needs to
+ * be rescheduled.
+ */
+static void jsm_poll_handler(ulong dummy)
+{
+	struct board_t *brd;
+	unsigned long lock_flags;
+	int i;
+
+	jsm_poll_counter++;
+
+	/*
+	 * Do not start the board state machine until
+	 * driver tells us its up and running, and has
+	 * everything it needs.
+	 */
+	if (jsm_driver_state != DRIVER_READY)
+		goto schedule_poller;
+
+	/* Go thru each board, kicking off a tasklet for each if needed */
+	for (i = 0; i < jsm_NumBoards; i++) {
+		if (jsm_Board[i] == NULL) continue;
+		brd = jsm_Board[i];
+
+		spin_lock_irqsave(&brd->bd_lock, lock_flags);
+
+		/* If board is in a failed state, don't bother scheduling a tasklet */
+		if (brd->state == BOARD_FAILED) {
+			spin_lock_irqsave(&brd->bd_lock, lock_flags);
+			continue;
+		}
+
+		spin_unlock_irqrestore(&brd->bd_lock, lock_flags);
+	}
+
+schedule_poller:
+
+	/*
+	 * Schedule ourself back at the nominal wakeup interval.
+	 */
+	if (current_NumBoards >= 0) {
+		ulong time;
+
+		spin_lock_irqsave(&jsm_poll_lock, lock_flags);
+		jsm_poll_time += jsm_jiffies_from_ms(jsm_poll_tick);
+
+		time = jsm_poll_time - jiffies;
+
+		if ((ulong) time >= 2 * jsm_poll_tick)
+			jsm_poll_time = jiffies +  jsm_jiffies_from_ms(jsm_poll_tick);
+
+		jsm_poll_timer.expires = jsm_poll_time;
+		spin_unlock_irqrestore(&jsm_poll_lock, lock_flags);
+
+		add_timer(&jsm_poll_timer);
+	}
+}
+
+/*
+ * Start of driver.
+ */
+static int jsm_start(void)
+{
+	int rc = 0;
+	unsigned long lock_flags;
+
+	if (!jsm_driver_start) {
+		jsm_driver_start = TRUE;
+
+		/* make sure that the globals are init'd before we do anything else */
+		jsm_init_globals();
+		jsm_NumBoards = 0;
+		current_NumBoards = -1;
+
+		APR(("For the tools package or updated drivers please visit http://www.digi.com\n"));
+
+		/*
+		 * Register our base character device into the kernel.
+		 * This allows the download daemon to connect to the downld device
+		 * before any of the boards are init'ed.
+		 */
+		if (!jsm_major_control_registered) {
+			/*
+			 * Register management/dpa devices
+			 */
+			rc = register_chrdev(0, "jsm", &jsm_BoardFops);
+			if (rc <= 0) {
+				APR(("Can't register jsm driver device (%d)\n", rc));
+				return -ENXIO;
+			}
+			jsm_Major = rc;
+			jsm_major_control_registered = TRUE;
+		}
+
+		/*
+		 * Register our basic stuff in /proc/jsm
+		 */
+		jsm_proc_register_basic_prescan();
+
+		if (rc < 0) {
+			APR(("tty preinit - not enough memory (%d)\n", rc));
+			return rc;
+		}
+
+		/* Start the poller */
+		spin_lock_irqsave(&jsm_poll_lock, lock_flags);
+		jsm_poll_time = jiffies + jsm_jiffies_from_ms(jsm_poll_tick);
+		jsm_poll_timer.expires = jsm_poll_time;
+		spin_unlock_irqrestore(&jsm_poll_lock, lock_flags);
+
+		add_timer(&jsm_poll_timer);
+
+		jsm_driver_state = DRIVER_READY;
+	}
+	return rc;
+}
+
+static int jsm_finalize_board_init(struct board_t *brd) 
+{
+	int rc = 0;
+
+	DPR_INIT(("jsm_finalize_board_init() - start\n"));
+
+	if (!brd || brd->magic != JSM_BOARD_MAGIC)
+		return -ENODEV;
+
+	DPR_INIT(("jsm_finalize_board_init() - start #2\n"));
+
+	if (brd->irq) {
+		rc = request_irq(brd->irq, brd->bd_ops->intr, SA_INTERRUPT|SA_SHIRQ, "JSM", brd);
+
+		if (rc) {
+			printk(KERN_WARNING "Failed to hook IRQ %d\n",brd->irq);
+			brd->state = BOARD_FAILED;
+			brd->dpastatus = BD_NOFEP;
+			rc = -ENODEV;
+		} else {
+			DPR_INIT(("Requested and received usage of IRQ %d\n", brd->irq));
+		}
+	}
+	return rc;
+}
+
+/*
+ * jsm_found_board()
+ *
+ * A board has been found, init it.
+ */
+static int jsm_found_board(struct pci_dev *pdev, int id)
+{
+	struct board_t *brd;
+	unsigned int pci_irq;
+	int i = 0;
+	int rc = 0;
+	int index;
+	int wen_board;
+	int hotplug = 0;
+
+	wen_board = jsm_NumBoards;
+	for (index = 0; index < jsm_NumBoards; index++) {
+		if (!strcmp(jsm_board_slot[index], pdev->slot_name)) {
+			wen_board = index;
+			hotplug = 1;
+			break;
+		}
+	}
+
+	brd = jsm_Board[wen_board] =
+	(struct board_t *)kmalloc(sizeof(struct board_t), GFP_KERNEL);
+	if (!brd) {
+		APR(("memory allocation for board structure failed\n"));
+		return -ENOMEM;
+	}
+	memset(brd, 0, sizeof(struct board_t));
+
+	/* store the info for the board we've found */
+	brd->magic = JSM_BOARD_MAGIC;
+	brd->boardnum = wen_board;
+	brd->vendor = jsm_pci_tbl[id].vendor;
+	brd->device = jsm_pci_tbl[id].device;
+	brd->pci_bus = pdev->bus->number;
+	brd->pci_slot = PCI_SLOT(pdev->devfn);
+	brd->pci_dev = pdev;
+	brd->name = jsm_Ids[id].name;
+	brd->maxports = jsm_Ids[id].maxports;
+	brd->dpastatus = BD_NOFEP;
+	strcpy(jsm_board_slot[wen_board],pdev->slot_name);
+	init_waitqueue_head(&brd->state_wait);
+
+	spin_lock_init(&brd->bd_lock);
+	spin_lock_init(&brd->bd_intr_lock);
+
+	brd->state = BOARD_FOUND;
+
+	for (i = 0; i < MAXPORTS; i++) 
+		brd->channels[i] = NULL;
+
+	/* store which card & revision we have */
+	pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &brd->subvendor);
+	pci_read_config_word(pdev, PCI_SUBSYSTEM_ID, &brd->subdevice);
+	pci_read_config_byte(pdev, PCI_REVISION_ID, &brd->rev);
+
+	pci_irq = pdev->irq;
+	brd->irq = pci_irq;
+
+	switch(brd->device) {
+
+	case PCI_DEVICE_NEO_4_DID:
+	case PCI_DEVICE_NEO_8_DID:
+	case PCI_DEVICE_NEO_2DB9_DID:
+	case PCI_DEVICE_NEO_2DB9PRI_DID:
+	case PCI_DEVICE_NEO_2RJ45_DID:
+	case PCI_DEVICE_NEO_2RJ45PRI_DID:
+	case PCI_DEVICE_NEO_1_422_DID:
+	case PCI_DEVICE_NEO_1_422_485_DID:
+	case PCI_DEVICE_NEO_2_422_485_DID:
+
+		/*
+		 * This chip is set up 100% when we get to it.
+		 * No need to enable global interrupts or anything. 
+		 */
+		brd->dpatype = T_NEO | T_PCIBUS;
+
+		DPR_INIT(("jsm_found_board - NEO.\n"));
+
+		/* get the PCI Base Address Registers */
+		brd->membase	= pci_resource_start(pdev, 0);
+		brd->membase_end = pci_resource_end(pdev, 0);
+
+		if (brd->membase & 1)
+			brd->membase &= ~3;
+		else
+			brd->membase &= ~15;
+
+		/* Assign the board_ops struct */
+		brd->bd_ops = &jsm_neo_ops;
+
+		brd->bd_uart_offset = 0x200;
+		brd->bd_dividend = 921600;
+
+		brd->re_map_membase = ioremap(brd->membase, 0x1000);
+		DPR_INIT(("remapped mem: 0x%p\n", brd->re_map_membase));
+		if (!brd->re_map_membase) {
+			kfree(brd);
+			APR(("card has no PCI Memory resources, failing board.\n"));
+			return -ENOMEM;
+		}
+		break;
+
+	default:
+		APR(("Did not find any compatible Neo or Classic PCI boards in system.\n"));
+		kfree(brd);
+		return -ENXIO;
+	}
+
+	/*
+	 * Do tty device initialization.
+	 */
+	rc = jsm_finalize_board_init(brd);
+	if (rc < 0) {
+		APR(("Can't finalize board init (%d)\n", rc));
+		brd->state = BOARD_FAILED;
+		brd->dpastatus = BD_NOFEP;
+		goto failed;
+	}
+
+	rc = jsm_tty_register(brd);
+	if (rc < 0) {
+		jsm_tty_uninit(brd);
+		APR(("Can't register tty devices (%d)\n", rc));
+		brd->state = BOARD_FAILED;
+		brd->dpastatus = BD_NOFEP;
+		goto failed;
+	}
+
+	rc = jsm_tty_init(brd);
+	if (rc < 0) {
+		jsm_tty_uninit(brd);
+		APR(("Can't init tty devices (%d)\n", rc));
+		brd->state = BOARD_FAILED;
+		brd->dpastatus = BD_NOFEP;
+		goto failed;
+	}
+
+	rc = jsm_uart_port_init(brd);
+	if (rc < 0)
+		goto failed;
+
+	brd->state = BOARD_READY;
+	brd->dpastatus = BD_RUNNING;
+
+	/* init our poll helper tasklet */
+	tasklet_init(&brd->helper_tasklet, brd->bd_ops->tasklet, (unsigned long) brd);
+
+	/* Log the information about the board */
+	APR(("board %d: %s (rev %d), irq %d\n",wen_board, brd->name, brd->rev, brd->irq));
+
+	/*
+	 * allocate flip buffer for board.
+	 *
+	 * Okay to malloc with GFP_KERNEL, we are not at interrupt
+	 * context, and there are no locks held.
+	 */
+	brd->flipbuf = kmalloc(MYFLIPLEN, GFP_KERNEL);
+	if (!brd->flipbuf) {
+		APR(("memory allocation for flipbuf failed\n"));
+		return -ENOMEM;
+	}
+	memset(brd->flipbuf, 0, MYFLIPLEN);
+
+	jsm_proc_register_basic_postscan(wen_board);
+	wake_up_interruptible(&brd->state_wait);
+
+	if (!hotplug)
+		jsm_NumBoards++;
+
+	current_NumBoards++;
+
+	return 0;
+
+failed:
+	kfree(brd);
+	iounmap((void *) brd->re_map_membase);
+	return -ENXIO;
+}
+
+
+static int jsm_probe1(struct pci_dev *pdev, int card_type)
+{
+	return jsm_found_board(pdev, card_type);
+}
+
+/* returns count (>= 0), or negative on error */
+static int jsm_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+	int rc;
+
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "Device enable FAILED\n");
+		return rc;
+	} 
+
+	if ((rc = jsm_probe1(pdev, ent->driver_data))) {
+		dev_err(&pdev->dev, "jsm_probe1 FAILED\n");
+		pci_disable_device(pdev);
+	 	return rc;
+	}
+	return rc;
+}
+
+
+/*
+ * jsm_cleanup_board()
+ *
+ * Free all the memory associated with a board
+ */
+static void jsm_cleanup_board(struct board_t *brd)
+{
+	int i = 0;
+
+	if (!brd || brd->magic != JSM_BOARD_MAGIC)
+		return ;
+
+	if (brd->irq)
+		free_irq(brd->irq, brd);
+
+	tasklet_kill(&brd->helper_tasklet);
+
+	if (brd->re_map_membase) {
+		iounmap(brd->re_map_membase);
+		brd->re_map_membase = NULL;
+	}
+
+	/* Free all allocated channels structs */
+	for (i = 0; i < MAXPORTS; i++) {
+		if (brd->channels[i]) {
+			if (brd->channels[i]->ch_rqueue)
+				kfree(brd->channels[i]->ch_rqueue);
+			if (brd->channels[i]->ch_equeue)
+				kfree(brd->channels[i]->ch_equeue);
+			if (brd->channels[i]->ch_wqueue)
+				kfree(brd->channels[i]->ch_wqueue);
+
+			kfree(brd->channels[i]);
+			brd->channels[i] = NULL;
+		}
+	}
+
+	if (brd->flipbuf)
+		kfree(brd->flipbuf);
+
+	jsm_Board[brd->boardnum] = NULL;
+
+	kfree(brd);
+	current_NumBoards--;
+}
+
+static void jsm_remove_one(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < jsm_NumBoards; i++) {
+		if ((jsm_Board[i] != NULL) && (jsm_Board[i]->pci_dev == dev)) {
+			jsm_proc_unregister_brd(i);
+			jsm_remove_uart_port(jsm_Board[i]);
+			jsm_tty_uninit(jsm_Board[i]);
+			jsm_cleanup_board(jsm_Board[i]);
+		}
+	}
+}
+
+/*
+ * jsm_init_module()
+ *
+ * Module load.  This is where it all starts.
+ */
+static int __init
+jsm_init_module(void)
+{
+	int rc = 0;
+
+	APR(("%s, Digi International Part Number %s\n", "jsm: 1.1-1-INKERNEL", "40002438_A-INKERNEL"));
+
+	/*
+	 * Initialize global stuff
+	 */
+	rc = jsm_start();
+	if (rc < 0) 
+		return rc;
+
+	rc = uart_register_driver(&jsm_uart_driver);
+	if (rc)
+		return rc;
+
+	rc = pci_register_driver(&jsm_driver);
+	if (rc < 0)
+		uart_unregister_driver(&jsm_uart_driver);
+
+	return rc;
+}
+
+module_init(jsm_init_module);
+
+/*
+ * jsm_exit_module()
+ *
+ * Module unload.  This is where it all ends.
+ */
+static void __exit
+jsm_exit_module(void)
+{
+	int i;
+
+	del_timer_sync(&jsm_poll_timer);
+
+	if (jsm_major_control_registered)
+		unregister_chrdev(jsm_Major, "jsm");
+
+	pci_unregister_driver(&jsm_driver);
+
+	jsm_proc_unregister_all();
+
+	for (i = 0; i < MAXBOARDS; i++) {
+		jsm_board_slot[i] = NULL;
+		kfree(jsm_board_slot[i]);
+	}
+	uart_unregister_driver(&jsm_uart_driver);
+}
+module_exit(jsm_exit_module);
+MODULE_LICENSE("GPL");
+
+/*
+ * jsm_ioctl_name()
+ *
+ * Returns a text version of each ioctl value.
+ */
+char *jsm_ioctl_name(int cmd)
+{
+	switch(cmd) {
+
+	case TCGETA:		return("TCGETA");
+	case TCGETS:		return("TCGETS");
+	case TCSETA:		return("TCSETA");
+	case TCSETS:		return("TCSETS");
+	case TCSETAW:		return("TCSETAW");
+	case TCSETSW:		return("TCSETSW");
+	case TCSETAF:		return("TCSETAF");
+	case TCSETSF:		return("TCSETSF");
+	case TCSBRK:		return("TCSBRK");
+	case TCXONC:		return("TCXONC");
+	case TCFLSH:		return("TCFLSH");
+	case TIOCGSID:		return("TIOCGSID");
+
+	case TIOCGETD:		return("TIOCGETD");
+	case TIOCSETD:		return("TIOCSETD");
+	case TIOCGWINSZ:	return("TIOCGWINSZ");
+	case TIOCSWINSZ:	return("TIOCSWINSZ");
+
+	case TIOCMGET:		return("TIOCMGET");
+	case TIOCMSET:		return("TIOCMSET");
+	case TIOCMBIS:		return("TIOCMBIS");
+	case TIOCMBIC:		return("TIOCMBIC");
+
+	/* from digi.h */
+	case DIGI_SETA:		return("DIGI_SETA");
+	case DIGI_SETAW:	return("DIGI_SETAW");
+	case DIGI_SETAF:	return("DIGI_SETAF");
+	case DIGI_SETFLOW:	return("DIGI_SETFLOW");
+	case DIGI_SETAFLOW:	return("DIGI_SETAFLOW");
+	case DIGI_GETFLOW:	return("DIGI_GETFLOW");
+	case DIGI_GETAFLOW:	return("DIGI_GETAFLOW");
+	case DIGI_GETA:		return("DIGI_GETA");
+	case DIGI_GEDELAY:	return("DIGI_GEDELAY");
+	case DIGI_SEDELAY:	return("DIGI_SEDELAY");
+	case DIGI_GETCUSTOMBAUD: return("DIGI_GETCUSTOMBAUD");
+	case DIGI_SETCUSTOMBAUD: return("DIGI_SETCUSTOMBAUD");
+	case TIOCMODG:		return("TIOCMODG");
+	case TIOCMODS:		return("TIOCMODS");
+	case TIOCSDTR:		return("TIOCSDTR");
+	case TIOCCDTR:		return("TIOCCDTR");
+
+	default:		return("unknown");
+	}
+}

Re: [ patch 1/7] drivers/serial/jsm: new serial device driver

[Kyle Moffett]

On Feb 27, 2005, at 18:38, Wen Xiong wrote:
> +/*
> + * jsm_init_globals()
> + *
> + * This is where we initialize the globals from the static insmod
> + * configuration variables.  These are declared near the head of
> + * this file.
> + */
> +static void jsm_init_globals(void)
> +{
> +	int i = 0;
> +
> +	jsm_rawreadok		= rawreadok;
> +	jsm_trcbuf_size		= trcbuf_size;
> +	jsm_debug		= debug;
> +
> +	for (i = 0; i < MAXBOARDS; i++) {
> +		jsm_Board[i] = NULL;
> +		jsm_board_slot[i] = (char *)kmalloc(20, GFP_KERNEL);
> +		memset(jsm_board_slot[i], 0, 20);
> +	}

Instead of several 20-byte kmallocs, you could help reduce memory
usage and fragmentation with something like this:

static void *jsm_board_slot_mem;

jsm_board_slot_mem = kmalloc(20*MAXBOARDS, GFP_KERNEL);
memset(jsm_board_slot_mem, 0, 20*MAXBOARDS)
for (i = 0; i < MAXBOARDS; i++) {
	jsm_Board[i] = NULL;
	jsm_board_slot[i] = &jsm_board_slot_mem[20*i];
}

Then free like this:
kfree(jsm_board_slot_mem);

On the other hand, it might be nice to have all these structures
dynamically allocated, so that the no-boards case only uses the 8 or
16 bytes of RAM required for a struct list_head.  In that case you
could clump the other board info into a single struct instead of
multiple independent static arrays.  Something like this might work:

struct jsm_board_instance {
	struct list_head board_list;
	struct board_t board;
	char slot[20];
	[...etc...]
};
static struct list_head jsm_board_list = LIST_HEAD_INIT(jsm_board_list);
static spinlock_t jsm_board_list_lock = SPIN_LOCK_UNLOCKED;

Then when doing hardware add/remove, take the lock and do the list
manipulation, then unlock.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

Re: [ patch 1/7] drivers/serial/jsm: new serial device driver

[Jeff Garzik]

Wen Xiong wrote:


General comment:  This driver has -way- too many global variables.

Normal drivers should associate state information with each instance of 
a device (e.g. each struct pci_dev), not globally.


> +/*
> + * Globals
> + */
> +uint		jsm_NumBoards,current_NumBoards;
> +struct board_t	*jsm_Board[MAXBOARDS];
> +char		*jsm_board_slot[MAXBOARDS];
> +spinlock_t	jsm_global_lock = SPIN_LOCK_UNLOCKED;
> +int		jsm_driver_state = DRIVER_INITIALIZED;
> +ulong		jsm_poll_counter;
> +uint		jsm_Major;
> +int		jsm_debug;
> +int		jsm_rawreadok;
> +int		jsm_trcbuf_size;

Eliminate MAXBOARDS.  Such static limits are completely unnecessary.


> +/*
> + * Static vars.
> + */
> +static uint	jsm_major_control_registered = FALSE;
> +static uint	jsm_driver_start = FALSE;

Eliminate jsm_driver_start.  This is unnecessary also.



> +/*
> + * Poller stuff
> + */
> +static spinlock_t	jsm_poll_lock = SPIN_LOCK_UNLOCKED;
> +static ulong		jsm_poll_time;				/* Time of next poll */
> +static ulong		jsm_poll_tick = 50;			/* Poll interval - 20 ms */
> +
> +static struct timer_list jsm_poll_timer = {
> +	.function =	jsm_poll_handler
> +};
> +
> +static struct pci_device_id jsm_pci_tbl[] = {
> +	{	DIGI_VID, PCI_DEVICE_NEO_4_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,		0 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_8_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,		1 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2DB9_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	2 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2DB9PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	3 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2RJ45_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	4 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2RJ45PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	5 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_1_422_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	6 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_1_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	7 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	8 },
> +	{0,}						/* 0 terminated list. */

Don't create your own constants, use standard linux/pci_ids.h ones.


> +MODULE_DEVICE_TABLE(pci, jsm_pci_tbl);
> +
> +struct board_id {
> +	uchar *name;
> +	uint maxports;
> +};
> +
> +static struct board_id jsm_Ids[] = {	
> +	{	PCI_DEVICE_NEO_4_PCI_NAME,		4	},
> +	{	PCI_DEVICE_NEO_8_PCI_NAME,		8	},
> +	{	PCI_DEVICE_NEO_2DB9_PCI_NAME,		2	},
> +	{	PCI_DEVICE_NEO_2DB9PRI_PCI_NAME,	2	},
> +	{	PCI_DEVICE_NEO_2RJ45_PCI_NAME,		2	},
> +	{	PCI_DEVICE_NEO_2RJ45PRI_PCI_NAME,	2	},
> +	{	PCI_DEVICE_NEO_1_422_PCI_NAME,		1	},
> +	{	PCI_DEVICE_NEO_1_422_485_PCI_NAME,	1	},
> +	{	PCI_DEVICE_NEO_2_422_485_PCI_NAME,	2	},
> +	{	NULL,					0	}
> +};
> +
> +struct pci_driver jsm_driver = {
> +	.name		= "jsm",
> +	.probe		= jsm_init_one,
> +	.id_table	= jsm_pci_tbl,
> +	.remove		= __devexit_p(jsm_remove_one),
> +};
> +
> +char *jsm_state_text[] = {
> +	"Board Failed",
> +	"Board Found",
> +	"Board READY",
> +};
> +
> +char *jsm_driver_state_text[] = {
> +	"Driver Initialized",
> +	"Driver Ready."
> +};
> +
> +/*
> + * jsm_init_globals()
> + *
> + * This is where we initialize the globals from the static insmod
> + * configuration variables.  These are declared near the head of
> + * this file.
> + */
> +static void jsm_init_globals(void)
> +{
> +	int i = 0;
> +
> +	jsm_rawreadok		= rawreadok;
> +	jsm_trcbuf_size		= trcbuf_size;
> +	jsm_debug		= debug;
> +
> +	for (i = 0; i < MAXBOARDS; i++) {
> +		jsm_Board[i] = NULL;
> +		jsm_board_slot[i] = (char *)kmalloc(20, GFP_KERNEL);
> +		memset(jsm_board_slot[i], 0, 20);
> +	}
> +
> +	init_timer(&jsm_poll_timer);
> +}

When MAXBOARDS is eliminated, and the poll timer is made per-device, 
this code will go away.


> +/*
> + * jsm_poll_handler
> + *
> + *	As each timer expires, it determines (a) whether the "transmit"
> + * waiter needs to be woken up, and (b) whether the poller needs to
> + * be rescheduled.
> + */
> +static void jsm_poll_handler(ulong dummy)
> +{
> +	struct board_t *brd;
> +	unsigned long lock_flags;
> +	int i;
> +
> +	jsm_poll_counter++;
> +
> +	/*
> +	 * Do not start the board state machine until
> +	 * driver tells us its up and running, and has
> +	 * everything it needs.
> +	 */
> +	if (jsm_driver_state != DRIVER_READY)
> +		goto schedule_poller;
> +
> +	/* Go thru each board, kicking off a tasklet for each if needed */
> +	for (i = 0; i < jsm_NumBoards; i++) {
> +		if (jsm_Board[i] == NULL) continue;
> +		brd = jsm_Board[i];
> +
> +		spin_lock_irqsave(&brd->bd_lock, lock_flags);
> +
> +		/* If board is in a failed state, don't bother scheduling a tasklet */
> +		if (brd->state == BOARD_FAILED) {
> +			spin_lock_irqsave(&brd->bd_lock, lock_flags);
> +			continue;
> +		}
> +
> +		spin_unlock_irqrestore(&brd->bd_lock, lock_flags);
> +	}
> +
> +schedule_poller:
> +
> +	/*
> +	 * Schedule ourself back at the nominal wakeup interval.
> +	 */
> +	if (current_NumBoards >= 0) {
> +		ulong time;
> +
> +		spin_lock_irqsave(&jsm_poll_lock, lock_flags);
> +		jsm_poll_time += jsm_jiffies_from_ms(jsm_poll_tick);
> +
> +		time = jsm_poll_time - jiffies;
> +
> +		if ((ulong) time >= 2 * jsm_poll_tick)
> +			jsm_poll_time = jiffies +  jsm_jiffies_from_ms(jsm_poll_tick);
> +
> +		jsm_poll_timer.expires = jsm_poll_time;
> +		spin_unlock_irqrestore(&jsm_poll_lock, lock_flags);
> +
> +		add_timer(&jsm_poll_timer);
> +	}
> +}

Don't use a single timer for all boards.  This makes information 
needlessly global.


> +/*
> + * Start of driver.
> + */
> +static int jsm_start(void)
> +{
> +	int rc = 0;
> +	unsigned long lock_flags;
> +
> +	if (!jsm_driver_start) {
> +		jsm_driver_start = TRUE;

As noted earlier, jsm_driver_start is completely pointless.

jsm_start() is the first function called in module_init(), and is never 
called from anywhere else.


> +		/* make sure that the globals are init'd before we do anything else */
> +		jsm_init_globals();
> +		jsm_NumBoards = 0;
> +		current_NumBoards = -1;
> +
> +		APR(("For the tools package or updated drivers please visit http://www.digi.com\n"));
> +
> +		/*
> +		 * Register our base character device into the kernel.
> +		 * This allows the download daemon to connect to the downld device
> +		 * before any of the boards are init'ed.
> +		 */
> +		if (!jsm_major_control_registered) {
> +			/*
> +			 * Register management/dpa devices
> +			 */
> +			rc = register_chrdev(0, "jsm", &jsm_BoardFops);
> +			if (rc <= 0) {
> +				APR(("Can't register jsm driver device (%d)\n", rc));
> +				return -ENXIO;
> +			}
> +			jsm_Major = rc;
> +			jsm_major_control_registered = TRUE;
> +		}
> +
> +		/*
> +		 * Register our basic stuff in /proc/jsm
> +		 */
> +		jsm_proc_register_basic_prescan();
> +
> +		if (rc < 0) {
> +			APR(("tty preinit - not enough memory (%d)\n", rc));
> +			return rc;
> +		}

Why is this not in sysfs?


> +		/* Start the poller */
> +		spin_lock_irqsave(&jsm_poll_lock, lock_flags);
> +		jsm_poll_time = jiffies + jsm_jiffies_from_ms(jsm_poll_tick);
> +		jsm_poll_timer.expires = jsm_poll_time;
> +		spin_unlock_irqrestore(&jsm_poll_lock, lock_flags);
> +
> +		add_timer(&jsm_poll_timer);

It's silly to add a timer and start polling, when you have nothing to poll.

But that's ok... this code will go away when the poll timer is made 
per-device.


> +		jsm_driver_state = DRIVER_READY;
> +	}
> +	return rc;
> +}
> +
> +static int jsm_finalize_board_init(struct board_t *brd) 
> +{
> +	int rc = 0;
> +
> +	DPR_INIT(("jsm_finalize_board_init() - start\n"));
> +
> +	if (!brd || brd->magic != JSM_BOARD_MAGIC)
> +		return -ENODEV;
> +
> +	DPR_INIT(("jsm_finalize_board_init() - start #2\n"));
> +
> +	if (brd->irq) {
> +		rc = request_irq(brd->irq, brd->bd_ops->intr, SA_INTERRUPT|SA_SHIRQ, "JSM", brd);
> +
> +		if (rc) {
> +			printk(KERN_WARNING "Failed to hook IRQ %d\n",brd->irq);
> +			brd->state = BOARD_FAILED;
> +			brd->dpastatus = BD_NOFEP;
> +			rc = -ENODEV;
> +		} else {
> +			DPR_INIT(("Requested and received usage of IRQ %d\n", brd->irq));
> +		}
> +	}
> +	return rc;
> +}
> +
> +/*
> + * jsm_found_board()
> + *
> + * A board has been found, init it.
> + */
> +static int jsm_found_board(struct pci_dev *pdev, int id)
> +{
> +	struct board_t *brd;
> +	unsigned int pci_irq;
> +	int i = 0;
> +	int rc = 0;
> +	int index;
> +	int wen_board;
> +	int hotplug = 0;
> +
> +	wen_board = jsm_NumBoards;
> +	for (index = 0; index < jsm_NumBoards; index++) {
> +		if (!strcmp(jsm_board_slot[index], pdev->slot_name)) {
> +			wen_board = index;
> +			hotplug = 1;
> +			break;
> +		}
> +	}

It is very wrong to store boards based on pdev->slot_name.

However, this goes away when more state is made per-device.



> +	brd = jsm_Board[wen_board] =
> +	(struct board_t *)kmalloc(sizeof(struct board_t), GFP_KERNEL);
> +	if (!brd) {
> +		APR(("memory allocation for board structure failed\n"));
> +		return -ENOMEM;
> +	}
> +	memset(brd, 0, sizeof(struct board_t));
> +
> +	/* store the info for the board we've found */
> +	brd->magic = JSM_BOARD_MAGIC;
> +	brd->boardnum = wen_board;
> +	brd->vendor = jsm_pci_tbl[id].vendor;
> +	brd->device = jsm_pci_tbl[id].device;
> +	brd->pci_bus = pdev->bus->number;
> +	brd->pci_slot = PCI_SLOT(pdev->devfn);

You should not need this information.


> +	brd->pci_dev = pdev;
> +	brd->name = jsm_Ids[id].name;
> +	brd->maxports = jsm_Ids[id].maxports;
> +	brd->dpastatus = BD_NOFEP;
> +	strcpy(jsm_board_slot[wen_board],pdev->slot_name);
> +	init_waitqueue_head(&brd->state_wait);
> +
> +	spin_lock_init(&brd->bd_lock);
> +	spin_lock_init(&brd->bd_intr_lock);
> +
> +	brd->state = BOARD_FOUND;
> +
> +	for (i = 0; i < MAXPORTS; i++) 
> +		brd->channels[i] = NULL;
> +
> +	/* store which card & revision we have */
> +	pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &brd->subvendor);
> +	pci_read_config_word(pdev, PCI_SUBSYSTEM_ID, &brd->subdevice);

Obtain this information from struct pci_dev.  Do not read it manually.


> +	pci_irq = pdev->irq;
> +	brd->irq = pci_irq;
> +
> +	switch(brd->device) {
> +
> +	case PCI_DEVICE_NEO_4_DID:
> +	case PCI_DEVICE_NEO_8_DID:
> +	case PCI_DEVICE_NEO_2DB9_DID:
> +	case PCI_DEVICE_NEO_2DB9PRI_DID:
> +	case PCI_DEVICE_NEO_2RJ45_DID:
> +	case PCI_DEVICE_NEO_2RJ45PRI_DID:
> +	case PCI_DEVICE_NEO_1_422_DID:
> +	case PCI_DEVICE_NEO_1_422_485_DID:
> +	case PCI_DEVICE_NEO_2_422_485_DID:

use standard pci_ids.h constants.


> +		/*
> +		 * This chip is set up 100% when we get to it.
> +		 * No need to enable global interrupts or anything. 
> +		 */
> +		brd->dpatype = T_NEO | T_PCIBUS;
> +
> +		DPR_INIT(("jsm_found_board - NEO.\n"));
> +
> +		/* get the PCI Base Address Registers */
> +		brd->membase	= pci_resource_start(pdev, 0);
> +		brd->membase_end = pci_resource_end(pdev, 0);
> +
> +		if (brd->membase & 1)
> +			brd->membase &= ~3;
> +		else
> +			brd->membase &= ~15;
> +
> +		/* Assign the board_ops struct */
> +		brd->bd_ops = &jsm_neo_ops;
> +
> +		brd->bd_uart_offset = 0x200;
> +		brd->bd_dividend = 921600;
> +
> +		brd->re_map_membase = ioremap(brd->membase, 0x1000);
> +		DPR_INIT(("remapped mem: 0x%p\n", brd->re_map_membase));
> +		if (!brd->re_map_membase) {
> +			kfree(brd);
> +			APR(("card has no PCI Memory resources, failing board.\n"));
> +			return -ENOMEM;
> +		}
> +		break;
> +
> +	default:
> +		APR(("Did not find any compatible Neo or Classic PCI boards in system.\n"));
> +		kfree(brd);
> +		return -ENXIO;
> +	}
> +
> +	/*
> +	 * Do tty device initialization.
> +	 */
> +	rc = jsm_finalize_board_init(brd);
> +	if (rc < 0) {
> +		APR(("Can't finalize board init (%d)\n", rc));
> +		brd->state = BOARD_FAILED;
> +		brd->dpastatus = BD_NOFEP;
> +		goto failed;
> +	}
> +
> +	rc = jsm_tty_register(brd);
> +	if (rc < 0) {
> +		jsm_tty_uninit(brd);
> +		APR(("Can't register tty devices (%d)\n", rc));
> +		brd->state = BOARD_FAILED;
> +		brd->dpastatus = BD_NOFEP;
> +		goto failed;
> +	}
> +
> +	rc = jsm_tty_init(brd);
> +	if (rc < 0) {
> +		jsm_tty_uninit(brd);
> +		APR(("Can't init tty devices (%d)\n", rc));
> +		brd->state = BOARD_FAILED;
> +		brd->dpastatus = BD_NOFEP;
> +		goto failed;
> +	}
> +
> +	rc = jsm_uart_port_init(brd);
> +	if (rc < 0)
> +		goto failed;
> +
> +	brd->state = BOARD_READY;
> +	brd->dpastatus = BD_RUNNING;
> +
> +	/* init our poll helper tasklet */
> +	tasklet_init(&brd->helper_tasklet, brd->bd_ops->tasklet, (unsigned long) brd);
> +
> +	/* Log the information about the board */
> +	APR(("board %d: %s (rev %d), irq %d\n",wen_board, brd->name, brd->rev, brd->irq));
> +
> +	/*
> +	 * allocate flip buffer for board.
> +	 *
> +	 * Okay to malloc with GFP_KERNEL, we are not at interrupt
> +	 * context, and there are no locks held.
> +	 */
> +	brd->flipbuf = kmalloc(MYFLIPLEN, GFP_KERNEL);
> +	if (!brd->flipbuf) {
> +		APR(("memory allocation for flipbuf failed\n"));
> +		return -ENOMEM;
> +	}

leak on error


> +	memset(brd->flipbuf, 0, MYFLIPLEN);
> +
> +	jsm_proc_register_basic_postscan(wen_board);
> +	wake_up_interruptible(&brd->state_wait);
> +
> +	if (!hotplug)
> +		jsm_NumBoards++;
> +
> +	current_NumBoards++;
> +
> +	return 0;
> +
> +failed:
> +	kfree(brd);
> +	iounmap((void *) brd->re_map_membase);
> +	return -ENXIO;
> +}
> +
> +
> +static int jsm_probe1(struct pci_dev *pdev, int card_type)
> +{
> +	return jsm_found_board(pdev, card_type);
> +}

eliminate this needless function.


> +/* returns count (>= 0), or negative on error */
> +static int jsm_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	int rc;
> +
> +	rc = pci_enable_device(pdev);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Device enable FAILED\n");
> +		return rc;
> +	} 
> +
> +	if ((rc = jsm_probe1(pdev, ent->driver_data))) {
> +		dev_err(&pdev->dev, "jsm_probe1 FAILED\n");
> +		pci_disable_device(pdev);
> +	 	return rc;
> +	}
> +	return rc;
> +}

Missing pci_request_regions().


> +/*
> + * jsm_cleanup_board()
> + *
> + * Free all the memory associated with a board
> + */
> +static void jsm_cleanup_board(struct board_t *brd)
> +{
> +	int i = 0;
> +
> +	if (!brd || brd->magic != JSM_BOARD_MAGIC)
> +		return ;
> +
> +	if (brd->irq)
> +		free_irq(brd->irq, brd);
> +
> +	tasklet_kill(&brd->helper_tasklet);
> +
> +	if (brd->re_map_membase) {
> +		iounmap(brd->re_map_membase);
> +		brd->re_map_membase = NULL;
> +	}

When will this 'if' test ever fail?


> +	/* Free all allocated channels structs */
> +	for (i = 0; i < MAXPORTS; i++) {
> +		if (brd->channels[i]) {
> +			if (brd->channels[i]->ch_rqueue)
> +				kfree(brd->channels[i]->ch_rqueue);
> +			if (brd->channels[i]->ch_equeue)
> +				kfree(brd->channels[i]->ch_equeue);
> +			if (brd->channels[i]->ch_wqueue)
> +				kfree(brd->channels[i]->ch_wqueue);
> +
> +			kfree(brd->channels[i]);
> +			brd->channels[i] = NULL;
> +		}
> +	}
> +
> +	if (brd->flipbuf)
> +		kfree(brd->flipbuf);
> +
> +	jsm_Board[brd->boardnum] = NULL;
> +
> +	kfree(brd);
> +	current_NumBoards--;
> +}
> +
> +static void jsm_remove_one(struct pci_dev *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < jsm_NumBoards; i++) {
> +		if ((jsm_Board[i] != NULL) && (jsm_Board[i]->pci_dev == dev)) {
> +			jsm_proc_unregister_brd(i);
> +			jsm_remove_uart_port(jsm_Board[i]);
> +			jsm_tty_uninit(jsm_Board[i]);
> +			jsm_cleanup_board(jsm_Board[i]);
> +		}
> +	}
> +}

pci_disable_device(), pci_release_regions()


> +/*
> + * jsm_init_module()
> + *
> + * Module load.  This is where it all starts.
> + */
> +static int __init
> +jsm_init_module(void)
> +{
> +	int rc = 0;
> +
> +	APR(("%s, Digi International Part Number %s\n", "jsm: 1.1-1-INKERNEL", "40002438_A-INKERNEL"));
> +
> +	/*
> +	 * Initialize global stuff
> +	 */
> +	rc = jsm_start();
> +	if (rc < 0) 
> +		return rc;
> +
> +	rc = uart_register_driver(&jsm_uart_driver);
> +	if (rc)
> +		return rc;

leak on error


> +	rc = pci_register_driver(&jsm_driver);
> +	if (rc < 0)
> +		uart_unregister_driver(&jsm_uart_driver);

leak on error


> +	return rc;
> +}
> +
> +module_init(jsm_init_module);
> +
> +/*
> + * jsm_exit_module()
> + *
> + * Module unload.  This is where it all ends.
> + */
> +static void __exit
> +jsm_exit_module(void)
> +{
> +	int i;
> +
> +	del_timer_sync(&jsm_poll_timer);
> +
> +	if (jsm_major_control_registered)
> +		unregister_chrdev(jsm_Major, "jsm");
> +
> +	pci_unregister_driver(&jsm_driver);
> +
> +	jsm_proc_unregister_all();
> +
> +	for (i = 0; i < MAXBOARDS; i++) {
> +		jsm_board_slot[i] = NULL;
> +		kfree(jsm_board_slot[i]);
> +	}
> +	uart_unregister_driver(&jsm_uart_driver);
> +}
> +module_exit(jsm_exit_module);
> +MODULE_LICENSE("GPL");
> +
> +/*
> + * jsm_ioctl_name()
> + *
> + * Returns a text version of each ioctl value.
> + */
> +char *jsm_ioctl_name(int cmd)
> +{
> +	switch(cmd) {
> +
> +	case TCGETA:		return("TCGETA");
> +	case TCGETS:		return("TCGETS");
> +	case TCSETA:		return("TCSETA");
> +	case TCSETS:		return("TCSETS");
> +	case TCSETAW:		return("TCSETAW");
> +	case TCSETSW:		return("TCSETSW");
> +	case TCSETAF:		return("TCSETAF");
> +	case TCSETSF:		return("TCSETSF");
> +	case TCSBRK:		return("TCSBRK");
> +	case TCXONC:		return("TCXONC");
> +	case TCFLSH:		return("TCFLSH");
> +	case TIOCGSID:		return("TIOCGSID");
> +
> +	case TIOCGETD:		return("TIOCGETD");
> +	case TIOCSETD:		return("TIOCSETD");
> +	case TIOCGWINSZ:	return("TIOCGWINSZ");
> +	case TIOCSWINSZ:	return("TIOCSWINSZ");
> +
> +	case TIOCMGET:		return("TIOCMGET");
> +	case TIOCMSET:		return("TIOCMSET");
> +	case TIOCMBIS:		return("TIOCMBIS");
> +	case TIOCMBIC:		return("TIOCMBIC");
> +
> +	/* from digi.h */
> +	case DIGI_SETA:		return("DIGI_SETA");
> +	case DIGI_SETAW:	return("DIGI_SETAW");
> +	case DIGI_SETAF:	return("DIGI_SETAF");
> +	case DIGI_SETFLOW:	return("DIGI_SETFLOW");
> +	case DIGI_SETAFLOW:	return("DIGI_SETAFLOW");
> +	case DIGI_GETFLOW:	return("DIGI_GETFLOW");
> +	case DIGI_GETAFLOW:	return("DIGI_GETAFLOW");
> +	case DIGI_GETA:		return("DIGI_GETA");
> +	case DIGI_GEDELAY:	return("DIGI_GEDELAY");
> +	case DIGI_SEDELAY:	return("DIGI_SEDELAY");
> +	case DIGI_GETCUSTOMBAUD: return("DIGI_GETCUSTOMBAUD");
> +	case DIGI_SETCUSTOMBAUD: return("DIGI_SETCUSTOMBAUD");
> +	case TIOCMODG:		return("TIOCMODG");
> +	case TIOCMODS:		return("TIOCMODS");
> +	case TIOCSDTR:		return("TIOCSDTR");
> +	case TIOCCDTR:		return("TIOCCDTR");
> +
> +	default:		return("unknown");
> +	}

Eliminate this, or make it debug-only.

	Jeff

Re: [ patch 1/7] drivers/serial/jsm: new serial device driver

[Jeff Garzik]

Kyle Moffett wrote:
> On Feb 27, 2005, at 18:38, Wen Xiong wrote:
> 
>> +/*
>> + * jsm_init_globals()
>> + *
>> + * This is where we initialize the globals from the static insmod
>> + * configuration variables.  These are declared near the head of
>> + * this file.
>> + */
>> +static void jsm_init_globals(void)
>> +{
>> +    int i = 0;
>> +
>> +    jsm_rawreadok        = rawreadok;
>> +    jsm_trcbuf_size        = trcbuf_size;
>> +    jsm_debug        = debug;
>> +
>> +    for (i = 0; i < MAXBOARDS; i++) {
>> +        jsm_Board[i] = NULL;
>> +        jsm_board_slot[i] = (char *)kmalloc(20, GFP_KERNEL);
>> +        memset(jsm_board_slot[i], 0, 20);
>> +    }
> 
> 
> Instead of several 20-byte kmallocs, you could help reduce memory
> usage and fragmentation with something like this:
> 
> static void *jsm_board_slot_mem;
> 
> jsm_board_slot_mem = kmalloc(20*MAXBOARDS, GFP_KERNEL);
> memset(jsm_board_slot_mem, 0, 20*MAXBOARDS)
> for (i = 0; i < MAXBOARDS; i++) {
>     jsm_Board[i] = NULL;
>     jsm_board_slot[i] = &jsm_board_slot_mem[20*i];
> }
> 
> Then free like this:
> kfree(jsm_board_slot_mem);

Agree with your initial comment, but you missed an overall point: 
MAXBOARDS must be eliminated completely.  We do not impose such limits 
in Linux.

Information should be allocated on a per-device basis.


> On the other hand, it might be nice to have all these structures
> dynamically allocated, so that the no-boards case only uses the 8 or
> 16 bytes of RAM required for a struct list_head.  In that case you
> could clump the other board info into a single struct instead of
> multiple independent static arrays.  Something like this might work:
> 
> struct jsm_board_instance {
>     struct list_head board_list;
>     struct board_t board;
>     char slot[20];
>     [...etc...]
> };
> static struct list_head jsm_board_list = LIST_HEAD_INIT(jsm_board_list);
> static spinlock_t jsm_board_list_lock = SPIN_LOCK_UNLOCKED;
> 
> Then when doing hardware add/remove, take the lock and do the list
> manipulation, then unlock.

Correct-a-mundo!

This is the way it should be done.

	Jeff

Re: [ patch 1/7] drivers/serial/jsm: new serial device driver

[Greg KH]

On Sun, Feb 27, 2005 at 06:38:23PM -0500, Wen Xiong wrote:
> +static struct pci_device_id jsm_pci_tbl[] = {
> +	{	DIGI_VID, PCI_DEVICE_NEO_4_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,		0 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_8_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,		1 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2DB9_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	2 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2DB9PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	3 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2RJ45_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	4 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2RJ45PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	5 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_1_422_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	6 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_1_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	7 },
> +	{	DIGI_VID, PCI_DEVICE_NEO_2_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0,	8 },

Use the PCI_DEVICE() macro to make this smaller.

thanks,

greg k-h