| Message ID | 20210121031159.559841-1-sjg@chromium.org |
|---|---|
| Headers | show |
| Series | gpio: Update and simplify the uclass API | expand |
Hi Simon, On 1/21/21 4:11 AM, Simon Glass wrote: > At present the GPIO uclass mirrors what was in U-Boot before driver model. > It works well in most cases but is becoming cumbersome with things like > pull-up/down and drive strength. In those cases it is easier for the > driver to deal with all the flags at one, rather than piece by piece. > > In fact the current API does not officially have a method for adjusting > anything other than the direction flags. While set_dir_flags() and > get_dir_flags() do in fact support changing other flags, this is not > documented. > > Secondly, set_dir_flags actually ORs the current flags with the new ones > so it is not possible to clear flags. It seems better to use a clr/set > interface (bit clear followed by OR) to provide more flexibility. > > Finally, direction_input() and direction_output() are really just the same > thing as set_dir_flags(), with a different name. We currently use the > latter if available, failing back to the former. But it makes sense to > deprecate the old methods. > > This series makes the above changes. Existing drivers are mostly left > alone, since they should continue to operate as is. The sandbox driver is > updated to add the required new tests and the x86 driver is switched over > to the new API. > > The STM32 driver should be checked to make sure the open source/open drain > features still work as intended. > > Changes in v2: > - Use set_flags() instead of update_flags() > - Fix 'provide' typo while we are here > - Make operation of set_flags() deterministic > - Swap newf and flags in sb_gpio_set_flags() > > Simon Glass (15): > gpio: Disable functions not used with of-platdata > dm: gpio: Rename set_dir_flags() method to update_flags() > dm: gpio: Rename get_dir_flags() method to get_flags() > gpio: Rename dm_gpio_get_dir_flags() to dm_gpio_get_flags() > gpio: Drop dm_gpio_set_dir() > gpio: sandbox: Rename GPIO dir_flags to flags > gpio: sandbox: Use a separate flag for the value > gpio: sandbox: Fully separate pin value from output value > gpio: sandbox: Make sandbox_gpio_set_flags() set all flags > dm: gpio: Add a way to update flags > gpio: Replace direction_input() and direction_output() > gpio: Use an 'ops' variable everywhere > gpio: x86: Drop the deprecated methods in intel_gpio > gpio: sandbox: Track whether a GPIO is driven > gpio: Add a way to read 3-way strapping pins > > arch/sandbox/include/asm/gpio.h | 17 +- > arch/x86/include/asm/intel_pinctrl_defs.h | 5 + > drivers/gpio/gpio-uclass.c | 228 ++++++++++++++----- > drivers/gpio/intel_gpio.c | 72 +++--- > drivers/gpio/sandbox.c | 138 ++++++++---- > drivers/gpio/stm32_gpio.c | 14 +- > drivers/pinctrl/pinctrl-stmfx.c | 14 +- > include/asm-generic/gpio.h | 130 +++++++++-- > test/dm/gpio.c | 261 +++++++++++++++++++--- > 9 files changed, 663 insertions(+), 216 deletions(-) > The open source/ open drain works correctly But I found a issue for GPIOD_ACTIVE_LOW management in STM32 drivers after the serie corrected by the next patch Tested with gpio hog on DK2 board (to force directly flags by DT) arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi + +&gpioi { + test1 { + gpio-hog; + input; + gpios = <0 0>; + }; + test2 { + gpio-hog; + input; + gpios = <1 GPIO_PULL_UP>; + }; + test3 { + gpio-hog; + input; + gpios = <2 GPIO_PULL_DOWN>; + }; + test4 { + gpio-hog; + output-low; + gpios = <3 (GPIO_OPEN_DRAIN)>; + }; + test5 { + gpio-hog; + output-low; + gpios = <4 (GPIO_OPEN_DRAIN | GPIO_PULL_UP)>; + }; + test6 { + gpio-hog; + output-high; + gpios = <5 (GPIO_OPEN_DRAIN | GPIO_PULL_DOWN)>; + }; + test7 { + gpio-hog; + output-high; + gpios = <6 0>; + }; + test8 { + gpio-hog; + output-low; + gpios = <7 0>; + }; + test9 { + gpio-hog; + output-high; + gpios = <8 GPIO_ACTIVE_LOW>; + }; + test10 { + gpio-hog; + output-low; + gpios = <9 GPIO_ACTIVE_LOW>; + }; +}; Then STM32MP> pinmux status -a -------------------------- pin-controller@50002000: .... GPIOI0 : gpio input test1.gpio-hog GPIOI1 : gpio input pull-up test2.gpio-hog GPIOI2 : gpio input pull-down test3.gpio-hog GPIOI3 : gpio output open-drain test4.gpio-hog GPIOI4 : gpio output open-drain pull-up test5.gp GPIOI5 : gpio output open-drain pull-down test6. GPIOI6 : gpio output push-pull test7.gpio-hog GPIOI7 : gpio output push-pull test8.gpio-hog GPIOI8 : gpio output push-pull test9.gpio-hog GPIOI9 : gpio output push-pull test10.gpio-hog GPIOI10 : analog GPIOI11 : analog .... And STM32MP> gpio status -a .... Bank GPIOI: GPIOI0: input: 0 [x] test1.gpio-hog GPIOI1: input: 1 [x] test2.gpio-hog GPIOI2: input: 0 [x] test3.gpio-hog GPIOI3: output: 0 [x] test4.gpio-hog GPIOI4: output: 0 [x] test5.gpio-hog GPIOI5: output: 0 [x] test6.gpio-hog GPIOI6: output: 1 [x] test7.gpio-hog GPIOI7: output: 0 [x] test8.gpio-hog GPIOI8: output: 0 [x] test9.gpio-hog GPIOI9: output: 1 [x] test10.gpio-hog GPIOI10: unused: 0 [ ] GPIOI11: unused: 0 [ ] Warning: the gpio value provide here the real GPIO level (result of ops->get_value() in gpio_get_status), and not the logical level (with GPIO_ACTIVE_LOW support) Regards Patrick fixup! dm: gpio: Add a way to update flags Proposed update as GPIOD_ are directly managed by driver in update_flags except GPIOD_ACTIVE_LOW, managed in GPIO u-class. removed GPIOD_FLAGS_OUTPUT_MASK and associated calls in drivers Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Change-Id: I781236d1abadd2a46deafb2f737099188a45ab51 -------------------------- drivers/gpio/gpio-uclass.c -------------------------- index 984c07d1df..f9ded5a624 100644 @@ -652,6 +652,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) { @@ -674,8 +675,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 04dd4fa076..b08e7202fc 100644 @@ -203,12 +203,13 @@ static int stm32_gpio_update_flags(struct udevice *dev, unsigned int offset, return idx; 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); @@ -315,6 +316,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 *