On Mon, Jun 30, 2008 at 07:24:48AM -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@xxxxxxxxxx>
> Signed-off-by: John Linn <john.linn@xxxxxxxxxx>
I don't know much about the serio conventions, but I can make some
general comments...
from MAINTAINERS:
INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN) DRIVERS
P: Dmitry Torokhov
M: dmitry.torokhov@xxxxxxxxx
M: dtor@xxxxxxx
L: linux-input@xxxxxxxxxxxxxxx
T: git kernel.org:/pub/scm/linux/kernel/git/dtor/input.git
S: Maintained
You need to cc: the linux-input mailing list and Dimitry when you post
the next version of this driver.
> ---
> drivers/input/serio/Kconfig | 5 +
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/xilinx_ps2.c | 464 ++++++++++++++++++++++++++++++++++++++
> drivers/input/serio/xilinx_ps2.h | 97 ++++++++
> 4 files changed, 567 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.
> +
Consider moving this block to somewhere in the middle of the file to
reduce the possibility of merge conflicts.
> 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
Ditto.
> diff --git a/drivers/input/serio/xilinx_ps2.c b/drivers/input/serio/xilinx_ps2.c
> new file mode 100644
> index 0000000..670d47f
> --- /dev/null
> +++ b/drivers/input/serio/xilinx_ps2.c
> @@ -0,0 +1,464 @@
> +/*
> + * xilinx_ps2.c
Don't put the .c filename in the header block.
> + *
> + * Xilinx PS/2 driver to interface PS/2 component to Linux
> + *
> + * Author: MontaVista Software, Inc.
> + * source@xxxxxxxxxx
Is this true anymore?
> + *
> + * (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.
> + */
> +
> +
> +#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
> +
> +#include "xilinx_ps2.h"
This header can simple be rolled into this .c file because the driver no
longer has multiple .c files.
> +#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
> +
> +
> +static DECLARE_MUTEX(cfg_sem);
This mutex should be part of the driver private data structure
> +
> +/*********************/
> +/* 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.
> + */
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
> +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 */
> +}
Since xps2_recv() and xps2_send() are used by the sxps2_* routines and
by the irq handler, they should be moved to about them in this file.
> +
> +/******************************/
> +/* The platform device driver */
> +/******************************/
You can drop the platform driver bindings for inclusion in mainline.
<snip>
> +static struct device_driver xps2_driver = {
> + .name = DRIVER_NAME,
> + .bus = &platform_bus_type,
> + .probe = xps2_probe,
> + .remove = xps2_remove
> +};
This is platform bus stuff that will disappear anyway, but I'm going to
make a comment here regardless. When registering a platform bus device
driver, you should use 'struct platform_driver' instead of 'struct
device_driver', and it should be registered with
platform_driver_register().
> 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.
Please drop this.
<snip>
> +
> +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);
These can be removed by reorganizing the .c file so that callees are
always above the caller.
Nice driver.
Cheers,
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
