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

[Richard Henderson <richard.henderson@linaro.org>]

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?

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

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

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

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

> +# 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?
Is that more valuable than e.g. a percentage?


r~