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

[Philippe Mathieu-Daudé]

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!