From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S942005AbcLWRUJ (ORCPT ); Fri, 23 Dec 2016 12:20:09 -0500 Received: from smtprelay.synopsys.com ([198.182.47.9]:58123 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932280AbcLWRUI (ORCPT ); Fri, 23 Dec 2016 12:20:08 -0500 Subject: Re: [PATCH] reset: Make optional functions really optional. To: Philipp Zabel , Laurent Pinchart References: <58b07c24e1bc263db0d43274b46022c8d8506302.1481822669.git.Ramiro.Oliveira@synopsys.com> <1482490737.2394.37.camel@pengutronix.de> <4238328.E0tRokcGUd@avalon> <1482494934.2394.53.camel@pengutronix.de> CC: Ramiro Oliveira , , From: Ramiro Oliveira Message-ID: <36d931b7-3bf4-c3b9-a8e8-24c9b080c39f@synopsys.com> Date: Fri, 23 Dec 2016 17:19:43 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1482494934.2394.53.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 12/23/2016 12:08 PM, Philipp Zabel wrote: > Hi Laurent, > > Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart: >> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2016_12_14_502&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=BHEb-RADEOm-lgrwdN4zqtr2BWZMjeocyTkjphE6PrA&m=_0T0di-X6zgDw8ZRLDNk2ExL2EieBiCmAmuxc8OGAg4&s=H5BfD4P5MB85jtyUjDrn6yKu-6ws5srNWNNiFpPL0pQ&e= >>>> >>>> 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". > > Does "no-op" implicate the return value 0? Maybe there is a better way > to express "no action, returns 0". > > Currently there is no central place for this information, and as long as > the text not much longer than a reference to the central location would > be, I'm fine with duplication. > >>>> */ >>>> 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 ? :-) > > Nothing wrong, it's just that the "exclusive" parameter is already int. > I'd be perfectly fine with using bool for both. > Do you prefer me to keep them both int, or change them to bool? BRs, Ramiro