| Message ID | 4C59E654.1090403@codeaurora.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <ppannuto@codeaurora.org> wrote: > Inspiration for this comes from: > http://www.mail-archive.com/linux-omap@vger.kernel.org/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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Magnus Damm <magnus.damm@gmail.com> writes: > On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <ppannuto@codeaurora.org> wrote: >> Inspiration for this comes from: >> http://www.mail-archive.com/linux-omap@vger.kernel.org/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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/04/2010 07:32 PM, Magnus Damm wrote: > On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <ppannuto@codeaurora.org> wrote: >> Inspiration for this comes from: >> http://www.mail-archive.com/linux-omap@vger.kernel.org/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
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,
Inspiration for this comes from: http://www.mail-archive.com/linux-omap@vger.kernel.org/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 <ppannuto@codeaurora.org> --- drivers/base/platform.c | 30 ++++++++++++++++++++++++++---- include/linux/platform_device.h | 2 ++ 2 files changed, 28 insertions(+), 4 deletions(-)