Messages in this thread | | | From | Laurent Pinchart <> | Subject | Re: [PATCH] reset: Make optional functions really optional. | Date | Fri, 23 Dec 2016 18:41:29 +0200 |
| |
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
| |