Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device

[Peter Maydell]

On Mon, 29 Jun 2020 at 10:39, Philippe Mathieu-Daudé <f4bug@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.maydell@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.
(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.

thanks
-- PMM