Looks like the stm32l4x5 is quite similar to my implementation. It didn’t exist
when I started with my implementation . I will take a closer look and work on
improving/extending the stm32l4x5 GPIO with my proposal. Is it OK if I rename
it to stm32_gpio? so it is clearer that the implementation is generic and can
fit almost any STM32 target.
Sorry about the issues with the patches, I am not familiar with this
methodology of working with git.
Could you guide me on how to send with you a revised version of patches 3 and 4?
> On 4 Oct 2024, at 20:08, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Fri, 27 Sept 2024 at 16:08, Román Cárdenas Rodríguez
> <rcardenas....@gmail.com> wrote:
>>
>> Generic GPIO class for STM32 devices. It can be used for most of STM32 chips.
>> Note that it does not implement configuration locking mechanisms.
>
> So we already have an stm32l4x5 GPIO device. How different
> is that one from these ones? Should we be sharing implementation,
> or are they too different?
>
> When adding a new file, could you please add lines to
> the MAINTAINERS file putting it in an appropriate subsection?
>
>> +static void stm32_gpio_irq_reset(void *opaque, int line, int value)
>> +{
>> + STM32GPIOState *s = STM32_GPIO(opaque);
>> +
>> + trace_stm32_gpio_irq_reset(line, value);
>> +
>> + bool prev_reset = s->reset;
>
> QEMU's coding style says that variable declarations
> should always be at the start of a code block, never
> in the middle. (There are some other cases of this here
> and in patch 4, I think.)
>
>> + s->reset = value != 0;
>> + if (prev_reset != s->reset) {
>> + if (s->reset) {
>> + stm32_gpio_reset(DEVICE(s));
>> + } else {
>> + stm32_gpio_update_state(s);
>> + }
>> + }
>> +}
>
>> +static Property stm32_gpio_properties[] = {
>> + DEFINE_PROP_UINT32("family", STM32GPIOState, family, STM32_F2),
>
> For this sort of situation where we have a set of devices that
> are very similar but have some slight model-to-model variation,
> rather than using a device property to tell the device which
> variation it is, we generally use a parent class with the
> implementation and a set of child classes which tweak fields
> controlling the parent class behaviour. For an example, look at
> hw/gpio/aspeed_gpio.c -- TYPE_ASPEED_GPIO is marked ".abstract = true",
> and it has most of the implementation. Then the various subtypes
> inherit from it, and their class init functions set fields in
> the class struct to define the behaviour.
> hw/char/stm32l4x5_usart.c is another example.
>
>> +static void stm32_gpio_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + device_class_set_props(dc, stm32_gpio_properties);
>> + dc->vmsd = &vmstate_stm32_gpio;
>> + dc->realize = stm32_gpio_realize;
>> + device_class_set_legacy_reset(dc, stm32_gpio_reset);
>
> Could you use the non-legacy reset instead? It's not much of
> a change, instead of calling this you set the ResettableClass
> phases.hold, and the reset function has a slightly different
> function signature. hw/char/stm32l4x5_usart.c has an example.
>
> thanks
> -- PMM
