From patchwork Thu May 26 20:25:05 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 9137397 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A5074607D3 for ; Thu, 26 May 2016 20:27:26 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7F7EB269E2 for ; Thu, 26 May 2016 20:27:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6512127D10; Thu, 26 May 2016 20:27:26 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id C7F0C269E2 for ; Thu, 26 May 2016 20:27:25 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1b61qb-0002D9-LI; Thu, 26 May 2016 20:25:37 +0000 Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1b61qY-0002BA-Cn for linux-arm-kernel@lists.infradead.org; Thu, 26 May 2016 20:25:35 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2AF1837E72; Thu, 26 May 2016 20:25:12 +0000 (UTC) Received: from shalem.localdomain (vpn1-6-159.ams2.redhat.com [10.36.6.159]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u4QKP6Z2023896 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 26 May 2016 16:25:08 -0400 Subject: Re: [PATCH] reset: Put back *_optional variants To: John Youn , Philipp Zabel References: From: Hans de Goede Message-ID: <12e450f0-70ea-29f8-0bf7-8a0697263f2d@redhat.com> Date: Thu, 26 May 2016 22:25:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 26 May 2016 20:25:12 +0000 (UTC) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160526_132534_510044_8A6BACF6 X-CRM114-Status: GOOD ( 27.88 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: stefan.wahren@i2se.com, Felipe Balbi , dinguyen@opensource.altera.com, linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, On 26-05-16 03:15, John Youn wrote: > Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] > functions wrappers"), the optional variants returned -ENOTSUPP when > CONFIG_RESET_CONTROLLER was not set. This patch reverts to this > behavior. Otherwise those calls will return -EINVAL causing users to > think that an error occurred when CONFIG_RESET_CONTROLLER is not set. > > Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions wrappers") > Signed-off-by: John Youn > --- > > Hi Philipp, Hans, > > The commit referenced above breaks an upcoming patch for the dwc2 > driver that adds an optional reset control. > > https://marc.info/?l=linux-usb&m=146161328211584&w=2 > > I've attempted to add the optional variants back the way they were > working before. Let me know if I need to do anything else to fix it or > if it should be done another way. > > Regards, > John Hmm, I don't like all the extra code your patch adds just to fix a return value... Looking at the code before my "reset: Make [of_]reset_control_get[_foo] functions wrappers" patch, all the dev*_get* functions were returning ENOTSUPP except for [devm_]reset_control_get, so following your logic we should also change the of_reset_control_get_by_index variant to return -ENOTSUP. Or better, simply make them all return -ENOTSUP, that seems both consistent and more KISS to me, this would mean an error code change for [devm_]reset_control_get, but will fix all the other getters from having a changed error-code, and I would callers of [devm_]reset_control_get to not care which error code they get, except for -EPROBE_DEFER. So IMHO the following change would be a better way to fix this: Regards, Hans > > include/linux/reset.h | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index ec0306ce..739d33d 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -14,10 +14,24 @@ int reset_control_status(struct reset_control *rstc); > > struct reset_control *__of_reset_control_get(struct device_node *node, > const char *id, int index, int shared); > + > +struct reset_control *__of_reset_control_get_optional(struct device_node *node, > + const char *id, int index, int shared) > +{ > + return __of_reset_control_get(node, id, index, shared); > +} > + > void reset_control_put(struct reset_control *rstc); > struct reset_control *__devm_reset_control_get(struct device *dev, > const char *id, int index, int shared); > > +static inline struct reset_control *__devm_reset_control_get_optional( > + struct device *dev, > + const char *id, int index, int shared) > +{ > + return __devm_reset_control_get(dev, id, index, shared); > +} > + > int __must_check device_reset(struct device *dev); > > static inline int device_reset_optional(struct device *dev) > @@ -74,6 +88,13 @@ static inline struct reset_control *__of_reset_control_get( > return ERR_PTR(-EINVAL); > } > > +static inline struct reset_control *__of_reset_control_get_optional( > + struct device_node *node, > + const char *id, int index, int shared) > +{ > + return ERR_PTR(-ENOTSUPP); > +} > + > static inline struct reset_control *__devm_reset_control_get( > struct device *dev, > const char *id, int index, int shared) > @@ -81,6 +102,13 @@ static inline struct reset_control *__devm_reset_control_get( > return ERR_PTR(-EINVAL); > } > > +static inline struct reset_control *__devm_reset_control_get_optional( > + struct device *dev, > + const char *id, int index, int shared) > +{ > + return ERR_PTR(-ENOTSUPP); > +} > + > #endif /* CONFIG_RESET_CONTROLLER */ > > /** > @@ -110,7 +138,8 @@ static inline struct reset_control *__must_check reset_control_get( > static inline struct reset_control *reset_control_get_optional( > struct device *dev, const char *id) > { > - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); > + return __of_reset_control_get_optional(dev ? dev->of_node : NULL, > + id, 0, 0); > } > > /** > @@ -194,7 +223,7 @@ static inline struct reset_control *__must_check devm_reset_control_get( > static inline struct reset_control *devm_reset_control_get_optional( > struct device *dev, const char *id) > { > - return __devm_reset_control_get(dev, id, 0, 0); > + return __devm_reset_control_get_optional(dev, id, 0, 0); > } > > /** > --- a/include/linux/reset.h +++ b/include/linux/reset.h @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( struct device_node *node, const char *id, int index, int shared) { - return ERR_PTR(-EINVAL); + return ERR_PTR(-ENOTSUPP); } static inline struct reset_control *__devm_reset_control_get( struct device *dev, const char *id, int index, int shared) { - return ERR_PTR(-EINVAL); + return ERR_PTR(-ENOTSUPP); } #endif /* CONFIG_RESET_CONTROLLER */