On Wed, 2023-10-11 at 16:27 +0200, Cédric Le Goater wrote: > On 9/27/23 22:32, Glenn Miles wrote: > > The pca9552 INPUT0 and INPUT1 registers are supposed to > > hold the logical values of the LED pins. A logical 0 > > should be seen in the INPUT0/1 registers for a pin when > > its corresponding LSn bits are set to 0, which is also > > the state needed for turning on an LED in a typical > > usage scenario. Existing code was doing the opposite > > and setting INPUT0/1 bit to a 1 when the LSn bit was > > set to 0, so this commit fixes that. > > > > Signed-off-by: Glenn Miles <mil...@linux.vnet.ibm.com> > > Looks OK to me. > > May be you could mention that the ON/OFF values are reversed on > the pca953x family. This is good to know if one wants to model > these devices one day. >
Yep, I will do that. Thanks, Glenn > Reviewed-by: Cdric Le Goater <c...@kaod.org> > > Thanks, > > C. > > > > --- > > hw/misc/pca9552.c | 13 +++++++++---- > > tests/qtest/pca9552-test.c | 6 +++--- > > 2 files changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > index fff19e369a..ad811fb249 100644 > > --- a/hw/misc/pca9552.c > > +++ b/hw/misc/pca9552.c > > @@ -112,13 +112,18 @@ static void > > pca955x_update_pin_input(PCA955xState *s) > > > > switch (config) { > > case PCA9552_LED_ON: > > - qemu_set_irq(s->gpio[i], 1); > > - s->regs[input_reg] |= 1 << input_shift; > > - break; > > - case PCA9552_LED_OFF: > > + /* Pin is set to 0V to turn on LED */ > > qemu_set_irq(s->gpio[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. > > + */ > > + qemu_set_irq(s->gpio[i], 1); > > + s->regs[input_reg] |= 1 << input_shift; > > + break; > > case PCA9552_LED_PWM0: > > case PCA9552_LED_PWM1: > > /* TODO */ > > diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552- > > test.c > > index d80ed93cd3..ccca2b3d91 100644 > > --- a/tests/qtest/pca9552-test.c > > +++ b/tests/qtest/pca9552-test.c > > @@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void > > *data, QGuestAllocator *alloc) > > g_assert_cmphex(value, ==, 0x55); > > > > value = i2c_get8(i2cdev, PCA9552_INPUT0); > > - g_assert_cmphex(value, ==, 0x0); > > + g_assert_cmphex(value, ==, 0xFF); > > > > pca9552_init(i2cdev); > > > > @@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void > > *data, QGuestAllocator *alloc) > > g_assert_cmphex(value, ==, 0x54); > > > > value = i2c_get8(i2cdev, PCA9552_INPUT0); > > - g_assert_cmphex(value, ==, 0x01); > > + g_assert_cmphex(value, ==, 0xFE); > > > > value = i2c_get8(i2cdev, PCA9552_LS3); > > g_assert_cmphex(value, ==, 0x54); > > > > value = i2c_get8(i2cdev, PCA9552_INPUT1); > > - g_assert_cmphex(value, ==, 0x10); > > + g_assert_cmphex(value, ==, 0xEF); > > } > > > > static void pca9552_register_nodes(void)