On Wed, 28 Nov 2012, Andy Green wrote:
> OK. So I try to sketch it out iteractively to try to get in sync:
>
> device.h:
>
> enum asset_event {
> AE_PROBED,
> AE_REMOVED
> };
>
> struct device_asset {
> char *name; /* name of regulator, clock, etc */
> void *asset; /* regulator, clock, etc */
> int (*handler)(struct device *dev_owner, enum asset_event asset_event,
> struct device_asset *asset);
> };
Another possibility is to have two handlers: one for pre-probe and the
other for post-remove. Then of course asset_event wouldn't be needed.
Linus tends to prefer this strategy -- separate functions for separate
events. That's why struct dev_pm_ops has so many method pointers.
> struct device {
> ...
> struct device_asset *assets;
Or a list instead of a NULL-terminated array. It depends on whether
people will want to add or remove assets dynamically. For now an array
would be fine.
> ...
> };
>
>
> drivers/base/dd.c | really_probe():
>
> ...
> struct device_asset *asset;
> ...
> asset = dev->assets;
> while (asset && asset->name) {
Maybe it would be better to test asset->handler. Some assets might not
need names.
> if (asset->handler(dev, AE_PROBED, asset)) {
> /* clean up and bail */
> }
> asset++;
> }
>
> /* do probe */
> ...
>
>
> drivers/base/dd.c | __device_release_driver: (is this really the best
> place to oppose probe()?)
The right place is just after the remove method is called, so yes.
> ...
> struct device_asset *asset;
> ...
>
> /* call device ->remove() */
> ...
> asset = dev->assets;
> while (asset && asset->name) {
> asset->handler(dev, AE_REMOVED, asset);
> asset++;
> }
It would be slightly safer to iterate in reverse order.
> ...
>
>
> board file:
>
> static struct regulator myreg = {
> .name = "mydevice-regulator",
> };
>
> static struct device_asset mydevice_assets[] = {
> {
> .name = "mydevice-regulator",
> .handler = regulator_default_asset_handler,
> },
> { }
> };
>
> static struct platform_device mydevice = {
> ...
> .dev = {
> .assets = mydevice_assets,
> },
> ...
> };
>
>
> regulator code:
>
> int regulator_default_asset_handler(struct device *dev_owner, enum
> asset_event asset_event, struct device_asset *asset)
> {
> struct regulator **reg = &asset->asset;
> int n;
>
> switch (asset_event) {
> case AE_PROBED:
> *reg = regulator_get(dev_owner, asset->name);
> if (IS_ERR(*reg))
> return *reg;
PTR_ERR.
> n = regulator_enable(*reg);
> if (n < 0)
> regulator_put(*reg);
> return n;
>
> case AE_REMOVED:
> regulator_put(*reg);
> break;
> }
>
> return 0;
> }
> EXPORT_SYMBOL_GPL(regulator_default_asset_handler);
>
>
> The subsystems that can expect to get used (clock...) might each want to
> define a default handler like the one for regulator. That'll be an end
> to the code duplication issue. The user can still do his own handler if
> he wants.
>
> I put a name field in so we can use regulator_get() nicely, we don't
> need access to the object pointer or that it exists at boardfile-time
> that way either. But I can see it's arguable.
It hadn't occurred to me, but it seems like a good idea.
Yes, overall this is almost exactly what I had in mind.
> >> Throwing out the path stuff and limiting this to platform_device means
> >> you cannot bind to dynamically created objects like hub or anything
> >> downstream of a hub. So Felipe's identification of the hub as the
> >> happening place to do this is out of luck.
> >
> > Greg pointed out that this could be useful for arbitrary devices, not
> > just platform devices, so it could be applied to dynamically created
> > objects.
>
> Well that is cool, but to exploit that in the dynamic object case
> arrangements for identifying the appropriate object has appeared are
> needed.
For example, this scheme wouldn't lend itself easily to associating the
regulator with the particular root-hub port that the LAN95xx is
attached to. I can't think of any reasonable way to do that other than
the approaches we have already considered.
> We have a nice array of platform_devices nicely there in the
> board file we can attach assets to like "pdev[1].dev.assets = xxx;" but
> that's not there in dynamic device case. Anyway this sounds like what
> we're discussing can be well worth establishing and might lead to that
> later.
Agreed.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
