On 4/25/2016 12:30 AM, Philipp Zabel wrote:
> Hi John,
>
> Am Freitag, den 22.04.2016, 20:31 -0700 schrieb John Youn:
>> On 4/20/2016 2:31 PM, dinguyen@xxxxxxxxxxxxxxxxxxxxx wrote:
>>> From: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx>
>>>
>>> Allow for platforms that have a reset controller driver in place to bring
>>> the USB IP out of reset.
>>>
>>> Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> v6: fix 80 line checkpatch warning in dev_err print
>>> v5: updated error conditions for not finding the reset property
>>> v4: use dev_dbg() if not a -EPROBE_DEFER
>>> v3: fix compile error
>>> v2: move to lowlevel_hw_init()
>>> ---
>>> drivers/usb/dwc2/core.h | 1 +
>>> drivers/usb/dwc2/platform.c | 21 +++++++++++++++++++++
>>> 2 files changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 3c58d63..f748132 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -837,6 +837,7 @@ struct dwc2_hsotg {
>>> void *priv;
>>> int irq;
>>> struct clk *clk;
>>> + struct reset_control *reset;
>>>
>>> unsigned int queuing_high_bandwidth:1;
>>> unsigned int srp_success:1;
>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>> index 88629be..fa2c097 100644
>>> --- a/drivers/usb/dwc2/platform.c
>>> +++ b/drivers/usb/dwc2/platform.c
>>> @@ -45,6 +45,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/phy/phy.h>
>>> #include <linux/platform_data/s3c-hsotg.h>
>>> +#include <linux/reset.h>
>>>
>>> #include <linux/usb/of.h>
>>>
>>> @@ -337,6 +338,23 @@ 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)) {
>>> + ret = PTR_ERR(hsotg->reset);
>>> + switch (ret) {
>>> + case -ENOENT:
>>> + hsotg->reset = NULL;
>>> + break;
>>> + default:
>>> + dev_err(hsotg->dev, "error getting reset control %d\n",
>>> + ret);
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + if (hsotg->reset)
>>> + reset_control_deassert(hsotg->reset);
>>> +
>>> /* Set default UTMI width */
>>> hsotg->phyif = GUSBCFG_PHYIF16;
>>>
>>> @@ -434,6 +452,9 @@ static int dwc2_driver_remove(struct platform_device *dev)
>>> if (hsotg->ll_hw_enabled)
>>> dwc2_lowlevel_hw_disable(hsotg);
>>>
>>> + if (hsotg->reset)
>>> + reset_control_assert(hsotg->reset);
>>> +
>>> return 0;
>>> }
>>>
>>>
>>
>> Hi Dinh,
>>
>> On my system, with no reset controller configured, this fails
>> devm_reset_control_get() with -EINVAL, and it also spits out a
>> backtrace.
>>
>> Hi Philipp,
>>
>> I think devm_reset_control_get() should return either NULL or some
>> other error to indicate that the reset controller is not
>> configured. That way we can check it and handle that condition. If we
>> check for -EINVAL, we might miss a legitimate error that is returning
>> -EINVAL.
>>
>> Also I think the WARN_ON(1) should be removed for this case.
>>
>> Any thoughts on this or other suggestions to resolve it?
>
> It is invalid to call (devm_)reset_control_get if the framework is
> disabled, so that error code is correct. The only reason there are even
> stubs for (devm_)reset_control_get is for compile tests. Either enable
> CONFIG_RESET_CONTROLLER or use (devm_)reset_control_get_optional.
>
Ok thanks Philipp.
I guess the correct call would be the optional one then.
Dinh,
Could you respin with the necessary changes?
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
