On 11/28/2012 04:10 AM, the mail apparently from Alan Stern included:
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.
OK.
I wonder if this needs extending to PM ops passing in to the assets.
Regulator is usually self-sufficient but sometimes clocks at least need
meddling in suspend paths.
Maybe it's enough instead to offer a standalone api for drivers that
want to meddle with assets, like enable / disable an asset clock...
void *device_find_asset(struct device *device, const char *name);
...if it wants to touch anything like that it needs to mandate a
nonambiguous name for the asset like "reg-mydriver-ehci-omap.0".
This is also handy since the driver can then adapt around absence or
presence of optional assets if it wants.
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.
OK.
...
};
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.
Good point.
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.
Good point.
...
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.
Right.
I'll offer a series with these adaptations shortly.
-Andy
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
--
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106 -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
