[RFC PATCH v2 0/5] hw/misc: Add LED device

[RFC PATCH v2 0/5] hw/misc: Add LED device

[Philippe Mathieu-Daudé]

Hello,

These patches are part of the GSoC unselected 'QEMU visualizer'
project.  As the AVR port is not merged, I switched to microbit
to keep working on it.

This series presents a proof of concept of LED device that can
be easily connected to a GPIO.
The LED emit QMP events, so an external visualizer can display
the LED events.

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

This is stable enough to be used for the GSoC UI.

Next steps planned:

- integrate Zephyr test

- have a centralized container for all the machine's LEDs, to
track state changes in a single place and send less QMP events
(grouping changes, restricted to what actually changed).
[see to include keyboard LEDs].

- look at LED array/matrix such 7segments.

Regards,

Phil.

$ git backport-diff -u rfc-v1
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0004] [FC] 'hw/misc: Add a LED device'
002/5:[0027] [FC] 'hw/misc/led: Add LED_STATUS_CHANGED QAPI event'
003/5:[----] [--] 'hw/misc/led: Add create_led_by_gpio_id() helper'
004/5:[----] [--] 'hw/arm/microbit: Add a fake LED to use as proof-of-concept with Zephyr'
005/5:[----] [--] 'hw/arm/tosa: Use LED device for the Bluetooth led'

Philippe Mathieu-Daudé (5):
  hw/misc: Add a LED device
  hw/misc/led: Add LED_STATUS_CHANGED QAPI event
  hw/misc/led: Add create_led_by_gpio_id() helper
  hw/arm/microbit: Add a fake LED to use as proof-of-concept with Zephyr
  hw/arm/tosa: Use LED device for the Bluetooth led

 qapi/led.json         |  47 ++++++++++++++++
 qapi/qapi-schema.json |   1 +
 include/hw/misc/led.h |  45 +++++++++++++++
 hw/arm/microbit.c     |   3 +
 hw/arm/tosa.c         |   7 +--
 hw/misc/led.c         | 126 ++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS           |   7 +++
 hw/arm/Kconfig        |   2 +
 hw/misc/Kconfig       |   3 +
 hw/misc/Makefile.objs |   1 +
 hw/misc/trace-events  |   3 +
 qapi/Makefile.objs    |   2 +-
 12 files changed, 242 insertions(+), 5 deletions(-)
 create mode 100644 qapi/led.json
 create mode 100644 include/hw/misc/led.h
 create mode 100644 hw/misc/led.c

-- 
2.21.3

[RFC PATCH v2 1/5] hw/misc: Add a LED device

[Philippe Mathieu-Daudé]

