Re: [PATCH v2 2/2] reset: make optional functions really optional

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Ramiro,

Am Dienstag, den 27.12.2016, 12:37 +0000 schrieb Ramiro Oliveira:
> The optional functions weren't really optional so this patch makes them
> really optional.

Please add a bit of detail to the description. Since this changes the
API, you should mention that the reset_control_get_optional variants now
return NULL instead of an error if there is no matching reset phandle in
the device tree and that the reset_control_* functions accept NULL rstc
pointers.

> Signed-off-by: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
> ---
>  drivers/reset/core.c  | 35 +++++++++++++++++++++++++++++------
>  include/linux/reset.h | 45 ++++++++++++++++++++++++++-------------------
>  2 files changed, 55 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 70023997d031..f933e9dfebc5 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -135,9 +135,15 @@ EXPORT_SYMBOL_GPL(devm_reset_controller_register);
>   * @rstc: reset controller
>   *
>   * Calling this on a shared reset controller is an error.
> + *
> + * 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 +164,15 @@ 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 rstc is NULL it is an optional reset and the function will just
> + * return 0.
>   */
>  int reset_control_assert(struct reset_control *rstc)
>  {
> +	if (!rstc)
> +		return 0;
> +
>  	if (!rstc->rcdev->ops->assert)
>  		return -ENOTSUPP;
>  
> @@ -181,9 +193,15 @@ EXPORT_SYMBOL_GPL(reset_control_assert);
>   * @rstc: reset controller
>   *
>   * After calling this function, the reset is guaranteed to be deasserted.
> + *
> + * If rstc is NULL it is an optional reset and the function will just
> + * return 0.
>   */
>  int reset_control_deassert(struct reset_control *rstc)
>  {
> +	if (!rstc)
> +		return 0;
> +
>  	if (!rstc->rcdev->ops->deassert)
>  		return -ENOTSUPP;
>  
> @@ -199,11 +217,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);
>  
> @@ -258,7 +279,8 @@ static void __reset_control_put(struct reset_control *rstc)
>  }
>  
>  struct reset_control *__of_reset_control_get(struct device_node *node,
> -				     const char *id, int index, bool shared)
> +				     const char *id, int index, bool shared,
> +				     bool optional)
>  {
>  	struct reset_control *rstc;
>  	struct reset_controller_dev *r, *rcdev;
> @@ -273,13 +295,13 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
>  		index = of_property_match_string(node,
>  						 "reset-names", id);
>  		if (index < 0)
> -			return ERR_PTR(-ENOENT);
> +			return optional ? NULL : ERR_PTR(-ENOENT);

of_property_match_string can return -EINVAL, -ENODATA, or -EILSEQ.
I think -EILSEQ should still be returned.

>  	}
>  
>  	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
>  					 index, &args);
>  	if (ret)
> -		return ERR_PTR(ret);
> +		return optional ? NULL : ERR_PTR(ret);

of_parse_phandle_with_args can return -ENOENT or -EINVAL.
I think -EINVAL should still be returned.

