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

From: Kevin Hilman
Date: Thu Aug 05 2010 - 11:57:26 EST

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:
>> 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 =;
>>> +}
>>> +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.

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.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at