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

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

[John Linn]

Added a new driver for Xilinx XPS PS2 IP. This driver is
a flat driver to better match the Linux driver pattern.

Signed-off-by: Sadanand <sadanan@xilinx.com>
Signed-off-by: John Linn <john.linn@xilinx.com>
---

V2

Changes from v1:

Ran the scripts/checkpatch.pl on the patch as I should have done
before sending it (thanks to Stephen).

 drivers/input/serio/Kconfig      |    5 +
 drivers/input/serio/Makefile     |    1 +
 drivers/input/serio/xilinx_ps2.c |  460 ++++++++++++++++++++++++++++++++++++++
 drivers/input/serio/xilinx_ps2.h |   97 ++++++++
 4 files changed, 563 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/serio/xilinx_ps2.c
 create mode 100644 drivers/input/serio/xilinx_ps2.h

diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index ec4b661..0e62b39 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -190,4 +190,9 @@ config SERIO_RAW
 	  To compile this driver as a module, choose M here: the
 	  module will be called serio_raw.
 
+config SERIO_XILINX_XPS_PS2
+	tristate "Xilinx XPS PS/2 Controller Support"
+	help
+	  This driver supports XPS PS/2 IP from Xilinx EDK.
+
 endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 38b8868..9b6c813 100644
--- a/drivers/input/serio/Makefile
+++ b/drivers/input/serio/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_SERIO_PCIPS2)	+= pcips2.o
 obj-$(CONFIG_SERIO_MACEPS2)	+= maceps2.o
 obj-$(CONFIG_SERIO_LIBPS2)	+= libps2.o
 obj-$(CONFIG_SERIO_RAW)		+= serio_raw.o
+obj-$(CONFIG_SERIO_XILINX_XPS_PS2)	+= xilinx_ps2.o
diff --git a/drivers/input/serio/xilinx_ps2.c b/drivers/input/serio/xilinx_ps2.c
new file mode 100644
index 0000000..d368827
--- /dev/null
+++ b/drivers/input/serio/xilinx_ps2.c
@@ -0,0 +1,460 @@
+/*
+ * xilinx_ps2.c
+ *
+ * Xilinx PS/2 driver to interface PS/2 component to Linux
+ *
+ * Author: MontaVista Software, Inc.
+ *	   source@mvista.com
+ *
+ * (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.
+ */
+
+
+#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 <linux/io.h>
+
+#ifdef CONFIG_OF		/* For open firmware */
+ #include <linux/of_device.h>
+ #include <linux/of_platform.h>
+#endif /* CONFIG_OF */
+
+#include "xilinx_ps2.h"
+
+#define DRIVER_NAME		"xilinx_ps2"
+#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"
+
+
+static DECLARE_MUTEX(cfg_sem);
+
+/*********************/
+/* 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);
+		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.
+ */
+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));
+
+	return 1;			/* transmission is frozen */
+}
+
+/*
+ * sxps2_open() is called when a port is open by the higher layer.
+ */
+static int sxps2_open(struct serio *pserio)
+{
+	struct xps2data *drvdata = pserio->port_data;
+	int retval;
+
+	retval = request_irq(drvdata->irq, &xps2_interrupt, 0,
+				DRIVER_NAME, drvdata);
+	if (retval) {
+		printk(KERN_ERR
+			"%s: Couldn't allocate interrupt %d\n",
+			drvdata->serio.name, drvdata->irq);
+		return retval;
+	}
+
+	/* start reception by enabling the interrupts */
+	out_be32(drvdata->base_address + XPS2_GIER_OFFSET, XPS2_GIER_GIE_MASK);
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, XPS2_IPIXR_RX_ALL);
+	(void)xps2_recv(drvdata, &drvdata->rxb);
+
+	return 0;		/* success */
+}
+
+/*
+ * sxps2_close() frees the interrupt.
+ */
+static void sxps2_close(struct serio *pserio)
+{
+	struct xps2data *drvdata = pserio->port_data;
+
+	/* Disable the PS2 interrupts */
+	out_be32(drvdata->base_address + XPS2_GIER_OFFSET, 0x00);
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0x00);
+	free_irq(drvdata->irq, drvdata);
+}
+
+/*************************/
+/* XPS PS/2 driver calls */
+/*************************/
+
+/*
+ * xps2_initialize() initializes the Xilinx PS/2 device.
+ */
+static int xps2_initialize(struct xps2data *drvdata)
+{
+	/* Disable all the interrupts just in case */
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0);
+
+	/* Reset the PS2 device and abort any current transaction, to make sure
+	 * we have the PS2 in a good state */
+	out_be32(drvdata->base_address + XPS2_SRST_OFFSET, XPS2_SRST_RESET);
+
+	return 0;
+}
+
+/*
+ * xps2_send() sends the specified byte of data to the PS/2 port in interrupt
+ * mode.
+ */
+static u8 xps2_send(struct xps2data *drvdata, u8 *byte)
+{
+	u32 sr;
+	u32 ier;
+	u8 retval = 0;
+
+	/* Enter a critical region by disabling the PS/2 transmit interrupts to
+	 * allow this call to stop a previous operation that may be interrupt
+	 * driven. Only stop the transmit interrupt since this critical region
+	 * is not really exited in the normal manner */
+	ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
+	ier &= (~(XPS2_IPIXR_TX_ALL & XPS2_IPIXR_ALL));
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
+
+	/* If the PS/2 transmitter is empty send a byte of data */
+	sr = in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
+	if ((sr & XPS2_STATUS_TX_FULL) == 0) {
+		out_be32(drvdata->base_address + XPS2_TX_DATA_OFFSET, *byte);
+		retval = 1;
+	}
+
+	/* Enable the TX interrupts to track the status of the transmission */
+	ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
+	ier |= ((XPS2_IPIXR_TX_ALL | XPS2_IPIXR_WDT_TOUT));
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
+
+	return retval;		/* no. of bytes sent */
+}
+
+/*
+ * xps2_recv() will attempt to receive a byte of data from the PS/2 port.
+ */
+static u8 xps2_recv(struct xps2data *drvdata, u8 *byte)
+{
+	u32 sr;
+	u8 retval = 0;
+
+	/* If there is data available in the PS/2 receiver, read it */
+	sr = in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
+	if (sr & XPS2_STATUS_RX_FULL) {
+		*byte = in_be32(drvdata->base_address + XPS2_RX_DATA_OFFSET);
+		retval = 1;
+	}
+
+	return retval;		/* no. of bytes received */
+}
+
+/******************************/
+/* The platform device driver */
+/******************************/
+
+static int xps2_probe(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	struct resource *irq_res = NULL;	/* Interrupt resources */
+	struct resource *regs_res = NULL;	/* IO mem resources */
+
+	if (!dev) {
+		dev_err(dev, "Probe called with NULL param\n");
+		return -EINVAL;
+	}
+
+	/* Find irq number, map the control registers in */
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	return xps2_setup(dev, pdev->id, regs_res, irq_res);
+}
+
+/*
+ * Shared device initialization code.
+ */
+static int xps2_setup(struct device *dev, int id, struct resource *regs_res,
+			struct resource *irq_res)
+{
+	struct xps2data *drvdata;
+	unsigned long remap_size;
+	int retval;
+
+	if (!dev)
+		return -EINVAL;
+
+	drvdata = kzalloc(sizeof(struct xps2data), GFP_KERNEL);
+	if (!drvdata) {
+		dev_err(dev, "Couldn't allocate device private record\n");
+		return -ENOMEM;
+	}
+	spin_lock_init(&drvdata->lock);
+	dev_set_drvdata(dev, (void *)drvdata);
+
+	if (!regs_res || !irq_res) {
+		dev_err(dev, "IO resource(s) not found\n");
+		retval = -EFAULT;
+		goto failed1;
+	}
+
+	drvdata->irq = irq_res->start;
+	remap_size = regs_res->end - regs_res->start + 1;
+	if (!request_mem_region(regs_res->start, remap_size, DRIVER_NAME)) {
+
+		dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
+			(unsigned int)regs_res->start);
+		retval = -EBUSY;
+		goto failed1;
+	}
+
+	/* Fill in configuration data and add them to the list */
+	drvdata->phys_addr = regs_res->start;
+	drvdata->remap_size = remap_size;
+	drvdata->base_address = ioremap(regs_res->start, remap_size);
+	if (drvdata->base_address == NULL) {
+
+		dev_err(dev, "Couldn't ioremap memory at 0x%08X\n",
+			(unsigned int)regs_res->start);
+		retval = -EFAULT;
+		goto failed2;
+	}
+
+	/* Initialize the PS/2 interface */
+	down(&cfg_sem);
+	if (xps2_initialize(drvdata)) {
+		up(&cfg_sem);
+		dev_err(dev, "Could not initialize device\n");
+		retval = -ENODEV;
+		goto failed3;
+	}
+	up(&cfg_sem);
+
+	dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=%d\n",
+		drvdata->phys_addr, (u32)drvdata->base_address, drvdata->irq);
+
+	drvdata->serio.id.type = SERIO_8042;
+	drvdata->serio.write = sxps2_write;
+	drvdata->serio.open = sxps2_open;
+	drvdata->serio.close = sxps2_close;
+	drvdata->serio.port_data = drvdata;
+	drvdata->serio.dev.parent = dev;
+	snprintf(drvdata->serio.name, sizeof(drvdata->serio.name),
+		 XPS2_NAME_DESC, id);
+	snprintf(drvdata->serio.phys, sizeof(drvdata->serio.phys),
+		 XPS2_PHYS_DESC, id);
+	serio_register_port(&drvdata->serio);
+
+	return 0;		/* success */
+
+failed3:
+	iounmap(drvdata->base_address);
+
+failed2:
+	release_mem_region(regs_res->start, remap_size);
+
+failed1:
+	kfree(drvdata);
+	dev_set_drvdata(dev, NULL);
+
+	return retval;
+}
+
+/*
+ * xps2_remove() dissociates the driver with the Xilinx PS/2 device.
+ */
+static int xps2_remove(struct device *dev)
+{
+	struct xps2data *drvdata;
+
+	if (!dev)
+		return -EINVAL;
+
+	drvdata = (struct xps2data *)dev_get_drvdata(dev);
+
+	serio_unregister_port(&drvdata->serio);
+
+	iounmap(drvdata->base_address);
+
+	release_mem_region(drvdata->phys_addr, drvdata->remap_size);
+
+	kfree(drvdata);
+	dev_set_drvdata(dev, NULL);
+
+	return 0;		/* success */
+}
+
+static struct device_driver xps2_driver = {
+	.name = DRIVER_NAME,
+	.bus = &platform_bus_type,
+	.probe = xps2_probe,
+	.remove = xps2_remove
+};
+
+#ifdef CONFIG_OF
+static int __devinit xps2_of_probe(struct of_device *ofdev,
+					const struct of_device_id *match)
+{
+	struct resource r_irq_struct;
+	struct resource r_mem_struct;
+	struct resource *r_irq = &r_irq_struct;	/* Interrupt resources */
+	struct resource *r_mem = &r_mem_struct;	/* IO mem resources */
+	int rc = 0;
+	const unsigned int *id;
+
+	printk(KERN_INFO "Device Tree Probing \'%s\'\n",
+			ofdev->node->name);
+
+	/* Get iospace for the device */
+	rc = of_address_to_resource(ofdev->node, 0, r_mem);
+	if (rc) {
+		dev_warn(&ofdev->dev, "invalid address\n");
+		return rc;
+	}
+
+	/* Get IRQ for the device */
+	rc = of_irq_to_resource(ofdev->node, 0, r_irq);
+	if (rc == NO_IRQ) {
+		dev_warn(&ofdev->dev, "no IRQ found\n");
+		return rc;
+	}
+
+	id = of_get_property(ofdev->node, "port-number", NULL);
+	return xps2_setup(&ofdev->dev, id ? *id : -1, r_mem, r_irq);
+}
+
+static int __devexit xps2_of_remove(struct of_device *dev)
+{
+	return xps2_remove(&dev->dev);
+}
+
+static struct of_device_id __devinitdata xps2_of_match[] = {
+	{ .compatible = "xlnx,xps-ps2-1.00.a", },
+	{ /* end of list */ },
+};
+
+MODULE_DEVICE_TABLE(of, xps2_of_match);
+
+static struct of_platform_driver xps2_of_driver = {
+	.name		= DRIVER_NAME,
+	.match_table	= xps2_of_match,
+	.probe		= xps2_of_probe,
+	.remove		= __devexit_p(xps2_of_remove),
+};
+#endif /* CONFIG_OF */
+
+static int __init xps2_init(void)
+{
+	int status = driver_register(&xps2_driver);
+#ifdef CONFIG_OF
+	status |= of_register_platform_driver(&xps2_of_driver);
+#endif /* CONFIG_OF */
+	return status;
+}
+
+static void __exit xps2_cleanup(void)
+{
+	driver_unregister(&xps2_driver);
+#ifdef CONFIG_OF
+	of_unregister_platform_driver(&xps2_of_driver);
+#endif /* CONFIG_OF */
+}
+
+module_init(xps2_init);
+module_exit(xps2_cleanup);
+
+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
+MODULE_LICENSE("GPL");
+
diff --git a/drivers/input/serio/xilinx_ps2.h b/drivers/input/serio/xilinx_ps2.h
new file mode 100644
index 0000000..4db73ca
--- /dev/null
+++ b/drivers/input/serio/xilinx_ps2.h
@@ -0,0 +1,97 @@
+/*****************************************************************************
+ *
+ *     Author: 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.
+ *
+ *     XILINX IS PROVIDING THIS DESIGN, CODE, OR INFORMATION "AS IS"
+ *     AS A COURTESY TO YOU, SOLELY FOR USE IN DEVELOPING PROGRAMS AND
+ *     SOLUTIONS FOR XILINX DEVICES.  BY PROVIDING THIS DESIGN, CODE,
+ *     OR INFORMATION AS ONE POSSIBLE IMPLEMENTATION OF THIS FEATURE,
+ *     APPLICATION OR STANDARD, XILINX IS MAKING NO REPRESENTATION
+ *     THAT THIS IMPLEMENTATION IS FREE FROM ANY CLAIMS OF INFRINGEMENT,
+ *     AND YOU ARE RESPONSIBLE FOR OBTAINING ANY RIGHTS YOU MAY REQUIRE
+ *     FOR YOUR IMPLEMENTATION.  XILINX EXPRESSLY DISCLAIMS ANY
+ *     WARRANTY WHATSOEVER WITH RESPECT TO THE ADEQUACY OF THE
+ *     IMPLEMENTATION, INCLUDING BUT NOT LIMITED TO ANY WARRANTIES OR
+ *     REPRESENTATIONS THAT THIS IMPLEMENTATION IS FREE FROM CLAIMS OF
+ *     INFRINGEMENT, IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ *     FOR A PARTICULAR PURPOSE.
+ *
+ *     Xilinx products are not intended for use in life support appliances,
+ *     devices, or systems. Use in such applications is expressly prohibited.
+ *
+ *     (c) Copyright 2008 Xilinx Inc.
+ *     All rights reserved.
+ *
+ *     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.
+ *
+ *****************************************************************************/
+
+#ifndef XILINX_PS2_H_	/* prevent circular inclusions */
+#define XILINX_PS2_H_	/* by using protection macros */
+
+/* Register offsets for the xps2 device */
+#define XPS2_SRST_OFFSET	0x00000000 /* Software Reset register */
+#define XPS2_STATUS_OFFSET	0x00000004 /* Status register */
+#define XPS2_RX_DATA_OFFSET	0x00000008 /* Receive Data register */
+#define XPS2_TX_DATA_OFFSET	0x0000000C /* Transmit Data register */
+#define XPS2_GIER_OFFSET	0x0000002C /* Global Interrupt Enable reg */
+#define XPS2_IPISR_OFFSET	0x00000030 /* Interrupt Status register */
+#define XPS2_IPIER_OFFSET	0x00000038 /* Interrupt Enable register */
+
+/* Reset Register Bit Definitions */
+#define XPS2_SRST_RESET		0x0000000A /* Software Reset  */
+
+/* Status Register Bit Positions */
+#define XPS2_STATUS_RX_FULL	0x00000001 /* Receive Full  */
+#define XPS2_STATUS_TX_FULL	0x00000002 /* Transmit Full  */
+
+/* Bit definitions for ISR/IER registers. Both the registers have the same bit
+ * definitions and are only defined once. */
+#define XPS2_IPIXR_WDT_TOUT	0x00000001 /* Watchdog Timeout Interrupt */
+#define XPS2_IPIXR_TX_NOACK	0x00000002 /* Transmit No ACK Interrupt */
+#define XPS2_IPIXR_TX_ACK	0x00000004 /* Transmit ACK (Data) Interrupt */
+#define XPS2_IPIXR_RX_OVF	0x00000008 /* Receive Overflow Interrupt */
+#define XPS2_IPIXR_RX_ERR	0x00000010 /* Receive Error Interrupt */
+#define XPS2_IPIXR_RX_FULL	0x00000020 /* Receive Data Interrupt */
+
+/* Mask for all the Transmit Interrupts */
+#define XPS2_IPIXR_TX_ALL	(XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_TX_ACK)
+
+/* Mask for all the Receive Interrupts */
+#define XPS2_IPIXR_RX_ALL	(XPS2_IPIXR_RX_OVF | XPS2_IPIXR_RX_ERR |  \
+					XPS2_IPIXR_RX_FULL)
+
+/* Mask for all the Interrupts */
+#define XPS2_IPIXR_ALL		(XPS2_IPIXR_TX_ALL | XPS2_IPIXR_RX_ALL |  \
+					XPS2_IPIXR_WDT_TOUT)
+
+/* Global Interrupt Enable mask */
+#define XPS2_GIER_GIE_MASK	0x80000000
+
+struct xps2data {
+	int irq;
+	u32 phys_addr;
+	u32 remap_size;
+	spinlock_t lock;
+	u8 rxb;				/* Rx buffer */
+	void __iomem *base_address;	/* virt. address of control registers */
+	unsigned long tx_end;
+	unsigned int dfl;
+	struct serio serio;		/* serio */
+};
+
+static u8 xps2_send(struct xps2data *drvdata, u8 *buffer_ptr);
+static u8 xps2_recv(struct xps2data *drvdata, u8 *buffer_ptr);
+static int xps2_initialize(struct xps2data *drvdata);
+static int xps2_setup(struct device *dev, int id, struct resource *regs_res,
+		      struct resource *irq_res);
+
+#endif					/* end of protection macro */
+
-- 
1.5.2.1



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

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

[Peter Korsgaard]

>>>>> "John" == John Linn <john.linn@xilinx.com> writes:

 John> Added a new driver for Xilinx XPS PS2 IP. This driver is
 John> a flat driver to better match the Linux driver pattern.

 John> Signed-off-by: Sadanand <sadanan@xilinx.com>
 John> Signed-off-by: John Linn <john.linn@xilinx.com>
 John> ---

 John> V2

 John> Changes from v1:

 John> Ran the scripts/checkpatch.pl on the patch as I should have done
 John> before sending it (thanks to Stephen).

What about my comments?

-- 
Bye, Peter Korsgaard

RE: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[John Linn]

Ahh... sorry about that Peter, didn't ignore your comments on purpose. =


Just got on a tangent about sending it to the other mailing list and
forgot to look on down.

I'll review them.

-- John

-----Original Message-----
From: Peter Korsgaard [mailto:jacmet@gmail.com] On Behalf Of Peter
Korsgaard
Sent: Monday, June 30, 2008 11:56 AM
To: John Linn
Cc: linuxppc-dev@ozlabs.org; linux-input@vger.kernel.org; Sadanand
Mutyala
Subject: Re: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

>>>>> "John" =3D=3D John Linn <john.linn@xilinx.com> writes:

 John> Added a new driver for Xilinx XPS PS2 IP. This driver is
 John> a flat driver to better match the Linux driver pattern.

 John> Signed-off-by: Sadanand <sadanan@xilinx.com>
 John> Signed-off-by: John Linn <john.linn@xilinx.com>
 John> ---

 John> V2

 John> Changes from v1:

 John> Ran the scripts/checkpatch.pl on the patch as I should have done
 John> before sending it (thanks to Stephen).

What about my comments?

-- =

Bye, Peter Korsgaard


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

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

[Dmitry Torokhov]

On Mon, Jun 30, 2008 at 08:38:21AM -0700, John Linn wrote:
> +	spin_lock_init(&drvdata->lock);
> +	dev_set_drvdata(dev, (void *)drvdata);

No need to cast to void *.

> +
> +	if (!regs_res || !irq_res) {
> +		dev_err(dev, "IO resource(s) not found\n");
> +		retval = -EFAULT;
> +		goto failed1;
> +	}
> +
> +	drvdata->irq = irq_res->start;
> +	remap_size = regs_res->end - regs_res->start + 1;
> +	if (!request_mem_region(regs_res->start, remap_size, DRIVER_NAME)) {
> +
> +		dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
> +			(unsigned int)regs_res->start);
> +		retval = -EBUSY;
> +		goto failed1;
> +	}
> +
> +	/* Fill in configuration data and add them to the list */
> +	drvdata->phys_addr = regs_res->start;
> +	drvdata->remap_size = remap_size;
> +	drvdata->base_address = ioremap(regs_res->start, remap_size);
> +	if (drvdata->base_address == NULL) {
> +
> +		dev_err(dev, "Couldn't ioremap memory at 0x%08X\n",
> +			(unsigned int)regs_res->start);
> +		retval = -EFAULT;
> +		goto failed2;
> +	}
> +
> +	/* Initialize the PS/2 interface */
> +	down(&cfg_sem);
> +	if (xps2_initialize(drvdata)) {
> +		up(&cfg_sem);
> +		dev_err(dev, "Could not initialize device\n");
> +		retval = -ENODEV;
> +		goto failed3;
> +	}
> +	up(&cfg_sem);

