Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Patrick Pannuto]

Mis-typed omap's list; sorry for the resend

On 08/03/2010 04:35 PM, Patrick Pannuto wrote:
> Inspiration for this comes from:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
> 
> 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 my_bus_type = {
> 		.name	= "mybus",
> 	};
> 	EXPORT_SYMBOL_GPL(my_bus_type);
> 
> 	struct platform_device sub_bus1 = {
> 		.name		= "sub_bus1",
> 		.id		= -1,
> 		.dev.bus	= &my_bus_type,
> 	}
> 	EXPORT_SYMBOL_GPL(sub_bus1);
> 
> 	struct platform_device sub_bus2 = {
> 		.name		= "sub_bus2",
> 		.id		= -1,
> 		.dev.bus	= &my_bus_type,
> 	}
> 	EXPORT_SYMBOL_GPL(sub_bus2);
> 
> 	static int __init my_bus_init(void)
> 	{
> 		int error;
> 		platform_bus_type_init(&my_bus_type);
> 
> 		error = bus_register(&my_bus_type);
> 		if (error)
> 			return error;
> 
> 		error = platform_device_register(&sub_bus1);
> 		if (error)
> 			goto fail_sub_bus1;
> 
> 		error = platform_device_register(&sub_bus2);
> 		if (error)
> 			goto fail_sub_bus2;
> 
> 		return error;
> 
> 	fail_sub_bus2:
> 		platform_device_unregister(&sub_bus1);
> 	fail_sub_bus2:
> 		bus_unregister(&my_bus_type);
> 
> 		return error;
> 	}
> 	postcore_initcall(my_bus_init);
> 	EXPORT_SYMBOL_CPY(my_bus_init);
> 
> /drivers/my_driver.c
> 	static struct platform_driver my_driver = {
> 		.driver	= {
> 			.name	= "my-driver",
> 			.owner	= THIS_MODULE,
> 			.bus	= &my_bus_type,
> 		},
> 	};
> 
> /somewhere/my_device.c
> 	static struct platform_device my_device = {
> 		.name		= "my-device",
> 		.id		= -1,
> 		.dev.bus	= &my_bus_type,
> 		.dev.parent	= &sub_bus_1.dev,
> 	};
> 
> Notice that for a device / driver, only 3 lines were added to
> switch from the platform bus to the new my_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 my_bus. The above
> code then becomes:
> 
> 	(possibly in a header)
> 	#ifdef CONFIG_MY_BUS
> 	#define MY_BUS_TYPE	&my_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.
> 
> This will build a device tree that mirrors the actual configuration:
>     /sys/bus
>     |-- my_bus
>     |   |-- devices
>     |   |   |-- sub_bus1 -> ../../../devices/platform/sub_bus1
>     |   |   |-- sub_bus2 -> ../../../devices/platform/sub_bus2
>     |   |   |-- my-device -> ../../../devices/platform/sub_bus1/my-device
>     |   |-- drivers
>     |   |   |-- my-driver
> 
> I believe this to be a fairly elegant and simple solution to the
> problem, but humbly RFC
> 
> -Pat
> 
> Signed-off-by: Patrick Pannuto <ppannuto@codeaurora.org>
> ---
>  drivers/base/platform.c         |   30 ++++++++++++++++++++++++++----
>  include/linux/platform_device.h |    2 ++
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4d99c8b..c86be03 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -241,7 +241,8 @@ int platform_device_add(struct platform_device *pdev)
>  	if (!pdev->dev.parent)
>  		pdev->dev.parent = &platform_bus;
>  
> -	pdev->dev.bus = &platform_bus_type;
> +	if (!pdev->dev.bus)
> +		pdev->dev.bus = &platform_bus_type;
>  
>  	if (pdev->id != -1)
>  		dev_set_name(&pdev->dev, "%s.%d", pdev->name,  pdev->id);
> @@ -482,7 +483,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)
> @@ -539,12 +541,12 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
>  	 * if the probe was successful, and make sure any forced probes of
>  	 * new devices fail.
>  	 */
> -	spin_lock(&platform_bus_type.p->klist_drivers.k_lock);
> +	spin_lock(&drv->driver.bus->p->klist_drivers.k_lock);
>  	drv->probe = NULL;
>  	if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list))
>  		retval = -ENODEV;
>  	drv->driver.probe = platform_drv_probe_fail;
> -	spin_unlock(&platform_bus_type.p->klist_drivers.k_lock);
> +	spin_unlock(&drv->driver.bus->p->klist_drivers.k_lock);
>  
>  	if (code != retval)
>  		platform_driver_unregister(drv);
> @@ -1017,6 +1019,26 @@ struct bus_type platform_bus_type = {
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>  
> +/** platform_bus_type_init - fill in a pseudo-platform-bus
> +  * @bus: foriegn bus type
> +  *
> +  * This init is basically a selective memcpy that
> +  * won't overwrite any user-defined attributes and
> +  * only copies things that platform bus defines anyway
> +  */
> +void platform_bus_type_init(struct bus_type *bus)
> +{
> +	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)
> +		bus->pm = platform_bus_type.pm;
> +}
> +EXPORT_SYMBOL_GPL(platform_bus_type_init);
> +
>  int __init platform_bus_init(void)
>  {
>  	int error;
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 5417944..fa8c35a 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -79,6 +79,8 @@ 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 void platform_bus_type_init(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: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Greg KH]

On Tue, Aug 03, 2010 at 04:35:06PM -0700, Patrick Pannuto wrote:
> Inspiration for this comes from:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
> 
> 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 my_bus_type = {
> 		.name	= "mybus",
> 	};
> 	EXPORT_SYMBOL_GPL(my_bus_type);
> 
> 	struct platform_device sub_bus1 = {
> 		.name		= "sub_bus1",
> 		.id		= -1,
> 		.dev.bus	= &my_bus_type,
> 	}
> 	EXPORT_SYMBOL_GPL(sub_bus1);

You really want a bus hanging off of a bus?  Normally you need a device
to do that, which is what I think you have here, but the naming is a bit
odd to me.

What would you do with this "sub bus"?  It's just a device, but you are
wanting it to be around for something.

> 
> 	struct platform_device sub_bus2 = {
> 		.name		= "sub_bus2",
> 		.id		= -1,
> 		.dev.bus	= &my_bus_type,
> 	}
> 	EXPORT_SYMBOL_GPL(sub_bus2);
> 
> 	static int __init my_bus_init(void)
> 	{
> 		int error;
> 		platform_bus_type_init(&my_bus_type);
> 
> 		error = bus_register(&my_bus_type);
> 		if (error)
> 			return error;
> 
> 		error = platform_device_register(&sub_bus1);
> 		if (error)
> 			goto fail_sub_bus1;
> 
> 		error = platform_device_register(&sub_bus2);
> 		if (error)
> 			goto fail_sub_bus2;
> 
> 		return error;
> 
> 	fail_sub_bus2:
> 		platform_device_unregister(&sub_bus1);
> 	fail_sub_bus2:
> 		bus_unregister(&my_bus_type);
> 
> 		return error;
> 	}
> 	postcore_initcall(my_bus_init);
> 	EXPORT_SYMBOL_CPY(my_bus_init);
> 
> /drivers/my_driver.c
> 	static struct platform_driver my_driver = {
> 		.driver	= {
> 			.name	= "my-driver",
> 			.owner	= THIS_MODULE,
> 			.bus	= &my_bus_type,
> 		},
> 	};
> 
> /somewhere/my_device.c
> 	static struct platform_device my_device = {
> 		.name		= "my-device",
> 		.id		= -1,
> 		.dev.bus	= &my_bus_type,
> 		.dev.parent	= &sub_bus_1.dev,
> 	};

Ah, you put devices on this "sub bus".  But why?  Why not just put it on
your "normal" bus that you created?  What's with the extra level of
nesting here?

Other than that, it looks like a nice idea, are there portions of kernel
code that can be simplified because of this?

> @@ -539,12 +541,12 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
>  	 * if the probe was successful, and make sure any forced probes of
>  	 * new devices fail.
>  	 */
> -	spin_lock(&platform_bus_type.p->klist_drivers.k_lock);
> +	spin_lock(&drv->driver.bus->p->klist_drivers.k_lock);
>  	drv->probe = NULL;
>  	if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list))
>  		retval = -ENODEV;
>  	drv->driver.probe = platform_drv_probe_fail;
> -	spin_unlock(&platform_bus_type.p->klist_drivers.k_lock);
> +	spin_unlock(&drv->driver.bus->p->klist_drivers.k_lock);
>  
>  	if (code != retval)
>  		platform_driver_unregister(drv);

I'm guessing that this chunk can be applied now, right?

> @@ -1017,6 +1019,26 @@ struct bus_type platform_bus_type = {
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>  
> +/** platform_bus_type_init - fill in a pseudo-platform-bus
> +  * @bus: foriegn bus type
> +  *
> +  * This init is basically a selective memcpy that
> +  * won't overwrite any user-defined attributes and
> +  * only copies things that platform bus defines anyway
> +  */
> +void platform_bus_type_init(struct bus_type *bus)
> +{
> +	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)
> +		bus->pm = platform_bus_type.pm;

Watch out for things in "write only" memory here.  That could cause
problems.

thanks,

greg k-h

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Patrick Pannuto]

On 08/03/2010 04:56 PM, Greg KH wrote:
> On Tue, Aug 03, 2010 at 04:35:06PM -0700, Patrick Pannuto wrote:
>> Inspiration for this comes from:
>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
>>
>> 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 my_bus_type = {
>> 		.name	= "mybus",
>> 	};
>> 	EXPORT_SYMBOL_GPL(my_bus_type);
>>
>> 	struct platform_device sub_bus1 = {
>> 		.name		= "sub_bus1",
>> 		.id		= -1,
>> 		.dev.bus	= &my_bus_type,
>> 	}
>> 	EXPORT_SYMBOL_GPL(sub_bus1);
> 
> You really want a bus hanging off of a bus?  Normally you need a device
> to do that, which is what I think you have here, but the naming is a bit
> odd to me.
> 
> What would you do with this "sub bus"?  It's just a device, but you are
> wanting it to be around for something.
> 

It's for power management stuff, basically, there are actual physical buses
involved that can be completely powered off IFF all of their devices are
not in use. Plus it actually matches bus topology this way.

>>
>> 	struct platform_device sub_bus2 = {
>> 		.name		= "sub_bus2",
>> 		.id		= -1,
>> 		.dev.bus	= &my_bus_type,
>> 	}
>> 	EXPORT_SYMBOL_GPL(sub_bus2);
>>
>> 	static int __init my_bus_init(void)
>> 	{
>> 		int error;
>> 		platform_bus_type_init(&my_bus_type);
>>
>> 		error = bus_register(&my_bus_type);
>> 		if (error)
>> 			return error;
>>
>> 		error = platform_device_register(&sub_bus1);
>> 		if (error)
>> 			goto fail_sub_bus1;
>>
>> 		error = platform_device_register(&sub_bus2);
>> 		if (error)
>> 			goto fail_sub_bus2;
>>
>> 		return error;
>>
>> 	fail_sub_bus2:
>> 		platform_device_unregister(&sub_bus1);
>> 	fail_sub_bus2:
>> 		bus_unregister(&my_bus_type);
>>
>> 		return error;
>> 	}
>> 	postcore_initcall(my_bus_init);
>> 	EXPORT_SYMBOL_CPY(my_bus_init);
>>
>> /drivers/my_driver.c
>> 	static struct platform_driver my_driver = {
>> 		.driver	= {
>> 			.name	= "my-driver",
>> 			.owner	= THIS_MODULE,
>> 			.bus	= &my_bus_type,
>> 		},
>> 	};
>>
>> /somewhere/my_device.c
>> 	static struct platform_device my_device = {
>> 		.name		= "my-device",
>> 		.id		= -1,
>> 		.dev.bus	= &my_bus_type,
>> 		.dev.parent	= &sub_bus_1.dev,
>> 	};
> 
> Ah, you put devices on this "sub bus".  But why?  Why not just put it on
> your "normal" bus that you created?  What's with the extra level of
> nesting here?
> 
> Other than that, it looks like a nice idea, are there portions of kernel
> code that can be simplified because of this?

See above.

> 
>> @@ -539,12 +541,12 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
>>  	 * if the probe was successful, and make sure any forced probes of
>>  	 * new devices fail.
>>  	 */
>> -	spin_lock(&platform_bus_type.p->klist_drivers.k_lock);
>> +	spin_lock(&drv->driver.bus->p->klist_drivers.k_lock);
>>  	drv->probe = NULL;
>>  	if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list))
>>  		retval = -ENODEV;
>>  	drv->driver.probe = platform_drv_probe_fail;
>> -	spin_unlock(&platform_bus_type.p->klist_drivers.k_lock);
>> +	spin_unlock(&drv->driver.bus->p->klist_drivers.k_lock);
>>  
>>  	if (code != retval)
>>  		platform_driver_unregister(drv);
> 
> I'm guessing that this chunk can be applied now, right?

Probably; right now this will always work since anything that
is a platform_driver will have .driver.bus = &platform_bus_type,
but that does change with this patch.

There is no need (IMHO) for it to be a separate patch.

> 
>> @@ -1017,6 +1019,26 @@ struct bus_type platform_bus_type = {
>>  };
>>  EXPORT_SYMBOL_GPL(platform_bus_type);
>>  
>> +/** platform_bus_type_init - fill in a pseudo-platform-bus
>> +  * @bus: foriegn bus type
>> +  *
>> +  * This init is basically a selective memcpy that
>> +  * won't overwrite any user-defined attributes and
>> +  * only copies things that platform bus defines anyway
>> +  */
>> +void platform_bus_type_init(struct bus_type *bus)
>> +{
>> +	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)
>> +		bus->pm = platform_bus_type.pm;
> 
> Watch out for things in "write only" memory here.  That could cause
> problems.

Pardon my ignorance (I'm quite new to kernel work), what do you mean
here? What memory could be "write only"?

> 
> thanks,
> 
> greg k-h


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

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Greg KH]

On Tue, Aug 03, 2010 at 05:02:29PM -0700, Patrick Pannuto wrote:
> On 08/03/2010 04:56 PM, Greg KH wrote:
> > On Tue, Aug 03, 2010 at 04:35:06PM -0700, Patrick Pannuto wrote:
> >> Inspiration for this comes from:
> >> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
> >>
> >> 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 my_bus_type = {
> >> 		.name	= "mybus",
> >> 	};
> >> 	EXPORT_SYMBOL_GPL(my_bus_type);

For your question below, this could be in write-only memory.  Well, I
guess it never is as we modify things in the bus structure, so nevermind
about that, false alarm.

> >>
> >> 	struct platform_device sub_bus1 = {
> >> 		.name		= "sub_bus1",
> >> 		.id		= -1,
> >> 		.dev.bus	= &my_bus_type,
> >> 	}
> >> 	EXPORT_SYMBOL_GPL(sub_bus1);
> > 
> > You really want a bus hanging off of a bus?  Normally you need a device
> > to do that, which is what I think you have here, but the naming is a bit
> > odd to me.
> > 
> > What would you do with this "sub bus"?  It's just a device, but you are
> > wanting it to be around for something.
> > 
> 
> It's for power management stuff, basically, there are actual physical buses
> involved that can be completely powered off IFF all of their devices are
> not in use. Plus it actually matches bus topology this way.

Then create a real bus hanging off of a device, not another device that
"acts" like a bus here, right?  Or am I missing the point?

> >> +void platform_bus_type_init(struct bus_type *bus)
> >> +{
> >> +	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)
> >> +		bus->pm = platform_bus_type.pm;
> > 
> > Watch out for things in "write only" memory here.  That could cause
> > problems.
> 
> Pardon my ignorance (I'm quite new to kernel work), what do you mean
> here? What memory could be "write only"?

See above.  I was thinking that struct bus would be a constant or
something.  Sorry.

greg k-h

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Patrick Pannuto]

>>>>
>>>> 	struct platform_device sub_bus1 = {
>>>> 		.name		= "sub_bus1",
>>>> 		.id		= -1,
>>>> 		.dev.bus	= &my_bus_type,
>>>> 	}
>>>> 	EXPORT_SYMBOL_GPL(sub_bus1);
>>>
>>> You really want a bus hanging off of a bus?  Normally you need a device
>>> to do that, which is what I think you have here, but the naming is a bit
>>> odd to me.
>>>
>>> What would you do with this "sub bus"?  It's just a device, but you are
>>> wanting it to be around for something.
>>>
>>
>> It's for power management stuff, basically, there are actual physical buses
>> involved that can be completely powered off IFF all of their devices are
>> not in use. Plus it actually matches bus topology this way.
> 
> Then create a real bus hanging off of a device, not another device that
> "acts" like a bus here, right?  Or am I missing the point?
> 

The motivation for doing it this was is that one driver could drive
devices on two different subbusses.  In the example, "my-driver" could
drive a device on sub_bus1 AND sub_bus2 (if there were 2+ devices, one
or more on each bus).

>From my understanding, this is not possible if they are actually
different busses.

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Timothy Meade]

On Tue, Aug 3, 2010 at 8:17 PM, Patrick Pannuto <ppannuto@quicinc.com> wrote:
>>>>>
>>>>>    struct platform_device sub_bus1 = {
>>>>>            .name           = "sub_bus1",
>>>>>            .id             = -1,
>>>>>            .dev.bus        = &my_bus_type,
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(sub_bus1);
>>>>
>>>> You really want a bus hanging off of a bus?  Normally you need a device
>>>> to do that, which is what I think you have here, but the naming is a bit
>>>> odd to me.
>>>>
>>>> What would you do with this "sub bus"?  It's just a device, but you are
>>>> wanting it to be around for something.
>>>>
>>>
>>> It's for power management stuff, basically, there are actual physical buses
>>> involved that can be completely powered off IFF all of their devices are
>>> not in use. Plus it actually matches bus topology this way.
>>
>> Then create a real bus hanging off of a device, not another device that
>> "acts" like a bus here, right?  Or am I missing the point?
>>
>
> The motivation for doing it this was is that one driver could drive
> devices on two different subbusses.  In the example, "my-driver" could
> drive a device on sub_bus1 AND sub_bus2 (if there were 2+ devices, one
> or more on each bus).
>
> From my understanding, this is not possible if they are actually
> different busses.

This could be quite useful on the Qualcomm devices where there are
many collections of "devices" that resemble a bus but cannot be
directly enumerated but are initialized by platform code or possibly
in future, a device tree.  One such bus is made up of SMI devices that
are identified to the applications core by the modem firmware and can
be in the form of character devices (smd), network devices (rmnet) rpc
router channel and others, we also have to deal with the MDDI video
bus which represented a challenge when migrating the HTC Rhodium to
2.6.32 as each mdp device (and others in later kernels) are added to
the platform bus dynamically, though they don't appear similararly
group in sysfs due to not actaully being on a bus.  While it would
have been possible to implement an mddi bus, the work would have
essentially recreated the platform bus with a new name.  A simliar
challenge exists for exposing rpc endpoints to userspace as the
current codebase uses character devices rather than introducing a new
network protocol to the kernel, if such as bus was exposed through
sysfs a userspace daemon could easily bind a GPS library to the PDAPI
endpoint or similar features requiring less configuration to adapt to
new AMSS firmware or device name layout.

Thank you,
Timothy Meade
tmzt #htc-linux
facebook.com/HTCLinux

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Grant Likely]

