Skip to contentSkip to conversation

OF-platform PATA driver

RFC PATCH In review Anton Vorontsov 7 participants 14 messages Nov 23, 2007 – Nov 26, 2007

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover OF-platform PATA driver

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?

arch/powerpc/platforms/83xx/mpc834x_itx.c +73 -0
index 150fafb..4caa90d 100644
@@ -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,
#1 lore raw

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

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.

---
 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
drivers/ata/pata_platform.c +82 -57
index ac03a90..6436c38 100644
@@ -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,
drivers/ata/pata_platform.h new file +12 -0
new file mode 100644
index 0000000..9752a42
@@ -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 */
Signature · 1 line
1.5.2.2
#2 lore raw

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

In reply to Anton Vorontsov
---
 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
drivers/ata/Kconfig +10 -0
index ba63619..5a492fa 100644
@@ -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
drivers/ata/Makefile +1 -0
index b13feb2..ebcee64 100644
@@ -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
drivers/ata/pata_of_platform.c new file +88 -0

88 lines not on this page · raw message

Signature · 1 line
1.5.2.2
#3 lore raw

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

In reply to Anton Vorontsov
---
 arch/powerpc/boot/dts/mpc8349emitx.dts    |    9 +++++++--
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   17 +++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
arch/powerpc/boot/dts/mpc8349emitx.dts +7 -2
index 5072f6d..898c294 100644
@@ -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>;
+	};
 };
arch/powerpc/platforms/83xx/mpc834x_itx.c +17 -0
index aa76819..150fafb 100644
@@ -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
Signature · 1 line
1.5.2.2
#4 lore raw

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

Quoted 6 lines · Anton Vorontsov
arch/powerpc/platforms/83xx/mpc834x_itx.c +5 -0
+static struct of_device_id mpc834x_ids[] = {
+       { .compatible = "pata-platform", },
+       {},
+};
+

arch/powerpc/platforms/83xx/mpc834x_itx.c line 55

Quoted 10 lines
arch/powerpc/platforms/83xx/mpc834x_itx.c +10 -0
+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.

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 <><
#5 lore raw

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

Quoted 13 lines · Arnd Bergmann
Quoted 12 lines · Anton Vorontsov
arch/powerpc/platforms/83xx/mpc834x_itx.c +11 -0
+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);
Quoted 8 lines

arch/powerpc/platforms/83xx/mpc834x_itx.c line 55

Quoted 4 lines
arch/powerpc/platforms/83xx/mpc834x_itx.c +4 -0
+
+       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,

Signature · 4 lines
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
#6 lore raw
In reply to Anton Vorontsov
Quoted 20 lines · 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.

Quoted 7 lines

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

#7 lore raw
Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Anton Vorontsov

Here is the PATA Platform driver using OF infrastructure.

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

Quoted 6 lines
Quoted 6 lines

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.

#8 lore raw
Quoted 7 lines · Paul Mundt
Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Anton Vorontsov

Here is the PATA Platform driver using OF infrastructure.

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

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines

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?

Quoted 11 lines

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.

Quoted 4 lines

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,

Signature · 4 lines
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
#9 lore raw
Quoted 29 lines · Anton Vorontsov
Quoted 7 lines · Paul Mundt
Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Anton Vorontsov

Here is the PATA Platform driver using OF infrastructure.

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

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines

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:

(quoted diff) +7 -0
+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;
[...]
Quoted 3 lines

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.

Quoted 9 lines
Quoted 4 lines

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.

#10 lore raw
In reply to Anton Vorontsov
Quoted 9 lines · Anton Vorontsov

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?).

Quoted 8 lines

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.

#11 lore raw
In reply to Anton Vorontsov
Quoted 20 lines · 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.

Quoted 8 lines

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.

Signature · 1 line
Sincerely, Vitaly
#12 lore raw
In reply to Anton Vorontsov
Quoted 4 lines · Anton Vorontsov

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 <><
#13 lore raw
Quoted 6 lines · Arnd Bergmann
Quoted 4 lines · Anton Vorontsov

Here is the PATA Platform driver using OF infrastructure.

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

Quoted 6 lines

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,

Signature · 4 lines
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2