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é <f4...@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é <f4...@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~
