[topic-gpio] samples: drivers: espi: Migrate to new gpio API #22154

albertofloyd · 2020-01-24T07:44:23Z

Use device tree definitions to simplify KConfig.
Remove per board project configurations.

This commit makes following changes to GPIO dt-bindings flags: - Added GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH to indicate pin active state. - Added GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE to configure single ended pin driving mode. - Added GPIO_PULL_UP, GPIO_PULL_DOWN flags. - GPIO_INPUT, GPIO_OUTPUT to configure pin as input or output. - Added GPIO_OUTPUT_LOW, GPIO_OUTPUT_HIGH flags to initialize output in low or high state. - reworked GPIO_INT_* flags to configure pin interrupts. - following flags were deprecated: GPIO_DIR_*, GPIO_DS_DISCONNECT_*, GPIO_PUD_*, GPIO_INT_ACTIVE_*, GPIO_INT_DOUBLE_EDGE, GPIO_POL_*. To be aligned with Linux DTS standard any GPIO flags that should not be used in DTS files are moved from include/dt-bindings/gpio/gpio.h file to include/drivers/gpio.h with an exception of several old flags which removal would cause DTS compilation errors. Those remaining old flags will be removed from include/dt-bindings/gpio/gpio.h at a later stage. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

This commit adds following functions which work with pin logical levels (take into account GPIO_ACTIVE_LOW flag): - gpio_port_get, gpio_port_set_masked, gpio_port_set_bits, gpio_port_clear_bits, gpio_port_set_clr_bits - gpio_pin_get, gpio_pin_set Functions which work with pin physical levels: - gpio_port_get_raw, gpio_port_set_masked_raw, gpio_port_set_bits_raw, gpio_port_clear_bits_raw, gpio_port_set_clr_bits_raw - gpio_pin_get_raw, gpio_pin_set_raw As well as functions: - gpio_port_toggle_bits, gpio_pin_toggle_bits Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

This commit moves interrupt configuration for a single pin from gpio_pin_configure to gpio_pin_interrupt_configure function. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Both pin sets and values encoding pin values are ultimately represented by 32-bit unsigned integers. Provide typedefs that make the role of a parameter explicit. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no> Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

gpio_api_1pin testcase verifies following new GPIO API functionality: - pin active level flags - input/output configuration flags - pin drive flags - gpio_port_*, gpio_pin_* functions - pin interrupts The test is using only 1 GPIO pin (defined in DTS as LED0) and relies on the ability of the driver to configure pin simultanously as in/out. Drivers that do not allow to configure pins in in/out mode should still pass the test, however, most of the testcases will be skipped. The test does not require any modifications to board hardware. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on efr32_slwstk6061a board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on sam_e70_xplained board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on nrf52840_pca10056 board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com> Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API as well as gpio_pin_interrupt_configure function. Tested on frdm_k64f board. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Update gpio_pin_configure() to take into account GPIO flags defined by the devicetree. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Update gpio_pin_configure() to take into account GPIO flags defined by the devicetree. Use gpio_pin_get/gpio_pin_set to verify reading/writing of logical pin values. Use gpio_pin_interrupt_configure() to configure interrupts. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Change when we clear interrupt status to happen before we handle the callbacks. As the callbacks might manipluate the GPIO state and cause a GPIO interrupt. If we clear interrupt status after than we might miss an interrupt. Also only clear interrupts for interrupts that are we have enabled and that were reported when we read from the interrupt status. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Move z_impl_gpio_pin_interrupt_configure before gpio_pin_configure so we can utilize z_impl_gpio_pin_interrupt_configure in gpio_pin_configure in the future. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Move handling of logical flag support into gpio_pin_configure and gpio_pin_interrupt_configure. This way drivers don't need to know anything about logical levels. Signed-off-by: Kumar Gala <kumar.gala@linaro.org> Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Remove handling for GPIO_INT_LEVELS_LOGICAL in driver now that we do that in gpio_pin_interrupt_configure and gpio_pin_configure. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

zephyrbot · 2020-01-24T07:57:23Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

carlescufi · 2020-01-24T10:00:16Z

@albertofloyd thank you. Can you fix the checkpatch issues please?

pabigot

Block pending review of devicetree binding approach.

dts/bindings/espi/gpio-espi.yaml

Define device tree bindings for eSPI pins required for a simple eSPI handshake between master and slave. Signed-off-by: Jose Alberto Meza <jose.a.meza.arellano@intel.com>

