Re: [PATCH v6 07/23] usb: common: add API to update usb otg capabilities by device tree

[Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org>]

On Tue, Jul 07, 2015 at 04:23:14PM +0300, Roger Quadros wrote:
> Hi,
> 
> On 29/06/15 10:47, Li Jun wrote:
> >Check property of usb hardware to update otg version and disable SRP, HNP
> >and ADP if its disable flag is present.
> >
> >Signed-off-by: Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >---
> >  drivers/usb/common/common.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/of.h      |  7 +++++++
> >  2 files changed, 54 insertions(+)
> >
> >diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> >index b530fd4..10c986b 100644
> >--- a/drivers/usb/common/common.c
> >+++ b/drivers/usb/common/common.c
> >@@ -154,6 +154,53 @@ bool of_usb_host_tpl_support(struct device_node *np)
> >  	return false;
> >  }
> >  EXPORT_SYMBOL_GPL(of_usb_host_tpl_support);
> >+
> >+/**
> >+ * of_usb_update_otg_caps - to update usb otg capabilities according to
> >+ * the passed properties in DT.
> >+ * @np: Pointer to the given device_node
> >+ * @otg_caps: Pointer to the target usb_otg_caps to be set
> >+ *
> >+ * The function updates the otg capabilities
> >+ */
> >+int of_usb_update_otg_caps(struct device_node *np,
> >+			struct usb_otg_caps *otg_caps)
> >+{
> >+	u32 otg_rev;
> >+
> >+	if (!otg_caps)
> >+		return -EINVAL;
> >+
> >+	if (!of_property_read_u32(np, "otg-rev", &otg_rev)) {
> >+		switch (otg_rev) {
> >+		case 0x0100:
> >+		case 0x0120:
> >+		case 0x0130:
> >+		case 0x0200:
> >+		case 0x0300:
> 
> we don't support 0x300 yet. I guess it is best to leave it out till
> we have real otg 3.0 support.
> 
okay.

> >+			otg_caps->otg_rev = otg_rev;
> >+			break;
> >+		default:
> >+			pr_err("%s: unsupported otg-rev: 0x%x\n",
> >+						np->full_name, otg_rev);
> >+			return -EINVAL;
> >+		}
> >+	} else {
> >+		otg_caps->otg_rev = 0;
> 
> Why do we set this to 0 here?
> controller might have set a valid otg_rev and we want to
> maintain it if nothing was passed in DT.
> 
Considering otg-rev is mandatory for non-legacy platformsr,
Now you mean what I did in previous version: do not set initial otg-rev
in controller driver, just let it to be 0, and wait it to be updated by DT
or by platform layer code.

Current idea is:
set otg capabilities in controller driver as otg _ability_ like below:
otg-rev = 0x0200;
hnp_support = true;

a) Call to this function to update otg caps if pass by DT, and then/or

b) Update otg caps if some platforms want to update it directly(not by DT). 

If both above 2 update not happen, then I need keep otg-rev to be 0 which will
be used to judge this is a legacy platform, so I set otg-rev to be 0 if it not
passed in DT in a).

We also can do this like my previous version:
In controller driver, only set HNP/SRP/ADP as otg ability:
hnp_support = true;

a) Call to this function to update otg caps if pass by DT, and then/or

b) Update otg caps if some platforms want to update it directly(not by DT). 

If both above 2 update not happen, otg-rev is still 0, so I need not set it
to be 0 if nothing pass in DT.

Either way is okay I think.

Li Jun

> >+	}
> >+
> >+	if (of_find_property(np, "hnp-disable", NULL))
> >+		otg_caps->hnp_support = false;
> >+	if (of_find_property(np, "srp-disable", NULL))
> >+		otg_caps->srp_support = false;
> >+	if (of_find_property(np, "adp-disable", NULL) ||
> >+				(otg_caps->otg_rev < 0x0200))
> >+		otg_caps->adp_support = false;
> >+
> >+	return 0;
> >+}
> >+EXPORT_SYMBOL_GPL(of_usb_update_otg_caps);
> >+
> >  #endif
> 
> cheers,
> -roger
> 
> >
> >  MODULE_LICENSE("GPL");
> >diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> >index cfe0528..8c5a818 100644
> >--- a/include/linux/usb/of.h
> >+++ b/include/linux/usb/of.h
> >@@ -15,6 +15,8 @@
> >  enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
> >  enum usb_device_speed of_usb_get_maximum_speed(struct device_node *np);
> >  bool of_usb_host_tpl_support(struct device_node *np);
> >+int of_usb_update_otg_caps(struct device_node *np,
> >+			struct usb_otg_caps *otg_caps);
> >  #else
> >  static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
> >  {
> >@@ -30,6 +32,11 @@ static inline bool of_usb_host_tpl_support(struct device_node *np)
> >  {
> >  	return false;
> >  }
> >+static inline int of_usb_update_otg_caps(struct device_node *np,
> >+				struct usb_otg_caps *otg_caps)
> >+{
> >+	return 0;
> >+}
> >  #endif
> >
> >  #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
> >
--
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