A LED device can be connected to a GPIO output.

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

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
new file mode 100644
index 0000000000..427ca1418e
--- /dev/null
+++ b/include/hw/misc/led.h
@@ -0,0 +1,30 @@
+/*
+ * 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"
+#include "hw/sysbus.h" /* FIXME remove */
+
+#define TYPE_LED "led"
+#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
+
+typedef struct LEDState {
+    /* Private */
+    SysBusDevice parent_obj; /* FIXME DeviceState */
+    /* Public */
+
+    qemu_irq irq;
+    uint8_t current_state;
+
+    /* Properties */
+    char *name;
+    uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
+} LEDState;
+
+#endif /* HW_MISC_LED_H */
diff --git a/hw/misc/led.c b/hw/misc/led.c
new file mode 100644
index 0000000000..1bae1a34c0
--- /dev/null
+++ b/hw/misc/led.c
@@ -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
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/led.h"
+#include "hw/irq.h"
+#include "trace.h"
+
+static void led_set(void *opaque, int line, int new_state)
+{
+    LEDState *s = LED(opaque);
+
+    trace_led_set(s->name, s->current_state, new_state);
+
+    s->current_state = new_state;
+}
+
+static void led_reset(DeviceState *dev)
+{
+    LEDState *s = LED(dev);
+
+    led_set(dev, 0, s->reset_state);
+}
+
+static const VMStateDescription vmstate_led = {
+    .name = TYPE_LED,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(reset_state, LEDState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void led_realize(DeviceState *dev, Error **errp)
+{
+    LEDState *s = LED(dev);
+
+    if (s->name == NULL) {
+        error_setg(errp, "property 'name' not specified");
+        return;
+    }
+
+    qdev_init_gpio_in(DEVICE(s), led_set, 1);
+}
+
+static Property led_properties[] = {
+    DEFINE_PROP_STRING("name", LEDState, name),
+    DEFINE_PROP_UINT8("reset_state", LEDState, reset_state, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void led_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "LED";
+    dc->vmsd = &vmstate_led;
+    dc->reset = led_reset;
+    dc->realize = led_realize;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    device_class_set_props(dc, led_properties);
+}
+
+static const TypeInfo led_info = {
+    .name = TYPE_LED,
+    .parent = TYPE_SYS_BUS_DEVICE, /* FIXME 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)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3abe3faa4e..10593863dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1857,6 +1857,12 @@ F: docs/specs/vmgenid.txt
 F: tests/qtest/vmgenid-test.c
 F: stubs/vmgenid.c
 
+LED
+M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+S: Maintained
+F: include/hw/misc/led.h
+F: hw/misc/led.c
+
 Unimplemented device
 M: Peter Maydell <peter.maydell@linaro.org>
 R: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index bdd77d8020..f60dce694d 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -126,6 +126,9 @@ config AUX
 config UNIMP
     bool
 
+config LED
+    bool
+
 config MAC_VIA
     bool
     select MOS6522
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 5aaca8a039..9efa3c941c 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -91,3 +91,4 @@ common-obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
 obj-$(CONFIG_MAC_VIA) += mac_via.o
 
 common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o
+common-obj-$(CONFIG_LED) += led.o
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 5561746866..e15b7f7c81 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
 # grlib_ahb_apb_pnp.c
 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(const char *name, uint8_t old_state, uint8_t new_state) "led name:'%s' state %d -> %d"
-- 
2.21.3

[RFC PATCH v2 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event

[Philippe Mathieu-Daudé]

Allow LED devices to emit STATUS_CHANGED events on a QMP chardev.

QMP event examples:

{
    "timestamp": {
        "seconds": 1591704274,
        "microseconds": 520850
    },
    "event": "LED_STATUS_CHANGED",
    "data": {
        "name": "Green LED #0",
        "status": "on"
    }
}
{
    "timestamp": {
        "seconds": 1591704275,
        "microseconds": 530912
    },
    "event": "LED_STATUS_CHANGED",
    "data": {
        "name": "Green LED #0",
        "status": "off"
    }
}

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v1: rate limit 4/sec (eblake)
---
 qapi/led.json         | 47 +++++++++++++++++++++++++++++++++++++++++++
 qapi/qapi-schema.json |  1 +
 include/hw/misc/led.h |  1 +
 hw/misc/led.c         | 24 +++++++++++++++++++++-
 MAINTAINERS           |  1 +
 qapi/Makefile.objs    |  2 +-
 6 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100644 qapi/led.json

diff --git a/qapi/led.json b/qapi/led.json
new file mode 100644
index 0000000000..b6cef8a5dd
--- /dev/null
+++ b/qapi/led.json
@@ -0,0 +1,47 @@
+# -*- Mode: Python -*-
+#
+
+##
+# = LED device
+##
+
+##
+# @LedState:
+#
+# Status of a LED
+#
+# @on: device is emitting
+#
+# @off: device is off
+#
+# Since: 5.1
+##
+{ 'enum': 'LedState', 'data': [ 'on', 'off' ] }
+
+##
+# @LED_STATUS_CHANGED:
+#
+# Emitted when LED status changed
+#
+# @name: LED description
+#
+# @status: New status
+#
+# Since: 5.1
+#
+# Example:
+#
+# <- {"timestamp": {"seconds": 1541579657, "microseconds": 986760},
+#     "event": "LED_STATUS_CHANGED",
+#     "data":
+#         {"name": "Blue LED #3",
+#          "status": "on"
+#         }
+#    }
+#
+##
+{ 'event': 'LED_STATUS_CHANGED',
+  'data': { 'name'      : 'str',
+            'status'    : 'LedState'
+          }
+}
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 43b0ba0dea..6f3ffc0ae1 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -84,3 +84,4 @@
 { 'include': 'misc.json' }
 { 'include': 'misc-target.json' }
 { 'include': 'audio.json' }
+{ 'include': 'led.json' }
diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index 427ca1418e..9300d4db6c 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -21,6 +21,7 @@ typedef struct LEDState {
 
     qemu_irq irq;
     uint8_t current_state;
+    int64_t last_event_ms;
 
     /* Properties */
     char *name;
diff --git a/hw/misc/led.c b/hw/misc/led.c
index 1bae1a34c0..11c7e8bb89 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -7,18 +7,40 @@
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-led.h"
+#include "qemu/timer.h"
 #include "migration/vmstate.h"
 #include "hw/qdev-properties.h"
 #include "hw/misc/led.h"
 #include "hw/irq.h"
 #include "trace.h"
 
+#define MAX_QMP_LED_EVENTS_PER_SEC  4 /* TODO shared between LED children? */
+
+static void emit_led_status_changed_event(LEDState *s, int state)
+{
+    static const int64_t delay_min_ms = NANOSECONDS_PER_SECOND / SCALE_MS
+                                        / MAX_QMP_LED_EVENTS_PER_SEC;
+    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+
+    if (now - s->last_event_ms > delay_min_ms) {
+        qapi_event_send_led_status_changed(s->name, state
+                                                    ? LED_STATE_ON
+                                                    : LED_STATE_OFF);
+    } else {
+        /* TODO count skipped events? */
+    }
+    s->last_event_ms = now;
+}
+
 static void led_set(void *opaque, int line, int new_state)
 {
     LEDState *s = LED(opaque);
 
     trace_led_set(s->name, s->current_state, new_state);
-
+    if (new_state != s->current_state) {
+        emit_led_status_changed_event(s, new_state);
+    }
     s->current_state = new_state;
 }
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 10593863dc..266b07c4b4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1860,6 +1860,7 @@ F: stubs/vmgenid.c
 LED
 M: Philippe Mathieu-Daudé <f4bug@amsat.org>
 S: Maintained
+F: qapi/led.json
 F: include/hw/misc/led.h
 F: hw/misc/led.c
 
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 4673ab7490..e9f6570c32 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -6,7 +6,7 @@ util-obj-y += qmp-event.o
 util-obj-y += qapi-util.o
 
 QAPI_COMMON_MODULES = audio authz block-core block char common control crypto
-QAPI_COMMON_MODULES += dump error introspect job machine migration misc
+QAPI_COMMON_MODULES += dump error introspect job led machine migration misc
 QAPI_COMMON_MODULES += net pragma qdev qom rdma rocker run-state sockets tpm
 QAPI_COMMON_MODULES += trace transaction ui
 QAPI_TARGET_MODULES = machine-target misc-target
-- 
2.21.3

[RFC PATCH v2 3/5] hw/misc/led: Add create_led_by_gpio_id() helper

[Philippe Mathieu-Daudé]

Add create_led_by_gpio_id() to easily connect a LED to
a GPIO output.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/led.h | 14 ++++++++++++++
 hw/misc/led.c         | 20 ++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index 9300d4db6c..1b2bb96712 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -28,4 +28,18 @@ typedef struct LEDState {
     uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
 } LEDState;
 
+/**
+ * create_led_by_gpio_id: create and LED device
+ * @parent: the parent object
+ * @gpio_dev: device exporting GPIOs
+ * @gpio_id: GPIO ID of this LED
+ * @name: name of the LED
+ *
+ * This utility function creates a LED and connects it to a
+ * GPIO exported by another device.
+ */
+DeviceState *create_led_by_gpio_id(Object *parentobj,
+                                   DeviceState *gpio_dev, unsigned gpio_id,
+                                   const char *led_name);
+
 #endif /* HW_MISC_LED_H */
diff --git a/hw/misc/led.c b/hw/misc/led.c
index 11c7e8bb89..36de80dd67 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -104,3 +104,23 @@ static void led_register_types(void)
 }
 
 type_init(led_register_types)
+
+DeviceState *create_led_by_gpio_id(Object *parentobj,
+                                   DeviceState *gpio_dev, unsigned gpio_id,
+                                   const char *led_name)
+{
+    DeviceState *dev;
+    char *name;
+
+    dev = qdev_create(NULL, TYPE_LED);
+    /* TODO set "reset_state" */
+    qdev_prop_set_string(dev, "name", led_name);
+    name = g_ascii_strdown(led_name, -1);
+    name = g_strdelimit(name, " #", '-');
+    object_property_add_child(parentobj, name, OBJECT(dev));
+    g_free(name);
+    qdev_init_nofail(dev);
+    qdev_connect_gpio_out(gpio_dev, gpio_id, qdev_get_gpio_in(dev, 0));
+
+    return dev;
+}
-- 
2.21.3

[RFC PATCH v2 4/5] hw/arm/microbit: Add a fake LED to use as proof-of-concept with Zephyr

[Philippe Mathieu-Daudé]

We were using an AVR based Arduino to use this device, but since
the port is not merged, the microbit is the easiest board to use
with Zephyr.
Note the microbit doesn't have a such LED, this is simply a proof
of concept.

How to test:

- Apply this patch on zephyr-v2.3.0

  diff --git a/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts b/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts
  index a1b3044275..61b39506b1 100644
  --- a/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts
  +++ b/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts
  @@ -21,6 +21,18 @@
                  zephyr,flash = &flash0;
                  zephyr,code-partition = &slot0_partition;
          };
  +
  +       leds {
  +               compatible = "gpio-leds";
  +               led0: led_0 {
  +                       gpios = <&gpio0 21 GPIO_ACTIVE_LOW>;
  +                       label = "Green LED 0";
  +               };
  +       };
  +
  +       aliases {
  +               led0 = &led0;
  +       };
   };

   &gpiote {

- Build Zephyr blinky:

  $ west build -b qemu_cortex_m0 samples/basic/blinky

- Run QEMU

  $ qemu-system-arm -M microbit -trace led\* \
      -kernel ~/zephyrproject/zephyr/build/zephyr/zephyr.elf -trace led\*
  2953@1591704866.319665:led_set led name:'Green LED #0' state 0 -> 0
  2953@1591704867.329143:led_set led name:'Green LED #0' state 0 -> 1
  2953@1591704868.332590:led_set led name:'Green LED #0' state 1 -> 0

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/microbit.c | 3 +++
 hw/arm/Kconfig    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index ef213695bd..102661b66a 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -18,6 +18,7 @@
 #include "hw/arm/nrf51_soc.h"
 #include "hw/i2c/microbit_i2c.h"
 #include "hw/qdev-properties.h"
+#include "hw/misc/led.h"
 
 typedef struct {
     MachineState parent;
@@ -58,6 +59,8 @@ static void microbit_init(MachineState *machine)
     memory_region_add_subregion_overlap(&s->nrf51.container, NRF51_TWI_BASE,
                                         mr, -1);
 
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(soc), 21, "Green LED #0");
+
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
                        NRF51_SOC(soc)->flash_size);
 }
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 9afa6eee79..2afaa7c8e9 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -436,6 +436,7 @@ config FSL_IMX6UL
 config MICROBIT
     bool
     select NRF51_SOC