Do you need a counting semaphore here?

> +
> +	dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=%d\n",
> +		drvdata->phys_addr, (u32)drvdata->base_address, drvdata->irq);
> +
> +	drvdata->serio.id.type = SERIO_8042;
> +	drvdata->serio.write = sxps2_write;
> +	drvdata->serio.open = sxps2_open;
> +	drvdata->serio.close = sxps2_close;
> +	drvdata->serio.port_data = drvdata;
> +	drvdata->serio.dev.parent = dev;
> +	snprintf(drvdata->serio.name, sizeof(drvdata->serio.name),
> +		 XPS2_NAME_DESC, id);
> +	snprintf(drvdata->serio.phys, sizeof(drvdata->serio.phys),
> +		 XPS2_PHYS_DESC, id);

I bet if you make a temp variable for drvdata->serio the code size wil
shrink a tiny bit.

-- 
Dmitry

RE: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[John Linn]

Thanks Dmitry, see my comments inline.

-- John

>On Mon, Jun 30, 2008 at 08:38:21AM -0700, John Linn wrote:
>> +	spin_lock_init(&drvdata->lock);
>> +	dev_set_drvdata(dev, (void *)drvdata);
>
>No need to cast to void *.

OK.

>
>> +
>> +	if (!regs_res || !irq_res) {
>> +		dev_err(dev, "IO resource(s) not found\n");
>> +		retval =3D -EFAULT;
>> +		goto failed1;
>> +	}
>> +
>> +	drvdata->irq =3D irq_res->start;
>> +	remap_size =3D regs_res->end - regs_res->start + 1;
>> +	if (!request_mem_region(regs_res->start, remap_size,
DRIVER_NAME)) {
>> +
>> +		dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
>> +			(unsigned int)regs_res->start);
>> +		retval =3D -EBUSY;
>> +		goto failed1;
>> +	}
>> +
>> +	/* Fill in configuration data and add them to the list */
>> +	drvdata->phys_addr =3D regs_res->start;
>> +	drvdata->remap_size =3D remap_size;
>> +	drvdata->base_address =3D ioremap(regs_res->start, remap_size);
>> +	if (drvdata->base_address =3D=3D NULL) {
>> +
>> +		dev_err(dev, "Couldn't ioremap memory at 0x%08X\n",
>> +			(unsigned int)regs_res->start);
>> +		retval =3D -EFAULT;
>> +		goto failed2;
>> +	}
>> +
>> +	/* Initialize the PS/2 interface */
>> +	down(&cfg_sem);
>> +	if (xps2_initialize(drvdata)) {
>> +		up(&cfg_sem);
>> +		dev_err(dev, "Could not initialize device\n");
>> +		retval =3D -ENODEV;
>> +		goto failed3;
>> +	}
>> +	up(&cfg_sem);
>
>Do you need a counting semaphore here?
>

Seems like a simpler kind of mutal exclusion would do the job, like a
spinlock.
Looks like mutual exclusion is needed as the device contains 2 ps2
controllers.

>> +
>> +	dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=3D%d\n",
>> +		drvdata->phys_addr, (u32)drvdata->base_address,
drvdata->irq);
>> +
>> +	drvdata->serio.id.type =3D SERIO_8042;
>> +	drvdata->serio.write =3D sxps2_write;
>> +	drvdata->serio.open =3D sxps2_open;
>> +	drvdata->serio.close =3D sxps2_close;
>> +	drvdata->serio.port_data =3D drvdata;
>> +	drvdata->serio.dev.parent =3D dev;
>> +	snprintf(drvdata->serio.name, sizeof(drvdata->serio.name),
>> +		 XPS2_NAME_DESC, id);
>> +	snprintf(drvdata->serio.phys, sizeof(drvdata->serio.phys),
>> +		 XPS2_PHYS_DESC, id);
>
>I bet if you make a temp variable for drvdata->serio the code size wil
>shrink a tiny bit.

Ok, not sure the complexity of the temp is worth the tiny code
shrinkage.
I would think the compiler should optimize it as well but your
experience
must say otherwise.

>
>-- =

>Dmitry
>

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] =

Sent: Monday, June 30, 2008 12:21 PM
To: John Linn
Cc: linuxppc-dev@ozlabs.org; linux-input@vger.kernel.org; Sadanand
Mutyala
Subject: Re: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

On Mon, Jun 30, 2008 at 08:38:21AM -0700, John Linn wrote:
> +	spin_lock_init(&drvdata->lock);
> +	dev_set_drvdata(dev, (void *)drvdata);

No need to cast to void *.

> +
> +	if (!regs_res || !irq_res) {
> +		dev_err(dev, "IO resource(s) not found\n");
> +		retval =3D -EFAULT;
> +		goto failed1;
> +	}
> +
> +	drvdata->irq =3D irq_res->start;
> +	remap_size =3D regs_res->end - regs_res->start + 1;
> +	if (!request_mem_region(regs_res->start, remap_size,
DRIVER_NAME)) {
> +
> +		dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
> +			(unsigned int)regs_res->start);
> +		retval =3D -EBUSY;
> +		goto failed1;
> +	}
> +
> +	/* Fill in configuration data and add them to the list */
> +	drvdata->phys_addr =3D regs_res->start;
> +	drvdata->remap_size =3D remap_size;
> +	drvdata->base_address =3D ioremap(regs_res->start, remap_size);
> +	if (drvdata->base_address =3D=3D NULL) {
> +
> +		dev_err(dev, "Couldn't ioremap memory at 0x%08X\n",
> +			(unsigned int)regs_res->start);
> +		retval =3D -EFAULT;
> +		goto failed2;
> +	}
> +
> +	/* Initialize the PS/2 interface */
> +	down(&cfg_sem);
> +	if (xps2_initialize(drvdata)) {
> +		up(&cfg_sem);
> +		dev_err(dev, "Could not initialize device\n");
> +		retval =3D -ENODEV;
> +		goto failed3;
> +	}
> +	up(&cfg_sem);

Do you need a counting semaphore here?

> +
> +	dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=3D%d\n",
> +		drvdata->phys_addr, (u32)drvdata->base_address,
drvdata->irq);
> +
> +	drvdata->serio.id.type =3D SERIO_8042;
> +	drvdata->serio.write =3D sxps2_write;
> +	drvdata->serio.open =3D sxps2_open;
> +	drvdata->serio.close =3D sxps2_close;
> +	drvdata->serio.port_data =3D drvdata;
> +	drvdata->serio.dev.parent =3D dev;
> +	snprintf(drvdata->serio.name, sizeof(drvdata->serio.name),
> +		 XPS2_NAME_DESC, id);
> +	snprintf(drvdata->serio.phys, sizeof(drvdata->serio.phys),
> +		 XPS2_PHYS_DESC, id);

I bet if you make a temp variable for drvdata->serio the code size wil
shrink a tiny bit.

-- =

Dmitry


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

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

[John Linn]

Added a new driver for Xilinx XPS PS2 IP. This driver is
a flat driver to better match the Linux driver pattern.

Signed-off-by: Sadanand <sadanan@xilinx.com>
Signed-off-by: John Linn <john.linn@xilinx.com>
---
V2
	Updated the driver based on feedback from Dmitry, Peter, and Grant.
	We believe Montavista copyright is still valid.

 drivers/input/serio/Kconfig      |    5 +
 drivers/input/serio/Makefile     |    1 +
 drivers/input/serio/xilinx_ps2.c |  448 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 454 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/serio/xilinx_ps2.c

diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index ec4b661..0e62b39 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -190,4 +190,9 @@ config SERIO_RAW
 	  To compile this driver as a module, choose M here: the
 	  module will be called serio_raw.
 
+config SERIO_XILINX_XPS_PS2
+	tristate "Xilinx XPS PS/2 Controller Support"
+	help
+	  This driver supports XPS PS/2 IP from Xilinx EDK.
+
 endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 38b8868..9b6c813 100644
--- a/drivers/input/serio/Makefile
+++ b/drivers/input/serio/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_SERIO_PCIPS2)	+= pcips2.o
 obj-$(CONFIG_SERIO_MACEPS2)	+= maceps2.o
 obj-$(CONFIG_SERIO_LIBPS2)	+= libps2.o
 obj-$(CONFIG_SERIO_RAW)		+= serio_raw.o
