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

Patrick DELAUNAY patrick.delaunay at st.com
Thu Jan 9 11:19:11 CET 2020


Hi Simon

> From: Simon Glass <sjg at chromium.org>
> Sent: lundi 30 décembre 2019 02:21
> 
> 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?

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 flags
> 0x%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


More information about the U-Boot mailing list