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

[Ramiro Oliveira]

Hi Philipp

On 1/9/2017 10:45 AM, Philipp Zabel wrote:
> 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.
> 

Would you be ok with something like this:

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

I'll make the function return NULL, -ENOENT, or -EILSEQ.

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

Same as above. I'll make the function return NULL, -ENOENT, or -EINVAL.

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

BRs,
Ramiro