+obj-$(CONFIG_SERIO_XILINX_XPS_PS2)	+= xilinx_ps2.o
diff --git a/drivers/input/serio/xilinx_ps2.c b/drivers/input/serio/xilinx_ps2.c
new file mode 100644
index 0000000..152bf2c
--- /dev/null
+++ b/drivers/input/serio/xilinx_ps2.c
@@ -0,0 +1,448 @@
+/*
+ * Xilinx XPS PS/2 device driver
+ *
+ * (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.
+ */
+
+
+#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 <linux/io.h>
+
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+
+#define DRIVER_NAME		"xilinx_ps2"
+
+/* Register offsets for the xps2 device */
+#define XPS2_SRST_OFFSET	0x00000000 /* Software Reset register */
+#define XPS2_STATUS_OFFSET	0x00000004 /* Status register */
+#define XPS2_RX_DATA_OFFSET	0x00000008 /* Receive Data register */
+#define XPS2_TX_DATA_OFFSET	0x0000000C /* Transmit Data register */
+#define XPS2_GIER_OFFSET	0x0000002C /* Global Interrupt Enable reg */
+#define XPS2_IPISR_OFFSET	0x00000030 /* Interrupt Status register */
+#define XPS2_IPIER_OFFSET	0x00000038 /* Interrupt Enable register */
+
+/* Reset Register Bit Definitions */
+#define XPS2_SRST_RESET		0x0000000A /* Software Reset  */
+
+/* Status Register Bit Positions */
+#define XPS2_STATUS_RX_FULL	0x00000001 /* Receive Full  */
+#define XPS2_STATUS_TX_FULL	0x00000002 /* Transmit Full  */
+
+/* Bit definitions for ISR/IER registers. Both the registers have the same bit
+ * definitions and are only defined once. */
+#define XPS2_IPIXR_WDT_TOUT	0x00000001 /* Watchdog Timeout Interrupt */
+#define XPS2_IPIXR_TX_NOACK	0x00000002 /* Transmit No ACK Interrupt */
+#define XPS2_IPIXR_TX_ACK	0x00000004 /* Transmit ACK (Data) Interrupt */
+#define XPS2_IPIXR_RX_OVF	0x00000008 /* Receive Overflow Interrupt */
+#define XPS2_IPIXR_RX_ERR	0x00000010 /* Receive Error Interrupt */
+#define XPS2_IPIXR_RX_FULL	0x00000020 /* Receive Data Interrupt */
+
+/* Mask for all the Transmit Interrupts */
+#define XPS2_IPIXR_TX_ALL	(XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_TX_ACK)
+
+/* Mask for all the Receive Interrupts */
+#define XPS2_IPIXR_RX_ALL	(XPS2_IPIXR_RX_OVF | XPS2_IPIXR_RX_ERR |  \
+					XPS2_IPIXR_RX_FULL)
+
+/* Mask for all the Interrupts */
+#define XPS2_IPIXR_ALL		(XPS2_IPIXR_TX_ALL | XPS2_IPIXR_RX_ALL |  \
+					XPS2_IPIXR_WDT_TOUT)
+
+/* Global Interrupt Enable mask */
+#define XPS2_GIER_GIE_MASK	0x80000000
+
+struct xps2data {
+	int irq;
+	u32 phys_addr;
+	u32 remap_size;
+	spinlock_t lock;
+	u8 rxb;				/* Rx buffer */
+	void __iomem *base_address;	/* virt. address of control registers */
+	unsigned int dfl;
+	struct serio serio;		/* serio */
+	struct mutex cfg_mutex;
+};
+
+/************************************/
+/* XPS PS/2 data transmission calls */
+/************************************/
+
+/*
+ * xps2_send() sends the specified byte of data to the PS/2 port.
+ */
+static int xps2_send(struct xps2data *drvdata, u8 *byte)
+{
+	u32 sr;
+	u32 ier;
+	int retval = -1;
+
+	/* Enter a critical region by disabling the PS/2 transmit interrupts to
+	 * allow this call to stop a previous operation that may be interrupt
+	 * driven. Only stop the transmit interrupt since this critical region
+	 * is not really exited in the normal manner */
+	ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
+	ier &= (~(XPS2_IPIXR_TX_ALL & XPS2_IPIXR_ALL));
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
+
+	/* If the PS/2 transmitter is empty send a byte of data */
+	sr = in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
+	if ((sr & XPS2_STATUS_TX_FULL) == 0) {
+		out_be32(drvdata->base_address + XPS2_TX_DATA_OFFSET, *byte);
+		retval = 0;
+	}
+
+	/* Enable the TX interrupts to track the status of the transmission */
+	ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
+	ier |= (XPS2_IPIXR_TX_ALL | XPS2_IPIXR_WDT_TOUT);
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
+
+	return retval;
+}
+
+/*
+ * xps2_recv() will attempt to receive a byte of data from the PS/2 port.
+ */
+static int xps2_recv(struct xps2data *drvdata, u8 *byte)
+{
+	u32 sr;
+	int retval = -1;
+
+	/* If there is data available in the PS/2 receiver, read it */
+	sr = in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
+	if (sr & XPS2_STATUS_RX_FULL) {
+		*byte = in_be32(drvdata->base_address + XPS2_RX_DATA_OFFSET);
+		retval = 0;
+	}
+
+	return retval;
+}
+
+/*********************/
+/* 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 a byte is not received */
+		if (retval) {
+			printk(KERN_ERR
+				"%s: wrong rcvd byte count (%d)\n",
+				drvdata->serio.name, retval);
+		} else {
+			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.
+ */
+static int sxps2_write(struct serio *pserio, unsigned char c)
+{
+	struct xps2data *drvdata = pserio->port_data;
+	unsigned long flags;
+	int retval;
+
+	spin_lock_irqsave(&drvdata->lock, flags);
+	retval = xps2_send(drvdata, &c);
+	spin_unlock_irqrestore(&drvdata->lock, flags);
+
+	return retval;
+}
+
+/*
+ * sxps2_open() is called when a port is open by the higher layer.
+ */
+static int sxps2_open(struct serio *pserio)
+{
+	struct xps2data *drvdata = pserio->port_data;
+	int retval;
+
+	retval = request_irq(drvdata->irq, &xps2_interrupt, 0,
+				DRIVER_NAME, drvdata);
+	if (retval) {
+		printk(KERN_ERR
+			"%s: Couldn't allocate interrupt %d\n",
+			drvdata->serio.name, drvdata->irq);
+		return retval;
+	}
+
+	/* start reception by enabling the interrupts */
+	out_be32(drvdata->base_address + XPS2_GIER_OFFSET, XPS2_GIER_GIE_MASK);
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, XPS2_IPIXR_RX_ALL);
+	(void)xps2_recv(drvdata, &drvdata->rxb);
+
+	return 0;		/* success */
+}
+
+/*
+ * sxps2_close() frees the interrupt.
+ */
+static void sxps2_close(struct serio *pserio)
+{
+	struct xps2data *drvdata = pserio->port_data;
+
+	/* Disable the PS2 interrupts */
+	out_be32(drvdata->base_address + XPS2_GIER_OFFSET, 0x00);
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0x00);
+	free_irq(drvdata->irq, drvdata);
+}
+
+/******************************/
+/* Device initialization code */
+/******************************/
+
+/*
+ * xps2_initialize() initializes the Xilinx PS/2 device.
+ */
+static int xps2_initialize(struct xps2data *drvdata)
+{
+	/* Disable all the interrupts, just in case */
+	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0);
+
+	/* Reset the PS2 device and abort any current transaction, to make sure
+	 * we have the PS2 in a good state */
+	out_be32(drvdata->base_address + XPS2_SRST_OFFSET, XPS2_SRST_RESET);
+
+	return 0;
+}
+
+/*
+ * XPS PS2 device setup code.
+ */
+static int xps2_setup(struct device *dev, int id, struct resource *regs_res,
+			struct resource *irq_res)
+{
+	struct xps2data *drvdata;
+	struct serio *serio;
+	unsigned long remap_size;
+	int retval;
+
+	if (!dev)
+		return -EINVAL;
+
+	drvdata = kzalloc(sizeof(struct xps2data), GFP_KERNEL);
+	if (!drvdata) {
+		dev_err(dev, "Couldn't allocate device private record\n");
+		return -ENOMEM;
+	}
+	spin_lock_init(&drvdata->lock);
+	mutex_init(&drvdata->cfg_mutex);
+	dev_set_drvdata(dev, drvdata);
+
+	if (!regs_res || !irq_res) {
+		dev_err(dev, "IO resource(s) not found\n");
+		retval = -EFAULT;
+		goto failed1;
+	}
+
+	drvdata->irq = irq_res->start;
+	remap_size = regs_res->end - regs_res->start + 1;
+	if (!request_mem_region(regs_res->start, remap_size, DRIVER_NAME)) {
+
+		dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
+			(unsigned int)regs_res->start);
+		retval = -EBUSY;
+		goto failed1;
+	}
+
+	/* Fill in configuration data and add them to the list */
+	drvdata->phys_addr = regs_res->start;
+	drvdata->remap_size = remap_size;
+	drvdata->base_address = ioremap(regs_res->start, remap_size);
+	if (drvdata->base_address == NULL) {
+
+		dev_err(dev, "Couldn't ioremap memory at 0x%08X\n",
+			(unsigned int)regs_res->start);
+		retval = -EFAULT;
+		goto failed2;
+	}
+
+	/* Initialize the PS/2 interface */
+	mutex_lock(&drvdata->cfg_mutex);
+	if (xps2_initialize(drvdata)) {
+		mutex_unlock(&drvdata->cfg_mutex);
+		dev_err(dev, "Could not initialize device\n");
+		retval = -ENODEV;
+		goto failed3;
+	}
+	mutex_unlock(&drvdata->cfg_mutex);
+
+	dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=%d\n",
+		drvdata->phys_addr, (u32)drvdata->base_address, drvdata->irq);
+
+	serio = &drvdata->serio;
+	serio->id.type = SERIO_8042;
+	serio->write = sxps2_write;
+	serio->open = sxps2_open;
+	serio->close = sxps2_close;
+	serio->port_data = drvdata;
+	serio->dev.parent = dev;
+	snprintf(drvdata->serio.name, sizeof(serio->name),
+		 "Xilinx XPS PS/2 Port #%d", id);
+	snprintf(drvdata->serio.phys, sizeof(serio->phys),
+		 "xilinxps2/serio%d", id);
+	serio_register_port(serio);
+
+	return 0;		/* success */
+
+failed3:
+	iounmap(drvdata->base_address);
+
+failed2:
+	release_mem_region(regs_res->start, remap_size);
+
+failed1:
+	kfree(drvdata);
+	dev_set_drvdata(dev, NULL);
+
+	return retval;
+}
+
+/***************************/
+/* OF Platform Bus Support */
+/***************************/
+
+static int __devinit xps2_of_probe(struct of_device *ofdev, const struct
+				   of_device_id * match)
+{
+	struct resource r_irq_struct;
+	struct resource r_mem_struct;
+	struct resource *r_irq = &r_irq_struct;	/* Interrupt resources */
+	struct resource *r_mem = &r_mem_struct;	/* IO mem resources */
+	int rc = 0;
+	const unsigned int *id;
+
+	printk(KERN_INFO "Device Tree Probing \'%s\'\n",
+			ofdev->node->name);
+
+	/* Get iospace for the device */
+	rc = of_address_to_resource(ofdev->node, 0, r_mem);
+	if (rc) {
+		dev_warn(&ofdev->dev, "invalid address\n");
+		return rc;
+	}
+
+	/* Get IRQ for the device */
+	rc = of_irq_to_resource(ofdev->node, 0, r_irq);
+	if (rc == NO_IRQ) {
+		dev_warn(&ofdev->dev, "no IRQ found\n");
+		return rc;
+	}
+
+	id = of_get_property(ofdev->node, "port-number", NULL);
+	return xps2_setup(&ofdev->dev, id ? *id : -1, r_mem, r_irq);
+}
+
+static int __devexit xps2_of_remove(struct of_device *of_dev)
+{
+	struct xps2data *drvdata;
+	struct device *dev;
+
+	dev = &of_dev->dev;
+	if (!dev)
+		return -EINVAL;
+
+	drvdata = (struct xps2data *)dev_get_drvdata(dev);
+
+	serio_unregister_port(&drvdata->serio);
+
+	iounmap(drvdata->base_address);
+
+	release_mem_region(drvdata->phys_addr, drvdata->remap_size);
+
+	kfree(drvdata);
+	dev_set_drvdata(dev, NULL);
+
+	return 0;		/* success */
+}
+
+/* Match table for of_platform binding */
+static struct of_device_id __devinitdata xps2_of_match[] = {
+	{ .compatible = "xlnx,xps-ps2-1.00.a", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, xps2_of_match);
+
+static struct of_platform_driver xps2_of_driver = {
+	.name		= DRIVER_NAME,
+	.match_table	= xps2_of_match,
+	.probe		= xps2_of_probe,
+	.remove		= __devexit_p(xps2_of_remove),
+};
+
+static int __init xps2_init(void)
+{
+	return of_register_platform_driver(&xps2_of_driver);
+}
+
+static void __exit xps2_cleanup(void)
+{
+	of_unregister_platform_driver(&xps2_of_driver);
+}
+
+module_init(xps2_init);
+module_exit(xps2_cleanup);
+
+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION("Xilinx XPS PS/2 driver");
+MODULE_LICENSE("GPL");
+
-- 
1.5.2.1



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

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

[Dmitry Torokhov]

Hi John,

On Thu, Jul 03, 2008 at 09:42:31AM -0700, John Linn wrote:
> +
> +	/* Initialize the PS/2 interface */
> +	mutex_lock(&drvdata->cfg_mutex);
> +	if (xps2_initialize(drvdata)) {
> +		mutex_unlock(&drvdata->cfg_mutex);
> +		dev_err(dev, "Could not initialize device\n");
> +		retval = -ENODEV;
> +		goto failed3;
> +	}
> +	mutex_unlock(&drvdata->cfg_mutex);

The drvdata is allocated per-port and so both (there are 2 PS/2 ports,
right?) ports get their own copy of cfg_mutex. Since you are trying to
serialze access to resource shared by both ports it will not work.
The original driver-global mutex was appropriate (the only thing I
objected there was use of a counting semaphore instead of a mutex).

-- 
Dmitry

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

[Grant Likely]

On Thu, Jul 03, 2008 at 01:27:00PM -0400, Dmitry Torokhov wrote:
> Hi John,
> 
> On Thu, Jul 03, 2008 at 09:42:31AM -0700, John Linn wrote:
> > +
> > +	/* Initialize the PS/2 interface */
> > +	mutex_lock(&drvdata->cfg_mutex);
> > +	if (xps2_initialize(drvdata)) {
> > +		mutex_unlock(&drvdata->cfg_mutex);
> > +		dev_err(dev, "Could not initialize device\n");
> > +		retval = -ENODEV;
> > +		goto failed3;
> > +	}
> > +	mutex_unlock(&drvdata->cfg_mutex);
> 
> The drvdata is allocated per-port and so both (there are 2 PS/2 ports,
> right?) ports get their own copy of cfg_mutex. Since you are trying to
> serialze access to resource shared by both ports it will not work.
> The original driver-global mutex was appropriate (the only thing I
> objected there was use of a counting semaphore instead of a mutex).

John, correct me if I'm wrong, but I don't think there are any shared
resources being accessed here and I believe the mutex is entirely
unnecessary.

Cheers,
g.

RE: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[John Linn]

In reviewing the data sheet for the device, I don't see any shared
information between the 2 channels of the device.

The mutex looks like it's not needed to me also.  =


I'll review with some others here to make sure I'm not overlooking
something. =


-- John

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of Grant
Likely
> Sent: Thursday, July 03, 2008 11:42 AM
> To: Dmitry Torokhov
> Cc: John Linn; linuxppc-dev@ozlabs.org; linux-input@vger.kernel.org;
jwboyer@linux.vnet.ibm.com;
> jacmet@sunsite.dk; Sadanand Mutyala
> Subject: Re: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2
driver
> =

> On Thu, Jul 03, 2008 at 01:27:00PM -0400, Dmitry Torokhov wrote:
> > Hi John,
> >
> > On Thu, Jul 03, 2008 at 09:42:31AM -0700, John Linn wrote:
> > > +
> > > +	/* Initialize the PS/2 interface */
> > > +	mutex_lock(&drvdata->cfg_mutex);
> > > +	if (xps2_initialize(drvdata)) {
> > > +		mutex_unlock(&drvdata->cfg_mutex);
> > > +		dev_err(dev, "Could not initialize device\n");
> > > +		retval =3D -ENODEV;
> > > +		goto failed3;
> > > +	}
> > > +	mutex_unlock(&drvdata->cfg_mutex);
> >
> > The drvdata is allocated per-port and so both (there are 2 PS/2
ports,
> > right?) ports get their own copy of cfg_mutex. Since you are trying
to
> > serialze access to resource shared by both ports it will not work.
> > The original driver-global mutex was appropriate (the only thing I
> > objected there was use of a counting semaphore instead of a mutex).
> =

> John, correct me if I'm wrong, but I don't think there are any shared
> resources being accessed here and I believe the mutex is entirely
> unnecessary.
> =

> Cheers,
> g.


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

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

[Grant Likely]

On Thu, Jul 03, 2008 at 09:42:31AM -0700, John Linn wrote:
> Added a new driver for Xilinx XPS PS2 IP. This driver is
> a flat driver to better match the Linux driver pattern.
> 
> Signed-off-by: Sadanand <sadanan@xilinx.com>
> Signed-off-by: John Linn <john.linn@xilinx.com>
> ---
> V2
> 	Updated the driver based on feedback from Dmitry, Peter, and Grant.
> 	We believe Montavista copyright is still valid.
> 
>  drivers/input/serio/Kconfig      |    5 +
>  drivers/input/serio/Makefile     |    1 +
>  drivers/input/serio/xilinx_ps2.c |  448 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 454 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/serio/xilinx_ps2.c
> 
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index ec4b661..0e62b39 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -190,4 +190,9 @@ config SERIO_RAW
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called serio_raw.
>  
> +config SERIO_XILINX_XPS_PS2
> +	tristate "Xilinx XPS PS/2 Controller Support"
> +	help
> +	  This driver supports XPS PS/2 IP from Xilinx EDK.
> +
>  endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 38b8868..9b6c813 100644
> --- a/drivers/input/serio/Makefile
> +++ b/drivers/input/serio/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_SERIO_PCIPS2)	+= pcips2.o
>  obj-$(CONFIG_SERIO_MACEPS2)	+= maceps2.o
>  obj-$(CONFIG_SERIO_LIBPS2)	+= libps2.o
>  obj-$(CONFIG_SERIO_RAW)		+= serio_raw.o
> +obj-$(CONFIG_SERIO_XILINX_XPS_PS2)	+= xilinx_ps2.o
> diff --git a/drivers/input/serio/xilinx_ps2.c b/drivers/input/serio/xilinx_ps2.c
> new file mode 100644
> index 0000000..152bf2c
> --- /dev/null
> +++ b/drivers/input/serio/xilinx_ps2.c
> @@ -0,0 +1,448 @@
> +/*
> + * Xilinx XPS PS/2 device driver
> + *
> + * (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.
> + */
> +
> +
> +#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 <linux/io.h>
> +
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +
> +#define DRIVER_NAME		"xilinx_ps2"
> +
> +/* Register offsets for the xps2 device */
> +#define XPS2_SRST_OFFSET	0x00000000 /* Software Reset register */
> +#define XPS2_STATUS_OFFSET	0x00000004 /* Status register */
> +#define XPS2_RX_DATA_OFFSET	0x00000008 /* Receive Data register */
> +#define XPS2_TX_DATA_OFFSET	0x0000000C /* Transmit Data register */
> +#define XPS2_GIER_OFFSET	0x0000002C /* Global Interrupt Enable reg */
> +#define XPS2_IPISR_OFFSET	0x00000030 /* Interrupt Status register */
> +#define XPS2_IPIER_OFFSET	0x00000038 /* Interrupt Enable register */
> +
> +/* Reset Register Bit Definitions */
> +#define XPS2_SRST_RESET		0x0000000A /* Software Reset  */
> +
> +/* Status Register Bit Positions */
> +#define XPS2_STATUS_RX_FULL	0x00000001 /* Receive Full  */
> +#define XPS2_STATUS_TX_FULL	0x00000002 /* Transmit Full  */
> +
> +/* Bit definitions for ISR/IER registers. Both the registers have the same bit
> + * definitions and are only defined once. */
> +#define XPS2_IPIXR_WDT_TOUT	0x00000001 /* Watchdog Timeout Interrupt */
> +#define XPS2_IPIXR_TX_NOACK	0x00000002 /* Transmit No ACK Interrupt */
> +#define XPS2_IPIXR_TX_ACK	0x00000004 /* Transmit ACK (Data) Interrupt */
> +#define XPS2_IPIXR_RX_OVF	0x00000008 /* Receive Overflow Interrupt */
> +#define XPS2_IPIXR_RX_ERR	0x00000010 /* Receive Error Interrupt */
> +#define XPS2_IPIXR_RX_FULL	0x00000020 /* Receive Data Interrupt */
> +
> +/* Mask for all the Transmit Interrupts */
> +#define XPS2_IPIXR_TX_ALL	(XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_TX_ACK)
> +
> +/* Mask for all the Receive Interrupts */
> +#define XPS2_IPIXR_RX_ALL	(XPS2_IPIXR_RX_OVF | XPS2_IPIXR_RX_ERR |  \
> +					XPS2_IPIXR_RX_FULL)
> +
> +/* Mask for all the Interrupts */
> +#define XPS2_IPIXR_ALL		(XPS2_IPIXR_TX_ALL | XPS2_IPIXR_RX_ALL |  \
> +					XPS2_IPIXR_WDT_TOUT)
> +
> +/* Global Interrupt Enable mask */
> +#define XPS2_GIER_GIE_MASK	0x80000000
> +
> +struct xps2data {
> +	int irq;
> +	u32 phys_addr;
> +	u32 remap_size;
> +	spinlock_t lock;
> +	u8 rxb;				/* Rx buffer */
> +	void __iomem *base_address;	/* virt. address of control registers */
> +	unsigned int dfl;
> +	struct serio serio;		/* serio */
> +	struct mutex cfg_mutex;
> +};
> +
> +/************************************/
> +/* XPS PS/2 data transmission calls */
> +/************************************/
> +
> +/*
> + * xps2_send() sends the specified byte of data to the PS/2 port.
> + */
> +static int xps2_send(struct xps2data *drvdata, u8 *byte)
> +{
> +	u32 sr;
> +	u32 ier;
> +	int retval = -1;
> +
> +	/* Enter a critical region by disabling the PS/2 transmit interrupts to
> +	 * allow this call to stop a previous operation that may be interrupt
> +	 * driven. Only stop the transmit interrupt since this critical region
> +	 * is not really exited in the normal manner */
> +	ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
> +	ier &= (~(XPS2_IPIXR_TX_ALL & XPS2_IPIXR_ALL));
> +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
> +
> +	/* If the PS/2 transmitter is empty send a byte of data */
> +	sr = in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
> +	if ((sr & XPS2_STATUS_TX_FULL) == 0) {
> +		out_be32(drvdata->base_address + XPS2_TX_DATA_OFFSET, *byte);
> +		retval = 0;
> +	}
> +
> +	/* Enable the TX interrupts to track the status of the transmission */
> +	ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
> +	ier |= (XPS2_IPIXR_TX_ALL | XPS2_IPIXR_WDT_TOUT);
> +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);

If this is a critical region, Wouldn't it be more appropriate to use a
spinlock with spin_lock_irqsave()/spin_unlock_irqrestore()  here?

...

Actually, looking deeper into the file; xps2_send() is already protected
with a spin lock w/ irqsave.  Why do interrupts need to be disabled
at all?  It looks like this whole routine could be collapsed into a
about three lines of code in the sxps2_write() function.

> +
> +	return retval;
> +}
> +
> +/*
> + * xps2_recv() will attempt to receive a byte of data from the PS/2 port.
> + */
> +static int xps2_recv(struct xps2data *drvdata, u8 *byte)
> +{
> +	u32 sr;
> +	int retval = -1;
> +
> +	/* If there is data available in the PS/2 receiver, read it */
> +	sr = in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
> +	if (sr & XPS2_STATUS_RX_FULL) {
> +		*byte = in_be32(drvdata->base_address + XPS2_RX_DATA_OFFSET);
> +		retval = 0;
> +	}
> +
> +	return retval;
> +}
> +
> +/*********************/
> +/* 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 a byte is not received */
> +		if (retval) {
> +			printk(KERN_ERR
> +				"%s: wrong rcvd byte count (%d)\n",
> +				drvdata->serio.name, retval);
> +		} else {
> +			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.
> + */
> +static int sxps2_write(struct serio *pserio, unsigned char c)
> +{
> +	struct xps2data *drvdata = pserio->port_data;
> +	unsigned long flags;
> +	int retval;
> +
> +	spin_lock_irqsave(&drvdata->lock, flags);
> +	retval = xps2_send(drvdata, &c);
> +	spin_unlock_irqrestore(&drvdata->lock, flags);
> +
> +	return retval;
> +}
> +
> +/*
> + * sxps2_open() is called when a port is open by the higher layer.
> + */
> +static int sxps2_open(struct serio *pserio)
> +{
> +	struct xps2data *drvdata = pserio->port_data;
> +	int retval;
> +
> +	retval = request_irq(drvdata->irq, &xps2_interrupt, 0,
> +				DRIVER_NAME, drvdata);
> +	if (retval) {
> +		printk(KERN_ERR
> +			"%s: Couldn't allocate interrupt %d\n",
> +			drvdata->serio.name, drvdata->irq);
> +		return retval;
> +	}
> +
> +	/* start reception by enabling the interrupts */
> +	out_be32(drvdata->base_address + XPS2_GIER_OFFSET, XPS2_GIER_GIE_MASK);
> +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, XPS2_IPIXR_RX_ALL);
> +	(void)xps2_recv(drvdata, &drvdata->rxb);
> +
> +	return 0;		/* success */
> +}
> +
> +/*
> + * sxps2_close() frees the interrupt.
> + */
> +static void sxps2_close(struct serio *pserio)
> +{
> +	struct xps2data *drvdata = pserio->port_data;
> +
> +	/* Disable the PS2 interrupts */
> +	out_be32(drvdata->base_address + XPS2_GIER_OFFSET, 0x00);
> +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0x00);
> +	free_irq(drvdata->irq, drvdata);
> +}
> +
> +/******************************/
> +/* Device initialization code */
> +/******************************/
> +
> +/*
> + * xps2_initialize() initializes the Xilinx PS/2 device.
> + */
> +static int xps2_initialize(struct xps2data *drvdata)
> +{
> +	/* Disable all the interrupts, just in case */
> +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0);
> +
> +	/* Reset the PS2 device and abort any current transaction, to make sure
> +	 * we have the PS2 in a good state */
> +	out_be32(drvdata->base_address + XPS2_SRST_OFFSET, XPS2_SRST_RESET);
> +
> +	return 0;
> +}

