Re: [PATCH RESEND v2 1/3] hw/char: add K230 DW 8250-compatible UART

[zhenbaii <wxchen0913@gmail.com>]

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