RTEMS | dev/serial: Refactor the pl011 driver to be extensible (!47)

[Gedare Bloom (@gedare)]

Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47 was reviewed by Gedare Bloom

--
  
Gedare Bloom started a new discussion on bsps/include/dev/serial/arm-pl011.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109464

 > +#define RXFIFO_IRQ_TRIGGER_LEVEL FIFO_LEVEL_ONE_HALF
 > +
 > +enum fifo_trigger_level {

This type should be prefaced with the scope, like `arm_pl011_fifo_trigger_level`. That said, it appears to be unused and can be removed? the `ONE_EIGHTH` and `ONE_HALF` are used, you can just directly `#define` those to `(0)` and `(2)`.?

--
  
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109465

 > +}
 > +
 > +static inline void arm_pl011_write_char(volatile pl011_base *regs_base, const char ch)

line length

--
  
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109466

 > +
 > +  /* Find the fractional part */
 > +  const uint16_t scalar = 1 << (PL011_UARTFBRD_BAUD_DIVFRAC_WIDTH + 1);

declare all the variables at the start of the block.

--
  
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109467

 > +
 > +  uint32_t ibrd, fbrd;
 > +  if (arm_pl011_compute_baudrate_params(&ibrd, &fbrd, baud, context->clock, 3) != 0)

line length. Better to do like `err = arm_...; if (err != 0)`

--
  
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109468

 > +}
 > +
 > +static bool arm_pl011_set_attributes(

this helper function is pretty long. consider refactoring to add more helpers

--
  
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109469

 > -  arm_pl011_context *ctx = (arm_pl011_context *) base;
 > +#ifndef BSP_CONSOLE_USE_INTERRUPTS
 > +  const

why only `const` with interrupts?

--
  
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109470

 > +  regs->uartlcr_h |= PL011_UARTLCR_H_FEN;
 > +  arm_pl011_disable_irq(regs, PL011_UARTI_MASK);
 > +  const rtems_status_code sc = rtems_interrupt_handler_install(

declare variables at start. these variables can be decld above in an `#ifdef`, or you might refactor this entire block of code within this `#ifdef` to a helper function

--
  
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_109471

 > +{
 > +#ifdef BSP_CONSOLE_USE_INTERRUPTS
 > +  arm_pl011_context *context = (arm_pl011_context *) base;

this block could also be `write_support_interrupt()`




-- 
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47
You're receiving this email because of your account on gitlab.rtems.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20240717/4307dfd2/attachment-0001.htm>