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

[Simon Glass]

Hi Patrick,

On Wed, 10 Feb 2021 at 01:38, Patrick DELAUNAY
<patrick.delaunay at foss.st.com> wrote:
>
>
> On 2/9/21 5:28 AM, Simon Glass wrote:
> > Hi Patrick,
> >
> > On Mon, 8 Feb 2021 at 10:33, Patrick DELAUNAY
> > <patrick.delaunay at foss.st.com> wrote:
> >> 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 <sjg at 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);
> > The bool type should do this automatically. I tested this and it seems
> > to do the right thing.
> >
> I confirmed that it is working, forget my remarks.
>
> for information: I tested it in stm32MP157C-DK2 board (with gcc 9.2)....
>
>
> After check, it is my old habit / coding rule, when the bool type
>
> don't exist in C (it was a typedef to int)
>
> but, since C++ introducing a proper bool type,
>
> the cast to 'bool' is enough with current compilers .

Yes I am still getting used to it myself!

>
> Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
>
> Tested-by: Patrick Delaunay <patrick.delaunay at foss.st.com>

Regards,
Simon