[PATCH 0/2] misc/pca9552: Changes to support powernv10

[PATCH 0/2] misc/pca9552: Changes to support powernv10

[Glenn Miles]

This is a series of patches targeted at getting the pca9552
model ready for use by the powernv10 machine.

Glenn Miles (2):
  misc/pca9552: Fix inverted input status
  misc/pca9552: Let external devices set pca9552 inputs

 hw/misc/pca9552.c          | 51 +++++++++++++++++++++++++++++++++-----
 include/hw/misc/pca9552.h  |  3 ++-
 tests/qtest/pca9552-test.c |  6 ++---
 3 files changed, 50 insertions(+), 10 deletions(-)

-- 
2.31.1

[PATCH 1/2] misc/pca9552: Fix inverted input status

[Glenn Miles]

The pca9552 INPUT0 and INPUT1 registers are supposed to
hold the logical values of the LED pins.  A logical 0
should be seen in the INPUT0/1 registers for a pin when
its corresponding LSn bits are set to 0, which is also
the state needed for turning on an LED in a typical
usage scenario.  Existing code was doing the opposite
and setting INPUT0/1 bit to a 1 when the LSn bit was
set to 0, so this commit fixes that.

Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

Changes from prior version:
    - Added comment regarding pca953X
    - Added cover letter

 hw/misc/pca9552.c          | 18 +++++++++++++-----
 tests/qtest/pca9552-test.c |  6 +++---
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index fff19e369a..445f56a9e8 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
 
 DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
                        TYPE_PCA955X)
-
+/*
+ * Note:  The LED_ON and LED_OFF configuration values for the PCA955X
+ *        chips are the reverse of the PCA953X family of chips.
+ */
 #define PCA9552_LED_ON   0x0
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
@@ -112,13 +115,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
 
         switch (config) {
         case PCA9552_LED_ON:
-            qemu_set_irq(s->gpio[i], 1);
-            s->regs[input_reg] |= 1 << input_shift;
-            break;
-        case PCA9552_LED_OFF:
+            /* Pin is set to 0V to turn on LED */
             qemu_set_irq(s->gpio[i], 0);
             s->regs[input_reg] &= ~(1 << input_shift);
             break;
+        case PCA9552_LED_OFF:
+            /*
+             * Pin is set to Hi-Z to turn off LED and
+             * pullup sets it to a logical 1.
+             */
+            qemu_set_irq(s->gpio[i], 1);
+            s->regs[input_reg] |= 1 << input_shift;
+            break;
         case PCA9552_LED_PWM0:
         case PCA9552_LED_PWM1:
             /* TODO */
diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552-test.c
index d80ed93cd3..ccca2b3d91 100644
--- a/tests/qtest/pca9552-test.c
+++ b/tests/qtest/pca9552-test.c
@@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
     g_assert_cmphex(value, ==, 0x55);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT0);
-    g_assert_cmphex(value, ==, 0x0);
+    g_assert_cmphex(value, ==, 0xFF);
 
     pca9552_init(i2cdev);
 
@@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
     g_assert_cmphex(value, ==, 0x54);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT0);
-    g_assert_cmphex(value, ==, 0x01);
+    g_assert_cmphex(value, ==, 0xFE);
 
     value = i2c_get8(i2cdev, PCA9552_LS3);
     g_assert_cmphex(value, ==, 0x54);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT1);
-    g_assert_cmphex(value, ==, 0x10);
+    g_assert_cmphex(value, ==, 0xEF);
 }
 
 static void pca9552_register_nodes(void)
-- 
2.31.1

[PATCH 2/2] misc/pca9552: Let external devices set pca9552 inputs

[Glenn Miles]

Allow external devices to drive pca9552 input pins by adding
input GPIO's to the model.  This allows a device to connect
its output GPIO's to the pca9552 input GPIO's.

In order for an external device to set the state of a pca9552
pin, the pin must first be configured for high impedance (LED
is off).  If the pca9552 pin is configured to drive the pin low
(LED is on), then external input will be ignored.

Here is a table describing the logical state of a pca9552 pin
given the state being driven by the pca9552 and an external device:

                   PCA9552
                   Configured
                   State

                  | Hi-Z | Low |
            ------+------+-----+
  External   Hi-Z |  Hi  | Low |
  Device    ------+------+-----+
  State      Low  |  Low | Low |
            ------+------+-----+

Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

Changes from previous version:
 - Added #define's for external state values
 - Added logic state table to commit message
 - Added cover letter

 hw/misc/pca9552.c         | 41 ++++++++++++++++++++++++++++++++++-----
 include/hw/misc/pca9552.h |  3 ++-
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 445f56a9e8..ed814d1f98 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -44,6 +44,8 @@ DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
 #define PCA9552_LED_PWM1 0x3
+#define PCA9552_PIN_LOW  0x0
+#define PCA9552_PIN_HIZ  0x1
 
 static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
 
@@ -116,16 +118,22 @@ static void pca955x_update_pin_input(PCA955xState *s)
         switch (config) {
         case PCA9552_LED_ON:
             /* Pin is set to 0V to turn on LED */
-            qemu_set_irq(s->gpio[i], 0);
+            qemu_set_irq(s->gpio_out[i], 0);
             s->regs[input_reg] &= ~(1 << input_shift);
             break;
         case PCA9552_LED_OFF:
             /*
              * Pin is set to Hi-Z to turn off LED and
-             * pullup sets it to a logical 1.
+             * pullup sets it to a logical 1 unless
+             * external device drives it low.
              */
-            qemu_set_irq(s->gpio[i], 1);
-            s->regs[input_reg] |= 1 << input_shift;
+            if (s->ext_state[i] == PCA9552_PIN_LOW) {
+                qemu_set_irq(s->gpio_out[i], 0);
+                s->regs[input_reg] &= ~(1 << input_shift);
+            } else {
+                qemu_set_irq(s->gpio_out[i], 1);
+                s->regs[input_reg] |= 1 << input_shift;
+            }
             break;
         case PCA9552_LED_PWM0:
         case PCA9552_LED_PWM1:
@@ -340,6 +348,7 @@ static const VMStateDescription pca9552_vmstate = {
         VMSTATE_UINT8(len, PCA955xState),
         VMSTATE_UINT8(pointer, PCA955xState),
         VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
+        VMSTATE_UINT8_ARRAY(ext_state, PCA955xState, PCA955X_PIN_COUNT_MAX),
         VMSTATE_I2C_SLAVE(i2c, PCA955xState),
         VMSTATE_END_OF_LIST()
     }
@@ -358,6 +367,7 @@ static void pca9552_reset(DeviceState *dev)
     s->regs[PCA9552_LS2] = 0x55;
     s->regs[PCA9552_LS3] = 0x55;
 
+    memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
     pca955x_update_pin_input(s);
 
     s->pointer = 0xFF;
@@ -380,6 +390,26 @@ static void pca955x_initfn(Object *obj)
     }
 }
 
+static void pca955x_set_ext_state(PCA955xState *s, int pin, int level)
+{
+    if (s->ext_state[pin] != level) {
+        uint16_t pins_status = pca955x_pins_get_status(s);
+        s->ext_state[pin] = level;
+        pca955x_update_pin_input(s);
+        pca955x_display_pins_status(s, pins_status);
+    }
+}
+
+static void pca955x_gpio_in_handler(void *opaque, int pin, int level)
+{
+
+    PCA955xState *s = PCA955X(opaque);
+    PCA955xClass *k = PCA955X_GET_CLASS(s);
+
+    assert((pin >= 0) && (pin < k->pin_count));
+    pca955x_set_ext_state(s, pin, level);
+}
+
 static void pca955x_realize(DeviceState *dev, Error **errp)
 {
     PCA955xClass *k = PCA955X_GET_CLASS(dev);
@@ -389,7 +419,8 @@ static void pca955x_realize(DeviceState *dev, Error **errp)
         s->description = g_strdup("pca-unspecified");
     }
 
-    qdev_init_gpio_out(dev, s->gpio, k->pin_count);
+    qdev_init_gpio_out(dev, s->gpio_out, k->pin_count);
+    qdev_init_gpio_in(dev, pca955x_gpio_in_handler, k->pin_count);
 }
 
 static Property pca955x_properties[] = {
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index b6f4e264fe..c36525f0c3 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -30,7 +30,8 @@ struct PCA955xState {
     uint8_t pointer;
 
     uint8_t regs[PCA955X_NR_REGS];
-    qemu_irq gpio[PCA955X_PIN_COUNT_MAX];
+    qemu_irq gpio_out[PCA955X_PIN_COUNT_MAX];
+    uint8_t ext_state[PCA955X_PIN_COUNT_MAX];
     char *description; /* For debugging purpose only */
 };
 
-- 
2.31.1

Re: [PATCH 1/2] misc/pca9552: Fix inverted input status

[Andrew Jeffery]

On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> > The pca9552 INPUT0 and INPUT1 registers are supposed to
> > hold the logical values of the LED pins.  A logical 0
> > should be seen in the INPUT0/1 registers for a pin when
> > its corresponding LSn bits are set to 0, which is also
> > the state needed for turning on an LED in a typical
> > usage scenario.  Existing code was doing the opposite
> > and setting INPUT0/1 bit to a 1 when the LSn bit was
> > set to 0, so this commit fixes that.
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> > 
> > Changes from prior version:
> >     - Added comment regarding pca953X
> >     - Added cover letter
> > 
> >  hw/misc/pca9552.c          | 18 +++++++++++++-----
> >  tests/qtest/pca9552-test.c |  6 +++---
> >  2 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index fff19e369a..445f56a9e8 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
> >  
> >  DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> >                         TYPE_PCA955X)
> > -
> > +/*
> > + * Note:  The LED_ON and LED_OFF configuration values for the PCA955X
> > + *        chips are the reverse of the PCA953X family of chips.
> > + */
> >  #define PCA9552_LED_ON   0x0
> >  #define PCA9552_LED_OFF  0x1
> >  #define PCA9552_LED_PWM0 0x2
> > @@ -112,13 +115,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
> >  
> >          switch (config) {
> >          case PCA9552_LED_ON:
> > -            qemu_set_irq(s->gpio[i], 1);
> > -            s->regs[input_reg] |= 1 << input_shift;
> > -            break;
> > -        case PCA9552_LED_OFF:
> > +            /* Pin is set to 0V to turn on LED */
> >              qemu_set_irq(s->gpio[i], 0);
> >              s->regs[input_reg] &= ~(1 << input_shift);
> >              break;
> > +        case PCA9552_LED_OFF:
> > +            /*
> > +             * Pin is set to Hi-Z to turn off LED and
> > +             * pullup sets it to a logical 1.
> > +             */
> > +            qemu_set_irq(s->gpio[i], 1);
> > +            s->regs[input_reg] |= 1 << input_shift;
> > +            break;

So the witherspoon-bmc machine was a user of the PCA9552 outputs as
LEDs. I guess its LEDs were in the wrong state the whole time? That
looks like the only user though, and shouldn't be negatively affected.

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

> >          case PCA9552_LED_PWM0:
> >          case PCA9552_LED_PWM1:
> >              /* TODO */
> > diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552-test.c
> > index d80ed93cd3..ccca2b3d91 100644
> > --- a/tests/qtest/pca9552-test.c
> > +++ b/tests/qtest/pca9552-test.c
> > @@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
> >      g_assert_cmphex(value, ==, 0x55);
> >  
> >      value = i2c_get8(i2cdev, PCA9552_INPUT0);
> > -    g_assert_cmphex(value, ==, 0x0);
> > +    g_assert_cmphex(value, ==, 0xFF);
> >  
> >      pca9552_init(i2cdev);
> >  
> > @@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
> >      g_assert_cmphex(value, ==, 0x54);
> >  
> >      value = i2c_get8(i2cdev, PCA9552_INPUT0);
> > -    g_assert_cmphex(value, ==, 0x01);
> > +    g_assert_cmphex(value, ==, 0xFE);
> >  
> >      value = i2c_get8(i2cdev, PCA9552_LS3);
> >      g_assert_cmphex(value, ==, 0x54);
> >  
> >      value = i2c_get8(i2cdev, PCA9552_INPUT1);
> > -    g_assert_cmphex(value, ==, 0x10);
> > +    g_assert_cmphex(value, ==, 0xEF);
> >  }
> >  
> >  static void pca9552_register_nodes(void)

