[PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Patrick Pannuto]

(lkml.org seems to have lost August 3rd...)
RFC: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01342.html
 v1: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01942.html

Based on the idea and code originally proposed by Kevin Hilman:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html

Changes from v1:

   * "Pseduo" buses are no longer init'd, they are [un]registered by:
       - pseudo_platform_bus_register(struct bus_type *)
       - pseudo_platform_bus_unregister(struct bus_type *)
   * These registrations [de]allocate a dev_pm_ops structure for the
     pseudo bus_type
   * Do not overwrite the parent if .bus is already set (that is, allow
     pseudo bus devices to be root devices)

   * Split into 2 patches:
       - 1/2: Already sent separately, but included here for clarity
       - 2/2: The real meat of the patch (this patch)

INTRO

As SOCs become more popular, the desire to quickly define a simple,
but functional, bus type with only a few unique properties becomes
desirable. As they become more complicated, the ability to nest these
simple busses and otherwise orchestrate them to match the actual
topology also becomes desirable.

EXAMPLE USAGE

/arch/ARCH/MY_ARCH/my_bus.c:

	#include <linux/device.h>
	#include <linux/platform_device.h>

	struct bus_type SOC_bus_type = {
		.name = "SOC-bus-type",
	};
	EXPORT_SYMBOL_GPL(SOC_bus_type);

	struct platform_device SOC_bus1 = {
		.name	 = "SOC-bus1",
		.id		= -1,
		.dev.bus = &SOC_bus_type;
	};
	EXPORT_SYMBOL_GPL(SOC_bus1);

	struct platform_device SOC_bus2 = {
		.name	 = "SOC-bus2",
		.id		= -2,
		.dev.bus = &SOC_bus_type;
	};
	EXPORT_SYMBOL_GPL(SOC_bus2);

	static int __init SOC_bus_init(void)
	{
		int error;

		error = pseudo_platform_bus_register(&SOC_bus_type);
		if (error)
			return error;

		error = platform_device_register(&SOC_bus1);
		if (error)
			goto fail_bus1;

		error = platform_device_register(&SOC_bus2);
		if (error)
			goto fail_bus2;

		return error;

		/* platform_device_unregister(&SOC_bus2); */
fail_bus2:
		platform_device_unregister(&SOC_bus1);
fail_bus1:
		pseudo_platform_bus_unregister(&SOC_bus_type);

		return error;
	}

/drivers/my_driver.c:
	static struct platform_driver my_driver = {
		.driver = {
			.name	= "my-driver",
			.owner	= THIS_MODULE,
			.bus	= &SOC_bus_type,
		},
	};

/somewhere/my_device.c:
	static struct platform_device my_device = {
		.name		= "my-device",
		.id		= -1,
		.dev.bus	= &my_bus_type,
		.dev.parent	= &SOC_bus1.dev,
	};

This will build a device tree that mirrors the actual system:

/sys/bus
|-- SOC-bus-type
|   |-- devices
|   |   |-- SOC_bus1 -> ../../../devices/SOC_bus1
|   |   |-- SOC_bus2 -> ../../../devices/SOC_bus2
|   |   |-- my-device -> ../../../devices/SOC_bus1/my-device
|   |-- drivers
|   |   |-- my-driver

/sys/devices
|-- SOC_bus1
|   |-- my-device
|-- SOC_bus2

Driver can drive any device on the SOC, which is logical, without
actually being registered on multiple /bus_types/, even though the
devices may be on different /physical buses/ (which are actually
just devices).

THOUGHTS:

1.
Notice that for a device / driver, only 3 lines were added to
switch from the platform bus to the new SOC_bus. This is
especially valuable if we consider supporting a legacy SOCs
and new SOCs where the same driver is used, but may need to
be on either the platform bus or the new SOC_bus. The above
code then becomes:

	(possibly in a header)
	#ifdef CONFIG_MY_BUS
	#define MY_BUS_TYPE     &SOC_bus_type
	#else
	#define MY_BUS_TYPE     NULL
	#endif

/drivers/my_driver.c
	static struct platform_driver my_driver = {
		.driver = {
			.name   = "my-driver",
			.owner  = THIS_MODULE,
			.bus    = MY_BUS_TYPE,
		},
	};

Which will allow the same driver to easily to used on either
the platform bus or the newly defined bus type.

2.
Implementations wishing to make dynamic / run-time decisions on where
devices are placed could easily create wrapper functions, that is

	int SOC_device_register(struct platform_device *pdev)
	{
		if (pdev->archdata->flag)
			pdev->dev.parent = &SOC_bus1.dev;
		else
			pdev->dev.parent = &SOC_bus2.dev;

		return platform_device_register(pdev);
	}

A similar solution also would allow for run-time determination of dev.bus,
if that were necessary for your platform.

3.
I'm not convinced that dynamically allocating a new copy of dev_pm_ops is
the best solution. I *AM*, however, convinced that removing const from
	struct bus_type {
		...
		const struct dev_pm_ops *pm;
		...
	};
would be a mistake; it is far too easy to overwrite one of the function
pointers on accident, and the const serves a very useful purpose here.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Signed-off-by: Patrick Pannuto <ppannuto@codeaurora.org>
---
 drivers/base/platform.c         |   92 +++++++++++++++++++++++++++++++++++++-
 include/linux/platform_device.h |    3 +
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index b69ccb4..933e0c1 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -238,8 +238,12 @@ int platform_device_add(struct platform_device *pdev)
 	if (!pdev)
 		return -EINVAL;
 
-	if (!pdev->dev.parent)
-		pdev->dev.parent = &platform_bus;
+	if (!pdev->dev.bus) {
+		pdev->dev.bus = &platform_bus_type;
+
+		if (!pdev->dev.parent)
+			pdev->dev.parent = &platform_bus;
+	}
 
 	pdev->dev.bus = &platform_bus_type;
 
