[PATCH] linux-2.6.2-rc3 Add icom serial device driver

[PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Michael Anderson]

I am posting the icom serial device driver code and supporting kernel
changes for submission to the linux kernel.  The icom serial
device driver enables linux to use a family of IBM iSeries serial
adapters.  The serial adapters supported by icom are intelligent adapters
that provide multiport serial access via iSeries RS232 cables on some
adapters or via internal modems on other adapters.  Today the icom device
driver only uses the family of adapters for asynchronous communications.

Please review this code for submission.

Please respond cc to mjanders@us.ibm.com.

The patch can be found at:

http://www-124.ibm.com/linux/patches/misc/icom-2.6.2-rc3.notrace.patch

Re: [PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Greg KH]

On Fri, Feb 06, 2004 at 02:12:28PM -0600, Michael Anderson wrote:
> I am posting the icom serial device driver code and supporting kernel
> changes for submission to the linux kernel.  The icom serial
> device driver enables linux to use a family of IBM iSeries serial
> adapters.  The serial adapters supported by icom are intelligent adapters
> that provide multiport serial access via iSeries RS232 cables on some
> adapters or via internal modems on other adapters.  Today the icom device
> driver only uses the family of adapters for asynchronous communications.
> 
> Please review this code for submission.

Why not include the patch in this message?  Putting patches on web sites
is a tough way to get your code reviewed by the community.

And why not forward port it to 2.6.2?  2.6.2-rc3 is a old kernel version
:)

thanks,

greg k-h

Re: [PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Michael Anderson]

>Why not include the patch in this message?  Putting patches on web sites
>is a tough way to get your code reviewed by the community.

SubmittingPatches states a 40 kB files size limit to post directly to
boards otherwise an Internet-accessible server should be use.

>And why not forward port it to 2.6.2?  2.6.2-rc3 is a old kernel version
:)

I just downloaded 2.6.2 and this patch applies and builds well against
2.6.2.  I will do functional testing against 2.6.2.

Re: [PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Russell King]

On Fri, Feb 06, 2004 at 02:12:28PM -0600, Michael Anderson wrote:
> Please review this code for submission.

Some comments... I won't say they're complete, but they're the
things which stood out.

diff -Nur orig/Documentation/icom.txt devel/Documentation/icom.txt
--- orig/Documentation/icom.txt	1969-12-31 18:00:00.000000000 -0600
+++ devel/Documentation/icom.txt	2004-02-03 17:35:31.000000000 -0600

Why not place this in the Documentation/serial subdirectory?

+#define ASYNC_CLOSING    0x08000000	/* Serial port is closing */
+#define ASYNC_HUP_NOTIFY 0x0001	/* Notify getty on hangups and closes 
+				   on the callout port */

These are unused, please remove them.

+	      vendor:PCI_VENDOR_ID_IBM,
+	      device:ICOM_DEV_ID_1,
+	      subvendor:PCI_ANY_ID,
+	      subdevice:PCI_ANY_ID,
+	      driver_data:ADAPTER_V1,

This should use C99 initialisers. IOW:
		.vendor = PCI_VENDOR_ID_IBM,
etc

+LIST_HEAD(icom_adapter_head);

Shouldn't this be prefixed by "static" ?

+static int startup(struct icom_port *icom_port)
...
+	spin_lock_irqsave(&icom_lock, flags);
...
+		icom_port->load_in_progress = 1;
+		spin_unlock_irq(&icom_lock);
+
+		load_code(icom_port);
+
+		spin_lock_irq(&icom_lock);
+		icom_port->load_in_progress = 0;
...
+	spin_unlock_irqrestore(&icom_lock, flags);

spin_unlock_irq() after spin_lock_irqsave() is potentially unsafe.
Also, the locking there is not necessary - opens and closes are
already serialised with each other and themselves by the serial_core
layer.