Re: [PATCH 2/2] misc/pca9552: Let external devices set pca9552 inputs

[Andrew Jeffery]

On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> Allow external devices to drive pca9552 input pins by adding
> input GPIO's to the model.  This allows a device to connect
> its output GPIO's to the pca9552 input GPIO's.
> 
> In order for an external device to set the state of a pca9552
> pin, the pin must first be configured for high impedance (LED
> is off).  If the pca9552 pin is configured to drive the pin low
> (LED is on), then external input will be ignored.
> 
> Here is a table describing the logical state of a pca9552 pin
> given the state being driven by the pca9552 and an external device:
> 
>                    PCA9552
>                    Configured
>                    State
> 
>                   | Hi-Z | Low |
>             ------+------+-----+
>   External   Hi-Z |  Hi  | Low |
>   Device    ------+------+-----+
>   State      Low  |  Low | Low |
>             ------+------+-----+
> 
> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> ---
> 
> Changes from previous version:
>  - Added #define's for external state values
>  - Added logic state table to commit message
>  - Added cover letter
> 
>  hw/misc/pca9552.c         | 41 ++++++++++++++++++++++++++++++++++-----
>  include/hw/misc/pca9552.h |  3 ++-
>  2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 445f56a9e8..ed814d1f98 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -44,6 +44,8 @@ DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
>  #define PCA9552_LED_OFF  0x1
>  #define PCA9552_LED_PWM0 0x2
>  #define PCA9552_LED_PWM1 0x3
> +#define PCA9552_PIN_LOW  0x0
> +#define PCA9552_PIN_HIZ  0x1
>  
>  static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
>  
> @@ -116,16 +118,22 @@ static void pca955x_update_pin_input(PCA955xState *s)
>          switch (config) {
>          case PCA9552_LED_ON:
>              /* Pin is set to 0V to turn on LED */
> -            qemu_set_irq(s->gpio[i], 0);
> +            qemu_set_irq(s->gpio_out[i], 0);

pca955x_update_pin_input() is called by the external GPIO handling path
as well as the I2C command handling path. If the I2C path sets the line
low followed by the device attached to the GPIO setting the line low
then I think each change will issue an event on the outbound GPIO. Is
that desired behaviour? Does it matter?

>              s->regs[input_reg] &= ~(1 << input_shift);
>              break;
>          case PCA9552_LED_OFF:
>              /*
>               * Pin is set to Hi-Z to turn off LED and
> -             * pullup sets it to a logical 1.
> +             * pullup sets it to a logical 1 unless
> +             * external device drives it low.
>               */
> -            qemu_set_irq(s->gpio[i], 1);
> -            s->regs[input_reg] |= 1 << input_shift;
> +            if (s->ext_state[i] == PCA9552_PIN_LOW) {
> +                qemu_set_irq(s->gpio_out[i], 0);

Similarly here - it might be the case that both devices have pulled the
line low and now the I2C path is releasing it. Given the external
device had already pulled the line low as well should we expect an
event to occur issued here? Should it matter?

Andrew

> +                s->regs[input_reg] &= ~(1 << input_shift);
> +            } else {
> +                qemu_set_irq(s->gpio_out[i], 1);
> +                s->regs[input_reg] |= 1 << input_shift;
> +            }
>              break;
>          case PCA9552_LED_PWM0:
>          case PCA9552_LED_PWM1:
> @@ -340,6 +348,7 @@ static const VMStateDescription pca9552_vmstate = {
>          VMSTATE_UINT8(len, PCA955xState),
>          VMSTATE_UINT8(pointer, PCA955xState),
>          VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
> +        VMSTATE_UINT8_ARRAY(ext_state, PCA955xState, PCA955X_PIN_COUNT_MAX),
>          VMSTATE_I2C_SLAVE(i2c, PCA955xState),
>          VMSTATE_END_OF_LIST()
>      }
> @@ -358,6 +367,7 @@ static void pca9552_reset(DeviceState *dev)
>      s->regs[PCA9552_LS2] = 0x55;
>      s->regs[PCA9552_LS3] = 0x55;
>  
> +    memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
>      pca955x_update_pin_input(s);
>  
>      s->pointer = 0xFF;
> @@ -380,6 +390,26 @@ static void pca955x_initfn(Object *obj)
>      }
>  }
>  
> +static void pca955x_set_ext_state(PCA955xState *s, int pin, int level)
> +{
> +    if (s->ext_state[pin] != level) {
> +        uint16_t pins_status = pca955x_pins_get_status(s);
> +        s->ext_state[pin] = level;
> +        pca955x_update_pin_input(s);
> +        pca955x_display_pins_status(s, pins_status);
> +    }
> +}
> +
> +static void pca955x_gpio_in_handler(void *opaque, int pin, int level)
> +{
> +
> +    PCA955xState *s = PCA955X(opaque);
> +    PCA955xClass *k = PCA955X_GET_CLASS(s);
> +
> +    assert((pin >= 0) && (pin < k->pin_count));
> +    pca955x_set_ext_state(s, pin, level);
> +}
> +
>  static void pca955x_realize(DeviceState *dev, Error **errp)
>  {
>      PCA955xClass *k = PCA955X_GET_CLASS(dev);
> @@ -389,7 +419,8 @@ static void pca955x_realize(DeviceState *dev, Error **errp)
>          s->description = g_strdup("pca-unspecified");
>      }
>  
> -    qdev_init_gpio_out(dev, s->gpio, k->pin_count);
> +    qdev_init_gpio_out(dev, s->gpio_out, k->pin_count);
> +    qdev_init_gpio_in(dev, pca955x_gpio_in_handler, k->pin_count);
>  }
>  
>  static Property pca955x_properties[] = {
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index b6f4e264fe..c36525f0c3 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -30,7 +30,8 @@ struct PCA955xState {
>      uint8_t pointer;
>  
>      uint8_t regs[PCA955X_NR_REGS];
> -    qemu_irq gpio[PCA955X_PIN_COUNT_MAX];
> +    qemu_irq gpio_out[PCA955X_PIN_COUNT_MAX];
> +    uint8_t ext_state[PCA955X_PIN_COUNT_MAX];
>      char *description; /* For debugging purpose only */
>  };
>  

Re: [PATCH 1/2] misc/pca9552: Fix inverted input status

[Philippe Mathieu-Daudé]

On 20/10/23 04:51, Andrew Jeffery wrote:
> On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
>>> The pca9552 INPUT0 and INPUT1 registers are supposed to
>>> hold the logical values of the LED pins.  A logical 0
>>> should be seen in the INPUT0/1 registers for a pin when
>>> its corresponding LSn bits are set to 0, which is also
>>> the state needed for turning on an LED in a typical
>>> usage scenario.  Existing code was doing the opposite
>>> and setting INPUT0/1 bit to a 1 when the LSn bit was
>>> set to 0, so this commit fixes that.
>>>
>>> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
>>> ---
>>>
>>> Changes from prior version:
>>>      - Added comment regarding pca953X
>>>      - Added cover letter
>>>
>>>   hw/misc/pca9552.c          | 18 +++++++++++++-----
>>>   tests/qtest/pca9552-test.c |  6 +++---
>>>   2 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>>> index fff19e369a..445f56a9e8 100644
>>> --- a/hw/misc/pca9552.c
>>> +++ b/hw/misc/pca9552.c
>>> @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
>>>   
>>>   DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
>>>                          TYPE_PCA955X)
>>> -
>>> +/*
>>> + * Note:  The LED_ON and LED_OFF configuration values for the PCA955X
>>> + *        chips are the reverse of the PCA953X family of chips.
>>> + */
>>>   #define PCA9552_LED_ON   0x0
>>>   #define PCA9552_LED_OFF  0x1
>>>   #define PCA9552_LED_PWM0 0x2
>>> @@ -112,13 +115,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
>>>   
>>>           switch (config) {
>>>           case PCA9552_LED_ON:
>>> -            qemu_set_irq(s->gpio[i], 1);
>>> -            s->regs[input_reg] |= 1 << input_shift;
>>> -            break;
>>> -        case PCA9552_LED_OFF:
>>> +            /* Pin is set to 0V to turn on LED */
>>>               qemu_set_irq(s->gpio[i], 0);
>>>               s->regs[input_reg] &= ~(1 << input_shift);
>>>               break;
>>> +        case PCA9552_LED_OFF:
>>> +            /*
>>> +             * Pin is set to Hi-Z to turn off LED and
>>> +             * pullup sets it to a logical 1.
>>> +             */
>>> +            qemu_set_irq(s->gpio[i], 1);
>>> +            s->regs[input_reg] |= 1 << input_shift;
>>> +            break;
> 
> So the witherspoon-bmc machine was a user of the PCA9552 outputs as
> LEDs. I guess its LEDs were in the wrong state the whole time? That
> looks like the only user though, and shouldn't be negatively affected.

Usually GPIO polarity is a machine/board property, not a device
one.

We could use the LED API (hw/misc/led.h) which initialize each
output with GpioPolarity.

Re: [PATCH 1/2] misc/pca9552: Fix inverted input status

[Miles Glenn]

On Fri, 2023-10-20 at 11:42 +0200, Philippe Mathieu-Daudé wrote:
> On 20/10/23 04:51, Andrew Jeffery wrote:
> > On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> > > > The pca9552 INPUT0 and INPUT1 registers are supposed to
> > > > hold the logical values of the LED pins.  A logical 0
> > > > should be seen in the INPUT0/1 registers for a pin when
> > > > its corresponding LSn bits are set to 0, which is also
> > > > the state needed for turning on an LED in a typical
> > > > usage scenario.  Existing code was doing the opposite
> > > > and setting INPUT0/1 bit to a 1 when the LSn bit was
> > > > set to 0, so this commit fixes that.
> > > > 
> > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > > > ---
> > > > 
> > > > Changes from prior version:
> > > >      - Added comment regarding pca953X
> > > >      - Added cover letter
> > > > 
> > > >   hw/misc/pca9552.c          | 18 +++++++++++++-----
> > > >   tests/qtest/pca9552-test.c |  6 +++---
> > > >   2 files changed, 16 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > > > index fff19e369a..445f56a9e8 100644
> > > > --- a/hw/misc/pca9552.c
> > > > +++ b/hw/misc/pca9552.c
> > > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
> > > >   
> > > >   DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> > > >                          TYPE_PCA955X)
> > > > -
> > > > +/*
> > > > + * Note:  The LED_ON and LED_OFF configuration values for the
> > > > PCA955X
> > > > + *        chips are the reverse of the PCA953X family of
> > > > chips.
> > > > + */
> > > >   #define PCA9552_LED_ON   0x0
> > > >   #define PCA9552_LED_OFF  0x1
> > > >   #define PCA9552_LED_PWM0 0x2
> > > > @@ -112,13 +115,18 @@ static void
> > > > pca955x_update_pin_input(PCA955xState *s)
> > > >   
> > > >           switch (config) {
> > > >           case PCA9552_LED_ON:
> > > > -            qemu_set_irq(s->gpio[i], 1);
> > > > -            s->regs[input_reg] |= 1 << input_shift;
> > > > -            break;
> > > > -        case PCA9552_LED_OFF:
> > > > +            /* Pin is set to 0V to turn on LED */
> > > >               qemu_set_irq(s->gpio[i], 0);
> > > >               s->regs[input_reg] &= ~(1 << input_shift);
> > > >               break;
> > > > +        case PCA9552_LED_OFF:
> > > > +            /*
> > > > +             * Pin is set to Hi-Z to turn off LED and
> > > > +             * pullup sets it to a logical 1.
> > > > +             */
> > > > +            qemu_set_irq(s->gpio[i], 1);
> > > > +            s->regs[input_reg] |= 1 << input_shift;
> > > > +            break;
> > 
> > So the witherspoon-bmc machine was a user of the PCA9552 outputs as
> > LEDs. I guess its LEDs were in the wrong state the whole time? That
> > looks like the only user though, and shouldn't be negatively
> > affected.
> 
> Usually GPIO polarity is a machine/board property, not a device
> one.
> 
> We could use the LED API (hw/misc/led.h) which initialize each
> output with GpioPolarity.
> 

Thanks for your comments!   This piqued my curiosity so I decided to
run a test with the witherspoon-bmc machine.  Without my changes, I ran
the following command to turn off LED13 on the pca9552 which I had
previously set to "on":

  qom-set /machine/unattached/device[18] led13 off

I had GDB connected at the time with a breakpoint set on
led_set_state() so that I could see what was happening.  Due to the
inversion bug, I expected the pca9552 code to set the pin low and also
set the irq low, which it did.  The connected LED's on this pca9552
were all configured as GPIO_POLARITY_ACTIVE_LOW, so I expected that
setting the irq state low would actually turn on the LED.  Instead it
turned off the LED.

Reviewing the LED code, I believe the problem lies here:

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


In my test, new_state was 0 and gpio_active_high was 0, resulting in
the boolean expression of ( 0 != 0 ) which is false and results in
turning off the LED.  So, this looks like a bug to me.

For another simpler example, if the LED polarity was set to
GPIO_POLARITY_ACTIVE_HIGH, then s->gpio_active_high would be 1.  Then,
if we set the irq line to 1, wouldn't we expect the LED to turn on? 
However, as the code stands, it would actually turn the LED off.  So, I
think we can remove one of the "!"'s from in front of new_state.  Then,
if the LED is active high and the irq line is set high, it would turn
on the LED.  Correct?

Thanks,

Glenn

Re: [PATCH 2/2] misc/pca9552: Let external devices set pca9552 inputs

[Miles Glenn]

On Fri, 2023-10-20 at 15:18 +1030, Andrew Jeffery wrote:
> On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> > Allow external devices to drive pca9552 input pins by adding
> > input GPIO's to the model.  This allows a device to connect
> > its output GPIO's to the pca9552 input GPIO's.
> > 
> > In order for an external device to set the state of a pca9552
> > pin, the pin must first be configured for high impedance (LED
> > is off).  If the pca9552 pin is configured to drive the pin low
> > (LED is on), then external input will be ignored.
> > 
> > Here is a table describing the logical state of a pca9552 pin
> > given the state being driven by the pca9552 and an external device:
> > 
> >                    PCA9552
> >                    Configured
> >                    State
> > 
> >                   | Hi-Z | Low |
> >             ------+------+-----+
> >   External   Hi-Z |  Hi  | Low |
> >   Device    ------+------+-----+
> >   State      Low  |  Low | Low |
> >             ------+------+-----+
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> > 
> > Changes from previous version:
> >  - Added #define's for external state values
> >  - Added logic state table to commit message
> >  - Added cover letter
> > 
> >  hw/misc/pca9552.c         | 41 ++++++++++++++++++++++++++++++++++-
> > ----
> >  include/hw/misc/pca9552.h |  3 ++-
> >  2 files changed, 38 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index 445f56a9e8..ed814d1f98 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -44,6 +44,8 @@ DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> >  #define PCA9552_LED_OFF  0x1
> >  #define PCA9552_LED_PWM0 0x2
> >  #define PCA9552_LED_PWM1 0x3
> > +#define PCA9552_PIN_LOW  0x0
> > +#define PCA9552_PIN_HIZ  0x1
> >  
> >  static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
> >  
> > @@ -116,16 +118,22 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> >          switch (config) {
> >          case PCA9552_LED_ON:
> >              /* Pin is set to 0V to turn on LED */
> > -            qemu_set_irq(s->gpio[i], 0);
> > +            qemu_set_irq(s->gpio_out[i], 0);
> 
> pca955x_update_pin_input() is called by the external GPIO handling
> path
> as well as the I2C command handling path. If the I2C path sets the
> line
> low followed by the device attached to the GPIO setting the line low
> then I think each change will issue an event on the outbound GPIO. Is
> that desired behaviour? Does it matter?
> 

I think these questions probably depend a lot on the recieving device,
but at best, I think it's inefficient and at worst, depending on the
recieving device, it could lead to bugs, so I'll add a check.

 
> >              s->regs[input_reg] &= ~(1 << input_shift);
> >              break;
> >          case PCA9552_LED_OFF:
> >              /*
> >               * Pin is set to Hi-Z to turn off LED and
> > -             * pullup sets it to a logical 1.
> > +             * pullup sets it to a logical 1 unless
> > +             * external device drives it low.
> >               */
> > -            qemu_set_irq(s->gpio[i], 1);
> > -            s->regs[input_reg] |= 1 << input_shift;
> > +            if (s->ext_state[i] == PCA9552_PIN_LOW) {
> > +                qemu_set_irq(s->gpio_out[i], 0);
> 
> Similarly here - it might be the case that both devices have pulled
> the
> line low and now the I2C path is releasing it. Given the external
> device had already pulled the line low as well should we expect an
> event to occur issued here? Should it matter?
> 
> Andrew
> 

See previous response.

Thanks for the review!

Glenn

> > +                s->regs[input_reg] &= ~(1 << input_shift);
> > +            } else {
> > +                qemu_set_irq(s->gpio_out[i], 1);
> > +                s->regs[input_reg] |= 1 << input_shift;
> > +            }
> >              break;
> >          case PCA9552_LED_PWM0:
> >          case PCA9552_LED_PWM1:
> > @@ -340,6 +348,7 @@ static const VMStateDescription pca9552_vmstate
> > = {
> >          VMSTATE_UINT8(len, PCA955xState),
> >          VMSTATE_UINT8(pointer, PCA955xState),
> >          VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
> > +        VMSTATE_UINT8_ARRAY(ext_state, PCA955xState,
> > PCA955X_PIN_COUNT_MAX),
> >          VMSTATE_I2C_SLAVE(i2c, PCA955xState),
> >          VMSTATE_END_OF_LIST()
> >      }
> > @@ -358,6 +367,7 @@ static void pca9552_reset(DeviceState *dev)
> >      s->regs[PCA9552_LS2] = 0x55;
> >      s->regs[PCA9552_LS3] = 0x55;
> >  
> > +    memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
> >      pca955x_update_pin_input(s);
> >  
> >      s->pointer = 0xFF;
> > @@ -380,6 +390,26 @@ static void pca955x_initfn(Object *obj)
> >      }
> >  }
> >  
> > +static void pca955x_set_ext_state(PCA955xState *s, int pin, int
> > level)
> > +{
> > +    if (s->ext_state[pin] != level) {
> > +        uint16_t pins_status = pca955x_pins_get_status(s);
> > +        s->ext_state[pin] = level;
> > +        pca955x_update_pin_input(s);
> > +        pca955x_display_pins_status(s, pins_status);
> > +    }
> > +}
> > +
> > +static void pca955x_gpio_in_handler(void *opaque, int pin, int
> > level)
> > +{
> > +
> > +    PCA955xState *s = PCA955X(opaque);
> > +    PCA955xClass *k = PCA955X_GET_CLASS(s);
> > +
> > +    assert((pin >= 0) && (pin < k->pin_count));
> > +    pca955x_set_ext_state(s, pin, level);
> > +}
> > +
> >  static void pca955x_realize(DeviceState *dev, Error **errp)
> >  {
> >      PCA955xClass *k = PCA955X_GET_CLASS(dev);
> > @@ -389,7 +419,8 @@ static void pca955x_realize(DeviceState *dev,
> > Error **errp)
> >          s->description = g_strdup("pca-unspecified");
> >      }
> >  
> > -    qdev_init_gpio_out(dev, s->gpio, k->pin_count);
> > +    qdev_init_gpio_out(dev, s->gpio_out, k->pin_count);
> > +    qdev_init_gpio_in(dev, pca955x_gpio_in_handler, k->pin_count);
> >  }
> >  
> >  static Property pca955x_properties[] = {
> > diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> > index b6f4e264fe..c36525f0c3 100644
> > --- a/include/hw/misc/pca9552.h
> > +++ b/include/hw/misc/pca9552.h
> > @@ -30,7 +30,8 @@ struct PCA955xState {
> >      uint8_t pointer;
> >  
> >      uint8_t regs[PCA955X_NR_REGS];
> > -    qemu_irq gpio[PCA955X_PIN_COUNT_MAX];
> > +    qemu_irq gpio_out[PCA955X_PIN_COUNT_MAX];
> > +    uint8_t ext_state[PCA955X_PIN_COUNT_MAX];
> >      char *description; /* For debugging purpose only */
> >  };
> >  

Re: [PATCH 1/2] misc/pca9552: Fix inverted input status

[Andrew Jeffery]

On Fri, 2023-10-20 at 11:32 -0500, Miles Glenn wrote:
> On Fri, 2023-10-20 at 11:42 +0200, Philippe Mathieu-Daudé wrote:
> > On 20/10/23 04:51, Andrew Jeffery wrote:
> > > On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> > > > > The pca9552 INPUT0 and INPUT1 registers are supposed to
> > > > > hold the logical values of the LED pins.  A logical 0
> > > > > should be seen in the INPUT0/1 registers for a pin when
> > > > > its corresponding LSn bits are set to 0, which is also
> > > > > the state needed for turning on an LED in a typical
> > > > > usage scenario.  Existing code was doing the opposite
> > > > > and setting INPUT0/1 bit to a 1 when the LSn bit was
> > > > > set to 0, so this commit fixes that.
> > > > > 
> > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > > > > ---
> > > > > 
> > > > > Changes from prior version:
> > > > >      - Added comment regarding pca953X
> > > > >      - Added cover letter
> > > > > 
> > > > >   hw/misc/pca9552.c          | 18 +++++++++++++-----
> > > > >   tests/qtest/pca9552-test.c |  6 +++---
> > > > >   2 files changed, 16 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > > > > index fff19e369a..445f56a9e8 100644
> > > > > --- a/hw/misc/pca9552.c
> > > > > +++ b/hw/misc/pca9552.c
> > > > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
> > > > >   
> > > > >   DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> > > > >                          TYPE_PCA955X)
> > > > > -
> > > > > +/*
> > > > > + * Note:  The LED_ON and LED_OFF configuration values for the
> > > > > PCA955X
> > > > > + *        chips are the reverse of the PCA953X family of
> > > > > chips.
> > > > > + */
> > > > >   #define PCA9552_LED_ON   0x0
> > > > >   #define PCA9552_LED_OFF  0x1
> > > > >   #define PCA9552_LED_PWM0 0x2
> > > > > @@ -112,13 +115,18 @@ static void
> > > > > pca955x_update_pin_input(PCA955xState *s)
> > > > >   
> > > > >           switch (config) {
> > > > >           case PCA9552_LED_ON:
> > > > > -            qemu_set_irq(s->gpio[i], 1);
> > > > > -            s->regs[input_reg] |= 1 << input_shift;
> > > > > -            break;
> > > > > -        case PCA9552_LED_OFF:
> > > > > +            /* Pin is set to 0V to turn on LED */
> > > > >               qemu_set_irq(s->gpio[i], 0);
> > > > >               s->regs[input_reg] &= ~(1 << input_shift);
> > > > >               break;
> > > > > +        case PCA9552_LED_OFF:
> > > > > +            /*
> > > > > +             * Pin is set to Hi-Z to turn off LED and
> > > > > +             * pullup sets it to a logical 1.
> > > > > +             */
> > > > > +            qemu_set_irq(s->gpio[i], 1);
> > > > > +            s->regs[input_reg] |= 1 << input_shift;
> > > > > +            break;
> > > 
> > > So the witherspoon-bmc machine was a user of the PCA9552 outputs as
> > > LEDs. I guess its LEDs were in the wrong state the whole time? That
> > > looks like the only user though, and shouldn't be negatively
> > > affected.
> > 
> > Usually GPIO polarity is a machine/board property, not a device
> > one.
> > 
> > We could use the LED API (hw/misc/led.h) which initialize each
> > output with GpioPolarity.
> > 
> 
> Thanks for your comments!   This piqued my curiosity so I decided to
> run a test with the witherspoon-bmc machine.  Without my changes, I ran
> the following command to turn off LED13 on the pca9552 which I had
> previously set to "on":
> 
>   qom-set /machine/unattached/device[18] led13 off
> 
> I had GDB connected at the time with a breakpoint set on
> led_set_state() so that I could see what was happening.  Due to the
> inversion bug, I expected the pca9552 code to set the pin low and also
> set the irq low, which it did.  The connected LED's on this pca9552
> were all configured as GPIO_POLARITY_ACTIVE_LOW, so I expected that
> setting the irq state low would actually turn on the LED.  Instead it
> turned off the LED.
> 
> Reviewing the LED code, I believe the problem lies here:
> 
> 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);
> }
> 
> 
> In my test, new_state was 0 and gpio_active_high was 0, resulting in
> the boolean expression of ( 0 != 0 ) which is false and results in
> turning off the LED.  So, this looks like a bug to me.
> 
> For another simpler example, if the LED polarity was set to
> GPIO_POLARITY_ACTIVE_HIGH, then s->gpio_active_high would be 1.  Then,
> if we set the irq line to 1, wouldn't we expect the LED to turn on? 
> However, as the code stands, it would actually turn the LED off.  So, I
> think we can remove one of the "!"'s from in front of new_state.  Then,
> if the LED is active high and the irq line is set high, it would turn
> on the LED.  Correct?

