Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:[email protected]]
> Sent: Thursday, August 26, 2010 3:15 PM
> To: Bhupesh SHARMA
> Cc: [email protected]
> Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver
>
> 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/s
> > pear320_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]>
> >
> > Index: spear320_can.c
> > ===================================================================
> > --- spear320_can.c (revision 0)
> > +++ spear320_can.c (revision 0)
> > @@ -0,0 +1,203 @@
> > +/*
> > + * drivers/net/can/spear320_can.c
> > + *
> > + * 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]>
> > + * which can be viewed here:
> > + * http://svn.berlios.de/svnroot/repos/socketcan/trunk/kernel/2.6/
> > + * drivers/net/can/old/ccan/
> > + *
> > + * 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) {
> > + 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;
> > + }
> > +
> > + 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);
> ^^^^^^^^^^^^
> this is already done in the common error handling path, isn't it?
ACK. Will be corrected in V2.
>
> > + 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) {
> > + 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);
>
> I think reg_base is always set, isn't it?
ACK. Will be corrected in V2.
> > +
> > + clk_put(priv->clk);
> > +
> > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + release_mem_region(mem->start, resource_size(mem));
> > +
> > + free_bosch_ccandev(dev);
>
> this isn't look symetrically to the probe function.
ACK. Will be correct in V2.
> > +
> > + 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 PM if not implemented
ACK. Will be corrected in V2.
> > +
> > +static struct platform_driver spear320_can_driver = {
> > + .driver = {
> > + .name = DRV_NAME,
> > + },
> > + .probe = spear320_can_drv_probe,
> > + .remove = spear320_can_drv_remove,
> > +#ifdef CONFIG_PM
> > + .suspend = spear320_can_drv_suspend,
> > + .resume = spear320_can_drv_resume,
> > +#endif /* CONFIG_PM */
> > +};
> > +
> > +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"); MODULE_DESCRIPTION("CAN bus driver for
> > +SPEAr320 which has 2 CCAN controllers");
> >
Regards,
Bhupesh
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
