From 7e0a96d559da493812220731535f5ba6351bcea1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 4 Feb 2021 21:22:03 -0700 Subject: [PATCH] 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 Tested-by: Kory Maincent Reviewed-by: Patrick Delaunay Tested-by: Patrick Delaunay Reviewed-by: Patrick Delaunay Tested-by: Patrick Delaunay --- drivers/gpio/gpio-uclass.c | 75 ++++++++++++++---- 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, 203 insertions(+), 43 deletions(-) diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 42479dba4d..eb8850dfe5 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -569,6 +569,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"); @@ -578,21 +579,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; } @@ -620,6 +633,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; @@ -638,38 +662,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, GPIOD_MASK_DIR, flags); } int dm_gpio_set_dir(struct gpio_desc *desc) @@ -1011,7 +1049,10 @@ 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); + + /* Keep any direction flags provided by the devicetree */ + ret = dm_gpio_set_dir_flags(desc, + flags | (desc->flags & GPIOD_MASK_DIR)); if (ret) { debug("%s: dm_gpio_set_dir failed\n", __func__); goto err; diff --git a/drivers/gpio/stm32_gpio.c b/drivers/gpio/stm32_gpio.c index c2d7046c0d..125c237551 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), ®s->bsrr); diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c index 6ba56a16a3..fe7a59d431 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 4f8d1938da..7d1e19ad53 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 * @@ -668,6 +668,25 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value); */ int dm_gpio_set_dir(struct gpio_desc *desc); +/** + * 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 dfbb634bf7..be5da95304 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); -- 2.39.5