[PATCH v5 0/7] hw/misc: Add LED device

[PATCH v5 0/7] hw/misc: Add LED device

[Philippe Mathieu-Daudé]

Hello,

These patches are part of the GSoC unselected 'QEMU visualizer'
project.

This series introduce a LED device that can be easily connected
to a GPIO output.

Since v4:
- Fixed typos (Luc)
- Removed TYPE_TOSA_MISC_GPIO qdev conversion patch (Peter)

Since v3:
- Rebased (TYPE_TOSA_MISC_GPIO)
- Rebased (Meson)
- Addressed Richard's review comments
- Improved doc/comments

Since v2:
- Rebased on PCA9552
- Model intensity to be ready for PWM use (Dave)
- Remove QMP events until we get a UI visualizer (Peter)
- Remove microbit patch (Peter)

Since v1: addressed Eric Blake review comments
- Added QMP rate limit

Next steps planned:

- PoC visualizer...
- look at using a dbus backend (elmarco)
- look at LED array/matrix such 7segments.

Regards,

Phil.

Philippe Mathieu-Daudé (7):
  hw/misc/led: Add a LED device
  hw/misc/led: Allow connecting from GPIO output
  hw/misc/led: Emit a trace event when LED intensity has changed
  hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1
  hw/misc/mps2-fpgaio: Use the LED device
  hw/misc/mps2-scc: Use the LED device
  hw/arm/tosa: Replace fprintf() calls by LED devices

 include/hw/misc/led.h         |  92 +++++++++++++++++++
 include/hw/misc/mps2-fpgaio.h |   2 +
 include/hw/misc/mps2-scc.h    |   2 +
 include/hw/qdev-core.h        |   8 ++
 hw/arm/aspeed.c               |  20 +++++
 hw/arm/tosa.c                 |  40 ++++-----
 hw/misc/led.c                 | 161 ++++++++++++++++++++++++++++++++++
 hw/misc/mps2-fpgaio.c         |  19 ++--
 hw/misc/mps2-scc.c            |  25 +++---
 MAINTAINERS                   |   6 ++
 hw/arm/Kconfig                |   2 +
 hw/misc/Kconfig               |   5 ++
 hw/misc/meson.build           |   1 +
 hw/misc/trace-events          |   6 +-
 14 files changed, 346 insertions(+), 43 deletions(-)
 create mode 100644 include/hw/misc/led.h
 create mode 100644 hw/misc/led.c

-- 
2.26.2

[PATCH v5 1/7] hw/misc/led: Add a LED device

