Inspiration for this comes from:
http://www.mail-archive.com/[email protected]/msg31161.html
RFC: http://lkml.org/lkml/2010/8/3/496
Patch is unchanged from the RFC. Reviews seemed generally positive
and it seemed this was desired functionality.
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_GPL(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
Signed-off-by: Patrick Pannuto <[email protected]>
---
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,
--
1.7.2
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Patrick Pannuto <[email protected]> writes:
> Inspiration for this comes from:
> http://www.mail-archive.com/[email protected]/msg31161.html
Also, later in that thread I also wrote[1] what seems to be the core of
what you've done here: namely, allow platform_devices and
platform_drivers to to be used on custom busses. Patch is at the end of
this mail with a more focused changelog. As Greg suggested in his reply
to your first version, this part could be merged today, and the
platform_bus_init stuff could be added later, after some more review.
Some comments below...
> 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_GPL(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.
Except it requires a re-compile.
Rather than doing this at compile time, it would be better to support
legacy devices at runtime. You could handle this by simply registering
the driver on the custom bus and the platform_bus and let the bus
matching code handle it. Then, the same binary would work on both
legacy and updated SoCs.
>
> 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
>
>
> Signed-off-by: Patrick Pannuto <[email protected]>
> ---
> 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);
Up to here, this looks exactly what I wrote in thread referenced above.
>
> 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
> + */
minor nit: kernel doc style has wrong indentation
> +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);
With this approach, you should note in the comments/changelog that
any selective customization of the bus PM methods must be done after
calling platform_bus_type_init().
Also, You've left out the legacy PM methods here. That implies that
moving a driver from the platform_bus to the custom bus is not entirely
transparent. If the driver still has legacy PM methods, it would stop
working on the custom bus.
While this is good motivation for converting a driver to dev_pm_ops, at
a minimum it should be clear in the changelog that the derivative busses
do not support legacy PM methods. However, since it's quite easy to do,
and you want the derivative busses to be *exactly* like the platform bus
except where explicitly changed, I'd suggest you also check/copy the
legacy PM methods.
In addition, you've missed several fields in 'struct bus_type'
(bus_attr, drv_attr, p, etc.) Without digging deeper into the driver
core, I'm not sure if they are all needed at init time, but it should be
clear in the comments why they can be excluded.
Kevin
[1] http://www.mail-archive.com/[email protected]/msg31289.html
>From b784009af1d0a7065dc5e58a13ce29afa3432d3e Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Mon, 28 Jun 2010 16:08:14 -0700
Subject: [PATCH] driver core: allow platform_devices and platform_drivers on custom busses
This allows platform_devices and platform_drivers to be registered onto
custom busses that are compatible with the platform_bus.
Signed-off-by: Kevin Hilman <[email protected]>
---
drivers/base/platform.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4d99c8b..2cf55e2 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);
--
1.7.2.1
On 08/04/2010 05:16 PM, Kevin Hilman wrote:
> Patrick Pannuto <[email protected]> writes:
>
>> Inspiration for this comes from:
>> http://www.mail-archive.com/[email protected]/msg31161.html
>
> Also, later in that thread I also wrote[1] what seems to be the core of
> what you've done here: namely, allow platform_devices and
> platform_drivers to to be used on custom busses. Patch is at the end of
> this mail with a more focused changelog. As Greg suggested in his reply
> to your first version, this part could be merged today, and the
> platform_bus_init stuff could be added later, after some more review.
> Some comments below...
>
I can split this into 2 patches.
Was your patch sent to linux-kernel or just linux-omap? I'm not on linux-omap...
>> [snip]
>>
>> Which will allow the same driver to easily to used on either
>> the platform bus or the newly defined bus type.
>
> Except it requires a re-compile.
>
> Rather than doing this at compile time, it would be better to support
> legacy devices at runtime. You could handle this by simply registering
> the driver on the custom bus and the platform_bus and let the bus
> matching code handle it. Then, the same binary would work on both
> legacy and updated SoCs.
>
Can you safely register a driver on more than one bus? I didn't think
that was safe -- normally it's impossible since you're calling
struct BUS_TYPE_driver mydriver;
BUS_TYPE_driver_register(&mydriver)
but now we have multiple "bus types" that are all actually platform type; still,
at a minimum you would need:
struct platform_driver mydrvier1 = {
.driver.bus = &sub_bus1,
};
struct platform_driver mydrvier2 = {
.driver.bus = &sub_bus2,
};
which would all point to the same driver functions, yet the respective devices
attached for the "same" driver would be on different buses. I fear this might
confuse some drivers. I don't think dynamic bus assignment is this easy
In short: I do not believe the same driver can be registered on multiple
different buses -- if this is wrong, please correct me.
>
> Up to here, this looks exactly what I wrote in thread referenced above.
>
It is, you just went on vacation :)
>>
>> 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
>> + */
>
> minor nit: kernel doc style has wrong indentation
>
will fix
>> +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);
>
> With this approach, you should note in the comments/changelog that
> any selective customization of the bus PM methods must be done after
> calling platform_bus_type_init().
No they don't. If you call platform_bus_type_init first then you'll
just overwrite them with new values; if you call it second then they
will all already be well-defined and thus not overwritten.
>
> Also, You've left out the legacy PM methods here. That implies that
> moving a driver from the platform_bus to the custom bus is not entirely
> transparent. If the driver still has legacy PM methods, it would stop
> working on the custom bus.
>
> While this is good motivation for converting a driver to dev_pm_ops, at
> a minimum it should be clear in the changelog that the derivative busses
> do not support legacy PM methods. However, since it's quite easy to do,
> and you want the derivative busses to be *exactly* like the platform bus
> except where explicitly changed, I'd suggest you also check/copy the
> legacy PM methods.
>
> In addition, you've missed several fields in 'struct bus_type'
> (bus_attr, drv_attr, p, etc.) Without digging deeper into the driver
> core, I'm not sure if they are all needed at init time, but it should be
> clear in the comments why they can be excluded.
>
I copied everything that was defined for platform_bus_type:
struct bus_type platform_bus_type = {
.name = "platform",
.dev_attrs = platform_dev_attrs,
.match = platform_match,
.uevent = platform_uevent,
.pm = &platform_dev_pm_ops,
};
EXPORT_SYMBOL_GPL(platform_bus_type);
struct bus_type {
const char *name;
struct bus_attribute *bus_attrs;
struct device_attribute *dev_attrs;
struct driver_attribute *drv_attrs;
int (*match)(struct device *dev, struct device_driver *drv);
int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
int (*probe)(struct device *dev);
int (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);
int (*suspend)(struct device *dev, pm_message_t state);
int (*resume)(struct device *dev);
const struct dev_pm_ops *pm;
struct bus_type_private *p;
};
It is my understanding that everything that I did not copy *should* remain
unique to each bus; remaining fields will be filled in by bus_register and
should not be copied.
> Kevin
>
> [1] http://www.mail-archive.com/[email protected]/msg31289.html
>
>
> From b784009af1d0a7065dc5e58a13ce29afa3432d3e Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <[email protected]>
> Date: Mon, 28 Jun 2010 16:08:14 -0700
> Subject: [PATCH] driver core: allow platform_devices and platform_drivers on custom busses
>
> This allows platform_devices and platform_drivers to be registered onto
> custom busses that are compatible with the platform_bus.
>
> Signed-off-by: Kevin Hilman <[email protected]>
> ---
> drivers/base/platform.c | 10 ++++++----
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4d99c8b..2cf55e2 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);
If you would like to lead this effort, please do so; I did not mean to step
on your toes, it's just that this is an issue for me as well. You had
indicated that you were going on vacation for a month and I had not seen any
more follow-up on this issue, so I forged ahead. If you'd like me to drop it,
please let me know and I will - but also please send stuff like this to wider
distribution than just linux-omap; it has much greater reach (and interest).
Thanks,
-Pat
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <[email protected]> wrote:
> Inspiration for this comes from:
> http://www.mail-archive.com/[email protected]/msg31161.html
>
> RFC: http://lkml.org/lkml/2010/8/3/496
> Patch is unchanged from the RFC. Reviews seemed generally positive
> and it seemed this was desired functionality.
Thanks for your patch, it's really nice to see work done in this area!
I'd like to see something like this merged in the not so distant
future. At this point I'm not so concerned about the details, so I'll
restrict myself to this:
> /drivers/my_driver.c
> ? ? ? ?static struct platform_driver my_driver = {
> ? ? ? ? ? ? ? ?.driver = {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? = "my-driver",
> ? ? ? ? ? ? ? ? ? ? ? ?.owner ?= THIS_MODULE,
> ? ? ? ? ? ? ? ? ? ? ? ?.bus ? ?= &my_bus_type,
> ? ? ? ? ? ? ? ?},
> ? ? ? ?};
I would really prefer not to have the bus type in the here. I
understand it's needed at this point, but I wonder if it's possible to
adjust the device<->driver matching for platform devices to allow any
type of pseudo-platform bus_type.
The reason why I'd like to avoid having the bus type in the driver is
that I'd like to reuse the platform driver across multiple
architectures and buses. For instance, on the SH architecture and
SH-Mobile ARM we have SoCs with SCIF hardware blocks driven by the
sh-sci.c serial driver. The sh-sci.c platform driver supports a wide
range of different SCI(F)(A)(B) hardware blocks, and on any given SoC
there is a mix of SCIF blocks spread out on different buses.
At this point our SH platform drivers are unaware where their driver
instanced are located on the SoC. The I/O address and IRQs are
assigned via struct resource and clocks are managed through clkdev. I
believe that adding the bus type in the driver will violate this
abstraction and make it more difficult to just instantiate a driver
somewhere on the SoC.
> /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,
> ? ? ? ?};
This I don't mind at all. Actually, this is the place where the
topology should be defined IMO.
Cheers,
/ magnus
Magnus Damm <[email protected]> writes:
> On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <[email protected]> wrote:
>> Inspiration for this comes from:
>> http://www.mail-archive.com/[email protected]/msg31161.html
>>
>> RFC: http://lkml.org/lkml/2010/8/3/496
>> Patch is unchanged from the RFC. Reviews seemed generally positive
>> and it seemed this was desired functionality.
>
> Thanks for your patch, it's really nice to see work done in this area!
> I'd like to see something like this merged in the not so distant
> future. At this point I'm not so concerned about the details, so I'll
> restrict myself to this:
>
>> /drivers/my_driver.c
>> ? ? ? ?static struct platform_driver my_driver = {
>> ? ? ? ? ? ? ? ?.driver = {
>> ? ? ? ? ? ? ? ? ? ? ? ?.name ? = "my-driver",
>> ? ? ? ? ? ? ? ? ? ? ? ?.owner ?= THIS_MODULE,
>> ? ? ? ? ? ? ? ? ? ? ? ?.bus ? ?= &my_bus_type,
>> ? ? ? ? ? ? ? ?},
>> ? ? ? ?};
>
> I would really prefer not to have the bus type in the here. I
> understand it's needed at this point, but I wonder if it's possible to
> adjust the device<->driver matching for platform devices to allow any
> type of pseudo-platform bus_type.
I totally agree here. Keeping the drivers ignorant of the bus (or SoC)
they are on will make them much more portable.
Kevin
Patrick Pannuto <[email protected]> writes:
> On 08/04/2010 05:16 PM, Kevin Hilman wrote:
>> Patrick Pannuto <[email protected]> writes:
>>
>>> Inspiration for this comes from:
>>> http://www.mail-archive.com/[email protected]/msg31161.html
>>
>> Also, later in that thread I also wrote[1] what seems to be the core of
>> what you've done here: namely, allow platform_devices and
>> platform_drivers to to be used on custom busses. Patch is at the end of
>> this mail with a more focused changelog. As Greg suggested in his reply
>> to your first version, this part could be merged today, and the
>> platform_bus_init stuff could be added later, after some more review.
>> Some comments below...
>>
>
> I can split this into 2 patches.
Yes, I think that would be better.
> Was your patch sent to linux-kernel or just linux-omap? I'm not on linux-omap...
That thread was on linux-arm-kernel and linux-omap
>
>>> [snip]
>>>
>>> Which will allow the same driver to easily to used on either
>>> the platform bus or the newly defined bus type.
>>
>> Except it requires a re-compile.
>>
>> Rather than doing this at compile time, it would be better to support
>> legacy devices at runtime. You could handle this by simply registering
>> the driver on the custom bus and the platform_bus and let the bus
>> matching code handle it. Then, the same binary would work on both
>> legacy and updated SoCs.
>>
>
> Can you safely register a driver on more than one bus? I didn't think
> that was safe -- normally it's impossible since you're calling
>
> struct BUS_TYPE_driver mydriver;
> BUS_TYPE_driver_register(&mydriver)
>
> but now we have multiple "bus types" that are all actually platform type; still,
> at a minimum you would need:
> struct platform_driver mydrvier1 = {
> .driver.bus = &sub_bus1,
> };
> struct platform_driver mydrvier2 = {
> .driver.bus = &sub_bus2,
> };
> which would all point to the same driver functions, yet the respective devices
> attached for the "same" driver would be on different buses. I fear this might
> confuse some drivers. I don't think dynamic bus assignment is this easy
>
> In short: I do not believe the same driver can be registered on multiple
> different buses -- if this is wrong, please correct me.
It is possible, and currently done in powerpc land where some
drivers handle devices on the platform_bus and the custom OF bus.
However, as noted by Magnus, what we really need here is a way for
drivers to not care at all what kind of bus they are on. There are an
increasing number of drivers that are re-used not just across different
SoCs in the same family, but across totally different SoCs (e.g. drivers
for hardware shared between TI OMAP and TI DaVinci, or SH and SH-Mobile/ARM)
>>
>> Up to here, this looks exactly what I wrote in thread referenced
>> above.
>>
>
> It is, you just went on vacation :)
>
Ah, OK. The changelog was missing credits to that affect, but I was
more concerned that you hadn't seen my example and didn't want to be
duplicating work.
>>>
>>> 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
>>> + */
>>
>> minor nit: kernel doc style has wrong indentation
>>
>
> will fix
>
>>> +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);
>>
>> With this approach, you should note in the comments/changelog that
>> any selective customization of the bus PM methods must be done after
>> calling platform_bus_type_init().
>
> No they don't. If you call platform_bus_type_init first then you'll
> just overwrite them with new values;
Yes.
> if you call it second then they will all already be well-defined and
> thus not overwritten.
Right, they will not be overwritten, but you'll be left with a mostly
empty dev_pm_ops on the custom bus.
IOW, Most of these custom busses will only want to customize a small
subset of the dev_pm_ops methods (e.g. only the runtime PM methods.) If
you setup your sparsly populated custom dev_pm_ops and then call
platform_bus_type_init() second, dev_pm_ops on the new buswill have *only*
your custom fields, and none of the defaults from platform_dev_pm_ops.
So, what I was getting at is that it should probably be clearer to the
users of platform_bus_type_init() that any customization of dev_pm_ops
should be done after.
>>
>> Also, You've left out the legacy PM methods here. That implies that
>> moving a driver from the platform_bus to the custom bus is not entirely
>> transparent. If the driver still has legacy PM methods, it would stop
>> working on the custom bus.
>>
>> While this is good motivation for converting a driver to dev_pm_ops, at
>> a minimum it should be clear in the changelog that the derivative busses
>> do not support legacy PM methods. However, since it's quite easy to do,
>> and you want the derivative busses to be *exactly* like the platform bus
>> except where explicitly changed, I'd suggest you also check/copy the
>> legacy PM methods.
>>
>> In addition, you've missed several fields in 'struct bus_type'
>> (bus_attr, drv_attr, p, etc.) Without digging deeper into the driver
>> core, I'm not sure if they are all needed at init time, but it should be
>> clear in the comments why they can be excluded.
>>
>
> I copied everything that was defined for platform_bus_type:
>
> struct bus_type platform_bus_type = {
> .name = "platform",
> .dev_attrs = platform_dev_attrs,
> .match = platform_match,
> .uevent = platform_uevent,
> .pm = &platform_dev_pm_ops,
> };
> EXPORT_SYMBOL_GPL(platform_bus_type);
>
> struct bus_type {
> const char *name;
> struct bus_attribute *bus_attrs;
> struct device_attribute *dev_attrs;
> struct driver_attribute *drv_attrs;
>
> int (*match)(struct device *dev, struct device_driver *drv);
> int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
> int (*probe)(struct device *dev);
> int (*remove)(struct device *dev);
> void (*shutdown)(struct device *dev);
>
> int (*suspend)(struct device *dev, pm_message_t state);
> int (*resume)(struct device *dev);
>
> const struct dev_pm_ops *pm;
>
> struct bus_type_private *p;
> };
>
> It is my understanding that everything that I did not copy *should* remain
> unique to each bus; remaining fields will be filled in by bus_register and
> should not be copied.
>
[...]
>
> If you would like to lead this effort, please do so; I did not mean to step
> on your toes, it's just that this is an issue for me as well.
No worries there, my toes are fine. :)
> You had indicated that you were going on vacation for a month and I
> had not seen any more follow-up on this issue, so I forged ahead.
Great, I'm glad you forged ahead. There is definitely a broader need
for something like this, and I have no personal attachment to the code.
I have no problems with you continuing the work (in fact, I'd prefer it.
I have lots of other things to catch up on after my vacation.)
In the future though, it's common (and kind) to note the original author
in the changelog when basing a patch on previous work. Something like
"originally written by..." or "based on the work of..." etc.
Thanks,
Kevin
>>>> [snip]
>>>>
>>>> Which will allow the same driver to easily to used on either
>>>> the platform bus or the newly defined bus type.
>>>
>>> Except it requires a re-compile.
>>>
>>> Rather than doing this at compile time, it would be better to support
>>> legacy devices at runtime. You could handle this by simply registering
>>> the driver on the custom bus and the platform_bus and let the bus
>>> matching code handle it. Then, the same binary would work on both
>>> legacy and updated SoCs.
>>>
>>
>> Can you safely register a driver on more than one bus? I didn't think
>> that was safe -- normally it's impossible since you're calling
>>
>> struct BUS_TYPE_driver mydriver;
>> BUS_TYPE_driver_register(&mydriver)
>>
>> but now we have multiple "bus types" that are all actually platform type; still,
>> at a minimum you would need:
>> struct platform_driver mydrvier1 = {
>> .driver.bus = &sub_bus1,
>> };
>> struct platform_driver mydrvier2 = {
>> .driver.bus = &sub_bus2,
>> };
>> which would all point to the same driver functions, yet the respective devices
>> attached for the "same" driver would be on different buses. I fear this might
>> confuse some drivers. I don't think dynamic bus assignment is this easy
>>
>> In short: I do not believe the same driver can be registered on multiple
>> different buses -- if this is wrong, please correct me.
>
> It is possible, and currently done in powerpc land where some
> drivers handle devices on the platform_bus and the custom OF bus.
>
> However, as noted by Magnus, what we really need here is a way for
> drivers to not care at all what kind of bus they are on. There are an
> increasing number of drivers that are re-used not just across different
> SoCs in the same family, but across totally different SoCs (e.g. drivers
> for hardware shared between TI OMAP and TI DaVinci, or SH and SH-Mobile/ARM)
>
I will start trying to work on this
>>>
>>> Up to here, this looks exactly what I wrote in thread referenced
>>> above.
>>>
>>
>> It is, you just went on vacation :)
>>
>
> Ah, OK. The changelog was missing credits to that affect, but I was
> more concerned that you hadn't seen my example and didn't want to be
> duplicating work.
>
will fix.
>>>> [snip]
>
>> if you call it second then they will all already be well-defined and
>> thus not overwritten.
>
> Right, they will not be overwritten, but you'll be left with a mostly
> empty dev_pm_ops on the custom bus.
>
> IOW, Most of these custom busses will only want to customize a small
> subset of the dev_pm_ops methods (e.g. only the runtime PM methods.) If
> you setup your sparsly populated custom dev_pm_ops and then call
> platform_bus_type_init() second, dev_pm_ops on the new buswill have *only*
> your custom fields, and none of the defaults from platform_dev_pm_ops.
>
> So, what I was getting at is that it should probably be clearer to the
> users of platform_bus_type_init() that any customization of dev_pm_ops
> should be done after.
>
I understand what you're saying now, and I can fix this as well.
>
>>
>> If you would like to lead this effort, please do so; I did not mean to step
>> on your toes, it's just that this is an issue for me as well.
>
> No worries there, my toes are fine. :)
Good :)
>
>> You had indicated that you were going on vacation for a month and I
>> had not seen any more follow-up on this issue, so I forged ahead.
>
> Great, I'm glad you forged ahead. There is definitely a broader need
> for something like this, and I have no personal attachment to the code.
>
> I have no problems with you continuing the work (in fact, I'd prefer it.
> I have lots of other things to catch up on after my vacation.)
>
> In the future though, it's common (and kind) to note the original author
> in the changelog when basing a patch on previous work. Something like
> "originally written by..." or "based on the work of..." etc.
Ok, I can do that; that was the intention of the "original inspiration from"
line at the beginning. Is there a more formal way of indicating this in the
next version of the patch? Should I add you as a "From:" or an "Author:"?
-Pat
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On 08/04/2010 07:32 PM, Magnus Damm wrote:
> On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <[email protected]> wrote:
>> Inspiration for this comes from:
>> http://www.mail-archive.com/[email protected]/msg31161.html
>>
>> RFC: http://lkml.org/lkml/2010/8/3/496
>> Patch is unchanged from the RFC. Reviews seemed generally positive
>> and it seemed this was desired functionality.
>
> Thanks for your patch, it's really nice to see work done in this area!
> I'd like to see something like this merged in the not so distant
> future. At this point I'm not so concerned about the details, so I'll
> restrict myself to this:
>
>> /drivers/my_driver.c
>> static struct platform_driver my_driver = {
>> .driver = {
>> .name = "my-driver",
>> .owner = THIS_MODULE,
>> .bus = &my_bus_type,
>> },
>> };
>
> I would really prefer not to have the bus type in the here. I
> understand it's needed at this point, but I wonder if it's possible to
> adjust the device<->driver matching for platform devices to allow any
> type of pseudo-platform bus_type.
>
> The reason why I'd like to avoid having the bus type in the driver is
> that I'd like to reuse the platform driver across multiple
> architectures and buses. For instance, on the SH architecture and
So would I :). That's where this was all heading eventually, I was just
originally doing it in two passes. I have some ideas for how to do this
and will try to send out a patchset either today or tomorrow.
> SH-Mobile ARM we have SoCs with SCIF hardware blocks driven by the
> sh-sci.c serial driver. The sh-sci.c platform driver supports a wide
> range of different SCI(F)(A)(B) hardware blocks, and on any given SoC
> there is a mix of SCIF blocks spread out on different buses.
>
> At this point our SH platform drivers are unaware where their driver
> instanced are located on the SoC. The I/O address and IRQs are
> assigned via struct resource and clocks are managed through clkdev. I
> believe that adding the bus type in the driver will violate this
> abstraction and make it more difficult to just instantiate a driver
> somewhere on the SoC.
>
>> /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,
>> };
>
> This I don't mind at all. Actually, this is the place where the
> topology should be defined IMO.
>
Agreed.
> Cheers,
>
> / magnus
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Patrick Pannuto <[email protected]> writes:
>>
>>> You had indicated that you were going on vacation for a month and I
>>> had not seen any more follow-up on this issue, so I forged ahead.
>>
>> Great, I'm glad you forged ahead. There is definitely a broader need
>> for something like this, and I have no personal attachment to the code.
>>
>> I have no problems with you continuing the work (in fact, I'd prefer it.
>> I have lots of other things to catch up on after my vacation.)
>>
>> In the future though, it's common (and kind) to note the original author
>> in the changelog when basing a patch on previous work. Something like
>> "originally written by..." or "based on the work of..." etc.
>
> Ok, I can do that; that was the intention of the "original inspiration from"
> line at the beginning.
Maybe we need an 'Inspired-by:' tag. :)
> Is there a more formal way of indicating this in the
> next version of the patch? Should I add you as a "From:" or an "Author:"?
I don't know if there is an official way of doing this.
I typically add something like "based on the idea/code originally
proposed/written by John Doe" in the changelog, and then add Cc: John
Doe <...> in the changelog too, but AFAIK, none of this is formalized.
Kevin
On Thu, Aug 5, 2010 at 9:57 AM, Kevin Hilman
<[email protected]> wrote:
> Patrick Pannuto <[email protected]> writes:
>
>> On 08/04/2010 05:16 PM, Kevin Hilman wrote:
>>> Patrick Pannuto <[email protected]> writes:
>>>
>>>> Inspiration for this comes from:
>>>> http://www.mail-archive.com/[email protected]/msg31161.html
>>>
>>> Also, later in that thread I also wrote[1] what seems to be the core of
>>> what you've done here: namely, allow platform_devices and
>>> platform_drivers to to be used on custom busses. ?Patch is at the end of
>>> this mail with a more focused changelog. ?As Greg suggested in his reply
>>> to your first version, this part could be merged today, and the
>>> platform_bus_init stuff could be added later, after some more review.
>>> Some comments below...
>>>
>>
>> I can split this into 2 patches.
>
> Yes, I think that would be better.
>
>> Was your patch sent to linux-kernel or just linux-omap? I'm not on linux-omap...
>
> That thread was on linux-arm-kernel and linux-omap
>
>>
>>>> [snip]
>>>>
>>>> Which will allow the same driver to easily to used on either
>>>> the platform bus or the newly defined bus type.
>>>
>>> Except it requires a re-compile.
>>>
>>> Rather than doing this at compile time, it would be better to support
>>> legacy devices at runtime. ?You could handle this by simply registering
>>> the driver on the custom bus and the platform_bus and let the bus
>>> matching code handle it. ?Then, the same binary would work on both
>>> legacy and updated SoCs.
>>>
>>
>> Can you safely register a driver on more than one bus? I didn't think
>> that was safe -- normally it's impossible since you're calling
>>
>> struct BUS_TYPE_driver mydriver;
>> BUS_TYPE_driver_register(&mydriver)
>>
>> but now we have multiple "bus types" that are all actually platform type; still,
>> at a minimum you would need:
>> ? ? ? struct platform_driver mydrvier1 = {
>> ? ? ? ? ? ? ? .driver.bus = &sub_bus1,
>> ? ? ? };
>> ? ? ? struct platform_driver mydrvier2 = {
>> ? ? ? ? ? ? ? .driver.bus = &sub_bus2,
>> ? ? ? };
>> which would all point to the same driver functions, yet the respective devices
>> attached for the "same" driver would be on different buses. I fear this might
>> confuse some drivers. I don't think dynamic bus assignment is this easy
>>
>> In short: I do not believe the same driver can be registered on multiple
>> different buses -- if this is wrong, please correct me.
>
> It is possible, and currently done in powerpc land where some
> drivers handle devices on the platform_bus and the custom OF bus.
As of now, the of_platform_bus_type has been removed. It was a bad
idea because it tried to encode non-bus-specific information into
something that was just a clone of the platform_bus. Drivers that
worked on both had to be bound to both busses. I do actually have
code that automatically registers a driver on more than one bus, but
it is rather a hack and was only a temporary measure.
The relevant question before going down this path is, "Is the
omap/sh/other-soc behaviour something fundamentally different from the
platform bus? Or is it something complementary that would be better
handled with a notifier or some orthogonal method of adding new
behaviour?"
I don't have a problem with multiple platform_bus instances using the
same code (I did suggest it after all), but I do worry about muddying
the Linux device model or making it overly complex. Binding single
drivers to multiple device types could be messy.
Cheers,
g.
On 08/05/2010 04:16 PM, Grant Likely wrote:
> On Thu, Aug 5, 2010 at 9:57 AM, Kevin Hilman
> <[email protected]> wrote:
>> Patrick Pannuto <[email protected]> writes:
>>
>>> On 08/04/2010 05:16 PM, Kevin Hilman wrote:
>>>> Patrick Pannuto <[email protected]> writes:
>>>>
>>>>> Inspiration for this comes from:
>>>>> http://www.mail-archive.com/[email protected]/msg31161.html
>>>>
>>>> Also, later in that thread I also wrote[1] what seems to be the core of
>>>> what you've done here: namely, allow platform_devices and
>>>> platform_drivers to to be used on custom busses. Patch is at the end of
>>>> this mail with a more focused changelog. As Greg suggested in his reply
>>>> to your first version, this part could be merged today, and the
>>>> platform_bus_init stuff could be added later, after some more review.
>>>> Some comments below...
>>>>
>>>
>>> I can split this into 2 patches.
>>
>> Yes, I think that would be better.
>>
>>> Was your patch sent to linux-kernel or just linux-omap? I'm not on linux-omap...
>>
>> That thread was on linux-arm-kernel and linux-omap
>>
>>>
>>>>> [snip]
>>>>>
>>>>> Which will allow the same driver to easily to used on either
>>>>> the platform bus or the newly defined bus type.
>>>>
>>>> Except it requires a re-compile.
>>>>
>>>> Rather than doing this at compile time, it would be better to support
>>>> legacy devices at runtime. You could handle this by simply registering
>>>> the driver on the custom bus and the platform_bus and let the bus
>>>> matching code handle it. Then, the same binary would work on both
>>>> legacy and updated SoCs.
>>>>
>>>
>>> Can you safely register a driver on more than one bus? I didn't think
>>> that was safe -- normally it's impossible since you're calling
>>>
>>> struct BUS_TYPE_driver mydriver;
>>> BUS_TYPE_driver_register(&mydriver)
>>>
>>> but now we have multiple "bus types" that are all actually platform type; still,
>>> at a minimum you would need:
>>> struct platform_driver mydrvier1 = {
>>> .driver.bus = &sub_bus1,
>>> };
>>> struct platform_driver mydrvier2 = {
>>> .driver.bus = &sub_bus2,
>>> };
>>> which would all point to the same driver functions, yet the respective devices
>>> attached for the "same" driver would be on different buses. I fear this might
>>> confuse some drivers. I don't think dynamic bus assignment is this easy
>>>
>>> In short: I do not believe the same driver can be registered on multiple
>>> different buses -- if this is wrong, please correct me.
>>
>> It is possible, and currently done in powerpc land where some
>> drivers handle devices on the platform_bus and the custom OF bus.
>
> As of now, the of_platform_bus_type has been removed. It was a bad
> idea because it tried to encode non-bus-specific information into
> something that was just a clone of the platform_bus. Drivers that
> worked on both had to be bound to both busses. I do actually have
> code that automatically registers a driver on more than one bus, but
> it is rather a hack and was only a temporary measure.
>
> The relevant question before going down this path is, "Is the
> omap/sh/other-soc behaviour something fundamentally different from the
> platform bus? Or is it something complementary that would be better
> handled with a notifier or some orthogonal method of adding new
> behaviour?"
>
> I don't have a problem with multiple platform_bus instances using the
> same code (I did suggest it after all), but I do worry about muddying
> the Linux device model or making it overly complex. Binding single
> drivers to multiple device types could be messy.
>
> Cheers,
> g.
>From your other email, the distinction of /bus_types/ versus /physical
or logical buses/ was very valuable (thanks). I am becoming less
convinced that I need this infrastructure or the ability to create
pseudo-platform buses. Let me outline some thoughts below, which I
think at least solves my problems ( ;) ), and if some of the OMAP folks
and Alan could chime in as to whether they can apply something similar,
or if they still have need of creating actual new bus types?
As we are exploring all of this, let me put a concrete (ish) scenario
out there to discuss:
SUB_BUS1---DEVICE1
/
CPU ---
\
SUB_BUS2---DEVICE2
DEVICE1 and DEVICE2 are the same device (say, usb host controller, or
whatever), and they should be driven by the same driver. However,
physically they are attached to different buses.
SUB_BUS1 and SUB_BUS2 are *different* and let's say require a different
suspend method. If I understand correctly, the right way to build the
topology would be:
struct device_type SUB_BUS1_type = {
.name = "sub-bus1-type",
.pm = &sub_bus1_pm_ops;
};
struct platform_device SUB_BUS1 = {
.init_name = "sub-bus1",
.dev.type = &sub_bus1_type,
};
struct platform_device DEVICE1 = {
.name = "device1",
.dev.parent = &SUB_BUS1.dev,
};
platform_device_register(&SUB_BUS1);
platform_device_register(&DEVICE1);
[analogous for *2]
which would yield:
/sys/bus/platform/devices/
|
|-SUB_BUS1
| |
| |-DEVICE1
|
|-SUB_BUS2
| |
| |-DEVICE2
Which is the correct topology, and logically provides all the correct
interfaces. The only thing that becomes confusing now, is that SUB_BUS*
is *actually* a physically different bus, yet it's not in /sys/bus;
although I suppose this is actually how the world works presently (you
don't see an individual entry in /sys/bus for each usb host controller,
which is technically defining a sub-bus...)
Thoughts? Am I understanding everything correctly?
Is there anything more not covered by co-opting the device-type of SUB_BUS
that people would still need the pseduo-platform-bus extensions for?
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Thu, Aug 5, 2010 at 7:25 PM, Patrick Pannuto <[email protected]> wrote:
> On 08/05/2010 04:16 PM, Grant Likely wrote:
>> The relevant question before going down this path is, "Is the
>> omap/sh/other-soc behaviour something fundamentally different from the
>> platform bus? ?Or is it something complementary that would be better
>> handled with a notifier or some orthogonal method of adding new
>> behaviour?"
>>
>> I don't have a problem with multiple platform_bus instances using the
>> same code (I did suggest it after all), but I do worry about muddying
>> the Linux device model or making it overly complex. ?Binding single
>> drivers to multiple device types could be messy.
>>
>> Cheers,
>> g.
>
> From your other email, the distinction of /bus_types/ versus /physical
> or logical buses/ was very valuable (thanks). I am becoming less
> convinced that I need this infrastructure or the ability to create
> pseudo-platform buses. Let me outline some thoughts below, which I
> think at least solves my problems ( ;) ), and if some of the OMAP folks
> and Alan could chime in as to whether they can apply something similar,
> or if they still have need of creating actual new bus types?
>
>
> As we are exploring all of this, let me put a concrete (ish) scenario
> out there to discuss:
>
> ? ? ? ?SUB_BUS1---DEVICE1
> ? ? ? /
> CPU ---
> ? ? ? \
> ? ? ? ?SUB_BUS2---DEVICE2
>
> DEVICE1 and DEVICE2 are the same device (say, usb host controller, or
> whatever), and they should be driven by the same driver. However,
> physically they are attached to different buses.
>
> SUB_BUS1 and SUB_BUS2 are *different* and let's say require a different
> suspend method. If I understand correctly, the right way to build the
> topology would be:
>
> struct device_type SUB_BUS1_type = {
> ? ? ? ?.name = "sub-bus1-type",
> ? ? ? ?.pm ? = &sub_bus1_pm_ops;
> };
>
> struct platform_device SUB_BUS1 = {
> ? ? ? ?.init_name = "sub-bus1",
> ? ? ? ?.dev.type ?= &sub_bus1_type,
> };
>
> struct platform_device DEVICE1 = {
> ? ? ? ?.name ? ? ? = "device1",
> ? ? ? ?.dev.parent = &SUB_BUS1.dev,
> };
>
> platform_device_register(&SUB_BUS1);
> platform_device_register(&DEVICE1);
>
> [analogous for *2]
Not quite. The device_type is intended to collect similar, but
different things (ie, all keyboards, regardless of how they are
attached to the system). Trying to capture the device-specific
behavour really belongs in the specific device driver attached to the
SUB_BUS* device. In fact, the way you've constructed your example,
SUB_BUS1 and SUB_BUS2 don't really need to be platform_devices at all;
you could have used a plain struct device because in this example you
aren't binding them to a device driver).
That being said, because each bus *does* have custom behaviour, it
probably does make sense to either bind each to a device driver, or to
do something to each child device that changes the PM behaviour. I
haven't dug into the problem domain deep enough to give you absolute
answers, but the devres approach looks promising, as does creating a
derived platform_bus_type (although I'm not fond of sharing device
drivers between multiple bus_types). Another option might be giving
the parent struct device some runtime PM operations that it performs
per-child. I'm just throwing out ideas here though.
> which would yield:
>
> /sys/bus/platform/devices/
> ?|
> ?|-SUB_BUS1
> ?| ?|
> ?| ?|-DEVICE1
> ?|
> ?|-SUB_BUS2
> ?| ?|
> ?| ?|-DEVICE2
You're looking in the wrong place. Look in /sys/devices instead of
/sys/bus... (see comments below)
> Which is the correct topology, and logically provides all the correct
> interfaces. ?The only thing that becomes confusing now, is that SUB_BUS*
> is *actually* a physically different bus, yet it's not in /sys/bus;
> although I suppose this is actually how the world works presently (you
> don't see an individual entry in /sys/bus for each usb host controller,
> which is technically defining a sub-bus...)
/sys/bus is for bus_types, not bus instances. All USB devices will be
listed in /sys/bus/usb/devices regardless of the bus instance that
they attached to.
However, look carefully at the files in /sys/bus/*/devices. Notice
that they are all symlinks? Notice that the all of them point to
directories in /sys/devices? /sys/bus tells you which devices are
registered against each bus type, but /sys/devices is the actual
structure. Always look in /sys/devices when you want to know the
topology of the system.
Cheers,
g.