On Tue, Aug 3, 2010 at 6:17 PM, Patrick Pannuto <ppannuto@quicinc.com> wrote:
>>>>>
>>>>>    struct platform_device sub_bus1 = {
>>>>>            .name           = "sub_bus1",
>>>>>            .id             = -1,
>>>>>            .dev.bus        = &my_bus_type,
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(sub_bus1);
>>>>
>>>> You really want a bus hanging off of a bus?  Normally you need a device
>>>> to do that, which is what I think you have here, but the naming is a bit
>>>> odd to me.
>>>>
>>>> What would you do with this "sub bus"?  It's just a device, but you are
>>>> wanting it to be around for something.
>>>>
>>>
>>> It's for power management stuff, basically, there are actual physical buses
>>> involved that can be completely powered off IFF all of their devices are
>>> not in use. Plus it actually matches bus topology this way.
>>
>> Then create a real bus hanging off of a device, not another device that
>> "acts" like a bus here, right?  Or am I missing the point?
>>
>
> The motivation for doing it this was is that one driver could drive
> devices on two different subbusses.  In the example, "my-driver" could
> drive a device on sub_bus1 AND sub_bus2 (if there were 2+ devices, one
> or more on each bus).
>
> From my understanding, this is not possible if they are actually
> different busses.

It sounds an awful lot like 2 separate concepts are being conflated
here.  In Linux terminology, we have two things:

/bus types/ describe the common behavour of similarly attached things.
/physical or logical busses/ are where devices are actually attached
registered to the system.

These are not the same thing.

The primary thing that a bus-type provides is the set of device driver
that can be bound to the device plus some common functionality.
Almost every struct device has the 'struct bus_type bus' field set so
that it can be bound against a device driver when it is registered.

Every struct device also has a 'struct device *parent' field.  The
parent field denotes where the device lives in the Linux device model
(see /sys/devices/*), but it says *nothing* about what the bus type of
device it is.  Remember I mentioned physical/logical busses earlier?
Well the core Linux device model doesn't really have a concept of bus
instances, it only knows about devices and bus types.  The .parent
field is the closest thing the device model has to a bus instance, but
the model places no constraints on the bus type of each child 'struct
device'.

(caveat: many bus_types do have a concept of bus instances; but that
is specific to the bus_type.  The core device model doesn't care.
Also, in practical terms, yes most sibling devices will also be of the
same bus_type, but it is not a given, and it is not enforced.)

For example, a system can have many physical pci busses, but all the
devices hanging off those busses are still just registered on
pci_bus_type.  The parent of those devices, however, reflects the
physical bus the device is attached to.

In fact, the bus type has absolutely no bearing on where a device
actually lives in the Linux driver model.  A common misunderstanding
is that all platform devices live under /sys/devices/platform.

This also is not true.

By default the platform bus will put devices without a parent into
/sys/devices/platform, but if parent is set, then the device will be
registered where the parent pointer tells it to be registered.  It is
totally okay to have the following structure (off the top of my head):

system_bus (platform_bus_type, registers a bunch of child devices)
|--pci_bus (platform_bus_type; implements a pci host bridge)
|  |--usb host controller 1 (pci_bus_type, implements a usb bus instance)
|  |--usb host controller 2 (pci_bus_type, implements a usb bus instance)
|  |--fpga (pci_bus_type, implements a bunch of devices)
|     |--spi controller (platform_bus_type)
|     |--custom peripheral (platform_bus_type)
|--uart (platform_bus_type)
|--uart (platform_bus_type)
|--ethernet (platform_bus_type)
|--external bus (platform_bus_type; represents and external local bus)
|  |--i2c controller (platform_bus_type)
|  |  |--eeprom (i2c_bus_type)
|  |  |--temperature sensor (i2c_bus_type)
|--gpio controller (platform_bus_type)

Platform devices can show up just about anywhere in the tree.

(On that point Greg, what is the reason for even having the
/sys/devices/platform/ parent?  Why not just let the platform devices
sit at the root of the device tree?  In the OF case (granted, I'm
biased) all of the platform_device registrations reflect the actual
device hierarchy expressed in the device tree data.)

So, the *only* reason to have a different bus_type is if the actual
behaviour of the bus is different.  A different bus type wouldn't even
cause a different topology if doing so wouldn't make sense.  For
example in the OMAP case, the topology might look like:

system_bus (platform_bus_type, registers a bunch of child devices)
|  ...
|--uart (omap_bus_type)
|--uart (omap_bus_type)
|--ethernet (platform_bus_type)

See?  Devices of different types can often be siblings at the same
device layer.  (note: not *always* true.  PCI busses for instance
always expect to only host pci_devices and visa-versa, but for simple
platform-like busses it may be true).

Now, having gone on this whole long tirade, it looks like having
separate platform bus types may not be the best approach after all.
Magnus and Kevin are exploring using devres to handle implementing the
system-specific behaviour.  It is certainly an area for more research
and experimental implementations.

g.

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

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Greg KH]

On Thu, Aug 05, 2010 at 04:59:35PM -0600, Grant Likely wrote:
> (On that point Greg, what is the reason for even having the
> /sys/devices/platform/ parent?  Why not just let the platform devices
> sit at the root of the device tree?  In the OF case (granted, I'm
> biased) all of the platform_device registrations reflect the actual
> device hierarchy expressed in the device tree data.)

If we sat them at the "root", there would be a bunch of them there.  I
don't know, we could drop the parent, I guess whoever created the
platform device oh so long ago, decided that it would look nicer to be
in this type of structure.

> Now, having gone on this whole long tirade, it looks like having
> separate platform bus types may not be the best approach after all.

I totally agree, and thanks for the detailed explaination, it saved me
from having to write up the same thing :)

greg k-h

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Grant Likely]

On Fri, Aug 6, 2010 at 8:27 AM, Greg KH <gregkh@suse.de> wrote:
> On Thu, Aug 05, 2010 at 04:59:35PM -0600, Grant Likely wrote:
>> (On that point Greg, what is the reason for even having the
>> /sys/devices/platform/ parent?  Why not just let the platform devices
>> sit at the root of the device tree?  In the OF case (granted, I'm
>> biased) all of the platform_device registrations reflect the actual
>> device hierarchy expressed in the device tree data.)
>
> If we sat them at the "root", there would be a bunch of them there.  I
> don't know, we could drop the parent, I guess whoever created the
> platform device oh so long ago, decided that it would look nicer to be
> in this type of structure.

Personally I'd rather see a meaningful structure used here.  Maybe
having them all in the root will encourage people to find realistic
parents for their platform devices.  :-)  Why don't I float a patch to
remove this and see if anybody freaks out.  Should I wrap it with a
CONFIG_ so that it can be configurable for a release or to, or just
make it unconditional?

>> Now, having gone on this whole long tirade, it looks like having
>> separate platform bus types may not be the best approach after all.
>
> I totally agree, and thanks for the detailed explaination, it saved me
> from having to write up the same thing :)

:-)

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

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Greg KH]

On Fri, Aug 06, 2010 at 09:12:27AM -0600, Grant Likely wrote:
> On Fri, Aug 6, 2010 at 8:27 AM, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Aug 05, 2010 at 04:59:35PM -0600, Grant Likely wrote:
> >> (On that point Greg, what is the reason for even having the
> >> /sys/devices/platform/ parent?  Why not just let the platform devices
> >> sit at the root of the device tree?  In the OF case (granted, I'm
> >> biased) all of the platform_device registrations reflect the actual
> >> device hierarchy expressed in the device tree data.)
> >
> > If we sat them at the "root", there would be a bunch of them there.  I
> > don't know, we could drop the parent, I guess whoever created the
> > platform device oh so long ago, decided that it would look nicer to be
> > in this type of structure.
> 
> Personally I'd rather see a meaningful structure used here.  Maybe
> having them all in the root will encourage people to find realistic
> parents for their platform devices.  :-)

That would be nice, but take your "standard" PC today:
	> ls /sys/devices/platform/
	Fixed MDIO bus.0  i8042  pcspkr  power  serial8250  uevent vesafb.0

There are tty devices below the serial port, which is nice to see, but
the others?  I don't know what type of bus they would be on, do you?

> Why don't I float a patch to remove this and see if anybody freaks
> out.  Should I wrap it with a CONFIG_ so that it can be configurable
> for a release or to, or just make it unconditional?

If you can figure out a structure for the desktop/server machines, sure,
I say just always enable it :)

thanks,

greg k-h

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Grant Likely]

On Fri, Aug 6, 2010 at 5:46 PM, Greg KH <gregkh@suse.de> wrote:
> On Fri, Aug 06, 2010 at 09:12:27AM -0600, Grant Likely wrote:
>> On Fri, Aug 6, 2010 at 8:27 AM, Greg KH <gregkh@suse.de> wrote:
>> > On Thu, Aug 05, 2010 at 04:59:35PM -0600, Grant Likely wrote:
>> >> (On that point Greg, what is the reason for even having the
>> >> /sys/devices/platform/ parent?  Why not just let the platform devices
>> >> sit at the root of the device tree?  In the OF case (granted, I'm
>> >> biased) all of the platform_device registrations reflect the actual
>> >> device hierarchy expressed in the device tree data.)
>> >
>> > If we sat them at the "root", there would be a bunch of them there.  I
>> > don't know, we could drop the parent, I guess whoever created the
>> > platform device oh so long ago, decided that it would look nicer to be
>> > in this type of structure.
>>
>> Personally I'd rather see a meaningful structure used here.  Maybe
>> having them all in the root will encourage people to find realistic
>> parents for their platform devices.  :-)
>
> That would be nice, but take your "standard" PC today:
>        > ls /sys/devices/platform/
>        Fixed MDIO bus.0  i8042  pcspkr  power  serial8250  uevent vesafb.0
>
> There are tty devices below the serial port, which is nice to see, but
> the others?  I don't know what type of bus they would be on, do you?

Does it matter?  On my PC I count 7 devices.  On my servers, I see the
same.  I don't personally don't see any disadvantage to having them at
the root.

However, looking at them:

"Fixed MDIO bus" is actually a complete hack that I'm going to try and
get rid of.
i8042 is keyboard/mouse that probably lives on the southbridge.  I
imagine hanging off the PCI bus in the ISA IO range.
ditto pcspkr
ditto serial8250
ditto vesafb

I wouldn't have any problem modifying those specific drivers to
register under something like /sys/devices/legacy, but I don't really
think it is in any way necessary.

I suppose one *could* try to figure out the actual PCI topology that
leads to these devices, but that gets into trying to guess what the
BIOS set up to decide where to register those things.  Probably a lot
of error-prone work for absolutely no benefit.  :-)

However, on the embedded side I'm seeing a lot of devices show up
under /sys/devices/platform.  Embedded is generally different.  We
*know* what the bus topology is.  However, it seems to me that many
developers are under the mistaken assumption that platform devices
must live in /sys/devices/platform, so they don't bother reflecting
the topology in the device model and it seems to be affecting how
embedded PM is being approached (my opinion).  Removing the "struct
device platform_bus" may reduce the confusion.

>> Why don't I float a patch to remove this and see if anybody freaks
>> out.  Should I wrap it with a CONFIG_ so that it can be configurable
>> for a release or to, or just make it unconditional?
>
> If you can figure out a structure for the desktop/server machines, sure,
> I say just always enable it :)

I've got a patch in my tree now.  I'll play with it a bit before
posting it for RFC.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Grant Likely]

On Sat, Aug 7, 2010 at 12:35 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, Aug 6, 2010 at 5:46 PM, Greg KH <gregkh@suse.de> wrote:
>> That would be nice, but take your "standard" PC today:
>>        > ls /sys/devices/platform/
>>        Fixed MDIO bus.0  i8042  pcspkr  power  serial8250  uevent vesafb.0
>>
>> There are tty devices below the serial port, which is nice to see, but
>> the others?  I don't know what type of bus they would be on, do you?
[...]
> I wouldn't have any problem modifying those specific drivers to
> register under something like /sys/devices/legacy, but I don't really
> think it is in any way necessary.

Or for that matter, make those drivers explicitly use
/sys/devices/platform so that I don't cause churn on PCs.  :-)  I'd
like to be rid of it as default behaviour for embedded though.

g.

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Greg KH]

On Sat, Aug 07, 2010 at 11:28:13AM -0600, Grant Likely wrote:
> On Sat, Aug 7, 2010 at 12:35 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Fri, Aug 6, 2010 at 5:46 PM, Greg KH <gregkh@suse.de> wrote:
> >> That would be nice, but take your "standard" PC today:
> >>        > ls /sys/devices/platform/
> >>        Fixed MDIO bus.0  i8042  pcspkr  power  serial8250  uevent vesafb.0
> >>
> >> There are tty devices below the serial port, which is nice to see, but
> >> the others?  I don't know what type of bus they would be on, do you?
> [...]
> > I wouldn't have any problem modifying those specific drivers to
> > register under something like /sys/devices/legacy, but I don't really
> > think it is in any way necessary.
> 
> Or for that matter, make those drivers explicitly use
> /sys/devices/platform so that I don't cause churn on PCs.  :-)  I'd
> like to be rid of it as default behaviour for embedded though.

Yes, that would be the easier (and better) solution.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] platform: Faciliatate the creation of pseduo-platform busses

[Grant Likely]

On Tue, Aug 3, 2010 at 5:56 PM, Greg KH <gregkh@suse.de> wrote:
> On Tue, Aug 03, 2010 at 04:35:06PM -0700, Patrick Pannuto wrote:
>> @@ -539,12 +541,12 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
>>        * if the probe was successful, and make sure any forced probes of
>>        * new devices fail.
>>        */
>> -     spin_lock(&platform_bus_type.p->klist_drivers.k_lock);
>> +     spin_lock(&drv->driver.bus->p->klist_drivers.k_lock);
>>       drv->probe = NULL;
>>       if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list))
>>               retval = -ENODEV;
>>       drv->driver.probe = platform_drv_probe_fail;
>> -     spin_unlock(&platform_bus_type.p->klist_drivers.k_lock);
>> +     spin_unlock(&drv->driver.bus->p->klist_drivers.k_lock);
>>
>>       if (code != retval)
>>               platform_driver_unregister(drv);
>
> I'm guessing that this chunk can be applied now, right?

Yes, I agree.  This chunk makes sense independently of the rest of the patch.

g.