[Philippe Mathieu-Daudé]

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..f5afaa34bfb
--- /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_percent: intensity as percentage in range 0 to 100.
+ */
+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_state: Set the state of a LED device
+ * @s: the LED object
+ * @is_emitting: boolean indicating whether the LED 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
+ * @parentobj: the parent object
+ * @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..89acd9acbb7
--- /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 (strcmp(color_name, led_color_name[i]) == 0) {
+            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 7d0a5e91e4f..ca7f47aa936 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"
-- 
2.26.2

[PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

Some devices expose GPIO lines.

Add a GPIO qdev input to our LED device, so we can
connect a GPIO output using qdev_connect_gpio_out().

When used with GPIOs, the intensity can only be either
minium or maximum. This depends of the polarity of the
GPIO (which can be inverted).
Declare the GpioPolarity type to model the polarity.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/led.h  |  8 ++++++++
 include/hw/qdev-core.h |  8 ++++++++
 hw/misc/led.c          | 17 ++++++++++++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index f5afaa34bfb..71c9b8c59bf 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -38,10 +38,16 @@ typedef struct LEDState {
     /* Public */
 
     uint8_t intensity_percent;
+    qemu_irq irq;
 
     /* Properties */
     char *description;
     char *color;
+    /*
+     * When used with GPIO, the intensity at reset is related
+     * to the GPIO polarity.
+     */
+    bool inverted_polarity;
 } LEDState;
 
 /**
@@ -71,6 +77,7 @@ void led_set_state(LEDState *s, bool is_emitting);
 /**
  * led_create_simple: Create and realize a LED device
  * @parentobj: the parent object
+ * @gpio_polarity: GPIO polarity
  * @color: color of the LED
  * @description: description of the LED (optional)
  *
@@ -78,6 +85,7 @@ void led_set_state(LEDState *s, bool is_emitting);
  * drop the reference to it (the device is realized).
  */
 LEDState *led_create_simple(Object *parentobj,
+                            GpioPolarity gpio_polarity,
                             LEDColor color,
                             const char *description);
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ea3f73a282d..846354736a5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -424,6 +424,14 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
 void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
+/**
+ * GpioPolarity: Polarity of a GPIO line
+ */
+typedef enum {
+    GPIO_POLARITY_ACTIVE_LOW,
+    GPIO_POLARITY_ACTIVE_HIGH
+} GpioPolarity;
+
 /**
  * qdev_get_gpio_in: Get one of a device's anonymous input GPIO lines
  * @dev: Device whose GPIO we want
diff --git a/hw/misc/led.c b/hw/misc/led.c
index 89acd9acbb7..3ef4f5e0469 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -10,6 +10,7 @@
 #include "migration/vmstate.h"
 #include "hw/qdev-properties.h"
 #include "hw/misc/led.h"
+#include "hw/irq.h"
 #include "trace.h"
 
 #define LED_INTENSITY_PERCENT_MAX   100
@@ -53,11 +54,19 @@ void led_set_state(LEDState *s, bool is_emitting)
     led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
 }
 
+static void led_set_state_gpio_handler(void *opaque, int line, int new_state)
+{
+    LEDState *s = LED(opaque);
+
+    assert(line == 0);
+    led_set_state(s, !!new_state != s->inverted_polarity);
+}
+
 static void led_reset(DeviceState *dev)
 {
     LEDState *s = LED(dev);
 
-    led_set_state(s, false);
+    led_set_state(s, s->inverted_polarity);
 }
 
 static const VMStateDescription vmstate_led = {
@@ -84,11 +93,14 @@ static void led_realize(DeviceState *dev, Error **errp)
     if (s->description == NULL) {
         s->description = g_strdup("n/a");
     }
+
+    qdev_init_gpio_in(DEVICE(s), led_set_state_gpio_handler, 1);
 }
 
 static Property led_properties[] = {
     DEFINE_PROP_STRING("color", LEDState, color),
     DEFINE_PROP_STRING("description", LEDState, description),
+    DEFINE_PROP_BOOL("polarity-inverted", LEDState, inverted_polarity, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -119,6 +131,7 @@ static void led_register_types(void)
 type_init(led_register_types)
 
 LEDState *led_create_simple(Object *parentobj,
+                            GpioPolarity gpio_polarity,
                             LEDColor color,
                             const char *description)
 {
@@ -126,6 +139,8 @@ LEDState *led_create_simple(Object *parentobj,
     DeviceState *dev;
 
     dev = qdev_new(TYPE_LED);
+    qdev_prop_set_bit(dev, "polarity-inverted",
+                      gpio_polarity == GPIO_POLARITY_ACTIVE_LOW);
     qdev_prop_set_string(dev, "color", led_color_name[color]);
     if (!description) {
         static unsigned undescribed_led_id;
-- 
2.26.2

[PATCH v5 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Philippe Mathieu-Daudé]

Track the LED intensity, and emit a trace event when it changes.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/led.c        | 4 ++++
 hw/misc/trace-events | 1 +
 2 files changed, 5 insertions(+)

diff --git a/hw/misc/led.c b/hw/misc/led.c
index 3ef4f5e0469..ebe1fa45b1e 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -41,6 +41,10 @@ void led_set_intensity(LEDState *s, unsigned intensity_percent)
         intensity_percent = LED_INTENSITY_PERCENT_MAX;
     }
     trace_led_set_intensity(s->description, s->color, intensity_percent);
+    if (intensity_percent != s->intensity_percent) {
+        trace_led_change_intensity(s->description, s->color,
+                                   s->intensity_percent, intensity_percent);
+    }
     s->intensity_percent = intensity_percent;
 }
 
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 76c9ddb54fe..89d15f05f9a 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -216,6 +216,7 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
 
 # led.c
 led_set_intensity(const char *color, const char *desc, uint8_t intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
+led_change_intensity(const char *color, const char *desc, uint8_t old_intensity_percent, uint8_t new_intensity_percent) "LED desc:'%s' color:%s intensity %u%% -> %u%%"
 
 # pca9552.c
 pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
-- 
2.26.2

[PATCH v5 4/7] hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1

[Philippe Mathieu-Daudé]

The Witherspoon has 3 LEDs connected to a PCA9552. Add them.
The names and reset values are taken from:
https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace led_change_intensity
  1592693373.997015:led_change_intensity LED desc:'front-fault-4' color:green intensity 0% -> 100%
  1592693373.997632:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
  1592693373.998239:led_change_intensity LED desc:'front-id-5' color:green intensity 0% -> 100%
  1592693500.291805:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
  1592693500.312041:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
  1592693500.821254:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
  1592693501.331517:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
  1592693501.841367:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
  1592693502.350839:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
  1592693502.861134:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
  1592693503.371090:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%

We notice the front-power LED starts to blink at a ~2Hz rate.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/aspeed.c | 20 ++++++++++++++++++++
 hw/arm/Kconfig  |  1 +
 2 files changed, 21 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 8bfb1c79ddc..83e322ea983 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -20,6 +20,7 @@
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/tmp105.h"
+#include "hw/misc/led.h"
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
 #include "sysemu/block-backend.h"
@@ -521,9 +522,20 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
 
 static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
 {
+    static const struct {
+        unsigned gpio_id;
+        LEDColor color;
+        const char *description;
+        bool gpio_polarity;
+    } pca1_leds[] = {
+        {13, LED_COLOR_GREEN, "front-fault-4",  GPIO_POLARITY_ACTIVE_LOW},
+        {14, LED_COLOR_GREEN, "front-power-3",  GPIO_POLARITY_ACTIVE_LOW},
+        {15, LED_COLOR_GREEN, "front-id-5",     GPIO_POLARITY_ACTIVE_LOW},
+    };
     AspeedSoCState *soc = &bmc->soc;
     uint8_t *eeprom_buf = g_malloc0(8 * 1024);
     DeviceState *dev;
+    LEDState *led;
 
     /* Bus 3: TODO bmp280@77 */
     /* Bus 3: TODO max31785@52 */
@@ -534,6 +546,14 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
                                 aspeed_i2c_get_bus(&soc->i2c, 3),
                                 &error_fatal);
 
+    for (size_t i = 0; i < ARRAY_SIZE(pca1_leds); i++) {
+        led = led_create_simple(OBJECT(bmc),
+                                pca1_leds[i].gpio_polarity,
+                                pca1_leds[i].color,
+                                pca1_leds[i].description);
+        qdev_connect_gpio_out(dev, pca1_leds[i].gpio_id,
+                              qdev_get_gpio_in(DEVICE(led), 0));
+    }
     i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
     i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index bc3a423940b..06ba1c355b1 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -394,6 +394,7 @@ config ASPEED_SOC
     select TMP105
     select TMP421
     select UNIMP
+    select LED
 
 config MPS2
     bool
-- 
2.26.2

[PATCH v5 5/7] hw/misc/mps2-fpgaio: Use the LED device

[Philippe Mathieu-Daudé]

Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
Reference Manual' (100112_0200_07_en):

  2.1  Overview of the MPS2 and MPS2+ hardware

       The MPS2 and MPS2+ FPGA Prototyping Boards contain the
       following components and interfaces:

       * User switches and user LEDs:

         - Two green LEDs and two push buttons that connect to
           the FPGA.
         - Eight green LEDs and one 8-way dip switch that connect
           to the MCC.

Add the 2 LEDs connected to the FPGA.

This remplaces the 'mps2_fpgaio_leds' trace events by the generic
'led_set_intensity' event.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/mps2-fpgaio.h |  2 ++
 hw/misc/mps2-fpgaio.c         | 19 ++++++++++++++-----
 hw/misc/Kconfig               |  1 +
 hw/misc/trace-events          |  1 -
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h
index 69e265cd4b2..901880cc3a7 100644
--- a/include/hw/misc/mps2-fpgaio.h
+++ b/include/hw/misc/mps2-fpgaio.h
@@ -22,6 +22,7 @@
 #define MPS2_FPGAIO_H
 
 #include "hw/sysbus.h"
+#include "hw/misc/led.h"
 
 #define TYPE_MPS2_FPGAIO "mps2-fpgaio"
 #define MPS2_FPGAIO(obj) OBJECT_CHECK(MPS2FPGAIO, (obj), TYPE_MPS2_FPGAIO)
@@ -32,6 +33,7 @@ typedef struct {
 
     /*< public >*/
     MemoryRegion iomem;
+    LEDState *led[2];
 
     uint32_t led0;
     uint32_t prescale;
diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
index 2f3fbeef348..86ca78eb235 100644
--- a/hw/misc/mps2-fpgaio.c
+++ b/hw/misc/mps2-fpgaio.c
@@ -24,6 +24,7 @@
 #include "migration/vmstate.h"
 #include "hw/registerfields.h"
 #include "hw/misc/mps2-fpgaio.h"
+#include "hw/misc/led.h"
 #include "hw/qdev-properties.h"
 #include "qemu/timer.h"
 
@@ -176,12 +177,9 @@ static void mps2_fpgaio_write(void *opaque, hwaddr offset, uint64_t value,
 
     switch (offset) {
     case A_LED0:
-        /* LED bits [1:0] control board LEDs. We don't currently have
-         * a mechanism for displaying this graphically, so use a trace event.
-         */
-        trace_mps2_fpgaio_leds(value & 0x02 ? '*' : '.',
-                               value & 0x01 ? '*' : '.');
         s->led0 = value & 0x3;
+        led_set_state(s->led[0], value & 0x01);
+        led_set_state(s->led[1], value & 0x02);
         break;
     case A_PRESCALE:
         resync_counter(s);
@@ -251,6 +249,16 @@ static void mps2_fpgaio_init(Object *obj)
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
+static void mps2_fpgaio_realize(DeviceState *dev, Error **errp)
+{
+    MPS2FPGAIO *s = MPS2_FPGAIO(dev);
+
+    s->led[0] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
+                                  LED_COLOR_GREEN, "USERLED0");
+    s->led[1] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
+                                  LED_COLOR_GREEN, "USERLED1");
+}
+
 static bool mps2_fpgaio_counters_needed(void *opaque)
 {
     /* Currently vmstate.c insists all subsections have a 'needed' function */
@@ -299,6 +307,7 @@ static void mps2_fpgaio_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &mps2_fpgaio_vmstate;
+    dc->realize = mps2_fpgaio_realize;
     dc->reset = mps2_fpgaio_reset;
     device_class_set_props(dc, mps2_fpgaio_properties);
 }
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 5c151fa3a83..0cecad45aad 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -93,6 +93,7 @@ config MIPS_ITU
 
 config MPS2_FPGAIO
     bool
+    select LED
 
 config MPS2_SCC
     bool
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 89d15f05f9a..43b9e0cf250 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -93,7 +93,6 @@ mps2_scc_cfg_read(unsigned function, unsigned device, uint32_t value) "MPS2 SCC
 mps2_fpgaio_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 FPGAIO read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_fpgaio_write(uint64_t offset, uint64_t data, unsigned size) "MPS2 FPGAIO write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_fpgaio_reset(void) "MPS2 FPGAIO: reset"
-mps2_fpgaio_leds(char led1, char led0) "MPS2 FPGAIO LEDs: %c%c"
 
 # msf2-sysreg.c
 msf2_sysreg_write(uint64_t offset, uint32_t val, uint32_t prev) "msf2-sysreg write: addr 0x%08" PRIx64 " data 0x%" PRIx32 " prev 0x%" PRIx32
-- 
2.26.2

[PATCH v5 6/7] hw/misc/mps2-scc: Use the LED device

[Philippe Mathieu-Daudé]

Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
Reference Manual' (100112_0200_07_en):

  2.1  Overview of the MPS2 and MPS2+ hardware

       The MPS2 and MPS2+ FPGA Prototyping Boards contain the
       following components and interfaces:

       * User switches and user LEDs:

         - Two green LEDs and two push buttons that connect to
           the FPGA.
         - Eight green LEDs and one 8-way dip switch that connect
           to the MCC.

Add the 8 LEDs connected to the MCC.

This remplaces the 'mps2_scc_leds' trace events by the generic
'led_set_intensity' event.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
https://youtu.be/l9kD70uPchk?t=288
---
 include/hw/misc/mps2-scc.h |  2 ++
 hw/misc/mps2-scc.c         | 25 ++++++++++++++-----------
 hw/misc/Kconfig            |  1 +
 hw/misc/trace-events       |  1 -
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/hw/misc/mps2-scc.h b/include/hw/misc/mps2-scc.h
index 7045473788b..8542f384227 100644
--- a/include/hw/misc/mps2-scc.h
+++ b/include/hw/misc/mps2-scc.h
@@ -13,6 +13,7 @@
 #define MPS2_SCC_H
 
 #include "hw/sysbus.h"
+#include "hw/misc/led.h"
 
 #define TYPE_MPS2_SCC "mps2-scc"
 #define MPS2_SCC(obj) OBJECT_CHECK(MPS2SCC, (obj), TYPE_MPS2_SCC)
@@ -25,6 +26,7 @@ typedef struct {
 
     /*< public >*/
     MemoryRegion iomem;
+    LEDState *led[8];
 
     uint32_t cfg0;
     uint32_t cfg1;
diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
index 9d0909e7b35..745505b849d 100644
--- a/hw/misc/mps2-scc.c
+++ b/hw/misc/mps2-scc.c
@@ -20,11 +20,13 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/bitops.h"
 #include "trace.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "hw/registerfields.h"
 #include "hw/misc/mps2-scc.h"
+#include "hw/misc/led.h"
 #include "hw/qdev-properties.h"
 
 REG32(CFG0, 0)
@@ -152,18 +154,10 @@ static void mps2_scc_write(void *opaque, hwaddr offset, uint64_t value,
         s->cfg0 = value;
         break;
     case A_CFG1:
-        /* CFG1 bits [7:0] control the board LEDs. We don't currently have
-         * a mechanism for displaying this graphically, so use a trace event.
-         */
-        trace_mps2_scc_leds(value & 0x80 ? '*' : '.',
-                            value & 0x40 ? '*' : '.',
-                            value & 0x20 ? '*' : '.',
-                            value & 0x10 ? '*' : '.',
-                            value & 0x08 ? '*' : '.',
-                            value & 0x04 ? '*' : '.',
-                            value & 0x02 ? '*' : '.',
-                            value & 0x01 ? '*' : '.');
         s->cfg1 = value;
+        for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
+            led_set_state(s->led[i], extract32(value, i, 1));
+        }
         break;
     case A_CFGDATA_OUT:
         s->cfgdata_out = value;
@@ -245,10 +239,19 @@ static void mps2_scc_init(Object *obj)
 
     memory_region_init_io(&s->iomem, obj, &mps2_scc_ops, s, "mps2-scc", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
+
 }
 
 static void mps2_scc_realize(DeviceState *dev, Error **errp)
 {
+    MPS2SCC *s = MPS2_SCC(dev);
+
+    for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
+        char *name = g_strdup_printf("SCC LED%zu", i);
+        s->led[i] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
+                                      LED_COLOR_GREEN, name);
+        g_free(name);
+    }
 }
 
 static const VMStateDescription mps2_scc_vmstate = {
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 0cecad45aad..7557a3e7b46 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -97,6 +97,7 @@ config MPS2_FPGAIO
 
 config MPS2_SCC
     bool
+    select LED
 
 config TZ_MPC
     bool
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 43b9e0cf250..a620a358feb 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -85,7 +85,6 @@ aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64
 mps2_scc_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_scc_write(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_scc_reset(void) "MPS2 SCC: reset"
-mps2_scc_leds(char led7, char led6, char led5, char led4, char led3, char led2, char led1, char led0) "MPS2 SCC LEDs: %c%c%c%c%c%c%c%c"
 mps2_scc_cfg_write(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config write: function %d device %d data 0x%" PRIx32
 mps2_scc_cfg_read(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config read: function %d device %d data 0x%" PRIx32
 
-- 
2.26.2

[PATCH v5 7/7] hw/arm/tosa: Replace fprintf() calls by LED devices

[Philippe Mathieu-Daudé]

The recently added LED device reports LED status changes with
the 'led_set_intensity' trace event. It is less invasive than
the fprintf() calls. We need however to have a binary built
with tracing support.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/tosa.c  | 40 +++++++++++++++-------------------------
 hw/arm/Kconfig |  1 +
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 90eef1f14dd..f23651fd775 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -24,6 +24,7 @@
 #include "hw/irq.h"
 #include "hw/ssi/ssi.h"
 #include "hw/sysbus.h"
+#include "hw/misc/led.h"
 #include "exec/address-spaces.h"
 
 #define TOSA_RAM 0x04000000
@@ -81,26 +82,6 @@ typedef struct TosaMiscGPIOState {
     SysBusDevice parent_obj;
 } TosaMiscGPIOState;
 
-static void tosa_gpio_leds(void *opaque, int line, int level)
-{
-    switch (line) {
-    case 0:
-        fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
-        break;
-    case 1:
-        fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
-        break;
-    case 2:
-        fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
-        break;
-    case 3:
-        fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
-        break;
-    default:
-        g_assert_not_reached();
-    }
-}
-
 static void tosa_reset(void *opaque, int line, int level)
 {
     if (level) {
@@ -112,7 +93,6 @@ static void tosa_misc_gpio_init(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
 
-    qdev_init_gpio_in_named(dev, tosa_gpio_leds, "leds", 4);
     qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
 }
 
@@ -122,6 +102,7 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
                 TC6393xbState *tmio)
 {
     DeviceState *misc_gpio;
+    LEDState *led[4];
 
     misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
 
@@ -143,14 +124,23 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
                         qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
                         NULL);
 
+    led[0] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+                               LED_COLOR_BLUE, "bluetooth");
+    led[1] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+                               LED_COLOR_GREEN, "note");
+    led[2] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+                               LED_COLOR_AMBER, "charger-error");
+    led[3] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+                               LED_COLOR_GREEN, "wlan");
+
     qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED,
-                          qdev_get_gpio_in_named(misc_gpio, "leds", 0));
+                          qdev_get_gpio_in(DEVICE(led[0]), 0));
     qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED,
-                          qdev_get_gpio_in_named(misc_gpio, "leds", 1));
+                          qdev_get_gpio_in(DEVICE(led[1]), 0));
     qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED,
-                          qdev_get_gpio_in_named(misc_gpio, "leds", 2));
+                          qdev_get_gpio_in(DEVICE(led[2]), 0));
     qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED,
-                          qdev_get_gpio_in_named(misc_gpio, "leds", 3));
+                          qdev_get_gpio_in(DEVICE(led[3]), 0));
 
     qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, tc6393xb_l3v_get(tmio));
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 06ba1c355b1..bbcfa098ae2 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -150,6 +150,7 @@ config TOSA
     select ZAURUS  # scoop
     select MICRODRIVE
     select PXA2XX
+    select LED
 
 config SPITZ
     bool
-- 
2.26.2

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Luc Michel]

Hi Phil,

On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
> Some devices expose GPIO lines.
> 
> Add a GPIO qdev input to our LED device, so we can
> connect a GPIO output using qdev_connect_gpio_out().
> 
> When used with GPIOs, the intensity can only be either
> minium or maximum. This depends of the polarity of the
> GPIO (which can be inverted).
> Declare the GpioPolarity type to model the polarity.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/misc/led.h  |  8 ++++++++
>   include/hw/qdev-core.h |  8 ++++++++
>   hw/misc/led.c          | 17 ++++++++++++++++-
>   3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> index f5afaa34bfb..71c9b8c59bf 100644
> --- a/include/hw/misc/led.h
> +++ b/include/hw/misc/led.h
> @@ -38,10 +38,16 @@ typedef struct LEDState {
>       /* Public */
>   
>       uint8_t intensity_percent;
> +    qemu_irq irq;
>   
>       /* Properties */
>       char *description;
>       char *color;
> +    /*
> +     * When used with GPIO, the intensity at reset is related
> +     * to the GPIO polarity.
> +     */
> +    bool inverted_polarity;
>   } LEDState;
>   
>   /**
> @@ -71,6 +77,7 @@ void led_set_state(LEDState *s, bool is_emitting);
>   /**
>    * led_create_simple: Create and realize a LED device
>    * @parentobj: the parent object
> + * @gpio_polarity: GPIO polarity
>    * @color: color of the LED
>    * @description: description of the LED (optional)
>    *
> @@ -78,6 +85,7 @@ void led_set_state(LEDState *s, bool is_emitting);
>    * drop the reference to it (the device is realized).
>    */
>   LEDState *led_create_simple(Object *parentobj,
> +                            GpioPolarity gpio_polarity,
>                               LEDColor color,
>                               const char *description);
>   
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index ea3f73a282d..846354736a5 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -424,6 +424,14 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>   void qdev_machine_creation_done(void);
>   bool qdev_machine_modified(void);
>   
> +/**
> + * GpioPolarity: Polarity of a GPIO line
> + */
> +typedef enum {
> +    GPIO_POLARITY_ACTIVE_LOW,
> +    GPIO_POLARITY_ACTIVE_HIGH
> +} GpioPolarity;
> +
>   /**
>    * qdev_get_gpio_in: Get one of a device's anonymous input GPIO lines
>    * @dev: Device whose GPIO we want
> diff --git a/hw/misc/led.c b/hw/misc/led.c
> index 89acd9acbb7..3ef4f5e0469 100644
> --- a/hw/misc/led.c
> +++ b/hw/misc/led.c
> @@ -10,6 +10,7 @@
>   #include "migration/vmstate.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/misc/led.h"
> +#include "hw/irq.h"
>   #include "trace.h"
>   
>   #define LED_INTENSITY_PERCENT_MAX   100
> @@ -53,11 +54,19 @@ void led_set_state(LEDState *s, bool is_emitting)
>       led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
>   }
>   
> +static void led_set_state_gpio_handler(void *opaque, int line, int new_state)
> +{
> +    LEDState *s = LED(opaque);
> +
> +    assert(line == 0);
> +    led_set_state(s, !!new_state != s->inverted_polarity);
> +}
> +
>   static void led_reset(DeviceState *dev)
>   {
>       LEDState *s = LED(dev);
>   
> -    led_set_state(s, false);
> +    led_set_state(s, s->inverted_polarity);
>   }
>   
>   static const VMStateDescription vmstate_led = {
> @@ -84,11 +93,14 @@ static void led_realize(DeviceState *dev, Error **errp)
>       if (s->description == NULL) {
>           s->description = g_strdup("n/a");
>       }
> +
> +    qdev_init_gpio_in(DEVICE(s), led_set_state_gpio_handler, 1);
>   }
>   
>   static Property led_properties[] = {
>       DEFINE_PROP_STRING("color", LEDState, color),
>       DEFINE_PROP_STRING("description", LEDState, description),
> +    DEFINE_PROP_BOOL("polarity-inverted", LEDState, inverted_polarity, false),
I was wondering, since the GpioPolarity type you introduce is not used 
in the GPIO API, and since you use a boolean property here. Wouldn't it 
be clearer is you name this property "active-low"? Because 
"polarity-inverted" doesn't tell what the polarity is in the first 
place. Moreover since this property only affects the LED GPIO, and not 
the LED API (led_set_state), I think you can even name it 
"gpio-active-low" to make this clear.

>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -119,6 +131,7 @@ static void led_register_types(void)
>   type_init(led_register_types)
>   
>   LEDState *led_create_simple(Object *parentobj,
> +                            GpioPolarity gpio_polarity,
You could go with a boolean here also and name the parameter 
gpio_active_low, but I don't have a strong opinion on this.

So with or without those modifications:
Reviewed-by: Luc Michel <luc.michel@greensocs.com>

>                               LEDColor color,
>                               const char *description)
>   {
> @@ -126,6 +139,8 @@ LEDState *led_create_simple(Object *parentobj,
>       DeviceState *dev;
>   
>       dev = qdev_new(TYPE_LED);
> +    qdev_prop_set_bit(dev, "polarity-inverted",
> +                      gpio_polarity == GPIO_POLARITY_ACTIVE_LOW);
>       qdev_prop_set_string(dev, "color", led_color_name[color]);
>       if (!description) {
>           static unsigned undescribed_led_id;
> 

Re: [PATCH v5 1/7] hw/misc/led: Add a LED device

[Luc Michel]

On 9/10/20 10:54 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>

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

> ---
>   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..f5afaa34bfb
> --- /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_percent: intensity as percentage in range 0 to 100.
> + */
> +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_state: Set the state of a LED device
> + * @s: the LED object
> + * @is_emitting: boolean indicating whether the LED 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
> + * @parentobj: the parent object
> + * @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..89acd9acbb7
> --- /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 (strcmp(color_name, led_color_name[i]) == 0) {
> +            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 7d0a5e91e4f..ca7f47aa936 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"
> 

Re: [PATCH v5 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Luc Michel]

On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
> Track the LED intensity, and emit a trace event when it changes.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

> ---
>   hw/misc/led.c        | 4 ++++
>   hw/misc/trace-events | 1 +
>   2 files changed, 5 insertions(+)
> 
> diff --git a/hw/misc/led.c b/hw/misc/led.c
> index 3ef4f5e0469..ebe1fa45b1e 100644
> --- a/hw/misc/led.c
> +++ b/hw/misc/led.c
> @@ -41,6 +41,10 @@ void led_set_intensity(LEDState *s, unsigned intensity_percent)
>           intensity_percent = LED_INTENSITY_PERCENT_MAX;
>       }
>       trace_led_set_intensity(s->description, s->color, intensity_percent);
> +    if (intensity_percent != s->intensity_percent) {
> +        trace_led_change_intensity(s->description, s->color,
> +                                   s->intensity_percent, intensity_percent);
> +    }
>       s->intensity_percent = intensity_percent;
>   }
>   
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 76c9ddb54fe..89d15f05f9a 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -216,6 +216,7 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
>   
>   # led.c
>   led_set_intensity(const char *color, const char *desc, uint8_t intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
> +led_change_intensity(const char *color, const char *desc, uint8_t old_intensity_percent, uint8_t new_intensity_percent) "LED desc:'%s' color:%s intensity %u%% -> %u%%"
>   
>   # pca9552.c
>   pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
> 

Re: [PATCH v5 7/7] hw/arm/tosa: Replace fprintf() calls by LED devices

[Luc Michel]

On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
> The recently added LED device reports LED status changes with
> the 'led_set_intensity' trace event. It is less invasive than
> the fprintf() calls. We need however to have a binary built
> with tracing support.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

> ---
>   hw/arm/tosa.c  | 40 +++++++++++++++-------------------------
>   hw/arm/Kconfig |  1 +
>   2 files changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 90eef1f14dd..f23651fd775 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -24,6 +24,7 @@
>   #include "hw/irq.h"
>   #include "hw/ssi/ssi.h"
>   #include "hw/sysbus.h"
> +#include "hw/misc/led.h"
>   #include "exec/address-spaces.h"
>   
>   #define TOSA_RAM 0x04000000
> @@ -81,26 +82,6 @@ typedef struct TosaMiscGPIOState {
>       SysBusDevice parent_obj;
>   } TosaMiscGPIOState;
>   
> -static void tosa_gpio_leds(void *opaque, int line, int level)
> -{
> -    switch (line) {
> -    case 0:
> -        fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
> -        break;
> -    case 1:
> -        fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
> -        break;
> -    case 2:
> -        fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
> -        break;
> -    case 3:
> -        fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
> -        break;
> -    default:
> -        g_assert_not_reached();
> -    }
> -}
> -
>   static void tosa_reset(void *opaque, int line, int level)
>   {
>       if (level) {
> @@ -112,7 +93,6 @@ static void tosa_misc_gpio_init(Object *obj)
>   {
>       DeviceState *dev = DEVICE(obj);
>   
> -    qdev_init_gpio_in_named(dev, tosa_gpio_leds, "leds", 4);
>       qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
>   }
>   
> @@ -122,6 +102,7 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
>                   TC6393xbState *tmio)
>   {
>       DeviceState *misc_gpio;
> +    LEDState *led[4];
>   
>       misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
>   
> @@ -143,14 +124,23 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
>                           qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
>                           NULL);
>   
> +    led[0] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
> +                               LED_COLOR_BLUE, "bluetooth");
> +    led[1] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
> +                               LED_COLOR_GREEN, "note");
> +    led[2] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
> +                               LED_COLOR_AMBER, "charger-error");
> +    led[3] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
> +                               LED_COLOR_GREEN, "wlan");
> +
>       qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED,
> -                          qdev_get_gpio_in_named(misc_gpio, "leds", 0));
> +                          qdev_get_gpio_in(DEVICE(led[0]), 0));
>       qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED,
> -                          qdev_get_gpio_in_named(misc_gpio, "leds", 1));
> +                          qdev_get_gpio_in(DEVICE(led[1]), 0));
>       qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED,
> -                          qdev_get_gpio_in_named(misc_gpio, "leds", 2));
> +                          qdev_get_gpio_in(DEVICE(led[2]), 0));
>       qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED,
> -                          qdev_get_gpio_in_named(misc_gpio, "leds", 3));
> +                          qdev_get_gpio_in(DEVICE(led[3]), 0));
>   
>       qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, tc6393xb_l3v_get(tmio));
>   
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 06ba1c355b1..bbcfa098ae2 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -150,6 +150,7 @@ config TOSA
>       select ZAURUS  # scoop
>       select MICRODRIVE
>       select PXA2XX
> +    select LED
>   
>   config SPITZ
>       bool
> 

