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

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

Hi Ramiro,

Am Donnerstag, den 12.01.2017, 18:34 +0000 schrieb Ramiro Oliveira:
> "The *_get_optional_* functions weren't really optional so this patch
> makes them really optional.
> 
> These *_get_optional_* functions will now return NULL instead of an error
> if no matching reset phandle is found in the DT, and all the
> reset_control_* functions now accept NULL rstc pointers.
> 
> Signed-off-by: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
> ---
>  drivers/reset/core.c  | 59 ++++++++++++++++++++++++++++++++++++++++-----------
>  include/linux/reset.h | 45 ++++++++++++++++++++++-----------------
>  2 files changed, 73 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 272c1e4ecb5c..7b679b0bfef8 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -143,12 +143,18 @@ EXPORT_SYMBOL_GPL(devm_reset_controller_register);
>   * a no-op.
>   * Consumers must not use reset_control_(de)assert on shared reset lines when
>   * reset_control_reset has been used.
> + *
> + * If rstc is NULL it is an optional reset and the function will just
> + * return 0.
>   */
>  int reset_control_reset(struct reset_control *rstc)
>  {
>  	int ret;
>  
> -	if (WARN_ON(IS_ERR_OR_NULL(rstc)))
> +	if (!rstc)
> +		return 0;
> +
> +	if (WARN_ON(IS_ERR(rstc)))
>  		return -EINVAL;
>  
>  	if (!rstc->rcdev->ops->reset)
> @@ -182,10 +188,17 @@ EXPORT_SYMBOL_GPL(reset_control_reset);
>   * internal state to be reset, but must be prepared for this to happen.
>   * Consumers must not use reset_control_reset on shared reset lines when
>   * reset_control_(de)assert has been used.
> + * return 0.
> + *
> + * 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 (WARN_ON(IS_ERR_OR_NULL(rstc)))
> +	if (!rstc)
> +		return 0;
> +
> +	if (WARN_ON(IS_ERR(rstc)))
>  		return -EINVAL;
>  
>  	if (!rstc->rcdev->ops->assert)
> @@ -213,10 +226,17 @@ EXPORT_SYMBOL_GPL(reset_control_assert);
>   * After calling this function, the reset is guaranteed to be deasserted.
>   * Consumers must not use reset_control_reset on shared reset lines when
>   * reset_control_(de)assert has been used.
> + * return 0.
> + *
> + * 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 (WARN_ON(IS_ERR_OR_NULL(rstc)))
> +	if (!rstc)
> +		return 0;
> +
> +	if (WARN_ON(IS_ERR(rstc)))
>  		return -EINVAL;
>  
>  	if (!rstc->rcdev->ops->deassert)
> @@ -237,12 +257,15 @@ 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 (WARN_ON(IS_ERR_OR_NULL(rstc)))
> +	if (!rstc)
> +		return 0;
> +
> +	if (WARN_ON(IS_ERR(rstc)))
>  		return -EINVAL;
>  
>  	if (rstc->rcdev->ops->status)
> @@ -299,7 +322,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;
> @@ -313,14 +337,24 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
>  	if (id) {
>  		index = of_property_match_string(node,
>  						 "reset-names", id);
> -		if (index < 0)
> -			return ERR_PTR(-ENOENT);
> +		if (index < 0) {
> +			if (optional)
> +				return NULL;
> +			else
> +				return (index == -EILSEQ) ?
> +					ERR_PTR(index) : ERR_PTR(-ENOENT);

That is not what I meant. I think reset_control_get should return an
error on EILSEQ even if optional is true, since that means there are
resets specified in the DT, but the reset-names property is broken.
How about:
		if (index == -EILSEQ)
			return ERR_PTR(index);
		if (index < 0)
			return optional ? NULL : ERR_PTR(-ENOENT);

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

That is not what I meant either. If the phandle parsing fails with
of_phandle_iterator_next returning -EINVAL, that should be passed along
in the optional case: that happens when we've already found a matching
entry in reset-names, but the resets property is broken.
How about:
	if (ret == -EINVAL)
		return ERR_PTR(ret);
	if (ret)
		return optional ? NULL : ERR_PTR(ret);

regards
Philipp