Introduce gpio-keys wake-up sources using pinctrl #64538

nandojve · 2023-10-29T11:24:28Z

Some devices require use of pinctrl driver to initialize and configure advanced features like wake-up sources. This introduce minimal support to pinctrl driver to enable such resource for those platforms in the gpio-keys driver.

Since the gpio-keys can be used as an standard mechanism as wake-up this introduces an example how to demonstrate such
functionality together with power-off API.

depends on #64213

zephyrbot · 2023-10-29T11:25:35Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_atmel	zephyrproject-rtos/hal_atmel@`942d664`	zephyrproject-rtos/hal_atmel@`add80d4` (master)	zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

This add the pin wake-up source configuration capabilities to the SAM pinctrl driver. It add the SUPC functions at SoC and update gpio and pinctrl drivers to allow user configure the wake-up core power supply functionality. Signed-off-by: Gerson Fernando Budke <[email protected]>

Some devices require use of pinctrl driver to initialize and configure advanced features like wake-up sources. This introduce minimal support to pinctrl driver to enable such resource for those platforms. Signed-off-by: Gerson Fernando Budke <[email protected]>

Add pinctrl configuration to enable wake-up sources for gpio-keys driver. After this the special function for some wake-up source will be enabled by default and can wake-up device from a power-off state. In addition it reorder pinctrl definitions to be ascendent. Signed-off-by: Gerson Fernando Budke <[email protected]>

The gpio-keys is a standard mechanism in Linux to be used as wake-up source. This introduces an example to demonstrate how to use such functionality toguether with power-off API. Signed-off-by: Gerson Fernando Budke <[email protected]>

nandojve · 2023-11-01T12:09:48Z

The first commit is from #64213 and can be ignored.

gmarull · 2023-11-02T09:05:00Z

boards/arm/sam4e_xpro/sam4e_xpro.dts

+		pinctrl-0 = <&gpio_keys_default>;
+		pinctrl-names = "default";


if you look at Linux, the way gpio-keys are configured as wake-up sources is via the wakeup-source property (in each subnode). So, I guess the proper way to handle this is to make the input driver handle such property and forward it to the GPIO/pinctrl(?) driver when configuring the pin.

ref https://www.kernel.org/doc/Documentation/devicetree/bindings/input/gpio-keys.txt

if you look at Linux, the way gpio-keys are configured as wake-up sources is via the wakeup-source property (in each subnode). So, I guess the proper way to handle this is to make the input driver handle such property and forward it to the GPIO/pinctrl(?) driver when configuring the pin.

This PR seems to be the most far I can go with what is implemented in tree. IMHO the use of wakeup-source can be addressed in another moment because it seems to require a dedicated API which, for the moment, is out of scope for this PR. To propose an API change or new API require in deep knowledge from other architectures which I don't have.

I'm fine adding these pinctrl entries (thanks for the https://github.com/torvalds/linux/blob/750b95887e567848ac2c851dae47922cac6db946/arch/arm/boot/dts/microchip/at91-sama5d2_xplained.dts#L708-L720 example). However, these entries should be of course optional, and the driver needs to be prepared to handle the case when they're not present, that is, handle the -ENOENT error case gracefully. Also, the wakeup-source can be used as a toggle to apply such configuration, I guess.

I'm fine adding these pinctrl entries

Ok, thank you. Can I get your approval on #64213 since it a dependency?

However, these entries should be of course optional, and the driver needs to be prepared to handle the case when they're not present, that is, handle the -ENOENT error case gracefully.

Perfect! I'll do it.

Also, the wakeup-source can be used as a toggle to apply such configuration, I guess.

This is what I want add later on another PR but that is the final idea.

gmarull · 2023-11-02T09:06:11Z

drivers/input/input_gpio_keys.c

+	if (ret < 0) {
+		return ret;
+	}


this will make the driver useless to any platform that has CONFIG_PINCTRL=y and no pinctrl entries for each gpio-keys subnode.

What is your suggestion about how to handle it?

see my comment above

fabiobaltieri · 2023-11-03T09:44:46Z

Hey this approach is cool for the demo, but how would it work if you wanted to setup wake up from a different source? Like a device interrupt or some other signal. I think that the wakeup property should be somewhere else, not in pinctrl.

nandojve · 2023-11-03T13:55:28Z

Hey this approach is cool for the demo, but how would it work if you wanted to setup wake up from a different source? Like a device interrupt or some other signal.

