lkml.org 
[lkml]   [2016]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] reset: Make optional functions really optional.
From
Date
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

\
 
 \ /
  Last update: 2016-12-23 18:20    [W:0.428 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site