[PATCH 3/3] mx51: Move OTG initialisation for all boards to a single file

[Uwe Kleine-König]

Hi Amit,

On Thu, Oct 07, 2010 at 03:04:59PM +0300, Amit Kucheria wrote:
> On 10 Oct 07, Uwe Kleine-König wrote:
> > On Thu, Oct 07, 2010 at 03:58:48AM +0300, Amit Kucheria wrote:
> > > +	/* Set the PHY clock to 24 MHz */
> > > +	v = __raw_readl(usbother_base + MXC_USB_PHY_CTR_FUNC2_OFFSET);
> > > +	v &= ~MX5_USB_UTMI_PHYCTRL1_PLLDIV_MASK;
> > I wonder why a field in a register called MXC_USB_PHY_CTR_FUNC2 is named
> > MX5_USB_UTMI_PHYCTRL1_PLLDIV.  Usually the register name is a prefix for
> > the field name?!
> 
> I have a patch renaming the various bit-fields and registers to conform to
> the reference manual. I'll post it soon.
great

> > > +	v |= MX51_USB_PLL_DIV_24_MHZ;
> > hmm, babbage used
> 
> > 	> -     v &= ~MX5_USB_UTMI_PHYCTRL1_PLLDIV_MASK;
> > 	> -     v |= MX51_USB_PLL_DIV_19_2_MHZ;
> > 
> > ?
> 
> Good catch :)
> 
> But if you look at the reference manual, table 60-6, 19.2 MHz would use a
> value of 0x0, but the actual code uses a value of 0x1 that corresponds to
> 24MHz. 19.2MHz (0x0) doesn't even work for me.
Then maybe note it in the commit log?!

> > Maybe you want to reorder your patches such that your patch 2 comes
> > after this one, as currently this patch removes ~80% of the code patch 2
> > introduces.
> 
> I did it to make bisection easier. I didn't want to add a new board feature
> and do consolidation in a single patch.
Yep, that is ok.  Currently you have:

	1/3: add board support for iforgotthename
	2/3: add usb support for iforgotthename
	3/3: consolidate usb support

I suggested to do 1, 3, 2 instead.  Thinking again 3, 1+2 might even be
better.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |