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é <f4...@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é <f4...@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.mic...@greensocs.com>
Thanks!
