Hi Wolfgang,
> -----Original Message-----
> From: Wolfgang Grandegger [mailto:[email protected]]
> Sent: Tuesday, August 31, 2010 2:05 PM
> To: Bhupesh SHARMA
> Cc: [email protected]
> Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver
>
> 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?
Yes, I will explain the case below.
Initial thoughts were to develop a CAN driver for SPEAr320 SoC.
Now, this SOC uses two Bosch CCAN controllers glued together with some glue
logic
to work with the APB bus and SPEAr.
The ways to do it could have been two:
- Develop a SPEAr320 CAN driver which includes everything (on the lines
of ti_hecc.c CAN driver..)
But, this would have limited the Bosch CCAN implementation as per
SPEAr glue logic and hence anyone
using Bosch CCAN controller inside their CAN implementations would
have to write separate code for the same.
- Develop two separate drivers: SPEAr320 CAN driver and Bosch CCAN
driver which
demarcate the platform details of SPEAr and the glue logic specific
to a SPEAr SoC and
the Bosch CCAN driver functions will be called by SPEAr CAN driver as
desired.
The second way seems more flexible as it allows any design with underlying
Bosch CCAN controllers to use
the Bosch CCAN driver.
It is also particularly useful as we now have a new SPEAr1310 SoC which also
uses the Bosch CCAN controller
, but the glue logic has changed from SPEAr320 and hence we can handle most of
the platform/machine specific details
in SPEAr1310 driver and keep Bosch CCAN driver as it is.
> - If such a CCAN driver interface makes sense, the code should go into
> a subdirectory named "ccan".
I would not prefer that, as then the SPEAr driver files that invoke the Bosch
CCAN
driver functionalities will also be have to be kept here which does not seem
logical.
> - I would prefer "s/bosch_ccan/ccan/" for all names.
Ditto.
> - Various CAN controller modes, like listen_only and loopback can be
> handled via "priv->can.ctrlmode". Please use that interface if
> appropriate.
Ack, as discussed in 2/4 patch comments.
> - Do we need to care about the endianess?
I will carefully check this but I cannot comprehend endianess as
an issue as of now. Can you please explain the details.
> 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/sp
> ear320_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.
Ack. V2 will correct this.
> > + *
> > + * 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.
Ack. V2 will correct this.
> > + * 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.
Ack. V2 will correct this.
> > + *
> > + * 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.
Ack. V2 will correct this.
> > +
> > + 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.
Ack. V2 will correct this.
> > +
> > +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.
Ack. V2 will correct this.
> > +};
> > +
> > +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 ?
Ack. V2 will correct this.
> > +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.
>
Ack. V2 will correct this.
Regards,
Bhupesh
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