+    select LED
 
 config NRF51_SOC
     bool
-- 
2.21.3

[RFC PATCH v2 5/5] hw/arm/tosa: Use LED device for the Bluetooth led

[Philippe Mathieu-Daudé]

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

diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 5dee2d76c6..86d7e0283a 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
@@ -68,9 +69,6 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
 static void tosa_out_switch(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;
@@ -119,7 +117,6 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
                         qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
                         NULL);
 
-    qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]);
     qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]);
     qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]);
     qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]);
@@ -234,6 +231,8 @@ static void tosa_init(MachineState *machine)
 
     scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
     scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
+                          TOSA_GPIO_BT_LED, "blue LED");
 
     tosa_gpio_setup(mpu, scp0, scp1, tmio);
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 2afaa7c8e9..009336cac8 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.21.3

Re: [RFC PATCH v2 1/5] hw/misc: Add a LED device

[Stefan Weil]

Am 12.06.20 um 19:54 schrieb Philippe Mathieu-Daudé:

> A LED device can be connected to a GPIO output.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/led.h | 30 ++++++++++++++++
>  hw/misc/led.c         | 84 +++++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS           |  6 ++++
>  hw/misc/Kconfig       |  3 ++
>  hw/misc/Makefile.objs |  1 +
>  hw/misc/trace-events  |  3 ++
>  6 files changed, 127 insertions(+)
>  create mode 100644 include/hw/misc/led.h
>  create mode 100644 hw/misc/led.c
>
> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> new file mode 100644
> index 0000000000..427ca1418e
> --- /dev/null
> +++ b/include/hw/misc/led.h
> @@ -0,0 +1,30 @@
> +/*
> + * 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"
> +#include "hw/sysbus.h" /* FIXME remove */
> +
> +#define TYPE_LED "led"
> +#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
> +
> +typedef struct LEDState {
> +    /* Private */
> +    SysBusDevice parent_obj; /* FIXME DeviceState */
> +    /* Public */
> +
> +    qemu_irq irq;
> +    uint8_t current_state;
> +
> +    /* Properties */
> +    char *name;
> +    uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
> +} LEDState;
> +
> +#endif /* HW_MISC_LED_H */


