Hi, On Thu, Apr 12, 2012 at 04:54:01PM +0300, Alexander Shishkin wrote: > On Wed, 11 Apr 2012 15:16:03 +0300, Felipe Balbi <balbi@xxxxxx> wrote: > > On Wed, Apr 11, 2012 at 03:14:12PM +0300, Alexander Shishkin wrote: > > > On Wed, 11 Apr 2012 14:37:35 +0300, Felipe Balbi <balbi@xxxxxx> wrote: > > > > On Wed, Apr 11, 2012 at 01:52:31PM +0300, Alexander Shishkin wrote: > > > > > On Wed, 11 Apr 2012 12:54:41 +0300, Felipe Balbi <balbi@xxxxxx> wrote: > > > > > > why do you need separate functions to read capability or operational > > > > > > registers ? They look the same. You could just define them a little > > > > > > differently: > > > > > > > > > > > > static u32 hw_read(void *base, u32 offset, u32 mask) > > > > > > { > > > > > > return ioread32(base + offset) & mask; > > > > > > } > > > > > > > > > > > > then, when calling it you can use: > > > > > > > > > > > > hw_read(hw_bank.op, ADDRESS, mask); > > > > > > hw_read(hw_bank.cap, ADDRESS, mask); > > > > > > > > > > That's what I've done in the "redo register access" patch. The reason I > > > > > didn't do this here is that I wanted to change one thing at a time. It > > > > > might make sense to squash these patches together, too. > > > > > > > > that patch is different. You use some extra trickery to try and guess > > > > which base to use (operational or capability) whereas you could just > > > > pass that as an argument. > > > > > > Yes, I really wanted to hide all that under the hood and let the hw_* > > > functions deal with the register banks, it looks better to me, but I > > > will change it to what you're suggesting, if you think it's better like > > > that. > > > > to me it looks better than trying to guess which base to use. I prefer > > explicitly passing the base address as argument. > > Actually, it's not so much *guessing*, we actually know which registers > are where. Another thing is, I still want to use lookup tables as in > "redo register access" to eliminate the ugly register definitions like > > #define REGISTER1 (some_global_var.lpm ? 0x10 : 0x20) > > and I couldn't come up with any better way for doing that. struct my_struct { unsigned lpm:1; }; ... static u32 hw_read(struct my_struct *ptr, void __iomem *base, u32 register, u32 mask) { u32 reg = register; if (ptr->lpm) reg += 0x10; return readl(base + reg) & mask; } or something similar. Another way could be: struct my_struct { unsigned int extra_offset; }; static u32 hw_read(struct my_struct *ptr, void __iomem *base, u32 register, u32 mask) { return readl(base + reg + ptr->extra_offset) & mask; } ... static int __devinit probe(struct platform_device *pdev) { ... if (lpm) ptr->extra_offset = 0x10; else ptr->extra_offset = 0x00; ... return 0; } which avoids extra branches on every read/write :-p -- balbi
Attachment:
signature.asc
Description: Digital signature