Specify and document current PM_DEVICE behavior, including PM_DEVICE=n and device_deinit() #90275
Conversation
bjarki-andreasen
requested review from
JordanYates,
ceolin,
decsny and
fabiobaltieri
bjarki-andreasen
force-pushed
the
pm-device-driver-deinit
branch
from
537633f to
4ea0b00
Compare
Check out |
There was a problem hiding this comment.
For now, this is a simple check ensuring the device is not in use
What is stopping a final implementation from being implemented now? The function should have defined semantics, and they should be implemented properly when the function is introduced, not modified at some point in "the future".
This is all we need for now, I don't think there is a "final" implementation, its just a hook for doing whatever is required of PM at a given time :) |
include/zephyr/pm/device.h
Outdated
| return rc; | ||
| } | ||
|
|
||
| rc = action_cb(dev, PM_DEVICE_ACTION_TURN_OFF); |
There was a problem hiding this comment.
This doesn't seem right, the device is not literally being turned off, just the software construct destroyed.
This would lead to GPIO's being put into incorrect states if used generically, which the PM API is.
Overloading the meaning of PM_DEVICE_ACTION_TURN_OFF is a bad idea IMO.
There was a problem hiding this comment.
I'm okay with removing the call to TURN_OFF, given that all resources like GPIOs are released as part of device_deinit(), I think you are right that it makes little sense to act as if the driver is being turned off before being deinitialized.
bjarki-andreasen
force-pushed
the
pm-device-driver-deinit
branch
from
4ea0b00 to
0f5a01c
Compare
bjarki-andreasen
force-pushed
the
pm-device-driver-deinit
branch
from
0f5a01c to
4a6a1d6
Compare
|
@JordanYates could you revisit this one? |
There was a problem hiding this comment.
It's still not clear to me how this helps to reliably implement the requirements of the device_deinit API.
zephyr/include/zephyr/device.h
Lines 880 to 885 in af46f62
It's on the caller to determine whether the device can be de-initialized, not the implementation.
Why is STATE_SUSPENDED any more likely to be the "reset state" of a device than STATE_ACTIVE? In many cases the suspend state is explicitly not the reset state of a device.
This to my knowledge was intended to clearly indicate that there is no reference counting or other tracking of who is using the device built in to the device model (which there was with an early version of device_deinit()), not that no checks are allowed to make sure it makes sense. Otherwise surely Lines 150 to 152 in 600cae6
If |
|
As a concrete request, please document what the function is actually supposed to be doing beyond just |
bjarki-andreasen
requested review from
decsny,
JordanYates,
ceolin and
mmahadevan108
| * | ||
| * The initial power state expected by the system is: | ||
| * | ||
| * - ACTIVE if CONFIG_PM_DEVICE=n or (CONFIG_PM_DEVICE=y and |
There was a problem hiding this comment.
This is very cryptic and very difficult to understand, simplify this in words instead of pseudo conditional code.
There was a problem hiding this comment.
I strongly expect using pseudo code to describe conditional logic is the opposite of "confusing and very difficult to understand" in a project tailored to software engineers...
There was a problem hiding this comment.
software engineers can look at the code and figure out the details of the logic, this is a docstring that should explain an API, not repeat the logic in the code. If that was an easy condition, sure, but when you have a condition with 3 or 4 parts involving negations and a mix of AND/OR it becomes less usefull in a docstring.
There was a problem hiding this comment.
software engineers can look at the code and figure out the details of the logic
This section is part of the @details portion of the docstring. It describes only what state the driver will be in after the function is called, not how to get to that state, which would be the internal logic. Describing what the target state of something is, AFTER the function has performed some action on a resource, is definitely something that should be part of the docstring.
doc/services/pm/device.rst
Outdated
| NULL | ||
| ); | ||
|
|
||
| Device Model without Power Management Support |
There was a problem hiding this comment.
This title is confusing and IMO wrong, i.e. it would not be here in the first places if it was "without Power Management". This mode is using "device driver PM action hook", so it does use power management implementation of the driver.
Maybe: "Device model driven Power Management" ?
There was a problem hiding this comment.
"Device model driven" is not a phrase used anywhere in the project. Users would have no way of understanding what this section describes without reading the section first if we change to this title I believe.
Breaking the current title down, it has a good likelihood of being understood as the intended "device driver model with PM_DEVICE=n"
"Device Model", as in device driver model, "without Power Management", as in PM_DEVICE=n
There was a problem hiding this comment.
See previous comment regarding the same thing #90275 (comment) and the definition of the PM_DEVICE config:
Lines 78 to 86 in ad4c3e3
We can change it to "Device Model without Device Power Management"? That seems to fit the config and is clearer?
There was a problem hiding this comment.
Changed to "Device Model without Device Power Management"
There was a problem hiding this comment.
PM_DEVICE kconfig help "This option enables the device power management interface. .."
I wonder what this interface is exactly and if anything would actually work without this interface being enabled or used. Isnt
static inline int pm_device_driver_deinit(const struct device *dev, pm_device_action_cb_t action_cb)
{
return action_cb(dev, PM_DEVICE_ACTION_SUSPEND);
}
calling this interface when CONFIG_PM_DEVICE=n , i.e.:
- PM_DEVICE_ACTION_SUSPEND is part of this interface
- pm_device_action_cb_t is also part of this interface
- The implementation of this interface is guarded in most device drivers with
#ifdef CONFIG_PM_DEVICE, which confirms the above. - pm_device_driver_deinit itself is part of this interface
So you are using device power management in a different way. "without Device Power Management" is simply not true.
This is the main issue here and why every reviewer was "confused" and many of the comments in this PR are talking about confusion.
There was a problem hiding this comment.
We spent two second arch meetings discussing this semantic issue. I do not find it efficient nor reasonable to continue to block this PR to repeat that same discussion.
The title "Device Model without Device Power Management" follows the same wording as the existing title
zephyr/doc/services/pm/device.rst
Line 195 in ad4c3e3
PM_DEVICE=y, while describing the case where PM_DEVICE=n. Thus, "with" is changed to "without".
Adding "Device" in there was an improvement IMO, but it can be reverted again no problem.
There was a problem hiding this comment.
We spent two second arch meetings discussing this semantic issue. I do not find it efficient nor reasonable to continue to block this PR to repeat that same discussion.
We do not review PRs in meetings and we need to continue discussing this.
The general agreement was that the implementation is "fine" for now, knowing there are issues we need to resolve to avoid confusion and fix / improve existing drivers to follow a model that works for them.
I disagree this is just a semantic issue. Even if it was, the documentation needs to reflect reality.
Take another look at #90275 (comment) please and address that comment.
The title "Device Model without Device Power Management" follows the same wording as the existing title
zephyr/doc/services/pm/device.rst
Line 195 in ad4c3e3
which describes
PM_DEVICE=y, while describing the case wherePM_DEVICE=n. Thus, "with" is changed to "without".
Adding "Device" in there was an improvement IMO, but it can be reverted again no problem.
Given that we heavily rely on device power management interface, even with PM_DEVICE=n, how about changing the title to " ... with Partial Device Power Management"?
There was a problem hiding this comment.
I like that, argument is solid, updated to "Device Model with Partial Device Power Management Support"
|
|
||
| return 0; | ||
| } | ||
| struct dummy_driver_data { |
There was a problem hiding this comment.
That is a lot of code that sooner or later will become stale , how about updating samples/subsys/pm/device_pm/, make it an annonated sample and instead reference the code from that sample here instead? This way we will know the code works and compiles and we have things in one place only.
There was a problem hiding this comment.
This example code will be updated whenever something changes in PM, that I will ensure, just like anything else in the docs will be updated to not become stale...
Such a sample would be very complex, given we have to emulate the most complex possible driver, mock PM states, mock GPIOs, mock busses, it would be quite the undertaking, and very much outside the scope of this PR. Not saying its a bad thing, I think it would be nice to have a single test suite with a mock driver covering all of device PM with its different states, but yeah, scope :)
There was a problem hiding this comment.
the sample in samples/subsys/pm/device_pm/src/dummy_driver.c already does most of that.
This example code will be updated whenever something changes in PM, that I will ensure, just like anything else in the docs will be updated to not become stale...
Not doubting your willingness to that, but this is an area that can become stale very fast if the code is not compiled. Putting the code in a sample and referencing it here through inlining will get you the same results.
There was a problem hiding this comment.
Yeah, from previous comment: "Not saying its a bad thing, I think it would be nice to have a single test suite with a mock driver covering all of device PM with its different states, but yeah, scope :)"
There was a problem hiding this comment.
I've had this issue in other areas, I'd like to have compiled inline samples in the docs more or less, but the only way to do this today is make it a full fledged sample with independent docs and all of that... which makes no sense given its meant to be inline in the context of the docs.
I asked before about this before but we didn't come to any agreement on how best to do this.
Ideal scenario in my mind:
- inline doc samples go into a dedicated samples/doc_inline directory or something like it
- this directory contains samples meant to be inlined into docs and can therefore skip being documented, they are contextual samples going into a larger doc page.
There was a problem hiding this comment.
If inlining is a an issue, we need to figure out something. @kartben how does this work in general and how we can improve this?
In the context of this PR, this can be done later. It is just lot of code being added to the docs which runs the risk of becoming stale, we have seen this in other areas in the past.
| * | ||
| * @note | ||
| * Device context may be lost. | ||
| * @details The device should be put into its lowest internal power state, |
There was a problem hiding this comment.
I still think the details about transitions here do not belong into the state definition, but fine, we will see how this will work out and can always change it.
There was a problem hiding this comment.
We most likely will if we move these states to the device model for example :)
There was a problem hiding this comment.
yes, we need to rethink this as a whole and consider this option.
doc/services/pm/device.rst
Outdated
| a :c:enum:`pm_device_state`. | ||
| Each :c:enum:`pm_device_state` state is defined as follows: | ||
|
|
||
| - ``OFF``: Device hardware is not powered. This is the initial state from which |
There was a problem hiding this comment.
my point, this is a state, you define it as a state, how you got there involves a lot of logic and conditions that should not be part of the definition of what the state is. All of that can be left to the PM mode you are using.
Same comment applies to all states below.
But ok, lets see how this goes....
bjarki-andreasen
force-pushed
the
pm-device-driver-deinit
branch
from
fd16cb4 to
d6b30b7
Compare
UpdateUpdated the titles
which primarily focus on the config
|
- Specify "Device" Power Management Support in sections covering CONFIG_PM_DEVICE=y or n - Extend the enum pm_device_state docstrings to define and detail expectations of devices in each enum pm_device_state state. - Extend pm device driver code example with more concise comments and include pm_device_driver_init, pm_device_driver_deinit, and DEVICE_DT_INST_DEINIT_DEFINE() (device deinit feature) - Describe the device driver PM states used if PM_DEVICE is not enabled. - Note which states a device driver is initialized from and can be deinitialized in and why. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Mention introduction of pm_device_driver_deinit() in the 4.2 release notes. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
bjarki-andreasen
force-pushed
the
pm-device-driver-deinit
branch
from
d6b30b7 to
5e0d6a2
Compare
|
|
@decsny can you take a look as well? |
|
I trust that it's probably fine, I won't be able to review it properly this week, don't wait on me |
This PR aims to document the status quo of device power management to provide a foundation for current driver development, and future refactoring discussions, since we need to agree on the current state before being able to discuss the future state :)
The PR also adds the missing counterpart to
pm_device_driver_init().