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

[Miles Glenn]

On Fri, 2023-10-20 at 15:18 +1030, Andrew Jeffery wrote:
> On Thu, 2023-10-19 at 15:40 -0500, 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.
> > 
> > Here is a table describing the logical state of a pca9552 pin
> > given the state being driven by the pca9552 and an external device:
> > 
> >                    PCA9552
> >                    Configured
> >                    State
> > 
> >                   | Hi-Z | Low |
> >             ------+------+-----+
> >   External   Hi-Z |  Hi  | Low |
> >   Device    ------+------+-----+
> >   State      Low  |  Low | Low |
> >             ------+------+-----+
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> > 
> > Changes from previous version:
> >  - Added #define's for external state values
> >  - Added logic state table to commit message
> >  - Added cover letter
> > 
> >  hw/misc/pca9552.c         | 41 ++++++++++++++++++++++++++++++++++-
> > ----
> >  include/hw/misc/pca9552.h |  3 ++-
> >  2 files changed, 38 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index 445f56a9e8..ed814d1f98 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -44,6 +44,8 @@ DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> >  #define PCA9552_LED_OFF  0x1
> >  #define PCA9552_LED_PWM0 0x2
> >  #define PCA9552_LED_PWM1 0x3
> > +#define PCA9552_PIN_LOW  0x0
> > +#define PCA9552_PIN_HIZ  0x1
> >  
> >  static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
> >  
> > @@ -116,16 +118,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);
> 
> pca955x_update_pin_input() is called by the external GPIO handling
> path
> as well as the I2C command handling path. If the I2C path sets the
> line
> low followed by the device attached to the GPIO setting the line low
> then I think each change will issue an event on the outbound GPIO. Is
> that desired behaviour? Does it matter?
> 

I think these questions probably depend a lot on the recieving device,
but at best, I think it's inefficient and at worst, depending on the
recieving device, it could lead to bugs, so I'll add a check.

 
> >              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] == PCA9552_PIN_LOW) {
> > +                qemu_set_irq(s->gpio_out[i], 0);
> 
> Similarly here - it might be the case that both devices have pulled
> the
> line low and now the I2C path is releasing it. Given the external
> device had already pulled the line low as well should we expect an
> event to occur issued here? Should it matter?
> 
> Andrew
> 

See previous response.

Thanks for the review!

Glenn

> > +                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:
> > @@ -340,6 +348,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()
> >      }
> > @@ -358,6 +367,7 @@ static void pca9552_reset(DeviceState *dev)
> >      s->regs[PCA9552_LS2] = 0x55;
> >      s->regs[PCA9552_LS3] = 0x55;
> >  
> > +    memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
> >      pca955x_update_pin_input(s);
> >  
> >      s->pointer = 0xFF;
> > @@ -380,6 +390,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);
> > @@ -389,7 +419,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 */
> >  };
> >