On Mon, 23 Jan 2023 at 15:21, Philippe Mathieu-Daudé <phi...@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
