Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver

Hi Grant, John,

On Mon, Jun 30, 2008 at 11:16:28AM -0600, Grant Likely wrote:

On Mon, Jun 30, 2008 at 07:24:48AM -0700, John Linn wrote:

quoted

+config SERIO_XILINX_XPS_PS2
+	tristate "Xilinx XPS PS/2 Controller Support"
+	help
+	  This driver supports XPS PS/2 IP from Xilinx EDK.
+

Consider moving this block to somewhere in the middle of the file to
reduce the possibility of merge conflicts.

I can take care of that, no worries.

quoted

+ *
+ * (c) 2005 MontaVista Software, Inc.
+ * (c) 2008 Xilinx Inc.
+ *
+ * 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 of the License, or (at your
+ * option) any later version.
+ *
+ * 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.

These two paragraphs are redundant.  Being in the Linux source tree
implies that it is GPL licensed.  You can remove them.

I prefer having the statement in right in the code actually. While
being the in kernel implies that the code is GPLv2 compatible it could
be dual-licensed or GPLv2 only. This removes any doubt as to what
license is used on this particular piece of code.

quoted

+ */
+
+
+#include <linux/module.h>
+#include <linux/serio.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <asm/io.h>
+
+#ifdef CONFIG_OF		/* For open firmware */
+ #include <linux/of_device.h>
+ #include <linux/of_platform.h>
+#endif /* CONFIG_OF */

This is a given for mainline since arch/ppc will not exist in 2.6.27

quoted

+
+#include "xilinx_ps2.h"

This header can simple be rolled into this .c file because the driver no
longer has multiple .c files.

quoted

+#define DRIVER_DESCRIPTION	"Xilinx XPS PS/2 driver"
+#define XPS2_NAME_DESC		"Xilinx XPS PS/2 Port #%d"
+#define XPS2_PHYS_DESC		"xilinxps2/serio%d"

These strings are only used in 1 place each, no need to use a #define

quoted

+
+
+static DECLARE_MUTEX(cfg_sem);

This mutex should be part of the driver private data structure

quoted

+
+/*********************/
+/* Interrupt handler */
+/*********************/
+static irqreturn_t xps2_interrupt(int irq, void *dev_id)
+{
+	struct xps2data *drvdata = (struct xps2data *)dev_id;
+	u32 intr_sr;
+	u32 ier;
+	u8 c;
+	u8 retval;
+
+	/* Get the PS/2 interrupts and clear them */
+	intr_sr = in_be32(drvdata->base_address + XPS2_IPISR_OFFSET);
+	out_be32(drvdata->base_address + XPS2_IPISR_OFFSET, intr_sr);
+
+	/* Check which interrupt is active */
+	if (intr_sr & XPS2_IPIXR_RX_OVF) {
+		printk(KERN_ERR "%s: receive overrun error\n",
+			drvdata->serio.name);
+	}
+
+	if (intr_sr & XPS2_IPIXR_RX_ERR) {
+		drvdata->dfl |= SERIO_PARITY;
+	}
+
+	if (intr_sr & (XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_WDT_TOUT)) {
+		drvdata->dfl |= SERIO_TIMEOUT;
+	}
+
+	if (intr_sr & XPS2_IPIXR_RX_FULL) {
+		retval = xps2_recv(drvdata, &drvdata->rxb);
+
+		/* Error, if 1 byte is not received */
+		if (retval != 1) {
+			printk(KERN_ERR
+				"%s: wrong rcvd byte count (%d)\n",
+				drvdata->serio.name, retval);

Don't you want to bail out here? Otherwise you will feed garbage to
serio_interrupt() I think.

quoted

+		}
+		c = drvdata->rxb;
+		serio_interrupt(&drvdata->serio, c, drvdata->dfl);
+		drvdata->dfl = 0;
+	}
+
+	if (intr_sr & XPS2_IPIXR_TX_ACK) {
+
+		/* Disable the TX interrupts after the transmission is
+		 * complete */
+		ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
+		ier &= (~(XPS2_IPIXR_TX_ACK & XPS2_IPIXR_ALL ));
+		out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
+		drvdata->dfl = 0;
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*******************/
+/* serio callbacks */
+/*******************/
+
+/*
+ * sxps2_write() sends a byte out through the PS/2 interface.
+ *
+ * The sole purpose of drvdata->tx_end is to prevent the driver
+ * from locking up in the do {} while; loop when nothing is connected
+ * to the given PS/2 port. That's why we do not try to recover
+ * from the transmission failure.
+ * drvdata->tx_end needs not to be initialized to some "far in the
+ * future" value, as the very first attempt to xps2_send() a byte
+ * is always successful, and drvdata->tx_end will be set to a proper
+ * value at that moment - before the 1st use in the comparison.
+ */

Good comment block.

nitpick: can you reformat the comment blocks to be in kerneldoc format?
That will allow the automatic document generation tools to parse it.

see: Documentation/kernel-doc-nano-HOWTO.txt

This is an internal function so its not going to be exposed in
kerneldoc though.

quoted

+static int sxps2_write(struct serio *pserio, unsigned char c)
+{
+	struct xps2data *drvdata = pserio->port_data;
+	unsigned long flags;
+	int retval;
+
+	do {
+		spin_lock_irqsave(&drvdata->lock, flags);
+		retval = xps2_send(drvdata, &c);
+		spin_unlock_irqrestore(&drvdata->lock, flags);
+
+		if (retval == 1) {
+			drvdata->tx_end = jiffies + HZ;
+			return 0;	/* success */
+		}
+	} while (!time_after(jiffies, drvdata->tx_end));

The logic escapes me... Let's say you send a byte and time when
jiffies were 10000 and now it is time 20000 and we try to send another
byte. Our first attempt fails and with time fence 11000 (HZ=1000) we
bail out of sxps2_write() after the very first unsiccessful attempt.
Is this what you intended?

quoted

+
+	return 1;			/* transmission is frozen */

It is better to return a negative on error, even if you don't report
actual -EXXXX error code.

-- 
Dmitry