Re: [RFC v2 3/8] ci13xxx_udc: rename register layouts

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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.

So, before I do a rebase which will involve resoving 750 conflicts, I
want to run this by you: following your idea and using lookup tables, I
could make the end result into

        hw_read(void *register, u32 mask);

as in

        /* read ep0 ctrl */
        ctrl = hw_read(udc->reg_table[OP_ENDPTCTRL], ~0);

where udc->reg_table contains absolute register addresses, calculated at
probe time based on the chip version/revision/capabilities. In the
current version the above looks like

         ctrl = hw_read(udc, OP_ENDPTCTRL, ~0);

which is shorter. The only effective difference would be calculating
offsets at probe time, which is probably sensible.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


B and H Foto and Electronics Corp.

[Linux Media]     [Video for Linux]     [Linux Input]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]     [More Archives]

Add to Google Powered by Linux