Comment 2 for bug 1753314

Hi. Thanks for this patch. I've had a quick look at it against the imx datasheet, and here are my comments:

* Firstly, we can't do anything with this patch without a Signed-off-by: line from you. In QEMU's process this is how people submitting code state that you're legally OK to contribute the code under QEMU's license and for it to go into QEMU.

 * Secondly, it would be very helpful if you could send patches as a simple patch format, rather than as a zipfile, and to the QEMU mailing list. https://wiki.qemu.org/Contribute/SubmitAPatch has our guidelines on this.

 * Simply adding a new VMSTATE_UINT32() field will break migration. It's better to put the new field into its own vmstate subsection so that this doesn't happen; see docs/devel/migration.rst. If we don't care about cross-version migration we could just bump the version_id/minimum_version_id fields.

 * If you run scripts/checkpatch.pl over your patch it should warn you about minor coding style issues. For instance we prefer all if() statements to use {}, even if there's only one line in them.

 * Your change to imx_update() does this:
+ if (s->ucr4 & UCR4_TXEN)
+ flags |= USR1_TRDY;

but that isn't what the spec says UCR4_TXEN does; it says we raise an interrupt only if UCR4_TXEN and USR2_TXDC are both high.

 * the imx_update() function is already rather confused in how it handles the 'flags' variable, and this change extends that confusion. The function is trying to treat 'flags' as a single set of interrupt flag bits, but the device doesn't actually have a single unified set of interrupt flags like that, they're spread over UTS and USR1 and USR2. The code as it is looks odd -- should USR1.TXMPTYEN == 0 really suppress USR1_TRDY interrupts ? I suspect this is a bug, and we should also clean things up to make 'flags' be a bool.