This function is only called by xps2_setup() and should probably be
collapsed into it.

> +
> +/*
> + * XPS PS2 device setup code.
> + */
> +static int xps2_setup(struct device *dev, int id, struct resource *regs_res,
> +			struct resource *irq_res)
> +{
> +	struct xps2data *drvdata;
> +	struct serio *serio;
> +	unsigned long remap_size;
> +	int retval;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	drvdata = kzalloc(sizeof(struct xps2data), GFP_KERNEL);
> +	if (!drvdata) {
> +		dev_err(dev, "Couldn't allocate device private record\n");
> +		return -ENOMEM;
> +	}
> +	spin_lock_init(&drvdata->lock);
> +	mutex_init(&drvdata->cfg_mutex);
> +	dev_set_drvdata(dev, drvdata);
> +
> +	if (!regs_res || !irq_res) {
> +		dev_err(dev, "IO resource(s) not found\n");
> +		retval = -EFAULT;
> +		goto failed1;
> +	}
> +
> +	drvdata->irq = irq_res->start;
> +	remap_size = regs_res->end - regs_res->start + 1;
> +	if (!request_mem_region(regs_res->start, remap_size, DRIVER_NAME)) {
> +
> +		dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
> +			(unsigned int)regs_res->start);
> +		retval = -EBUSY;
> +		goto failed1;
> +	}
> +
> +	/* Fill in configuration data and add them to the list */
> +	drvdata->phys_addr = regs_res->start;
> +	drvdata->remap_size = remap_size;
> +	drvdata->base_address = ioremap(regs_res->start, remap_size);
> +	if (drvdata->base_address == NULL) {
> +
> +		dev_err(dev, "Couldn't ioremap memory at 0x%08X\n",
> +			(unsigned int)regs_res->start);
> +		retval = -EFAULT;
> +		goto failed2;
> +	}
> +
> +	/* Initialize the PS/2 interface */
> +	mutex_lock(&drvdata->cfg_mutex);
> +	if (xps2_initialize(drvdata)) {
> +		mutex_unlock(&drvdata->cfg_mutex);
> +		dev_err(dev, "Could not initialize device\n");
> +		retval = -ENODEV;
> +		goto failed3;
> +	}
> +	mutex_unlock(&drvdata->cfg_mutex);

This is the only user of cfg_mutex.  The IRQ isn't registered yet, and
the driver isn't registered with the serio layer either so you can just
drop the mutex entirely.

