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
