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