Re: [PATCH 3/4] powerpc/p1010: FlexCAN Controller for of_ type

[Marc Kleine-Budde]

On 08/09/2011 12:29 PM, U Bhaskar-B22300 wrote:
>> NACK - your patch does more than the description states (debug code).
> [Bhaskar] Ok I will change description as below
>       This patch introduces the following features
>       1. provides the support for FlexCAN on P1010 SoC.
>       2. Provides the common read/write interface for ARM and PowerPC based 
> FlexCAN.
>       3. Provides the clock interface for PowerPC based FlexCAN.
>       Will it be OK ?
> 
>> Further you still add bugs to the driver. I've send you patches to fix
>> them.
> [Bhaskar] Please mention what sort of BUG you are seeing ..

I have, it's in that mail:

> @@ -957,24 +952,24 @@ static int __devinit register_flexcandev(struct 
> net_device *dev)
> >  
> >     /* select "bus clock", chip must be disabled */
> >     flexcan_chip_disable(priv);
> > -   reg = readl(&regs->ctrl);
> > +   reg = flexcan_read(&regs->ctrl);
> >     reg |= FLEXCAN_CTRL_CLK_SRC;
> > -   writel(reg, &regs->ctrl);
> > +   flexcan_write(reg, &regs->ctrl);
> >  
> >     flexcan_chip_enable(priv);
> >  
> >     /* set freeze, halt and activate FIFO, restrict register access */
> > -   reg = readl(&regs->mcr);
> > +   reg = flexcan_read(&regs->mcr);
> >     reg |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
> >             FLEXCAN_MCR_FEN | FLEXCAN_MCR_SUPV;
> > -   writel(reg, &regs->mcr);
> > +   flexcan_write(reg, &regs->mcr);
> >  
> >     /*
> >      * Currently we only support newer versions of this core
> >      * featuring a RX FIFO. Older cores found on some Coldfire
> >      * derivates are not yet supported.
> >      */
> > -   reg = readl(&regs->mcr);
> > +   reg = flexcan_read(&regs->mcr);
> >     if (!(reg & FLEXCAN_MCR_FEN)) {
> >             dev_err(dev->dev.parent,
> >                     "Could not enable RX FIFO, unsupported core\n");
> > @@ -984,6 +979,7 @@ static int __devinit register_flexcandev(struct 
> > net_device *dev)
> >  
> >     err = register_candev(dev);
> >  
> > +   return err;
If you return here, the clock stays enabled....not good

> >   out:
> >     /* disable core and turn off clocks */
> >     flexcan_chip_disable(priv);
> > @@ -992,7 +988,7 @@ static int __devinit register_flexcandev(struct 
> > net_device *dev)
> >     return err;
> >  }

For reference bugreport + fix:
https://lists.berlios.de/pipermail/socketcan-core/2011-July/005838.html

cheers, Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

signature.asc
Description: OpenPGP digital signature

_______________________________________________
Socketcan-core mailing list
Socketcan-core@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/socketcan-core