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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
