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

Sergei Shtylyov
2007-07-25T18:59:04Z

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);