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

[Philippe Mathieu-Daudé]

On 6/29/20 2:14 PM, Peter Maydell wrote:
> 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.

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é <f4bug@amsat.org>