On 23/1/23 17:23, Peter Maydell wrote:
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...
I agree this deserves a better name; maybe this is not a
convention but I'd expect functions starting with is_* / can_*
to return a boolean value, not a number:
/**
* IOCanReadHandler: Return the number of bytes that #IOReadHandler can
accept
*
* This function reports how many bytes #IOReadHandler is prepared to
accept.
* #IOReadHandler may be invoked with up to this number of bytes. If this
* function returns 0 then #IOReadHandler is not invoked.
*
* This function is typically called from an event loop. If the number of
* bytes changes outside the event loop (e.g. because a vcpu thread
drained the
* buffer), then it is necessary to kick the event loop so that this
function
* is called again. aio_notify() or qemu_notify_event() can be used to
kick
* the event loop.
*/
typedef int IOCanReadHandler(void *opaque);
Also, maybe using unsigned or size_t type for the return value would
better fit. Anyhow, not really a priority :)