Re: [PATCH v5 4/7] hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1

[Luc Michel]

On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
> The Witherspoon has 3 LEDs connected to a PCA9552. Add them.
> The names and reset values are taken from:
> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
> 
> Example booting obmc-phosphor-image:
> 
>    $ qemu-system-arm -M witherspoon-bmc -trace led_change_intensity
>    1592693373.997015:led_change_intensity LED desc:'front-fault-4' color:green intensity 0% -> 100%
>    1592693373.997632:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
>    1592693373.998239:led_change_intensity LED desc:'front-id-5' color:green intensity 0% -> 100%
>    1592693500.291805:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
>    1592693500.312041:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
>    1592693500.821254:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
>    1592693501.331517:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
>    1592693501.841367:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
>    1592693502.350839:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
>    1592693502.861134:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
>    1592693503.371090:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
> 
> We notice the front-power LED starts to blink at a ~2Hz rate.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

> ---
>   hw/arm/aspeed.c | 20 ++++++++++++++++++++
>   hw/arm/Kconfig  |  1 +
>   2 files changed, 21 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 8bfb1c79ddc..83e322ea983 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -20,6 +20,7 @@
>   #include "hw/i2c/smbus_eeprom.h"
>   #include "hw/misc/pca9552.h"
>   #include "hw/misc/tmp105.h"
> +#include "hw/misc/led.h"
>   #include "hw/qdev-properties.h"
>   #include "qemu/log.h"
>   #include "sysemu/block-backend.h"
> @@ -521,9 +522,20 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
>   
>   static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>   {
> +    static const struct {
> +        unsigned gpio_id;
> +        LEDColor color;
> +        const char *description;
> +        bool gpio_polarity;
> +    } pca1_leds[] = {
> +        {13, LED_COLOR_GREEN, "front-fault-4",  GPIO_POLARITY_ACTIVE_LOW},
> +        {14, LED_COLOR_GREEN, "front-power-3",  GPIO_POLARITY_ACTIVE_LOW},
> +        {15, LED_COLOR_GREEN, "front-id-5",     GPIO_POLARITY_ACTIVE_LOW},
> +    };
>       AspeedSoCState *soc = &bmc->soc;
>       uint8_t *eeprom_buf = g_malloc0(8 * 1024);
>       DeviceState *dev;
> +    LEDState *led;
>   
>       /* Bus 3: TODO bmp280@77 */
>       /* Bus 3: TODO max31785@52 */
> @@ -534,6 +546,14 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>                                   aspeed_i2c_get_bus(&soc->i2c, 3),
>                                   &error_fatal);
>   
> +    for (size_t i = 0; i < ARRAY_SIZE(pca1_leds); i++) {
> +        led = led_create_simple(OBJECT(bmc),
> +                                pca1_leds[i].gpio_polarity,
> +                                pca1_leds[i].color,
> +                                pca1_leds[i].description);
> +        qdev_connect_gpio_out(dev, pca1_leds[i].gpio_id,
> +                              qdev_get_gpio_in(DEVICE(led), 0));
> +    }
>       i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
>       i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
>   
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index bc3a423940b..06ba1c355b1 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -394,6 +394,7 @@ config ASPEED_SOC
>       select TMP105
>       select TMP421
>       select UNIMP
> +    select LED
>   
>   config MPS2
>       bool
> 

Re: [PATCH v5 5/7] hw/misc/mps2-fpgaio: Use the LED device

[Luc Michel]

Hi Phil,

On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
> Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
> Reference Manual' (100112_0200_07_en):
> 
>    2.1  Overview of the MPS2 and MPS2+ hardware
> 
>         The MPS2 and MPS2+ FPGA Prototyping Boards contain the
>         following components and interfaces:
> 
>         * User switches and user LEDs:
> 
>           - Two green LEDs and two push buttons that connect to
>             the FPGA.
>           - Eight green LEDs and one 8-way dip switch that connect
>             to the MCC.
> 
> Add the 2 LEDs connected to the FPGA.
> 
> This remplaces the 'mps2_fpgaio_leds' trace events by the generic
replaces
> 'led_set_intensity' event.

If I'm not mistaken the LED device being a DEVICE and not a 
SYS_BUS_DEVICE, it needs to be manually reset. So you probably need to 
reset it in mps2_fpgaio_reset so it doesn't get out of sync on system reset.

