Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761197AbcLWLWl (ORCPT ); Fri, 23 Dec 2016 06:22:41 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:39070 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757890AbcLWLWk (ORCPT ); Fri, 23 Dec 2016 06:22:40 -0500 From: Laurent Pinchart To: Philipp Zabel Cc: Ramiro Oliveira , linux-kernel@vger.kernel.org, CARLOS.PALMINHA@synopsys.com Subject: Re: [PATCH] reset: Make optional functions really optional. Date: Fri, 23 Dec 2016 13:23:10 +0200 Message-ID: <4238328.E0tRokcGUd@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: <1482490737.2394.37.camel@pengutronix.de> References: <58b07c24e1bc263db0d43274b46022c8d8506302.1481822669.git.Ramiro.Oliveira@synopsys.com> <1482490737.2394.37.camel@pengutronix.de> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6011 Lines: 206 Hello, On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote: > Am Donnerstag, den 15.12.2016, 18:05 +0000 schrieb Ramiro Oliveira: > > Up until now optional functions in the reset API were similar to the non > > optional. > > > > This patch corrects that, while maintaining compatibility with existing > > drivers. > > > > As suggested here: https://lkml.org/lkml/2016/12/14/502 > > > > Signed-off-by: Ramiro Oliveira > > --- > > > > drivers/reset/core.c | 21 +++++++++++++++++++-- > > include/linux/reset.h | 46 ++++++++++++++++++++++++++++++++++++++++++---- > > 2 files changed, 61 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > index 395dc9c..6150e7c 100644 > > --- a/drivers/reset/core.c > > +++ b/drivers/reset/core.c > > @@ -135,9 +135,14 @@ EXPORT_SYMBOL_GPL(devm_reset_controller_register); > > * @rstc: reset controller > > * > > * Calling this on a shared reset controller is an error. > > + * > > + * If it's an optional reset it will return 0. > > I'd prefer this to explicitly mention that rstc==NULL means this is an > optional reset: > > "If rstc is NULL it is an optional reset and the function will just > return 0." Maybe we should document in a single place that NULL is a valid value for a reset_control pointer and will result in the API behaving as a no-op ? If you want to duplicate the information I'd still prefer talking about no-op than about "just returning 0". > > */ > > int reset_control_reset(struct reset_control *rstc) > > { > > + if (!rstc) > > + return 0; > > + > > if (WARN_ON(rstc->shared)) > > return -EINVAL; > > > > @@ -158,9 +163,14 @@ EXPORT_SYMBOL_GPL(reset_control_reset); > > * > > * For shared reset controls a driver cannot expect the hw's registers > > and > > * internal state to be reset, but must be prepared for this to happen. > > + * > > + * If it's an optional reset it will return 0. > > Same as above. > > > */ > > > > int reset_control_assert(struct reset_control *rstc) > > { > > + if (!rstc) > > + return 0; > > + > > if (!rstc->rcdev->ops->assert) > > return -ENOTSUPP; > > > > @@ -180,10 +190,14 @@ EXPORT_SYMBOL_GPL(reset_control_assert); > > * reset_control_deassert - deasserts the reset line > > * @rstc: reset controller > > * > > - * After calling this function, the reset is guaranteed to be deasserted. > > + * After calling this function, the reset is guaranteed to be deasserted, > > if > > + * it's not optional. > > Same as above. > > > */ > > int reset_control_deassert(struct reset_control *rstc) > > { > > + if (!rstc) > > + return 0; > > + > > if (!rstc->rcdev->ops->deassert) > > return -ENOTSUPP; > > > > @@ -199,11 +213,14 @@ 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 (!rstc) > > + return 0; > > + > > if (rstc->rcdev->ops->status) > > return rstc->rcdev->ops->status(rstc->rcdev, rstc->id); > > > > diff --git a/include/linux/reset.h b/include/linux/reset.h > > index 5daff15..1af1e62 100644 > > --- a/include/linux/reset.h > > +++ b/include/linux/reset.h > > @@ -138,13 +138,33 @@ static inline struct reset_control > > *reset_control_get_shared(> > > static inline struct reset_control *reset_control_get_optional_exclusive( > > struct device *dev, const char *id) > > { > > - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); > > + struct reset_control *desc; > > + > > + desc = __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); > > Note that the __of_reset_control_get stub returns -ENOTSUPP if > CONFIG_RESET_CONTROLLER is disabled. > > > + if (IS_ERR(desc)) { > > + if (PTR_ERR(desc) == -ENOENT) > > + return NULL; > > + } > > + > > + return desc; > > + > > } > > > > static inline struct reset_control *reset_control_get_optional_shared( > > struct device *dev, const char *id) > > { > > - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1); > > + > > + struct reset_control *desc; > > + > > + desc = __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1); > > + > > + if (IS_ERR(desc)) { > > + if (PTR_ERR(desc) == -ENOENT) > > + return NULL; > > + } > > With this duplication, I think it might be better to add an int optional > parameter What's wrong with bool by the way ? :-) > to __of_reset_control_get and let that return NULL if optional > is set and either of_property_match_string or of_parse_phandle_with_args > would cause an -ENOENT return. > > The stub could then > return optional ? NULL : ERR_PTR(-EONOENT); > > > + return desc; > > } > > > > /** > > @@ -273,13 +293,31 @@ static inline struct reset_control > > *devm_reset_control_get_shared( > > static inline struct reset_control > > *devm_reset_control_get_optional_exclusive( > > struct device *dev, const char *id) > > { > > - return __devm_reset_control_get(dev, id, 0, 0); > > + struct reset_control *desc; > > + > > + desc = __devm_reset_control_get(dev, id, 0, 0); > > + > > + if (IS_ERR(desc)) { > > + if (PTR_ERR(desc) == -ENOENT) > > + return NULL; > > + } > > Same as for __of_reset_control_get above. > > > + return desc; > > } > > > > static inline struct reset_control > > *devm_reset_control_get_optional_shared( > > struct device *dev, const char *id) > > { > > - return __devm_reset_control_get(dev, id, 0, 1); > > + struct reset_control *desc; > > + > > + desc = __devm_reset_control_get(dev, id, 0, 1); > > + > > + if (IS_ERR(desc)) { > > + if (PTR_ERR(desc) == -ENOENT) > > + return NULL; > > + } > > + > > + return desc; > > } > > > > /** -- Regards, Laurent Pinchart