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