Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183 was reviewed by Michal Lenc
-- Michal Lenc commented on a discussion on cpukit/include/dev/can/sja1000.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148056 > + * @return Pointer to CAN chip structure on success, NULL otherwise. > + */ > +struct rtems_can_chip *rtems_sja1000_initialize( The naming logic is same as in CTU CAN FD driver - `rtems_ctucanfd_initialize`. Though I agree it should probably be `rtems_can_sja1000_initialize` and `rtems_can_ctucanfd_initialize`. I will change it here and submit the change for CTU CAN FD as well if you agree - it's an incompatible API change though. The function is called either from the application or board support package to initialize the controller. It's the standard char dev driver initialization, for example ```c struct rtems_can_bus bus; bus.chip = rtems_sja1000_initialize( 0x43c90000, RTEMS_SJA1000_HW_REG_SPAN_4, 64, SJA1000_WORKER_PRIORITY, RTEMS_INTERRUPT_UNIQUE, 100000000 ); int ret = rtems_can_bus_register( &bus, "/dev/can0" ); ``` -- Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000_regs.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148057 > + > +/* PeliCAN mode */ > +enum sja1000_regs { Fixed -- Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148058 > + rtems_build_name( 'C', 'A', 'N', 'S' ), > + worker_priority, > + RTEMS_MINIMUM_STACK_SIZE + 0x1000, This is the same issue with CTU CAN FD controller #5193. Me and @ppisa plan to do stack analysis for both controllers, but haven't got to it yet. I would expect 4096 which is the value of `RTEMS_MINIMUM_STACK_SIZE` for almost all platforms should be enough though. What do you think @ppisa? -- Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148060 > + ret = sja1000_enable_configuration( internal ); > + if ( ret < 0) > + goto start_chip_unlock; Yes, replaced with copy-pasted unlock and return. -- Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148061 > + struct rtems_can_queue_ends_dev *qends_dev = chip->qends_dev; > + struct rtems_can_queue_ends *qends = &qends_dev->base; > + while ( 1 ) { Ok, changed. -- Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148062 > + atomic_fetch_and( &internal->isr, ~( REG_INT_EPI ) ); > + > + if ( FIELD_GET( REG_INT_EI, isr ) ) { I am not sure if it makes the function more comprehensible. Error frame `err_frame` is used in all errors and is cumulative as it can return more that one error type in single message. I could pass a pointer to the error frame to specific functions, but the current implementation imho better indicates the variable is modified in all if statements. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183 You're receiving this email because of your account on gitlab.rtems.org.
_______________________________________________ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