-- 
Luc

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/misc/mps2-fpgaio.h |  2 ++
>   hw/misc/mps2-fpgaio.c         | 19 ++++++++++++++-----
>   hw/misc/Kconfig               |  1 +
>   hw/misc/trace-events          |  1 -
>   4 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h
> index 69e265cd4b2..901880cc3a7 100644
> --- a/include/hw/misc/mps2-fpgaio.h
> +++ b/include/hw/misc/mps2-fpgaio.h
> @@ -22,6 +22,7 @@
>   #define MPS2_FPGAIO_H
>   
>   #include "hw/sysbus.h"
> +#include "hw/misc/led.h"
>   
>   #define TYPE_MPS2_FPGAIO "mps2-fpgaio"
>   #define MPS2_FPGAIO(obj) OBJECT_CHECK(MPS2FPGAIO, (obj), TYPE_MPS2_FPGAIO)
> @@ -32,6 +33,7 @@ typedef struct {
>   
>       /*< public >*/
>       MemoryRegion iomem;
> +    LEDState *led[2];
>   
>       uint32_t led0;
>       uint32_t prescale;
> diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
> index 2f3fbeef348..86ca78eb235 100644
> --- a/hw/misc/mps2-fpgaio.c
> +++ b/hw/misc/mps2-fpgaio.c
> @@ -24,6 +24,7 @@
>   #include "migration/vmstate.h"
>   #include "hw/registerfields.h"
>   #include "hw/misc/mps2-fpgaio.h"
> +#include "hw/misc/led.h"
>   #include "hw/qdev-properties.h"
>   #include "qemu/timer.h"
>   
> @@ -176,12 +177,9 @@ static void mps2_fpgaio_write(void *opaque, hwaddr offset, uint64_t value,
>   
>       switch (offset) {
>       case A_LED0:
> -        /* LED bits [1:0] control board LEDs. We don't currently have
> -         * a mechanism for displaying this graphically, so use a trace event.
> -         */
> -        trace_mps2_fpgaio_leds(value & 0x02 ? '*' : '.',
> -                               value & 0x01 ? '*' : '.');
>           s->led0 = value & 0x3;
> +        led_set_state(s->led[0], value & 0x01);
> +        led_set_state(s->led[1], value & 0x02);
>           break;
>       case A_PRESCALE:
>           resync_counter(s);
> @@ -251,6 +249,16 @@ static void mps2_fpgaio_init(Object *obj)
>       sysbus_init_mmio(sbd, &s->iomem);
>   }
>   
> +static void mps2_fpgaio_realize(DeviceState *dev, Error **errp)
> +{
> +    MPS2FPGAIO *s = MPS2_FPGAIO(dev);
> +
> +    s->led[0] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
> +                                  LED_COLOR_GREEN, "USERLED0");
> +    s->led[1] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
> +                                  LED_COLOR_GREEN, "USERLED1");
> +}
> +
>   static bool mps2_fpgaio_counters_needed(void *opaque)
>   {
>       /* Currently vmstate.c insists all subsections have a 'needed' function */
> @@ -299,6 +307,7 @@ static void mps2_fpgaio_class_init(ObjectClass *klass, void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       dc->vmsd = &mps2_fpgaio_vmstate;
> +    dc->realize = mps2_fpgaio_realize;
>       dc->reset = mps2_fpgaio_reset;
>       device_class_set_props(dc, mps2_fpgaio_properties);
>   }
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 5c151fa3a83..0cecad45aad 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -93,6 +93,7 @@ config MIPS_ITU
>   
>   config MPS2_FPGAIO
>       bool
> +    select LED
>   
>   config MPS2_SCC
>       bool
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 89d15f05f9a..43b9e0cf250 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -93,7 +93,6 @@ mps2_scc_cfg_read(unsigned function, unsigned device, uint32_t value) "MPS2 SCC
>   mps2_fpgaio_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 FPGAIO read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>   mps2_fpgaio_write(uint64_t offset, uint64_t data, unsigned size) "MPS2 FPGAIO write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>   mps2_fpgaio_reset(void) "MPS2 FPGAIO: reset"
> -mps2_fpgaio_leds(char led1, char led0) "MPS2 FPGAIO LEDs: %c%c"
>   
>   # msf2-sysreg.c
>   msf2_sysreg_write(uint64_t offset, uint32_t val, uint32_t prev) "msf2-sysreg write: addr 0x%08" PRIx64 " data 0x%" PRIx32 " prev 0x%" PRIx32
> 

Re: [PATCH v5 6/7] hw/misc/mps2-scc: Use the LED device

[Luc Michel]

On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
> Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
> Reference Manual' (100112_0200_07_en):
> 
>    2.1  Overview of the MPS2 and MPS2+ hardware
> 
>         The MPS2 and MPS2+ FPGA Prototyping Boards contain the
>         following components and interfaces:
> 
>         * User switches and user LEDs:
> 
>           - Two green LEDs and two push buttons that connect to
>             the FPGA.
>           - Eight green LEDs and one 8-way dip switch that connect
>             to the MCC.
> 
> Add the 8 LEDs connected to the MCC.
> 
> This remplaces the 'mps2_scc_leds' trace events by the generic
replaces
> 'led_set_intensity' event.

Same as patch 5, I think you need to reset your LEDs in mps2_scc_reset.

Also, see below for a superfluous new line.

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> https://youtu.be/l9kD70uPchk?t=288
> ---
>   include/hw/misc/mps2-scc.h |  2 ++
>   hw/misc/mps2-scc.c         | 25 ++++++++++++++-----------
>   hw/misc/Kconfig            |  1 +
>   hw/misc/trace-events       |  1 -
>   4 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/misc/mps2-scc.h b/include/hw/misc/mps2-scc.h
> index 7045473788b..8542f384227 100644
> --- a/include/hw/misc/mps2-scc.h
> +++ b/include/hw/misc/mps2-scc.h
> @@ -13,6 +13,7 @@
>   #define MPS2_SCC_H
>   
>   #include "hw/sysbus.h"
> +#include "hw/misc/led.h"
>   
>   #define TYPE_MPS2_SCC "mps2-scc"
>   #define MPS2_SCC(obj) OBJECT_CHECK(MPS2SCC, (obj), TYPE_MPS2_SCC)
> @@ -25,6 +26,7 @@ typedef struct {
>   
>       /*< public >*/
>       MemoryRegion iomem;
> +    LEDState *led[8];
>   
>       uint32_t cfg0;
>       uint32_t cfg1;
> diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
> index 9d0909e7b35..745505b849d 100644
> --- a/hw/misc/mps2-scc.c
> +++ b/hw/misc/mps2-scc.c
> @@ -20,11 +20,13 @@
>   #include "qemu/osdep.h"
>   #include "qemu/log.h"
>   #include "qemu/module.h"
> +#include "qemu/bitops.h"
>   #include "trace.h"
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
>   #include "hw/registerfields.h"
>   #include "hw/misc/mps2-scc.h"
> +#include "hw/misc/led.h"
>   #include "hw/qdev-properties.h"
>   
>   REG32(CFG0, 0)
> @@ -152,18 +154,10 @@ static void mps2_scc_write(void *opaque, hwaddr offset, uint64_t value,
>           s->cfg0 = value;
>           break;
>       case A_CFG1:
> -        /* CFG1 bits [7:0] control the board LEDs. We don't currently have
> -         * a mechanism for displaying this graphically, so use a trace event.
> -         */
> -        trace_mps2_scc_leds(value & 0x80 ? '*' : '.',
> -                            value & 0x40 ? '*' : '.',
> -                            value & 0x20 ? '*' : '.',
> -                            value & 0x10 ? '*' : '.',
> -                            value & 0x08 ? '*' : '.',
> -                            value & 0x04 ? '*' : '.',
> -                            value & 0x02 ? '*' : '.',
> -                            value & 0x01 ? '*' : '.');
>           s->cfg1 = value;
> +        for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
> +            led_set_state(s->led[i], extract32(value, i, 1));
> +        }
>           break;
>       case A_CFGDATA_OUT:
>           s->cfgdata_out = value;
> @@ -245,10 +239,19 @@ static void mps2_scc_init(Object *obj)
>   
>       memory_region_init_io(&s->iomem, obj, &mps2_scc_ops, s, "mps2-scc", 0x1000);
>       sysbus_init_mmio(sbd, &s->iomem);
> +
Superfluous new line.

-- 
Luc
>   }
>   
>   static void mps2_scc_realize(DeviceState *dev, Error **errp)
>   {
> +    MPS2SCC *s = MPS2_SCC(dev);
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
> +        char *name = g_strdup_printf("SCC LED%zu", i);
> +        s->led[i] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
> +                                      LED_COLOR_GREEN, name);
> +        g_free(name);
> +    }
>   }
>   
>   static const VMStateDescription mps2_scc_vmstate = {
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 0cecad45aad..7557a3e7b46 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -97,6 +97,7 @@ config MPS2_FPGAIO
>   
>   config MPS2_SCC
>       bool
> +    select LED
>   
>   config TZ_MPC
>       bool
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 43b9e0cf250..a620a358feb 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -85,7 +85,6 @@ aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64
>   mps2_scc_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>   mps2_scc_write(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>   mps2_scc_reset(void) "MPS2 SCC: reset"
> -mps2_scc_leds(char led7, char led6, char led5, char led4, char led3, char led2, char led1, char led0) "MPS2 SCC LEDs: %c%c%c%c%c%c%c%c"
>   mps2_scc_cfg_write(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config write: function %d device %d data 0x%" PRIx32
>   mps2_scc_cfg_read(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config read: function %d device %d data 0x%" PRIx32
>   
> 

Re: [PATCH v5 1/7] hw/misc/led: Add a LED device

[Richard Henderson]

On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
> +static const char *led_color_name[] = {

const char * const

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Richard Henderson]

On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
> Some devices expose GPIO lines.
> 
> Add a GPIO qdev input to our LED device, so we can
> connect a GPIO output using qdev_connect_gpio_out().
> 
> When used with GPIOs, the intensity can only be either
> minium or maximum. This depends of the polarity of the
> GPIO (which can be inverted).
> Declare the GpioPolarity type to model the polarity.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/led.h  |  8 ++++++++
>  include/hw/qdev-core.h |  8 ++++++++
>  hw/misc/led.c          | 17 ++++++++++++++++-
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> index f5afaa34bfb..71c9b8c59bf 100644
> --- a/include/hw/misc/led.h
> +++ b/include/hw/misc/led.h
> @@ -38,10 +38,16 @@ typedef struct LEDState {
>      /* Public */
>  
>      uint8_t intensity_percent;
> +    qemu_irq irq;
>  
>      /* Properties */
>      char *description;
>      char *color;
> +    /*
> +     * When used with GPIO, the intensity at reset is related
> +     * to the GPIO polarity.
> +     */
> +    bool inverted_polarity;

Why are you not using the GpioPolarity enum that you added?


r~

Re: [PATCH v5 5/7] hw/misc/mps2-fpgaio: Use the LED device

[Richard Henderson]

On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
> Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
> Reference Manual' (100112_0200_07_en):
> 
>   2.1  Overview of the MPS2 and MPS2+ hardware
> 
>        The MPS2 and MPS2+ FPGA Prototyping Boards contain the
>        following components and interfaces:
> 
>        * User switches and user LEDs:
> 
>          - Two green LEDs and two push buttons that connect to
>            the FPGA.
>          - Eight green LEDs and one 8-way dip switch that connect
>            to the MCC.
> 
> Add the 2 LEDs connected to the FPGA.
> 
> This remplaces the 'mps2_fpgaio_leds' trace events by the generic
> 'led_set_intensity' event.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v5 6/7] hw/misc/mps2-scc: Use the LED device

[Richard Henderson]

On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
> Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
> Reference Manual' (100112_0200_07_en):
> 
>   2.1  Overview of the MPS2 and MPS2+ hardware
> 
>        The MPS2 and MPS2+ FPGA Prototyping Boards contain the
>        following components and interfaces:
> 
>        * User switches and user LEDs:
> 
>          - Two green LEDs and two push buttons that connect to
>            the FPGA.
>          - Eight green LEDs and one 8-way dip switch that connect
>            to the MCC.
> 
> Add the 8 LEDs connected to the MCC.
> 
> This remplaces the 'mps2_scc_leds' trace events by the generic
> 'led_set_intensity' event.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v5 7/7] hw/arm/tosa: Replace fprintf() calls by LED devices

[Richard Henderson]

On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
> The recently added LED device reports LED status changes with
> the 'led_set_intensity' trace event. It is less invasive than
> the fprintf() calls. We need however to have a binary built
> with tracing support.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/tosa.c  | 40 +++++++++++++++-------------------------
>  hw/arm/Kconfig |  1 +
>  2 files changed, 16 insertions(+), 25 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v5 5/7] hw/misc/mps2-fpgaio: Use the LED device

[Philippe Mathieu-Daudé]

On 9/11/20 10:12 PM, Luc Michel wrote:
> Hi Phil,
> 
> On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
>> Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
>> Reference Manual' (100112_0200_07_en):
>>
>>    2.1  Overview of the MPS2 and MPS2+ hardware
>>
>>         The MPS2 and MPS2+ FPGA Prototyping Boards contain the
>>         following components and interfaces:
>>
>>         * User switches and user LEDs:
>>
>>           - Two green LEDs and two push buttons that connect to
>>             the FPGA.
>>           - Eight green LEDs and one 8-way dip switch that connect
>>             to the MCC.
>>
>> Add the 2 LEDs connected to the FPGA.
>>
>> This remplaces the 'mps2_fpgaio_leds' trace events by the generic
> replaces
>> 'led_set_intensity' event.
> 
> If I'm not mistaken the LED device being a DEVICE and not a
> SYS_BUS_DEVICE, it needs to be manually reset. So you probably need to
> reset it in mps2_fpgaio_reset so it doesn't get out of sync on system
> reset.

Correct...

Alternatively we could see a LED as a SysBusDevice exposing a MMIO
region of 1 writable bit =) But it is unlikely to be mapped on the
main system bus.

I'll add the reset, thanks for reviewing!

Phil.

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

Eduardo is already in Cc, adding Markus.

On 9/12/20 12:44 AM, Richard Henderson wrote:
> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
>> Some devices expose GPIO lines.
>>
>> Add a GPIO qdev input to our LED device, so we can
>> connect a GPIO output using qdev_connect_gpio_out().
>>
>> When used with GPIOs, the intensity can only be either
>> minium or maximum. This depends of the polarity of the
>> GPIO (which can be inverted).
>> Declare the GpioPolarity type to model the polarity.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/led.h  |  8 ++++++++
>>  include/hw/qdev-core.h |  8 ++++++++
>>  hw/misc/led.c          | 17 ++++++++++++++++-
>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> index f5afaa34bfb..71c9b8c59bf 100644
>> --- a/include/hw/misc/led.h
>> +++ b/include/hw/misc/led.h
>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>      /* Public */
>>  
>>      uint8_t intensity_percent;
>> +    qemu_irq irq;
>>  
>>      /* Properties */
>>      char *description;
>>      char *color;
>> +    /*
>> +     * When used with GPIO, the intensity at reset is related
>> +     * to the GPIO polarity.
>> +     */
>> +    bool inverted_polarity;
> 
> Why are you not using the GpioPolarity enum that you added?

Because it is migrated...

Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
enum visitor in hw/core/qdev-properties.c (which is included in
user-mode builds because pulled by the CPU type).

A sane cleanup would be to make get_enum(), set_enum()
and set_default_value_enum() public (prefixed with 'qdev_')
in include/hw/qdev-properties.h.

Out of the scope of this series, but might be worth it.

Eduardo, Markus, what do you think?

Thanks Richard for reviewing this series!

Phil.

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

On 9/11/20 9:42 PM, Luc Michel wrote:
> Hi Phil,
> 
> On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
>> Some devices expose GPIO lines.
>>
>> Add a GPIO qdev input to our LED device, so we can
>> connect a GPIO output using qdev_connect_gpio_out().
>>
>> When used with GPIOs, the intensity can only be either
>> minium or maximum. This depends of the polarity of the
>> GPIO (which can be inverted).
>> Declare the GpioPolarity type to model the polarity.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/hw/misc/led.h  |  8 ++++++++
>>   include/hw/qdev-core.h |  8 ++++++++
>>   hw/misc/led.c          | 17 ++++++++++++++++-
>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> index f5afaa34bfb..71c9b8c59bf 100644
>> --- a/include/hw/misc/led.h
>> +++ b/include/hw/misc/led.h
>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>       /* Public */
>>         uint8_t intensity_percent;
>> +    qemu_irq irq;
>>         /* Properties */
>>       char *description;
>>       char *color;
>> +    /*
>> +     * When used with GPIO, the intensity at reset is related
>> +     * to the GPIO polarity.
>> +     */
>> +    bool inverted_polarity;
>>   } LEDState;
>>     /**
>> @@ -71,6 +77,7 @@ void led_set_state(LEDState *s, bool is_emitting);
>>   /**
>>    * led_create_simple: Create and realize a LED device
>>    * @parentobj: the parent object
>> + * @gpio_polarity: GPIO polarity
>>    * @color: color of the LED
>>    * @description: description of the LED (optional)
>>    *
>> @@ -78,6 +85,7 @@ void led_set_state(LEDState *s, bool is_emitting);
>>    * drop the reference to it (the device is realized).
>>    */
>>   LEDState *led_create_simple(Object *parentobj,
>> +                            GpioPolarity gpio_polarity,
>>                               LEDColor color,
>>                               const char *description);
>>   diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index ea3f73a282d..846354736a5 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -424,6 +424,14 @@ void qdev_simple_device_unplug_cb(HotplugHandler
>> *hotplug_dev,
>>   void qdev_machine_creation_done(void);
>>   bool qdev_machine_modified(void);
>>   +/**
>> + * GpioPolarity: Polarity of a GPIO line
>> + */
>> +typedef enum {
>> +    GPIO_POLARITY_ACTIVE_LOW,
>> +    GPIO_POLARITY_ACTIVE_HIGH
>> +} GpioPolarity;
>> +
>>   /**
>>    * qdev_get_gpio_in: Get one of a device's anonymous input GPIO lines
>>    * @dev: Device whose GPIO we want
>> diff --git a/hw/misc/led.c b/hw/misc/led.c
>> index 89acd9acbb7..3ef4f5e0469 100644
>> --- a/hw/misc/led.c
>> +++ b/hw/misc/led.c
>> @@ -10,6 +10,7 @@
>>   #include "migration/vmstate.h"
>>   #include "hw/qdev-properties.h"
>>   #include "hw/misc/led.h"
>> +#include "hw/irq.h"
>>   #include "trace.h"
>>     #define LED_INTENSITY_PERCENT_MAX   100
>> @@ -53,11 +54,19 @@ void led_set_state(LEDState *s, bool is_emitting)
>>       led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
>>   }
>>   +static void led_set_state_gpio_handler(void *opaque, int line, int
>> new_state)
>> +{
>> +    LEDState *s = LED(opaque);
>> +
>> +    assert(line == 0);
>> +    led_set_state(s, !!new_state != s->inverted_polarity);
>> +}
>> +
>>   static void led_reset(DeviceState *dev)
>>   {
>>       LEDState *s = LED(dev);
>>   -    led_set_state(s, false);
>> +    led_set_state(s, s->inverted_polarity);
>>   }
>>     static const VMStateDescription vmstate_led = {
>> @@ -84,11 +93,14 @@ static void led_realize(DeviceState *dev, Error
>> **errp)
>>       if (s->description == NULL) {
>>           s->description = g_strdup("n/a");
>>       }
>> +
>> +    qdev_init_gpio_in(DEVICE(s), led_set_state_gpio_handler, 1);
>>   }
>>     static Property led_properties[] = {
>>       DEFINE_PROP_STRING("color", LEDState, color),
>>       DEFINE_PROP_STRING("description", LEDState, description),
>> +    DEFINE_PROP_BOOL("polarity-inverted", LEDState,
>> inverted_polarity, false),
> I was wondering, since the GpioPolarity type you introduce is not used
> in the GPIO API, and since you use a boolean property here.

"GpioPolarity not used in GPIO API": For now, but I expect it to be
used there too. Maybe not soon, but some places could use it and
become clearer.

> Wouldn't it
> be clearer is you name this property "active-low"? Because
> "polarity-inverted" doesn't tell what the polarity is in the first
> place. Moreover since this property only affects the LED GPIO, and not
> the LED API (led_set_state), I think you can even name it
> "gpio-active-low" to make this clear.

Very good point, thanks for your suggestion!

> 
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   @@ -119,6 +131,7 @@ static void led_register_types(void)
>>   type_init(led_register_types)
>>     LEDState *led_create_simple(Object *parentobj,
>> +                            GpioPolarity gpio_polarity,
> You could go with a boolean here also and name the parameter
> gpio_active_low, but I don't have a strong opinion on this.

I'll try, as this might postpone the need for the GpioPolarity enum
(including its migration and the qapi enum visitors...).

> 
> So with or without those modifications:
> Reviewed-by: Luc Michel <luc.michel@greensocs.com>
> 
>>                               LEDColor color,
>>                               const char *description)
>>   {
>> @@ -126,6 +139,8 @@ LEDState *led_create_simple(Object *parentobj,
>>       DeviceState *dev;
>>         dev = qdev_new(TYPE_LED);
>> +    qdev_prop_set_bit(dev, "polarity-inverted",
>> +                      gpio_polarity == GPIO_POLARITY_ACTIVE_LOW);
>>       qdev_prop_set_string(dev, "color", led_color_name[color]);
>>       if (!description) {
>>           static unsigned undescribed_led_id;
>>
> 

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

On 9/12/20 11:02 AM, Philippe Mathieu-Daudé wrote:
> On 9/11/20 9:42 PM, Luc Michel wrote:
>> Hi Phil,
>>
>> On 9/10/20 10:54 PM, Philippe Mathieu-Daudé wrote:
>>> Some devices expose GPIO lines.
>>>
>>> Add a GPIO qdev input to our LED device, so we can
>>> connect a GPIO output using qdev_connect_gpio_out().
>>>
>>> When used with GPIOs, the intensity can only be either
>>> minium or maximum. This depends of the polarity of the
>>> GPIO (which can be inverted).
>>> Declare the GpioPolarity type to model the polarity.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>   include/hw/misc/led.h  |  8 ++++++++
>>>   include/hw/qdev-core.h |  8 ++++++++
>>>   hw/misc/led.c          | 17 ++++++++++++++++-
>>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>>> index f5afaa34bfb..71c9b8c59bf 100644
>>> --- a/include/hw/misc/led.h
>>> +++ b/include/hw/misc/led.h
>>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>>       /* Public */
>>>         uint8_t intensity_percent;
>>> +    qemu_irq irq;
>>>         /* Properties */
>>>       char *description;
>>>       char *color;
>>> +    /*
>>> +     * When used with GPIO, the intensity at reset is related
>>> +     * to the GPIO polarity.
>>> +     */
>>> +    bool inverted_polarity;
>>>   } LEDState;
>>>     /**
>>> @@ -71,6 +77,7 @@ void led_set_state(LEDState *s, bool is_emitting);
>>>   /**
>>>    * led_create_simple: Create and realize a LED device
>>>    * @parentobj: the parent object
>>> + * @gpio_polarity: GPIO polarity
>>>    * @color: color of the LED
>>>    * @description: description of the LED (optional)
>>>    *
>>> @@ -78,6 +85,7 @@ void led_set_state(LEDState *s, bool is_emitting);
>>>    * drop the reference to it (the device is realized).
>>>    */
>>>   LEDState *led_create_simple(Object *parentobj,
>>> +                            GpioPolarity gpio_polarity,
>>>                               LEDColor color,
>>>                               const char *description);
>>>   diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>> index ea3f73a282d..846354736a5 100644
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -424,6 +424,14 @@ void qdev_simple_device_unplug_cb(HotplugHandler
>>> *hotplug_dev,
>>>   void qdev_machine_creation_done(void);
>>>   bool qdev_machine_modified(void);
>>>   +/**
>>> + * GpioPolarity: Polarity of a GPIO line
>>> + */
>>> +typedef enum {
>>> +    GPIO_POLARITY_ACTIVE_LOW,
>>> +    GPIO_POLARITY_ACTIVE_HIGH
>>> +} GpioPolarity;
>>> +
>>>   /**
>>>    * qdev_get_gpio_in: Get one of a device's anonymous input GPIO lines
>>>    * @dev: Device whose GPIO we want
>>> diff --git a/hw/misc/led.c b/hw/misc/led.c
>>> index 89acd9acbb7..3ef4f5e0469 100644
>>> --- a/hw/misc/led.c
>>> +++ b/hw/misc/led.c
>>> @@ -10,6 +10,7 @@
>>>   #include "migration/vmstate.h"
>>>   #include "hw/qdev-properties.h"
>>>   #include "hw/misc/led.h"
>>> +#include "hw/irq.h"
>>>   #include "trace.h"
>>>     #define LED_INTENSITY_PERCENT_MAX   100
>>> @@ -53,11 +54,19 @@ void led_set_state(LEDState *s, bool is_emitting)
>>>       led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
>>>   }
>>>   +static void led_set_state_gpio_handler(void *opaque, int line, int
>>> new_state)
>>> +{
>>> +    LEDState *s = LED(opaque);
>>> +
>>> +    assert(line == 0);
>>> +    led_set_state(s, !!new_state != s->inverted_polarity);
>>> +}
>>> +
>>>   static void led_reset(DeviceState *dev)
>>>   {
>>>       LEDState *s = LED(dev);
>>>   -    led_set_state(s, false);
>>> +    led_set_state(s, s->inverted_polarity);
>>>   }
>>>     static const VMStateDescription vmstate_led = {
>>> @@ -84,11 +93,14 @@ static void led_realize(DeviceState *dev, Error
>>> **errp)
>>>       if (s->description == NULL) {
>>>           s->description = g_strdup("n/a");
>>>       }
>>> +
>>> +    qdev_init_gpio_in(DEVICE(s), led_set_state_gpio_handler, 1);
>>>   }
>>>     static Property led_properties[] = {
>>>       DEFINE_PROP_STRING("color", LEDState, color),
>>>       DEFINE_PROP_STRING("description", LEDState, description),
>>> +    DEFINE_PROP_BOOL("polarity-inverted", LEDState,
>>> inverted_polarity, false),
>> I was wondering, since the GpioPolarity type you introduce is not used
>> in the GPIO API, and since you use a boolean property here.
> 
> "GpioPolarity not used in GPIO API": For now, but I expect it to be
> used there too. Maybe not soon, but some places could use it and
> become clearer.
> 
>> Wouldn't it
>> be clearer is you name this property "active-low"? Because
>> "polarity-inverted" doesn't tell what the polarity is in the first
>> place. Moreover since this property only affects the LED GPIO, and not
>> the LED API (led_set_state), I think you can even name it
>> "gpio-active-low" to make this clear.
> 
> Very good point, thanks for your suggestion!
> 
>>
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>   @@ -119,6 +131,7 @@ static void led_register_types(void)
>>>   type_init(led_register_types)
>>>     LEDState *led_create_simple(Object *parentobj,
>>> +                            GpioPolarity gpio_polarity,
>> You could go with a boolean here also and name the parameter
>> gpio_active_low, but I don't have a strong opinion on this.
> 
> I'll try, as this might postpone the need for the GpioPolarity enum
> (including its migration and the qapi enum visitors...).

After testing using a simple boolean, I think I'll keep the enum
as it makes the caller code easier to review:

    s->led = led_create_simple(OBJECT(dev),
                               GPIO_POLARITY_ACTIVE_HIGH,
                               LED_COLOR_GREEN, name);

vs

    s->led = led_create_simple(OBJECT(dev), true,
                               LED_COLOR_GREEN, name);

> 
>>
>> So with or without those modifications:
>> Reviewed-by: Luc Michel <luc.michel@greensocs.com>
>>
>>>                               LEDColor color,
>>>                               const char *description)
>>>   {
>>> @@ -126,6 +139,8 @@ LEDState *led_create_simple(Object *parentobj,
>>>       DeviceState *dev;
>>>         dev = qdev_new(TYPE_LED);
>>> +    qdev_prop_set_bit(dev, "polarity-inverted",
>>> +                      gpio_polarity == GPIO_POLARITY_ACTIVE_LOW);
>>>       qdev_prop_set_string(dev, "color", led_color_name[color]);
>>>       if (!description) {
>>>           static unsigned undescribed_led_id;
>>>
>>
> 

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

On 9/12/20 10:50 AM, Philippe Mathieu-Daudé wrote:
> Eduardo is already in Cc, adding Markus.
> 
> On 9/12/20 12:44 AM, Richard Henderson wrote:
>> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
>>> Some devices expose GPIO lines.
>>>
>>> Add a GPIO qdev input to our LED device, so we can
>>> connect a GPIO output using qdev_connect_gpio_out().
>>>
>>> When used with GPIOs, the intensity can only be either
>>> minium or maximum. This depends of the polarity of the
>>> GPIO (which can be inverted).
>>> Declare the GpioPolarity type to model the polarity.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  include/hw/misc/led.h  |  8 ++++++++
>>>  include/hw/qdev-core.h |  8 ++++++++
>>>  hw/misc/led.c          | 17 ++++++++++++++++-
>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>>> index f5afaa34bfb..71c9b8c59bf 100644
>>> --- a/include/hw/misc/led.h
>>> +++ b/include/hw/misc/led.h
>>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>>      /* Public */
>>>  
>>>      uint8_t intensity_percent;
>>> +    qemu_irq irq;
>>>  
>>>      /* Properties */
>>>      char *description;
>>>      char *color;
>>> +    /*
>>> +     * When used with GPIO, the intensity at reset is related
>>> +     * to the GPIO polarity.
>>> +     */
>>> +    bool inverted_polarity;
>>
>> Why are you not using the GpioPolarity enum that you added?
> 
> Because it is migrated...

Luc made me realize we don't need to keep the enum in the device
state, a 'is_gpio_polarity_high' boolean is enough, so I don't
need to worry about migrating the enum.

https://www.mail-archive.com/qemu-devel@nongnu.org/msg739790.html

> 
> Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
> enum visitor in hw/core/qdev-properties.c (which is included in
> user-mode builds because pulled by the CPU type).
> 
> A sane cleanup would be to make get_enum(), set_enum()
> and set_default_value_enum() public (prefixed with 'qdev_')
> in include/hw/qdev-properties.h.
> 
> Out of the scope of this series, but might be worth it.
> 
> Eduardo, Markus, what do you think?

This question still stands however :)

