[U-Boot] [PATCH] ppc4xx: Support dual link OSD

Eibach, Dirk Eibach at gdsys.de
Tue Jan 18 09:10:33 CET 2011


 

> Dear Dirk Eibach,

Dear Wolfgang,

> ...
> >  #define BASE_WIDTH 32
> > @@ -38,12 +44,18 @@
> >  enum {
> >  	REG_CONTROL = 0x0010,
> >  	REG_MPC3W_CONTROL = 0x001a,
> > +	REG_EXT_INTERRUPT = 0x001c,
> > +	REG_EXT_INTERRUPT_ENABLE = 0x001e,
> > +	REG_IIC_WRITE_MAILBOX = 0x0030,
> > +	REG_IIC_WRITE_MAILBOX_EXT = 0x0032,
> > +	REG_IIC_READ_MAILBOX = 0x0034,
> > +	REG_IIC_READ_MAILBOX_EXT = 0x0036,
> >  	REG_VIDEOCONTROL = 0x0042,
> > -	REG_OSDVERSION = 0x0100,
> > -	REG_OSDFEATURES = 0x0102,
> > -	REG_OSDCONTROL = 0x0104,
> > -	REG_XY_SIZE = 0x0106,
> > -	REG_VIDEOMEM = 0x0800,
> > +	REG_OSDVERSION = CONFIG_SYS_FPGA_OSD_BASE + 0x0000,
> > +	REG_OSDFEATURES = CONFIG_SYS_FPGA_OSD_BASE + 0x0002,
> > +	REG_OSDCONTROL = CONFIG_SYS_FPGA_OSD_BASE + 0x0004,
> > +	REG_XY_SIZE = CONFIG_SYS_FPGA_OSD_BASE + 0x0006,
> > +	REG_VIDEOMEM = CONFIG_SYS_FPGA_OSD_MEM,
> 
> This has a certain smell to me - it smells like device 
> accesses based on a base + offset notation, which is not waht 
> we want to see.  Why don't you use proper C structs instead 
> to describe your devices?

yup, got me. I know you prefer C struct based register access while we
don't. I'm sure anyone has excellent reasons for his opinion, and I
don't want to start a discussion here which would probably lead nowhere.
So, if you insist, I will convert our code to C struct based access.
Maybe you could point me to a nice clean implementation in u-boot, so I
can use the semantics you prefer (e.g. how holes in address space are
labeled).

> Best regards,
> 
> Wolfgang Denk

Cheers
Dirk




More information about the U-Boot mailing list