Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Greg KH]

On Tue, Jul 23, 2013 at 10:07:52PM +0200, Tomasz Figa wrote:
> On Tuesday 23 of July 2013 12:44:23 Greg KH wrote:
> > On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
> > > > You don't "know" the id of the device you are looking up, due to
> > > > multiple devices being in the system (dynamic ids, look back earlier
> > > > in
> > > > this thread for details about that.)
> > > 
> > > I got copied in very late so don't have most of the thread I'm afraid,
> > > I did try looking at web archives but didn't see a clear problem
> > > statement.  In any case this is why the APIs doing lookups do the
> > > lookups in the context of the requesting device - devices ask for
> > > whatever name they use locally.
> > 
> > What do you mean by "locally"?
> > 
> > The problem with the api was that the phy core wanted a id and a name to
> > create a phy, and then later other code was doing a "lookup" based on
> > the name and id (mushed together), because it "knew" that this device
> > was the one it wanted.
> > 
> > Just like the clock api, which, for multiple devices, has proven to
> > cause problems.  I don't want to see us accept an api that we know has
> > issues in it now, I'd rather us fix it up properly.
> > 
> > Subsystems should be able to create ids how ever they want to, and not
> > rely on the code calling them to specify the names of the devices that
> > way, otherwise the api is just too fragile.
> > 
> > I think, that if you create a device, then just carry around the pointer
> > to that device (in this case a phy) and pass it to whatever other code
> > needs it.  No need to do lookups on "known names" or anything else,
> > just normal pointers, with no problems for multiple devices, busses, or
> > naming issues.
> 
> PHY object is not a device, it is something that a device driver creates 
> (one or more instances of) when it is being probed.

But you created a 'struct device' for it, so I think of it as a "device"
be it "virtual" or "real" :)

> You don't have a clean way to export this PHY object to other driver,
> other than keeping this PHY on a list inside PHY core with some
> well-known ID (e.g. device name + consumer port name/index, like in
> regulator core) and then to use this well-known ID inside consumer
> driver as a lookup key passed to phy_get();
> 
> Actually I think for PHY case, exactly the same way as used for
> regulators might be completely fine:
> 
> 1. Each PHY would have some kind of platform, non-unique name, that is 
> just used to print some messages (like the platform/board name of a 
> regulator).
> 2. Each PHY would have an array of consumers. Consumer specifier would 
> consist of consumer device name and consumer port name - just like in 
> regulator subsystem.
> 3. PHY driver receives an array of, let's say, phy_init_data inside its 
> platform data that it would use to register its PHYs.
> 4. Consumer drivers would have constant consumer port names and wouldn't 
> receive any information about PHYs from platform code.
> 
> Code example:
> 
> [Board file]
> 
> static const struct phy_consumer_data usb_20_phy0_consumers[] = {
> 	{
> 		.devname = "foo-ehci",
> 		.port = "usbphy",
> 	},
> };
> 
> static const struct phy_consumer_data usb_20_phy1_consumers[] = {
> 	{
> 		.devname = "foo-otg",
> 		.port = "otgphy",
> 	},
> };
> 
> static const struct phy_init_data my_phys[] = {
> 	{
> 		.name = "USB 2.0 PHY 0",
> 		.consumers = usb_20_phy0_consumers,
> 		.num_consumers = ARRAY_SIZE(usb_20_phy0_consumers),
> 	},
> 	{
> 		.name = "USB 2.0 PHY 1",
> 		.consumers = usb_20_phy1_consumers,
> 		.num_consumers = ARRAY_SIZE(usb_20_phy1_consumers),
> 	},
> 	{ }
> };
> 
> static const struct platform_device usb_phy_pdev = {
> 	.name = "foo-usbphy",
> 	.id = -1,
> 	.dev = {
> 		.platform_data = my_phys,
> 	},
> };
> 
> [PHY driver]
> 
> static int foo_usbphy_probe(pdev)
> {
> 	struct foo_usbphy *foo;
> 	struct phy_init_data *init_data = pdev->dev.platform_data;
> 	/* ... */
> 	// for each PHY in init_data {
> 		phy_register(&foo->phy[i], &init_data[i]);
> 	// }
> 	/* ... */
> }
> 
> [EHCI driver]
> 
> static int foo_ehci_probe(pdev)
> {
> 	struct phy *phy;
> 	/* ... */
> 	phy = phy_get(&pdev->dev, "usbphy");
> 	/* ... */
> }
> 
> [OTG driver]
> 
> static int foo_otg_probe(pdev)
> {
> 	struct phy *phy;
> 	/* ... */
> 	phy = phy_get(&pdev->dev, "otgphy");
> 	/* ... */
> }

That's not so bad, as long as you let the phy core use whatever name it
wants for the device when it registers it with sysfs.  Use the name you
are requesting as a "tag" or some such "hint" as to what the phy can be
looked up by.

Good luck handling duplicate "tags" :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/