-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement at6561 can trx driver #20480
base: master
Are you sure you want to change the base?
Implement at6561 can trx driver #20480
Conversation
d76c88d
to
398a1e2
Compare
398a1e2
to
ee8da05
Compare
52360c7
to
5aca0e3
Compare
5aca0e3
to
ccd7fec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't a generic can_8pin_transceiver driver be a better option?
I don't know why there is a tja1042-driver in the first place, But this one certainly fits the intention of the tja1042-driver author to keep every can trx driver seperate.
? @vincent-d might know ?
gpio_t stby_pin; /**< AT6561 standby pin */ | ||
can_trx_t trx; /**< AT6561 interface */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gpio_t stby_pin; /**< AT6561 standby pin */ | |
can_trx_t trx; /**< AT6561 interface */ | |
can_trx_t trx; /**< AT6561 interface */ | |
gpio_t stby_pin; /**< AT6561 standby pin */ |
please swap to keep consistent with the other cantrx_drivers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem with swapping them, but I don't like the usage of can_trx_t *trx = &at6561
as other drivers are used in the test app tests/sys/can_trx
(I find it very error prone and it limits the freedom to define structure members)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just swap -- so it won't fail if someone does it like can_trx_t *trx = &at6561
as they might if they looked at the other drivers ( make it more simmilar)
I agree with that partially. We have already the |
You could have just used |
I could yes, they are identical with the slight difference that at6561 does not provide the silent mode. But as I found different CAN transceiver drivers I though about adding a new one. |
As far as I can tell |
You're right indeed. I just checked the driver and saw that the silent mode is same as normal mode configuration-wise. |
We can rename TJA1042 to something more generic. |
@firas-hamdi this is enough to convey all information ( just a tip on how to shorten commit messages) the area of work does not need to be repeated in the description |
Should we classify the CAN transceivers according to how many modes they provide and we generate a naming from that? |
most can-trx are 8 pin with one control pin the modes that set the pin can be put into a bitmask then set_mode function has just has to check if the bit-mask says it is has that bit set or not |
There are some CAN transceivers providing two control pins (ncv7356), should these be included too to the generic driver? |
seems like a secial case (single wire can) that we better might not cover in our generic can-8pin-trx-driver |
Contribution description
Write a driver for the AT6561 CAN transceiver built on the CAN generic transceiver interface
can_trx
.Testing procedure
The driver is tested with
tests/sys/can_trx
application. Hardware used for testing is the same54-xpro devBoard.