Dear Wolfgang,
Wolfgang Denk wrote:
Dear Dirk,
In message 1227445293-23887-1-git-send-email-dirk.behme@googlemail.com you wrote:
Convert readl/writel to base + offset style. Replace hardcoded values with macros.
I think repeating the sequence "base + offset(foo)" again and again is not exactly nice or easy to read.
But it's what the patch does ;) Anyway, if I remember correctly this is why you asked me to re-send OMAP3 patch series to mailing list again once all the review comments are fixed and not directly git pull from OMAP3 branch. To get rid of all these "fix here, clean up there" comments.
- writel((a_add_high | a_add_low), SDRC_CS_CFG);
- writel((a_add_high | a_add_low), sdrc_base + OFFS(SDRC_CS_CFG));
I find this new code is even less readable.
At this point I was tempted to compain that you should not re-invent the offsetof() macro when I realized that the actual definition of OFFS is
#define OFFS(x) ((x) >> 2)
Argh... that's awfully error prone. So you rely on havnig to deal with 32 bit register accesses only, without any way of compiler supported type checking?
That's awful.
This style is what Jean-Christophe and Scott asked me to use [1]. I was asked to convert all OMAP3 read/write access to this style to get the patches accepted.
Disussing this at IRC, we agreed on
-- cut -- #define GPMC_BASE 0x6E000000
OFFS(x) ((x) >> 2)
#define GPMC_ECC_CONFIG 0x1F4
static uint32_t *gpmc_base = (uint32_t *)GPMC_BASE; ... writel(0x000, gpmc_base + OFFS(GPMC_ECC_CONFIG)); -- cut --
style for 32-bit accesses. OFFS is used to be able to use offset macros in assembly, too, and to be able to directly use offset addresses from TRM not needing them to divide by 4 "manually" (error prone).
[1] http://lists.denx.de/pipermail/u-boot/2008-October/042483.html
I am aware that many areas of ARM code use such a horrible program- ming syle of defining only register offsets instead of proper data structures like for example PowerPC has been doing for ages.
Do you see a chance to convert the OMAP3 code to use data structures instead of offset definitions to describe the processor registers and peripherals?
Looking at the time I already spent into converting to the style used now, no, unfortunately, not. This was the last read/write converting patch, btw.
Best regards
Dirk
Btw.: Did you notice how e.g. Nomadik patch [2] does it?
-- cut -- #define NOMADIK_GPIO1_BASE 0x101E5000 ... writel(0xC37800F0, NOMADIK_GPIO1_BASE + 0x20); -- cut --
Having this patch applied in this format I slightly wonder
a) why I have to convert all hardcoded values to macros
b) why I have to convert all writex/readx
Style used by Nomadik patch is the same OMAP3 initially used and that wasn't accepted...
[2] http://lists.denx.de/pipermail/u-boot/2008-November/043651.html