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

[Miles Glenn]

On Tue, 2023-10-10 at 23:02 +1030, Joel Stanley wrote:
> 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?

Yes, as long as no other device is attached to the input gpio of the
device, it will behave the same as it did before this change.  The
behavior of the device only changes when an external device attaches
its output gpio to one of the pca9552's input gpio's and drives a
low voltage.  In that case, regardless of what is written to the LS0-
LS3 registers, the INPUT* bit for that pin will read as '0'.  This is
because (physically) pca9552 devices don't actually drive the voltage
high.  Instead, they set the output to "high impedance" and allow the
pin voltage to go wherever some external component (usually a pullup
resistor) takes it.  

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

I'm definitely open to providing a test.  Maybe the existing tests can
be added to?  I'm not sure if that environment allows for hooking up
some of the output gpio's of the pca9552 to some of the input gpios,
but I can look into it.

My testing so far has been more of an integrated test that has an I2C
master connected to an I2C bus with a couple of pca9552 devices hanging
off of that, but it's very specific to the powernv platform.

 
> 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
> 

Ok, I'll try to remember this for future work.  Thanks!

> > +    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.
> 

Ok, I can certainly do that.

Thanks for the review!

Glenn

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