Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

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-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html