@@ -482,7 +486,8 @@ static void platform_drv_shutdown(struct device *_dev)
  */
 int platform_driver_register(struct platform_driver *drv)
 {
-	drv->driver.bus = &platform_bus_type;
+	if (!drv->driver.bus)
+		drv->driver.bus = &platform_bus_type;
 	if (drv->probe)
 		drv->driver.probe = platform_drv_probe;
 	if (drv->remove)
@@ -1017,6 +1022,87 @@ struct bus_type platform_bus_type = {
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
+/** pseudo_platform_bus_register - register an "almost platform bus"
+ * @bus: partially complete bus type to register
+ *
+ * This init will fill in any ommitted fields in @bus, however, it
+ * also allocates memory and replaces the pm field in @bus, since
+ * pm is const, but some of its pointers may need this one-time
+ * initialziation overwrite.
+ *
+ * @bus's registered this way should be released with
+ * pseudo_platform_bus_unregister
+ */
+int pseudo_platform_bus_register(struct bus_type *bus)
+{
+	int error;
+	struct dev_pm_ops* heap_pm;
+	heap_pm = kmalloc(sizeof (*heap_pm), GFP_KERNEL);
+
+	if (!heap_pm)
+		return -ENOMEM;
+
+	if (!bus->dev_attrs)
+		bus->dev_attrs = platform_bus_type.dev_attrs;
+	if (!bus->match)
+		bus->match = platform_bus_type.match;
+	if (!bus->uevent)
+		bus->uevent = platform_bus_type.uevent;
+	if (!bus->pm)
+		memcpy(heap_pm, &platform_bus_type.pm, sizeof(*heap_pm));
+	else {
+		heap_pm->prepare = (bus->pm->prepare) ?
+			bus->pm->prepare : platform_pm_prepare;
+		heap_pm->complete = (bus->pm->complete) ?
+			bus->pm->complete : platform_pm_complete;
+		heap_pm->suspend = (bus->pm->suspend) ?
+			bus->pm->suspend : platform_pm_suspend;
+		heap_pm->resume = (bus->pm->resume) ?
+			bus->pm->resume : platform_pm_resume;
+		heap_pm->freeze = (bus->pm->freeze) ?
+			bus->pm->freeze : platform_pm_freeze;
+		heap_pm->thaw = (bus->pm->thaw) ?
+			bus->pm->thaw : platform_pm_thaw;
+		heap_pm->poweroff = (bus->pm->poweroff) ?
+			bus->pm->poweroff : platform_pm_poweroff;
+		heap_pm->restore = (bus->pm->restore) ?
+			bus->pm->restore : platform_pm_restore;
+		heap_pm->suspend_noirq = (bus->pm->suspend_noirq) ?
+			bus->pm->suspend_noirq : platform_pm_suspend_noirq;
+		heap_pm->resume_noirq = (bus->pm->resume_noirq) ?
+			bus->pm->resume_noirq : platform_pm_resume_noirq;
+		heap_pm->freeze_noirq = (bus->pm->freeze_noirq) ?
+			bus->pm->freeze_noirq : platform_pm_freeze_noirq;
+		heap_pm->thaw_noirq = (bus->pm->thaw_noirq) ?
+			bus->pm->thaw_noirq : platform_pm_thaw_noirq;
+		heap_pm->poweroff_noirq = (bus->pm->poweroff_noirq) ?
+			bus->pm->poweroff_noirq : platform_pm_poweroff_noirq;
+		heap_pm->restore_noirq = (bus->pm->restore_noirq) ?
+			bus->pm->restore_noirq : platform_pm_restore_noirq;
+		heap_pm->runtime_suspend = (bus->pm->runtime_suspend) ?
+			bus->pm->runtime_suspend : platform_pm_runtime_suspend;
+		heap_pm->runtime_resume = (bus->pm->runtime_resume) ?
+			bus->pm->runtime_resume : platform_pm_runtime_resume;
+		heap_pm->runtime_idle = (bus->pm->runtime_idle) ?
+			bus->pm->runtime_idle : platform_pm_runtime_idle;
+	}
+	bus->pm = heap_pm;
+
+	error = bus_register(bus);
+	if (error)
+		kfree(bus->pm);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(pseudo_platform_bus_register);
+
+void pseudo_platform_bus_unregister(struct bus_type *bus)
+{
+	bus_unregister(bus);
+	kfree(bus->pm);
+}
+EXPORT_SYMBOL_GPL(pseudo_platform_bus_unregister);
+
 int __init platform_bus_init(void)
 {
 	int error;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 5417944..5ec827c 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -79,6 +79,9 @@ extern int platform_driver_probe(struct platform_driver *driver,
 #define platform_get_drvdata(_dev)	dev_get_drvdata(&(_dev)->dev)
 #define platform_set_drvdata(_dev,data)	dev_set_drvdata(&(_dev)->dev, (data))
 
+extern int pseudo_platform_bus_register(struct bus_type *);
+extern void pseudo_platform_bus_unregister(struct bus_type *);
+
 extern struct platform_device *platform_create_bundle(struct platform_driver *driver,
 					int (*probe)(struct platform_device *),
 					struct resource *res, unsigned int n_res,
-- 
1.7.2

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Patrick Pannuto]

On 08/10/2010 04:49 PM, Patrick Pannuto wrote:
> (lkml.org seems to have lost August 3rd...)
> RFC: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01342.html
>  v1: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01942.html
> 
> Based on the idea and code originally proposed by Kevin Hilman:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
> 
> Changes from v1:
> 
>    * "Pseduo" buses are no longer init'd, they are [un]registered by:
>        - pseudo_platform_bus_register(struct bus_type *)
>        - pseudo_platform_bus_unregister(struct bus_type *)
>    * These registrations [de]allocate a dev_pm_ops structure for the
>      pseudo bus_type
>    * Do not overwrite the parent if .bus is already set (that is, allow
>      pseudo bus devices to be root devices)
> 
>    * Split into 2 patches:
>        - 1/2: Already sent separately, but included here for clarity
>        - 2/2: The real meat of the patch (this patch)
> 
> INTRO
> 
> As SOCs become more popular, the desire to quickly define a simple,
> but functional, bus type with only a few unique properties becomes
> desirable. As they become more complicated, the ability to nest these
> simple busses and otherwise orchestrate them to match the actual
> topology also becomes desirable.
> 
> EXAMPLE USAGE
> 
> /arch/ARCH/MY_ARCH/my_bus.c:
> 
> 	#include <linux/device.h>
> 	#include <linux/platform_device.h>
> 
> 	struct bus_type SOC_bus_type = {
> 		.name = "SOC-bus-type",
> 	};
> 	EXPORT_SYMBOL_GPL(SOC_bus_type);
> 
> 	struct platform_device SOC_bus1 = {
> 		.name	 = "SOC-bus1",
> 		.id		= -1,
> 		.dev.bus = &SOC_bus_type;
> 	};
> 	EXPORT_SYMBOL_GPL(SOC_bus1);
> 
> 	struct platform_device SOC_bus2 = {
> 		.name	 = "SOC-bus2",
> 		.id		= -2,
> 		.dev.bus = &SOC_bus_type;
> 	};
> 	EXPORT_SYMBOL_GPL(SOC_bus2);
> 
> 	static int __init SOC_bus_init(void)
> 	{
> 		int error;
> 
> 		error = pseudo_platform_bus_register(&SOC_bus_type);
> 		if (error)
> 			return error;
> 
> 		error = platform_device_register(&SOC_bus1);
> 		if (error)
> 			goto fail_bus1;
> 
> 		error = platform_device_register(&SOC_bus2);
> 		if (error)
> 			goto fail_bus2;
> 
> 		return error;
> 
> 		/* platform_device_unregister(&SOC_bus2); */
> fail_bus2:
> 		platform_device_unregister(&SOC_bus1);
> fail_bus1:
> 		pseudo_platform_bus_unregister(&SOC_bus_type);
> 
> 		return error;
> 	}
> 
> /drivers/my_driver.c:
> 	static struct platform_driver my_driver = {
> 		.driver = {
> 			.name	= "my-driver",
> 			.owner	= THIS_MODULE,
> 			.bus	= &SOC_bus_type,
> 		},
> 	};
> 
> /somewhere/my_device.c:
> 	static struct platform_device my_device = {
> 		.name		= "my-device",
> 		.id		= -1,
> 		.dev.bus	= &my_bus_type,
> 		.dev.parent	= &SOC_bus1.dev,
> 	};
> 
> This will build a device tree that mirrors the actual system:
> 
> /sys/bus
> |-- SOC-bus-type
> |   |-- devices
> |   |   |-- SOC_bus1 -> ../../../devices/SOC_bus1
> |   |   |-- SOC_bus2 -> ../../../devices/SOC_bus2
> |   |   |-- my-device -> ../../../devices/SOC_bus1/my-device
> |   |-- drivers
> |   |   |-- my-driver
> 
> /sys/devices
> |-- SOC_bus1
> |   |-- my-device
> |-- SOC_bus2
> 
> Driver can drive any device on the SOC, which is logical, without
> actually being registered on multiple /bus_types/, even though the
> devices may be on different /physical buses/ (which are actually
> just devices).
> 
> THOUGHTS:
> 
> 1.
> Notice that for a device / driver, only 3 lines were added to
> switch from the platform bus to the new SOC_bus. This is
> especially valuable if we consider supporting a legacy SOCs
> and new SOCs where the same driver is used, but may need to
> be on either the platform bus or the new SOC_bus. The above
> code then becomes:
> 
> 	(possibly in a header)
> 	#ifdef CONFIG_MY_BUS
> 	#define MY_BUS_TYPE     &SOC_bus_type
> 	#else
> 	#define MY_BUS_TYPE     NULL
> 	#endif
> 
> /drivers/my_driver.c
> 	static struct platform_driver my_driver = {
> 		.driver = {
> 			.name   = "my-driver",
> 			.owner  = THIS_MODULE,
> 			.bus    = MY_BUS_TYPE,
> 		},
> 	};
> 
> Which will allow the same driver to easily to used on either
> the platform bus or the newly defined bus type.
> 
> 2.
> Implementations wishing to make dynamic / run-time decisions on where
> devices are placed could easily create wrapper functions, that is
> 
> 	int SOC_device_register(struct platform_device *pdev)
> 	{
> 		if (pdev->archdata->flag)
> 			pdev->dev.parent = &SOC_bus1.dev;
> 		else
> 			pdev->dev.parent = &SOC_bus2.dev;
> 
> 		return platform_device_register(pdev);
> 	}
> 
> A similar solution also would allow for run-time determination of dev.bus,
> if that were necessary for your platform.
> 
> 3.
> I'm not convinced that dynamically allocating a new copy of dev_pm_ops is
> the best solution. I *AM*, however, convinced that removing const from
> 	struct bus_type {
> 		...
> 		const struct dev_pm_ops *pm;
> 		...
> 	};
> would be a mistake; it is far too easy to overwrite one of the function
> pointers on accident, and the const serves a very useful purpose here.
> 
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Patrick Pannuto <ppannuto@codeaurora.org>
> ---
>  drivers/base/platform.c         |   92 +++++++++++++++++++++++++++++++++++++-
>  include/linux/platform_device.h |    3 +
>  2 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index b69ccb4..933e0c1 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -238,8 +238,12 @@ int platform_device_add(struct platform_device *pdev)
>  	if (!pdev)
>  		return -EINVAL;
>  
> -	if (!pdev->dev.parent)
> -		pdev->dev.parent = &platform_bus;
> +	if (!pdev->dev.bus) {
> +		pdev->dev.bus = &platform_bus_type;
> +
> +		if (!pdev->dev.parent)
> +			pdev->dev.parent = &platform_bus;
> +	}
>  
>  	pdev->dev.bus = &platform_bus_type;

^^^ small bug here, this line should be deleted; any other comments though?

>  
> @@ -482,7 +486,8 @@ static void platform_drv_shutdown(struct device *_dev)
>   */
>  int platform_driver_register(struct platform_driver *drv)
>  {
> -	drv->driver.bus = &platform_bus_type;
> +	if (!drv->driver.bus)
> +		drv->driver.bus = &platform_bus_type;
>  	if (drv->probe)
>  		drv->driver.probe = platform_drv_probe;
>  	if (drv->remove)
> @@ -1017,6 +1022,87 @@ struct bus_type platform_bus_type = {
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>  
> +/** pseudo_platform_bus_register - register an "almost platform bus"
> + * @bus: partially complete bus type to register
> + *
> + * This init will fill in any ommitted fields in @bus, however, it
> + * also allocates memory and replaces the pm field in @bus, since
> + * pm is const, but some of its pointers may need this one-time
> + * initialziation overwrite.
> + *
> + * @bus's registered this way should be released with
> + * pseudo_platform_bus_unregister
> + */
> +int pseudo_platform_bus_register(struct bus_type *bus)
> +{
> +	int error;
> +	struct dev_pm_ops* heap_pm;
> +	heap_pm = kmalloc(sizeof (*heap_pm), GFP_KERNEL);
> +
> +	if (!heap_pm)
> +		return -ENOMEM;
> +
> +	if (!bus->dev_attrs)
> +		bus->dev_attrs = platform_bus_type.dev_attrs;
> +	if (!bus->match)
> +		bus->match = platform_bus_type.match;
> +	if (!bus->uevent)
> +		bus->uevent = platform_bus_type.uevent;
> +	if (!bus->pm)
> +		memcpy(heap_pm, &platform_bus_type.pm, sizeof(*heap_pm));
> +	else {
> +		heap_pm->prepare = (bus->pm->prepare) ?
> +			bus->pm->prepare : platform_pm_prepare;
> +		heap_pm->complete = (bus->pm->complete) ?
> +			bus->pm->complete : platform_pm_complete;
> +		heap_pm->suspend = (bus->pm->suspend) ?
> +			bus->pm->suspend : platform_pm_suspend;
> +		heap_pm->resume = (bus->pm->resume) ?
> +			bus->pm->resume : platform_pm_resume;
> +		heap_pm->freeze = (bus->pm->freeze) ?
> +			bus->pm->freeze : platform_pm_freeze;
> +		heap_pm->thaw = (bus->pm->thaw) ?
> +			bus->pm->thaw : platform_pm_thaw;
> +		heap_pm->poweroff = (bus->pm->poweroff) ?
> +			bus->pm->poweroff : platform_pm_poweroff;
> +		heap_pm->restore = (bus->pm->restore) ?
> +			bus->pm->restore : platform_pm_restore;
> +		heap_pm->suspend_noirq = (bus->pm->suspend_noirq) ?
> +			bus->pm->suspend_noirq : platform_pm_suspend_noirq;
> +		heap_pm->resume_noirq = (bus->pm->resume_noirq) ?
> +			bus->pm->resume_noirq : platform_pm_resume_noirq;
> +		heap_pm->freeze_noirq = (bus->pm->freeze_noirq) ?
> +			bus->pm->freeze_noirq : platform_pm_freeze_noirq;
> +		heap_pm->thaw_noirq = (bus->pm->thaw_noirq) ?
> +			bus->pm->thaw_noirq : platform_pm_thaw_noirq;
> +		heap_pm->poweroff_noirq = (bus->pm->poweroff_noirq) ?
> +			bus->pm->poweroff_noirq : platform_pm_poweroff_noirq;
> +		heap_pm->restore_noirq = (bus->pm->restore_noirq) ?
> +			bus->pm->restore_noirq : platform_pm_restore_noirq;
> +		heap_pm->runtime_suspend = (bus->pm->runtime_suspend) ?
> +			bus->pm->runtime_suspend : platform_pm_runtime_suspend;
> +		heap_pm->runtime_resume = (bus->pm->runtime_resume) ?
> +			bus->pm->runtime_resume : platform_pm_runtime_resume;
> +		heap_pm->runtime_idle = (bus->pm->runtime_idle) ?
> +			bus->pm->runtime_idle : platform_pm_runtime_idle;
> +	}
> +	bus->pm = heap_pm;
> +
> +	error = bus_register(bus);
> +	if (error)
> +		kfree(bus->pm);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_register);
> +
> +void pseudo_platform_bus_unregister(struct bus_type *bus)
> +{
> +	bus_unregister(bus);
> +	kfree(bus->pm);
> +}
> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_unregister);
> +
>  int __init platform_bus_init(void)
>  {
>  	int error;
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 5417944..5ec827c 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -79,6 +79,9 @@ extern int platform_driver_probe(struct platform_driver *driver,
>  #define platform_get_drvdata(_dev)	dev_get_drvdata(&(_dev)->dev)
>  #define platform_set_drvdata(_dev,data)	dev_set_drvdata(&(_dev)->dev, (data))
>  
> +extern int pseudo_platform_bus_register(struct bus_type *);
> +extern void pseudo_platform_bus_unregister(struct bus_type *);
> +
>  extern struct platform_device *platform_create_bundle(struct platform_driver *driver,
>  					int (*probe)(struct platform_device *),
>  					struct resource *res, unsigned int n_res,


-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Greg KH]

On Thu, Aug 12, 2010 at 06:13:09PM -0700, Patrick Pannuto wrote:
> On 08/10/2010 04:49 PM, Patrick Pannuto wrote:
> 
> ^^^ small bug here, this line should be deleted; any other comments though?

Sorry, been busy with the -rc1 merge stuff, I'll have more time to look
at it next week.

Well, after you address Grant's issues first :)

thanks,

greg k-h

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Grant Likely]

On Tue, Aug 10, 2010 at 5:49 PM, Patrick Pannuto
<ppannuto@codeaurora.org> wrote:
> (lkml.org seems to have lost August 3rd...)
> RFC: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01342.html
>  v1: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01942.html
>
> Based on the idea and code originally proposed by Kevin Hilman:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html

Hi Patrick,

Before acking this as something that should be merged, I'd like to see
some real device drivers converted to use this interface so I can
better judge whether or not it is a good idea.  More comments below.

> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index b69ccb4..933e0c1 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -238,8 +238,12 @@ int platform_device_add(struct platform_device *pdev)
>        if (!pdev)
>                return -EINVAL;
>
> -       if (!pdev->dev.parent)
> -               pdev->dev.parent = &platform_bus;
> +       if (!pdev->dev.bus) {
> +               pdev->dev.bus = &platform_bus_type;
> +
> +               if (!pdev->dev.parent)
> +                       pdev->dev.parent = &platform_bus;
> +       }
>        pdev->dev.bus = &platform_bus_type;
>

For safety, I think you'd want to have a separate add function for
each new platform bus type, and change the name of this function to
explicitly pass the bus type:

int __platform_device_add(struct platform_device *pdev)
{
	if (!pdev->dev.bus)
		return -EINVAL;
	[... existing code ...]
}

int platform_device_add(struct platform_device *pdev)
{
       if (!pdev->dev.parent)
               pdev->dev.parent = &platform_bus;
       pdev->dev.bus = &platform_bus_type;
       __platform_device_add(pdev);
}

And then for each bus type (in this example, I'll call it
foo_bus_type, and assume foo_device wraps platform_device):

int foo_device_add(struct foo_device *foo)
{
	foo->pdev.dev.bus = &foo_bus_type;
	__platform_device_add(&foo->pdev);
}

That will allow the new bus_type code to keep foo_bus_type as an
unexported static and will provide a bit more type safety.  For
example, it avoids accidentally registering a plain (unwrapped)
platform device on the foo_bus_type.

> @@ -482,7 +486,8 @@ static void platform_drv_shutdown(struct device *_dev)
>  */
>  int platform_driver_register(struct platform_driver *drv)
>  {
> -       drv->driver.bus = &platform_bus_type;
> +       if (!drv->driver.bus)
> +               drv->driver.bus = &platform_bus_type;
>        if (drv->probe)
>                drv->driver.probe = platform_drv_probe;
>        if (drv->remove)

Ditto here; better to have a separate foo_driver_register() for each
new bus type.

> @@ -1017,6 +1022,87 @@ struct bus_type platform_bus_type = {
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>
> +/** pseudo_platform_bus_register - register an "almost platform bus"

Kerneldoc style error.  Should be:

+/**
+ * pseudo_platform_bus_register - register an "almost platform bus"

> + * @bus: partially complete bus type to register
> + *
> + * This init will fill in any ommitted fields in @bus, however, it
> + * also allocates memory and replaces the pm field in @bus, since
> + * pm is const, but some of its pointers may need this one-time
> + * initialziation overwrite.
> + *
> + * @bus's registered this way should be released with
> + * pseudo_platform_bus_unregister
> + */
> +int pseudo_platform_bus_register(struct bus_type *bus)
> +{
> +       int error;
> +       struct dev_pm_ops* heap_pm;

Nit: heap_pm is an odd name.  Typically Linux local vars are named
according to what they are, not where the memory is allocated from.
How about simply 'ops'

> +       heap_pm = kmalloc(sizeof (*heap_pm), GFP_KERNEL);
> +
> +       if (!heap_pm)
> +               return -ENOMEM;
> +
> +       if (!bus->dev_attrs)
> +               bus->dev_attrs = platform_bus_type.dev_attrs;
> +       if (!bus->match)
> +               bus->match = platform_bus_type.match;
> +       if (!bus->uevent)
> +               bus->uevent = platform_bus_type.uevent;
> +       if (!bus->pm)
> +               memcpy(heap_pm, &platform_bus_type.pm, sizeof(*heap_pm));
> +       else {
> +               heap_pm->prepare = (bus->pm->prepare) ?
> +                       bus->pm->prepare : platform_pm_prepare;
> +               heap_pm->complete = (bus->pm->complete) ?
> +                       bus->pm->complete : platform_pm_complete;
> +               heap_pm->suspend = (bus->pm->suspend) ?
> +                       bus->pm->suspend : platform_pm_suspend;
> +               heap_pm->resume = (bus->pm->resume) ?
> +                       bus->pm->resume : platform_pm_resume;
> +               heap_pm->freeze = (bus->pm->freeze) ?
> +                       bus->pm->freeze : platform_pm_freeze;
> +               heap_pm->thaw = (bus->pm->thaw) ?
> +                       bus->pm->thaw : platform_pm_thaw;
> +               heap_pm->poweroff = (bus->pm->poweroff) ?
> +                       bus->pm->poweroff : platform_pm_poweroff;
> +               heap_pm->restore = (bus->pm->restore) ?
> +                       bus->pm->restore : platform_pm_restore;
> +               heap_pm->suspend_noirq = (bus->pm->suspend_noirq) ?
> +                       bus->pm->suspend_noirq : platform_pm_suspend_noirq;
> +               heap_pm->resume_noirq = (bus->pm->resume_noirq) ?
> +                       bus->pm->resume_noirq : platform_pm_resume_noirq;
> +               heap_pm->freeze_noirq = (bus->pm->freeze_noirq) ?
> +                       bus->pm->freeze_noirq : platform_pm_freeze_noirq;
> +               heap_pm->thaw_noirq = (bus->pm->thaw_noirq) ?
> +                       bus->pm->thaw_noirq : platform_pm_thaw_noirq;
> +               heap_pm->poweroff_noirq = (bus->pm->poweroff_noirq) ?
> +                       bus->pm->poweroff_noirq : platform_pm_poweroff_noirq;
> +               heap_pm->restore_noirq = (bus->pm->restore_noirq) ?
> +                       bus->pm->restore_noirq : platform_pm_restore_noirq;
> +               heap_pm->runtime_suspend = (bus->pm->runtime_suspend) ?
> +                       bus->pm->runtime_suspend : platform_pm_runtime_suspend;
> +               heap_pm->runtime_resume = (bus->pm->runtime_resume) ?
> +                       bus->pm->runtime_resume : platform_pm_runtime_resume;
> +               heap_pm->runtime_idle = (bus->pm->runtime_idle) ?
> +                       bus->pm->runtime_idle : platform_pm_runtime_idle;
> +       }
> +       bus->pm = heap_pm;
> +
> +       error = bus_register(bus);
> +       if (error)
> +               kfree(bus->pm);
> +
> +       return error;
> +}
> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_register);

Ick, this is a nasty list of conditional statements.  :-)  Instead it
could have an unconditional initialize function which sets it up just
like the platform bus without registering.  Then allow the
foo_bus_type initialization code override the ones it cares about, and
then register directly.

> +
> +void pseudo_platform_bus_unregister(struct bus_type *bus)
> +{
> +       bus_unregister(bus);
> +       kfree(bus->pm);
> +}
> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_unregister);
> +
>  int __init platform_bus_init(void)
>  {
>        int error;
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 5417944..5ec827c 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -79,6 +79,9 @@ extern int platform_driver_probe(struct platform_driver *driver,
>  #define platform_get_drvdata(_dev)     dev_get_drvdata(&(_dev)->dev)
>  #define platform_set_drvdata(_dev,data)        dev_set_drvdata(&(_dev)->dev, (data))
>
> +extern int pseudo_platform_bus_register(struct bus_type *);
> +extern void pseudo_platform_bus_unregister(struct bus_type *);
> +
>  extern struct platform_device *platform_create_bundle(struct platform_driver *driver,
>                                        int (*probe)(struct platform_device *),
>                                        struct resource *res, unsigned int n_res,
> --
> 1.7.2
>
>



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

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Patrick Pannuto]

On 08/13/2010 03:05 PM, Grant Likely wrote:
> On Tue, Aug 10, 2010 at 5:49 PM, Patrick Pannuto
> <ppannuto@codeaurora.org> wrote:
>> (lkml.org seems to have lost August 3rd...)
>> RFC: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01342.html
>>  v1: http://lkml.indiana.edu/hypermail//linux/kernel/1008.0/01942.html
>>
>> Based on the idea and code originally proposed by Kevin Hilman:
>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
> 
> Hi Patrick,
> 
> Before acking this as something that should be merged, I'd like to see
> some real device drivers converted to use this interface so I can
> better judge whether or not it is a good idea.  More comments below.
> 

Ok, I can do that.

>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index b69ccb4..933e0c1 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -238,8 +238,12 @@ int platform_device_add(struct platform_device *pdev)
>>        if (!pdev)
>>                return -EINVAL;
>>
>> -       if (!pdev->dev.parent)
>> -               pdev->dev.parent = &platform_bus;
>> +       if (!pdev->dev.bus) {
>> +               pdev->dev.bus = &platform_bus_type;
>> +
>> +               if (!pdev->dev.parent)
>> +                       pdev->dev.parent = &platform_bus;
>> +       }
>>        pdev->dev.bus = &platform_bus_type;
>>
> 
> For safety, I think you'd want to have a separate add function for
> each new platform bus type, and change the name of this function to
> explicitly pass the bus type:
> 
> int __platform_device_add(struct platform_device *pdev)
> {
> 	if (!pdev->dev.bus)
> 		return -EINVAL;
> 	[... existing code ...]
> }
> 
> int platform_device_add(struct platform_device *pdev)
> {
>        if (!pdev->dev.parent)
>                pdev->dev.parent = &platform_bus;
>        pdev->dev.bus = &platform_bus_type;
>        __platform_device_add(pdev);
> }
> 
> And then for each bus type (in this example, I'll call it
> foo_bus_type, and assume foo_device wraps platform_device):
> 
> int foo_device_add(struct foo_device *foo)
> {
> 	foo->pdev.dev.bus = &foo_bus_type;
> 	__platform_device_add(&foo->pdev);
> }
> 
> That will allow the new bus_type code to keep foo_bus_type as an
> unexported static and will provide a bit more type safety.  For
> example, it avoids accidentally registering a plain (unwrapped)
> platform device on the foo_bus_type.
> 
> [...snip...]

Yes, this makes sense.  Originally, I was trying to avoid this due
to a misguided notion of backwards compatibility - namely that
devices could register themselves conditionally via

	.bus = HAVE_FOO_BUS,

and always call the same platform_[device|driver]_register, but
thinking about this more, such a compile (or run)-time decision can
just as easily be made in the foo_[device|driver]_register. The
impact on legacy code is the same either way, but your suggested
interface is much better.

I will modify all of these.

> 
>> @@ -1017,6 +1022,87 @@ struct bus_type platform_bus_type = {
>>  };
>>  EXPORT_SYMBOL_GPL(platform_bus_type);
>>
>> +/** pseudo_platform_bus_register - register an "almost platform bus"
> 
> Kerneldoc style error.  Should be:
> 
> +/**
> + * pseudo_platform_bus_register - register an "almost platform bus"
> 

Fixed.

>> + * @bus: partially complete bus type to register
>> + *
>> + * This init will fill in any ommitted fields in @bus, however, it
>> + * also allocates memory and replaces the pm field in @bus, since
>> + * pm is const, but some of its pointers may need this one-time
>> + * initialziation overwrite.
>> + *
>> + * @bus's registered this way should be released with
>> + * pseudo_platform_bus_unregister
>> + */
>> +int pseudo_platform_bus_register(struct bus_type *bus)
>> +{
>> +       int error;
>> +       struct dev_pm_ops* heap_pm;
> 
> Nit: heap_pm is an odd name.  Typically Linux local vars are named
> according to what they are, not where the memory is allocated from.
> How about simply 'ops'
> 

Ah yes. This was deliberate, and to draw attention to it :)

>> +       heap_pm = kmalloc(sizeof (*heap_pm), GFP_KERNEL);
>> +
>> +       if (!heap_pm)
>> +               return -ENOMEM;
>> +
>> +       if (!bus->dev_attrs)
>> +               bus->dev_attrs = platform_bus_type.dev_attrs;
>> +       if (!bus->match)
>> +               bus->match = platform_bus_type.match;
>> +       if (!bus->uevent)
>> +               bus->uevent = platform_bus_type.uevent;
>> +       if (!bus->pm)
>> +               memcpy(heap_pm, &platform_bus_type.pm, sizeof(*heap_pm));
>> +       else {
>> +               heap_pm->prepare = (bus->pm->prepare) ?
>> +                       bus->pm->prepare : platform_pm_prepare;
>> +               heap_pm->complete = (bus->pm->complete) ?
>> +                       bus->pm->complete : platform_pm_complete;
>> +               heap_pm->suspend = (bus->pm->suspend) ?
>> +                       bus->pm->suspend : platform_pm_suspend;
>> +               heap_pm->resume = (bus->pm->resume) ?
>> +                       bus->pm->resume : platform_pm_resume;
>> +               heap_pm->freeze = (bus->pm->freeze) ?
>> +                       bus->pm->freeze : platform_pm_freeze;
>> +               heap_pm->thaw = (bus->pm->thaw) ?
>> +                       bus->pm->thaw : platform_pm_thaw;
>> +               heap_pm->poweroff = (bus->pm->poweroff) ?
>> +                       bus->pm->poweroff : platform_pm_poweroff;
>> +               heap_pm->restore = (bus->pm->restore) ?
>> +                       bus->pm->restore : platform_pm_restore;
>> +               heap_pm->suspend_noirq = (bus->pm->suspend_noirq) ?
>> +                       bus->pm->suspend_noirq : platform_pm_suspend_noirq;
>> +               heap_pm->resume_noirq = (bus->pm->resume_noirq) ?
>> +                       bus->pm->resume_noirq : platform_pm_resume_noirq;
>> +               heap_pm->freeze_noirq = (bus->pm->freeze_noirq) ?
>> +                       bus->pm->freeze_noirq : platform_pm_freeze_noirq;
>> +               heap_pm->thaw_noirq = (bus->pm->thaw_noirq) ?
>> +                       bus->pm->thaw_noirq : platform_pm_thaw_noirq;
>> +               heap_pm->poweroff_noirq = (bus->pm->poweroff_noirq) ?
>> +                       bus->pm->poweroff_noirq : platform_pm_poweroff_noirq;
>> +               heap_pm->restore_noirq = (bus->pm->restore_noirq) ?
>> +                       bus->pm->restore_noirq : platform_pm_restore_noirq;
>> +               heap_pm->runtime_suspend = (bus->pm->runtime_suspend) ?
>> +                       bus->pm->runtime_suspend : platform_pm_runtime_suspend;
>> +               heap_pm->runtime_resume = (bus->pm->runtime_resume) ?
>> +                       bus->pm->runtime_resume : platform_pm_runtime_resume;
>> +               heap_pm->runtime_idle = (bus->pm->runtime_idle) ?
>> +                       bus->pm->runtime_idle : platform_pm_runtime_idle;
>> +       }
>> +       bus->pm = heap_pm;
>> +
>> +       error = bus_register(bus);
>> +       if (error)
>> +               kfree(bus->pm);
>> +
>> +       return error;
>> +}
>> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_register);
> 
> Ick, this is a nasty list of conditional statements.  :-)  Instead it
> could have an unconditional initialize function which sets it up just
> like the platform bus without registering.  Then allow the
> foo_bus_type initialization code override the ones it cares about, and
> then register directly.
> 

No, this won't work. (Unless, every pseudo_bus_type author did this same
kludge to work around const - better to do once IMHO)

struct bus_type {
	...
	const struct dev_pm_ops *pm;
	...
};

That const is there to *prevent* device/driver writers from overriding
pm ops on accident, and to that end, it has value. What we would really
like here is 'const after registered' semantics, where the bus author
can fill in half the structure, and the platform code can fill in the
rest. However, allowing subclasses to modify private data elements isn't
possible in C :)

I believe the 'const' here provides valuable safety. My original attempt
at doing this removed the const, and I overwrote one of the pointers in
platform_dev_pm_ops on my first try at implementing it!

This was the best solution I could come up with while preserving the const
nature of the pm_ops and introducing minimal complexity / potential to
screw up for pseudo-bus-type authors.


>> +
>> +void pseudo_platform_bus_unregister(struct bus_type *bus)
>> +{
>> +       bus_unregister(bus);
>> +       kfree(bus->pm);
>> +}
>> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_unregister);
>> +
>>  int __init platform_bus_init(void)
>>  {
>>        int error;
>> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
>> index 5417944..5ec827c 100644
>> --- a/include/linux/platform_device.h
>> +++ b/include/linux/platform_device.h
>> @@ -79,6 +79,9 @@ extern int platform_driver_probe(struct platform_driver *driver,
>>  #define platform_get_drvdata(_dev)     dev_get_drvdata(&(_dev)->dev)
>>  #define platform_set_drvdata(_dev,data)        dev_set_drvdata(&(_dev)->dev, (data))
>>
>> +extern int pseudo_platform_bus_register(struct bus_type *);
>> +extern void pseudo_platform_bus_unregister(struct bus_type *);
>> +
>>  extern struct platform_device *platform_create_bundle(struct platform_driver *driver,
>>                                        int (*probe)(struct platform_device *),
>>                                        struct resource *res, unsigned int n_res,
>> --
>> 1.7.2
>>
>>
> 
> 
> 


-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Grant Likely]

On Mon, Aug 16, 2010 at 12:47 PM, Patrick Pannuto
<ppannuto@codeaurora.org> wrote:
> On 08/13/2010 03:05 PM, Grant Likely wrote:
>>> @@ -1017,6 +1022,87 @@ struct bus_type platform_bus_type = {
>>>  };
>>>  EXPORT_SYMBOL_GPL(platform_bus_type);
>>>
>>> +/** pseudo_platform_bus_register - register an "almost platform bus"
>>
>> Kerneldoc style error.  Should be:
>>
>> +/**
>> + * pseudo_platform_bus_register - register an "almost platform bus"
>>
>
> Fixed.

Actually, I also made a kerneldoc style error here because the braces
are missing.  This should be:

+/**
+ * pseudo_platform_bus_register() - register an "almost platform bus"

See Documentation/kernel-doc-nano-HOWTO.txt for details.

I'm also not fond of the "pseudo" name.  It isn't really a pseudo bus,
but rather a different bus type that happens to inherit most of the
platform_bus infrastructure.  It may be better just to expose the
platform_bus helper functions without any reference to pseudo, and let
the users be responsible for any naming differentiation.  This is
particularly true if the custom bus becomes responsible for
registering itself and only the initialization functions are exposed.

[...]
>>> +       heap_pm = kmalloc(sizeof (*heap_pm), GFP_KERNEL);
>>> +
>>> +       if (!heap_pm)
>>> +               return -ENOMEM;
>>> +
>>> +       if (!bus->dev_attrs)
>>> +               bus->dev_attrs = platform_bus_type.dev_attrs;
>>> +       if (!bus->match)
>>> +               bus->match = platform_bus_type.match;
>>> +       if (!bus->uevent)
>>> +               bus->uevent = platform_bus_type.uevent;
>>> +       if (!bus->pm)
>>> +               memcpy(heap_pm, &platform_bus_type.pm, sizeof(*heap_pm));
>>> +       else {
>>> +               heap_pm->prepare = (bus->pm->prepare) ?
>>> +                       bus->pm->prepare : platform_pm_prepare;
>>> +               heap_pm->complete = (bus->pm->complete) ?
>>> +                       bus->pm->complete : platform_pm_complete;
>>> +               heap_pm->suspend = (bus->pm->suspend) ?
>>> +                       bus->pm->suspend : platform_pm_suspend;
>>> +               heap_pm->resume = (bus->pm->resume) ?
>>> +                       bus->pm->resume : platform_pm_resume;
>>> +               heap_pm->freeze = (bus->pm->freeze) ?
>>> +                       bus->pm->freeze : platform_pm_freeze;
>>> +               heap_pm->thaw = (bus->pm->thaw) ?
>>> +                       bus->pm->thaw : platform_pm_thaw;
>>> +               heap_pm->poweroff = (bus->pm->poweroff) ?
>>> +                       bus->pm->poweroff : platform_pm_poweroff;
>>> +               heap_pm->restore = (bus->pm->restore) ?
>>> +                       bus->pm->restore : platform_pm_restore;
>>> +               heap_pm->suspend_noirq = (bus->pm->suspend_noirq) ?
>>> +                       bus->pm->suspend_noirq : platform_pm_suspend_noirq;
>>> +               heap_pm->resume_noirq = (bus->pm->resume_noirq) ?
>>> +                       bus->pm->resume_noirq : platform_pm_resume_noirq;
>>> +               heap_pm->freeze_noirq = (bus->pm->freeze_noirq) ?
>>> +                       bus->pm->freeze_noirq : platform_pm_freeze_noirq;
>>> +               heap_pm->thaw_noirq = (bus->pm->thaw_noirq) ?
>>> +                       bus->pm->thaw_noirq : platform_pm_thaw_noirq;
>>> +               heap_pm->poweroff_noirq = (bus->pm->poweroff_noirq) ?
>>> +                       bus->pm->poweroff_noirq : platform_pm_poweroff_noirq;
>>> +               heap_pm->restore_noirq = (bus->pm->restore_noirq) ?
>>> +                       bus->pm->restore_noirq : platform_pm_restore_noirq;
>>> +               heap_pm->runtime_suspend = (bus->pm->runtime_suspend) ?
>>> +                       bus->pm->runtime_suspend : platform_pm_runtime_suspend;
>>> +               heap_pm->runtime_resume = (bus->pm->runtime_resume) ?
>>> +                       bus->pm->runtime_resume : platform_pm_runtime_resume;
>>> +               heap_pm->runtime_idle = (bus->pm->runtime_idle) ?
>>> +                       bus->pm->runtime_idle : platform_pm_runtime_idle;
>>> +       }
>>> +       bus->pm = heap_pm;
>>> +
>>> +       error = bus_register(bus);
>>> +       if (error)
>>> +               kfree(bus->pm);
>>> +
>>> +       return error;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_register);
>>
>> Ick, this is a nasty list of conditional statements.  :-)  Instead it
>> could have an unconditional initialize function which sets it up just
>> like the platform bus without registering.  Then allow the
>> foo_bus_type initialization code override the ones it cares about, and
>> then register directly.
>>
>
> No, this won't work. (Unless, every pseudo_bus_type author did this same
> kludge to work around const - better to do once IMHO)
>
> struct bus_type {
>        ...
>        const struct dev_pm_ops *pm;
>        ...
> };
>
> That const is there to *prevent* device/driver writers from overriding
> pm ops on accident, and to that end, it has value. What we would really
> like here is 'const after registered' semantics, where the bus author
> can fill in half the structure, and the platform code can fill in the
> rest. However, allowing subclasses to modify private data elements isn't
> possible in C :)

Okay then, here is a better approach:  Don't do any allocation in this
function.  Just initialize the bus_type exactly the way it is
initialized for the platform bus and assign the default dev_pm_ops.
If the custom bus needs to override them, then it should be
responsible to kmemdup the default dev_pm_ops structure and modify the
copy.  The code will be a lot simpler that way.

> I believe the 'const' here provides valuable safety. My original attempt
> at doing this removed the const, and I overwrote one of the pointers in
> platform_dev_pm_ops on my first try at implementing it!

Yes, overriding the const is a bad idea and you'd be wrong to override it.  :-)

Cheers,
g.

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Michał Mirosław]

2010/8/16 Patrick Pannuto <ppannuto@codeaurora.org>:
> On 08/13/2010 03:05 PM, Grant Likely wrote:
>> On Tue, Aug 10, 2010 at 5:49 PM, Patrick Pannuto
[...]
>>> + * @bus: partially complete bus type to register
>>> + *
>>> + * This init will fill in any ommitted fields in @bus, however, it
>>> + * also allocates memory and replaces the pm field in @bus, since
>>> + * pm is const, but some of its pointers may need this one-time
>>> + * initialziation overwrite.
>>> + *
>>> + * @bus's registered this way should be released with
>>> + * pseudo_platform_bus_unregister
>>> + */
>>> +int pseudo_platform_bus_register(struct bus_type *bus)
>>> +{
>>> +       int error;
>>> +       struct dev_pm_ops* heap_pm;
>>> +       heap_pm = kmalloc(sizeof (*heap_pm), GFP_KERNEL);
>>> +
>>> +       if (!heap_pm)
>>> +               return -ENOMEM;
>>> +
>>> +       if (!bus->dev_attrs)
>>> +               bus->dev_attrs = platform_bus_type.dev_attrs;
>>> +       if (!bus->match)
>>> +               bus->match = platform_bus_type.match;
>>> +       if (!bus->uevent)
>>> +               bus->uevent = platform_bus_type.uevent;
>>> +       if (!bus->pm)
>>> +               memcpy(heap_pm, &platform_bus_type.pm, sizeof(*heap_pm));
>>> +       else {
>>> +               heap_pm->prepare = (bus->pm->prepare) ?
>>> +                       bus->pm->prepare : platform_pm_prepare;
>>> +               heap_pm->complete = (bus->pm->complete) ?
>>> +                       bus->pm->complete : platform_pm_complete;
[and so on ...]
>>> +               heap_pm->runtime_idle = (bus->pm->runtime_idle) ?
>>> +                       bus->pm->runtime_idle : platform_pm_runtime_idle;
>>> +       }
>>> +       bus->pm = heap_pm;
>>> +
>>> +       error = bus_register(bus);
>>> +       if (error)
>>> +               kfree(bus->pm);
>>> +
>>> +       return error;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pseudo_platform_bus_register);
>>
>> Ick, this is a nasty list of conditional statements.  :-)  Instead it
>> could have an unconditional initialize function which sets it up just
>> like the platform bus without registering.  Then allow the
>> foo_bus_type initialization code override the ones it cares about, and
>> then register directly.
>>
> No, this won't work. (Unless, every pseudo_bus_type author did this same
> kludge to work around const - better to do once IMHO)

Actually you can do:

const struct dev_pm_ops test = {
    DEFAULT_PLATFORM_PM_OPS,
    .prepare = my_func,
    ...
};

where:

#define DEFAULT_PLATFORM_PM_OPS \
    .prepare = platform_pm_prepare, \
    .complete = platform_pm_complete, \
    ...

In case of repeated field assignments, gcc uses the last value (as per
http://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html).

Best Regards,
Michał Mirosław

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Grant Likely]

"Moffett, Kyle D" <Kyle.D.Moffett@boeing.com> wrote:

>On Aug 10, 2010, at 19:49, Patrick Pannuto wrote:
>> As SOCs become more popular, the desire to quickly define a simple,
>> but functional, bus type with only a few unique properties becomes
>> desirable. As they become more complicated, the ability to nest these
>> simple busses and otherwise orchestrate them to match the actual
>> topology also becomes desirable.
>>
>> EXAMPLE USAGE
>>
>> /arch/ARCH/MY_ARCH/my_bus.c:
>>
>> 	#include <linux/device.h>
>> 	#include <linux/platform_device.h>
>>
>> 	struct bus_type SOC_bus_type = {
>> 		.name = "SOC-bus-type",
>> 	};
>> 	EXPORT_SYMBOL_GPL(SOC_bus_type);
>>
>> 	struct platform_device SOC_bus1 = {
>> 		.name	 = "SOC-bus1",
>> 		.id		= -1,
>> 		.dev.bus = &SOC_bus_type;
>> 	};
>> 	EXPORT_SYMBOL_GPL(SOC_bus1);
>>
>> 	struct platform_device SOC_bus2 = {
>> 		.name	 = "SOC-bus2",
>> 		.id		= -2,
>> 		.dev.bus = &SOC_bus_type;
>> 	};
>> 	EXPORT_SYMBOL_GPL(SOC_bus2);
>>
>> 	static int __init SOC_bus_init(void)
>> 	{
>> 		int error;
>>
>> 		error = pseudo_platform_bus_register(&SOC_bus_type);
>> 		if (error)
>> 			return error;
>>
>> 		error = platform_device_register(&SOC_bus1);
>> 		if (error)
>> 			goto fail_bus1;
>>
>> 		error = platform_device_register(&SOC_bus2);
>> 		if (error)
>> 			goto fail_bus2;
>>
>> 		return error;
>>
>> 		/* platform_device_unregister(&SOC_bus2); */
>> fail_bus2:
>> 		platform_device_unregister(&SOC_bus1);
>> fail_bus1:
>> 		pseudo_platform_bus_unregister(&SOC_bus_type);
>>
>> 		return error;
>> 	}
>>
>> /drivers/my_driver.c:
>> 	static struct platform_driver my_driver = {
>> 		.driver = {
>> 			.name	= "my-driver",
>> 			.owner	= THIS_MODULE,
>> 			.bus	= &SOC_bus_type,
>> 		},
>> 	};
>>
>> /somewhere/my_device.c:
>> 	static struct platform_device my_device = {
>> 		.name		= "my-device",
>> 		.id		= -1,
>> 		.dev.bus	= &my_bus_type,
>> 		.dev.parent	= &SOC_bus1.dev,
>> 	};
>>
>> This will build a device tree that mirrors the actual system:
>>
>> /sys/bus
>> |-- SOC-bus-type
>> |   |-- devices
>> |   |   |-- SOC_bus1 -> ../../../devices/SOC_bus1
>> |   |   |-- SOC_bus2 -> ../../../devices/SOC_bus2
>> |   |   |-- my-device -> ../../../devices/SOC_bus1/my-device
>> |   |-- drivers
>> |   |   |-- my-driver
>>
>> /sys/devices
>> |-- SOC_bus1
>> |   |-- my-device
>> |-- SOC_bus2
>>
>> Driver can drive any device on the SOC, which is logical, without
>> actually being registered on multiple /bus_types/, even though the
>> devices may be on different /physical buses/ (which are actually
>> just devices).
>
>Hmm...
>
>To me this seems like a really painful implementation of what the OpenFirmware-esque "Flattened Device Tree" does on many embedded systems.

Patrick and Kevin have been trying to solve a different problem.  The
FDT is really good at describing the topology and interconnections of
the system, but it doesn't solve the problem of how the different bus
behaviour is implemented in the kernel.  They need a method of
registering devices that have subtly different behaviour from the
plain-vanilla platform bus.  Whether or not the device data originates
from the FDT is irrelevant, and the problem remains the same.

I'm not convinced (yet) that this is the right approach, and I'd like
to see a few sample drivers converted to the new approach.  Creating
new bus_types that "inherit" from the platform_bus is actually not a
bad idea, and it is an elegant way to change the behaviour (for
example, how power management is implemented) for devices connected in
a different way.

A problem with the approach that Kevin pointed out is that drivers
that need to work on both the platform_bus_type and the new
soc_bus_type must explicitly register themselves on both bus types.
There is no mechanism to allow drivers from one bus type to also be
made available to another bus type. Certainly it would be possible to
invent a mechanism, but the more I think about it, them more I think
it will be a bad idea.  The runtime-PM use-case that kicked this
discussion off makes the assumption that a driver will behave
identically when attached to either the platform_bus_type or the
soc_bus_type.  However, I think that in the general case that
assumption will prove to be false.  I strongly suspect that the new
bus type will turn out to be not as similar to the platform_bus_type
as originally assumed and that there will still be bus-type-specific
impact on device drivers (but I digress; this paragraph is more
directed to Patrick and Kevin, and doesn't address your comments).

More below...

>For example, to build an equivalent device tree using an OpenFirmware FDT file, I'd just use this:
>
>/dts-v1/;
>/ {
>	#address-cells = <1>;
>	#size-cells = <1>;
>	[...snip...]
>
>	soc-bus-1@fc000000 {
>		/* of_platform driver matches against this: */
>		compatible = "my-company-name,soc-bus-type";
>
>		/* Define base address and size of the bus */
>		reg = <0xfc000000 0x01000000>;
>		#address-cells = <1>;
>		#size-cells = <1>;
>
>		/*
>		 * Define logical memory mapping relative to the bus addr:
>		 * First field is the relative base address for children,
>		 * second field is the address in the parent's memory map,
>		 * third field is the size of the range.
>		 */
>		ranges = <0x0 0xfc000000 0x01000000>;
>
>		/* Now for sub-devices */
>		my-device@0x10000 {
>			compatible = "my-company-name,my-driver";
>			reg = <0x10000 0x100>; /* Address and size */
>		};
>	};
>
>	soc-bus-2@fd000000 {
>		/* of_platform driver matches against this: */
>		compatible = "my-company-name,soc-bus-type";
>
>		/* Define base address and size of the bus */
>		reg = <0xfd000000 0x01000000>;
>		#address-cells = <1>;
>		#size-cells = <1>;
>
>		/*
>		 * Define logical memory mapping relative to the bus addr:
>		 * First field is the relative base address for children,
>		 * second field is the address in the parent's memory map,
>		 * third field is the size of the range.
>		 */
>		ranges = <0x0 0xfd000000 0x01000000>;
>	};
>};
>
>If you don't need to actually do anything special at the bus level, you can just:
>	static struct of_device_id soc_bus_ids[] = {
>		{ .compatible = "soc-bus-type", },
>		{},
>	};
>	of_platform_bus_probe(NULL, &soc_bus_ids, NULL);
>
>Any of_platform driver that matches something on one of those busses is automatically probed.  Alternatively, if you need special bus behavior:
>
>	static struct of_device_id soc_bus_ids[] = {
>		{ .compatible = "soc-bus-type", },
>		{},
>	};
>	static struct of_platform_driver soc_bus_type = {
>		.name = "soc-bus-type",
>		.match_table = &soc_bus_ids,
>		.owner = THIS_MODULE,
>		.probe = mybus_probe,
>		.remove = mybus_remove,
>		.suspend = mybus_suspend,
>		.resume = mybus_resume,
>		.shutdown = mybus_shutdown,
>	};
>
>Then your .probe function actually registers a new OF bus.

Be careful!  This is actually a confusing example.  Linux already has
a concept of a bus_type, and it is not the same as what is shown in
this example.  Part of the problem is that Patrick's explanation leads
the reader to conflate two separate concepts; device topology and
bus_types (but I believe that Patrick understands the difference
between the two).  Topology is the tree of struct devices in the LInux
device model (specified by the .parent pointer).  It is represented by
the /sys/devices/* directory tree.

bus_type groups devices that use the same bus infrastructure, but it
says nothing about topology.  bus_types are represented by the
/sys/bus directory.  For example, a device on the i2c_bus_type must be
an i2c_device, is accessed with the i2c infrastructure, and can be
bound to an i2c_driver.  There can be multiple physical i2c busses in
a system, but all i2c_devices are grouped together in
/sys/bus/i2c/devices (as symlinks to the real location in
/sys/devices).

Using the name "soc-bus-type" in this example makes it easy to confuse
this of_platform_driver with an actual bus_type.

BTW, you'll probably be interested to know that as of about a week
ago, Linus pulled my tree which replaces of_platform_bus_type with the
regular platform_bus_type.  There no longer is any differentiation
between OF and non-OF devices.  Any device may have a pointer to an OF
device tree node regardless of bus_type.  Existing of_platform_drivers
do still work, but a shim is used to register them onto the platform
driver.  Next step is to convert existing of_platform_drivers (like
your example above) into normal platform_drivers.

>The best part is... all devices registered as "of_platform" devices can be used to support many entirely different board models from the exact same kernel.
>
>Fully commented and with actual physical addresses in, my FDT example is comparable to your sample code.  Furthermore, all of the error handling is automatically done, a bunch of device drivers are already ported over, and all the kinks regarding interrupts, etc are already taken care of.  I highly recommend taking a look to see if you can use the very nice existing OF bus code to solve your problem instead of writing yet another half-hard-coded platform bus type.
>
>Cheers,
>Kyle Moffett
>

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Kevin Hilman]

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

> I'm not convinced (yet) that this is the right approach, and I'd like
> to see a few sample drivers converted to the new approach.  Creating
> new bus_types that "inherit" from the platform_bus is actually not a
> bad idea, and it is an elegant way to change the behaviour (for
> example, how power management is implemented) for devices connected in
> a different way.
>
> A problem with the approach that Kevin pointed out is that drivers
> that need to work on both the platform_bus_type and the new
> soc_bus_type must explicitly register themselves on both bus types.

And potentially more than 2 if a driver exists for a hardware block on
different SoCs.  Magnus raised this issue for drivers used across SH and
ARM/SH-Mobile, and the same issue exists between TI OMAP and TI DaVinci
SoCs.

> There is no mechanism to allow drivers from one bus type to also be
> made available to another bus type. Certainly it would be possible to
> invent a mechanism, but the more I think about it, them more I think
> it will be a bad idea.  The runtime-PM use-case that kicked this
> discussion off makes the assumption that a driver will behave
> identically when attached to either the platform_bus_type or the
> soc_bus_type.  However, I think that in the general case that
> assumption will prove to be false.  I strongly suspect that the new
> bus type will turn out to be not as similar to the platform_bus_type
> as originally assumed and that there will still be bus-type-specific
> impact on device drivers

What makes you suspect that?  Maybe Patrick has other use-cases in mind,
but from a PM perspective, I have not seen any impact.

One of the primary goals of this (at least for me and it seems Magnus also)
is to keep drivers ignorant of their bus type, and any bus-type-specific
code should be done in the bus_type implementation.

Both for SH and OMAP, we've been using the (admiteddly broken)
weak-symbol-override approach to getting a custom bus-type based on the
platform_bus.  We've been using that in OMAP for a while now and have
not seen any need to for the drivers to know if they are on the vanilla
platform_bus or the customized one.

I'm very curious to hear what type of impact you expect to the drivers.

Kevin

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Grant Likely]

On Thu, Aug 19, 2010 at 1:20 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
>> There is no mechanism to allow drivers from one bus type to also be
>> made available to another bus type. Certainly it would be possible to
>> invent a mechanism, but the more I think about it, them more I think
>> it will be a bad idea.  The runtime-PM use-case that kicked this
>> discussion off makes the assumption that a driver will behave
>> identically when attached to either the platform_bus_type or the
>> soc_bus_type.  However, I think that in the general case that
>> assumption will prove to be false.  I strongly suspect that the new
>> bus type will turn out to be not as similar to the platform_bus_type
>> as originally assumed and that there will still be bus-type-specific
>> impact on device drivers
>
> What makes you suspect that?  Maybe Patrick has other use-cases in mind,
> but from a PM perspective, I have not seen any impact.

Call it a gut reaction if you like, but here's my line of thinking.
In your use case, sure, there is zero impact on drivers, but will that
continue to be the case for other bus architectures as they use this
interface?  Or, in other words, will this solution end up being short
sighted and we'll be having this discussion all over again in a year?

The big concern for me is the idea of partial divergence from the
platform_bus_type (specifically having different bus_type, but same
pool of drivers).  For starts, how is it represented in the device
model?  Where do the device drivers live in /sys/bus/*/drivers?  What
happens when one bus type is unregistered but has drivers used by
another bus type?  Does it scale to the (granted, currently
speculative) use-case where the device driver does need knowledge
about which bus_type it is registered against?  I'm also concerned
about the complexity required to implement driver sharing and ensuring
that correct locking and consistency is maintained between bus_types.

Note: I consider partial divergence to be different from
platform_bus_type inheritance which Patrick has been implementing.  I
don't have a technical issue with inheritance (drivers aren't shared),
I'm just not convinced it is the best solution yet.  :-)

My other thought is this:  If from both the device perspective and the
driver perspective the interface is exactly the same as the
platform_bus_type, then platform_bus_type really is the correct
interface.  That says to me that putting the runtime PM behaviour into
a new bus_type is abstracting at the wrong level, and a different
method should be used to modify PM behaviour, either based on device
model topology (ie, the parent implement's the child's runtime PM
behaviour), or maybe a "struct dev_pm_ops *bus_pm" in the struct
device.

However, the number of device drivers affected is still very small,
and I don't have concerns if each driver registers itself on multiple
bus types explicitly.  For example:

static struct platform_driver omap2_mcspi_platform_driver = {
        .driver = {
                .name =         "omap2_mcspi",
                .owner =        THIS_MODULE,
        },
        .probe = omap2_mcspi_probe,
        .remove =       __exit_p(omap2_mcspi_remove),
};
static struct platform_driver omap2_mcspi_omap_driver = {
        .driver = {
                .name =         "omap2_mcspi",
                .owner =        THIS_MODULE,
        },
        .probe = omap2_mcspi_probe,
        .remove =       __exit_p(omap2_mcspi_remove),
};

static int __init omap2_mcspi_init(void)
{
        platform_driver_register(&omap2_mcspi_platform_driver);
        omap_driver_register(&omap2_mcspi_omap_driver);
}
module_init(omap2_mcspi_init);

I'd even be okay with a helper function that kmemdups() a
platform_driver and registers it onto multiple busses.  Then if the
driver ever really does need to be differentiated, then the driver can
stop using the helper function and provide a different probe hook for
each registration.

> One of the primary goals of this (at least for me and it seems Magnus also)
> is to keep drivers ignorant of their bus type, and any bus-type-specific
> code should be done in the bus_type implementation.

Heh; which just screams to me that bus_type is the wrong level to be
abstracting the behaviour (but I also understand your need for the
"omap_device" wrapper around platform_device which also requires some
method to sort out when a platform_device really is an omap_device
without an unsafe dereference).

> Both for SH and OMAP, we've been using the (admiteddly broken)
> weak-symbol-override approach to getting a custom bus-type based on the
> platform_bus.  We've been using that in OMAP for a while now and have
> not seen any need to for the drivers to know if they are on the vanilla
> platform_bus or the customized one.
>
> I'm very curious to hear what type of impact you expect to the drivers.

My fears on this point may very well be unfounded.  This isn't the
hill I'm going to die on either.  Show me an implementation of driver
sharing that is clean and prove me wrong!  :-)

Cheers,
g.

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

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Kevin Hilman]

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

