Re: [PATCH] reset: Make optional functions really optional.

[Laurent Pinchart]

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@xxxxxxxxxxxx>
> >>> ---
> >>> 
> >>>  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