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.
--
balbi
Attachment:
signature.asc
Description: Digital signature
