Hi Grant, John,
On Mon, Jun 30, 2008 at 11:16:28AM -0600, Grant Likely wrote:
> On Mon, Jun 30, 2008 at 07:24:48AM -0700, John Linn wrote:
> > +config SERIO_XILINX_XPS_PS2
> > + tristate "Xilinx XPS PS/2 Controller Support"
> > + help
> > + This driver supports XPS PS/2 IP from Xilinx EDK.
> > +
>
> Consider moving this block to somewhere in the middle of the file to
> reduce the possibility of merge conflicts.
>
I can take care of that, no worries.
> > + *
> > + * (c) 2005 MontaVista Software, Inc.
> > + * (c) 2008 Xilinx Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write to the Free Software Foundation, Inc.,
> > + * 675 Mass Ave, Cambridge, MA 02139, USA.
>
> These two paragraphs are redundant. Being in the Linux source tree
> implies that it is GPL licensed. You can remove them.
>
I prefer having the statement in right in the code actually. While
being the in kernel implies that the code is GPLv2 compatible it could
be dual-licensed or GPLv2 only. This removes any doubt as to what
license is used on this particular piece of code.
> > + */
> > +
> > +
> > +#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);
Don't you want to bail out here? Otherwise you will feed garbage to
serio_interrupt() I think.
> > + }
> > + c = drvdata->rxb;
> > + serio_interrupt(&drvdata->serio, c, drvdata->dfl);
> > + drvdata->dfl = 0;
> > + }
> > +
> > + if (intr_sr & XPS2_IPIXR_TX_ACK) {
> > +
> > + /* Disable the TX interrupts after the transmission is
> > + * complete */
> > + ier = in_be32(drvdata->base_address + XPS2_IPIER_OFFSET);
> > + ier &= (~(XPS2_IPIXR_TX_ACK & XPS2_IPIXR_ALL ));
> > + out_be32(drvdata->base_address + XPS2_IPIER_OFFSET, ier);
> > + drvdata->dfl = 0;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*******************/
> > +/* serio callbacks */
> > +/*******************/
> > +
> > +/*
> > + * sxps2_write() sends a byte out through the PS/2 interface.
> > + *
> > + * The sole purpose of drvdata->tx_end is to prevent the driver
> > + * from locking up in the do {} while; loop when nothing is connected
> > + * to the given PS/2 port. That's why we do not try to recover
> > + * from the transmission failure.
> > + * drvdata->tx_end needs not to be initialized to some "far in the
> > + * future" value, as the very first attempt to xps2_send() a byte
> > + * is always successful, and drvdata->tx_end will be set to a proper
> > + * value at that moment - before the 1st use in the comparison.
> > + */
>
> Good comment block.
>
> nitpick: can you reformat the comment blocks to be in kerneldoc format?
> That will allow the automatic document generation tools to parse it.
>
> see: Documentation/kernel-doc-nano-HOWTO.txt
This is an internal function so its not going to be exposed in
kerneldoc though.
>
> > +static int sxps2_write(struct serio *pserio, unsigned char c)
> > +{
> > + struct xps2data *drvdata = pserio->port_data;
> > + unsigned long flags;
> > + int retval;
> > +
> > + do {
> > + spin_lock_irqsave(&drvdata->lock, flags);
> > + retval = xps2_send(drvdata, &c);
> > + spin_unlock_irqrestore(&drvdata->lock, flags);
> > +
> > + if (retval == 1) {
> > + drvdata->tx_end = jiffies + HZ;
> > + return 0; /* success */
> > + }
> > + } while (!time_after(jiffies, drvdata->tx_end));
The logic escapes me... Let's say you send a byte and time when
jiffies were 10000 and now it is time 20000 and we try to send another
byte. Our first attempt fails and with time fence 11000 (HZ=1000) we
bail out of sxps2_write() after the very first unsiccessful attempt.
Is this what you intended?
> > +
> > + return 1; /* transmission is frozen */
It is better to return a negative on error, even if you don't report
actual -EXXXX error code.
--
Dmitry
--
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
