Re: [PATCH v3 10/11] usb: otg: Add dual-role device (DRD) support

[Roger Quadros <rogerq@ti.com>]

On 17/07/15 12:02, Li Jun wrote:
> On Wed, Jul 08, 2015 at 01:19:36PM +0300, Roger Quadros wrote:
> 
> [...]
> 
>>  struct otg_fsm *usb_otg_register(struct device *parent_dev,
>> -				 struct otg_fsm_ops *fsm_ops)
>> +				 struct otg_fsm_ops *fsm_ops,
>> +				 bool drd_only)
>>  {
>>  	struct otg_data *otgd;
>>  	int ret = 0;
>> @@ -328,7 +482,15 @@ struct otg_fsm *usb_otg_register(struct device *parent_dev,
>>  		goto err_wq;
>>  	}
>>  
>> -	usb_otg_init_timers(otgd);
>> +	otgd->drd_only = drd_only;
>> +	/* For DRD mode we don't need OTG timers */
>> +	if (!drd_only) {
>> +		usb_otg_init_timers(otgd);
>> +
>> +		/* FIXME: we ignore caller's timer ops */
>> +		otgd->fsm_ops.add_timer = usb_otg_add_timer;
>> +		otgd->fsm_ops.del_timer = usb_otg_del_timer;
>> +	}
>>  
>>  	/* save original start host/gadget ops */
>>  	otgd->start_host = fsm_ops->start_host;
>> @@ -338,9 +500,6 @@ struct otg_fsm *usb_otg_register(struct device *parent_dev,
> 
> Your above override will be override back to be fsm_ops's by below copy:
> 
> 	/* create copy of original ops */
> 	otgd->fsm_ops = *fsm_ops;
> 
> So add/del_timer must be override after the copy.

Good catch :).

> 
>>  	/* override ops */
>>  	otgd->fsm_ops.start_host = usb_otg_start_host;
>>  	otgd->fsm_ops.start_gadget = usb_otg_start_gadget;
>> -	/* FIXME: we ignore caller's timer ops */
>> -	otgd->fsm_ops.add_timer = usb_otg_add_timer;
>> -	otgd->fsm_ops.del_timer = usb_otg_del_timer;
>>  	/* set otg ops */
>>  	otgd->fsm.ops = &otgd->fsm_ops;
>>  	otgd->fsm.otg = &otgd->otg;
>> @@ -443,8 +602,10 @@ static void usb_otg_stop_fsm(struct otg_fsm *fsm)
>>  	otgd->fsm_running = false;
>>  
>>  	/* Stop state machine / timers */
>> -	for (i = 0; i < ARRAY_SIZE(otgd->timers); i++)
>> -		hrtimer_cancel(&otgd->timers[i].timer);
>> +	if (!otgd->drd_only) {
>> +		for (i = 0; i < ARRAY_SIZE(otgd->timers); i++)
>> +			hrtimer_cancel(&otgd->timers[i].timer);
>> +	}
>>  
>>  	flush_workqueue(otgd->wq);
>>  	fsm->otg->state = OTG_STATE_UNDEFINED;
>> diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
>> index 22d8baa..ae9c30a 100644
>> --- a/include/linux/usb/otg-fsm.h
>> +++ b/include/linux/usb/otg-fsm.h
>> @@ -48,6 +48,11 @@ enum otg_fsm_timer {
>>  /**
>>   * struct otg_fsm - OTG state machine according to the OTG spec
>>   *
>> + * DRD mode hardware Inputs
>> + *
>> + * @id:		TRUE for B-device, FALSE for A-device.
>> + * @vbus:	VBUS voltage in regulation.
>> + *
>>   * OTG hardware Inputs
>>   *
>>   *	Common inputs for A and B device
>> @@ -122,7 +127,8 @@ enum otg_fsm_timer {
>>   */
>>  struct otg_fsm {
>>  	/* Input */
>> -	int id;
>> +	int id;			/* DRD + OTG */
>> +	int vbus;		/* DRD only */
> 
> Existing b_sess_vld can be also used for drd only case, no need create
> a new flag.

b_sess_vld is a bit confusing to people not familiar with OTG.
My suggestion is to use dedicated 'vbus' flag for DRD case
for simplicity.

> 
>>  	int adp_change;
>>  	int power_up;
>>  	int a_srp_det;

cheers,
-roger