> 
> Thanks Richard for reviewing this series!
> 
> Phil.
> 

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Eduardo is already in Cc, adding Markus.
>
> On 9/12/20 12:44 AM, Richard Henderson wrote:
>> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
>>> Some devices expose GPIO lines.
>>>
>>> Add a GPIO qdev input to our LED device, so we can
>>> connect a GPIO output using qdev_connect_gpio_out().
>>>
>>> When used with GPIOs, the intensity can only be either
>>> minium or maximum. This depends of the polarity of the
>>> GPIO (which can be inverted).
>>> Declare the GpioPolarity type to model the polarity.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  include/hw/misc/led.h  |  8 ++++++++
>>>  include/hw/qdev-core.h |  8 ++++++++
>>>  hw/misc/led.c          | 17 ++++++++++++++++-
>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>>> index f5afaa34bfb..71c9b8c59bf 100644
>>> --- a/include/hw/misc/led.h
>>> +++ b/include/hw/misc/led.h
>>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>>      /* Public */
>>>  
>>>      uint8_t intensity_percent;
>>> +    qemu_irq irq;
>>>  
>>>      /* Properties */
>>>      char *description;
>>>      char *color;
>>> +    /*
>>> +     * When used with GPIO, the intensity at reset is related
>>> +     * to the GPIO polarity.
>>> +     */
>>> +    bool inverted_polarity;
>> 
>> Why are you not using the GpioPolarity enum that you added?
>
> Because it is migrated...
>
> Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
> enum visitor in hw/core/qdev-properties.c (which is included in
> user-mode builds because pulled by the CPU type).

