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

[Philippe Mathieu-Daudé]

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;
>>>
>>
>