[...]

>> One of the primary goals of this (at least for me and it seems Magnus also)
>> is to keep drivers ignorant of their bus type, and any bus-type-specific
>> code should be done in the bus_type implementation.
>
> Heh; which just screams to me that bus_type is the wrong level to be
> abstracting the behaviour 

Heh, now I feel like we're going around in circles.  Remember, I never
wanted to create add a new bus_type.  Someone else [ahem] suggested
doing the abstraction at the bus_type level.  ;)

> (but I also understand your need for the
> "omap_device" wrapper around platform_device which also requires some
> method to sort out when a platform_device really is an omap_device
> without an unsafe dereference).

Yes, I'm working on the devres approach to that now, as is Magnus for
the sh-mobile version (proposed for .36-rc1[1])

>> Both for SH and OMAP, we've been using the (admiteddly broken)
>> weak-symbol-override approach to getting a custom bus-type based on the
>> platform_bus.  We've been using that in OMAP for a while now and have
>> not seen any need to for the drivers to know if they are on the vanilla
>> platform_bus or the customized one.
>>
>> I'm very curious to hear what type of impact you expect to the drivers.
>
> My fears on this point may very well be unfounded.  This isn't the
> hill I'm going to die on either.  Show me an implementation of driver
> sharing that is clean and prove me wrong!  :-)

