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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included:
On Wed, 28 Nov 2012, Andy Green wrote:

Greg's advice was simply not to rely on pathnames in sysfs because they
aren't fixed in stone.  That leaves plenty of other ways to approach
this problem.

It's sage advice, but there is zero code provided in my patches that
"relies on pathnames in sysfs".

In your 1/5 patch, _device_path_generate() concatenates device name
strings, starting from a device root and separating elements with '/'
characters.  Isn't that the same as a sysfs pathname?

It's nothing to do with sysfs... yes some unrelated bits of sysfs also walk the device path. If we want to talk about how fragile the device path is as an id scheme over time we need to talk about likelihood of individual device names changing, not "sysfs". Anyway -->

Basically, what you want is for something related to device A (the
regulator or the GPIO) to happen whenever device B (the ehci-omap.0
platform device) is bound to a driver.  The most straightforward way to
arrange this is for A's driver to have a callback that is invoked
whenever B is bound or unbound.  The most straightforward way to
arrange _that_ is to allow each platform_device to have a list of
callbacks.

Sorry I didn't really understand this proposal yet.  You want "A", the
regulator, driver to grow a callback function that gets called when the
targeted platform_device ("B", ehci-omap.0) probe happens.  Could you
expand what the callback prototype or new members in the struct might
look like?  It's your tuple thing or we pass it an opaque pointer that
is the struct regulator * or suchlike?

Well, it won't be exactly the same as the tuple thing because no
strings will be involved, but it would be similar.  The callback would
receive an opaque pointer (presumably to the regulator) and a device
pointer (the B device).

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);
	};

	struct device {
	...
		struct device_asset *assets;
	...
	};


drivers/base/dd.c | really_probe():

...
	struct device_asset *asset;
...
	asset = dev->assets;
	while (asset && asset->name) {
		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()?)

...
	struct device_asset *asset;
...

	/* call device ->remove() */
...
	asset = dev->assets;
	while (asset && asset->name) {
		asset->handler(dev, AE_REMOVED, asset);
		asset++;
	}
...


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;
		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.

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. 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.

As for what Felipe said...  He suggested doing this when the hub driver
binds to the controller's root hub.  The root hub is created when the
controller's driver registers the new USB bus.  This happens as part of
the driver's probe routine.  So what I have been talking about is very
similar (in terms of when it happens) to what Felipe wanted.

Besides, Felipe wasn't thinking in the most general terms.  (In fact,
at first he confused the root hub with the LAN95xx's hub.)  There's no
reason to restrict this sort of thing to USB hubs (or to regulators,
for that matter).  The driver core is the right place for it.

Sounds good to me.

-Andy

--
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux