Prev: linux-next: manual merge of the genesis tree with Linus' tree
Next: [PATCH] checkpatch: Fix extraneous EXPORT_SYMBOL* warnings
From: Patrick Pannuto on 4 Aug 2010 21:00
On 08/04/2010 05:16 PM, Kevin Hilman wrote:
> Patrick Pannuto <ppannuto(a)codeaurora.org> writes:
>
>> Inspiration for this comes from:
>> http://www.mail-archive.com/linux-omap(a)vger.kernel.org/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/linux-omap(a)vger.kernel.org/msg31289.html
>
>
> From b784009af1d0a7065dc5e58a13ce29afa3432d3e Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman(a)deeprootsystems.com>
> 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 <khilman(a)deeprootsystems.com>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Kevin Hilman on 5 Aug 2010 11:30
Magnus Damm <magnus.damm(a)gmail.com> writes:

> On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <ppannuto(a)codeaurora.org> wrote:
>> Inspiration for this comes from:
>> http://www.mail-archive.com/linux-omap(a)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 linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Patrick Pannuto on 5 Aug 2010 12:40
>>>> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: linux-next: manual merge of the genesis tree with Linus' tree
Next: [PATCH] checkpatch: Fix extraneous EXPORT_SYMBOL* warnings