>>>>> "John" == John Linn <john.linn@xxxxxxxxxx> writes:
> Review comments were incorporated to improve the driver.
> 1. Some data was eliminated that was not needed.
> 2. Renaming of variables for clarity.
> 3. Removed unneeded type casting.
> 4. Changed to use dev_err rather than other I/O.
> 5. Merged together some functions.
> 6. Added kerneldoc format to functions.
> Signed-off-by: Sadanand <sadanan@xxxxxxxxxx>
> Signed-off-by: John Linn <john.linn@xxxxxxxxxx>
If the minor issues below gets fixed:
Acked-by: Peter Korsgaard <jacmet@xxxxxxxxxx>
> This patch is an incremental patch to be applied to
> [V3] powerpc: Xilinx: PS2: Added new XPS PS2 driver.
> drivers/input/serio/xilinx_ps2.c | 220 +++++++++++++++++++++----------------
> 1 files changed, 125 insertions(+), 95 deletions(-)
> diff --git a/drivers/input/serio/xilinx_ps2.c b/drivers/input/serio/xilinx_ps2.c
> index e86f11b..eba46c7 100644
> --- a/drivers/input/serio/xilinx_ps2.c
> +++ b/drivers/input/serio/xilinx_ps2.c
> @@ -58,23 +58,20 @@
> /* Mask for all the Receive Interrupts */
> #define XPS2_IPIXR_RX_ALL (XPS2_IPIXR_RX_OVF | XPS2_IPIXR_RX_ERR | \
> - XPS2_IPIXR_RX_FULL)
> + 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)
> + 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;
> + unsigned int flags;
> struct serio serio; /* serio */
> };
> @@ -82,8 +79,13 @@ struct xps2data {
> /* XPS PS/2 data transmission calls */
> /************************************/
> -/*
> - * xps2_recv() will attempt to receive a byte of data from the PS/2 port.
> +/**
> + * xps2_recv() - attempts to receive a byte from the PS/2 port.
> + * @drvdata: pointer to ps2 device private data structure
> + * @byte: address where the read data will be copied
> + *
> + * If there is any data available in the PS/2 receiver, this functions reads
> + * the data, otherwise it returns error.
> */
> static int xps2_recv(struct xps2data *drvdata, u8 *byte)
> {
> @@ -105,7 +107,7 @@ static int xps2_recv(struct xps2data *drvdata, u8 *byte)
> /*********************/
> static irqreturn_t xps2_interrupt(int irq, void *dev_id)
> {
> - struct xps2data *drvdata = (struct xps2data *)dev_id;
> + struct xps2data *drvdata = dev_id;
> u32 intr_sr;
> u8 c;
> int status;
> @@ -115,35 +117,28 @@ static irqreturn_t xps2_interrupt(int irq, void *dev_id)
> 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_OVF)
> + dev_err(drvdata->serio.dev.parent, "receive overrun error\n");
> if (intr_sr & XPS2_IPIXR_RX_ERR)
> - drvdata->dfl |= SERIO_PARITY;
> + drvdata->flags |= SERIO_PARITY;
> if (intr_sr & (XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_WDT_TOUT))
> - drvdata->dfl |= SERIO_TIMEOUT;
> + drvdata->flags |= SERIO_TIMEOUT;
> if (intr_sr & XPS2_IPIXR_RX_FULL) {
> - status = xps2_recv(drvdata, &drvdata->rxb);
> + status = xps2_recv(drvdata, &c);
> /* Error, if a byte is not received */
> if (status) {
> - printk(KERN_ERR
> - "%s: wrong rcvd byte count (%d)\n",
> - drvdata->serio.name, status);
> + dev_err(drvdata->serio.dev.parent,
> + "wrong rcvd byte count\n");
You used to print the byte count - Isn't that interesting debugging
data?
> } else {
> - c = drvdata->rxb;
> - serio_interrupt(&drvdata->serio, c, drvdata->dfl);
> - drvdata->dfl = 0;
> + serio_interrupt(&drvdata->serio, c, drvdata->flags);
> + drvdata->flags = 0;
> }
> }
> - if (intr_sr & XPS2_IPIXR_TX_ACK)
> - drvdata->dfl = 0;
> -
> return IRQ_HANDLED;
> }
> @@ -151,8 +146,15 @@ static irqreturn_t xps2_interrupt(int irq, void *dev_id)
> /* serio callbacks */
> /*******************/
> -/*
> - * sxps2_write() sends a byte out through the PS/2 interface.
> +/**
> + * sxps2_write() - sends a byte out through the PS/2 port.
> + * @pserio: pointer to the serio structure of the PS/2 port
> + * @c: data that needs to be written to the PS/2 port
> + *
> + * This fucntion checks if the PS/2 transmitter is empty and sends a byte.
s/fucntion/function/
> + * Otherwise it returns error. Transmission fails only when nothing is connected
> + * to the PS/2 port. Thats why, we do not try to resend the data in case of a
> + * failure.
> */
> static int sxps2_write(struct serio *pserio, unsigned char c)
> {
> @@ -173,33 +175,39 @@ static int sxps2_write(struct serio *pserio, unsigned char c)
> return status;
> }
> -/*
> - * sxps2_open() is called when a port is open by the higher layer.
> +/**
> + * sxps2_open() - called when a port is opened by the higher layer.
> + * @pserio: pointer to the serio structure of the PS/2 device
> + *
> + * This function requests irq and enables interrupts for the PS/2 device.
> */
> static int sxps2_open(struct serio *pserio)
> {
> struct xps2data *drvdata = pserio->port_data;
> int retval;
> + u8 c;
> 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);
> + dev_err(drvdata->serio.dev.parent,
> + "Couldn't allocate interrupt %d\n", 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);
> + (void)xps2_recv(drvdata, &c);
> return 0; /* success */
> }
> -/*
> - * sxps2_close() frees the interrupt.
> +/**
> + * sxps2_close() - frees the interrupt.
> + * @pserio: pointer to the serio structure of the PS/2 device
> + *
> + * This function frees the irq and disables interrupts for the PS/2 device.
> */
> static void sxps2_close(struct serio *pserio)
> {
> @@ -211,21 +219,60 @@ static void sxps2_close(struct serio *pserio)
> free_irq(drvdata->irq, drvdata);
> }
> -/*********************/
> -/* Device setup code */
> -/*********************/
> +/***************************/
> +/* OF Platform Bus Support */
> +/***************************/
> -static int xps2_setup(struct device *dev, struct resource *regs_res,
> - struct resource *irq_res)
> +/**
> + * xps2_of_probe - probe method for the PS/2 device.
> + * @of_dev: pointer to OF device structure
> + * @match: pointer to the stucture used for matching a device
> + *
> + * This function probes for a matching PS/2 device using OF properties and
> + * attaches it to the platform bus. It initializes the driver data structure
> + * and also initializes the hardware.
> + * It returns 0, if the driver is bound to the PS/2 device, or a negative value
> + * if there is an error. It releases all the allocated resources in case of an
> + * error.
The function doesn't have anything to do with the platform bus, and
the comment is imho more verbose than needed.
> + *
> + */
> +static int __devinit xps2_of_probe(struct of_device *ofdev, const struct
> + of_device_id * match)
> {
> + struct resource r_irq; /* Interrupt resources */
> + struct resource r_mem; /* IO mem resources */
> struct xps2data *drvdata;
> struct serio *serio;
> - unsigned long remap_size;
> - int retval;
> + struct device *dev;
> + resource_size_t remap_size, phys_addr;
> + int rc = 0;
> + dev = &ofdev->dev;
> if (!dev)
> return -EINVAL;
> + dev_info(dev, "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_err(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_err(dev, "no IRQ found\n");
> + return rc;
> + }
> +
> + if (!(&r_mem) || !(&r_irq)) {
> + dev_err(dev, "IO resource(s) not found\n");
> + return -EFAULT;
> + }
How can this ever fail? They are local structures, not pointers
> +
> drvdata = kzalloc(sizeof(struct xps2data), GFP_KERNEL);
> if (!drvdata) {
> dev_err(dev, "Couldn't allocate device private record\n");
> @@ -234,31 +281,24 @@ static int xps2_setup(struct device *dev, struct resource *regs_res,
> spin_lock_init(&drvdata->lock);
> 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)) {
> + drvdata->irq = r_irq.start;
> + phys_addr = r_mem.start;
> + remap_size = r_mem.end - r_mem.start + 1;
> + if (!request_mem_region(phys_addr, remap_size, DRIVER_NAME)) {
> dev_err(dev, "Couldn't lock memory region at 0x%08X\n",
> - (unsigned int)regs_res->start);
> - retval = -EBUSY;
> + (unsigned int)phys_addr);
> + rc = -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);
> + drvdata->base_address = ioremap(phys_addr, 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;
> + (unsigned int)phys_addr);
> + rc = -EFAULT;
> goto failed2;
> }
> @@ -270,7 +310,8 @@ static int xps2_setup(struct device *dev, struct resource *regs_res,
> out_be32(drvdata->base_address + XPS2_SRST_OFFSET, XPS2_SRST_RESET);
> dev_info(dev, "Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=%d\n",
> - drvdata->phys_addr, (u32)drvdata->base_address, drvdata->irq);
> + (unsigned int)phys_addr, (u32)drvdata->base_address,
> + drvdata->irq);
> serio = &drvdata->serio;
serio-> id.type = SERIO_8042;
> @@ -280,58 +321,38 @@ static int xps2_setup(struct device *dev, struct resource *regs_res,
serio-> port_data = drvdata;
serio-> dev.parent = dev;
> snprintf(drvdata->serio.name, sizeof(serio->name),
> - "Xilinx XPS PS/2 at %08X", drvdata->phys_addr);
> + "Xilinx XPS PS/2 at %08X", (unsigned int)phys_addr);
> snprintf(drvdata->serio.phys, sizeof(serio->phys),
> - "xilinxps2/serio at %08X", drvdata->phys_addr);
> + "xilinxps2/serio at %08X", (unsigned int)phys_addr);
> serio_register_port(serio);
> return 0; /* success */
> failed2:
> - release_mem_region(regs_res->start, remap_size);
> + release_mem_region(phys_addr, 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; /* Interrupt resources */
> - struct resource r_mem; /* IO mem resources */
> - int rc = 0;
> -
> - 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_err(&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_err(&ofdev->dev, "no IRQ found\n");
> - return rc;
> - }
> -
> - return xps2_setup(&ofdev->dev, &r_mem, &r_irq);
> + return rc;
> }
> +/**
> + * xps2_of_remove - unbinds the driver from the PS/2 device.
> + * @of_dev: pointer to OF device structure
> + *
> + * This function is called if a device is physically removed from the system or
> + * if the driver module is being unloaded. It checks if the device is present
Where's that check?
> + * and frees any resources allocated to the device.
> + */
> static int __devexit xps2_of_remove(struct of_device *of_dev)
> {
> struct xps2data *drvdata;
> struct device *dev;
> + struct resource r_mem; /* IO mem resources */
> + resource_size_t phys_addr, remap_size;
> + int rc;
> dev = &of_dev->dev;
> if (!dev)
> @@ -343,7 +364,16 @@ static int __devexit xps2_of_remove(struct of_device *of_dev)
> iounmap(drvdata->base_address);
> - release_mem_region(drvdata->phys_addr, drvdata->remap_size);
> + /* Get iospace of the device */
> + rc = of_address_to_resource(of_dev->node, 0, &r_mem);
> + if (rc) {
> + dev_err(dev, "invalid address\n");
> + return rc;
> + }
> +
> + phys_addr = r_mem.start;
> + remap_size = r_mem.end - r_mem.start + 1;
> + release_mem_region(phys_addr, remap_size);
You only use phys_addr / remap_size once - just use r_mem directly instead.
> kfree(drvdata);
> dev_set_drvdata(dev, NULL);
> --
> 1.5.2.1
--
Bye, Peter Korsgaard
--
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
