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