On 6/15/20 12:55 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (f4...@amsat.org) wrote:
>> A LED device can be connected to a GPIO output.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4...@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é <f4...@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é <f4...@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é <f4...@amsat.org>
>> +S: Maintained
>> +F: include/hw/misc/led.h
>> +F: hw/misc/led.c
>> +
>> Unimplemented device
>> M: Peter Maydell <peter.mayd...@linaro.org>
>> R: Philippe Mathieu-Daudé <f4...@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 / dgilb...@redhat.com / Manchester, UK
>
