| Message ID | 1229007937-5501-1-git-send-email-w.sang@pengutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Dec 11, 2008 at 04:05:37PM +0100, Wolfram Sang wrote: > Make the generic uio-driver also accessible for of devices. It utilizes the > standard 'reg' and 'interrupt' properties. A typical usage would look like > this: > > fpga-io@00003000 { > compatible = "generic-uio"; > reg = <0x00003000 0x20>; > interrupts = <0xa>; > interrupt-parent = <&fpga_irq_mux>; > }; > > To achieve this, the probe function has been refactored, so it can be used by > platform and of code. Then, the of driver has been added. > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > --- > drivers/uio/uio_pdrv_genirq.c | 178 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 154 insertions(+), 24 deletions(-) > > Index: playground/drivers/uio/uio_pdrv_genirq.c > =================================================================== > --- playground.orig/drivers/uio/uio_pdrv_genirq.c > +++ playground/drivers/uio/uio_pdrv_genirq.c > @@ -1,13 +1,15 @@ > /* > * drivers/uio/uio_pdrv_genirq.c > * > - * Userspace I/O platform driver with generic IRQ handling code. > + * Userspace I/O platform & of driver with generic IRQ handling code. > * > * Copyright (C) 2008 Magnus Damm > + * Copyright (C) 2008 Wolfram Sang, Pengutronix e.K. > * > * Based on uio_pdrv.c by Uwe Kleine-Koenig, > * Copyright (C) 2008 by Digi International Inc. > * All rights reserved. > + * Adding of_platform_driver based on xilinxfb.c by Grant Likely. > * > * 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 > @@ -20,6 +22,10 @@ > #include <linux/bitops.h> > #include <linux/interrupt.h> > #include <linux/stringify.h> > +#if defined(CONFIG_OF) > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#endif You should never need to test a config variable in order to know to include a header file or not. > +/* --------------------------------------------------------------------- > + * OF bus binding > + */ > + > +#if defined(CONFIG_OF) Same goes here, don't put #if in .c files please. thanks, greg k-h
On Thu, Dec 11, 2008 at 04:05:37PM +0100, Wolfram Sang wrote: > Make the generic uio-driver also accessible for of devices. It utilizes the > standard 'reg' and 'interrupt' properties. A typical usage would look like > this: > > fpga-io@00003000 { > compatible = "generic-uio"; > reg = <0x00003000 0x20>; > interrupts = <0xa>; > interrupt-parent = <&fpga_irq_mux>; > }; > > To achieve this, the probe function has been refactored, so it can be used by > platform and of code. Then, the of driver has been added. > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> It is pretty poor form to not even bother to Cc the only author of the code you are modifying, and have no Signed-off-by or Acked-by to even suggest that it was ever even looked at. This isn't something that ought to have to be pointed out, either. In addition to the stuff pointed out by Greg, I don't see what you actually gain by hacking the OF crap in to this driver. You would be better off layering the OF driver on top of this, rather than trying to make them live together in the same file. See pata_platform/pata_of_platform for an example of how to do this a bit more sanely.
> > +/* --------------------------------------------------------------------- > > + * OF bus binding > > + */ > > + > > +#if defined(CONFIG_OF) > > Same goes here, don't put #if in .c files please. So, generally speaking, this means that I should not put a platform_driver and an of_platform_driver into one source file, but rather create an of_$DRIVER.c then? I found both ways used in the kernel and could not find hints about the preferred way to do it. I took this approach as I hoped it would save some code to directly convert the of_node to uioinfo without the detour of using a platform_device inbetween. Kind regards, Wolfram
> It is pretty poor form to not even bother to Cc the only author of the > code you are modifying, and have no Signed-off-by or Acked-by to even > suggest that it was ever even looked at. This isn't something that ought > to have to be pointed out, either. Oops, yes, forgot this in the resend, I am sorry. I did CC Magnus in the first round, though, and he replied to me that he liked adding OF and just waited for Hans' reply first. > In addition to the stuff pointed out by Greg, I don't see what you > actually gain by hacking the OF crap in to this driver. You would be > better off layering the OF driver on top of this, rather than trying to > make them live together in the same file. > > See pata_platform/pata_of_platform for an example of how to do this a bit > more sanely. See my reply just posted. I found two ways to go, and from reading discussions on the lists, finally decided for this way. May have been the wrong path, but nothing that could not be changed. Kind regards, Wolfram
On Tue, Dec 16, 2008 at 01:41:56PM +0100, Wolfram Sang wrote: > > In addition to the stuff pointed out by Greg, I don't see what you > > actually gain by hacking the OF crap in to this driver. You would be > > better off layering the OF driver on top of this, rather than trying to > > make them live together in the same file. > > > > See pata_platform/pata_of_platform for an example of how to do this a bit > > more sanely. > > See my reply just posted. I found two ways to go, and from reading > discussions on the lists, finally decided for this way. May have been > the wrong path, but nothing that could not be changed. > I guess there are a couple of things to consider. If it fits in fairly nicely and can be optimized out for the users that don't care, then integration generally makes sense. In this case however you have a large and insular block that is ifdef'ed in and selected by Kconfig, suggesting that the only benefit is for your driver which wishes to tie in to parts of uio_pdrv_genirq, with there being no benefits the other way. This sort of situation suggests you are better off layering and simply exposing the functionality you need from uio_pdrv_genirq. This was certainly the case with pata_platform as well, and it worked out pretty well there compared to hacking it in-place.
On Tue, Dec 16, 2008 at 01:27:32PM +0100, Wolfram Sang wrote: > > > +/* --------------------------------------------------------------------- > > > + * OF bus binding > > > + */ > > > + > > > +#if defined(CONFIG_OF) > > > > Same goes here, don't put #if in .c files please. > > So, generally speaking, this means that I should not put a > platform_driver and an of_platform_driver into one source file, but > rather create an of_$DRIVER.c then? Yes, this is the preferred way to do it. thanks, greg k-h
On Tue, Dec 16, 2008 at 9:41 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > >> It is pretty poor form to not even bother to Cc the only author of the >> code you are modifying, and have no Signed-off-by or Acked-by to even >> suggest that it was ever even looked at. This isn't something that ought >> to have to be pointed out, either. > > Oops, yes, forgot this in the resend, I am sorry. I did CC Magnus in the > first round, though, and he replied to me that he liked adding OF and > just waited for Hans' reply first. Right, that's exactly what happened. Last time when the platform drivers got merged was very very very intense so I expected a similar situation. Please excuse the hands-off handling. The result of our discussions last time was basically that we should have two reusable uio platform drivers; the regular uio_pdrv and uio_pdrv_genirq. The good thing with this is that we clearly show the difference between the two drivers, most people should just use the regular uio_pdrv driver and crazy people with unique interrupt sources should use the genirq version. The downside with the two drivers is the duplicated code, but I guess that's ok for now. >> In addition to the stuff pointed out by Greg, I don't see what you >> actually gain by hacking the OF crap in to this driver. You would be >> better off layering the OF driver on top of this, rather than trying to >> make them live together in the same file. I totally agree. > See my reply just posted. I found two ways to go, and from reading > discussions on the lists, finally decided for this way. May have been > the wrong path, but nothing that could not be changed. In my mind, the best way forward is to write a new uio driver that interfaces to open firmware. After that we can break out things like resource handling and maybe generic interrupt code. Please CC me and I'll help out. Cheers, / magnus
Index: playground/drivers/uio/uio_pdrv_genirq.c =================================================================== --- playground.orig/drivers/uio/uio_pdrv_genirq.c +++ playground/drivers/uio/uio_pdrv_genirq.c @@ -1,13 +1,15 @@ /* * drivers/uio/uio_pdrv_genirq.c * - * Userspace I/O platform driver with generic IRQ handling code. + * Userspace I/O platform & of driver with generic IRQ handling code. * * Copyright (C) 2008 Magnus Damm + * Copyright (C) 2008 Wolfram Sang, Pengutronix e.K. * * Based on uio_pdrv.c by Uwe Kleine-Koenig, * Copyright (C) 2008 by Digi International Inc. * All rights reserved. + * Adding of_platform_driver based on xilinxfb.c by Grant Likely. * * 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 @@ -20,6 +22,10 @@ #include <linux/bitops.h> #include <linux/interrupt.h> #include <linux/stringify.h> +#if defined(CONFIG_OF) +#include <linux/of_device.h> +#include <linux/of_platform.h> +#endif #define DRIVER_NAME "uio_pdrv_genirq" @@ -68,28 +74,18 @@ return 0; } -static int uio_pdrv_genirq_probe(struct platform_device *pdev) +static int uio_pdrv_genirq_setup(struct device *dev, struct uio_info *uioinfo, + struct resource *resources, unsigned int num_resources) { - struct uio_info *uioinfo = pdev->dev.platform_data; struct uio_pdrv_genirq_platdata *priv; struct uio_mem *uiomem; - int ret = -EINVAL; - int i; - - if (!uioinfo || !uioinfo->name || !uioinfo->version) { - dev_err(&pdev->dev, "missing platform_data\n"); - goto bad0; - } - - if (uioinfo->handler || uioinfo->irqcontrol || uioinfo->irq_flags) { - dev_err(&pdev->dev, "interrupt configuration error\n"); - goto bad0; - } + unsigned int i; + int ret; priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) { ret = -ENOMEM; - dev_err(&pdev->dev, "unable to kmalloc\n"); + dev_err(dev, "unable to kmalloc\n"); goto bad0; } @@ -99,14 +95,15 @@ uiomem = &uioinfo->mem[0]; - for (i = 0; i < pdev->num_resources; ++i) { - struct resource *r = &pdev->resource[i]; + for (i = 0; i < num_resources; ++i) { + + struct resource *r = resources + i; if (r->flags != IORESOURCE_MEM) continue; if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) { - dev_warn(&pdev->dev, "device has more than " + dev_warn(dev, "device has more than " __stringify(MAX_UIO_MAPS) " I/O memory resources.\n"); break; @@ -137,13 +134,13 @@ uioinfo->irqcontrol = uio_pdrv_genirq_irqcontrol; uioinfo->priv = priv; - ret = uio_register_device(&pdev->dev, priv->uioinfo); + ret = uio_register_device(dev, priv->uioinfo); if (ret) { - dev_err(&pdev->dev, "unable to register uio device\n"); + dev_err(dev, "unable to register uio device\n"); goto bad1; } - platform_set_drvdata(pdev, priv); + dev_set_drvdata(dev, priv); return 0; bad1: kfree(priv); @@ -151,6 +148,24 @@ return ret; } +static int uio_pdrv_genirq_probe(struct platform_device *pdev) +{ + struct uio_info *uioinfo = pdev->dev.platform_data; + + if (!uioinfo || !uioinfo->name || !uioinfo->version) { + dev_err(&pdev->dev, "missing platform_data\n"); + return -EINVAL; + } + + if (uioinfo->handler || uioinfo->irqcontrol || uioinfo->irq_flags) { + dev_err(&pdev->dev, "interrupt configuration error\n"); + return -EINVAL; + } + + return uio_pdrv_genirq_setup(&pdev->dev, uioinfo, pdev->resource, + pdev->num_resources); +} + static int uio_pdrv_genirq_remove(struct platform_device *pdev) { struct uio_pdrv_genirq_platdata *priv = platform_get_drvdata(pdev); @@ -169,20 +184,135 @@ }, }; +/* --------------------------------------------------------------------- + * OF bus binding + */ + +#if defined(CONFIG_OF) + +#define OF_DRIVER_NAME "uio_of_genirq" +#define OF_DRIVER_VERSION "1" + +static int uio_of_genirq_probe(struct of_device *op, + const struct of_device_id *match) +{ + struct uio_info *uioinfo; + struct resource *resources; + int i, ret = -ENOMEM; + + uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL); + if (!uioinfo) + goto bad0; + + uioinfo->name = (char *)of_get_property(op->node, "name", NULL); + if (!uioinfo->name) { + ret = -ENODEV; + dev_err(&op->dev, "could not get node name\n"); + goto bad1; + } + + uioinfo->version = OF_DRIVER_VERSION; + uioinfo->irq = irq_of_parse_and_map(op->node, 0); + if (!uioinfo->irq) + uioinfo->irq = UIO_IRQ_NONE; + + resources = kzalloc(MAX_UIO_MAPS * sizeof(struct resource), GFP_KERNEL); + if (!resources) + goto bad2; + + for (i = 0; i < MAX_UIO_MAPS; ++i) + if (of_address_to_resource(op->node, i, resources + i)) + break; + + ret = uio_pdrv_genirq_setup(&op->dev, uioinfo, resources, i); + kfree(resources); + if (ret) + goto bad2; + + return 0; + + bad2: + if (uioinfo->irq != UIO_IRQ_NONE) + irq_dispose_mapping(uioinfo->irq); + bad1: + kfree(uioinfo); + bad0: + return ret; +} + +static int uio_of_genirq_remove(struct of_device *op) +{ + struct uio_pdrv_genirq_platdata *priv = dev_get_drvdata(&op->dev); + + uio_unregister_device(priv->uioinfo); + + if (priv->uioinfo->irq != UIO_IRQ_NONE) + irq_dispose_mapping(priv->uioinfo->irq); + + kfree(priv->uioinfo); + kfree(priv); + return 0; +} + +/* Match table for of_platform binding */ +static struct of_device_id uio_of_genirq_match[] = { + { .compatible = "generic-uio", }, + {}, +}; +MODULE_DEVICE_TABLE(of, uio_of_genirq_match); + +static struct of_platform_driver uio_of_genirq_driver = { + .owner = THIS_MODULE, + .name = OF_DRIVER_NAME, + .match_table = uio_of_genirq_match, + .probe = uio_of_genirq_probe, + .remove = uio_of_genirq_remove, +}; + +/* Registration helpers to keep the number of #ifdefs to a minimum */ +static inline int __init uio_of_genirq_register(void) +{ + return of_register_platform_driver(&uio_of_genirq_driver); +} + +static inline void __exit uio_of_genirq_unregister(void) +{ + of_unregister_platform_driver(&uio_of_genirq_driver); +} +#else /* CONFIG_OF */ +/* CONFIG_OF not enabled; do nothing helpers */ +static inline int __init uio_of_genirq_register(void) { return 0; } +static inline void __exit uio_of_genirq_unregister(void) { } +#endif /* CONFIG_OF */ + +/* --------------------------------------------------------------------- */ + static int __init uio_pdrv_genirq_init(void) { - return platform_driver_register(&uio_pdrv_genirq); + int retval; + + retval = uio_of_genirq_register(); + if (retval) + return retval; + + retval = platform_driver_register(&uio_pdrv_genirq); + if (retval) + uio_of_genirq_unregister(); + + return retval; } static void __exit uio_pdrv_genirq_exit(void) { platform_driver_unregister(&uio_pdrv_genirq); + uio_of_genirq_unregister(); } module_init(uio_pdrv_genirq_init); module_exit(uio_pdrv_genirq_exit); MODULE_AUTHOR("Magnus Damm"); -MODULE_DESCRIPTION("Userspace I/O platform driver with generic IRQ handling"); +MODULE_DESCRIPTION("Userspace I/O platform & of driver with generic" + "IRQ handling"); MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:" DRIVER_NAME);
Make the generic uio-driver also accessible for of devices. It utilizes the standard 'reg' and 'interrupt' properties. A typical usage would look like this: fpga-io@00003000 { compatible = "generic-uio"; reg = <0x00003000 0x20>; interrupts = <0xa>; interrupt-parent = <&fpga_irq_mux>; }; To achieve this, the probe function has been refactored, so it can be used by platform and of code. Then, the of driver has been added. Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> --- drivers/uio/uio_pdrv_genirq.c | 178 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 154 insertions(+), 24 deletions(-)