Dear Albert Aribaud,
In message 1258239796-21528-2-git-send-email-albert.aribaud@free.fr you wrote:
This patch adds support for the Marvell Orion5x SoC. It has no use alone, and must be followed by a patch to add Orion5x support for serial, then support for the ED Mini V2, an Orion5x-based board from LaCie.
...
diff --git a/cpu/arm926ejs/orion5x/timer.c b/cpu/arm926ejs/orion5x/timer.c new file mode 100644
...
+#define READ_TIMER \
- (readl(CNTMR_VAL_REG(UBOOT_CNTR)) / (CONFIG_SYS_TCLK / 1000))
...
- /* reset time */
- lastdec = READ_TIMER;
Macros resembling functions should look like functions, i. e. have parens. Also please note that generally, inline functions are preferable to macros resembling functions.
...
+#define UBOOT_CNTR_VAL readl(CNTMR_VAL_REG(UBOOT_CNTR))
Ditto.
+void udelay(unsigned long usec) +{
- uint current;
- ulong delayticks;
- current = readl(CNTMR_VAL_REG(UBOOT_CNTR));
Why do you add the #define above when you then don't use it?
- delayticks = (usec * (CONFIG_SYS_TCLK / 1000000));
- if (current < delayticks) {
delayticks -= current;while (readl(CNTMR_VAL_REG(UBOOT_CNTR)) < current)
Ditto.
diff --git a/cpu/arm926ejs/orion5x/mpp.c b/cpu/arm926ejs/orion5x/mpp.c new file mode 100644 index 0000000..f341747 --- /dev/null +++ b/cpu/arm926ejs/orion5x/mpp.c ... + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied.
NAK. License must be "v2 or any later version". Ditto for some other files.
diff --git a/include/asm-arm/arch-orion5x/88f5182.h b/include/asm-arm/arch-orion5x/88f5182.h new file mode 100644 index 0000000..b16b23f --- /dev/null +++ b/include/asm-arm/arch-orion5x/88f5182.h
88f5182 is a terrible file name. Can you not come up with something more descriptive, please?
diff --git a/include/asm-arm/arch-orion5x/gpio.h b/include/asm-arm/arch-orion5x/gpio.h new file mode 100644 index 0000000..58592ad --- /dev/null +++ b/include/asm-arm/arch-orion5x/gpio.h
...
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
NAK. See above.
+#define GPIO_MAX 26 +#define GPIO_OUT(pin) (ORION5X_GPIO0_BASE + 0x00) +#define GPIO_IO_CONF(pin) (ORION5X_GPIO0_BASE + 0x04) +#define GPIO_BLINK_EN(pin) (ORION5X_GPIO0_BASE + 0x08) +#define GPIO_IN_POL(pin) (ORION5X_GPIO0_BASE + 0x0c) +#define GPIO_DATA_IN(pin) (ORION5X_GPIO0_BASE + 0x10) +#define GPIO_EDGE_CAUSE(pin) (ORION5X_GPIO0_BASE + 0x14) +#define GPIO_EDGE_MASK(pin) (ORION5X_GPIO0_BASE + 0x18) +#define GPIO_LEVEL_MASK(pin) (ORION5X_GPIO0_BASE + 0x1c)
NAK. Please make this a C struct.
+/*
- Orion5x-specific GPIO API
- */
Why do we need an Orion5x-specific GPIO API? Can't we use something that is more general?
diff --git a/include/asm-arm/arch-orion5x/mpp.h b/include/asm-arm/arch-orion5x/mpp.h new file mode 100644 index 0000000..31fade7 --- /dev/null +++ b/include/asm-arm/arch-orion5x/mpp.h
...
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
NAK. See above.
+#define MPP(_num, _sel, _in, _out, _F5182, _F5281) ( \
- /* MPP number */ ((_num) & 0xff) | \
- /* MPP select value */ (((_sel) & 0xf) << 8) | \
- /* may be input signal */ ((!!(_in)) << 12) | \
- /* may be output signal */ ((!!(_out)) << 13) | \
- /* available on F5182 */ ((!!(_F5182)) << 14) | \
- /* available on F5281 */ ((!!(_F5182)) << 15))
Comments follow the code, not vice versa.
Best regards,
Wolfgang Denk