2016-12-15 18:06:05

by Ramiro Oliveira

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

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 <[email protected]>
---
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

2016-12-23 10:59:01

by Philipp Zabel

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

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 <[email protected]>
> ---
> 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

2016-12-23 11:22:41

by Laurent Pinchart

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

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 <[email protected]>
> > ---
> >
> > 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

2016-12-23 12:08:58

by Philipp Zabel

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

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 <[email protected]>
> > > ---
> > >
> > > 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

2016-12-23 16:41:01

by Laurent Pinchart

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

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 <[email protected]>
> >>> ---
> >>>
> >>> 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

2016-12-23 16:57:13

by Ramiro Oliveira

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

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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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;
>

2016-12-23 17:20:09

by Ramiro Oliveira

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

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 <[email protected]>
>>>> ---
>>>>
>>>> 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

2016-12-23 17:56:50

by Laurent Pinchart

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

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 <[email protected]>
> >>>> ---
> >>>>
> >>>> 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