[RFC 0/3] Introduce of_probe_platform_driver()

[RFC 0/3] Introduce of_probe_platform_driver()

[Wolfram Sang]

First, I noted a section mismatch in the mpc52xx_psc_spi-driver which tried to
have its probe in __init. Then, I noted that the mpc512x_psc_spi-driver was in
the __devinit-section, not in __init. I finally saw that there is no
of-equivalent to platform_driver_probe() and tried to implement one.

This is the outcome which seems to work on a MPC5121-board. I have my
spi-device and the sysfs does not show bind/unbind-files.

I can't yet tell why, but the approach looks a bit clumsy. Probably there is
some condition I have missed so far. It should work as a first RFC, however.
Release early, right? :)

All the best,

   Wolfram

Wolfram Sang (3):
  of: platform: introduce of_probe_platform_driver()
  spi: mpc52xx_psc_spi: fix section mismatch warning
  spi: mpc512x_psc_spi: move init-routines to __init

 drivers/of/platform.c         |   30 +++++++++++++++++++++++++++---
 drivers/spi/mpc512x_psc_spi.c |   10 +++++-----
 drivers/spi/mpc52xx_psc_spi.c |    4 ++--
 include/linux/of_platform.h   |    2 ++
 4 files changed, 36 insertions(+), 10 deletions(-)

-- 
1.7.2.3

[PATCH 1/3] of: platform: introduce of_probe_platform_driver()

[Wolfram Sang]

Introduce a function equivalent to platform_driver_probe() for of. This is
needed to keep some SoC devices in the __init section.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/of/platform.c       |   30 +++++++++++++++++++++++++++---
 include/linux/of_platform.h |    2 ++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 5b4a07f..511657b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -72,7 +72,7 @@ static void platform_driver_shutdown_shim(struct platform_device *pdev)
 /**
  * of_register_platform_driver
  */
-int of_register_platform_driver(struct of_platform_driver *drv)
+int __of_register_platform_driver(struct of_platform_driver *drv, bool do_probe)
 {
 	char *of_name;
 
@@ -90,18 +90,42 @@ int of_register_platform_driver(struct of_platform_driver *drv)
 	sprintf(of_name, "of:%s", drv->driver.name);
 	drv->platform_driver.driver.name = of_name;
 
-	if (drv->probe)
-		drv->platform_driver.probe = platform_driver_probe_shim;
 	drv->platform_driver.remove = drv->remove;
 	if (drv->shutdown)
 		drv->platform_driver.shutdown = platform_driver_shutdown_shim;
 	drv->platform_driver.suspend = drv->suspend;
 	drv->platform_driver.resume = drv->resume;
 
+	if (do_probe)
+		return platform_driver_probe(&drv->platform_driver,
+					platform_driver_probe_shim);
+
+	if (drv->probe)
+		drv->platform_driver.probe = platform_driver_probe_shim;
+
 	return platform_driver_register(&drv->platform_driver);
 }
+
+int of_register_platform_driver(struct of_platform_driver *drv)
+{
+	return __of_register_platform_driver(drv, false);
+}
+
 EXPORT_SYMBOL(of_register_platform_driver);
 
