Hi Patrick,
On Mon, 8 Feb 2021 at 10:33, Patrick DELAUNAY
<patrick.delau...@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 <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), ®s->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.delau...@foss.st.com>
> Tested-by: Patrick Delaunay <patrick.delau...@foss.st.com>
>
Regards,
SImon
