Hello, Thanks for your contribution. This is quick review of your driver. First some general comments:
- Do we really need a separate driver interface for the CCAN? Is a platform driver with appropriate platform declarations in "include/linx/can/platform/ccan.h" not sufficient? - If such a CCAN driver interface makes sense, the code should go into a subdirectory named "ccan". - I would prefer "s/bosch_ccan/ccan/" for all names. - Various CAN controller modes, like listen_only and loopback can be handled via "priv->can.ctrlmode". Please use that interface if appropriate. - Do we need to care about the endianess? Some more comments inline. On 08/26/2010 06:41 AM, Bhupesh SHARMA wrote: > SPEAr320 design contains two boards: > a) CPU board (which houses the ARM926ejs CPU and DDR in addition to some > other interfaces like USB host etc..) > b) Application PLC board (which contains two Bosch CCAN IPs that are > interfaced to APB bus in addition to other interfaces like UART etc..) > [See details here: > http://www.st.com/stonline/products/families/embedded_mpu/spear_mpus/spear320_single_core.htm] > > The SPEAr CAN driver relies on 'platform data/board specific details' that > are passed by means of relevant evb files present in 'arch/arm/mach-spear3xx' > directory. > > Signed-off-by: Bhupesh Sharma <[email protected]> Please truncate to the usual < 80 lines for readability. > Index: spear320_can.c > =================================================================== > --- spear320_can.c (revision 0) > +++ spear320_can.c (revision 0) > @@ -0,0 +1,203 @@ > +/* > + * drivers/net/can/spear320_can.c This is redundant information, please remove. > + * > + * CAN bus driver for SPEAr320 SoC that houses two independent > + * Bosch CCAN controllers. > + * > + * Copyright (C) 2010 ST Microelectronics > + * Bhupesh Sharma <[email protected]> > + * > + * Borrowed heavily from the original code written for Hynix7202 by: > + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix <[email protected]> > + * - Simon Kallweit, intefo AG <[email protected]> Please keep the proper copyright notes from that code. > + * which can be viewed here: > + * http://svn.berlios.de/svnroot/repos/socketcan/trunk/kernel/2.6/ > + * drivers/net/can/old/ccan/ Please remove such links. > + * > + * TODO: > + * - Power Management support to be added > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/netdevice.h> > +#include <socketcan/can.h> > +#include <socketcan/can/dev.h> > + > +#include "bosch_ccan.h" > + > +#define DRV_NAME "spear_can" > +#define CAN_ENABLE 0x0e > +static u16 spear320_can_read_reg(const struct bosch_ccan_priv *priv, > + enum ccan_regs reg) > +{ > + u16 val; > + > + /* shifting 1 place because 16 bit registers are word aligned */ > + val = readw(priv->reg_base + (reg << 1)); > + return val; > +} > + > +static void spear320_can_write_reg(const struct bosch_ccan_priv *priv, > + enum ccan_regs reg, u16 val) > +{ > + /* shifting 1 place because 16 bit registers are word aligned */ > + writew(val, priv->reg_base + (reg << 1)); > +} > + > +static int spear320_can_drv_probe(struct platform_device *pdev) __devinit? s/drv_probe/probe/ ? > +{ > + int ret; > + void __iomem *addr; > + struct net_device *dev; > + struct bosch_ccan_priv *priv; > + struct resource *mem, *irq; > + struct clk *clk; > + > + /* get the appropriate clk */ > + clk = clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "no clock defined\n"); > + ret = -ENODEV; > + goto exit; > + } > + > + /* get the platform data */ > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (!mem || !irq) { > + ret = -ENODEV; > + goto exit_free_clk; > + } IIRC, we should use "irq <= 0" to handle invalid irq numbers. > + > + if (!request_mem_region(mem->start, resource_size(mem), DRV_NAME)) { > + dev_err(&pdev->dev, "resource unavailable\n"); > + ret = -ENODEV; > + goto exit_free_clk; > + } > + > + addr = ioremap(mem->start, resource_size(mem)); > + if (!addr) { > + dev_err(&pdev->dev, "failed to map can port\n"); > + ret = -ENOMEM; > + goto exit_release_mem; > + } > + > + /* allocate the ccan device */ > + dev = alloc_bosch_ccandev(0); > + if (!dev) { > + clk_put(clk); > + ret = -ENOMEM; > + goto exit_iounmap; > + } > + > + priv = netdev_priv(dev); > + dev->irq = irq->start; > + priv->irq_flags = irq->flags; > + priv->reg_base = addr; > + priv->can.clock.freq = clk_get_rate(clk); > + priv->read_reg = spear320_can_read_reg; > + priv->write_reg = spear320_can_write_reg; > + priv->clk = clk; > + > + platform_set_drvdata(pdev, dev); > + SET_NETDEV_DEV(dev, &pdev->dev); > + > + /* register ccan */ > + spear320_can_write_reg(priv, CAN_ENABLE, 1); > + ret = register_bosch_ccandev(dev); > + if (ret) { > + dev_err(&pdev->dev, "registering %s failed (err=%d)\n", > + DRV_NAME, ret); > + goto exit_free_device; > + } > + > + dev_info(&pdev->dev, "%s device registered (reg_base=%p, irq=%d)\n", > + DRV_NAME, priv->reg_base, dev->irq); > + return 0; > + > +exit_free_device: > + platform_set_drvdata(pdev, NULL); > + free_bosch_ccandev(dev); > +exit_iounmap: > + iounmap(addr); > +exit_release_mem: > + release_mem_region(mem->start, resource_size(mem)); > +exit_free_clk: > + clk_put(clk); > +exit: > + dev_err(&pdev->dev, "probe failed\n"); > + > + return ret; > +} > + > +static int spear320_can_drv_remove(struct platform_device *pdev) __devexit ? s/drv_remove/remove/ ? > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + struct bosch_ccan_priv *priv = netdev_priv(dev); > + struct resource *mem; > + > + unregister_bosch_ccandev(dev); > + platform_set_drvdata(pdev, NULL); > + > + if (priv->reg_base) > + iounmap(priv->reg_base); > + > + clk_put(priv->clk); > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + release_mem_region(mem->start, resource_size(mem)); > + > + free_bosch_ccandev(dev); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int spear320_can_drv_suspend(struct platform_device *pdev, > + pm_message_t state) > +{ > + return 0; > +} > + > +static int spear320_can_drv_resume(struct platform_device *pdev) > +{ > + return 0; > +} > +#endif /* CONFIG_PM */ Please remove as long as there is no functional PM support. > + > +static struct platform_driver spear320_can_driver = { > + .driver = { > + .name = DRV_NAME, > + }, > + .probe = spear320_can_drv_probe, > + .remove = spear320_can_drv_remove, __devexit_p ? > +#ifdef CONFIG_PM > + .suspend = spear320_can_drv_suspend, > + .resume = spear320_can_drv_resume, > +#endif /* CONFIG_PM */ Ditto. > +}; > + > +static int __init spear320_can_init(void) > +{ > + return platform_driver_register(&spear320_can_driver); > +} > +module_init(spear320_can_init); > + > +static void __exit spear320_can_cleanup(void) > +{ > + platform_driver_unregister(&spear320_can_driver); > +} > +module_exit(spear320_can_cleanup); > + > +MODULE_AUTHOR("Bhupesh Sharma <[email protected]>"); > +MODULE_LICENSE("GPL"); GPL v2 ? > +MODULE_DESCRIPTION("CAN bus driver for SPEAr320 which has 2 CCAN > controllers"); s/ which has 2 CCAN controllers// ? The number of devices is usually defined by the platform code. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
