[U-Boot] [PATCH] add new board pm9g45
Wolfgang Denk
wd at denx.de
Tue Mar 16 20:00:12 CET 2010
Dear Asen Dimov,
In message <1268744233-2497-1-git-send-email-dimov at ronetix.at> you wrote:
> Hello everyone,
>
> here is the new board PM9G45 from Ronetix GmbH,
> based on at91sam9g45 MCU. It has 128MB DDR2 SDRAM, 256MB NAND,
> could be with or without DataFlash.
> The board is made as SODIMM200 module.
> For more info www.ronatix.at or info at ronetix.at.
>
> Regards,
> Asen
You sent a similar patch les than one hour before this one.
None of your patches includes any indication what they are - if you
are resubmitting a patch, you are suppoosed to mark it as "[PATCH
v2]" or "[PATCH v3]" or similar in the Subject: line.
> Signed-off-by: Asen Dimov <dimov at ronetix.at>
> ---
> MAKEALL | 1 +
...
Also, you are supposed to include a descripotion of what has been
changed compared to the previous version(s) of the patch here, below
the "---" line.
At fist glance, the two patches look identical to me. Do you expect
me to scan through some 1500+ lines of patches to check which lines
or characters might have changed?
Also, a commit message including "Hello everyone," and "Regards, Asen"
is not exactly useful. Please omit this in patches.
> ---
> MAKEALL | 1 +
> Makefile | 4 +
> board/ronetix/pm9g45/Makefile | 54 +++
> .../at91sam9m10g45ek => ronetix/pm9g45}/config.mk | 0
> board/ronetix/pm9g45/pm9g45.c | 365 ++++++++++++++++++++
> include/configs/pm9g45.h | 246 +++++++++++++
> 6 files changed, 670 insertions(+), 0 deletions(-)
> create mode 100644 board/ronetix/pm9g45/Makefile
> copy board/{atmel/at91sam9m10g45ek => ronetix/pm9g45}/config.mk (100%)
> create mode 100644 board/ronetix/pm9g45/pm9g45.c
> create mode 100644 include/configs/pm9g45.h
MAINTAINERS entry is missing.
> diff --git a/board/atmel/at91sam9m10g45ek/config.mk b/board/ronetix/pm9g45/config.mk
> similarity index 100%
> copy from board/atmel/at91sam9m10g45ek/config.mk
> copy to board/ronetix/pm9g45/config.mk
> diff --git a/board/ronetix/pm9g45/pm9g45.c b/board/ronetix/pm9g45/pm9g45.c
> new file mode 100644
> index 0000000..d11f40f
> --- /dev/null
> +++ b/board/ronetix/pm9g45/pm9g45.c
> @@ -0,0 +1,365 @@
> +/*
> + * (C) Copyright 2005-2010
> + * Ilko Iliev <iliev at ronetix.at>
> + * Asen Dimov <dimov at ronetix.at>
> + * Ronetix GmbH <www.ronetix.at>
2005- ? Is this really correct?
> + writel(pin_to_mask(AT91_PIN_PA15),
> + pin_to_controller(AT91_PIN_PA0) + PIO_PUDR);
> + writel(pin_to_mask(AT91_PIN_PA12) |
> + pin_to_mask(AT91_PIN_PA13),
> + pin_to_controller(AT91_PIN_PA0) + PIO_PUDR);
> +
> + /* Re-enable pull-up */
> + writel(pin_to_mask(AT91_PIN_PA15),
> + pin_to_controller(AT91_PIN_PA0) + PIO_PUER);
> + writel(pin_to_mask(AT91_PIN_PA12) |
> + pin_to_mask(AT91_PIN_PA13),
> + pin_to_controller(AT91_PIN_PA0) + PIO_PUER);
The use of base address plus offset is deprecated. Please use C
strucxts to desribe the register layout.
> +#ifdef CONFIG_LCD
> +/*
> + * LCD name TX09D50VM1CCA
> + */
> +vidinfo_t panel_info = {
> + vl_col: 240,
> + vl_row: 320,
> + vl_clk: 4965000,
> + vl_sync: ATMEL_LCDC_INVLINE_NORMAL |
> + ATMEL_LCDC_INVFRAME_NORMAL,
> + vl_bpix: 3,
> + vl_tft: 1,
> + vl_hsync_len: 5,
> + vl_left_margin: 1,
> + vl_right_margin:33,
> + vl_vsync_len: 1,
> + vl_upper_margin:1,
> + vl_lower_margin:0,
> + mmio: AT91SAM9G45_LCDC_BASE,
> +};
This information should not be board-specific. The panel information
is generic and should moved to a separate header file that is not part
of the board code.
...
> +}
> +
> +#ifdef CONFIG_LCD_INFO
> +#include <nand.h>
> +#include <version.h>
Please move #includes to the top of the file.
> +#ifdef CONFIG_HAS_DATAFLASH
> + dataflash_size = 0;
> + for (i = 0; i < CONFIG_SYS_MAX_DATAFLASH_BANKS; i++)
> + dataflash_size += (unsigned int) dataflash_info[i].Device.pages_number *
> + dataflash_info[i].Device.pages_size;
> +#endif
Line too long. Please check and fix globally. Also, multiline
statements require curly braces.
> +void spi_cs_activate(struct spi_slave *slave)
> +{
> + switch(slave->cs) {
> + case 1:
> + at91_set_gpio_output(AT91_PIN_PB18, 0);
> + break;
> + case 0:
> + default:
> + at91_set_gpio_output(AT91_PIN_PB3, 0);
> + break;
> + }
> +}
> +
> +void spi_cs_deactivate(struct spi_slave *slave)
> +{
> + switch(slave->cs) {
> + case 1:
> + at91_set_gpio_output(AT91_PIN_PB18, 1);
> + break;
> + case 0:
> + default:
> + at91_set_gpio_output(AT91_PIN_PB3, 1);
> + break;
> + }
> +}
Incorrect indentation. Please fix globally.
...
> +#undef CONFIG_USE_IRQ /* we don't need IRQ/FIQ stuff */
^^^^^^^^^^^^^^^^^^^^^^^^
...
> +#define CONFIG_AT91_GPIO 1
> +#define CONFIG_ATMEL_USART 1
> +#undef CONFIG_USART0
^^^^^^^^^^^^^^^^^^^^^^^
> +#undef CONFIG_USART1
^^^^^^^^^^^^^^^^^^^^^^^
> +#undef CONFIG_USART2
^^^^^^^^^^^^^^^^^^^^^^^
Please do not undef what is not #defined anyway.
> +#include <config_cmd_default.h>
> +#undef CONFIG_CMD_BDI
> +#undef CONFIG_CMD_FPGA
> +#undef CONFIG_CMD_IMI
> +#undef CONFIG_CMD_IMLS
> +#undef CONFIG_CMD_AUTOSCRIPT
AUTOSCRIP has been deprecated long ago.
Is there any good reason for disabling pretty useful commands like
bdinfo, iminfo or imls ? It does not appear to me as if the memory
footprint was critical to you.
> +#define CONFIG_ENV_SIZE 0x20000 /* 1 sector = 128 kB */
> +#define CONFIG_BOOTCOMMAND "nand read 0x72000000 0x200000 0x200000; bootm"
Again: lines too long. Please fix everywhere.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Brontosaurus Principle: Organizations can grow faster than their
brains can manage them in relation to their environment and to their
own physiology: when this occurs, they are an endangered species.
- Thomas K. Connellan
More information about the U-Boot
mailing list