IMHO, simply overriding the few dev_pm_ops methods was the cleanest and
simplest.

Since we seem to be in agreement now that the a new bus may not the
right abstraction for this (since we want it to be completely
transparent to the drivers), I'll go back to the original design.  No new
bus types, keep the platform_bus as is, but simply override the few
dev_pm_ops methods I care about.  This is what is done on SH,
SH-Mobile[1] and my original version for OMAP that started this
conversation.

Yes, the weak-symbol method of overriding is not scalable, but that's a
separate issue from whether or not to create a new bus.  I have a
proposed fix for the weak which I'll post shortly.

Kevin

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022411.html

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Grant Likely]

On Fri, Aug 20, 2010 at 12:51 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
> [...]
>
>>> One of the primary goals of this (at least for me and it seems Magnus also)
>>> is to keep drivers ignorant of their bus type, and any bus-type-specific
>>> code should be done in the bus_type implementation.
>>
>> Heh; which just screams to me that bus_type is the wrong level to be
>> abstracting the behaviour
>
> Heh, now I feel like we're going around in circles.  Remember, I never
> wanted to create add a new bus_type.  Someone else [ahem] suggested
> doing the abstraction at the bus_type level.  ;)

Hey!  I didn't suggest it either!  I believe that was Greg at ELC.  I
just... um... kind of took it and ran with it.  :-)

>> (but I also understand your need for the
>> "omap_device" wrapper around platform_device which also requires some
>> method to sort out when a platform_device really is an omap_device
>> without an unsafe dereference).
>
> Yes, I'm working on the devres approach to that now, as is Magnus for
> the sh-mobile version (proposed for .36-rc1[1])
>
>>> Both for SH and OMAP, we've been using the (admiteddly broken)
>>> weak-symbol-override approach to getting a custom bus-type based on the
>>> platform_bus.  We've been using that in OMAP for a while now and have
>>> not seen any need to for the drivers to know if they are on the vanilla
>>> platform_bus or the customized one.
>>>
>>> I'm very curious to hear what type of impact you expect to the drivers.
>>
>> My fears on this point may very well be unfounded.  This isn't the
>> hill I'm going to die on either.  Show me an implementation of driver
>> sharing that is clean and prove me wrong!  :-)
>
> IMHO, simply overriding the few dev_pm_ops methods was the cleanest and
> simplest.
>
> Since we seem to be in agreement now that the a new bus may not the
> right abstraction for this (since we want it to be completely
> transparent to the drivers), I'll go back to the original design.  No new
> bus types, keep the platform_bus as is, but simply override the few
> dev_pm_ops methods I care about.  This is what is done on SH,
> SH-Mobile[1] and my original version for OMAP that started this
> conversation.
>
> Yes, the weak-symbol method of overriding is not scalable, but that's a
> separate issue from whether or not to create a new bus.  I have a
> proposed fix for the weak which I'll post shortly.

Okay.

One constraint remains though:  If you can override the dev_pm_ops on
a per-device or per-device-parent basis, then you've got my support.
If the override (even when fixed to work at runtime) applies to every
device on the platform_bus_type, then I'll nack it.  My concern here
is that the SoC or platform support code doesn't get to "own" the
platform_bus_type.  Other drivers/code can register their own set of
platform_devices, which may also need to perform their own dev_pm_ops
override.  If the override is global to the platform_bus_type, then
the model will not scale.

Cheers,
g.

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Kevin Hilman]

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

[...]

>>>
>>> My fears on this point may very well be unfounded.  This isn't the
>>> hill I'm going to die on either.  Show me an implementation of driver
>>> sharing that is clean and prove me wrong!  :-)
>>
>> IMHO, simply overriding the few dev_pm_ops methods was the cleanest and
>> simplest.
>>
>> Since we seem to be in agreement now that the a new bus may not the
>> right abstraction for this (since we want it to be completely
>> transparent to the drivers), I'll go back to the original design.  No new
>> bus types, keep the platform_bus as is, but simply override the few
>> dev_pm_ops methods I care about.  This is what is done on SH,
>> SH-Mobile[1] and my original version for OMAP that started this
>> conversation.
>>
>> Yes, the weak-symbol method of overriding is not scalable, but that's a
>> separate issue from whether or not to create a new bus.  I have a
>> proposed fix for the weak which I'll post shortly.
>
> Okay.
>
> One constraint remains though:  If you can override the dev_pm_ops on
> a per-device or per-device-parent basis, then you've got my support.

hmm, a new requirement?, and one that would require some significant
changes to the driver model.

Currently, dev_pm_ops for a bus applies globally to *all* devices on
that bus (or class or type) and changing that would require changing the
platform_bus code to start having per-device (or per-parent-device)
checks. 

> If the override (even when fixed to work at runtime) applies to every
> device on the platform_bus_type, then I'll nack it.  

/me can't help but wonder why the OMAP implementation is getting all the
negative attention while the other identical implementations are already
upstream.

> My concern here is that the SoC or platform support code doesn't get
> to "own" the platform_bus_type.

Well, I'm not proposing that here.  This "feature" already exists in
mainline (albeit using the less-than-optimal weak symbol approach.)  All
I'm proposing is fixing it to make it multi-arch friendly.

If you think this behavior should be changed to non global, that should
be done a separate series since it is not directly related to runtime PM
support for a given platform, IMO.

> Other drivers/code can register their own set of
> platform_devices, which may also need to perform their own dev_pm_ops
> override.  

IMHO, we should deal with such hypothetical, future problems *if* they
arise down the road.

> If the override is global to the platform_bus_type, then the model
> will not scale.

It will not scale any more (or less) than the current functionality of
the driver model which handles this globally.  Again, if scalabilty
becomes a problem down the road, lets fix it then.

Kevin

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Grant Likely]

On Fri, Aug 20, 2010 at 6:10 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
> [...]
>
>>>>
>>>> My fears on this point may very well be unfounded.  This isn't the
>>>> hill I'm going to die on either.  Show me an implementation of driver
>>>> sharing that is clean and prove me wrong!  :-)
>>>
>>> IMHO, simply overriding the few dev_pm_ops methods was the cleanest and
>>> simplest.
>>>
>>> Since we seem to be in agreement now that the a new bus may not the
>>> right abstraction for this (since we want it to be completely
>>> transparent to the drivers), I'll go back to the original design.  No new
>>> bus types, keep the platform_bus as is, but simply override the few
>>> dev_pm_ops methods I care about.  This is what is done on SH,
>>> SH-Mobile[1] and my original version for OMAP that started this
>>> conversation.
>>>
>>> Yes, the weak-symbol method of overriding is not scalable, but that's a
>>> separate issue from whether or not to create a new bus.  I have a
>>> proposed fix for the weak which I'll post shortly.
>>
>> Okay.
>>
>> One constraint remains though:  If you can override the dev_pm_ops on
>> a per-device or per-device-parent basis, then you've got my support.
>
> hmm, a new requirement?, and one that would require some significant
> changes to the driver model.

Nope!  Not a new requirement; this is the issue that this entire
thread has been about.  Hijacking the entire platform_bus_type
behaviour for a particular bus is the wrong thing to do because there
are other users in the same system.

> Currently, dev_pm_ops for a bus applies globally to *all* devices on
> that bus (or class or type) and changing that would require changing the
> platform_bus code to start having per-device (or per-parent-device)
> checks.

I'm not opposed to modifying the platform_bus_type to support the use-case.

>> If the override (even when fixed to work at runtime) applies to every
>> device on the platform_bus_type, then I'll nack it.
>
> /me can't help but wonder why the OMAP implementation is getting all the
> negative attention while the other identical implementations are already
> upstream.

OMAP is not getting singled out.  This applies to all the implementations.

>> My concern here is that the SoC or platform support code doesn't get
>> to "own" the platform_bus_type.
>
> Well, I'm not proposing that here.  This "feature" already exists in
> mainline (albeit using the less-than-optimal weak symbol approach.)  All
> I'm proposing is fixing it to make it multi-arch friendly.

Oops, I didn't phrase that very well.  What I meant was that SoC or
platform support code must not be allowed to "own" or hijack the
platform_bus_type for it's own purposes.  What I wrote sounds like I
was suggesting that as a good thing.

> If you think this behavior should be changed to non global, that should
> be done a separate series since it is not directly related to runtime PM
> support for a given platform, IMO.
>
>> Other drivers/code can register their own set of
>> platform_devices, which may also need to perform their own dev_pm_ops
>> override.
>
> IMHO, we should deal with such hypothetical, future problems *if* they
> arise down the road.
>
>> If the override is global to the platform_bus_type, then the model
>> will not scale.
>
> It will not scale any more (or less) than the current functionality of
> the driver model which handles this globally.  Again, if scalabilty
> becomes a problem down the road, lets fix it then.

Alright, I can agree to that.  I do agree that the runtime override is
far and away better than the weak symbol approach.  However, there
needs to be some very clear rules in place for users of the override,
namely that users must understand that they are stewards of the
platform_bus_type, and must take care to preserve the default
behaviour for "uninteresting" devices.

Also, the expectation should be that it is a temporary measure until a
better abstraction is implemented.  It might be that a separate
bus_type is the way to go (if the multiple driver registration problem
can be solved) or it might be a way to differentiate pm_ops either
per-device or per-parent.  I'm not sure, but I'm starting on an OMAP
project soon, so I may very well end up working on this.

Cheers,
g.

Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses

[Kevin Hilman]

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

>>> If the override is global to the platform_bus_type, then the model
>>> will not scale.
>>
>> It will not scale any more (or less) than the current functionality of
>> the driver model which handles this globally.  Again, if scalabilty
>> becomes a problem down the road, lets fix it then.
>
> Alright, I can agree to that.  I do agree that the runtime override is
> far and away better than the weak symbol approach.  However, there
> needs to be some very clear rules in place for users of the override,
> namely that users must understand that they are stewards of the
> platform_bus_type, and must take care to preserve the default
> behaviour for "uninteresting" devices.

Completely agree here.

Any overrides of the dev_pm_ops functions that do not also call the
pm_generic_* functions (or their equivalents) should be suspect.

I'll post a proposal for a runtime override shortly.

> Also, the expectation should be that it is a temporary measure until a
> better abstraction is implemented.  It might be that a separate
> bus_type is the way to go (if the multiple driver registration problem
> can be solved) or it might be a way to differentiate pm_ops either
> per-device or per-parent.  I'm not sure, but I'm starting on an OMAP
> project soon, so I may very well end up working on this.

Yes, I'll be glad to work on this too, but first I need to get through
reviewing the backlog of all the OMAP drivers we're converting to
runtime PM.

Kevin