* [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs @ 2023-10-05 20:41 Glenn Miles 2023-10-10 12:32 ` Joel Stanley 2023-10-11 17:05 ` Cédric Le Goater 0 siblings, 2 replies; 5+ messages in thread From: Glenn Miles @ 2023-10-05 20:41 UTC (permalink / raw) To: qemu-devel Cc: Glenn Miles, qemu-arm, Cédric Le Goater, andrew, Joel Stanley 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. Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> --- Based-on: <20230927203221.3286895-1-milesg@linux.vnet.ibm.com> ([PATCH] misc/pca9552: Fix inverted input status) hw/misc/pca9552.c | 39 ++++++++++++++++++++++++++++++++++----- include/hw/misc/pca9552.h | 3 ++- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c index ad811fb249..f28b5ecd7e 100644 --- a/hw/misc/pca9552.c +++ b/hw/misc/pca9552.c @@ -113,16 +113,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] == 0) { + 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: @@ -337,6 +343,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() } @@ -355,6 +362,7 @@ static void pca9552_reset(DeviceState *dev) s->regs[PCA9552_LS2] = 0x55; s->regs[PCA9552_LS3] = 0x55; + memset(s->ext_state, 1, PCA955X_PIN_COUNT_MAX); pca955x_update_pin_input(s); s->pointer = 0xFF; @@ -377,6 +385,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); @@ -386,7 +414,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 */ }; -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs 2023-10-05 20:41 [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs Glenn Miles @ 2023-10-10 12:32 ` Joel Stanley 2023-10-12 22:40 ` Miles Glenn 2023-10-11 17:05 ` Cédric Le Goater 1 sibling, 1 reply; 5+ messages in thread From: Joel Stanley @ 2023-10-10 12:32 UTC (permalink / raw) To: Glenn Miles, Cédric Le Goater; +Cc: qemu-devel, qemu-arm, andrew On Fri, 6 Oct 2023 at 07:23, Glenn Miles <milesg@linux.vnet.ibm.com> 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. Does this let us use qom-set from the monitor, and have the guest see the state change? An example in the commit message, or even better would be a test. Some other comments below. > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > --- > Based-on: <20230927203221.3286895-1-milesg@linux.vnet.ibm.com> > ([PATCH] misc/pca9552: Fix inverted input status) > hw/misc/pca9552.c | 39 ++++++++++++++++++++++++++++++++++----- > include/hw/misc/pca9552.h | 3 ++- > 2 files changed, 36 insertions(+), 6 deletions(-) > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > index ad811fb249..f28b5ecd7e 100644 > --- a/hw/misc/pca9552.c > +++ b/hw/misc/pca9552.c > @@ -113,16 +113,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] == 0) { > + 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: > @@ -337,6 +343,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() > } > @@ -355,6 +362,7 @@ static void pca9552_reset(DeviceState *dev) > s->regs[PCA9552_LS2] = 0x55; > s->regs[PCA9552_LS3] = 0x55; > > + memset(s->ext_state, 1, PCA955X_PIN_COUNT_MAX); > pca955x_update_pin_input(s); > > s->pointer = 0xFF; > @@ -377,6 +385,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); > @@ -386,7 +414,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]; I wondered if the renaming of gpio to gpio_out could be a separate patch, but once I'd read the entire patch it made sense, so don't bother. I think Cédric has some magic for sorting the header file changes at the start of the diff output. Here it is: https://gitlab.com/qemu-project/qemu/-/blob/master/scripts/git.orderfile?ref_type=heads We should add that to the tips and tricks part of docs/devel/submitting-a-patch.rst > + uint8_t ext_state[PCA955X_PIN_COUNT_MAX]; State is 0 or 1, representing driving the pin low, or high impedance? I think some #defines or enums would make the states clearer. > char *description; /* For debugging purpose only */ > }; > > -- > 2.31.1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs 2023-10-10 12:32 ` Joel Stanley @ 2023-10-12 22:40 ` Miles Glenn 0 siblings, 0 replies; 5+ messages in thread From: Miles Glenn @ 2023-10-12 22:40 UTC (permalink / raw) To: Joel Stanley, Cédric Le Goater; +Cc: qemu-devel, qemu-arm, andrew On Tue, 2023-10-10 at 23:02 +1030, Joel Stanley wrote: > On Fri, 6 Oct 2023 at 07:23, Glenn Miles <milesg@linux.vnet.ibm.com> > 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. > > Does this let us use qom-set from the monitor, and have the guest see > the state change? Yes, as long as no other device is attached to the input gpio of the device, it will behave the same as it did before this change. The behavior of the device only changes when an external device attaches its output gpio to one of the pca9552's input gpio's and drives a low voltage. In that case, regardless of what is written to the LS0- LS3 registers, the INPUT* bit for that pin will read as '0'. This is because (physically) pca9552 devices don't actually drive the voltage high. Instead, they set the output to "high impedance" and allow the pin voltage to go wherever some external component (usually a pullup resistor) takes it. > > An example in the commit message, or even better would be a test. > I'm definitely open to providing a test. Maybe the existing tests can be added to? I'm not sure if that environment allows for hooking up some of the output gpio's of the pca9552 to some of the input gpios, but I can look into it. My testing so far has been more of an integrated test that has an I2C master connected to an I2C bus with a couple of pca9552 devices hanging off of that, but it's very specific to the powernv platform. > Some other comments below. > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > > --- > > Based-on: <20230927203221.3286895-1-milesg@linux.vnet.ibm.com> > > ([PATCH] misc/pca9552: Fix inverted input status) > > hw/misc/pca9552.c | 39 ++++++++++++++++++++++++++++++++++- > > ---- > > include/hw/misc/pca9552.h | 3 ++- > > 2 files changed, 36 insertions(+), 6 deletions(-) > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > index ad811fb249..f28b5ecd7e 100644 > > --- a/hw/misc/pca9552.c > > +++ b/hw/misc/pca9552.c > > @@ -113,16 +113,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] == 0) { > > + 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: > > @@ -337,6 +343,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() > > } > > @@ -355,6 +362,7 @@ static void pca9552_reset(DeviceState *dev) > > s->regs[PCA9552_LS2] = 0x55; > > s->regs[PCA9552_LS3] = 0x55; > > > > + memset(s->ext_state, 1, PCA955X_PIN_COUNT_MAX); > > pca955x_update_pin_input(s); > > > > s->pointer = 0xFF; > > @@ -377,6 +385,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); > > @@ -386,7 +414,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]; > > I wondered if the renaming of gpio to gpio_out could be a separate > patch, but once I'd read the entire patch it made sense, so don't > bother. > > I think Cédric has some magic for sorting the header file changes at > the start of the diff output. Here it is: > > https://gitlab.com/qemu-project/qemu/-/blob/master/scripts/git.orderfile?ref_type=heads > > We should add that to the tips and tricks part of > docs/devel/submitting-a-patch.rst > Ok, I'll try to remember this for future work. Thanks! > > + uint8_t ext_state[PCA955X_PIN_COUNT_MAX]; > > State is 0 or 1, representing driving the pin low, or high impedance? > I think some #defines or enums would make the states clearer. > Ok, I can certainly do that. Thanks for the review! Glenn > > char *description; /* For debugging purpose only */ > > }; > > > > -- > > 2.31.1 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs 2023-10-05 20:41 [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs Glenn Miles 2023-10-10 12:32 ` Joel Stanley @ 2023-10-11 17:05 ` Cédric Le Goater 2023-10-11 19:16 ` Miles Glenn 1 sibling, 1 reply; 5+ messages in thread From: Cédric Le Goater @ 2023-10-11 17:05 UTC (permalink / raw) To: Glenn Miles, qemu-devel; +Cc: qemu-arm, andrew, Joel Stanley On 10/5/23 22:41, 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. > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> Why an extra ext_state array ? The input handler should simply update the PCA9552_INPUT* registers if the PCA9552_LS* register is programmed in input mode ? It would be nice to add some test case also. Thanks, C. > --- > Based-on: <20230927203221.3286895-1-milesg@linux.vnet.ibm.com> > ([PATCH] misc/pca9552: Fix inverted input status) > hw/misc/pca9552.c | 39 ++++++++++++++++++++++++++++++++++----- > include/hw/misc/pca9552.h | 3 ++- > 2 files changed, 36 insertions(+), 6 deletions(-) > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > index ad811fb249..f28b5ecd7e 100644 > --- a/hw/misc/pca9552.c > +++ b/hw/misc/pca9552.c > @@ -113,16 +113,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] == 0) { > + 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: > @@ -337,6 +343,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() > } > @@ -355,6 +362,7 @@ static void pca9552_reset(DeviceState *dev) > s->regs[PCA9552_LS2] = 0x55; > s->regs[PCA9552_LS3] = 0x55; > > + memset(s->ext_state, 1, PCA955X_PIN_COUNT_MAX); > pca955x_update_pin_input(s); > > s->pointer = 0xFF; > @@ -377,6 +385,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); > @@ -386,7 +414,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 */ > }; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs 2023-10-11 17:05 ` Cédric Le Goater @ 2023-10-11 19:16 ` Miles Glenn 0 siblings, 0 replies; 5+ messages in thread From: Miles Glenn @ 2023-10-11 19:16 UTC (permalink / raw) To: Cédric Le Goater, qemu-devel; +Cc: qemu-arm, andrew, Joel Stanley On Wed, 2023-10-11 at 19:05 +0200, Cédric Le Goater wrote: > On 10/5/23 22:41, 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. > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > > Why an extra ext_state array ? The input handler should simply update > the PCA9552_INPUT* registers if the PCA9552_LS* register is > programmed > in input mode ? > The ext_state array is needed because the PCA9552_INPUT* values are now determined by two inputs: 1) The ouput state of the pca9552 device (high impedance or low) and 2) The input from an external device (also high impedance or low). Either of these inputs could change independently of each other, so we can't assume that just because the pca9552 is driving the gpio low that any external device is also driving the gpio low. When the pca9552 switches its output to high impedance, we must look at how the exernal device is currently driving the gpio before we can know the new value of the pin. > It would be nice to add some test case also. Yes, I agree. I have a crude test case that I'm using for my own testing, but I think it has dependencies on other code (not mine) that has not been upstreamed yet. I will look into this. Thanks, Glenn > > Thanks, > > C. > > > > > --- > > Based-on: <20230927203221.3286895-1-milesg@linux.vnet.ibm.com> > > ([PATCH] misc/pca9552: Fix inverted input status) > > hw/misc/pca9552.c | 39 > > ++++++++++++++++++++++++++++++++++----- > > include/hw/misc/pca9552.h | 3 ++- > > 2 files changed, 36 insertions(+), 6 deletions(-) > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > index ad811fb249..f28b5ecd7e 100644 > > --- a/hw/misc/pca9552.c > > +++ b/hw/misc/pca9552.c > > @@ -113,16 +113,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] == 0) { > > + 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: > > @@ -337,6 +343,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() > > } > > @@ -355,6 +362,7 @@ static void pca9552_reset(DeviceState *dev) > > s->regs[PCA9552_LS2] = 0x55; > > s->regs[PCA9552_LS3] = 0x55; > > > > + memset(s->ext_state, 1, PCA955X_PIN_COUNT_MAX); > > pca955x_update_pin_input(s); > > > > s->pointer = 0xFF; > > @@ -377,6 +385,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); > > @@ -386,7 +414,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 */ > > }; > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-12 22:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-05 20:41 [PATCH v2] misc/pca9552: Let external devices set pca9552 inputs Glenn Miles 2023-10-10 12:32 ` Joel Stanley 2023-10-12 22:40 ` Miles Glenn 2023-10-11 17:05 ` Cédric Le Goater 2023-10-11 19:16 ` Miles Glenn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).