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.delau...@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
*
