On Sunday 21 June 2009 15:54:13 Remy Bohmer wrote:
Hello Mike,
I got a remark about this patch:
2009/6/16 Mike Frysinger vapier@gentoo.org:
From: Bryan Wu bryan.wu@analog.com
Signed-off-by: Bryan Wu bryan.wu@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org
drivers/usb/musb/Makefile | 1 + drivers/usb/musb/blackfin_usb.c | 152 +++++++++++++++ drivers/usb/musb/blackfin_usb.h | 27 +++ drivers/usb/musb/musb_core.h | 73 +++++++ include/asm-blackfin/mach-common/bits/usb.h | 280 +++++++++++++++++++++++++++ include/usb.h | 3 +- 6 files changed, 535 insertions(+), 1 deletions(-)
--- a/drivers/usb/musb/musb_core.h +++ b/drivers/usb/musb/musb_core.h @@ -42,6 +42,68 @@
/* Mentor USB core register overlay structure */ struct musb_regs { +#if defined(CONFIG_USB_BLACKFIN)
/* Every register is 32bit aligned, but only 16bits in size */+# define ureg(name) u16 name; u16 __pad_##name;
/* common registers */ureg(faddr)ureg(power)ureg(intrtx)ureg(intrrx)ureg(intrtxe)ureg(intrrxe)ureg(intrusb)ureg(intrusbe)ureg(frame)ureg(index)ureg(testmode)ureg(globintr)ureg(global_ctl)u32 reserved0[3];/* indexed registers */ureg(txmaxp)ureg(txcsr)ureg(rxmaxp)ureg(rxcsr)ureg(rxcount)ureg(txtype)ureg(txinterval)ureg(rxtype)ureg(rxinterval)u32 reserved1;ureg(txcount)u32 reserved2[5];/* fifo */u16 fifox[32];/* OTG, dynamic FIFO, version & vendor registers */u32 reserved3[16];ureg(devctl)ureg(vbus_irq)ureg(vbus_mask)u32 reserved4[15];ureg(linkinfo)ureg(vplen)ureg(hseof1)ureg(fseof1)ureg(lseof1)u32 reserved5[41];/* target address registers */struct musb_tar_regs {ureg(txmaxp)ureg(txcsr)ureg(rxmaxp)ureg(rxcsr)ureg(rxcount)ureg(txtype)ureg(txinternal)ureg(rxtype)ureg(rxinternal)u32 reserved6;ureg(txcount)u32 reserved7[5];} tar[8];+# undef ureg +#else /* common registers */ u8 faddr; u8 power; @@ -97,6 +159,7 @@ struct musb_regs { u8 rxhubaddr; u8 rxhubport; } tar[16]; +#endif
Can both branches of this ifdef be merged somehow, such that the ifdef is much smaller?
Both branches seem to contain the same elements, only type differs ( which could maybe be handled with ureg() constructions also)
the layout isnt exactly the same, the sizes arent uniform enough for ureg(), and the registers available dont match either. we'd still have ifdefs, but interleaved, and i think that'd be much more likely to lead to breakage for architectures invovled.
} __attribute__((aligned(32)));
Shouldn't this be '__attribute__((packed))' too?
i didnt add that, but adding packed would probably make sense -mike