Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

drivers: gpio: move non-standard dts flags to be soc specific #39767

Merged
merged 1 commit into from Mar 10, 2022
Merged

drivers: gpio: move non-standard dts flags to be soc specific #39767

merged 1 commit into from Mar 10, 2022

Conversation

henrikbrixandersen
Copy link
Member

@henrikbrixandersen henrikbrixandersen commented Oct 27, 2021 •
edited

Reserve the upper 8 bits of gpio_dt_flags_t for SoC specific flags and move the non-standard, hardware-specific GPIO devicetree flags (IO voltage level, drive strength, debounce filter) from the generic dt-bindings/gpio/gpio.h header to SoC specific dt-bindings headers.

Some of the SoC specific dt-bindings flags take up more bits than necessary in order to retain backwards compatibility with the deprecated GPIO flags. The width of these fields can be reduced/optimized once the deprecated flags are removed.

Remove hardcoded use of GPIO_INT_DEBOUNCE in GPIO client drivers. This flag can now be set in the devicetree for boards/SoCs with debounce filter support. The SoC specific debounce flags have had the _INT part of their name removed since these flag must be passed to gpio_pin_configure(), not gpio_pin_interrupt_configure().

Signed-off-by: Henrik Brix Andersen hebad@vestas.com

@github-actions github-actions bot added area: API Changes to public APIs area: Devicetree Binding PR modifies or adds a Device Tree binding labels Oct 27, 2021
@zephyrbot zephyrbot requested a review from mnkp October 28, 2021 12:04
keith-zephyr
keith-zephyr previously requested changes Nov 2, 2021
View reviewed changes
Copy link
Contributor

@keith-zephyr keith-zephyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the dt-bindings are going to be extended, I think all the GPIO flags should be moved into the dt-bindings instead of just the drive strength flags.

include/dt-bindings/gpio/gpio.h Outdated Show resolved Hide resolved
@carlescufi
Copy link
Member

API meeting:

@carlescufi
Copy link
Member

@henrikbrixandersen ping

@henrikbrixandersen henrikbrixandersen marked this pull request as draft November 30, 2021 17:31
@keith-zephyr
Copy link
Contributor

I'd like to see this change get approved. What's the current status of this PR?

@henrikbrixandersen henrikbrixandersen force-pushed the gpio_ds_flags_dts branch 3 times, most recently from a6e0693 to 2756a23 Compare January 19, 2022 11:02
@henrikbrixandersen henrikbrixandersen marked this pull request as ready for review January 19, 2022 11:13
@henrikbrixandersen
Copy link
Member Author

I'd like to see this change get approved. What's the current status of this PR?

@keith-zephyr Thanks for reminding me. I have updated this PR to extend the size of gpio_dt_flags_t from 8 to 16 bits and moved the flags accordingly.

@henrikbrixandersen henrikbrixandersen added the dev-review To be discussed in dev-review meeting label Jan 20, 2022
@henrikbrixandersen henrikbrixandersen removed the dev-review To be discussed in dev-review meeting label Jan 20, 2022
@keith-zephyr keith-zephyr dismissed their stale review January 21, 2022 21:24

Removing my blocking vote.

@carlescufi
Copy link
Member

@mnkp @gmarull @mbolivar-nordic please take a look

gmarull
gmarull previously approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. In some cases, like nRF, we can now offer more meaningful options for the users. Will look at it after this gets in.

mbolivar-nordic
mbolivar-nordic previously approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need an index of device drivers in the documentation that includes information on driver-specific features more than ever with changes like this going in...

mnkp
mnkp requested changes Feb 22, 2022
View reviewed changes
Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @henrikbrixandersen, great work! I have some minor but blocking comments. Also, I think we should squash "drivers: gpio: allow specifying GPIO drive strength/debounce flags in dts" commit. I.e. implement all the changes in a single commit.

include/drivers/gpio.h Show resolved Hide resolved
@henrikbrixandersen
Copy link
Member Author

henrikbrixandersen commented Feb 22, 2022 •
edited

