Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

Sergei Shtylyov
2007-07-25T17:04:52Z

Hello.

Vitaly Bordug wrote:

> This updates relevant platform code (freescale mpc8349itx target)
> to make the CompactFlash work in TrueIDE mode.

Erm, I'm not sure it's worth submitting the platform device driver for
PowerPC at this point, but well...

> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

> arch/powerpc/boot/dts/mpc8349emitx.dts | 9 ++++
> arch/powerpc/platforms/83xx/mpc834x_itx.c | 70 +++++++++++++++++++++++++++++
> 2 files changed, 79 insertions(+), 0 deletions(-)

> diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
> index db0d003..f8f0e8a 100644
> --- a/arch/powerpc/boot/dts/mpc8349emitx.dts
> +++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
> @@ -37,6 +37,15 @@
> reg = <00000000 10000000>;
> };
>
> + ide@f0000000 {
> + compatible = "mmio-ide";
> + device_type = "ide";

Why not "ata"?

> + reg = <f0000000 10 f000020c 4>;
> + ioport_shift = <1>;

Please use hyphen, not underscore in the property names ("device_type"
seems an only exception from this rule).
And since using shift instead of size buys you nothing is this case, I'd
prefer this property to be called reg-size or reg-stride.

[...]
> diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
> index 40a0194..d63a104 100644
> --- a/arch/powerpc/platforms/83xx/mpc834x_itx.c
> +++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c
[...]
> @@ -43,6 +44,75 @@ unsigned long isa_io_base = 0;
> unsigned long isa_mem_base = 0;
> #endif
>
> +static int __init mmio_ide_of_init(void)
> +{
> + struct device_node *np;
> + unsigned int i;
> +
> + for (np = NULL, i = 0;
> + (np = of_find_compatible_node(np, "ide", "mmio-ide")) != NULL;
> + i++) {
> + int ret = 0;

Unneeded initialization.

> + struct resource res[3];
> + struct platform_device *pdev = NULL;

Another one.

> + static struct pata_platform_info pdata;
> +
> + memset(res, 0, sizeof(res));
> +
> + ret = of_address_to_resource(np, 0, &res[0]);
> + if (ret) {
> + printk(KERN_ERR "mmio-ide.%d: unable to get "
> + "resource from OF\n", i);
> + goto err0;
> + }
> +
> + ret = of_address_to_resource(np, 1, &res[1]);
> + if (ret) {
> + printk(KERN_ERR "mmio-ide.%d: unable to get "
> + "resource from OF\n", i);
> + goto err0;

Erm, these printk's are repetitive, isn't it better to put them under err0
label?

> + }
> +
> + res[2].start = res[2].end = irq_of_parse_and_map(np, 0);
> + if (res[2].start == NO_IRQ) {
> + printk(KERN_ERR "mmio-ide.%d: no IRQ\n", i);
> + goto err0;
> + }
> + res[2].name = "pata_platform";
> + res[2].flags = IORESOURCE_IRQ;
> +
> + pdata.ioport_shift = *((u32 *)of_get_property(np,
> + "ioport_shift", NULL));
> +
> + pdev = platform_device_alloc("pata_platform", i);
> + if (!pdev)
> + goto err1;

Hm, not err0?

> +
> + ret = platform_device_add_data(pdev, &pdata, sizeof(pdata));
> + if (ret)
> + goto err1;
> +
> + ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
> + if (ret)
> + goto err1;
> +
> + ret = platform_device_register(pdev);
> + if (ret)
> + goto err1;
> +
> + continue;
> +err1:
> + printk(KERN_ERR "mmio-ide.%d: registration failed\n", i);
> + platform_device_del(pdev); /* it will free everything */
> +err0:
> + /* Even if some device failed, try others */
> + continue;
> + }
> +
> + return 0;
> +}
> +device_initcall(mmio_ide_of_init);
> +
> /* ************************************************************************
> *
> * Setup the architecture

MBR, Sergei