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

From: Philipp Zabel
Date: Mon Jan 09 2017 - 05:48:31 EST

Next message: Linus Walleij: "Re: [PATCH] iio: misc: add a generic regulator driver"
Previous message: Suzuki K Poulose: "Re: [PATCH v3 4/9] arm64: cpufeature: Document the rules of safe value for features"
Next in thread: Ramiro Oliveira: "Re: [PATCH v2 2/2] reset: make optional functions really optional"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

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

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

Next message: Linus Walleij: "Re: [PATCH] iio: misc: add a generic regulator driver"
Previous message: Suzuki K Poulose: "Re: [PATCH v3 4/9] arm64: cpufeature: Document the rules of safe value for features"
Next in thread: Ramiro Oliveira: "Re: [PATCH v2 2/2] reset: make optional functions really optional"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]