From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1034241AbdAIRUA (ORCPT ); Mon, 9 Jan 2017 12:20:00 -0500 Received: from us01smtprelay-2.synopsys.com ([198.182.47.9]:56438 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031145AbdAIRT5 (ORCPT ); Mon, 9 Jan 2017 12:19:57 -0500 Subject: Re: [PATCH v2 2/2] reset: make optional functions really optional To: Philipp Zabel , Ramiro Oliveira References: <1483958747.13625.11.camel@pengutronix.de> CC: , , From: Ramiro Oliveira Message-ID: <88f890ef-b16e-a558-3c2f-cfeba107b578@synopsys.com> Date: Mon, 9 Jan 2017 17:19:47 +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: <1483958747.13625.11.camel@pengutronix.de> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.107.25.125] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Philipp On 1/9/2017 10:45 AM, Philipp Zabel wrote: > Hi Ramiro, > > Am Dienstag, den 27.12.2016, 12:37 +0000 schrieb Ramiro Oliveira: >> The optional functions weren't really optional so this patch makes them >> really optional. > > Please add a bit of detail to the description. Since this changes the > API, you should mention that the reset_control_get_optional variants now > return NULL instead of an error if there is no matching reset phandle in > the device tree and that the reset_control_* functions accept NULL rstc > pointers. > Would you be ok with something like this: "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 | 35 +++++++++++++++++++++++++++++------ >> include/linux/reset.h | 45 ++++++++++++++++++++++++++------------------- >> 2 files changed, 55 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> index 70023997d031..f933e9dfebc5 100644 >> --- a/drivers/reset/core.c >> +++ b/drivers/reset/core.c >> @@ -135,9 +135,15 @@ EXPORT_SYMBOL_GPL(devm_reset_controller_register); >> * @rstc: reset controller >> * >> * Calling this on a shared reset controller is an error. >> + * >> + * If rstc is NULL it is an optional reset and the function will just >> + * return 0. >> */ >> int reset_control_reset(struct reset_control *rstc) >> { >> + if (!rstc) >> + return 0; >> + >> if (WARN_ON(rstc->shared)) >> return -EINVAL; >> >> @@ -158,9 +164,15 @@ 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 rstc is NULL it is an optional reset and the function will just >> + * return 0. >> */ >> int reset_control_assert(struct reset_control *rstc) >> { >> + if (!rstc) >> + return 0; >> + >> if (!rstc->rcdev->ops->assert) >> return -ENOTSUPP; >> >> @@ -181,9 +193,15 @@ EXPORT_SYMBOL_GPL(reset_control_assert); >> * @rstc: reset controller >> * >> * After calling this function, the reset is guaranteed to be deasserted. >> + * >> + * 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 (!rstc) >> + return 0; >> + >> if (!rstc->rcdev->ops->deassert) >> return -ENOTSUPP; >> >> @@ -199,11 +217,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); >> >> @@ -258,7 +279,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; >> @@ -273,13 +295,13 @@ struct reset_control *__of_reset_control_get(struct device_node *node, >> index = of_property_match_string(node, >> "reset-names", id); >> if (index < 0) >> - return ERR_PTR(-ENOENT); >> + return optional ? NULL : ERR_PTR(-ENOENT); > > of_property_match_string can return -EINVAL, -ENODATA, or -EILSEQ. > I think -EILSEQ should still be returned. > I'll make the function return NULL, -ENOENT, or -EILSEQ. >> } >> >> ret = of_parse_phandle_with_args(node, "resets", "#reset-cells", >> index, &args); >> if (ret) >> - return ERR_PTR(ret); >> + return optional ? NULL : ERR_PTR(ret); > > of_parse_phandle_with_args can return -ENOENT or -EINVAL. > I think -EINVAL should still be returned. > Same as above. I'll make the function return NULL, -ENOENT, or -EINVAL. >> >> mutex_lock(&reset_list_mutex); >> rcdev = NULL; >> @@ -338,7 +360,8 @@ static void devm_reset_control_release(struct device *dev, void *res) >> } >> >> struct reset_control *__devm_reset_control_get(struct device *dev, >> - const char *id, int index, bool shared) >> + const char *id, int index, bool shared, >> + bool optional) >> { >> struct reset_control **ptr, *rstc; >> >> @@ -348,7 +371,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev, >> return ERR_PTR(-ENOMEM); >> >> rstc = __of_reset_control_get(dev ? dev->of_node : NULL, >> - id, index, shared); >> + id, index, shared, optional); >> if (!IS_ERR(rstc)) { >> *ptr = rstc; >> devres_add(dev, ptr); >> diff --git a/include/linux/reset.h b/include/linux/reset.h >> index ec1d1fd28f5f..e4cc7146b5ed 100644 >> --- a/include/linux/reset.h >> +++ b/include/linux/reset.h >> @@ -13,10 +13,12 @@ int reset_control_deassert(struct reset_control *rstc); >> int reset_control_status(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); >> void reset_control_put(struct reset_control *rstc); >> struct reset_control *__devm_reset_control_get(struct device *dev, >> - const char *id, int index, bool shared); >> + const char *id, int index, bool shared, >> + bool optional); >> >> int __must_check device_reset(struct device *dev); >> >> @@ -69,14 +71,15 @@ static inline int device_reset_optional(struct device *dev) >> >> static inline 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) >> { >> return ERR_PTR(-ENOTSUPP); >> } >> >> static inline struct reset_control *__devm_reset_control_get( >> - struct device *dev, >> - const char *id, int index, bool shared) >> + struct device *dev, const char *id, >> + int index, bool shared, bool optional) >> { >> return ERR_PTR(-ENOTSUPP); >> } >> @@ -104,7 +107,8 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id) >> #ifndef CONFIG_RESET_CONTROLLER >> WARN_ON(1); >> #endif >> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false, >> + false); >> } >> >> /** >> @@ -132,19 +136,22 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id) >> static inline struct reset_control *reset_control_get_shared( >> struct device *dev, const char *id) >> { >> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true); >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true, >> + false); >> } >> >> 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, false); >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false, >> + true); >> } >> >> 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, true); >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true, >> + true); >> } >> >> /** >> @@ -160,7 +167,7 @@ static inline struct reset_control *reset_control_get_optional_shared( >> static inline struct reset_control *of_reset_control_get_exclusive( >> struct device_node *node, const char *id) >> { >> - return __of_reset_control_get(node, id, 0, 0); >> + return __of_reset_control_get(node, id, 0, false, false); >> } >> >> /** >> @@ -185,7 +192,7 @@ static inline struct reset_control *of_reset_control_get_exclusive( >> static inline struct reset_control *of_reset_control_get_shared( >> struct device_node *node, const char *id) >> { >> - return __of_reset_control_get(node, id, 0, true); >> + return __of_reset_control_get(node, id, 0, true, false); >> } >> >> /** >> @@ -202,7 +209,7 @@ static inline struct reset_control *of_reset_control_get_shared( >> static inline struct reset_control *of_reset_control_get_exclusive_by_index( >> struct device_node *node, int index) >> { >> - return __of_reset_control_get(node, NULL, index, false); >> + return __of_reset_control_get(node, NULL, index, false, false); >> } >> >> /** >> @@ -230,7 +237,7 @@ static inline struct reset_control *of_reset_control_get_exclusive_by_index( >> static inline struct reset_control *of_reset_control_get_shared_by_index( >> struct device_node *node, int index) >> { >> - return __of_reset_control_get(node, NULL, index, true); >> + return __of_reset_control_get(node, NULL, index, true, false); >> } >> >> /** >> @@ -252,7 +259,7 @@ __must_check devm_reset_control_get_exclusive(struct device *dev, >> #ifndef CONFIG_RESET_CONTROLLER >> WARN_ON(1); >> #endif >> - return __devm_reset_control_get(dev, id, 0, false); >> + return __devm_reset_control_get(dev, id, 0, false, false); >> } >> >> /** >> @@ -267,19 +274,19 @@ __must_check devm_reset_control_get_exclusive(struct device *dev, >> static inline struct reset_control *devm_reset_control_get_shared( >> struct device *dev, const char *id) >> { >> - return __devm_reset_control_get(dev, id, 0, true); >> + return __devm_reset_control_get(dev, id, 0, true, false); >> } >> >> 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, false); >> + return __devm_reset_control_get(dev, id, 0, false, true); >> } >> >> 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, true); >> + return __devm_reset_control_get(dev, id, 0, true, true); >> } >> >> /** >> @@ -297,7 +304,7 @@ static inline struct reset_control *devm_reset_control_get_optional_shared( >> static inline struct reset_control * >> devm_reset_control_get_exclusive_by_index(struct device *dev, int index) >> { >> - return __devm_reset_control_get(dev, NULL, index, false); >> + return __devm_reset_control_get(dev, NULL, index, false, false); >> } >> >> /** >> @@ -313,7 +320,7 @@ devm_reset_control_get_exclusive_by_index(struct device *dev, int index) >> static inline struct reset_control * >> devm_reset_control_get_shared_by_index(struct device *dev, int index) >> { >> - return __devm_reset_control_get(dev, NULL, index, true); >> + return __devm_reset_control_get(dev, NULL, index, true, false); >> } >> >> /* > > regards > Philipp > BRs, Ramiro