[RFC][PATCH 0/3] OF-platform PATA driver

[RFC][PATCH 0/3] OF-platform PATA driver

[Anton Vorontsov]

Hi all,

Here is the PATA Platform driver using OF infrastructure.

Mostly it's just a wrapper around a bit modified pata_platform
driver.

Patches are well split for the easier review:

First one factors out platform_device specific bits and modifies
pata_platform to be a library-alike driver (with platform_device
default binding).

Second patch is OF-driver itself which is using pata_platform
"library".

Third patch is PowerPC specific, but I'm still Cc'ing linux-ide,
just to show how we're using it.


As an alternative approach we can use plain pata_platform
driver, but I'm not sure how Linux OF bindings' ideologists will
or will not like it.

So, these patches are strongly Request For Comments. Feel free
to train your nitpicking skills ;-), and/or vote for the option
you most pleased about (or suggest another?).


Thanks.

---
Down here is "alternative approach".

Probably board-neutral version may be placed somewhere in
the drivers/of/...? But who will call it: board file, or
device_initcall for all boards?

diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
index 150fafb..4caa90d 100644
--- a/arch/powerpc/platforms/83xx/mpc834x_itx.c
+++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c
@@ -24,6 +24,7 @@
 #include <linux/seq_file.h>
 #include <linux/root_dev.h>
 #include <linux/of_platform.h>
+#include <linux/pata_platform.h>
 
 #include <asm/system.h>
 #include <asm/atomic.h>
@@ -102,6 +103,78 @@ static int __init mpc834x_itx_probe(void)
         return of_flat_dt_is_compatible(root, "MPC834xMITX");
 }
 
+static int __init mpc834x_itx_pata_init(void)
+{
+	struct device_node *np;
+	unsigned int i;
+
+	if (!machine_is(mpc834x_itx))
+		return 0;
+
+	for (np = NULL, i = 0;
+	     (np = of_find_compatible_node(np, NULL, "pata-platform"));
+	     i++) {
+		int ret;
+		struct resource res[3];
+		const u32 *ioport_shift;
+		struct platform_device *pdev;
+		struct pata_platform_info pdata = {};
+
+		memset(res, 0, sizeof(res));
+
+		ret = of_address_to_resource(np, 0, &res[0]);
+		if (ret) {
+			printk(KERN_ERR "pata.%d: unable to get IO address "
+			       "from the device tree\n", i);
+			goto err0;
+		}
+
+		ret = of_address_to_resource(np, 1, &res[1]);
+		if (ret) {
+			printk(KERN_ERR "pata.%d: unable to get CTL address "
+			       "from the device tree\n", i);
+			goto err0;
+		}
+
+		ret = of_irq_to_resource(np, 0, &res[2]);
+		if (ret == NO_IRQ) {
+			printk(KERN_ERR "pata.%d: no IRQ\n", i);
+			goto err0;
+		}
+
+		ioport_shift = of_get_property(np, "ioport-shift", NULL);
+		if (ioport_shift)
+			pdata.ioport_shift = *ioport_shift;
+
+		pdev = platform_device_alloc("pata_platform", i);
+		if (!pdev)
+			goto 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 "pata.%d: registration failed\n", i);
+		platform_device_del(pdev); /* will free everything */
+err0:
+		/* Even if some device failed, try others */
+		continue;
+	}
+
+	return 0;
+}
+device_initcall(mpc834x_itx_pata_init);
+
 define_machine(mpc834x_itx) {
 	.name			= "MPC834x ITX",
 	.probe			= mpc834x_itx_probe,

[PATCH 1/3] [libata] pata_platform: make probe and remove functions device type neutral

[Anton Vorontsov]

Split pata_platform_{probe,remove} into two pieces:
1. pata_platform_{probe,remove} -- platform_device-dependant bits
2. __ptata_platform_{probe,remove} -- device type neutral bits.

This is done to not duplicate code for the OF-platform driver.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/ata/pata_platform.c |  139 +++++++++++++++++++++++++------------------
 drivers/ata/pata_platform.h |   12 ++++
 2 files changed, 94 insertions(+), 57 deletions(-)
 create mode 100644 drivers/ata/pata_platform.h

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index ac03a90..6436c38 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -93,14 +93,9 @@ static struct ata_port_operations pata_platform_port_ops = {
 };
 
 static void pata_platform_setup_port(struct ata_ioports *ioaddr,
-				     struct pata_platform_info *info)
+				     unsigned int shift)
 {
-	unsigned int shift = 0;
-
 	/* Fixup the port shift for platforms that need it */
-	if (info && info->ioport_shift)
-		shift = info->ioport_shift;
-
 	ioaddr->data_addr	= ioaddr->cmd_addr + (ATA_REG_DATA    << shift);
 	ioaddr->error_addr	= ioaddr->cmd_addr + (ATA_REG_ERR     << shift);
 	ioaddr->feature_addr	= ioaddr->cmd_addr + (ATA_REG_FEATURE << shift);
@@ -114,8 +109,12 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
 }
 
 /**
- *	pata_platform_probe		-	attach a platform interface
- *	@pdev: platform device
+ *	__pata_platform_probe		-	attach a platform interface
+ *	@dev: device
+ *	@io_res: Resource representing I/O base
+ *	@ctl_res: Resource representing CTL base
+ *	@irq_res: Resource representing IRQ and its flags
+ *	@ioport_shift: I/O port shift
  *
  *	Register a platform bus IDE interface. Such interfaces are PIO and we
  *	assume do not support IRQ sharing.
@@ -135,42 +134,17 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
  *
  *	If no IRQ resource is present, PIO polling mode is used instead.
  */
-static int __devinit pata_platform_probe(struct platform_device *pdev)
+int __devinit __pata_platform_probe(struct device *dev,
+				    struct resource *io_res,
+				    struct resource *ctl_res,
+				    struct resource *irq_res,
+				    unsigned int ioport_shift)
 {
-	struct resource *io_res, *ctl_res;
 	struct ata_host *host;
 	struct ata_port *ap;
-	struct pata_platform_info *pp_info;
 	unsigned int mmio;
-	int irq;
-
-	/*
-	 * Simple resource validation ..
-	 */
-	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
-		dev_err(&pdev->dev, "invalid number of resources\n");
-		return -EINVAL;
-	}
-
-	/*
-	 * Get the I/O base first
-	 */
-	io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (io_res == NULL) {
-		io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		if (unlikely(io_res == NULL))
-			return -EINVAL;
-	}
-
-	/*
-	 * Then the CTL base
-	 */
-	ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
-	if (ctl_res == NULL) {
-		ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		if (unlikely(ctl_res == NULL))
-			return -EINVAL;
-	}
+	int irq = 0;
+	int irq_flags = 0;
 
 	/*
 	 * Check for MMIO
@@ -181,14 +155,15 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)
 	/*
 	 * And the IRQ
 	 */
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		irq = 0;	/* no irq */
+	if (irq_res && irq_res->start > 0) {
+		irq = irq_res->start;
+		irq_flags = irq_res->flags;
+	}
 
 	/*
 	 * Now that that's out of the way, wire up the port..
 	 */
-	host = ata_host_alloc(&pdev->dev, 1);
+	host = ata_host_alloc(dev, 1);
 	if (!host)
 		return -ENOMEM;
 	ap = host->ports[0];