>  
>  	mutex_lock(&reset_list_mutex);
>  	rcdev = NULL;
> @@ -338,7 +360,8 @@ static void devm_reset_control_release(struct device *dev, void *res)
>  }
>  
>  struct reset_control *__devm_reset_control_get(struct device *dev,
> -				     const char *id, int index, bool shared)
> +				     const char *id, int index, bool shared,
> +				     bool optional)
>  {
>  	struct reset_control **ptr, *rstc;
>  
> @@ -348,7 +371,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
>  		return ERR_PTR(-ENOMEM);
>  
>  	rstc = __of_reset_control_get(dev ? dev->of_node : NULL,
> -				      id, index, shared);
> +				      id, index, shared, optional);
>  	if (!IS_ERR(rstc)) {
>  		*ptr = rstc;
>  		devres_add(dev, ptr);
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index ec1d1fd28f5f..e4cc7146b5ed 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -13,10 +13,12 @@ int reset_control_deassert(struct reset_control *rstc);
>  int reset_control_status(struct reset_control *rstc);
>  
>  struct reset_control *__of_reset_control_get(struct device_node *node,
> -				     const char *id, int index, bool shared);
> +				     const char *id, int index, bool shared,
> +				     bool optional);
>  void reset_control_put(struct reset_control *rstc);
>  struct reset_control *__devm_reset_control_get(struct device *dev,
> -				     const char *id, int index, bool shared);
> +				     const char *id, int index, bool shared,
> +				     bool optional);
>  
>  int __must_check device_reset(struct device *dev);
>  
> @@ -69,14 +71,15 @@ static inline int device_reset_optional(struct device *dev)
>  
>  static inline struct reset_control *__of_reset_control_get(
>  					struct device_node *node,
> -					const char *id, int index, bool shared)
> +					const char *id, int index, bool shared,
> +					bool optional)
>  {
>  	return ERR_PTR(-ENOTSUPP);
>  }
>  
>  static inline struct reset_control *__devm_reset_control_get(
> -					struct device *dev,
> -					const char *id, int index, bool shared)
> +					struct device *dev, const char *id,
> +					int index, bool shared, bool optional)
>  {
>  	return ERR_PTR(-ENOTSUPP);
>  }
> @@ -104,7 +107,8 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>  #ifndef CONFIG_RESET_CONTROLLER
>  	WARN_ON(1);
>  #endif
> -	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0);
> +	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
> +									false);
>  }
>  
>  /**
> @@ -132,19 +136,22 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>  static inline struct reset_control *reset_control_get_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true);
> +	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
> +									false);
>  }
>  
>  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, false);
> +	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
> +									true);
>  }
>  
>  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, true);
> +	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
> +									true);
>  }
>  
>  /**
> @@ -160,7 +167,7 @@ static inline struct reset_control *reset_control_get_optional_shared(
>  static inline struct reset_control *of_reset_control_get_exclusive(
>  				struct device_node *node, const char *id)
>  {
> -	return __of_reset_control_get(node, id, 0, 0);
> +	return __of_reset_control_get(node, id, 0, false, false);
>  }
>  
>  /**
> @@ -185,7 +192,7 @@ static inline struct reset_control *of_reset_control_get_exclusive(
>  static inline struct reset_control *of_reset_control_get_shared(
>  				struct device_node *node, const char *id)
>  {
> -	return __of_reset_control_get(node, id, 0, true);
> +	return __of_reset_control_get(node, id, 0, true, false);
>  }
>  
>  /**
> @@ -202,7 +209,7 @@ static inline struct reset_control *of_reset_control_get_shared(
>  static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>  					struct device_node *node, int index)
>  {
> -	return __of_reset_control_get(node, NULL, index, false);
> +	return __of_reset_control_get(node, NULL, index, false, false);
>  }
>  
>  /**
> @@ -230,7 +237,7 @@ static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>  static inline struct reset_control *of_reset_control_get_shared_by_index(
>  					struct device_node *node, int index)
>  {
> -	return __of_reset_control_get(node, NULL, index, true);
> +	return __of_reset_control_get(node, NULL, index, true, false);
>  }
>  
>  /**
> @@ -252,7 +259,7 @@ __must_check devm_reset_control_get_exclusive(struct device *dev,
>  #ifndef CONFIG_RESET_CONTROLLER
>  	WARN_ON(1);
>  #endif
> -	return __devm_reset_control_get(dev, id, 0, false);
> +	return __devm_reset_control_get(dev, id, 0, false, false);
>  }
>  
>  /**
> @@ -267,19 +274,19 @@ __must_check devm_reset_control_get_exclusive(struct device *dev,
>  static inline struct reset_control *devm_reset_control_get_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, true);
> +	return __devm_reset_control_get(dev, id, 0, true, false);
>  }
>  
>  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, false);
> +	return __devm_reset_control_get(dev, id, 0, false, true);
>  }
>  
>  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, true);
> +	return __devm_reset_control_get(dev, id, 0, true, true);
>  }
>  
>  /**
> @@ -297,7 +304,7 @@ static inline struct reset_control *devm_reset_control_get_optional_shared(
>  static inline struct reset_control *
>  devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>  {
> -	return __devm_reset_control_get(dev, NULL, index, false);
> +	return __devm_reset_control_get(dev, NULL, index, false, false);
>  }
>  
>  /**
> @@ -313,7 +320,7 @@ devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>  static inline struct reset_control *
>  devm_reset_control_get_shared_by_index(struct device *dev, int index)
>  {
> -	return __devm_reset_control_get(dev, NULL, index, true);
> +	return __devm_reset_control_get(dev, NULL, index, true, false);
>  }
>  
>  /*

regards
Philipp