Hi Kishon,
On Wed, 21 Jul 2021 at 06:16, Kishon Vijay Abraham I kishon@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 kishon@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 sjg@chromium.org Signed-off-by: Kishon Vijay Abraham I kishon@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;elsereturn 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