On 09/04/2012 07:51 PM, ABRAHAM, KISHON VIJAY wrote:
>>>>>>>>>>> Since it's already a common function, we may give phandler property
>>>>>>>>>>> a common name too. So we will not need phandle argument.
>>>>>>>>>>> Please also don't forget to document the devm_xxx and dt binding.
>>>>>>>>>>
>>>>>>>>>> I don't think this is a good idea. If we hardcode the phandle name, we
>>>>>>>>>> introduce a limit of one phy per usb device. The usb3 controllers
>>>>>>>>>> alreadyt use two phys (one for usb2, the othere for usb3) for one
>>>>>>>>>> controller. So I think we should not make the same mistake again.
>>>>>>>> That only means current binding is not good enough. Rather not, means
>>>>>>>> it should not be in common code.
>>>>>>>> Maybe something like:
>>>>>>>> usbport0-phys = <&phy0>;
>>>>>>>> usbport1-phys = <&phy1 &phy2>; /* usb2 & usb3 */
>>>>>>>
>>>>>>> Granted. Do we need strings that describe the phys, like in pinctrl or
>>>>>>> is a index enough? What about this?
>>>>>>>
>>>>>>> struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
>>>>>>> int index)
>>>>>>>
>>>>>>
>>>>>> Comments? The phandle_name string will be "usbphy".
>>>>>
>>>>> I don't think phandle_name should be usbphy. Eventually we want to turn
>>>>> this into a kernel-wide phy subsystem and if we use usbphy, we will just
>>>>> have to patch a bunch of dts files once we make the move.
>>> Coud you please give a link of "kernel-wide phy subsystem" discussion?
>>>>
>>>> Is just "phy" better?
>>> If the property name don't include port number, how do we know what
>>> port the phy is attached to?
>
> We can use something like "xxxx-phy" as the phandle name. And the
> users can get the phy by using
> devm_usb_get_phy_by_phandle(dev, "xxxx").
> (So the frwrk appends *-phy* to the name and searches). Or we don't
> have any restriction on the phandle naming conventions and search for
> the phandle by the name the user passes in devm_usb_get_phy_by_phandle
> directly.
Maintainer, I need a Maintainer. Can someone please decide what we want
to have here. I can code all that, but please someone has to make a
decision. Now, please.
> Btw the regulator framework uses something like "xxxx-supply" and it
> took me sometime to figure out the regulator phandle name should be
> appended with "-supply".
>
> + const char *phandle)
>>
>> Take this ci13xxx-imx dts snippet for example:
>>
>> usb@02184000 { /* USB OTG */
>> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>> reg = <0x02184000 0x200>;
>> interrupts = <0 43 0x04>;
>> phy = <&usbphy1>;
>> };
>>
>> The existing "fsl,usbphy" will be renamed into just "phy". If a usb/otg
>> device needs more than one phy the dts will look like this:
>>
>> usb@02184000 { /* USB OTG */
>> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>> reg = <0x02184000 0x200>;
>> interrupts = <0 43 0x04>;
>> phy = <&usbphy1 &usbphy2>;
>
> Will this work? Can we return two phandles for a single phandle name?
Not two handles with one devm_usb_get_phy_by_phandle call :), but if you
want to get both you do:
phy0 = devm_usb_get_phy_by_phandle(dev, 0);
phy1 = devm_usb_get_phy_by_phandle(dev, 1);
The magic lies in the third parameter of the of_parse_phandle():
of_parse_phandle(dev->of_node, "phy", index);
This even fits into Ravi Babu's use case, two phys are attached to one
device (at least at the DT abstraction level).
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature
