[PATCH v2 08/14] gpio: add ops for configuration with dir flags
Simon Glass
sjg at chromium.org
Mon Dec 30 02:21:15 CET 2019
Hi Patrick,
On Tue, 26 Nov 2019 at 01:49, Patrick Delaunay <patrick.delaunay at 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 at 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 at 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;
> + else
> + desc->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?
> +
> + 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 flags 0x%lx\n",
> + desc->dev->name,
> + uc_priv->bank_name ? uc_priv->bank_name : "",
> + desc->offset, flags);
log_debug()
Also should return error
>
> - if (flags & GPIOD_IS_OUT) {
> - int value = flags & GPIOD_IS_OUT_ACTIVE ? 1 : 0;
> + return ret;
> + }
>
> - 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 *
> + 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/ ?
> #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
>
> 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.
> /**
> * 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.
Regards,
Simon
More information about the U-Boot
mailing list