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
