| Message ID | 20231019204011.3174115-3-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc/pca9552: Changes to support powernv10 | expand |
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? > 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 > + 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 */ > }; >
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 */ > > }; > >
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); 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); + 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 */ };
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(-)