Yes.

You could also use DEFINE_PROP_UINT8(), and use it with your enumeration
constants.

I'd be tempted to ditch the enum entirely.  Matter of taste, no big deal
either way.

> A sane cleanup would be to make get_enum(), set_enum()
> and set_default_value_enum() public (prefixed with 'qdev_')
> in include/hw/qdev-properties.h.

Purpose?  To be able to define enum properties outside
qdev-properties.c?

> Out of the scope of this series, but might be worth it.
>
> Eduardo, Markus, what do you think?
>
> Thanks Richard for reviewing this series!
>
> Phil.

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

On 9/14/20 9:27 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> Eduardo is already in Cc, adding Markus.
>>
>> On 9/12/20 12:44 AM, Richard Henderson wrote:
>>> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
>>>> Some devices expose GPIO lines.
>>>>
>>>> Add a GPIO qdev input to our LED device, so we can
>>>> connect a GPIO output using qdev_connect_gpio_out().
>>>>
>>>> When used with GPIOs, the intensity can only be either
>>>> minium or maximum. This depends of the polarity of the
>>>> GPIO (which can be inverted).
>>>> Declare the GpioPolarity type to model the polarity.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  include/hw/misc/led.h  |  8 ++++++++
>>>>  include/hw/qdev-core.h |  8 ++++++++
>>>>  hw/misc/led.c          | 17 ++++++++++++++++-
>>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>>>> index f5afaa34bfb..71c9b8c59bf 100644
>>>> --- a/include/hw/misc/led.h
>>>> +++ b/include/hw/misc/led.h
>>>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>>>      /* Public */
>>>>  
>>>>      uint8_t intensity_percent;
>>>> +    qemu_irq irq;
>>>>  
>>>>      /* Properties */
>>>>      char *description;
>>>>      char *color;
>>>> +    /*
>>>> +     * When used with GPIO, the intensity at reset is related
>>>> +     * to the GPIO polarity.
>>>> +     */
>>>> +    bool inverted_polarity;
>>>
>>> Why are you not using the GpioPolarity enum that you added?
>>
>> Because it is migrated...
>>
>> Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
>> enum visitor in hw/core/qdev-properties.c (which is included in
>> user-mode builds because pulled by the CPU type).
> 
> Yes.
> 
> You could also use DEFINE_PROP_UINT8(), and use it with your enumeration
> constants.
> 
> I'd be tempted to ditch the enum entirely.  Matter of taste, no big deal
> either way.

Done in v6!

> 
>> A sane cleanup would be to make get_enum(), set_enum()
>> and set_default_value_enum() public (prefixed with 'qdev_')
>> in include/hw/qdev-properties.h.
> 
> Purpose?  To be able to define enum properties outside
> qdev-properties.c?

Yes, to avoid pulling in PCI and MAC address properties
into qemu-storage-daemon and linux-user binaries...

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Eduardo Habkost]

