Hi Simon,
On 21/07/21 12:02 am, Simon Glass wrote:
> Hi Kishon,
>
> On Mon, 12 Jul 2021 at 00:20, Kishon Vijay Abraham I <kis...@ti.com> wrote:
>>
>> Add devm_to_reset() to return dummy "struct reset_ctl", useful for
>> invoking reset APIs which doesn't have a valid "struct reset_ctl".
>>
>> Suggested-by: Simon Glass <s...@chromium.org>
>> Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
>> ---
>> drivers/reset/reset-uclass.c | 16 ++++++++++++++++
>> drivers/reset/sandbox-reset-test.c | 4 +++-
>> include/reset.h | 17 +++++++++++++++++
>> 3 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
>> index 8caa616ed9..9f2eeb4fe4 100644
>> --- a/drivers/reset/reset-uclass.c
>> +++ b/drivers/reset/reset-uclass.c
>> @@ -15,6 +15,8 @@
>> #include <dm/devres.h>
>> #include <dm/lists.h>
>>
>> +static struct reset_ctl devm_no_reset;
>> +
>> static inline struct reset_ops *reset_dev_ops(struct udevice *dev)
>> {
>> return (struct reset_ops *)dev->driver->ops;
>> @@ -256,6 +258,20 @@ int reset_release_all(struct reset_ctl *reset_ctl, int
>> count)
>> return 0;
>> }
>>
>> +int devm_to_reset(struct reset_ctl *in, struct reset_ctl **outp)
>> +{
>> + if (IS_ERR_OR_NULL(in)) {
>> + *outp = &devm_no_reset;
>
> This var ends up in BSS so won't work in early SPL / pre-relocation in
> U-Boot. Please add a priv_auto struct in the UCLASS_DRIVER() and put
> this value in there.
Sure.
>
>> + if (!in)
>> + return -ENOENT;
>> + else
>> + return PTR_ERR(in);
>> + }
>> + *outp = in;
>> +
>> + return 0;
>> +}
>> +
>> static void devm_reset_release(struct udevice *dev, void *res)
>> {
>> reset_free(res);
>> diff --git a/drivers/reset/sandbox-reset-test.c
>> b/drivers/reset/sandbox-reset-test.c
>> index 51b79810c8..4389bde5e7 100644
>> --- a/drivers/reset/sandbox-reset-test.c
>> +++ b/drivers/reset/sandbox-reset-test.c
>> @@ -32,13 +32,15 @@ int sandbox_reset_test_get_devm(struct udevice *dev)
>
> Can this function move into the test itself? It seems strange for it to be
> here.
You mean merge drivers/reset/sandbox-reset-test.c to test/dm/reset.c?
>
>> {
>> struct sandbox_reset_test *sbrt = dev_get_priv(dev);
>> struct reset_ctl *r;
>> + int ret;
>>
>> r = devm_reset_control_get(dev, "not-a-valid-reset-ctl");
>> if (!IS_ERR(r))
>> return -EINVAL;
>>
>> r = devm_reset_control_get_optional(dev, "not-a-valid-reset-ctl");
>> - if (r)
>> + ret = devm_to_reset(r, &r);
>> + if (reset_valid(r))
>> return -EINVAL;
>
> Can you not do:
>
> if (ret)
> return ret;
>
> ?
For a reset that doesn't exist (where we return devm_no_reset), the
return value is still going to be a negative value. We should probably
change the check to
if (ret && reset_valid(r))
return -EINVAL;
>
>>
>> sbrt->ctlp = devm_reset_control_get(dev, "test");
>> diff --git a/include/reset.h b/include/reset.h
>> index cde2c4b4a8..21affc1455 100644
>> --- a/include/reset.h
>> +++ b/include/reset.h
>> @@ -341,6 +341,18 @@ int reset_status(struct reset_ctl *reset_ctl);
>> */
>> int reset_release_all(struct reset_ctl *reset_ctl, int count);
>>
>> +/**
>> + * devm_to_reset - Provide a dummy reset_ctl for invalid reset_ctl
>
> Returns the reset_ctl to use for a devm reset?
>
>> + *
>> + * Provide a dummy reset_ctl for invalid reset_ctl or give the same
>> + * input reset_ctl
>> + *
>> + * @in: Input reset_ctl returned by devm_reset_control_get()
>> + * @outp: Dummy reset_ctl for invalid reset_ctl or Input reset_ctl
>
> Put the positive case first.
>
> Returns @in if valid. If not valid, returns a dummy reset_ctl for
> which reset_valid() will always return false.
>
>> + * @return 0 if OK, or a negative error code.
>
> Please document what specific errors this returns as this is important.
>
>> + */
>> +int devm_to_reset(struct reset_ctl *in, struct reset_ctl **outp);
>> +
>> /**
>> * reset_release_bulk - Assert/Free an array of previously requested reset
>> * signals in a reset control bulk struct.
>> @@ -456,6 +468,11 @@ static inline int reset_release_bulk(struct
>> reset_ctl_bulk *bulk)
>> {
>> return 0;
>> }
>> +
>> +static inline int devm_to_reset(struct reset_ctl *in, struct reset_ctl
>> **outp)
>> +{
>> + return 0;
>
> That indicates success but *outp is not set. Is that OK? Should you
> return -ENOSYS, or maybe set *outp to something?
yeah, return 0 and *outp to something looks correct.
Thanks
Kishon