Define pins used for eSPI handshake in MEC1501 boards. Signed-off-by: Jose Alberto Meza <jose.a.meza.arellano@intel.com>

Use device tree definitions to simplify KConfig. Remove per board project configurations. Signed-off-by: Jose Alberto Meza <jose.a.meza.arellano@intel.com>

pabigot

Thanks, the devicetree looks more like what I expected, but there are a few additional changes.

pabigot · 2020-01-24T23:42:18Z

dts/bindings/espi/gpio-espi.yaml

@@ -0,0 +1,19 @@
+# Copyright (c) 2018, Intel Corporation


Please update copyright date since this is a new file.

pabigot · 2020-01-24T23:44:42Z

samples/drivers/espi/src/main.c

 	if (ret) {
 		printk("RSMRST_PWRGD timeout!\n");
 		return;
 	}

-	ret = gpio_pin_write(gpio_dev1, CONFIG_ESPI_INIT_PIN, 1);
+	ret = gpio_pin_write(gpio_dev1, DT_GPIO_ESPI_PINS_RSM_GPIOS_PIN, 1);


Should be gpio_pin_set.

pabigot · 2020-01-24T23:46:12Z

samples/drivers/espi/src/main.c

-	ret = gpio_pin_configure(gpio_dev0, CONFIG_PWRGD_PIN, GPIO_DIR_IN);
+#ifdef DT_GPIO_ESPI_PINS_PWRG_GPIOS_CONTROLLER
+	ret = gpio_pin_configure(gpio_dev0, DT_GPIO_ESPI_PINS_PWRG_GPIOS_PIN,
+				 DT_GPIO_ESPI_PINS_PWRG_GPIOS_FLAGS);


This needs to have a direction, probably GPIO_OUTPUT_ACTIVE, ORd in.

pabigot · 2020-01-24T23:47:10Z

boards/arm/mec1501modular_assy6885/mec1501modular_assy6885.dts

