[PATCH] reset: Make optional functions really optional.

[PATCH] reset: Make optional functions really optional.

[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.
  */
 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.
  */
 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.
  */
 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);
+
+	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;
+	}
+
+	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;
+	}
+
+	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;
 }
 
 /**
-- 
2.10.2

Re: [PATCH] reset: Make optional functions really optional.

[Philipp Zabel]

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

Re: [PATCH] reset: Make optional functions really optional.

[Laurent Pinchart]

Hello,

On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote:
> 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."

Maybe we should document in a single place that NULL is a valid value for a 
reset_control pointer and will result in the API behaving as a no-op ? If you 
want to duplicate the information I'd still prefer talking about no-op than 
about "just returning 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

What's wrong with bool by the way ? :-)

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

Laurent Pinchart

Re: [PATCH] reset: Make optional functions really optional.

[Philipp Zabel]

Hi Laurent,

Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart:
> Hello,
> 
> On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote:
> > 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."
> 
> Maybe we should document in a single place that NULL is a valid value for a 
> reset_control pointer and will result in the API behaving as a no-op ? If you 
> want to duplicate the information I'd still prefer talking about no-op than 
> about "just returning 0".

Does "no-op" implicate the return value 0? Maybe there is a better way
to express "no action, returns 0".

Currently there is no central place for this information, and as long as
the text not much longer than a reference to the central location would
be, I'm fine with duplication.

> > >   */
> > >  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
> 
> What's wrong with bool by the way ? :-)

Nothing wrong, it's just that the "exclusive" parameter is already int.
I'd be perfectly fine with using bool for both.

regards
Philipp

Re: [PATCH] reset: Make optional functions really optional.

[Laurent Pinchart]

Hi Philipp,

On Friday 23 Dec 2016 13:08:54 Philipp Zabel wrote:
> Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart:
> > On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote:
> >> 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."
> > 
> > Maybe we should document in a single place that NULL is a valid value for
> > a reset_control pointer and will result in the API behaving as a no-op ?
> > If you want to duplicate the information I'd still prefer talking about
> > no-op than about "just returning 0".
> 
> Does "no-op" implicate the return value 0? Maybe there is a better way
> to express "no action, returns 0".

The important point in my opinion is that a NULL argument will result in the 
function performing no operation and indicating success exactly like a call 
with a non-NULL pointer would. The proposed text makes it sound like a 0 
return value is specific to the NULL argument case. This is a detail though.

> Currently there is no central place for this information, and as long as
> the text not much longer than a reference to the central location would
> be, I'm fine with duplication.
> 
> >>>   */
> >>>  int reset_control_reset(struct reset_control *rstc)
> >>>  {
> >>> +	if (!rstc)
> >>> +		return 0;
> >>> +
> >>>  	if (WARN_ON(rstc->shared))
> >>>  		return -EINVAL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH] reset: Make optional functions really optional.

[Ramiro Oliveira]

Hi Laurent and Philipp

On 12/23/2016 4:41 PM, Laurent Pinchart wrote:
> Hi Philipp,
> 
> On Friday 23 Dec 2016 13:08:54 Philipp Zabel wrote:
>> Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart:
>>> On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote:
>>>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2016_12_14_502&d=DgICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=BHEb-RADEOm-lgrwdN4zqtr2BWZMjeocyTkjphE6PrA&m=8s4unvlk7rXGYKdQcMBxpYLmdnROh5aQ_iHU03InFoM&s=oNBgTOo47LBs0JvtJ5Qd_6uVqrcMkWAq1PmNN4qt16g&e= 
>>>>>
>>>>> 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."
>>>
>>> Maybe we should document in a single place that NULL is a valid value for
>>> a reset_control pointer and will result in the API behaving as a no-op ?
>>> If you want to duplicate the information I'd still prefer talking about
>>> no-op than about "just returning 0".
>>
>> Does "no-op" implicate the return value 0? Maybe there is a better way
>> to express "no action, returns 0".
> 
> The important point in my opinion is that a NULL argument will result in the 
> function performing no operation and indicating success exactly like a call 
> with a non-NULL pointer would. The proposed text makes it sound like a 0 
> return value is specific to the NULL argument case. This is a detail though.
> 

"If rstc is NULL it is an optional reset and the function will just return 0
like any other successful call."

Do you guys think the above message is more explicit?

>> Currently there is no central place for this information, and as long as
>> the text not much longer than a reference to the central location would
>> be, I'm fine with duplication.
>>
>>>>>   */
>>>>>  int reset_control_reset(struct reset_control *rstc)
>>>>>  {
>>>>> +	if (!rstc)
>>>>> +		return 0;
>>>>> +
>>>>>  	if (WARN_ON(rstc->shared))
>>>>>  		return -EINVAL;
> 

Re: [PATCH] reset: Make optional functions really optional.

[Ramiro Oliveira]

Hi Philipp

On 12/23/2016 12:08 PM, Philipp Zabel wrote:
> Hi Laurent,
> 
> Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart:
>> Hello,
>>
>> On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote:
>>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2016_12_14_502&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=BHEb-RADEOm-lgrwdN4zqtr2BWZMjeocyTkjphE6PrA&m=_0T0di-X6zgDw8ZRLDNk2ExL2EieBiCmAmuxc8OGAg4&s=H5BfD4P5MB85jtyUjDrn6yKu-6ws5srNWNNiFpPL0pQ&e= 
>>>>
>>>> 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."
>>
>> Maybe we should document in a single place that NULL is a valid value for a 
>> reset_control pointer and will result in the API behaving as a no-op ? If you 
>> want to duplicate the information I'd still prefer talking about no-op than 
>> about "just returning 0".
> 
> Does "no-op" implicate the return value 0? Maybe there is a better way
> to express "no action, returns 0".
> 
> Currently there is no central place for this information, and as long as
> the text not much longer than a reference to the central location would
> be, I'm fine with duplication.
> 
>>>>   */
>>>>  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
>>
>> What's wrong with bool by the way ? :-)
> 
> Nothing wrong, it's just that the "exclusive" parameter is already int.
> I'd be perfectly fine with using bool for both.
> 

Do you prefer me to keep them both int, or change them to bool?

BRs,
Ramiro

Re: [PATCH] reset: Make optional functions really optional.

[Laurent Pinchart]

Hi Ramiro,

On Friday 23 Dec 2016 17:19:43 Ramiro Oliveira wrote:
> On 12/23/2016 12:08 PM, Philipp Zabel wrote:
> > Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart:
> >> On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote:
> >>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_201
> >>>> 6_12_14_502&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=BHEb-RADEOm-lgrwdN4zqtr
> >>>> 2BWZMjeocyTkjphE6PrA&m=_0T0di-X6zgDw8ZRLDNk2ExL2EieBiCmAmuxc8OGAg4&s=H5
> >>>> BfD4P5MB85jtyUjDrn6yKu-6ws5srNWNNiFpPL0pQ&e=
> >>>> 
> >>>> 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

[snip]

> >>>>  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
> >> 
> >> What's wrong with bool by the way ? :-)
> > 
> > Nothing wrong, it's just that the "exclusive" parameter is already int.
> > I'd be perfectly fine with using bool for both.
> 
> Do you prefer me to keep them both int, or change them to bool?

I'd prefer bool myself, it's slightly more explicit.

-- 
Regards,

Laurent Pinchart