On 22/10/2019 00:53, Simon Glass wrote:
Hi Jean-Jacques,
On Mon, 21 Oct 2019 at 01:45, Jean-Jacques Hiblot <jjhib...@ti.com> wrote:
On 18/10/2019 22:38, Simon Glass wrote:
Hi Jean-Jacques,
On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhib...@ti.com> wrote:
Prepare the way for a managed GPIO API by handling NULL pointers without
crashing nor failing.
VALIDATE_DESC() and validate_desc() come straight from Linux.
Signed-off-by: Jean-Jacques Hiblot <jjhib...@ti.com>
---
drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
include/asm-generic/gpio.h | 2 +-
2 files changed, 57 insertions(+), 11 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 01cfa2f788..63c10f438b 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -18,6 +18,33 @@
DECLARE_GLOBAL_DATA_PTR;
+/*
+ * This descriptor validation needs to be inserted verbatim into each
+ * function taking a descriptor, so we need to use a preprocessor
+ * macro to avoid endless duplication. If the desc is NULL it is an
+ * optional GPIO and calls should just bail out.
+ */
+static int validate_desc(const struct gpio_desc *desc, const char *func)
+{
+ if (!desc)
+ return 0;
+ if (IS_ERR(desc)) {
+ pr_warn("%s: invalid GPIO (errorpointer)\n", func);
+ return PTR_ERR(desc);
+ }
+ if (!desc->dev) {
+ pr_warn("%s: invalid GPIO (no device)\n", func);
+ return -EINVAL;
+ }
+ return 1;
+}
+
+#define VALIDATE_DESC(desc) do { \
+ int __valid = validate_desc(desc, __func__); \
+ if (__valid <= 0) \
+ return __valid; \
+ } while (0)
This adds to code size so should be behind a CONFIG I think.
I'm not sure we really want to keep this out. Most of the added code
size, will be about the error messages. I would rather remove them (or
use a debug() or warn_non_spl()
You should probably do that anyway.
But these checks do add to code size, and we should be careful not to
have unnecessary checks in the final firmware. We can enable them
during development, but don't want the code bloated with lots of
pointless checks.
I see, but I don't agree that this is pointless. It allows for optional
GPIO support.
Practically speaking, in u-boot, that will probably be used only by the
users of the managed API: devm_gpiod_get_optional() and
devm_gpiod_get_index_optional()
+
/**
I't the implicit return that I am not keen on. Is it needed? I don't
see that sort of thing in U-Boot at present.
Well it bothered me too at first when I looked at how it was done in
linux. But I got used to it pretty quickly :)
I'll remove this in the v2
Thanks for the review.
JJ
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
