On Mon, 5 May 2008 20:23:29 +0200
"Giuseppe Coviello" <[EMAIL PROTECTED]> wrote:
> This patch adds the support for the sam440ep board.
Urgh. Could you split this into a number of smaller patches, each with
a proper commit message that doesn't break the build on its own?
See some initial comments below.
> diff --git a/arch/powerpc/boot/cuboot-sam440ep.c
> b/arch/powerpc/boot/cuboot-sam440ep.c
> new file mode 100644
> index 0000000..200b35b
> --- /dev/null
> +++ b/arch/powerpc/boot/cuboot-sam440ep.c
> @@ -0,0 +1,35 @@
> +/*
> + * Old U-boot compatibility for Sam440ep based off bamboo.c code
> + * original copyrights below
> + *
> + * Author: Josh Boyer <[EMAIL PROTECTED]>
> + *
> + * Copyright 2007 IBM Corporation
> + *
> + * Based on cuboot-ebony.c
> + *
> + * Modified from cuboot-bamboo.c for sam440ep:
> + * Copyright 2008 Giuseppe Coviello <[EMAIL PROTECTED]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include "ops.h"
> +#include "stdio.h"
> +#include "44x.h"
> +#include "cuboot.h"
> +
> +#define TARGET_4xx
> +#define TARGET_44x
> +#include "ppcboot.h"
> +
> +static bd_t bd;
> +
> +void platform_init(unsigned long r3, unsigned long r4, unsigned long r5,
> + unsigned long r6, unsigned long r7)
> +{
> + CUBOOT_INIT();
> + sam440ep_init(&bd.bi_enetaddr, &bd.bi_enet1addr);
> +}
I don't see a reason to split out the bootwrapper support into two
separate files. You should be able to have a single file for it.
> diff --git a/arch/powerpc/platforms/44x/sam440ep.c
> b/arch/powerpc/platforms/44x/sam440ep.c
> new file mode 100644
> index 0000000..a95eb80
> --- /dev/null
> +++ b/arch/powerpc/platforms/44x/sam440ep.c
> @@ -0,0 +1,66 @@
> +#include <linux/init.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/machdep.h>
> +#include <asm/prom.h>
> +#include <asm/udbg.h>
> +#include <asm/time.h>
> +#include <asm/uic.h>
> +#include <asm/pci-bridge.h>
> +#include <asm/ppc4xx.h>
> +
> +static __initdata struct of_device_id sam440ep_of_bus[] = {
> + { .compatible = "ibm,plb4", },
> + { .compatible = "ibm,opb", },
> + { .compatible = "ibm,ebc", },
> + {},
> +};
> +
> +static int __init sam440ep_device_probe(void)
> +{
> + of_platform_bus_probe(NULL, sam440ep_of_bus, NULL);
> +
> + return 0;
> +}
> +machine_device_initcall(sam440ep, sam440ep_device_probe);
> +
> +static int __init sam440ep_probe(void)
> +{
> + unsigned long root = of_get_flat_dt_root();
> +
> + if (!of_flat_dt_is_compatible(root, "acube,sam440ep"))
> + return 0;
> +
> + ppc_pci_flags = PPC_PCI_REASSIGN_ALL_RSRC;
> +
> + return 1;
> +}
> +
> +define_machine(sam440ep) {
> + .name = "Sam440ep",
> + .probe = sam440ep_probe,
> + .progress = udbg_progress,
> + .init_IRQ = uic_init_tree,
> + .get_irq = uic_get_irq,
> + .restart = ppc4xx_reset_system,
> + .calibrate_decr = generic_calibrate_decr,
> +};
You don't do anything platform specific at all in this file. You
should be able to just reuse the bamboo.c file. Look at Yosemite for
how that is done.
> +static int __init sam440ep_i2c_of_init(void)
> +{
> + struct device_node *np;
> + unsigned int i = 0;
> + struct platform_device *i2c_dev;
> + int ret;
> +
> + for_each_compatible_node(np, NULL, "ibm,iic") {
> + struct resource r[2];
> +
> + memset(&r, 0, sizeof(r));
> +
> + ret = of_address_to_resource(np, 0, &r[0]);
> + if (ret)
> + goto err;
> +
> + of_irq_to_resource(np, 0, &r[1]);
> +
> + i2c_dev = platform_device_register_simple("ibm,iic", i, r, 2);
> + if (IS_ERR(i2c_dev)) {
> + ret = PTR_ERR(i2c_dev);
> + goto err;
> + }
> +
> + of_register_i2c_devices(np, i++);
> + }
> +
> + return 0;
> +
> +err:
> + return ret;
> +}
> +
> +arch_initcall(sam440ep_i2c_of_init);
Ok, now I'm confused as to why you aren't doing this in your platform
file (which would make it specific, and therefore justify its existence.
> diff --git a/include/asm-powerpc/dma-mapping.h
> b/include/asm-powerpc/dma-mapping.h
> index bbefb69..3a6235a 100644
> --- a/include/asm-powerpc/dma-mapping.h
> +++ b/include/asm-powerpc/dma-mapping.h
You definitely need to explain what you're doing here with a commit log.
> diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
> index 23b7bc0..e7c64ac 100644
> --- a/sound/core/memalloc.c
> +++ b/sound/core/memalloc.c
This needs to be sent to the sound maintainer. And I'm sure they
probably won't like the ifdefs.
josh
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
