Re: [PATCH v6 2/7] hw/misc/led: Allow connecting from GPIO output

[Luc Michel <luc.michel@greensocs.com>]

On 9/12/20 3:40 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>

Reviewed-by: Luc Michel <luc.michel@greensocs.com>

> ---
>   include/hw/misc/led.h  | 10 ++++++++++
>   include/hw/qdev-core.h | 16 ++++++++++++++++
>   hw/misc/led.c          | 17 ++++++++++++++++-
>   3 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> index 286d37c75c1..aa359b87c20 100644
> --- a/include/hw/misc/led.h
> +++ b/include/hw/misc/led.h
> @@ -9,6 +9,7 @@
>   #define HW_MISC_LED_H
>   
>   #include "qom/object.h"
> +#include "hw/qdev-core.h"
>   
>   #define TYPE_LED "led"
>   
> @@ -37,10 +38,17 @@ struct LEDState {
>       /* Public */
>   
>       uint8_t intensity_percent;
> +    qemu_irq irq;
>   
>       /* Properties */
>       char *description;
>       char *color;
> +    /*
> +     * Determines whether a GPIO is using a positive (active-high)
> +     * logic (when used with GPIO, the intensity at reset is related
> +     * to the GPIO polarity).
> +     */
> +    bool gpio_active_high;
>   };
>   typedef struct LEDState LEDState;
>   DECLARE_INSTANCE_CHECKER(LEDState, LED, TYPE_LED)
> @@ -72,6 +80,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)
>    *
> @@ -81,6 +90,7 @@ void led_set_state(LEDState *s, bool is_emitting);
>    * Returns: The newly allocated and instantiated LED object.
>    */
>   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 e025ba9653f..ec1f1efc37b 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -423,6 +423,22 @@ 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
> + *
> + * GPIO lines use either positive (active-high) logic,
> + * or negative (active-low) logic.
> + *
> + * In active-high logic (%GPIO_POLARITY_ACTIVE_HIGH), a pin is
> + * active when the voltage on the pin is high (relative to ground);
> + * whereas in active-low logic (%GPIO_POLARITY_ACTIVE_LOW), a pin
> + * is active when the voltage on the pin is low (or grounded).
> + */
> +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 1e2f49c5710..c5fa09a613a 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->gpio_active_high);
> +}
> +
>   static void led_reset(DeviceState *dev)
>   {
>       LEDState *s = LED(dev);
>   
> -    led_set_state(s, false);
> +    led_set_state(s, s->gpio_active_high);
>   }
>   
>   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("gpio-active-high", LEDState, gpio_active_high, true),
>       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, "gpio-active-high",
> +                      gpio_polarity == GPIO_POLARITY_ACTIVE_HIGH);
>       qdev_prop_set_string(dev, "color", led_color_name[color]);
>       if (!description) {
>           static unsigned undescribed_led_id;
>