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 <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
