CAN termination GPIO support #83422
CAN termination GPIO support #83422
Conversation
KozhinovAlexander
force-pushed
the
can_transceiver_term_gpio
branch
from
0b83668 to
95a6406
Compare
KozhinovAlexander
requested review from
henrikbrixandersen and
fabiobaltieri
zephyrbot
requested review from
alexanderwachter,
decsny,
galak,
martinjaeger,
rruuaanng and
str4t0m
| * @brief Callback API upon setting CAN transceiver termination | ||
| * See @a can_transceiver_term() for argument description | ||
| */ | ||
| typedef int (*can_transceiver_term_t)(const struct device *dev, bool state); |
There was a problem hiding this comment.
should this be something like temination_enable and termination_disable? for symmetry with the two above
There was a problem hiding this comment.
Could be done, but ends up with one function more in the API. Except the "cosmetic" symmetry would be here another reason for it?
KozhinovAlexander
force-pushed
the
can_transceiver_term_gpio
branch
from
95a6406 to
3cbfa29
Compare
|
With the last commit I've added now |
|
I don't see a reason for a disable in the API. To be honest, I don't even see a benefit over a gpio hog. This node is the last one on the bus, it needs to enable the termination immediately or the other nodes on the bus cannot communicate. If you disable the termination, you stop the whole bus. Edit: |
KozhinovAlexander
force-pushed
the
can_transceiver_term_gpio
branch
from
3cbfa29 to
5cdcb2a
Compare
The only useful application I see at the moment is some nonvolatile configuration check at runtime. |
include/zephyr/drivers/can.h
Outdated
| * @retval 0 if successful. | ||
| * @retval -EIO General input/output error, failed to toggle GPIO device. | ||
| */ | ||
| __syscall int can_termination_enable(const struct device *dev); |
There was a problem hiding this comment.
In my opinion, termination is not a property of the can controller, but from the bus. There is no need to enable/disable on a state change of the controller.
I would only add an enable to the can transceiver if we agree to add a gpio controlled termination.
The termination is most probably not even part of the transceiver.
There was a problem hiding this comment.
Thank you. I could remove it from API of course.
There was a problem hiding this comment.
I removed it from can.h (e.g. CAN API)
There was a problem hiding this comment.
The termination is most probably not even part of the transceiver.
Good point. I see termination as part of physical transport layer and transceiver a tool to control it. Thus maybe it is not wrong to have termination gpis control within phy.
Thanks. It is a good point with hard-wire. I missed it. I will remove the functionality from can.h (e.g. CAN API). |
The functionality for termination gpio is used on some boards like ST's b-g431b-esc1 motor controller board. This feature is also supported by SocketCAN in linux. Signed-off-by: Alexander Kozhinov <[email protected]>
KozhinovAlexander
force-pushed
the
can_transceiver_term_gpio
branch
from
5cdcb2a to
502e76c
Compare
There was a problem hiding this comment.
In my opinion, termination is not a property of the can controller, but from the bus. There is no need to enable/disable on a state change of the controller.
I wholeheartedly agree.
The termination is most probably not even part of the transceiver.
Again, I wholeheartedly agree.
CAN termination resistors are not physically controlled via the CAN controller, nor via the CAN transceiver (and certainly not via the group of GPIOs used for controlling a GPIO-controlled CAN transceiver, which is just a subset of the available CAN transceiver types out there).
The CAN bus termination resistors are a property of the CAN bus. If a board allows turning CAN termination resistors on/off - or even allow selecting their resistance value, which is not covered by your proposal here - that would be a function of the board, not the board's CAN controller, not the board's CAN transceiver. One way to reflect this on the board-level in Zephyr could be to use a dedicated, board-specific GPIO hog or even represent it as a GPIO-controlled regulator. Control of these resistors does not belong in the CAN controller/tranceiver APIs (I know the Linux kernel put it there, but it is misplaced, if you ask me - and Zephyr is not Linux).
@KozhinovAlexander I appreciate your eagerness in wanting to contribute to the Zephyr CAN subsystem, but I really wish you would start by opening an RFC instead of sketching up a solution based on immediate PR review comments. The RFC process often leads to a better solution and can save both your and the reviewer's time compared to the many back and forth changes, that has already happened on this PR during the last 10 hours.
Thanks for bringing it to together. I am fine with an RFC. Will do it next time first :) |
|
@alexanderwachter @henrikbrixandersen Thank you for your time and reviews. I close this PR, since the functionality is not a part of CAN-Controller and CAN-Transceiver. |
The functionality for termination gpio is used on
some boards like ST's b-g431b-esc1 motor controller board. This feature is also supported by SocketCAN in linux.
UPD:
There is only one open question regarding implementation: Would be can_mcan API the best way to propagate this functionality to the application?