[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