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

[Simon Glass]

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.


>
>
> 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.delaunay at foss.st.com>
> Tested-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
>

Regards,
SImon