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.
> + 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.
> {
> 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;
?
>
> 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?
> +}
> #endif
>
> /**
> --
> 2.17.1
>
Regards,
Simon
