Re: [PATCH] hw/char/serial: Support serial_mm_init() with wakeup event.

[Peter Maydell]

On Thu, 24 Mar 2022 at 10:00, Dylan Jhong <dylan@andestech.com> wrote:
>
> On Wed, Mar 23, 2022 at 05:37:10PM +0800, Peter Maydell wrote:
> > On Wed, 23 Mar 2022 at 09:20, Dylan Jhong <dylan@andestech.com> wrote:
> > >
> > > Although the "wakeup" parameter is declared in SerialState,
> > > but there is no function actually setting it up.
> > > Support "wakeup" as parameter in serial_mm_init().
> >
> > This patch seems to provide a new argument which every
> > caller passes the same value for, unless I missed one
> > somewhere. What's the reason for this change?

> First of all, thank you for your review.
> The purpose of this variable is to allow users to specify their own wakeup 
> reason id.
>
> At present, there are only 4 wakeup reasons provided by QEMU[*1].
> Take uart as an example, which are classified as QEMU_WAKEUP_REASON_OTHER,
> so there is no way to distinguish the source of the wakeup reason when 
> designing a custom power manager device.
>
> Indeed, as you can see, currently there is no device that supports the use of 
> "wakeup_reason".
> But it will be used on the board we are going to upstream later.

None of this tells me why different UARTs would want different
wakeup reasons, or how, if I'm writing a new UART device model,
I should select the wakeup reason, though.

If the intention of this change is as an initial step in
the addition of a new board model, then you should provide
it as part of the patchseries that's upstreaming that board,
so that we can see in context how the proposed new API
is going to be used.

Also, the patch changes the wakeup reason for every user
of serial_mm_init() from "OTHER" to "NONE", which doesn't
seem right.

Since serial_mm_init() is now just a legacy wrapper for
creating a device and setting its properties, consider
just having your new board model create and configure
the device directly, including the new wakeup-reason
property, rather than using serial_mm_init() and forcing
all the other callers of it to change. (Doing that is bette
modern QEMU coding style anyway, usually.)

thanks
-- PMM