Nick Thompson wrote:
Wolfgang Denk wrote:
Dear Nick Thompson,
In message 4AE5DFFD.2090801@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.thompson@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