[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device |
Date: |
Mon, 13 Jul 2020 12:55:23 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device,
Philippe Mathieu-Daudé <=