Nick Thompson wrote: > Wolfgang Denk wrote: >> Dear Nick Thompson, >> >> In message <4ae5dffd.2090...@gefanuc.com> you wrote: >>> Provides initial support for TI OMAP-L1x/DA8xx SoC devices. >>> See http://www.ti.com >>> >>> The DA8xx devices are similar to DaVinci devices but have a differing >>> memory map and updated peripheral versions. >>> >>> Signed-off-by: Nick Thompson <nick.thomp...@gefanuc.com> >>> --- >> ... >>> +/* required by davinci drivers */ >> Is this really an essential comment? Drop it! >> >>> #define REG(addr) (*(volatile unsigned int *)(addr)) >>> -#define REG_P(addr) ((volatile unsigned int *)(addr)) >>> >>> +/* required by davinci drivers */ >> Ditto. >> >>> typedef volatile unsigned int dv_reg; >>> typedef volatile unsigned int * dv_reg_p; >> ... >> >>> +/* for LPSCs in PSC1, 32 + actual id is being used for differentiation */ >>> +#define DAVINCI_LPSC_BASE 32 >>> +#define DAVINCI_LPSC_USB11 (DAVINCI_LPSC_BASE + 1) >>> +#define DAVINCI_LPSC_USB20 (DAVINCI_LPSC_BASE + 2) >>> +#define DAVINCI_LPSC_GPIO (DAVINCI_LPSC_BASE + 3) >>> +#define DAVINCI_LPSC_UHPI (DAVINCI_LPSC_BASE + 4) >>> +#define DAVINCI_LPSC_EMAC (DAVINCI_LPSC_BASE + 5) >>> +#define DAVINCI_LPSC_DDR_EMIF (DAVINCI_LPSC_BASE + 6) >>> +#define DAVINCI_LPSC_McASP0 (DAVINCI_LPSC_BASE + 7) >>> +#define DAVINCI_LPSC_McASP1 (DAVINCI_LPSC_BASE + 8) >>> +#define DAVINCI_LPSC_McASP2 (DAVINCI_LPSC_BASE + 9) >>> +#define DAVINCI_LPSC_SPI1 (DAVINCI_LPSC_BASE + 10) >>> +#define DAVINCI_LPSC_I2C1 (DAVINCI_LPSC_BASE + 11) >>> +#define DAVINCI_LPSC_UART1 (DAVINCI_LPSC_BASE + 12) >>> +#define DAVINCI_LPSC_UART2 (DAVINCI_LPSC_BASE + 13) >>> +#define DAVINCI_LPSC_LCDC (DAVINCI_LPSC_BASE + 16) >>> +#define DAVINCI_LPSC_ePWM (DAVINCI_LPSC_BASE + 17) >>> +#define DAVINCI_LPSC_eCAP (DAVINCI_LPSC_BASE + 20) >>> +#define DAVINCI_LPSC_eQEP (DAVINCI_LPSC_BASE + 21) >>> +#define DAVINCI_LPSC_SCR_P0 (DAVINCI_LPSC_BASE + 22) >>> +#define DAVINCI_LPSC_SCR_P1 (DAVINCI_LPSC_BASE + 23) >>> +#define DAVINCI_LPSC_CR_P3 (DAVINCI_LPSC_BASE + 26) >>> +#define DAVINCI_LPSC_L3_CBA_RAM (DAVINCI_LPSC_BASE + 31) >> I think you actually want to use a C struct here, like in many other >> places. >> >> Please do not access device registers using plain pointers like (base >> address plus offet), but always use I/O accessors with C structs, so >> we can have strict type checking by the compiler. > > As I mentioned in the patch introduction "[PATCH V3 0/4]..." there are > several places where out of fashion code is present in these patches. > > DA8xx SoC's are considered a member of the DaVinci family of SoC's and > so these headers need to maintain compatibility with the relevant > drivers already in the U-Boot git tree. > > Those drivers do not use C structures and I/O accessors and so require > the use of #defines as presented here. > > Where possible I have not used legacy code forms in new code, but in > the case of new code that also has to use these same registers, I have > again opted for compatibility. > > Of course I could duplicate the definitions as defines /and/ C structs, > or even rewrite all the DaVinci family drivers, but I'm hoping neither > of those options was the intention of your comments here. > > Can you please confirm whether my assumptions as correct and acceptable > in this case? I.e. (as Tom Rix put it) Can I get a pass on this one? > > I think you have a point in the PINMUX cases below however, as I believe > these #defines are only used in SoC specific code... > > Best Regards, > Nick >
There are a large number of #define's in davinci that could be converted to structures. My opinion is that da8xx sharing code with davinci is a big enough change. The reworking of basic davici register calling through structs falls outside of what I expect. The cleanup on the calling I am looking for is switching for direct access to registers to calling them through readl/writel. Tom _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
