Patrick Pannuto <ppannuto@xxxxxxxxxxxxxx> writes: > On 08/04/2010 05:16 PM, Kevin Hilman wrote: >> Patrick Pannuto <ppannuto@xxxxxxxxxxxxxx> writes: >> >>> Inspiration for this comes from: >>> http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
