Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

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

On Fri, Jun 12, 2015 at 11:31:02AM +0300, Roger Quadros wrote:
> 
> 
> On Fri, 12 Jun 2015 11:09:17 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
[...]

> > Other than those new flags, I have not found other good way to judge
> > whether some platform is a legacy one.
> > Directly use dt property? Then above code has to be moved to controller
> > driver, consequently the usb_otg_descriptor has to be allocated by
> > controller driver at runtime, and I have to create some new way to pass it
> > to gadget driver, for those legacy platform, I still need gadget driver to
> > allocate it...too complicated.
> 
> Right. let's not do that.
> 
> > 
> > (You know, there are 2 different otg_desc structures: otg_desc and
> > otg_20_desc, I need select one and allocate it at runtime)
> > 
> > > This also has a side effect of SRP and HNP being enabled for any platform
> > > even if enable-srp/enable-hnp is not set in DT.
> > That's the current situation before my patch, not the side effect
> > brought by me.
> 
> Agreed. I was thinking of fixing that side effect but if you want
> legacy behaviour as well then it looks like it can't be done.
> 
> > 
> > > This will be more of a bug than supporting legacy users.
> > I have to leave the _existing_ bug there, because I can't know
> > which platform can really support HNP/SRP, which one cannot.
> > So I do not fix the _existing_ bug, meanwhile I also do not introduce
> > a new bug either. If some legacy platform with this bug, want to fix it,
> > fine, use dt or other way to set gadget->xyz_support correctly
> > in its controller driver, no more change needed.
> > 
> > Any other reason you think enable flags are still not reasonable?
> 
> Yes. We can't specify that we don't want all 3 features from DT.
> It will treat it as legacy and enable HNP/SRP. ;)
> 
> This is true for dual-role devices. We set dr_mode = "otg"
> but disable all 3 features.
> 
> > 
> > > 
> > > Instead we could have ADP disabled by default for all cases
> > > and expect enable-adp in DT to get it enabled. SRP/HNP could still
> > > be disable flags.
> > > 
> > Yes, this can work, but seems they look some odd:), some are xxx_disable,
> > some are xxx_enable.
> >  
> > > Then your above code reduces to
> > > 
> > > 		if (gadget->adp_support)
> > >  			otg_desc->bmAttributes |= USB_OTG_ADP;
> > >  		if (gadget->hnp_support)
> > >  			otg_desc->bmAttributes |= USB_OTG_HNP;
> > >  		if (gadget->srp_support)
> > >  			otg_desc->bmAttributes |= USB_OTG_SRP;
> > >
> > Yes if those code is put in controller driver of new platforms.
> > No if we put it in common routine and called by both new and
> > legacy platforms.
> 
> Agreed. We still need to determine legacy platform if
> none of the features are set.
> 
> How about defining the followong enum for gadget->xyz_support
> 
> enum otg_feature {
> 	OTG_FEATURE_UNDEFINED = 0,
> 	OTG_FEATURE_ENABLED,
> 	OTG_FEATURE_DISABLED,
> };
> 
> for legacy platforms this will be UNDEFINED for all so you can detect it
> and continue legacy behaviour i.e. enable HNP/SRP, disable ADP.
> 
> For new platforms at least one of them won't be UNDEFINED so you can
> enable the feature if ENABLED and disable if UNDEFINED/DISABLED.
> 

Then some legacy platform will have problem if it wants to use those
new properties and flags, the existing DTs cannot work as before, E.g. for
some controller driver, before my patch, the HNP/SRP are enabled,
ADP is disabled, after utilize those new properties and flags in its
controller driver, it becomes a new platform, but the existing DTs
using this controller driver did not pass any xyz_disable, so
gadget->xyz_support will be updated to be OTG_FEATURE_ENABLED, and
the result will be HNP/SRP/ADP all are enabled.

In some controller driver:
if (xyz_disable_property_appear_in_dt())
	gadget->xyz_support = OTG_FEATURE_DISABLED;
else
	gadget->xyz_support = OTG_FEATURE_ENABLED;
The existing/old DT with this controller driver will enable all
OTG features no matter it support it or not, but it does not want
ADP.
 
Li Jun
> We can still keep disable flags so that users can disable all 3 OTG features
> while in OTG mode.
> 
> cheers,
> -roger
> 
> > > 
> > > > 
> 
--
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