On Tue, 20 Sept 2022 at 19:46, Jian Zhang <zhangjian.3...@bytedance.com> wrote:
>
> Implement a simple latching switch device.
>
> The latching switch is a switch that can be turned on and off.
> When the input new state and match the trigger edge, the switch
> state will be toggled.
>
> This device privide 2 properties `state(bool)` and `trigger-edge(string)`,
> and 2 gpios `input` and `output`.
>
> The `state` property is the initial state of the switch, and the
> `trigger-edge`
> property is the edge that will trigger the switch state to be toggled,
> the value can be `rising`, `falling` or `both`.
> +static void toggle_output(LatchingSwitchState *s)
> +{
> + s->state = !(s->state);
> + qemu_set_irq(s->output, !!(s->state));
s->state is a bool so this !! is unnecessary.
> +}
> +
> +static void input_handler(void *opaque, int line, int new_state)
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(opaque);
> +
> + assert(line == 0);
> +
> + if (s->trigger_edge == TRIGGER_EDGE_FALLING && new_state == 0) {
> + toggle_output(s);
This won't have the correct behaviour, because the device
on the other end of this input line might call
qemu_set_irq() on it twice in a row with new_state == 0 both times.
It's only a falling edge if new_state is 0 and the old
input line state was not 0.
If you need to detect edges then you need to record the state
of the input line within LatchingSwitchState.
> + } else if (s->trigger_edge == TRIGGER_EDGE_RISING && new_state == 1) {
> + toggle_output(s);
> + } else if (s->trigger_edge == TRIGGER_EDGE_BOTH) {
> + toggle_output(s);
> + }
> +}
> +
> +static void latching_switch_reset(DeviceState *dev)
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(dev);
> + /* reset to off */
> + s->state = false;
> + /* reset to falling edge trigger */
> + s->trigger_edge = TRIGGER_EDGE_FALLING;
trigger_edge isn't guest-visible runtime modifiable state, it's how
the device or board configures the latch when it creates it, right?
Configuration shouldn't be reset, because if the board creates a
rising-edge switch, it should stay a rising edge switch even if the
guest power-cycles itself.
(If the QOM property can be changed at runtime things get more
complex, but in this case it can't be.)
> +}
> +
> +static const VMStateDescription vmstate_latching_switch = {
> + .name = TYPE_LATCHING_SWITCH,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_BOOL(state, LatchingSwitchState),
> + VMSTATE_U8(trigger_edge, LatchingSwitchState),
trigger_edge isn't runtime changeable so it doesn't need to be
saved in the vmstate.
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void latching_switch_realize(DeviceState *dev, Error **errp)
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(dev);
> +
> + /* init a input io */
> + qdev_init_gpio_in(dev, input_handler, 1);
> +
> + /* init a output io */
> + qdev_init_gpio_out(dev, &(s->output), 1);
> +}
> +
> +static void latching_switch_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->desc = "Latching Switch";
> + dc->vmsd = &vmstate_latching_switch;
> + dc->reset = latching_switch_reset;
> + dc->realize = latching_switch_realize;
> + set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
This is definitely not a display device :-)
> +}
> +
> +static void latching_switch_get_state(Object *obj, Visitor *v, const char
> *name,
> + void *opaque, Error **errp)
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(obj);
> + bool value = s->state;
> +
> + visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void latching_switch_set_state(Object *obj, Visitor *v, const char
> *name,
> + void *opaque, Error **errp)
What's the requirement for being able to set the output state via a
QOM property ?
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(obj);
> + bool value;
> + Error *err = NULL;
> +
> + visit_type_bool(v, name, &value, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> +
> + if (value != s->state) {
> + toggle_output(s);
This will call qemu_set_irq() on the output, which isn't a valid thing
to do during board creation or in certain stages of reset.
If you need to handle "the board can create a switch which starts off
with its output asserted", that can be done, but it's a little more
complicated (it involves implementing your reset handler as 3-phase-reset).
It looks from patch 3 like your use case in the aspeed board doesn't
require this, so my suggestion would be simply to not implement it:
make the switch always start with the output state being 0.
> + }
> +}
> +static void latching_switch_get_trigger_edge(Object *obj, Visitor *v,
Missing newline between the two functions.
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(obj);
> + int value = s->trigger_edge;
> + char *p = NULL;
> +
> + if (value == TRIGGER_EDGE_FALLING) {
> + p = g_strdup("falling");
> + visit_type_str(v, name, &p, errp);
> + } else if (value == TRIGGER_EDGE_RISING) {
> + p = g_strdup("rising");
> + visit_type_str(v, name, &p, errp);
> + } else if (value == TRIGGER_EDGE_BOTH) {
> + p = g_strdup("both");
> + visit_type_str(v, name, &p, errp);
> + } else {
> + error_setg(errp, "Invalid trigger edge value");
> + }
> + g_free(p);
> +}
> +
> +static void latching_switch_set_trigger_edge(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(obj);
> + char *value;
> + Error *err = NULL;
> +
> + visit_type_str(v, name, &value, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> +
> + if (strcmp(value, "falling") == 0) {
> + s->trigger_edge = TRIGGER_EDGE_FALLING;
> + } else if (strcmp(value, "rising") == 0) {
> + s->trigger_edge = TRIGGER_EDGE_RISING;
> + } else if (strcmp(value, "both") == 0) {
> + s->trigger_edge = TRIGGER_EDGE_BOTH;
> + } else {
> + error_setg(errp, "Invalid trigger edge type: %s", value);
> + }
> +}
> +
> +static void latching_switch_init(Object *obj)
> +{
> + LatchingSwitchState *s = LATCHING_SWITCH(obj);
> +
> + s->state = false;
> + s->trigger_edge = TRIGGER_EDGE_FALLING;
> +
> + object_property_add(obj, "state", "bool",
> + latching_switch_get_state,
> + latching_switch_set_state,
> + NULL, NULL);
> + object_property_add(obj, "trigger-edge", "string",
> + latching_switch_get_trigger_edge,
> + latching_switch_set_trigger_edge,
> + NULL, NULL);
> +}
> +
> +static const TypeInfo latching_switch_info = {
> + .name = TYPE_LATCHING_SWITCH,
> + .parent = TYPE_DEVICE,
Don't implement new devices as direct child types of TYPE_DEVICE.
At the moment that makes them never get reset. Make it a child
of TYPE_SYS_BUS_DEVICE instead.
> + .instance_size = sizeof(LatchingSwitchState),
> + .class_init = latching_switch_class_init,
> + .instance_init = latching_switch_init,
> +};
> +
> +static void latching_switch_register_types(void)
> +{
> + type_register_static(&latching_switch_info);
> +}
> +
> +type_init(latching_switch_register_types);
> +
> +LatchingSwitchState *latching_switch_create_simple(Object *parentobj,
> + bool state,
> + uint8_t trigger_edge)
> +{
> + static const char *name = "latching-switch";
> + DeviceState *dev;
> +
> + dev = qdev_new(TYPE_LATCHING_SWITCH);
> +
> + qdev_prop_set_bit(dev, "state", state);
> +
> + if (trigger_edge == TRIGGER_EDGE_FALLING) {
> + qdev_prop_set_string(dev, "trigger-edge", "falling");
> + } else if (trigger_edge == TRIGGER_EDGE_RISING) {
> + qdev_prop_set_string(dev, "trigger-edge", "rising");
> + } else if (trigger_edge == TRIGGER_EDGE_BOTH) {
> + qdev_prop_set_string(dev, "trigger-edge", "both");
> + } else {
> + error_report("Invalid trigger edge value");
> + exit(1);
> + }
> +
> + object_property_add_child(parentobj, name, OBJECT(dev));
> + qdev_realize_and_unref(dev, NULL, &error_fatal);
Current QEMU style doesn't favour providing this sort of
create-and-configure-the-device wrapper function. Instead just
create and set the properties on the device in the board code.
> +
> + return LATCHING_SWITCH(dev);
> +}
thanks
-- PMM