Seems reasonable to me. Looks like Philippe added the LED behaviour in
Fddb67f6402b8 ("hw/misc/led: Allow connecting from GPIO output"), so
his input would be helpful. Perhaps send a fix for review?

Andrew

Re: [PATCH 1/2] misc/pca9552: Fix inverted input status

[Miles Glenn]

On Tue, 2023-10-24 at 10:04 +1030, Andrew Jeffery wrote:
> On Fri, 2023-10-20 at 11:32 -0500, Miles Glenn wrote:
> > On Fri, 2023-10-20 at 11:42 +0200, Philippe Mathieu-Daudé wrote:
> > > On 20/10/23 04:51, Andrew Jeffery wrote:
> > > > On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> > > > > > The pca9552 INPUT0 and INPUT1 registers are supposed to
> > > > > > hold the logical values of the LED pins.  A logical 0
> > > > > > should be seen in the INPUT0/1 registers for a pin when
> > > > > > its corresponding LSn bits are set to 0, which is also
> > > > > > the state needed for turning on an LED in a typical
> > > > > > usage scenario.  Existing code was doing the opposite
> > > > > > and setting INPUT0/1 bit to a 1 when the LSn bit was
> > > > > > set to 0, so this commit fixes that.
> > > > > > 
> > > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > > > > > ---
> > > > > > 
> > > > > > Changes from prior version:
> > > > > >      - Added comment regarding pca953X
> > > > > >      - Added cover letter
> > > > > > 
> > > > > >   hw/misc/pca9552.c          | 18 +++++++++++++-----
> > > > > >   tests/qtest/pca9552-test.c |  6 +++---
> > > > > >   2 files changed, 16 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > > > > > index fff19e369a..445f56a9e8 100644
> > > > > > --- a/hw/misc/pca9552.c
> > > > > > +++ b/hw/misc/pca9552.c
> > > > > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass
> > > > > > PCA955xClass;
> > > > > >   
> > > > > >   DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> > > > > >                          TYPE_PCA955X)
> > > > > > -
> > > > > > +/*
> > > > > > + * Note:  The LED_ON and LED_OFF configuration values for
> > > > > > the
> > > > > > PCA955X
> > > > > > + *        chips are the reverse of the PCA953X family of
> > > > > > chips.
> > > > > > + */
> > > > > >   #define PCA9552_LED_ON   0x0
> > > > > >   #define PCA9552_LED_OFF  0x1
> > > > > >   #define PCA9552_LED_PWM0 0x2
> > > > > > @@ -112,13 +115,18 @@ static void
> > > > > > pca955x_update_pin_input(PCA955xState *s)
> > > > > >   
> > > > > >           switch (config) {
> > > > > >           case PCA9552_LED_ON:
> > > > > > -            qemu_set_irq(s->gpio[i], 1);
> > > > > > -            s->regs[input_reg] |= 1 << input_shift;
> > > > > > -            break;
> > > > > > -        case PCA9552_LED_OFF:
> > > > > > +            /* Pin is set to 0V to turn on LED */
> > > > > >               qemu_set_irq(s->gpio[i], 0);
> > > > > >               s->regs[input_reg] &= ~(1 << input_shift);
> > > > > >               break;
> > > > > > +        case PCA9552_LED_OFF:
> > > > > > +            /*
> > > > > > +             * Pin is set to Hi-Z to turn off LED and
> > > > > > +             * pullup sets it to a logical 1.
> > > > > > +             */
> > > > > > +            qemu_set_irq(s->gpio[i], 1);
> > > > > > +            s->regs[input_reg] |= 1 << input_shift;
> > > > > > +            break;
> > > > 
> > > > So the witherspoon-bmc machine was a user of the PCA9552
> > > > outputs as
> > > > LEDs. I guess its LEDs were in the wrong state the whole time?
> > > > That
> > > > looks like the only user though, and shouldn't be negatively
> > > > affected.
> > > 
> > > Usually GPIO polarity is a machine/board property, not a device
> > > one.
> > > 
> > > We could use the LED API (hw/misc/led.h) which initialize each
> > > output with GpioPolarity.
> > > 
> > 
> > Thanks for your comments!   This piqued my curiosity so I decided
> > to
> > run a test with the witherspoon-bmc machine.  Without my changes, I
> > ran
> > the following command to turn off LED13 on the pca9552 which I had
> > previously set to "on":
> > 
> >   qom-set /machine/unattached/device[18] led13 off
> > 
> > I had GDB connected at the time with a breakpoint set on
> > led_set_state() so that I could see what was happening.  Due to the
> > inversion bug, I expected the pca9552 code to set the pin low and
> > also
> > set the irq low, which it did.  The connected LED's on this pca9552
> > were all configured as GPIO_POLARITY_ACTIVE_LOW, so I expected that
> > setting the irq state low would actually turn on the LED.  Instead
> > it
> > turned off the LED.
> > 
> > Reviewing the LED code, I believe the problem lies here:
> > 
> > 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);
> > }
> > 
> > 
> > In my test, new_state was 0 and gpio_active_high was 0, resulting
> > in
> > the boolean expression of ( 0 != 0 ) which is false and results in
> > turning off the LED.  So, this looks like a bug to me.
> > 
> > For another simpler example, if the LED polarity was set to
> > GPIO_POLARITY_ACTIVE_HIGH, then s->gpio_active_high would be
> > 1.  Then,
> > if we set the irq line to 1, wouldn't we expect the LED to turn
> > on? 
> > However, as the code stands, it would actually turn the LED
> > off.  So, I
> > think we can remove one of the "!"'s from in front of
> > new_state.  Then,
> > if the LED is active high and the irq line is set high, it would
> > turn
> > on the LED.  Correct?
> 
> Seems reasonable to me. Looks like Philippe added the LED behaviour
> in
> Fddb67f6402b8 ("hw/misc/led: Allow connecting from GPIO output"), so
> his input would be helpful. Perhaps send a fix for review?
> 
> Andrew

Sounds good.  I'll do that.

Glenn