> +
> +	dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=%d\n",
> +		drvdata->phys_addr, (u32)drvdata->base_address, drvdata->irq);
> +
> +	serio = &drvdata->serio;
> +	serio->id.type = SERIO_8042;
> +	serio->write = sxps2_write;
> +	serio->open = sxps2_open;
> +	serio->close = sxps2_close;
> +	serio->port_data = drvdata;
> +	serio->dev.parent = dev;
> +	snprintf(drvdata->serio.name, sizeof(serio->name),
> +		 "Xilinx XPS PS/2 Port #%d", id);
> +	snprintf(drvdata->serio.phys, sizeof(serio->phys),
> +		 "xilinxps2/serio%d", id);
> +	serio_register_port(serio);
> +
> +	return 0;		/* success */
> +
> +failed3:
> +	iounmap(drvdata->base_address);
> +
> +failed2:
> +	release_mem_region(regs_res->start, remap_size);
> +
> +failed1:
> +	kfree(drvdata);
> +	dev_set_drvdata(dev, NULL);
> +
> +	return retval;
> +}
> +
> +/***************************/
> +/* OF Platform Bus Support */
> +/***************************/
> +
> +static int __devinit xps2_of_probe(struct of_device *ofdev, const struct
> +				   of_device_id * match)
> +{
> +	struct resource r_irq_struct;
> +	struct resource r_mem_struct;
> +	struct resource *r_irq = &r_irq_struct;	/* Interrupt resources */
> +	struct resource *r_mem = &r_mem_struct;	/* IO mem resources */

This is very vebose; do this instead:

+	struct resource r_irq;
+	struct resource r_mem;
[...]
+	rc = of_address_to_resource(ofdev->node, 0, &r_mem);
[...]
+	rc = of_irq_to_resource(ofdev->node, 0, &r_irq);
[...]
+	return xps2_setup(&ofdev->dev, id ? *id : -1, &r_mem, &r_irq);

> +	int rc = 0;
> +	const unsigned int *id;
> +
> +	printk(KERN_INFO "Device Tree Probing \'%s\'\n",
> +			ofdev->node->name);
> +
> +	/* Get iospace for the device */
> +	rc = of_address_to_resource(ofdev->node, 0, r_mem);
> +	if (rc) {
> +		dev_warn(&ofdev->dev, "invalid address\n");

Probably should be dev_err(...);

> +		return rc;
> +	}
> +
> +	/* Get IRQ for the device */
> +	rc = of_irq_to_resource(ofdev->node, 0, r_irq);
> +	if (rc == NO_IRQ) {
> +		dev_warn(&ofdev->dev, "no IRQ found\n");

ditto

> +		return rc;
> +	}
> +
> +	id = of_get_property(ofdev->node, "port-number", NULL);

Drop this line.  port-number is a poor way to specify a global value.
If really needed, you should search the aliases node for a matching 
property to get the id.  Otherwise, you should probably fall back to
using the physical address of the device instead of -1.  At least it
guarantees that the value is unique.

> +	return xps2_setup(&ofdev->dev, id ? *id : -1, r_mem, r_irq);
> +}
> +
> +static int __devexit xps2_of_remove(struct of_device *of_dev)
> +{
> +	struct xps2data *drvdata;
> +	struct device *dev;
> +
> +	dev = &of_dev->dev;
> +	if (!dev)
> +		return -EINVAL;
> +
> +	drvdata = (struct xps2data *)dev_get_drvdata(dev);
> +
> +	serio_unregister_port(&drvdata->serio);
> +
> +	iounmap(drvdata->base_address);
> +
> +	release_mem_region(drvdata->phys_addr, drvdata->remap_size);
> +
> +	kfree(drvdata);
> +	dev_set_drvdata(dev, NULL);
> +
> +	return 0;		/* success */
> +}
> +
> +/* Match table for of_platform binding */
> +static struct of_device_id __devinitdata xps2_of_match[] = {

__devinitdata in the wrong place; should be:

+static struct of_device_id xps2_of_match[] __devinitdata = {

> +	{ .compatible = "xlnx,xps-ps2-1.00.a", },
> +	{ /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, xps2_of_match);
> +
> +static struct of_platform_driver xps2_of_driver = {
> +	.name		= DRIVER_NAME,
> +	.match_table	= xps2_of_match,
> +	.probe		= xps2_of_probe,
> +	.remove		= __devexit_p(xps2_of_remove),
> +};
> +
> +static int __init xps2_init(void)
> +{
> +	return of_register_platform_driver(&xps2_of_driver);
> +}
> +
> +static void __exit xps2_cleanup(void)
> +{
> +	of_unregister_platform_driver(&xps2_of_driver);
> +}
> +
> +module_init(xps2_init);
> +module_exit(xps2_cleanup);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Xilinx XPS PS/2 driver");
> +MODULE_LICENSE("GPL");
> +
> -- 
> 1.5.2.1
> 
> 
> 
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
> 
> 

RE: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[John Linn]

Hi Grant,

Good comments.

With regard to the port-number, I don't see the value of using it unless
I'm just missing something.  The address of the device seems just as
useful.

We'll spin the patch again taking these into account and Dmitry's
comment also.

Thanks,
John


> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of Grant
Likely
> Sent: Thursday, July 03, 2008 11:37 AM
> To: John Linn
> Cc: linuxppc-dev@ozlabs.org; linux-input@vger.kernel.org;
jwboyer@linux.vnet.ibm.com;
> dmitry.torokhov@gmail.com; jacmet@sunsite.dk; Sadanand Mutyala
> Subject: Re: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2
driver
> =

> On Thu, Jul 03, 2008 at 09:42:31AM -0700, John Linn wrote:
> > Added a new driver for Xilinx XPS PS2 IP. This driver is
> > a flat driver to better match the Linux driver pattern.
> >
> > Signed-off-by: Sadanand <sadanan@xilinx.com>
> > Signed-off-by: John Linn <john.linn@xilinx.com>
> > ---
> > V2
> > 	Updated the driver based on feedback from Dmitry, Peter, and
Grant.
> > 	We believe Montavista copyright is still valid.
> >
> >  drivers/input/serio/Kconfig      |    5 +
> >  drivers/input/serio/Makefile     |    1 +
> >  drivers/input/serio/xilinx_ps2.c |  448
++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 454 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/input/serio/xilinx_ps2.c
> >
> > diff --git a/drivers/input/serio/Kconfig
b/drivers/input/serio/Kconfig
> > index ec4b661..0e62b39 100644
> > --- a/drivers/input/serio/Kconfig
> > +++ b/drivers/input/serio/Kconfig
> > @@ -190,4 +190,9 @@ config SERIO_RAW
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called serio_raw.
> >
> > +config SERIO_XILINX_XPS_PS2
> > +	tristate "Xilinx XPS PS/2 Controller Support"
> > +	help
> > +	  This driver supports XPS PS/2 IP from Xilinx EDK.
> > +
> >  endif
> > diff --git a/drivers/input/serio/Makefile
b/drivers/input/serio/Makefile
> > index 38b8868..9b6c813 100644
> > --- a/drivers/input/serio/Makefile
> > +++ b/drivers/input/serio/Makefile
> > @@ -21,3 +21,4 @@ obj-$(CONFIG_SERIO_PCIPS2)	+=3D pcips2.o
> >  obj-$(CONFIG_SERIO_MACEPS2)	+=3D maceps2.o
> >  obj-$(CONFIG_SERIO_LIBPS2)	+=3D libps2.o
> >  obj-$(CONFIG_SERIO_RAW)		+=3D serio_raw.o
> > +obj-$(CONFIG_SERIO_XILINX_XPS_PS2)	+=3D xilinx_ps2.o
> > diff --git a/drivers/input/serio/xilinx_ps2.c
b/drivers/input/serio/xilinx_ps2.c
> > new file mode 100644
> > index 0000000..152bf2c
> > --- /dev/null
> > +++ b/drivers/input/serio/xilinx_ps2.c
> > @@ -0,0 +1,448 @@
> > +/*
> > + * Xilinx XPS PS/2 device driver
> > + *
> > + * (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.
> > + */
> > +
> > +
> > +#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 <linux/io.h>
> > +
> > +#include <linux/of_device.h>
> > +#include <linux/of_platform.h>
> > +
> > +#define DRIVER_NAME		"xilinx_ps2"
> > +
> > +/* Register offsets for the xps2 device */
> > +#define XPS2_SRST_OFFSET	0x00000000 /* Software Reset register */
> > +#define XPS2_STATUS_OFFSET	0x00000004 /* Status register */
> > +#define XPS2_RX_DATA_OFFSET	0x00000008 /* Receive Data
register */
> > +#define XPS2_TX_DATA_OFFSET	0x0000000C /* Transmit Data
register */
> > +#define XPS2_GIER_OFFSET	0x0000002C /* Global Interrupt Enable
reg */
> > +#define XPS2_IPISR_OFFSET	0x00000030 /* Interrupt Status register
*/
> > +#define XPS2_IPIER_OFFSET	0x00000038 /* Interrupt Enable register
*/
> > +
> > +/* Reset Register Bit Definitions */
> > +#define XPS2_SRST_RESET		0x0000000A /* Software Reset  */
> > +
> > +/* Status Register Bit Positions */
> > +#define XPS2_STATUS_RX_FULL	0x00000001 /* Receive Full  */
> > +#define XPS2_STATUS_TX_FULL	0x00000002 /* Transmit Full  */
> > +
> > +/* Bit definitions for ISR/IER registers. Both the registers have
the same bit
> > + * definitions and are only defined once. */
> > +#define XPS2_IPIXR_WDT_TOUT	0x00000001 /* Watchdog Timeout
Interrupt */
> > +#define XPS2_IPIXR_TX_NOACK	0x00000002 /* Transmit No ACK
Interrupt */
> > +#define XPS2_IPIXR_TX_ACK	0x00000004 /* Transmit ACK (Data)
Interrupt */
> > +#define XPS2_IPIXR_RX_OVF	0x00000008 /* Receive Overflow Interrupt
*/
> > +#define XPS2_IPIXR_RX_ERR	0x00000010 /* Receive Error Interrupt */
> > +#define XPS2_IPIXR_RX_FULL	0x00000020 /* Receive Data Interrupt */
> > +
> > +/* Mask for all the Transmit Interrupts */
> > +#define XPS2_IPIXR_TX_ALL	(XPS2_IPIXR_TX_NOACK |
XPS2_IPIXR_TX_ACK)
> > +
> > +/* Mask for all the Receive Interrupts */
> > +#define XPS2_IPIXR_RX_ALL	(XPS2_IPIXR_RX_OVF | XPS2_IPIXR_RX_ERR |
\
> > +					XPS2_IPIXR_RX_FULL)
> > +
> > +/* Mask for all the Interrupts */
> > +#define XPS2_IPIXR_ALL		(XPS2_IPIXR_TX_ALL |
XPS2_IPIXR_RX_ALL |  \
> > +					XPS2_IPIXR_WDT_TOUT)
> > +
> > +/* Global Interrupt Enable mask */
> > +#define XPS2_GIER_GIE_MASK	0x80000000
> > +
> > +struct xps2data {
> > +	int irq;
> > +	u32 phys_addr;
> > +	u32 remap_size;
> > +	spinlock_t lock;
> > +	u8 rxb;				/* Rx buffer */
> > +	void __iomem *base_address;	/* virt. address of control
registers */
> > +	unsigned int dfl;
> > +	struct serio serio;		/* serio */
> > +	struct mutex cfg_mutex;
> > +};
> > +
> > +/************************************/
> > +/* XPS PS/2 data transmission calls */
> > +/************************************/
> > +
> > +/*
> > + * xps2_send() sends the specified byte of data to the PS/2 port.
> > + */
> > +static int xps2_send(struct xps2data *drvdata, u8 *byte)
> > +{
> > +	u32 sr;
> > +	u32 ier;
> > +	int retval =3D -1;
> > +
> > +	/* Enter a critical region by disabling the PS/2 transmit
interrupts to
> > +	 * allow this call to stop a previous operation that may be
interrupt
> > +	 * driven. Only stop the transmit interrupt since this critical
region
> > +	 * is not really exited in the normal manner */
> > +	ier =3D in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
> > +	ier &=3D (~(XPS2_IPIXR_TX_ALL & XPS2_IPIXR_ALL));
> > +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
> > +
> > +	/* If the PS/2 transmitter is empty send a byte of data */
> > +	sr =3D in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
> > +	if ((sr & XPS2_STATUS_TX_FULL) =3D=3D 0) {
> > +		out_be32(drvdata->base_address + XPS2_TX_DATA_OFFSET,
*byte);
> > +		retval =3D 0;
> > +	}
> > +
> > +	/* Enable the TX interrupts to track the status of the
transmission */
> > +	ier =3D in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
> > +	ier |=3D (XPS2_IPIXR_TX_ALL | XPS2_IPIXR_WDT_TOUT);
> > +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
> =

> If this is a critical region, Wouldn't it be more appropriate to use a
> spinlock with spin_lock_irqsave()/spin_unlock_irqrestore()  here?
> =

> ...
> =

> Actually, looking deeper into the file; xps2_send() is already
protected
> with a spin lock w/ irqsave.  Why do interrupts need to be disabled
> at all?  It looks like this whole routine could be collapsed into a
> about three lines of code in the sxps2_write() function.
> =

> > +
> > +	return retval;
> > +}
> > +
> > +/*
> > + * xps2_recv() will attempt to receive a byte of data from the PS/2
port.
> > + */
> > +static int xps2_recv(struct xps2data *drvdata, u8 *byte)
> > +{
> > +	u32 sr;
> > +	int retval =3D -1;
> > +
> > +	/* If there is data available in the PS/2 receiver, read it */
> > +	sr =3D in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
> > +	if (sr & XPS2_STATUS_RX_FULL) {
> > +		*byte =3D in_be32(drvdata->base_address +
XPS2_RX_DATA_OFFSET);
> > +		retval =3D 0;
> > +	}
> > +
> > +	return retval;
> > +}
> > +
> > +/*********************/
> > +/* Interrupt handler */
> > +/*********************/
> > +static irqreturn_t xps2_interrupt(int irq, void *dev_id)
> > +{
> > +	struct xps2data *drvdata =3D (struct xps2data *)dev_id;
> > +	u32 intr_sr;
> > +	u32 ier;
> > +	u8 c;
> > +	u8 retval;
> > +
> > +	/* Get the PS/2 interrupts and clear them */
> > +	intr_sr =3D 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 |=3D SERIO_PARITY;
> > +
> > +	if (intr_sr & (XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_WDT_TOUT))
> > +		drvdata->dfl |=3D SERIO_TIMEOUT;
> > +
> > +	if (intr_sr & XPS2_IPIXR_RX_FULL) {
> > +		retval =3D xps2_recv(drvdata, &drvdata->rxb);
> > +
> > +		/* Error, if a byte is not received */
> > +		if (retval) {
> > +			printk(KERN_ERR
> > +				"%s: wrong rcvd byte count (%d)\n",
> > +				drvdata->serio.name, retval);
> > +		} else {
> > +			c =3D drvdata->rxb;
> > +			serio_interrupt(&drvdata->serio, c,
drvdata->dfl);
> > +			drvdata->dfl =3D 0;
> > +		}
> > +	}
> > +
> > +	if (intr_sr & XPS2_IPIXR_TX_ACK) {
> > +
> > +		/* Disable the TX interrupts after the transmission is
> > +		 * complete */
> > +		ier =3D in_be32(drvdata->base_address +
XPS2_IPIER_OFFSET);
> > +		ier &=3D (~(XPS2_IPIXR_TX_ACK & XPS2_IPIXR_ALL));
> > +		out_be32(drvdata->base_address + XPS2_IPIER_OFFSET,
ier);
> > +		drvdata->dfl =3D 0;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/*******************/
> > +/* serio callbacks */
> > +/*******************/
> > +
> > +/*
> > + *	sxps2_write() - sends a byte out through the PS/2 interface.
> > + */
> > +static int sxps2_write(struct serio *pserio, unsigned char c)
> > +{
> > +	struct xps2data *drvdata =3D pserio->port_data;
> > +	unsigned long flags;
> > +	int retval;
> > +
> > +	spin_lock_irqsave(&drvdata->lock, flags);
> > +	retval =3D xps2_send(drvdata, &c);
> > +	spin_unlock_irqrestore(&drvdata->lock, flags);
> > +
> > +	return retval;
> > +}
> > +
> > +/*
> > + * sxps2_open() is called when a port is open by the higher layer.
> > + */
> > +static int sxps2_open(struct serio *pserio)
> > +{
> > +	struct xps2data *drvdata =3D pserio->port_data;
> > +	int retval;
> > +
> > +	retval =3D request_irq(drvdata->irq, &xps2_interrupt, 0,
> > +				DRIVER_NAME, drvdata);
> > +	if (retval) {
> > +		printk(KERN_ERR
> > +			"%s: Couldn't allocate interrupt %d\n",
> > +			drvdata->serio.name, drvdata->irq);
> > +		return retval;
> > +	}
> > +
> > +	/* start reception by enabling the interrupts */
> > +	out_be32(drvdata->base_address + XPS2_GIER_OFFSET,
XPS2_GIER_GIE_MASK);
> > +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET,
XPS2_IPIXR_RX_ALL);
> > +	(void)xps2_recv(drvdata, &drvdata->rxb);
> > +
> > +	return 0;		/* success */
> > +}
> > +
> > +/*
> > + * sxps2_close() frees the interrupt.
> > + */
> > +static void sxps2_close(struct serio *pserio)
> > +{
> > +	struct xps2data *drvdata =3D pserio->port_data;
> > +
> > +	/* Disable the PS2 interrupts */
> > +	out_be32(drvdata->base_address + XPS2_GIER_OFFSET, 0x00);
> > +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0x00);
> > +	free_irq(drvdata->irq, drvdata);
> > +}
> > +
> > +/******************************/
> > +/* Device initialization code */
> > +/******************************/
> > +
> > +/*
> > + * xps2_initialize() initializes the Xilinx PS/2 device.
> > + */
> > +static int xps2_initialize(struct xps2data *drvdata)
> > +{
> > +	/* Disable all the interrupts, just in case */
> > +	out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, 0);
> > +
> > +	/* Reset the PS2 device and abort any current transaction, to
make sure
> > +	 * we have the PS2 in a good state */
> > +	out_be32(drvdata->base_address + XPS2_SRST_OFFSET,
XPS2_SRST_RESET);
> > +
> > +	return 0;
> > +}
> =

