On Tuesday 23 of July 2013 13:50:07 Greg KH wrote: > 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" :) Keep in mind that those virtual devices are created by PHY driver bound to a real device and one real device can have multiple virtual devices behind it. > > 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. Yes, in regulator core consumer names are completely separated from this. Regulator core simply assigns a sequential integer ID to each regulator and registers /sys/class/regulator/regulator.ID for each regulator. > 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" :) The tag alone is not a key. Lookup key consists of two components, consumer device name and consumer tag. What kind of duplicate tags can be a problem here? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html