Hi Daniel,
Thanks for the review!
On 8/6/26 04:01, Daniel Henrique Barboza wrote:
I believe a helper like 'static bool k230_uart_receiver_status_int' that
encapsulates it is more idiomatic.
Agreed. I'll make it into a helper in v3.
If I have to make a guess I think these macros were being used as easy
knobs to test
the emulation, i.e. manually enabling/disabling things in the header
file, rebuilding
and see if the behaves as expected. Which is of course fine, but
unless we're
willing to make these knobs available to users (either by making these
available during
./configure or create device properties we can set in the command
line) I think we
shouldn't use them in the logic since they're all hardwired.
This is more pronounced down there in k230_uart_read(). We have this:
> + /* DLL accessible only when not busy. */
> + ret = (K230_UART_16550_COMPATIBLE ||
!k230_uart_is_busy(s))
> + ? s->dll : 0;
K230_UART_16550_COMPATIBLE is hardwired to 0, hence this is equal to:
ret = !k230_uart_is_busy(s) ? s->dll : 0;
I believe we have more instances where we have a hardwired macro being
used
as conditionals in the logic. We should eliminate all of them to make
the
logic simpler.
Yes as you say I use them for testing the emulation. Indeed it shouldn't
be left in the logic. I will remove them in v3.
These are all hardwired thus I recommend creating a macro in the
header like
#define K230_UART_R_CPR (....)
that encapsulates this value.
Agreed. I will fix add K230_UART_R_CPR and the UCV macro in v3.
Regards,
WX Chen
