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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]