+	status = readb(&ICOM_PORT->dram->isr);
+	control = readb(&ICOM_PORT->dram->osr);
+
+	result = ((status & ICOM_DCD) ? TIOCM_CAR : 0)
+	    | ((status & ICOM_RI) ? TIOCM_RNG : 0)
+	    | ((status & ICOM_DSR) ? TIOCM_DSR : 0)
+	    | ((status & ICOM_CTS) ? TIOCM_CTS : 0);
+	return result;
+}

Hmm, you never seem to use "control".

+static struct uart_driver icom_uart_driver = {
+	.owner = THIS_MODULE,
+	.driver_name = ICOM_DRIVER_NAME,
+	.dev_name = "ttyS",
+	.major = ICOM_MAJOR,
+	.minor = ICOM_MINOR_START,
+	.nr = NR_PORTS,
+	.cons = ICOM_CONSOLE,
+};

+#define ICOM_MAJOR       TTY_MAJOR
+#define ICOM_MINOR_START 64

I take it you'll never ever have one of these adapters in a machine
which also has standard PC serial ports as well?  Since you're using
the same major/minor numbers as the standard PC serial driver, it
only allows one or the other driver to be loaded at any one time.

+static int __init icom_init(void)
+{
+	int ret;
+
+	spin_lock_init(&icom_lock);
+
+	ret = uart_register_driver(&icom_uart_driver);
+	if (ret)
+		return ret;
+
+	pci_register_driver(&icom_pci_driver);
+
+	return 0;
+}

What if pci_register_driver() fails?

+static void __exit icom_exit(void)
+{
+	struct icom_adapter *icom_adapter;
+	struct list_head *tmp;
+
+	pci_unregister_driver(&icom_pci_driver);
+
+	while (1) {
+		list_for_each(tmp, &icom_adapter_head) {
+			icom_adapter = list_entry(tmp, struct icom_adapter,
+						  icom_adapter_entry);
+			icom_remove_adapter(icom_adapter);
+			break;
+		}
+		break;
+	}
+
+	uart_unregister_driver(&icom_uart_driver);
+}

Is it really necessary to loop over the list of adapters?
pci_unregister_driver() will call the remove method for each PCI device
which was assigned to this driver, so your icom_remove function should
have already removed all instances.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Michael Anderson]

>diff -Nur orig/Documentation/icom.txt devel/Documentation/icom.txt
>--- orig/Documentation/icom.txt           1969-12-31 18:00:00.000000000
-0600
>+++ devel/Documentation/icom.txt          2004-02-03 17:35:31.000000000
-0600
>
>Why not place this in the Documentation/serial subdirectory?

Yes, I will move icom.txt to the serial subdirectory.

>+#define ASYNC_CLOSING    0x08000000            /* Serial port is closing
*/
>+#define ASYNC_HUP_NOTIFY 0x0001          /* Notify getty on hangups and
closes
>+                                                  on the callout port */
>
>These are unused, please remove them.

They will be removed.

>+                 vendor:PCI_VENDOR_ID_IBM,
>+                 device:ICOM_DEV_ID_1,
>+                 subvendor:PCI_ANY_ID,
>+                 subdevice:PCI_ANY_ID,
>+                 driver_data:ADAPTER_V1,
>
>This should use C99 initialisers. IOW:
>                        .vendor = PCI_VENDOR_ID_IBM,
>etc

I will update as suggested.

>+LIST_HEAD(icom_adapter_head);
>
>Shouldn't this be prefixed by "static" ?

yes.

>+static int startup(struct icom_port *icom_port)
>...
>+           spin_lock_irqsave(&icom_lock, flags);
>...
>+                       icom_port->load_in_progress = 1;
>+                       spin_unlock_irq(&icom_lock);
>+
>+                       load_code(icom_port);
>+
>+                       spin_lock_irq(&icom_lock);
>+                       icom_port->load_in_progress = 0;
>...
>+           spin_unlock_irqrestore(&icom_lock, flags);
>
>spin_unlock_irq() after spin_lock_irqsave() is potentially unsafe.
>Also, the locking there is not necessary - opens and closes are
>already serialised with each other and themselves by the serial_core
>layer.

The spin locks will be removed.

