Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Christoph Hellwig <hch@infradead.org>]

> I cleaned this up a bit.  There still are a couple of issues. One is
> that the sgiioc4 driver also "uses" the ioc4 card for ide. So the probe
> needs to "fail" so that both driver's probes will be called (is there a
> better way?). Because of that the ->remove function isn't called
> directly.

So both claim the same PCI ID?  In this case you need to creat a small
shim driver that exports a pseudo-bus to the serial and ide driver using
the driver model.  You must never return an error from ->probe if you
actually use that particular device.

> I still save off the pci_dev ptrs for all cards found, so I can
> register with the serial core after probe (is there a better way?).
> Should I register the driver separately for each card ? That seems a
> bit overkill.

You should register them with the serial core in ->probe.

+#include <asm/sn/ioc4.h>

Please put that header into drivers/serial/ioc4.h or if you need it from
other drivers in the future to include/linux/ioc4.h

+/* Various states/options */
+#define ENABLE_FLOW_CONTROL
+#define ENABLE_OUTPUT_INTERRUPTS

So why do you need these ifdefs?  Unless there's a very good reason
kill all these conditionals.

+/* To use dynamic numbers only and not use the assigned major and minor,
+ * define the following.. */
+#define USE_DYNAMIC_MINOR 0	/* Don't rely on misc_register dynamic minor */
+static struct miscdevice misc;	/* used with misc_register for dynamic */

Dito.  Please kill the miscdevice code as you seem to have an assigned
number.

+/* defining this will force the driver to run in polled mode */
+//#define POLLING_FOR_CHARACTERS

Again, what's the need for these conditionals?

+/* Infinite loop detection.
+ */
+#define MAXITER 10000000
+#define SPIN(cond, success) \
+{ \
+	 int spiniter = 0; \
+	 success = 1; \
+	 while(cond) { \
+		 spiniter++; \
+		 if (spiniter > MAXITER) { \
+			 success = 0; \
+			 break; \
+		 } \
+	 } \
+}

Opencoding this in the callers would make the code a lot more readable.

+static struct pci_dev *Pdevs[IOC4_NUM_CARDS];

As mentioned above this shouldn't be needed when the driver uses
proper attachment.

+/* a table to keep the card names as passed to request_region */
+static struct {
+	char c_name[20];
+} Table_o_cards[IOC4_NUM_CARDS];

Completely superflous.  Just pass "ioc4_serial" as argument to request_region.

+/* Prototypes */
+static void ioc4_cb_output_lowat(struct ioc4_port *);
+static void ioc4_cb_post_ncs(struct uart_port *, int);
+static void receive_chars(struct uart_port *);
+static void handle_intr(void *arg, uint32_t sio_ir);

Please try to avoid forward-declarations where possible.

+
+/*
+ * support routines for local atomic operations.
+ */
+
+static spinlock_t local_lock;
+
+static inline unsigned int atomicClearInt(atomic_t * a, unsigned int b)
+{
+	unsigned long s;
+	unsigned int ret, new;
+
+	spin_lock_irqsave(&local_lock, s);
+	new = ret = atomic_read(a);
+	new &= ~b;
+	atomic_set(a, new);
+	spin_unlock_irqrestore(&local_lock, s);
+
+	return ret;
+}

There's only a singler caller, so better open-code it with a per-device
lock and avoid the bogus atomic_t use.  It looks like using bitops.h
functions would help that code aswell.

+static inline void
+write_ireg(struct ioc4_soft *ioc4_soft, uint32_t val, int which, int type)
+{
+	struct ioc4_mem *mem = ioc4_soft->is_ioc4_mem_addr;
+	spinlock_t *lp = &ioc4_soft->is_ir_lock;

small style nitpick: In general we try to avoid using spinlock_t pointers
just as local variables as that makes it more clear what's actually locked.

+	unsigned long s;

normally we call that variable flags.  Doing so aswell  here makes the
code easier to read.

+	spin_lock_irqsave(lp, s);
+
+	switch (type) {
+	case IOC4_SIO_INTR_TYPE:
+		switch (which) {
+		case IOC4_W_IES:
+			writel(val, (void *)&mem->sio_ies_ro);

The second argumnet to writeX (and readX) is actually void __iomem *,
but to see the difference you need to run sparse (from sparse.bkbits.net)
over the driver.  Please store all I/O addresses in void __iomem * pointers
in your structures and avoid the cast here and in all the other places.

Remember that in 90% of the cases a cast hides a possible bug.

+		return (1);

Please avoid braces around the return values.

+	if (ioc4_revid < ioc4_revid_min) {
+		printk(KERN_WARNING
+		    "IOC4 serial not supported on firmware rev %d on card %d, "
+				"please upgrade to rev %d or higher\n",
+				ioc4_revid, card_number, ioc4_revid_min);
+		return -1;

Please return meaninfull values from errno.h

+		port = (struct ioc4_port *)kmalloc(sizeof(struct ioc4_port),
+								GFP_KERNEL);

no need to cast the return value from kmalloc (dito for the other places)

+	if (pci_enable_device(pdev)) {

pci_enable_device returns an detailed error, please pass it on to the
caller.

+	/* Map in the ioc4 memory */
+	mem = (struct ioc4_mem *)pci_resource_start(pdev, 0);

You're missing an ioremap somewhere.

+	pdev->dev.driver_data = (void *)control;

please use pci_set_drvdata, the cast isn't needed here aswell.

+	control = (struct ioc4_control *)pdev->dev.driver_data;

please use pca_get_drvdata, again no need for a cast.

+	/* do the pci probing */
+	pci_register_driver(&ioc4_s_driver);

You need to check the return value from pci_register_driver.

+		if (uart_register_driver(&ioc4_uart) < 0) {

Please call uart_register_driver before calling pci_register_driver
so you can register the ports from ->probe.

+	spin_lock_irqsave(&IOC4_lock, flags);
+#ifdef POLLING_FOR_CHARACTERS
+	del_timer_sync(&IOC4_timer_list);
+#endif

del_timer_sync can sleep and must not be called inside a spinlock.

+	pci_unregister_driver(&ioc4_s_driver);

dito for pci_unregister_driver.