From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762896AbdDSMC6 (ORCPT ); Wed, 19 Apr 2017 08:02:58 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:40654 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762813AbdDSMCz (ORCPT ); Wed, 19 Apr 2017 08:02:55 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 3906860AA8 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=vivek.gautam@codeaurora.org Subject: Re: [PATCH V3 3/4] usb: dwc3: of-simple: Add support to get resets for the device To: Philipp Zabel References: <1492514488-27385-1-git-send-email-vivek.gautam@codeaurora.org> <1492514488-27385-4-git-send-email-vivek.gautam@codeaurora.org> <1492597929.2970.66.camel@pengutronix.de> Cc: swarren@wwwdotorg.org, balbi@kernel.org, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org, linux-usb@vger.kernel.org, thierry.reding@gmail.com, gregkh@linuxfoundation.org, linux-arm-msm@vger.kernel.org From: Vivek Gautam Message-ID: <5a165379-1e33-1010-f502-8b4697150391@codeaurora.org> Date: Wed, 19 Apr 2017 17:32:47 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1492597929.2970.66.camel@pengutronix.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/19/2017 04:02 PM, Philipp Zabel wrote: > On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote: >> Add support to get a list of resets available for the device. >> These resets must be kept de-asserted until the device is >> in use. >> >> Cc: Felipe Balbi >> Cc: Philipp Zabel >> Signed-off-by: Vivek Gautam >> --- >> drivers/usb/dwc3/dwc3-of-simple.c | 36 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c >> index fe414e7a9c78..9116df649f0b 100644 >> --- a/drivers/usb/dwc3/dwc3-of-simple.c >> +++ b/drivers/usb/dwc3/dwc3-of-simple.c >> @@ -29,13 +29,39 @@ >> #include >> #include >> #include >> +#include >> >> struct dwc3_of_simple { >> struct device *dev; >> struct clk **clks; >> int num_clocks; >> + struct reset_control_array *resets; >> }; >> >> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple) >> +{ >> + struct device *dev = simple->dev; >> + int ret; >> + >> + simple->resets = of_reset_control_array_get_exclusive(dev->of_node); >> + if (IS_ERR(simple->resets)) { >> + ret = PTR_ERR(simple->resets); >> + if (ret == -ENOENT) >> + /* not all controllers required resets */ >> + return 0; > If you use the of_reset_control_array_get_optional_exclusive variant, > you can remove the three lines directly above. No, it's a little tricky here. of_reset_control_get_count() returns an error code, that we then return from of_reset_control_array_get(). This is something that tegra/pmc driver required (return errorno in case count of reset controls is zero. So we handle that error separately here. > >> + dev_err(dev, "failed to get device resets\n"); > It would be nice to print the error code here. Sure. > >> + return ret; >> + } >> + >> + ret = reset_control_array_deassert(simple->resets); >> + if (ret) { >> + reset_control_array_put(simple->resets); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count) >> { >> struct device *dev = simple->dev; >> @@ -100,6 +126,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> + ret = dwc3_of_simple_reset_init(simple); >> + if (ret) >> + return ret; >> + > I would just move the contents of dwc3_of_simple_reset_init here and add > a goto error path at the end of dwc3_of_simple_clk_init to handle the > reset control cleanup as needed. Okay, i can restructure it. > >> ret = of_platform_populate(np, NULL, NULL, dev); >> if (ret) { >> for (i = 0; i < simple->num_clocks; i++) { >> @@ -107,6 +137,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) >> clk_put(simple->clks[i]); >> } >> >> + reset_control_array_assert(simple->resets); >> + reset_control_array_put(simple->resets); >> + >> return ret; >> } >> >> @@ -128,6 +161,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev) >> clk_put(simple->clks[i]); >> } >> >> + reset_control_array_assert(simple->resets); >> + reset_control_array_put(simple->resets); >> + > Must the reset be asserted before of_platform_depopulate? Right, the order must be taken care of here. Will change this. > >> of_platform_depopulate(dev); > Given the order above, I'd expect the reset cleanup here. Sure. > >> pm_runtime_put_sync(dev); > regards > Philipp Thanks for reviewing. Best Regards Vivek -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project