Re: [PATCH 1/2] [IDE] Platform IDE driver (was: MMIO IDE driver)

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Vitaly Bordug wrote:

> This is now very similar to pata_platform.c, they both use
> same platform data structure and same resources.

> To achieve that, byte_lanes_swapping platform data variable
> and platform specified iops removed from that driver. It's fine,
> since those were never used anyway.

    Hopefully, PPC will never need them.

> pata_platform and ide_platform are carrying same driver names,
> to easily switch between these drivers, without need to touch
> platform code.

> diff --git a/drivers/ide/legacy/ide_platform.c b/drivers/ide/legacy/ide_platform.c
> new file mode 100644
> index 0000000..0b3f5b5
> --- /dev/null
> +++ b/drivers/ide/legacy/ide_platform.c
> @@ -0,0 +1,181 @@
> +/*
> + * Platform IDE driver
> + *
> + * Copyright (C) 2007 MontaVista Software
> + *
> + * Maintainer: Kumar Gala <galak@kernel.crashing.org>

    Poor Kumar, I guess he was surprised be being assigned a maintainer of the 
driver he didn't write (well, even if it borrowed some code from his earlier 
one ;-)...

> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/ide.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/pata_platform.h>
> +#include <linux/io.h>
> +
> +static struct {
> +	void __iomem *plat_ide_mapbase;
> +	void __iomem *plat_ide_alt_mapbase;
> +	ide_hwif_t *hwif;
> +	int index;
> +} hwif_prop;
> +
> +static ide_hwif_t *__devinit plat_ide_locate_hwif(void __iomem *base,
> +	    void __iomem *ctrl, struct pata_platform_info *pdata, int irq,
> +	    int mmio)
> +{
> +	unsigned long port = (unsigned long)base;
> +	ide_hwif_t *hwif;
> +	int index, i;
> +
> +	for (index = 0; index < MAX_HWIFS; ++index) {
> +		hwif = ide_hwifs + index;
> +		if (hwif->io_ports[IDE_DATA_OFFSET] == port)
> +			goto found;
> +	}
> +
> +	for (index = 0; index < MAX_HWIFS; ++index) {
> +		hwif = ide_hwifs + index;
> +		if (hwif->io_ports[IDE_DATA_OFFSET] == 0)
> +			goto found;
> +	}
> +
> +	return NULL;
> +
> +found:
> +
> +	hwif->hw.io_ports[IDE_DATA_OFFSET] = port;
> +
> +	port += (1 << pdata->ioport_shift);
> +	for (i = IDE_ERROR_OFFSET; i <= IDE_STATUS_OFFSET;
> +	     i++, port += (1 << pdata->ioport_shift))

    Looks like shift doesn't buy as anything, why not just use stride?

> +		hwif->hw.io_ports[i] = port;
> +
> +	hwif->hw.io_ports[IDE_CONTROL_OFFSET] = (unsigned long)ctrl;
> +
> +	memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
> +	hwif->hw.irq = hwif->irq = irq;
> +
> +	hwif->hw.dma = NO_DMA;
> +	hwif->hw.chipset = ide_generic;
> +
> +	if (mmio) {
> +		hwif->mmio = 1;
> +		default_hwif_mmiops(hwif);
> +	}

    And why default_hwif_iops(hwif) is not called else?
    And I remember the previos variant was overriding INSW()/OUTSW() methods 
-- turned out to be unnecessary? :-)

> +
> +	hwif_prop.hwif = hwif;
> +	hwif_prop.index = index;
> +
> +	return hwif;
> +}
> +
> +static int __devinit plat_ide_probe(struct platform_device *pdev)
> +{
> +	struct resource *res_base, *res_alt, *res_irq;
> +	ide_hwif_t *hwif;
> +	struct pata_platform_info *pdata;
> +	int ret = 0;
> +	int mmio = 0;
> +
> +	pdata = pdev->dev.platform_data;
> +
> +	/* get a pointer to the register memory */
> +	res_base = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	res_alt = platform_get_resource(pdev, IORESOURCE_IO, 1);
> +
> +	if (!res_base || !res_alt) {
> +		res_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		res_alt = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		if (!res_base || !res_alt) {
> +			ret = -ENOMEM;

    ENODEV or EINVAL you mean? :-)

> +			goto out;

    Bleh... just return please.

> +		}
> +		mmio = 1;
> +	}
> +
> +	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!res_irq) {
> +		ret = -EINVAL;
> +		goto out;

    Bleh... return -EINVAL, please. Or maybe -ENODEV.

> +	}
> +
[...]
> +	hwif = plat_ide_locate_hwif(hwif_prop.plat_ide_mapbase,
> +	         hwif_prop.plat_ide_alt_mapbase, pdata, res_irq->start, mmio);
> +
> +	if (!hwif) {
> +		ret = -ENODEV;
> +		goto out;

    Bleh... please just

return -ENODEV.

> +	}
> +	hwif->gendev.parent = &pdev->dev;
> +	hwif->noprobe = 0;
> +
> +	probe_hwif_init(hwif);
> +
> +	platform_set_drvdata(pdev, hwif);
> +	ide_proc_register_port(hwif);
> +
> +	return 0;
> +
> +out:

    No need to abuse the function cleanup style when you have nothing to 
cleanup. :-/

> +	return ret;
> +}
> +
> +static int __devexit plat_ide_remove(struct platform_device *pdev)
> +{
> +	ide_hwif_t *hwif = pdev->dev.driver_data;
> +
> +	if (hwif != hwif_prop.hwif) {
> +		dev_printk(KERN_DEBUG, &pdev->dev, "%s: hwif value error",
> +		           pdev->name);
> +	} else {
> +		ide_unregister(hwif_prop.index);
> +		hwif_prop.index = 0;
> +		hwif_prop.hwif = NULL;
> +	}
> +

    I'd have interchanged the success/failure branches...

> +	return 0;
> +}
> +
> +static struct platform_driver platform_ide_driver = {
> +	.driver {
> +		.name = "pata_platform",
> +	},
> +	.probe = plat_ide_probe,
> +	.remove = __devexit_p(plat_ide_remove),
> +};
> +
> +static int __init platform_ide_init(void)
> +{
> +	return platform_driver_register(&platform_ide_driver);
> +}
> +
> +static void __exit platform_ide_exit(void)
> +{
> +	platform_driver_unregister(&platform_ide_driver);
> +}
> +
> +MODULE_DESCRIPTION("Platform IDE driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(platform_ide_init);
> +module_exit(platform_ide_exit);