| Message ID | 20231020182321.163109-2-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 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 <milesg@linux.vnet.ibm.com> I sent a Reviewed-by tag for v1, don't forget to collect those on your patches before sending out a new set. Something for next time :) Anyway, Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
On Tue, 2023-10-24 at 10:15 +1030, Andrew Jeffery wrote: > On Fri, 2023-10-20 at 13:23 -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 <milesg@linux.vnet.ibm.com> > > I sent a Reviewed-by tag for v1, don't forget to collect those on > your > patches before sending out a new set. Something for next time :) > > Anyway, > > Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> > Yeah, of course I realized that I forgot that after sending it! :-) Glenn
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; 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)
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 <milesg@linux.vnet.ibm.com> --- No change from v1 hw/misc/pca9552.c | 18 +++++++++++++----- tests/qtest/pca9552-test.c | 6 +++--- 2 files changed, 16 insertions(+), 8 deletions(-)