Re: [PATCHv3 2/2] usb: dwc2: Add reset control to dwc2

[John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>]

On 4/13/2016 7:04 PM, Arnd Bergmann wrote:
> On Thursday 14 April 2016, dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>> @@ -337,6 +338,17 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
>>  {
>>         int i, ret;
>>  
>> +       hsotg->reset = devm_reset_control_get(hsotg->dev, "dwc2");
>> +       if (IS_ERR(hsotg->reset)) {
>> +               if (PTR_ERR(hsotg->reset) == -EPROBE_DEFER)
>> +                       return -EPROBE_DEFER;
>> +               dev_dbg(hsotg->dev, "Could not get reset control.\n");
>> +               hsotg->reset = NULL;
>> +       }
>> +
>> +       if(hsotg->reset)
>> +               reset_control_deassert(hsotg->reset);
> 
> 
> The error handling seems a bit odd here. If there is a failure to get the
> reset control and it's actually needed, I would argue the init function should
> not continue. Conversely, if there was no reset line specified in the
> device, why even print a message about it?

Yes it's optional. I think this needs to be checked like the PHY's are
checked in the same function. That is, by checking for specific error
codes that indicate a reset control is not specified and continue only
in that case. All other errors should be returned.

Dinh, care to make these changes?

As for the message, it's only purpose is to aid debugging.

John
--
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