Re: [PATCH 2/3] usb: otg: add device tree support to otg library

Hi,

On Sat, Aug 25, 2012 at 11:43 PM, Marc Kleine-Budde [off-list ref] wrote:

On 08/24/2012 08:46 AM, Richard Zhao wrote:

quoted

Did you try both enableing and disabing DT pass build?

Impossible on mx28 :) The platform always selects USE_DT, but on imx it
builds without DT support.

quoted

On Thu, Aug 23, 2012 at 07:22:53PM +0200, Marc Kleine-Budde wrote:

quoted

From: Kishon Vijay Abraham I <redacted>

This patch adds an API to get usb phy by passing a device node phandle value.
The new added devm_usb_get_phy_by_phandle() function will return a pointer to
the phy on success, -EPROBE_DEFER if there is a device_node for the phandle,
but the phy has not been added, or a ERR_PTR() otherwise.

Since it's possible to obtain a phy by phandle, the checks in usb_add_phy() for
a valid phy type is removed (now it's just a debug message if a user tries to
add a phy with undefined type). This also allows to add multiple phys of same
type.

Cc: Richard Zhao <redacted>
Cc: Marek Vasut <redacted>
Signed-off-by: Kishon Vijay Abraham I <redacted>
Signed-off-by: Marc Kleine-Budde <redacted>
---
 drivers/usb/otg/otg.c   |   96 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/usb/otg.h |    8 ++++
 2 files changed, 90 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/otg/otg.c b/drivers/usb/otg/otg.c
index 98c430e..23618de 100644
--- a/drivers/usb/otg/otg.c
+++ b/drivers/usb/otg/otg.c

@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/of.h>

 #include <linux/usb/otg.h>

@@ -36,6 +37,21 @@ static struct usb_phy *__usb_find_phy(struct list_head *list,
     return ERR_PTR(-ENODEV);
 }

+static struct usb_phy *__of_usb_find_phy(struct list_head *list,
+    struct device_node *node)
+{
+    struct usb_phy  *phy;
+
+    list_for_each_entry(phy, list, head) {
+            if (node != phy->dev->of_node)
+                    continue;
+
+            return phy;
+    }
+
+    return ERR_PTR(-ENODEV);
+}
+
 static void devm_usb_phy_release(struct device *dev, void *res)
 {
     struct usb_phy *phy = *(struct usb_phy **)res;

@@ -112,6 +128,66 @@ err0:
 EXPORT_SYMBOL(usb_get_phy);

 /**
+ * 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)

quoted

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.

Good point. This is the device tree snippet from imx28.dtsi:

quoted

        usb0: usb@80080000 {
                compatible = "fsl,imx28-usb", "fsl,imx27-usb";
                reg = <0x80080000 0x10000>;
                interrupts = <93>;
                fsl,usbphy = <&usbphy0>;

                  ^^^^

What about removing the "fsl,", so it just would be "usbphy".

quoted

                status = "disabled";
        };

quoted

quoted

+{
+    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;

quoted

why do you remove re-intialize check? Maybe you can add a
USB_PHY_TYPE_DT and below logic will be more clear.

I don't know why this check is removed in Kishon's original patch. From
my point of view all these checks can be removed. Kishon can you explain
the rationale behind this?

I dint remove the check actually. I just avoided the function from
returning -EINVAL if the phy type is UNDEFINED since now the phy can
be obtained by phandle (even if it has undefined phy type). I still
have the check to *warn* if the phy type is undefined (see below).

regards, Marc

quoted

quoted

-    }
+    if (x && x->type != USB_PHY_TYPE_UNDEFINED)
+            dev_dbg(x->dev, "add a phy with undefined type %s\n", x->label);

     spin_lock_irqsave(&phy_lock, flags);

-    list_for_each_entry(phy, &phy_list, head) {
-            if (phy->type == type) {
-                    ret = -EBUSY;
-                    dev_err(x->dev, "transceiver type %s already exists\n",
+    list_for_each_entry(phy, &phy_list, head)
+            if (phy->type == type)
+                    dev_dbg(x->dev, "transceiver type %s already exists\n",
                                             usb_phy_type_string(type));
-                    goto out;
-            }
-    }

     x->type = type;
     list_add_tail(&x->head, &phy_list);

-out:
     spin_unlock_irqrestore(&phy_lock, flags);
-    return ret;
+    return 0;
 }
 EXPORT_SYMBOL(usb_add_phy);

diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index 45824be..602d6b4 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h

@@ -190,6 +190,8 @@ usb_phy_shutdown(struct usb_phy *x)
 extern struct usb_phy *usb_get_phy(enum usb_phy_type type);
 extern struct usb_phy *devm_usb_get_phy(struct device *dev,
     enum usb_phy_type type);
+extern struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
+    const char *phandle);
 extern void usb_put_phy(struct usb_phy *);
 extern void devm_usb_put_phy(struct device *dev, struct usb_phy *x);
 extern const char *otg_state_string(enum usb_otg_state state);

@@ -205,6 +207,12 @@ static inline struct usb_phy *devm_usb_get_phy(struct device *dev,
     return NULL;
 }

+static inline struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
+    const char *phandle)
+{
+    return NULL;
+}
+
 static inline void usb_put_phy(struct usb_phy *x)
 {
 }

Thanks
Richard

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
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   |

Thanks
Kishon