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

[Vitaly Bordug <vitb@kernel.crashing.org>]

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 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.
> 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...


-- 
Sincerely, Vitaly