Re: [PATCH 2/9] 8xx: Infrastructure code cleanup.

[David Gibson <david@gibson.dropbear.id.au>]

On Fri, Sep 14, 2007 at 12:21:14PM +0400, Vitaly Bordug wrote:
> Hello David,
> 
> On Fri, 14 Sep 2007 14:09:34 +1000
> David Gibson wrote:
> 
> > On Thu, Sep 13, 2007 at 12:16:40PM +0400, Vitaly Bordug wrote:
> > [snip]
> > > > This looks bogus.  You're replacing the old crap immr_map() functions,
> > > > which ioremap()ed the registers every time, with a much simpler
> > > > version which uses an established-once mapping of the register
> > > > region.  AFAICT, anywah.
> > > > 
> > > > So far, so good - but your immr_unmap() still does an iounmap() which
> > > > is surely wrong - it should now be a no-op, leaving the mpc8xx_immr
> > > > mapping intact.  You probably get away with it by accident, because I
> > > > imagine attempting to unmap an unaligned chunk of the region will just
> > > > fail.
> > > >
> > > 
> > > yes, it should do nop instead of iounmap. 
> > > > In fact, with this patch in place, I'd like to see another patch which
> > > > removes all calls to immr_map() and immr_unmap(), simply accessing the
> > > > common mapping directly.
> > > > 
> > > Sorry, but originally, that stuff was created to get rid of BSP
> > > ifdefs in drivers. For PQ family, it is a common practice to have
> > > single driver handling all 3 CPU families, which use the same logic,
> > > but immr structure differs a little bit.
> > > 
> > > At this point it's clear case-by-case ioremapping does not have firm
> > > benefit, but getting back to the way it was is useless either.  In
> > > ideal world, we'd have all those stuff put into dts and have
> > > specific drivers be a shim layer between core hw and IO drivers.
> > 
> > Err.. I don't understand what you're getting at.  As the code stands
> > after Scott's cleanup, the map() and unmap() calls can certainly be
> > trivially removed, regardless of the history for them.
> > 
> I don't argue if they can be removed, but if we aught to do
> that. Direct immr dereference adds plenty of mess into driver

Um... better one line of mess than the 3 lines it is now
(map/access/unmap).

> code. I would like to keep the situation when immr accesses factored
> out as a starting point, rather then turn them back to &immap-> or
> cpm2_immr-> refs.

I can see no advantage to the current "factoring".

> > And, yes, the drivers should certainly uses addresses from the device
> > tree, rather than that revolting structure covering all the inbuilt
> > device retgisters.
> hehe, then you prolly know, that this structure does not fin well
> into device/driver model, either platform_ or of_device. And I am
> going to sort it out at some point...

Well, that's good to know.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson