[PATCH v2 0/6] SystemACE: rework and of_platform bus binding patches

[PATCH v2 0/6] SystemACE: rework and of_platform bus binding patches

[Grant Likely]

Here is a set of rework patches on the Xilinx SystemACE driver which ends
in the addition of an of_platform bus binding.  The of_platform bus binding
is needed to use the driver from arch/powerpc.  SystemACE is most commonly
used in Xilinx Virtex system (ppc405).

Jens, I'm hoping I can get these changes in for 2.6.24.  Assuming nobody
raises any issues, can you please merge these into your tree?

Thanks,
g.

Grant Likely (6):
 Add Xilinx SystemACE entry to maintainers
 Sysace: Use the established platform bus api
 Sysace: Move structure allocation from bus binding into common code
 Sysace: minor rework and cleanup changes
 Sysace: Move IRQ handler registration to occur after FSM is initialized
 Sysace: Add of_platform_bus binding

 MAINTAINERS             |    7 ++
 drivers/block/xsysace.c |  249 +++++++++++++++++++++++++++++++++++-----------
       2 files changed, 196 insertions(+), 60 deletions(-)

--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.

[PATCH v2 1/6] Add Xilinx SystemACE entry to maintainers

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

I'm the author of the SystemACE driver

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 MAINTAINERS |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8f80068..cb6323e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4179,6 +4179,13 @@ W:	http://oss.sgi.com/projects/xfs
 T:	git git://oss.sgi.com:8090/xfs/xfs-2.6.git
 S:	Supported
 
+XILINX SYSTEMACE DRIVER
+P:	Grant Likely
+M:	grant.likely@secretlab.ca
+W:	http://www.secretlab.ca/
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+
 XILINX UARTLITE SERIAL DRIVER
 P:	Peter Korsgaard
 M:	jacmet@sunsite.dk

[PATCH v2 2/6] Sysace: Use the established platform bus api

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

SystemACE uses the platform bus binding, but it doesn't use the
platform bus API.  Move to using the correct API for consistency
sake and future proofing against platform bus changes.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/block/xsysace.c |   48 +++++++++++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 3ede0b6..b104476 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -1060,13 +1060,12 @@ static void __devexit ace_teardown(struct ace_device *ace)
  * Platform Bus Support
  */
 
-static int __devinit ace_probe(struct device *device)
+static int __devinit ace_probe(struct platform_device *dev)
 {
-	struct platform_device *dev = to_platform_device(device);
 	struct ace_device *ace;
 	int i;
 
-	dev_dbg(device, "ace_probe(%p)\n", device);
+	dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
 
 	/*
 	 * Allocate the ace device structure
@@ -1075,7 +1074,7 @@ static int __devinit ace_probe(struct device *device)
 	if (!ace)
 		goto err_alloc;
 
-	ace->dev = device;
+	ace->dev = &dev->dev;
 	ace->id = dev->id;
 	ace->irq = NO_IRQ;
 
@@ -1089,7 +1088,7 @@ static int __devinit ace_probe(struct device *device)
 	/* FIXME: Should get bus_width from the platform_device struct */
 	ace->bus_width = 1;
 
-	dev_set_drvdata(&dev->dev, ace);
+	platform_set_drvdata(dev, ace);
 
 	/* Call the bus-independant setup code */
 	if (ace_setup(ace) != 0)
@@ -1098,7 +1097,7 @@ static int __devinit ace_probe(struct device *device)
 	return 0;
 
       err_setup:
-	dev_set_drvdata(&dev->dev, NULL);
+	platform_set_drvdata(dev, NULL);
 	kfree(ace);
       err_alloc:
 	printk(KERN_ERR "xsysace: could not initialize device\n");
@@ -1108,25 +1107,27 @@ static int __devinit ace_probe(struct device *device)
 /*
  * Platform bus remove() method
  */
-static int __devexit ace_remove(struct device *device)
+static int __devexit ace_remove(struct platform_device *dev)
 {
-	struct ace_device *ace = dev_get_drvdata(device);
-
-	dev_dbg(device, "ace_remove(%p)\n", device);
+	struct ace_device *ace =  platform_get_drvdata(dev);
+	dev_dbg(&dev->dev, "ace_remove(%p)\n", dev);
 
 	if (ace) {
 		ace_teardown(ace);
+		platform_set_drvdata(dev, NULL);
 		kfree(ace);
 	}
 
 	return 0;
 }
 
-static struct device_driver ace_driver = {
-	.name = "xsysace",
-	.bus = &platform_bus_type,
+static struct platform_driver ace_platform_driver = {
 	.probe = ace_probe,
 	.remove = __devexit_p(ace_remove),
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "xsysace",
+	},
 };
 
 /* ---------------------------------------------------------------------
@@ -1134,20 +1135,31 @@ static struct device_driver ace_driver = {
  */
 static int __init ace_init(void)
 {
+	int rc;
+
 	ace_major = register_blkdev(ace_major, "xsysace");
 	if (ace_major <= 0) {
-		printk(KERN_WARNING "xsysace: register_blkdev() failed\n");
-		return ace_major;
+		rc = -ENOMEM;
+		goto err_blk;
 	}
 
-	pr_debug("Registering Xilinx SystemACE driver, major=%i\n", ace_major);
-	return driver_register(&ace_driver);
+	if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
+		goto err_plat;
+
+	pr_info("Xilinx SystemACE device driver, major=%i\n", ace_major);
+	return 0;
+
+      err_plat:
+	unregister_blkdev(ace_major, "xsysace");
+      err_blk:
+	printk(KERN_ERR "xsysace: registration failed; err=%i\n", rc);
+	return rc;
 }
 
 static void __exit ace_exit(void)
 {
 	pr_debug("Unregistering Xilinx SystemACE driver\n");
-	driver_unregister(&ace_driver);
+	platform_driver_unregister(&ace_platform_driver);
 	unregister_blkdev(ace_major, "xsysace");
 }
 

[PATCH v2 3/6] Sysace: Move structure allocation from bus binding into common code

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

Split the determination of device registers/irqs/etc from the actual
allocation and initialization of the device structure.  This cleans
up the code a bit in preparation to add an of_platform bus binding

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/block/xsysace.c |   99 +++++++++++++++++++++++++++++------------------
 1 files changed, 61 insertions(+), 38 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index b104476..555939b 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -1033,7 +1033,7 @@ static int __devinit ace_setup(struct ace_device *ace)
 	if (ace->irq != NO_IRQ)
 		free_irq(ace->irq, ace);
       err_ioremap:
-	printk(KERN_INFO "xsysace: error initializing device at 0x%lx\n",
+	dev_info(ace->dev, "xsysace: error initializing device at 0x%lx\n",
 	       ace->physaddr);
 	return -ENOMEM;
 }
@@ -1056,68 +1056,91 @@ static void __devexit ace_teardown(struct ace_device *ace)
 	iounmap(ace->baseaddr);
 }
 
-/* ---------------------------------------------------------------------
- * Platform Bus Support
- */
-
-static int __devinit ace_probe(struct platform_device *dev)
+static int __devinit
+ace_alloc(struct device *dev, int id, unsigned long physaddr,
+	  int irq, int bus_width)
 {
 	struct ace_device *ace;
-	int i;
+	int rc;
+	dev_dbg(dev, "ace_alloc(%p)\n", dev);
 
-	dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
+	if (!physaddr) {
+		rc = -ENODEV;
+		goto err_noreg;
+	}
 
-	/*
-	 * Allocate the ace device structure
-	 */
+	/* Allocate and initialize the ace device structure */
 	ace = kzalloc(sizeof(struct ace_device), GFP_KERNEL);
-	if (!ace)
+	if (!ace) {
+		rc = -ENOMEM;
 		goto err_alloc;
-
-	ace->dev = &dev->dev;
-	ace->id = dev->id;
-	ace->irq = NO_IRQ;
-
-	for (i = 0; i < dev->num_resources; i++) {
-		if (dev->resource[i].flags & IORESOURCE_MEM)
-			ace->physaddr = dev->resource[i].start;
-		if (dev->resource[i].flags & IORESOURCE_IRQ)
-			ace->irq = dev->resource[i].start;
 	}
 
-	/* FIXME: Should get bus_width from the platform_device struct */
-	ace->bus_width = 1;
-
-	platform_set_drvdata(dev, ace);
+	ace->dev = dev;
+	ace->id = id;
+	ace->physaddr = physaddr;
+	ace->irq = irq;
+	ace->bus_width = bus_width;
 
-	/* Call the bus-independant setup code */
-	if (ace_setup(ace) != 0)
+	/* Call the setup code */
+	if ((rc = ace_setup(ace)) != 0)
 		goto err_setup;
 
+	dev_set_drvdata(dev, ace);
 	return 0;
 
       err_setup:
-	platform_set_drvdata(dev, NULL);
+	dev_set_drvdata(dev, NULL);
 	kfree(ace);
       err_alloc:
-	printk(KERN_ERR "xsysace: could not initialize device\n");
-	return -ENOMEM;
+      err_noreg:
+	dev_err(dev, "could not initialize device, err=%i\n", rc);
+	return rc;
 }
 
-/*
- * Platform bus remove() method
- */
-static int __devexit ace_remove(struct platform_device *dev)
+static void __devexit ace_free(struct device *dev)
 {
-	struct ace_device *ace =  platform_get_drvdata(dev);
-	dev_dbg(&dev->dev, "ace_remove(%p)\n", dev);
+	struct ace_device *ace = dev_get_drvdata(dev);
+	dev_dbg(dev, "ace_free(%p)\n", dev);
 
 	if (ace) {
 		ace_teardown(ace);
-		platform_set_drvdata(dev, NULL);
+		dev_set_drvdata(dev, NULL);
 		kfree(ace);
 	}
+}
+
+/* ---------------------------------------------------------------------
+ * Platform Bus Support
+ */
+
+static int __devinit ace_probe(struct platform_device *dev)
+{
+	unsigned long physaddr = 0;
+	int bus_width = 1; /* FIXME: should not be hard coded */
+	int id = dev->id;
+	int irq = NO_IRQ;
+	int i;
+
+	dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
+
+	for (i = 0; i < dev->num_resources; i++) {
+		if (dev->resource[i].flags & IORESOURCE_MEM)
+			physaddr = dev->resource[i].start;
+		if (dev->resource[i].flags & IORESOURCE_IRQ)
+			irq = dev->resource[i].start;
+	}
+
+	/* Call the bus-independant setup code */
+	return ace_alloc(&dev->dev, id, physaddr, irq, bus_width);
+}
 
+/*
+ * Platform bus remove() method
+ */
+static int __devexit ace_remove(struct platform_device *dev)
+{
+	ace_free(&dev->dev);
 	return 0;
 }
 

[PATCH v2 4/6] Sysace: minor rework and cleanup changes

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

Miscellanious rework to the sysace driver; Not critical, but makes the
subsequent addition of the of_platform bus binding a wee bit cleaner

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/block/xsysace.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 555939b..10bb4e5 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -158,6 +158,9 @@ MODULE_LICENSE("GPL");
 #define ACE_FIFO_SIZE (32)
 #define ACE_BUF_PER_SECTOR (ACE_SECTOR_SIZE / ACE_FIFO_SIZE)
 
+#define ACE_BUS_WIDTH_8  0
+#define ACE_BUS_WIDTH_16 1
+
 struct ace_reg_ops;
 
 struct ace_device {
@@ -931,9 +934,11 @@ static int __devinit ace_setup(struct ace_device *ace)
 {
 	u16 version;
 	u16 val;
-
 	int rc;
 
+	dev_dbg(ace->dev, "ace_setup(ace=0x%p)\n", ace);
+	dev_dbg(ace->dev, "physaddr=0x%lx irq=%i\n", ace->physaddr, ace->irq);
+
 	spin_lock_init(&ace->lock);
 	init_completion(&ace->id_completion);
 
@@ -982,7 +987,7 @@ static int __devinit ace_setup(struct ace_device *ace)
 	snprintf(ace->gd->disk_name, 32, "xs%c", ace->id + 'a');
 
 	/* set bus width */
-	if (ace->bus_width == 1) {
+	if (ace->bus_width == ACE_BUS_WIDTH_16) {
 		/* 0x0101 should work regardless of endianess */
 		ace_out_le16(ace, ACE_BUSMODE, 0x0101);
 
@@ -1117,7 +1122,7 @@ static void __devexit ace_free(struct device *dev)
 static int __devinit ace_probe(struct platform_device *dev)
 {
 	unsigned long physaddr = 0;
-	int bus_width = 1; /* FIXME: should not be hard coded */
+	int bus_width = ACE_BUS_WIDTH_16; /* FIXME: should not be hard coded */
 	int id = dev->id;
 	int irq = NO_IRQ;
 	int i;
@@ -1166,6 +1171,7 @@ static int __init ace_init(void)
 		goto err_blk;
 	}
 
+	pr_debug("xsysace: registering platform binding\n");
 	if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
 		goto err_plat;
 

[PATCH v2 5/6] Sysace: Move IRQ handler registration to occur after FSM is initialized

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

The FSM needs to be initialized before it is safe to call the ISR

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/block/xsysace.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 10bb4e5..296d567 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -949,15 +949,6 @@ static int __devinit ace_setup(struct ace_device *ace)
 	if (!ace->baseaddr)
 		goto err_ioremap;
 
-	if (ace->irq != NO_IRQ) {
-		rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
-		if (rc) {
-			/* Failure - fall back to polled mode */
-			dev_err(ace->dev, "request_irq failed\n");
-			ace->irq = NO_IRQ;
-		}
-	}
-
 	/*
 	 * Initialize the state machine tasklet and stall timer
 	 */
@@ -1015,6 +1006,16 @@ static int __devinit ace_setup(struct ace_device *ace)
 	val |= ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ;
 	ace_out(ace, ACE_CTRL, val);
 
+	/* Now we can hook up the irq handler */
+	if (ace->irq != NO_IRQ) {
+		rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
+		if (rc) {
+			/* Failure - fall back to polled mode */
+			dev_err(ace->dev, "request_irq failed\n");
+			ace->irq = NO_IRQ;
+		}
+	}
+
 	/* Print the identification */
 	dev_info(ace->dev, "Xilinx SystemACE revision %i.%i.%i\n",
 		 (version >> 12) & 0xf, (version >> 8) & 0x0f, version & 0xff);
@@ -1035,8 +1036,6 @@ static int __devinit ace_setup(struct ace_device *ace)
 	blk_cleanup_queue(ace->queue);
       err_blk_initq:
 	iounmap(ace->baseaddr);
-	if (ace->irq != NO_IRQ)
-		free_irq(ace->irq, ace);
       err_ioremap:
 	dev_info(ace->dev, "xsysace: error initializing device at 0x%lx\n",
 	       ace->physaddr);

[PATCH v2 6/6] Sysace: Add of_platform_bus binding

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

The of_platform bus binding is needed to make the device driver usable
under arch/powerpc.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/block/xsysace.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 296d567..56c4af3 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -91,6 +91,10 @@
 #include <linux/blkdev.h>
 #include <linux/hdreg.h>
 #include <linux/platform_device.h>
+#if defined(CONFIG_OF)
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#endif
 
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_DESCRIPTION("Xilinx SystemACE device driver");
@@ -1158,6 +1162,85 @@ static struct platform_driver ace_platform_driver = {
 };
 
 /* ---------------------------------------------------------------------
+ * OF_Platform Bus Support
+ */
+
+#if defined(CONFIG_OF)
+static int __devinit
+ace_of_probe(struct of_device *op, const struct of_device_id *match)
+{
+	struct resource res;
+	unsigned long physaddr;
+	const u32 *id;
+	int irq, bus_width, rc;
+
+	dev_dbg(&op->dev, "ace_of_probe(%p, %p)\n", op, match);
+
+	/* device id */
+	id = of_get_property(op->node, "port-number", NULL);
+
+	/* physaddr */
+	rc = of_address_to_resource(op->node, 0, &res);
+	if (rc) {
+		dev_err(&op->dev, "invalid address\n");
+		return rc;
+	}
+	physaddr = res.start;
+
+	/* irq */
+	irq = irq_of_parse_and_map(op->node, 0);
+
+	/* bus width */
+	bus_width = ACE_BUS_WIDTH_16;
+	if (of_find_property(op->node, "8-bit", NULL))
+		bus_width = ACE_BUS_WIDTH_8;
+
+	/* Call the bus-independant setup code */
+	return ace_alloc(&op->dev, id ? *id : 0, physaddr, irq, bus_width);
+}
+
+static int __devexit ace_of_remove(struct of_device *op)
+{
+	ace_free(&op->dev);
+	return 0;
+}
+
+/* Match table for of_platform binding */
+static struct of_device_id __devinit ace_of_match[] = {
+	{ .compatible = "xilinx,xsysace", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ace_of_match);
+
+static struct of_platform_driver ace_of_driver = {
+	.owner = THIS_MODULE,
+	.name = "xsysace",
+	.match_table = ace_of_match,
+	.probe = ace_of_probe,
+	.remove = __devexit_p(ace_of_remove),
+	.driver = {
+		.name = "xsysace",
+	},
+};
+
+/* Registration helpers to keep the number of #ifdefs to a minimum */
+static inline int __init ace_of_register(void)
+{
+	pr_debug("xsysace: registering OF binding\n");
+	return of_register_platform_driver(&ace_of_driver);
+}
+
+static inline void __exit ace_of_unregister(void)
+{
+	of_unregister_platform_driver(&ace_of_driver);
+}
+#else /* CONFIG_OF */
+/* CONFIG_OF not enabled; do nothing helpers */
+static inline int __init ace_of_register(void) { return 0; }
+static inline void __exit ace_of_unregister(void) { }
+#endif /* CONFIG_OF */
+
+/* ---------------------------------------------------------------------
  * Module init/exit routines
  */
 static int __init ace_init(void)
@@ -1170,6 +1253,9 @@ static int __init ace_init(void)
 		goto err_blk;
 	}
 
+	if ((rc = ace_of_register()) != 0)
+		goto err_of;
+
 	pr_debug("xsysace: registering platform binding\n");
 	if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
 		goto err_plat;
@@ -1178,6 +1264,8 @@ static int __init ace_init(void)
 	return 0;
 
       err_plat:
+	ace_of_unregister();
+      err_of:
 	unregister_blkdev(ace_major, "xsysace");
       err_blk:
 	printk(KERN_ERR "xsysace: registration failed; err=%i\n", rc);
@@ -1188,6 +1276,7 @@ static void __exit ace_exit(void)
 {
 	pr_debug("Unregistering Xilinx SystemACE driver\n");
 	platform_driver_unregister(&ace_platform_driver);
+	ace_of_unregister();
 	unregister_blkdev(ace_major, "xsysace");
 }
 

Re: [PATCH v2 2/6] Sysace: Use the established platform bus api

[Christoph Hellwig]

On Sun, Sep 30, 2007 at 04:57:09PM -0600, Grant Likely wrote:
> +	if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
> +		goto err_plat;

	rc = platform_driver_register(&ace_platform_driver);
	if (rc)
		 goto err_plat;

please.

> +      err_plat:
> +	unregister_blkdev(ace_major, "xsysace");
> +      err_blk:

labels should be indented zero or one space, but not more.

Re: [PATCH v2 2/6] Sysace: Use the established platform bus api

[Grant Likely]

On 9/30/07, Christoph Hellwig <hch@infradead.org> wrote:
> On Sun, Sep 30, 2007 at 04:57:09PM -0600, Grant Likely wrote:
> > +     if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
> > +             goto err_plat;
>
>         rc = platform_driver_register(&ace_platform_driver);
>         if (rc)
>                  goto err_plat;
>
> please.

Okay, will do.

>
> > +      err_plat:
> > +     unregister_blkdev(ace_major, "xsysace");
> > +      err_blk:
>
> labels should be indented zero or one space, but not more.

scripts/Lindent does this.  Originally, I *didn't* have my labels
indented.  :-)   Does Lindent need to be fixed?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH v2 2/6] Sysace: Use the established platform bus api

[Jens Axboe]

On Sun, Sep 30 2007, Grant Likely wrote:
> On 9/30/07, Christoph Hellwig <hch@infradead.org> wrote:
> > On Sun, Sep 30, 2007 at 04:57:09PM -0600, Grant Likely wrote:
> > > +     if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
> > > +             goto err_plat;
> >
> >         rc = platform_driver_register(&ace_platform_driver);
> >         if (rc)
> >                  goto err_plat;
> >
> > please.
> 
> Okay, will do.
> 
> >
> > > +      err_plat:
> > > +     unregister_blkdev(ace_major, "xsysace");
> > > +      err_blk:
> >
> > labels should be indented zero or one space, but not more.
> 
> scripts/Lindent does this.  Originally, I *didn't* have my labels
> indented.  :-)   Does Lindent need to be fixed?

Seems so, if it idents labels.

Just send a fixup patch for that, I'll add your series to the block tree
for 2.6.24.

-- 
Jens Axboe

Re: [PATCH v2 2/6] Sysace: Use the established platform bus api

[Grant Likely]

On 10/1/07, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Sun, Sep 30 2007, Grant Likely wrote:
> > On 9/30/07, Christoph Hellwig <hch@infradead.org> wrote:
> > > On Sun, Sep 30, 2007 at 04:57:09PM -0600, Grant Likely wrote:
> > > > +     if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
> > > > +             goto err_plat;
> > >
> > >         rc = platform_driver_register(&ace_platform_driver);
> > >         if (rc)
> > >                  goto err_plat;
> > >
> > > please.
> >
> > Okay, will do.
> >
> > >
> > > > +      err_plat:
> > > > +     unregister_blkdev(ace_major, "xsysace");
> > > > +      err_blk:
> > >
> > > labels should be indented zero or one space, but not more.
> >
> > scripts/Lindent does this.  Originally, I *didn't* have my labels
> > indented.  :-)   Does Lindent need to be fixed?
>
> Seems so, if it idents labels.
>
> Just send a fixup patch for that, I'll add your series to the block tree
> for 2.6.24.

Cool, thanks Jens.  I'll generate a patch to unindent the labels this afternoon.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH v2 5/6] Sysace: Move IRQ handler registration to occur after FSM is initialized

[Benjamin Herrenschmidt]

On Sun, 2007-09-30 at 16:57 -0600, Grant Likely wrote:
>         val |= ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ;
>         ace_out(ace, ACE_CTRL, val);
>  
> +       /* Now we can hook up the irq handler */
> +       if (ace->irq != NO_IRQ) {
> +               rc = request_irq(ace->irq, ace_interrupt, 0,
> "systemace", ace);
> +               if (rc) {
> +                       /* Failure - fall back to polled mode */
> +                       dev_err(ace->dev, "request_irq failed\n");
> +                       ace->irq = NO_IRQ;
> +               }
> +       }
> +

I don't know the HW but from the above, it looks like you enable
interrupt emission on the HW before you register the handler, which is
wrong. You should make sure on the contrary that IRQs on the HW are
disabled until after you have registered a handler.

Only really a problem if you have shared interrupts but still...

Ben.

Re: [PATCH v2 2/6] Sysace: Use the established platform bus api

[Benjamin Herrenschmidt]

On Mon, 2007-10-01 at 13:59 +0200, Jens Axboe wrote:
> On Sun, Sep 30 2007, Grant Likely wrote:
> > On 9/30/07, Christoph Hellwig <hch@infradead.org> wrote:
> > > On Sun, Sep 30, 2007 at 04:57:09PM -0600, Grant Likely wrote:
> > > > +     if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
> > > > +             goto err_plat;
> > >
> > >         rc = platform_driver_register(&ace_platform_driver);
> > >         if (rc)
> > >                  goto err_plat;
> > >
> > > please.
> > 
> > Okay, will do.
> > 
> > >
> > > > +      err_plat:
> > > > +     unregister_blkdev(ace_major, "xsysace");
> > > > +      err_blk:
> > >
> > > labels should be indented zero or one space, but not more.
> > 
> > scripts/Lindent does this.  Originally, I *didn't* have my labels
> > indented.  :-)   Does Lindent need to be fixed?
> 
> Seems so, if it idents labels.
> 
> Just send a fixup patch for that, I'll add your series to the block tree
> for 2.6.24.

It's actually better off living in the powerpc tree I think as it's
really about adding support for a new powerpc platform and somewhat
needs to sync with other things in there. Unless you really want the
whole thing in your tree :-)

Cheers
Ben.

Re: [PATCH v2 2/6] Sysace: Use the established platform bus api

[Jens Axboe]

On Tue, Oct 02 2007, Benjamin Herrenschmidt wrote:
> 
> On Mon, 2007-10-01 at 13:59 +0200, Jens Axboe wrote:
> > On Sun, Sep 30 2007, Grant Likely wrote:
> > > On 9/30/07, Christoph Hellwig <hch@infradead.org> wrote:
> > > > On Sun, Sep 30, 2007 at 04:57:09PM -0600, Grant Likely wrote:
> > > > > +     if ((rc = platform_driver_register(&ace_platform_driver)) != 0)
> > > > > +             goto err_plat;
> > > >
> > > >         rc = platform_driver_register(&ace_platform_driver);
> > > >         if (rc)
> > > >                  goto err_plat;
> > > >
> > > > please.
> > > 
> > > Okay, will do.
> > > 
> > > >
> > > > > +      err_plat:
> > > > > +     unregister_blkdev(ace_major, "xsysace");
> > > > > +      err_blk:
> > > >
> > > > labels should be indented zero or one space, but not more.
> > > 
> > > scripts/Lindent does this.  Originally, I *didn't* have my labels
> > > indented.  :-)   Does Lindent need to be fixed?
> > 
> > Seems so, if it idents labels.
> > 
> > Just send a fixup patch for that, I'll add your series to the block tree
> > for 2.6.24.
> 
> It's actually better off living in the powerpc tree I think as it's
> really about adding support for a new powerpc platform and somewhat
> needs to sync with other things in there. Unless you really want the
> whole thing in your tree :-)

