| Message ID | 20210205042210.2949365-11-sjg@chromium.org |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | gpio: Update and simplify the uclass API | expand |
On Thu, 4 Feb 2021 21:22:03 -0700 Simon Glass <sjg@chromium.org> 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(-) > Tested-by: Kory Maincent <kory.maincent@bootlin.com>
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); 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, Patrick
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
On 2/9/21 5:28 AM, Simon Glass wrote: > 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. > 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 . > > Regards, > SImon Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Tested-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Regards
Hi Patrick, On Wed, 10 Feb 2021 at 01:38, Patrick DELAUNAY <patrick.delaunay@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@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. > > > 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@foss.st.com> > > Tested-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Regards, Simon
On Thu, Feb 04, 2021 at 09:22:03PM -0700, 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> > Tested-by: Kory Maincent <kory.maincent@bootlin.com> > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > Tested-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > Tested-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Applied to u-boot/next, thanks!
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 8931c420845..daaa1401087 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -568,6 +568,7 @@ int dm_gpio_get_value(const struct gpio_desc *desc) int dm_gpio_set_value(const struct gpio_desc *desc, int value) { + const struct dm_gpio_ops *ops; int ret; ret = check_reserved(desc, "set_value"); @@ -577,21 +578,33 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value) if (desc->flags & GPIOD_ACTIVE_LOW) value = !value; + /* GPIOD_ are directly managed by driver in set_flags */ + ops = gpio_get_ops(desc->dev); + if (ops->set_flags) { + ulong flags = desc->flags; + + if (value) + flags |= GPIOD_IS_OUT_ACTIVE; + else + flags &= ~GPIOD_IS_OUT_ACTIVE; + return ops->set_flags(desc->dev, desc->offset, flags); + } + /* * Emulate open drain by not actively driving the line high or * Emulate open source by not actively driving the line low */ if ((desc->flags & GPIOD_OPEN_DRAIN && value) || (desc->flags & GPIOD_OPEN_SOURCE && !value)) - return gpio_get_ops(desc->dev)->direction_input(desc->dev, - desc->offset); + return ops->direction_input(desc->dev, desc->offset); else if (desc->flags & GPIOD_OPEN_DRAIN || desc->flags & GPIOD_OPEN_SOURCE) - return gpio_get_ops(desc->dev)->direction_output(desc->dev, - desc->offset, - value); + return ops->direction_output(desc->dev, desc->offset, value); + + ret = ops->set_value(desc->dev, desc->offset, value); + if (ret) + return ret; - gpio_get_ops(desc->dev)->set_value(desc->dev, desc->offset, value); return 0; } @@ -619,6 +632,17 @@ static int check_dir_flags(ulong flags) return 0; } +/** + * _dm_gpio_set_flags() - Send flags to the driver + * + * This uses the best available method to send the given flags to the driver. + * Note that if flags & GPIOD_ACTIVE_LOW, the driver sees the opposite value + * of GPIOD_IS_OUT_ACTIVE. + * + * @desc: GPIO description + * @flags: flags value to set + * @return 0 if OK, -ve on error + */ static int _dm_gpio_set_flags(struct gpio_desc *desc, ulong flags) { struct udevice *dev = desc->dev; @@ -637,38 +661,52 @@ static int _dm_gpio_set_flags(struct gpio_desc *desc, ulong flags) return ret; } + /* If active low, invert the output state */ + if ((flags & (GPIOD_IS_OUT | GPIOD_ACTIVE_LOW)) == + (GPIOD_IS_OUT | GPIOD_ACTIVE_LOW)) + flags ^= GPIOD_IS_OUT_ACTIVE; + /* GPIOD_ are directly managed by driver in set_flags */ if (ops->set_flags) { ret = ops->set_flags(dev, desc->offset, flags); } else { if (flags & GPIOD_IS_OUT) { - ret = ops->direction_output(dev, desc->offset, - GPIOD_FLAGS_OUTPUT(flags)); + bool value = flags & GPIOD_IS_OUT_ACTIVE; + + ret = ops->direction_output(dev, desc->offset, value); } else if (flags & GPIOD_IS_IN) { ret = ops->direction_input(dev, desc->offset); } } - /* save the flags also in descriptor */ - if (!ret) - desc->flags = flags; - return ret; } -int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) +int dm_gpio_clrset_flags(struct gpio_desc *desc, ulong clr, ulong set) { + ulong flags; int ret; ret = check_reserved(desc, "set_dir_flags"); if (ret) return ret; - /* combine the requested flags (for IN/OUT) and the descriptor flags */ - flags |= desc->flags; + flags = (desc->flags & ~clr) | set; + ret = _dm_gpio_set_flags(desc, flags); + if (ret) + return ret; - return ret; + /* save the flags also in descriptor */ + desc->flags = flags; + + return 0; +} + +int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags) +{ + /* combine the requested flags (for IN/OUT) and the descriptor flags */ + return dm_gpio_clrset_flags(desc, GPIOD_MASK_DIR, flags); } int dm_gpio_get_flags(struct gpio_desc *desc, ulong *flagsp) @@ -999,7 +1037,10 @@ static int gpio_request_tail(int ret, const char *nodename, debug("%s: dm_gpio_requestf failed\n", __func__); goto err; } - ret = dm_gpio_set_dir_flags(desc, flags); + + /* Keep any direction flags provided by the devicetree */ + ret = dm_gpio_set_dir_flags(desc, + flags | (desc->flags & GPIOD_MASK_DIR)); if (ret) { debug("%s: dm_gpio_set_dir failed\n", __func__); goto err; 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; 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; + 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) diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index f7f10c469c0..af3ce21a7cf 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -128,6 +128,12 @@ struct gpio_desc { #define GPIOD_PULL_UP BIT(7) /* GPIO has pull-up enabled */ #define GPIOD_PULL_DOWN BIT(8) /* GPIO has pull-down enabled */ +/* Flags for updating the above */ +#define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \ + GPIOD_IS_OUT_ACTIVE) +#define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE) +#define GPIOD_MASK_PULL (GPIOD_PULL_UP | GPIOD_PULL_DOWN) + uint offset; /* GPIO offset within the device */ /* * We could consider adding the GPIO label in here. Possibly we could @@ -135,12 +141,6 @@ struct gpio_desc { */ }; -/* helper to compute the value of the gpio output */ -#define GPIOD_FLAGS_OUTPUT_MASK (GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE) -#define GPIOD_FLAGS_OUTPUT(flags) \ - (((((flags) & GPIOD_FLAGS_OUTPUT_MASK) == GPIOD_IS_OUT_ACTIVE) || \ - (((flags) & GPIOD_FLAGS_OUTPUT_MASK) == GPIOD_ACTIVE_LOW))) - /** * dm_gpio_is_valid() - Check if a GPIO is valid * @@ -657,6 +657,25 @@ int dm_gpio_get_value(const struct gpio_desc *desc); int dm_gpio_set_value(const struct gpio_desc *desc, int value); +/** + * dm_gpio_clrset_flags() - Update flags + * + * This updates the flags as directled. Note that desc->flags is updated by this + * function on success. If any changes cannot be made, best efforts are made. + * + * By use of @clr and @set any of flags can be individually updated, or left + * alone + * + * @desc: GPIO description containing device, offset and flags, + * previously returned by gpio_request_by_name() + * @clr: Flags to clear (GPIOD_...) + * @set: Flags to set (GPIOD_...) + * @return 0 if OK, -EINVAL if the flags had obvious conflicts, + * -ERECALLCONFLICT if there was a non-obvious hardware conflict when attempting + * to set the flags + */ +int dm_gpio_clrset_flags(struct gpio_desc *desc, ulong clr, ulong set); + /** * dm_gpio_set_dir_flags() - Set direction using description and added flags * diff --git a/test/dm/gpio.c b/test/dm/gpio.c index dfbb634bf71..be5da953045 100644 --- a/test/dm/gpio.c +++ b/test/dm/gpio.c @@ -178,15 +178,15 @@ static int dm_test_gpio_opendrain_opensource(struct unit_test_state *uts) ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_DRAIN, sandbox_gpio_get_flags(gpio_c, 0)); - /* Set it as output high, should become an input */ + /* Set it as output high */ ut_assertok(dm_gpio_set_value(&desc_list[0], 1)); - ut_assertok(gpio_get_status(gpio_c, 0, buf, sizeof(buf))); - ut_asserteq_str("c0: input: 0 [x] a-test.test3-gpios0", buf); + ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_DRAIN | GPIOD_IS_OUT_ACTIVE, + sandbox_gpio_get_flags(gpio_c, 0)); - /* Set it as output low, should become output low */ + /* Set it as output low */ ut_assertok(dm_gpio_set_value(&desc_list[0], 0)); - ut_assertok(gpio_get_status(gpio_c, 0, buf, sizeof(buf))); - ut_asserteq_str("c0: output: 0 [x] a-test.test3-gpios0", buf); + ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_DRAIN, + sandbox_gpio_get_flags(gpio_c, 0)); /* GPIO 1 is (GPIO_OUT|GPIO_OPEN_SOURCE) */ ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, @@ -197,13 +197,21 @@ static int dm_test_gpio_opendrain_opensource(struct unit_test_state *uts) ut_assertok(gpio_get_status(gpio_c, 1, buf, sizeof(buf))); ut_asserteq_str("c1: output: 1 [x] a-test.test3-gpios1", buf); - /* Set it as output low, should become an input */ + /* Set it as output low */ ut_assertok(dm_gpio_set_value(&desc_list[1], 0)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, + sandbox_gpio_get_flags(gpio_c, 1)); + ut_assertok(gpio_get_status(gpio_c, 1, buf, sizeof(buf))); - ut_asserteq_str("c1: input: 1 [x] a-test.test3-gpios1", buf); + ut_asserteq_str("c1: output: 0 [x] a-test.test3-gpios1", buf); - /* GPIO 6 is (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_DRAIN) */ - ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_DRAIN, + /* + * GPIO 6 is (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_DRAIN). Looking at it + * directlt from the driver, we get GPIOD_IS_OUT_ACTIVE also, since it + * is active low + */ + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_DRAIN | + GPIOD_IS_OUT_ACTIVE, sandbox_gpio_get_flags(gpio_c, 6)); /* Set it as output high, should become output low */ @@ -211,19 +219,21 @@ static int dm_test_gpio_opendrain_opensource(struct unit_test_state *uts) ut_assertok(gpio_get_status(gpio_c, 6, buf, sizeof(buf))); ut_asserteq_str("c6: output: 0 [x] a-test.test3-gpios6", buf); - /* Set it as output low, should become an input */ + /* Set it as output low */ ut_assertok(dm_gpio_set_value(&desc_list[6], 0)); - ut_assertok(gpio_get_status(gpio_c, 6, buf, sizeof(buf))); - ut_asserteq_str("c6: input: 0 [x] a-test.test3-gpios6", buf); + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_DRAIN | + GPIOD_IS_OUT_ACTIVE, + sandbox_gpio_get_flags(gpio_c, 6)); /* GPIO 7 is (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_SOURCE) */ - ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_SOURCE | + GPIOD_IS_OUT_ACTIVE, sandbox_gpio_get_flags(gpio_c, 7)); - /* Set it as output high, should become an input */ + /* Set it as output high */ ut_assertok(dm_gpio_set_value(&desc_list[7], 1)); - ut_assertok(gpio_get_status(gpio_c, 7, buf, sizeof(buf))); - ut_asserteq_str("c7: input: 0 [x] a-test.test3-gpios7", buf); + ut_asserteq(GPIOD_ACTIVE_LOW | GPIOD_IS_OUT | GPIOD_OPEN_SOURCE, + sandbox_gpio_get_flags(gpio_c, 7)); /* Set it as output low, should become output high */ ut_assertok(dm_gpio_set_value(&desc_list[7], 0)); @@ -582,3 +592,91 @@ static int dm_test_gpio_devm(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_gpio_devm, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +static int dm_test_clrset_flags(struct unit_test_state *uts) +{ + struct gpio_desc desc; + struct udevice *dev; + ulong flags; + + ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev)); + ut_asserteq_str("a-test", dev->name); + ut_assertok(gpio_request_by_name(dev, "test-gpios", 1, &desc, 0)); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_MASK_DIR, GPIOD_IS_OUT)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT, flags); + ut_asserteq(GPIOD_IS_OUT, desc.flags); + ut_asserteq(0, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_clrset_flags(&desc, 0, GPIOD_IS_OUT_ACTIVE)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE, flags); + ut_asserteq(GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE, desc.flags); + ut_asserteq(1, sandbox_gpio_get_value(desc.dev, desc.offset)); + ut_asserteq(1, dm_gpio_get_value(&desc)); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_MASK_DIR, GPIOD_IS_IN)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_IN, flags & GPIOD_MASK_DIR); + ut_asserteq(GPIOD_IS_IN, desc.flags & GPIOD_MASK_DIR); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_MASK_PULL, + GPIOD_PULL_UP)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_IN | GPIOD_PULL_UP, flags); + ut_asserteq(GPIOD_IS_IN | GPIOD_PULL_UP, desc.flags); + + /* Check we cannot set both PULL_UP and PULL_DOWN */ + ut_asserteq(-EINVAL, dm_gpio_clrset_flags(&desc, 0, GPIOD_PULL_DOWN)); + + return 0; +} +DM_TEST(dm_test_clrset_flags, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +/* Check that an active-low GPIO works as expected */ +static int dm_test_clrset_flags_invert(struct unit_test_state *uts) +{ + struct gpio_desc desc; + struct udevice *dev; + ulong flags; + + ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev)); + ut_asserteq_str("a-test", dev->name); + ut_assertok(gpio_request_by_name(dev, "test-gpios", 1, &desc, + GPIOD_IS_OUT | GPIOD_ACTIVE_LOW)); + + /* + * From this size we see it as 0 (active low), but the sandbox driver + * sees the pin value high + */ + ut_asserteq(0, dm_gpio_get_value(&desc)); + ut_asserteq(1, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_set_value(&desc, 1)); + ut_asserteq(1, dm_gpio_get_value(&desc)); + ut_asserteq(0, sandbox_gpio_get_value(desc.dev, desc.offset)); + + /* Do the same with dm_gpio_clrset_flags() */ + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_IS_OUT_ACTIVE, 0)); + ut_asserteq(0, dm_gpio_get_value(&desc)); + ut_asserteq(1, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_clrset_flags(&desc, 0, GPIOD_IS_OUT_ACTIVE)); + ut_asserteq(1, dm_gpio_get_value(&desc)); + ut_asserteq(0, sandbox_gpio_get_value(desc.dev, desc.offset)); + + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE, + flags); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE, + desc.flags); + + ut_assertok(dm_gpio_clrset_flags(&desc, GPIOD_IS_OUT_ACTIVE, 0)); + ut_assertok(dm_gpio_get_flags(&desc, &flags)); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW, flags); + ut_asserteq(GPIOD_IS_OUT | GPIOD_ACTIVE_LOW, desc.flags); + + return 0; +} +DM_TEST(dm_test_clrset_flags_invert, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
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(-)