From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751341AbdAMJ33 (ORCPT ); Fri, 13 Jan 2017 04:29:29 -0500 Received: from smtprelay.synopsys.com ([198.182.47.9]:44994 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750923AbdAMJ30 (ORCPT ); Fri, 13 Jan 2017 04:29:26 -0500 Subject: Re: [PATCH v3 2/2] reset: make optional functions really optional To: Philipp Zabel , Ramiro Oliveira References: <57aba7a2ae9c18ac4047733585e610f7976d8f77.1484245505.git.Ramiro.Oliveira@synopsys.com> <1484298707.2436.14.camel@pengutronix.de> CC: , , From: Ramiro Oliveira Message-ID: <616e543c-04c7-6933-65aa-ea6e1ec50202@synopsys.com> Date: Fri, 13 Jan 2017 09:27:10 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <1484298707.2436.14.camel@pengutronix.de> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.107.25.128] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Philipp On 1/13/2017 9:11 AM, Philipp Zabel wrote: > Hi Ramiro, > > Am Donnerstag, den 12.01.2017, 18:34 +0000 schrieb Ramiro Oliveira: >> "The *_get_optional_* functions weren't really optional so this patch >> makes them really optional. >> >> These *_get_optional_* functions will now return NULL instead of an error >> if no matching reset phandle is found in the DT, and all the >> reset_control_* functions now accept NULL rstc pointers. >> >> Signed-off-by: Ramiro Oliveira >> --- >> drivers/reset/core.c | 59 ++++++++++++++++++++++++++++++++++++++++----------- >> include/linux/reset.h | 45 ++++++++++++++++++++++----------------- >> 2 files changed, 73 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> index 272c1e4ecb5c..7b679b0bfef8 100644 >> --- a/drivers/reset/core.c >> +++ b/drivers/reset/core.c >> @@ -143,12 +143,18 @@ EXPORT_SYMBOL_GPL(devm_reset_controller_register); >> * a no-op. >> * Consumers must not use reset_control_(de)assert on shared reset lines when >> * reset_control_reset has been used. >> + * >> + * If rstc is NULL it is an optional reset and the function will just >> + * return 0. >> */ >> int reset_control_reset(struct reset_control *rstc) >> { >> int ret; >> >> - if (WARN_ON(IS_ERR_OR_NULL(rstc))) >> + if (!rstc) >> + return 0; >> + >> + if (WARN_ON(IS_ERR(rstc))) >> return -EINVAL; >> >> if (!rstc->rcdev->ops->reset) >> @@ -182,10 +188,17 @@ EXPORT_SYMBOL_GPL(reset_control_reset); >> * internal state to be reset, but must be prepared for this to happen. >> * Consumers must not use reset_control_reset on shared reset lines when >> * reset_control_(de)assert has been used. >> + * return 0. >> + * >> + * If rstc is NULL it is an optional reset and the function will just >> + * return 0. >> */ >> int reset_control_assert(struct reset_control *rstc) >> { >> - if (WARN_ON(IS_ERR_OR_NULL(rstc))) >> + if (!rstc) >> + return 0; >> + >> + if (WARN_ON(IS_ERR(rstc))) >> return -EINVAL; >> >> if (!rstc->rcdev->ops->assert) >> @@ -213,10 +226,17 @@ EXPORT_SYMBOL_GPL(reset_control_assert); >> * After calling this function, the reset is guaranteed to be deasserted. >> * Consumers must not use reset_control_reset on shared reset lines when >> * reset_control_(de)assert has been used. >> + * return 0. >> + * >> + * If rstc is NULL it is an optional reset and the function will just >> + * return 0. >> */ >> int reset_control_deassert(struct reset_control *rstc) >> { >> - if (WARN_ON(IS_ERR_OR_NULL(rstc))) >> + if (!rstc) >> + return 0; >> + >> + if (WARN_ON(IS_ERR(rstc))) >> return -EINVAL; >> >> if (!rstc->rcdev->ops->deassert) >> @@ -237,12 +257,15 @@ EXPORT_SYMBOL_GPL(reset_control_deassert); >> /** >> * reset_control_status - returns a negative errno if not supported, a >> * positive value if the reset line is asserted, or zero if the reset >> - * line is not asserted. >> + * line is not asserted or if the desc is NULL (optional reset). >> * @rstc: reset controller >> */ >> int reset_control_status(struct reset_control *rstc) >> { >> - if (WARN_ON(IS_ERR_OR_NULL(rstc))) >> + if (!rstc) >> + return 0; >> + >> + if (WARN_ON(IS_ERR(rstc))) >> return -EINVAL; >> >> if (rstc->rcdev->ops->status) >> @@ -299,7 +322,8 @@ static void __reset_control_put(struct reset_control *rstc) >> } >> >> struct reset_control *__of_reset_control_get(struct device_node *node, >> - const char *id, int index, bool shared) >> + const char *id, int index, bool shared, >> + bool optional) >> { >> struct reset_control *rstc; >> struct reset_controller_dev *r, *rcdev; >> @@ -313,14 +337,24 @@ struct reset_control *__of_reset_control_get(struct device_node *node, >> if (id) { >> index = of_property_match_string(node, >> "reset-names", id); >> - if (index < 0) >> - return ERR_PTR(-ENOENT); >> + if (index < 0) { >> + if (optional) >> + return NULL; >> + else >> + return (index == -EILSEQ) ? >> + ERR_PTR(index) : ERR_PTR(-ENOENT); > > That is not what I meant. I think reset_control_get should return an > error on EILSEQ even if optional is true, since that means there are > resets specified in the DT, but the reset-names property is broken. > How about: > if (index == -EILSEQ) > return ERR_PTR(index); > if (index < 0) > return optional ? NULL : ERR_PTR(-ENOENT); > When you first suggested I wasn't entirely sure what you meant, but now it makes total sense to me. I should have asked you about it before implementing it. >> + } >> } >> >> ret = of_parse_phandle_with_args(node, "resets", "#reset-cells", >> index, &args); >> - if (ret) >> - return ERR_PTR(ret); >> + if (ret) { >> + if (optional) >> + return NULL; >> + else >> + return (index == -EINVAL) ? >> + ERR_PTR(index) : ERR_PTR(-ENOENT); >> + } > > That is not what I meant either. If the phandle parsing fails with > of_phandle_iterator_next returning -EINVAL, that should be passed along > in the optional case: that happens when we've already found a matching > entry in reset-names, but the resets property is broken. > How about: > if (ret == -EINVAL) > return ERR_PTR(ret); > if (ret) > return optional ? NULL : ERR_PTR(ret); > Same as above. > regards > Philipp > I'll refactor the code and send a new patch ASAP. BRs, Ramiro