On Thu, 2023-10-19 at 15:40 -0500, 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>
> > ---
> >
> > Changes from prior version:
> > - Added comment regarding pca953X
> > - Added cover letter
> >
> > hw/misc/pca9552.c | 18 +++++++++++++-----
> > tests/qtest/pca9552-test.c | 6 +++---
> > 2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index fff19e369a..445f56a9e8 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
> >
> > DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> > TYPE_PCA955X)
> > -
> > +/*
> > + * Note: The LED_ON and LED_OFF configuration values for the PCA955X
> > + * chips are the reverse of the PCA953X family of chips.
> > + */
> > #define PCA9552_LED_ON 0x0
> > #define PCA9552_LED_OFF 0x1
> > #define PCA9552_LED_PWM0 0x2
> > @@ -112,13 +115,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;
So the witherspoon-bmc machine was a user of the PCA9552 outputs as
LEDs. I guess its LEDs were in the wrong state the whole time? That
looks like the only user though, and shouldn't be negatively affected.
Reviewed-by: Andrew Jeffery <and...@codeconstruct.com.au>
> > 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)