On Mon, Sep 14, 2020 at 09:48:45AM +0200, Philippe Mathieu-Daudé wrote:
> On 9/14/20 9:27 AM, Markus Armbruster wrote:
> > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > 
> >> Eduardo is already in Cc, adding Markus.
> >>
> >> On 9/12/20 12:44 AM, Richard Henderson wrote:
> >>> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
> >>>> Some devices expose GPIO lines.
> >>>>
> >>>> Add a GPIO qdev input to our LED device, so we can
> >>>> connect a GPIO output using qdev_connect_gpio_out().
> >>>>
> >>>> When used with GPIOs, the intensity can only be either
> >>>> minium or maximum. This depends of the polarity of the
> >>>> GPIO (which can be inverted).
> >>>> Declare the GpioPolarity type to model the polarity.
> >>>>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>> ---
> >>>>  include/hw/misc/led.h  |  8 ++++++++
> >>>>  include/hw/qdev-core.h |  8 ++++++++
> >>>>  hw/misc/led.c          | 17 ++++++++++++++++-
> >>>>  3 files changed, 32 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> >>>> index f5afaa34bfb..71c9b8c59bf 100644
> >>>> --- a/include/hw/misc/led.h
> >>>> +++ b/include/hw/misc/led.h
> >>>> @@ -38,10 +38,16 @@ typedef struct LEDState {
> >>>>      /* Public */
> >>>>  
> >>>>      uint8_t intensity_percent;
> >>>> +    qemu_irq irq;
> >>>>  
> >>>>      /* Properties */
> >>>>      char *description;
> >>>>      char *color;
> >>>> +    /*
> >>>> +     * When used with GPIO, the intensity at reset is related
> >>>> +     * to the GPIO polarity.
> >>>> +     */
> >>>> +    bool inverted_polarity;
> >>>
> >>> Why are you not using the GpioPolarity enum that you added?
> >>
> >> Because it is migrated...
> >>
> >> Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
> >> enum visitor in hw/core/qdev-properties.c (which is included in
> >> user-mode builds because pulled by the CPU type).
> > 
> > Yes.
> > 
> > You could also use DEFINE_PROP_UINT8(), and use it with your enumeration
> > constants.
> > 
> > I'd be tempted to ditch the enum entirely.  Matter of taste, no big deal
> > either way.
> 
> Done in v6!
> 
> > 
> >> A sane cleanup would be to make get_enum(), set_enum()
> >> and set_default_value_enum() public (prefixed with 'qdev_')
> >> in include/hw/qdev-properties.h.
> > 

Where and how exactly do you expect those functions to be used?
Having additional PropertyInfo structs defined manually would not
be desirable (especially if they are outside qdev*.c).  Having a
DEFINE_PROP_ENUM macro that does the enum magic behind the scenes
would be nice.

> > Purpose?  To be able to define enum properties outside
> > qdev-properties.c?
> 
> Yes, to avoid pulling in PCI and MAC address properties
> into qemu-storage-daemon and linux-user binaries...

I don't understand what enum functions have to do with PCI and
MAC address properties.

-- 
Eduardo

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

On 9/14/20 4:03 PM, Eduardo Habkost wrote:
> On Mon, Sep 14, 2020 at 09:48:45AM +0200, Philippe Mathieu-Daudé wrote:
>> On 9/14/20 9:27 AM, Markus Armbruster wrote:
>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>
>>>> Eduardo is already in Cc, adding Markus.
>>>>
>>>> On 9/12/20 12:44 AM, Richard Henderson wrote:
>>>>> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
>>>>>> Some devices expose GPIO lines.
>>>>>>
>>>>>> Add a GPIO qdev input to our LED device, so we can
>>>>>> connect a GPIO output using qdev_connect_gpio_out().
>>>>>>
>>>>>> When used with GPIOs, the intensity can only be either
>>>>>> minium or maximum. This depends of the polarity of the
>>>>>> GPIO (which can be inverted).
>>>>>> Declare the GpioPolarity type to model the polarity.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> ---
>>>>>>  include/hw/misc/led.h  |  8 ++++++++
>>>>>>  include/hw/qdev-core.h |  8 ++++++++
>>>>>>  hw/misc/led.c          | 17 ++++++++++++++++-
>>>>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>>>>>> index f5afaa34bfb..71c9b8c59bf 100644
>>>>>> --- a/include/hw/misc/led.h
>>>>>> +++ b/include/hw/misc/led.h
>>>>>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>>>>>      /* Public */
>>>>>>  
>>>>>>      uint8_t intensity_percent;
>>>>>> +    qemu_irq irq;
>>>>>>  
>>>>>>      /* Properties */
>>>>>>      char *description;
>>>>>>      char *color;
>>>>>> +    /*
>>>>>> +     * When used with GPIO, the intensity at reset is related
>>>>>> +     * to the GPIO polarity.
>>>>>> +     */
>>>>>> +    bool inverted_polarity;
>>>>>
>>>>> Why are you not using the GpioPolarity enum that you added?
>>>>
>>>> Because it is migrated...
>>>>
>>>> Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
>>>> enum visitor in hw/core/qdev-properties.c (which is included in
>>>> user-mode builds because pulled by the CPU type).
>>>
>>> Yes.
>>>
>>> You could also use DEFINE_PROP_UINT8(), and use it with your enumeration
>>> constants.
>>>
>>> I'd be tempted to ditch the enum entirely.  Matter of taste, no big deal
>>> either way.
>>
>> Done in v6!
>>
>>>
>>>> A sane cleanup would be to make get_enum(), set_enum()
>>>> and set_default_value_enum() public (prefixed with 'qdev_')
>>>> in include/hw/qdev-properties.h.
>>>
> 
> Where and how exactly do you expect those functions to be used?
> Having additional PropertyInfo structs defined manually would not
> be desirable (especially if they are outside qdev*.c).  Having a
> DEFINE_PROP_ENUM macro that does the enum magic behind the scenes
> would be nice.
> 
>>> Purpose?  To be able to define enum properties outside
>>> qdev-properties.c?
>>
>> Yes, to avoid pulling in PCI and MAC address properties
>> into qemu-storage-daemon and linux-user binaries...
> 
> I don't understand what enum functions have to do with PCI and
> MAC address properties.

My guess is that as get_enum()/set_enum()/set_default_value_enum()
are static, enum properties using it ended in this core file:

const PropertyInfo qdev_prop_bios_chs_trans = {
    .name = "BiosAtaTranslation",
    .description = "Logical CHS translation algorithm, "
                   "auto/none/lba/large/rechs",
    .enum_table = &BiosAtaTranslation_lookup,
    .get = get_enum,
    .set = set_enum,
    .set_default_value = set_default_value_enum,
};

const PropertyInfo qdev_prop_fdc_drive_type = {
    .name = "FdcDriveType",
    .description = "FDC drive type, "
                   "144/288/120/none/auto",
    .enum_table = &FloppyDriveType_lookup,
    .get = get_enum,
    .set = set_enum,
    .set_default_value = set_default_value_enum,
};

const PropertyInfo qdev_prop_pcie_link_speed = {
    .name = "PCIELinkSpeed",
    .description = "2_5/5/8/16",
    .enum_table = &PCIELinkSpeed_lookup,
    .get = get_prop_pcielinkspeed,
    .set = set_prop_pcielinkspeed,
    .set_default_value = set_default_value_enum,
};

Looking at qdev_prop_macaddr[] and qdev_prop_set_macaddr()
maybe I was remembering incorrectly, it seems this one can
be extracted easily.

Regards,

Phil.

Re: [PATCH v5 2/7] hw/misc/led: Allow connecting from GPIO output

[Philippe Mathieu-Daudé]

On 9/14/20 5:05 PM, Philippe Mathieu-Daudé wrote:
> On 9/14/20 4:03 PM, Eduardo Habkost wrote:
>> On Mon, Sep 14, 2020 at 09:48:45AM +0200, Philippe Mathieu-Daudé wrote:
>>> On 9/14/20 9:27 AM, Markus Armbruster wrote:
>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>>
>>>>> Eduardo is already in Cc, adding Markus.
>>>>>
>>>>> On 9/12/20 12:44 AM, Richard Henderson wrote:
>>>>>> On 9/10/20 1:54 PM, Philippe Mathieu-Daudé wrote:
>>>>>>> Some devices expose GPIO lines.
>>>>>>>
>>>>>>> Add a GPIO qdev input to our LED device, so we can
>>>>>>> connect a GPIO output using qdev_connect_gpio_out().
>>>>>>>
>>>>>>> When used with GPIOs, the intensity can only be either
>>>>>>> minium or maximum. This depends of the polarity of the
>>>>>>> GPIO (which can be inverted).
>>>>>>> Declare the GpioPolarity type to model the polarity.
>>>>>>>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>>> ---
>>>>>>>  include/hw/misc/led.h  |  8 ++++++++
>>>>>>>  include/hw/qdev-core.h |  8 ++++++++
>>>>>>>  hw/misc/led.c          | 17 ++++++++++++++++-
>>>>>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>>>>>>> index f5afaa34bfb..71c9b8c59bf 100644
>>>>>>> --- a/include/hw/misc/led.h
>>>>>>> +++ b/include/hw/misc/led.h
>>>>>>> @@ -38,10 +38,16 @@ typedef struct LEDState {
>>>>>>>      /* Public */
>>>>>>>  
>>>>>>>      uint8_t intensity_percent;
>>>>>>> +    qemu_irq irq;
>>>>>>>  
>>>>>>>      /* Properties */
>>>>>>>      char *description;
>>>>>>>      char *color;
>>>>>>> +    /*
>>>>>>> +     * When used with GPIO, the intensity at reset is related
>>>>>>> +     * to the GPIO polarity.
>>>>>>> +     */
>>>>>>> +    bool inverted_polarity;
>>>>>>
>>>>>> Why are you not using the GpioPolarity enum that you added?
>>>>>
>>>>> Because it is migrated...
>>>>>
>>>>> Using DEFINE_PROP_BOOL() is simpler that adding hardware specific
>>>>> enum visitor in hw/core/qdev-properties.c (which is included in
>>>>> user-mode builds because pulled by the CPU type).
>>>>
>>>> Yes.
>>>>
>>>> You could also use DEFINE_PROP_UINT8(), and use it with your enumeration
>>>> constants.
>>>>
>>>> I'd be tempted to ditch the enum entirely.  Matter of taste, no big deal
>>>> either way.
>>>
>>> Done in v6!
>>>
>>>>
>>>>> A sane cleanup would be to make get_enum(), set_enum()
>>>>> and set_default_value_enum() public (prefixed with 'qdev_')
>>>>> in include/hw/qdev-properties.h.
>>>>
>>
>> Where and how exactly do you expect those functions to be used?
>> Having additional PropertyInfo structs defined manually would not
>> be desirable (especially if they are outside qdev*.c).  Having a
>> DEFINE_PROP_ENUM macro that does the enum magic behind the scenes
>> would be nice.
>>
>>>> Purpose?  To be able to define enum properties outside
>>>> qdev-properties.c?
>>>
>>> Yes, to avoid pulling in PCI and MAC address properties
>>> into qemu-storage-daemon and linux-user binaries...
>>
>> I don't understand what enum functions have to do with PCI and
>> MAC address properties.
> 
> My guess is that as get_enum()/set_enum()/set_default_value_enum()
> are static, enum properties using it ended in this core file:
> 
> const PropertyInfo qdev_prop_bios_chs_trans = {
>     .name = "BiosAtaTranslation",
>     .description = "Logical CHS translation algorithm, "
>                    "auto/none/lba/large/rechs",
>     .enum_table = &BiosAtaTranslation_lookup,
>     .get = get_enum,
>     .set = set_enum,
>     .set_default_value = set_default_value_enum,
> };
> 
> const PropertyInfo qdev_prop_fdc_drive_type = {
>     .name = "FdcDriveType",
>     .description = "FDC drive type, "
>                    "144/288/120/none/auto",
>     .enum_table = &FloppyDriveType_lookup,
>     .get = get_enum,
>     .set = set_enum,
>     .set_default_value = set_default_value_enum,
> };
> 
> const PropertyInfo qdev_prop_pcie_link_speed = {
>     .name = "PCIELinkSpeed",
>     .description = "2_5/5/8/16",
>     .enum_table = &PCIELinkSpeed_lookup,
>     .get = get_prop_pcielinkspeed,
>     .set = set_prop_pcielinkspeed,
>     .set_default_value = set_default_value_enum,
> };
> 
> Looking at qdev_prop_macaddr[] and qdev_prop_set_macaddr()
> maybe I was remembering incorrectly, it seems this one can
> be extracted easily.

Already done in "user-mode: Prune build dependencies (part 3)"
actually :)
https://www.mail-archive.com/qemu-devel@nongnu.org/msg688867.html