> This function is only called by xps2_setup() and should probably be
> collapsed into it.
> =

> > +
> > +/*
> > + * XPS PS2 device setup code.
> > + */
> > +static int xps2_setup(struct device *dev, int id, struct resource
*regs_res,
> > +			struct resource *irq_res)
> > +{
> > +	struct xps2data *drvdata;
> > +	struct serio *serio;
> > +	unsigned long remap_size;
> > +	int retval;
> > +
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	drvdata =3D kzalloc(sizeof(struct xps2data), GFP_KERNEL);
> > +	if (!drvdata) {
> > +		dev_err(dev, "Couldn't allocate device private
record\n");
> > +		return -ENOMEM;
> > +	}
> > +	spin_lock_init(&drvdata->lock);
> > +	mutex_init(&drvdata->cfg_mutex);
> > +	dev_set_drvdata(dev, drvdata);
> > +
> > +	if (!regs_res || !irq_res) {
> > +		dev_err(dev, "IO resource(s) not found\n");
> > +		retval =3D -EFAULT;
> > +		goto failed1;
> > +	}
> > +
> > +	drvdata->irq =3D irq_res->start;
> > +	remap_size =3D regs_res->end - regs_res->start + 1;
> > +	if (!request_mem_region(regs_res->start, remap_size,
DRIVER_NAME)) {
> > +
> > +		dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
> > +			(unsigned int)regs_res->start);
> > +		retval =3D -EBUSY;
> > +		goto failed1;
> > +	}
> > +
> > +	/* Fill in configuration data and add them to the list */
> > +	drvdata->phys_addr =3D regs_res->start;
> > +	drvdata->remap_size =3D remap_size;
> > +	drvdata->base_address =3D ioremap(regs_res->start, remap_size);
> > +	if (drvdata->base_address =3D=3D NULL) {
> > +
> > +		dev_err(dev, "Couldn't ioremap memory at 0x%08X\n",
> > +			(unsigned int)regs_res->start);
> > +		retval =3D -EFAULT;
> > +		goto failed2;
> > +	}
> > +
> > +	/* Initialize the PS/2 interface */
> > +	mutex_lock(&drvdata->cfg_mutex);
> > +	if (xps2_initialize(drvdata)) {
> > +		mutex_unlock(&drvdata->cfg_mutex);
> > +		dev_err(dev, "Could not initialize device\n");
> > +		retval =3D -ENODEV;
> > +		goto failed3;
> > +	}
> > +	mutex_unlock(&drvdata->cfg_mutex);
> =

> This is the only user of cfg_mutex.  The IRQ isn't registered yet, and
> the driver isn't registered with the serio layer either so you can
just
> drop the mutex entirely.
> =

> > +
> > +	dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=3D%d\n",
> > +		drvdata->phys_addr, (u32)drvdata->base_address,
drvdata->irq);
> > +
> > +	serio =3D &drvdata->serio;
> > +	serio->id.type =3D SERIO_8042;
> > +	serio->write =3D sxps2_write;
> > +	serio->open =3D sxps2_open;
> > +	serio->close =3D sxps2_close;
> > +	serio->port_data =3D drvdata;
> > +	serio->dev.parent =3D dev;
> > +	snprintf(drvdata->serio.name, sizeof(serio->name),
> > +		 "Xilinx XPS PS/2 Port #%d", id);
> > +	snprintf(drvdata->serio.phys, sizeof(serio->phys),
> > +		 "xilinxps2/serio%d", id);
> > +	serio_register_port(serio);
> > +
> > +	return 0;		/* success */
> > +
> > +failed3:
> > +	iounmap(drvdata->base_address);
> > +
> > +failed2:
> > +	release_mem_region(regs_res->start, remap_size);
> > +
> > +failed1:
> > +	kfree(drvdata);
> > +	dev_set_drvdata(dev, NULL);
> > +
> > +	return retval;
> > +}
> > +
> > +/***************************/
> > +/* OF Platform Bus Support */
> > +/***************************/
> > +
> > +static int __devinit xps2_of_probe(struct of_device *ofdev, const
struct
> > +				   of_device_id * match)
> > +{
> > +	struct resource r_irq_struct;
> > +	struct resource r_mem_struct;
> > +	struct resource *r_irq =3D &r_irq_struct;	/* Interrupt resources
*/
> > +	struct resource *r_mem =3D &r_mem_struct;	/* IO mem resources */
> =

> This is very vebose; do this instead:
> =

> +	struct resource r_irq;
> +	struct resource r_mem;
> [...]
> +	rc =3D of_address_to_resource(ofdev->node, 0, &r_mem);
> [...]
> +	rc =3D of_irq_to_resource(ofdev->node, 0, &r_irq);
> [...]
> +	return xps2_setup(&ofdev->dev, id ? *id : -1, &r_mem, &r_irq);
> =

> > +	int rc =3D 0;
> > +	const unsigned int *id;
> > +
> > +	printk(KERN_INFO "Device Tree Probing \'%s\'\n",
> > +			ofdev->node->name);
> > +
> > +	/* Get iospace for the device */
> > +	rc =3D of_address_to_resource(ofdev->node, 0, r_mem);
> > +	if (rc) {
> > +		dev_warn(&ofdev->dev, "invalid address\n");
> =

> Probably should be dev_err(...);
> =

> > +		return rc;
> > +	}
> > +
> > +	/* Get IRQ for the device */
> > +	rc =3D of_irq_to_resource(ofdev->node, 0, r_irq);
> > +	if (rc =3D=3D NO_IRQ) {
> > +		dev_warn(&ofdev->dev, "no IRQ found\n");
> =

> ditto
> =

> > +		return rc;
> > +	}
> > +
> > +	id =3D of_get_property(ofdev->node, "port-number", NULL);
> =

> Drop this line.  port-number is a poor way to specify a global value.
> If really needed, you should search the aliases node for a matching
> property to get the id.  Otherwise, you should probably fall back to
> using the physical address of the device instead of -1.  At least it
> guarantees that the value is unique.
> =

> > +	return xps2_setup(&ofdev->dev, id ? *id : -1, r_mem, r_irq);
> > +}
> > +
> > +static int __devexit xps2_of_remove(struct of_device *of_dev)
> > +{
> > +	struct xps2data *drvdata;
> > +	struct device *dev;
> > +
> > +	dev =3D &of_dev->dev;
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	drvdata =3D (struct xps2data *)dev_get_drvdata(dev);
> > +
> > +	serio_unregister_port(&drvdata->serio);
> > +
> > +	iounmap(drvdata->base_address);
> > +
> > +	release_mem_region(drvdata->phys_addr, drvdata->remap_size);
> > +
> > +	kfree(drvdata);
> > +	dev_set_drvdata(dev, NULL);
> > +
> > +	return 0;		/* success */
> > +}
> > +
> > +/* Match table for of_platform binding */
> > +static struct of_device_id __devinitdata xps2_of_match[] =3D {
> =

> __devinitdata in the wrong place; should be:
> =

> +static struct of_device_id xps2_of_match[] __devinitdata =3D {
> =

> > +	{ .compatible =3D "xlnx,xps-ps2-1.00.a", },
> > +	{ /* end of list */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, xps2_of_match);
> > +
> > +static struct of_platform_driver xps2_of_driver =3D {
> > +	.name		=3D DRIVER_NAME,
> > +	.match_table	=3D xps2_of_match,
> > +	.probe		=3D xps2_of_probe,
> > +	.remove		=3D __devexit_p(xps2_of_remove),
> > +};
> > +
> > +static int __init xps2_init(void)
> > +{
> > +	return of_register_platform_driver(&xps2_of_driver);
> > +}
> > +
> > +static void __exit xps2_cleanup(void)
> > +{
> > +	of_unregister_platform_driver(&xps2_of_driver);
> > +}
> > +
> > +module_init(xps2_init);
> > +module_exit(xps2_cleanup);
> > +
> > +MODULE_AUTHOR("Xilinx, Inc.");
> > +MODULE_DESCRIPTION("Xilinx XPS PS/2 driver");
> > +MODULE_LICENSE("GPL");
> > +
> > --
> > 1.5.2.1
> >
> >
> >
> > This email and any attachments are intended for the sole use of the
named recipient(s) and
> contain(s) confidential information that may be proprietary,
privileged or copyrighted under
> applicable law. If you are not the intended recipient, do not read,
copy, or forward this email
> message or any attachments. Delete this email message and any
attachments immediately.
> >
> >


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.