On 6/29/20 2:14 PM, Peter Maydell wrote: > On Mon, 29 Jun 2020 at 10:39, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: >> >> Hi Peter, >> >> On 6/28/20 10:37 PM, Peter Maydell wrote: >>> Currently we have a free-floating set of IRQs and a function >>> tosa_out_switch() which handle the GPIO lines on the tosa board which >>> connect to LEDs, and another free-floating IRQ and tosa_reset() >>> function to handle the GPIO line that resets the system. Encapsulate >>> this behaviour in a simple QOM device. >>> >>> This commit fixes Coverity issue CID 1421929 (which pointed out that >>> the 'outsignals' in tosa_gpio_setup() were leaked), because it >>> removes the use of the qemu_allocate_irqs() API from this code >>> entirely. >>> >>> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> >>> --- >>> This is simpler than the spitz changes because the new device >>> doesn't need to refer to any of the other devices on the board. >>> --- > >>> +#define TYPE_TOSA_MISC_GPIO "tosa-misc-gpio" >>> +#define TOSA_MISC_GPIO(obj) \ >>> + OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO) >>> + >>> +typedef struct TosaMiscGPIOState { >>> + SysBusDevice parent_obj; >>> +} TosaMiscGPIOState; >> >> Since we don't really use this type, can we avoid declaring it? > > I prefer to be consistent. QOM's implementation allows this kind > of shortcut where you don't provide everything that a typical > leaf class provides if you don't need all of it, but then it > gets confusing if later on somebody wants to add something > to that leaf class. I think it's less confusing to have > just two standard patterns: > * leaf class, no subclasses > * a class that will be subclassed > and then always provide the same set of TYPE_*, cast macros, > structs, etc for whichever of the patterns you're following, > even if it happens that these aren't all needed.
Understood. > (https://wiki.qemu.org/Documentation/QOMConventions > does a reasonable job of saying what the standard pattern > is for the subclassed-class case, but is a bit less clear > on the leaf-class case.) > > >>> +static void tosa_misc_gpio_init(Object *obj) >>> +{ >>> + DeviceState *dev = DEVICE(obj); >>> + >> >> Ah, MachineClass does not inherit from DeviceClass, so we can use >> it to create GPIOs. >> >> Something is bugging me here, similar with the LEDs series I sent >> recently. >> >> GPIOs are not specific to a bus. I see ResettableClass takes Object >> arguments. >> >> We should be able to wire GPIO lines to generic Objects like LEDs. >> Parents don't have to be qdev. > > Yes, this is awkward. You pretty much have to inherit from > SysBusDevice assuming you want to get reset (the few cases > we have which directly inherit from Device like CPUs then > end up needing special handling). > >> Having to create a container to wire GPIOs or hold a reference >> to a MemoryRegion sounds wrong. > > I agree that it would be nicer if MachineState inherited from > Device (and if Device got reset properly). But that's a huge > amount of rework. For this series I'm just trying to improve > the setup for the spitz board, and "logic that's more than > just wiring up devices to each other needs to live inside some > device, and can't be in the board itself" is the system we have today. We have a large room for improvement :) Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>