The demo is quite specific, it uses gpio-keys and sys_poweroff. If the MCU already uses GPIO to enable wake-up it should be transparent, right? However, if it is possible to do using the GPIO API why it is so important to use wakeup-source here? In the same way, why pinctrl could not be a solution since it extends GPIO ?

I think that the wakeup property should be somewhere else, not in pinctrl.

Why? Why the pinctrl is not the right place to stored the information required by this SoC to enable the required functionality?
I was wondering if can you give a direction about how to bind GPIO + SUPC without PINCTRL to enable this functionality ? Can it be made without a custom solution?

I question because maybe it is missing context about why pinctrl should be present. The Atmel requires specific bit to be set at SUPC which doesn't have nothing to do with gpio itself but it is defined as a special attribute for a specific gpio pin (random). This can be easy defined at pinconfig structure as stated in the datasheet, see #64213 (comment). Further, this information can be used together with wakeup-source since not all pins defined at gpio-keys may have the wake-up feature, or even because they have the feature but user don't want to use it on that pin. In the Atmel SAM this is so critical that user lost even the hardware reset functionality. Using pinctrl allows us to make future checks to confirm that the pin has the required functionality to improve edge cases, for instance function priority.

That said, because after this PR it is possible to use the wake-up feature directly for the Atmel platform by pinctrl, as a short cut, it doesn't mean that it is wrong nor it is violating something. However, using the GPIO only will require a custom solution on my view because it necessary to bind SUPC with GPIO. It is important mention that there is no wakeup-source in the pinctrl definition. What is there is a flag that tells that the feature is available on that pin as stated in the datasheet and the SUPC index to be consumed by a final solution and it is not clear yet how to pursue that.

As reference, the gpio-keys can define/use pinctrl as here in Linux mainline: https://github.com/torvalds/linux/blob/750b95887e567848ac2c851dae47922cac6db946/arch/arm/boot/dts/microchip/at91-sama5d2_xplained.dts#L708-L720
So, add support to pinctrl itself should not be an issue and it is missing in the gpio-keys for Zephyr anyway.

About wakeup-source, I'm never mentioned that wakeup-source should be ignored. I agreed that it should be used to enabled the feature in the final solution (not in this PR). Both PRs are intermediate steps which may require more API to provide the final solution.

Remarks:

I understand that I've been criticized a lot for the good but I question why Zephyr allowed people add their own custom example without same criticism drivers: power: sam: Introduce Atmel SAM SUPC, wakeup sources and poweroff implementation #63961 (comment).
IMHO both changes go in the right direction without any customization.
I don't see a technical problem to have multiple ways to do same thing if there is no violation of API and there is no standard well defined.
To propose a final API to bind all this requires deep knowledge about all other platforms. in this case, I think it should be addressed in the future PR.

fabiobaltieri · 2023-11-03T14:22:42Z

I'm fine with a vendor specific solution but this seems like would require modifying existing drivers to add pinctrl code when they don't need it for any other reasons. How would you wakeup, say, from an accelerometer interrupt? The driver does not have a pinctrl setup, it does not have to, would you start adding pinctrl call to every driver in the repository?

nandojve · 2023-11-03T19:14:19Z

I'm fine with a vendor specific solution but this seems like would require modifying existing drivers to add pinctrl code when they don't need it for any other reasons.

There is no API changes here or on any place. So, if I understood you correctly the answer is no. There is no reason to do any changes on any pinctrl driver. For the specific edge case of Atmel it was necessary the functionality but that doesn't means that others driver will require that. Even for the Atmel, the information was encoded in a way that don't require any pinctrl API changes.

As @gmarull suggested above, I need to fix the -ENOENT to avoid the problem when someone do not define the pinctrl entries.

How would you wakeup, say, from an accelerometer interrupt? The driver does not have a pinctrl setup, it does not have to, would you start adding pinctrl call to every driver in the repository?

The same way it is made today.

In the case of gpio-keys, the only thing that I made was add the pinctrl support, which will be optional if the platform has pinctrl support. Otherwise driver build without that, see POSIX cases for instance.

nandojve · 2023-11-11T08:47:36Z

Hi @ceolin ,

Here there is the proposal how to connect a few bits about wakeup. With this, at least with Atmel, will be possible to link pinctrl pin info with gpio flags to indicate to SUPC controller what are the active wakeup sources. After that, the SUPC driver could be used to monitor what sources were active for that purpose since it knows that all the time. Then, I imagine that PM could consult the SoC supply/PM driver about the registered sources and take acton.

