| Message ID | 20200620230719.32139-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/misc: Add LED device | expand |
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~
Hi Richard, On 6/21/20 4:00 AM, Richard Henderson wrote: > 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? The idea of this device is to easily visualize events. Currently via tracing, but eventually an external UI could introspect the board for devices able to represent visual changes such LEDs, and automatically display them. To limit the list of representable object the visualizer has to support, I prefer to restrict this device to the existing LED physical colors. > >> +/* 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? I want to, but it seems having a enum qdev property involves a lot of code. > >> +/** >> + * 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? Yes. > >> +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? Because when we don't have access to the hardware schematics, we can not specify the label. I'll add a comment about this. > >> +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? Good catch... I will either use an enum propery, or check the property is in the allowed color names. > >> +# 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? You are right. I was thinking of PWM resolution (limiting to 16-bits). This is a different API to model, I mixed. > Is that more valuable than e.g. a percentage? Yes, the [0-100] range of integer is sufficient to represent light intensity =) Thanks for your review! Phil.
On 6/21/20 1:35 PM, Philippe Mathieu-Daudé wrote: >> Is color especially interesting, given that we only actually "display" the >> color via tracing? > > The idea of this device is to easily visualize events. Currently > via tracing, but eventually an external UI could introspect the > board for devices able to represent visual changes such LEDs, and > automatically display them. > To limit the list of representable object the visualizer has to > support, I prefer to restrict this device to the existing LED > physical colors. Ok. This does suggest that we do use then enum in the structure. >> 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? > > Because when we don't have access to the hardware schematics, > we can not specify the label. I'll add a comment about this. Well, if we don't have a hardware schematic label, then we must have the i/o pin; we could insist that the board label the led in some way that makes some sense. r~
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; + +/* 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; + /* + * When used with GPIO, the intensity at reset is related to GPIO polarity + */ + uint16_t reset_intensity; +} LEDState; + +/** + * led_set_intensity: set the intensity of a LED device + * @s: the LED object + * @intensity: new intensity + * + * This utility is meant for LED connected to PWM. + */ +void led_set_intensity(LEDState *s, uint16_t intensity); + +/** + * 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); + +/** + * create_led: create and LED device + * @parent: the parent object + * @color: color of the LED + * @description: description of the LED (optional) + * @reset_intensity: LED intensity at reset + * + * This utility function creates a LED object. + */ +DeviceState *create_led(Object *parentobj, + LEDColor color, + const char *description, + uint16_t reset_intensity); + +#endif /* HW_MISC_LED_H */ diff --git a/hw/misc/led.c b/hw/misc/led.c new file mode 100644 index 0000000000..e55ed7dbc4 --- /dev/null +++ b/hw/misc/led.c @@ -0,0 +1,121 @@ +/* + * 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" + +static const char *led_color(LEDColor color) +{ + static const char *color_name[LED_COLOR_COUNT] = { + [LED_COLOR_RED] = "red", + [LED_COLOR_ORANGE] = "orange", + [LED_COLOR_AMBER] = "amber", + [LED_COLOR_YELLOW] = "yellow", + [LED_COLOR_GREEN] = "green", + [LED_COLOR_BLUE] = "blue", + [LED_COLOR_VIOLET] = "violet", /* PURPLE */ + [LED_COLOR_WHITE] = "white", + }; + return color_name[color] ? color_name[color] : "unknown"; +} + +void led_set_intensity(LEDState *s, uint16_t new_intensity) +{ + trace_led_set_intensity(s->description ? s->description : "n/a", + s->color, new_intensity); + s->current_intensity = new_intensity; +} + +void led_set_state(LEDState *s, bool is_on) +{ + led_set_intensity(s, is_on ? UINT16_MAX : 0); +} + +static void led_reset(DeviceState *dev) +{ + LEDState *s = LED(dev); + + led_set_intensity(s, s->reset_intensity); +} + +static const VMStateDescription vmstate_led = { + .name = TYPE_LED, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + 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; + } +} + +static Property led_properties[] = { + DEFINE_PROP_STRING("color", LEDState, color), + DEFINE_PROP_STRING("description", LEDState, description), + DEFINE_PROP_UINT16("reset_intensity", LEDState, reset_intensity, 0), + 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) + +DeviceState *create_led(Object *parentobj, + LEDColor color, + const char *description, + uint16_t reset_intensity) +{ + DeviceState *dev; + char *name; + + assert(description); + dev = qdev_new(TYPE_LED); + qdev_prop_set_uint16(dev, "reset_intensity", reset_intensity); + qdev_prop_set_string(dev, "color", led_color(color)); + 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)); + g_free(name); + qdev_realize_and_unref(dev, NULL, &error_fatal); + + return dev; +} diff --git a/MAINTAINERS b/MAINTAINERS index 955cc8dd5c..0fb8896b43 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1863,6 +1863,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 bdd77d8020..f60dce694d 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/Makefile.objs b/hw/misc/Makefile.objs index 5aaca8a039..9efa3c941c 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -91,3 +91,4 @@ common-obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o obj-$(CONFIG_MAC_VIA) += mac_via.o common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o +common-obj-$(CONFIG_LED) += led.o diff --git a/hw/misc/trace-events b/hw/misc/trace-events index 805d2110e0..f58853d367 100644 --- a/hw/misc/trace-events +++ b/hw/misc/trace-events @@ -207,6 +207,9 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto 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, uint16_t intensity) "LED desc:'%s' color:%s intensity: 0x%04"PRIx16 + # pca9552.c pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]" pca9552_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
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