Re: [PATCH v4 07/13] usb: otg: add OTG core

[Roger Quadros]

On 10/09/15 12:28, Li Jun wrote:
> On Wed, Sep 09, 2015 at 01:01:14PM +0300, Roger Quadros wrote:
> ... ...
> 
>>>>>> +	return -EINVAL;
>>>>>
>>>>> Return non-zero, then if err, do we need call usb_otg_add_hcd() after
>>>>> usb_otg_register_hcd() fails?
>>>>
>>>> You should not call usb_otg_register_hcd() but usb_add_hcd().
>>>> If that fails then you fail as ususal.
>>>
>>> My point is if we use usb_add_hcd(), but failed in usb_otg_register_hcd(),
>>> then usb_otg_add_hcd() will be called in *all* error case, is this your
>>> expectation?
>>> 	if (usb_otg_register_hcd(hcd, irqnum, irqflags, &otg_hcd_intf))
>>> 		return usb_otg_add_hcd(hcd, irqnum, irqflags);
>>>
>>
>> Yes, my intention was that if otg fails then it is a non otg HCD so register normally.
>> Let me correct my previous statement. If you are absolutely sure
>> that the HCD is for otg/dual-role usage then you should call usb_otg_register_hcd().
>>
> 
> I think this is not just about a statement, in your usb_otg_register_hcd()
> implementation, there are several places will return error, I think only
> the first two are for a non-otg HCD case, the following error cases seems
> mean this is for otg usage, but it fails in middle of registration, if that
> is the case, is it reasonable to call usb_otg_add_hcd()?

OK. We need to check the return value then and differentiate if it is non-otg
or otg with failure.
If it is non-otg then only we must call usb_otg_add_hcd().

I will fix usb_add_hcd().

> 
>>>>>> + * @fsm_running: state machine running/stopped indicator
>>>>>> + */
>>>>>>  struct usb_otg {
>>>>>>  	u8			default_a;
>>>>>>  
>>>>>>  	struct phy		*phy;
>>>>>>  	/* old usb_phy interface */
>>>>>>  	struct usb_phy		*usb_phy;
>>>>>> +
>>>>>
>>>>> add a blank line?
>>>>>
>>>
>>> You missed this.
>>
>> Sorry. Did you suggest to remove that blank line
>> or add a new one before usb_phy?
>>
> 
> Remove it.

OK.

> 
>>>
>>>>>>  	struct usb_bus		*host;
>>>>>>  	struct usb_gadget	*gadget;
>>>>>>  
>>>>>>  	enum usb_otg_state	state;
>>>>>>  
>>>>>> +	struct device *dev;
>>>>>> +	struct usb_otg_caps *caps;
>>>>>> +	struct otg_fsm fsm;
>>>>>> +	struct otg_fsm_ops fsm_ops;
>>>>>> +
>>>>>> +	/* internal use only */
>>>>>> +	struct otg_hcd primary_hcd;
>>>>>> +	struct otg_hcd shared_hcd;
>>>>>> +	struct otg_gadget_ops *gadget_ops;
>>>>>> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
>>>>>> +	struct list_head list;
>>>>>> +	struct work_struct work;
>>>>>> -- 
>>>>>> 2.1.4
>>>
--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/