[U-Boot] [PATCH v1 0/3] gpio: Add a managed API
This is the 4th of a few series, the goal of which is to facilitate porting drivers from the linux kernel. Most of the series will be about adding managed API to existing infrastructure (GPIO, reset, phy,...)
This particular series is about GPIOs. It adds a managed API using the API as Linux. To make it 100% compatible with linux, there is a small deviation from u-boot's way of naming the gpio lists: the managed equivalent of gpio_request_by_name(..,"blabla-gpios", ...) is devm_gpiod_get_index(..., "blabla", ...)
Jean-Jacques Hiblot (3): drivers: gpio: Handle gracefully NULL pointers drivers: gpio: Add a managed API to get a GPIO from the device-tree test: gpio: Add tests for the managed API
arch/sandbox/dts/test.dts | 10 +++ drivers/gpio/gpio-uclass.c | 134 ++++++++++++++++++++++++++++++++++--- include/asm-generic/gpio.h | 49 +++++++++++++- test/dm/gpio.c | 102 ++++++++++++++++++++++++++++ 4 files changed, 284 insertions(+), 11 deletions(-)
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 jjhiblot@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) + /** * gpio_to_device() - Convert global GPIO number to device, number * @@ -269,11 +296,14 @@ int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc)
int dm_gpio_request(struct gpio_desc *desc, const char *label) { - struct udevice *dev = desc->dev; + struct udevice *dev; struct gpio_dev_priv *uc_priv; char *str; int ret;
+ VALIDATE_DESC(desc); + dev = desc->dev; + uc_priv = dev_get_uclass_priv(dev); if (uc_priv->name[desc->offset]) return -EBUSY; @@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc, const char *func) { struct gpio_dev_priv *uc_priv;
+ VALIDATE_DESC(desc); + if (!dm_gpio_is_valid(desc)) return -ENOENT;
@@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc) int value; int ret;
+ VALIDATE_DESC(desc); + ret = check_reserved(desc, "get_value"); if (ret) return ret; @@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value) { int ret;
+ VALIDATE_DESC(desc); + ret = check_reserved(desc, "set_value"); if (ret) return ret; @@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
int dm_gpio_get_open_drain(struct gpio_desc *desc) { - struct dm_gpio_ops *ops = gpio_get_ops(desc->dev); + struct dm_gpio_ops *ops; int ret;
+ VALIDATE_DESC(desc); + ops = gpio_get_ops(desc->dev); + ret = check_reserved(desc, "get_open_drain"); if (ret) return ret; @@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
int dm_gpio_set_open_drain(struct gpio_desc *desc, int value) { - struct dm_gpio_ops *ops = gpio_get_ops(desc->dev); + struct dm_gpio_ops *ops; int ret;
+ VALIDATE_DESC(desc); + ops = gpio_get_ops(desc->dev); + ret = check_reserved(desc, "set_open_drain"); if (ret) return ret; @@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) { - struct udevice *dev = desc->dev; - struct dm_gpio_ops *ops = gpio_get_ops(dev); + struct udevice *dev; + struct dm_gpio_ops *ops; int ret;
+ VALIDATE_DESC(desc); + dev = desc->dev; + ops = gpio_get_ops(dev); + ret = check_reserved(desc, "set_dir"); if (ret) return ret; @@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc) int gpio_get_value(unsigned gpio) { int ret; - struct gpio_desc desc;
ret = gpio_to_device(gpio, &desc); @@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc) { + VALIDATE_DESC(desc); + /* For now, we don't do any checking of dev */ return _dm_gpio_free(desc->dev, desc->offset); } @@ -981,12 +1028,11 @@ static int gpio_renumber(struct udevice *removed_dev)
int gpio_get_number(const struct gpio_desc *desc) { - struct udevice *dev = desc->dev; struct gpio_dev_priv *uc_priv;
- if (!dev) - return -1; - uc_priv = dev->uclass_priv; + VALIDATE_DESC(desc); + + uc_priv = dev_get_uclass_priv(desc->dev);
return uc_priv->gpio_base + desc->offset; } diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index d6cf18744f..ba669b65a9 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -139,7 +139,7 @@ struct gpio_desc { */ static inline bool dm_gpio_is_valid(const struct gpio_desc *desc) { - return desc->dev != NULL; + return desc && desc->dev; }
/**
Hi Jean-Jacques,
On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot jjhiblot@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 jjhiblot@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.
/**
- gpio_to_device() - Convert global GPIO number to device, number
@@ -269,11 +296,14 @@ int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc)
int dm_gpio_request(struct gpio_desc *desc, const char *label) {
struct udevice *dev = desc->dev;
struct udevice *dev; struct gpio_dev_priv *uc_priv; char *str; int ret;VALIDATE_DESC(desc);dev = desc->dev;uc_priv = dev_get_uclass_priv(dev); if (uc_priv->name[desc->offset]) return -EBUSY;@@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc, const char *func) { struct gpio_dev_priv *uc_priv;
VALIDATE_DESC(desc);if (!dm_gpio_is_valid(desc)) return -ENOENT;@@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc) int value; int ret;
VALIDATE_DESC(desc);ret = check_reserved(desc, "get_value"); if (ret) return ret;@@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value) { int ret;
VALIDATE_DESC(desc);ret = check_reserved(desc, "set_value"); if (ret) return ret;@@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
int dm_gpio_get_open_drain(struct gpio_desc *desc) {
struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);ops = gpio_get_ops(desc->dev);ret = check_reserved(desc, "get_open_drain"); if (ret) return ret;@@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
int dm_gpio_set_open_drain(struct gpio_desc *desc, int value) {
struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);ops = gpio_get_ops(desc->dev);ret = check_reserved(desc, "set_open_drain"); if (ret) return ret;@@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) {
struct udevice *dev = desc->dev;struct dm_gpio_ops *ops = gpio_get_ops(dev);
struct udevice *dev;struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);dev = desc->dev;ops = gpio_get_ops(dev);ret = check_reserved(desc, "set_dir"); if (ret) return ret;@@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc) int gpio_get_value(unsigned gpio) { int ret;
unrelated change?
struct gpio_desc desc; ret = gpio_to_device(gpio, &desc);@@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc) {
VALIDATE_DESC(desc);/* For now, we don't do any checking of dev */ return _dm_gpio_free(desc->dev, desc->offset);} @@ -981,12 +1028,11 @@ static int gpio_renumber(struct udevice *removed_dev)
int gpio_get_number(const struct gpio_desc *desc) {
struct udevice *dev = desc->dev; struct gpio_dev_priv *uc_priv;if (!dev)return -1;uc_priv = dev->uclass_priv;
VALIDATE_DESC(desc);
I think this is pretty opaque. How about writing the code out in full, with a helper function to do the check. The helper function can return the error code perhaps.
ret = validate_desc(desc); if (ret) return log_msg_ret("gpio_free", ret);
Regards, Simon
uc_priv = dev_get_uclass_priv(desc->dev); return uc_priv->gpio_base + desc->offset;} diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index d6cf18744f..ba669b65a9 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -139,7 +139,7 @@ struct gpio_desc { */ static inline bool dm_gpio_is_valid(const struct gpio_desc *desc) {
return desc->dev != NULL;
return desc && desc->dev;}
/**
2.17.1
On 18/10/2019 22:38, Simon Glass wrote:
Hi Jean-Jacques,
On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot jjhiblot@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 jjhiblot@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()
- /**
- gpio_to_device() - Convert global GPIO number to device, number
@@ -269,11 +296,14 @@ int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc)
int dm_gpio_request(struct gpio_desc *desc, const char *label) {
struct udevice *dev = desc->dev;
struct udevice *dev; struct gpio_dev_priv *uc_priv; char *str; int ret;VALIDATE_DESC(desc);dev = desc->dev;uc_priv = dev_get_uclass_priv(dev); if (uc_priv->name[desc->offset]) return -EBUSY;@@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc, const char *func) { struct gpio_dev_priv *uc_priv;
VALIDATE_DESC(desc);if (!dm_gpio_is_valid(desc)) return -ENOENT;@@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc) int value; int ret;
VALIDATE_DESC(desc);ret = check_reserved(desc, "get_value"); if (ret) return ret;@@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value) { int ret;
VALIDATE_DESC(desc);ret = check_reserved(desc, "set_value"); if (ret) return ret;@@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
int dm_gpio_get_open_drain(struct gpio_desc *desc) {
struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);ops = gpio_get_ops(desc->dev);ret = check_reserved(desc, "get_open_drain"); if (ret) return ret;@@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
int dm_gpio_set_open_drain(struct gpio_desc *desc, int value) {
struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);ops = gpio_get_ops(desc->dev);ret = check_reserved(desc, "set_open_drain"); if (ret) return ret;@@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) {
struct udevice *dev = desc->dev;struct dm_gpio_ops *ops = gpio_get_ops(dev);
struct udevice *dev;struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);dev = desc->dev;ops = gpio_get_ops(dev);ret = check_reserved(desc, "set_dir"); if (ret) return ret;@@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc) int gpio_get_value(unsigned gpio) { int ret;
unrelated change?
struct gpio_desc desc; ret = gpio_to_device(gpio, &desc);@@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc) {
VALIDATE_DESC(desc);/* For now, we don't do any checking of dev */ return _dm_gpio_free(desc->dev, desc->offset);@@ -981,12 +1028,11 @@ static int gpio_renumber(struct udevice *removed_dev)
int gpio_get_number(const struct gpio_desc *desc) {
struct udevice *dev = desc->dev; struct gpio_dev_priv *uc_priv;if (!dev)return -1;uc_priv = dev->uclass_priv;
VALIDATE_DESC(desc);I think this is pretty opaque. How about writing the code out in full, with a helper function to do the check. The helper function can return the error code perhaps.
ret = validate_desc(desc); if (ret) return log_msg_ret("gpio_free", ret);
The helper is there already. I could remove macro usage, or maybe rename the macro as VALIDATE_DESC_OR_EXIT().
JJ
Regards, Simon
uc_priv = dev_get_uclass_priv(desc->dev); return uc_priv->gpio_base + desc->offset;}
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index d6cf18744f..ba669b65a9 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -139,7 +139,7 @@ struct gpio_desc { */ static inline bool dm_gpio_is_valid(const struct gpio_desc *desc) {
return desc->dev != NULL;
return desc && desc->dev;}
/**
-- 2.17.1
Hi Jean-Jacques,
On Mon, 21 Oct 2019 at 01:45, Jean-Jacques Hiblot jjhiblot@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 jjhiblot@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 jjhiblot@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.
- /**
- gpio_to_device() - Convert global GPIO number to device, number
@@ -269,11 +296,14 @@ int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc)
int dm_gpio_request(struct gpio_desc *desc, const char *label) {
struct udevice *dev = desc->dev;
struct udevice *dev; struct gpio_dev_priv *uc_priv; char *str; int ret;VALIDATE_DESC(desc);dev = desc->dev;uc_priv = dev_get_uclass_priv(dev); if (uc_priv->name[desc->offset]) return -EBUSY;@@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc, const char *func) { struct gpio_dev_priv *uc_priv;
VALIDATE_DESC(desc);if (!dm_gpio_is_valid(desc)) return -ENOENT;@@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc) int value; int ret;
VALIDATE_DESC(desc);ret = check_reserved(desc, "get_value"); if (ret) return ret;@@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value) { int ret;
VALIDATE_DESC(desc);ret = check_reserved(desc, "set_value"); if (ret) return ret;@@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
int dm_gpio_get_open_drain(struct gpio_desc *desc) {
struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);ops = gpio_get_ops(desc->dev);ret = check_reserved(desc, "get_open_drain"); if (ret) return ret;@@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
int dm_gpio_set_open_drain(struct gpio_desc *desc, int value) {
struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);ops = gpio_get_ops(desc->dev);ret = check_reserved(desc, "set_open_drain"); if (ret) return ret;@@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) {
struct udevice *dev = desc->dev;struct dm_gpio_ops *ops = gpio_get_ops(dev);
struct udevice *dev;struct dm_gpio_ops *ops; int ret;VALIDATE_DESC(desc);dev = desc->dev;ops = gpio_get_ops(dev);ret = check_reserved(desc, "set_dir"); if (ret) return ret;@@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc) int gpio_get_value(unsigned gpio) { int ret;
unrelated change?
struct gpio_desc desc; ret = gpio_to_device(gpio, &desc);@@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc) {
VALIDATE_DESC(desc);/* For now, we don't do any checking of dev */ return _dm_gpio_free(desc->dev, desc->offset);@@ -981,12 +1028,11 @@ static int gpio_renumber(struct udevice *removed_dev)
int gpio_get_number(const struct gpio_desc *desc) {
struct udevice *dev = desc->dev; struct gpio_dev_priv *uc_priv;if (!dev)return -1;uc_priv = dev->uclass_priv;
VALIDATE_DESC(desc);I think this is pretty opaque. How about writing the code out in full, with a helper function to do the check. The helper function can return the error code perhaps.
ret = validate_desc(desc); if (ret) return log_msg_ret("gpio_free", ret);
The helper is there already. I could remove macro usage, or maybe rename the macro as VALIDATE_DESC_OR_EXIT().
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.
Regards, Simon
On 22/10/2019 00:53, Simon Glass wrote:
Hi Jean-Jacques,
On Mon, 21 Oct 2019 at 01:45, Jean-Jacques Hiblot jjhiblot@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 jjhiblot@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 jjhiblot@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
Add managed functions to get a gpio from the devce-tree, based on a property name (minus the '-gpios' suffix) and optionally an index.
When the device is unbound, the GPIO is automatically released and the data structure is freed.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com ---
drivers/gpio/gpio-uclass.c | 68 ++++++++++++++++++++++++++++++++++++++ include/asm-generic/gpio.h | 47 ++++++++++++++++++++++++++ 2 files changed, 115 insertions(+)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 63c10f438b..6563809306 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -1077,6 +1077,74 @@ int gpio_dev_request_index(struct udevice *dev, const char *nodename, flags, 0, dev); }
+static void devm_gpiod_release(struct udevice *dev, void *res) +{ + dm_gpio_free(dev, res); +} + +static int devm_gpiod_match(struct udevice *dev, void *res, void *data) +{ + return res == data; +} + +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id, + unsigned int index, int flags) +{ + int rc; + struct gpio_desc *desc; + char *propname; + static const char suffix[] = "-gpios"; + + propname = malloc(strlen(id) + sizeof(suffix)); + if (!propname) { + rc = -ENOMEM; + goto end; + } + + strcpy(propname, id); + strcat(propname, suffix); + + desc = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc), + __GFP_ZERO); + if (unlikely(!desc)) { + rc = -ENOMEM; + goto end; + } + + rc = gpio_request_by_name(dev, propname, index, desc, flags); + +end: + if (propname) + free(propname); + + if (rc) + return ERR_PTR(rc); + + devres_add(dev, desc); + return desc; +} + +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev, + const char *id, + unsigned int index, + int flags) +{ + struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index, flags); + + if (IS_ERR(desc)) + return NULL; + + return desc; +} + +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc) +{ + int rc; + + rc = devres_release(dev, devm_gpiod_release, devm_gpiod_match, desc); + WARN_ON(rc); +} + static int gpio_post_bind(struct udevice *dev) { struct udevice *child; diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index ba669b65a9..af0122f304 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -657,4 +657,51 @@ int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags); */ int gpio_get_number(const struct gpio_desc *desc);
+/** + * devm_gpiod_get_index - Resource-managed gpiod_get() + * @dev: GPIO consumer + * @con_id: function within the GPIO consumer + * @index: index of the GPIO to obtain in the consumer + * @flags: optional GPIO initialization flags + * + * Managed gpiod_get(). GPIO descriptors returned from this function are + * automatically disposed on driver detach. + * Return the GPIO descriptor corresponding to the function con_id of device + * dev, -ENOENT if no GPIO has been assigned to the requested function, or + * another IS_ERR() code if an error occurred while trying to acquire the GPIO. + */ +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id, + unsigned int index, int flags); + +#define devm_gpiod_get(dev, id, flags) devm_gpiod_get_index(dev, id, 0, flags) +/** + * gpiod_get_optional - obtain an optional GPIO for a given GPIO function + * @dev: GPIO consumer, can be NULL for system-global GPIOs + * @con_id: function within the GPIO consumer + * @index: index of the GPIO to obtain in the consumer + * @flags: optional GPIO initialization flags + * + * This is equivalent to devm_gpiod_get(), except that when no GPIO was + * assigned to the requested function it will return NULL. This is convenient + * for drivers that need to handle optional GPIOs. + */ +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev, + const char *id, + unsigned int index, + int flags); + +#define devm_gpiod_get_optional(dev, id, flags) \ + devm_gpiod_get_index_optional(dev, id, 0, flags) + +/** + * devm_gpiod_put - Resource-managed gpiod_put() + * @dev: GPIO consumer + * @desc: GPIO descriptor to dispose of + * + * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or + * devm_gpiod_get_index(). Normally this function will not be called as the GPIO + * will be disposed of by the resource management code. + */ +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc); + #endif /* _ASM_GENERIC_GPIO_H_ */
Add a test to verify that GPIOs can be acquired/released using the managed API. Also check that the GPIOs are released when the consumer device is removed.
Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com
---
arch/sandbox/dts/test.dts | 10 ++++ test/dm/gpio.c | 102 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+)
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 27b0baab27..f9aaef1eed 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -87,10 +87,20 @@ test2-gpios = <&gpio_a 1>, <&gpio_a 4>, <&gpio_b 6 1 3 2 1>, <&gpio_b 7 2 3 2 1>, <&gpio_b 8 4 3 2 1>, <&gpio_b 9 0xc 3 2 1>; + test3-gpios = <&gpio_a 14>, <&gpio_b 4 1 3 2 1>; + test4-gpios = <&gpio_a 19>; + int-value = <1234>; uint-value = <(-1234)>; };
+ another-test { + reg = <0 2>; + compatible = "denx,u-boot-fdt-test"; + test3-gpios = <&gpio_a 14>, <&gpio_b 4 1 3 2 1>; + test4-gpios = <&gpio_a 19>; + }; + junk { reg = <1 1>; compatible = "not,compatible"; diff --git a/test/dm/gpio.c b/test/dm/gpio.c index bb4b20cea9..10653bcb41 100644 --- a/test/dm/gpio.c +++ b/test/dm/gpio.c @@ -6,6 +6,7 @@ #include <common.h> #include <fdtdec.h> #include <dm.h> +#include <dm/device-internal.h> #include <dm/root.h> #include <dm/test.h> #include <dm/util.h> @@ -247,3 +248,104 @@ static int dm_test_gpio_phandles(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_gpio_phandles, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); + +/* Test that we can get/release GPIOs using managed API */ +static int dm_test_gpio_devm(struct unit_test_state *uts) +{ + static const u32 flags = GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE; + struct gpio_desc *desc1, *desc2, *desc3, *desc_err; + struct udevice *dev; + struct udevice *dev2; + + ut_assertok(uclass_get_device_by_name(UCLASS_TEST_FDT, "a-test", + &dev)); + ut_assertok(uclass_get_device_by_name(UCLASS_TEST_FDT, "another-test", + &dev2)); + + /* Get 3 GPIOs from 'a-test' dev */ + desc1 = devm_gpiod_get_index(dev, "test3", 0, flags); + ut_assert(!IS_ERR(desc1)); + desc2 = devm_gpiod_get_index(dev, "test3", 1, flags); + ut_assert(!IS_ERR(desc2)); + desc3 = devm_gpiod_get_index_optional(dev, "test4", 0, flags); + ut_assert(!IS_ERR(desc3)); + ut_assert(desc3); + + /* + * Try get the same 3 GPIOs from 'a-test' and 'another-test' devices. + * check that it fails + */ + desc_err = devm_gpiod_get_index(dev, "test3", 0, flags); + ut_asserteq(-EBUSY, PTR_ERR(desc_err)); + desc_err = devm_gpiod_get_index(dev2, "test3", 0, flags); + ut_asserteq(-EBUSY, PTR_ERR(desc_err)); + desc_err = devm_gpiod_get_index(dev, "test3", 1, flags); + ut_asserteq(-EBUSY, PTR_ERR(desc_err)); + desc_err = devm_gpiod_get_index(dev2, "test3", 1, flags); + ut_asserteq(-EBUSY, PTR_ERR(desc_err)); + desc_err = devm_gpiod_get_index_optional(dev, "test4", 0, flags); + ut_asserteq_ptr(NULL, desc_err); + desc_err = devm_gpiod_get_index_optional(dev2, "test4", 0, flags); + ut_asserteq_ptr(NULL, desc_err); + + /* Try get GPIOs outside of the list */ + desc_err = devm_gpiod_get_index(dev, "test3", 2, flags); + ut_assert(IS_ERR(desc_err)); + desc_err = devm_gpiod_get_index_optional(dev, "test4", 1, flags); + ut_asserteq_ptr(NULL, desc_err); + + /* Manipulate the GPIOs */ + ut_assertok(dm_gpio_set_value(desc1, 1)); + ut_asserteq(1, dm_gpio_get_value(desc1)); + ut_assertok(dm_gpio_set_value(desc1, 0)); + ut_asserteq(0, dm_gpio_get_value(desc1)); + + ut_assertok(dm_gpio_set_value(desc2, 1)); + ut_asserteq(1, dm_gpio_get_value(desc2)); + ut_assertok(dm_gpio_set_value(desc2, 0)); + ut_asserteq(0, dm_gpio_get_value(desc2)); + + ut_assertok(dm_gpio_set_value(desc3, 1)); + ut_asserteq(1, dm_gpio_get_value(desc3)); + ut_assertok(dm_gpio_set_value(desc3, 0)); + ut_asserteq(0, dm_gpio_get_value(desc3)); + + /* Check that the GPIO cannot be owned by more than one device */ + desc_err = devm_gpiod_get_index(dev2, "test3", 0, flags); + ut_asserteq(-EBUSY, PTR_ERR(desc_err)); + desc_err = devm_gpiod_get_index(dev2, "test3", 1, flags); + ut_asserteq(-EBUSY, PTR_ERR(desc_err)); + desc_err = devm_gpiod_get_index_optional(dev2, "test4", 0, flags); + ut_asserteq_ptr(NULL, desc_err); + + /* + * Release one GPIO and check that we can get it back using + * 'another-test' and then 'a-test' + */ + devm_gpiod_put(dev, desc2); + desc2 = devm_gpiod_get_index(dev2, "test3", 1, flags); + ut_assert(!IS_ERR(desc2)); + + devm_gpiod_put(dev2, desc2); + desc2 = devm_gpiod_get_index(dev, "test3", 1, flags); + ut_assert(!IS_ERR(desc2)); + + /* Release one GPIO before removing the 'a-test' dev. */ + devm_gpiod_put(dev, desc2); + device_remove(dev, DM_REMOVE_NORMAL); + + /* All the GPIOs must have been freed. We should be able to claim + * them with the 'another-test' device. + */ + desc1 = devm_gpiod_get_index(dev2, "test3", 0, flags); + ut_assert(!IS_ERR(desc1)); + desc2 = devm_gpiod_get_index(dev2, "test3", 1, flags); + ut_assert(!IS_ERR(desc2)); + desc3 = devm_gpiod_get_index_optional(dev2, "test4", 0, flags); + ut_assert(!IS_ERR(desc3)); + ut_assert(desc3); + + device_remove(dev2, DM_REMOVE_NORMAL); + return 0; +} +DM_TEST(dm_test_gpio_devm, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
participants (2)
-
Jean-Jacques Hiblot -
Simon Glass