Re: [PATCH 00/18] RFC: Remove deprecated audio features

On 25/04/2022 09:21, Martin Kletzander wrote: > I wanted to deal with https://bugzilla.redhat.com/2043498 and I got a > suggesstion that removing deprecated features could actually make it > easier to propagate the error. In the end (last patch) it turns out the > error is still just reported with error_fatal, so it probably is not > really needed, but I really wanted to dig into QEMU more and learn some > of the internals for quite some time now. So I used the opportunity. > The one-liner ended up being an 18 patch series which was, for someone > who has just one commit in QEMU codebase, a pretty challenging task. > Although I tried my best to do things properly, I am not sure whether I > handled everything correctly, hence the RFC. > > Any comments are very much appreciated. Thanks and have a nice day ;) > > Martin Kletzander (18): > hw/audio: Remove -soundhw support > hw/input/tsc210x: Extract common init code into new function > hw/audio: Simplify hda audio init > hw/audio/lm4549: Add errp error reporting to init function > tests/qtest: Specify audiodev= and -audiodev > ui/vnc: Require audiodev= > Introduce machine's default-audiodev property > audio: Add easy dummy audio initialiser > hw/display/xlnx_dp.c: Add audiodev property > hw/input/tsc210x.c: Support machine-default audiodev with fallback > hw/arm: Support machine-default audiodev with fallback > hw/ppc: Support machine-default audiodev with fallback > audio: Make AUD_register_card fallible and require audiodev= > audio: Require AudioState in AUD_add_capture > audio: Be more strict during audio backend initialisation > audio: Remove legacy audio environment variables and options > audio: Remove unused can_be_default > audio/spiceaudio: Fail initialisation when not using spice >

Thanks for the tidy-up! I'm not too familiar with the audio code, however one thing I noticed is that in patch 11 you are using qdev_prop_set_string() to set the audiodev for the machines. For CharDevs there already exists a qdev_prop_set_chr() function which is used to assign the chardev backend to the device, so I'm wondering if it makes sense to add a similar qdev_prop_set_audiodev() function in the same way? Then if the method of referencing the audiodev from the device ever changes from being a string containing the name, it won't require updating all of the callers.

Finally a quick glance at the tsc210x and lm4549 devices indicates that they are using global device _init() functions which shouldn't really be used in modern implementations. If you are interested in QOMify-ing these devices as part of this work, then I can certainly help provide some pointers for getting started.

ATB, Mark.