Re: [PATCH v3 1/7] hw/misc: Add a LED device

[Philippe Mathieu-Daudé]

Hi Richard,

On 6/21/20 4:00 AM, Richard Henderson wrote:
> On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
>> Add a LED device which can be connected to a GPIO output.
>> LEDs are limited to a set of colors.
>> They can also be dimmed with PWM devices. For now we do
>> not implement the dimmed mode, but in preparation of a
>> future implementation, we start using the LED intensity.
>> When used with GPIOs, the intensity can only be either
>> minium or maximum. This depends of the polarity of the
>> GPIO.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/led.h |  79 +++++++++++++++++++++++++++
>>  hw/misc/led.c         | 121 ++++++++++++++++++++++++++++++++++++++++++
>>  MAINTAINERS           |   6 +++
>>  hw/misc/Kconfig       |   3 ++
>>  hw/misc/Makefile.objs |   1 +
>>  hw/misc/trace-events  |   3 ++
>>  6 files changed, 213 insertions(+)
>>  create mode 100644 include/hw/misc/led.h
>>  create mode 100644 hw/misc/led.c
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> new file mode 100644
>> index 0000000000..821ee1247d
>> --- /dev/null
>> +++ b/include/hw/misc/led.h
>> @@ -0,0 +1,79 @@
>> +/*
>> + * QEMU single LED device
>> + *
>> + * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +#ifndef HW_MISC_LED_H
>> +#define HW_MISC_LED_H
>> +
>> +#include "hw/qdev-core.h"
>> +
>> +#define TYPE_LED "led"
>> +#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
>> +
>> +typedef enum {
>> +    LED_COLOR_UNKNOWN,
>> +    LED_COLOR_RED,
>> +    LED_COLOR_ORANGE,
>> +    LED_COLOR_AMBER,
>> +    LED_COLOR_YELLOW,
>> +    LED_COLOR_GREEN,
>> +    LED_COLOR_BLUE,
>> +    LED_COLOR_VIOLET, /* PURPLE */
>> +    LED_COLOR_WHITE,
>> +    LED_COLOR_COUNT
>> +} LEDColor;
> 
> Is color especially interesting, given that we only actually "display" the
> color via tracing?

The idea of this device is to easily visualize events. Currently
via tracing, but eventually an external UI could introspect the
board for devices able to represent visual changes such LEDs, and
automatically display them.
To limit the list of representable object the visualizer has to
support, I prefer to restrict this device to the existing LED
physical colors.

> 
>> +/* Definitions useful when a LED is connected to a GPIO */
>> +#define LED_RESET_INTENSITY_ACTIVE_LOW  UINT16_MAX
>> +#define LED_RESET_INTENSITY_ACTIVE_HIGH 0U
>> +
>> +typedef struct LEDState {
>> +    /* Private */
>> +    DeviceState parent_obj;
>> +    /* Public */
>> +
>> +    /* Properties */
>> +    char *description;
>> +    char *color;
> 
> The enumeration is unused by the actual device, it would seem?

I want to, but it seems having a enum qdev property involves
a lot of code.

> 
>> +/**
>> + * led_set_intensity: set the state of a LED device
>> + * @s: the LED object
>> + * @is_on: boolean indicating whether the LED is emitting
>> + *
>> + * This utility is meant for LED connected to GPIO.
>> + */
>> +void led_set_state(LEDState *s, bool is_on);
> 
> Comment mismatch.
> 
> 
>> +void led_set_intensity(LEDState *s, uint16_t new_intensity)
>> +{
>> +    trace_led_set_intensity(s->description ? s->description : "n/a",
>> +                            s->color, new_intensity);
> 
> Why not default description upon reset/realize?

Yes.

> 
>> +static void led_realize(DeviceState *dev, Error **errp)
>> +{
>> +    LEDState *s = LED(dev);
>> +
>> +    if (s->color == NULL) {
>> +        error_setg(errp, "property 'color' not specified");
>> +        return;
>> +    }
>> +}
> 
> Indeed, why not insist that description is set?  If a board is forced to say
> that the led is red, should it not also be forced to label it?

Because when we don't have access to the hardware schematics,
we can not specify the label. I'll add a comment about this.

> 
>> +static Property led_properties[] = {
>> +    DEFINE_PROP_STRING("color", LEDState, color),
> 
> It would appear that one can set any color via properties, including "plaid".
> So if you do want the char *color field, what's the point in the enum?

Good catch... I will either use an enum propery, or check the
property is in the allowed color names.

> 
>> +# led.c
>> +led_set_intensity(const char *color, const char *desc, uint16_t intensity) 
>> "LED desc:'%s' color:%s intensity: 0x%04"PRIx16
> 
> Is 0...65535 the best set of intensities?

You are right. I was thinking of PWM resolution (limiting to
16-bits). This is a different API to model, I mixed.

> Is that more valuable than e.g. a percentage?

Yes, the [0-100] range of integer is sufficient to represent
light intensity =)

Thanks for your review!

Phil.