From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751341AbdAMJLv (ORCPT ); Fri, 13 Jan 2017 04:11:51 -0500 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:53967 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751146AbdAMJLt (ORCPT ); Fri, 13 Jan 2017 04:11:49 -0500 Message-ID: <1484298707.2436.14.camel@pengutronix.de> Subject: Re: [PATCH v3 2/2] reset: make optional functions really optional From: Philipp Zabel To: Ramiro Oliveira Cc: linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com, CARLOS.PALMINHA@synopsys.com Date: Fri, 13 Jan 2017 10:11:47 +0100 In-Reply-To: <57aba7a2ae9c18ac4047733585e610f7976d8f77.1484245505.git.Ramiro.Oliveira@synopsys.com> References: <57aba7a2ae9c18ac4047733585e610f7976d8f77.1484245505.git.Ramiro.Oliveira@synopsys.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:96de:80ff:fec2:9969 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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); > + } > } > > 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); regards Philipp