Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

[Roger Quadros <rogerq@ti.com>]

Hi,

On 27/04/16 06:15, Peter Chen wrote:
> On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
>> On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
>>>> Sent: Tuesday, April 26, 2016 2:28 PM
>>>> To: Jun Li <jun.li@nxp.com>
>>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
>>>> balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
>>>> dan.j.williams@intel.com; jun.li@freescale.com;
>>>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
>>>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>
>>>> On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
>>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
>>>>>> To: Jun Li <jun.li@nxp.com>
>>>>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
>>>>>> balbi@kernel.org; gregkh@linuxfoundation.org;
>>>>>> peter.chen@freescale.com; dan.j.williams@intel.com;
>>>>>> jun.li@freescale.com; mathias.nyman@linux.intel.com;
>>>>>> tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org;
>>>>>> r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>>>
>>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
>>>>>>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>>>>>>> +				 struct usb_otg_config *config) {
>>>>>>>> +	struct usb_otg *otg;
>>>>>>>> +	struct otg_wait_data *wait;
>>>>>>>> +	int ret = 0;
>>>>>>>> +
>>>>>>>> +	if (!dev || !config || !config->fsm_ops)
>>>>>>>> +		return ERR_PTR(-EINVAL);
>>>>>>>> +
>>>>>>>> +	/* already in list? */
>>>>>>>> +	mutex_lock(&otg_list_mutex);
>>>>>>>> +	if (usb_otg_get_data(dev)) {
>>>>>>>> +		dev_err(dev, "otg: %s: device already in otg list\n",
>>>>>>>> +			__func__);
>>>>>>>> +		ret = -EINVAL;
>>>>>>>> +		goto unlock;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	/* allocate and add to list */
>>>>>>>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>>>>>>> +	if (!otg) {
>>>>>>>> +		ret = -ENOMEM;
>>>>>>>> +		goto unlock;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	otg->dev = dev;
>>>>>>>> +	otg->caps = config->otg_caps;
>>>>>>>> +
>>>>>>>> +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>>>>>>> +	     otg->caps->adp_support) && !config->otg_work)
>>>>>>>> +		dev_info(dev, "otg: limiting to dual-role\n");
>>>>>>>
>>>>>>> dev_err, this should be an error.
>>>>>>
>>>>>> The condition may be wrong, but it is an information to show that
>>>>>> current OTG is dual-role.
>>>>>
>>>>> This should not happen in any correct design, I even doubt if we
>>>>> should try to continue by "downgrade" it to be duel role, currently
>>>>> the only example user is dual role, so doing like this can't be tested
>>>>> by real case, this downgrade is not so easy like we image, at least
>>>>> for chipidea otg driver, simply replace a queue worker may not work,
>>>>> as we have much more difference between the 2 configs.
>>>>>
>>>>
>>>> Would you show more why chipidea can't work just replace the work item,
>>>> and see if anything we still can improve for this framework?
>>>
>>> In real OTG, we need enable AVV irq,
>>
>> Enable and Handling AVV is platform stuff. In this framework, we are
>> focus on how otg device manages host and gadget together, and the state
>> machine when the related otg event occurs.
>>
>>> but for duel role, nobody care/handle,
>>> there are much more resource required for OTG: timers, hnp polling,
>>> otg test device handling... 
>>
>> They are common things for fully OTG fsm, you can move them
>> to common code (In fact, hnp polling handling is already common code).
>>
>>>
>>> with current design, chipidea driver can support real OTG with its own
>>> queue worker, or DRD with Roger's drd work item if config is correct.
>>>
>>> But improve something to work on a *wrong* config will make it complicated
>>> and does not make much sense IMO.
>>>
>>
>> What does above "config" you mean?
>>
>> If the configure is fully OTG, you can choose different state machine,
>> eg otg_statemachine, if you find it is hard for chipidea to use this
>> framework, just list the reason, and see if we can improve.
>>
> 
> Roger, after discussing with Jun off line, we think usb_otg_register
> should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
> ADP), since this patch set does not cover fully otg features, the users

But this series isn't preventing full otg implementation. You can
still do that via config->otg_work.

I can modify the following condition to return -ENOTSUPP instead of
defaulting to dual-role

struct usb_otg *usb_otg_register(...)
{
...
        if ((otg->caps->hnp_support || otg->caps->srp_support ||
             otg->caps->adp_support) && !config->otg_work) {
		dev_err(dev, "otg: otg_work must be provided for OTG support\n");
		return -ENOTSUPP;
	}
...
}

Is this sufficient?

> should not be confused when try to implement fully otg using this
> framework.
> 
> Later, after your patch set is merged, we can add fully OTG features
> using this framework, and remove this check.
> 
> What's your opinion?
> 

--
cheers,
-roger