[PATCH v3 10/16] dm: gpio: Add a way to update flags

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.

Also update the STM32 drivers to let the uclass handle the active low/high logic.

Drop the GPIOD_FLAGS_OUTPUT() macro which is no-longer used.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v3: - Incorporate GPIOD_FLAGS_OUTPUT() changes from Patrick Delaunay

drivers/gpio/gpio-uclass.c | 70 +++++++++++++---- drivers/gpio/stm32_gpio.c | 3 +- drivers/pinctrl/pinctrl-stmfx.c | 5 +- include/asm-generic/gpio.h | 31 ++++++-- test/dm/gpio.c | 132 ++++++++++++++++++++++++++++---- 5 files changed, 199 insertions(+), 42 deletions(-)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 8931c420845..b74976ed951 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 set_flags */ + ops = gpio_get_ops(desc->dev); + if (ops->set_flags) { + ulong flags = desc->flags; + + if (value) + flags |= GPIOD_IS_OUT_ACTIVE; + else + flags &= ~GPIOD_IS_OUT_ACTIVE; + return ops->set_flags(desc->dev, desc->offset, flags); + } + /* * Emulate open drain by not actively driving the line high or * Emulate open source by not actively driving the line low */ if ((desc->flags & GPIOD_OPEN_DRAIN && value) || (desc->flags & GPIOD_OPEN_SOURCE && !value)) - return gpio_get_ops(desc->dev)->direction_input(desc->dev, - desc->offset); + return ops->direction_input(desc->dev, desc->offset); else if (desc->flags & GPIOD_OPEN_DRAIN || desc->flags & GPIOD_OPEN_SOURCE) - 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_set_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_set_flags(struct gpio_desc *desc, ulong flags) { struct udevice *dev = desc->dev; @@ -637,38 +661,52 @@ static int _dm_gpio_set_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; + /* GPIOD_ are directly managed by driver in set_flags */ if (ops->set_flags) { ret = ops->set_flags(dev, desc->offset, flags); } else { if (flags & GPIOD_IS_OUT) { - ret = ops->direction_output(dev, desc->offset, - GPIOD_FLAGS_OUTPUT(flags)); + bool 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); } }

- /* save the flags also in descriptor */ - if (!ret) - desc->flags = flags; - return ret; }

-int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) +int dm_gpio_clrset_flags(struct gpio_desc *desc, ulong clr, ulong set) { + ulong flags; int ret;

ret = check_reserved(desc, "set_dir_flags"); if (ret) return ret;

- /* combine the requested flags (for IN/OUT) and the descriptor flags */ - flags |= desc->flags; + flags = (desc->flags & ~clr) | set; + ret = _dm_gpio_set_flags(desc, flags); + if (ret) + return ret;

- return ret; + /* save the flags also in descriptor */ + desc->flags = flags; + + return 0; +} + +int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) +{ + /* combine the requested flags (for IN/OUT) and the descriptor flags */ + return dm_gpio_clrset_flags(desc, 0, flags); }

int dm_gpio_get_flags(struct gpio_desc *desc, ulong *flagsp) diff --git a/drivers/gpio/stm32_gpio.c b/drivers/gpio/stm32_gpio.c index c2d7046c0dd..125c237551c 100644 --- a/drivers/gpio/stm32_gpio.c +++ b/drivers/gpio/stm32_gpio.c @@ -203,12 +203,13 @@ static int stm32_gpio_set_flags(struct udevice *dev, unsigned int offset, return idx;

if (flags & GPIOD_IS_OUT) { - int value = GPIOD_FLAGS_OUTPUT(flags); + bool 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), &regs->bsrr);

diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c index 8ddbc3dc281..711b1a5d8c4 100644 --- a/drivers/pinctrl/pinctrl-stmfx.c +++ b/drivers/pinctrl/pinctrl-stmfx.c @@ -169,6 +169,8 @@ static int stmfx_gpio_set_flags(struct udevice *dev, unsigned int offset, int ret = -ENOTSUPP;

if (flags & GPIOD_IS_OUT) { + bool 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_set_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) diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index f7f10c469c0..af3ce21a7cf 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -128,6 +128,12 @@ struct gpio_desc { #define GPIOD_PULL_UP BIT(7) /* GPIO has pull-up enabled */ #define GPIOD_PULL_DOWN BIT(8) /* GPIO has pull-down enabled */

+/* Flags for updating the above */ +#define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \ + GPIOD_IS_OUT_ACTIVE) +#define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE) +#define GPIOD_MASK_PULL (GPIOD_PULL_UP | GPIOD_PULL_DOWN) + uint offset; /* GPIO offset within the device */ /* * We could consider adding the GPIO label in here. Possibly we could @@ -135,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 * @@ -657,6 +657,25 @@ int dm_gpio_get_value(const struct gpio_desc *desc);

int dm_gpio_set_value(const struct gpio_desc *desc, int value);

+/** + * dm_gpio_clrset_flags() - Update flags + * + * This updates the flags as directled. Note that desc->flags is updated by this + * function on success. If any changes cannot be made, best efforts are made. + * + * By use of @clr and @set any of flags can be individually updated, or left + * alone + * + * @desc: GPIO description containing device, offset and flags, + * previously returned by gpio_request_by_name() + * @clr: Flags to clear (GPIOD_...) + * @set: Flags to set (GPIOD_...) + * @return 0 if OK, -EINVAL if the flags had obvious conflicts, + * -ERECALLCONFLICT if there was a non-obvious hardware conflict when attempting + * to set the flags + */ +int dm_gpio_clrset_flags(struct gpio_desc *desc, ulong clr, ulong set); + /** * dm_gpio_set_dir_flags() - Set direction using description and added flags * diff --git a/test/dm/gpio.c b/test/dm/gpio.c index dfbb634bf71..be5da953045 100644 --- a/test/dm/gpio.c +++ b/test/dm/gpio.c @@ -178,15 +178,15 @@ static int dm_test_gpio_opendrain_opensource(struct unit_test_state *uts) ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_DRAIN, sandbox_gpio_get_flags(gpio_c, 0));

- /* Set it as output high, should become an input */ + /* Set it as output high */ ut_assertok(dm_gpio_set_value(&desc_list[0], 1)); - ut_assertok(gpio_get_status(gpio_c, 0, buf, sizeof(buf))); - ut_asserteq_str("c0: input: 0 [x] a-test.test3-gpios0", buf); + ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_DRAIN | GPIOD_IS_OUT_ACTIVE, + sandbox_gpio_get_flags(gpio_c, 0));

- /* Set it as output low, should become output low */ + /* Set it as output low */ ut_assertok(dm_gpio_set_value(&desc_list[0], 0)); - ut_assertok(gpio_get_status(gpio_c, 0, buf, sizeof(buf))); - ut_asserteq_str("c0: output: 0 [x] a-test.test3-gpios0", buf); + ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_DRAIN, + sandbox_gpio_get_flags(gpio_c, 0));

/* GPIO 1 is (GPIO_OUT|GPIO_OPEN_SOURCE) */ ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, @@ -197,13 +197,21 @@ static int dm_test_gpio_opendrain_opensource(struct unit_test_state *uts) ut_assertok(gpio_get_status(gpio_c, 1, buf, sizeof(buf))); ut_asserteq_str("c1: output: 1 [x] a-test.test3-gpios1", buf);

- /* Set it as output low, should become an input */ + /* Set it as output low */ ut_assertok(dm_gpio_set_value(&desc_list[1], 0)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, + sandbox_gpio_get_flags(gpio_c, 1)); + ut_assertok(gpio_get_status(gpio_c, 1, buf, sizeof(buf))); - ut_asserteq_str("c1: input: 1 [x] a-test.test3-gpios1", buf); + ut_asserteq_str("c1: output: 0 [x] a-test.test3-gpios1", buf);

- /* GPIO 6 is (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_DRAIN) */ - ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_DRAIN, + /* + * GPIO 6 is (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_DRAIN). Looking at it + * directlt from the driver, we get GPIOD_IS_OUT_ACTIVE also, since it + * is active low + */ + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_DRAIN | + GPIOD_IS_OUT_ACTIVE, sandbox_gpio_get_flags(gpio_c, 6));

/* Set it as output high, should become output low */ @@ -211,19 +219,21 @@ static int dm_test_gpio_opendrain_opensource(struct unit_test_state *uts) ut_assertok(gpio_get_status(gpio_c, 6, buf, sizeof(buf))); ut_asserteq_str("c6: output: 0 [x] a-test.test3-gpios6", buf);

