From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>,
        linux-kernel@vger.kernel.org, CARLOS.PALMINHA@synopsys.com
Subject: Re: [PATCH] reset: Make optional functions really optional.
Date: Fri, 23 Dec 2016 18:41:29 +0200
Message-ID: <21804273.N6pjm8MiSn@avalon>
User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; )
In-Reply-To: <1482494934.2394.53.camel@pengutronix.de>
References: <58b07c24e1bc263db0d43274b46022c8d8506302.1481822669.git.Ramiro.Oliveira@synopsys.com> <4238328.E0tRokcGUd@avalon> <1482494934.2394.53.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
Content-Length: 2534
Lines: 69

Hi Philipp,

On Friday 23 Dec 2016 13:08:54 Philipp Zabel wrote:
> Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart:
> > 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 <Ramiro.Oliveira@synopsys.com>
> >>> ---
> >>> 
> >>>  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".

The important point in my opinion is that a NULL argument will result in the 
function performing no operation and indicating success exactly like a call 
with a non-NULL pointer would. The proposed text makes it sound like a 0 
return value is specific to the NULL argument case. This is a detail though.

> 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;

-- 
Regards,

Laurent Pinchart