Hi Simon,
On 1/15/21 3:04 PM, Simon Glass wrote:
It is convenient to be able to adjust some of the flags for a GPIO while leaving others alone. Add a function for this.
Update dm_gpio_set_dir_flags() to make use of this.
Also update dm_gpio_set_value() to use this also, since this allows the open-drain / open-source features to be implemented directly in the driver, rather than using the uclass workaround.
Update the sandbox tests accordingly. This involves a lot of changes to dm_test_gpio_opendrain_opensource() since we no-longer have the direciion being reported differently depending on the open drain/open source flags.
Signed-off-by: Simon Glass sjg@chromium.org
drivers/gpio/gpio-uclass.c | 65 ++++++++++++++----- include/asm-generic/gpio.h | 25 ++++++++ test/dm/gpio.c | 125 ++++++++++++++++++++++++++++++++----- 3 files changed, 184 insertions(+), 31 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index aa0e3852122..05627ecdc30 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -568,6 +568,7 @@ int dm_gpio_get_value(const struct gpio_desc *desc)
int dm_gpio_set_value(const struct gpio_desc *desc, int value) {
const struct dm_gpio_ops *ops; int ret;
ret = check_reserved(desc, "set_value");
@@ -577,21 +578,33 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value) if (desc->flags & GPIOD_ACTIVE_LOW) value = !value;
- /* GPIOD_ are directly managed by driver in update_flags */
- ops = gpio_get_ops(desc->dev);
- if (ops->update_flags) {
ulong flags = desc->flags;if (value)flags |= GPIOD_IS_OUT_ACTIVE;elseflags &= ~GPIOD_IS_OUT_ACTIVE;return ops->update_flags(desc->dev, desc->offset, flags);- }
- /*
*/ if ((desc->flags & GPIOD_OPEN_DRAIN && value) || (desc->flags & GPIOD_OPEN_SOURCE && !value))
- Emulate open drain by not actively driving the line high or
- Emulate open source by not actively driving the line low
return gpio_get_ops(desc->dev)->direction_input(desc->dev,desc->offset);
return ops->direction_input(desc->dev, desc->offset);
return gpio_get_ops(desc->dev)->direction_output(desc->dev,desc->offset,value);
return ops->direction_output(desc->dev, desc->offset, value);- ret = ops->set_value(desc->dev, desc->offset, value);
- if (ret)
return ret;
- gpio_get_ops(desc->dev)->set_value(desc->dev, desc->offset, value); return 0; }
@@ -619,6 +632,17 @@ static int check_dir_flags(ulong flags) return 0; }
+/**
- _dm_gpio_update_flags() - Send flags to the driver
- This uses the best available method to send the given flags to the driver.
- Note that if flags & GPIOD_ACTIVE_LOW, the driver sees the opposite value
- of GPIOD_IS_OUT_ACTIVE.
- @desc: GPIO description
- @flags: flags value to set
- @return 0 if OK, -ve on error
- */ static int _dm_gpio_update_flags(struct gpio_desc *desc, ulong flags) { struct udevice *dev = desc->dev;
@@ -637,6 +661,11 @@ static int _dm_gpio_update_flags(struct gpio_desc *desc, ulong flags) return ret; }
- /* If active low, invert the output state */
- if ((flags & (GPIOD_IS_OUT | GPIOD_ACTIVE_LOW)) ==
(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW))flags ^= GPIOD_IS_OUT_ACTIVE;
This modifciation imply that GPIOD_ACTIVE_LOW must no more managed in ops update_flags
(as indicated previously in the serie in ops description)
/* GPIOD_ are directly managed by driver in update_flags */ if (ops->update_flags) { ret = ops->update_flags(dev, desc->offset, flags); @@ -649,26 +678,34 @@ static int _dm_gpio_update_flags(struct gpio_desc *desc, ulong flags) } }
- /* save the flags also in descriptor */
- if (!ret)
desc->flags = flags;- return ret; }
(...)
In current STM32 implementation, the set_dir_flags could be called with GPIOD_ACTIVE_LOW, so it was managed in ops set_dir_flags()
it was hndle by using the macro GPIOD_FLAGS_OUTPUT.
As after the serie the GPIOD_ACTIVE_LOW must no more mamaged in driver: I agree that is is more clean / simple.
But this patch need also modification in driver using GPIOD_FLAGS_OUTPUT
drivers/gpio/stm32_gpio.c:208: int value = GPIOD_FLAGS_OUTPUT(flags);
drivers/gpio/gpio-uclass.c:680: GPIOD_FLAGS_OUTPUT(flags));
I think GPIOD_FLAGS_OUTPUT could be remove, as it is only used in
drivers/gpio/stm32_gpio.c::stm32_gpio_update_flags()
Something as:
-------------------------- drivers/gpio/gpio-uclass.c -------------------------- index b240ddd3d9..45fe791431 100644 @@ -654,6 +654,7 @@ static int _dm_gpio_update_flags(struct gpio_desc *desc, ulong flags) const struct dm_gpio_ops *ops = gpio_get_ops(dev); struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); int ret = 0; + int value;
ret = check_dir_flags(flags); if (ret) { @@ -676,8 +677,8 @@ static int _dm_gpio_update_flags(struct gpio_desc *desc, ulong flags) ret = ops->update_flags(dev, desc->offset, flags); } else { if (flags & GPIOD_IS_OUT) { - ret = ops->direction_output(dev, desc->offset, - GPIOD_FLAGS_OUTPUT(flags)); + value = !!(flags & GPIOD_IS_OUT_ACTIVE); + ret = ops->direction_output(dev, desc->offset, value); } else if (flags & GPIOD_IS_IN) { ret = ops->direction_input(dev, desc->offset); }
-------------------------- drivers/gpio/stm32_gpio.c -------------------------- index da9a40ebf8..d9ad50f3c9 100644 @@ -205,12 +205,13 @@ static int stm32_gpio_update_flags(struct udevice *dev, unsigned int offset, printf("%s: %s %d flags = %lx\n", __func__, dev->name, idx, flags);
if (flags & GPIOD_IS_OUT) { - int value = GPIOD_FLAGS_OUTPUT(flags); + int value = !!(flags & GPIOD_IS_OUT_ACTIVE);
if (flags & GPIOD_OPEN_DRAIN) stm32_gpio_set_otype(regs, idx, STM32_GPIO_OTYPE_OD); else stm32_gpio_set_otype(regs, idx, STM32_GPIO_OTYPE_PP); + stm32_gpio_set_moder(regs, idx, STM32_GPIO_MODE_OUT); writel(BSRR_BIT(idx, value), ®s->bsrr); printf("%s: %s %d moder = %x value = %d\n", @@ -304,7 +305,7 @@ static int gpio_stm32_probe(struct udevice *dev) ret = dev_read_phandle_with_args(dev, "gpio-ranges", NULL, 3, i, &args);
- dev_dbg(dev, "dev_read_phandle_with_args = %d %d %d\n", ret, -ENOENT, dev_read_u32_default(dev, "ngpios", 0)); + dev_dbg(dev, "dev_read_phandle_with_args(%d) = %d %d %d\n", i, ret, -ENOENT, dev_read_u32_default(dev, "ngpios", 0));
if (!ret && args.args_count < 3) return -EINVAL; @@ -322,6 +323,8 @@ static int gpio_stm32_probe(struct udevice *dev)
ret = dev_read_phandle_with_args(dev, "gpio-ranges", NULL, 3, ++i, &args); + dev_dbg(dev, "dev_read_phandle_with_args(%d) = %d %d %d\n", i-1, ret, -ENOENT, dev_read_u32_default(dev, "ngpios", 0)); + if (!ret && args.args_count < 3) return -EINVAL; }
----------------------- drivers/pinctrl/pinctrl-stmfx.c ----------------------- index 6477febbaa..e2c95d8d42 100644 @@ -169,6 +169,8 @@ static int stmfx_gpio_update_flags(struct udevice *dev, unsigned int offset, int ret = -ENOTSUPP;
if (flags & GPIOD_IS_OUT) { + int value = !!(flags & GPIOD_IS_OUT_ACTIVE); + if (flags & GPIOD_OPEN_SOURCE) return -ENOTSUPP; if (flags & GPIOD_OPEN_DRAIN) @@ -177,8 +179,7 @@ static int stmfx_gpio_update_flags(struct udevice *dev, unsigned int offset, ret = stmfx_conf_set_type(dev, offset, 1); if (ret) return ret; - ret = stmfx_gpio_direction_output(dev, offset, - GPIOD_FLAGS_OUTPUT(flags)); + ret = stmfx_gpio_direction_output(dev, offset, value); } else if (flags & GPIOD_IS_IN) { ret = stmfx_gpio_direction_input(dev, offset); if (ret)
-------------------------- include/asm-generic/gpio.h -------------------------- index 1cf81a3fc7..62514db5be 100644 @@ -141,12 +141,6 @@ 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))) - /** * dm_gpio_is_valid() - Check if a GPIO is valid *
Regards
Patrick