[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.