@@ -209,25 +184,24 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)
 	 * Handle the MMIO case
 	 */
 	if (mmio) {
-		ap->ioaddr.cmd_addr = devm_ioremap(&pdev->dev, io_res->start,
+		ap->ioaddr.cmd_addr = devm_ioremap(dev, io_res->start,
 				io_res->end - io_res->start + 1);
-		ap->ioaddr.ctl_addr = devm_ioremap(&pdev->dev, ctl_res->start,
+		ap->ioaddr.ctl_addr = devm_ioremap(dev, ctl_res->start,
 				ctl_res->end - ctl_res->start + 1);
 	} else {
-		ap->ioaddr.cmd_addr = devm_ioport_map(&pdev->dev, io_res->start,
+		ap->ioaddr.cmd_addr = devm_ioport_map(dev, io_res->start,
 				io_res->end - io_res->start + 1);
-		ap->ioaddr.ctl_addr = devm_ioport_map(&pdev->dev, ctl_res->start,
+		ap->ioaddr.ctl_addr = devm_ioport_map(dev, ctl_res->start,
 				ctl_res->end - ctl_res->start + 1);
 	}
 	if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) {
-		dev_err(&pdev->dev, "failed to map IO/CTL base\n");
+		dev_err(dev, "failed to map IO/CTL base\n");
 		return -ENOMEM;
 	}
 
 	ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
 
-	pp_info = pdev->dev.platform_data;
-	pata_platform_setup_port(&ap->ioaddr, pp_info);
+	pata_platform_setup_port(&ap->ioaddr, ioport_shift);
 
 	ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport",
 		      (unsigned long long)io_res->start,
@@ -235,26 +209,77 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)
 
 	/* activate */
 	return ata_host_activate(host, irq, irq ? ata_interrupt : NULL,
-				 pp_info ? pp_info->irq_flags : 0,
-				 &pata_platform_sht);
+				 irq_flags, &pata_platform_sht);
 }
+EXPORT_SYMBOL_GPL(__pata_platform_probe);
 
 /**
- *	pata_platform_remove	-	unplug a platform interface
- *	@pdev: platform device
+ *	__pata_platform_remove		-	unplug a platform interface
+ *	@dev: device
  *
  *	A platform bus ATA device has been unplugged. Perform the needed
  *	cleanup. Also called on module unload for any active devices.
  */
-static int __devexit pata_platform_remove(struct platform_device *pdev)
+int __devexit __pata_platform_remove(struct device *dev)
 {
-	struct device *dev = &pdev->dev;
 	struct ata_host *host = dev_get_drvdata(dev);
 
 	ata_host_detach(host);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(__pata_platform_remove);
+
+static int __devinit pata_platform_probe(struct platform_device *pdev)
+{
+	struct resource *io_res;
+	struct resource *ctl_res;
+	struct resource *irq_res;
+	struct pata_platform_info *pp_info = pdev->dev.platform_data;
+
+	/*
+	 * Simple resource validation ..
+	 */
+	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
+		dev_err(&pdev->dev, "invalid number of resources\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Get the I/O base first
+	 */
+	io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (io_res == NULL) {
+		io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (unlikely(io_res == NULL))
+			return -EINVAL;
+	}
+
+	/*
+	 * Then the CTL base
+	 */
+	ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
+	if (ctl_res == NULL) {
+		ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (unlikely(ctl_res == NULL))
+			return -EINVAL;
+	}
+
+	/*
+	 * And the IRQ
+	 */
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (irq_res)
+		irq_res->flags = pp_info ? pp_info->irq_flags : 0;
+
+	return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
+				     pp_info ? pp_info->ioport_shift : 0);
+}
+
+static int __devexit pata_platform_remove(struct platform_device *pdev)
+{
+	return __pata_platform_remove(&pdev->dev);
+}
 
 static struct platform_driver pata_platform_driver = {
 	.probe		= pata_platform_probe,
diff --git a/drivers/ata/pata_platform.h b/drivers/ata/pata_platform.h
new file mode 100644
index 0000000..9752a42
--- /dev/null
+++ b/drivers/ata/pata_platform.h
@@ -0,0 +1,12 @@
+#ifndef __DRIVERS_ATA_PATA_PLATFORM_H
+#define __DRIVERS_ATA_PATA_PLATFORM_H
+
+extern int __devinit __pata_platform_probe(struct device *dev,
+					   struct resource *io_res,
+					   struct resource *ctl_res,
+					   struct resource *irq_res,
+					   unsigned int ioport_shift);
+
+extern int __devexit __pata_platform_remove(struct device *dev);
+
+#endif /* __DRIVERS_ATA_PATA_PLATFORM_H */
-- 
1.5.2.2

[PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/ata/Kconfig            |   10 +++++
 drivers/ata/Makefile           |    1 +
 drivers/ata/pata_of_platform.c |   88 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ata/pata_of_platform.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ba63619..5a492fa 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -614,6 +614,16 @@ config PATA_PLATFORM
 
 	  If unsure, say N.
 
+config PATA_OF_PLATFORM
+	tristate "OpenFirmware platform device PATA support"
+	depends on PATA_PLATFORM && PPC_OF
+	help
+	  This option enables support for generic directly connected ATA
+	  devices commonly found on embedded systems with OpenFirmware
+	  bindings.
+
+	  If unsure, say N.
+
 config PATA_ICSIDE
 	tristate "Acorn ICS PATA support"
 	depends on ARM && ARCH_ACORN
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b13feb2..ebcee64 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF)	+= pata_ixp4xx_cf.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_BF54X)	+= pata_bf54x.o
 obj-$(CONFIG_PATA_PLATFORM)	+= pata_platform.o
+obj-$(CONFIG_PATA_OF_PLATFORM)	+= pata_of_platform.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
 # Should be last but two libata driver
 obj-$(CONFIG_PATA_ACPI)		+= pata_acpi.o
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
new file mode 100644
index 0000000..b4eca70
--- /dev/null
+++ b/drivers/ata/pata_of_platform.c
@@ -0,0 +1,88 @@
+/*
+ * OF-platform PATA driver
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ *                     Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include "pata_platform.h"
+
+static int __devinit pata_of_platform_probe(struct of_device *ofdev,
+					    const struct of_device_id *match)
+{
+	int ret;
+	struct device_node *dn = ofdev->node;
+	struct resource io_res;
+	struct resource ctl_res;
+	struct resource irq_res;
+	unsigned int ioport_shift = 0;
+	uint32_t *prop;
+
+	ret = of_address_to_resource(dn, 0, &io_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get IO address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(dn, 1, &ctl_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get CTL address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_irq_to_resource(dn, 0, &irq_res);
+	if (ret == NO_IRQ)
+		irq_res.start = irq_res.end = -1;
+	else
+		irq_res.flags = 0;
+
+	prop = (uint32_t *)of_get_property(dn, "ioport-shift", NULL);
+	if (prop)
+		ioport_shift = *prop;
+
+	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, &irq_res,
+				     ioport_shift);
+}
+
+static int __devexit pata_of_platform_remove(struct of_device *ofdev)
+{
+	return __pata_platform_remove(&ofdev->dev);
+}
+
+static struct of_device_id pata_of_platform_match[] = {
+	{
+		.compatible = "pata-platform",
+	},
+};
+
+static struct of_platform_driver pata_of_platform_driver = {
+	.name		= "pata_of_platform",
+	.match_table	= pata_of_platform_match,
+	.probe		= pata_of_platform_probe,
+	.remove		= __devexit_p(pata_of_platform_remove),
+};
+
+static int __init pata_of_platform_init(void)
+{
+	return of_register_platform_driver(&pata_of_platform_driver);
+}
+module_init(pata_of_platform_init);
+
+static void __exit pata_platform_exit(void)
+{
+	of_unregister_platform_driver(&pata_of_platform_driver);
+}
+module_exit(pata_platform_exit);
+
+MODULE_DESCRIPTION("OF-platform PATA driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.2.2

[PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce pata node, make use pata_of_platform driver

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/boot/dts/mpc8349emitx.dts    |    9 +++++++--
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   17 +++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
index 5072f6d..898c294 100644
--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
@@ -249,6 +249,11 @@
 		device_type = "pci";
 	};
 
-
-
+	pata@f0000000 {
+		compatible = "fsl,mpc8349emitx", "pata-platform";
+		reg = <f0000000 10 f000020c 4>;
+		ioport-shift = <1>;
+		interrupts = <17 8>;
+		interrupt-parent = <&ipic>;
+	};
 };
diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
index aa76819..150fafb 100644
--- a/arch/powerpc/platforms/83xx/mpc834x_itx.c
+++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/seq_file.h>
 #include <linux/root_dev.h>
+#include <linux/of_platform.h>
 
 #include <asm/system.h>
 #include <asm/atomic.h>
@@ -37,6 +38,22 @@
 
 #include "mpc83xx.h"
 
+static struct of_device_id mpc834x_ids[] = {
+	{ .compatible = "pata-platform", },
+	{},
+};
+
+static int __init mpc834x_declare_of_platform_devices(void)
+{
+	if (!machine_is(mpc834x_itx))
+		return 0;
+
+	of_platform_bus_probe(NULL, mpc834x_ids, NULL);
+
+	return 0;
+}
+device_initcall(mpc834x_declare_of_platform_devices);
+
 /* ************************************************************************
  *
  * Setup the architecture
-- 
1.5.2.2

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Jeff Garzik]

Anton Vorontsov wrote:
> Hi all,
> 
> Here is the PATA Platform driver using OF infrastructure.
> 
> Mostly it's just a wrapper around a bit modified pata_platform
> driver.
> 
> Patches are well split for the easier review:
> 
> First one factors out platform_device specific bits and modifies
> pata_platform to be a library-alike driver (with platform_device
> default binding).
> 
> Second patch is OF-driver itself which is using pata_platform
> "library".
> 
> Third patch is PowerPC specific, but I'm still Cc'ing linux-ide,
> just to show how we're using it.
> 
> 
> As an alternative approach we can use plain pata_platform
> driver, but I'm not sure how Linux OF bindings' ideologists will
> or will not like it.
> 
> So, these patches are strongly Request For Comments. Feel free
> to train your nitpicking skills ;-), and/or vote for the option
> you most pleased about (or suggest another?).

Seems reasonable from a libata point of view...


(pata_platform maintainer CC'd)

	Jeff

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Benjamin Herrenschmidt]

On Fri, 2007-11-23 at 20:52 +0300, Anton Vorontsov wrote:

> As an alternative approach we can use plain pata_platform
> driver, but I'm not sure how Linux OF bindings' ideologists will
> or will not like it.
> 
> So, these patches are strongly Request For Comments. Feel free
> to train your nitpicking skills ;-), and/or vote for the option
> you most pleased about (or suggest another?).
> 
> 
> Thanks.
> 
> ---
> Down here is "alternative approach".
> 
> Probably board-neutral version may be placed somewhere in
> the drivers/of/...? But who will call it: board file, or
> device_initcall for all boards?

 .../...

I'm fine with either approaches in fact. I think it's fair enough when
there is existing platform devices to avoid duplicating the whole thing
for an of_platform_device (unless you find a nice way to split to a
library like you just did) and instead, have a "constructor" that
constructs the platform device based on the OF node.

Now, as you pointed out, the question is where to call it... the
initcall has the problem of potentially being called for the wrong board
and the thing having duplicate OF device and platform device created for
it...

One of the thing I've been brewing in my mind is the idea of reworking
the current code that create OF platform devices when walking busses
down, and have it instead call registered constructor functions. The
default ones would then create OF platform devices but one could
override that with constructors creating something else (such as a
platform device).

Now, it's not trivial, as there is a bit of a mixup on whether a "bus"
should create all children and thus define their type (PCI would do such
a thing) or whether the type is defined by the device object itself
which typically would be the case for "generic" busses such as SoCs or
PLBs when creating platform devices. I haven't quite found out a nice
way to enable both possibilities... if you have ideas, please share.

In the meantime, maybe it's best to merge the of_platform_device
approach since Jeff agrees with it.

Cheers,
Ben.

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Vitaly Bordug]

On Fri, 23 Nov 2007 20:52:29 +0300
Anton Vorontsov wrote:

> Hi all,
> 
> Here is the PATA Platform driver using OF infrastructure.
> 
> Mostly it's just a wrapper around a bit modified pata_platform
> driver.
> 
> Patches are well split for the easier review:
> 
> First one factors out platform_device specific bits and modifies
> pata_platform to be a library-alike driver (with platform_device
> default binding).
> 
> Second patch is OF-driver itself which is using pata_platform
> "library".
> 
> Third patch is PowerPC specific, but I'm still Cc'ing linux-ide,
> just to show how we're using it.
> 
> 
> As an alternative approach we can use plain pata_platform
> driver, but I'm not sure how Linux OF bindings' ideologists will
> or will not like it.
> 
> So, these patches are strongly Request For Comments. Feel free
> to train your nitpicking skills ;-), and/or vote for the option
> you most pleased about (or suggest another?).
> 

I vote for of_platform_device, since we seem to dodge the clash
with platform_device here.

The code looks good, so I'm going to ack all 3 patches and if there will
be no more nagging, I'm inclined to ask Kumar to queue this for the next merge window.



-- 
Sincerely, Vitaly

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Paul Mundt]

On Fri, Nov 23, 2007 at 07:49:33PM -0500, Jeff Garzik wrote:
> Anton Vorontsov wrote:
> >Here is the PATA Platform driver using OF infrastructure.
> >
> >Mostly it's just a wrapper around a bit modified pata_platform
> >driver.
> >
> >Patches are well split for the easier review:
> >
> >First one factors out platform_device specific bits and modifies
> >pata_platform to be a library-alike driver (with platform_device
> >default binding).
> >
The only issue I have here is that this library-like version has subtle
semantic changes that will break existing drivers.

irq_flags exists in struct pata_platform_info precisely for the IRQ
resource IRQ flags (as opposed to the IORESOURCE flags, which are what
the IRQ resource flags refer to instead). This change takes that for
granted and just assumes we're going to be using the res->flags, which is
both an invalid assumption, and will utterly break blackfin and others
that depend on it.

Incidentally, we already have an include/linux/pata_platform.h. If this
is going to be library-like, through the prototypes in there, rather than
splitting them up betewen include/linux and drivers/ata. We don't need
two headers.

These patches basically look fine to me otherwise, though it would be
nice if the semantic-changing bits had been abstracted. So as long as the
old irq_flags behaviour is maintained and that irq_res->flags stuff is
ripped out, I'll add my Acked-by as well.

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Arnd Bergmann]

On Friday 23 November 2007, Anton Vorontsov wrote:
> Here is the PATA Platform driver using OF infrastructure.
> 
> Mostly it's just a wrapper around a bit modified pata_platform
> driver.

Thanks a lot for doing this. Patches 2/3 are what I tried to get
people to do for some time now but was too lazy to do myself.

As a further thought, do the drivers now still need to be
pata specific, or should the OF part be called ata_of_platform
instead and also be used for sata devices?

	Arnd <><

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce pata node, make use pata_of_platform driver

[Arnd Bergmann]

On Friday 23 November 2007, Anton Vorontsov wrote:
>=20
> +static struct of_device_id mpc834x_ids[] =3D {
> +=A0=A0=A0=A0=A0=A0=A0{ .compatible =3D "pata-platform", },
> +=A0=A0=A0=A0=A0=A0=A0{},
> +};
> +
> +static int __init mpc834x_declare_of_platform_devices(void)
> +{
> +=A0=A0=A0=A0=A0=A0=A0if (!machine_is(mpc834x_itx))
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return 0;
> +
> +=A0=A0=A0=A0=A0=A0=A0of_platform_bus_probe(NULL, mpc834x_ids, NULL);
> +
> +=A0=A0=A0=A0=A0=A0=A0return 0;
> +}
> +device_initcall(mpc834x_declare_of_platform_devices);

This is not really how of_platform_bus_probe was meant to be used.
Instead of listing the device you want to probe, you should list
all buses that potentially contain a device that you are probing.

Normally, an ata controller is not a top-level device but instead
the child of an SOC bus device, and then you just probe all
SOC devices, which means their children get added to the device
tree.

In your case, that would probably mean that you have to another
entry in the "ranges" of the soc8349 node, or add a second socXXXX
node that has the 0xf0000000 ranges, depending on the actual layout
of the SOC.

	Arnd <><

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Anton Vorontsov]

On Sat, Nov 24, 2007 at 09:50:07PM +0100, Arnd Bergmann wrote:
> On Friday 23 November 2007, Anton Vorontsov wrote:
> > Here is the PATA Platform driver using OF infrastructure.
> > 
> > Mostly it's just a wrapper around a bit modified pata_platform
> > driver.
> 
> Thanks a lot for doing this. Patches 2/3 are what I tried to get
> people to do for some time now but was too lazy to do myself.
> 
> As a further thought, do the drivers now still need to be
> pata specific, or should the OF part be called ata_of_platform
> instead and also be used for sata devices?

Ugh, I don't know much about sata, it should act just as a pata
in the very basic usage, IIRC. So it's worth trying, but I don't
have any platform satas to try... :-/

Hereby, I'd rather stick with pata name, as it's never too late
to simply rename things later.

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Anton Vorontsov]

On Sat, Nov 24, 2007 at 04:26:13PM +0900, Paul Mundt wrote:
> On Fri, Nov 23, 2007 at 07:49:33PM -0500, Jeff Garzik wrote:
> > Anton Vorontsov wrote:
> > >Here is the PATA Platform driver using OF infrastructure.
> > >
> > >Mostly it's just a wrapper around a bit modified pata_platform
> > >driver.
> > >
> > >Patches are well split for the easier review:
> > >
> > >First one factors out platform_device specific bits and modifies
> > >pata_platform to be a library-alike driver (with platform_device
> > >default binding).
> > >
> The only issue I have here is that this library-like version has subtle
> semantic changes that will break existing drivers.

Actually I've tried to keep semantics intact:

+static int __devinit pata_platform_probe(struct platform_device *pdev)
[...]
+       /*
+        * And the IRQ
+        */
+       irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+       if (irq_res)
+               irq_res->flags = pp_info ? pp_info->irq_flags : 0;
[...]

So, I'm passing flags from the platform data. Did you overlook these
bits, or I'm still changing semantics somewhere else?

> irq_flags exists in struct pata_platform_info precisely for the IRQ
> resource IRQ flags (as opposed to the IORESOURCE flags, which are what
> the IRQ resource flags refer to instead). This change takes that for
> granted and just assumes we're going to be using the res->flags, which is
> both an invalid assumption, and will utterly break blackfin and others
> that depend on it.
> 
> Incidentally, we already have an include/linux/pata_platform.h. If this
> is going to be library-like, through the prototypes in there, rather than
> splitting them up betewen include/linux and drivers/ata. We don't need
> two headers.

My intention was: keep "private" declarations in the drivers/ata/ and
"public" in the include/linux/ -- to not confuse pata_platform users
by __pata_platform_* internal stuff. But okay, I'll merge them.

> These patches basically look fine to me otherwise, though it would be
> nice if the semantic-changing bits had been abstracted. So as long as the
> old irq_flags behaviour is maintained and that irq_res->flags stuff is
> ripped out, I'll add my Acked-by as well.


Much thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce pata node, make use pata_of_platform driver

[Anton Vorontsov]

On Sat, Nov 24, 2007 at 09:57:46PM +0100, Arnd Bergmann wrote:
> On Friday 23 November 2007, Anton Vorontsov wrote:
> > 
> > +static struct of_device_id mpc834x_ids[] = {
> > +       { .compatible = "pata-platform", },
> > +       {},
> > +};
> > +
> > +static int __init mpc834x_declare_of_platform_devices(void)
> > +{
> > +       if (!machine_is(mpc834x_itx))
> > +               return 0;
> > +
> > +       of_platform_bus_probe(NULL, mpc834x_ids, NULL);
> > +
> > +       return 0;
> > +}
> > +device_initcall(mpc834x_declare_of_platform_devices);
> 
> This is not really how of_platform_bus_probe was meant to be used.
> Instead of listing the device you want to probe, you should list
> all buses that potentially contain a device that you are probing.

Yup, I sort of knew it. For mpc8349emitx, pata node should be in
the localbus node. But there is no localbus node yet... I'll fix
that issue in the next round of these patches.


Much thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [RFC][PATCH 0/3] OF-platform PATA driver

[Paul Mundt]

On Mon, Nov 26, 2007 at 03:23:14AM +0300, Anton Vorontsov wrote:
> On Sat, Nov 24, 2007 at 04:26:13PM +0900, Paul Mundt wrote:
> > On Fri, Nov 23, 2007 at 07:49:33PM -0500, Jeff Garzik wrote:
> > > Anton Vorontsov wrote:
> > > >Here is the PATA Platform driver using OF infrastructure.
> > > >
> > > >Mostly it's just a wrapper around a bit modified pata_platform
> > > >driver.
> > > >
> > > >Patches are well split for the easier review:
> > > >
> > > >First one factors out platform_device specific bits and modifies
> > > >pata_platform to be a library-alike driver (with platform_device
> > > >default binding).
> > > >
> > The only issue I have here is that this library-like version has subtle
> > semantic changes that will break existing drivers.
> 
> Actually I've tried to keep semantics intact:
> 
> +static int __devinit pata_platform_probe(struct platform_device *pdev)
> [...]
> +       /*
> +        * And the IRQ
> +        */
> +       irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       if (irq_res)
> +               irq_res->flags = pp_info ? pp_info->irq_flags : 0;
> [...]
> 
> So, I'm passing flags from the platform data. Did you overlook these
> bits, or I'm still changing semantics somewhere else?
> 
Oh, I overlooked that. Using irq_res->flags as a temporary for
pp_info->irq_flags seems a bit hacky, but as long as pp_info->irq_flags
semantics are intact, I'm not too violently opposed to this anyways.

> > Incidentally, we already have an include/linux/pata_platform.h. If this
> > is going to be library-like, through the prototypes in there, rather than
> > splitting them up betewen include/linux and drivers/ata. We don't need
> > two headers.
> 
> My intention was: keep "private" declarations in the drivers/ata/ and
> "public" in the include/linux/ -- to not confuse pata_platform users
> by __pata_platform_* internal stuff. But okay, I'll merge them.
> 
I suppose that depends on whether the intent is that all pata_platform
users will be stuck in drivers/ata or not. If this is treated as more of
a library, implementations can simply bury themselves in arch/ land if
they feel like it.

[PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

This driver nicely wraps around pata_platform library functions,
and provides OF platform bus bindings to the PATA devices.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/ata/Kconfig            |   10 +++++
 drivers/ata/Makefile           |    1 +
 drivers/ata/pata_of_platform.c |   86 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ata/pata_of_platform.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ba63619..5a492fa 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -614,6 +614,16 @@ config PATA_PLATFORM
 
 	  If unsure, say N.
 
+config PATA_OF_PLATFORM
+	tristate "OpenFirmware platform device PATA support"
+	depends on PATA_PLATFORM && PPC_OF
+	help
+	  This option enables support for generic directly connected ATA
+	  devices commonly found on embedded systems with OpenFirmware
+	  bindings.
+
+	  If unsure, say N.
+
 config PATA_ICSIDE
 	tristate "Acorn ICS PATA support"
 	depends on ARM && ARCH_ACORN
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b13feb2..ebcee64 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF)	+= pata_ixp4xx_cf.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_BF54X)	+= pata_bf54x.o
 obj-$(CONFIG_PATA_PLATFORM)	+= pata_platform.o
