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

[Miles Glenn <milesg@linux.vnet.ibm.com>]

On Wed, 2023-10-11 at 19:05 +0200, Cédric Le Goater wrote:
> On 10/5/23 22:41, Glenn Miles 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.
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> 
> Why an extra ext_state array ? The input handler should simply update
> the PCA9552_INPUT* registers if the PCA9552_LS* register is
> programmed
> in input mode ?
> 

The ext_state array is needed because the PCA9552_INPUT* values are
now determined by two inputs:  1) The ouput state of the pca9552
device (high impedance or low) and 2) The input from an external
device (also high impedance or low).  Either of these inputs could
change independently of each other, so we can't assume that just
because the pca9552 is driving the gpio low that any external
device is also driving the gpio low.  When the pca9552 switches
its output to high impedance, we must look at how the exernal device
is currently driving the gpio before we can know the new value of the
pin.

> It would be nice to add some test case also.

Yes, I agree.  I have a crude test case that I'm using for my own
testing, but I think it has dependencies on other code (not mine) that
has not been upstreamed yet.  I will look into this.

Thanks,

Glenn

> 
> Thanks,
> 
> C.
> 
> 
> 
> > ---
> > 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];
> > +    uint8_t ext_state[PCA955X_PIN_COUNT_MAX];
> >       char *description; /* For debugging purpose only */
> >   };
> >