On 08/29/2012 10:11 PM, Marc Kleine-Budde wrote:
[...]
>>>>>> +struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
>>>>>> + const char *phandle)
>>>>
>>>>> 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".
>>>>>> +{
>>>>>> + struct usb_phy *phy = NULL, **ptr;
>>>>>> + unsigned long flags;
>>>>>> + struct device_node *node;
>>>>>> +
>>>>>> + if (!dev->of_node) {
>>>>>> + dev_dbg(dev, "device does not have a device node entry\n");
>>>>>> + return ERR_PTR(-EINVAL);
>>>>>> + }
>>>>>> +
>>>>>> + node = of_parse_phandle(dev->of_node, phandle, 0);
>>>>>> + if (!node) {
>>>>>> + dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
>>>>>> + dev->of_node->full_name);
>>>>>> + return ERR_PTR(-ENODEV);
>>>>>> + }
>>>>>> +
>>>>>> + ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
>>>>>> + if (!ptr) {
>>>>>> + dev_dbg(dev, "failed to allocate memory for devres\n");
>>>>>> + return ERR_PTR(-ENOMEM);
>>>>>> + }
>>>>>> +
>>>>>> + spin_lock_irqsave(&phy_lock, flags);
>>>>>> +
>>>>>> + phy = __of_usb_find_phy(&phy_list, node);
>>>>>> + if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
>>>>>> + phy = ERR_PTR(-EPROBE_DEFER);
>>>>>> + devres_free(ptr);
>>>>>> + goto err0;
>>>>>> + }
>>>>>> +
>>>>>> + *ptr = phy;
>>>>>> + devres_add(dev, ptr);
>>>>>> +
>>>>>> + get_device(phy->dev);
>>>>>> +
>>>>>> +err0:
>>>>>> + spin_unlock_irqrestore(&phy_lock, flags);
>>>>>> +
>>>>>> + return phy;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(devm_usb_get_phy_by_phandle);
>>>>>> +
>>>>>> +/**
>>>>>> * devm_usb_put_phy - release the USB PHY
>>>>>> * @dev - device that wants to release this phy
>>>>>> * @phy - the phy returned by devm_usb_get_phy()
>>>>>> @@ -158,32 +234,24 @@ EXPORT_SYMBOL(usb_put_phy);
>>>>>> */
>>>>>> int usb_add_phy(struct usb_phy *x, enum usb_phy_type type)
>>>>>> {
>>>>>> - int ret = 0;
>>>>>> unsigned long flags;
>>>>>> struct usb_phy *phy;
>>>>>>
>>>>>> - if (x && x->type != USB_PHY_TYPE_UNDEFINED) {
>>>>>> - dev_err(x->dev, "not accepting initialized PHY %s\n", x->label);
>>>>>> - return -EINVAL;
>>>
>>> how about having
>>> if (x && !x->dev.of_node && x->type != USB_PHY_TYPE_UNDEFINED) {
>>> dev_err(x->dev, "not accepting initialized PHY %s\n", x->label);
>>> return -EINVAL;
>>> }
>>>
>>> By using this we'll return error if the phy device does not have an
>>> of_node. (So it can't get back the phy by phandle).
>> Maybe it worth to create a new set functions. When using DT, the way to
>> add and get phy is totally different.
>
> Getting already will be (get_by_phandle). Do we need a seperate List for
> DT and non DT phys? usb_add_of_phy()? usb_add_dt_phy()?
comments?
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
