On 08/29/2012 05:14 AM, Richard Zhao wrote:
> On Tue, Aug 28, 2012 at 07:43:11PM +0530, ABRAHAM, KISHON VIJAY wrote:
>> Hi,
>>
>> On Tue, Aug 28, 2012 at 7:11 PM, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
>>> On 08/24/2012 08:46 AM, Richard Zhao wrote:
>>> [...]
>>>>> /**
>>>>> + * devm_usb_get_phy_by_phandle - find the USB PHY by phandle
>>>>> + * @dev - device that requests this phy
>>>>> + * @phandle - name of the property holding the phy phandle value
>>>>> + *
>>>>> + * Returns the phy driver associated with the given phandle value,
>>>>> + * after getting a refcount to it, -ENODEV if there is no such phy or
>>>>> + * -EPROBE_DEFER if there is a phandle to the phy, but the device is
>>>>> + * not yet loaded. While at that, it also associates the device with
>>>>> + * the phy using devres. On driver detach, release function is invoked
>>>>> + * on the devres data, then, devres data is freed.
>>>>> + *
>>>>> + * For use by USB host and peripheral drivers.
>>>>> + */
>>>>> +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)
>>>
>>>>> +{
>>>>> + 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()?
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
