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

[PATCH v2 0/3] 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.

Changes from v1:
 - Added check to only update output GPIO if state changed

Glenn Miles (3):
  misc/pca9552: Fix inverted input status
  misc/pca9552: Let external devices set pca9552 inputs
  misc/pca9552: Only update output GPIOs if state changed

 hw/misc/pca9552.c          | 64 +++++++++++++++++++++++++++++++++-----
 include/hw/misc/pca9552.h  |  3 +-
 tests/qtest/pca9552-test.c |  6 ++--
 3 files changed, 62 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] 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>
---

No change from v1

 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

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

[Andrew Jeffery]

On Fri, 2023-10-20 at 13:23 -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>

I sent a Reviewed-by tag for v1, don't forget to collect those on your
patches before sending out a new set. Something for next time :)

Anyway,

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

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

[Miles Glenn]

On Tue, 2023-10-24 at 10:15 +1030, Andrew Jeffery wrote:
> On Fri, 2023-10-20 at 13:23 -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>
> 
> I sent a Reviewed-by tag for v1, don't forget to collect those on
> your
> patches before sending out a new set. Something for next time :)
> 
> Anyway,
> 
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 

Yeah, of course I realized that I forgot that after sending it! :-)

Glenn

[PATCH v2 2/3] 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>
---

No change from v1

 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

[PATCH v2 3/3] misc/pca9552: Only update output GPIOs if state changed

[Glenn Miles]

The pca9552 code was updating output GPIO states whenever
the pin state was updated even if the state did not change.
This commit adds a check so that we only update the GPIO
output when the pin state actually changes.

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

New commit in v2

 hw/misc/pca9552.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index ed814d1f98..4ed6903404 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -112,14 +112,15 @@ static void pca955x_update_pin_input(PCA955xState *s)
 
     for (i = 0; i < k->pin_count; i++) {
         uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
-        uint8_t input_shift = (i % 8);
+        uint8_t bit_mask = 1 << (i % 8);
         uint8_t config = pca955x_pin_get_config(s, i);
+        uint8_t old_value = s->regs[input_reg] & bit_mask;
+        uint8_t new_value;
 
         switch (config) {
         case PCA9552_LED_ON:
             /* Pin is set to 0V to turn on LED */
-            qemu_set_irq(s->gpio_out[i], 0);
-            s->regs[input_reg] &= ~(1 << input_shift);
+            s->regs[input_reg] &= ~bit_mask;
             break;
         case PCA9552_LED_OFF:
             /*
@@ -128,11 +129,9 @@ static void pca955x_update_pin_input(PCA955xState *s)
              * external device drives it low.
              */
             if (s->ext_state[i] == PCA9552_PIN_LOW) {
-                qemu_set_irq(s->gpio_out[i], 0);
-                s->regs[input_reg] &= ~(1 << input_shift);
+                s->regs[input_reg] &= ~bit_mask;
             } else {
-                qemu_set_irq(s->gpio_out[i], 1);
-                s->regs[input_reg] |= 1 << input_shift;
+                s->regs[input_reg] |=  bit_mask;
             }
             break;
         case PCA9552_LED_PWM0:
@@ -141,6 +140,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
         default:
             break;
         }
+
+        /* update irq state only if pin state changed */
+        new_value = s->regs[input_reg] & bit_mask;
+        if (new_value != old_value) {
+            if (new_value) {
+                /* changed from 0 to 1 */
+                qemu_set_irq(s->gpio_out[i], 1);
+            } else {
+                /* changed from 1 to 0 */
+                qemu_set_irq(s->gpio_out[i], 0);
+            }
+        }
     }
 }
 
-- 
2.31.1

Re: [PATCH v2 3/3] misc/pca9552: Only update output GPIOs if state changed

[Andrew Jeffery]

On Fri, 2023-10-20 at 13:23 -0500, Glenn Miles wrote:
> The pca9552 code was updating output GPIO states whenever
> the pin state was updated even if the state did not change.
> This commit adds a check so that we only update the GPIO
> output when the pin state actually changes.

Given this is intertwined with patch 2/3 that adds the input mode
capability, I think they should be squashed together?

