On Tue, 2023-10-24 at 10:13 +1030, Andrew Jeffery wrote:
> On Fri, 2023-10-20 at 13:23 -0500, Glenn Miles wrote:
> > The pca9552 code was updating output GPIO states whenever
> > the pin state was updated even if the state did not change.
> > This commit adds a check so that we only update the GPIO
> > output when the pin state actually changes.
>
> Given this is intertwined with patch 2/3 that adds the input mode
> capability, I think they should be squashed together?
>
Sure, no problem.
> > Signed-off-by: Glenn Miles <mil...@linux.vnet.ibm.com>
> > ---
> >
> > New commit in v2
> >
> > hw/misc/pca9552.c | 25 ++++++++++++++++++-------
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index ed814d1f98..4ed6903404 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -112,14 +112,15 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> >
> > for (i = 0; i < k->pin_count; i++) {
> > uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> > - uint8_t input_shift = (i % 8);
> > + uint8_t bit_mask = 1 << (i % 8);
> > uint8_t config = pca955x_pin_get_config(s, i);
> > + uint8_t old_value = s->regs[input_reg] & bit_mask;
> > + uint8_t new_value;
> >
> > switch (config) {
> > case PCA9552_LED_ON:
> > /* Pin is set to 0V to turn on LED */
> > - qemu_set_irq(s->gpio_out[i], 0);
> > - s->regs[input_reg] &= ~(1 << input_shift);
> > + s->regs[input_reg] &= ~bit_mask;
> > break;
> > case PCA9552_LED_OFF:
> > /*
> > @@ -128,11 +129,9 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> > * external device drives it low.
> > */
> > if (s->ext_state[i] == PCA9552_PIN_LOW) {
> > - qemu_set_irq(s->gpio_out[i], 0);
> > - s->regs[input_reg] &= ~(1 << input_shift);
> > + s->regs[input_reg] &= ~bit_mask;
> > } else {
> > - qemu_set_irq(s->gpio_out[i], 1);
> > - s->regs[input_reg] |= 1 << input_shift;
> > + s->regs[input_reg] |= bit_mask;
> > }
> > break;
> > case PCA9552_LED_PWM0:
> > @@ -141,6 +140,18 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> > default:
> > break;
> > }
> > +
> > + /* update irq state only if pin state changed */
> > + new_value = s->regs[input_reg] & bit_mask;
> > + if (new_value != old_value) {
> > + if (new_value) {
> > + /* changed from 0 to 1 */
> > + qemu_set_irq(s->gpio_out[i], 1);
> > + } else {
> > + /* changed from 1 to 0 */
> > + qemu_set_irq(s->gpio_out[i], 0);
> > + }
>
> Slightly code-golf-y, but the inner if-else here may be compressed
> to:
>
> qemu_set_irq(s->gpio_out[i], !!new_value);
>
> Andrew
I like it!
Glenn
