| Message ID | 20231020182321.163109-4-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc/pca9552: Changes to support powernv10 | expand |
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? > > Signed-off-by: Glenn Miles <milesg@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
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 <milesg@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
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); + } + } } }
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. Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> --- New commit in v2 hw/misc/pca9552.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)