Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1557714 > unrolled thread

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

Started byRamiro Oliveira <Ramiro.Oliveira@synopsys.com>
First post2017-01-12 19:40 +0100
Last post2017-01-13 10:30 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/2] reset: make optional functions really optional Ramiro Oliveira <Ramiro.Oliveira@synopsys.com> - 2017-01-12 19:40 +0100
    Re: [PATCH v3 2/2] reset: make optional functions really optional Philipp Zabel <p.zabel@pengutronix.de> - 2017-01-13 10:20 +0100
      Re: [PATCH v3 2/2] reset: make optional functions really optional Ramiro Oliveira <Ramiro.Oliveira@synopsys.com> - 2017-01-13 10:30 +0100

#1557714 — [PATCH v3 2/2] reset: make optional functions really optional

FromRamiro Oliveira <Ramiro.Oliveira@synopsys.com>
Date2017-01-12 19:40 +0100
Subject[PATCH v3 2/2] reset: make optional functions really optional
Message-ID<sYSs1-7lL-11@gated-at.bofh.it>
"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);
+		}
 	}
 
 	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);
+	}
 
 	mutex_lock(&reset_list_mutex);
 	rcdev = NULL;
@@ -379,7 +413,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;
 
@@ -389,7 +424,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..86b4ed75359e 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);
 }
 
 /*
-- 
2.11.0

[toc] | [next] | [standalone]

#1558151

FromPhilipp Zabel <p.zabel@pengutronix.de>
Date2017-01-13 10:20 +0100
Message-ID<sZ6bE-7uW-37@gated-at.bofh.it>
In reply to#1557714
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

[toc] | [prev] | [next] | [standalone]

#1558152

FromRamiro Oliveira <Ramiro.Oliveira@synopsys.com>
Date2017-01-13 10:30 +0100
Message-ID<sZ6lj-7yb-1@gated-at.bofh.it>
In reply to#1558151
Hi Philipp

On 1/13/2017 9:11 AM, Philipp Zabel wrote:
> 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);
> 

When you first suggested I wasn't entirely sure what you meant, but now it makes
total sense to me.

I should have asked you about it before implementing it.

>> +		}
>>  	}
>>  
>>  	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);
> 

Same as above.

> regards
> Philipp
> 

I'll refactor the code and send a new patch ASAP.

BRs,
Ramiro

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web