+int of_probe_platform_driver(struct of_platform_driver *drv,
+		int (*probe)(struct platform_device *, const struct of_device_id *match))
+{
+	int ret;
+
+	drv->probe = probe;
+	ret = __of_register_platform_driver(drv, true);
+	drv->probe = NULL;
+
+	return ret;
+}
+EXPORT_SYMBOL(of_probe_platform_driver);
+
 void of_unregister_platform_driver(struct of_platform_driver *drv)
 {
 	platform_driver_unregister(&drv->platform_driver);
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index a68716a..c4082c7 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -53,6 +53,8 @@ extern void of_unregister_driver(struct of_platform_driver *drv);
 
 /* Platform drivers register/unregister */
 extern int of_register_platform_driver(struct of_platform_driver *drv);
+extern int of_probe_platform_driver(struct of_platform_driver *drv,
+		int (*probe)(struct platform_device *, const struct of_device_id *match));
 extern void of_unregister_platform_driver(struct of_platform_driver *drv);
 
 extern struct platform_device *of_device_alloc(struct device_node *np,
-- 
1.7.2.3

Re: [PATCH 1/3] of: platform: introduce of_probe_platform_driver()

[Grant Likely]

On Thu, Dec 16, 2010 at 10:13 AM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Introduce a function equivalent to platform_driver_probe() for of. This is
> needed to keep some SoC devices in the __init section.
>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Hi Wolfram

Nack for two reasons:
1- Anything needed at driver probe time should be in an __devinit
section.  What specifically are you wanting to keep in __init
2- of_platform_driver is deprecated.  Drivers should be converted to
use platform_driver directly.

g.

> ---
>  drivers/of/platform.c       |   30 +++++++++++++++++++++++++++---
>  include/linux/of_platform.h |    2 ++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 5b4a07f..511657b 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -72,7 +72,7 @@ static void platform_driver_shutdown_shim(struct platform_device *pdev)
>  /**
>  * of_register_platform_driver
>  */
> -int of_register_platform_driver(struct of_platform_driver *drv)
> +int __of_register_platform_driver(struct of_platform_driver *drv, bool do_probe)
>  {
>        char *of_name;
>
> @@ -90,18 +90,42 @@ int of_register_platform_driver(struct of_platform_driver *drv)
>        sprintf(of_name, "of:%s", drv->driver.name);
>        drv->platform_driver.driver.name = of_name;
>
> -       if (drv->probe)
> -               drv->platform_driver.probe = platform_driver_probe_shim;
>        drv->platform_driver.remove = drv->remove;
>        if (drv->shutdown)
>                drv->platform_driver.shutdown = platform_driver_shutdown_shim;
>        drv->platform_driver.suspend = drv->suspend;
>        drv->platform_driver.resume = drv->resume;
>
> +       if (do_probe)
> +               return platform_driver_probe(&drv->platform_driver,
> +                                       platform_driver_probe_shim);
> +
> +       if (drv->probe)
> +               drv->platform_driver.probe = platform_driver_probe_shim;
> +
>        return platform_driver_register(&drv->platform_driver);
>  }
> +
> +int of_register_platform_driver(struct of_platform_driver *drv)
> +{
> +       return __of_register_platform_driver(drv, false);
> +}
> +
>  EXPORT_SYMBOL(of_register_platform_driver);
>
> +int of_probe_platform_driver(struct of_platform_driver *drv,
> +               int (*probe)(struct platform_device *, const struct of_device_id *match))
> +{
> +       int ret;
> +
> +       drv->probe = probe;
> +       ret = __of_register_platform_driver(drv, true);
> +       drv->probe = NULL;
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(of_probe_platform_driver);
> +
>  void of_unregister_platform_driver(struct of_platform_driver *drv)
>  {
>        platform_driver_unregister(&drv->platform_driver);
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index a68716a..c4082c7 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -53,6 +53,8 @@ extern void of_unregister_driver(struct of_platform_driver *drv);
>
>  /* Platform drivers register/unregister */
>  extern int of_register_platform_driver(struct of_platform_driver *drv);
> +extern int of_probe_platform_driver(struct of_platform_driver *drv,
> +               int (*probe)(struct platform_device *, const struct of_device_id *match));
>  extern void of_unregister_platform_driver(struct of_platform_driver *drv);
>
>  extern struct platform_device *of_device_alloc(struct device_node *np,
> --
> 1.7.2.3
>
>



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

Re: [PATCH 1/3] of: platform: introduce of_probe_platform_driver()

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 392 bytes --]


> 2- of_platform_driver is deprecated.  Drivers should be converted to
> use platform_driver directly.

Ouch, I really should have remembered that. /me grabs a brown paper bag. Sorry
for the noise, dunno what carried me away.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 2/3] spi: mpc52xx_psc_spi: fix section mismatch warning

[Wolfram Sang]

Done by using of_probe_platform_driver() instead of
of_register_platform_driver().

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/spi/mpc52xx_psc_spi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.c
index 983fbbf..4bf6d37 100644
--- a/drivers/spi/mpc52xx_psc_spi.c
+++ b/drivers/spi/mpc52xx_psc_spi.c
@@ -509,7 +509,6 @@ static const struct of_device_id mpc52xx_psc_spi_of_match[] = {
 MODULE_DEVICE_TABLE(of, mpc52xx_psc_spi_of_match);
 
 static struct of_platform_driver mpc52xx_psc_spi_of_driver = {
-	.probe = mpc52xx_psc_spi_of_probe,
 	.remove = __exit_p(mpc52xx_psc_spi_of_remove),
 	.driver = {
 		.name = "mpc52xx-psc-spi",
@@ -520,7 +519,8 @@ static struct of_platform_driver mpc52xx_psc_spi_of_driver = {
 
 static int __init mpc52xx_psc_spi_init(void)
 {
-	return of_register_platform_driver(&mpc52xx_psc_spi_of_driver);
+	return of_probe_platform_driver(&mpc52xx_psc_spi_of_driver,
+					mpc52xx_psc_spi_of_probe);
 }
 module_init(mpc52xx_psc_spi_init);
 
-- 
1.7.2.3

Re: [PATCH 2/3] spi: mpc52xx_psc_spi: fix section mismatch warning

[Grant Likely]

On Thu, Dec 16, 2010 at 10:13 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Done by using of_probe_platform_driver() instead of
> of_register_platform_driver().
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Move the probe hook to the __devinit section instead.

g.

> ---
>  drivers/spi/mpc52xx_psc_spi.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.c
> index 983fbbf..4bf6d37 100644
> --- a/drivers/spi/mpc52xx_psc_spi.c
> +++ b/drivers/spi/mpc52xx_psc_spi.c
> @@ -509,7 +509,6 @@ static const struct of_device_id mpc52xx_psc_spi_of_match[] = {
>  MODULE_DEVICE_TABLE(of, mpc52xx_psc_spi_of_match);
>
>  static struct of_platform_driver mpc52xx_psc_spi_of_driver = {
> -       .probe = mpc52xx_psc_spi_of_probe,
>        .remove = __exit_p(mpc52xx_psc_spi_of_remove),
>        .driver = {
>                .name = "mpc52xx-psc-spi",
> @@ -520,7 +519,8 @@ static struct of_platform_driver mpc52xx_psc_spi_of_driver = {
>
>  static int __init mpc52xx_psc_spi_init(void)
>  {
> -       return of_register_platform_driver(&mpc52xx_psc_spi_of_driver);
> +       return of_probe_platform_driver(&mpc52xx_psc_spi_of_driver,
> +                                       mpc52xx_psc_spi_of_probe);
>  }
>  module_init(mpc52xx_psc_spi_init);
>
> --
> 1.7.2.3
>
>



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

[PATCH 3/3] spi: mpc512x_psc_spi: move probe-routine to __init

[Wolfram Sang]

Since of_probe_platform_driver(), this SoC-device can be moved from
__devinit to __init because it is not hotpluggable.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/spi/mpc512x_psc_spi.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/mpc512x_psc_spi.c b/drivers/spi/mpc512x_psc_spi.c
index 77d9e7e..c672144 100644
--- a/drivers/spi/mpc512x_psc_spi.c
+++ b/drivers/spi/mpc512x_psc_spi.c
@@ -507,7 +507,7 @@ static int __devexit mpc512x_psc_spi_do_remove(struct device *dev)
 	return 0;
 }
 
-static int __devinit mpc512x_psc_spi_of_probe(struct platform_device *op,
+static int __init mpc512x_psc_spi_of_probe(struct platform_device *op,
 					      const struct of_device_id *match)
 {
 	const u32 *regaddr_p;
@@ -539,7 +539,7 @@ static int __devinit mpc512x_psc_spi_of_probe(struct platform_device *op,
 				irq_of_parse_and_map(op->dev.of_node, 0), id);
 }
 
-static int __devexit mpc512x_psc_spi_of_remove(struct platform_device *op)
+static int __exit mpc512x_psc_spi_of_remove(struct platform_device *op)
 {
 	return mpc512x_psc_spi_do_remove(&op->dev);
 }
@@ -552,8 +552,7 @@ static struct of_device_id mpc512x_psc_spi_of_match[] = {
 MODULE_DEVICE_TABLE(of, mpc512x_psc_spi_of_match);
 
 static struct of_platform_driver mpc512x_psc_spi_of_driver = {
-	.probe = mpc512x_psc_spi_of_probe,
-	.remove = __devexit_p(mpc512x_psc_spi_of_remove),
+	.remove = __exit_p(mpc512x_psc_spi_of_remove),
 	.driver = {
 		.name = "mpc512x-psc-spi",
 		.owner = THIS_MODULE,
@@ -563,7 +562,8 @@ static struct of_platform_driver mpc512x_psc_spi_of_driver = {
 
 static int __init mpc512x_psc_spi_init(void)
 {
-	return of_register_platform_driver(&mpc512x_psc_spi_of_driver);
+	return of_probe_platform_driver(&mpc512x_psc_spi_of_driver,
+					mpc512x_psc_spi_of_probe);
 }
 module_init(mpc512x_psc_spi_init);
 
-- 
1.7.2.3

Re: [PATCH 3/3] spi: mpc512x_psc_spi: move probe-routine to __init

[Grant Likely]

On Thu, Dec 16, 2010 at 10:13 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Since of_probe_platform_driver(), this SoC-device can be moved from
> __devinit to __init because it is not hotpluggable.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

I really wouldn't bother.  You're counting pennies when the kernel
uses upwards of 4 megabytes for .text alone, and I don't like fiddling
with the driver model the way platform_bus_probe() does.

g.