Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
> "John" == John Linn <[EMAIL PROTECTED]> writes:
> Added a new driver for Xilinx XPS PS2 IP. This driver is
> a flat driver to better match the Linux driver pattern.
This should probably go to the linux-input@ list as well.
> Signed-off-by: Sadanand <[EMAIL PROTECTED]>
> Signed-off-by: John Linn <[EMAIL PROTECTED]>
> ---
> 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.
> +
> 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 000..670d47f
> --- /dev/null
> +++ b/drivers/input/serio/xilinx_ps2.c
> @@ -0,0 +1,464 @@
> +/*
> + * xilinx_ps2.c
> + *
> + * Xilinx PS/2 driver to interface PS/2 component to Linux
> + *
> + * Author: MontaVista Software, Inc.
> + * [EMAIL PROTECTED]
> + *
> + * (c) 2005 MontaVista Software, Inc.
> + * (c) 2008 Xilinx Inc.
> + *
Is the montavista stuff still valid?
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
linux/io.h please.
> +
> +#ifdef CONFIG_OF/* For open firmware */
Why support !CONFIG_OF?
> + #include
> + #include
> +#endif /* CONFIG_OF */
> +
> +#include "xilinx_ps2.h"
Why the seperate header? You're the only user of it, right?
> +
> +#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"
Why have defines to stuff only used once?
...
> +
> +/**/
> +/* The platform device driver */
> +/**/
> +
> +static int xps2_probe(struct device *dev)
__devinit please.
> +{
> +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);
> +}
...
> +
> +/*
> + * xps2_remove() dissociates the driver with the Xilinx PS/2 device.
> + */
> +static int xps2_remove(struct device *dev)
__devexit please.
> +{
> +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 = {
Please use a real struct platform_driver instead.
> +.name = DRIVER_NAME,
> +.bus = &platform_bus_type,
> +.probe = xps2_probe,
> +.remove = xps2_remove
>
RE: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
Thanks Peter, will do.
-Original Message-
From: Peter Korsgaard [mailto:[EMAIL PROTECTED] On Behalf Of Peter
Korsgaard
Sent: Monday, June 30, 2008 8:45 AM
To: John Linn
Cc: linuxppc-dev@ozlabs.org; Sadanand Mutyala
Subject: Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
>>>>> "John" == John Linn <[EMAIL PROTECTED]> writes:
> Added a new driver for Xilinx XPS PS2 IP. This driver is
> a flat driver to better match the Linux driver pattern.
This should probably go to the linux-input@ list as well.
> Signed-off-by: Sadanand <[EMAIL PROTECTED]>
> Signed-off-by: John Linn <[EMAIL PROTECTED]>
> ---
> 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.
> +
> 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 000..670d47f
> --- /dev/null
> +++ b/drivers/input/serio/xilinx_ps2.c
> @@ -0,0 +1,464 @@
> +/*
> + * xilinx_ps2.c
> + *
> + * Xilinx PS/2 driver to interface PS/2 component to Linux
> + *
> + * Author: MontaVista Software, Inc.
> + * [EMAIL PROTECTED]
> + *
> + * (c) 2005 MontaVista Software, Inc.
> + * (c) 2008 Xilinx Inc.
> + *
Is the montavista stuff still valid?
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
linux/io.h please.
> +
> +#ifdef CONFIG_OF/* For open firmware */
Why support !CONFIG_OF?
> + #include
> + #include
> +#endif /* CONFIG_OF */
> +
> +#include "xilinx_ps2.h"
Why the seperate header? You're the only user of it, right?
> +
> +#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"
Why have defines to stuff only used once?
...
> +
> +/**/
> +/* The platform device driver */
> +/**/
> +
> +static int xps2_probe(struct device *dev)
__devinit please.
> +{
> +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);
> +}
...
> +
> +/*
> + * xps2_remove() dissociates the driver with the Xilinx PS/2 device.
> + */
> +static i
Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
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 <[EMAIL PROTECTED]>
> Signed-off-by: John Linn <[EMAIL PROTECTED]>
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: [EMAIL PROTECTED]
M: [EMAIL PROTECTED]
L: [EMAIL PROTECTED]
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 000..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.
> + * [EMAIL PROTECTED]
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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#ifdef CONFIG_OF /* For open firmware */
> + #include
> + #include
> +#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
Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
Hi Grant, John,
On Mon, Jun 30, 2008 at 11:16:28AM -0600, Grant Likely wrote:
> On Mon, Jun 30, 2008 at 07:24:48AM -0700, John Linn wrote:
> > +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
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#ifdef CONFIG_OF /* For open firmware */
> > + #include
> > + #include
> > +#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 -
Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
On Mon, Jun 30, 2008 at 02:10:23PM -0400, Dmitry Torokhov wrote:
> 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:
> > > +/*
> > > + * 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.
Just to complete this discussion, quoting from kernel-doc-nano-HOWTO.txt:
"We also recommend providing kernel-doc formatted documentation
for private (file "static") routines, for consistency of kernel
source code layout. But this is lower priority and at the
discretion of the MAINTAINER of that kernel source file."
but it's just a nitpick, and I'll say nothing more about it.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
RE: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
Thanks Peter, those make sense. We'll incorporate those comments. -Original Message- From: Peter Korsgaard [mailto:[EMAIL PROTECTED] On Behalf Of Peter Korsgaard Sent: Monday, June 30, 2008 8:45 AM To: John Linn Cc: linuxppc-dev@ozlabs.org; Sadanand Mutyala Subject: Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver >>>>> "John" == John Linn <[EMAIL PROTECTED]> writes: > > Added a new driver for Xilinx XPS PS2 IP. This driver is > > a flat driver to better match the Linux driver pattern. > >This should probably go to the linux-input@ list as well. Agreed, done. > > > Signed-off-by: Sadanand <[EMAIL PROTECTED]> > > Signed-off-by: John Linn <[EMAIL PROTECTED]> > > --- > > 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. > > + > > 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 000..670d47f > > --- /dev/null > > +++ b/drivers/input/serio/xilinx_ps2.c > > @@ -0,0 +1,464 @@ > > +/* > > + * xilinx_ps2.c > > + * > > + * Xilinx PS/2 driver to interface PS/2 component to Linux > > + * > > + * Author: MontaVista Software, Inc. > > + *[EMAIL PROTECTED] > > + * > > + * (c) 2005 MontaVista Software, Inc. > > + * (c) 2008 Xilinx Inc. > > + * > >Is the montavista stuff still valid? > We derived the driver from MontaVista code is the reason we left it in. Since a significant part of the code was derived, I think we defaulted to leave it in. > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > >linux/io.h please. > Done in v2. > > + > > +#ifdef CONFIG_OF /* For open firmware */ > >Why support !CONFIG_OF? > Agreed, we just got in the habit. > > + #include > > + #include > > +#endif /* CONFIG_OF */ > > + > > +#include "xilinx_ps2.h" > >Why the seperate header? You're the only user of it, right? > That's true, can be incorporated into the c file. > > + > > +#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" > >Why have defines to stuff only used once? > Agreed, out of habit. >... > > > + > > +/**/ > > +/* The platform device driver */ > > +/**/ > > + > > +static int xps2_probe(struct device *dev) > >__devinit
RE: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
Thanks Grant. Sounds like there's a few things that not everyone does the same based on comments from others. In general, all pretty easy changes that in general make sense. Thanks, John >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 <[EMAIL PROTECTED]> >> Signed-off-by: John Linn <[EMAIL PROTECTED]> > >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: [EMAIL PROTECTED] >M: [EMAIL PROTECTED] >L: [EMAIL PROTECTED] >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. > Makes sense, gotta get into the habit of that. >> --- >> 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. > Sounds like this is not needed by Dmitry, but maybe it's a better habit to get into. >> 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 000..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. OK. > >> + * >> + * Xilinx PS/2 driver to interface PS/2 component to Linux >> + * >> + * Author: MontaVista Software, Inc. >> + * [EMAIL PROTECTED] > >Is this true anymore? > Not clear to me since we derived the code from an existing driver. Can be removed. >> + * >> + * (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. > Sounds like there's not consistency as Dmitry said it can be left. Fine either way. What is the best habit to get into that will satisfy the majority of the crowd? >> + */ >> + >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#ifdef CONFIG_OF/* For open firmware */ >> + #include >> + #include >> +#endif /* CONFIG_OF */ > >This is a given for mainline since arch/ppc will not exist in 2.6.27 > Agreed, we'll remove. >> + >> +#include "xilinx_ps2.h" > >This header can simple be rolled into this .c file because the driver no >longer has multiple .c files. > Agreed. > >> +#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 > Agreed. >> + >> + >> +static DECLARE_MUTEX(cfg_sem); > >This mutex should be part of the driver private data structure > OK. I assume just trying to keep all the driver private data together. >> + >> +/*/ >> +/* Interrupt handler */ >> +/**
Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver
On Mon, Jun 30, 2008 at 02:00:12PM -0600, John Linn wrote: > > > >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. > > > > OK, keeping them close makes maintenance easier I guess. > Oops, there is a typo in my comment: "... they should be moved to *above* them in this file." 'about' doesn't make much sense. g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
