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 <mil...@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 */
> > };
> >
