[v4,1/8] hw/misc/led: Add a LED device

Add a LED device which can be connected to a GPIO output.
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.

LEDs are limited to a fixed set of colors.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/led.h |  84 +++++++++++++++++++++++++
 hw/misc/led.c         | 142 ++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS           |   6 ++
 hw/misc/Kconfig       |   3 +
 hw/misc/meson.build   |   1 +
 hw/misc/trace-events  |   3 +
 6 files changed, 239 insertions(+)
 create mode 100644 include/hw/misc/led.h
 create mode 100644 hw/misc/led.c

Hi Philippe,

On 9/7/20 6:32 PM, Philippe Mathieu-Daudé wrote:
> Add a LED device which can be connected to a GPIO output.
> 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.
> 
> LEDs are limited to a fixed set of colors.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/misc/led.h |  84 +++++++++++++++++++++++++
>   hw/misc/led.c         | 142 ++++++++++++++++++++++++++++++++++++++++++
>   MAINTAINERS           |   6 ++
>   hw/misc/Kconfig       |   3 +
>   hw/misc/meson.build   |   1 +
>   hw/misc/trace-events  |   3 +
>   6 files changed, 239 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 00000000000..1aaabbebafc
> --- /dev/null
> +++ b/include/hw/misc/led.h
> @@ -0,0 +1,84 @@
> +/*
> + * 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)
> +
> +/**
> + * LEDColor: Color of a LED
> + *
> + * This set is restricted to physically available LED colors.
> + *
> + * LED colors from 'Table 1. Product performance of LUXEON Rebel Color
> + * Line' of the 'DS68 LUXEON Rebel Color Line' datasheet available at:
> + * https://www.lumileds.com/products/color-leds/luxeon-rebel-color/
> + */
> +typedef enum {          /* Coarse wavelength range */
> +    LED_COLOR_VIOLET,   /* 425 nm */
> +    LED_COLOR_BLUE,     /* 475 nm */
> +    LED_COLOR_CYAN,     /* 500 nm */
> +    LED_COLOR_GREEN,    /* 535 nm */
> +    LED_COLOR_AMBER,    /* 590 nm */
> +    LED_COLOR_ORANGE,   /* 615 nm */
> +    LED_COLOR_RED,      /* 630 nm */
> +} LEDColor;
> +
> +typedef struct LEDState {
> +    /* Private */
> +    DeviceState parent_obj;
> +    /* Public */
> +
> +    uint8_t intensity_percent;
> +
> +    /* Properties */
> +    char *description;
> +    char *color;
> +} LEDState;
> +
> +/**
> + * led_set_intensity: set the intensity of a LED device
> + * @s: the LED object
> + * @intensity: intensity as percentage in range 0 to 100.
@intensity_percent

> + */
> +void led_set_intensity(LEDState *s, unsigned intensity_percent);
> +
> +/**
> + * led_get_intensity:
> + * @s: the LED object
> + *
> + * Returns: The LED intensity as percentage in range 0 to 100.
> + */
> +unsigned led_get_intensity(LEDState *s);
> +
> +/**
> + * led_set_intensity: set the state of a LED device
led_set_state

> + * @s: the LED object
> + * @is_on: boolean indicating whether the LED is emitting
@is_emitting

> + *
> + * This utility is meant for LED connected to GPIO.
> + */
> +void led_set_state(LEDState *s, bool is_emitting);
> +
> +/**
> + * led_create_simple: Create and realize a LED device
> + * @parent: the parent object
@parentobj

> + * @color: color of the LED
> + * @description: description of the LED (optional)
> + *
> + * Create the device state structure, initialize it, and
> + * drop the reference to it (the device is realized).
> + */
> +LEDState *led_create_simple(Object *parentobj,
> +                            LEDColor color,
> +                            const char *description);
> +
> +#endif /* HW_MISC_LED_H */
> diff --git a/hw/misc/led.c b/hw/misc/led.c
> new file mode 100644
> index 00000000000..f2140739b68
> --- /dev/null
> +++ b/hw/misc/led.c
> @@ -0,0 +1,142 @@
> +/*
> + * QEMU single LED device
> + *
> + * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/misc/led.h"
> +#include "trace.h"
> +
> +#define LED_INTENSITY_PERCENT_MAX   100
> +
> +static const char *led_color_name[] = {
> +    [LED_COLOR_VIOLET]  = "violet",
> +    [LED_COLOR_BLUE]    = "blue",
> +    [LED_COLOR_CYAN]    = "cyan",
> +    [LED_COLOR_GREEN]   = "green",
> +    [LED_COLOR_AMBER]   = "amber",
> +    [LED_COLOR_ORANGE]  = "orange",
> +    [LED_COLOR_RED]     = "red",
> +};
> +
> +static bool led_color_name_is_valid(const char *color_name)
> +{
> +    for (size_t i = 0; i < ARRAY_SIZE(led_color_name); i++) {
> +        if (led_color_name[i] && strcmp(color_name, led_color_name[i]) == 0) {

Why are you checking led_color_name[i] here?

Otherwise, seems good to me.

Reviewed-by: Luc Michel <luc.michel@greensocs.com>

> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +void led_set_intensity(LEDState *s, unsigned intensity_percent)
> +{
> +    if (intensity_percent > LED_INTENSITY_PERCENT_MAX) {
> +        intensity_percent = LED_INTENSITY_PERCENT_MAX;
> +    }
> +    trace_led_set_intensity(s->description, s->color, intensity_percent);
> +    s->intensity_percent = intensity_percent;
> +}
> +
> +unsigned led_get_intensity(LEDState *s)
> +{
> +    return s->intensity_percent;
> +}
> +
> +void led_set_state(LEDState *s, bool is_emitting)
> +{
> +    led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
> +}
> +
> +static void led_reset(DeviceState *dev)
> +{
> +    LEDState *s = LED(dev);
> +
> +    led_set_state(s, false);
> +}
> +
> +static const VMStateDescription vmstate_led = {
> +    .name = TYPE_LED,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(intensity_percent, LEDState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void led_realize(DeviceState *dev, Error **errp)
> +{
> +    LEDState *s = LED(dev);
> +
> +    if (s->color == NULL) {
> +        error_setg(errp, "property 'color' not specified");
> +        return;
> +    } else if (!led_color_name_is_valid(s->color)) {
> +        error_setg(errp, "property 'color' invalid or not supported");
> +        return;
> +    }
> +    if (s->description == NULL) {
> +        s->description = g_strdup("n/a");
> +    }
> +}
> +
> +static Property led_properties[] = {
> +    DEFINE_PROP_STRING("color", LEDState, color),
> +    DEFINE_PROP_STRING("description", LEDState, description),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void led_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc = "LED";
> +    dc->vmsd = &vmstate_led;
> +    dc->reset = led_reset;
> +    dc->realize = led_realize;
> +    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> +    device_class_set_props(dc, led_properties);
> +}
> +
> +static const TypeInfo led_info = {
> +    .name = TYPE_LED,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(LEDState),
> +    .class_init = led_class_init
> +};
> +
> +static void led_register_types(void)
> +{
> +    type_register_static(&led_info);
> +}
> +
> +type_init(led_register_types)
> +
> +LEDState *led_create_simple(Object *parentobj,
> +                            LEDColor color,
> +                            const char *description)
> +{
> +    g_autofree char *name = NULL;
> +    DeviceState *dev;
> +
> +    dev = qdev_new(TYPE_LED);
> +    qdev_prop_set_string(dev, "color", led_color_name[color]);
> +    if (!description) {
> +        static unsigned undescribed_led_id;
> +        name = g_strdup_printf("undescribed-led-#%u", undescribed_led_id++);
> +    } else {
> +        qdev_prop_set_string(dev, "description", description);
> +        name = g_ascii_strdown(description, -1);
> +        name = g_strdelimit(name, " #", '-');
> +    }
> +    object_property_add_child(parentobj, name, OBJECT(dev));
> +    qdev_realize_and_unref(dev, NULL, &error_fatal);
> +
> +    return LED(dev);
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b233da2a737..d040846b868 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1951,6 +1951,12 @@ F: docs/specs/vmgenid.txt
>   F: tests/qtest/vmgenid-test.c
>   F: stubs/vmgenid.c
>   
> +LED
> +M: Philippe Mathieu-Daudé <f4bug@amsat.org>
> +S: Maintained
> +F: include/hw/misc/led.h
> +F: hw/misc/led.c
> +
>   Unimplemented device
>   M: Peter Maydell <peter.maydell@linaro.org>
>   R: Philippe Mathieu-Daudé <f4bug@amsat.org>
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 92c397ca07a..5c151fa3a83 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -126,6 +126,9 @@ config AUX
>   config UNIMP
>       bool
>   
> +config LED
> +    bool
> +
>   config MAC_VIA
>       bool
>       select MOS6522
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index e1576b81cf9..26f6dd037dc 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -11,6 +11,7 @@ softmmu_ss.add(when: 'CONFIG_TMP105', if_true: files('tmp105.c'))
>   softmmu_ss.add(when: 'CONFIG_TMP421', if_true: files('tmp421.c'))
>   softmmu_ss.add(when: 'CONFIG_UNIMP', if_true: files('unimp.c'))
>   softmmu_ss.add(when: 'CONFIG_EMPTY_SLOT', if_true: files('empty_slot.c'))
> +softmmu_ss.add(when: 'CONFIG_LED', if_true: files('led.c'))
>   
>   # ARM devices
>   softmmu_ss.add(when: 'CONFIG_PL310', if_true: files('arm_l2x0.c'))
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 066752aa900..76c9ddb54fe 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -214,6 +214,9 @@ via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size
>   grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
>   grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
>   
> +# led.c
> +led_set_intensity(const char *color, const char *desc, uint8_t intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
> +
>   # pca9552.c
>   pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
>   pca955x_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
>

On 9/7/20 10:03 PM, Luc Michel wrote:
> Hi Philippe,
> 
> On 9/7/20 6:32 PM, Philippe Mathieu-Daudé wrote:
>> Add a LED device which can be connected to a GPIO output.
>> 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.
>>
>> LEDs are limited to a fixed set of colors.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/hw/misc/led.h |  84 +++++++++++++++++++++++++
>>   hw/misc/led.c         | 142 ++++++++++++++++++++++++++++++++++++++++++
>>   MAINTAINERS           |   6 ++
>>   hw/misc/Kconfig       |   3 +
>>   hw/misc/meson.build   |   1 +
>>   hw/misc/trace-events  |   3 +
>>   6 files changed, 239 insertions(+)
>>   create mode 100644 include/hw/misc/led.h
>>   create mode 100644 hw/misc/led.c
...
>> +/**
>> + * led_set_intensity: set the intensity of a LED device
>> + * @s: the LED object
>> + * @intensity: intensity as percentage in range 0 to 100.
> @intensity_percent
> 
>> + */
>> +void led_set_intensity(LEDState *s, unsigned intensity_percent);
>> +
>> +/**
>> + * led_get_intensity:
>> + * @s: the LED object
>> + *
>> + * Returns: The LED intensity as percentage in range 0 to 100.
>> + */
>> +unsigned led_get_intensity(LEDState *s);
>> +
>> +/**
>> + * led_set_intensity: set the state of a LED device
> led_set_state
> 
>> + * @s: the LED object
>> + * @is_on: boolean indicating whether the LED is emitting
> @is_emitting
> 
>> + *
>> + * This utility is meant for LED connected to GPIO.
>> + */
>> +void led_set_state(LEDState *s, bool is_emitting);
>> +
>> +/**
>> + * led_create_simple: Create and realize a LED device
>> + * @parent: the parent object
> @parentobj
> 
>> + * @color: color of the LED
>> + * @description: description of the LED (optional)
>> + *
>> + * Create the device state structure, initialize it, and
>> + * drop the reference to it (the device is realized).
>> + */
>> +LEDState *led_create_simple(Object *parentobj,
>> +                            LEDColor color,
>> +                            const char *description);
>> +
>> +#endif /* HW_MISC_LED_H */
>> diff --git a/hw/misc/led.c b/hw/misc/led.c
>> new file mode 100644
>> index 00000000000..f2140739b68
>> --- /dev/null
>> +++ b/hw/misc/led.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * QEMU single LED device
>> + *
>> + * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "migration/vmstate.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/misc/led.h"
>> +#include "trace.h"
>> +
>> +#define LED_INTENSITY_PERCENT_MAX   100
>> +
>> +static const char *led_color_name[] = {
>> +    [LED_COLOR_VIOLET]  = "violet",
>> +    [LED_COLOR_BLUE]    = "blue",
>> +    [LED_COLOR_CYAN]    = "cyan",
>> +    [LED_COLOR_GREEN]   = "green",
>> +    [LED_COLOR_AMBER]   = "amber",
>> +    [LED_COLOR_ORANGE]  = "orange",
>> +    [LED_COLOR_RED]     = "red",
>> +};
>> +
>> +static bool led_color_name_is_valid(const char *color_name)
>> +{
>> +    for (size_t i = 0; i < ARRAY_SIZE(led_color_name); i++) {
>> +        if (led_color_name[i] && strcmp(color_name,
>> led_color_name[i]) == 0) {
> 
> Why are you checking led_color_name[i] here?

It could happen in v3 but not now, thanks for catching this :)

I'll address your comment and respin after few days.

> 
> Otherwise, seems good to me.
> 
> Reviewed-by: Luc Michel <luc.michel@greensocs.com>

Thanks!