Re: [PATCH 0/9] MPC8xxx support

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Renaud,

On Tue, Jan 24, 2012 at 12:33:54PM +0000, Renaud Barbier wrote:
> These patches are a work in progress to introduce the mpc8xxx
> architecture to barebox.
> They include preparation work to add the new architecture and
> a minimal support for the P2020RDB. That is, serial only, no
> network driver or storage drivers.
> I will follow up with the Ethernet, NAND and I2C driver.

Sorry, but the patches in their current form a nowhere close for being
mergable.

- Generally the #ifdef density is much too high. We have good mechanisms
  to avoid that (initcalls, put code together that belongs together, add
  function parameters)
- Lots of dead code:
  - unused CONFIG_ variables:
    CONFIG_USE_IRQ, CONFIG_SYS_SRIO, CONFIG_CMD_SATA, CONFIG_FSL_SATA,
    CONFIG_BACKSIDE_L2_CACHE, CONFIG_QE, CONFIG_VSC7385_ENET,...
  - unused functions:
    arch_preboot_os
  - referenced functions not contained in these patches:
    qe_init, srio_init
  - Files which are not compiled (and also won't compile if enabled)
    fdt_8xxx.c, fdt.c
  - unused header files:
    fsl_portals.h, lmb.h
  - driver specific header files under arch/ppc (should be next to the
    driver):
    tsec.h
- please no typedefs for structs
- All functions in your board file are global, they should *all* be
  static. If you need to call them from somewhere else, there is
  something wrong.
- default MAC address in code. Don't do that. barebox generates random
  MAC addresses if no valid one is found in the EEPROM.

The following snippet is somewhat symptomatic for this patch series:

static struct i2c_board_info i2c_devices[] = {
        {
                I2C_BOARD_INFO("eepromid", CONFIG_SYS_I2C_EEPROM_ADDR),
        },
        {
                I2C_BOARD_INFO("rtc", CONFIG_SYS_I2C_RTC_ADDR),
        },
};

This snippet is board specific, so you don't need a define for the
addresses. "rtc" is not a driver we will ever add to barebox, instead
it must be the specific clock chip in the platform data. btw
CONFIG_SYS_I2C_EEPROM_ADDR is defined twice in your board header file.
Also, please don't #undef things. Just don't define them in the first
place.
Generally please don't use the CONFIG_ namespace for anything else but
kconfig variables.

I suggest that you start by removing all dead code in the patches, then
the patches may be small enough for a closer review.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox