Dear Grzegorz Bernacki,
In message 12345332203460-git-send-email-gjb@semihalf.com you wrote:
Signed-off-by: Grzegorz Bernacki gjb@semihalf.com
I was about to apply this as John probably has very little time to care about this.
But I have a few comments / requests first:
...
+void static set_ethaddr(void) +{
...
Eventually this should be rebased against the "next" branch, and board_get_enetaddr() should be used here?
- for (n = 0; n < 6 ; n++) {
addr = addr_of_eth_addr + n;chip = EEPROM_ADDR + ((addr & 0x300)>>8);i2c_read(chip, (addr & 0xFF), 1, (uchar *)ð_addr[n], 1);- }
Can we not do this with a single i2c_read() call with length = 6 ?
- sprintf(str_eth_addr, "%02X:%02X:%02X:%02X:%02X:%02X",
eth_addr[0], eth_addr[1], eth_addr[2],eth_addr[3], eth_addr[4], eth_addr[5]);- setenv("ethaddr", str_eth_addr);
eth_setenv_enetaddr() should be used here.
See commit 3754a3b3 (in the "next" branch) for details.
+int last_stage_init (void) +{
- if (getenv("ethaddr") == NULL)
set_ethaddr();- return 0;
+}
I think this should be removed in the new context.
...
+void init_ide_reset(void) +{
- debug ("init_ide_reset\n");
- /* set gpio output value to 1 */
- out_be32((void *)MPC5XXX_WU_GPIO_DATA_O,
in_be32((void *)MPC5XXX_WU_GPIO_DATA_O) | (1 << 25));
Should that not be replaced by the (much easier to read)
setbits_be32((void *)MPC5XXX_WU_GPIO_DATA_O, (1 << 25));
?
- /* open drain output */
- out_be32((void *)MPC5XXX_WU_GPIO_ODE,
in_be32((void *)MPC5XXX_WU_GPIO_ODE) | (1 << 25));- /* direction output */
- out_be32((void *)MPC5XXX_WU_GPIO_DIR,
in_be32((void *)MPC5XXX_WU_GPIO_DIR) | (1 << 25));- /* enable gpio */
- out_be32((void *)MPC5XXX_WU_GPIO_ENABLE,
in_be32((void *)MPC5XXX_WU_GPIO_ENABLE) | (1 << 25));
Ditto here and elsewhere.
+void ide_set_reset(int idereset) +{
- debug ("ide_reset(%d)\n", idereset);
- /* set gpio output value to 0 */
- out_be32((void *)MPC5XXX_WU_GPIO_DATA_O,
in_be32((void *)MPC5XXX_WU_GPIO_DATA_O) & ~(1 << 25));
And this should probably be replaced by
clrbits_be32((void *)MPC5XXX_WU_GPIO_DATA_O, (1 << 25));
?
diff --git a/cpu/mpc5xxx/ide.c b/cpu/mpc5xxx/ide.c index df5b4ac..88da199 100644 --- a/cpu/mpc5xxx/ide.c +++ b/cpu/mpc5xxx/ide.c @@ -42,7 +42,7 @@ int ide_preinit (void) struct mpc5xxx_sdma *psdma = (struct mpc5xxx_sdma *) MPC5XXX_SDMA;
reg = *(vu_long *) MPC5XXX_GPS_PORT_CONFIG; -#if defined(CONFIG_TOTAL5200) +#if defined(CONFIG_TOTAL5200) || defined(CONFIG_DIGSY_MTC) /* ATA cs0/1 on i2c2 clk/io */ reg = (reg & ~0x03000000ul) | 0x02000000ul; #else
Instead of adding board-specific #ifdef's here, we should change it to a feature-specific one, and #define some CONFIG_ATA_CS_ON_I2C2 or similar in the two board config files.
What do you think?
Best regards,
Wolfgang Denk