- /* Set it as output low, should become an input */ + /* Set it as output low */ ut_assertok(dm_gpio_set_value(&desc_list[6], 0)); - ut_assertok(gpio_get_status(gpio_c, 6, buf, sizeof(buf))); - ut_asserteq_str("c6: input: 0 [x] a-test.test3-gpios6", buf); + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_DRAIN | + GPIOD_IS_OUT_ACTIVE, + sandbox_gpio_get_flags(gpio_c, 6));

/* GPIO 7 is (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_SOURCE) */ - ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_SOURCE | + GPIOD_IS_OUT_ACTIVE, sandbox_gpio_get_flags(gpio_c, 7));

- /* Set it as output high, should become an input */ + /* Set it as output high */ ut_assertok(dm_gpio_set_value(&desc_list[7], 1)); - ut_assertok(gpio_get_status(gpio_c, 7, buf, sizeof(buf))); - ut_asserteq_str("c7: input: 0 [x] a-test.test3-gpios7", buf); + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, + sandbox_gpio_get_flags(gpio_c, 7));

/* Set it as output low, should become output high */ ut_assertok(dm_gpio_set_value(&desc_list[7], 0)); @@ -582,3 +592,91 @@ static int dm_test_gpio_devm(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_gpio_devm, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +static int dm_test_clrset_flags(struct unit_test_state *uts) +{ + struct gpio_desc desc; + struct udevice *dev; + ulong flags; + + ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev)); + ut_asserteq_str("a-test", dev->name); + ut_assertok(gpio_request_by_name(dev, "test-gpios", 1, &desc, 0)); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_MASK_DIR, GPIOD_IS_OUT)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT, flags); + ut_asserteq(GPIOD_IS_OUT, desc.flags); + ut_asserteq(0, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_clrset_flags(&desc, 0, GPIOD_IS_OUT_ACTIVE)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE, flags); + ut_asserteq(GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE, desc.flags); + ut_asserteq(1, sandbox_gpio_get_value(desc.dev, desc.offset)); + ut_asserteq(1, dm_gpio_get_value(&desc)); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_MASK_DIR, GPIOD_IS_IN)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_IN, flags & GPIOD_MASK_DIR); + ut_asserteq(GPIOD_IS_IN, desc.flags & GPIOD_MASK_DIR); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_MASK_PULL, + GPIOD_PULL_UP)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_IN | GPIOD_PULL_UP, flags); + ut_asserteq(GPIOD_IS_IN | GPIOD_PULL_UP, desc.flags); + + /* Check we cannot set both PULL_UP and PULL_DOWN */ + ut_asserteq(-EINVAL, dm_gpio_clrset_flags(&desc, 0, GPIOD_PULL_DOWN)); + + return 0; +} +DM_TEST(dm_test_clrset_flags, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +/* Check that an active-low GPIO works as expected */ +static int dm_test_clrset_flags_invert(struct unit_test_state *uts) +{ + struct gpio_desc desc; + struct udevice *dev; + ulong flags; + + ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev)); + ut_asserteq_str("a-test", dev->name); + ut_assertok(gpio_request_by_name(dev, "test-gpios", 1, &desc, + GPIOD_IS_OUT | GPIOD_ACTIVE_LOW)); + + /* + * From this size we see it as 0 (active low), but the sandbox driver + * sees the pin value high + */ + ut_asserteq(0, dm_gpio_get_value(&desc)); + ut_asserteq(1, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_set_value(&desc, 1)); + ut_asserteq(1, dm_gpio_get_value(&desc)); + ut_asserteq(0, sandbox_gpio_get_value(desc.dev, desc.offset)); + + /* Do the same with dm_gpio_clrset_flags() */ + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_IS_OUT_ACTIVE, 0)); + ut_asserteq(0, dm_gpio_get_value(&desc)); + ut_asserteq(1, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_clrset_flags(&desc, 0, GPIOD_IS_OUT_ACTIVE)); + ut_asserteq(1, dm_gpio_get_value(&desc)); + ut_asserteq(0, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE, + flags); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE, + desc.flags); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_IS_OUT_ACTIVE, 0)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW, flags); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW, desc.flags); + + return 0; +} +DM_TEST(dm_test_clrset_flags_invert, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);