Re: [PATCH v3 5/5] hw/char/pl011: check if UART is enabled before RX or TX operation

[Peter Maydell]

On Mon, 23 Jan 2023 at 15:21, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> pl011_can_receive() returns the number of bytes that pl011_receive() can
> accept, pl011_can_transmit() returns a boolean.
>
> I was thinking of:
>
> -- >8 --
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index dd20b76609..ea5769a31c 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State *s)
>       return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
>   }
>
> +static inline bool pl011_can_receive(PL011State *s)
> +{
> +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
> +}
> +
>   static void pl011_write(void *opaque, hwaddr offset,
>                           uint64_t value, unsigned size)
>   {
> @@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr offset,
>       }
>   }
>
> -static int pl011_can_receive(void *opaque)
> +static int pl011_receivable_bytes(void *opaque)
>   {
>       PL011State *s = (PL011State *)opaque;
>       int r;
>
> -    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
> +    if (!pl011_can_receive(s)) {
>           r = 0;
>       } else {
>           r = s->read_count < pl011_get_fifo_depth(s);
> @@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error
> **errp)
>   {
>       PL011State *s = PL011(dev);
>
> -    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> +    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes,
> pl011_receive,
>                                pl011_event, NULL, s, NULL, true);
>   }
>
> ---
>
> with maybe a better name for pl011_receivable_bytes().

Our standard-ish name for the function you pass to
qemu_chr_fe_set_handlers() is either foo_can_receive
or foo_can_read, though. That is followed through in
the name of the function argument (fd_can_read),
its type (IOCanReadHandler), and the field it gets stored
in (CharBackend::chr_can_read). It's not a great convention
but at least it is a convention...

thanks
-- PMM