From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751202AbdCPFm2 (ORCPT ); Thu, 16 Mar 2017 01:42:28 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:44564 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750866AbdCPFm0 (ORCPT ); Thu, 16 Mar 2017 01:42:26 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 8680960706 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 2/2] usb; dwc3: of-simple: Add support to get resets for the device To: Philipp Zabel References: <1487741048-24659-1-git-send-email-vivek.gautam@codeaurora.org> <1487741048-24659-2-git-send-email-vivek.gautam@codeaurora.org> <1489574727.2528.13.camel@pengutronix.de> Cc: balbi@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org From: Vivek Gautam Message-ID: <0f0ed385-30ea-7819-9533-f06b5e4eccfe@codeaurora.org> Date: Thu, 16 Mar 2017 11:04:20 +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: <1489574727.2528.13.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 Hi, On 03/15/2017 04:15 PM, Philipp Zabel wrote: > On Wed, 2017-02-22 at 10:54 +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 >> Signed-off-by: Vivek Gautam >> --- >> >> Based on torvald's master branch. >> >> drivers/usb/dwc3/dwc3-of-simple.c | 49 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c >> index fe414e7a9c78..025de7342d28 100644 >> --- a/drivers/usb/dwc3/dwc3-of-simple.c >> +++ b/drivers/usb/dwc3/dwc3-of-simple.c >> @@ -29,13 +29,52 @@ >> #include >> #include >> #include >> +#include >> >> struct dwc3_of_simple { >> struct device *dev; >> struct clk **clks; >> int num_clocks; >> + struct reset_control **resets; >> + int num_resets; >> }; >> >> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple, int count) >> +{ >> + struct device *dev = simple->dev; >> + int i; >> + >> + simple->num_resets = count; >> + >> + if (!count) >> + return 0; >> + >> + simple->resets = devm_kcalloc(dev, simple->num_resets, >> + sizeof(struct reset_control *), GFP_KERNEL); >> + if (!simple->resets) >> + return -ENOMEM; >> + >> + for (i = 0; i < simple->num_resets; i++) { >> + struct reset_control *reset; >> + int ret; >> + >> + reset = devm_reset_control_get_by_index(dev, i); > Please use devm_reset_control_get_exclusive_by_index instead. See > include/linux/reset.h for details. Sure, will make use of *exclusive version of the api. > >> + if (IS_ERR(reset)) >> + return PTR_ERR(reset); >> + >> + simple->resets[i] = reset; >> + >> + ret = reset_control_deassert(reset); >> + if (ret) { >> + while (--i >= 0) >> + reset_control_assert(reset); >> + return ret; >> + } >> + } >> + >> + return 0; >> +} > This looks rather generic. Should we have a > reset_control_get/assert/deassert_array functionality at the reset API > level? Yes, i think we should. Something on the lines of 'regulator_bulk_*' interface? > >> static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count) >> { >> struct device *dev = simple->dev; >> @@ -100,6 +139,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> + ret = dwc3_of_simple_reset_init(simple, of_reset_control_get_count(np)); >> + if (ret) >> + return ret; >> + > Not a blocker, but it seems a bit inconsistent to count the reset > controls via the device node (of_...), but then get them via the device > (devm_reset_control_get_... instead of of_reset_control_get_...). You are right, it looks inconsistent. I thought of using a resource managed API. But now i think it doesn't make much sense. Best Regards Vivek > > regards > Philipp > -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project