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

[Joel Stanley <joel@jms.id.au>]

On Fri, 6 Oct 2023 at 07:23, Glenn Miles <milesg@linux.vnet.ibm.com> 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.

Does this let us use qom-set from the monitor, and have the guest see
the state change?

An example in the commit message, or even better would be a test.

Some other comments below.

>
> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> ---
> Based-on: <20230927203221.3286895-1-milesg@linux.vnet.ibm.com>
> ([PATCH] misc/pca9552: Fix inverted input status)
>  hw/misc/pca9552.c         | 39 ++++++++++++++++++++++++++++++++++-----
>  include/hw/misc/pca9552.h |  3 ++-
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index ad811fb249..f28b5ecd7e 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -113,16 +113,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] == 0) {
> +                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:
> @@ -337,6 +343,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()
>      }
> @@ -355,6 +362,7 @@ static void pca9552_reset(DeviceState *dev)
>      s->regs[PCA9552_LS2] = 0x55;
>      s->regs[PCA9552_LS3] = 0x55;
>
> +    memset(s->ext_state, 1, PCA955X_PIN_COUNT_MAX);
>      pca955x_update_pin_input(s);
>
>      s->pointer = 0xFF;
> @@ -377,6 +385,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);
> @@ -386,7 +414,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];

I wondered if the renaming of gpio to gpio_out could be a separate
patch, but once I'd read the entire patch it made sense, so don't
bother.

I think Cédric has some magic for sorting the header file changes at
the start of the diff output. Here it is:

https://gitlab.com/qemu-project/qemu/-/blob/master/scripts/git.orderfile?ref_type=heads

We should add that to the tips and tricks part of
docs/devel/submitting-a-patch.rst

> +    uint8_t ext_state[PCA955X_PIN_COUNT_MAX];

State is 0 or 1, representing driving the pin low, or high impedance?
I think some #defines or enums would make the states clearer.

>      char *description; /* For debugging purpose only */
>  };
>
> --
> 2.31.1
>
>