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

[Peter Crosthwaite]

On Tue, Jun 16, 2015 at 10:16 AM, Liviu Ionescu <address@hidden> wrote:
>
>> 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?
>

All. Generic is implicit from the fact there is no specifics.

>>> +    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?
>

Just the C99 thing.

>>
>>> +
>>> +    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? ;-)
>

Don't shoot the messenger :) It is not a hard rule, but you need a
solid reason to break it (#ifdeffery being the only one I can think
of).

>>> +    /*
>>> +     * 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...
>

Wont a machine level table parse make this a one-liner.

>>> +    .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.
>

Your autoformatter does a surprisingly good job of getting close to
qemu coding style. Can the rules just be tweaked any maybe QEMU coding
style can be added to eclipse?

To check your coding style, commit your work to git and:

git show | ./scripts/checkpatch.pl -

>>
>> 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?
>

They are not really aliases, they are the GPIO names proper.
qdev_[init|get|connect]_gpio_[in|out]_named are direct replacements
for the non named variants. The non-human internal names should go
away if you use them. The name of the GPIO should match something in
the TRM for the device. E.g. if the SoC define "bank A" gpios, then a
suitable string name would be "bank-A". This should match the SoC
level, not the board level so it wouldn't be names like "green-led".
Your new code handles that information. For your generic LED however,
that is a QEMU invention which is why I propose a single unnamed GPIO
in that case.

>> 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.
>

The struct definition and some helper functions to do the iteration
can go in common code. Just the tables go in each file.

I have to wonder though if the printfery is the right approach. Can
eclipse use QEMUs existing GPIO introspection capabilites to get the
LED states and do something with them? Then the LED definition is pure
GPIO passing and these is no need for new devices at all.

Regards,
Peter

>>> +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
>
>
>
>