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é <f4...@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.mic...@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;
>>
>