> 
> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> ---
> 
> New commit in v2
> 
>  hw/misc/pca9552.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index ed814d1f98..4ed6903404 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -112,14 +112,15 @@ static void pca955x_update_pin_input(PCA955xState *s)
>  
>      for (i = 0; i < k->pin_count; i++) {
>          uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> -        uint8_t input_shift = (i % 8);
> +        uint8_t bit_mask = 1 << (i % 8);
>          uint8_t config = pca955x_pin_get_config(s, i);
> +        uint8_t old_value = s->regs[input_reg] & bit_mask;
> +        uint8_t new_value;
>  
>          switch (config) {
>          case PCA9552_LED_ON:
>              /* Pin is set to 0V to turn on LED */
> -            qemu_set_irq(s->gpio_out[i], 0);
> -            s->regs[input_reg] &= ~(1 << input_shift);
> +            s->regs[input_reg] &= ~bit_mask;
>              break;
>          case PCA9552_LED_OFF:
>              /*
> @@ -128,11 +129,9 @@ static void pca955x_update_pin_input(PCA955xState *s)
>               * external device drives it low.
>               */
>              if (s->ext_state[i] == PCA9552_PIN_LOW) {
> -                qemu_set_irq(s->gpio_out[i], 0);
> -                s->regs[input_reg] &= ~(1 << input_shift);
> +                s->regs[input_reg] &= ~bit_mask;
>              } else {
> -                qemu_set_irq(s->gpio_out[i], 1);
> -                s->regs[input_reg] |= 1 << input_shift;
> +                s->regs[input_reg] |=  bit_mask;
>              }
>              break;
>          case PCA9552_LED_PWM0:
> @@ -141,6 +140,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
>          default:
>              break;
>          }
> +
> +        /* update irq state only if pin state changed */
> +        new_value = s->regs[input_reg] & bit_mask;
> +        if (new_value != old_value) {
> +            if (new_value) {
> +                /* changed from 0 to 1 */
> +                qemu_set_irq(s->gpio_out[i], 1);
> +            } else {
> +                /* changed from 1 to 0 */
> +                qemu_set_irq(s->gpio_out[i], 0);
> +            }

Slightly code-golf-y, but the inner if-else here may be compressed to:

    qemu_set_irq(s->gpio_out[i], !!new_value);

Andrew

Re: [PATCH v2 3/3] misc/pca9552: Only update output GPIOs if state changed

[Miles Glenn]

On Tue, 2023-10-24 at 10:13 +1030, Andrew Jeffery wrote:
> On Fri, 2023-10-20 at 13:23 -0500, Glenn Miles wrote:
> > The pca9552 code was updating output GPIO states whenever
> > the pin state was updated even if the state did not change.
> > This commit adds a check so that we only update the GPIO
> > output when the pin state actually changes.
> 
> Given this is intertwined with patch 2/3 that adds the input mode
> capability, I think they should be squashed together?
> 

Sure, no problem.

> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> > 
> > New commit in v2
> > 
> >  hw/misc/pca9552.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index ed814d1f98..4ed6903404 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -112,14 +112,15 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> >  
> >      for (i = 0; i < k->pin_count; i++) {
> >          uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> > -        uint8_t input_shift = (i % 8);
> > +        uint8_t bit_mask = 1 << (i % 8);
> >          uint8_t config = pca955x_pin_get_config(s, i);
> > +        uint8_t old_value = s->regs[input_reg] & bit_mask;
> > +        uint8_t new_value;
> >  
> >          switch (config) {
> >          case PCA9552_LED_ON:
> >              /* Pin is set to 0V to turn on LED */
> > -            qemu_set_irq(s->gpio_out[i], 0);
> > -            s->regs[input_reg] &= ~(1 << input_shift);
> > +            s->regs[input_reg] &= ~bit_mask;
> >              break;
> >          case PCA9552_LED_OFF:
> >              /*
> > @@ -128,11 +129,9 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> >               * external device drives it low.
> >               */
> >              if (s->ext_state[i] == PCA9552_PIN_LOW) {
> > -                qemu_set_irq(s->gpio_out[i], 0);
> > -                s->regs[input_reg] &= ~(1 << input_shift);
> > +                s->regs[input_reg] &= ~bit_mask;
> >              } else {
> > -                qemu_set_irq(s->gpio_out[i], 1);
> > -                s->regs[input_reg] |= 1 << input_shift;
> > +                s->regs[input_reg] |=  bit_mask;
> >              }
> >              break;
> >          case PCA9552_LED_PWM0:
> > @@ -141,6 +140,18 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> >          default:
> >              break;
> >          }
> > +
> > +        /* update irq state only if pin state changed */
> > +        new_value = s->regs[input_reg] & bit_mask;
> > +        if (new_value != old_value) {
> > +            if (new_value) {
> > +                /* changed from 0 to 1 */
> > +                qemu_set_irq(s->gpio_out[i], 1);
> > +            } else {
> > +                /* changed from 1 to 0 */
> > +                qemu_set_irq(s->gpio_out[i], 0);
> > +            }
> 
> Slightly code-golf-y, but the inner if-else here may be compressed
> to:
> 
>     qemu_set_irq(s->gpio_out[i], !!new_value);
> 
> Andrew

I like it!

Glenn