Thanks @henrikbrixandersen, great work! I have some minor but blocking comments. Also, I think we should squash "drivers: gpio: allow specifying GPIO drive strength/debounce flags in dts" commit. I.e. implement all the changes in a single commit.

@mnkp Thank you for reviewing. I have squashed the two commits into one. Please see my question on how you'd like to handle deprecation of the old macros.

brockus-zephyr
brockus-zephyr previously approved these changes Feb 22, 2022
View reviewed changes
@henrikbrixandersen
Copy link
Member Author

@mnkp Ping?

@henrikbrixandersen henrikbrixandersen added this to the v3.1.0 milestone Mar 2, 2022
@henrikbrixandersen henrikbrixandersen mentioned this pull request Mar 8, 2022
Merged
anangl
anangl reviewed Mar 8, 2022
View reviewed changes
drivers/gpio/gpio_andes_atcgpio100.c Outdated
@@ -140,7 +141,7 @@ static int gpio_atcgpio100_config(const struct device *port,
key = k_spin_lock(&data->lock);

/* Set de-bounce */
if (flags & GPIO_INT_DEBOUNCE) {
if (flags & ATCGPIO100_GPIO_INT_DEBOUNCE) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should get rid of this misleading INT_ component from the names of debounce flags. Unlike all other GPIO_INT_* flags, the debounce flag is supposed to be used in calls to gpio_pin_configure(), not gpio_pin_interrupt_configure().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anangl Thank you for reviewing. I have removed the _INT part of the name for the SoC specific debounce flags.

gmarull
gmarull previously approved these changes Mar 9, 2022
View reviewed changes
@henrikbrixandersen
drivers: gpio: move non-standard dts flags to be soc specific
c061702 
Reserve the upper 8 bits of gpio_dt_flags_t for SoC specific flags and
move the non-standard, hardware-specific GPIO devicetree flags (IO
voltage level, drive strength, debounce filter) from the generic
dt-bindings/gpio/gpio.h header to SoC specific dt-bindings headers.

Some of the SoC specific dt-bindings flags take up more bits than
necessary in order to retain backwards compatibility with the deprecated
GPIO flags. The width of these fields can be reduced/optimized once the
deprecated flags are removed.

Remove hardcoded use of GPIO_INT_DEBOUNCE in GPIO client drivers. This
flag can now be set in the devicetree for boards/SoCs with debounce
filter support. The SoC specific debounce flags have had the _INT part
of their name removed since these flag must be passed to
gpio_pin_configure(), not gpio_pin_interrupt_configure().

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
anangl
anangl approved these changes Mar 9, 2022
View reviewed changes
include/dt-bindings/gpio/gpio.h Show resolved Hide resolved
@nashif nashif merged commit d4023b3 into zephyrproject-rtos:main Mar 10, 2022
@henrikbrixandersen henrikbrixandersen deleted the gpio_ds_flags_dts branch March 15, 2022 15:37
@FRASTM FRASTM mentioned this pull request Mar 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anangl anangl anangl approved these changes

@gmarull gmarull gmarull approved these changes

@mnkp mnkp mnkp approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@nashif nashif Awaiting requested review from nashif

@keith-zephyr keith-zephyr Awaiting requested review from keith-zephyr

@galak galak Awaiting requested review from galak

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@Mani-Sadhasivam Mani-Sadhasivam Awaiting requested review from Mani-Sadhasivam

@glaubermaroto glaubermaroto Awaiting requested review from glaubermaroto

@mbolivar-nordic mbolivar-nordic Awaiting requested review from mbolivar-nordic

@brockus-zephyr brockus-zephyr Awaiting requested review from brockus-zephyr

Assignees

@mnkp mnkp

Labels
area: API Changes to public APIs area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: GPIO area: Samples Samples platform: NXP NXP
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@henrikbrixandersen @carlescufi @keith-zephyr @mbolivar-nordic @mnkp @gmarull @galak @anangl @brockus-zephyr @nashif