Re: [Qemu-devel] [RFC] generic-gpio-led & stm32-gpio-led

[Liviu Ionescu]

> On 16 Jun 2015, at 19:10, Peter Crosthwaite <address@hidden> wrote:
> 
>> +    bool use_stderr;
> 
> Why do you want to switch between stderr and stdout?

I wasn't sure which one is more appropriate, I first used stdout then tried 
stderr.

> I think stderr is more correct,

yes, that was my conclusion too. I'll make it default.

> or should at least be forced in
> nographic QEMU where stdout may be owned by a terminal mux. Otherwise
> you should probably talk to the monitor.

most of my use cases are running with semihosting active, and the application 
may printf() whatever it likes, so it is not realistic to expect no traffic on 
stdout.

but I got your point.

>> +} GenericGPIOLEDClass;
> 
> Drop the "Generic"

from the typedef or from the string type name? what about the file name?

>> +    switch (level) {
>> +    case 0:
>> +        fprintf(file, "%s",
>> +                info->active_low ? info->on_message : info->off_message);
>> +        break;
>> +
>> +    case 1:
>> +        fprintf(file, "%s",
>> +                info->active_low ? info->off_message : info->on_message);
>> +        break;
> 
> Can you do an ^ or != between level and active low to remove dup?

yes, but code readability decreases.

>> +static void generic_gpio_led_construct_callback(Object *obj,
>> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
>> +{
>> +    qemu_log_function_name();
> 
> Function call after QOM casts.

could you be more explicit?

> 
>> +
>> +    GenericGPIOLEDState *state = GENERIC_GPIO_LED_STATE(obj);
>> +    GenericGPIOLEDClass *klass = GENERIC_GPIO_LED_GET_CLASS(state);
>> +
> 
> As we don't strictly forbid C99 decls but try and avoid them.

come on, we're in 2015, 16 years were not enough to get used to C99? ;-)
 
>> +    /*
>> +     * Connect the LED interrupt to the originating GPIO pin.
>> +     * This will finally create a link inside the STM32 GPIO device.
>> +     */
>> +    qdev_connect_gpio_out(gpio_dev, info->port_bit, state->irq);
> 
> I think this is where the big QOM issue is. You are creating your own
> GPIO connection mechanism on the abstract device level. Why can't you
> do the GPIO connections on the machine level? Instead, this would be a
> call to qdev_init_gpio_in() and you can remove all the refs to
> gpio_dev.

I'll try to see how the code looks like.

the main idea was to automate all these connection details.

but if it is just a single additional line, I can live with it.

>> +static Property generic_gpio_led_properties[] = {
>> +        DEFINE_PROP_GENERIC_GPIO_LED_PTR("info", GenericGPIOLEDState, info),
>> +        DEFINE_PROP_DEVICE_STATE_PTR("mcu", GenericGPIOLEDState, mcu),
> 
> With my proposal to remove connectivity info from the struct, the
> struct simple enough to just list out the remaining props one-by-one
> rather than having to QOMify a struct for parameters.

right, just that it requires some extra typing...

>> +    .class_size = sizeof(GenericGPIOLEDClass) };
> 
> newline before }

unfortunately this is way too complicated to fix. I'm not doing any formatting 
manually, I use the Eclipse auto formatter, which generally is ok, but when 
asked to do the antiquated R&K formatting, it uses this peculiar closing brace 
syntax. (except for QEMU, in my all other sources I use the GNU formatting 
style, which is ok in Eclipse).

so manually fixing this is useless, since I auto-format after entering every 
few lines and the manual formatting would be lost.

> 
> In my proposal the machine model would do this.
> 
> qdev_connect_gpio_out_named(mcu, "name", index, qdev_get_gpio_in(gpio_dev, 
> 0));
> 
> Or something like that.

aha! I saw some examples where such names were used, but had no idea where the 
names came from.

what names do you suggest to use for GPIOs? I guess some nice aliases, since 
the internal names are not for humans. could you give me a hint how these 
aliases should be defined and used?

> As you are trying to create a large number of machines, SoC and GPIO
> messages you may want to do some tabulation of these connections and
> rip through it in a loop. The same table may contain the on/off
> messages and drive the creation of the GPIOs (and you may need to
> parse the table multiple times if there are construction pass
> inter-deps).

I'm using tables and loop type creation when defining MCUs (for example about a 
dozen STM32 MCUs are created like this), but for machines things are not as 
uniform, and are anyway grouped by board vendor, so not all boards are in a 
file.

>> +static void stm_gpio_led_construct_callback(Object *obj,
>> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
>> +{
>> +    qemu_log_function_name();
>> +
> 
> What would you want to put here?

>> +    /* Explicitly call the parent constructor. */
>> +    GENERIC_GPIO_LED_GET_CLASS(obj)->construct(obj, info, mcu);

in this case nothing, because the derived class just implements a base abstract.

but the constructor was kept to call the parent constructor, like usual.


thank you,

Liviu