I already included it yesterday, it'll go up once 2.6.24 opens. Let me
know if you want me to rip it out, though.

-- 
Jens Axboe

Re: [PATCH v2 2/6] Sysace: Use the established platform bus api

[Grant Likely]

On 10/2/07, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Tue, Oct 02 2007, Benjamin Herrenschmidt wrote:
> >
> > On Mon, 2007-10-01 at 13:59 +0200, Jens Axboe wrote:
> > > Just send a fixup patch for that, I'll add your series to the block tree
> > > for 2.6.24.
> >
> > It's actually better off living in the powerpc tree I think as it's
> > really about adding support for a new powerpc platform and somewhat
> > needs to sync with other things in there. Unless you really want the
> > whole thing in your tree :-)
>
> I already included it yesterday, it'll go up once 2.6.24 opens. Let me
> know if you want me to rip it out, though.

No, it's safe.  Nothing will break if one goes in before the other.
Please keep it in.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH v2 5/6] Sysace: Move IRQ handler registration to occur after FSM is initialized

[Grant Likely]

On 10/1/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Sun, 2007-09-30 at 16:57 -0600, Grant Likely wrote:
> >         val |= ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ;
> >         ace_out(ace, ACE_CTRL, val);
> >
> > +       /* Now we can hook up the irq handler */
> > +       if (ace->irq != NO_IRQ) {
> > +               rc = request_irq(ace->irq, ace_interrupt, 0,
> > "systemace", ace);
> > +               if (rc) {
> > +                       /* Failure - fall back to polled mode */
> > +                       dev_err(ace->dev, "request_irq failed\n");
> > +                       ace->irq = NO_IRQ;
> > +               }
> > +       }
> > +
>
> I don't know the HW but from the above, it looks like you enable
> interrupt emission on the HW before you register the handler, which is
> wrong. You should make sure on the contrary that IRQs on the HW are
> disabled until after you have registered a handler.
>
> Only really a problem if you have shared interrupts but still...

Yeah, you're right.  Fortunately all current in-tree platforms which
use this do not have shared interrupts, but I'd like to be correct on
this.

I'll tidy this up and send a fixup patch.

Thanks,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195