Message-ID: <1482494934.2394.53.camel@pengutronix.de>
Subject: Re: [PATCH] reset: Make optional functions really optional.
From: Philipp Zabel <p.zabel@pengutronix.de>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>,
        linux-kernel@vger.kernel.org, CARLOS.PALMINHA@synopsys.com
Date: Fri, 23 Dec 2016 13:08:54 +0100
In-Reply-To: <4238328.E0tRokcGUd@avalon>
References: <58b07c24e1bc263db0d43274b46022c8d8506302.1481822669.git.Ramiro.Oliveira@synopsys.com>
         <1482490737.2394.37.camel@pengutronix.de> <4238328.E0tRokcGUd@avalon>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5489
Lines: 166

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://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".

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.

regards
Philipp