Introduce opamp device driver api #94040
Conversation
ZhaoxiangJin
force-pushed
the
feature/create-opamp-device-driver-api
branch
from
3e77ba6 to
4c49202
Compare
zephyrbot
added
area: Samples
zephyrbot
requested review from
butok,
cfriedt,
dbaluta,
decsny,
DerekSnell,
dleach02,
EmilioCBen,
galak,
jhedberg,
kartben,
manuargue,
mbolivar,
mmahadevan108,
nashif and
rruuaanng
|
It's not clear to me what the value of an op amp API is, especially since they are generally used as part of a higher level circuit. What consumers would you expect to use this API, and what do they gain from it. What are the specific use cases of the op amp you're trying to implement under this API that can't be done without this API |
Could you explain or give an example of what a ‘higher level circuit’ is in real engineering?
OPAMP is used to amplify small voltage signals in motor applications. If Zephyr does not have a device driver API for OPAMP, then users can only call vendor HAL directly in the application layer. I think this is an ugly way. Instead of this, why not provide an OPAMP device driver API after considering all vendor OPAMPs? |
ZhaoxiangJin
force-pushed
the
feature/create-opamp-device-driver-api
branch
from
4c49202 to
9a4ada0
Compare
There was a problem hiding this comment.
Great addition. Thanks.
I've added few comments and will do more reviews later.
I think samples could be safely replaced by tests. Or you may leave samples, but tests are must-have imho.
Take a look at opamp branch in my fork of zephyr. I have there tests - feel free to grab them ;) main...KozhinovAlexander:zephyr:opamp
The test will be also fairly simple, since API is simple too.
UPD: General documentation for opamp (an .rst file) is also missed. I think with such documentation the questions from @decsny could be answerde simpler ;)
| - base.yaml | ||
|
|
||
| properties: | ||
| functional-mode: |
There was a problem hiding this comment.
Maybe renaming to mod would be better. The other mode in vendor related .yaml could be renamed to speed related to slew-rate.
There was a problem hiding this comment.
Renaming to mode is not clear. I suggest dividing the mode into functional mode in opamp-controller (such as inverting mode, non-inverting mode, etc.) and operation mode in vendor_opamp.yml (such as high speed, slow speed mode, etc.).
There was a problem hiding this comment.
In that case operation mode will be nothing else as a speed. :) This is why I am suggesting to rename it to speed. Maybe we agree on speed mode? :)
There was a problem hiding this comment.
operation mode is sometimes not completely the same as speed mode. For example, NXP OPAMP has operation mode named high performance mode and low performance mode, voltage noise and startup time will be shortened in high performance mode. In this case, if we do not use operation mode but use speed mode, will it look strange?
|
|
||
| typedef int (*opamp_api_enable_t)(const struct device *dev); | ||
| typedef int (*opamp_api_disable_t)(const struct device *dev); | ||
| typedef int (*opamp_api_set_gain_t)(const struct device *dev, const struct opamp_gain_cfg *cfg); |
There was a problem hiding this comment.
I am for either adding additional opamp_api_configure function or renaming the set function to it.
There was a problem hiding this comment.
I'd like to know if there is anything else that needs to be changed in the OPAMP runtime besides gain?
There was a problem hiding this comment.
I'd like to know if there is anything else that needs to be changed in the OPAMP runtime besides gain?
This is a good question. I think either we agree here that no parameters shall be changed at runtime or every parameter (except pin mapping and clock) are allowed to be changed at runtime. If lall parameters are allowed to be changed, then opamp_api_configure_t has more meaning.
There was a problem hiding this comment.
From a practical engineering perspective, the input pins or opamp functional mode will not be changed during runtime. Generally, only the gain will be changed.
henrikbrixandersen
added
the
Architecture Review
|
A question to all participants: Do you see twister tests as must-have in context of this PR? Just thumbs up if so. |
yes but author can wait for arch wg approval of the idea first before adding everything like that I think |
Hello @KozhinovAlexander, you also do some excellent work, I looked your commit, and you have done a better job in testing side. If you are willing, I would like to invite you to add your test part based on this PR. |
Hi. Thank you for your feedback. I'll prepare today anyways everything reworked on top of your branch and try to stay synched with this PR holding my changes back until a review of the architecture proposed here is done. |
Add opamp API header. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add opamp and opamp_fast driver to zephyr build tree. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
ZhaoxiangJin
force-pushed
the
feature/create-opamp-device-driver-api
branch
2 times, most recently
from
abe8f40 to
6fd2d8d
Compare
This commit includes the following changes:
1. Add top level CMakeLists.txt entry and Kconfig.
2. Add bindings for opamp-controller, it includes
opamp functional mode property.
2. Add bindings for nxp,opamp and nxp,opamp_fast.
3. Implemented NXP opamp and opamp_fast device driver.
Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add opamp node for mcxa156. Support opamp for frdm_mcxa156. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add opamp node for mcxa166. Support opamp for frdm_mcxa166. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add opamp node for mcxa276. Support opamp for frdm_mcxa276. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add opamp node for mcxn947. Support opamp for frdm_mcxn947. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add opamp node for lpc55S36. Support opamp for lpcxpresso55s36. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add output_measure exmaple, enabled on frdm_mcxa156, frdm_mcxa166, frdm_mcxa276, frdm_mcxn947_mcxn947_cpu0, lpcxpresso55s36. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Add documentation for the opamp peripheral. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
Added opamp driver and example support to release notes for release 4.3 Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
ZhaoxiangJin
force-pushed
the
feature/create-opamp-device-driver-api
branch
from
6fd2d8d to
b045046
Compare
|
ZhaoxiangJin
requested review from
msalau,
rruuaanng,
decsny and
KozhinovAlexander
|
@ZhaoxiangJin Thank you for the review request. I think first two major things shall be done: 1. Fix CI failures. 2. Take care of architecture review. |
| - "Input0" # MCU internal DAC out | ||
| - "Input1" # VDD / 2 | ||
| - "Input2" # MCU internal VREF out | ||
| - "Input3" # Fixed 520mv |
There was a problem hiding this comment.
You can refer to the following for the enumeration description:
from i3c/renesas,ra-i3c.yaml
clock-names:
required: true
enum:
- "pclk"
- "tclk"
description: |
Clocks must be given corresponding names so that the shim driver can recognize them.
"pclk": peripheral clock source
"tclk": transfer clock source
(Even better if we keep the style the same.)
| operation-mode: | ||
| type: string | ||
| enum: | ||
| - "low_noise" # OPAMP in low noise mode |
| bias-current: | ||
| type: int | ||
| enum: | ||
| - 0 # Default value. Keep power consumption constant |
| /** Inverting gain identifier of the OPAMP that should be configured. */ | ||
| uint8_t inverting_gain; | ||
| /** Non-inverting gain identifier of the OPAMP that should be configured. */ | ||
| uint8_t non_inverting_gain; |
There was a problem hiding this comment.
For this to be portable, these values need to be SI units of some sort, like ADC_GAIN_1_5 (1/5) etc. or an actual fixed point fraction in micro or something (micro_gain)
There was a problem hiding this comment.
Looking at STM32 they support gains of 2, 4, 8 ... and -1, -3, ... which seems rather common
There was a problem hiding this comment.
STM32 support inverting mode and non-inverting mode, for inverting mode, it support 2,4,8...for non-inverting it support -1, -3.
So under the OPAMP device driver model, we need to first determine the OPAMP functional mode in DT, and then determine the gain. For inverting mode, we need to set the gain to 1, 3...
I haven't seen any fractional gain, if you know, please let me know.
There was a problem hiding this comment.
Great discussion here. My two cents:
- I have also not seen fractional gain so far.
- To keep API interface as long as possible stable from beginning - it could be better to typedef an arbitrary integer type with some opamp_gain_t type. This shall allow all the API functions always accept this gain only, while it's type could be adjusted later as API approaches more stable status.
There was a problem hiding this comment.
the issue with this approach is portability, if the value passed to the API is an opaque soc/driver specific value, its not actually portable, unless the value is defined in the devicetree or something similar. just an integer could mean anything.
There was a problem hiding this comment.
Thank you. Makes sense. I think we shall wait for the architecture review, as it was proposed earlier.
There was a problem hiding this comment.
@KozhinovAlexander architecture review call already just happened, that's why he was commenting. that is the review.
| typedef int (*opamp_api_enable_t)(const struct device *dev); | ||
| typedef int (*opamp_api_disable_t)(const struct device *dev); |
There was a problem hiding this comment.
Enable and disable are synonymous with PM resume and suspend. If the reason to disable the opamp is to preserve power, PM can be used instead.
There was a problem hiding this comment.
Would pm then disable clock over MCUs clock system?
ST has two types of disabling for example: An older stm32h745 will disable clock system driving opamps logic, while newer stm32g4 will not have any disable bits, since it is kind of asynchronous driven in terms of configuration.
There was a problem hiding this comment.
The PM suspend and resume would have the same logic which disable() and enable() would have I would assume. RESUME means device is operational, so if that requires a clock, the clock would be on in this case. Not sure what "kind of asynchronous driven in terms of configuration" means :)
|
Architecture WG meeting:
|
This is not true, it's already been done plenty of times, most common is having ADC input to another peripheral |
|
Other comment from arch WG not listed above from @bjarki-andreasen was that the API may not be generic enough, the config structure seems tailored to NXP op amp. The reason is likely because NXP op amp can configure positive and negative side of op amp |
What is OPAMP
The operational amplifier (OPAMP) is an analog device that amplifies differential input signals (difference between inverting
and non-inverting input) to give a resulting output voltage. OPAMP can amplify small voltage signals across shunt resistors in motor applications.
The core of the OPAMP includes:
OPAMPs from different vendors may have different peripheral circuit designs. Users can set the necessary properties in the device driver and bindings.
Functions implemented in device driver API
The OPAMP device driver API includes:
In actual applications, the OPAMP mode will not change at runtime, so there is no device driver API to change the OPAMP mode.
Hardware references
Work List