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 <s...@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;
+ else
+ flags &= ~GPIOD_IS_OUT_ACTIVE;
+ return ops->update_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_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