+obj-$(CONFIG_PATA_OF_PLATFORM)	+= pata_of_platform.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
 # Should be last but two libata driver
 obj-$(CONFIG_PATA_ACPI)		+= pata_acpi.o
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
new file mode 100644
index 0000000..e6c769c
--- /dev/null
+++ b/drivers/ata/pata_of_platform.c
@@ -0,0 +1,86 @@
+/*
+ * OF-platform PATA driver
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ *                     Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pata_platform.h>
+
+static int __devinit pata_of_platform_probe(struct of_device *ofdev,
+					    const struct of_device_id *match)
+{
+	int ret;
+	struct device_node *dn = ofdev->node;
+	struct resource io_res;
+	struct resource ctl_res;
+	struct resource irq_res;
+	unsigned int ioport_shift = 0;
+	uint32_t *prop;
+
+	ret = of_address_to_resource(dn, 0, &io_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get IO address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(dn, 1, &ctl_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get CTL address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_irq_to_resource(dn, 0, &irq_res);
+	if (ret == NO_IRQ)
+		irq_res.start = irq_res.end = -1;
+	else
+		irq_res.flags = 0;
+
+	prop = (uint32_t *)of_get_property(dn, "ioport-shift", NULL);
+	if (prop)
+		ioport_shift = *prop;
+
+	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, &irq_res,
+				     ioport_shift);
+}
+
+static int __devexit pata_of_platform_remove(struct of_device *ofdev)
+{
+	return __pata_platform_remove(&ofdev->dev);
+}
+
+static struct of_device_id pata_of_platform_match[] = {
+	{ .compatible = "pata-platform", },
+};
+
+static struct of_platform_driver pata_of_platform_driver = {
+	.name		= "pata_of_platform",
+	.match_table	= pata_of_platform_match,
+	.probe		= pata_of_platform_probe,
+	.remove		= __devexit_p(pata_of_platform_remove),
+};
+
+static int __init pata_of_platform_init(void)
+{
+	return of_register_platform_driver(&pata_of_platform_driver);
+}
+module_init(pata_of_platform_init);
+
+static void __exit pata_of_platform_exit(void)
+{
+	of_unregister_platform_driver(&pata_of_platform_driver);
+}
+module_exit(pata_of_platform_exit);
+
+MODULE_DESCRIPTION("OF-platform PATA driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.2.2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Olof Johansson]

On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:
> This driver nicely wraps around pata_platform library functions,
> and provides OF platform bus bindings to the PATA devices.

> +static struct of_device_id pata_of_platform_match[] = {
> +	{ .compatible = "pata-platform", },
> +};

"pata-platform" really means nothing outside of linux. A more
generic label would be useful.


-Olof

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Arnd Bergmann]

On Tuesday 27 November 2007, Olof Johansson wrote:
> On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:
> > This driver nicely wraps around pata_platform library functions,
> > and provides OF platform bus bindings to the PATA devices.
>=20
> > +static struct of_device_id pata_of_platform_match[] =3D {
> > +=A0=A0=A0=A0=A0{ .compatible =3D "pata-platform", },
> > +};
>=20
> "pata-platform" really means nothing outside of linux. A more
> generic label would be useful.

Maybe the name of the standards it supports? Could be
"ata-4", "ata-5" and the like, or the exact transfer mode, like
"pata-udma-5", "pata-pio-3", "sata-150", etc.

	Arnd <><

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

On Tue, 27 Nov 2007 18:39:08 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
> +static int __devinit pata_of_platform_probe(struct of_device *ofdev,
> +					    const struct of_device_id *match)
> +{
> +	uint32_t *prop;

Make this "const uint32_t *prop" or (more kernel like) "const u32 *prop" ...

> +	prop = (uint32_t *)of_get_property(dn, "ioport-shift", NULL);

then you don't need this cast.  Anytime you use a cast to get rid of
"const", is probably a mistake.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Tue, Nov 27, 2007 at 10:32:58PM +0100, Arnd Bergmann wrote:
> On Tuesday 27 November 2007, Olof Johansson wrote:
> > On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:
> > > This driver nicely wraps around pata_platform library functions,
> > > and provides OF platform bus bindings to the PATA devices.
> > 
> > > +static struct of_device_id pata_of_platform_match[] = {
> > > +     { .compatible = "pata-platform", },
> > > +};
> > 
> > "pata-platform" really means nothing outside of linux. A more
> > generic label would be useful.

Agreed.

> Maybe the name of the standards it supports? Could be
> "ata-4", "ata-5" and the like, or the exact transfer mode, like
> "pata-udma-5", "pata-pio-3", "sata-150", etc.

You're quite optimistic about pata_platform capabilities. ;-)

As far as I know it is [obviously] supports PIO modes only. And so
far I was able to get max 5.28 MB/s read transfers. Which looks like
ideal case for PIO1 (CF I'm testing on is 3.0, max. PIO4).

I've modified pio_mask appropriately, plus I've tried to comment
out .set_mode = pata_platform_set_mode, and now it says:

ata5: PATA max PIO4 mmio cmd 0xf0000000 ctl 0xf000020c irq 24
ata5.00: CFA: TOSHIBA THNCF512MQG, 3.00, max PIO4
ata5.00: configured for PIO4
ata5.00: configured for PIO4

That looks good, but speed is the same. Oh well, it's another
matter.


Back to dts, I think pata-pio-X is good scheme. That way we can
pass pio_mask via device tree. Sounds reasonable?

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>>This driver nicely wraps around pata_platform library functions,
>>>>and provides OF platform bus bindings to the PATA devices.

>>>>+static struct of_device_id pata_of_platform_match[] = {
>>>>+     { .compatible = "pata-platform", },
>>>>+};

>>>"pata-platform" really means nothing outside of linux. A more
>>>generic label would be useful.

> Agreed.

    Now you're quick to agree. :-)

>>Maybe the name of the standards it supports? Could be
>>"ata-4", "ata-5" and the like, or the exact transfer mode, like
>>"pata-udma-5", "pata-pio-3", "sata-150", etc.

> You're quite optimistic about pata_platform capabilities. ;-)

    Indeed. :-)

> As far as I know it is [obviously] supports PIO modes only. And so
> far I was able to get max 5.28 MB/s read transfers. Which looks like
> ideal case for PIO1 (CF I'm testing on is 3.0, max. PIO4).

    Believe me, it's a great speed even for PIO4. Most systems only show 3+ 
MiB/s in this mode according to hdparm.

> I've modified pio_mask appropriately, plus I've tried to comment
> out .set_mode = pata_platform_set_mode, and now it says:

> ata5: PATA max PIO4 mmio cmd 0xf0000000 ctl 0xf000020c irq 24
> ata5.00: CFA: TOSHIBA THNCF512MQG, 3.00, max PIO4
> ata5.00: configured for PIO4
> ata5.00: configured for PIO4

> That looks good, but speed is the same. Oh well, it's another
> matter.

> Back to dts, I think pata-pio-X is good scheme. That way we can
> pass pio_mask via device tree. Sounds reasonable?

    Grumble. Can't we pass this via some property other than "compatible"? I'm 
opposed to "ata-5" and the like in there as well cause it's not clear what 
information this would provide. Why people so love to make things complex WRT 
the "compatible" property -- instead of making the task of selecting a proper 
driver more simple, they tend to make it mode complex by trying to specify 
values that have quite little to do with the device's programming interface 
itself...

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Hello.

Arnd Bergmann wrote:

>>>This driver nicely wraps around pata_platform library functions,
>>>and provides OF platform bus bindings to the PATA devices.

>>>+static struct of_device_id pata_of_platform_match[] = {
>>>+     { .compatible = "pata-platform", },
>>>+};

>>"pata-platform" really means nothing outside of linux. A more
>>generic label would be useful.

> Maybe the name of the standards it supports? Could be
> "ata-4", "ata-5" and the like,

    It's not clear what info this would provide.

> or the exact transfer mode, like
> "pata-udma-5", "pata-pio-3", "sata-150", etc.

    I think this info should follow from the compatible property value 
implicitly, or maybe this info should be conveyed in some optional properties. 
It doesn't make sense to the generic platform driver anyway since it has no 
notion about the mode programming specifics. I think that as the device being 
driven is assumed to be a generic IDE device, the "compatible" property should 
contain "generic" or something alike (as well as usual board's and chip's names).

> 	Arnd <><

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Wed, Nov 28, 2007 at 07:11:19PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>>This driver nicely wraps around pata_platform library functions,
> >>>>and provides OF platform bus bindings to the PATA devices.
> 
> >>>>+static struct of_device_id pata_of_platform_match[] = {
> >>>>+     { .compatible = "pata-platform", },
> >>>>+};
> 
> >>>"pata-platform" really means nothing outside of linux. A more
> >>>generic label would be useful.
> 
> > Agreed.
> 
>     Now you're quick to agree. :-)

I'm quick to change my mind either. ;-)

*BOOM*, I changed my mind.

> >>Maybe the name of the standards it supports? Could be
> >>"ata-4", "ata-5" and the like, or the exact transfer mode, like
> >>"pata-udma-5", "pata-pio-3", "sata-150", etc.
> 
> > You're quite optimistic about pata_platform capabilities. ;-)
> 
>     Indeed. :-)
> 
> > As far as I know it is [obviously] supports PIO modes only. And so
> > far I was able to get max 5.28 MB/s read transfers. Which looks like
> > ideal case for PIO1 (CF I'm testing on is 3.0, max. PIO4).
> 
>     Believe me, it's a great speed even for PIO4. Most systems only show 3+ 
> MiB/s in this mode according to hdparm.
> 
> > I've modified pio_mask appropriately, plus I've tried to comment
> > out .set_mode = pata_platform_set_mode, and now it says:
> 
> > ata5: PATA max PIO4 mmio cmd 0xf0000000 ctl 0xf000020c irq 24
> > ata5.00: CFA: TOSHIBA THNCF512MQG, 3.00, max PIO4
> > ata5.00: configured for PIO4
> > ata5.00: configured for PIO4
> 
> > That looks good, but speed is the same. Oh well, it's another
> > matter.
> 
> > Back to dts, I think pata-pio-X is good scheme. That way we can
> > pass pio_mask via device tree. Sounds reasonable?
> 
>     Grumble. Can't we pass this via some property other than "compatible"? I'm 
> opposed to "ata-5" and the like in there as well cause it's not clear what 
> information this would provide. Why people so love to make things complex WRT 
> the "compatible" property -- instead of making the task of selecting a proper 
> driver more simple, they tend to make it mode complex by trying to specify 
> values that have quite little to do with the device's programming interface 
> itself...

Ok, now I'm agree here. dts already specifying "fsl,mpc8349emitx-pata",
second compatible entry is okay to mean nothing outside Linux itself,
there are plenty of examples for such kind.

Remaining question: any preferred name for that property? pio-mode okay?
It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

> Remaining question: any preferred name for that property? pio-mode okay?
> It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

    I've already suggested "generic". A name "simple" also comes to my mind.

WBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Fri, Nov 30, 2007 at 01:17:22PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >Remaining question: any preferred name for that property? pio-mode okay?
> >It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.
> 
>    I've already suggested "generic". A name "simple" also comes to my mind.

You've misread my question. I didn't ask about driver name, but pio-mode
property.


As for the driver name, it doesn't matter at all, as I've said already:
it's Linux specific anyway, and another compatible properties could be
added at any time, to a device tree and/or to the OF driver itself (if
some real OpenFirmware will pass some meaningful compatible property
that we'll have to match in that driver).

"generic" name is also bad one, it's confusing wrt ata_generic.c
driver (PCI). "simple" name doesn't tell anything at all. So, I'd
rather stick with -platform name.

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>Remaining question: any preferred name for that property? pio-mode okay?
>>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

>>   I've already suggested "generic". A name "simple" also comes to my mind.

> You've misread my question. I didn't ask about driver name, but pio-mode
> property.

    I'm OK with "pio-mode" then. Just don't think it makes much sense in the 
context of this driver which has no provision for the programming the mode 
timings (and if there were some provision, the *generic* platform driver 
couldn't handle it anyway).

> As for the driver name, it doesn't matter at all, as I've said already:
> it's Linux specific anyway, and another compatible properties could be
> added at any time, to a device tree and/or to the OF driver itself (if
> some real OpenFirmware will pass some meaningful compatible property
> that we'll have to match in that driver).

> "generic" name is also bad one, it's confusing wrt ata_generic.c
> driver (PCI).

    The "compatible" property doesn't have to contain the driver name, so 
there should be no confusion with the driver names. It's just different name 
spaces. :-)

> "simple" name doesn't tell anything at all. So, I'd rather stick with -platform name.

    Well, those two should be "generic-ata" and "simple-ata" of course. And it 
*does* tell that the driver just provides taskfile control, without the 
transfer timing control and other fancy stuff...

> Thanks,

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>Remaining question: any preferred name for that property? pio-mode okay?
>>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

>>   I've already suggested "generic". A name "simple" also comes to my mind.

> You've misread my question. I didn't ask about driver name, but pio-mode
> property.

    Probably "max-pio-mode" is a better variant.

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Fri, Nov 30, 2007 at 02:05:01PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>Remaining question: any preferred name for that property? pio-mode okay?
> >>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.
> 
> >>  I've already suggested "generic". A name "simple" also comes to my mind.
> 
> >You've misread my question. I didn't ask about driver name, but pio-mode
> >property.
> 
>    I'm OK with "pio-mode" then. Just don't think it makes much sense in the 
> context of this driver which has no provision for the programming the mode 
> timings (and if there were some provision, the *generic* platform driver 
> couldn't handle it anyway).

What sense it makes then?.. We're specifying bus limitations wrt
PIO modes.

Oh, I think you meant that bus can't be reconfigured by generic
driver, that's true. But I didn't mean this as a purpose for
pio-mode property (though it still could be used for that
purpose by other drivers).

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Fri, Nov 30, 2007 at 02:43:50PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>Remaining question: any preferred name for that property? pio-mode okay?
> >>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.
> 
> >>  I've already suggested "generic". A name "simple" also comes to my mind.
> 
> >You've misread my question. I didn't ask about driver name, but pio-mode
> >property.
> 
>    Probably "max-pio-mode" is a better variant.

Why? pio-mode in pata-platform context is just something highly
dependent on hardware, can not be touched. And we're passing
pio-mode information from the hardware dependent source --
device tree.

The only difference between pata-platform's max-pio-mode and
dedicated ata driver's pio-mode is that that for the first case
bus already configured for specified mode, and for the second
case dedicated driver should actually use this information
to configure hardware.

No need for max- prefix, I think.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Olof Johansson]

On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:

> +static struct of_device_id pata_of_platform_match[] = {
> +	{ .compatible = "pata-platform", },
> +};

On top of previous comment about the compatible string being
inappropriate:

You should add a MODULE_DEVICE_TABLE() entry for this. Dave Woodhouse
pointed out it's needed for autoloading modules (i.e. by the fedora
installer, etc).


-Olof

[PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

This driver nicely wraps around pata_platform library functions,
and provides OF platform bus bindings to the PATA devices.

Also add || PPC to the PATA_PLATFORM's "depends on" Kconfig entry,
needed for PA Semi Electra.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Reviewed-by: Olof Johansson <olof@lixom.net>
---
 drivers/ata/Kconfig            |   12 ++++-
 drivers/ata/Makefile           |    1 +
 drivers/ata/pata_of_platform.c |  104 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ata/pata_of_platform.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ba63619..64b4964 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -607,13 +607,23 @@ config PATA_WINBOND_VLB
 
 config PATA_PLATFORM
 	tristate "Generic platform device PATA support"
-	depends on EMBEDDED || ARCH_RPC
+	depends on EMBEDDED || ARCH_RPC || PPC
 	help
 	  This option enables support for generic directly connected ATA
 	  devices commonly found on embedded systems.
 
 	  If unsure, say N.
 
+config PATA_OF_PLATFORM
+	tristate "OpenFirmware platform device PATA support"
+	depends on PATA_PLATFORM && PPC_OF
+	help
+	  This option enables support for generic directly connected ATA
+	  devices commonly found on embedded systems with OpenFirmware
+	  bindings.
+
+	  If unsure, say N.
+
 config PATA_ICSIDE
 	tristate "Acorn ICS PATA support"
 	depends on ARM && ARCH_ACORN
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b13feb2..ebcee64 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF)	+= pata_ixp4xx_cf.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_BF54X)	+= pata_bf54x.o
 obj-$(CONFIG_PATA_PLATFORM)	+= pata_platform.o
+obj-$(CONFIG_PATA_OF_PLATFORM)	+= pata_of_platform.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
 # Should be last but two libata driver
 obj-$(CONFIG_PATA_ACPI)		+= pata_acpi.o
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
new file mode 100644
index 0000000..4daf118
--- /dev/null
+++ b/drivers/ata/pata_of_platform.c
@@ -0,0 +1,104 @@
+/*
+ * OF-platform PATA driver
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ *                     Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pata_platform.h>
+
+static int __devinit pata_of_platform_probe(struct of_device *ofdev,
+					    const struct of_device_id *match)
+{
+	int ret;
+	struct device_node *dn = ofdev->node;
+	struct resource io_res;
+	struct resource ctl_res;
+	struct resource irq_res;
+	unsigned int reg_shift = 0;
+	int pio_mode = 0;
+	int pio_mask;
+	const u32 *prop;
+
+	ret = of_address_to_resource(dn, 0, &io_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get IO address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(dn, 1, &ctl_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get CTL address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_irq_to_resource(dn, 0, &irq_res);
+	if (ret == NO_IRQ)
+		irq_res.start = irq_res.end = -1;
+	else
+		irq_res.flags = 0;
+
+	prop = (u32 *)of_get_property(dn, "reg-shift", NULL);
+	if (prop)
+		reg_shift = *prop;
+
+	prop = (u32 *)of_get_property(dn, "pio-mode", NULL);
+	if (prop) {
+		pio_mode = *prop;
+		if (pio_mode > 6) {
+			dev_err(&ofdev->dev, "invalid pio-mode\n");
+			return -EINVAL;
+		}
+	} else {
+		dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n");
+	}
+
+	pio_mask = 1 << pio_mode;
+	pio_mask |= (1 << pio_mode) - 1;
+
+	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, &irq_res,
+				     reg_shift, pio_mask);
+}
+
+static int __devexit pata_of_platform_remove(struct of_device *ofdev)
+{
+	return __pata_platform_remove(&ofdev->dev);
+}
+
+static struct of_device_id pata_of_platform_match[] = {
+	{ .compatible = "ata-generic", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pata_of_platform_match);
+
+static struct of_platform_driver pata_of_platform_driver = {
+	.name		= "pata_of_platform",
+	.match_table	= pata_of_platform_match,
+	.probe		= pata_of_platform_probe,
+	.remove		= __devexit_p(pata_of_platform_remove),
+};
+
+static int __init pata_of_platform_init(void)
+{
+	return of_register_platform_driver(&pata_of_platform_driver);
+}
+module_init(pata_of_platform_init);
+
+static void __exit pata_of_platform_exit(void)
+{
+	of_unregister_platform_driver(&pata_of_platform_driver);
+}
+module_exit(pata_of_platform_exit);
+
+MODULE_DESCRIPTION("OF-platform PATA driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.2.2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On Fri, 14 Dec 2007 21:24:38 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
> +	prop = (u32 *)of_get_property(dn, "reg-shift", NULL);

This cast is unnecessary.

> +	if (prop)
> +		reg_shift = *prop;
> +
> +	prop = (u32 *)of_get_property(dn, "pio-mode", NULL);

So is this one.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Sun, Dec 16, 2007 at 01:09:22AM +1100, Stephen Rothwell wrote:
> On Fri, 14 Dec 2007 21:24:38 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> >
> > +	prop = (u32 *)of_get_property(dn, "reg-shift", NULL);
> 
> This cast is unnecessary.
> 
> > +	if (prop)
> > +		reg_shift = *prop;
> > +
> > +	prop = (u32 *)of_get_property(dn, "pio-mode", NULL);
> 
> So is this one.

Thanks. I changed "prop" to const but obviously forgot to remove casts.

- - - - 
From: Anton Vorontsov <avorontsov@ru.mvista.com>

[libata] pata_of_platform: OF-Platform PATA device driver

This driver nicely wraps around pata_platform library functions,
and provides OF platform bus bindings to the PATA devices.

Also add || PPC to the PATA_PLATFORM's "depends on" Kconfig entry,
needed for PA Semi Electra.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Reviewed-by: Olof Johansson <olof@lixom.net>
---
 drivers/ata/Kconfig            |   12 ++++-
 drivers/ata/Makefile           |    1 +
 drivers/ata/pata_of_platform.c |  104 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ata/pata_of_platform.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ba63619..64b4964 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -607,13 +607,23 @@ config PATA_WINBOND_VLB
 
 config PATA_PLATFORM
 	tristate "Generic platform device PATA support"
-	depends on EMBEDDED || ARCH_RPC
+	depends on EMBEDDED || ARCH_RPC || PPC
 	help
 	  This option enables support for generic directly connected ATA
 	  devices commonly found on embedded systems.
 
 	  If unsure, say N.
 
+config PATA_OF_PLATFORM
+	tristate "OpenFirmware platform device PATA support"
+	depends on PATA_PLATFORM && PPC_OF
+	help
+	  This option enables support for generic directly connected ATA
+	  devices commonly found on embedded systems with OpenFirmware
+	  bindings.
+
+	  If unsure, say N.
+
 config PATA_ICSIDE
 	tristate "Acorn ICS PATA support"
 	depends on ARM && ARCH_ACORN
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b13feb2..ebcee64 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF)	+= pata_ixp4xx_cf.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_BF54X)	+= pata_bf54x.o
 obj-$(CONFIG_PATA_PLATFORM)	+= pata_platform.o
+obj-$(CONFIG_PATA_OF_PLATFORM)	+= pata_of_platform.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
 # Should be last but two libata driver
 obj-$(CONFIG_PATA_ACPI)		+= pata_acpi.o
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
new file mode 100644
index 0000000..b7bc4e4
--- /dev/null
+++ b/drivers/ata/pata_of_platform.c
@@ -0,0 +1,104 @@
+/*
+ * OF-platform PATA driver
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ *                     Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pata_platform.h>
+
+static int __devinit pata_of_platform_probe(struct of_device *ofdev,
+					    const struct of_device_id *match)
+{
+	int ret;
+	struct device_node *dn = ofdev->node;
+	struct resource io_res;
+	struct resource ctl_res;
+	struct resource irq_res;
+	unsigned int reg_shift = 0;
+	int pio_mode = 0;
+	int pio_mask;
+	const u32 *prop;
+
+	ret = of_address_to_resource(dn, 0, &io_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get IO address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(dn, 1, &ctl_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get CTL address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_irq_to_resource(dn, 0, &irq_res);
+	if (ret == NO_IRQ)
+		irq_res.start = irq_res.end = -1;
+	else
+		irq_res.flags = 0;
+
+	prop = of_get_property(dn, "reg-shift", NULL);
+	if (prop)
+		reg_shift = *prop;
+
+	prop = of_get_property(dn, "pio-mode", NULL);
+	if (prop) {
+		pio_mode = *prop;
+		if (pio_mode > 6) {
+			dev_err(&ofdev->dev, "invalid pio-mode\n");
+			return -EINVAL;
+		}
+	} else {
+		dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n");
+	}
+
+	pio_mask = 1 << pio_mode;
+	pio_mask |= (1 << pio_mode) - 1;
+
+	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, &irq_res,
+				     reg_shift, pio_mask);
+}
+
+static int __devexit pata_of_platform_remove(struct of_device *ofdev)
+{
+	return __pata_platform_remove(&ofdev->dev);
+}
+
+static struct of_device_id pata_of_platform_match[] = {
+	{ .compatible = "ata-generic", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pata_of_platform_match);
+
+static struct of_platform_driver pata_of_platform_driver = {
+	.name		= "pata_of_platform",
+	.match_table	= pata_of_platform_match,
+	.probe		= pata_of_platform_probe,
+	.remove		= __devexit_p(pata_of_platform_remove),
+};
+
+static int __init pata_of_platform_init(void)
+{
+	return of_register_platform_driver(&pata_of_platform_driver);
+}
+module_init(pata_of_platform_init);
+
+static void __exit pata_of_platform_exit(void)
+{
+	of_unregister_platform_driver(&pata_of_platform_driver);
+}
+module_exit(pata_of_platform_exit);
+
+MODULE_DESCRIPTION("OF-platform PATA driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.2.2

[PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

This driver nicely wraps around pata_platform library functions,
and provides OF platform bus bindings to the PATA devices.

Also add || PPC to the PATA_PLATFORM's "depends on" Kconfig entry,
needed for PA Semi Electra.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Reviewed-by: Olof Johansson <olof@lixom.net>
---
 drivers/ata/Kconfig            |   12 ++++-
 drivers/ata/Makefile           |    1 +
 drivers/ata/pata_of_platform.c |  104 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ata/pata_of_platform.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ba63619..64b4964 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -607,13 +607,23 @@ config PATA_WINBOND_VLB
 
 config PATA_PLATFORM
 	tristate "Generic platform device PATA support"
-	depends on EMBEDDED || ARCH_RPC
+	depends on EMBEDDED || ARCH_RPC || PPC
 	help
 	  This option enables support for generic directly connected ATA
 	  devices commonly found on embedded systems.
 
 	  If unsure, say N.
 
+config PATA_OF_PLATFORM
+	tristate "OpenFirmware platform device PATA support"
+	depends on PATA_PLATFORM && PPC_OF
+	help
+	  This option enables support for generic directly connected ATA
+	  devices commonly found on embedded systems with OpenFirmware
+	  bindings.
+
+	  If unsure, say N.
+
 config PATA_ICSIDE
 	tristate "Acorn ICS PATA support"
 	depends on ARM && ARCH_ACORN
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b13feb2..ebcee64 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF)	+= pata_ixp4xx_cf.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_BF54X)	+= pata_bf54x.o
 obj-$(CONFIG_PATA_PLATFORM)	+= pata_platform.o
+obj-$(CONFIG_PATA_OF_PLATFORM)	+= pata_of_platform.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
 # Should be last but two libata driver
 obj-$(CONFIG_PATA_ACPI)		+= pata_acpi.o
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
new file mode 100644
index 0000000..b7bc4e4
--- /dev/null
+++ b/drivers/ata/pata_of_platform.c
@@ -0,0 +1,104 @@
+/*
+ * OF-platform PATA driver
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ *                     Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pata_platform.h>
+
+static int __devinit pata_of_platform_probe(struct of_device *ofdev,
+					    const struct of_device_id *match)
+{
+	int ret;
+	struct device_node *dn = ofdev->node;
+	struct resource io_res;
+	struct resource ctl_res;
+	struct resource irq_res;
+	unsigned int reg_shift = 0;
+	int pio_mode = 0;
+	int pio_mask;
+	const u32 *prop;
+
+	ret = of_address_to_resource(dn, 0, &io_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get IO address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(dn, 1, &ctl_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get CTL address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_irq_to_resource(dn, 0, &irq_res);
+	if (ret == NO_IRQ)
+		irq_res.start = irq_res.end = -1;
+	else
+		irq_res.flags = 0;
+
+	prop = of_get_property(dn, "reg-shift", NULL);
+	if (prop)
+		reg_shift = *prop;
+
+	prop = of_get_property(dn, "pio-mode", NULL);
+	if (prop) {
+		pio_mode = *prop;
+		if (pio_mode > 6) {
+			dev_err(&ofdev->dev, "invalid pio-mode\n");
+			return -EINVAL;
+		}
+	} else {
+		dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n");
+	}
+
+	pio_mask = 1 << pio_mode;
+	pio_mask |= (1 << pio_mode) - 1;
+
+	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, &irq_res,
+				     reg_shift, pio_mask);
+}
+
+static int __devexit pata_of_platform_remove(struct of_device *ofdev)
+{
+	return __pata_platform_remove(&ofdev->dev);
+}
+
+static struct of_device_id pata_of_platform_match[] = {
+	{ .compatible = "ata-generic", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pata_of_platform_match);
+
+static struct of_platform_driver pata_of_platform_driver = {
+	.name		= "pata_of_platform",
+	.match_table	= pata_of_platform_match,
+	.probe		= pata_of_platform_probe,
+	.remove		= __devexit_p(pata_of_platform_remove),
+};
+
+static int __init pata_of_platform_init(void)
+{
+	return of_register_platform_driver(&pata_of_platform_driver);
+}
+module_init(pata_of_platform_init);
+
+static void __exit pata_of_platform_exit(void)
+{
+	of_unregister_platform_driver(&pata_of_platform_driver);
+}
+module_exit(pata_of_platform_exit);
+
+MODULE_DESCRIPTION("OF-platform PATA driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.2.2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

Hi Anton,

Juts one small trivial comment (could be fixed later).

On Wed, 9 Jan 2008 22:10:41 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
> +static struct of_device_id pata_of_platform_match[] = {

This could be declared const.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Olof Johansson]

On Thu, Jan 10, 2008 at 10:40:48AM +1100, Stephen Rothwell wrote:
> Hi Anton,
> 
> Juts one small trivial comment (could be fixed later).
> 
> On Wed, 9 Jan 2008 22:10:41 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> >
> > +static struct of_device_id pata_of_platform_match[] = {
> 
> This could be declared const.

Good point, but let's not hold up merge based on this. Need something
for janitors to do too, and it's good enough to merge as-is. :)


-Olof

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

On Wed, 9 Jan 2008 18:36:34 -0600 Olof Johansson <olof@lixom.net> wrote:
>
> On Thu, Jan 10, 2008 at 10:40:48AM +1100, Stephen Rothwell wrote:
> > Hi Anton,
> > 
> > Juts one small trivial comment (could be fixed later).
> > 
> > On Wed, 9 Jan 2008 22:10:41 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > >
> > > +static struct of_device_id pata_of_platform_match[] = {
> > 
> > This could be declared const.
> 
> Good point, but let's not hold up merge based on this. Need something
> for janitors to do too, and it's good enough to merge as-is. :)

Absolutely. To me that is what "(could be fixed later)" means.  :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]