Hi Kishon,
On Wed, 21 Jul 2021 at 06:16, Kishon Vijay Abraham I <kis...@ti.com> wrote:
>
> 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?
Just the sandbox_reset_test_get_devm() function. It doesn't seem to
need to be in the driver, but I may be wrong.
> >
> >> {
> >> 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;
Well when you add comments about the return value to devm_to_reset() I
will understand this better :-)
But ret must be zero if the reset is valid and non-zero if not, right?
So why return a different error from the one that occured? That will
make it harder for people to figure out.
Also, I suggest using
return log_msg_ret("something", ret)
so we can get logging to show where the error was originated.
> >
> >>
> >> 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.
OK
Regards,
Simon