LEDSTate could be made smaller (less holes) by simply re-ordering the
elements: irq, name, current_state, reset_state

Kind regards

Stefan

Re: [RFC PATCH v2 1/5] hw/misc: Add a LED device

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> A LED device can be connected to a GPIO output.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/led.h | 30 ++++++++++++++++
>  hw/misc/led.c         | 84 +++++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS           |  6 ++++
>  hw/misc/Kconfig       |  3 ++
>  hw/misc/Makefile.objs |  1 +
>  hw/misc/trace-events  |  3 ++
>  6 files changed, 127 insertions(+)
>  create mode 100644 include/hw/misc/led.h
>  create mode 100644 hw/misc/led.c
> 
> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> new file mode 100644
> index 0000000000..427ca1418e
> --- /dev/null
> +++ b/include/hw/misc/led.h
> @@ -0,0 +1,30 @@
> +/*
> + * 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"
> +#include "hw/sysbus.h" /* FIXME remove */
> +
> +#define TYPE_LED "led"
> +#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
> +
> +typedef struct LEDState {
> +    /* Private */
> +    SysBusDevice parent_obj; /* FIXME DeviceState */
> +    /* Public */
> +
> +    qemu_irq irq;

Why an irq?

> +    uint8_t current_state;

Is the state of this device boolean or is it a 0..255 0=off, 255=full
on, analog thing?
Can an LED device be connected to a PWM device driving a GPIO - what
happens?

Dave


> +    /* Properties */
> +    char *name;
> +    uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
> +} LEDState;
> +
> +#endif /* HW_MISC_LED_H */
> diff --git a/hw/misc/led.c b/hw/misc/led.c
> new file mode 100644
> index 0000000000..1bae1a34c0
> --- /dev/null
> +++ b/hw/misc/led.c
> @@ -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
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/misc/led.h"
> +#include "hw/irq.h"
> +#include "trace.h"
> +
> +static void led_set(void *opaque, int line, int new_state)
> +{
> +    LEDState *s = LED(opaque);
> +
> +    trace_led_set(s->name, s->current_state, new_state);
> +
> +    s->current_state = new_state;
> +}
> +
> +static void led_reset(DeviceState *dev)
> +{
> +    LEDState *s = LED(dev);
> +
> +    led_set(dev, 0, s->reset_state);
> +}
> +
> +static const VMStateDescription vmstate_led = {
> +    .name = TYPE_LED,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(reset_state, LEDState),

I'm not sure you need to migrate this - this is a property that's set on
the device, not a dynamic state of the device
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void led_realize(DeviceState *dev, Error **errp)
> +{
> +    LEDState *s = LED(dev);
> +
> +    if (s->name == NULL) {
> +        error_setg(errp, "property 'name' not specified");
> +        return;
> +    }
> +
> +    qdev_init_gpio_in(DEVICE(s), led_set, 1);
> +}
> +
> +static Property led_properties[] = {
> +    DEFINE_PROP_STRING("name", LEDState, name),
> +    DEFINE_PROP_UINT8("reset_state", LEDState, reset_state, 0),

I suggest you add a property for the notional colour; that way any UIs
that are built can use that as a hint.

Dave

> +    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_SYS_BUS_DEVICE, /* FIXME 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)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3abe3faa4e..10593863dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1857,6 +1857,12 @@ F: docs/specs/vmgenid.txt
>  F: tests/qtest/vmgenid-test.c
>  F: stubs/vmgenid.c
>  
> +LED
> +M: Philippe Mathieu-Daudé <f4bug@amsat.org>
> +S: Maintained
> +F: include/hw/misc/led.h
> +F: hw/misc/led.c
> +
>  Unimplemented device
>  M: Peter Maydell <peter.maydell@linaro.org>
>  R: Philippe Mathieu-Daudé <f4bug@amsat.org>
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index bdd77d8020..f60dce694d 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -126,6 +126,9 @@ config AUX
>  config UNIMP
>      bool
>  
> +config LED
> +    bool
> +
>  config MAC_VIA
>      bool
>      select MOS6522
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 5aaca8a039..9efa3c941c 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -91,3 +91,4 @@ common-obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
>  obj-$(CONFIG_MAC_VIA) += mac_via.o
>  
>  common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o
> +common-obj-$(CONFIG_LED) += led.o
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 5561746866..e15b7f7c81 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>  # grlib_ahb_apb_pnp.c
>  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(const char *name, uint8_t old_state, uint8_t new_state) "led name:'%s' state %d -> %d"
> -- 
> 2.21.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH v2 1/5] hw/misc: Add a LED device

[Philippe Mathieu-Daudé]

On 6/15/20 12:55 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
>> A LED device can be connected to a GPIO output.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/led.h | 30 ++++++++++++++++
>>  hw/misc/led.c         | 84 +++++++++++++++++++++++++++++++++++++++++++
>>  MAINTAINERS           |  6 ++++
>>  hw/misc/Kconfig       |  3 ++
>>  hw/misc/Makefile.objs |  1 +
>>  hw/misc/trace-events  |  3 ++
>>  6 files changed, 127 insertions(+)
>>  create mode 100644 include/hw/misc/led.h
>>  create mode 100644 hw/misc/led.c
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> new file mode 100644
>> index 0000000000..427ca1418e
>> --- /dev/null
>> +++ b/include/hw/misc/led.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * 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"
>> +#include "hw/sysbus.h" /* FIXME remove */
>> +
>> +#define TYPE_LED "led"
>> +#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
>> +
>> +typedef struct LEDState {
>> +    /* Private */
>> +    SysBusDevice parent_obj; /* FIXME DeviceState */
>> +    /* Public */
>> +
>> +    qemu_irq irq;
> 
> Why an irq?

We model GPIO/IRQ the same way, this is simply a GPIO, right?

It makes modeling easier IMO. This is for visualization purpose.

> 
>> +    uint8_t current_state;
> 
> Is the state of this device boolean or is it a 0..255 0=off, 255=full
> on, analog thing?
> Can an LED device be connected to a PWM device driving a GPIO - what
> happens?

Well I simply wanted to use a boolean, but I need to consider
if we can model intensity here in case of PWM. This is interesting.

> 
> Dave
> 
> 
>> +    /* Properties */
>> +    char *name;
>> +    uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
>> +} LEDState;
>> +
>> +#endif /* HW_MISC_LED_H */
>> diff --git a/hw/misc/led.c b/hw/misc/led.c
>> new file mode 100644
>> index 0000000000..1bae1a34c0
>> --- /dev/null
>> +++ b/hw/misc/led.c
>> @@ -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
>> + */
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "migration/vmstate.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/misc/led.h"
>> +#include "hw/irq.h"
>> +#include "trace.h"
>> +
>> +static void led_set(void *opaque, int line, int new_state)
>> +{
>> +    LEDState *s = LED(opaque);
>> +
>> +    trace_led_set(s->name, s->current_state, new_state);
>> +
>> +    s->current_state = new_state;
>> +}
>> +
>> +static void led_reset(DeviceState *dev)
>> +{
>> +    LEDState *s = LED(dev);
>> +
>> +    led_set(dev, 0, s->reset_state);
>> +}
>> +
>> +static const VMStateDescription vmstate_led = {
>> +    .name = TYPE_LED,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(reset_state, LEDState),
> 
> I'm not sure you need to migrate this - this is a property that's set on
> the device, not a dynamic state of the device

Yes you are right.

>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void led_realize(DeviceState *dev, Error **errp)
>> +{
>> +    LEDState *s = LED(dev);
>> +
>> +    if (s->name == NULL) {
>> +        error_setg(errp, "property 'name' not specified");
>> +        return;
>> +    }
>> +
>> +    qdev_init_gpio_in(DEVICE(s), led_set, 1);
>> +}
>> +
>> +static Property led_properties[] = {
>> +    DEFINE_PROP_STRING("name", LEDState, name),
>> +    DEFINE_PROP_UINT8("reset_state", LEDState, reset_state, 0),
> 
> I suggest you add a property for the notional colour; that way any UIs
> that are built can use that as a hint.

Great idea, thanks!

> 
> Dave
> 
>> +    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_SYS_BUS_DEVICE, /* FIXME 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)
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3abe3faa4e..10593863dc 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1857,6 +1857,12 @@ F: docs/specs/vmgenid.txt
>>  F: tests/qtest/vmgenid-test.c
>>  F: stubs/vmgenid.c
>>  
>> +LED
>> +M: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> +S: Maintained
>> +F: include/hw/misc/led.h
>> +F: hw/misc/led.c
>> +
>>  Unimplemented device
>>  M: Peter Maydell <peter.maydell@linaro.org>
>>  R: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
>> index bdd77d8020..f60dce694d 100644
>> --- a/hw/misc/Kconfig
>> +++ b/hw/misc/Kconfig
>> @@ -126,6 +126,9 @@ config AUX
>>  config UNIMP
>>      bool
>>  
>> +config LED
>> +    bool
>> +
>>  config MAC_VIA
>>      bool
>>      select MOS6522
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 5aaca8a039..9efa3c941c 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -91,3 +91,4 @@ common-obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
>>  obj-$(CONFIG_MAC_VIA) += mac_via.o
>>  
>>  common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o
>> +common-obj-$(CONFIG_LED) += led.o
>> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
>> index 5561746866..e15b7f7c81 100644
>> --- a/hw/misc/trace-events
>> +++ b/hw/misc/trace-events
>> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>>  # grlib_ahb_apb_pnp.c
>>  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(const char *name, uint8_t old_state, uint8_t new_state) "led name:'%s' state %d -> %d"
>> -- 
>> 2.21.3
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [RFC PATCH v2 1/5] hw/misc: Add a LED device

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> On 6/15/20 12:55 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> >> A LED device can be connected to a GPIO output.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>  include/hw/misc/led.h | 30 ++++++++++++++++
> >>  hw/misc/led.c         | 84 +++++++++++++++++++++++++++++++++++++++++++
> >>  MAINTAINERS           |  6 ++++
> >>  hw/misc/Kconfig       |  3 ++
> >>  hw/misc/Makefile.objs |  1 +
> >>  hw/misc/trace-events  |  3 ++
> >>  6 files changed, 127 insertions(+)
> >>  create mode 100644 include/hw/misc/led.h
> >>  create mode 100644 hw/misc/led.c
> >>
> >> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> >> new file mode 100644
> >> index 0000000000..427ca1418e
> >> --- /dev/null
> >> +++ b/include/hw/misc/led.h
> >> @@ -0,0 +1,30 @@
> >> +/*
> >> + * 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"
> >> +#include "hw/sysbus.h" /* FIXME remove */
> >> +
> >> +#define TYPE_LED "led"
> >> +#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
> >> +
> >> +typedef struct LEDState {
> >> +    /* Private */
> >> +    SysBusDevice parent_obj; /* FIXME DeviceState */
> >> +    /* Public */
> >> +
> >> +    qemu_irq irq;
> > 
> > Why an irq?
> 
> We model GPIO/IRQ the same way, this is simply a GPIO, right?
> 
> It makes modeling easier IMO. This is for visualization purpose.
> 
> > 
> >> +    uint8_t current_state;
> > 
> > Is the state of this device boolean or is it a 0..255 0=off, 255=full
> > on, analog thing?
> > Can an LED device be connected to a PWM device driving a GPIO - what
> > happens?
> 
> Well I simply wanted to use a boolean, but I need to consider
> if we can model intensity here in case of PWM. This is interesting.

Which then leads to an interesting question about your events in the
next mail;  if I was to connect an LED to a PWM driven GPIO I'd get
zillions of events which may not be what I want.

Dave

> > 
> > Dave
> > 
> > 
> >> +    /* Properties */
> >> +    char *name;
> >> +    uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
> >> +} LEDState;
> >> +
> >> +#endif /* HW_MISC_LED_H */
> >> diff --git a/hw/misc/led.c b/hw/misc/led.c
> >> new file mode 100644
> >> index 0000000000..1bae1a34c0
> >> --- /dev/null
> >> +++ b/hw/misc/led.c
> >> @@ -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
> >> + */
> >> +#include "qemu/osdep.h"
> >> +#include "qapi/error.h"
> >> +#include "migration/vmstate.h"
> >> +#include "hw/qdev-properties.h"
> >> +#include "hw/misc/led.h"
> >> +#include "hw/irq.h"
> >> +#include "trace.h"
> >> +
> >> +static void led_set(void *opaque, int line, int new_state)
> >> +{
> >> +    LEDState *s = LED(opaque);
> >> +
> >> +    trace_led_set(s->name, s->current_state, new_state);
> >> +
> >> +    s->current_state = new_state;
> >> +}
> >> +
> >> +static void led_reset(DeviceState *dev)
> >> +{
> >> +    LEDState *s = LED(dev);
> >> +
> >> +    led_set(dev, 0, s->reset_state);
> >> +}
> >> +
> >> +static const VMStateDescription vmstate_led = {
> >> +    .name = TYPE_LED,
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT8(reset_state, LEDState),
> > 
> > I'm not sure you need to migrate this - this is a property that's set on
> > the device, not a dynamic state of the device
> 
> Yes you are right.
> 
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> >> +static void led_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    LEDState *s = LED(dev);
> >> +
> >> +    if (s->name == NULL) {
> >> +        error_setg(errp, "property 'name' not specified");
> >> +        return;
> >> +    }
> >> +
> >> +    qdev_init_gpio_in(DEVICE(s), led_set, 1);
> >> +}
> >> +
> >> +static Property led_properties[] = {
> >> +    DEFINE_PROP_STRING("name", LEDState, name),
> >> +    DEFINE_PROP_UINT8("reset_state", LEDState, reset_state, 0),
> > 
> > I suggest you add a property for the notional colour; that way any UIs
> > that are built can use that as a hint.
> 
> Great idea, thanks!
> 
> > 
> > Dave
> > 
> >> +    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_SYS_BUS_DEVICE, /* FIXME 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)
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 3abe3faa4e..10593863dc 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -1857,6 +1857,12 @@ F: docs/specs/vmgenid.txt
> >>  F: tests/qtest/vmgenid-test.c
> >>  F: stubs/vmgenid.c
> >>  
> >> +LED
> >> +M: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> +S: Maintained
> >> +F: include/hw/misc/led.h
> >> +F: hw/misc/led.c
> >> +
> >>  Unimplemented device
> >>  M: Peter Maydell <peter.maydell@linaro.org>
> >>  R: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> >> index bdd77d8020..f60dce694d 100644
> >> --- a/hw/misc/Kconfig
> >> +++ b/hw/misc/Kconfig
> >> @@ -126,6 +126,9 @@ config AUX
> >>  config UNIMP
> >>      bool
> >>  
> >> +config LED
> >> +    bool
> >> +
> >>  config MAC_VIA
> >>      bool
> >>      select MOS6522
> >> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> >> index 5aaca8a039..9efa3c941c 100644
> >> --- a/hw/misc/Makefile.objs
> >> +++ b/hw/misc/Makefile.objs
> >> @@ -91,3 +91,4 @@ common-obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
> >>  obj-$(CONFIG_MAC_VIA) += mac_via.o
> >>  
> >>  common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o
> >> +common-obj-$(CONFIG_LED) += led.o
> >> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> >> index 5561746866..e15b7f7c81 100644
> >> --- a/hw/misc/trace-events
> >> +++ b/hw/misc/trace-events
> >> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
> >>  # grlib_ahb_apb_pnp.c
> >>  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(const char *name, uint8_t old_state, uint8_t new_state) "led name:'%s' state %d -> %d"
> >> -- 
> >> 2.21.3
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH v2 5/5] hw/arm/tosa: Use LED device for the Bluetooth led

[Peter Maydell]

On Fri, 12 Jun 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/tosa.c  | 7 +++----
>  hw/arm/Kconfig | 1 +
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 5dee2d76c6..86d7e0283a 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
> @@ -68,9 +69,6 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
>  static void tosa_out_switch(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;

Why convert the blue LED and not the green, amber or wlan LEDs ?

thanks
-- PMM

Re: [RFC PATCH v2 4/5] hw/arm/microbit: Add a fake LED to use as proof-of-concept with Zephyr

[Peter Maydell]

On Fri, 12 Jun 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> We were using an AVR based Arduino to use this device, but since
> the port is not merged, the microbit is the easiest board to use
> with Zephyr.
> Note the microbit doesn't have a such LED, this is simply a proof
> of concept.

I know this is an RFC patchset, but just for the record, I
don't think we should add things to the board that the
real hardware doesn't have. (The microbit does have a 5x5
LED "display" but I dunno if this would be the best way
to implement that...)

thanks
-- PMM

Re: [RFC PATCH v2 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event

[Peter Maydell]

On Fri, 12 Jun 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Allow LED devices to emit STATUS_CHANGED events on a QMP chardev.
>
> QMP event examples:
>
> {
>     "timestamp": {
>         "seconds": 1591704274,
>         "microseconds": 520850
>     },
>     "event": "LED_STATUS_CHANGED",
>     "data": {
>         "name": "Green LED #0",
>         "status": "on"
>     }
> }
> {
>     "timestamp": {
>         "seconds": 1591704275,
>         "microseconds": 530912
>     },
>     "event": "LED_STATUS_CHANGED",
>     "data": {
>         "name": "Green LED #0",
>         "status": "off"
>     }
> }

Is there a proof-of-concept of what might be on the other end of this
QMP link consuming these events and doing something useful with them?

thanks
-- PMM

Re: [RFC PATCH v2 4/5] hw/arm/microbit: Add a fake LED to use as proof-of-concept with Zephyr

[Philippe Mathieu-Daudé]

On 6/15/20 6:02 PM, Peter Maydell wrote:
> On Fri, 12 Jun 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> We were using an AVR based Arduino to use this device, but since
>> the port is not merged, the microbit is the easiest board to use
>> with Zephyr.
>> Note the microbit doesn't have a such LED, this is simply a proof
>> of concept.
> 
> I know this is an RFC patchset, but just for the record, I
> don't think we should add things to the board that the
> real hardware doesn't have. (The microbit does have a 5x5
> LED "display" but I dunno if this would be the best way
> to implement that...)

Yes, I plan to implement a generic LED array matrix and
use it in the microbit (and few other boards).

Without the AVR port merged, the microbit is the quickest
way for me to test with Zephyr.

> 
> thanks
> -- PMM
> 

Re: [RFC PATCH v2 5/5] hw/arm/tosa: Use LED device for the Bluetooth led

[Philippe Mathieu-Daudé]

On 6/15/20 6:00 PM, Peter Maydell wrote:
> On Fri, 12 Jun 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/tosa.c  | 7 +++----
>>  hw/arm/Kconfig | 1 +
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
>> index 5dee2d76c6..86d7e0283a 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
>> @@ -68,9 +69,6 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
>>  static void tosa_out_switch(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;
> 
> Why convert the blue LED and not the green, amber or wlan LEDs ?

I don't have the schematics to verify the mapping is correct:

#define TOSA_GPIO_BT_LED		(TOSA_SCOOP_JC_GPIO_BASE + 0)
#define TOSA_GPIO_NOTE_LED		(TOSA_SCOOP_JC_GPIO_BASE + 1)
#define TOSA_GPIO_CHRG_ERR_LED		(TOSA_SCOOP_JC_GPIO_BASE + 2)
#define TOSA_GPIO_TC6393XB_L3V_ON	(TOSA_SCOOP_JC_GPIO_BASE + 5)
#define TOSA_GPIO_WLAN_LED		(TOSA_SCOOP_JC_GPIO_BASE + 7)

static void tosa_out_switch(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:
            fprintf(stderr, "Uhandled out event: %d = %d\n", line, level);
            break;
    }
}

    qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]);
    qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]);
    qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]);
    qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]);

I wanted a simple proof-of-concept I could test with a board,
to show how easy this API can be used.

Using the binaries from here:
https://www.omegamoon.com/blog/static.php?page=ZaurusAndroid

the blue LED never blinks, probably because we remove the
bluetooth stack. Or maybe I haven't figured the correct
command line options.

For the next RFC I plan to rework to have a single LED container
per board, to have a unique singleton object that generates QMP
events.

> 
> thanks
> -- PMM
>