Hi Simon
From: Simon Glass sjg@chromium.org Sent: lundi 30 décembre 2019 02:21
Hi Patrick,
On Tue, 26 Nov 2019 at 01:49, Patrick Delaunay patrick.delaunay@st.com wrote:
This commit manages the dir flags that can be used in gpio specifiers to indicate if a pull-up resistor or pull-down resistor should be enabled for output gpio (GPIO_PULL_UP, GPIO_PULL_DOWN) and the Open Drain/Open Source configuration for input gpio (GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE).
These flags are already supported in Linux kernel in gpiolib; this patch provides the same support in U-Boot.
The dir flags are managed in gpio drivers with two optional ops in gpio uclass: set_dir_flags and get_dir_flags.
ops set_dir_flags() set the direction and configuration of each GPIO with a only call to dm_gpio_set_dir_flags() / dm_gpio_set_dir() and respecting the configuration provided by device tree (saved in desc->flags).
ops get_dir_flags() return dynamically the current gpio configuration, it is used by the new API dm_gpio_get_dir_flags().
When these optional ops are absent, the gpio uclass use the mandatory ops (direction_output, direction_input, get_value) and desc->flags to manage only the main dir flags:
- GPIOD_IS_IN
- GPIOD_IS_OUT
- GPIOD_IS_OUT_ACTIVE
- GPIOD_ACTIVE_LOW
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Changes in v2:
- change the proposed ops for pin config to
set_dir_flags/get_dir_flags
- reused the existing API dm_gpio_set_dir_flags/dm_gpio_set_dir
- add a new API dm_gpio_get_dir_flags
drivers/gpio/gpio-uclass.c | 157 +++++++++++++++++++++++++++++++------ include/asm-generic/gpio.h | 65 +++++++++++++-- 2 files changed, 192 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 0870458e96..241293f4b4 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -140,8 +140,27 @@ int gpio_xlate_offs_flags(struct udevice *dev, struct
gpio_desc *desc,
if (args->args_count < 2) return 0;
desc->flags = 0; if (args->args[1] & GPIO_ACTIVE_LOW)
desc->flags = GPIOD_ACTIVE_LOW;
desc->flags |= GPIOD_ACTIVE_LOW;/** need to test 2 bits for gpio output binding:* OPEN_DRAIN (0x6) = SINGLE_ENDED (0x2) | LINE_OPEN_DRAIN(0x4)
* OPEN_SOURCE (0x2) = SINGLE_ENDED (0x2) |LINE_OPEN_SOURCE (0x0)
*/if (args->args[1] & GPIO_SINGLE_ENDED) {if (args->args[1] & GPIO_LINE_OPEN_DRAIN)desc->flags |= GPIOD_OPEN_DRAIN;elsedesc->flags |= GPIOD_OPEN_SOURCE;}if (args->args[1] & GPIO_PULL_UP)desc->flags |= GPIOD_PULL_UP;if (args->args[1] & GPIO_PULL_DOWN)desc->flags |= GPIOD_PULL_DOWN; return 0;} @@ -476,18 +495,24 @@ int gpio_direction_output(unsigned gpio, int value) desc.offset, value); }
-int dm_gpio_get_value(const struct gpio_desc *desc) +static int _gpio_get_value(const struct gpio_desc *desc) { int value;
value = gpio_get_ops(desc->dev)->get_value(desc->dev,- desc->offset);
return desc->flags & GPIOD_ACTIVE_LOW ? !value : value; }+int dm_gpio_get_value(const struct gpio_desc *desc) { int ret;
ret = check_reserved(desc, "get_value"); if (ret) return ret;
value = gpio_get_ops(desc->dev)->get_value(desc->dev, desc->offset);return desc->flags & GPIOD_ACTIVE_LOW ? !value : value;
return _gpio_get_value(desc);}
int dm_gpio_set_value(const struct gpio_desc *desc, int value) @@ -504,39 +529,119 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int
value)
return 0;}
-int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) +/* check dir flags invalid configuration */ static int +check_dir_flags(ulong flags) {
if ((flags & GPIOD_IS_OUT) && (flags & GPIOD_IS_IN))return -EINVAL;if ((flags & GPIOD_PULL_UP) && (flags & GPIOD_PULL_DOWN))return -EINVAL;if ((flags & GPIOD_OPEN_DRAIN) && (flags &GPIOD_OPEN_SOURCE))
return -EINVAL;Can we use different error numbers here, so that people can figure out what is going on?
It is really a basic error that should be never occur, I think that EINVAL is the correct return value here (it is difficult to have 3 different errno)
But I will add a debug trace before each return...
return 0;+}
+static 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 gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); int ret;
ret = check_reserved(desc, "set_dir");if (ret)return ret;
ret = check_dir_flags(flags);if (ret) {dev_err(dev,"%s error: set_dir_flags for gpio %s%d has invalid dir flags0x%lx\n",
desc->dev->name,uc_priv->bank_name ? uc_priv->bank_name : "",desc->offset, flags);log_debug()
Ok change to dev_dbg log_debug is preferered to dev_dbg ?
for me dev_.... functions are preferred when 'dev' is available.
Also should return error
if (flags & GPIOD_IS_OUT) {int value = flags & GPIOD_IS_OUT_ACTIVE ? 1 : 0;
return ret;}
In fact return is inserted here (test moved in check_dir_flags)
The final code become :
ret = check_dir_flags(flags); if (ret) { dev_dbg(dev, "%s error: set_dir_flags for gpio %s%d has invalid dir flags 0x%lx\n", desc->dev->name, uc_priv->bank_name ? uc_priv->bank_name : "", desc->offset, flags);
return ret; }
/* GPIOD_ are directly managed by driver in set_dir_flags*/
if (flags & GPIOD_ACTIVE_LOW)value = !value;ret = ops->direction_output(dev, desc->offset, value);} else if (flags & GPIOD_IS_IN) {ret = ops->direction_input(dev, desc->offset);
/* GPIOD_ are directly managed by driver in set_dir_flags*/if (ops->set_dir_flags) {ret = ops->set_dir_flags(dev, desc->offset, flags);} else {if (flags & GPIOD_IS_OUT) {ret = ops->direction_output(dev, desc->offset,GPIOD_FLAGS_OUTPUT(flags));} else if (flags & GPIOD_IS_IN) {ret = ops->direction_input(dev, desc->offset);} }return ret;+}
+int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) {
int ret;ret = check_reserved(desc, "set_dir_flags"); if (ret) return ret;
/** Update desc->flags here, so that GPIO_ACTIVE_LOW is honoured in* futures*/desc->flags = flags;return 0;
/* combine the requested flags (for IN/OUT) and the descriptor flags */flags |= desc->flags;ret = _dm_gpio_set_dir_flags(desc, flags);/* update the descriptor flags */if (ret)desc->flags = flags;return ret;}
int dm_gpio_set_dir(struct gpio_desc *desc) {
return dm_gpio_set_dir_flags(desc, desc->flags);
int ret;ret = check_reserved(desc, "set_dir");if (ret)return ret;return _dm_gpio_set_dir_flags(desc, desc->flags); }+int dm_gpio_get_dir_flags(struct gpio_desc *desc, ulong *flags) {
struct udevice *dev = desc->dev;struct dm_gpio_ops *ops = gpio_get_ops(dev);int ret, value;ulong dir_flags;ret = check_reserved(desc, "get_dir_flags");if (ret)return ret;/* GPIOD_ are directly provided by driver except- GPIOD_ACTIVE_LOW*/
Space before *
ok
if (ops->get_dir_flags) {ret = ops->get_dir_flags(dev, desc->offset, &dir_flags);if (ret)return ret;/* GPIOD_ACTIVE_LOW is saved in desc->flags */value = dir_flags & GPIOD_IS_OUT_ACTIVE ? 1 : 0;if (desc->flags & GPIOD_ACTIVE_LOW)value = !value;dir_flags &= ~(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE);dir_flags |= (desc->flags & GPIOD_ACTIVE_LOW);if (value)dir_flags |= GPIOD_IS_OUT_ACTIVE;} else {dir_flags = desc->flags;/* only GPIOD_IS_OUT_ACTIVE is provided by uclass */dir_flags &= ~GPIOD_IS_OUT_ACTIVE;if ((desc->flags & GPIOD_IS_OUT) && _gpio_get_value(desc))dir_flags |= GPIOD_IS_OUT_ACTIVE;}*flags = dir_flags;return 0;}
/** @@ -809,7 +914,7 @@ static int gpio_request_tail(int ret, const char
*nodename,
debug("%s: dm_gpio_requestf failed\n", __func__); goto err; }
ret = dm_gpio_set_dir_flags(desc, flags | desc->flags);
ret = dm_gpio_set_dir_flags(desc, flags); if (ret) { debug("%s: dm_gpio_set_dir failed\n", __func__); goto err;@@ -1036,6 +1141,10 @@ static int gpio_post_bind(struct udevice *dev) ops->get_function += gd->reloc_off; if (ops->xlate) ops->xlate += gd->reloc_off;
if (ops->set_dir_flags)ops->set_dir_flags += gd->reloc_off;if (ops->get_dir_flags)ops->get_dir_flags += gd->reloc_off; reloc_done++; }diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 454578c8d2..c6991be1c9 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -117,10 +117,15 @@ struct udevice; struct gpio_desc { struct udevice *dev; /* Device, NULL for invalid GPIO */ unsigned long flags;
#define GPIOD_IS_OUT BIT(1) /* GPIO is an output */ #define GPIOD_IS_IN BIT(2) /* GPIO is an input */ #define GPIOD_ACTIVE_LOW BIT(3) /* value has active low */
s/has/is/ ?
GPIO is active when value is low
#define GPIOD_IS_OUT_ACTIVE BIT(4) /* set output active */ +#define GPIOD_OPEN_DRAIN BIT(5) /* GPIO is open drain type */ +#define GPIOD_OPEN_SOURCE BIT(6) /* GPIO is open source type */ +#define GPIOD_PULL_UP BIT(7) /* GPIO has pull up enabled */ +#define GPIOD_PULL_DOWN BIT(8) /* GPIO has pull down enabled
*/
pull-down or pulldown
Ok => pull-down
uint offset; /* GPIO offset within the device */ /*@@ -129,6 +134,12 @@ struct gpio_desc { */ };
+/* helper to compute the value of the gpio output */ #define +GPIOD_FLAGS_OUTPUT_MASK (GPIOD_ACTIVE_LOW |
GPIOD_IS_OUT_ACTIVE)
+#define GPIOD_FLAGS_OUTPUT(flags) \
(((((flags) & GPIOD_FLAGS_OUTPUT_MASK) ==GPIOD_IS_OUT_ACTIVE) || \
(((flags) & GPIOD_FLAGS_OUTPUT_MASK) ==GPIOD_ACTIVE_LOW)) ?
+1 : 0)
You can drop the ? 1 : 0 since boolean expressions do that anyway.
Yes I drop it ....
/**
- dm_gpio_is_valid() - Check if a GPIO is valid
@@ -253,6 +264,7 @@ struct dm_gpio_ops { int value); int (*get_value)(struct udevice *dev, unsigned offset); int (*set_value)(struct udevice *dev, unsigned offset, int value);
/** * get_function() Get the GPIO function *@@ -287,6 +299,37 @@ struct dm_gpio_ops { */ int (*xlate)(struct udevice *dev, struct gpio_desc *desc, struct ofnode_phandle_args *args);
/*** set_dir_flags() - Set GPIO dir flags** This function should set up the GPIO configuration according to the* information provide by the direction flags bitfield.** This method is optional.** @dev: GPIO device* @offset: GPIO offset within that device* @flags: GPIO configuration to use* @return 0 if OK, -ve on error*/int (*set_dir_flags)(struct udevice *dev, unsigned int offset,ulong flags);/*** get_dir_flags() - Get GPIO dir flags** This function return the GPIO direction flags used.** This method is optional.** @dev: GPIO device* @offset: GPIO offset within that device* @flags: place to put the used direction flags by GPIO* @return 0 if OK, -ve on error*/int (*get_dir_flags)(struct udevice *dev, unsigned int offset,ulong *flags);};
/** @@ -586,8 +629,7 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value); /**
- dm_gpio_set_dir() - Set the direction for a GPIO
- This sets up the direction according tot the provided flags. It
will do
- nothing unless the direction is actually specified.
- This sets up the direction according to the GPIO flags: desc->flags.
- @desc: GPIO description containing device, offset and flags,
previously returned by gpio_request_by_name()@@ -596,11 +638,10 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value); int dm_gpio_set_dir(struct gpio_desc *desc);
/**
- dm_gpio_set_dir_flags() - Set direction using specific flags
- dm_gpio_set_dir_flags() - Set direction using add flags
- This is like dm_gpio_set_dir() except that the flags value is
provided
- instead of being used from desc->flags. This is needed because in
many
- cases the GPIO description does not include direction information.
- This sets up the direction according tot the provided flags and
- the GPIO
- description (desc->flags) which include direction information.
- Note that desc->flags is updated by this function.
- @desc: GPIO description containing device, offset and flags,
@@ -610,6 +651,18 @@ int dm_gpio_set_dir(struct gpio_desc *desc); */ int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags);
+/**
- dm_gpio_get_dir_flags() - Get direction flags
- read the current direction flags
- @desc: GPIO description containing device, offset and flags,
previously returned by gpio_request_by_name()
- @flags: place to put the used flags
- @return 0 if OK, -ve on error, in which case desc->flags is not
+updated */ int dm_gpio_get_dir_flags(struct gpio_desc *desc, ulong +*flags);
/**
- gpio_get_number() - Get the global GPIO number of a GPIO
-- 2.17.1
There's a lot going on in this patch.
Yes, I can split the work to help the review
1/ prepare 2/ add get_dir_flags 3/ add set_dir_flags
Regards, Simon
Regards Patrick