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

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

* 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

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
> 

* 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