>+           status = readb(&ICOM_PORT->dram->isr);
>+           control = readb(&ICOM_PORT->dram->osr);
>+
>+           result = ((status & ICOM_DCD) ? TIOCM_CAR : 0)
>+               | ((status & ICOM_RI) ? TIOCM_RNG : 0)
>+               | ((status & ICOM_DSR) ? TIOCM_DSR : 0)
>+               | ((status & ICOM_CTS) ? TIOCM_CTS : 0);
>+           return result;
>+}
>
>Hmm, you never seem to use "control".

Line of code and variable definition will be removed.

>+static struct uart_driver icom_uart_driver = {
>+           .owner = THIS_MODULE,
>+           .driver_name = ICOM_DRIVER_NAME,
>+           .dev_name = "ttyS",
>+           .major = ICOM_MAJOR,
>+           .minor = ICOM_MINOR_START,
>+           .nr = NR_PORTS,
>+           .cons = ICOM_CONSOLE,
>+};
>
>+#define ICOM_MAJOR       TTY_MAJOR
>+#define ICOM_MINOR_START 64
>
>I take it you'll never ever have one of these adapters in a machine
>which also has standard PC serial ports as well?  Since you're using
>the same major/minor numbers as the standard PC serial driver, it
>only allows one or the other driver to be loaded at any one time.

The icom driver is intended for iSeries platform linux which does
not have a standard serial port.  Ideally, I believe it would be
best if icom had its own MAJOR number.

>+static int __init icom_init(void)
>+{
>+           int ret;
>+
>+           spin_lock_init(&icom_lock);
>+
>+           ret = uart_register_driver(&icom_uart_driver);
>+           if (ret)
>+                       return ret;
>+
>+           pci_register_driver(&icom_pci_driver);
>+
>+           return 0;
>+}
>
>What if pci_register_driver() fails?

I will update to return failure code of pci_register_driver when
failure occurs.

>+static void __exit icom_exit(void)
>+{
>+           struct icom_adapter *icom_adapter;
>+           struct list_head *tmp;
>+
>+           pci_unregister_driver(&icom_pci_driver);
>+
>+           while (1) {
>+                       list_for_each(tmp, &icom_adapter_head) {
>+                                   icom_adapter = list_entry(tmp, struct
icom_adapter,
>+
icom_adapter_entry);
>+                                   icom_remove_adapter(icom_adapter);
>+                                   break;
>+                       }
>+                       break;
>+           }
>+
>+           uart_unregister_driver(&icom_uart_driver);
>+}
>
>Is it really necessary to loop over the list of adapters?
>pci_unregister_driver() will call the remove method for each PCI device
>which was assigned to this driver, so your icom_remove function should
>have already removed all instances.

Correct, this should be unreachable code, I will remove it.

Re: [PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Michael Anderson]

Updated icom patch can be found at:

http://www-124.ibm.com/linux/patches/misc/icom-2.6.3-rc2.patch

Re: [PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Russell King]

On Thu, Feb 12, 2004 at 03:44:35PM -0600, Michael Anderson wrote:
> Updated icom patch can be found at:
> 
> http://www-124.ibm.com/linux/patches/misc/icom-2.6.3-rc2.patch

I was just giving this it's final going over, and I noticed the following
in icom_init():

+	if (ret < 0) {
+		pci_unregister_driver(&icom_pci_driver);
+		uart_unregister_driver(&icom_uart_driver);
+	}

If pci_register_driver() fails in 2.6, you must not call
pci_unregister_driver.

I think there may be issues concerning the firmware in the .h file.
Can you give any information about the licensing policy on it please?
It may need to use the kernels firmware loader (for more info on that
see drivers/base/firmware_class.c)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH] linux-2.6.2-rc3 Add icom serial device driver

[Michael Anderson]

An updated icom patch will be found at:

http://www-124.ibm.com/linux/patches/misc/icom-2.6.4-rc2.patch

This includes the rework which removes the firmware images from the icom.h
and uses the request_firmware interface to load the firmware images during
startup time.