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 */
> >
> >>> +{
> >>> + 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.
Thanks
Richard
>
> Thanks
> Kishon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
