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

Hi Ramiro,

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

>   */
>  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 to __of_reset_control_get and let that return NULL if optional
is set and either of_property_match_string or of_parse_phandle_with_args
would cause an -ENOENT return.

The stub could then
	return optional ? NULL : ERR_PTR(-EONOENT);

> +	return desc;
>  }
>  
>  /**
> @@ -273,13 +293,31 @@ static inline struct reset_control *devm_reset_control_get_shared(
>  static inline struct reset_control *devm_reset_control_get_optional_exclusive(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, 0);
> +	struct reset_control *desc;
> +
> +	desc = __devm_reset_control_get(dev, id, 0, 0);
> +
> +	if (IS_ERR(desc)) {
> +		if (PTR_ERR(desc) == -ENOENT)
> +			return NULL;
> +	}

Same as for __of_reset_control_get above.

> +	return desc;
>  }
>  
>  static inline struct reset_control *devm_reset_control_get_optional_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, 1);
> +	struct reset_control *desc;
> +
> +	desc = __devm_reset_control_get(dev, id, 0, 1);
> +
> +	if (IS_ERR(desc)) {
> +		if (PTR_ERR(desc) == -ENOENT)
> +			return NULL;
> +	}
> +
> +	return desc;
>  }
>  
>  /**

regards
Philipp