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

[Patrick DELAUNAY]

Hi Simon,

2 minor remarks,
On 2/5/21 5:22 AM, 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.

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 <s...@chromium.org>
---

Changes in v4:
- Update dm_gpio_set_dir_flags() to clear direction flags first

Changes in v3:
- Incorporate GPIOD_FLAGS_OUTPUT() changes from 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/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;

here the bool variable valeu can be 0 or GPIOD_IS_OUT_ACTIVE = BIT(4), 
so it should have

+ bool value = !!(flags & GPIOD_IS_OUT_ACTIVE);

or

+ int value = flags & GPIOD_IS_OUT_ACTIVE;

PS: not functional impact as

#define BSRR_BIT(gpio_pin, value)    BIT((gpio_pin) + (value ? 0 : 16))

  
  		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;
+


same here

+               int value = flags & GPIOD_IS_OUT_ACTIVE;
or
+               bool value = !!(flags & GPIOD_IS_OUT_ACTIVE);


But no impact


                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)


(...)


With the 2 remarks

Reviewed-by: Patrick Delaunay <patrick.delau...@foss.st.com>
Tested-by: Patrick Delaunay <patrick.delau...@foss.st.com>

Regards,

Patrick