| Message ID | 20200910205429.727766-3-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/misc: Add LED device | expand |
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; >
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~
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.
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; >> >
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; >>> >> >
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. >
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.
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...
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.
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.
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
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;
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(-)