+	pins {
+		compatible = "gpio-espi";
+		/* MCHP_GPIO_012 */
+		pwrg-gpios = <&gpioa 10 0>;


Specify GPIO_ACTIVE_LOW or GPIO_ACTIVE_HIGH, for all gpio selectors.

pabigot · 2020-01-24T23:48:16Z

dts/bindings/espi/gpio-espi.yaml

+   pwrg-gpios:
+      type: phandle-array
+      required: true
+      description: Board GPIO used to detect power rails are stable.


Document whether this is an input or an output of the device (I probably guessed wrong in my comment at the configuration call where I assumed it was a processor output acting like a switch) and its nominal active level.

pabigot · 2020-01-24T23:51:07Z

samples/drivers/espi/src/main.c

@@ -12,12 +12,15 @@
 #include <drivers/gpio.h>
 #include <drivers/espi.h>

-#ifdef CONFIG_ESPI_GPIO_DEV_NEEDED
+#ifdef DT_GPIO_ESPI_PINS_PWRG_GPIOS_CONTROLLER
 static struct device *gpio_dev0;


It would be nice if this were pwrg_gpio_dev (similarly rsm_gpio below) so maintainers don't have to guess what the numbering indicates.

pabigot · 2020-01-27T15:52:11Z

@albertofloyd This needs to be rebased, and the review comments addressed. If you can't get to this I will rebase it and modify the code if you wish. We need to get this to a mergeable state in the next two days to meet deadline.

pabigot · 2020-01-28T22:13:39Z

Looking closely there are some problems with this.

gpio-espi as a compatible isn't acceptable as it's too generic. AFAICT this is related to a Microchip MEC15xx eSPI controller, but there's no documentation on that device. The compatible might be something like microchip,espi-gpio, similar to microchip,pic32-gpio which exists in linux. But it's not clear to me what this thing is, since there's no driver, just a sample.

After rebasing the sample does not build due to a lingering reference to CONFIG_ESPI_GPIO_DEV0 due to changes to the sample in master. So it's going to need rework.

This sample doesn't need to be converted to devicetree at the same time as converting new GPIO API. For integration testing and branch cleanup I'm proposing to use pabigot@6593d39 which is far simpler and allows us to make progress on the topic branch.

I'll issue a PR for https://github.com/pabigot/zephyr/commits/tg/espi which contains that commit as soon as the topic branch rebase that's in progress has been completed. This will allow time for a more careful determination of how devicetree information for the eSPI infrastructure should be implemented. For now I'm marking this DNM.

carlescufi · 2020-01-29T10:28:24Z

Looking closely there are some problems with this.

gpio-espi as a compatible isn't acceptable as it's too generic. AFAICT this is related to a Microchip MEC15xx eSPI controller, but there's no documentation on that device. The compatible might be something like microchip,espi-gpio, similar to microchip,pic32-gpio which exists in linux. But it's not clear to me what this thing is, since there's no driver, just a sample.

After rebasing the sample does not build due to a lingering reference to CONFIG_ESPI_GPIO_DEV0 due to changes to the sample in master. So it's going to need rework.

This sample doesn't need to be converted to devicetree at the same time as converting new GPIO API. For integration testing and branch cleanup I'm proposing to use pabigot@6593d39 which is far simpler and allows us to make progress on the topic branch.

I'll issue a PR for https://github.com/pabigot/zephyr/commits/tg/espi which contains that commit as soon as the topic branch rebase that's in progress has been completed. This will allow time for a more careful determination of how devicetree information for the eSPI infrastructure should be implemented. For now I'm marking this DNM.

I think this makes sense. The conversion to DT will probably require additional discussions, including @galak and others. Using #22298 instead allows us to move forward with the GPIO work and delay the transition of this driver to DT.
@MaureenHelm, @mnkp and @galak thoughts?

MaureenHelm · 2020-01-29T20:43:07Z

I think this makes sense. The conversion to DT will probably require additional discussions, including @galak and others. Using #22298 instead allows us to move forward with the GPIO work and delay the transition of this driver to DT.
@MaureenHelm, @mnkp and @galak thoughts?

Agreed. Please file an issue to make sure it isn't forgotten.

pabigot · 2020-01-30T11:14:32Z

Closing: #22298 solves this for topic-gpio, the additional work is tracked in #22344.

mnkp and others added 30 commits January 15, 2020 17:20

gpio: add gpio_pin_interrupt_configure function

16e04b4

This commit moves interrupt configuration for a single pin from gpio_pin_configure to gpio_pin_interrupt_configure function. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

samples: blinky: update to use new GPIO API

28410bb

Update gpio_pin_configure() to take into account GPIO flags defined by the devicetree. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

gpio: Reorder code in drivers/gpio.h

1bcf2eb

Move z_impl_gpio_pin_interrupt_configure before gpio_pin_configure so we can utilize z_impl_gpio_pin_interrupt_configure in gpio_pin_configure in the future. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_mcux: Update for handling local layer in core

0efc75d

Remove handling for GPIO_INT_LEVELS_LOGICAL in driver now that we do that in gpio_pin_interrupt_configure and gpio_pin_configure. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_gecko: Update for handling local layer in core

7881f69

Remove handling for GPIO_INT_LEVELS_LOGICAL in driver now that we do that in gpio_pin_interrupt_configure and gpio_pin_configure. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_sam: Update for handling local layer in core

da6cd08

Remove handling for GPIO_INT_LEVELS_LOGICAL in driver now that we do that in gpio_pin_interrupt_configure and gpio_pin_configure. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_nrfx: Update for handling local layer in core

e21f73b

Remove handling for GPIO_INT_LEVELS_LOGICAL in driver now that we do that in gpio_pin_interrupt_configure and gpio_pin_configure. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_cc13xx_cc26xx: Add gpio_driver_data to driver data

1820f78

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_cc2650: Add gpio_driver_data to driver data

c078190

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_cc32xx: Add gpio_driver_data to driver data

04dd0a4

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_cmsdk_ahb: Add gpio_driver_data to driver data

47b6f4e

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_dw: Add gpio_driver_data to driver data

3a004b5

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_esp32: Add gpio_driver_data to driver data

867d10d

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_ht16k33: Add gpio_driver_data to driver data

0894fa9

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_imx: Add gpio_driver_data to driver data

824ae34

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_intel_apl: Add gpio_driver_data to driver data

5687d9f

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_mchp_xec: Add gpio_driver_data to driver data

d7b7355

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_mcux_lpc: Add gpio_driver_data to driver data

c358231

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: gpio_pcal9535a: Add gpio_driver_data to driver data

e9e59cf

Add gpio_driver_data as the first element in the driver data as the gpio core expects this. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

albertofloyd requested a review from nashif as a code owner January 24, 2020 07:44

zephyrbot added area: Devicetree area: Boards area: Samples Samples labels Jan 24, 2020

carlescufi mentioned this pull request Jan 24, 2020

Convert GPIO users to new GPIO API #20017

Closed

carlescufi requested review from mnkp and pabigot January 24, 2020 14:56

albertofloyd force-pushed the espi_sample branch from 3881eeb to 96d7f97 Compare January 24, 2020 16:49

pabigot requested changes Jan 24, 2020

View reviewed changes

dts/bindings/espi/gpio-espi.yaml Show resolved Hide resolved

albertofloyd force-pushed the espi_sample branch from 96d7f97 to 1cdb0c1 Compare January 24, 2020 21:56

albertofloyd requested a review from pabigot January 24, 2020 22:08

albertofloyd added 3 commits January 24, 2020 14:11

dts: espi: Add GPIO alias for pins required for eSPI handshake

2f23a7d

Define device tree bindings for eSPI pins required for a simple eSPI handshake between master and slave. Signed-off-by: Jose Alberto Meza <jose.a.meza.arellano@intel.com>

board: arm: mec1501modular: Define pins required for eSPI handshake

bc15083

Define pins used for eSPI handshake in MEC1501 boards. Signed-off-by: Jose Alberto Meza <jose.a.meza.arellano@intel.com>

samples: drivers: espi: Migrate to new gpio API

1cdb0c1

Use device tree definitions to simplify KConfig. Remove per board project configurations. Signed-off-by: Jose Alberto Meza <jose.a.meza.arellano@intel.com>

pabigot requested changes Jan 24, 2020

View reviewed changes

MaureenHelm changed the title ~~samples: drivers: espi: Migrate to new gpio API~~ [topic-gpio] samples: drivers: espi: Migrate to new gpio API Jan 25, 2020

pabigot force-pushed the topic-gpio branch from 51e02e6 to 73d24e3 Compare January 26, 2020 22:06

pabigot added the DNM This PR should not be merged (Do Not Merge) label Jan 28, 2020

pabigot force-pushed the topic-gpio branch from cf85b5f to 0dba087 Compare January 29, 2020 10:05

pabigot mentioned this pull request Jan 29, 2020

[TOPIC-GPIO] samples: drivers: espi: update to new GPIO API #22298

Merged

pabigot force-pushed the topic-gpio branch from 3459b35 to 2842eb3 Compare January 30, 2020 11:00

pabigot mentioned this pull request Jan 30, 2020

convert espi sample to devicetree #22344

Closed

pabigot closed this Jan 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[topic-gpio] samples: drivers: espi: Migrate to new gpio API #22154

[topic-gpio] samples: drivers: espi: Migrate to new gpio API #22154

albertofloyd commented Jan 24, 2020

zephyrbot commented Jan 24, 2020 •

edited

Loading

carlescufi commented Jan 24, 2020

pabigot left a comment

pabigot left a comment

pabigot Jan 24, 2020

pabigot Jan 24, 2020

pabigot Jan 24, 2020

pabigot Jan 24, 2020

pabigot Jan 24, 2020

pabigot Jan 24, 2020 •

edited

Loading

pabigot commented Jan 27, 2020

pabigot commented Jan 28, 2020

carlescufi commented Jan 29, 2020

MaureenHelm commented Jan 29, 2020

pabigot commented Jan 30, 2020

[topic-gpio] samples: drivers: espi: Migrate to new gpio API #22154

[topic-gpio] samples: drivers: espi: Migrate to new gpio API #22154

Conversation

albertofloyd commented Jan 24, 2020

zephyrbot commented Jan 24, 2020 • edited Loading

carlescufi commented Jan 24, 2020

pabigot left a comment

Choose a reason for hiding this comment

pabigot left a comment

Choose a reason for hiding this comment

pabigot Jan 24, 2020

Choose a reason for hiding this comment

pabigot Jan 24, 2020

Choose a reason for hiding this comment

pabigot Jan 24, 2020

Choose a reason for hiding this comment

pabigot Jan 24, 2020

Choose a reason for hiding this comment

pabigot Jan 24, 2020

Choose a reason for hiding this comment

pabigot Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

pabigot commented Jan 27, 2020

pabigot commented Jan 28, 2020

carlescufi commented Jan 29, 2020

MaureenHelm commented Jan 29, 2020

pabigot commented Jan 30, 2020

zephyrbot commented Jan 24, 2020 •

edited

Loading

pabigot Jan 24, 2020 •

edited

Loading