Messages in this thread | | | From | Grant Likely <> | Date | Fri, 26 Mar 2010 15:33:41 -0600 | Subject | Re: [PATCH 3/4] dmaengine: xlldma platform bus driver |
| |
On Wed, Mar 17, 2010 at 10:26 AM, Steven J. Magnani <steve@digidescorp.com> wrote: > Platform bus attachment for the Xilinx MPMC Soft Direct Memory > Access Controller DMA engine. > > Signed-off-by: Steven J. Magnani <steve@digidescorp.com> > --- > diff -uprN a/drivers/dma/xlldma_plat.c b/drivers/dma/xlldma_plat.c > --- a/drivers/dma/xlldma_plat.c 1969-12-31 18:00:00.000000000 -0600 > +++ b/drivers/dma/xlldma_plat.c 2010-03-17 11:11:16.000000000 -0500
Not really worth putting into a separate file. Just put this hunk in the xlldma.c file to keep a few more symbols out of the global namespace.
> @@ -0,0 +1,222 @@ > +/* > + * Xilinx Multi-Port Memory Controller (MPMC) DMA Engine support > + * > + * Copyright (C) 2010 Digital Design Corporation. All rights reserved. > + * > + * Author: > + * Steve Magnani <steve@digidescorp.com>, Mar 2010 > + * > + * Description: > + * Platform bus attachment for the Xilinx MPMC Soft Direct Memory > + * Access Controller DMA engine. > + * > + * This 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. > + * > + */ > + > +#include <linux/platform_device.h> > +#include <linux/module.h> > + > +#include "xlldma.h" > + > +static int __devinit xlldma_plat_chan_probe(struct xlldma_device *xdev) > +{ > + struct xlldma_chan *chan; > + struct resource *plat_rsrc; > + int err; > + struct platform_device *pdev = to_platform_device(xdev->dev); > + char irq_name[9]; > + > + plat_rsrc = platform_get_resource(pdev, IORESOURCE_MEM, > + xdev->common.chancnt); > + if (!plat_rsrc) > + return -ENXIO; > + > + /* alloc channel */ > + chan = kzalloc(sizeof(struct xlldma_chan), GFP_KERNEL); > + if (!chan) { > + dev_err(xdev->dev, > + "No free memory for allocating dma channels!\n"); > + return -ENOMEM; > + } > + > + /* get dma channel register base */ > + chan->reg_base = NULL;
Superfluous statement.
> + > + /* map device registers */ > + chan->reg_base = ioremap(plat_rsrc->start, > + plat_rsrc->end - plat_rsrc->start + 1); > + if (chan->reg_base) { > + dev_dbg(xdev->dev, "MEM base: %p\n", chan->reg_base); > + } else { > + dev_err(xdev->dev, "unable to map DMA registers\n"); > + err = -ENXIO; > + goto err_no_reg; > + }
General convention is to do this the other way around. No else clause required:
if (!chan->reg_base) { /* bail out here */ } dev_dbg(...)
> + > + chan->xdev = xdev; > + > + chan->id = xdev->common.chancnt; > + if (chan->id >= XLLDMA_MAX_CHANS_PER_DEVICE) { > + dev_err(xdev->dev, "There is no %d channel!\n", chan->id); > + err = -EINVAL; > + goto err_no_chan; > + } > + xdev->chan[chan->id] = chan; > + tasklet_init(&chan->tasklet, xlldma_cleanup_tasklet, > + (unsigned long)chan); > + > + spin_lock_init(&chan->desc_lock); > + INIT_LIST_HEAD(&chan->desc_queue); > + INIT_LIST_HEAD(&chan->pending_free); > + > + chan->common.device = &xdev->common; > + > + /* Add the channel to DMA device channel list */ > + list_add_tail(&chan->common.device_node, &xdev->common.channels); > + xdev->common.chancnt++;
Don't you need a spinlock protecting the list access?
> + > + /* RX interrupt is required */ > + sprintf(irq_name, "chan%d-rx", chan->id);
Be defensive. Make irq_name at least 20 bytes long in case chan->id somehow becomes huge.
> + plat_rsrc = platform_get_resource_byname(pdev, IORESOURCE_IRQ, > + irq_name); > + if (!plat_rsrc) { > + dev_err(xdev->dev, "DMA channel %d - missing %s IRQ\n", > + chan->id, irq_name); > + err = -ENXIO; > + goto err_no_irq; > + } > + > + chan->rx_irq = plat_rsrc->start; > + BUG_ON(plat_rsrc->end != plat_rsrc->start); > + > + err = request_irq(chan->rx_irq, &xlldma_chan_rx_interrupt, > + IRQF_SHARED, "xlldma-rx", chan); > + if (err) { > + dev_err(xdev->dev, "request_irq error on %s, return %d\n", > + irq_name, err); > + err = -ENXIO; > + goto err_no_irq; > + } > + > + /* TX interrupt is optional - only needed for DMA_INTERRUPT */ > + irq_name[6] = 't';
And if irq # is in the double digits? This is clever, but unclear and potentially risky. Just do another sprintf()
> + plat_rsrc = platform_get_resource_byname(pdev, IORESOURCE_IRQ, > + irq_name); > + if (plat_rsrc) { > + chan->tx_irq = plat_rsrc->start; > + BUG_ON(plat_rsrc->end != plat_rsrc->start); > + } else { > + chan->tx_irq = NO_IRQ; > + }
put chan->tx_irq = NO_IRQ before the if(), and you can drop the else statement (a bit simpler).
> + > + if (chan->tx_irq != NO_IRQ) { > + err = request_irq(chan->tx_irq, &xlldma_chan_tx_interrupt, > + IRQF_SHARED, "xlldma-tx", chan); > + if (err) { > + dev_err(xdev->dev, > + "request_irq error on %s, return %d\n", > + irq_name, err); > + goto err_rx_irq; > + } > + } > + > + dev_info(xdev->dev, "#%d, tx irq %d, rx irq %d\n", chan->id, > + chan->tx_irq, chan->rx_irq); > + > + return 0; > +err_rx_irq: > + free_irq(chan->rx_irq, chan); > +err_no_irq: > + list_del(&chan->common.device_node); > +err_no_chan: > + iounmap(chan->reg_base); > +err_no_reg: > + kfree(chan); > + return err; > +} > + > +static int __devinit xlldma_plat_probe(struct platform_device *pdev) > +{ > + struct xlldma_device *xdev; > + int i, rc; > + > + dev_info(&pdev->dev, "Xilinx DMA engine...\n"); > + > + xdev = kzalloc(sizeof(struct xlldma_device), GFP_KERNEL); > + if (!xdev) { > + dev_err(&pdev->dev, "Not enough memory for 'priv'\n"); > + rc = -ENOMEM; > + goto fail_alloc; > + } > + xdev->dev = &pdev->dev; > + INIT_LIST_HEAD(&xdev->common.channels); > + > + xdev->common.dev = &pdev->dev; > + platform_set_drvdata(pdev, xdev); > + > + do { > + rc = xlldma_plat_chan_probe(xdev); > + } while (rc == 0); > + > + if (xdev->common.chancnt == 0) > + goto fail_nochan; > + > + rc = xlldma_device_setup(xdev->dev); > + if (rc == 0) > + return 0; > + > +/* fail: */ > + for (i = 0; i < XLLDMA_MAX_CHANS_PER_DEVICE; i++) { > + if (xdev->chan[i]) > + xlldma_chan_remove(xdev->chan[i]); > + } > +fail_nochan: > + platform_set_drvdata(pdev, NULL); > + kfree(xdev); > + > +fail_alloc: > + return rc; > +} > + > +static int __devexit xlldma_plat_remove(struct platform_device *pdev) > +{ > + return xlldma_device_teardown(&pdev->dev); > +} > + > +static struct platform_driver xlldma_driver = { > + .probe = xlldma_plat_probe, > + .remove = __devexit_p(xlldma_plat_remove), > + .driver = { > + .owner = THIS_MODULE, > + .name = "xlldma", > + }, > +}; > + > +static int __init xlldma_plat_init(void) > +{ > + int ret; > + > + pr_info("Xilinx LocalLink DMA driver\n"); > + > + ret = platform_driver_register(&xlldma_driver); > + if (ret) > + pr_err("xlldma: failed to register platform driver\n"); > + > + return ret; > +}
Don't bother with all this. sysfs will tell you whether or not the driver got registered. Do this instead:
+static int __init xlldma_plat_init(void) +{ + return platform_driver_register(&xlldma_driver); +}
> + > +static void __exit xlldma_plat_exit(void) > +{ > + platform_driver_unregister(&xlldma_driver); > +} > + > +module_init(xlldma_plat_init);
Put the module_init directly after the xlldma_plat_init() function definition.
> +module_exit(xlldma_plat_exit); > + > +MODULE_AUTHOR("Digital Design Corporation"); > +MODULE_DESCRIPTION("Xilinx LocalLink DMA Platform driver"); > +MODULE_LICENSE("GPL"); > > >
-- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |