Re: [PATCH v2 08/14] gpio: add ops for configuration with dir flags

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;
  

•           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