On this model each driver will be responsible to register itself on the SoC supply manager. I understand that those connections are anyway vendor specific and should not be a problem. The PM than only needs to talk with SoC supply manager to know what to do. Users donˋt need to do anything.

Maybe I still didnˋt figure out all the implications but I think is a start.

github-actions · 2024-01-11T00:29:36Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

github-actions · 2024-04-08T00:30:01Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

github-actions · 2024-06-08T00:30:06Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

nandojve added this to the v3.6.0 milestone Oct 29, 2023

nandojve requested review from kartben and bjarki-andreasen October 29, 2023 11:25

zephyrbot added manifest manifest-hal_atmel DNM This PR should not be merged (Do Not Merge) labels Oct 29, 2023

nandojve mentioned this pull request Nov 1, 2023

drivers: pinctrl: sam: Add wake-up source support #64213

Closed

nandojve force-pushed the sam/add_gpio_keys_wakeup_from_pinctrl branch from ffc2386 to 071d958 Compare November 1, 2023 07:26

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Nov 1, 2023

nandojve added 2 commits November 1, 2023 09:47

nandojve force-pushed the sam/add_gpio_keys_wakeup_from_pinctrl branch from 071d958 to 282bdb3 Compare November 1, 2023 08:47

nandojve force-pushed the sam/add_gpio_keys_wakeup_from_pinctrl branch from 282bdb3 to 9d8833f Compare November 1, 2023 10:51

nandojve marked this pull request as ready for review November 1, 2023 12:09

nandojve requested review from nashif, carlescufi, galak, MaureenHelm, gmarull and ioannisg as code owners November 1, 2023 12:09

zephyrbot added platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) area: Pinctrl area: Samples Samples area: Devicetree Binding PR modifies or adds a Device Tree binding area: Input Input Subsystem and Drivers labels Nov 1, 2023

zephyrbot requested a review from attie-argentum November 1, 2023 12:10

zephyrbot requested review from fabiobaltieri, mnkp, pdgendt and stephanosio November 1, 2023 12:10

zephyrbot assigned kartben Nov 1, 2023

fabiobaltieri assigned fabiobaltieri and gmarull and unassigned kartben Nov 1, 2023

gmarull requested changes Nov 2, 2023

View reviewed changes

nandojve requested a review from gmarull November 2, 2023 17:50

nandojve requested a review from ceolin November 11, 2023 08:24

github-actions bot added the Stale label Jan 11, 2024

nandojve removed the Stale label Jan 11, 2024

henrikbrixandersen removed this from the v3.6.0 milestone Feb 7, 2024

github-actions bot added the Stale label Apr 8, 2024

nandojve removed the Stale label Apr 8, 2024

github-actions bot added the Stale label Jun 8, 2024

github-actions bot closed this Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce gpio-keys wake-up sources using pinctrl #64538

Introduce gpio-keys wake-up sources using pinctrl #64538

nandojve commented Oct 29, 2023

zephyrbot commented Oct 29, 2023

nandojve commented Nov 1, 2023

gmarull Nov 2, 2023

gmarull Nov 2, 2023

nandojve Nov 2, 2023

gmarull Nov 3, 2023

nandojve Nov 3, 2023

gmarull Nov 2, 2023

nandojve Nov 2, 2023

gmarull Nov 3, 2023

fabiobaltieri commented Nov 3, 2023

nandojve commented Nov 3, 2023

fabiobaltieri commented Nov 3, 2023

nandojve commented Nov 3, 2023

nandojve commented Nov 11, 2023

github-actions bot commented Jan 11, 2024

github-actions bot commented Apr 8, 2024

github-actions bot commented Jun 8, 2024

Introduce gpio-keys wake-up sources using pinctrl #64538

Introduce gpio-keys wake-up sources using pinctrl #64538

Conversation

nandojve commented Oct 29, 2023

zephyrbot commented Oct 29, 2023

nandojve commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiobaltieri commented Nov 3, 2023

nandojve commented Nov 3, 2023

fabiobaltieri commented Nov 3, 2023

nandojve commented Nov 3, 2023

nandojve commented Nov 11, 2023

github-actions bot commented Jan 11, 2024

github-actions bot commented Apr 8, 2024

github-actions bot commented Jun 8, 2024